diff mbox

[FFmpeg-devel] make work (live) libsrt

Message ID alpine.LSU.2.20.1808282235430.23954@iq
State New
Headers show

Commit Message

Marton Balint Aug. 28, 2018, 9:27 p.m. UTC 
On Sat, 25 Aug 2018, Tudor Suciu wrote:

> Hello Marton,
>
> The new version takes into account your remarks, modifications.

I checked the srt API, and we are doing this all wrong. 
The SRTO_PAYLOADSIZE socket parameter defines the maximum allowed packet 
size. So we should query this parameter, and set 
UrlContext->max_packet_size accordingly.

And should simply set SRTO_PACKETSIZE to our pkt_size parameter if we 
don't want the libsrt default 1356.

I have attached a new patch. Let me know what you think.

Thanks,
Marton

Comments

Tudor Suciu Aug. 28, 2018, 11:01 p.m. UTC | #1 
Hello Marton,

And should simply set SRTO_PACKETSIZE to our pkt_size parameter if we
> don't want the libsrt default 1356.
>
At least for the specific case of mpegts I believe it's much better to have
fixed size packets(188x*).
It helps error recovery if we don't have to re-synchronize ts so
(7*188=1316) seems a better default.
The 1356 value could be treated as a maximum special case value, all sorts
of network configurations can eat from the MTU (vpn comes to mind).
If I understand correctly the working of libsrt, it "creates" a packet each
time we send it some data. So without touching on the "maximum" value,
h->max_packet_size insures that the "medium" value is effectively the
expected one.
The 1356 value, assuming no vpn/tunnel will spoil the party, will make ts
advance with 40 bytes each packet, so almost any packet loss will produce
ts that does not start with ts header. This doesn't seem good for packet
loss recovery.
Rtp encapsulated mpeg ts with pkt_size of 1316 will add an rtp header of ~
12 bytes so the needed srt payloadsize will be 1328. As wireshark does not
recognize the protocol yet, I couldn't check in detail. I'm absolutely
certain that ffmpeg will do bad things if rtp header is misplaced (not
synchronized with a lost packet), it will end up happily reading random ts
data as rtp header at some point given enough packet loss.

Regards,
Marton Balint Aug. 29, 2018, 8:48 a.m. UTC | #2 
On Wed, 29 Aug 2018, Tudor Suciu wrote:

> Hello Marton,
>
> And should simply set SRTO_PACKETSIZE to our pkt_size parameter if we
>> don't want the libsrt default 1356.
>>
> At least for the specific case of mpegts I believe it's much better to have
> fixed size packets(188x*).
> It helps error recovery if we don't have to re-synchronize ts so
> (7*188=1316) seems a better default.
> The 1356 value could be treated as a maximum special case value, all sorts
> of network configurations can eat from the MTU (vpn comes to mind).
> If I understand correctly the working of libsrt, it "creates" a packet each
> time we send it some data. So without touching on the "maximum" value,
> h->max_packet_size insures that the "medium" value is effectively the
> expected one.
> The 1356 value, assuming no vpn/tunnel will spoil the party, will make ts
> advance with 40 bytes each packet, so almost any packet loss will produce
> ts that does not start with ts header. This doesn't seem good for packet
> loss recovery.
> Rtp encapsulated mpeg ts with pkt_size of 1316 will add an rtp header of ~
> 12 bytes so the needed srt payloadsize will be 1328. As wireshark does not
> recognize the protocol yet, I couldn't check in detail. I'm absolutely
> certain that ffmpeg will do bad things if rtp header is misplaced (not
> synchronized with a lost packet), it will end up happily reading random ts
> data as rtp header at some point given enough packet loss.

I am sorry for the consfusion, I meant 1316 and not 1356, and libsrt 
provides 1316 as default max payload size exactly for the reasons you 
described, and therefore that becomes the default max packet size. So all 
is fine in the code as far as I see, only my descriptive text was 
misleading, sorry.

Thanks,
Marton
Tudor Suciu Aug. 29, 2018, 8:12 p.m. UTC | #3 
Well, when this is done, working, we can begin to talk business:
-add an option to ffmpeg to drop unused input data like srt-file-transmit
(before first client connects)
-add an option/document if it's already working to ffmpeg to have multiple
srt clients like gstreamer

