Fix remote video admission for outgoing calls - #145
Conversation
|
Warning Review limit reachedNext included review available in 35 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (14)
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (22)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change adds outgoing-call acceptance handling, native call fixtures, test-support wiring, and remote-video validation. It also replaces WebCodecs timer-based synchronization with explicit copy and frame-consumption waits. ChangesOutgoing call acceptance
WebCodecs readback synchronization
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: ⚪ Minimal · up to No actionable merge-blocking risk is established beyond normal checks. Sequence Diagram(s)sequenceDiagram
participant WhatsAppClient
participant CallRegistry
participant SessionBridge
participant Frames
WhatsAppClient->>CallRegistry: Forward call accept advertisement
CallRegistry->>CallRegistry: Validate outgoing target and peer state
CallRegistry->>SessionBridge: Emit outgoing call states
SessionBridge->>Frames: Apply call state updates
Frames->>Frames: Create remote decoder when remote video is expected
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 68.49% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 73 functions across 13 files. (9 skipped: 9 unsupported.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. A rabbit checks each call, Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 976f2c5bbf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
1 participant
Checkpoint
Stacked on #140, which is stacked on #139. This isolates outgoing-call activation from the previous camera, rotation and readback fixes. Requires oxidezap/whatsapp-rust#1472 for ordered source-bearing video events and the native test fixture. All WhatsApp dependencies resolve to that branch at a45320848ce4808bdcc2b4a7436f2a7f1e0c66a0.
Proven Defect
In the failed web-originated call, 166 remote access units reached the session and were published toward the GUI, including three marked keyframes. No remote decoder started. Outgoing acceptance announced only Local=true; without a separate peer Enabled event, Remote remained false and Frames discarded the incoming video. The working Android-originated path explicitly announced both directions. Both logs used the same deployed preview bundle.
Fix
Tests And Evidence
The native fixture completes real Noise XX and login, drives the public outgoing builder, holds actual offer-send completion and injects stanzas through the library parser/handlers. It does not fabricate a winner or OutgoingReady.
The production-chain regression runs library events through EventLanes, the session handler, daemon state, GUI Frames and generated H.264 decoding. It fails before the remote-admission correction and passes after. Sixteen cases cover early acceptance, audio-only and video accepts, absent/invalid orientation, siblings, PN addressing, wrong-user selection, 300 unrelated accepts, duplicates, closed/replaced cameras and teardown. Ordering tests cover delayed consumers, pre-activation replay, queue pressure and legacy-only input.
During verification, the shared-WASM readback control exposed a test synchronization bug. A single timer turn did not guarantee the BGRA probe and Rust continuations completed. Tests now wait for actual copy and closure milestones, with deadlines only for failure. All ordinary and shared suites pass; a duplicate-copy mutation still fails the exact-count assertion. No production readback change was made.
Verification
Remaining Boundary
This fixes the proven client-side rejection of remote video. It does NOT establish that Android now displays the outbound web picture.
Separate research captured synthetic caller-before-accept, caller-after-rekey and callee video at RelayTransport::send. Independent Node/OpenSSL verification and FFmpeg decoding succeeded for all three. This covers outgoing packet construction, not browser networking, real relay forwarding or Android ingress. No speculative wire change was made.
The upstream fixture still exposes existing authorization weaknesses and the handle queue remains lossy under pressure. Those are documented, not claimed fixed here. Native fixture stanza injection bypasses inbound Noise framing; no physical Android decoder, live call or GPUI browser painting was used. A live retest of both remote pictures is still required.
Summary by cubic
Fixes remote video admission for outgoing web-originated calls so the incoming stream is decoded and displayed. Acceptance previously announced only the local camera, so
Remotestayed false and inbound frames were discarded; the video child from the winning accept is now preserved and activated once.Bug Fixes
PeerVideoStateChangedhandle queue, ignoring legacy companions and globalVideoStateevents.Dependencies
test/voip-call-fixturefor ordered source-bearing video events and the native call fixture; fixture exports are gated behind atest-supportfeature so production builds stay clean.Written for commit 273f0fe. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Documentation