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 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
and select a program. I did not like selecting the program between open and

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


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

ffmpeg-devel mailing list

Reply via email to