| 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 | expand |
| 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 |
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()?
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
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".
> 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".
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 --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);