[FFmpeg-devel] [PATCH v2 3/5] avformat/hls: add http_persistent option

[Aman Gupta]

On Sun, Dec 17, 2017 at 12:34 PM Anssi Hannula <anssi.hannula at iki.fi> wrote:

> Hi!
>
> Aman Gupta kirjoitti 2017-12-13 02:35:
> > From: Aman Gupta <aman at 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.
> > +
> > + at 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
>