diff mbox series

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

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
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Fu, Linjie April 6, 2020, 11:14 a.m. UTC
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(+)

Comments

Anton Khirnov April 10, 2020, 10:12 a.m. UTC | #1
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?
Fu, Linjie April 10, 2020, 1:27 p.m. UTC | #2
> 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>
Anton Khirnov April 14, 2020, 11:02 a.m. UTC | #3
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 mbox series

Patch

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