[FFmpeg-devel] [PATCH] avformat/hlsenc: free varstreams after write all varstreams info

[Steven Liu]

> On Dec 22, 2018, at 22:26, Jan Ekström <jeebjp at gmail.com> wrote:
> 
> On Sat, Dec 22, 2018 at 10:55 AM Steven Liu <lq at chinaffmpeg.org> wrote:
>> 
>> fix ticket: 7631
>> 
>> Signed-off-by: Steven Liu <lq at chinaffmpeg.org>
>> ---
>> libavformat/hlsenc.c | 50 +++++++++++++++++++++++++++++++++-----------------
>> 1 file changed, 33 insertions(+), 17 deletions(-)
>> 
>> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
>> index bdd2a113bd..e3cd6f375a 100644
>> --- a/libavformat/hlsenc.c
>> +++ b/libavformat/hlsenc.c
>> @@ -2360,6 +2360,37 @@ static int hls_write_packet(AVFormatContext *s, AVPacket *pkt)
>>     return ret;
>> }
>> 
>> +static void hls_varstreams_free(struct AVFormatContext *s)
>> +{
> 
> I think you only use the AVFormatContext to get the HLSContext, and
> you already have that pointer in hls_write_trailer.
> 
> Thus I think you could just make this function take in a HLSContext *hls.
Okay.
> 
> Also the function could be something like "hls_free_variant_streams",
> but this is just an opinion.
good opinion.
> 
>> +    int i = 0;
>> +    HLSContext *hls = s->priv_data;
>> +    AVFormatContext *vtt_oc = NULL;
>> +    VariantStream *vs = NULL;
>> +
>> +    for (i = 0; i < hls->nb_varstreams; i++) {
> 
> As far as I asked on IRC, we're OK with declaring iterators in a for
> loop, so it can be "int i = 0;”
I think the better declared the i at the start block of this function.
> 
>> +        vs = &hls->var_streams[i];
>> +        vtt_oc = vs->vtt_avf;
>> +
> 
> Also asked if things utilized within a for loop can have their own
> variables declared at the top of the loop.
> Thus the AVFormatContext and VariantStream can be declared here.
> 
>> +        av_freep(&vs->basename);
>> +        av_freep(&vs->base_output_dirname);
>> +        av_freep(&vs->fmp4_init_filename);
>> +        if (vtt_oc) {
>> +            av_freep(&vs->vtt_basename);
>> +            av_freep(&vs->vtt_m3u8_name);
>> +            avformat_free_context(vtt_oc);
>> +        }
>> +
>> +        hls_free_segments(vs->segments);
>> +        hls_free_segments(vs->old_segments);
>> +        av_freep(&vs->m3u8_name);
>> +        av_freep(&vs->streams);
>> +        av_freep(&vs->agroup);
>> +        av_freep(&vs->ccgroup);
>> +        av_freep(&vs->baseurl);
> 
> I also wonder if you should separate actual per-variant stream freeing
> into its own function which takes a VariantStream *vs in?
Okay,
> 
>> +    }
>> +
>> +
>> +}
> 
> Leave one empty line after the function, and no need for empty lines
> at the end of the function inside of it.
> 
> F.ex.
> {
>    for (...) {
>    }
> }
> 
> following_function {
>    ...
> 
>> static int hls_write_trailer(struct AVFormatContext *s)
>> {
>>     HLSContext *hls = s->priv_data;
>> @@ -2451,30 +2482,15 @@ failed:
>>             vs->size = avio_tell(vs->vtt_avf->pb) - vs->start_pos;
>>             ff_format_io_close(s, &vtt_oc->pb);
>>         }
>> -        av_freep(&vs->basename);
>> -        av_freep(&vs->base_output_dirname);
>>         avformat_free_context(oc);
>> 
>>         vs->avf = NULL;
>>         hls_window(s, 1, vs);
>> -
>> -        av_freep(&vs->fmp4_init_filename);
>> -        if (vtt_oc) {
>> -            av_freep(&vs->vtt_basename);
>> -            av_freep(&vs->vtt_m3u8_name);
>> -            avformat_free_context(vtt_oc);
>> -        }
>> -
>> -        hls_free_segments(vs->segments);
>> -        hls_free_segments(vs->old_segments);
>>         av_free(old_filename);
>> -        av_freep(&vs->m3u8_name);
>> -        av_freep(&vs->streams);
>> -        av_freep(&vs->agroup);
>> -        av_freep(&vs->ccgroup);
>> -        av_freep(&vs->baseurl);
>>     }
>> 
>> +    hls_varstreams_free(s);
>> +
>>     for (i = 0; i < hls->nb_ccstreams; i++) {
>>         ClosedCaptionsStream *ccs = &hls->cc_streams[i];
>>         av_freep(&ccs->ccgroup);
>> --
>> 2.15.1
>> 
> 
> Initial quick look.

New patch will come.

> 
> Jan
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Thanks
Steven