(Re-added ffmpeg-devel@)

Rainer Hochecker kirjoitti 2017-12-03 09:16:
2017-12-02 16:09 GMT+01:00 Anssi Hannula <anssi.hann...@iki.fi>:

Sorry about the delay.

Rainer Hochecker kirjoitti 2017-11-28 00:23:

2017-11-27 22:53 GMT+01:00 Anssi Hannula <anssi.hann...@iki.fi>:


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


+@item load_all_variants
+If 0, only the first variant/playlist is loaded on open. All other
+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
warranted, I wonder if we should maybe just never read any variant
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
and select a program. I did not like selecting the program between open

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

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

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
         pls->finished = 0;
         pls->type = PLS_TYPE_UNSPECIFIED;
+        pls->parsed = 1;

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

I'd think it should rather be at the end of the function alongside
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
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
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)


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

ffmpeg-devel mailing list

Reply via email to