[FFmpeg-devel] [PATCH v2] lavc/qsvenc: allows the SDK runtime to choose LowPower/non-LowPower modes

[Soft Works]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> Haihao Xiang
> Sent: Thursday, 12 August 2021 04:34
> To: ffmpeg-devel at ffmpeg.org
> Cc: Haihao Xiang <haihao.xiang at intel.com>
> Subject: [FFmpeg-devel] [PATCH v2] lavc/qsvenc: allows the SDK
> runtime to choose LowPower/non-LowPower modes
> 
> The SDK supports LowPower and non-LowPower modes, but some features
> are
> available only under one of the two modes. Currently non-LowPower
> mode
> is always chosen in FFmpeg if the mode is not set to LowPower
> explicitly. User will experience some SDK errors if a LowPower
> related
> feature is specified but the mode is not set to LowPower. With this
> patch, the mode is set to unknown by default in FFmpeg, the SDK is
> able
> to choose a workable mode for the specified features.
> ---
> v2: update commit log and rebase the patch against the current master
> 
>  libavcodec/qsvenc.c | 6 ++++--
>  libavcodec/qsvenc.h | 2 +-
>  2 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c
> index e7e65ebfcf..50ec7065ca 100644
> --- a/libavcodec/qsvenc.c
> +++ b/libavcodec/qsvenc.c
> @@ -514,7 +514,7 @@ static int init_video_param(AVCodecContext
> *avctx, QSVEncContext *q)
>          }
>      }
> 
> -    if (q->low_power) {
> +    if (q->low_power == 1) {
>  #if QSV_HAVE_VDENC
>          q->param.mfx.LowPower = MFX_CODINGOPTION_ON;
>  #else
> @@ -523,7 +523,9 @@ static int init_video_param(AVCodecContext
> *avctx, QSVEncContext *q)
>          q->low_power = 0;
>          q->param.mfx.LowPower = MFX_CODINGOPTION_OFF;
>  #endif
> -    } else
> +    } else if (q->low_power == -1)
> +        q->param.mfx.LowPower = MFX_CODINGOPTION_UNKNOWN;
> +    else
>          q->param.mfx.LowPower = MFX_CODINGOPTION_OFF;
> 
>      q->param.mfx.CodecProfile       = q->profile;
> diff --git a/libavcodec/qsvenc.h b/libavcodec/qsvenc.h
> index ba20b6f906..31516b8e55 100644
> --- a/libavcodec/qsvenc.h
> +++ b/libavcodec/qsvenc.h
> @@ -96,7 +96,7 @@
>  { "adaptive_b",     "Adaptive B-frame placement",
> OFFSET(qsv.adaptive_b),     AV_OPT_TYPE_INT, { .i64 = -1 }, -1,
> 1, VE },                         \
>  { "b_strategy",     "Strategy to choose between I/P/B-frames",
> OFFSET(qsv.b_strategy),    AV_OPT_TYPE_INT, { .i64 = -1 }, -1,
> 1, VE },                         \
>  { "forced_idr",     "Forcing I frames as IDR frames",
> OFFSET(qsv.forced_idr),     AV_OPT_TYPE_BOOL,{ .i64 = 0  },  0,
> 1, VE },                         \
> -{ "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},\
> +{ "low_power", "enable low power mode(experimental: many limitations
> by mfx version, BRC modes, etc.)", OFFSET(qsv.low_power),
> AV_OPT_TYPE_BOOL, { .i64 = -1}, -1, 1, VE},\

I don't know what the "official" guideline is for AV_OPT_TYPE_BOOL,
but IMO it is confusing to have a tristate logic, especially when 
it is unclear what happens in each of the three cases.

I've seen that there are a few cases in all ffmpeg where it is
done like that, but in most cases it is unclear what happens 
with each of the three values and in many cases, there are 
only 2 effective values anyway.
And even inconsistent: In some cases, -1 and 1 are both taken 
for true, in other cases it is checked for x < 1 and sometimes
x >= 0.

IMO, that's a pattern that shouldn't be adopted. An INTEGER option
(still with -1, 0 and 1) with three named option values and an 
indication what the default is, would be a nicer way - and still
compatible.
(for all other options as well in a later patch). 

In general though, the patch makes sense to me!

softworkz