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

2021-02-01 Thread Anton Khirnov 
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.

-- 
Anton Khirnov
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

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

2021-01-31 Thread Xiang, Haihao 

> 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 
> 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(>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-requ...@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-requ...@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-requ...@ffmpeg.org with subject "unsubscribe".

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

2021-01-29 Thread Guangxin Xu 
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 
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(>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-requ...@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-requ...@ffmpeg.org with subject "unsubscribe".

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

2021-01-28 Thread Xiang, Haihao 
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(>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-requ...@ffmpeg.org with subject "unsubscribe".

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

2021-01-27 Thread Anton Khirnov 
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(>qsv.load_plugins);

Does this not get freed by av_opt_free()?

-- 
Anton Khirnov
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".