| Message ID | 20171213003511.25342-4-ffmpeg@tmm1.net |
|---|---|
| State | New |
| Headers | show |
Hi! Aman Gupta kirjoitti 2017-12-13 02:35: > From: Aman Gupta <aman@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? > --- > 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. [..]
On Sun, Dec 17, 2017 at 12:34 PM Anssi Hannula <anssi.hannula@iki.fi> wrote: > Hi! > > Aman Gupta kirjoitti 2017-12-13 02:35: > > From: Aman Gupta <aman@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. > > > --- > > 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 >
Aman Gupta kirjoitti 2017-12-17 22:41: > On Sun, Dec 17, 2017 at 12:34 PM Anssi Hannula <anssi.hannula@iki.fi> > wrote: > >> Hi! >> >> Aman Gupta kirjoitti 2017-12-13 02:35: >> > From: Aman Gupta <aman@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? 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
On Sun, Dec 17, 2017 at 1:21 PM Anssi Hannula <anssi.hannula@iki.fi> wrote: > Aman Gupta kirjoitti 2017-12-17 22:41: > > On Sun, Dec 17, 2017 at 12:34 PM Anssi Hannula <anssi.hannula@iki.fi> > > wrote: > > > >> Hi! > >> > >> Aman Gupta kirjoitti 2017-12-13 02:35: > >> > From: Aman Gupta <aman@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? > > > Also, what happens if the hostname in the URI varies, does it properly > open a new connection then? Yes, ff_http_do_new_request will return EINVAL because of an earlier patch in the series, and so the demuxer will print "keepalive request failed" and fallback to a new connection. I also tested with an http server configured to allow only N keepalive requests on a socket. An ECONNRESET is similarly caught and printed, and then a new connection is made. There might be other cases or specific webserver/proxy behaviors not correctly accounted for, which I why I think the option should be left in place for now in case anyone need to force http/1.0 compatible behavior. Aman > > > > >> > >> > --- > >> > 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 >
On Sun, Dec 17, 2017 at 1:13 PM, Anssi Hannula <anssi.hannula@iki.fi> wrote: > Aman Gupta kirjoitti 2017-12-17 22:41: > >> On Sun, Dec 17, 2017 at 12:34 PM Anssi Hannula <anssi.hannula@iki.fi> >> wrote: >> >> Hi! >>> >>> Aman Gupta kirjoitti 2017-12-13 02:35: >>> > From: Aman Gupta <aman@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. 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 >
On Wed, Dec 20, 2017 at 11:36 AM, Aman Gupta <ffmpeg@tmm1.net> wrote: > > > On Sun, Dec 17, 2017 at 1:13 PM, Anssi Hannula <anssi.hannula@iki.fi> > wrote: > >> Aman Gupta kirjoitti 2017-12-17 22:41: >> >>> On Sun, Dec 17, 2017 at 12:34 PM Anssi Hannula <anssi.hannula@iki.fi> >>> wrote: >>> >>> Hi! >>>> >>>> Aman Gupta kirjoitti 2017-12-13 02:35: >>>> > From: Aman Gupta <aman@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 >> > >
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; + } + } + if (!in) { #if 1 AVDictionary *opts = NULL; - close_in = 1; /* Some HLS servers don't like being sent the range header */ av_dict_set(&opts, "seekable", "0", 0); @@ -696,10 +735,18 @@ static int parse_playlist(HLSContext *c, const char *url, av_dict_set(&opts, "headers", c->headers, 0); av_dict_set(&opts, "http_proxy", c->http_proxy, 0); + if (c->http_persistent) + av_dict_set(&opts, "multiple_requests", "1", 0); + ret = c->ctx->io_open(c->ctx, &in, url, AVIO_FLAG_READ, &opts); av_dict_free(&opts); if (ret < 0) return ret; + + if (c->http_persistent) + c->playlist_pb = in; + else + close_in = 1; #else ret = open_in(c, &in, url); if (ret < 0) @@ -1111,6 +1158,9 @@ static int open_input(HLSContext *c, struct playlist *pls, struct segment *seg) av_dict_set(&opts, "http_proxy", c->http_proxy, 0); av_dict_set(&opts, "seekable", "0", 0); + if (c->http_persistent) + av_dict_set(&opts, "multiple_requests", "1", 0); + if (seg->size >= 0) { /* try to restrict the HTTP request to the part we want * (if this is in fact a HTTP request) */ @@ -1316,7 +1366,7 @@ restart: if (!v->needed) return AVERROR_EOF; - if (!v->input) { + if (!v->input || (c->http_persistent && v->input_read_done)) { int64_t reload_interval; struct segment *seg; @@ -1368,6 +1418,7 @@ reload: goto reload; } + v->input_read_done = 0; seg = current_segment(v); /* load/update Media Initialization Section, if any */ @@ -1405,7 +1456,11 @@ reload: return ret; } - ff_format_io_close(v->parent, &v->input); + if (c->http_persistent) { + v->input_read_done = 1; + } else { + ff_format_io_close(v->parent, &v->input); + } v->cur_seq_no++; c->cur_seq_no = v->cur_seq_no; @@ -1666,6 +1721,7 @@ static int hls_close(AVFormatContext *s) free_rendition_list(c); av_dict_free(&c->avio_opts); + ff_format_io_close(c->ctx, &c->playlist_pb); return 0; } @@ -1903,6 +1959,7 @@ static int recheck_discard_flags(AVFormatContext *s, int first) } else if (first && !cur_needed && pls->needed) { if (pls->input) ff_format_io_close(pls->parent, &pls->input); + pls->input_read_done = 0; pls->needed = 0; changed = 1; av_log(s, AV_LOG_INFO, "No longer receiving playlist %d\n", i); @@ -2137,6 +2194,7 @@ static int hls_read_seek(AVFormatContext *s, int stream_index, struct playlist *pls = c->playlists[i]; if (pls->input) ff_format_io_close(pls->parent, &pls->input); + pls->input_read_done = 0; av_packet_unref(&pls->pkt); reset_packet(&pls->pkt); pls->pb.eof_reached = 0; @@ -2191,6 +2249,8 @@ static const AVOption hls_options[] = { INT_MIN, INT_MAX, FLAGS}, {"max_reload", "Maximum number of times a insufficient list is attempted to be reloaded", OFFSET(max_reload), AV_OPT_TYPE_INT, {.i64 = 1000}, 0, INT_MAX, FLAGS}, + {"http_persistent", "Use persistent HTTP connections", + OFFSET(http_persistent), AV_OPT_TYPE_BOOL, {.i64 = 0}, 0, 1, FLAGS }, {NULL} };
From: Aman Gupta <aman@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 --- doc/demuxers.texi | 3 +++ libavformat/hls.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 67 insertions(+), 4 deletions(-)