Skip to content

webrtc-sys: detach the frame transformer when a FrameCryptor is destroyed - #1408

Open
ctarabocchia wants to merge 1 commit into
livekit:mainfrom
wherryapp:wherry/frame-cryptor-detach
Open

webrtc-sys: detach the frame transformer when a FrameCryptor is destroyed#1408
ctarabocchia wants to merge 1 commit into
livekit:mainfrom
wherryapp:wherry/frame-cryptor-detach

Conversation

@ctarabocchia

Copy link
Copy Markdown

rust-sdks-frame-cryptor-detach.patch

One commit on top of livekit/rust-sdks at dee418bb (the revision
Cargo.toml pins): FrameCryptor::~FrameCryptor in
webrtc-sys/src/frame_cryptor.cpp detaches the transformer from the
RtpSender/RtpReceiver it was attached to. Without it every cryptor
leaves its FrameCryptorTransformer thread behind until the peer
connection is destroyed — one thread per cryptor per call, never
reclaimed (docs/prompts/native-media-plan.md §5.1, gate 1).

Verified 2026-09-07 on the dev Mac against a browser peer: 20 leaked
threads after 10 calls on the unpatched bridge; 0 after 6 calls with the
patch, twice, with the audio still sealed both ways.

In the build since 2026-09-07 (evening) through the fork:
wherryapp/rust-sdks, branch wherry/frame-cryptor-detach, commit
bf2ea2d59290d1411d6e659229ade397bb7e667d — the [patch] section in
client/src-tauri/Cargo.toml. The upstream pull request has not been
opened
(the session that would have opened it was not allowed to);
the branch is pushed and the command is:

gh pr create --repo livekit/rust-sdks --base main \
  --head wherryapp:wherry/frame-cryptor-detach \
  --title "webrtc-sys: detach the frame transformer when a FrameCryptor is destroyed" \
  --body-file client/src-tauri/patches/rust-sdks-frame-cryptor-detach.md

