Repository navigation
webrtc-sys: Resolve memory leak for server-initiated room closure - #1473
Conversation
Changeset ✓This PR includes a changeset covering all affected packages:
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 1 potential issue.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
There was a problem hiding this comment.
🟡 Failed unpublish sends contradictory binding events
When sender removal fails, unpublish_track now emits LocalTrackUnpublished but returns an error. The FFI unpublish request forwards that event, then reports failure to its caller.
(Refers to this code)
Learn more
The room emits LocalTrackUnpublished from this callback. The FFI event handler forwards it and removes the publication lookup entry. Meanwhile, the FFI request treats unpublish_track's returned error as a failed operation and sends an error callback. A closed peer connection triggers this mismatch even though the local track has been detached. Clients can receive a publication-removed event alongside a failed unpublish request.
Example: A participant unpublishes camera TR_1 while a server deletion closes its connection. Sender removal fails, so the SDK detaches TR_1 and sends LocalTrackUnpublished; the FFI request then reports an error for the same unpublish.
Recommended fix: Decide how the API represents completed local cleanup after sender-removal failure. Coordinate the unpublish_track return value with FFI's callback and event forwarding so the same operation has one consistent outcome; preserve the underlying removal error through logging or a distinct diagnostic if it cannot be returned as the operation result.
Was this helpful? React with 👍 or 👎 to provide feedback.
xianshijing-lk
left a comment
xianshijing-lk
left a comment
There was a problem hiding this comment.
lgtm with one nit.
11c1b73 to
efcb517
Compare
efcb517 to
10f4813
Compare
alan-george-lk
changed the title
10f4813 to
d4b5c40
Compare
## Joint Summary Stacked/works in junction with #1473. Relationship: - #1473: releases local publications and tracks after remote room deletion, allowing their encoders to be destroyed - #1500: terminates VAAPI displays at the correct lifecycle boundaries, releasing the associated driver resources See #1473 for full write up and test results. ## This PR Summary - Make the platform VAAPI display wrappers own the complete display lifecycle: acquire and initialize once, then call `vaTerminate` during `Close`/destruction. - Keep capability-probe displays valid while they are queried and terminate them when the probe's display owner goes out of scope. - Keep encoder displays initialized for the full encoder lifetime, then release VA buffers, context, surfaces, and configuration before terminating the display. - Make encoder teardown idempotent and safe after partial initialization or reinitialization. - Propagate VAAPI encoder initialization failures instead of continuing with an unusable encoder. - Replace manual entrypoint arrays with scoped storage and improve VAAPI initialization diagnostics. This resolves the premature-display-termination issue identified in review while retaining cleanup of the VA driver resources and threads created by capability detection. ## Validation Tested on Linux with an AMD VAAPI device and an RTX 5090: - Release C++ SDK/test build succeeds with LLVM 21. - Forced-VAAPI `local_video` test-pattern publishing continuously encoded and sent 329/329 frames in 12 seconds using `VAAPI H264 Encoder`. The previous implementation stalled after its first frame. - See #1473 for full 100 cycle test results. --------- Co-authored-by: Cursor <cursoragent@cursor.com>
3 participants
Joint Summary
Stacked/works in junction with #1500. Relationship:
These in combination fix a fairly hefty memory leak reported by a customer. Their use case is remotely killing a room via server API, which kills the client room locally, and joining another room in the same client process.
This PR Summary
Previously,
unpublish_trackcalledremove_track(sender)?first. When a remote room deletion had already closed the connection, that call errored and returned early, leaving thepublication → track → transceivergraph attached, which could retain the room session.The patch:
remove_trackresult instead of returning immediately.The added reconnection test publishes a local video track, deletes the room remotely, then verifies both that the publication loses its track and that the room session can actually drop. That directly demonstrates the retained-reference lifecycle issue is resolved.
Testing/Validation
A standalone C++ app was created to reproduce the behavior that runs relevant code in cycles (described below). The SDK is initialized once for the entire run with
MALLOC_ARENA_MAX=1set to tighten memory measurements. The environment was a Linux desktop with a 5090.For each of the 100 cycles:
lk --dev room create.lk --dev room delete <room name>.Note: The first 20 cycles are treated as warmup, growth is measured from cycle 20 through cycle 100.
Results Pre-PRs
Fresh clean build and 100-cycle run completed on Rust
maincommit535ead0d.Results Post-PRs
Note: this combines this branch + #1500.
Customer Validation
I cut a specific C++ SDK branch build against this Rust fix and the customer confirmed this resolved the problem.