| Message ID | 20181222085513.8901-1-lq@chinaffmpeg.org |
|---|---|
| State | New |
| Headers | show |
On Sat, Dec 22, 2018 at 10:55 AM Steven Liu <lq@chinaffmpeg.org> wrote: > > fix ticket: 7631 > > Signed-off-by: Steven Liu <lq@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. Also the function could be something like "hls_free_variant_streams", but this is just an 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;" > + 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? > + } > + > + > +} 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. Jan
> On Dec 22, 2018, at 22:26, Jan Ekström <jeebjp@gmail.com> wrote: > > On Sat, Dec 22, 2018 at 10:55 AM Steven Liu <lq@chinaffmpeg.org> wrote: >> >> fix ticket: 7631 >> >> Signed-off-by: Steven Liu <lq@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
> On Dec 22, 2018, at 23:02, Steven Liu <lq@chinaffmpeg.org> wrote: > > > >> On Dec 22, 2018, at 22:26, Jan Ekström <jeebjp@gmail.com> wrote: >> >> On Sat, Dec 22, 2018 at 10:55 AM Steven Liu <lq@chinaffmpeg.org> wrote: >>> >>> fix ticket: 7631 >>> >>> Signed-off-by: Steven Liu <lq@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, Ah, sorry, my mistake, i think this takes a HLSContext is better than VariantStream, move the for loop into the function, That should better than call function in loop, because call function will push stack, and out function will pop stack, So void function { for (i = 0; i < n; i++); } is better than for (i = 0; i< n; i++) { function(); } the VariantStream is member in the HLSContext. >> >>> + } >>> + >>> + >>> +} >> >> 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 Thanks Steven
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) +{ + int i = 0; + HLSContext *hls = s->priv_data; + AVFormatContext *vtt_oc = NULL; + VariantStream *vs = NULL; + + for (i = 0; i < hls->nb_varstreams; i++) { + vs = &hls->var_streams[i]; + vtt_oc = vs->vtt_avf; + + 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); + } + + +} 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);
fix ticket: 7631 Signed-off-by: Steven Liu <lq@chinaffmpeg.org> --- libavformat/hlsenc.c | 50 +++++++++++++++++++++++++++++++++----------------- 1 file changed, 33 insertions(+), 17 deletions(-)