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

[Xiang, Haihao]

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. 

Thanks
Haihao

> 
> In general though, the patch makes sense to me!
> 
> softworkz
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".