Tvheadend

Jaroslav Kysela wrote:

Applied 1-3. I need think more about 4 and I tried to resolve the SETUP/PLAY states in another way.

Dear Jaroslav,

Thank you!
We will test your solution for the issue 5 with some clients. We will keep you informed.

Regarding the issue 4, we will be happy if you implement a better solution. However, keep in mind that our patch is fairly simple and it can’t disturb any current implementation. Therefore, we suggest that you commit it. Without this patch any combination of two TVH will suffer from this problem. The easiest environment is this: one TVH configured as SAT>IP server with only IPTV inputs; and a second one configured as SAT>IP server as well with the other serving as the SAT>IP input tuner. Without this patch, the second server never sends the error to the client.

So, please consider to commit the patch now until you find a better solution.
We hope you agree!
Regards,

Jaroslav Kysela wrote:

Added another patch which should correctly identify the 'no data/bad service' state plus another bunch of cleanups and functional changes which I believe should fix the reported problems. v4.3-307-g4ee77e1

Hi,

After this patch, using two TVH servers ("remote" and "local"), and using the Windows free "SatIP DVBViewer" tool connected to the "local" server the scanning process is very slow... when using some IPTV inputs and some frequencies are empty.

See this log as example:

2017-08-03 17:44:17.359 mpegts: 10774H in ASTRA-192 - tuning on SAT>IP DVB-S Tuner #1 (10.100.120.2)(TCP)
2017-08-03 17:44:17.359 epggrab: 10774H in ASTRA-192 - registering mux for OTA EPG
2017-08-03 17:44:17.360 subscription: 000D: "SAT>IP" subscribing to mux "10774H", weight: 100, adapter: "SAT>IP DVB-S Tuner #1 (10.100.120.2)(TCP)", network: "ASTRA-192", service: "Raw PID Subscription", hostname="192.168.1.2" 
2017-08-03 17:44:17.449 satip: SAT>IP DVB-S Tuner #1 (10.100.120.2)(TCP) - RTSP SETUP error -5 (Input/output error) [6-405]
2017-08-03 17:44:19.360 subscription: 000D: service instance is bad, reason: Tuning failed
2017-08-03 17:44:19.363 mpegts: 10774H in ASTRA-192 - tuning on SAT>IP DVB-S Tuner #2 (10.100.120.2)(TCP)
2017-08-03 17:44:19.364 subscription: 000D: "SAT>IP" subscribing to mux "10774H", weight: 100, adapter: "SAT>IP DVB-S Tuner #2 (10.100.120.2)(TCP)", network: "ASTRA-192", service: "Raw PID Subscription", hostname="192.168.1.2" 
2017-08-03 17:44:19.450 satip: SAT>IP DVB-S Tuner #2 (10.100.120.2)(TCP) - RTSP SETUP error -5 (Input/output error) [6-405]
2017-08-03 17:44:21.361 subscription: 000D: service instance is bad, reason: Tuning failed
2017-08-03 17:44:23.364 subscription: 000D: No input source available for subscription "SAT>IP" to mux "10774H in ASTRA-192" 
2017-08-03 17:44:25.362 subscription: 000D: No input source available for subscription "SAT>IP" to mux "10774H in ASTRA-192" 
2017-08-03 17:44:27.363 subscription: 000D: No input source available for subscription "SAT>IP" to mux "10774H in ASTRA-192" 
2017-08-03 17:44:29.364 subscription: 000D: No input source available for subscription "SAT>IP" to mux "10774H in ASTRA-192" 
2017-08-03 17:44:31.362 subscription: 000D: No input source available for subscription "SAT>IP" to mux "10774H in ASTRA-192" 
2017-08-03 17:44:33.362 subscription: 000D: No input source available for subscription "SAT>IP" to mux "10774H in ASTRA-192" 
2017-08-03 17:44:35.363 subscription: 000D: No input source available for subscription "SAT>IP" to mux "10774H in ASTRA-192" 
2017-08-03 17:44:37.364 subscription: 000D: No input source available for subscription "SAT>IP" to mux "10774H in ASTRA-192" 
2017-08-03 17:44:39.365 subscription: 000D: No input source available for subscription "SAT>IP" to mux "10774H in ASTRA-192" 
2017-08-03 17:44:41.366 subscription: 000D: No input source available for subscription "SAT>IP" to mux "10774H in ASTRA-192" 
2017-08-03 17:44:43.367 subscription: 000D: No input source available for subscription "SAT>IP" to mux "10774H in ASTRA-192" 
2017-08-03 17:44:45.368 subscription: 000D: No input source available for subscription "SAT>IP" to mux "10774H in ASTRA-192" 
2017-08-03 17:44:47.368 subscription: 000D: No input source available for subscription "SAT>IP" to mux "10774H in ASTRA-192" 
2017-08-03 17:44:47.382 satips: 0/22F231CF/2: create mux DVB-S freq 10788000 V sym 22000000 fec 5/6 mod QPSK roff 35 is_id -1 pls_mode ROOT pls_code 0
2017-08-03 17:44:47.383 subscription: 000D: "SAT>IP" unsubscribing, hostname="192.168.1.2" 
2017-08-03 17:44:47.383 mpegts: 10774H in ASTRA-192 (0x7f66e773d360) - deleting
2017-08-03 17:44:47.383 mpegts: 10788V in ASTRA-192 - tuning on SAT>IP DVB-S Tuner #1 (10.100.120.2)(TCP)
2017-08-03 17:44:47.383 epggrab: 10788V in ASTRA-192 - registering mux for OTA EPG
2017-08-03 17:44:47.384 subscription: 000F: "SAT>IP" subscribing to mux "10788V", weight: 100, adapter: "SAT>IP DVB-S Tuner #1 (10.100.120.2)(TCP)", network: "ASTRA-192", service: "Raw PID Subscription", hostname="192.168.1.2" 
...

- As you can see, the client receives the error at some point (Time=0s)
"RTSP SETUP error -5 (Input/output error) [6-405]"

- At Time=2s, the subscription fails
"subscription: 000D: service instance is bad, reason: Tuning failed"
and the server tries the second tuner and it receives the same error (as the frequency is invalid in both tuners)

- At Time=4s, the subscription fails again,
and 2 seconds after a log message appears each two seconds
"No input source available for subscription "SAT>IP" to mux "10774H in ASTRA-192""

- And only at Time=26s, 20 seconds after the subscription is cancelled the client receives the error.

So, with around 50 frequencies invalid, around 50x30s are added, 25 minutes in total!
Can this time be improved?
Can the subscription be cancelled directly when the service instace is bad because a failed tuning?
I feel with a 405 Error is not need to wait more time!

Hi,

More info: When you have configured two tuners, and the you are using the first... and the RTSP connection is interrupted (I'm using tcpkill) you see this message:

2017-08-03 19:11:54.752 satip: SAT>IP DVB-S Tuner #1 (10.100.120.2) - RTSP error -104 (Connection reset by peer) [9874-0]
2017-08-03 19:11:55.421 subscription: 000C: service instance is bad, reason: Tuning failed
2017-08-03 19:11:55.422 mpegts: 11778V in ASTRA-192 - tuning on SAT>IP DVB-S Tuner #2 (10.100.120.2)

So the second Tuner is inmediatly started opening a new TCP RTSP connection.

However, when the same error appears and you are using the second tuner, then and error is triggered:

2017-08-03 19:12:22.375 satip: SAT>IP DVB-S Tuner #2 (10.100.120.2) - RTSP error -104 (Connection reset by peer) [9874-0]
2017-08-03 19:12:23.432 subscription: 000C: service instance is bad, reason: Tuning failed
2017-08-03 19:12:25.432 subscription: 000C: No input source available for subscription "SAT>IP" to mux "11778V in ASTRA-192" 
2017-08-03 19:12:27.433 subscription: 000C: No input source available for subscription "SAT>IP" to mux "11778V in ASTRA-192" 
2017-08-03 19:12:29.433 subscription: 000C: No input source available for subscription "SAT>IP" to mux "11778V in ASTRA-192" 
...

So, I suggest this:

1. When a socket error closes the RTSP connection, try one time to reconnect to the same tuner.
2. If the tuner fails, then request the next free in the list, even if the tuner number is lower.

You agree?

Jaroslav Kysela wrote:

Added another patch which should correctly identify the 'no data/bad service' state plus another bunch of cleanups and functional changes which I believe should fix the reported problems. v4.3-307-g4ee77e1

Dear Jaroslav,

We make deep tests with your latest modifications and commits over our previous patches. We are happy with the results. And we think the project is now a robust SAT>IP server. Congratulations! Therefore, we recommend to backport all changes to previous releases, if it's ok with you.

There is still one point left to solve. It is the targeted by our patch number #4. And unfortunately, without our patch, several hardware clients are still suffering troubles. But the problem now is that our patch no longer works with the latest changes. Please, can you try to upgrade the code to implement some direct error response when the tuner has received a 405 Error? Our initial idea was to return an error when the subscription is in an incorrect state (SUBSCRIPTION_BAD_SERVICE), which is the state of the subscription when the SAT>IP client tuner has received an error.

Please, be advised that without this fix the scanning process is practically impossible when the source server only covers about half of the frequencies with IPTV sources. Therefore, we hope you want to solve this problem.

Regards,
D.

The logic is implemented in https://tvheadend.org/projects/tvheadend/repository/revisions/4ee77e1249b2df818ebb66e8c818d6b27aecf7fa/diff/ . Could you verify, if no_data variable is set correctly?

Jaroslav Kysela wrote:

The logic is implemented in https://tvheadend.org/projects/tvheadend/repository/revisions/4ee77e1249b2df818ebb66e8c818d6b27aecf7fa/diff/ . Could you verify, if no_data variable is set correctly?

Hi Jaroslav,

We do all tests after this commit. And we can confirm that this logic doesn't work.

I feel the problem is that nobody calls to the "opaque" callback for setting the no_data variable. Please, think on this: how receives the 405 error? It is the satip "client" code. However, this commit only modifies files from the satip "server" part. The solution should be that the satip "client" thread informs the satip "server" thread when the error is received. Or not?

Regards,
D.

The streaming scheduler should deliver SMT_NOSTART message to all message clients when an serious error occured. My point is that I don't like to do a lookup directly to a private value in the scheduler structure.

Jaroslav Kysela wrote:

The streaming scheduler should deliver SMT_NOSTART message to all message clients when an serious error occured. My point is that I don't like to do a lookup directly to a private value in the scheduler structure.

Hi Jaroslav,

Thank you for your patience. We have tested and checked your modifications over our patches, and we have prepared a new patch list. This is because not all problems were solved. We believe that with these new changes all problems will be solved.

In this case an unique patch is provided:

diff --git a/src/input/mpegts/satip/satip_frontend.c b/src/input/mpegts/satip/satip_frontend.c
index f9193c3..acca4a6 100644
--- a/src/input/mpegts/satip/satip_frontend.c
+++ b/src/input/mpegts/satip/satip_frontend.c
@@ -1834,6 +1834,7 @@ new_tune:
                      buf, r, strerror(-r), rtsp->hc_cmd, rtsp->hc_code);
             satip_frontend_tuning_error(lfe, tr);
             fatal = 1;
+            continue;
           } else {
             strncpy((char *)session, rtsp->hc_rtsp_session ?: "", sizeof(session));
             session[sizeof(session)-1] = '\0';
diff --git a/src/satip/rtsp.c b/src/satip/rtsp.c
index 31fa433..23afb5b 100644
--- a/src/satip/rtsp.c
+++ b/src/satip/rtsp.c
@@ -205,7 +205,7 @@ rtsp_find_session(http_connection_t *hc, int stream)
     if (hc->hc_session &&
         strcmp(rs->session, hc->hc_session) == 0 &&
         strcmp(rs->peer_ipstr, hc->hc_peer_ipstr) == 0) {
-      if (stream == rs->stream)
+      if ((stream == rs->stream) && (rs->no_data == 0))
         return rs;
       if (first == NULL)
         first = rs;
@@ -586,9 +586,13 @@ rtsp_start
               (rtsp_muxcnf == MUXCNF_REJECT || rtsp_muxcnf == MUXCNF_REJECT_EXACT_MATCH ) ? " (configuration)" : "");
       goto endclean;
     }
+    if (rs->subs && rs->no_data) {
+      dvb_mux_conf_str(&rs->dmc, buf, sizeof(buf));
+      tvhwarn(LS_SATIPS, "%i/%s/%i: subscription fails because mux %s can't tune",
+              rs->frontend, rs->session, rs->stream, buf);
+      goto endclean;
+    }
     if (rs->mux == mux && rs->subs) {
-      if (rs->no_data)
-        goto endclean;
       goto pids;
     }
     rtsp_clean(rs);
