[FFmpeg-devel] [PATCH v5 1/5] lavc/libopenh264enc: Add qmin/qmax support

[Martin Storsjö]

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