VoIP 1:1 video calls - #1024
VoIP 1:1 video calls#1024
Conversation
📝 WalkthroughWalkthroughThis PR adds end-to-end VoIP video support across signaling, H.264 RTP media, SRTP/SRTCP, engine and drive-loop control, public facade APIs, call registry coordination, and an ffmpeg/ffplay-backed CLI. ChangesVoIP video plane
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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: 5d821709ff
ℹ️ 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".
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
Baseline: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/voip-cli/src/video.rs`:
- Around line 77-91: Convert ensure_tool to an async function using
tokio::process::Command and await its status probe instead of blocking with
std::process::Command. Propagate async and await through spawn_video_source and
spawn_video_sink and their async callers, including run_video_loopback and the
relevant main.rs paths. In spawn_video_sink, perform the tool probe once, reuse
its result for both the warning and use_window decision, and preserve the
existing error handling and behavior.
In `@wacore/src/stanza/call.rs`:
- Around line 707-738: Add a stanza id to the video-state call path by extending
VideoStateParams with an id value or generating one before build_video_state,
then apply it to the enclosing call wrapper produced by build_video_state.
Ensure every video upgrade and downgrade sent through send_node carries the id
needed for typed video-ack correlation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 284846bf-68b4-40a5-ac7a-11f506ba1182
📒 Files selected for processing (18)
examples/voip-cli/src/main.rsexamples/voip-cli/src/video.rssrc/handlers/call.rssrc/voip/facade.rssrc/voip/mod.rssrc/voip/transport.rssrc/voip/video.rswacore/benches/voip_benchmark.rswacore/src/stanza/call.rswacore/src/types/call.rswacore/src/voip/driver.rswacore/src/voip/engine.rswacore/src/voip/h264.rswacore/src/voip/mod.rswacore/src/voip/registry.rswacore/src/voip/rtp.rswacore/src/voip/session.rswacore/src/voip/ssrc.rs
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 18 files
Confidence score: 5/5
- Safe to merge after the addressed issues were fixed.
You’re at about 90% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
|
@codex review |
5d82170 to
d605f22
Compare
d605f22 to
fe4b0a3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/voip-cli/src/video.rs`:
- Around line 61-66: Update the WA_VIDEO_INPUT parsing match so existing paths
are classified as VideoInput::Media only when they are not Linux device nodes;
allow values such as /dev/video0 to fall through to the VideoInput::Webcam arm
while preserving URL and regular media-path handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5864883f-07fe-4502-b5fb-966db4f6f74e
📒 Files selected for processing (18)
examples/voip-cli/src/main.rsexamples/voip-cli/src/video.rssrc/handlers/call.rssrc/voip/facade.rssrc/voip/mod.rssrc/voip/transport.rssrc/voip/video.rswacore/benches/voip_benchmark.rswacore/src/stanza/call.rswacore/src/types/call.rswacore/src/voip/driver.rswacore/src/voip/engine.rswacore/src/voip/h264.rswacore/src/voip/mod.rswacore/src/voip/registry.rswacore/src/voip/rtp.rswacore/src/voip/session.rswacore/src/voip/ssrc.rs
fe4b0a3 to
39e906b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 39e906b2e0
ℹ️ 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".
39e906b to
865d3f2
Compare
865d3f2 to
9aca2cc
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/handlers/call.rs`:
- Around line 177-182: Update the cancellation handling around
build_call_video_ack in the call handler so *cancelled is set to true only after
a typed video ack is successfully built. If the builder returns None, leave
cancellation unchanged so the router can send its generic acknowledgement;
preserve the existing send_node error warning for successfully built typed
acknowledgements.
In `@wacore/src/voip/registry.rs`:
- Around line 201-209: Update run_video_teardown to avoid invoking the
video_teardown callback while holding inner: store the hook as an Arc-compatible
value, clone it while the registry lock is held, release the lock, then invoke
the cloned callback. Preserve the existing no-op behavior when the call ID or
hook is absent and align with the lock-release pattern used by the other
dispatch helpers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f6eb3eb4-d049-47d9-8836-25deddb0ad8b
📒 Files selected for processing (18)
examples/voip-cli/src/main.rsexamples/voip-cli/src/video.rssrc/handlers/call.rssrc/voip/facade.rssrc/voip/mod.rssrc/voip/transport.rssrc/voip/video.rswacore/benches/voip_benchmark.rswacore/src/stanza/call.rswacore/src/types/call.rswacore/src/voip/driver.rswacore/src/voip/engine.rswacore/src/voip/h264.rswacore/src/voip/mod.rswacore/src/voip/registry.rswacore/src/voip/rtp.rswacore/src/voip/session.rswacore/src/voip/ssrc.rs
9aca2cc to
593eacb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/handlers/call.rs (1)
177-196: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winWe're still killing the generic ack before we know we can send a typed one — that's a broken call, and I flagged this last time.
*cancelled = trueruns at Line 177 unconditionally. Whenbuild_call_video_ackreturnsNone(empty wrapper stanza id) we hit theNonebranch, send no typed ack, yet the generic router ack is already suppressed. Net result: the peer gets zero acknowledgement, times out (~5s), and the upgrade reverts. That's exactly the failure mode we don't want to ship. Move the cancellation so it only fires once a typed ack is actually built; otherwise let the generic ack stand.🐛 Gate the suppression on a real typed ack
- *cancelled = true; // The typed ack is what tells the peer we received its state; only COMMIT // the local plane transition once it is sent. ... let acked = match build_call_video_ack(&call) { - Some(ack) => match client.send_node(ack).await { + Some(ack) => { + // Only suppress the generic ack once we own the typed one. + *cancelled = true; + match client.send_node(ack).await { Ok(()) => true, Err(e) => { warn!("call: failed to send typed video ack: {e}"); false } - }, + } + } None => { warn!("call: video stanza has no id; cannot send the typed ack"); false } };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/handlers/call.rs` around lines 177 - 196, Move the unconditional *cancelled = true assignment out of the start of the video-ack flow and gate it on build_call_video_ack returning Some. Preserve the existing typed-ack send behavior, but leave the generic router acknowledgement unsuppressed when no typed ack can be built, including the missing-stanza-id case.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wacore/src/voip/driver.rs`:
- Around line 297-317: Update the video_ctl handling in the main select loop so
it invokes the same inline poll_timeout()/Input::Timeout processing used by the
relay, mic, and video_in arms after dispatching a control message. Preserve the
existing VideoControl behavior while ensuring a continuously ready video_ctl
channel cannot defer overdue keepalive handling.
---
Duplicate comments:
In `@src/handlers/call.rs`:
- Around line 177-196: Move the unconditional *cancelled = true assignment out
of the start of the video-ack flow and gate it on build_call_video_ack returning
Some. Preserve the existing typed-ack send behavior, but leave the generic
router acknowledgement unsuppressed when no typed ack can be built, including
the missing-stanza-id case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e6f272cd-4f3d-4033-8930-f654c0f94535
📒 Files selected for processing (18)
examples/voip-cli/src/main.rsexamples/voip-cli/src/video.rssrc/handlers/call.rssrc/voip/facade.rssrc/voip/mod.rssrc/voip/transport.rssrc/voip/video.rswacore/benches/voip_benchmark.rswacore/src/stanza/call.rswacore/src/types/call.rswacore/src/voip/driver.rswacore/src/voip/engine.rswacore/src/voip/h264.rswacore/src/voip/mod.rswacore/src/voip/registry.rswacore/src/voip/rtp.rswacore/src/voip/session.rswacore/src/voip/ssrc.rs
…peg example The video media plane rides the audio call's relay and E2E keys: H.264 Annex-B access units are RFC 6184-packetized (single NAL / FU-A), protected by the same per-participant E2E-SRTP + WARP MI tag under a video SSRC (slot 2), and demuxed from audio by payload type (97). The library never touches pixels: VideoSource/VideoSink carry pre-encoded AUs, so the codec stays with the consumer. Signaling implements the in-call <video state=N> handshake (UpgradeRequestV2 -> UpgradeAccept -> Enabled; Stopped/Disabled to downgrade), with the upgrade marker attr only on the request, a generated wrapper id so the peer's typed <ack class="call" type="video"> can correlate, and that typed ack replacing the generic one (an untyped ack makes the requester revert the upgrade). CallHandle gains start_video/accept_video/stop_video; the builders gain .video(source, sink) for video-from-the-start calls; peer states surface as CallEvent::VideoStateChanged. A downgrade preserves the video SRTP send sequence/ROC (the pipeline is deactivated, not dropped) so a re-upgrade never resets the packet index under the same key+SSRC and repeats the AES-CTR keystream. FU-A reassembly tracks the RTP sequence number and drops a partial NAL on a gap rather than emitting it truncated. Local video setup rolls back on a failed signaling send, a peer reject/cancel releases the source, and handle ops are generation-guarded against glare. The voip-cli example gains --video on every subcommand, using ffmpeg/ffplay subprocesses as the external codec (webcam per OS, any file/URL, or a testsrc pattern; window or .h264-file sink), an IDR-gated backpressure dropper on both sides, and a stdin toggle (v/q) for mid-call upgrade/downgrade. Wire details taken from the meowcaller/WaCalls reference are marked as live-validation points (video SSRC slot word, PT 97, no SFrame on video, <video> node shape in offer/accept, the fixed 15 fps timestamp stride); the signaling flow matches what the mock server proved against real WA Web clients. No new dependencies; everything stays inside the existing voip feature.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4fae9659c6
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/voip-cli/src/main.rs`:
- Around line 740-759: Replace production Mutex::lock().unwrap() calls
throughout the affected CallState handlers, including begin_call_startup,
complete_call_startup, record_peer_terminate, and the additional referenced
sections, with a shared poison-recovering lock helper or a non-poisoning mutex.
Preserve each handler’s existing state-update behavior while ensuring poisoned
locks do not panic during event handling, cleanup, or UI tasks.
- Around line 102-108: Update video_source_is_ignored and its callers so the
WA_VIDEO_INPUT warning is emitted only for audio loopback and rejecting listen
mode; do not warn for listen accept or call modes because their stdin UI can use
the variable for upgrades. Adjust the associated tests to cover these
mode-specific behaviors.
- Around line 817-833: Rework the call setup flow around respond_to_offer and
start_media so it sends a preaccept, prepares camera/sink/ffmpeg endpoints,
waits for the first IDR, and only then sends the final accept. Apply the same
ordering to the related call paths at the other indicated locations, and
explicitly terminate the call if media preparation or readiness fails after
setup begins.
In `@examples/voip-cli/src/video.rs`:
- Around line 197-210: The VideoOpts::from_env method performs a blocking
Path::exists check on the async setup path. Move path classification out of this
synchronous parser by making the setup flow use Tokio filesystem APIs or by
separating pure environment parsing from asynchronous path existence checking,
while preserving the existing VideoInput selection behavior.
- Around line 565-712: Update spawn_video_source to retain a JoinHandle or abort
handle for the spawned ffmpeg-reading task. Abort the task before returning on
every pre-IDR timeout/error path, and ensure dropping FfmpegVideoSource also
cancels the task; preserve normal startup and frame delivery behavior.
In `@wacore/src/stanza/call.rs`:
- Around line 784-805: Update build_video_state and VideoStateParams to remove
the upgrade_marker Boolean and derive marker emission directly from p.state.
Emit voip_settings="video" only when state is UpgradeRequestV2, ensuring
downgrade states never receive the marker; update all callers accordingly.
In `@wacore/src/voip/e2e_srtp.rs`:
- Around line 176-203: Extend unprotect_srtcp to return the authenticated 31-bit
SRTCP index alongside plaintext, then in wacore/src/voip/session.rs lines
227-235 add per-sender-SSRC highest-index and replay-bitmap state. Update the
receive path at lines 368-372 to validate and mutate replay state only after
authentication, rejecting duplicate or stale indices while accepting new packets
and committing the updated window.
In `@wacore/src/voip/h264.rs`:
- Around line 320-325: Update the H264 packet handling around flush_on,
queue_ready, and ready.pop_front so multiple completed access units produced by
one push are not stranded when no subsequent packet arrives. Preserve delivery
ordering while returning all newly completed AUs, or expose a queue-draining
interface that VideoPipeline can consume immediately instead of limiting push to
one result.
In `@wacore/src/voip/rtp.rs`:
- Around line 407-415: Update VideoRtpStream::new to reject ts_stride == 0 at
construction, matching the existing setter invariant; preserve normal
initialization for positive strides and use the established rejection mechanism
from the setter.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1c67c7f4-2eed-4b82-a073-b8a0cc599023
📒 Files selected for processing (25)
examples/voip-cli/src/main.rsexamples/voip-cli/src/video.rssrc/handlers/call.rssrc/voip/facade.rssrc/voip/mod.rssrc/voip/transport.rssrc/voip/video.rswacore/benches/voip_benchmark.rswacore/src/stanza/call.rswacore/src/types/call.rswacore/src/voip/demux.rswacore/src/voip/driver.rswacore/src/voip/e2e_srtp.rswacore/src/voip/engine.rswacore/src/voip/h264.rswacore/src/voip/hbh_srtp.rswacore/src/voip/mod.rswacore/src/voip/registry.rswacore/src/voip/relay_parse.rswacore/src/voip/rtcp.rswacore/src/voip/rtp.rswacore/src/voip/session.rswacore/src/voip/ssrc.rswacore/src/voip/stun.rswacore/src/voip/testdata/kats.json
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
wacore/src/voip/engine.rs (1)
732-739: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
disable_video()should re-armkeyframe_requiredon downgrade. A plain downgrade→re-upgrade path can otherwise resume without forcing a fresh IDR, while the gated-resume path already handles that case. Setv.keyframe_required = truehere so the state machine stays consistent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wacore/src/voip/engine.rs` around lines 732 - 739, Update disable_video to set v.keyframe_required = true alongside deactivating the video plane, ensuring every downgrade requires a fresh keyframe when video is re-enabled while preserving the existing idempotent behavior.wacore/src/voip/registry.rs (1)
221-255: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winGuard the video-path helpers with generation
send_call_event,send_video_ctl,set_is_video, andrun_video_teardownstill key only oncall_id, so a delayed stale path can act on a same-id replacement. Threadgenerationthrough these methods to match the existing ABA guards.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wacore/src/voip/registry.rs` around lines 221 - 255, Update send_call_event, send_video_ctl, set_is_video, and run_video_teardown to accept and validate the call generation alongside call_id before accessing or mutating registry state. Propagate the generation through every caller and preserve the existing behavior only when both identifiers match, preventing delayed stale paths from affecting same-id replacements.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@wacore/src/voip/engine.rs`:
- Around line 732-739: Update disable_video to set v.keyframe_required = true
alongside deactivating the video plane, ensuring every downgrade requires a
fresh keyframe when video is re-enabled while preserving the existing idempotent
behavior.
In `@wacore/src/voip/registry.rs`:
- Around line 221-255: Update send_call_event, send_video_ctl, set_is_video, and
run_video_teardown to accept and validate the call generation alongside call_id
before accessing or mutating registry state. Propagate the generation through
every caller and preserve the existing behavior only when both identifiers
match, preventing delayed stale paths from affecting same-id replacements.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: dc8422be-cea2-4e90-b8ba-cf670c2025bd
📒 Files selected for processing (13)
examples/voip-cli/src/main.rsexamples/voip-cli/src/video.rssrc/handlers/call.rssrc/voip/facade.rswacore/src/stanza/call.rswacore/src/voip/driver.rswacore/src/voip/e2e_srtp.rswacore/src/voip/engine.rswacore/src/voip/h264.rswacore/src/voip/hbh_srtp.rswacore/src/voip/registry.rswacore/src/voip/rtp.rswacore/src/voip/session.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb0365053e
ℹ️ 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".
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/handlers/call.rs (2)
217-257: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHold the transition permit until every committed effect completes.
permit.send(...)consumes the permit immediately, releasing the reservation before orientation, video state, controls, and theEnabledannouncement finish. Another handler can overtake this transition and then have its state overwritten. Publish without consuming the permit and drop it only after the entire committed block.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/handlers/call.rs` around lines 217 - 257, Update the transition handling around the event_permit and the committed video-state effects so permit.send publishes the CallEvent without consuming the permit, retaining the permit through orientation updates, registry video-state/control changes, and the UpgradeAccept Enabled announcement. Explicitly drop the permit only after all these effects complete, while preserving the existing receiver-closed warning behavior.
177-214: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy liftDo not apply a stale video stanza to a replacement call.
The permit captures the old entry, but
send_node(...).awaitallows a same-ID replacement. Subsequent teardown, state, and control calls resolve bycall_idand can mutate or tear down the new generation. Bind the permit to its generation and generation-guard every post-ack effect.Also applies to: 226-257
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/handlers/call.rs` around lines 177 - 214, Prevent stale video stanzas from affecting replacement calls after the await in the handling flow around reserve_call_event and send_node. Bind the event permit to the original call generation, then generation-guard every post-ack effect, including run_video_teardown, set_is_video, state updates, and control operations in the related 226–257 block. If the generation no longer matches, skip those effects.wacore/src/voip/registry.rs (1)
229-238: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winConsume the teardown hook before invoking it.
Cloning leaves the hook installed, so duplicate rejects and the later
CallEntrydrop invoke it again. The callback has no idempotency contract;take()it under the lock, then invoke it off-lock.Proposed fix
- let hook = self + let hook = self .inner .lock() .expect("registry lock poisoned") - .get(call_id) - .and_then(|entry| entry.video_teardown.clone()); + .get_mut(call_id) + .and_then(|entry| entry.video_teardown.take());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wacore/src/voip/registry.rs` around lines 229 - 238, Update Registry::run_video_teardown to remove the video_teardown hook from the CallEntry under the registry lock using take() rather than cloning it, then invoke the extracted hook outside the lock. Preserve the existing behavior when no hook is installed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/handlers/call.rs`:
- Around line 217-257: Update the transition handling around the event_permit
and the committed video-state effects so permit.send publishes the CallEvent
without consuming the permit, retaining the permit through orientation updates,
registry video-state/control changes, and the UpgradeAccept Enabled
announcement. Explicitly drop the permit only after all these effects complete,
while preserving the existing receiver-closed warning behavior.
- Around line 177-214: Prevent stale video stanzas from affecting replacement
calls after the await in the handling flow around reserve_call_event and
send_node. Bind the event permit to the original call generation, then
generation-guard every post-ack effect, including run_video_teardown,
set_is_video, state updates, and control operations in the related 226–257
block. If the generation no longer matches, skip those effects.
In `@wacore/src/voip/registry.rs`:
- Around line 229-238: Update Registry::run_video_teardown to remove the
video_teardown hook from the CallEntry under the registry lock using take()
rather than cloning it, then invoke the extracted hook outside the lock.
Preserve the existing behavior when no hook is installed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 86a82fe0-2763-4d9a-8845-f468d8237bdf
📒 Files selected for processing (2)
src/handlers/call.rswacore/src/voip/registry.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9b3fb4ee9
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/handlers/call.rs (1)
193-203: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBound outbound sends while holding the transition permit.
send_nodecan remain pending, leavingevent_permitreserved indefinitely. Every subsequent video transition then fails reservation and reverts. Add timeout/cancellation handling for both the typed ACK and Enabled announcement, releasing the permit on failure.Also applies to: 263-277
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/handlers/call.rs` around lines 193 - 203, Bound the asynchronous send operations in the video transition handling, including the typed ACK in build_call_video_ack and the Enabled announcement in the corresponding branch around those sends. Apply the existing timeout or cancellation mechanism to client.send_node so pending sends fail, and ensure every failure path releases event_permit before returning or reverting the transition.src/voip/facade.rs (1)
1574-1592: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winStop on a lost generation before sending video signals.
set_is_video()can returnfalseafter a same-call-id replacement lands, but the code still sendsVideoControland the<video>stanzas. Return immediately onfalseso a stale handle doesn't keep signaling the newer call.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/voip/facade.rs` around lines 1574 - 1592, Update the video setup flow around set_is_video so it checks the returned boolean and returns immediately when the generation is no longer active. Perform this validation before send_control, ensuring stale handles do not emit VideoControl or video stanzas, while preserving the existing successful path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/handlers/call.rs`:
- Around line 279-285: Update the transition commit flow around dispatch_call in
the call handler so Event::IncomingCall is dispatched while event_permit is
still held. Move the permit release after publication, preserving the existing
dispatch condition and ensuring subsequent transitions cannot publish ahead of
the current one.
In `@wacore/src/voip/registry.rs`:
- Around line 293-310: Update send_video_ctl so only
VideoControl::SetOrientation uses best-effort try_send; route all
state-transition controls, including Enable and Disable, through a non-evicting
send operation that preserves queued commands. Ensure the video_ctl channel and
its send path no longer allow force_send to evict pending transitions.
---
Outside diff comments:
In `@src/handlers/call.rs`:
- Around line 193-203: Bound the asynchronous send operations in the video
transition handling, including the typed ACK in build_call_video_ack and the
Enabled announcement in the corresponding branch around those sends. Apply the
existing timeout or cancellation mechanism to client.send_node so pending sends
fail, and ensure every failure path releases event_permit before returning or
reverting the transition.
In `@src/voip/facade.rs`:
- Around line 1574-1592: Update the video setup flow around set_is_video so it
checks the returned boolean and returns immediately when the generation is no
longer active. Perform this validation before send_control, ensuring stale
handles do not emit VideoControl or video stanzas, while preserving the existing
successful path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d23d6cb6-193d-46b8-b628-19125d12a963
📒 Files selected for processing (6)
src/handlers/call.rssrc/voip/facade.rswacore/src/stanza/call.rswacore/src/voip/driver.rswacore/src/voip/engine.rswacore/src/voip/registry.rs
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/handlers/call.rs (1)
183-225: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winKeep refusal teardown independent of event publication.
generationonly comes fromevent_permit. If reservation fails,UpgradeReject/UpgradeCancelskips teardown and can leave camera capture andis_videoactive after the peer refuses the upgrade.Proposed fix
let event_permit = registry.reserve_call_event(call_id); let generation = event_permit.as_ref().map(|permit| permit.generation()); +let teardown_generation = + generation.or_else(|| registry.generation_of(call_id)); ... if matches!(state, VideoState::UpgradeReject | VideoState::UpgradeCancel) - && let Some(generation) = generation + && let Some(generation) = teardown_generation {Add coverage with the event permit unavailable or already reserved.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/handlers/call.rs` around lines 183 - 225, Make refusal teardown independent of event permit reservation in the call video transition handler: for UpgradeReject and UpgradeCancel, always run the appropriate local teardown and clear is_video even when reserve_call_event returns None or the event is already reserved. Obtain or pass the needed call generation through a path that remains available without event publication, while preserving the existing ack and dispatch behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/handlers/call.rs`:
- Around line 255-290: Reorder the logic in the video-state handler so
orientation, is_video, and Enable effects are applied before sending
CallEvent::VideoStateChanged. Retain event_permit through those updates, then
publish the event and preserve the existing warning when delivery fails.
In `@src/voip/facade.rs`:
- Around line 1624-1628: Add telemetry or structured logging to the
send_state(VideoState::Enabled, None) failure branch, capturing the handshake
failure and relevant available context before teardown_local_video and returning
the error. Keep the existing rollback and error propagation behavior unchanged.
---
Outside diff comments:
In `@src/handlers/call.rs`:
- Around line 183-225: Make refusal teardown independent of event permit
reservation in the call video transition handler: for UpgradeReject and
UpgradeCancel, always run the appropriate local teardown and clear is_video even
when reserve_call_event returns None or the event is already reserved. Obtain or
pass the needed call generation through a path that remains available without
event publication, while preserving the existing ack and dispatch behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 931cc942-5a15-4540-ba44-6ee980f659e0
📒 Files selected for processing (2)
src/handlers/call.rssrc/voip/facade.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a904536d18
ℹ️ 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".
| if let Some(video_ssrc) = video_ssrc | ||
| && requests_keyframe(&summary.feedback, video_ssrc) | ||
| && let Some(video) = m.video.as_mut() | ||
| { | ||
| video.keyframe_required = true; |
There was a problem hiding this comment.
This new PLI/FIR handling only runs if the RTCP packet reaches the engine, but in the native relay path relay_read_pump still drops every full-queue packet except STUN (src/voip/transport.rs:394-396). During video congestion—exactly when keyframe feedback matters—a peer's RTCP PLI/FIR can be discarded before this sets keyframe_required, leaving the remote side on corrupt/black video until the encoder's next periodic IDR; RTCP feedback should be preserved as control traffic rather than shed with media.
Useful? React with 👍 / 👎.
| t if (1..=23).contains(&t) => { | ||
| self.drop_partial_fu(); | ||
| self.append_nal(payload); |
There was a problem hiding this comment.
When an AU is packetized as separate single-NAL RTP packets (for example SPS, PPS, then IDR) and any non-FU packet is lost or reordered, this branch appends later NALs without checking that seq is contiguous; a following marker then flushes a partial AU as if it were complete. That can feed sinks an IDR missing SPS/PPS or slices, causing decoder errors/corruption until a later keyframe, so track gaps across all packets in an AU and discard/resync instead of emitting the partial frame.
Useful? React with 👍 / 👎.
| pub fn disable_video(&mut self) { | ||
| if let Some(v) = self.media.as_mut().and_then(|m| m.video.as_mut()) { | ||
| v.active = false; | ||
| } |
There was a problem hiding this comment.
When Disable arrives after some inbound H.264 packets were accepted, this only flips active to false; the preserved VideoPipeline also preserves its depacketizer buffers. A quick re-enable can then make the next forward RTP timestamp flush the old partial AU to the newly attached sink before the new negotiation's frames, so the inbound reassembly/ready queue should be cleared when disabling video while still preserving the SRTP counters.
Useful? React with 👍 / 👎.
2 participants
Summary
Adds interoperable 1:1 video calls to the existing VoIP implementation. Calls can start with video, upgrade from audio to video, return to audio, and accept the same transitions initiated by an official WhatsApp client.
The library remains codec-neutral: consumers provide and receive complete H.264 Annex-B access units through
VideoSource/VideoSink. Encoding, decoding, capture, and display stay outside the core crates; the CLI example usesffmpegandffplaywithout adding codec dependencies to the library.This is additive to the existing 1:1 voice path. Bidirectional audio continues over its established pipeline while video is independently signaled, demultiplexed, encrypted, paced, and recovered.
WhatsApp interoperability
The implementation covers the media and signaling profile verified against WhatsApp Web/Android captures and live Android calls:
StreamDescriptorsfor live audio/video SSRCs;Enabledtransition used to complete accepted upgrades.The Android peer reports reception of the Rust participant's video SSRC and renders outbound video from both synthetic input and a real V4L2 webcam.
Empirically confirmed webcam recovery
The webcam-only failure was isolated by replaying the exact captured webcam Annex-B stream and inspecting authenticated peer RTCP. The H.264 was valid and reached Android, but Android repeatedly sent PLI for the local video SSRC. Continuing to send dependent frames before the next IDR kept the official decoder outside a valid recovery point.
The engine now treats authenticated PLI/FIR as a decoder resynchronization boundary: dependent access units are withheld without consuming RTP/SRTP sequence state until the next IDR, then normal transmission resumes. The live V4L2 test changed from PLI every ~440 ms with no picture to one recovery request followed by continuous rendered webcam video.
The same invariant protects from-start video, reactivation, upgrade ungating, and relay backpressure. The recovery gate clears only after an IDR packetizes successfully. Disable purges queued, unstarted video access units while preserving any batch already on the transport, so reactivation resumes at a complete IDR without truncating an in-flight frame; audio and control traffic stay live.
API and lifecycle
VideoSourceexposes its RTP timestamp stride, so 15 fps, 20 fps, 30 fps, and other valid 90 kHz cadences do not drift; zero stride is rejected at construction.Enabledsend succeeds. A failed second handshake send emits phase-tagged diagnostics, tears down endpoints, and clears local video state in both initiator and acceptor paths, leaving the official peer to perform its normal incomplete-upgrade timeout.participant/recipientmetadata in typed acknowledgements, bind transitions to the call generation, and hold serialization through every post-ack effect and accepted-upgradeEnabledsend. Unacknowledged or stale same-ID transitions cannot mutate a replacement call, and a committed state supersedes an older queued event under backpressure.VideoState, preventing invalid marker/state combinations.Disablepurge always precedes a laterEnable; orientation is isolated in a one-slot latest-value mailbox and cannot evict negotiated transitions.H.264 media plane
CLI example
Every call/listen mode accepts
--video;vtoggles video during a call andqperforms a signaled hangup. Sources can be an OS webcam, a file/URL, orWA_VIDEO_INPUT=testsrc; sinks can be anffplaywindow, raw.h264, or discard mode.The example asynchronously classifies input paths, probes V4L2 capabilities, selects an exact supported capture mode, normalizes pixel format/geometry, and waits for a decodable SPS/PPS/IDR before accepting the call. Its default encoder contract matches the captured WhatsApp Web high-quality tier: H.264 Constrained Baseline Level 3.1, 1280×720 at 20 fps, about 1.98 Mbps, one slice per frame, repeated SPS/PPS, and a 60-frame GOP.
WA_VIDEO_SIZE,WA_VIDEO_FPS, andWA_VIDEO_BITRATE_KBPSremain explicit overrides.The preview uses arrival-time timestamps, bounded low-latency queues, and inverse
device_orientationcorrection. This prevents long-run delay accumulation and correctly displays Android portrait video. Official clients still adapt quality dynamically; the example matches a captured high-quality tier but does not claim to implement WhatsApp's full bandwidth estimator.Validation
cargo fmt --all -- --checkcargo clippy --all --tests -- -D warningscargo test --workspace --exclude e2e-testswacore: 1,403 passed, 1 ignoredwhatsapp-rust: 1,068 passed, 1 ignoredwhatsapp-rust-voip-cli: 16 passedcargo test -p wacore --features voip voip::: 297 passed, 1 diagnostic ignoredcargo test -p whatsapp-rust --features voip voip::: 53 passedcargo test -p whatsapp-rust --features voip handlers::call::tests::: 24 passedcargo build -p whatsapp-rust-voip-cli --releasecargo test --alladditionally requires the documented E2E mock server; GitHub Actions supplies that environment.Scope
wacoreorwhatsapp-rust.