[FFmpeg-devel] [PATCH v4 2/9] lavc/libopenh264enc: add default gop size and bit rate

[Martin Storsjö]

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

>> From: Martin Storsjö <martin at martin.st>
>> Sent: Tuesday, April 28, 2020 14:28
>> 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 2/9] lavc/libopenh264enc: add
>> default gop size and bit rate
>>
>> On Tue, 28 Apr 2020, Fu, Linjie wrote:
>>
>>>> From: Martin Storsjö <martin at martin.st>
>>>> Sent: Tuesday, April 28, 2020 03:28
>>>>> static const AVCodecDefault svc_enc_defaults[] = {
>>>>> +    { "b",         "0"     },
>>>>> +    { "g",         "120"   },
>>>>>     { "qmin",      "-1"    },
>>>>
>>>> Why do you hardcode a value for g here, but put the default bitrate value
>>>> in the code above? Wouldn't it be clearer to have both defaults here at
>>>> the same place?
>>>>
>>> A default value in svc_enc_defaults[] would help to distinguish between
>>> "the user specified the bitrate to be <x>" vs. "the user did not specify
>> anything
>>> about the target bitrate", as Anton has suggested in [1].
>>>
>>> Considering about your suggestions in patch 1/9, IMO it seems to be more
>> reasonable
>>> to keep the uiIntraPeriod untouched. The libopenh264 library would fill the
>> default
>>> value of uiIntraPeriod to 0, and as a consequence the gop size would be
>> rather large.
>>>
>>> Updated the default "g" to "-1", same as libx264 did.
>>> (Note that it's not acceptable for bitrate, since bitrate = 0 as default in
>> library is not
>>> valid)
>>
>> If we have the option of not setting the bitrate on the encoder, then yes,
>> it's better to avoid setting one in svc_enc_defaults. But in this case, if
>> no bitrate was chosen by the user, we end up enforcing a default value
>> anyway - just that the default is not written in the global defaults table
>> (libavcodec/options_table.h) and not in svc_enc_defaults, but instead in
>> code.
>>
>> If we actually need to set a bitrate, it's IMO better to put this default
>
> Indeed we have to set the bitrate.
>
>> in the table, especially if we have other defaults like gop size there.
>>
>
> In previous discussion in [1], we seems to come to an alignment that this would
> help to determine whether it's specified by user, and hence allow libopenh264enc
> to select the RC_MODE through settings like avctx->bit_rate, max/min/avg bitrates
> if rc_mode is not set explicitly.
>
> VAAPI encoder has the similar logic in [2].
>
> I'm okay with either solution, if above logic is not going to be implemented.

Right, so if logic that selects rc mode based on those settings will be 
implemented, then yes it makes sense to keep the default at -1 and fall 
back to a default bitrate within code. In that case I'd kind of prefer to 
have the default bitrate specified in a define high up in the file, 
instead of buried in init logic though.

// Martin