diff mbox series

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

Message ID 1588065363-7104-2-git-send-email-linjie.fu@intel.com
State Superseded
Headers show
Series enhancement for libopenh264
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Fu, Linjie April 28, 2020, 9:15 a.m. UTC
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(+)

Comments

Martin Storsjö April 28, 2020, 10:27 a.m. UTC | #1
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
Fu, Linjie April 28, 2020, 4:48 p.m. UTC | #2
> 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 mbox series

Patch

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",
 };