(the body below the rule is written to be that PR's description). When it
merges, delete the [patch] section and bump rev with LIVEKIT_REV.

Cargo cannot apply a patch file to a git dependency, so the choices were:

  • fork livekit/rust-sdks (under the wherryapp org), git am this
    file on a branch from dee418bb, and add to client/src-tauri/Cargo.toml:

    [patch."https://github.com/livekit/rust-sdks"]
    webrtc-sys = { git = "https://github.com/wherryapp/rust-sdks", rev = "<that commit>" }

    (only webrtc-sys is patched; livekit itself keeps coming from
    upstream at the pinned rev, and webrtc-sys-build follows the patched
    crate's workspace), or

  • get the change merged upstream and bump rev in Cargo.toml together
    with LIVEKIT_REV in src/voice.rs.

To reproduce the measurement build locally without a fork:

git clone https://github.com/livekit/rust-sdks /tmp/rust-sdks
git -C /tmp/rust-sdks checkout dee418bba599fd505fa548ac6b9ab3379fc548fc
git -C /tmp/rust-sdks am /path/to/rust-sdks-frame-cryptor-detach.patch
# then, temporarily, in client/src-tauri/Cargo.toml:
# [patch."https://github.com/livekit/rust-sdks"]
# webrtc-sys = { path = "/tmp/rust-sdks/webrtc-sys" }

Upstream issue — drafted, not filed

Title: webrtc-sys: FrameCryptor never detaches its transformer, leaking one FrameCryptorTransformer thread per cryptor per room

FrameCryptor's constructors (webrtc-sys/src/frame_cryptor.cpp) attach a
FrameCryptorTransformer to the RtpSender / RtpReceiver with
SetEncoderToPacketizerFrameTransformer / SetDepacketizerToDecoderFrameTransformer.
FrameCryptor::~FrameCryptor only unregisters the observer; it never clears
the transformer on the sender/receiver, and the bindings expose no way for
the Rust side to. libwebrtc keeps a transformer for as long as it is set on
the sender/receiver, and the transformer owns a thread that stops only in
its destructor, so the thread outlives the FrameCryptor, the Room and
every handle the application holds.

Reproduction (macOS 15, Apple Silicon, rev = dee418bb, livekit with
rustls-tls-native-roots, E2EE Gcm with KeyDerivationAlgorithm::HKDF):

  1. Room::connect with E2eeOptions, publish one microphone audio track
    (PlatformAudio), subscribe to one remote audio track.
  2. unpublish_track, Room::close, drop the Room.
  3. Repeat N times, then sample <pid> 1 | grep -c FrameCryptorTransformer.

Observed: the count grows by one per cryptor per call and never comes
back — 20 FrameCryptorTransformer threads after 10 calls that each had a
sender and a receiver cryptor. E2eeManager::cleanup does run on close
(it disables the cryptors and clears the map, so the Rust FrameCryptor
is dropped), which is what points at the C++ destructor.

Fix that measured clean here — clear the transformer in the destructor:

FrameCryptor::~FrameCryptor() {
  if (observer_) {
    unregister_observer();
  }
  if (sender_) {
    sender_->SetEncoderToPacketizerFrameTransformer(nullptr);
  }
  if (receiver_) {
    receiver_->SetDepacketizerToDecoderFrameTransformer(nullptr);
  }
}

With this, the same soak reads 0 threads after 6 calls (12 cryptors), and
encryption state stays Ok at both ends. Happy to open a PR.

@ctarabocchia
ctarabocchia requested a review from ladvoc as a code owner September 7, 2026 18:02
@CLAassistant

CLAassistant commented Sep 7, 2026

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 potential issue.

1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)

Devin Review

Comment on lines +177 to +181
if (sender_) {
sender_->SetEncoderToPacketizerFrameTransformer(nullptr);
}
if (receiver_) {
receiver_->SetDepacketizerToDecoderFrameTransformer(nullptr);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Cryptor destruction removes other transforms

Dropping a cryptor after set_packet_trailer_handler also detaches the retained packet-trailer handler. A stale cryptor can likewise detach a replacement cryptor.

Prompt for agents
FrameCryptor::~FrameCryptor unconditionally sets the sender or receiver transformer to null. FrameCryptor::set_packet_trailer_handler replaces that slot with chained_transformer_, which contains both the cryptor and a PacketTrailerTransformer. Clearing the slot therefore disables packet-trailer processing even when the independently retained PacketTrailerHandler remains alive. The same ownership problem lets an older FrameCryptor clear a newer transformer installed on the same sender or receiver when its final SharedPtr is dropped later. Redesign teardown so a cryptor removes only the transformer installation it owns and preserves or restores other processing. Account for replacement cryptors and chained packet-trailer handlers rather than blindly clearing the shared endpoint slot.
Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

ctarabocchia added a commit to wherryapp/wherry that referenced this pull request Sep 7, 2026
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…oyed

FrameCryptor's constructors attach a FrameCryptorTransformer to the
RtpSender or RtpReceiver they were given, and the destructor only
unregistered the observer. libwebrtc keeps a transformer alive for as long
as it is set on the sender/receiver, so every FrameCryptor left its
transformer -- and the thread that transformer owns -- behind until the
peer connection was destroyed. Measured from a Room that is connected,
publishes one audio track under E2EE and closes: one leaked
FrameCryptorTransformer thread per cryptor per room, never reclaimed.

Clear the transformer on the sender/receiver in the destructor.
@ctarabocchia
ctarabocchia force-pushed the wherry/frame-cryptor-detach branch from bf2ea2d to 73ff28c Compare September 7, 2026 21:03
ctarabocchia added a commit to wherryapp/wherry that referenced this pull request Sep 7, 2026
… and the fork commit is re-authored

Both gates the native media plan named are answered, so the per-device
checkbox in Settings → Voice starts ticked on a fresh desktop device
(client/src/voice/prefs.ts; a device that unticks it keeps its choice; the
web and the phones have no engine and read the value as nothing). The
maintainer's decision, made the same evening the second gate closed.
Verified on macOS from tauri dev and one debug bundle; believed for the
bundled release build and for Windows, which has never run the engine --
docs/prompts/windows-rebuild-plan.md is the plan for that pass, written
for the Windows machine.

The fork commit carrying the cryptor fix (livekit/rust-sdks#1408) was
re-authored to the maintainer's GitHub noreply address so the CLA bot can
link it -- a real address is refused by the account's email-privacy
setting on push -- and force-pushed; Cargo.toml's [patch] rev, Cargo.lock,
the patch files and every doc mention now name 73ff28ce. Signing the CLA
is the maintainer's own step.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants