diff mbox series

[FFmpeg-devel,v4,1/9] lavc/libopenh264enc: Add default qmin/qmax support

Message ID 1586926427-20170-2-git-send-email-linjie.fu@intel.com
State New
Headers show
Series Enhancement for libopenh264 encoder
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 15, 2020, 4:53 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

Martin Storsjö April 27, 2020, 7:26 p.m. UTC | #1
On Wed, 15 Apr 2020, Linjie Fu wrote:

> 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 dd5d4ee..c7ae5b1 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;

If qmax/qmin < 0, i.e. wasn't specified by the user, wouldn't it be better 
to not touch param.iMax/MinQp at all (and use the default value of the 
library, which may change between versions), instead of overriding it with 
a value hardcoded here?

// Martin
Fu, Linjie April 28, 2020, 5:58 a.m. UTC | #2
> From: Martin Storsjö <martin@martin.st>
> Sent: Tuesday, April 28, 2020 03:27
> 
> If qmax/qmin < 0, i.e. wasn't specified by the user, wouldn't it be better
> to not touch param.iMax/MinQp at all (and use the default value of the
> library, which may change between versions), instead of overriding it with
> a value hardcoded here?
> 
Okay, this seems more natural if the recommended QP range varies between
versions, though one of my original purposes is to avoid the warning in default
situation for changing the QP inside libopenh264 library.

Updated locally as suggested, and thanks for the review for the whole set.

- Linjie
Martin Storsjö April 28, 2020, 6:18 a.m. UTC | #3
On Tue, 28 Apr 2020, Fu, Linjie wrote:

>> From: Martin Storsjö <martin@martin.st>
>> Sent: Tuesday, April 28, 2020 03:27
>>
>> If qmax/qmin < 0, i.e. wasn't specified by the user, wouldn't it be better
>> to not touch param.iMax/MinQp at all (and use the default value of the
>> library, which may change between versions), instead of overriding it with
>> a value hardcoded here?
>>
> Okay, this seems more natural if the recommended QP range varies between
> versions, though one of my original purposes is to avoid the warning in default
> situation for changing the QP inside libopenh264 library.

Well in general I'd want to avoid hardcoding opinionated defaults within 
our own wrapper - I'd like it to behave as close to what upstream 
intended, so that whatever issues we see with defaults, are the same 
issues that everyone else sees as well, so any fixes to those defaults 
upstream also end up for us - so we don't get stuck on whatever we thought 
was a good default at some point.

What warnings about changing QP are you referring to?

// Martin
Fu, Linjie April 28, 2020, 6:26 a.m. UTC | #4
> From: Martin Storsjö <martin@martin.st>
> Sent: Tuesday, April 28, 2020 14:19
> To: Fu, Linjie <linjie.fu@intel.com>
> Cc: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: RE: [FFmpeg-devel] [PATCH v4 1/9] lavc/libopenh264enc: Add
> default qmin/qmax support
> 
> On Tue, 28 Apr 2020, Fu, Linjie wrote:
> 
> >> From: Martin Storsjö <martin@martin.st>
> >> Sent: Tuesday, April 28, 2020 03:27
> >>
> >> If qmax/qmin < 0, i.e. wasn't specified by the user, wouldn't it be better
> >> to not touch param.iMax/MinQp at all (and use the default value of the
> >> library, which may change between versions), instead of overriding it with
> >> a value hardcoded here?
> >>
> > Okay, this seems more natural if the recommended QP range varies
> between
> > versions, though one of my original purposes is to avoid the warning in
> default
> > situation for changing the QP inside libopenh264 library.
> 
> Well in general I'd want to avoid hardcoding opinionated defaults within
> our own wrapper - I'd like it to behave as close to what upstream
> intended, so that whatever issues we see with defaults, are the same
> issues that everyone else sees as well, so any fixes to those defaults
> upstream also end up for us - so we don't get stuck on whatever we thought
> was a good default at some point.

Agree, this makes more sense.
 
> What warnings about changing QP are you referring to?
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 

The main reason is that "0" is not supported well, which is the default value of iMinQp.

- Linjie
Martin Storsjö April 28, 2020, 6:30 a.m. UTC | #5
On Tue, 28 Apr 2020, Fu, Linjie wrote:

