On Sun, Dec 17, 2017 at 1:21 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? > > > 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 > _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