@@ -954,7 +958,9 @@ rtsp_parse_cmd
       if (delsys == DVB_SYS_NONE) goto end;
       if (msys == DVB_SYS_NONE) goto end;
       if (!(*valid)) goto end;
-      alloc_stream_id = 1;
+      if (!(stream > 0) || (freq != rs->dmc.dmc_fe_freq)) {
+        alloc_stream_id = 1;
+      }
     } else if (stream != rs->stream) {
       rs = rtsp_new_session(hc->hc_peer_ipstr, msys, rs->nsession, stream);
       if (delsys == DVB_SYS_NONE) goto end;
@@ -985,6 +991,11 @@ rtsp_parse_cmd
     }
     rs->rtp_peer_port = r;
     rs->frontend = fe > 0 ? fe : 1;
+    if (stream > 0) {
+      /* At this point a SETUP cmd with a STREAM parameter is received,
+         so enable the play data flag! */
+      *oldstate = STATE_PLAY;
+    }
   } else {
     if (!rs || stream != rs->stream) {
       if (rs)
@@ -1138,8 +1149,10 @@ rtsp_parse_cmd
     rs->stream = stream_id % 0x7fff;
   } else {
     /* don't subscribe to the new mux, if it was already done */
-    if (memcmp(dmc, &rs->dmc_tuned, sizeof(*dmc)) == 0)
-      *valid = 0;
+    if (memcmp(dmc, &rs->dmc_tuned, sizeof(*dmc)) == 0) {
+      if (!rs->no_data)
+        *valid = 0;
+    }
   }

   if (cmd == RTSP_CMD_DESCRIBE)
diff --git a/src/tcp.c b/src/tcp.c
index 04c1998..30a96c7 100644
--- a/src/tcp.c
+++ b/src/tcp.c
@@ -91,6 +91,7 @@ tcp_connect(const char *hostname, int port, const char *bindaddr,
       snprintf(errbuf, errbufsize, "Cannot bind to addr '%s'", bindaddr);
       return -1;
     }
+    IP_PORT_SET(bindip, htons(0));
   }

   snprintf(portstr, 6, "%u", port);
@@ -135,9 +136,9 @@ again:

   if (bindip.ss_family != AF_UNSPEC) {
     if (bind(fd, (struct sockaddr *)&bindip, IP_IN_ADDRLEN(bindip)) < 0) {
-      snprintf(errbuf, errbufsize, "Cannot bind to IPv%s addr '%s'",
+      snprintf(errbuf, errbufsize, "Cannot bind to IPv%s addr '%s:%i' (failed: %s)",
                                    bindip.ss_family == AF_INET6 ? "6" : "4",
-                                   bindaddr);
+                                   bindaddr,htons(IP_PORT(bindip)),strerror(errno));
       goto error;
     }
   }

Comments about the patch:

1) The first modification (last in the patch) is in the file "src/tcp.c". This is required as the revision 2a5aab6b has generated a side effect. The problem is quite obscure. Without this patch the source port for connecting to the remote host will be the same all the time. This is only true when binding to an specific source address. Setting the value '0' for the source port leaves the kernel the option for autoselecting one free port.