>> From: Martin Storsjö <martin@martin.st>
>> Sent: Tuesday, April 28, 2020 14:19
>> To: Fu, Linjie <linjie.fu@intel.com>
>> Cc: FFmpeg development discussions and patches <ffmpeg-
>> devel@ffmpeg.org>
>> Subject: RE: [FFmpeg-devel] [PATCH v4 1/9] lavc/libopenh264enc: Add
>> default qmin/qmax support
>>
>> On Tue, 28 Apr 2020, Fu, Linjie wrote:
>>
>>>> From: Martin Storsjö <martin@martin.st>
>>>> Sent: Tuesday, April 28, 2020 03:27
>>>>
>>>> If qmax/qmin < 0, i.e. wasn't specified by the user, wouldn't it be better
>>>> to not touch param.iMax/MinQp at all (and use the default value of the
>>>> library, which may change between versions), instead of overriding it with
>>>> a value hardcoded here?
>>>>
>>> Okay, this seems more natural if the recommended QP range varies
>> between
>>> versions, though one of my original purposes is to avoid the warning in
>> default
>>> situation for changing the QP inside libopenh264 library.
>>
>> Well in general I'd want to avoid hardcoding opinionated defaults within
>> our own wrapper - I'd like it to behave as close to what upstream
>> intended, so that whatever issues we see with defaults, are the same
>> issues that everyone else sees as well, so any fixes to those defaults
>> upstream also end up for us - so we don't get stuck on whatever we thought
>> was a good default at some point.
>
> Agree, this makes more sense.
>
>> What warnings about changing QP are you referring to?
> 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
>
> The main reason is that "0" is not supported well, which is the default 
> value of iMinQp.

Ah, right.

In that case, setting some other default might make sense indeed. 
How/where does OpenH264 set suitable values for this, in order not to get 
the warnings everywhere? Are the values e.g. 12-42 hardcoded everywhere in 
every caller?

// Martin
Fu, Linjie April 28, 2020, 6:45 a.m. UTC | #6
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Martin Storsjö
> Sent: Tuesday, April 28, 2020 14:31
> To: Fu, Linjie <linjie.fu@intel.com>
> Cc: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH v4 1/9] lavc/libopenh264enc: Add
> default qmin/qmax support
> 
> On Tue, 28 Apr 2020, Fu, Linjie wrote:
> 
> >> From: Martin Storsjö <martin@martin.st>
> >> Sent: Tuesday, April 28, 2020 14:19
> >> To: Fu, Linjie <linjie.fu@intel.com>
> >> Cc: FFmpeg development discussions and patches <ffmpeg-
> >> devel@ffmpeg.org>
> >> Subject: RE: [FFmpeg-devel] [PATCH v4 1/9] lavc/libopenh264enc: Add
> >> default qmin/qmax support
> >>
> >> On Tue, 28 Apr 2020, Fu, Linjie wrote:
> >>
> >>>> From: Martin Storsjö <martin@martin.st>
> >>>> Sent: Tuesday, April 28, 2020 03:27
> >>>>
> >>>> If qmax/qmin < 0, i.e. wasn't specified by the user, wouldn't it be
> better
> >>>> to not touch param.iMax/MinQp at all (and use the default value of the
> >>>> library, which may change between versions), instead of overriding it
> with
> >>>> a value hardcoded here?
> >>>>
> >>> Okay, this seems more natural if the recommended QP range varies
> >> between
> >>> versions, though one of my original purposes is to avoid the warning in
> >> default
> >>> situation for changing the QP inside libopenh264 library.
> >>
> >> Well in general I'd want to avoid hardcoding opinionated defaults within
> >> our own wrapper - I'd like it to behave as close to what upstream
> >> intended, so that whatever issues we see with defaults, are the same
> >> issues that everyone else sees as well, so any fixes to those defaults
> >> upstream also end up for us - so we don't get stuck on whatever we
> thought
> >> was a good default at some point.
> >
> > Agree, this makes more sense.
> >
> >> What warnings about changing QP are you referring to?
> > Warning:Change QP Range from(0,51) to (12,42)
> >
> https://github.com/cisco/openh264/blob/master/codec/encoder/core/src/e
> ncoder_ext.cpp#L375
> >
> > The main reason is that "0" is not supported well, which is the default
> > value of iMinQp.
> 
> Ah, right.
> 
> In that case, setting some other default might make sense indeed.
> How/where does OpenH264 set suitable values for this, in order not to get
> the warnings everywhere? Are the values e.g. 12-42 hardcoded everywhere
> in
> every caller?
> 
IIRC, it depends on the iUsageType, and hardcoded in the libopenh264 library function
ParamValidation() in [1]:
For SCREEN_CONTENT_REAL_TIME,
	it's (MIN_SCREEN_QP, MAX_SCREEN_QP), e.g. (26, 35);
