[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 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 
in the table, especially if we have other defaults like gop size there.

// Martin