Get build/tests working for M114 by kevindhanna · Pull Request #49 · WonderInventions/node-webrtc

kevindhanna · 2026-03-20T12:52:40Z

Hi there 👋

I got the build and I think all the required changes for M114 working, and got the test suite to run reliably

FYI Claude Code was used when making these changes, hopefully that's ok (though I see from your develop commits maybe you're using it as well)

Threading & lifecycle

Separate Network Thread: M114 moved operations to the network thread, and PeerConnection::Close() does sequential BlockingCalls to nework then worker. Sharing the thread caused contention, so changed to match Chrome's architecture
DataChannel close even ordering: PeerConnection::Close() calls PrepareForShutdown() which deactivates the SafeTask safety flag, silently dropping OnStateChange callbacks. Close events are now dispatched before the `Close()
SIngle observer registration: SctpDataChannel uses an ObserverAdapter that relays callbacks via SafeTask. There was a race between RegisterObserver calls (DataChannelObserver and RTCDataChannel) that resulted in messages being silently dropped when sent on dc.onopen

Build

Link SimulcastEncoderAdapter: library was never linked, causing a segfault
macOS 26 SDK compatibility: GC_AVAILABLE_BUT_DEPRECATED has een removed since 26 Tahoe, so we patch a fallback
nix.gni fallback: use some default values when outside nix

Tests

Remove simple-peer dependency: It's unmaintained for 4 years
Fix flakey audio sink tests: count events instead of using a fixed runtime
msid deduplication test: align with with M114 behaviour

…tion Since M111 (commit e04c3970994b), RtpSenderBase::set_stream_ids() deduplicates stream IDs before they reach SDP serialization. This aligns with the W3C WebRTC spec: "For each stream in streams, add stream.id to [[AssociatedMediaStreamIds]] if it's not already there." The test creates stream2 and stream3 with the same id "testStreamId", so M114 correctly deduplicates: 2 unique stream IDs × 2 tracks = 4 msid lines, not 6. See: https://webrtc-review.googlesource.com/c/src/+/288701 See: webrtc bug 14769

duvallj

Thanks for the PR!! I too have been using Claude Code, and you seem to have used it responsibly since the code changes are overall of good quality. I do have some feedback I'd like to be addressed before this can be merged however:

duvallj · 2026-03-20T17:31:03Z

src/interfaces/rtc_ice_transport.cc

+  default:
+    break;


just a compiler warning fix:
src/interfaces/rtc_ice_transport.cc:162:3: warning: 'switch' missing 'default' label [-Wswitch-default]

I'll undo it

duvallj · 2026-03-20T17:31:24Z

src/interfaces/rtc_data_channel.cc

  if (_jingleDataChannel != nullptr) {
-    Stop();
+    CleanupInternals();
+    HandleStateChange(*this, webrtc::DataChannelInterface::kClosed);


re-added Stop(), seems important 😄

duvallj · 2026-03-20T17:33:11Z

src/node/error_factory.cc

  case kOperationError:
    return "OperationError";
+  default:
+    assert(false && "unhandled DOMExceptionName enum value");


Don't panic here, find a way to throw to Javascript if possible

JS should get the UnknownError in release builds, but we don't explicitly set NDEBUG so maybe best leave the assert out?

test/custom-settings.js

test/multiconnect.js

duvallj · 2026-03-20T17:39:35Z

CMakeLists.txt

  BUILD_BYPRODUCTS  ${byproducts}

  DOWNLOAD_COMMAND  ${CMAKE_COMMAND} -E env DEPOT_TOOLS=${depot_tools_install_dir} PLATFORM=${PLATFORM} WEBRTC_REVISION=${WEBRTC_REVISION} ${CMAKE_SOURCE_DIR}/scripts/download-webrtc.${suffix}
+  PATCH_COMMAND     ${CMAKE_SOURCE_DIR}/scripts/patch-webrtc.sh <SOURCE_DIR> ${CMAKE_SOURCE_DIR}/patches


I'm not crazy about having to patch, but there are some other patches I've had to apply on Windows, so overall this functionality is OK, so long as it stays minimal.

Dispatch close events proactively during RTCPeerConnection.close(), before calling the underlying PeerConnection::Close(). This matters because PeerConnection::Close() calls PrepareForShutdown() (data_channel_controller.cc:173) which deactivates the SafeTask safety flag, silently cancelling any pending OnStateChange callbacks from the network thread. By dispatching close events first, they reach JS regardless of the C++ shutdown sequence.

The macOS 26 (Tahoe) SDK removed the CG_AVAILABLE_BUT_DEPRECATED macro from CoreGraphics. This macro was used in M114's modules/desktop_capture/mac/screen_capturer_mac.mm to provide correct deprecation annotations for CGDisplayStream wrapper functions (a workaround for incorrect annotations in the 13.3 SDK, see https://crbug.com/1431897). The patch adds a fallback #define that maps CG_AVAILABLE_BUT_DEPRECATED to API_DEPRECATED when the macro is not provided by the SDK, preserving the deprecation annotations on older SDKs while allowing compilation on macOS 26+. This only affects macOS builds — the file is not compiled on Linux or Windows. Also adds a general-purpose patch script (scripts/patch-webrtc.sh) and PATCH_COMMAND to the libwebrtc ExternalProject in CMakeLists.txt, so patches are applied automatically and idempotently during the build.

…le-peer Replace the simple-peer dependency with direct RTCPeerConnection usage. simple-peer is unmaintained (last release 2021) and was only used in these two test files.

…based The timing approach was flakey due to event loop timing jitter.

BuiltinVideoEncoderFactory::CreateVideoEncoder() constructs a SimulcastEncoderAdapter (builtin_video_encoder_factory.cc:45), but librtc_simulcast_encoder_adapter.a was never linked into the final binary. The constructor symbol resolved to 0x0, causing a segfault on the EncoderQueue when sending video frames through a negotiated peer connection. Found via `nm -g wrtc.node | grep "U "` which showed a single undefined webrtc symbol: SimulcastEncoderAdapterC1.

…loss In M114, SctpDataChannel uses an ObserverAdapter that relays callbacks through the signaling thread via SafeTask. When RegisterObserver was called twice (first from DataChannelObserver's constructor, then from RTCDataChannel's constructor), the adapter's SetDelegate call could race with in-flight message delivery tasks on the signaling thread, causing messages sent immediately after dc.onopen to be silently lost. The fix removes the initial RegisterObserver from DataChannelObserver, so registration only happens once in RTCDataChannel's constructor. The SCTP layer queues any messages that arrive before an observer is registered, and DeliverQueuedReceivedData() delivers them when RegisterObserver is called. Also dispatches the "open" state change event if the channel has already transitioned to kOpen before the RTCDataChannel wrapper is constructed, ensuring JS onopen handlers fire.

Previously, the same thread was passed for both network_thread and worker_thread to CreatePeerConnectionFactory. In M114, many operations moved to the network thread, and PeerConnection::Close() does sequential BlockingCalls to network then worker threads (pc/peer_connection.cc:1909-1929). Sharing a single thread caused cleanup contention that limited sequential connection throughput. This matches Chrome's architecture (pc/connection_context.cc:85-98) where network, worker, and signaling threads are all separate. The network thread gets a socket server for ICE port management, matching the CreateWithSocketServer() pattern Chrome uses.

CMakeLists.txt previously required a nix.gni file (generated by the Nix shell hook) containing platform-specific GN args. Builds outside Nix failed with "file STRINGS file nix.gni cannot be read". Now falls back to sensible defaults (is_clang=true, use_lld=false, clang_use_chrome_plugins=false) when the file doesn't exist. Also add a default UknownError path for ErrorFactory::DOMExceptionNameToString

kevindhanna changed the title ~~Final M114~~ Get build/tests working for M114 Mar 20, 2026

kevindhanna force-pushed the develop branch 5 times, most recently from 2b04f68 to 7d785f2 Compare March 20, 2026 13:53

kevindhanna force-pushed the develop branch from 7d785f2 to 62be1b0 Compare March 20, 2026 13:56

duvallj requested changes Mar 20, 2026

View reviewed changes

kevindhanna force-pushed the develop branch 3 times, most recently from 24c8ab3 to a255fd2 Compare March 21, 2026 15:20

kevindhanna added 2 commits March 21, 2026 15:21

kevindhanna force-pushed the develop branch 2 times, most recently from 89c7df4 to 406136f Compare March 21, 2026 15:27

kevindhanna added 6 commits March 21, 2026 15:28

refactor(test): rewrite multiconnect and custom-settings without simp…

8ebbaa9

…le-peer Replace the simple-peer dependency with direct RTCPeerConnection usage. simple-peer is unmaintained (last release 2021) and was only used in these two test files.

fix(test): replace time-based audio sink assertions with event-count-…

ae0fa0d

…based The timing approach was flakey due to event loop timing jitter.

kevindhanna force-pushed the develop branch from 406136f to ab7942a Compare March 21, 2026 15:29

kevindhanna requested a review from duvallj March 21, 2026 15:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get build/tests working for M114#49

Get build/tests working for M114#49
kevindhanna wants to merge 9 commits intoWonderInventions:developfrom
kevindhanna:develop

kevindhanna commented Mar 20, 2026 •

edited

Loading

Uh oh!

duvallj left a comment

Uh oh!

duvallj Mar 20, 2026

Uh oh!

kevindhanna Mar 21, 2026

Uh oh!

duvallj Mar 20, 2026

Uh oh!

kevindhanna Mar 21, 2026

Uh oh!

duvallj Mar 20, 2026

Uh oh!

kevindhanna Mar 21, 2026

Uh oh!

Uh oh!

Uh oh!

duvallj Mar 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

kevindhanna commented Mar 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Threading & lifecycle

Build

Tests

Uh oh!

duvallj left a comment

Choose a reason for hiding this comment

Uh oh!

duvallj Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

kevindhanna Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

duvallj Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

kevindhanna Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

duvallj Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

kevindhanna Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

duvallj Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kevindhanna commented Mar 20, 2026 •

edited

Loading