Regards,

On Wed, Aug 29, 2018 at 10:48 AM Marton Balint <cus@passwd.hu> wrote:

>
>
> On Wed, 29 Aug 2018, Tudor Suciu wrote:
>
> > Hello Marton,
> >
> > And should simply set SRTO_PACKETSIZE to our pkt_size parameter if we
> >> don't want the libsrt default 1356.
> >>
> > At least for the specific case of mpegts I believe it's much better to
> have
> > fixed size packets(188x*).
> > It helps error recovery if we don't have to re-synchronize ts so
> > (7*188=1316) seems a better default.
> > The 1356 value could be treated as a maximum special case value, all
> sorts
> > of network configurations can eat from the MTU (vpn comes to mind).
> > If I understand correctly the working of libsrt, it "creates" a packet
> each
> > time we send it some data. So without touching on the "maximum" value,
> > h->max_packet_size insures that the "medium" value is effectively the
> > expected one.
> > The 1356 value, assuming no vpn/tunnel will spoil the party, will make ts
> > advance with 40 bytes each packet, so almost any packet loss will produce
> > ts that does not start with ts header. This doesn't seem good for packet
> > loss recovery.
> > Rtp encapsulated mpeg ts with pkt_size of 1316 will add an rtp header of
> ~
> > 12 bytes so the needed srt payloadsize will be 1328. As wireshark does
> not
> > recognize the protocol yet, I couldn't check in detail. I'm absolutely
> > certain that ffmpeg will do bad things if rtp header is misplaced (not
> > synchronized with a lost packet), it will end up happily reading random
> ts
> > data as rtp header at some point given enough packet loss.
>
> I am sorry for the consfusion, I meant 1316 and not 1356, and libsrt
> provides 1316 as default max payload size exactly for the reasons you
> described, and therefore that becomes the default max packet size. So all
> is fine in the code as far as I see, only my descriptive text was
> misleading, sorry.
>
> Thanks,
> Marton
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Marton Balint Sept. 1, 2018, 8:05 p.m. UTC | #4 
On Wed, 29 Aug 2018, Tudor Suciu wrote:

> Well, when this is done, working, we can begin to talk business:
> -add an option to ffmpeg to drop unused input data like srt-file-transmit
> (before first client connects)
> -add an option/document if it's already working to ffmpeg to have multiple
> srt clients like gstreamer

I pushed the patch which adds the pkt_size option and also fixes the 
error you were seeing. I cannot comment on the other things.

Regards,
Marton


>
> Regards,
>
> On Wed, Aug 29, 2018 at 10:48 AM Marton Balint <cus@passwd.hu> wrote:
>
>>
>>
>> On Wed, 29 Aug 2018, Tudor Suciu wrote:
>>
>> > Hello Marton,
>> >
>> > And should simply set SRTO_PACKETSIZE to our pkt_size parameter if we
>> >> don't want the libsrt default 1356.
>> >>
>> > At least for the specific case of mpegts I believe it's much better to
>> have
>> > fixed size packets(188x*).
>> > It helps error recovery if we don't have to re-synchronize ts so
>> > (7*188=1316) seems a better default.
>> > The 1356 value could be treated as a maximum special case value, all
>> sorts
>> > of network configurations can eat from the MTU (vpn comes to mind).
>> > If I understand correctly the working of libsrt, it "creates" a packet
>> each
>> > time we send it some data. So without touching on the "maximum" value,
>> > h->max_packet_size insures that the "medium" value is effectively the
>> > expected one.
>> > The 1356 value, assuming no vpn/tunnel will spoil the party, will make ts
>> > advance with 40 bytes each packet, so almost any packet loss will produce
>> > ts that does not start with ts header. This doesn't seem good for packet
>> > loss recovery.
>> > Rtp encapsulated mpeg ts with pkt_size of 1316 will add an rtp header of
>> ~
>> > 12 bytes so the needed srt payloadsize will be 1328. As wireshark does
>> not
>> > recognize the protocol yet, I couldn't check in detail. I'm absolutely
>> > certain that ffmpeg will do bad things if rtp header is misplaced (not
>> > synchronized with a lost packet), it will end up happily reading random
>> ts
>> > data as rtp header at some point given enough packet loss.
>>
>> I am sorry for the consfusion, I meant 1316 and not 1356, and libsrt
>> provides 1316 as default max payload size exactly for the reasons you
>> described, and therefore that becomes the default max packet size. So all
>> is fine in the code as far as I see, only my descriptive text was
>> misleading, sorry.
>>
>> Thanks,
>> Marton
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
diff mbox

