> On Dec 22, 2018, at 22:26, Jan Ekström <jee...@gmail.com> wrote: > > On Sat, Dec 22, 2018 at 10:55 AM Steven Liu <l...@chinaffmpeg.org> wrote: >> >> fix ticket: 7631 >> >> Signed-off-by: Steven Liu <l...@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@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel Thanks Steven _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel