(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>:
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?
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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
