Bug #4499

SAT>IP: Fix five different bugs

Added by DSA APF 5 months ago. Updated 3 months ago.

Status:FixedStart date:2017-07-24
Priority:NormalDue date:
Assignee:Jaroslav Kysela% Done:

100%

Category:SAT>IP
Target version:-
Found in version:master Affected Versions:

Description

Hi Jaroslav,

After a lot of testing and debugging, we are glad to publish a fix for some SAT>IP server bugs.
We present the patches divided into 5 groups for a better understanding. In fact, the more relevant fix is the last one, but we present them in reverse order.

1) Add default short DESCRIBE:

At this time, when a session has incomplete configuration data or the delivery system is unsupported, in this case the session description is incomplete. This patch implements a short/neutral response. This is required by some clients that refuse to continue if the response is incomplete.

--- a/src/satip/rtp.c
+++ b/src/satip/rtp.c
@@ -811,7 +811,9 @@ satip_status_build(satip_rtp_session_t *rtp, char *buf, int len)
       c2tft, ds, plp, specinv, pids);
     break;
   default:
-    return 0;
+    // Send a short description (required by some SAT>IP clients)
+    r = snprintf(buf, len, "ver=1.0;src=%d;tuner=%d,%d,%d,%d,,,,,,,,;pids=%s",
+        rtp->source, rtp->frontend, level, lock, quality, pids);
   }

   return r >= len ? len - 1 : r;

2) Fix for incorrect delivery system in SESSION:

With the current implementation, when a new session is created the delivery session is not set correctly. Only the value for the MUX is set, but not the value in the SESSION.

--- a/src/satip/rtsp.c
+++ b/src/satip/rtsp.c
@@ -562,6 +562,7 @@ rtsp_start
                            &rs->dmc, 1);
       if (mux) {
         created = 1;
+        dmc = ((dvb_mux_t *)mux)->lm_tuning;
         rs->perm_lock = 1;
       }
     }
@@ -1109,6 +1110,7 @@ rtsp_parse_cmd

   dmc->dmc_fe_freq = freq;
   dmc->dmc_fe_modulation = mtype;
+  dmc->dmc_fe_delsys = delsys;
   rs->delsys = delsys;
   rs->frontend = fe;
   rs->findex = findex;

3) Fix incorrect Frontend & Source values in the DESCRIPTON:

The current implementation generates incorrect values for the feID and srcID in the description responses (for both, the RTCP header and for the DESCRIBE request). As documented in the “3.5.11 Query Syntax” section of the SAT>IP Standard Specification these values start with “1” and not “0”. The range for feID is [1..65535], and the range for srcID is [1..255]. It’s very easy to check the values generated by the TVH using the tcpdump command. Currently, “src=” and “tuner=” are often equal to “0”. The problem is in the initialization of the RTP queue. The call to the “[email protected]_rtp_queue()” function has incorrect call parameters when called from “[email protected]_start()”. Furthermore, the current code uses three values: frontend, frontend-index, and source. However, only two are defined in the standard. The provided patch is sufficiently tested and it uses frontend-index and source as the parameters for creating the RTP stream. In addition, the initialization of the frontend-index is set to 1 instead of 0.

--- a/src/satip/rtsp.c
+++ b/src/satip/rtsp.c
@@ -626,7 +626,7 @@ pids:
                     &hc->hc_fd_lock, hc->hc_peer, rs->rtp_peer_port,
                     rs->udp_rtp ? rs->udp_rtp->fd : hc->hc_fd,
                     rs->udp_rtcp ? rs->udp_rtcp->fd : -1,
-                    rs->frontend, rs->findex, &rs->dmc_tuned,
+                    rs->findex, rs->src, &rs->dmc_tuned,
                     &rs->pids,
                     ocmd == RTSP_CMD_PLAY || oldstate == STATE_PLAY,
                     rs->perm_lock);