2) The second variation is related to the issue '4' in our original patch (aka, the no_data function). We agree that your solution is be far more elegant and robust. However, after a deep analysis of the code and some tests we detected that the behavior is inconsistent. So, two changes are required. The first is a simple 'continue' command in the "satip_frontend.c" when the tuner fails. And the second is the changes at lines 205 & 586+ in "rtsp.c". Both blocks are required to send a correct response to the SAT>IP client when the tuner has no data.

3) The third change is related to the creation of the a new stream value. We need to re-introduce our modification, now at block 954, for not generating a new value when the SETUP command has the stream parameter. Please, be advised that without this patch a lot of hardware clients will refuse to work. The solution is quite simple: not allocate a new value when the value is in the SETUP and the frequency is the same.

4) The last revision is one issue that you forget to apply from our patches. When a SETUP command uses a running stream, then the PLAY command is not required for starting the streaming. Therefore, our patch for the block at 985 tigers the auto-play when this condition occurs. Once again, without this patch a lot of hardware clients refuse to complete the scanning process.

So, we hope that you will agree with this patch and commit it.

However, there is still room for improvements to be incorporated.
One issue, that we can't solve, is to remove the "grace period" when the remote SAT>IP server responds with a 405. With this response the frequency is permanently unavailable. So the best behavior is not wait more and go for testing with the next tuner or inform directly to the upper layers. Be free to improve this if you like. Without this enhancement the scanning process is quite slow when certain frequencies are not used.

Regards,
D.

Thank you for your contribution, I applied some pieces (many thanks for tcp.c fixes), but several things are not straight:

diff --git a/src/satip/rtsp.c b/src/satip/rtsp.c
index 31fa433..23afb5b 100644
--- a/src/satip/rtsp.c
+++ b/src/satip/rtsp.c
@@ -205,7 +205,7 @@ rtsp_find_session(http_connection_t *hc, int stream)
     if (hc->hc_session &&
         strcmp(rs->session, hc->hc_session) == 0 &&
         strcmp(rs->peer_ipstr, hc->hc_peer_ipstr) == 0) {
-      if (stream == rs->stream)
+      if ((stream == rs->stream) && (rs->no_data == 0))
         return rs;
       if (first == NULL)
         first = rs;

This look like a really horrible condition.

@@ -954,7 +958,9 @@ rtsp_parse_cmd
       if (delsys == DVB_SYS_NONE) goto end;
       if (msys == DVB_SYS_NONE) goto end;
       if (!(*valid)) goto end;
-      alloc_stream_id = 1;
+      if (!(stream > 0) || (freq != rs->dmc.dmc_fe_freq)) {
+        alloc_stream_id = 1;
+      }
     } else if (stream != rs->stream) {
       rs = rtsp_new_session(hc->hc_peer_ipstr, msys, rs->nsession, stream);
       if (delsys == DVB_SYS_NONE) goto end;

This code change is probably required, because of the no_data condition in rtsp_find_session() - previous patch. It does not look like a correct thing.

@@ -586,9 +586,13 @@ rtsp_start
               (rtsp_muxcnf == MUXCNF_REJECT || rtsp_muxcnf == MUXCNF_REJECT_EXACT_MATCH ) ? " (configuration)" : "");
       goto endclean;
     }
+    if (rs->subs && rs->no_data) {
+      dvb_mux_conf_str(&rs->dmc, buf, sizeof(buf));
+      tvhwarn(LS_SATIPS, "%i/%s/%i: subscription fails because mux %s can't tune",
+              rs->frontend, rs->session, rs->stream, buf);
+      goto endclean;
+    }
     if (rs->mux == mux && rs->subs) {
-      if (rs->no_data)
-        goto endclean;
       goto pids;
     }
     rtsp_clean(rs);

The only difference is that you check no_data for any new mux, but the current code does 'quick failure' only when new requested mux is identical to previous (when things are working as I expect), because we don't know, if new mux will fail.

