diff mbox series

[FFmpeg-devel,v2,1/6] avcodec/qsv_h2645: fix memory leak for plugin load

Message ID 20210105024342.85970-1-guangxin.xu@intel.com
State Accepted
Commit 4c47b41782e9f8f875b1f7a791d53f5cbc933e63
Headers show
Series [FFmpeg-devel,v2,1/6] avcodec/qsv_h2645: fix memory leak for plugin load
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Xu Guangxin Jan. 5, 2021, 2:43 a.m. UTC
---
 libavcodec/qsvdec_h2645.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Anton Khirnov Jan. 27, 2021, 12:44 p.m. UTC | #1
Quoting Xu Guangxin (2021-01-05 03:43:37)
> ---
>  libavcodec/qsvdec_h2645.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/libavcodec/qsvdec_h2645.c b/libavcodec/qsvdec_h2645.c
> index 02c41883b6..3d6e85230f 100644
> --- a/libavcodec/qsvdec_h2645.c
> +++ b/libavcodec/qsvdec_h2645.c
> @@ -69,6 +69,8 @@ static av_cold int qsv_decode_close(AVCodecContext *avctx)
>  {
>      QSVH2645Context *s = avctx->priv_data;
>  
> +    av_freep(&s->qsv.load_plugins);

Does this not get freed by av_opt_free()?
Xiang, Haihao Jan. 29, 2021, 12:37 a.m. UTC | #2
On Wed, 2021-01-27 at 13:44 +0100, Anton Khirnov wrote:
> Quoting Xu Guangxin (2021-01-05 03:43:37)
> > ---
> >  libavcodec/qsvdec_h2645.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/libavcodec/qsvdec_h2645.c b/libavcodec/qsvdec_h2645.c
> > index 02c41883b6..3d6e85230f 100644
> > --- a/libavcodec/qsvdec_h2645.c
> > +++ b/libavcodec/qsvdec_h2645.c
> > @@ -69,6 +69,8 @@ static av_cold int qsv_decode_close(AVCodecContext *avctx)
> >  {
> >      QSVH2645Context *s = avctx->priv_data;
> >  
> > +    av_freep(&s->qsv.load_plugins);
> 
> Does this not get freed by av_opt_free()?
> 

Yes, it gets freed by av_opt_free() when closing the AVCodecContext, we may
remove the above code from qsvdec. 

Thanks
Haihao
Guangxin Xu Jan. 30, 2021, 3:19 a.m. UTC | #3
Hi  Anton, Haihao
If this is the case, we allocated the string in caller, free and reallocate
it in callee.
It's not a good practice.
1. It will make the user confused, The original qsvdec_other.c author and I
are both confused about this.
https://github.com/FFmpeg/FFmpeg/blob/399c1f923574234e899beef72fe249863bd1722a/libavcodec/qsvdec_other.c#L86
2. The av_opt_free may change the design in the future, the new design may
not use av_freep to free the string

Is it possible use  av_opt_set(s, "load_plugin", uid, 0) in qsv_decode_init?
thanks



On Fri, Jan 29, 2021 at 8:37 AM Xiang, Haihao <haihao.xiang@intel.com>
wrote:

> On Wed, 2021-01-27 at 13:44 +0100, Anton Khirnov wrote:
> > Quoting Xu Guangxin (2021-01-05 03:43:37)
> > > ---
> > >  libavcodec/qsvdec_h2645.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/libavcodec/qsvdec_h2645.c b/libavcodec/qsvdec_h2645.c
> > > index 02c41883b6..3d6e85230f 100644
> > > --- a/libavcodec/qsvdec_h2645.c
> > > +++ b/libavcodec/qsvdec_h2645.c
> > > @@ -69,6 +69,8 @@ static av_cold int qsv_decode_close(AVCodecContext
> *avctx)
> > >  {
> > >      QSVH2645Context *s = avctx->priv_data;
> > >
> > > +    av_freep(&s->qsv.load_plugins);
> >
> > Does this not get freed by av_opt_free()?
> >
>
> Yes, it gets freed by av_opt_free() when closing the AVCodecContext, we may
> remove the above code from qsvdec.
>
> Thanks
> Haihao
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Xiang, Haihao Feb. 1, 2021, 1 a.m. UTC | #4
> Hi  Anton, Haihao
> If this is the case, we allocated the string in caller, free and reallocate
> it in callee.
> It's not a good practice.
> 1. It will make the user confused, The original qsvdec_other.c author and I
> are both confused about this.
> 
https://github.com/FFmpeg/FFmpeg/blob/399c1f923574234e899beef72fe249863bd1722a/libavcodec/qsvdec_other.c#L86
> 2. The av_opt_free may change the design in the future, the new design may
> not use av_freep to free the string
> 
> Is it possible use  av_opt_set(s, "load_plugin", uid, 0) in qsv_decode_init?

The type of option 'load_plugin' is AV_OPT_TYPE_INT. Did you mean 'load_plugins'
?  I agree with you that it would be better to use 'av_opt_set(s,
"load_plugins", uid, 0)' to set the value for option 'load_plugins'. 

Thanks
Haihao


> thanks
> 
> 
> 
> On Fri, Jan 29, 2021 at 8:37 AM Xiang, Haihao <haihao.xiang@intel.com>
> wrote:
> 
> > On Wed, 2021-01-27 at 13:44 +0100, Anton Khirnov wrote:
> > > Quoting Xu Guangxin (2021-01-05 03:43:37)
> > > > ---
> > > >  libavcodec/qsvdec_h2645.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > > 
> > > > diff --git a/libavcodec/qsvdec_h2645.c b/libavcodec/qsvdec_h2645.c
> > > > index 02c41883b6..3d6e85230f 100644
> > > > --- a/libavcodec/qsvdec_h2645.c
> > > > +++ b/libavcodec/qsvdec_h2645.c
> > > > @@ -69,6 +69,8 @@ static av_cold int qsv_decode_close(AVCodecContext
> > 
> > *avctx)
> > > >  {
> > > >      QSVH2645Context *s = avctx->priv_data;
> > > > 
> > > > +    av_freep(&s->qsv.load_plugins);
> > > 
> > > Does this not get freed by av_opt_free()?
> > > 
> > 
> > Yes, it gets freed by av_opt_free() when closing the AVCodecContext, we may
> > remove the above code from qsvdec.
> > 
> > Thanks
> > Haihao
> > 
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > 
> > To unsubscribe, visit link above, or email
> > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Anton Khirnov Feb. 1, 2021, 9:57 a.m. UTC | #5
Quoting Guangxin Xu (2021-01-30 04:19:27)
> Hi  Anton, Haihao
> If this is the case, we allocated the string in caller, free and reallocate
> it in callee.
> It's not a good practice.
> 1. It will make the user confused, The original qsvdec_other.c author and I
> are both confused about this.
> https://github.com/FFmpeg/FFmpeg/blob/399c1f923574234e899beef72fe249863bd1722a/libavcodec/qsvdec_other.c#L86

I see no problem with reallocating the string really, as long as av_free
is used it makes no difference. Also note, that all other
AV_OPT_TYPE_STRING options are freed in this manner, so for someone who
understand the AVOption API it is confusing to free just this one
explicitly.

> 2. The av_opt_free may change the design in the future, the new design may
> not use av_freep to free the string

That is very unlikely, as that would be a massive API break. I can think
of no reason to do it.

Also note that extra frees are harmless, other than causing confusion.
diff mbox series

Patch

diff --git a/libavcodec/qsvdec_h2645.c b/libavcodec/qsvdec_h2645.c
index 02c41883b6..3d6e85230f 100644
--- a/libavcodec/qsvdec_h2645.c
+++ b/libavcodec/qsvdec_h2645.c
@@ -69,6 +69,8 @@  static av_cold int qsv_decode_close(AVCodecContext *avctx)
 {
     QSVH2645Context *s = avctx->priv_data;
 
+    av_freep(&s->qsv.load_plugins);
+
     ff_qsv_decode_close(&s->qsv);
 
     qsv_clear_buffers(s);