[FFmpeg-devel] lavformat/utils: Fix a memleak that st->codec->hw_frames_ctx

Submitted by Huang, Zhengxu on Jan. 20, 2017, 2:06 a.m.

Details

Message ID cfbcc9de-19d3-1292-857f-7a208a0f361b@gmail.com
State New
Headers show

Commit Message

Huang, Zhengxu Jan. 20, 2017, 2:06 a.m.
From 9ceb2ac6a89246f2e686eb3ad3448fbaff5328f7 Mon Sep 17 00:00:00 2001
From: Zhengxu <zhengxu.maxwell@gmail.com>
Date: Fri, 13 Jan 2017 10:33:05 +0800
Subject: [PATCH] lavformat/utils: Fix a memleak that st->codec->hw_frames_ctx
 is not released.

Signed-off-by: ChaoX A Liu <chaox.a.liu@gmail.com>
Signed-off-by: Huang, Zhengxu <zhengxu.maxwell@gmail.com>
Signed-off-by: Andrew, Zhang <huazh407@gmail.com>
---
 libavformat/utils.c | 1 +
 1 file changed, 1 insertion(+)

Comments

wm4 Jan. 20, 2017, 8:47 a.m.
On Fri, 20 Jan 2017 10:06:50 +0800
"Huang, Zhengxu" <zhengxu.maxwell@gmail.com> wrote:

> From 9ceb2ac6a89246f2e686eb3ad3448fbaff5328f7 Mon Sep 17 00:00:00 2001
> From: Zhengxu <zhengxu.maxwell@gmail.com>
> Date: Fri, 13 Jan 2017 10:33:05 +0800
> Subject: [PATCH] lavformat/utils: Fix a memleak that st->codec->hw_frames_ctx
>  is not released.
> 
> Signed-off-by: ChaoX A Liu <chaox.a.liu@gmail.com>
> Signed-off-by: Huang, Zhengxu <zhengxu.maxwell@gmail.com>
> Signed-off-by: Andrew, Zhang <huazh407@gmail.com>
> ---
>  libavformat/utils.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/libavformat/utils.c b/libavformat/utils.c
> index d5dfca7..cadec15 100644
> --- a/libavformat/utils.c
> +++ b/libavformat/utils.c
> @@ -4127,6 +4127,7 @@ static void free_stream(AVStream **pst)
>  FF_DISABLE_DEPRECATION_WARNINGS
>      av_freep(&st->codec->extradata);
>      av_freep(&st->codec->subtitle_header);
> +    av_buffer_unref(&st->codec->hw_frames_ctx);
>      av_freep(&st->codec);
>  FF_ENABLE_DEPRECATION_WARNINGS
>  #endif

What triggers this? In a sane world this should never ever happen.
Chao Liu Jan. 20, 2017, 9:35 a.m.
Have you ever used valgrind? Please just run the command below:
valgrind --leak-check=full --log-file=out.log ffmpeg -hwaccel qsv
-qsv_device /dev/dri/renderD128 -c:v h264_qsv -i a.h264 -c:v h264_qsv -b:v
2M  -y out.h264

See line 3323 of ffmpeg.c,
        ret = avcodec_copy_context(ost->st->codec, ost->enc_ctx);
and see what have been done in avcodec_copy_context:
    if (src->hw_frames_ctx) {
        dest->hw_frames_ctx = av_buffer_ref(src->hw_frames_ctx);
        if (!dest->hw_frames_ctx)
            goto fail;
    }
However, that is not freed when calling avformat_free_context.

On Fri, Jan 20, 2017 at 4:47 PM, wm4 <nfxjfg@googlemail.com> wrote:

