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

[Rainer Hochecker]

2017-11-27 22:53 GMT+01:00 Anssi Hannula <anssi.hannula at iki.fi>:
> 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?

I actually tried it like this but somwhow did not like it because this
is different
to all other demuxers/formats. In general you open an input, call
find_stream_info,
and select a program. I did not like selecting the program between open and
find_stream_info.

>
>
>>  @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.
>

I put it at the beginning because I wanted to avoid that it gets tried again
on error.

>>      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.

Sure. At least I can try.

>
> [...]
>>
>> +    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.

That was pitfall for my v1. The first playlist gets parsed with no
playlist given as argument.
fate failed because non-master playlists were not set to parsed.

>
>>      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?

playlists not parsed are invalid, right? maybe n_segments is 0 for
not parsed playlists but this was no obvious to me.

>
>>          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
>