For other usage like CAMERA_VIDEO_REAL_TIME by default,
	It's (GOM_MIN_QP_MODE, MAX_LOW_BR_QP), e.g. (12, 42)

- Linjie

[1] https://github.com/cisco/openh264/blob/master/codec/encoder/core/src/encoder_ext.cpp#L379
Martin Storsjö April 28, 2020, 7:01 a.m. UTC | #7
On Tue, 28 Apr 2020, Fu, Linjie wrote:

>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
>> Martin Storsjö
>> Sent: Tuesday, April 28, 2020 14:31
>> To: Fu, Linjie <linjie.fu@intel.com>
>> Cc: FFmpeg development discussions and patches <ffmpeg-
>> devel@ffmpeg.org>
>> Subject: Re: [FFmpeg-devel] [PATCH v4 1/9] lavc/libopenh264enc: Add
>> default qmin/qmax support
>>
>> On Tue, 28 Apr 2020, Fu, Linjie wrote:
>>
>>>> From: Martin Storsjö <martin@martin.st>
>>>> Sent: Tuesday, April 28, 2020 14:19
>>>> To: Fu, Linjie <linjie.fu@intel.com>
>>>> Cc: FFmpeg development discussions and patches <ffmpeg-
>>>> devel@ffmpeg.org>
>>>> Subject: RE: [FFmpeg-devel] [PATCH v4 1/9] lavc/libopenh264enc: Add
>>>> default qmin/qmax support
>>>>
>>>> On Tue, 28 Apr 2020, Fu, Linjie wrote:
>>>>
>>>>>> From: Martin Storsjö <martin@martin.st>
>>>>>> Sent: Tuesday, April 28, 2020 03:27
>>>>>>
>>>>>> If qmax/qmin < 0, i.e. wasn't specified by the user, wouldn't it be
>> better
>>>>>> to not touch param.iMax/MinQp at all (and use the default value of the
>>>>>> library, which may change between versions), instead of overriding it
>> with
>>>>>> a value hardcoded here?
>>>>>>
>>>>> Okay, this seems more natural if the recommended QP range varies
>>>> between
>>>>> versions, though one of my original purposes is to avoid the warning in
>>>> default
>>>>> situation for changing the QP inside libopenh264 library.
>>>>
>>>> Well in general I'd want to avoid hardcoding opinionated defaults within
>>>> our own wrapper - I'd like it to behave as close to what upstream
>>>> intended, so that whatever issues we see with defaults, are the same
>>>> issues that everyone else sees as well, so any fixes to those defaults
>>>> upstream also end up for us - so we don't get stuck on whatever we
>> thought
>>>> was a good default at some point.
>>>
>>> Agree, this makes more sense.
>>>
>>>> What warnings about changing QP are you referring to?
>>> Warning:Change QP Range from(0,51) to (12,42)
>>>
>> https://github.com/cisco/openh264/blob/master/codec/encoder/core/src/e
>> ncoder_ext.cpp#L375
>>>
>>> The main reason is that "0" is not supported well, which is the default
>>> value of iMinQp.
>>
>> Ah, right.
>>
>> In that case, setting some other default might make sense indeed.
>> How/where does OpenH264 set suitable values for this, in order not to get
>> the warnings everywhere? Are the values e.g. 12-42 hardcoded everywhere
>> in
>> every caller?
>>
> IIRC, it depends on the iUsageType, and hardcoded in the libopenh264 library function
> ParamValidation() in [1]:
> For SCREEN_CONTENT_REAL_TIME,
> 	it's (MIN_SCREEN_QP, MAX_SCREEN_QP), e.g. (26, 35);
> For other usage like CAMERA_VIDEO_REAL_TIME by default,
> 	It's (GOM_MIN_QP_MODE, MAX_LOW_BR_QP), e.g. (12, 42)
>
> - Linjie
>
> [1] https://github.com/cisco/openh264/blob/master/codec/encoder/core/src/encoder_ext.cpp#L379

Right, so looking at that code, it looks like it has a good intent - if 
the user hasn't specified that he wants a custom setting of min/max qp, 
then it uses the defaults.

So the problem is that this writes a warning in the log - and you want to 
avoid the warning.

Wouldn't it just be better to change this message to WELS_LOG_INFO within 
openh264, to avoid it being treated as a warning - as it's not a fault, 
it's the intended behaviour of picking sensible defaults when the user 
hasn't requested anything in particular?

// Martin
diff mbox series

Patch

diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c
index dd5d4ee..c7ae5b1 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;
@@ -331,6 +334,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 +353,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",
 };