> On Fri, 20 Jan 2017 10:06:50 +0800
> "Huang, Zhengxu" <zhengxu.maxwell@gmail.com> wrote:
>
> > From 9ceb2ac6a89246f2e686eb3ad3448fbaff5328f7 Mon Sep 17 00:00:00 2001
> > From: Zhengxu <zhengxu.maxwell@gmail.com>
> > Date: Fri, 13 Jan 2017 10:33:05 +0800
> > Subject: [PATCH] lavformat/utils: Fix a memleak that
> st->codec->hw_frames_ctx
> >  is not released.
> >
> > Signed-off-by: ChaoX A Liu <chaox.a.liu@gmail.com>
> > Signed-off-by: Huang, Zhengxu <zhengxu.maxwell@gmail.com>
> > Signed-off-by: Andrew, Zhang <huazh407@gmail.com>
> > ---
> >  libavformat/utils.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/libavformat/utils.c b/libavformat/utils.c
> > index d5dfca7..cadec15 100644
> > --- a/libavformat/utils.c
> > +++ b/libavformat/utils.c
> > @@ -4127,6 +4127,7 @@ static void free_stream(AVStream **pst)
> >  FF_DISABLE_DEPRECATION_WARNINGS
> >      av_freep(&st->codec->extradata);
> >      av_freep(&st->codec->subtitle_header);
> > +    av_buffer_unref(&st->codec->hw_frames_ctx);
> >      av_freep(&st->codec);
> >  FF_ENABLE_DEPRECATION_WARNINGS
> >  #endif
>
> What triggers this? In a sane world this should never ever happen.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Hendrik Leppkes Jan. 20, 2017, 9:48 a.m.
On Fri, Jan 20, 2017 at 8:35 PM, Chao Liu <chaox.a.liu@gmail.com> wrote:
> Have you ever used valgrind? Please just run the command below:
> valgrind --leak-check=full --log-file=out.log ffmpeg -hwaccel qsv
> -qsv_device /dev/dri/renderD128 -c:v h264_qsv -i a.h264 -c:v h264_qsv -b:v
> 2M  -y out.h264
>
> See line 3323 of ffmpeg.c,
>         ret = avcodec_copy_context(ost->st->codec, ost->enc_ctx);
> and see what have been done in avcodec_copy_context:
>     if (src->hw_frames_ctx) {
>         dest->hw_frames_ctx = av_buffer_ref(src->hw_frames_ctx);
>         if (!dest->hw_frames_ctx)
>             goto fail;
>     }
> However, that is not freed when calling avformat_free_context.
>

avcodec_copy_context is deprecated and should generally not be used
anymore. It would be more appropriate to resolve whatever issue
remains in ffmpeg that it needs to call it.
Otherwise, hw_frames_ctx is supplied by the caller to libavcodec, so
it also should manage its lifetime, I would think, and not be blindly
free'ed by avcodec.

PS:
Please don't top post.

- Hendrik
wm4 Jan. 20, 2017, 9:56 a.m.
On Fri, 20 Jan 2017 17:35:33 +0800
Chao Liu <chaox.a.liu@gmail.com> wrote:

> Have you ever used valgrind? Please just run the command below:
> valgrind --leak-check=full --log-file=out.log ffmpeg -hwaccel qsv
> -qsv_device /dev/dri/renderD128 -c:v h264_qsv -i a.h264 -c:v h264_qsv -b:v
> 2M  -y out.h264
> 
> See line 3323 of ffmpeg.c,
>         ret = avcodec_copy_context(ost->st->codec, ost->enc_ctx);
> and see what have been done in avcodec_copy_context:
>     if (src->hw_frames_ctx) {
>         dest->hw_frames_ctx = av_buffer_ref(src->hw_frames_ctx);
>         if (!dest->hw_frames_ctx)
>             goto fail;
>     }
> However, that is not freed when calling avformat_free_context.
> 

The hardware decoder should never be created in the demuxer.
Huang, Zhengxu Jan. 22, 2017, 2:26 a.m.
在 2017/1/20 17:56, wm4 写道:

