[FFmpeg-devel,v3,2/2] avformat/{http, tls}: enable tcp_nodelay

Submitted by Aman Gupta on Nov. 15, 2017, 5:01 a.m.

Details

Message ID 20171115050134.4385-2-ffmpeg@tmm1.net
State New
Headers show

Commit Message

Aman Gupta Nov. 15, 2017, 5:01 a.m.
From: Aman Gupta <aman@tmm1.net>

Signed-off-by: Aman Gupta <aman@tmm1.net>
---
 libavformat/http.c | 1 +
 libavformat/tls.c  | 1 +
 2 files changed, 2 insertions(+)

Comments

kjeyapal@akamai.com Nov. 15, 2017, 5:28 a.m.
>On 11/15/17, 10:32 AM, "Aman Gupta" <ffmpeg@tmm1.net> wrote:

>

>From: Aman Gupta <aman@tmm1.net>

>

>Signed-off-by: Aman Gupta <aman@tmm1.net>

>---

> libavformat/http.c | 1 +

> libavformat/tls.c  | 1 +

> 2 files changed, 2 insertions(+)


Again, do want to hardcode tcp_nodelay at http and tls level? Because there could be other muxers 
that send data in very small chunks where setting this option would actually increase the tcp overhead
severely for such use-cases.

I thought it would be better if the http plugin exposed this as an option and hls or dash muxers enable it, 
after we make sure that the relevant TS and MOV muxers used in hls and dash plugins writes data as 
big chunks(atleast one frame at a time or some kind of size limit). I think movenc already handles it with
dynbuf avio and pushes it only at the end of the segment. But same can’t be said about the mpd file or
m3u8 file posts. I think we need to examine the tcpdump logs(or wireshark) to make sure that the number
of tcp packets doesn’t increase unexpectedly after this change. 

I am only saying this from muxer point of view. Similarly, we need think from demuxer point of view and
make sure that enabling this doesn’t add any unnecessary tcp overhead there as well.

My suggestion is to add tcp_nodelay as an option for http and tls and push this change.
Later we can modify the hls and dash plugins separately to enable this optimization.

Thanks and regards,
Karthick
Aman Gupta Nov. 15, 2017, 5:34 a.m.
On Tue, Nov 14, 2017 at 9:28 PM, Jeyapal, Karthick <kjeyapal@akamai.com>
wrote:

>
> >On 11/15/17, 10:32 AM, "Aman Gupta" <ffmpeg@tmm1.net> wrote:
> >
> >From: Aman Gupta <aman@tmm1.net>
> >
> >Signed-off-by: Aman Gupta <aman@tmm1.net>
> >---
> > libavformat/http.c | 1 +
> > libavformat/tls.c  | 1 +
> > 2 files changed, 2 insertions(+)
>
> Again, do want to hardcode tcp_nodelay at http and tls level? Because
> there could be other muxers
> that send data in very small chunks where setting this option would
> actually increase the tcp overhead
> severely for such use-cases.
>
> I thought it would be better if the http plugin exposed this as an option
> and hls or dash muxers enable it,
> after we make sure that the relevant TS and MOV muxers used in hls and
> dash plugins writes data as
> big chunks(atleast one frame at a time or some kind of size limit). I
> think movenc already handles it with
> dynbuf avio and pushes it only at the end of the segment. But same can’t
> be said about the mpd file or
> m3u8 file posts. I think we need to examine the tcpdump logs(or wireshark)
> to make sure that the number
> of tcp packets doesn’t increase unexpectedly after this change.
>
> I am only saying this from muxer point of view. Similarly, we need think
> from demuxer point of view and
> make sure that enabling this doesn’t add any unnecessary tcp overhead
> there as well.
>
> My suggestion is to add tcp_nodelay as an option for http and tls and push
> this change.
> Later we can modify the hls and dash plugins separately to enable this
> optimization.
>

Hmm, I guess if we want this to propagate down we need to add the option to
the tls protocol as well, then pass it down when it instantiates the tcp
protocol. Then add the same option to hls/dash, and again propagate it down
to tls or tcp.

This is turning into a much bigger change than I anticipated, and I don't
really care enough about it to continue. If someone else wants to take over
this patchset, they are more than welcome to it.

Aman


