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.
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-requ...@ffmpeg.org with subject "unsubscribe".
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
