diff mbox series

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

Message ID 20210812023356.8195-1-haihao.xiang@intel.com
State Accepted
Commit 115f5e803502451ac64fa698730923299e5acc4e
Headers show
Series [FFmpeg-devel,v2] lavc/qsvenc: allows the SDK runtime to choose LowPower/non-LowPower modes | 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

Xiang, Haihao Aug. 12, 2021, 2:33 a.m. UTC
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(-)

Comments

Soft Works Aug. 12, 2021, 3:22 a.m. UTC | #1
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Haihao Xiang
> Sent: Thursday, 12 August 2021 04:34
> To: ffmpeg-devel@ffmpeg.org
> Cc: Haihao Xiang <haihao.xiang@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
Xiang, Haihao Aug. 12, 2021, 7:04 a.m. UTC | #2
On Thu, 2021-08-12 at 03:22 +0000, Soft Works wrote:
> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > Haihao Xiang
> > Sent: Thursday, 12 August 2021 04:34
> > To: ffmpeg-devel@ffmpeg.org
> > Cc: Haihao Xiang <haihao.xiang@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. 

Thanks
Haihao

> 
> In general though, the patch makes sense to me!
> 
> softworkz
> 
> 
> _______________________________________________
> 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".
Soft Works Aug. 12, 2021, 8:46 a.m. UTC | #3
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@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-bounces@ffmpeg.org> On Behalf Of
> > > Haihao Xiang
> > > Sent: Thursday, 12 August 2021 04:34
> > > To: ffmpeg-devel@ffmpeg.org
> > > Cc: Haihao Xiang <haihao.xiang@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
James Almer Aug. 13, 2021, 1:26 a.m. UTC | #4
On 8/12/2021 5:46 AM, Soft Works wrote:
> 
> 
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces@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-bounces@ffmpeg.org> On Behalf Of
>>>> Haihao Xiang
>>>> Sent: Thursday, 12 August 2021 04:34
>>>> To: ffmpeg-devel@ffmpeg.org
>>>> Cc: Haihao Xiang <haihao.xiang@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.
diff mbox series

Patch

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},\
 
 extern const AVCodecHWConfigInternal *const ff_qsv_enc_hw_configs[];