@@ -985,6 +991,11 @@ rtsp_parse_cmd
     }
     rs->rtp_peer_port = r;
     rs->frontend = fe > 0 ? fe : 1;
+    if (stream > 0) {
+      /* At this point a SETUP cmd with a STREAM parameter is received,
+         so enable the play data flag! */
+      *oldstate = STATE_PLAY;
+    }
   } else {
     if (!rs || stream != rs->stream) {
       if (rs)

The *oldstate should be handled only when 'session is found' and play command was issued before - the code '*oldstate = rs->state' tries to do this (few lines above in the source code).

Jaroslav Kysela wrote:

Thank you for your contribution, I applied some pieces (many thanks for tcp.c fixes), but several things are not straight:

Dear Jaroslav,

Thank you for accepting our proposals!

As for the parts of the patch that are not understood (I change to reverse order for a more simple explanation):

Issue 4:¶

@@ -985,6 +991,11 @@ rtsp_parse_cmd
     }
     rs->rtp_peer_port = r;
     rs->frontend = fe > 0 ? fe : 1;
+    if (stream > 0) {
+      /* At this point a SETUP cmd with a STREAM parameter is received,
+         so enable the play data flag! */
+      *oldstate = STATE_PLAY;
+    }
   } else {
     if (!rs || stream != rs->stream) {
       if (rs)

The *oldstate should be handled only when 'session is found' and play command was issued before - the code '*oldstate = rs->state' tries to do this (few lines above in the source code).

If you force this behavior (only when session is found and play was issued before), then a lot of clients will be in trouble. To illustrate the problem think on this particular case:

C->S: SETUP
S->C: OK, stream=1
C->S: PLAY stream=1
S->C: OK, stream=1 (the streaming starts)
C->S: PLAY stream=1 (changing the freq.)
S->C: ERROR, stream=1 (the streaming continues, as no TEARDOWN is received)
C->S: SETUP stream=1 (changing again to a new freq.)
S->C: OK, stream=1

Now, in what state should be the stream? Remember that one SETUP after a PLAY implies a running streaming. Even if the stream is ‘invalid’. Despite what you may think several hardware clients do this assumption: any SETUP made after a first PLAY will imply that the stream continues running. However the current code (without our patch) is waiting for a second PLAY after the SETUP that will never arrive. You can found similar examples. So, our patch targets to this specific case. And how can we verify that the SETUP is after a PLAY? Because the SETUP includes the stream parameter, which is only true when the client has initiated a session.

Perhaps you can find a better option. But our patch is quite simple and effective. And without it a lot of clients fail.

Issue 3:¶

@@ -586,9 +586,13 @@ rtsp_start
               (rtsp_muxcnf == MUXCNF_REJECT || rtsp_muxcnf == MUXCNF_REJECT_EXACT_MATCH ) ? " (configuration)" : "");
       goto endclean;
     }
+    if (rs->subs && rs->no_data) {
+      dvb_mux_conf_str(&rs->dmc, buf, sizeof(buf));
+      tvhwarn(LS_SATIPS, "%i/%s/%i: subscription fails because mux %s can't tune",
+              rs->frontend, rs->session, rs->stream, buf);
+      goto endclean;
+    }
     if (rs->mux == mux && rs->subs) {
-      if (rs->no_data)
-        goto endclean;
       goto pids;
     }
     rtsp_clean(rs);

The only difference is that you check no_data for any new mux, but the current code does 'quick failure' only when new requested mux is identical to previous (when things are working as I expect), because we don't know, if new mux will fail.

You missed a specific case: When a tuning process fails, the “rs->no_data” flag is set and the “mux” could have been destroyed. Then you also need to inform the clients of the failures of the “new” muxes. In fact, they are not “new” because the tuning will only be invalid if someone has requested it. Furthermore, you're forgetting that the client has received an OK to his tuning request. Only some time later (when you receive the response from the remote server) the session will be invalid with the flasg “no_data”. So, from the client’s point of view, he must to retry for the tuning process... and then this “new” SETUP will generate a “new” mux. But, the session is the same, and only now you know that the session is invalid and you must send the 405 response.

