| Message ID | 1588065363-7104-2-git-send-email-linjie.fu@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | enhancement for libopenh264 | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
On Tue, 28 Apr 2020, Linjie Fu wrote: > Clip iMinQp/iMaxQp to (1, 51) if user specified qp range > explicitly since QP = 0 is not well supported currently > in libopenh264, otherwise leave iMinQp/iMaxQp untouched > and use the values initialized in FillDefault(). I'd suggest changing the commit message. It's not that "QP = 0 is not well supported". Setting QP=0 means "use the defaults", and that's an intended usecase. It's unfortunate that the library logs a warning message in this case though. Can you make a PR to openh264 to change the verbosity level of that message, from warning to info? > diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c > index dd5d4ee..1b6b53a 100644 > --- a/libavcodec/libopenh264enc.c > +++ b/libavcodec/libopenh264enc.c > @@ -135,6 +135,11 @@ 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 clip to (1, 51) > + if (avctx->qmax >= 0) > + param.iMaxQp = av_clip(avctx->qmax, 1, 51); > + if (avctx->qmin >= 0) > + param.iMinQp = av_clip(avctx->qmin, 1, param.iMaxQp); Same here, I'd suggest simply removing the comment - as it's not a case of "not well supported", but that the value 0 means default. // Martin
> From: Martin Storsjö <martin@martin.st> > Sent: Tuesday, April 28, 2020 18:28 > To: FFmpeg development discussions and patches <ffmpeg- > devel@ffmpeg.org> > Cc: Fu, Linjie <linjie.fu@intel.com> > Subject: Re: [FFmpeg-devel] [PATCH v5 1/5] lavc/libopenh264enc: Add > qmin/qmax support > > On Tue, 28 Apr 2020, Linjie Fu wrote: > > > Clip iMinQp/iMaxQp to (1, 51) if user specified qp range > > explicitly since QP = 0 is not well supported currently > > in libopenh264, otherwise leave iMinQp/iMaxQp untouched > > and use the values initialized in FillDefault(). > > I'd suggest changing the commit message. It's not that "QP = 0 is not well > supported". Setting QP=0 means "use the defaults", and that's an intended > usecase. It's unfortunate that the library logs a warning message in this > case though. > > Can you make a PR to openh264 to change the verbosity level of that > message, from warning to info? PR filed in: https://github.com/cisco/openh264/pull/3278 - Linjie
diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c index dd5d4ee..1b6b53a 100644 --- a/libavcodec/libopenh264enc.c +++ b/libavcodec/libopenh264enc.c @@ -135,6 +135,11 @@ 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 clip to (1, 51) + if (avctx->qmax >= 0) + param.iMaxQp = av_clip(avctx->qmax, 1, 51); + if (avctx->qmin >= 0) + param.iMinQp = av_clip(avctx->qmin, 1, param.iMaxQp); param.iTemporalLayerNum = 1; param.iSpatialLayerNum = 1; param.bEnableDenoise = 0; @@ -331,6 +336,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"), @@ -344,6 +355,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", };
Clip iMinQp/iMaxQp to (1, 51) if user specified qp range explicitly since QP = 0 is not well supported currently in libopenh264, otherwise leave iMinQp/iMaxQp untouched and use the values initialized in FillDefault(). Note that if iMaxQp/iMinQp equals 0, the QP range would be adjusted inside libopenh264 with a warning: Warning:Change QP Range from(0,51) to (12,42) <https://github.com/cisco/openh264/blob/master/codec/encoder/core/src/encoder_ext.cpp#L375> <https://github.com/cisco/openh264/issues/3259> Signed-off-by: Linjie Fu <linjie.fu@intel.com> --- libavcodec/libopenh264enc.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)