> On Fri, 20 Jan 2017 17:35:33 +0800
> Chao Liu <chaox.a.liu@gmail.com> wrote:
>
>> Have you ever used valgrind? Please just run the command below:
>> valgrind --leak-check=full --log-file=out.log ffmpeg -hwaccel qsv
>> -qsv_device /dev/dri/renderD128 -c:v h264_qsv -i a.h264 -c:v h264_qsv -b:v
>> 2M  -y out.h264
>>
>> See line 3323 of ffmpeg.c,
>>          ret = avcodec_copy_context(ost->st->codec, ost->enc_ctx);
>> and see what have been done in avcodec_copy_context:
>>      if (src->hw_frames_ctx) {
>>          dest->hw_frames_ctx = av_buffer_ref(src->hw_frames_ctx);
>>          if (!dest->hw_frames_ctx)
>>              goto fail;
>>      }
>> However, that is not freed when calling avformat_free_context.
>>
> The hardware decoder should never be created in the demuxer.
I quite can't understand why this related to the "demuxer". Firstly we 
should admint that if
there is a memleak issue. Secondly, how or where to free the hw_frames_ctx.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
wm4 Jan. 22, 2017, 11:15 a.m.
On Sun, 22 Jan 2017 10:26:58 +0800
"Huang, Zhengxu" <zhengxu.maxwell@gmail.com> wrote:

> 在 2017/1/20 17:56, wm4 写道:
> 
> > On Fri, 20 Jan 2017 17:35:33 +0800
> > Chao Liu <chaox.a.liu@gmail.com> wrote:
> >  
> >> Have you ever used valgrind? Please just run the command below:
> >> valgrind --leak-check=full --log-file=out.log ffmpeg -hwaccel qsv
> >> -qsv_device /dev/dri/renderD128 -c:v h264_qsv -i a.h264 -c:v h264_qsv -b:v
> >> 2M  -y out.h264
> >>
> >> See line 3323 of ffmpeg.c,
> >>          ret = avcodec_copy_context(ost->st->codec, ost->enc_ctx);
> >> and see what have been done in avcodec_copy_context:
> >>      if (src->hw_frames_ctx) {
> >>          dest->hw_frames_ctx = av_buffer_ref(src->hw_frames_ctx);
> >>          if (!dest->hw_frames_ctx)
> >>              goto fail;
> >>      }
> >> However, that is not freed when calling avformat_free_context.
> >>  
> > The hardware decoder should never be created in the demuxer.  
> I quite can't understand why this related to the "demuxer". Firstly we 
> should admint that if
> there is a memleak issue. Secondly, how or where to free the hw_frames_ctx.

demuxer = libavformat. libavformat sometimes/often opens a decoder to
probe codec parameters. (Which I think is madness, but that's another
issue.)

What absolutely shouldn't happen is that libavformat opens a
hardware-based decoder. What should happen even _less_ is that closing
the decoder somehow leaves hw_frames_ctx set.

I'm against painting bugs just over without knowing the real underlying
issue.
Mark Thompson Jan. 22, 2017, 1:03 p.m.
On 20/01/17 02:06, Huang, Zhengxu wrote:
> From 9ceb2ac6a89246f2e686eb3ad3448fbaff5328f7 Mon Sep 17 00:00:00 2001
> From: Zhengxu <zhengxu.maxwell@gmail.com>
> Date: Fri, 13 Jan 2017 10:33:05 +0800
> Subject: [PATCH] lavformat/utils: Fix a memleak that st->codec->hw_frames_ctx
>  is not released.
> 
> Signed-off-by: ChaoX A Liu <chaox.a.liu@gmail.com>
> Signed-off-by: Huang, Zhengxu <zhengxu.maxwell@gmail.com>
> Signed-off-by: Andrew, Zhang <huazh407@gmail.com>
> ---
>  libavformat/utils.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/libavformat/utils.c b/libavformat/utils.c
> index d5dfca7..cadec15 100644
> --- a/libavformat/utils.c
> +++ b/libavformat/utils.c
> @@ -4127,6 +4127,7 @@ static void free_stream(AVStream **pst)
>  FF_DISABLE_DEPRECATION_WARNINGS
>      av_freep(&st->codec->extradata);
>      av_freep(&st->codec->subtitle_header);
> +    av_buffer_unref(&st->codec->hw_frames_ctx);
>      av_freep(&st->codec);
>  FF_ENABLE_DEPRECATION_WARNINGS
>  #endif
> -- 
> 1.8.3.1
> 

