Re: [FFmpeg-devel] [PATCH] lavformat/utils: Fix a memleak that st->codec->hw_frames_ctx
On Sun, Jan 22, 2017 at 9:03 PM, Mark Thompson wrote: > On 20/01/17 02:06, Huang, Zhengxu wrote: > > From 9ceb2ac6a89246f2e686eb3ad3448fbaff5328f7 Mon Sep 17 00:00:00 2001 > > From: Zhengxu > > 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 > > Signed-off-by: Huang, Zhengxu > > Signed-off-by: Andrew, Zhang > > --- > > 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 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] lavformat/utils: Fix a memleak that st->codec->hw_frames_ctx
On 20/01/17 02:06, Huang, Zhengxu wrote: > From 9ceb2ac6a89246f2e686eb3ad3448fbaff5328f7 Mon Sep 17 00:00:00 2001 > From: Zhengxu > 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 > Signed-off-by: Huang, Zhengxu > Signed-off-by: Andrew, Zhang > --- > 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 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] lavformat/utils: Fix a memleak that st->codec->hw_frames_ctx
On Sun, 22 Jan 2017 10:26:58 +0800
"Huang, Zhengxu" wrote:
> 在 2017/1/20 17:56, wm4 写道:
>
> > On Fri, 20 Jan 2017 17:35:33 +0800
> > Chao Liu 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.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] lavformat/utils: Fix a memleak that st->codec->hw_frames_ctx
在 2017/1/20 17:56, wm4 写道:
On Fri, 20 Jan 2017 17:35:33 +0800
Chao Liu 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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] lavformat/utils: Fix a memleak that st->codec->hw_frames_ctx
On Fri, 20 Jan 2017 17:35:33 +0800
Chao Liu 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.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] lavformat/utils: Fix a memleak that st->codec->hw_frames_ctx
On Fri, Jan 20, 2017 at 8:35 PM, Chao Liu 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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] lavformat/utils: Fix a memleak that st->codec->hw_frames_ctx
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 wrote:
> On Fri, 20 Jan 2017 10:06:50 +0800
> "Huang, Zhengxu" wrote:
>
> > From 9ceb2ac6a89246f2e686eb3ad3448fbaff5328f7 Mon Sep 17 00:00:00 2001
> > From: Zhengxu
> > 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
> > Signed-off-by: Huang, Zhengxu
> > Signed-off-by: Andrew, Zhang
> > ---
> > 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
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] lavformat/utils: Fix a memleak that st->codec->hw_frames_ctx
On Fri, 20 Jan 2017 10:06:50 +0800 "Huang, Zhengxu" wrote: > From 9ceb2ac6a89246f2e686eb3ad3448fbaff5328f7 Mon Sep 17 00:00:00 2001 > From: Zhengxu > 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 > Signed-off-by: Huang, Zhengxu > Signed-off-by: Andrew, Zhang > --- > 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