@@ -881,7 +881,7 @@ rtsp_parse_cmd
    session_t **rrs, int *valid, int *oldstate)
 {
   session_t *rs = NULL;
-  int errcode = HTTP_STATUS_BAD_REQUEST, r, findex = 0, has_args, weight = 0;
+  int errcode = HTTP_STATUS_BAD_REQUEST, r, findex = 1, has_args, weight = 0;
   int delsys = DVB_SYS_NONE, msys, fe, src, freq, pol, sr;
   int fec, ro, plts, bw, tmode, mtype, gi, plp, t2id, sm, c2tft, ds, specinv;
   char *s;

4) Fix response with a remote SAT>IP server returning BAD SERVICE:

This is a very obscure bug. The problem occurs when you use two instances of the TVH. One is the local server and the second is the remote server. When a client requests for a MUX to the local server, and the remote server responses with a 405-STATUS NOT ALLOWED (or a similar error), in this case the client never receives an error. Therefore, the client only continues after its timeout expires. The solution is recheck for every client request if the associated subscription is invalid. This is the way it works, since the asynchronous coupling between the SAT>IP server and client process inside the TVH makes it impossible to send back the response from the remote server. Fortunately, all SAT>IP clients resend the tuning command after a short period of time when the signal is unlocked. This patch fixes this problem with a very simple implementation.

--- a/src/satip/rtsp.c
+++ b/src/satip/rtsp.c
@@ -573,6 +573,12 @@ rtsp_start
               (rtsp_muxcnf == MUXCNF_REJECT || rtsp_muxcnf == MUXCNF_REJECT_EXACT_MATCH ) ? " (configuration)" : "");
       goto endclean;
     }
+    if (mux && rs->subs && rs->subs->ths_state == SUBSCRIPTION_BAD_SERVICE) {
+      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)
       goto pids;
     rtsp_clean(rs);

5) Fix SETUP with STREAM parameter:

This is a definitive patch for resolve the problem described in #4474. Two questions are addressed in this patch:

a) The generation of a new stream identifier is restricted only to cases when it is necessary. For example, when a new SETUP command is received without a stream parameter. Or when a new subscription is created without a corresponding stream id. Additionally, the code now verifies that the stream value starts with “1” and not “0”. Therefore, each time the stream value at the end of the “rtsp_parse_cmd()” function is less than 1 a new identifier is created... and only in this case! So, in the case of a SETUP command with a stream parameter the received stream id is then reused.

b) The second problem is the behaviour of the RTP queue when a SETUP with a stream parameter is received. In this case, the stream must be in play when the signal is locked. Furthermore, and related to the previous fix (4), it’s required to not close the current session in case of receiving the same SETUP request. Only when the SETUP targets a new session, the current session assigned to the stream id must be closed.

This patch then replaces the burden solution described in #4474.

--- a/src/satip/rtsp.c
+++ b/src/satip/rtsp.c
@@ -964,7 +964,11 @@ rtsp_parse_cmd
         goto ok;
       }
       *oldstate = rs->state;
-      rtsp_close_session(rs);
+      /* At this point a SETUP cmd with a STREAM parameter is received,
+         only close the current session if the new request is different! */
+      if (freq != rs->dmc.dmc_fe_freq) {
+         rtsp_close_session(rs);
+      }
     }
     if (cmd == RTSP_CMD_SETUP) {
       r = parse_transport(hc);
@@ -980,6 +984,11 @@ rtsp_parse_cmd
     }
     rs->frontend = fe > 0 ? fe : 1;
     dmc = &rs->dmc;
+    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)
@@ -1127,10 +1136,16 @@ rtsp_parse_cmd
   rs->old_hc = hc;

   if (cmd == RTSP_CMD_SETUP) {
+    if ((stream > 0) && (rs->stream < 1)) {
+      rs->stream = stream;
+    }
+  }
+  if (rs->stream < 1) {
     stream_id++;
     if (stream_id == 0)
       stream_id++;
     rs->stream = stream_id % 0x7fff;
+    tvhinfo(LS_SATIPS, "new stream-id generated (%d)",rs->stream);
   }
   rs->src = src;

We have tested all of these patches, and hope that they will be commited.
Regards,
D.

Associated revisions

Revision ca308e0a
Added by Jaroslav Kysela 5 months ago

satip server: add short DESCRIBE, fix incorrect delivery system, fix frontend and source in DESCRIBE, fixes #4499

Revision debe129a
Added by Jaroslav Kysela 5 months ago

