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

[Fu, Linjie]

> From: Martin Storsjö <martin at martin.st>
> Sent: Tuesday, April 28, 2020 16:08
> 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 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

Updated.
Separated the patch set and resend, thanks for the suggestions.

- Linjie