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

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
_______________________________________________
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".

Reply via email to