Patch

From eac0d702fd91de1f62bb9c001ce83ce5f178993e Mon Sep 17 00:00:00 2001
From: Tudor Suciu <tudor.suciu@gmail.com>
Date: Sat, 25 Aug 2018 14:06:57 +0200
Subject: [PATCH] avformat/libsrt: add pkt_size parameter to libsrt

Also make sure we set the URL context max packet size accordingly.

Signed-off-by: Marton Balint <cus@passwd.hu>
---
 configure            |  2 +-
 doc/protocols.texi   |  6 ++++++
 libavformat/libsrt.c | 26 ++++++++++++++++++++++++++
 3 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/configure b/configure
index 3cf9412947..27a594f246 100755
--- a/configure
+++ b/configure
@@ -6112,7 +6112,7 @@  enabled libsnappy         && require libsnappy snappy-c.h snappy_compress -lsnap
 enabled libsoxr           && require libsoxr soxr.h soxr_create -lsoxr
 enabled libssh            && require_pkg_config libssh libssh libssh/sftp.h sftp_init
 enabled libspeex          && require_pkg_config libspeex speex speex/speex.h speex_decoder_init
-enabled libsrt            && require_pkg_config libsrt "srt >= 1.2.0" srt/srt.h srt_socket
+enabled libsrt            && require_pkg_config libsrt "srt >= 1.3.0" srt/srt.h srt_socket
 enabled libtensorflow     && require libtensorflow tensorflow/c/c_api.h TF_Version -ltensorflow
 enabled libtesseract      && require_pkg_config libtesseract tesseract tesseract/capi.h TessBaseAPICreate
 enabled libtheora         && require libtheora theora/theoraenc.h th_info_init -ltheoraenc -ltheoradec -logg
diff --git a/doc/protocols.texi b/doc/protocols.texi
index e9091e068c..4c5e972a04 100644
--- a/doc/protocols.texi
+++ b/doc/protocols.texi
@@ -1263,6 +1263,12 @@  Not required on receiver (set to 0),
 key size obtained from sender in HaiCrypt handshake.
 Default value is 0.
 
+@item pkt_size=@var{bytes}
+Set maximum SRT payload size, expressed in bytes. Default is -1 (automatic),
+which typically means 1316 as that is the libsrt default for live mode. Libsrt
+can also support payload sizes up to 1456 bytes. (MTU(1500) - UDP.hdr(28) -
+SRT.hdr(16))
+
 @item recv_buffer_size=@var{bytes}
 Set receive buffer size, expressed in bytes.
 
