> From: Martin Storsjö <mar...@martin.st> > Sent: Wednesday, April 29, 2020 03:18 > To: Fu, Linjie <linjie...@intel.com> > Cc: FFmpeg development discussions and patches <ffmpeg- > de...@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ö <mar...@martin.st> > >> Sent: Tuesday, April 28, 2020 18:31 > >> To: FFmpeg development discussions and patches <ffmpeg- > >> de...@ffmpeg.org> > >> Cc: Fu, Linjie <linjie...@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 _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".