[FFmpeg-devel] lavc/qsvenc: expose low_power option in H264 QSV

Submitted by Linjie Fu on March 25, 2019, 1:32 p.m.

Details

Message ID 20190325133224.11493-1-linjie.fu@intel.com
State New
Headers show

Commit Message

Linjie Fu March 25, 2019, 1:32 p.m.
Always exposes low_power option for h264 qsv, and reports a warning
if VDENC is not supported with current version of MSDK.

Signed-off-by: Linjie Fu <linjie.fu@intel.com>
---
 libavcodec/qsvenc.c      | 11 ++++++++++-
 libavcodec/qsvenc_h264.c |  2 --
 2 files changed, 10 insertions(+), 3 deletions(-)

Comments

Zhong Li March 26, 2019, 4:01 a.m.
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf

> Of Linjie Fu

> Sent: Monday, March 25, 2019 9:32 PM

> To: ffmpeg-devel@ffmpeg.org

> Cc: Fu, Linjie <linjie.fu@intel.com>

> Subject: [FFmpeg-devel] [PATCH] lavc/qsvenc: expose low_power option in

> H264 QSV

> 

> Always exposes low_power option for h264 qsv, and reports a warning if

> VDENC is not supported with current version of MSDK.

> 

> Signed-off-by: Linjie Fu <linjie.fu@intel.com>

> ---

>  libavcodec/qsvenc.c      | 11 ++++++++++-

>  libavcodec/qsvenc_h264.c |  2 --

>  2 files changed, 10 insertions(+), 3 deletions(-)

> 

> diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c index

> 5aa020d47b..8264d0a413 100644

> --- a/libavcodec/qsvenc.c

> +++ b/libavcodec/qsvenc.c

> @@ -495,9 +495,18 @@ static int init_video_param(AVCodecContext *avctx,

> QSVEncContext *q)

>          }

>      }

> 

> +    if (q->low_power) {

>  #if QSV_HAVE_VDENC

> -    q->param.mfx.LowPower           = q->low_power ?

> MFX_CODINGOPTION_ON : MFX_CODINGOPTION_OFF;

> +        q->param.mfx.LowPower = MFX_CODINGOPTION_ON; #else

> +        av_log(avctx, AV_LOG_WARNING, "The QSV VDENC option is "

> +                            "not supported with this MSDK

> version.\n");


VDEnc is driver and hardware scope should not be exposed in FFmpeg level. 
Lower power is easier for user to understand, and it is the option user set. So I believe the would be better to change the log to be
"low power mode is not supported ..."

> +        q->low_power = 0;

> +        q->param.mfx.LowPower = MFX_CODINGOPTION_OFF;

>  #endif

> +    } else

> +        q->param.mfx.LowPower = MFX_CODINGOPTION_OFF;


The rest LGTM
Linjie Fu March 26, 2019, 5:45 a.m.
> -----Original Message-----

> From: Li, Zhong

> Sent: Tuesday, March 26, 2019 12:02

> To: FFmpeg development discussions and patches <ffmpeg-

> devel@ffmpeg.org>

> Cc: Fu, Linjie <linjie.fu@intel.com>

> Subject: RE: [FFmpeg-devel] [PATCH] lavc/qsvenc: expose low_power option

> in H264 QSV

> 

> > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On

> Behalf

> > Of Linjie Fu

> > Sent: Monday, March 25, 2019 9:32 PM

> > To: ffmpeg-devel@ffmpeg.org

> > Cc: Fu, Linjie <linjie.fu@intel.com>

> > Subject: [FFmpeg-devel] [PATCH] lavc/qsvenc: expose low_power option

> in

> > H264 QSV

> >

> > Always exposes low_power option for h264 qsv, and reports a warning if

> > VDENC is not supported with current version of MSDK.

> >

> > Signed-off-by: Linjie Fu <linjie.fu@intel.com>

> > ---

> >  libavcodec/qsvenc.c      | 11 ++++++++++-

> >  libavcodec/qsvenc_h264.c |  2 --

> >  2 files changed, 10 insertions(+), 3 deletions(-)

> >

> > diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c index

> > 5aa020d47b..8264d0a413 100644

> > --- a/libavcodec/qsvenc.c

> > +++ b/libavcodec/qsvenc.c

> > @@ -495,9 +495,18 @@ static int init_video_param(AVCodecContext

> *avctx,

> > QSVEncContext *q)

> >          }

> >      }

> >

> > +    if (q->low_power) {

> >  #if QSV_HAVE_VDENC

> > -    q->param.mfx.LowPower           = q->low_power ?

> > MFX_CODINGOPTION_ON : MFX_CODINGOPTION_OFF;

> > +        q->param.mfx.LowPower = MFX_CODINGOPTION_ON; #else

> > +        av_log(avctx, AV_LOG_WARNING, "The QSV VDENC option is "

> > +                            "not supported with this MSDK

> > version.\n");

> 

> VDEnc is driver and hardware scope should not be exposed in FFmpeg level.

> Lower power is easier for user to understand, and it is the option user set. So

> I believe the would be better to change the log to be

> "low power mode is not supported ..."

> 

It's true "low power" is better, will update.
> > +        q->low_power = 0;

> > +        q->param.mfx.LowPower = MFX_CODINGOPTION_OFF;

> >  #endif

> > +    } else

> > +        q->param.mfx.LowPower = MFX_CODINGOPTION_OFF;

> 

> The rest LGTM

Patch hide | download patch | download mbox

diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c
index 5aa020d47b..8264d0a413 100644
--- a/libavcodec/qsvenc.c
+++ b/libavcodec/qsvenc.c
@@ -495,9 +495,18 @@  static int init_video_param(AVCodecContext *avctx, QSVEncContext *q)
         }
     }
 
+    if (q->low_power) {
 #if QSV_HAVE_VDENC
-    q->param.mfx.LowPower           = q->low_power ? MFX_CODINGOPTION_ON : MFX_CODINGOPTION_OFF;
+        q->param.mfx.LowPower = MFX_CODINGOPTION_ON;
+#else
+        av_log(avctx, AV_LOG_WARNING, "The QSV VDENC option is "
+                            "not supported with this MSDK version.\n");
+        q->low_power = 0;
+        q->param.mfx.LowPower = MFX_CODINGOPTION_OFF;
 #endif
+    } else
+        q->param.mfx.LowPower = MFX_CODINGOPTION_OFF;
+
     q->param.mfx.CodecProfile       = q->profile;
     q->param.mfx.TargetUsage        = avctx->compression_level;
     q->param.mfx.GopPicSize         = FFMAX(0, avctx->gop_size);
diff --git a/libavcodec/qsvenc_h264.c b/libavcodec/qsvenc_h264.c
index f458137848..93044a58c9 100644
--- a/libavcodec/qsvenc_h264.c
+++ b/libavcodec/qsvenc_h264.c
@@ -154,9 +154,7 @@  static const AVOption options[] = {
     { "auto"   , NULL, 0, AV_OPT_TYPE_CONST, { .i64 = MFX_MF_AUTO     }, INT_MIN, INT_MAX,     VE, "mfmode" },
 #endif
 
-#if QSV_HAVE_VDENC
     { "low_power", "enable low power mode(experimental: many limitations by mfx version, BRC modes, etc.)", OFFSET(qsv.low_power), AV_OPT_TYPE_BOOL, { .i64 =  0 }, 0, 1, VE},
-#endif
 
     { "repeat_pps", "repeat pps for every frame", OFFSET(qsv.repeat_pps), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, VE },