Project

General

Profile

Feature #4505

SAT>IP: configurable RTP/AVP/TCP support

Added by Mono Polimorph over 7 years ago. Updated about 7 years ago.

Status:
Fixed
Priority:
Normal
Category:
SAT>IP
Target version:
-
Start date:
2017-07-07
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)

Description

Hi,

I improved my previous patch about the RTP/AVP/TCP support (#4476).

Now I added the configuration in the client side at TUNER level. This works like this:

  • By default TVH not use RTP_over_TCP with a SAT>IP server.
  • But, if you enable RTP_over_TCP for a specific SERVER then you (always) use TCP for connections to this server (if the server supports it).
  • Now you can disable the RTP_over_TCP at TUNER level. So, if you have configured the server for TCP, then you can select now which tuners from this server you want to transport using regular RTP over UDP.

Additionally, this patch includes my previous one for disabling at SAT>IP support for RTP_over_TCP at server level.

In which scenario you want to use some tuners with TCP and some others with UDP with the same server? One example:

  • Imagine that you want to use the position AA (src=1) for UDP, and position AB (src=2) for TCP (both for the same real position). The position value is very easy to configure (any modify) in any SAT>IP client. Then you can create two networks in your TVH. The first one for UDP and the second for TCP. Then in the SAT>IP client configuration you can duplicate the number of the tuners in your server. And you can copy the configuration of the previous tuners to the duplicates. Now, you only need to enable the RTP/AVP/TCP support for the entire server, and disable it for the original tuners. From this point only rests to configure the duplicated tuners with the new network (the one with position 2)... And magically when you request position AA you will use the original tuners with UDP, and when request with position AB you receive the same using TCP.

I found this very useful. So, I suggest to accept this new patch:

---
 src/input/mpegts/satip/satip_frontend.c |   12 +++++++++++-
 src/input/mpegts/satip/satip_private.h  |    1 +
 src/satip/rtsp.c                        |    2 +-
 src/satip/server.c                      |   10 ++++++++++
 src/satip/server.h                      |    1 +
 5 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/src/input/mpegts/satip/satip_frontend.c b/src/input/mpegts/satip/satip_frontend.c
index f9193c3..91582ca 100644
--- a/src/input/mpegts/satip/satip_frontend.c
+++ b/src/input/mpegts/satip/satip_frontend.c
@@ -165,6 +165,16 @@ const idclass_t satip_frontend_class =
       .opts     = PO_ADVANCED,
     },
     {
+      .type     = PT_BOOL,
+      .id       = "no_tcp_mode",
+      .name     = N_("Not use RTP/AVP/TCP"),
+      .desc     = N_("Not use the RTP/AVP/TCP transfer mode " 
+                     "for this tuner even if the server supports it " 
+                     "and the client is configured to do so."),
+      .off      = offsetof(satip_frontend_t, sf_no_tcp_mode),
+      .opts     = PO_ADVANCED,
+    },
+    {
       .type     = PT_INT,
       .id       = "tdelay",
       .name     = N_("Next tune delay in ms (0-2000)"),
@@ -1569,7 +1579,7 @@ new_tune:
   seq         = -1;
   lfe->sf_seq = -1;
   play2       = 1;
-  rtsp_flags  = lfe->sf_device->sd_tcp_mode ? SATIP_SETUP_TCP : 0;
+  rtsp_flags  = lfe->sf_device->sd_tcp_mode ? lfe->sf_no_tcp_mode ? 0 : SATIP_SETUP_TCP : 0;

   if ((rtsp_flags & SATIP_SETUP_TCP) == 0) {
     if (udp_bind_double(&rtp, &rtcp,
diff --git a/src/input/mpegts/satip/satip_private.h b/src/input/mpegts/satip/satip_private.h
index 7c49ea1..b263cb4 100644
--- a/src/input/mpegts/satip/satip_private.h
+++ b/src/input/mpegts/satip/satip_private.h
@@ -134,6 +134,7 @@ struct satip_frontend
   char                      *sf_type_override;
   int                        sf_master;
   int                        sf_udp_rtp_port;
+  int                        sf_no_tcp_mode;
   int                        sf_play2;
   int                        sf_tdelay;
   int                        sf_grace_period;
diff --git a/src/satip/rtsp.c b/src/satip/rtsp.c
index aa40668..6e46871 100644
--- a/src/satip/rtsp.c
+++ b/src/satip/rtsp.c
@@ -861,7 +861,7 @@ parse_transport(http_connection_t *hc)
     if (a + 1 != b)
       return -1;
     return a;
-  } else if (strncmp(s, "RTP/AVP/TCP;interleaved=0-1", 27) == 0) {
+  } else if ((strncmp(s, "RTP/AVP/TCP;interleaved=0-1", 27) == 0) && !satip_server_conf.satip_notcp_mode) {
     return RTSP_TCP_DATA;
   }
   return -1;
diff --git a/src/satip/server.c b/src/satip/server.c
index c429056..feca0a8 100644
--- a/src/satip/server.c
+++ b/src/satip/server.c
@@ -717,6 +717,16 @@ const idclass_t satip_server_class = {
       .group  = 1,
     },
     {
+      .type   = PT_BOOL,
+      .id     = "satip_notcp_mode",
+      .name   = N_("Disable RTP/AVP/TCP support"),
+      .desc   = N_("Remove server support for RTP/AVP/TCP transfer mode " 
+                   "(embedded data in the RTSP session)."),
+      .off    = offsetof(struct satip_server_conf, satip_notcp_mode),
+      .opts   = PO_EXPERT,
+      .group  = 1,
+    },
+    {
       .type   = PT_U32,
       .id     = "satip_iptv_sig_level",
       .name   = N_("IPTV signal level"),
diff --git a/src/satip/server.h b/src/satip/server.h
index 471027f..f228f60 100644
--- a/src/satip/server.h
+++ b/src/satip/server.h
@@ -47,6 +47,7 @@ struct satip_server_conf {
   int satip_rewrite_pmt;
   int satip_muxcnf;
   int satip_nom3u;
+  int satip_notcp_mode;
   int satip_anonymize;
   int satip_iptv_sig_level;
   int satip_force_sig_level;
--
1.7.10.4


Files


Subtasks

Feature #4476: SAT>IP: Patch for disable RTP/AVP/TCPRejectedJaroslav Kysela

Actions

History

#2

Updated by Mono Polimorph over 7 years ago

Jaroslav Kysela wrote:

Create PR. https://github.com/tvheadend/tvheadend/pulls

Hi,

If I not create the PR the patch will never be accepted? :(

#3

Updated by Jaroslav Kysela over 7 years ago

Or attach the patch to this bug.. I just don't want to retype it.

#4

Updated by Mono Polimorph over 7 years ago

Jaroslav Kysela wrote:

Or attach the patch to this bug.. I just don't want to retype it.

Great! & done!

#5

Updated by Jaroslav Kysela over 7 years ago

  • Status changed from New to Fixed

Applied in changeset commit:tvheadend|aec317cdf6fad2c01fb215d513e22497e70500e1.

#6

Updated by Mono Polimorph over 7 years ago

Jaroslav Kysela wrote:

Applied in changeset commit:tvheadend|aec317cdf6fad2c01fb215d513e22497e70500e1.

Hi,

Thank you for commit my first part of the patch! That is, the part for disabling the server support.

However, the second part is not already commited. This part is for the client support. Futhermore, it's for TUNER level. In my case, this is very useful as you can select TCP or UDP transport based on the 'source' used.

Please, consider to add also this part. :)

#7

Updated by Jaroslav Kysela over 7 years ago

Could you elaborate more why you need a different transfer for one client and one server? I just don't see ANY benefit.

#8

Updated by Mono Polimorph over 7 years ago

Jaroslav Kysela wrote:

Could you elaborate more why you need a different transfer for one client and one server? I just don't see ANY benefit.

Ah, Ok, I do! ;)

- I have my TVH master server running at my home in a PC.

- Also I have one Raspberry Pi traveller mini-server. In this server I installed also a TVH server. This server connects with my master server using the SAT>IP protocol (as the master server is also serving my TV and my notebook).

- Futhermore, my traveller mini-server connects to my home using oVPN. So the connection is done using UDP.

- In my notebook I have installed the free "Satip DVBviewer" for Windows. This SAT>IP client connects to my TVH mini-server. In this client I have configured two satellite positions (Astra-UDP and Astra-TCP). Both are the same, but the position 1 uses regular UDP transport and position 2 uses TCP transport.

- Then when I have a very lossy Internet connection I use the position 2 (TCP), and when I have a reliable connection then I use position 1 (UDP). I don't need to change anything inside the TVH configuration, and in the client it's only change the position. Nothing more.

So, you agree now to add the option for disabling RTP_over_TCP at tuner level?

#9

Updated by Jaroslav Kysela over 7 years ago

I'm not convinced. You may use the TCP data transfer mode only for this config. It seems a bit overcomplicated.

#10

Updated by Mono Polimorph over 7 years ago

Jaroslav Kysela wrote:

I'm not convinced. You may use the TCP data transfer mode only for this config. It seems a bit overcomplicated.

I use both: TCP and UDP. And I prefer to only modify one parameter in the final client (the source), instead of doing changes in the TVH configuration.

Futhermore, the patch is completed and safe (and simple). Why not include it then?

#11

Updated by Mono Polimorph over 7 years ago

Hi,

I prepared a new patch, ready to commit. Now the disable RTP_over_UDP at tuner level is in the EXPERT zone.

Please, consider to accept this patch. It's very simple and non-intrusive.

---
 src/input/mpegts/satip/satip_frontend.c |   12 +++++++++++-
 src/input/mpegts/satip/satip_private.h  |    1 +
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/src/input/mpegts/satip/satip_frontend.c b/src/input/mpegts/satip/satip_frontend.c
index f9193c3..1b85ecc 100644
--- a/src/input/mpegts/satip/satip_frontend.c
+++ b/src/input/mpegts/satip/satip_frontend.c
@@ -154,6 +154,16 @@ const idclass_t satip_frontend_class =
       .opts     = PO_RDONLY | PO_NOSAVE,
       .off      = offsetof(satip_frontend_t, sf_number),
     },
+     {
+      .type     = PT_BOOL,
+      .id       = "no_tcp_mode",
+      .name     = N_("Disable RTP/AVP/TCP transport"),
+      .desc     = N_("Not use the RTP/AVP/TCP transfer mode " 
+                     "for this tuner even if the server supports it " 
+                     "and the client is configured to do so."),
+      .off      = offsetof(satip_frontend_t, sf_no_tcp_mode),
+      .opts     = PO_EXPERT,
+    },
     {
       .type     = PT_INT,
       .id       = "udp_rtp_port",
@@ -1569,7 +1579,7 @@ new_tune:
   seq         = -1;
   lfe->sf_seq = -1;
   play2       = 1;
-  rtsp_flags  = lfe->sf_device->sd_tcp_mode ? SATIP_SETUP_TCP : 0;
+  rtsp_flags  = lfe->sf_device->sd_tcp_mode ? lfe->sf_no_tcp_mode ? 0 : SATIP_SETUP_TCP : 0;

   if ((rtsp_flags & SATIP_SETUP_TCP) == 0) {
     if (udp_bind_double(&rtp, &rtcp,
diff --git a/src/input/mpegts/satip/satip_private.h b/src/input/mpegts/satip/satip_private.h
index 7c49ea1..142375d 100644
--- a/src/input/mpegts/satip/satip_private.h
+++ b/src/input/mpegts/satip/satip_private.h
@@ -133,6 +133,7 @@ struct satip_frontend
   int                        sf_type_v2;
   char                      *sf_type_override;
   int                        sf_master;
+  int                        sf_no_tcp_mode;
   int                        sf_udp_rtp_port;
   int                        sf_play2;
   int                        sf_tdelay;
--
1.7.10.4

Thank you! ;)

#12

Updated by Mono Polimorph about 7 years ago

Mono Polimorph wrote:

Hi,

I prepared a new patch, ready to commit. Now the disable RTP_over_UDP at tuner level is in the EXPERT zone.

Please, consider to accept this patch. It's very simple and non-intrusive.

[...]

Thank you!

Hi Jaroslav,

I prepared this patch weeks ago, and I'm using it!
As you can see, it's impossible that this patch can introduce an adverse effect. Even more, it doesn't change the default configuration. Only if one user likes to use it, he can do it.

I do a lot of tests using RTP and TCP with the SAT>IP protocol. So, changing only the "src=" parameter (orbital position) it's very easy to select the protocol if you configured the TVH properly.

So, please commit it! (/)

#13

Updated by Mono Polimorph about 7 years ago

Jaroslav Kysela wrote:

I'm not convinced. You may use the TCP data transfer mode only for this config. It seems a bit overcomplicated.

Hi,

I found another use of this improvement:

  • Imagine you have two network connections between a remote TVH and a local one. You like to use RTP (UDP) in one path, and RTP-over-RSTP (TCP) over the other. But because the remote server is the same sever, you can only select UDP or TCP, not both.

A simple solution is this patch. It only enforces to not use the Interlaved mode at TUNER level, instead of server level.

So, as the changes are few, without side effects, and grouped in the EXPERT zone... why not accept it?
Regards.

#14

Updated by Jaroslav Kysela about 7 years ago

Create a list (default server config, UDP only, TCP only, allow both). This solution is a bit mix of dog-cat or apple-pear as we say here in Czech.

#15

Updated by Mono Polimorph about 7 years ago

Jaroslav Kysela wrote:

Create a list (default server config, UDP only, TCP only, allow both). This solution is a bit mix of dog-cat or apple-pear as we say here in Czech.

Hi,

Great to hear that you consider to discuss this point!
I aggree that is possible that my patch isn't (again) the best solution. However, think on this:

  • One function is configure the SAT>IP server inside the TVH to support UDP and TCP (the default at time), only UDP (at time possible selecting "not use RTP-over-RTSP"), and only TCP (now impossible). If you conclude that TCP only is a valid mode (I don't think so), then your suggestion of "create a list of valid modes" is the correct way.
  • However, my last patch is for a different behaviour. Is for override the configuration of the SERVER at TUNER level. You can configure the SERVER with TCP, and force one tuner to use only UDP.
  • But, if you say that we can have a "list of valid modes" in a GLOBAL (=SERVER) level, and a "list of valid modes" at a TUNER level (overriding all the global config if it's set), then perhaps this will be the best solution.

If you think that the last option is the best, then I'll go in this direction. ;)
Thank you!

#16

Updated by Jaroslav Kysela about 7 years ago

You may add a list to the server level, too. Perhaps, it will be more readable than 'RTP/AVP/TCP (embedded data)'.

So something like this:

SERVER: Field name: Data transfer mode

  • UDP
  • TCP

(keep satip_device_t->tcp_mode variable for this - zero = UDP, one = TCP)

FRONTEND (tuner): Field name: Data transfer mode

  • Use server config
  • UDP
  • TCP
#17

Updated by Mono Polimorph about 7 years ago

Jaroslav Kysela wrote:

You may add a list to the server level, too. Perhaps, it will be more readable than 'RTP/AVP/TCP (embedded data)'.

OK.
However, you think that TCP only has sense?
Futhermore, in the SERVER will be necessy to support UDP, TCP and BOTH. Right?

#18

Updated by Jaroslav Kysela about 7 years ago

Mono Polimorph wrote:

Jaroslav Kysela wrote:

You may add a list to the server level, too. Perhaps, it will be more readable than 'RTP/AVP/TCP (embedded data)'.

OK.
However, you think that TCP only has sense?

I don't understand. My idea is to have 'global' config which can be 'freely' overriden in the tuner config. So the UDP global + TCP tuner config combo should be also allowed.

Futhermore, in the SERVER will be necessy to support UDP, TCP and BOTH. Right?

There's no fallback or something like that. The client cannot check, if server supports TCP mode, so user should always specify UDP or TCP - not both. TVH will try to use the specified (forced) transfer mode.

#19

Updated by Mono Polimorph about 7 years ago

Jaroslav Kysela wrote:

There's no fallback or something like that. The client cannot check, if server supports TCP mode, so user should always specify UDP or TCP - not both. TVH will try to use the specified (forced) transfer mode.

Yes! Sorry! You're right... in the client side, only has sense one or another, not both.

So, for the SERVER (in the configuration as "client"), the parameters will be:
Data Transport Mode (select supported modes)
[ ] RTP (UDP)
[ ] Interlaved (TCP)

And for the TUNER (in the configuration as "client" as well), the options will be:
Data Transport Mode (select from the list)
[Server Config | RTP | Interlaved]

That's right?

#20

Updated by Jaroslav Kysela about 7 years ago

Yes.

#21

Updated by Mono Polimorph about 7 years ago

Mono Polimorph wrote:

That's right?

Jaroslav Kysela wrote:

Yes.

Hi Jaroslav,

I implemented the patch as suggested!
Some comments:

- I've done some cleaning in "satip.c" as there is a writing error. In addition, I have added some settings that I use.

- For the server configuration I only do aesthetic changes to maintain the same configutation settings as before. However, now it's more clearer (I think).

- The implementation at tuner level is as you suggested. But, I let you decide whether to accept the code that sets the "rtsp_flags" or change it. Why? Because now, if the server isn't configured with TCP but you select TCP, then it's enforced to use TCP. This is good for me, but it's inconsistent because the configuration in the server indicates "supports TCP" and if you don't select it means that you don't have support.

Feel free to commit it as you wish!

Here the patch:

diff --git a/src/input/mpegts/satip/satip.c b/src/input/mpegts/satip/satip.c
index c326233..4b9b0f5 100644
--- a/src/input/mpegts/satip/satip.c
+++ b/src/input/mpegts/satip/satip.c
@@ -198,10 +198,14 @@ static const char *satip_tunercfg_tab[] = {
   "DVBS2-2,DVBT-2",
   "DVBT-1,DVBS2-1",
   "DVBT-2,DVBS2-2",
-  "DVBS2-1,DVB-C1",
-  "DVBS2-2,DVB-C2",
+  "DVBS2-1,DVBC-1",
+  "DVBS2-2,DVBC-2",
   "DVBC-1,DVBS2-1",
   "DVBC-2,DVBS2-2",
+  "DVBS2-4,DVBT-2",
+  "DVBS2-4,DVBC-2",
+  "DVBS2-4,DVBT-2,DVBC-2",
+  "DVBS2-8,DVBT-4,DVBC-4",
   NULL
 };

@@ -257,9 +261,10 @@ const idclass_t satip_device_class =
     {
       .type     = PT_BOOL,
       .id       = "tcp_mode",
-      .name     = N_("RTP/AVP/TCP (embedded data)"),
-      .desc     = N_("Enable or disable RTP/AVP/TCP transfer mode " 
-                     "(embedded data in the RTSP session) support."),
+      .name     = N_("RTP/AVP/TCP transport supported"),
+      .desc     = N_("The server suports the Interlaved TCP transfer mode " 
+                     "(embedded data in the RTSP session). And this option " 
+                     "enables this mode in all tuners by default."),
       .opts     = PO_ADVANCED,
       .off      = offsetof(satip_device_t, sd_tcp_mode),
     },
diff --git a/src/input/mpegts/satip/satip_frontend.c b/src/input/mpegts/satip/satip_frontend.c
index acca4a6..c35011b 100644
--- a/src/input/mpegts/satip/satip_frontend.c
+++ b/src/input/mpegts/satip/satip_frontend.c
@@ -29,6 +29,15 @@
 #include <sys/socket.h>
 #endif

+
+typedef enum rtp_transport_mode
+{
+  RTP_SERVER_DEFAULT,     // Use server configuretion
+  RTP_UDP,                // Use regular RTP
+  RTP_INTERLAVED,         // Use Interlaved RTP/AVP/TCP
+} rtp_transport_mode_t;
+
+
 /*
  *
  */
@@ -136,6 +145,17 @@ satip_frontend_class_override_enum( void * p, const char *lang )
   return m;
 }

+static htsmsg_t *
+satip_frontend_transport_mode_list ( void *o, const char *lang )
+{
+  static const struct strtab tab[] = {
+    { N_("SERVER Config"),   RTP_SERVER_DEFAULT },
+    { N_("RTP over UDP"),    RTP_UDP },
+    { N_("TCP Interlaved"),  RTP_INTERLAVED },
+  };
+  return strtab2htsmsg(tab, 1, lang);
+}
+
 CLASS_DOC(satip_frontend)

 const idclass_t satip_frontend_class =
@@ -156,6 +176,15 @@ const idclass_t satip_frontend_class =
     },
     {
       .type     = PT_INT,
+      .id       = "transport_mode",
+      .name     = N_("Transport mode"),
+      .desc     = N_("Select the transport used for this tuner."),
+      .list     = satip_frontend_transport_mode_list,
+      .off      = offsetof(satip_frontend_t, sf_transport_mode),
+      .opts     = PO_ADVANCED,
+    },
+    {
+      .type     = PT_INT,
       .id       = "udp_rtp_port",
       .name     = N_("UDP RTP port number (2 ports)"),
       .desc     = N_("Force the local UDP Port number here. The number " 
@@ -1569,7 +1598,7 @@ new_tune:
   seq         = -1;
   lfe->sf_seq = -1;
   play2       = 1;
-  rtsp_flags  = lfe->sf_device->sd_tcp_mode ? SATIP_SETUP_TCP : 0;
+  rtsp_flags  = (lfe->sf_transport_mode == RTP_SERVER_DEFAULT)? lfe->sf_device->sd_tcp_mode : (lfe->sf_transport_mode == RTP_INTERLAVED )? SATIP_SETUP_TCP : 0; 

   if ((rtsp_flags & SATIP_SETUP_TCP) == 0) {
     if (udp_bind_double(&rtp, &rtcp,
diff --git a/src/input/mpegts/satip/satip_private.h b/src/input/mpegts/satip/satip_private.h
index 7c49ea1..64ffa90 100644
--- a/src/input/mpegts/satip/satip_private.h
+++ b/src/input/mpegts/satip/satip_private.h
@@ -134,6 +134,7 @@ struct satip_frontend
   char                      *sf_type_override;
   int                        sf_master;
   int                        sf_udp_rtp_port;
+  int                        sf_transport_mode;
   int                        sf_play2;
   int                        sf_tdelay;
   int                        sf_grace_period;

Regards!

#22

Updated by Mono Polimorph about 7 years ago

Hi Jaroslav,

This is implemented as you suggested!

So, please, can you commit it? ;)
Thank you!

PD: I want to apologize for not being able to create a PR in Github, but it's impossible for me at the moment.

#23

Updated by Mono Polimorph about 7 years ago

Still waiting for the acceptance of this patch. ;)

#24

Updated by Mono Polimorph about 7 years ago

I hope this patch will be accepted.
If there is something to change, please tell me what I have to do. ;)

Also available in: Atom PDF