[FFmpeg-devel] lavf/rtmp: Add option to set TCP_NODELAY for rtmp

Hi,

$subject seems to be useful depending on the network.

-Thilo
From 936826eec00ac2ceaa4579e8a6030a85191320a5 Mon Sep 17 00:00:00 2001
From: Nick Ruff <nickruff@devvm32779.prn1.facebook.com>
Date: Wed, 9 Jun 2021 12:26:03 +0200
Subject: [PATCH] lavf/rtmp: Add option to set TCP_NODELAY for rtmp

Suggested-By: ffmpeg@fb.com
---
 doc/protocols.texi      | 3 +++
 libavformat/rtmpproto.c | 8 +++++---
 libavformat/tcp.c       | 3 +++
 3 files changed, 11 insertions(+), 3 deletions(-)

Thilo Borgman (12021-06-09):
> $subject seems to be useful depending on the network.

It looks premature. TCP_NODELAY should only ever used when the network
code already takes care of writing data in a single system call. This is
not what happens in ff_rtmp_packet_write() (libavformat/rtmppkt.c): the
header is written separately.

Before allowing this option, this piece of code needs to be rewritten to
reduce system calls.

(TCP_NODELAY is a terrible hack for a terrible API design. An explicit
flush system call would have been a much better choice.)

Regards,

On Wed, Jun 09, 2021 at 12:41:17PM +0200, Nicolas George wrote:
> (TCP_NODELAY is a terrible hack for a terrible API design. An explicit
> flush system call would have been a much better choice.)

