[FFmpeg-devel] [PATCH 01/10] lavc/libopenh264enc: Add default qmin/qmax support

[Anton Khirnov]

Quoting Fu, Linjie (2020-04-10 15:27:02)
> > From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> > Anton Khirnov
> > Sent: Friday, April 10, 2020 18:12
> > To: FFmpeg development discussions and patches <ffmpeg-
> > devel at ffmpeg.org>
> > Subject: Re: [FFmpeg-devel] [PATCH 01/10] lavc/libopenh264enc: Add
> > default qmin/qmax support
> > 
> > Quoting Linjie Fu (2020-04-06 13:14:44)
> > > Set default QP range to (1, 51) instead of (2, 32).
> > >
> > > QP = 0 is not well supported currently in libopenh264. If iMaxQp/iMinQp
> > > equals 0, the QP range would be changed unexpectedly inside libopenh264
> > > with a warning:
> > >
> > > Warning:Change QP Range from(0,51) to (12,42)
> > >
> > > [1]
> > <https://github.com/cisco/openh264/blob/master/codec/encoder/core/src/
> > encoder_ext.cpp#L375>
> > > [2] <https://github.com/cisco/openh264/issues/3259>
> > >
> > > Signed-off-by: Linjie Fu <linjie.fu at intel.com>
> > > ---
> > >  libavcodec/libopenh264enc.c | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > >
> > > diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c
> > > index ae6d17c..364926f 100644
> > > --- a/libavcodec/libopenh264enc.c
> > > +++ b/libavcodec/libopenh264enc.c
> > > @@ -135,6 +135,9 @@ FF_ENABLE_DEPRECATION_WARNINGS
> > >      param.iTargetBitrate             = avctx->bit_rate;
> > >      param.iMaxBitrate                = FFMAX(avctx->rc_max_rate, avctx-
> > >bit_rate);
> > >      param.iRCMode                    = RC_QUALITY_MODE;
> > > +    // QP = 0 is not well supported, so default to (1, 51)
> > > +    param.iMaxQp                     = avctx->qmax >= 0 ? av_clip(avctx->qmax, 1,
> > 51) : 51;
> > > +    param.iMinQp                     = avctx->qmin >= 0 ? av_clip(avctx->qmin, 1,
> > param.iMaxQp) : 1;
> > 
> > Should we set them at all if they are not specified by the user?
> 
> The answer is no and that's why I set qmin/qmax to -1 by default, otherwise
> they would be set to (2, 31) by default [1] in VBR mode in FFmpeg.
> 
> > Wouldn't it be better to leave the unset, as is done now?
> 
> Most encoders would prefer to get a default QP range specifically if it's not
> explicitly set by user, either choosing a default value inside ffmpeg or leave it
> unset to be determined in core library/driver, like libx264, vaapi, qsv, nvenc.
> 
> For libopenh264enc specifically, the supported range is (1, 51), qp = 0 is not well
> Supported yet in libopenh264 library [2]. If either of qmin or qmax equals 0, the
> qp range would be reduced to (12, 42) inside libopenh264 library.
> 
> The default QP range is supposed to be as wide as enough IMHO, so set it to (1, 51)
> and avoid the warning inside the library. (and MaxQp =51 is the recommended value
> in the demo config file in ..../openh264/testbin/welsenc.cfg)
> 
> Thanks for the review.

ok then

-- 
Anton Khirnov