T5.1: PriorityMode enum + SetPriorityMode signal; extend QualityProfile with video fields

2026-05-12 12:21:11 +04:00
parent 001d94f9ae
commit 276ecc660e
13 changed files with 227 additions and 13 deletions
--- 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 |
--- 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<ParticipantId>` 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.
--- a/docs/PRD/reports/T5.1-report.md
+++ 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<u32>`, `video_resolution: Option<(u16, u16)>`, `video_fps: Option<u8>`. 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