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

[Martin Storsjö]

On Tue, 28 Apr 2020, Fu, Linjie wrote:

>> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
>> Martin Storsjö
>> Sent: Tuesday, April 28, 2020 14:31
>> To: Fu, Linjie <linjie.fu at intel.com>
>> Cc: FFmpeg development discussions and patches <ffmpeg-
>> devel at 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 at martin.st>
>>>> Sent: Tuesday, April 28, 2020 14:19
>>>> To: Fu, Linjie <linjie.fu at intel.com>
>>>> Cc: FFmpeg development discussions and patches <ffmpeg-
>>>> devel at 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 at 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