As stated elsewhere, this should never happen in a sane program (AVStream.codec was deprecated before AVCodecContext.hw_frames_ctx was introduced), but I admit that ffmpeg.c cannot be said to be a sane program.

I think the patch is probably ok, but it is very difficult to tell.  Why doesn't the code here already call avcodec_free_context() here to free st->codec properly, including unreffing hw_frames_ctx?  I imagine there is some crazy reason for this (some fields have been copied somewhere else, maybe?), and without knowing that I don't think this patch should be applied.

So, I mostly agree with Hendrik that it is probably better to fix ffmpeg.c to not cause the problem in the first place rather than changing this deprecated code in lavf.

Thanks,

- Mark
Chao Liu Jan. 23, 2017, 1:40 a.m.
On Sun, Jan 22, 2017 at 9:03 PM, Mark Thompson <sw@jkqxz.net> wrote:

> On 20/01/17 02:06, Huang, Zhengxu wrote:
> > From 9ceb2ac6a89246f2e686eb3ad3448fbaff5328f7 Mon Sep 17 00:00:00 2001
> > From: Zhengxu <zhengxu.maxwell@gmail.com>
> > Date: Fri, 13 Jan 2017 10:33:05 +0800
> > Subject: [PATCH] lavformat/utils: Fix a memleak that
> st->codec->hw_frames_ctx
> >  is not released.
> >
> > Signed-off-by: ChaoX A Liu <chaox.a.liu@gmail.com>
> > Signed-off-by: Huang, Zhengxu <zhengxu.maxwell@gmail.com>
> > Signed-off-by: Andrew, Zhang <huazh407@gmail.com>
> > ---
> >  libavformat/utils.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/libavformat/utils.c b/libavformat/utils.c
> > index d5dfca7..cadec15 100644
> > --- a/libavformat/utils.c
> > +++ b/libavformat/utils.c
> > @@ -4127,6 +4127,7 @@ static void free_stream(AVStream **pst)
> >  FF_DISABLE_DEPRECATION_WARNINGS
> >      av_freep(&st->codec->extradata);
> >      av_freep(&st->codec->subtitle_header);
> > +    av_buffer_unref(&st->codec->hw_frames_ctx);
> >      av_freep(&st->codec);
> >  FF_ENABLE_DEPRECATION_WARNINGS
> >  #endif
> > --
> > 1.8.3.1
> >
>
> As stated elsewhere, this should never happen in a sane program
> (AVStream.codec was deprecated before AVCodecContext.hw_frames_ctx was
> introduced), but I admit that ffmpeg.c cannot be said to be a sane program.
>
> I think the patch is probably ok, but it is very difficult to tell.  Why
> doesn't the code here already call avcodec_free_context() here to free
> st->codec properly, including unreffing hw_frames_ctx?  I imagine there is
> some crazy reason for this (some fields have been copied somewhere else,
> maybe?), and without knowing that I don't think this patch should be
> applied.
>
> So, I mostly agree with Hendrik that it is probably better to fix ffmpeg.c
> to not cause the problem in the first place rather than changing this
> deprecated code in lavf.
>
> Thanks,
>
> - Mark


OK. Let's end this topic till we find out a graceful way to fix it.

Thanks for all your patient explaining.

Regards,
Chao

Patch hide | download patch | download mbox

diff --git a/libavformat/utils.c b/libavformat/utils.c
index d5dfca7..cadec15 100644
--- a/libavformat/utils.c
+++ b/libavformat/utils.c
@@ -4127,6 +4127,7 @@  static void free_stream(AVStream **pst)
 FF_DISABLE_DEPRECATION_WARNINGS
     av_freep(&st->codec->extradata);
     av_freep(&st->codec->subtitle_header);
+    av_buffer_unref(&st->codec->hw_frames_ctx);
     av_freep(&st->codec);
 FF_ENABLE_DEPRECATION_WARNINGS
 #endif