| Message ID | 1586171693-7836-2-git-send-email-linjie.fu@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Patch set for the enhancement of libopenh264 encoder | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
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@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? Wouldn't it be better to leave the unset, as is done now?
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > Anton Khirnov > Sent: Friday, April 10, 2020 18:12 > To: FFmpeg development discussions and patches <ffmpeg- > devel@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@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. - Linjie [1] https://github.com/FFmpeg/FFmpeg/blob/master/libavcodec/options_table.h#L97 [2] <https://github.com/cisco/openh264/issues/3259>
Quoting Fu, Linjie (2020-04-10 15:27:02) > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > > Anton Khirnov > > Sent: Friday, April 10, 2020 18:12 > > To: FFmpeg development discussions and patches <ffmpeg- > > devel@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@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
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; param.iTemporalLayerNum = 1; param.iSpatialLayerNum = 1; param.bEnableDenoise = 0; @@ -332,6 +335,12 @@ static int svc_encode_frame(AVCodecContext *avctx, AVPacket *avpkt, return 0; } +static const AVCodecDefault svc_enc_defaults[] = { + { "qmin", "-1" }, + { "qmax", "-1" }, + { NULL }, +}; + AVCodec ff_libopenh264_encoder = { .name = "libopenh264", .long_name = NULL_IF_CONFIG_SMALL("OpenH264 H.264 / AVC / MPEG-4 AVC / MPEG-4 part 10"), @@ -345,6 +354,7 @@ AVCodec ff_libopenh264_encoder = { .caps_internal = FF_CODEC_CAP_INIT_THREADSAFE | FF_CODEC_CAP_INIT_CLEANUP, .pix_fmts = (const enum AVPixelFormat[]){ AV_PIX_FMT_YUV420P, AV_PIX_FMT_NONE }, + .defaults = svc_enc_defaults, .priv_class = &class, .wrapper_name = "libopenh264", };
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@intel.com> --- libavcodec/libopenh264enc.c | 10 ++++++++++ 1 file changed, 10 insertions(+)