[FFmpeg-devel] [PATCH] hls demuxer: add option to defer parsing of variants
Anssi Hannula
anssi.hannula at iki.fi
Sat Dec 9 21:17:07 EET 2017
(Re-added ffmpeg-devel@)
Rainer Hochecker kirjoitti 2017-12-03 09:16:
> 2017-12-02 16:09 GMT+01:00 Anssi Hannula <anssi.hannula at iki.fi>:
>> 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?
>
> I thought this could become the new default after a while. Clients that
> don't
> select a program would still work. Selecting the first available
> program, if
> none was selected seems the most natural way. Selecting all or none
> make
> less sense.
Hmm.. I wonder if selecting the highest-bandwidth-one might make more
sense as the playlist/program order is arbitrary (IIRC) so currently an
arbitrary playlist is parsed.
>> 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.
>>
>
> Right, but this is acceptable. The majority of user don't have set
> network
> bandwidth anyway.
If the user does not have bandwidth set, Kodi selects the
highest-bandwidth program => so both the first playlist and the
highest-bandwidth playlist get parsed.
>>
>> 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.
>
> Actually I call open again after having set AVFMTCTX_NOHEADER.
> Could you please be more verbose on how you would like to have it?
With your patch, with load_all_variants=0, this happens, if I follow the
code right:
1. hls_read_header()
- A single playlist is opened - in this case the subdemuxer has no
AVFTMCTX_NOHEADER so AVFMTCTX_NOHEADER is not set on the main context.
- Other playlists are not parsed.
...
2. user undiscards a program.
3. hls_read_packet()
- Discard flag checking notices a new playlist is needed and it is
parsed.
- update_streams_from_subdemuxer() gets called from init_playlist(), and
it calls avformat_new_stream()
The read_packet documentation says:
> 'avformat_new_stream' can be called only if the flag
> * AVFMTCTX_NOHEADER is used and only in the calling thread (not in
> a
> * background thread).
And you have not set AVFMTCTX_NOHEADER.
So probably update_noheader_flag() needs this change:
- if (pls->has_noheader_flag) {
+ if (pls->has_noheader_flag || !pls->is_parsed) {
So that AVFMTCTX_NOHEADER gets always set if there are unparsed
playlists left.
That might cause e.g. avformat_find_stream_info (if it is called - looks
like Kodi always does for HLS - which might be another thing to check to
improve launch performance) to probe longer as it tries to find the
missing streams in vain, though, so better check the performance is
still OK afterwards...
>>
>>
>>>>
>>>>
>>>>> @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?
>
> Does this solve the issue with the master pls? For master playlist
> parse_playlist
> is called with NULL for argument playlist.
I think
if (pls)
pls->is_parsed = 1;
should work.
A master playlist has no struct playlist, but if the URL we starts with
is a media playlist then a pls is allocated during parse_playlist().
[...]
>>>>> 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 = parse_playlist(c, s->filename, NULL, s->pb)) < 0)
> goto fail;
>
> How can parse_playlist set anything on pls if called with NULL for ps?
If parse_playlist sees a media playlist, it allocates a pls, so it can
set it to is_parsed as long as the set is at the end of the function.
>>
>>>>
>>>>> 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.
>
> Just drop this?
Should be OK to drop, I think.
[...]
--
Anssi Hannula
More information about the ffmpeg-devel
mailing list