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

[James Almer]

On 8/12/2021 5:46 AM, Soft Works wrote:
> 
> 
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
>> Xiang, Haihao
>> Sent: Thursday, 12 August 2021 09:05
>> To: ffmpeg-devel at 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-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.
>>
>> 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.
> 
> I'd let others comment, I don't know what the intended way is for
> such cases.
> In many cases it's not clear what options mean, especially such cases
> like "Auto", while options like the following are much clearer
> and easier to understand.
> 
> -dash_segment_type <int>   E..... set dash segment files type (from 0 to 2) (default auto)
>     auto            0       E..... select segment file format based on codec
>     mp4             1       E..... make segment file in ISOBMFF format
>     webm            2       E..... make segment file in WebM format
> 
> 
> This way, there's also more room for better explanation.
> 
> The information you gave..:
> 
> 
> The SDK supports LowPower and non-LowPower modes, but some features
> Are available only under one of the two modes.
> [If] mode is set to unknown, the SDK is able to choose a workable
> mode for the specified features.
> 
> ..is very valuable. Even I didn't know that and AFAIR, the SDK
> manual doesn’t tell much about it.
> 
> So many times did I need to look at the source code to understand
> how certain options work, because it wasn't clear from the docs
> or from help output.
> 
> As you are Intel, you might want to present your features in the
> best possible way ;-)
> 
> It's surely a cosmetical issue and would make the most sense when
> applied more parameters than just this one, so
> 
> LGTM.
> 
> softworkz

Pushed.