>
> Thanks and regards,
> Karthick
>
>
kjeyapal@akamai.com Nov. 15, 2017, 10:35 a.m.
>Hmm, I guess if we want this to propagate down we need to add the option to the tls protocol as well, then pass it down when it instantiates the tcp protocol. Then add the same option to hls/dash, and again propagate it >down to tls or tcp.

>

>This is turning into a much bigger change than I anticipated, and I don't really care enough about it to continue. If someone else wants to take over this patchset, they are more than welcome to it.

>

>Aman


Well, I would suggest you could push the first tcp patch and leave the http/tls patch for somebody else to take it up later.

Regards,
Karthick
Aman Gupta Nov. 16, 2017, 2:21 a.m.
On Wed, Nov 15, 2017 at 2:35 AM, Jeyapal, Karthick <kjeyapal@akamai.com>
wrote:

> >Hmm, I guess if we want this to propagate down we need to add the option
> to the tls protocol as well, then pass it down when it instantiates the tcp
> protocol. Then add the same option to hls/dash, and again propagate it
> >down to tls or tcp.
>
> >
>
> >This is turning into a much bigger change than I anticipated, and I don't
> really care enough about it to continue. If someone else wants to take over
> this patchset, they are more than welcome to it.
>
> >
>
> >Aman
>
>
>
> Well, I would suggest you could push the first tcp patch and leave the
> http/tls patch for somebody else to take it up later.
>

That's fair.

I've tested compiling the change on windows (w/ mingw), freebsd, linux and
macOS. I hope there are no other obscure supported platforms where
TCP_NODELAY might not be defined?

Aman


>
>
> Regards,
>
> Karthick
>
Aman Gupta Nov. 17, 2017, 6:50 p.m.
On Wed, Nov 15, 2017 at 6:21 PM, Aman Gupta <aman@tmm1.net> wrote:

>
>
> On Wed, Nov 15, 2017 at 2:35 AM, Jeyapal, Karthick <kjeyapal@akamai.com>
> wrote:
>
>> >Hmm, I guess if we want this to propagate down we need to add the option
>> to the tls protocol as well, then pass it down when it instantiates the tcp
>> protocol. Then add the same option to hls/dash, and again propagate it
>> >down to tls or tcp.
>>
>> >
>>
>> >This is turning into a much bigger change than I anticipated, and I
>> don't really care enough about it to continue. If someone else wants to
>> take over this patchset, they are more than welcome to it.
>>
>> >
>>
>> >Aman
>>
>>
>>
>> Well, I would suggest you could push the first tcp patch and leave the
>> http/tls patch for somebody else to take it up later.
>>
>
> That's fair.
>
> I've tested compiling the change on windows (w/ mingw), freebsd, linux and
> macOS. I hope there are no other obscure supported platforms where
> TCP_NODELAY might not be defined?
>

I've pushed the change to add the tcp_nodelay option to the tcp protocol.
The second patch to use it from http/tls has been abandoned.

Aman


>
> Aman
>
>
>>
>>
>> Regards,
>>
>> Karthick
>>
>
>

Patch hide | download patch | download mbox

diff --git a/libavformat/http.c b/libavformat/http.c
index 056d5f6583..f0a80b7add 100644
--- a/libavformat/http.c
+++ b/libavformat/http.c
@@ -468,6 +468,7 @@  static int http_listen(URLContext *h, const char *uri, int flags,
                 NULL);
     if ((ret = av_dict_set_int(options, "listen", s->listen, 0)) < 0)
         goto fail;
+    av_dict_set_int(options, "tcp_nodelay", 1, 0);
     if ((ret = ffurl_open_whitelist(&s->hd, lower_url, AVIO_FLAG_READ_WRITE,
                                     &h->interrupt_callback, options,
                                     h->protocol_whitelist, h->protocol_blacklist, h
diff --git a/libavformat/tls.c b/libavformat/tls.c
index 10e0792e29..a56f51f85b 100644
--- a/libavformat/tls.c
+++ b/libavformat/tls.c
@@ -104,6 +104,7 @@  int ff_tls_open_underlying(TLSShared *c, URLContext *parent, const char *uri, AV
                     proxy_port, "/%s", dest);
     }
 
+    av_dict_set_int(options, "tcp_nodelay", 1, 0);
     return ffurl_open_whitelist(&c->tcp, buf, AVIO_FLAG_READ_WRITE,
                                 &parent->interrupt_callback, options,
                                 parent->protocol_whitelist, parent->protocol_blacklist, parent);