satip server: try to fix the stream/session mismatch, fixes #4499

Revision 4ee77e12
Added by Jaroslav Kysela 5 months ago

satip server: signalize pernament 'no data' state to rtsp layer, fixes #4499

Revision eb828fd4
Added by Jaroslav Kysela 3 months ago

satip client: fix the error path, issue #4499

Revision be637ea7
Added by Jaroslav Kysela 3 months ago

tcp: tcp_connect - initialize bindip variable correctly, issue #4499

Revision db593aa4
Added by Jaroslav Kysela 3 months ago

satip server: add one more no_data check, issue #4499

Revision 7672841a
Added by Jaroslav Kysela 3 months ago

satip server: try to change state and mux handling logic, issue #4499

Revision 453a3367
Added by Jaroslav Kysela 3 months ago

satip server: rtsp - replace oldstate with playing session flag, issue #4499

Revision 54c99971
Added by Jaroslav Kysela 3 months ago

satip server: rtsp - fix playing session flag, issue #4499

Revision f62eaafd
Added by Jaroslav Kysela 3 months ago

satip server: add short DESCRIBE, fix incorrect delivery system, fix frontend and source in DESCRIBE, fixes #4499

Revision 46f32702
Added by Jaroslav Kysela 3 months ago

satip server: try to fix the stream/session mismatch, fixes #4499

Revision 9480a970
Added by Jaroslav Kysela 3 months ago

satip server: signalize pernament 'no data' state to rtsp layer, fixes #4499

Revision c2aee8e3
Added by Jaroslav Kysela 3 months ago

satip client: fix the error path, issue #4499

Revision b015c024
Added by Jaroslav Kysela 3 months ago

tcp: tcp_connect - initialize bindip variable correctly, issue #4499

Revision 8cf79e89
Added by Jaroslav Kysela 3 months ago

satip server: try to change state and mux handling logic, issue #4499

Revision 00f5be55
Added by Jaroslav Kysela 3 months ago

satip server: rtsp - replace oldstate with playing session flag, issue #4499

Revision 3fc99837
Added by Jaroslav Kysela 3 months ago

satip server: rtsp - fix playing session flag, issue #4499

Revision e7ad8de9
Added by Jaroslav Kysela 2 months ago

satip server: add one more no_data check, issue #4499

History

#1 Updated by Jaroslav Kysela 5 months ago

  • Status changed from New to Fixed
  • % Done changed from 0 to 100

#2 Updated by Jaroslav Kysela 5 months ago

  • Status changed from Fixed to Accepted

Applied 1-3. I need think more about 4 and I tried to resolve the SETUP/PLAY states in another way - https://tvheadend.org/projects/tvheadend/repository/revisions/debe129a49d90570f485e262ed0d21dede87b831 .

#3 Updated by DSA APF 5 months ago

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,

#4 Updated by Jaroslav Kysela 5 months ago

  • Status changed from Accepted to Fixed

#5 Updated by Jaroslav Kysela 5 months ago

  • Status changed from Fixed to Accepted

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

#6 Updated by Mono Polimorph 4 months ago

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!

#7 Updated by Mono Polimorph 4 months ago

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?

#8 Updated by DSA APF 4 months ago

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.

#9 Updated by Jaroslav Kysela 4 months ago

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

#10 Updated by DSA APF 4 months ago

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.

#11 Updated by Jaroslav Kysela 4 months ago

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.

#12 Updated by DSA APF 3 months ago

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.

#13 Updated by Jaroslav Kysela 3 months ago

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).

#14 Updated by Mono Polimorph 3 months ago

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.

#15 Updated by Jaroslav Kysela 3 months ago

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

#16 Updated by Mono Polimorph 3 months ago

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.

#17 Updated by Jaroslav Kysela 3 months ago

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.

#20 Updated by Mono Polimorph 3 months ago

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()”:

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.

#22 Updated by Jaroslav Kysela 3 months ago

  • Status changed from Accepted to Fixed

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.

#23 Updated by Mono Polimorph 3 months ago

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.

#24 Updated by Jaroslav Kysela 3 months ago

Already done. v4.2.3-80-g2296cfd32

Also available in: Atom PDF