[FFmpeg-devel] [PATCH v5 3/5] lavc/libopenh264enc: add bit rate control select support

[Fu, Linjie]

> From: Martin Storsjö <martin at martin.st>
> Sent: Wednesday, April 29, 2020 03:18
> To: Fu, Linjie <linjie.fu at intel.com>
> Cc: FFmpeg development discussions and patches <ffmpeg-
> devel at ffmpeg.org>
> Subject: RE: [FFmpeg-devel] [PATCH v5 3/5] lavc/libopenh264enc: add bit
> rate control select support
> 
> On Tue, 28 Apr 2020, Fu, Linjie wrote:
> 
> >> From: Martin Storsjö <martin at martin.st>
> >> Sent: Tuesday, April 28, 2020 18:31
> >> To: FFmpeg development discussions and patches <ffmpeg-
> >> devel at ffmpeg.org>
> >> Cc: Fu, Linjie <linjie.fu at intel.com>
> >> Subject: Re: [FFmpeg-devel] [PATCH v5 3/5] lavc/libopenh264enc: add bit
> >> rate control select support
> >>
> >> On Tue, 28 Apr 2020, Linjie Fu wrote:
> >>
> >> We don't need checks for 1.2 - the initial version of openh264 that we
> >> support is 1.3, so we only need checks for 1.4 and newer.
> >
> > Ahh, didn't noticed this until checked the commit message of
> > 8a3d9ca603f4d15ecaa9ca379cbaab4ecaec8ce4.
> >
> > IMHO it seems to be better if we add this version check in the configure as
> well.
> > (Did a quick verification with version modified in pkgconfig to 1.1.0,
> configure is
> > still good for libopenh264 )
> 
> We don't need an explicit version check for 1.3 in configure - we require
> that WelsGetCodecVersion can be found in the configure check, and that
> function was added in 1.3 (explicitly for the purpose so that we can check
> that the version that we have linked is the same as we compiled against).
> 

Checked and confirmed that you are right, WelsGetCodecVersion is enough
for  1.3.0 version check, thanks for elaborations.

> >
> >>> +#if OPENH264_VER_AT_LEAST(1, 4)
> >>> +        { "timestamp", "bit rate control based on timestamp",
> >> 0, AV_OPT_TYPE_CONST, { .i64 = RC_TIMESTAMP_MODE },   0, 0, VE,
> >> "rc_mode" },
> >>> +#endif
> >>> +
> >>>     { NULL }
> >>> };
> >>
> >> This commit seems to lack something that actually sets the rc_mode value
> >> in the parameters struct though - wasn't that part of the previous version
> >> of the patch?
> >>
> > Did you mean setting rc_mode through parameters like bit_rate/qp?
> > This patch enables explicitly setting the rc_mode as part of that logic.
> 
> No, I didn't mean that.
> 
> The previous version of this patch had the following change:
> 
> -    param.iRCMode                    = RC_QUALITY_MODE;
> +    param.iRCMode                    = s->rc_mode;
> 
> Now I don't find that part any longer in this patch - and without that
> change, the rest of the commit is pretty pointless, no?

it's somehow missed while rebasing the set, thanks.

- Linjie