In all of our tests, without this modification the client never receives the 405 response. If you find an improved method to check the “no_data” flag when a client makes a SETUP, then please do it. In any case it is necessary to do it asynchronously.

Issue 1 & 2:¶

@@ -205,7 +205,7 @@ rtsp_find_session(http_connection_t *hc, int stream)
     if (hc->hc_session &&
         strcmp(rs->session, hc->hc_session) == 0 &&
         strcmp(rs->peer_ipstr, hc->hc_peer_ipstr) == 0) {
-      if (stream == rs->stream)
+      if ((stream == rs->stream) && (rs->no_data == 0))
         return rs;
       if (first == NULL)
         first = rs;

This look like a really horrible condition.

@@ -954,7 +958,9 @@ rtsp_parse_cmd
       if (delsys == DVB_SYS_NONE) goto end;
       if (msys == DVB_SYS_NONE) goto end;
       if (!(*valid)) goto end;
-      alloc_stream_id = 1;
+      if (!(stream > 0) || (freq != rs->dmc.dmc_fe_freq)) {
+        alloc_stream_id = 1;
+      }
     } else if (stream != rs->stream) {
       rs = rtsp_new_session(hc->hc_peer_ipstr, msys, rs->nsession, stream);
       if (delsys == DVB_SYS_NONE) goto end;

This code change is probably required, because of the no_data condition in rtsp_find_session() - previous patch. It does not look like a correct thing.

We'll run more tests. Maybe you're right!
Be on hold and we will send you more information.

Regards,
D.

Pls, test v4.3-462-g7672841a9 . The commit https://tvheadend.org/projects/tvheadend/repository/revisions/7672841a98ed26733834a758a630ee0356839522 should fix the both issues.

Jaroslav Kysela wrote:

Pls, test v4.3-462-g7672841a9 . The commit https://tvheadend.org/projects/tvheadend/repository/revisions/7672841a98ed26733834a758a630ee0356839522 should fix the both issues.

Hi Jaroslav,

Thank you for your recent updates!
After doing some initial tests with certain hardware clients, we can consider all issues solved... except one. Let me explain more about it:

Issue 4:¶

The problem that has not yet been fixed is the case that we may call auto-PLAY after SETUP with stream parameter. As discussed before, the specification in this case is not very clear. Furthermore, many (if not all) hardware clients are making this assumption: after a SETUP with a stream value the PLAY of the stream is implicit. You are thinking correctly if you suppose that with a new stream value it's required to send a PLAY for that stream. However, this is not the case for the greater part of the SAT>IP client implementations.

Here’s a case that fails and illustrates the problem:

- List of frequencies to scan: A, B, C, D, E, F.
- Valid frequencies: A, B, E, F
- Unavailable frequencies: C, D (the SAT>IP server returns 405)

In this scenario this is the current behavior:

C->S: SETUP freq=A
S->C: OK, stream=1
C->S: PLAY stream=1
S->C: OK, stream=1 (the streaming ‘starts’ with RTCP+RTP)
C->S: SETUP stream=1 freq=B
S->C: OK, stream=2 (the streaming ‘continues’ with RTCP+RTP)
C->S: SETUP stream=2 freq=C
S->C: OK, stream=3 (the streaming ‘continues’ only with RTCP messages)
C->S: SETUP stream=3 freq=C
S->C: 405, stream=4 (the streaming ‘stops’)
C->S: SETUP stream=4 freq=D
S->C: OK, stream=5 (the streaming ‘starts’ only with RTCP messages)
C->S: SETUP stream=5 freq=D
S->C: 405, stream=6 (the streaming ‘stops’)
C->S: SETUP stream=6 freq=E
S->C: OK, stream=7 (the streaming ‘starts’ only with RTCP messages!!!)
Trouble: * No RTP messages *
C->S: SETUP stream=7 freq=F
S->C: OK, stream=8 (the streaming ‘continues’ with RTCP+RTP)
C->S: TEARDOWN stream=8
S->C: OK (the streaming ‘stops’)

As you can see, the problem is with the following frequency after one invalid frequency. In the current implementation the streaming of the RTP is never started. Our patch targets this case by setting the STATE_PLAY when a SETUP message arrives with a stream parameter.

Maybe you can find another solution. However, in our case without this patch the scanning process isn't completed for all valid frequencies. So this patch is a must have.

@@ -985,6 +991,11 @@ rtsp_parse_cmd
     }
     rs->rtp_peer_port = r;
     rs->frontend = fe > 0 ? fe : 1;
