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

Submitted by Steven Liu on Dec. 22, 2018, 8:55 a.m.

Details

Message ID 20181222085513.8901-1-lq@chinaffmpeg.org
State New
Headers show

Commit Message

Steven Liu Dec. 22, 2018, 8:55 a.m.
fix ticket: 7631

Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
---
 libavformat/hlsenc.c | 50 +++++++++++++++++++++++++++++++++-----------------
 1 file changed, 33 insertions(+), 17 deletions(-)

Comments

Jan Ekström Dec. 22, 2018, 2:26 p.m.
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
Steven Liu Dec. 22, 2018, 3:02 p.m.
> 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
Steven Liu Dec. 22, 2018, 3:10 p.m.
> 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

Patch hide | download patch | download mbox

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);