Bug 202538 – [WebRTC][GStreamer] Build and use the openh264 based encoder if present on the system

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Thibault Saunier]

[WebRTC][GStreamer] Build and use the openh264 based encoder if present on the system

[Thibault Saunier]

Created attachment 380140 [details]
Patch

[Thibault Saunier]

Created attachment 380142 [details]
Patch

[Konstantin Tokarev]

Comment on attachment 380142 [details]
Patch

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

> Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoderFactory.cpp:304
> +#else

Is it intentional that you exclude all other encoders if OpenH264 is enabled?

[Thibault Saunier]

Comment on attachment 380142 [details]
Patch

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

>> Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoderFactory.cpp:304
>> +#else
> 
> Is it intentional that you exclude all other encoders if OpenH264 is enabled?

Yes, the encoder provided by LibWebRTC is preffered over GStreamer based one on purpose.

[Philippe Normand]

Comment on attachment 380142 [details]
Patch

The GStreamer bits look good but I would like Eric or Youenn to check the libwebrtc changes.

[Eric Carlson]

Comment on attachment 380142 [details]
Patch

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

The libWebRTC parts look fine to me.

> Source/ThirdParty/libwebrtc/Source/webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:596
> +      encoder_params.sSpatialLayers[0].sSliceCfg.uiSliceMode = SM_AUTO_SLICE; // FIXME

Nit: a few words about what needs to be fixed, and a bug number if there is one, would be helpful.

[Peng Liu]

Comment on attachment 380142 [details]
Patch

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

> Source/ThirdParty/libwebrtc/ChangeLog:6
> +        Reviewed by NOBODY (OOPS!).

It would be very helpful if some background information is included here. :-)

> Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoderFactory.cpp:531
> +    return makeUnique<GStreamerH264Encoder>(format);

Nit. The indentation is incorrect.

[Philippe Normand]

Comment on attachment 380142 [details]
Patch

I doubt this still applies cleanly on ToT. Likely this would need a rebase, moving off the review queue.

[Philippe Normand]

Created attachment 422099 [details]
Patch

[Thibault Saunier]

Comment on attachment 422099 [details]
Patch

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

Lgtm otherwise., thanks Phil!

> Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoderFactory.cpp:455
> +#ifdef WEBRTC_WEBKIT_BUILD

Shouldn't that be about whether OpenH264 was found or not instead?

[Philippe Normand]

Right, I think I misunderstood the purpose of WEBRTC_WEBKIT_BUILD. I'll revise this :)

[Philippe Normand]

(In reply to Thibault Saunier from comment #10)
> Comment on attachment 422099 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=422099&action=review
> 
> Lgtm otherwise., thanks Phil!
> 
> > Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoderFactory.cpp:455
> > +#ifdef WEBRTC_WEBKIT_BUILD
> 
> Shouldn't that be about whether OpenH264 was found or not instead?

I'm not sure how the initial patch even worked, because we can define a new macro for the libwebrtc cmake target, but it won't be visible in WebCore... 

We'd need to write a new header file in libwebrtc, exporting a new macro, and include that file where needed in WebCore, unless I misunderstood something :)

[Philippe Normand]

Created attachment 422244 [details]
Patch

[Thibault Saunier]

Comment on attachment 422244 [details]
Patch

Informal r+

[Xabier Rodríguez Calvar]

Comment on attachment 422244 [details]
Patch

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

> Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoderFactory.cpp:456
> +        GST_DEBUG("Using OpenH264 libwebrtc encoder.");
> +#if WEBKIT_LIBWEBRTC_OPENH264_ENCODER

Shouldn't these two lines be swapped? Maybe adding another one for the GStreamer encoder? And maybe INFO rather than just debug.

[Philippe Normand]

Comment on attachment 422244 [details]
Patch

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

>> Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoderFactory.cpp:456
>> +#if WEBKIT_LIBWEBRTC_OPENH264_ENCODER
> 
> Shouldn't these two lines be swapped? Maybe adding another one for the GStreamer encoder? And maybe INFO rather than just debug.

Which lines?

[Xabier Rodríguez Calvar]

(In reply to Philippe Normand from comment #16)
> Comment on attachment 422244 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=422244&action=review
> 
> >> Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoderFactory.cpp:456
> >> +#if WEBKIT_LIBWEBRTC_OPENH264_ENCODER
> > 
> > Shouldn't these two lines be swapped? Maybe adding another one for the GStreamer encoder? And maybe INFO rather than just debug.
> 
> Which lines?

Sorry, the #if and the GST_DEBUG, which I would turn into INFO, maybe and I would also add one GST_INFO to the #else.

[Philippe Normand]

Created attachment 424632 [details]
Patch

[Adrian Perez]

Comment on attachment 424632 [details]
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 :)

[Philippe Normand]

Created attachment 424639 [details]
Patch

[Philippe Normand]

I'll check EWS failures.

[Philippe Normand]

Created attachment 424745 [details]
Patch

[Philippe Normand]

Created attachment 424751 [details]
Patch

[EWS]

Committed r275275: <https://commits.webkit.org/r275275>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 424751 [details].

[Radar WebKit Bug Importer]

<rdar://problem/76047172>

[Yury Semikhatsky]

(In reply to EWS from comment #24)
> Committed r275275: <https://commits.webkit.org/r275275>
> 

Ubuntu 20.04 build is now failing (e.g. https://build.webkit.org/#/builders/30/builds/60):

FAILED: Source/ThirdParty/libwebrtc/CMakeFiles/webrtc.dir/Source/webrtc/modules/video_coding/codecs/h264/h264.cc.o 
/usr/lib/ccache/c++  -DBUILDING_GTK__=1 -DBUILDING_WITH_CMAKE=1 -DDISABLE_H265 -DDYNAMIC_ANNOTATIONS_ENABLED=1 -DEXPAT_RELATIVE_PATH -DGETTEXT_PACKAGE=\"WebKit2GTK-4.0\" -DHAVE_CONFIG_H=1 -DHAVE_LRINT -DHAVE_LRINTF -DHAVE_NETINET_IN_H -DHAVE_SCTP -DHAVE_WEBRTC_VIDEO -DHAVE_WEBRTC_VOICE -DJSC_GLIB_API_ENABLED -DJSON_USE_EXCEPTION=0 -DNON_WINDOWS_DEFINE -DOPENSSL_NO_ASM -DOPUS_BUILD -DOPUS_EXPORT="" -DRTC_DISABLE_VP9 -DSCTP_PROCESS_LEVEL_LOCKS -DSCTP_SIMPLE_ALLOCATOR -DSCTP_USE_OPENSSL_SHA1 -DSVN_REVISION=\"r275450\" -DVAR_ARRAYS -DWEBKITGTK_API_VERSION_STRING=\"4.0\" -DWEBRTC_APM_DEBUG_DUMP=0 -DWEBRTC_CODEC_G711 -DWEBRTC_CODEC_G722 -DWEBRTC_CODEC_ILBC -DWEBRTC_CODEC_ISAC -DWEBRTC_CODEC_OPUS -DWEBRTC_CODEC_RED -DWEBRTC_ENABLE_LINUX_ALSA -DWEBRTC_INCLUDE_INTERNAL_AUDIO_DEVICE -DWEBRTC_INTELLIGIBILITY_ENHANCER=0 -DWEBRTC_LINUX -DWEBRTC_NS_FLOAT -DWEBRTC_OPUS_SUPPORT_120MS_PTIME=0 -DWEBRTC_OPUS_VARIABLE_COMPLEXITY=0 -DWEBRTC_POSIX -DWEBRTC_USE_BUILTIN_ISAC_FIX=1 -DWEBRTC_USE_BUILTIN_ISAC_FLOAT=0 -DWEBRTC_USE_BUILTIN_OPUS=1 -DWEBRTC_USE_H264=1 -DWTF_USE_DYNAMIC_ANNOTATIONS=1 -D_GNU_SOURCE -D__Userspace__ -D__Userspace_os_Linux -I../../Source/ThirdParty/libwebrtc/Source -I../../Source/ThirdParty/libwebrtc/Source/third_party/abseil-cpp -I../../Source/ThirdParty/libwebrtc/Source/third_party/boringssl/src/include -I../../Source/ThirdParty/libwebrtc/Source/third_party/jsoncpp/generated -I../../Source/ThirdParty/libwebrtc/Source/third_party/jsoncpp/overrides/include -I../../Source/ThirdParty/libwebrtc/Source/third_party/jsoncpp/source/include -I../../Source/ThirdParty/libwebrtc/Source/third_party/jsoncpp/source/src/lib_json -I../../Source/ThirdParty/libwebrtc/Source/third_party/libsrtp/config -I../../Source/ThirdParty/libwebrtc/Source/third_party/libsrtp/crypto/include -I../../Source/ThirdParty/libwebrtc/Source/third_party/libsrtp/include -I../../Source/ThirdParty/libwebrtc/Source/third_party/libyuv/include -I../../Source/ThirdParty/libwebrtc/Source/third_party/opus/src/celt -I../../Source/ThirdParty/libwebrtc/Source/third_party/opus/src/include -I../../Source/ThirdParty/libwebrtc/Source/third_party/opus/src/silk -I../../Source/ThirdParty/libwebrtc/Source/third_party/opus/src/silk/float -I../../Source/ThirdParty/libwebrtc/Source/third_party/usrsctp -I../../Source/ThirdParty/libwebrtc/Source/third_party/usrsctp/usrsctplib -I../../Source/ThirdParty/libwebrtc/Source/third_party/usrsctp/usrsctplib/usrsctplib -I../../Source/ThirdParty/libwebrtc/Source/third_party/usrsctp/usrsctplib/usrsctplib/netinet -I../../Source/ThirdParty/libwebrtc/Source/webrtc -I../../Source/ThirdParty/libwebrtc/Source/webrtc/common_audio/resampler/include -I../../Source/ThirdParty/libwebrtc/Source/webrtc/common_audio/signal_processing/include -I../../Source/ThirdParty/libwebrtc/Source/webrtc/common_audio/vad/include -I../../Source/ThirdParty/libwebrtc/Source/webrtc/modules/audio_coding/codecs/isac/main/include -fdiagnostics-color=always -Wextra -Wall -Wno-expansion-to-defined -Wno-noexcept-type -Wno-psabi -Wno-misleading-indentation -Wno-maybe-uninitialized -Wwrite-strings -Wundef -Wpointer-arith -Wmissing-format-attribute -Wformat-security -Wcast-align -Wno-tautological-compare  -fno-strict-aliasing -fno-exceptions -fno-rtti -O3 -DNDEBUG -fPIC -fvisibility=hidden   -std=gnu++11 -UHAVE_CONFIG_H -DWEBRTC_WEBKIT_BUILD=1 -w -std=c++17 -MD -MT Source/ThirdParty/libwebrtc/CMakeFiles/webrtc.dir/Source/webrtc/modules/video_coding/codecs/h264/h264.cc.o -MF Source/ThirdParty/libwebrtc/CMakeFiles/webrtc.dir/Source/webrtc/modules/video_coding/codecs/h264/h264.cc.o.d -o Source/ThirdParty/libwebrtc/CMakeFiles/webrtc.dir/Source/webrtc/modules/video_coding/codecs/h264/h264.cc.o -c ../../Source/ThirdParty/libwebrtc/Source/webrtc/modules/video_coding/codecs/h264/h264.cc
In file included from ../../Source/ThirdParty/libwebrtc/Source/webrtc/modules/video_coding/codecs/h264/h264.cc:25:
../../Source/ThirdParty/libwebrtc/Source/webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.h:34:10: fatal error: wels/codec_app_def.h: No such file or directory
   34 | #include "wels/codec_app_def.h"
      |          ^~~~~~~~~~~~~~~~~~~~~~
compilation terminated.

[Philippe Normand]

Can you try a clean build please? I suspect the cmake stuff needs to be nuked.

[Philippe Normand]

I checked locally the build is passing even if I check bogus libraries in FindOpenh264.cmake.

[Yury Semikhatsky]

(In reply to Philippe Normand from comment #28)
> I checked locally the build is passing even if I check bogus libraries in
> FindOpenh264.cmake.

Just checked with clean build (removed WebKitBuild/Release with all the cmake caches etc), it is still failing with exactly same error (which is the same as on the buildbot https://build.webkit.org/#/builders/30/builds/60).

[Yury Semikhatsky]

The command line I use to build:

WEBKIT_JHBUILD=1 WEBKIT_JHBUILD_MODULESET=minimal WEBKIT_OUTPUTDIR=WebKitBuild/GTK Tools/jhbuild/jhbuild-wrapper --gtk run cmake --build WebKitBuild/GTK/Release --config Release

[Philippe Normand]

Comment on attachment 424751 [details]
Patch

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

> Source/ThirdParty/libwebrtc/Source/webrtc/modules/video_coding/codecs/h264/h264.cc:22
> +#if !defined(WEBRTC_WEBKIT_BUILD)

Change this to `#if WEBKIT_LIBWEBRTC_OPENH264_ENCODER` or similar?

[Carlos Garcia Campos]

Same build failure here in debian testing. I just disabled webrtc

[Philippe Normand]

Created attachment 425279 [details]
Patch

[Philippe Normand]

Carlos & Yury, can you test this patch please?

[Yury Semikhatsky]

Just checked on a clean build, same error with your patch applied :(

[Yury Semikhatsky]

Note that I do see the following in the cmake logs:

...
-- Could NOT find Openh264 (missing: Openh264_LIBRARY Openh264_INCLUDE_DIR) 
CMake Warning at Source/ThirdParty/libwebrtc/CMakeLists.txt:1540 (message):
  openh264 is not found, not building support.
...

[Philippe Normand]

(In reply to Yury Semikhatsky from comment #30)
> The command line I use to build:
> 
> WEBKIT_JHBUILD=1 WEBKIT_JHBUILD_MODULESET=minimal
> WEBKIT_OUTPUTDIR=WebKitBuild/GTK Tools/jhbuild/jhbuild-wrapper --gtk run
> cmake --build WebKitBuild/GTK/Release --config Release

Error: could not load cache

[Philippe Normand]

Created attachment 425295 [details]
Patch

[Yury Semikhatsky]

(In reply to Philippe Normand from comment #38)
> Created attachment 425295 [details]
> Patch

Same error with this patch applied:

In file included from ../../../Source/ThirdParty/libwebrtc/Source/webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:17:
../../../Source/ThirdParty/libwebrtc/Source/webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.h:34:10: fatal error: 'wels/codec_app_def.h' file not found
#include "wels/codec_app_def.h"
         ^~~~~~~~~~~~~~~~~~~~~~
1 error generated.

[Philippe Normand]

So what's in WebKitBuild/GTK/Release/Source/ThirdParty/libwebrtc/LibWebRTCWebKitMacros.h ?

[Yury Semikhatsky]

#define WEBKIT_LIBWEBRTC_OPENH264_ENCODER 1

[Philippe Normand]

Created attachment 425309 [details]
Patch

[Yury Semikhatsky]

(In reply to Philippe Normand from comment #42)
> Created attachment 425309 [details]
> Patch

Same error on clean build :(

[Philippe Normand]

Reverting in https://bugs.webkit.org/show_bug.cgi?id=224244

[Carlos Alberto Lopez Perez]

*** Bug 224246 has been marked as a duplicate of this bug. ***

[Philippe Normand]

Created attachment 425517 [details]
Patch

[Philippe Normand]

(In reply to Philippe Normand from comment #46)
> Created attachment 425517 [details]
> Patch

Can you test please Yury?
(We really need a GTK or WPE LTS EWS...)

[Yury Semikhatsky]

(In reply to Philippe Normand from comment #47)
> 
> Can you test please Yury?
> (We really need a GTK or WPE LTS EWS...)

It builds with no errors on both GTK and WPE for me.

[Philippe Normand]

Created attachment 425597 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 425597 [details]
Patch

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

> Source/ThirdParty/libwebrtc/CMakeLists.txt:1539
> +      #add_definitions(-DWEBRTC_USE_H264=1)

Remove this?

[Philippe Normand]

Created attachment 426096 [details]
Patch

[EWS]

Committed r276019 (236571@main): <https://commits.webkit.org/236571@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 426096 [details].