[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
> 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