Summary: | [WebRTC][GStreamer] Build and use the openh264 based encoder if present on the system | ||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Thibault Saunier <tsaunier> | ||||||||||||||||||||||||||||||
Component: | New Bugs | Assignee: | Thibault Saunier <tsaunier> | ||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | annulen, aperez, bugs-noreply, calvaris, cgarcia, clopez, eric.carlson, ews-watchlist, glenn, gyuyoung.kim, hta, jer.noble, peng.liu6, philipj, pnormand, rakuco, ryuan.choi, sergio, tommyw, vjaquez, webkit-bug-importer, yurys | ||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=224089 https://bugs.webkit.org/show_bug.cgi?id=224246 |
||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Thibault Saunier
2019-10-03 10:58:43 PDT
Created attachment 380140 [details]
Patch
Created attachment 380142 [details]
Patch
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? 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. Comment on attachment 380142 [details]
Patch
The GStreamer bits look good but I would like Eric or Youenn to check the libwebrtc changes.
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. 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. 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.
Created attachment 422099 [details]
Patch
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? Right, I think I misunderstood the purpose of WEBRTC_WEBKIT_BUILD. I'll revise this :) (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 :) Created attachment 422244 [details]
Patch
Comment on attachment 422244 [details]
Patch
Informal r+
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. 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? (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. Created attachment 424632 [details]
Patch
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 :) Created attachment 424639 [details]
Patch
I'll check EWS failures. Created attachment 424745 [details]
Patch
Created attachment 424751 [details]
Patch
Committed r275275: <https://commits.webkit.org/r275275> All reviewed patches have been landed. Closing bug and clearing flags on attachment 424751 [details]. (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. Can you try a clean build please? I suspect the cmake stuff needs to be nuked. I checked locally the build is passing even if I check bogus libraries in FindOpenh264.cmake. (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). 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 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? Same build failure here in debian testing. I just disabled webrtc Created attachment 425279 [details]
Patch
Carlos & Yury, can you test this patch please? Just checked on a clean build, same error with your patch applied :( 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. ... (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 Created attachment 425295 [details]
Patch
(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. So what's in WebKitBuild/GTK/Release/Source/ThirdParty/libwebrtc/LibWebRTCWebKitMacros.h ? #define WEBKIT_LIBWEBRTC_OPENH264_ENCODER 1 Created attachment 425309 [details]
Patch
(In reply to Philippe Normand from comment #42) > Created attachment 425309 [details] > Patch Same error on clean build :( Reverting in https://bugs.webkit.org/show_bug.cgi?id=224244 *** Bug 224246 has been marked as a duplicate of this bug. *** Created attachment 425517 [details]
Patch
(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...) (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. Created attachment 425597 [details]
Patch
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? Created attachment 426096 [details]
Patch
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]. |