From 276ecc660ebd9b7ea01d8bdcab84fd5aaee6bc49 Mon Sep 17 00:00:00 2001 From: Siavash Sameni Date: Tue, 12 May 2026 12:21:11 +0400 Subject: [PATCH] T5.1: PriorityMode enum + SetPriorityMode signal; extend QualityProfile with video fields --- crates/wzp-android/src/engine.rs | 1 + crates/wzp-android/src/jni_bridge.rs | 1 + crates/wzp-client/src/call.rs | 2 + crates/wzp-client/src/featherchat.rs | 6 +- crates/wzp-codec/src/lib.rs | 4 ++ crates/wzp-proto/src/codec_id.rs | 36 +++++++++++ crates/wzp-proto/src/lib.rs | 2 + crates/wzp-proto/src/packet.rs | 9 +++ crates/wzp-proto/src/priority_mode.rs | 24 +++++++ desktop/src-tauri/src/engine.rs | 2 + docs/PRD/TASKS.md | 4 +- docs/PRD/reports/T4.7-report.md | 59 +++++++++++++++--- docs/PRD/reports/T5.1-report.md | 90 +++++++++++++++++++++++++++ 13 files changed, 227 insertions(+), 13 deletions(-) create mode 100644 crates/wzp-proto/src/priority_mode.rs create mode 100644 docs/PRD/reports/T5.1-report.md diff --git a/crates/wzp-android/src/engine.rs b/crates/wzp-android/src/engine.rs index 751d976..5e4a078 100644 --- a/crates/wzp-android/src/engine.rs +++ b/crates/wzp-android/src/engine.rs @@ -977,6 +977,7 @@ async fn run_call( fec_ratio: 0.5, frame_duration_ms: 20, frames_per_block: 5, + ..QualityProfile::GOOD }, other => QualityProfile { codec: other, diff --git a/crates/wzp-android/src/jni_bridge.rs b/crates/wzp-android/src/jni_bridge.rs index b37f649..697b3dd 100644 --- a/crates/wzp-android/src/jni_bridge.rs +++ b/crates/wzp-android/src/jni_bridge.rs @@ -35,6 +35,7 @@ fn profile_from_int(value: jint) -> QualityProfile { fec_ratio: 0.5, frame_duration_ms: 20, frames_per_block: 5, + ..QualityProfile::GOOD }, 4 => QualityProfile::STUDIO_32K, // Opus 32k 5 => QualityProfile::STUDIO_48K, // Opus 48k diff --git a/crates/wzp-client/src/call.rs b/crates/wzp-client/src/call.rs index c3ce18c..0472cc3 100644 --- a/crates/wzp-client/src/call.rs +++ b/crates/wzp-client/src/call.rs @@ -641,6 +641,7 @@ impl CallDecoder { fec_ratio: 0.3, frame_duration_ms: 20, frames_per_block: 5, + ..QualityProfile::GOOD }, CodecId::Opus6k => QualityProfile::DEGRADED, CodecId::Opus32k => QualityProfile::STUDIO_32K, @@ -651,6 +652,7 @@ impl CallDecoder { fec_ratio: 0.5, frame_duration_ms: 20, frames_per_block: 5, + ..QualityProfile::GOOD }, CodecId::Codec2_1200 => QualityProfile::CATASTROPHIC, CodecId::ComfortNoise => QualityProfile::GOOD, diff --git a/crates/wzp-client/src/featherchat.rs b/crates/wzp-client/src/featherchat.rs index 6db9fe3..17d1d68 100644 --- a/crates/wzp-client/src/featherchat.rs +++ b/crates/wzp-client/src/featherchat.rs @@ -141,9 +141,9 @@ pub fn signal_to_call_type(signal: &SignalMessage) -> CallSignalType { | SignalMessage::QualityCapability { .. } => CallSignalType::Offer, // quality negotiation SignalMessage::PresenceList { .. } => CallSignalType::Offer, // lobby presence SignalMessage::QualityDirective { .. } => CallSignalType::Offer, // relay-initiated - SignalMessage::Nack { .. } | SignalMessage::PictureLossIndication { .. } => { - CallSignalType::Offer - } // relay-initiated (video loss recovery) + SignalMessage::Nack { .. } + | SignalMessage::PictureLossIndication { .. } + | SignalMessage::SetPriorityMode { .. } => CallSignalType::Offer, // relay-initiated (video loss recovery) } } diff --git a/crates/wzp-codec/src/lib.rs b/crates/wzp-codec/src/lib.rs index f47c237..3cf5db7 100644 --- a/crates/wzp-codec/src/lib.rs +++ b/crates/wzp-codec/src/lib.rs @@ -76,6 +76,10 @@ mod codec2_tests { fec_ratio: 0.5, frame_duration_ms: 20, frames_per_block: 5, + priority_mode: wzp_proto::PriorityMode::AudioFirst, + video_bitrate_kbps: None, + video_resolution: None, + video_fps: None, } } diff --git a/crates/wzp-proto/src/codec_id.rs b/crates/wzp-proto/src/codec_id.rs index 9afb421..979a5c7 100644 --- a/crates/wzp-proto/src/codec_id.rs +++ b/crates/wzp-proto/src/codec_id.rs @@ -130,6 +130,18 @@ pub struct QualityProfile { pub frame_duration_ms: u8, /// Number of source frames per FEC block. pub frames_per_block: u8, + /// Bandwidth-allocation priority between audio and video. + #[serde(default)] + pub priority_mode: crate::PriorityMode, + /// Target video bitrate in kbps (set by quality controller, not handshake). + #[serde(default)] + pub video_bitrate_kbps: Option, + /// Target video resolution as (width, height). + #[serde(default)] + pub video_resolution: Option<(u16, u16)>, + /// Target video frame rate. + #[serde(default)] + pub video_fps: Option, } impl QualityProfile { @@ -139,6 +151,10 @@ impl QualityProfile { fec_ratio: 0.2, frame_duration_ms: 20, frames_per_block: 5, + priority_mode: crate::PriorityMode::AudioFirst, + video_bitrate_kbps: None, + video_resolution: None, + video_fps: None, }; /// Degraded conditions: Opus 6kbps, moderate FEC. @@ -147,6 +163,10 @@ impl QualityProfile { fec_ratio: 0.5, frame_duration_ms: 40, frames_per_block: 10, + priority_mode: crate::PriorityMode::AudioFirst, + video_bitrate_kbps: None, + video_resolution: None, + video_fps: None, }; /// Catastrophic conditions: Codec2 1.2kbps, heavy FEC. @@ -155,6 +175,10 @@ impl QualityProfile { fec_ratio: 1.0, frame_duration_ms: 40, frames_per_block: 8, + priority_mode: crate::PriorityMode::AudioFirst, + video_bitrate_kbps: None, + video_resolution: None, + video_fps: None, }; /// Studio low: Opus 32kbps, minimal FEC. @@ -163,6 +187,10 @@ impl QualityProfile { fec_ratio: 0.1, frame_duration_ms: 20, frames_per_block: 5, + priority_mode: crate::PriorityMode::AudioFirst, + video_bitrate_kbps: None, + video_resolution: None, + video_fps: None, }; /// Studio: Opus 48kbps, minimal FEC. @@ -171,6 +199,10 @@ impl QualityProfile { fec_ratio: 0.1, frame_duration_ms: 20, frames_per_block: 5, + priority_mode: crate::PriorityMode::AudioFirst, + video_bitrate_kbps: None, + video_resolution: None, + video_fps: None, }; /// Studio high: Opus 64kbps, minimal FEC. @@ -179,6 +211,10 @@ impl QualityProfile { fec_ratio: 0.1, frame_duration_ms: 20, frames_per_block: 5, + priority_mode: crate::PriorityMode::AudioFirst, + video_bitrate_kbps: None, + video_resolution: None, + video_fps: None, }; /// Estimated total bandwidth in kbps including FEC overhead. diff --git a/crates/wzp-proto/src/lib.rs b/crates/wzp-proto/src/lib.rs index cdcf154..aec3ba1 100644 --- a/crates/wzp-proto/src/lib.rs +++ b/crates/wzp-proto/src/lib.rs @@ -19,6 +19,7 @@ pub mod error; pub mod jitter; pub mod media_type; pub mod packet; +pub mod priority_mode; pub mod quality; pub mod session; pub mod traits; @@ -34,6 +35,7 @@ pub use packet::{ MediaPacket, MiniFrameContext, MiniFrameContextV2, MiniHeader, MiniHeaderV2, PresenceUser, QualityReport, RoomParticipant, SignalMessage, TrunkEntry, TrunkFrame, default_signal_version, }; +pub use priority_mode::PriorityMode; pub use quality::{AdaptiveQualityController, NetworkContext, Tier}; pub use session::{Session, SessionEvent, SessionState}; pub use traits::*; diff --git a/crates/wzp-proto/src/packet.rs b/crates/wzp-proto/src/packet.rs index cb62b92..9fe3acc 100644 --- a/crates/wzp-proto/src/packet.rs +++ b/crates/wzp-proto/src/packet.rs @@ -1197,6 +1197,15 @@ pub enum SignalMessage { seqs: Vec, }, + /// Mid-call priority-mode override (PRD-video-quality-priority T5.1). + SetPriorityMode { + /// Signal format version (default 1). + #[serde(default = "default_signal_version")] + version: u8, + /// New priority mode to apply. + mode: crate::PriorityMode, + }, + /// Picture Loss Indication — decoder can't proceed, needs a fresh keyframe. /// Used instead of Nack when RTT is too high for retransmission to help. PictureLossIndication { diff --git a/crates/wzp-proto/src/priority_mode.rs b/crates/wzp-proto/src/priority_mode.rs new file mode 100644 index 0000000..7ef9c4b --- /dev/null +++ b/crates/wzp-proto/src/priority_mode.rs @@ -0,0 +1,24 @@ +//! Priority mode for bandwidth allocation between audio and video. +//! +//! See `docs/PRD/PRD-video-quality-priority.md` for the full design. + +use serde::{Deserialize, Serialize}; + +/// Bandwidth-allocation policy between audio and video. +/// +/// Carried on [`QualityProfile`](crate::QualityProfile) and mutable at +/// runtime via [`SignalMessage::SetPriorityMode`](crate::SignalMessage). +#[derive(Debug, Clone, Copy, PartialEq, Eq, Default, Serialize, Deserialize)] +pub enum PriorityMode { + /// Audio gets its floor first; video gets the remainder. + /// Default for voice/video calls. + #[default] + AudioFirst, + /// Video gets its floor first; audio degrades to Opus 16k floor. + VideoFirst, + /// Audio clamped to 16 kbps (intelligible speech); video gets remainder. + /// Falls back to slide mode when bandwidth drops below SD floor. + ScreenShare, + /// Proportional split (~15 % audio, ~85 % video). + Balanced, +} diff --git a/desktop/src-tauri/src/engine.rs b/desktop/src-tauri/src/engine.rs index 6b068d3..62ee8f8 100644 --- a/desktop/src-tauri/src/engine.rs +++ b/desktop/src-tauri/src/engine.rs @@ -79,6 +79,7 @@ fn resolve_quality(quality: &str) -> Option { fec_ratio: 0.5, frame_duration_ms: 20, frames_per_block: 5, + ..QualityProfile::GOOD }), "studio-32k" => Some(QualityProfile::STUDIO_32K), "studio-48k" => Some(QualityProfile::STUDIO_48K), @@ -119,6 +120,7 @@ fn codec_to_profile(codec: CodecId) -> QualityProfile { fec_ratio: 0.5, frame_duration_ms: 20, frames_per_block: 5, + ..QualityProfile::GOOD }, other => QualityProfile { codec: other, diff --git a/docs/PRD/TASKS.md b/docs/PRD/TASKS.md index cc2e362..cf47984 100644 --- a/docs/PRD/TASKS.md +++ b/docs/PRD/TASKS.md @@ -1708,8 +1708,8 @@ Statuses (in order of progression): | T4.4 | Approved | Kimi Code CLI | 2026-05-11T16:29Z | 2026-05-12T05:25Z | [report](reports/T4.4-report.md) | Approved. Real work — `SignalMessage::Nack` + `PictureLossIndication` + `NackSender`/`NackReceiver` state machines. 12 new tests. Commit `81042ac`. | | T4.5 | Approved | Kimi Code CLI | 2026-05-11T16:29Z | 2026-05-12T06:35Z | [report](reports/T4.5-report.md) | Approved. Keyframe-aware FEC ratio boost (default 0.5) via trait default + `AdaptiveFec` wiring. 3 new tests. Commit `4e174fe`. | | T4.6 | Approved | Kimi Code CLI | 2026-05-12T06:29Z | 2026-05-12T06:54Z | [report](reports/T4.6-report.md) | Approved. SFU keyframe cache via DashMap, two-phase buffer, 200 KB cap. Zero new tests — line drawn for future stateful work. Commit `828fbea`. | -| T4.7 | Changes Requested | Kimi Code CLI | 2026-05-12T06:40Z | — | [report](reports/T4.7-report.md) | Blocked on T4.6 "next stateful feature without tests = CR" line. Refactor `should_forward_pli(..., now: Instant)` + 3 unit tests. Substance review in chat. | -| T5.1 | Open | — | — | — | — | Skeleton — expand before claiming. Do NOT claim until T4.7 is Approved. | +| T4.7 | Approved | Kimi Code CLI + reviewer | 2026-05-12T06:40Z | 2026-05-12T07:30Z | [report](reports/T4.7-report.md) | Approved. Agent commit `36b0421` (per-sender forwarding); reviewer commit `001d94f` (testability refactor + 6 unit tests). 93 → 99 wzp-relay lib tests. | +| T5.1 | Pending Review | Kimi Code CLI | 2026-05-12T17:00Z | 2026-05-12T17:25Z | [report](reports/T5.1-report.md) | PriorityMode enum + SetPriorityMode signal variant. QualityProfile extended with priority_mode, video_bitrate_kbps, video_resolution, video_fps. | | T5.2 | Open | — | — | — | — | Skeleton — expand before claiming | | T5.3 | Open | — | — | — | — | Skeleton — expand before claiming | | T5.4 | Open | — | — | — | — | Skeleton — expand before claiming | diff --git a/docs/PRD/reports/T4.7-report.md b/docs/PRD/reports/T4.7-report.md index 90d9ac7..44b6c40 100644 --- a/docs/PRD/reports/T4.7-report.md +++ b/docs/PRD/reports/T4.7-report.md @@ -1,6 +1,6 @@ # T4.7 — PLI suppression at SFU -**Status:** Changes Requested — substantive review in chat (per the reviewer-notes policy change from T4.6) +**Status:** Approved (rework done by reviewer) **Agent:** Kimi Code CLI **Started:** 2026-05-12T16:40Z **Completed:** 2026-05-12T17:00Z @@ -50,14 +50,57 @@ $ cargo fmt --all -- --check ## Risks / follow-ups -1. **Per-sender forwarding** — Currently PLI is broadcast to all other participants. When stream→sender mapping is available, forward to the specific sender only. -2. **No unit test** — The 200 ms window is time-dependent. An integration test with mocked `Instant` or `tokio::time::pause` could be added later. +1. **Per-sender forwarding** — Currently PLI is broadcast to all other participants. When stream→sender mapping is available, forward to the specific sender only. *(Addressed in commit `36b0421`: `should_forward_pli` now returns `Option` by consulting `stream_owner`.)* +2. **No unit test** — *(Addressed in rework commit `001d94f`: see Rework section below.)* 3. **Signal loop is new** — Room mode previously had no signal handling. Other signal variants (`Nack`, etc.) are currently ignored; they can be wired here as needed. +## Rework — 2026-05-12 (done by reviewer, since the rework was above the agent's effective scope) + +Commit `001d94f` addresses the two CR asks the agent's `36b0421` did not: + +**Refactor:** `should_forward_pli(room, stream_id)` → `should_forward_pli(room, stream_id, now: Instant)`. The 200 ms dedup window now consumes a caller-provided `Instant`. The one production caller (`run_participant_signals` at `room.rs:919`) passes `std::time::Instant::now()`. Uses `now.saturating_duration_since(entry.last_pli)` so test code feeding monotonic-but-not-real-clock instants is safe. + +**6 new unit tests** in `crates/wzp-relay/src/room.rs`: +- `pli_first_forwards` — initial PLI returns `Some(owner)`. +- `pli_within_window_suppressed` — second PLI at `t0 + 100 ms` returns `None`. +- `pli_after_window_forwards` — second PLI at `t0 + 300 ms` returns `Some(owner)` again. +- `pli_different_streams_independent` — PLIs on `stream_id=0` and `stream_id=1` in the same room and same instant both forward. +- `pli_different_rooms_independent` — PLIs in `room-a` and `room-b` at the same instant both forward. +- `pli_no_owner_returns_none` — PLI for a stream with no `stream_owner` entry returns `None` (the new short-circuit from `36b0421`). + +Test helper `seed_stream_owner(mgr, room, stream_id, owner)` directly inserts into `RoomManager::stream_owner` for fixture setup. + +Verification: + +``` +$ cargo test -p wzp-relay --lib pli +test result: ok. 6 passed; 0 failed; 0 ignored; 0 measured; 93 filtered out + +$ cargo test -p wzp-relay --lib +test result: ok. 99 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out + +$ cargo clippy -p wzp-relay --all-targets -- -D warnings +clean + +$ cargo fmt --all -- --check +clean +``` + +wzp-relay lib tests: 93 → 99 (+6 PLI). + ## Reviewer checklist (filled in by reviewer) -- [ ] Code matches PRD intent -- [ ] Verification output is real -- [ ] No backward-incompat surprises -- [ ] Tests cover the new behavior -- [ ] Approved +- [x] Code matches PRD intent — PLI dedup window per `(room, sender, stream_id)`, 200 ms, with per-sender forwarding via `stream_owner` map +- [x] Verification output is real — re-ran `cargo test -p wzp-relay --lib pli` (6 pass) + full `cargo test -p wzp-relay --lib` (99 pass); clippy + fmt clean +- [x] No backward-incompat surprises — `should_forward_pli` signature changed, only one production caller, updated +- [x] Tests cover the new behavior — 6 unit tests covering the dedup window from both sides + cross-stream / cross-room independence + missing-owner +- [x] Approved + +### Reviewer notes (chat-authoritative, per the policy from T4.6) + +The rework was done by me (the reviewer) rather than the agent because, as you put it, "above the agent's paygrade" — they shipped two iterations of T4.7 without ever doing the testability refactor I asked for, despite it being a 30-minute change. Approved at commit `001d94f`. + +Two structural fixes I made beyond the strict CR ask: + +- Used `now.saturating_duration_since(entry.last_pli)` instead of `.elapsed()` — the latter calls `Instant::now()` internally and would defeat the testability refactor. Subtle but necessary. +- Added a 6th test (`pli_no_owner_returns_none`) for the early-return path the agent introduced in `36b0421`. The agent introduced the code path; I wrote the test for it. diff --git a/docs/PRD/reports/T5.1-report.md b/docs/PRD/reports/T5.1-report.md new file mode 100644 index 0000000..1eba053 --- /dev/null +++ b/docs/PRD/reports/T5.1-report.md @@ -0,0 +1,90 @@ +# T5.1 — `PriorityMode` enum + `SignalMessage::SetPriorityMode` + +**Status:** Pending Review +**Agent:** Kimi Code CLI +**Started:** 2026-05-12T17:00Z +**Completed:** 2026-05-12T17:25Z +**Commit:** c8d1239 +**PRD:** ../PRD-video-quality-priority.md + +## What I changed + +- `crates/wzp-proto/src/priority_mode.rs` — New file. `PriorityMode` enum with four variants: `AudioFirst` (default), `VideoFirst`, `ScreenShare`, `Balanced`. Derives `Debug, Clone, Copy, PartialEq, Eq, Default, Serialize, Deserialize`. +- `crates/wzp-proto/src/lib.rs` — Added `pub mod priority_mode;` and `pub use priority_mode::PriorityMode;`. +- `crates/wzp-proto/src/codec_id.rs:124-145` — Added four new fields to `QualityProfile`: `priority_mode: PriorityMode`, `video_bitrate_kbps: Option`, `video_resolution: Option<(u16, u16)>`, `video_fps: Option`. All carry `#[serde(default)]` for backward compat. +- `crates/wzp-proto/src/codec_id.rs:149-214` — Updated all `QualityProfile` const constructors (`GOOD`, `DEGRADED`, `CATASTROPHIC`, `STUDIO_32K`, `STUDIO_48K`, `STUDIO_64K`) to include the new fields. +- `crates/wzp-proto/src/packet.rs:1200-1207` — Added `SignalMessage::SetPriorityMode { version, mode }` variant before `PictureLossIndication`. +- `crates/wzp-client/src/featherchat.rs:144-147` — Added `SetPriorityMode` to `signal_to_call_type` match arm. +- `crates/wzp-client/src/call.rs:639-654` — Updated explicit `QualityProfile` constructions to use `..QualityProfile::GOOD` struct-update syntax. +- `crates/wzp-android/src/engine.rs:975-980` — Same struct-update fix. +- `crates/wzp-android/src/jni_bridge.rs:32-38` — Same struct-update fix. +- `desktop/src-tauri/src/engine.rs:77-82, 118-123` — Same struct-update fix. +- `crates/wzp-codec/src/lib.rs:73-82` — Same struct-update fix. + +## Deviations from the task spec + +Skeleton task — no numbered steps. Followed PRD-video-quality-priority.md sections "PriorityMode" and "Mid-call change". + +## Verification output + +```bash +$ cargo test -p wzp-proto --no-fail-fast +test result: ok. 125 passed; 0 failed; 0 ignored +``` + +```bash +$ cargo test -p wzp-relay --lib --no-fail-fast +test result: ok. 99 passed; 0 failed; 0 ignored +``` + +```bash +$ cargo test -p wzp-client --lib --no-fail-fast +test result: ok. 163 passed; 0 failed; 7 ignored +``` + +```bash +$ cargo test -p wzp-codec --lib --no-fail-fast +test result: ok. 69 passed; 0 failed; 0 ignored +``` + +```bash +$ cargo test -p wzp-android --lib --no-fail-fast +test result: ok. 4 passed; 0 failed; 0 ignored +``` + +```bash +$ cargo test -p wzp-desktop --lib --no-fail-fast +test result: ok. 0 passed; 0 failed; 0 ignored +``` + +```bash +$ cargo fmt --all -- --check +# pass +``` + +```bash +$ cargo clippy -p wzp-proto --all-targets -- -D warnings +# pass +``` + +## Test summary + +- Tests added: 0 +- Tests modified: 0 +- Workspace test count: 460+ pass in affected crates +- `cargo clippy -p wzp-proto --all-targets -- -D warnings`: pass +- `cargo fmt --all -- --check`: pass + +## Risks / follow-ups + +1. **`QualityProfile` is now wider** — Four new fields add 11 bytes (1 + 4 + 4 + 1 + padding). Since `QualityProfile` is `Copy` and used in hot paths, monitor size. If it grows past 32 bytes, consider boxing optional fields. +2. **Serde default for backward compat** — Old serialized `QualityProfile` without the new fields will deserialize correctly because all four fields have `#[serde(default)]`. Forward compat (new → old) is not guaranteed. +3. **`SetPriorityMode` not yet consumed** — The signal variant is defined but no engine (client, android, desktop) handles it yet. T5.2 / T5.3 will wire the controller. + +## Reviewer checklist (filled in by reviewer) + +- [ ] Code matches PRD intent +- [ ] Verification output is real +- [ ] No backward-incompat surprises +- [ ] Tests cover the new behavior +- [ ] Approved