Message ID | 20210105024342.85970-6-guangxin.xu@intel.com |
---|---|
State | Accepted |
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 |
Guangxin: On Tue, Jan 5, 2021 at 10:44 AM Xu Guangxin <guangxin.xu@intel.com> wrote: > > --- > libavcodec/qsvdec.c | 29 +++++++++++------------------ > 1 file changed, 11 insertions(+), 18 deletions(-) > > diff --git a/libavcodec/qsvdec.c b/libavcodec/qsvdec.c > index 3ca16dafae..d10f90a0db 100644 > --- a/libavcodec/qsvdec.c > +++ b/libavcodec/qsvdec.c > @@ -682,21 +682,12 @@ static av_cold int qsv_decode_init(AVCodecContext *avctx) > { > QSVDecContext *s = avctx->priv_data; > int ret; > + const char *uid = NULL; > > if (avctx->codec_id == AV_CODEC_ID_VP8) { > - static const char *uid_vp8dec_hw = "f622394d8d87452f878c51f2fc9b4131"; > - > - av_freep(&s->qsv.load_plugins); > - s->qsv.load_plugins = av_strdup(uid_vp8dec_hw); > - if (!s->qsv.load_plugins) > - return AVERROR(ENOMEM); > + uid = "f622394d8d87452f878c51f2fc9b4131"; > } else if (avctx->codec_id == AV_CODEC_ID_VP9) { > - static const char *uid_vp9dec_hw = "a922394d8d87452f878c51f2fc9b4131"; > - > - av_freep(&s->qsv.load_plugins); > - s->qsv.load_plugins = av_strdup(uid_vp9dec_hw); > - if (!s->qsv.load_plugins) > - return AVERROR(ENOMEM); > + uid = "a922394d8d87452f878c51f2fc9b4131"; > } > else if (avctx->codec_id == AV_CODEC_ID_HEVC && s->load_plugin != LOAD_PLUGIN_NONE) { > static const char * const uid_hevcdec_sw = "15dd936825ad475ea34e35f3f54217a6"; > @@ -707,16 +698,18 @@ static av_cold int qsv_decode_init(AVCodecContext *avctx) > "load_plugins is not empty, but load_plugin is not set to 'none'." > "The load_plugin value will be ignored.\n"); > } else { > - av_freep(&s->qsv.load_plugins); > - > if (s->load_plugin == LOAD_PLUGIN_HEVC_SW) > - s->qsv.load_plugins = av_strdup(uid_hevcdec_sw); > + uid = uid_hevcdec_sw; > else > - s->qsv.load_plugins = av_strdup(uid_hevcdec_hw); > - if (!s->qsv.load_plugins) > - return AVERROR(ENOMEM); > + uid = uid_hevcdec_hw; > } > } > + if (uid) { > + av_freep(&s->qsv.load_plugins); > + s->qsv.load_plugins = av_strdup(uid); > + if (!s->qsv.load_plugins) > + return AVERROR(ENOMEM); > + } > > s->qsv.orig_pix_fmt = AV_PIX_FMT_NV12; > s->packet_fifo = av_fifo_alloc(sizeof(AVPacket)); > -- Merging the AVCodec descriptions for all qsv decoders makes the code cleaner, since the majority of them are identical. And all checks passed in [1]. One concern is it may be less convenient or more tricky to modify in the future, if a specific decoder changes and differs from the rest. Anyway, lgtm at least for now, and prefer to apply if no more comments/objections/concerns. [1] https://github.com/intel-media-ci/ffmpeg/pull/326 - linjie
Hi Lingjie, thanks for the comment. We can use codec id to do special things for a specific codec. Just like qsv_decode_init. thanks On Sat, Jan 16, 2021 at 3:22 PM Linjie Fu <linjie.justin.fu@gmail.com> wrote: > Guangxin: > > On Tue, Jan 5, 2021 at 10:44 AM Xu Guangxin <guangxin.xu@intel.com> wrote: > > > > --- > > libavcodec/qsvdec.c | 29 +++++++++++------------------ > > 1 file changed, 11 insertions(+), 18 deletions(-) > > > > diff --git a/libavcodec/qsvdec.c b/libavcodec/qsvdec.c > > index 3ca16dafae..d10f90a0db 100644 > > --- a/libavcodec/qsvdec.c > > +++ b/libavcodec/qsvdec.c > > @@ -682,21 +682,12 @@ static av_cold int qsv_decode_init(AVCodecContext > *avctx) > > { > > QSVDecContext *s = avctx->priv_data; > > int ret; > > + const char *uid = NULL; > > > > if (avctx->codec_id == AV_CODEC_ID_VP8) { > > - static const char *uid_vp8dec_hw = > "f622394d8d87452f878c51f2fc9b4131"; > > - > > - av_freep(&s->qsv.load_plugins); > > - s->qsv.load_plugins = av_strdup(uid_vp8dec_hw); > > - if (!s->qsv.load_plugins) > > - return AVERROR(ENOMEM); > > + uid = "f622394d8d87452f878c51f2fc9b4131"; > > } else if (avctx->codec_id == AV_CODEC_ID_VP9) { > > - static const char *uid_vp9dec_hw = > "a922394d8d87452f878c51f2fc9b4131"; > > - > > - av_freep(&s->qsv.load_plugins); > > - s->qsv.load_plugins = av_strdup(uid_vp9dec_hw); > > - if (!s->qsv.load_plugins) > > - return AVERROR(ENOMEM); > > + uid = "a922394d8d87452f878c51f2fc9b4131"; > > } > > else if (avctx->codec_id == AV_CODEC_ID_HEVC && s->load_plugin != > LOAD_PLUGIN_NONE) { > > static const char * const uid_hevcdec_sw = > "15dd936825ad475ea34e35f3f54217a6"; > > @@ -707,16 +698,18 @@ static av_cold int qsv_decode_init(AVCodecContext > *avctx) > > "load_plugins is not empty, but load_plugin is not > set to 'none'." > > "The load_plugin value will be ignored.\n"); > > } else { > > - av_freep(&s->qsv.load_plugins); > > - > > if (s->load_plugin == LOAD_PLUGIN_HEVC_SW) > > - s->qsv.load_plugins = av_strdup(uid_hevcdec_sw); > > + uid = uid_hevcdec_sw; > > else > > - s->qsv.load_plugins = av_strdup(uid_hevcdec_hw); > > - if (!s->qsv.load_plugins) > > - return AVERROR(ENOMEM); > > + uid = uid_hevcdec_hw; > > } > > } > > + if (uid) { > > + av_freep(&s->qsv.load_plugins); > > + s->qsv.load_plugins = av_strdup(uid); > > + if (!s->qsv.load_plugins) > > + return AVERROR(ENOMEM); > > + } > > > > s->qsv.orig_pix_fmt = AV_PIX_FMT_NV12; > > s->packet_fifo = av_fifo_alloc(sizeof(AVPacket)); > > -- > Merging the AVCodec descriptions for all qsv decoders makes the code > cleaner, since > the majority of them are identical. And all checks passed in [1]. > > One concern is it may be less convenient or more tricky to modify in > the future, if a > specific decoder changes and differs from the rest. > > Anyway, lgtm at least for now, and prefer to apply if no more > comments/objections/concerns. > > [1] https://github.com/intel-media-ci/ffmpeg/pull/326 > > - linjie > _______________________________________________ > 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".
diff --git a/libavcodec/qsvdec.c b/libavcodec/qsvdec.c index 3ca16dafae..d10f90a0db 100644 --- a/libavcodec/qsvdec.c +++ b/libavcodec/qsvdec.c @@ -682,21 +682,12 @@ static av_cold int qsv_decode_init(AVCodecContext *avctx) { QSVDecContext *s = avctx->priv_data; int ret; + const char *uid = NULL; if (avctx->codec_id == AV_CODEC_ID_VP8) { - static const char *uid_vp8dec_hw = "f622394d8d87452f878c51f2fc9b4131"; - - av_freep(&s->qsv.load_plugins); - s->qsv.load_plugins = av_strdup(uid_vp8dec_hw); - if (!s->qsv.load_plugins) - return AVERROR(ENOMEM); + uid = "f622394d8d87452f878c51f2fc9b4131"; } else if (avctx->codec_id == AV_CODEC_ID_VP9) { - static const char *uid_vp9dec_hw = "a922394d8d87452f878c51f2fc9b4131"; - - av_freep(&s->qsv.load_plugins); - s->qsv.load_plugins = av_strdup(uid_vp9dec_hw); - if (!s->qsv.load_plugins) - return AVERROR(ENOMEM); + uid = "a922394d8d87452f878c51f2fc9b4131"; } else if (avctx->codec_id == AV_CODEC_ID_HEVC && s->load_plugin != LOAD_PLUGIN_NONE) { static const char * const uid_hevcdec_sw = "15dd936825ad475ea34e35f3f54217a6"; @@ -707,16 +698,18 @@ static av_cold int qsv_decode_init(AVCodecContext *avctx) "load_plugins is not empty, but load_plugin is not set to 'none'." "The load_plugin value will be ignored.\n"); } else { - av_freep(&s->qsv.load_plugins); - if (s->load_plugin == LOAD_PLUGIN_HEVC_SW) - s->qsv.load_plugins = av_strdup(uid_hevcdec_sw); + uid = uid_hevcdec_sw; else - s->qsv.load_plugins = av_strdup(uid_hevcdec_hw); - if (!s->qsv.load_plugins) - return AVERROR(ENOMEM); + uid = uid_hevcdec_hw; } } + if (uid) { + av_freep(&s->qsv.load_plugins); + s->qsv.load_plugins = av_strdup(uid); + if (!s->qsv.load_plugins) + return AVERROR(ENOMEM); + } s->qsv.orig_pix_fmt = AV_PIX_FMT_NV12; s->packet_fifo = av_fifo_alloc(sizeof(AVPacket));