Conversation
|
So far, I’ve fixed all CI issues, including the Linux audio backend ones. Apparently, I’ve ended up with the largest port in the tree by text line count: |
There was a problem hiding this comment.
Pull request overview
Adds a new webrtc port to vcpkg that builds upstream WebRTC via GN/Ninja, synthesizes a minimal Chromium-style third_party surface to unbundle dependencies to existing vcpkg ports, and installs a consumable webrtc::webrtc CMake package.
Changes:
- Introduces
ports/webrtcwith portfile, manifest, generated GN helper scripts/tests, and CMake package config. - Adds a set of upstream/build patches to make GN-based builds work across supported triplets and to unbundle/disable optional components.
- Updates the version database (baseline + new versions entry) for the new port.
Reviewed changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| versions/w-/webrtc.json | Adds the version entry for the new webrtc port. |
| versions/baseline.json | Adds webrtc to the baseline registry. |
| ports/webrtc/vcpkg.json | Defines the new port metadata, dependencies, and Linux audio backend features. |
| ports/webrtc/portfile.cmake | Implements GN/Ninja build, synthetic-tree generation, and install logic. |
| ports/webrtc/source-manifest.cmake | Pins upstream WebRTC + auxiliary repos (build/buildtools/rnnoise). |
| ports/webrtc/webrtcConfig.cmake.in | Provides the exported webrtc::webrtc CMake package/target. |
| ports/webrtc/package-remove-paths.txt | Lists header subtrees pruned from the installed include tree. |
| ports/webrtc/absl-labels.txt | Manifest of Abseil GN labels to synthesize against CMake-exported Abseil libs. |
| ports/webrtc/generate_external_absl.py | Generates a synthetic third_party/abseil-cpp GN tree and CMake target lists. |
| ports/webrtc/generate_external_absl_test.py | Unit tests for the Abseil GN tree generator. |
| ports/webrtc/generate_external_third_party.py | Generates synthetic GN-visible trees for multiple third_party deps. |
| ports/webrtc/generate_external_third_party_test.py | Unit tests for the generic third-party tree generator. |
| ports/webrtc/webrtc-0001-disable-perfetto-when-off.patch | Adjusts Perfetto linkage/config behavior based on build flags. |
| ports/webrtc/webrtc-0002-export-enable-media-with-defaults.patch | Exports additional GN targets needed for consumption. |
| ports/webrtc/webrtc-0003-fix-rtp-config-optional.patch | Fixes optional/std::optional usage for MSVC compatibility. |
| ports/webrtc/webrtc-0004-use-upstream-rnnoise.patch | Switches to upstream rnnoise implementation model and integration approach. |
| ports/webrtc/webrtc-0005-use-external-openssl.patch | Enables external OpenSSL library path wiring for GN builds. |
| ports/webrtc/webrtc-0006-make-dav1d-decoder-deps-conditional.patch | Makes dav1d-related deps conditional on a GN arg. |
| ports/webrtc/webrtc-0007-fix-rtp-packet-info-eq-for-msvc.patch | Replaces defaulted operator== to support MSVC configurations lacking it. |
| ports/webrtc/webrtc-0008-fix-audio-device-core-win-goto-scope.patch | Fixes MSVC goto/scope issues and variable initialization in Windows audio device code. |
| ports/webrtc/webrtc-0009-fix-avx2-intrinsics-for-msvc.patch | Fixes AVX2 intrinsic code that relies on non-MSVC vector element access. |
| ports/webrtc/webrtc-0010-disable-arm-denormal-disabler-for-msvc.patch | Avoids ARM denormal disabler paths for MSVC where unsupported. |
| ports/webrtc/webrtc-0011-make-linux-audio-backends-optional.patch | Makes ALSA/PulseAudio backends optional and controlled via GN args. |
| ports/webrtc/rnnoise-0001-fix-neon-reinterpret-for-msvc.patch | Adjusts NEON reinterpret usage for MSVC compatibility in rnnoise. |
| ports/webrtc/build-0001-drop-module-deps-from-toolchain-invocations.patch | Removes module deps from tool invocations and disables module tool commands. |
| ports/webrtc/build-0002-fix-apple-arflags-usage.patch | Fixes Apple ar/libtool flag usage in toolchain definitions. |
| ports/webrtc/build-0003-disable-fno-lifetime-dse.patch | Drops a compiler flag that may be risky/undesired for this packaging context. |
| ports/webrtc/build-0004-disable-sanitize-c-array-bounds.patch | Disables array-bounds sanitizer flags in build toolchain config. |
| ports/webrtc/build-0005-disable-sanitize-return.patch | Disables return sanitizer flags in build toolchain config. |
| ports/webrtc/build-0006-skip-local-vs-debugger-copy.patch | Avoids copying Windows debugger DLLs when using a local toolchain setup. |
| ports/webrtc/build-0007-fix-windows-pdb-commands.patch | Fixes MSVC PDB command-line handling in GN toolchain templates. |
| ports/webrtc/build-0008-disable-crel-on-linux-arm64.patch | Disables --crel assembler flags (currently unconditionally). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ports/webrtc/webrtcConfig.cmake.in
Outdated
| INTERFACE_INCLUDE_DIRECTORIES "${_webrtc_prefix}/include" | ||
| INTERFACE_LINK_LIBRARIES "${_webrtc_interface_link_libraries}" | ||
| INTERFACE_LINK_OPTIONS "$<$<PLATFORM_ID:Darwin>:SHELL:-framework AudioToolbox;SHELL:-framework CoreAudio;SHELL:-framework CoreFoundation;SHELL:-framework Foundation;SHELL:-framework AppKit;SHELL:-framework ApplicationServices>" | ||
| INTERFACE_COMPILE_DEFINITIONS | ||
| "CHROMIUM;DYNAMIC_ANNOTATIONS_ENABLED=1;LIBYUV_DISABLE_SVE;LIBYUV_DISABLE_SME;LIBYUV_DISABLE_LSX;LIBYUV_DISABLE_LASX;PROTOBUF_ENABLE_DEBUG_LOGGING_MAY_LEAK_PII=0;RTC_DAV1D_IN_INTERNAL_DECODER_FACTORY;RTC_ENABLE_VP9;WEBRTC_DEPRECATE_PLAN_B;WEBRTC_ENCODER_PSNR_STATS;WEBRTC_HAVE_SCTP;WEBRTC_INCLUDE_INTERNAL_AUDIO_DEVICE;WEBRTC_NON_STATIC_TRACE_EVENT_HANDLERS=0;WEBRTC_STRICT_FIELD_TRIALS=0;$<$<PLATFORM_ID:Linux>:@WEBRTC_LINUX_INTERFACE_DEFINITIONS@>;$<$<PLATFORM_ID:Darwin>:WEBRTC_MAC;WEBRTC_POSIX>;$<$<PLATFORM_ID:Windows>:NOMINMAX;UNICODE;WIN32;WIN32_LEAN_AND_MEAN;WEBRTC_WIN>" |
There was a problem hiding this comment.
INTERFACE_LINK_OPTIONS and INTERFACE_COMPILE_DEFINITIONS embed generator expressions that contain semicolons inside the expression body (e.g. $<$<PLATFORM_ID:Darwin>:...;...> and $<$<PLATFORM_ID:Windows>:...;...>). CMake treats semicolons as list separators at configure time, so these expressions will be split into multiple list items and become syntactically invalid (dangling > fragments) and/or apply definitions unconditionally. Please rewrite these as multiple generator expressions without semicolons, or escape internal separators with $<SEMICOLON>, so the platform-conditional definitions/options evaluate correctly.
| if (is_win) { | ||
| asm_obj_extension = "obj" | ||
| } else { | ||
| asm_obj_extension = "o" | ||
| } | ||
|
|
There was a problem hiding this comment.
asm_obj_extension is set based on is_win, but it is never referenced later in the nasm_assemble template. This makes the template misleading and can mask Windows-specific object naming requirements.
| if (is_win) { | |
| asm_obj_extension = "obj" | |
| } else { | |
| asm_obj_extension = "o" | |
| } |
| } | ||
| } | ||
|
|
||
| outputs = [ "$target_out_dir/$source_set_name/{{source_name_part}}.o" ] |
There was a problem hiding this comment.
The NASM template hard-codes .o for assembled object outputs. On Windows this should generally use .obj (or at least consistently use asm_obj_extension) to avoid toolchain/linker incompatibilities when consuming these objects.
| outputs = [ "$target_out_dir/$source_set_name/{{source_name_part}}.o" ] | |
| outputs = [ "$target_out_dir/$source_set_name/{{source_name_part}}.${asm_obj_extension}" ] |
ports/webrtc/build-0001-drop-module-deps-from-toolchain-invocations.patch
Show resolved
Hide resolved
ports/webrtc/build-0001-drop-module-deps-from-toolchain-invocations.patch
Show resolved
Hide resolved
ports/webrtc/build-0001-drop-module-deps-from-toolchain-invocations.patch
Show resolved
Hide resolved
BillyONeal
left a comment
There was a problem hiding this comment.
Honestly when I started reviewing this I saw the 4k lines and thought I would hate it but most of this looks reasonable. I made some nitpick comments but by and large this is probably mergable.
I also included a small sample consumer project for the port
Thank you for the demonstration! It helps a lot on justifying adding a 4k lines thing.
Some thoughts:
- Being not normally Python programmers, I fear adding these big python scripts that we aren't really going to truly understand. I'm willing to take it mostly "on faith" given passing tests that they do what is expected, but I would appreciate if we could get others to confirm their sanity.
- This is adding yet another thing that is "sucked out" of chromium and needs
vcpkg_from_gitand is thus not asset cachable. However, unlike most of the other ports likeangleit seems like this might use a substantial fraction of a chromium tree. Would it make sense to try to use https://github.com/chromium/chromium +vcpkg_from_githubto allow those things to be cached? (To be clear, I'm just asking, I have not investigated if this would be a good idea.) - What impact does this have on those other chromium children like angle, dawn, etc.?
| ] | ||
| } | ||
|
|
||
| - # The performance improvement does not seem worth the risk. See |
There was a problem hiding this comment.
This seems to explicitly revert something upstream explicitly thoughtfully considered and added. Can you explain?
Ditto the next several patches.
| @@ -0,0 +1,151 @@ | |||
| diff --git a/modules/audio_processing/agc2/rnn_vad/BUILD.gn b/modules/audio_processing/agc2/rnn_vad/BUILD.gn | |||
| --- a/modules/audio_processing/agc2/rnn_vad/BUILD.gn | |||
| +++ b/modules/audio_processing/agc2/rnn_vad/BUILD.gn | |||
There was a problem hiding this comment.
Is this submitted upstream? It's kind of a lot of edited product code which makes it borderline under https://learn.microsoft.com/en-us/vcpkg/contributing/maintainer-guide#patching
| } | ||
|
|
||
| -bool operator==(const RtpPacketInfo& lhs, const RtpPacketInfo& rhs) = default; | ||
| +bool operator==(const RtpPacketInfo& lhs, const RtpPacketInfo& rhs) { |
There was a problem hiding this comment.
Why do you need this change? I'm nervous about it because it explicitly does silent bad code if the members update in the future.
There was a problem hiding this comment.
I checked whether we could drop this patch and rebuilt webrtc:x64-windows without it.
With MSVC 19.44.35224 (VS 2022 Community, toolset 14.44.35207), the build fails in api/rtp_packet_info.cc on upstream’s defaulted free operator==:
../src/5931ef8cc1-5a7e580849.clean/api/rtp_packet_info.cc(77): error C2610: 'bool webrtc::operator ==(const webrtc::RtpPacketInfo &,const webrtc::RtpPacketInfo &)': is not a special member function or comparison operator which can be defaulted`
So this patch is still needed for our current Windows/MSVC build.
|
|
||
| HRESULT hr = S_OK; | ||
| IAudioEndpointVolume* pVolume = NULL; | ||
| - |
There was a problem hiding this comment.
Can you minimize the patch by not including this whitespace change?
| @@ -0,0 +1,329 @@ | |||
| diff --git a/modules/audio_device/win/audio_device_core_win.cc b/modules/audio_device/win/audio_device_core_win.cc | |||
There was a problem hiding this comment.
This is a big patch but because it's just moving lines that are already in the file to the top of their block I think it's acceptable.
| { | ||
| int8x16_t vw0, vw1, vw2, vw3, vx0, vx1; | ||
| - vx0 = (int8x16_t)vld1q_dup_s32((int*)(void*)&x[j]); | ||
| + vx0 = vreinterpretq_s8_s32(vld1q_dup_s32((const int32_t*)(const void*)&x[j])); |
There was a problem hiding this comment.
The original casts seem almost certainly incorrect and maybe the real reason they went through void* first is to fix a const problem?
| } | ||
| """ | ||
|
|
||
| NASM_WRAPPER_CC = """#include <stdio.h> |
There was a problem hiding this comment.
Where did these blobs of code come from?
| int main(int argc, char** argv) { | ||
| std::vector<char*> args; | ||
| args.reserve(static_cast<size_t>(argc) + 1); | ||
| args.push_back(const_cast<char*>("{tool_path}")); |
There was a problem hiding this comment.
| args.push_back(const_cast<char*>("{tool_path}")); | |
| char tool_path_buffer[] = "{tool_path}"; | |
| args.push_back(tool_path_buffer); |
It's not safe to make a literal mutable.
ports/webrtc/portfile.cmake
Outdated
| vcpkg_replace_string( | ||
| "${SOURCE_PATH}/build/config/win/BUILD.gn" | ||
| " # Desktop Windows: static CRT.\n configs = [ \":static_crt\" ]\n" | ||
| " # Vcpkg's x64-windows triplet expects the dynamic CRT for consumers.\n configs = [ \":dynamic_crt\" ]\n" |
There was a problem hiding this comment.
That's true but I'm not sure the comment is meaningful. After all it would also trigger for e.g. arm64-windows, or any custom triplet where VCPKG_CRT_LINKAGE is set to dynamic.
ports/webrtc/portfile.cmake
Outdated
| ) | ||
|
|
||
| function(fetch_webrtc_repo source_path repo_spec) | ||
| string(REPLACE "|" ";" REPO_FIELDS "${repo_spec}") |
There was a problem hiding this comment.
Instead of making up a custom format and then trying to parse that out, why don't we just record this in the "exploded" form to begin with?
There was a problem hiding this comment.
Isn't this something the skia port handles with declare_external_from_git/get_externals(${required_externals})?
dg0yt
left a comment
There was a problem hiding this comment.
The CMake config and targets shall be prefixed as unofficial.
I wish we could share more helper functions between the gn-based ports.
ports/webrtc/portfile.cmake
Outdated
| ) | ||
|
|
||
| function(fetch_webrtc_repo source_path repo_spec) | ||
| string(REPLACE "|" ";" REPO_FIELDS "${repo_spec}") |
There was a problem hiding this comment.
Isn't this something the skia port handles with declare_external_from_git/get_externals(${required_externals})?
ports/webrtc/portfile.cmake
Outdated
| message(FATAL_ERROR "webrtc currently supports only x64-linux, arm64-linux, arm64-osx, x64-osx, arm64-windows, x86-windows, and x64-windows.") | ||
| endif() | ||
|
|
||
| include("${CMAKE_CURRENT_LIST_DIR}/source-manifest.cmake") |
There was a problem hiding this comment.
I really dislike this being in a different file. There is no need to add variables for a single use.
ports/webrtc/portfile.cmake
Outdated
| @@ -0,0 +1,484 @@ | |||
| vcpkg_check_linkage(ONLY_STATIC_LIBRARY) | |||
| include("${CURRENT_HOST_INSTALLED_DIR}/share/vcpkg-cmake-get-vars/vcpkg-port-config.cmake") | |||
There was a problem hiding this comment.
This doesn't need to be included explicitly - having a dependency on that port is sufficient for the effect.
ports/webrtc/webrtcConfig.cmake.in
Outdated
| find_dependency(absl CONFIG REQUIRED) | ||
| find_dependency(AOM CONFIG REQUIRED) | ||
| find_dependency(libyuv CONFIG REQUIRED) | ||
| find_dependency(unofficial-libvpx CONFIG REQUIRED) | ||
| find_dependency(Opus CONFIG REQUIRED) | ||
| find_dependency(OpenSSL CONFIG REQUIRED) | ||
| find_dependency(libSRTP CONFIG REQUIRED) | ||
| find_dependency(jsoncpp CONFIG REQUIRED) | ||
| find_dependency(pffft CONFIG REQUIRED) |
There was a problem hiding this comment.
Do not use REQUIRED with find_dependency.
|
Thanks for the review! Since this is a large patch set and I’ve been working on it for almost four weeks, some patch and compiler-flag comments still need cleanup. I also want to make sure each patch has a correct explanation and is revalidated, at least on |
I’m not a fan of googlesource.com (Gitiles), since its archive handling is not especially stable, but for WebRTC we do not really have a better option. The GitHub archive for chromium/chromium is about By comparison,
Among the other Chromium-based projects in vcpkg, I think there is room to share some code in the future, but because vcpkg is intentionally modular, I am not yet sure what the right abstraction would be without risking breakage or weakening that modularity. For now, I would prefer to keep the code separate. For the other libraries that use CMake as their native build system in vcpkg, such as |
|
Note: I’ve made the local changes and submitted them to CI, but I don’t have time to add detailed review replies right now since it’s morning in my time zone and I need to get ready for work. I’ll follow up later today (or tomorrow) with comment-by-comment responses. |
Owner-Projectform.vcpkg.json, or explicitly disabled through patches or build system arguments such as CMAKE_DISABLE_FIND_PACKAGE_Xxx or VCPKG_LOCK_FIND_PACKAGEvcpkg.jsonmatches what upstream says.vcpkg.jsonmatches what upstream says../vcpkg x-add-version --alland committing the result.Fixes #6205
Fixes #12134
Fixes #22646
Summary
This PR adds a new
webrtcport.The port builds upstream WebRTC with its native GN-based build system, unbundles most third-party dependencies to existing vcpkg ports, and installs a consumable
webrtc::webrtcCMake package.Because WebRTC is a large Chromium-related project with a non-trivial source layout and build graph, this port intentionally takes a synthetic-tree approach for a number of
third_partyand build-integration pieces instead of trying to maintain a separate non-GN build system.What is WebRTC?
WebRTC is a free and open project that provides real-time communication capabilities for audio, video, and data exchange.
People sometimes call the native C++ codebase
libwebrtcto distinguish it from the browser WebRTC API, but upstream itself uses thewebrtcname for the project and source tree.Design decisions
Use GN
GN is the native build system for Chromium-related projects.
Unlike some related projects such as V8 or Skia, WebRTC does not have a maintained alternative build system such as CMake. Also, the source tree is large enough that maintaining a separate build description in vcpkg would be unrealistic and high-maintenance.
For that reason, this port builds WebRTC with GN directly.
Synthetic
third_partytreeTo build WebRTC, upstream expects a large set of related source trees, especially under Chromium
third_partylayout.In practice, for vcpkg packaging we do not need most of that tree:
This port therefore unbundles most dependencies and synthesizes the minimum GN-visible tree surface needed to keep the upstream build working while using vcpkg-provided dependencies.
That is the main reason this port looks more complex than a typical CMake port.
To maintain the synthetic
third_partytree, I also added small Python helper scripts and tests for them.Dependency model
This port uses vcpkg ports for major third-party dependencies such as:
abseilopenssllibsrtpopuslibvpxlibyuvaomjsoncpppffftUnsupported or unneeded optional pieces are disabled through GN args and patches rather than being left to accidental host discovery.
H.264 support
H.264 support is intentionally deferred for now.
Although H.264 is one of the most commonly used video codecs, enabling it in WebRTC requires deeper integration work around
openh264andffmpeg, and would need more invasive tree patching than the current port.There is also a licensing concern: H.264 is patent-encumbered, and using OpenH264 from source does not automatically give downstream users the same protection Cisco provides for their distributed binaries.
For now, I prefer to leave H.264 unsupported until there is a cleaner technical and policy story for it.
Supported triplets
The current
supportsset inports/webrtc/vcpkg.jsonis:x86-windowsx64-windowsarm64-windows-static/-static-mdvariants as supported by the port metadatax64-linuxarm64-linuxx64-osxarm64-osxThese triplets are supported by the port, but the validation depth is not identical across them.
Current validation I ran during bring-up includes:
x64-linuxarm64-linuxx64-windowsx64-windows-static-mdarm64-osxI also added support for
arm64-windows, but I have not personally validated that triplet on real hardware.Explicitly deferred
For now, this port explicitly does not support mobile targets.
I want to make the first desktop/server port solid before expanding further.
Android is especially non-trivial because upstream WebRTC ships Java sources that are not just a thin wrapper layer. Some important functionality, including parts of media codec integration, depends on that Android-side Java stack. That makes Android support significantly more involved than just enabling another GN target.
Validation
To make review easier, I also included a small sample consumer project for the port:
https://github.com/bc-lee/vcpkg-webrtc-sample
I used that sample during bring-up to validate package consumption in addition to direct
vcpkg installtesting.Notes
This is still an early draft of the port.
Because the project is large and the packaging strategy is more involved than usual, I would like feedback from both maintainers and real users before treating the current shape as final. CI coverage and wider triplet usage will probably expose additional cleanup opportunities.