diff mbox series

[FFmpeg-devel,v2,6/6] avcodec/qsvdec: refact, remove duplicate code for plugin loading

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

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.c | 29 +++++++++++------------------
 1 file changed, 11 insertions(+), 18 deletions(-)

Comments

Linjie Fu Jan. 16, 2021, 7 a.m. UTC | #1 
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
Guangxin Xu Jan. 17, 2021, 2:17 a.m. UTC | #2 
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 mbox series

Patch

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));