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.
>
>
>> @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.
>> 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.
>
>> 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.
>
>> pls->cur_seq_no = select_cur_seq_no(c, pls);
>> @@ -1736,97 +1892,9 @@ static int hls_read_header(AVFormatContext *s)
>> /* Open the demuxer for each playlist */
>> for (i = 0; i < c->n_playlists; i++) {
>> struct playlist *pls = c->playlists[i];
>> - AVInputFormat *in_fmt = NULL;
>> -
>
> [...]
>
> --
> Anssi Hannula
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
