[webkit-reviews] review denied: [Bug 202538] [WebRTC][GStreamer] Build and use the openh264 based encoder if present on the system : [Attachment 424632] Patch

[bugzilla-daemon at webkit.org]

Adrian Perez <aperez at igalia.com> has denied  review:
Bug 202538: [WebRTC][GStreamer] Build and use the openh264 based encoder if
present on the system
https://bugs.webkit.org/show_bug.cgi?id=202538

Attachment 424632: Patch

https://bugs.webkit.org/attachment.cgi?id=424632&action=review




--- Comment #19 from Adrian Perez <aperez at igalia.com> ---
Comment on attachment 424632
  --> https://bugs.webkit.org/attachment.cgi?id=424632
Patch

The code part LGTM, but I won't comment on it because I am not a media
expert. On the CMake side of things, I have a couple of comments that
would be nice to address before landing :)


View in context: https://bugs.webkit.org/attachment.cgi?id=424632&action=review

> Source/ThirdParty/libwebrtc/cmake/FindOpenh264.cmake:34
> +``OPENH264::libopenh264``

Following usual CMake idioms, this target should have Openh264::
as prefix, to match the <name> from the “Find<name>.cmake” file.

> Source/ThirdParty/libwebrtc/cmake/FindOpenh264.cmake:42
> +``OPENH264_FOUND``

Same for variables: these should be named Openh264_{FOUND,VERSION,…} because
the convention is to name output variables of a CMake find-module after
the <name> part from the “Find<name>.cmake” file as well.

In this case (output variables), if we don't follow the convention. Recent
versions
of CMake are already showing a warning, which might be an error in the future
:)

> Source/ThirdParty/libwebrtc/cmake/FindOpenh264.cmake:71
> +	   string(REGEX MATCH "#define +OPENH264_MAJOR +\\(([0-9]+)\\)" _dummy
"${OPENH264_VERSION_CONTENT}")

To make the regexps a bit more robust, I recommend using “[ \t]+”
instead of only “ +” to allow both tabs and spaces between the macro
name and the version number. Also there can be optional whitespace
between the octothorpe and the “define” keyword.

My suggested regexp string would be:

  "#[ \t]*define[ \t]+OPENH264_MAJOR[ \t]+\\(([0-9]+)\\)"

(And the same for OPENH264_MINOR and OPENH264_REVISION below.)

> Source/ThirdParty/libwebrtc/cmake/FindOpenh264.cmake:92
> +    add_library(OPENH264::libopenh264 UNKNOWN IMPORTED GLOBAL)

It's nice to see that new CMake find-modules using imported targets :)