+    if (stream > 0) {
+      /* At this point a SETUP cmd with a STREAM parameter is received,
+         so enable the play data flag! */
+      *oldstate = STATE_PLAY;
+    }
   } else {
     if (!rs || stream != rs->stream) {
       if (rs)

We hope that you can commit a solution to this problem.
Regards,
D.

If the client tries to override the allocated/running stream, then it should reach this block:

      if (!has_args && rs->state == STATE_DESCRIBE) {
        r = parse_transport(hc);
        if (r < 0) {
          errcode = HTTP_STATUS_BAD_TRANSFER;
          goto end;
        }
        rs->rtp_peer_port = r;
        *valid = 1;
        goto ok;
      }
      *oldstate = rs->state;

The rs->state should be STATE_PLAY when one PLAY was sent by the client. With my modification, the rs->state is not touched on error (only subscribed service is unsubscribed).

Looking again to the code, the oldstate variable should be probably removed and we can introduce variable 'playing' in session_t to make code more readable.

https://tvheadend.org/projects/tvheadend/repository/revisions/453a3367bc4e5a303cb1560ac4ac021f31b0b6a0

v4.3-467-g453a3367b

A quick fix for the playing flag:

https://tvheadend.org/projects/tvheadend/repository/revisions/54c9997177852f390f98039241b008132493d1f7

v4.3-468-g54c999717

Jaroslav Kysela wrote:

The rs->state should be STATE_PLAY when one PLAY was sent by the client. With my modification, the rs->state is not touched on error (only subscribed service is unsubscribed).

Looking again to the code, the oldstate variable should be probably removed and we can introduce variable 'playing' in session_t to make code more readable.
https://tvheadend.org/projects/tvheadend/repository/revisions/453a3367bc4e5a303cb1560ac4ac021f31b0b6a0
v4.3-467-g453a3367b

A quick fix for the playing flag:
https://tvheadend.org/projects/tvheadend/repository/revisions/54c9997177852f390f98039241b008132493d1f7
v4.3-468-g54c999717

Hi,

Sorry, but your last commit doesn’t fix the problem. The same behaviour continues: after one invalid frequency, the next valid frequency isn’t started. The reason seems to be:

- When an error is received in the tuner, then you indirectly reset the new “playing” flag. The action is done in the function “rtsp_close_session()”:

• Error received:
  http://github.com/tvheadend/tvheadend/blob/master/src/satip/rtsp.c#L1431
• Error block executed after:
  http://github.com/tvheadend/tvheadend/blob/master/src/satip/rtsp.c#L1464
• Flag reset after the error:
  http://github.com/tvheadend/tvheadend/blob/master/src/satip/rtsp.c#L1628

Therefore, as the assumption is that a SETUP with a stream parameter implies an implicit PLAY... why not set the flag “playing” when you receive a SETUP with a stream parameter? I think this is the simplest solution, don't you think so?

Regards.

This is because of #4471 .

https://tvheadend.org/projects/tvheadend/repository/revisions/72128777940978239ba535c98d2c27648687c93f

Could you try to remove this code block?

Jaroslav Kysela wrote:

I'm closing this lengty bug. The above patch is reverted so last issue should be resolved, too. Please, create a new bug for new issue.

I like only to comment that with the last commit it seems that all SAT>IP bugs are resolved.

Please, backport them if you can.

Already done. v4.2.3-80-g2296cfd32