Hi,
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>:
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(-)
[...]
+
+@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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
