On Wed, Dec 20, 2017 at 11:36 AM, Aman Gupta <ffm...@tmm1.net> wrote:
> > > On Sun, Dec 17, 2017 at 1:13 PM, Anssi Hannula <anssi.hann...@iki.fi> > wrote: > >> Aman Gupta kirjoitti 2017-12-17 22:41: >> >>> On Sun, Dec 17, 2017 at 12:34 PM Anssi Hannula <anssi.hann...@iki.fi> >>> wrote: >>> >>> Hi! >>>> >>>> Aman Gupta kirjoitti 2017-12-13 02:35: >>>> > From: Aman Gupta <a...@tmm1.net> >>>> > >>>> > This teaches the HLS demuxer to use the HTTP protocols >>>> > multiple_requests=1 option, to take advantage of "Connection: >>>> > Keep-Alive" when downloading playlists and segments from the HLS >>>> > server. >>>> > >>>> > With the new option, you can avoid TCP connection and TLS negotiation >>>> > overhead, which is particularly beneficial when streaming via a >>>> > high-latency internet connection. >>>> > >>>> > Similar to the http_persistent option recently implemented in hlsenc.c >>>> >>>> Why is this implemented as an option instead of simply being always used >>>> by the demuxer? >>>> >>> >>> >>> I have no strong feeling on this either way. I've tested the new option >>> against a few different HLS servers and would be comfortable enabling it >>> by >>> default. I do think it's worth keeping as an option in case someone wants >>> to turn it off for whatever reason. >>> >> >> OK. The only other demuxer that reuses HTTP connections seems to be >> rtmphttp, and it does not have an option. >> I think I'd prefer no option here as well unless a use case is known, but >> I'm not too strongly against it so I guess it could stay (but default to >> true). >> >> Does anyone else have any thoughts? >> > > If no one objects, I will push this patchset with the requested changed in > a few days. > Patchset applied. I ran some more tests before pushing to master, with: `-i https://bitdash-a.akamaihd.net/content/sintel/hls/video/4000kbit.m3u8 -t 30 -f null /dev/null` -http_persistent 0 -http_multiple 0 frame= 722 fps=296 q=-1.0 Lsize=N/A time=00:00:29.95 bitrate=N/A speed=12.3x frame= 722 fps=324 q=-1.0 Lsize=N/A time=00:00:29.95 bitrate=N/A speed=13.4x -http_persistent 0 -http_multiple 1 frame= 722 fps=304 q=-1.0 Lsize=N/A time=00:00:29.95 bitrate=N/A speed=12.6x frame= 722 fps=325 q=-1.0 Lsize=N/A time=00:00:29.95 bitrate=N/A speed=13.5x -http_persistent 1 -http_multiple 0 frame= 722 fps=682 q=-1.0 Lsize=N/A time=00:00:29.95 bitrate=N/A speed=28.3x frame= 722 fps=713 q=-1.0 Lsize=N/A time=00:00:29.95 bitrate=N/A speed=29.6x -http_persistent 1 -http_multiple 1 frame= 722 fps=0.0 q=-1.0 Lsize=N/A time=00:00:29.95 bitrate=N/A speed=39.4x frame= 722 fps=0.0 q=-1.0 Lsize=N/A time=00:00:29.95 bitrate=N/A speed=40.6x In this case, the new default settings achieve a 3x throughput increase as compared to the previous defaults. Aman > > Aman > > >> >> >> Also, what happens if the hostname in the URI varies, does it properly >> open a new connection then? >> >> >>> >>>> > --- >>>> > doc/demuxers.texi | 3 +++ >>>> > libavformat/hls.c | 68 >>>> > +++++++++++++++++++++++++++++++++++++++++++++++++++---- >>>> > 2 files changed, 67 insertions(+), 4 deletions(-) >>>> > >>>> > diff --git a/doc/demuxers.texi b/doc/demuxers.texi >>>> > index 73dc0feec1..18ff7101ac 100644 >>>> > --- a/doc/demuxers.texi >>>> > +++ b/doc/demuxers.texi >>>> > @@ -316,6 +316,9 @@ segment index to start live streams at (negative >>>> > values are from the end). >>>> > @item max_reload >>>> > Maximum number of times a insufficient list is attempted to be >>>> > reloaded. >>>> > Default value is 1000. >>>> > + >>>> > +@item http_persistent >>>> > +Use persistent HTTP connections. Applicable only for HTTP streams. >>>> > @end table >>>> > >>>> > @section image2 >>>> > diff --git a/libavformat/hls.c b/libavformat/hls.c >>>> > index ab6ff187a6..5c1895c180 100644 >>>> > --- a/libavformat/hls.c >>>> > +++ b/libavformat/hls.c >>>> > @@ -26,6 +26,7 @@ >>>> > * http://tools.ietf.org/html/draft-pantos-http-live-streaming >>>> > */ >>>> > >>>> > +#include "libavformat/http.h" >>>> > #include "libavutil/avstring.h" >>>> > #include "libavutil/avassert.h" >>>> > #include "libavutil/intreadwrite.h" >>>> > @@ -94,6 +95,7 @@ struct playlist { >>>> > AVIOContext pb; >>>> > uint8_t* read_buffer; >>>> > AVIOContext *input; >>>> > + int input_read_done; >>>> > AVFormatContext *parent; >>>> > int index; >>>> > AVFormatContext *ctx; >>>> > @@ -206,6 +208,8 @@ typedef struct HLSContext { >>>> > int strict_std_compliance; >>>> > char *allowed_extensions; >>>> > int max_reload; >>>> > + int http_persistent; >>>> > + AVIOContext *playlist_pb; >>>> > } HLSContext; >>>> > >>>> > static int read_chomp_line(AVIOContext *s, char *buf, int maxlen) >>>> > @@ -256,6 +260,7 @@ static void free_playlist_list(HLSContext *c) >>>> > av_freep(&pls->pb.buffer); >>>> > if (pls->input) >>>> > ff_format_io_close(c->ctx, &pls->input); >>>> > + pls->input_read_done = 0; >>>> > if (pls->ctx) { >>>> > pls->ctx->pb = NULL; >>>> > avformat_close_input(&pls->ctx); >>>> > @@ -640,7 +645,24 @@ static int open_url(AVFormatContext *s, >>>> > AVIOContext **pb, const char *url, >>>> > else if (strcmp(proto_name, "file") || !strncmp(url, "file,", 5)) >>>> > return AVERROR_INVALIDDATA; >>>> > >>>> > - ret = s->io_open(s, pb, url, AVIO_FLAG_READ, &tmp); >>>> > + if (c->http_persistent && *pb && av_strstart(proto_name, "http", >>>> > NULL)) { >>>> > + URLContext *uc = ffio_geturlcontext(*pb); >>>> > + av_assert0(uc); >>>> > + (*pb)->eof_reached = 0; >>>> > + ret = ff_http_do_new_request(uc, url); >>>> > + if (ret == AVERROR_EXIT) { >>>> > + ff_format_io_close(c->ctx, &c->playlist_pb); >>>> > + return ret; >>>> > + } else if (ret < 0) { >>>> > + av_log(s, AV_LOG_WARNING, >>>> > + "keepalive request failed for '%s', retrying with new >>>> > connection: %s\n", >>>> > + url, av_err2str(ret)); >>>> > + ff_format_io_close(c->ctx, pb); >>>> > + ret = s->io_open(s, pb, url, AVIO_FLAG_READ, &tmp); >>>> > + } >>>> > + } else { >>>> > + ret = s->io_open(s, pb, url, AVIO_FLAG_READ, &tmp); >>>> > + } >>>> > if (ret >= 0) { >>>> > // update cookies on http response with setcookies. >>>> > char *new_cookies = NULL; >>>> > @@ -683,10 +705,27 @@ static int parse_playlist(HLSContext *c, const >>>> > char *url, >>>> > char tmp_str[MAX_URL_SIZE]; >>>> > struct segment *cur_init_section = NULL; >>>> > >>>> > + if (!in && c->http_persistent && c->playlist_pb) { >>>> > + in = c->playlist_pb; >>>> > + URLContext *uc = ffio_geturlcontext(in); >>>> > + av_assert0(uc); >>>> > + in->eof_reached = 0; >>>> > + ret = ff_http_do_new_request(uc, url); >>>> > + if (ret == AVERROR_EXIT) { >>>> > + ff_format_io_close(c->ctx, &c->playlist_pb); >>>> > + return ret; >>>> > + } else if (ret < 0) { >>>> > + av_log(c->ctx, AV_LOG_WARNING, >>>> > + "keepalive request failed for '%s', retrying with new >>>> > connection: %s\n", >>>> > + url, av_err2str(ret)); >>>> > + ff_format_io_close(c->ctx, &c->playlist_pb); >>>> > + in = NULL; >>>> > + } >>>> > + } >>>> > + >>>> >>>> The above code seems duplicated, please put it in a common function. >>>> >>> >>> >>> Sure, will do. >>> >>> Thanks for reviewing! >>> >>> >>> >>>> >>>> [..] >>>> -- >>>> Anssi Hannula >>>> >>>> _______________________________________________ >>> ffmpeg-devel mailing list >>> ffmpeg-devel@ffmpeg.org >>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >>> >> >> -- >> Anssi Hannula >> _______________________________________________ >> 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