diff mbox series

[FFmpeg-devel,1/4] avformat/avidec: Don't reimplement ff_free_stream()

Message ID 20200327095557.24069-1-andreas.rheinhardt@gmail.com
State Accepted
Headers show
Series [FFmpeg-devel,1/4] avformat/avidec: Don't reimplement ff_free_stream()
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Andreas Rheinhardt March 27, 2020, 9:55 a.m. UTC
Using ff_free_stream() makes the code more readable, more future-proof
(the old code freed AVCodecContexts and AVCodecParameters and its
substructures manually, so that there is a chance that there would be a
memleak for some time if new substructures were added) and reduces
code size.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
 libavformat/avidec.c | 17 ++---------------
 1 file changed, 2 insertions(+), 15 deletions(-)

Comments

Andreas Rheinhardt April 4, 2020, 8:26 a.m. UTC | #1
Andreas Rheinhardt:
> Using ff_free_stream() makes the code more readable, more future-proof
> (the old code freed AVCodecContexts and AVCodecParameters and its
> substructures manually, so that there is a chance that there would be a
> memleak for some time if new substructures were added) and reduces
> code size.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
>  libavformat/avidec.c | 17 ++---------------
>  1 file changed, 2 insertions(+), 15 deletions(-)
> 
> diff --git a/libavformat/avidec.c b/libavformat/avidec.c
> index 00c3978b2d..ae343e732a 100644
> --- a/libavformat/avidec.c
> +++ b/libavformat/avidec.c
> @@ -600,21 +600,8 @@ static int avi_read_header(AVFormatContext *s)
>                      goto fail;
>  
>                  ast = s->streams[0]->priv_data;
> -                av_freep(&s->streams[0]->codecpar->extradata);
> -                av_freep(&s->streams[0]->codecpar);
> -#if FF_API_LAVF_AVCTX
> -FF_DISABLE_DEPRECATION_WARNINGS
> -                av_freep(&s->streams[0]->codec);
> -FF_ENABLE_DEPRECATION_WARNINGS
> -#endif
> -                if (s->streams[0]->info)
> -                    av_freep(&s->streams[0]->info->duration_error);
> -                av_freep(&s->streams[0]->info);
> -                if (s->streams[0]->internal)
> -                    av_freep(&s->streams[0]->internal->avctx);
> -                av_freep(&s->streams[0]->internal);
> -                av_freep(&s->streams[0]);
> -                s->nb_streams = 0;
> +                st->priv_data = NULL;
> +                ff_free_stream(s, st);
>                  if (CONFIG_DV_DEMUXER) {
>                      avi->dv_demux = avpriv_dv_init_demux(s);
>                      if (!avi->dv_demux)
> 
Any comments on this patchset? If there are no objections, I'll apply it
tomorrow.

- Andreas
James Almer April 4, 2020, 1:59 p.m. UTC | #2
On 3/27/2020 6:55 AM, Andreas Rheinhardt wrote:
> Using ff_free_stream() makes the code more readable, more future-proof
> (the old code freed AVCodecContexts and AVCodecParameters and its
> substructures manually, so that there is a chance that there would be a
> memleak for some time if new substructures were added) and reduces
> code size.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
>  libavformat/avidec.c | 17 ++---------------
>  1 file changed, 2 insertions(+), 15 deletions(-)
> 
> diff --git a/libavformat/avidec.c b/libavformat/avidec.c
> index 00c3978b2d..ae343e732a 100644
> --- a/libavformat/avidec.c
> +++ b/libavformat/avidec.c
> @@ -600,21 +600,8 @@ static int avi_read_header(AVFormatContext *s)
>                      goto fail;
>  
>                  ast = s->streams[0]->priv_data;
> -                av_freep(&s->streams[0]->codecpar->extradata);
> -                av_freep(&s->streams[0]->codecpar);
> -#if FF_API_LAVF_AVCTX
> -FF_DISABLE_DEPRECATION_WARNINGS
> -                av_freep(&s->streams[0]->codec);
> -FF_ENABLE_DEPRECATION_WARNINGS
> -#endif
> -                if (s->streams[0]->info)
> -                    av_freep(&s->streams[0]->info->duration_error);
> -                av_freep(&s->streams[0]->info);
> -                if (s->streams[0]->internal)
> -                    av_freep(&s->streams[0]->internal->avctx);
> -                av_freep(&s->streams[0]->internal);
> -                av_freep(&s->streams[0]);
> -                s->nb_streams = 0;
> +                st->priv_data = NULL;
> +                ff_free_stream(s, st);

I stream_index guaranteed to be 0 here? Or in turn, was s->streams[0]
correct in this chunk to begin with?

>                  if (CONFIG_DV_DEMUXER) {
>                      avi->dv_demux = avpriv_dv_init_demux(s);
>                      if (!avi->dv_demux)
>
Andreas Rheinhardt April 4, 2020, 2:09 p.m. UTC | #3
James Almer:
> On 3/27/2020 6:55 AM, Andreas Rheinhardt wrote:
>> Using ff_free_stream() makes the code more readable, more future-proof
>> (the old code freed AVCodecContexts and AVCodecParameters and its
>> substructures manually, so that there is a chance that there would be a
>> memleak for some time if new substructures were added) and reduces
>> code size.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>> ---
>>  libavformat/avidec.c | 17 ++---------------
>>  1 file changed, 2 insertions(+), 15 deletions(-)
>>
>> diff --git a/libavformat/avidec.c b/libavformat/avidec.c
>> index 00c3978b2d..ae343e732a 100644
>> --- a/libavformat/avidec.c
>> +++ b/libavformat/avidec.c
>> @@ -600,21 +600,8 @@ static int avi_read_header(AVFormatContext *s)
>>                      goto fail;
>>  
>>                  ast = s->streams[0]->priv_data;
>> -                av_freep(&s->streams[0]->codecpar->extradata);
>> -                av_freep(&s->streams[0]->codecpar);
>> -#if FF_API_LAVF_AVCTX
>> -FF_DISABLE_DEPRECATION_WARNINGS
>> -                av_freep(&s->streams[0]->codec);
>> -FF_ENABLE_DEPRECATION_WARNINGS
>> -#endif
>> -                if (s->streams[0]->info)
>> -                    av_freep(&s->streams[0]->info->duration_error);
>> -                av_freep(&s->streams[0]->info);
>> -                if (s->streams[0]->internal)
>> -                    av_freep(&s->streams[0]->internal->avctx);
>> -                av_freep(&s->streams[0]->internal);
>> -                av_freep(&s->streams[0]);
>> -                s->nb_streams = 0;
>> +                st->priv_data = NULL;
>> +                ff_free_stream(s, st);
> 
> I stream_index guaranteed to be 0 here? Or in turn, was s->streams[0]
> correct in this chunk to begin with?
> 

Yes. See lines 592-595:

                /* After some consideration -- I don't think we
                 * have to support anything but DV in type1 AVIs. */
                if (s->nb_streams != 1)
                    goto fail;

(I basically don't know DV, so I don't know how much additional work it
would be to extend this. I also don't know if simply freeing the last
stream (as ff_free_stream() does automatically) would mean that the
above check could be removed. All I know is that the
avpriv_dv_init_demux() will add a new stream to s.)

- Andreas
Andreas Rheinhardt April 5, 2020, 4:54 p.m. UTC | #4
Andreas Rheinhardt:
> Using ff_free_stream() makes the code more readable, more future-proof
> (the old code freed AVCodecContexts and AVCodecParameters and its
> substructures manually, so that there is a chance that there would be a
> memleak for some time if new substructures were added) and reduces
> code size.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
>  libavformat/avidec.c | 17 ++---------------
>  1 file changed, 2 insertions(+), 15 deletions(-)
> 
> diff --git a/libavformat/avidec.c b/libavformat/avidec.c
> index 00c3978b2d..ae343e732a 100644
> --- a/libavformat/avidec.c
> +++ b/libavformat/avidec.c
> @@ -600,21 +600,8 @@ static int avi_read_header(AVFormatContext *s)
>                      goto fail;
>  
>                  ast = s->streams[0]->priv_data;
> -                av_freep(&s->streams[0]->codecpar->extradata);
> -                av_freep(&s->streams[0]->codecpar);
> -#if FF_API_LAVF_AVCTX
> -FF_DISABLE_DEPRECATION_WARNINGS
> -                av_freep(&s->streams[0]->codec);
> -FF_ENABLE_DEPRECATION_WARNINGS
> -#endif
> -                if (s->streams[0]->info)
> -                    av_freep(&s->streams[0]->info->duration_error);
> -                av_freep(&s->streams[0]->info);
> -                if (s->streams[0]->internal)
> -                    av_freep(&s->streams[0]->internal->avctx);
> -                av_freep(&s->streams[0]->internal);
> -                av_freep(&s->streams[0]);
> -                s->nb_streams = 0;
> +                st->priv_data = NULL;
> +                ff_free_stream(s, st);
>                  if (CONFIG_DV_DEMUXER) {
>                      avi->dv_demux = avpriv_dv_init_demux(s);
>                      if (!avi->dv_demux)
> 

Applied the set.

- Andreas
diff mbox series

Patch

diff --git a/libavformat/avidec.c b/libavformat/avidec.c
index 00c3978b2d..ae343e732a 100644
--- a/libavformat/avidec.c
+++ b/libavformat/avidec.c
@@ -600,21 +600,8 @@  static int avi_read_header(AVFormatContext *s)
                     goto fail;
 
                 ast = s->streams[0]->priv_data;
-                av_freep(&s->streams[0]->codecpar->extradata);
-                av_freep(&s->streams[0]->codecpar);
-#if FF_API_LAVF_AVCTX
-FF_DISABLE_DEPRECATION_WARNINGS
-                av_freep(&s->streams[0]->codec);
-FF_ENABLE_DEPRECATION_WARNINGS
-#endif
-                if (s->streams[0]->info)
-                    av_freep(&s->streams[0]->info->duration_error);
-                av_freep(&s->streams[0]->info);
-                if (s->streams[0]->internal)
-                    av_freep(&s->streams[0]->internal->avctx);
-                av_freep(&s->streams[0]->internal);
-                av_freep(&s->streams[0]);
-                s->nb_streams = 0;
+                st->priv_data = NULL;
+                ff_free_stream(s, st);
                 if (CONFIG_DV_DEMUXER) {
                     avi->dv_demux = avpriv_dv_init_demux(s);
                     if (!avi->dv_demux)