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

[Anssi Hannula]

Hi,

Sorry about the delay.

Rainer Hochecker kirjoitti 2017-11-28 00:23:
> 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.

OK I guess. Though arguably hls is already quite different from mpegts 
which is the only other demuxer that creates multiple AVPrograms.

In the long run, I think I'd prefer the HLS demuxer to have a mode where 
only a single program/variant is given to the user at a time, with the 
demuxer handling the switching between the variants. But that would be a 
lot more work and I'm not even sure it would be feasible. So I guess 
your solution is OK, at least for now.


Is there a reason the first variant/playlist is still parsed, i.e. why 
not simply not parse any media playlists (i.e. only master pls) when the 
new mode is selected?
If the player is selecting the variant/program based on bitrate (like 
Kodi does), then the first playlist would likely have been 
downloaded+parsed unnecessarily.


Also, even with your current patch you will need to set 
AVFMTCTX_NOHEADER as you defer avformat_new_stream() calls to 
read_packet(). I think you can update update_noheader_flag() to set 
flag_needed=1 if any pls is !pls->is_parsed.

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

OK. But now it is not set for master playlists, so maybe move 
pls->parsed = 1 below the "fail" label then?

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

I don't think I follow. If parse_playlist() would set the playlist as 
parsed on every call (whether failed or not), then all playlists would 
be set to parsed and this line would be unnecessary.

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

Yes, but after your change any parsed zero-segment playlists will get 
select_cur_seq_no() called while they didn't before.

[...]
>     return changed;
>  }
> 
> +static void recheck_discard_programs(AVFormatContext *s)
> +{
> +    HLSContext *c = s->priv_data;
> +    int i, j;
> +
> +    for (i = 0; i < c->n_variants; i++) {
> +        struct variant *var = c->variants[i];
> +        AVProgram *program = s->programs[i];
> +
> +        if (program->discard >= AVDISCARD_ALL)
> +            continue;
> +
> +        for (j = 0; j < c->variants[i]->n_playlists; j++) {
> +            struct playlist *pls = c->variants[i]->playlists[j];
> +
> +            if (!pls->parsed) {
> +                if (parse_playlist(c, pls->url, pls, NULL) < 0)
> +                    continue;
> +                if (var->audio_group[0])
> +                    add_renditions_to_variant(c, var, 
> AVMEDIA_TYPE_AUDIO, var->audio_group);
> +                if (var->video_group[0])
> +                    add_renditions_to_variant(c, var, 
> AVMEDIA_TYPE_VIDEO, var->video_group);
> +                if (var->subtitles_group[0])
> +                    add_renditions_to_variant(c, var, 
> AVMEDIA_TYPE_SUBTITLE, var->subtitles_group);
> +                init_playlist(c, pls);
> +            }
> +        }
> +    }
> +}
> +

I applied a fix few days ago for hls that refactored the discard flag 
handling.

Instead of adding this new recheck_discard_programs(), could you instead 
update the playlist_needed() logic to return 0 on appropriate situations 
in the new mode, and then have recheck_discard_flags() call a function 
to do the needed initialization/parsing in the "if (cur_needed && 
!pls->needed)" case?

Sorry about the timing.

[...]
> +    {"load_all_variants", "if > 0 all playlists of all variants are 
> opened at startup",
> +        OFFSET(load_all_variants), AV_OPT_TYPE_BOOL, {.i64 = 1}, 0, 1, 
> FLAGS},

The help text is a bit confusing for the user as it is a boolean. Maybe 
just:
"parse all playlists of all variants at startup"


-- 
Anssi Hannula