If you want explicit flush, you can enable TCP_CORK (but it's Linux-only).
Disable for the flush.

/* Steinar */

Am 09.06.21 um 12:45 schrieb Steinar H. Gunderson:
> On Wed, Jun 09, 2021 at 12:41:17PM +0200, Nicolas George wrote:
>> (TCP_NODELAY is a terrible hack for a terrible API design. An explicit
>> flush system call would have been a much better choice.)
> 
> If you want explicit flush, you can enable TCP_CORK (but it's Linux-only).
> Disable for the flush.

Platform dependency sounds even worse. I'm looking into reducing system calls.

Thanks,
Thilo

Steinar H. Gunderson (12021-06-09):
> If you want explicit flush, you can enable TCP_CORK (but it's Linux-only).
> Disable for the flush.

Interesting thanks. I wonder why they did it like this, requiring two
extra system calls for each flush; a flag on send would have served
without extra system calls.

But as Thilo pointed, we cannot accept something so non-portable.
Optimizing the writing code is the way to go.

Regards,

Nicolas George (12021-06-09):
> Interesting thanks. I wonder why they did it like this, requiring two
> extra system calls for each flush; a flag on send would have served
> without extra system calls.

Ah, I see they have MSG_MORE, introduced at the same time, that does
just that. If it was portable, that would be exactly what we need.

Regards,

Hi,

Am 09.06.21 um 12:41 schrieb Nicolas George:
> Thilo Borgman (12021-06-09):
>> $subject seems to be useful depending on the network.
> 
> It looks premature. TCP_NODELAY should only ever used when the network
> code already takes care of writing data in a single system call. This is
> not what happens in ff_rtmp_packet_write() (libavformat/rtmppkt.c): the
> header is written separately.
> 
> Before allowing this option, this piece of code needs to be rewritten to
> reduce system calls.

I'm sorry for a repeated mistake... I remember having asked s.o. in the past to give me feedback about a patch off-list and had been told to ask on the list for everyone to benefit from an elaborate answer. Same thing happened again - now I want it on the list myself as it iterates the discussion. Since Nicolas is sharing his thought here anyway, I dare not to ask him before quoting:


> Thilo Borgman (12021-06-09):
>> thanks for looking at it!
>> Ok let's try, I'm looking at libavformat/rtmppkt.c:377 ff.
>>
>> If I understand you correctly, we might generate the whole output in an
>> extra buffer and then calling ffurl_write() only once on that buffer. Not?
> 
> The ideal solution would be to be able to provide several buffers all at
> once, like writev(). But we do not have this, so buffering would be the
> next best thing.
> 
>> Before I get into implementing this and blindly send it, I wonder if it is
>> worth to have 3 sys calls less for the need to have a new buffer & copy over
>> from header & pkt..?
>>
>> Maybe I just missed something?
> 
> In terms of CPU use, it would bear benchmarking, but it is usually
> considered that an extra buffer copy is better than an extra system
> call. So that is already that.
> 
> But in this case, we are not concerned about CPU use but about network
> overhead and latency. The sockets API flushes after all system calls, we
> can do nothing about it (portably).
> 
> But now that I think on it some more, I realize it would not be
> reasonable to ask for this change. It would be isolated and
> inconsistent: the same issue certainly happens for many network
> protocols.
> 
> What we need is infrastructure to do that properly. Since I have started
> really thinking on making avio callbacks-based, it should give us an
> infrastructure.
> 
> So, for now, I would say: no objection to the option, but please add a
> comment in the code and in the documentation that it is not yet optimal.

Will do before push, thank you!

-Thilo

Hi,

> $subject seems to be useful depending on the network.

v2 attached with updates to documentation and code.
Please check if I missed the point of it.

Thanks!
-Thilo
From 79f1589ba5d9224146d71c2605ca4cc02c852cb2 Mon Sep 17 00:00:00 2001
From: Nick Ruff <nickruff@devvm32779.prn1.facebook.com>
Date: Fri, 11 Jun 2021 21:02:02 +0200
Subject: [PATCH 1/2] lavf/rtmp: Add option to set TCP_NODELAY for rtmp

Suggested-By: ffmpeg@fb.com
---
 doc/protocols.texi      | 5 +++++
 libavformat/rtmppkt.c   | 6 ++++++
 libavformat/rtmpproto.c | 8 +++++---
 libavformat/tcp.c       | 3 +++
 4 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/doc/protocols.texi b/doc/protocols.texi
index d3095c8..6b5a9cf 100644
--- a/doc/protocols.texi
+++ b/doc/protocols.texi
@@ -843,6 +843,11 @@ URL to player swf file, compute hash/size automatically.
 @item rtmp_tcurl
 URL of the target stream. Defaults to proto://host[:port]/app.
 
+@item tcp_nodelay=@var{1|0}
+Set TCP_NODELAY to disable Nagle's algorithm. Default value is 0.
+
+@emph{Remark: Writing to the socket is currently not optimized to minimize system calls and reduces the efficiency / effect of TCP_NODELAY.}
+
 @end table
 
 For example to read with @command{ffplay} a multimedia resource named
diff --git a/libavformat/rtmppkt.c b/libavformat/rtmppkt.c
index 00eb087..4b97c08 100644
--- a/libavformat/rtmppkt.c
+++ b/libavformat/rtmppkt.c
@@ -374,6 +374,12 @@ int ff_rtmp_packet_write(URLContext *h, RTMPPacket *pkt,
     prev_pkt[pkt->channel_id].ts_field   = pkt->ts_field;
     prev_pkt[pkt->channel_id].extra      = pkt->extra;
 
+    // FIXME:
+    // Writing packets is currently not optimized to minimize system calls.
+    // Since system calls flush on exit which we cannot change in a system-independant way.
+    // We should fix this behavior and by writing packets in a single or in as few as possible system calls.
+    // Protocols like TCP and RTMP should benefit from this when enabling TCP_NODELAY.
+
     if ((ret = ffurl_write(h, pkt_hdr, p - pkt_hdr)) < 0)
         return ret;
     written = p - pkt_hdr + pkt->size;
diff --git a/libavformat/rtmpproto.c b/libavformat/rtmpproto.c
index 5a540e3..b14d23b 100644
--- a/libavformat/rtmpproto.c
+++ b/libavformat/rtmpproto.c
@@ -123,6 +123,7 @@ typedef struct RTMPContext {
     int           listen_timeout;             ///< listen timeout to wait for new connections
     int           nb_streamid;                ///< The next stream id to return on createStream calls
     double        duration;                   ///< Duration of the stream in seconds as returned by the server (only valid if non-zero)
+    int           tcp_nodelay;                ///< Use TCP_NODELAY to disable Nagle's algorithm if set to 1
     char          username[50];
     char          password[50];
     char          auth_params[500];
@@ -2653,10 +2654,10 @@ static int rtmp_open(URLContext *s, const char *uri, int flags, AVDictionary **o
             port = RTMP_DEFAULT_PORT;
         if (rt->listen)
             ff_url_join(buf, sizeof(buf), "tcp", NULL, hostname, port,
-                        "?listen&listen_timeout=%d",
-                        rt->listen_timeout * 1000);
+                        "?listen&listen_timeout=%d&tcp_nodelay=%d",
+                        rt->listen_timeout * 1000, rt->tcp_nodelay);
         else
-            ff_url_join(buf, sizeof(buf), "tcp", NULL, hostname, port, NULL);
+            ff_url_join(buf, sizeof(buf), "tcp", NULL, hostname, port, "?tcp_nodelay=%d", rt->tcp_nodelay);
     }
 
 reconnect:
@@ -3115,6 +3116,7 @@ static const AVOption rtmp_options[] = {
     {"rtmp_tcurl", "URL of the target stream. Defaults to proto://host[:port]/app.", OFFSET(tcurl), AV_OPT_TYPE_STRING, {.str = NULL }, 0, 0, DEC|ENC},
     {"rtmp_listen", "Listen for incoming rtmp connections", OFFSET(listen), AV_OPT_TYPE_INT, {.i64 = 0}, INT_MIN, INT_MAX, DEC, "rtmp_listen" },
     {"listen",      "Listen for incoming rtmp connections", OFFSET(listen), AV_OPT_TYPE_INT, {.i64 = 0}, INT_MIN, INT_MAX, DEC, "rtmp_listen" },
+    {"tcp_nodelay", "Use TCP_NODELAY to disable Nagle's algorithm", OFFSET(tcp_nodelay), AV_OPT_TYPE_INT, {.i64 = 0}, 0, 1, DEC|ENC},
     {"timeout", "Maximum timeout (in seconds) to wait for incoming connections. -1 is infinite. Implies -rtmp_listen 1",  OFFSET(listen_timeout), AV_OPT_TYPE_INT, {.i64 = -1}, INT_MIN, INT_MAX, DEC, "rtmp_listen" },
     { NULL },
 };
diff --git a/libavformat/tcp.c b/libavformat/tcp.c
index 2198e0f..1c19aed 100644
--- a/libavformat/tcp.c
+++ b/libavformat/tcp.c
@@ -135,6 +135,9 @@ static int tcp_open(URLContext *h, const char *uri, int flags)
         if (av_find_info_tag(buf, sizeof(buf), "listen_timeout", p)) {
             s->listen_timeout = strtol(buf, NULL, 10);
         }
+        if (av_find_info_tag(buf, sizeof(buf), "tcp_nodelay", p)) {
+            s->tcp_nodelay = strtol(buf, NULL, 10);
+        }
     }
     if (s->rw_timeout >= 0) {
         s->open_timeout =

Am 11.06.21 um 21:07 schrieb Thilo Borgmann:
> Hi,
> 
>> $subject seems to be useful depending on the network.
> 
> v2 attached with updates to documentation and code.
> Please check if I missed the point of it.

Follow-up to add the remark also to TCP documentation.

-Thilo
From 69e6fb2acab42811c442eddafd86a82d00ee4abb Mon Sep 17 00:00:00 2001
From: Thilo Borgmann <thilo.borgmann@mail.de>
Date: Fri, 11 Jun 2021 21:03:20 +0200
Subject: [PATCH 2/2] doc/protocols: Add remark about TCP_NODELAY to
 documentation of TCP

---
 doc/protocols.texi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/doc/protocols.texi b/doc/protocols.texi
index 6b5a9cf..ccdfb6e 100644
--- a/doc/protocols.texi
+++ b/doc/protocols.texi
@@ -1728,6 +1728,8 @@ Set send buffer size, expressed bytes.
 @item tcp_nodelay=@var{1|0}
 Set TCP_NODELAY to disable Nagle's algorithm. Default value is 0.
 
+@emph{Remark: Writing to the socket is currently not optimized to minimize system calls and reduces the efficiency / effect of TCP_NODELAY.}
+
 @item tcp_mss=@var{bytes}
 Set maximum segment size for outgoing TCP packets, expressed in bytes.
 @end table

Thilo Borgman (12021-06-11):
> Am 11.06.21 um 21:07 schrieb Thilo Borgmann:
> > Hi,
> > 
> > > $subject seems to be useful depending on the network.
> > 
> > v2 attached with updates to documentation and code.
> > Please check if I missed the point of it.
> 
> Follow-up to add the remark also to TCP documentation.

I have no objection to this version. This is not my area of code to
maintain.

Thanks.

Regards,

Am 17.06.21 um 11:53 schrieb Nicolas George:
> Thilo Borgman (12021-06-11):
>> Am 11.06.21 um 21:07 schrieb Thilo Borgmann:
>>> Hi,
>>>
>>>> $subject seems to be useful depending on the network.
>>>
>>> v2 attached with updates to documentation and code.
>>> Please check if I missed the point of it.
>>
>> Follow-up to add the remark also to TCP documentation.
> 
> I have no objection to this version. This is not my area of code to
> maintain.
> 
> Thanks.

Pushed, thanks!

-Thilo