[FFmpeg-devel] [PATCH] hls demuxer: add option to defer parsing of variants

[Anssi Hannula]

Hi,

Rainer Hochecker kirjoitti 2017-11-26 12:46:
> fixed mem leak poined out by Steven
> 
> ---
>  doc/demuxers.texi |   5 +
>  libavformat/hls.c | 304 
> ++++++++++++++++++++++++++++++++++++------------------
>  2 files changed, 209 insertions(+), 100 deletions(-)
> 
[...]
> +
> + at item load_all_variants
> +If 0, only the first variant/playlist is loaded on open. All other 
> variants
> +get disabled and can be enabled by setting discard option in program.
> +Default value is 1.
>  @end table

If playlist download+parsing is indeed taking long enough time that this 
is warranted, I wonder if we should maybe just never read any variant 
playlists in read_header(), and instead set AVFMTCTX_NOHEADER.
Then the user may discard AVPrograms right after open, before unneeded 
playlists are loaded+parsed.

This would avoid having a separate option/mode for this.

Any thoughts?


>  @section image2
> diff --git a/libavformat/hls.c b/libavformat/hls.c
> index 786934af03..c42e0b0f95 100644
> --- a/libavformat/hls.c
> +++ b/libavformat/hls.c
> @@ -112,6 +112,7 @@ struct playlist {
>      int n_segments;
>      struct segment **segments;
>      int needed, cur_needed;
> +    int parsed;
>      int cur_seq_no;
>      int64_t cur_seg_offset;
>      int64_t last_load_time;
> @@ -206,6 +207,7 @@ typedef struct HLSContext {
>      int strict_std_compliance;
>      char *allowed_extensions;
>      int max_reload;
> +    int load_all_variants;
>  } HLSContext;
[...]
> @@ -721,6 +726,7 @@ static int parse_playlist(HLSContext *c, const char 
> *url,
>          free_segment_list(pls);
>          pls->finished = 0;
>          pls->type = PLS_TYPE_UNSPECIFIED;
> +        pls->parsed = 1;
>      }

That "pls->parsed = 1" is in the "reinit old playlist for re-parse" 
block, is that intentional?

I'd think it should rather be at the end of the function alongside 
setting pls->last_load_time, so it is set to 1 whenever parsing has been 
done.

>      while (!avio_feof(in)) {
>          read_chomp_line(in, line, sizeof(line));
[...]
> @@ -1631,6 +1655,124 @@ static int hls_close(AVFormatContext *s)
>      return 0;
>  }
> 
> +static int init_playlist(HLSContext *c, struct playlist *pls)
> +{

This init_playlist() seems to be mostly code moved from below, could you 
split the patch so that the first patch moves the code around but does 
not change behavior, and the second patch makes the actual changes (i.e. 
adds the option)?

That would ease e.g. future bisecting.

[...]
> +    return 0;
> +}
> +
>  static int hls_read_header(AVFormatContext *s)
>  {
>      void *u = (s->flags & AVFMT_FLAG_CUSTOM_IO) ? NULL : s->pb;
> @@ -1663,6 +1805,9 @@ static int hls_read_header(AVFormatContext *s)
>      if ((ret = parse_playlist(c, s->filename, NULL, s->pb)) < 0)
>          goto fail;
> 
> +    /* first playlist was created, set it to parsed */
> +    c->variants[0]->playlists[0]->parsed = 1;
> +

I think parse_playlist() should set this when it parses something.

>      if ((ret = save_avio_options(s)) < 0)
>          goto fail;
> 
> @@ -1675,8 +1820,15 @@ static int hls_read_header(AVFormatContext *s)
>          goto fail;
[...]
> 
>      /* Select the starting segments */
>      for (i = 0; i < c->n_playlists; i++) {
>          struct playlist *pls = c->playlists[i];
> 
> -        if (pls->n_segments == 0)
> +        if (pls->n_segments == 0 && !pls->parsed)
>              continue;

Why?

>          pls->cur_seq_no = select_cur_seq_no(c, pls);
> @@ -1736,97 +1892,9 @@ static int hls_read_header(AVFormatContext *s)
>      /* Open the demuxer for each playlist */
>      for (i = 0; i < c->n_playlists; i++) {
>          struct playlist *pls = c->playlists[i];
> -        AVInputFormat *in_fmt = NULL;
> -
[...]

-- 
Anssi Hannula