diff --git a/libavformat/libsrt.c b/libavformat/libsrt.c
index 0f9529d263..9260c1bf37 100644
--- a/libavformat/libsrt.c
+++ b/libavformat/libsrt.c
@@ -48,6 +48,7 @@  typedef struct SRTContext {
     int64_t listen_timeout;
     int recv_buffer_size;
     int send_buffer_size;
+    int pkt_size;
 
     int64_t maxbw;
     int pbkeylen;
@@ -73,6 +74,7 @@  static const AVOption libsrt_options[] = {
     { "listen_timeout", "Connection awaiting timeout",                                          OFFSET(listen_timeout),   AV_OPT_TYPE_INT64, { .i64 = -1 }, -1, INT64_MAX, .flags = D|E },
     { "send_buffer_size", "Socket send buffer size (in bytes)",                                 OFFSET(send_buffer_size), AV_OPT_TYPE_INT,      { .i64 = -1 }, -1, INT_MAX,   .flags = D|E },
     { "recv_buffer_size", "Socket receive buffer size (in bytes)",                              OFFSET(recv_buffer_size), AV_OPT_TYPE_INT,      { .i64 = -1 }, -1, INT_MAX,   .flags = D|E },
+    { "pkt_size",       "Maximum SRT packet size",                                              OFFSET(pkt_size),         AV_OPT_TYPE_INT,      { .i64 = -1 }, -1, INT_MAX,   .flags = D|E },
     { "maxbw",          "Maximum bandwidth (bytes per second) that the connection can use",     OFFSET(maxbw),            AV_OPT_TYPE_INT64,    { .i64 = -1 }, -1, INT64_MAX, .flags = D|E },
     { "pbkeylen",       "Crypto key len in bytes {16,24,32} Default: 16 (128-bit)",             OFFSET(pbkeylen),         AV_OPT_TYPE_INT,      { .i64 = -1 }, -1, 32,        .flags = D|E },
     { "passphrase",     "Crypto PBKDF2 Passphrase size[0,10..64] 0:disable crypto",             OFFSET(passphrase),       AV_OPT_TYPE_STRING,   { .str = NULL },              .flags = D|E },
@@ -240,6 +242,15 @@  static int libsrt_setsockopt(URLContext *h, int fd, SRT_SOCKOPT optname, const c
     return 0;
 }
 
+static int libsrt_getsockopt(URLContext *h, int fd, SRT_SOCKOPT optname, const char * optnamestr, void * optval, int * optlen)
+{
+    if (srt_getsockopt(fd, 0, optname, optval, optlen) < 0) {
+        av_log(h, AV_LOG_ERROR, "failed to get option %s on socket: %s\n", optnamestr, srt_getlasterror_str());
+        return AVERROR(EIO);
+    }
+    return 0;
+}
+
 /* - The "POST" options can be altered any time on a connected socket.
      They MAY have also some meaning when set prior to connecting; such
      option is SRTO_RCVSYN, which makes connect/accept call asynchronous.
@@ -276,6 +287,7 @@  static int libsrt_set_options_pre(URLContext *h, int fd)
         (tsbpddelay >= 0 && libsrt_setsockopt(h, fd, SRTO_TSBPDDELAY, "SRTO_TSBPDELAY", &tsbpddelay, sizeof(tsbpddelay)) < 0) ||
         (s->tlpktdrop >= 0 && libsrt_setsockopt(h, fd, SRTO_TLPKTDROP, "SRTO_TLPKDROP", &s->tlpktdrop, sizeof(s->tlpktdrop)) < 0) ||
         (s->nakreport >= 0 && libsrt_setsockopt(h, fd, SRTO_NAKREPORT, "SRTO_NAKREPORT", &s->nakreport, sizeof(s->nakreport)) < 0) ||
+        (s->pkt_size >= 0 && libsrt_setsockopt(h, fd, SRTO_PAYLOADSIZE, "SRTO_PAYLOADSIZE", &s->pkt_size, sizeof(s->pkt_size)) < 0) ||
         (connect_timeout >= 0 && libsrt_setsockopt(h, fd, SRTO_CONNTIMEO, "SRTO_CONNTIMEO", &connect_timeout, sizeof(connect_timeout)) <0 )) {
         return AVERROR(EIO);
     }
@@ -384,6 +396,17 @@  static int libsrt_setup(URLContext *h, const char *uri, int flags)
     s->fd = fd;
 
     freeaddrinfo(ai);
+
+    if (flags & AVIO_FLAG_WRITE) {
+        int packet_size = 0;
+        int optlen = sizeof(packet_size);
+        ret = libsrt_getsockopt(h, fd, SRTO_PAYLOADSIZE, "SRTO_PAYLOADSIZE", &packet_size, &optlen);
+        if (ret < 0)
+            goto fail;
+        if (packet_size > 0)
+            h->max_packet_size = packet_size;
+    }
+
     return 0;
 
  fail:
@@ -442,6 +465,9 @@  static int libsrt_open(URLContext *h, const char *uri, int flags)
         if (av_find_info_tag(buf, sizeof(buf), "oheadbw", p)) {
             s->oheadbw = strtoll(buf, NULL, 10);
         }
+        if (av_find_info_tag(buf, sizeof(buf), "pkt_size", p)) {
+            s->pkt_size = strtol(buf, NULL, 10);
+        }
         if (av_find_info_tag(buf, sizeof(buf), "tsbpddelay", p)) {
             s->tsbpddelay = strtol(buf, NULL, 10);
         }
-- 
2.16.4