Message ID | 4813bcaf-cf3a-ef22-7f52-13feaea41280@mail.de |
---|---|
State | Accepted |
Headers | show |
Series | [FFmpeg-devel] lavf/rtmp: Add option to set TCP_NODELAY for rtmp | expand |
Context | Check | Description |
---|---|---|
andriy/x86_make | success | Make finished |
andriy/x86_make_fate | success | Make fate finished |
andriy/PPC64_make | success | Make finished |
andriy/PPC64_make_fate | success | Make fate finished |
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
diff --git a/doc/protocols.texi b/doc/protocols.texi index 8371f83..3b1cf12 100644 --- a/doc/protocols.texi +++ b/doc/protocols.texi @@ -843,6 +843,9 @@ 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. + @end table For example to read with @command{ffplay} a multimedia resource named 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 =
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(-)