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

Martin Storsjö martin at martin.st
Tue Apr 28 22:17:52 EEST 2020


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).

> Will send another fix for this if no against.
>
>>> +#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?

// Martin


More information about the ffmpeg-devel mailing list