-----Original Message-----
From: ffmpeg-devel <ffmpeg-devel-boun...@ffmpeg.org> On Behalf Of
Xiang, Haihao
Sent: Thursday, 12 August 2021 09:05
To: ffmpeg-devel@ffmpeg.org
Subject: Re: [FFmpeg-devel] [PATCH v2] lavc/qsvenc: allows the SDK
runtime to choose LowPower/non-LowPower modes
On Thu, 2021-08-12 at 03:22 +0000, Soft Works wrote:
-----Original Message-----
From: ffmpeg-devel <ffmpeg-devel-boun...@ffmpeg.org> On Behalf Of
Haihao Xiang
Sent: Thursday, 12 August 2021 04:34
To: ffmpeg-devel@ffmpeg.org
Cc: Haihao Xiang <haihao.xi...@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.
According to
https://github.com/FFmpeg/FFmpeg/blob/master/libavutil/opt.c#L364-
L393, -1 is
taken as 'auto', 1 is taken as 'on', 'true', ..., 0 is taken as
'off', 'false',
...
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).
If so, we will have to create constants for
'true,y,yes,enable,enabled,on,
false,n,no,disable,disabled,off' for compatibility. I may update it
if you still
prefer this way.
