feat(video+desktop): camera capture, video UI, E2E AEAD wiring, test fixes
Blockers 4 & 5: browser getUserMedia → JPEG IPC → Rust I420 pipeline; remote video strip renders decoded frames via canvas; EncryptingTransport wraps QuinnTransport so WZP AEAD is applied to all media (C2 fix). Test fixes: HandshakeResult.session destructuring across relay/client/crypto integration tests; video_codecs field added to all CallOffer/CallAnswer structs; wzp-video pipeline_roundtrip integration tests added. PRD docs: five Kimi-ready specs for E2E encryption, Android NDK 0.9 migration, quality upgrade flow, wire-format hardening, and clippy debt. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
225
docs/PRD/PRD-android-mediacodec-ndk9.md
Normal file
225
docs/PRD/PRD-android-mediacodec-ndk9.md
Normal file
@@ -0,0 +1,225 @@
|
||||
# PRD: Android MediaCodec NDK 0.9 Compatibility
|
||||
|
||||
> **Status:** proposed
|
||||
> **Resolves:** 31 compile errors in `crates/wzp-video/src/mediacodec.rs` blocking all Android video.
|
||||
> **Depends on:** Remote build server `manwe@188.245.59.196` with Docker image `wzp-android-builder:latest`.
|
||||
|
||||
## Problem
|
||||
|
||||
`crates/wzp-video/src/mediacodec.rs` fails to compile for
|
||||
`aarch64-linux-android` against the NDK 0.9 Rust crate. There are 31 errors
|
||||
in 5 categories. Android video is completely blocked.
|
||||
|
||||
The file already compiles for non-Android targets (all Android code is behind
|
||||
`#[cfg(target_os = "android")]`). Only the Android target path needs fixing.
|
||||
|
||||
## Goals
|
||||
|
||||
- `cargo build --target aarch64-linux-android -p wzp-video` produces 0 errors on the remote server.
|
||||
- Each fix category lands in a separate commit so failures can be bisected.
|
||||
- Non-Android compilation is not broken.
|
||||
|
||||
## Non-goals
|
||||
|
||||
- Upgrading the NDK Docker image or changing the NDK version.
|
||||
- Fixing video functionality beyond compilation (runtime testing is a separate task).
|
||||
- Any files outside `crates/wzp-video/`.
|
||||
|
||||
## Design
|
||||
|
||||
### Build command (run after each fix)
|
||||
|
||||
```bash
|
||||
ssh manwe@188.245.59.196 'cd ~/wzp-builder/data/source && \
|
||||
git fetch github && git reset --hard github/experimental-ui && \
|
||||
docker run --rm \
|
||||
-v ~/wzp-builder/data/source:/build/source \
|
||||
-v ~/wzp-builder/data/cache/cargo-registry:/home/builder/.cargo/registry \
|
||||
-v ~/wzp-builder/data/cache/cargo-git:/home/builder/.cargo/git \
|
||||
-v ~/wzp-builder/data/cache/target:/build/source/target \
|
||||
wzp-android-builder:latest bash -c \
|
||||
"cd /build/source && cargo build --target aarch64-linux-android -p wzp-video 2>&1 | grep -E \"^error\" | head -30"'
|
||||
```
|
||||
|
||||
### Fix order (commit one per category)
|
||||
|
||||
#### Fix 1 — `E0433`: `ndk_sys` not declared as a dependency
|
||||
|
||||
**Symptom**: `use of undeclared crate or module 'ndk_sys'`
|
||||
|
||||
**File**: `crates/wzp-video/Cargo.toml`
|
||||
|
||||
NDK 0.9 no longer re-exports raw `ndk_sys` symbols; they must be declared as
|
||||
a direct dependency. Add to the `[target.'cfg(target_os = "android")'.dependencies]`
|
||||
section (or create it if absent):
|
||||
|
||||
```toml
|
||||
[target.'cfg(target_os = "android")'.dependencies]
|
||||
ndk = { version = "0.9" }
|
||||
ndk-sys = { version = "0.6" } # ndk 0.9 depends on ndk-sys 0.6
|
||||
```
|
||||
|
||||
If `mediacodec.rs` only uses safe wrappers from the `ndk` crate and the
|
||||
`ndk_sys` imports are not strictly needed, remove the `use ndk_sys::*` lines
|
||||
from `mediacodec.rs` instead — whichever approach results in fewer changes.
|
||||
|
||||
After this fix the `E0433` errors should drop from the build output.
|
||||
|
||||
#### Fix 2 — `E0425`: `BITRATE_MODE_CBR` constant missing
|
||||
|
||||
**Symptom**: `cannot find value 'BITRATE_MODE_CBR' in this scope`
|
||||
|
||||
**File**: `crates/wzp-video/src/mediacodec.rs`
|
||||
|
||||
`BITRATE_MODE_CBR` is already defined as a local constant at line 44:
|
||||
|
||||
```rust
|
||||
#[cfg(target_os = "android")]
|
||||
const BITRATE_MODE_CBR: i32 = 2;
|
||||
```
|
||||
|
||||
If the error persists after Fix 1, the issue is that `ndk_sys` was providing
|
||||
a conflicting symbol. Verify the constant is still at line 44 after Fix 1. If
|
||||
NDK 0.9 moved `BITRATE_MODE_CBR` to an enum, update the usage at line 516
|
||||
(`format.set_i32("bitrate-mode", BITRATE_MODE_CBR)`) to use the integer
|
||||
value directly (`2`) or update the constant's value.
|
||||
|
||||
If `ndk 0.9` defines `MediaCodecBitrateMode::Cbr` as an enum, the call site
|
||||
in `MediaCodecAv1Encoder::new` (line ~516) can be updated to:
|
||||
|
||||
```rust
|
||||
format.set_i32(
|
||||
"bitrate-mode",
|
||||
ndk::media::media_codec::MediaCodecBitrateMode::Cbr as i32,
|
||||
);
|
||||
```
|
||||
|
||||
#### Fix 3 — `E0308`: `InputBuffer` returns `&mut [MaybeUninit<u8>]`
|
||||
|
||||
**Symptom**: `expected &mut [u8], found &mut [MaybeUninit<u8>]`
|
||||
|
||||
**File**: `crates/wzp-video/src/mediacodec.rs`
|
||||
|
||||
NDK 0.9 changed `InputBuffer::buffer_mut()` from `&mut [u8]` to
|
||||
`&mut [MaybeUninit<u8>]`. There are multiple write sites in the file — all
|
||||
follow the same pattern:
|
||||
|
||||
```rust
|
||||
// Before (NDK 0.8):
|
||||
let buf = buffer.buffer_mut(); // &mut [u8]
|
||||
let n = frame.data.len().min(buf.len());
|
||||
buf[..n].copy_from_slice(&frame.data[..n]);
|
||||
```
|
||||
|
||||
```rust
|
||||
// After (NDK 0.9):
|
||||
let buf = buffer.buffer_mut(); // &mut [MaybeUninit<u8>]
|
||||
let n = frame.data.len().min(buf.len());
|
||||
for (d, &s) in buf[..n].iter_mut().zip(frame.data[..n].iter()) {
|
||||
d.write(s);
|
||||
}
|
||||
```
|
||||
|
||||
The file already uses the `d.write(s)` pattern in some places (lines 125–127,
|
||||
297–299, etc.). Search for **every** occurrence of `buffer.buffer_mut()` and
|
||||
`buffer_mut()` and apply the same pattern. Affected structs:
|
||||
`MediaCodecEncoder::encode` (~line 123), `MediaCodecDecoder::decode`
|
||||
(~line 294), `MediaCodecHevcEncoder::encode` (~line 439),
|
||||
`MediaCodecHevcDecoder::decode` (~line 773), `MediaCodecAv1Encoder::encode`
|
||||
(~line 560), `MediaCodecAv1Decoder::decode` (~line 907).
|
||||
|
||||
Do NOT use `unsafe { std::mem::transmute }` — the `d.write(s)` pattern is
|
||||
already present and safe.
|
||||
|
||||
Note: if the file already uses `d.write(s)` everywhere, this category may
|
||||
already be addressed by the existing code. Check the actual error count.
|
||||
|
||||
#### Fix 4 — `E0599`: `.index()` is private
|
||||
|
||||
**Symptom**: `method 'index' is private`
|
||||
|
||||
**File**: `crates/wzp-video/src/mediacodec.rs`
|
||||
|
||||
NDK 0.9 removed the public `.index()` method from `DequeuedInputBuffer` and
|
||||
`DequeuedOutputBuffer`. The pattern that broke:
|
||||
|
||||
```rust
|
||||
// Broken: buffer.index() is private in NDK 0.9
|
||||
let idx = buffer.index();
|
||||
codec.queue_input_buffer_index(idx, ...);
|
||||
```
|
||||
|
||||
In NDK 0.9 the correct API is to pass the buffer object directly to
|
||||
`queue_input_buffer`:
|
||||
|
||||
```rust
|
||||
codec.queue_input_buffer(buffer, offset, size, pts_us, flags)?;
|
||||
```
|
||||
|
||||
The file already uses `codec.queue_input_buffer(buffer, 0, to_copy, ...)` in
|
||||
most places (lines 131, 303, 447, etc.). Search for any remaining `.index()`
|
||||
calls on buffer objects and replace them with the direct-pass pattern shown
|
||||
above.
|
||||
|
||||
#### Fix 5 — `E0277`: `NonNull<AMediaCodec>` is not `Send`
|
||||
|
||||
**Symptom**: `NonNull<AMediaCodec>` cannot be sent between threads safely
|
||||
|
||||
**File**: `crates/wzp-video/src/mediacodec.rs`
|
||||
|
||||
Each codec struct must have an `unsafe impl Send` declaration. Audit all six
|
||||
codec structs:
|
||||
|
||||
| Struct | `unsafe impl Send` present? |
|
||||
|--------|----------------------------|
|
||||
| `MediaCodecEncoder` | Yes (line 51) |
|
||||
| `MediaCodecDecoder` | Yes (line 228) |
|
||||
| `MediaCodecHevcEncoder` | Yes (line 374) |
|
||||
| `MediaCodecHevcDecoder` | Yes (line 705) |
|
||||
| `MediaCodecAv1Encoder` | Yes (line 503) |
|
||||
| `MediaCodecAv1Decoder` | Yes (line 844) |
|
||||
|
||||
If any are missing, add them with a safety comment:
|
||||
|
||||
```rust
|
||||
// SAFETY: AMediaCodec is documented as thread-safe.
|
||||
#[cfg(target_os = "android")]
|
||||
unsafe impl Send for MediaCodecXxxYyy {}
|
||||
```
|
||||
|
||||
This category may already be clean. Confirm with the build output.
|
||||
|
||||
## Implementation steps
|
||||
|
||||
1. Push the current branch to `github/experimental-ui` before starting.
|
||||
2. **Commit 1**: Fix `ndk_sys` dependency (`Cargo.toml`). Push. Run build.
|
||||
Confirm `E0433` errors drop.
|
||||
3. **Commit 2**: Fix `BITRATE_MODE_CBR`. Push. Run build. Confirm `E0425` gone.
|
||||
4. **Commit 3**: Fix `MaybeUninit` write sites. Push. Run build. Confirm
|
||||
`E0308` gone.
|
||||
5. **Commit 4**: Remove any `.index()` calls. Push. Run build. Confirm
|
||||
`E0599` gone.
|
||||
6. **Commit 5**: Add missing `unsafe impl Send` if any. Push. Run build.
|
||||
Confirm `E0277` gone and total error count is 0.
|
||||
|
||||
## Files to read before implementing
|
||||
|
||||
- `crates/wzp-video/src/mediacodec.rs` (full file — 45 KB; read in chunks)
|
||||
- `crates/wzp-video/Cargo.toml` (check existing `[dependencies]` sections)
|
||||
|
||||
## Verify
|
||||
|
||||
Final build command (see Design section). Expected output: no lines matching
|
||||
`^error`.
|
||||
|
||||
Also verify non-Android host still compiles:
|
||||
|
||||
```bash
|
||||
cargo check -p wzp-video
|
||||
```
|
||||
|
||||
## Done when
|
||||
|
||||
`cargo build --target aarch64-linux-android -p wzp-video` on the remote
|
||||
server produces 0 `error[...]` lines. Non-Android `cargo check -p wzp-video`
|
||||
also passes.
|
||||
260
docs/PRD/PRD-clippy-debt.md
Normal file
260
docs/PRD/PRD-clippy-debt.md
Normal file
@@ -0,0 +1,260 @@
|
||||
# PRD: Fix wzp-codec Clippy Lint Debt
|
||||
|
||||
> **Status:** proposed
|
||||
> **Resolves:** 9 pre-existing clippy lints in `crates/wzp-codec/src/` that cause `cargo clippy --workspace -D warnings` to fail, breaking any strict-CI configuration.
|
||||
> **Depends on:** Nothing — all changes are in `crates/wzp-codec/src/`.
|
||||
|
||||
## Problem
|
||||
|
||||
`cargo clippy -p wzp-codec -- -D warnings` fails with 9 lints across 5 files.
|
||||
These are pre-existing code patterns that were never flagged during development
|
||||
because the CI flag was not set. They have no runtime impact today but prevent
|
||||
adding `-D warnings` to CI without first cleaning them up.
|
||||
|
||||
The 3 errors in `deps/featherchat` are in a submodule — do NOT touch them.
|
||||
`warzone_protocol` clippy errors are accepted debt (not our code).
|
||||
|
||||
## Goals
|
||||
|
||||
- `cargo clippy -p wzp-codec -- -D warnings` exits 0.
|
||||
- No behavior changes — every fix is a semantically equivalent rewrite.
|
||||
- No changes outside `crates/wzp-codec/src/`.
|
||||
|
||||
## Non-goals
|
||||
|
||||
- Fixing clippy lints in any crate other than `wzp-codec`.
|
||||
- Adding new functionality.
|
||||
- Touching the `deps/featherchat` submodule.
|
||||
|
||||
## Design
|
||||
|
||||
### Lint inventory
|
||||
|
||||
| Lint | Count | File | Approx line | Fix |
|
||||
|------|-------|------|-------------|-----|
|
||||
| `implicit_saturating_sub` | 1 | `aec.rs` | 117–119 | `saturating_sub` |
|
||||
| `needless_range_loop` | 2 | `aec.rs:164`, `resample.rs:51` | — | iterate with `iter().enumerate()` or direct iter |
|
||||
| `manual_div_ceil` | 2 | `codec2_dec.rs:48`, `codec2_enc.rs:48` | — | `div_ceil` |
|
||||
| `manual_clamp` | 2 | `denoise.rs:59`, `opus_enc.rs:250` | — | `.clamp(min, max)` |
|
||||
| `manual_ascii_check` | 1 | `opus_enc.rs:104` | — | `.eq_ignore_ascii_case()` |
|
||||
| `same_item_push` | 1 | `resample.rs:184` | — | `vec.resize` or `extend(repeat)` |
|
||||
|
||||
### Fix details
|
||||
|
||||
#### 1. `implicit_saturating_sub` — `aec.rs` line ~117
|
||||
|
||||
Current code:
|
||||
|
||||
```rust
|
||||
fn delay_available(&self) -> usize {
|
||||
let buffered = self.delay_write - self.delay_read;
|
||||
if buffered > self.delay_samples {
|
||||
buffered - self.delay_samples
|
||||
} else {
|
||||
0
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
Clippy wants `saturating_sub` because the subtraction can underflow if
|
||||
`buffered < self.delay_samples`:
|
||||
|
||||
```rust
|
||||
fn delay_available(&self) -> usize {
|
||||
let buffered = self.delay_write - self.delay_read;
|
||||
buffered.saturating_sub(self.delay_samples)
|
||||
}
|
||||
```
|
||||
|
||||
This is semantically identical (both return 0 when `buffered <= delay_samples`).
|
||||
|
||||
#### 2a. `needless_range_loop` — `aec.rs` line ~164
|
||||
|
||||
Current code:
|
||||
|
||||
```rust
|
||||
for i in 0..n {
|
||||
let near_f = nearend[i] as f32;
|
||||
let base = (self.far_pos + fl * ((n / fl) + 2) + i - n) % fl;
|
||||
...
|
||||
}
|
||||
```
|
||||
|
||||
`i` is used both to index `nearend[i]` and in arithmetic (`+ i - n`).
|
||||
Clippy fires because `nearend[i]` could use `.iter().enumerate()`.
|
||||
Convert to `enumerate`:
|
||||
|
||||
```rust
|
||||
for (i, &sample) in nearend.iter().enumerate() {
|
||||
let near_f = sample as f32;
|
||||
let base = (self.far_pos + fl * ((n / fl) + 2) + i - n) % fl;
|
||||
...
|
||||
}
|
||||
```
|
||||
|
||||
Make sure to update any references to `nearend[i]` inside the loop body
|
||||
to use `sample` (or `near_f` directly). Also update the NLMS adaptation
|
||||
sub-loop if it references `nearend[i]`.
|
||||
|
||||
#### 2b. `needless_range_loop` — `resample.rs` line ~51
|
||||
|
||||
Current code:
|
||||
|
||||
```rust
|
||||
for i in 0..FIR_TAPS {
|
||||
let n = i as f64 - m / 2.0;
|
||||
let sinc = ...;
|
||||
let t = 2.0 * i as f64 / m - 1.0;
|
||||
let kaiser = ...;
|
||||
kernel[i] = sinc * kaiser;
|
||||
}
|
||||
```
|
||||
|
||||
`i` is used both as an index (`kernel[i]`) and in arithmetic. Use
|
||||
`iter_mut().enumerate()`:
|
||||
|
||||
```rust
|
||||
for (i, slot) in kernel.iter_mut().enumerate() {
|
||||
let n = i as f64 - m / 2.0;
|
||||
let sinc = ...;
|
||||
let t = 2.0 * i as f64 / m - 1.0;
|
||||
let kaiser = ...;
|
||||
*slot = sinc * kaiser;
|
||||
}
|
||||
```
|
||||
|
||||
#### 3a. `manual_div_ceil` — `codec2_dec.rs` line ~48
|
||||
|
||||
Current code:
|
||||
|
||||
```rust
|
||||
fn bytes_per_frame(&self) -> usize {
|
||||
(self.inner.bits_per_frame() + 7) / 8
|
||||
}
|
||||
```
|
||||
|
||||
Replace with:
|
||||
|
||||
```rust
|
||||
fn bytes_per_frame(&self) -> usize {
|
||||
self.inner.bits_per_frame().div_ceil(8)
|
||||
}
|
||||
```
|
||||
|
||||
`div_ceil` is stable as of Rust 1.73. The builder uses a recent enough
|
||||
toolchain. If `bits_per_frame()` returns `usize`, the method is available.
|
||||
If it returns a different integer type, cast accordingly.
|
||||
|
||||
#### 3b. `manual_div_ceil` — `codec2_enc.rs` line ~48
|
||||
|
||||
Same pattern, same fix:
|
||||
|
||||
```rust
|
||||
fn bytes_per_frame(&self) -> usize {
|
||||
self.inner.bits_per_frame().div_ceil(8)
|
||||
}
|
||||
```
|
||||
|
||||
#### 4a. `manual_clamp` — `denoise.rs` line ~59
|
||||
|
||||
Current code:
|
||||
|
||||
```rust
|
||||
let clamped = val.max(-32768.0).min(32767.0);
|
||||
```
|
||||
|
||||
Replace with:
|
||||
|
||||
```rust
|
||||
let clamped = val.clamp(-32768.0_f32, 32767.0_f32);
|
||||
```
|
||||
|
||||
Note: `.clamp()` on `f32` requires both bounds to be the same type. If `val`
|
||||
is already `f32`, no extra cast is needed. Verify the type of `val` in
|
||||
context (it is `f32` per the output array type `[f32; 480]`).
|
||||
|
||||
#### 4b. `manual_clamp` — `opus_enc.rs` line ~252
|
||||
|
||||
Read the surrounding code for the exact pattern. It will be something like:
|
||||
|
||||
```rust
|
||||
let v = if x < min_val { min_val } else if x > max_val { max_val } else { x };
|
||||
```
|
||||
|
||||
or the `.max().min()` chain. Replace with `x.clamp(min_val, max_val)`.
|
||||
|
||||
#### 5. `manual_ascii_check` — `opus_enc.rs` line ~104
|
||||
|
||||
Current code:
|
||||
|
||||
```rust
|
||||
Ok(v) => !v.is_empty() && v != "0" && v.to_ascii_lowercase() != "false",
|
||||
```
|
||||
|
||||
Clippy wants `.eq_ignore_ascii_case()` instead of lowercasing the whole string:
|
||||
|
||||
```rust
|
||||
Ok(v) => !v.is_empty() && v != "0" && !v.eq_ignore_ascii_case("false"),
|
||||
```
|
||||
|
||||
#### 6. `same_item_push` — `resample.rs` line ~183
|
||||
|
||||
Current code:
|
||||
|
||||
```rust
|
||||
for _ in 1..RATIO {
|
||||
work.push(0.0);
|
||||
}
|
||||
```
|
||||
|
||||
This pushes the same `0.0` value `(RATIO - 1)` times. Replace with:
|
||||
|
||||
```rust
|
||||
work.resize(work.len() + (RATIO - 1), 0.0f64);
|
||||
```
|
||||
|
||||
Or equivalently:
|
||||
|
||||
```rust
|
||||
work.extend(std::iter::repeat(0.0f64).take(RATIO - 1));
|
||||
```
|
||||
|
||||
Note: `RATIO` is a `const usize`. Verify `work` is `Vec<f64>` in context
|
||||
(it is — `work.push(s as f64)` immediately before).
|
||||
|
||||
## Implementation steps
|
||||
|
||||
1. Read each file at the line numbers listed above to confirm the exact current
|
||||
code before editing (line numbers may shift slightly due to prior edits).
|
||||
2. Apply all 9 fixes. They are independent — no ordering requirement.
|
||||
3. Run `cargo clippy -p wzp-codec -- -D warnings` locally or via the CI
|
||||
command.
|
||||
4. If any lint persists, re-read that file section and adjust.
|
||||
|
||||
## Files to read before implementing
|
||||
|
||||
- `crates/wzp-codec/src/aec.rs` lines 114–200
|
||||
- `crates/wzp-codec/src/resample.rs` lines 45–70 and 178–190
|
||||
- `crates/wzp-codec/src/codec2_dec.rs` lines 40–55
|
||||
- `crates/wzp-codec/src/codec2_enc.rs` lines 40–55
|
||||
- `crates/wzp-codec/src/denoise.rs` lines 45–65
|
||||
- `crates/wzp-codec/src/opus_enc.rs` lines 96–110 and 244–260
|
||||
|
||||
## Verify
|
||||
|
||||
```bash
|
||||
cargo clippy -p wzp-codec -- -D warnings
|
||||
```
|
||||
|
||||
Expected: exits 0 with no warnings.
|
||||
|
||||
Also run to confirm no regressions:
|
||||
|
||||
```bash
|
||||
cargo test -p wzp-codec
|
||||
```
|
||||
|
||||
## Done when
|
||||
|
||||
`cargo clippy -p wzp-codec -- -D warnings` exits 0. All 9 lints are gone.
|
||||
`cargo test -p wzp-codec` passes. No changes outside `crates/wzp-codec/src/`.
|
||||
195
docs/PRD/PRD-e2e-media-encryption.md
Normal file
195
docs/PRD/PRD-e2e-media-encryption.md
Normal file
@@ -0,0 +1,195 @@
|
||||
# PRD: E2E Media Encryption — Wire EncryptingTransport on Relay Path
|
||||
|
||||
> **Status:** proposed
|
||||
> **Resolves:** Security gap — relay-path media travels in QUIC TLS only; WZP application-layer ChaCha20-Poly1305 is negotiated but never applied.
|
||||
> **Depends on:** `wzp_client::encrypted_transport::EncryptingTransport` (already implemented).
|
||||
|
||||
## Problem
|
||||
|
||||
`CallEngine::start` (both the Android path and the desktop path) calls
|
||||
`wzp_client::handshake::perform_handshake`, which returns a `HandshakeResult`
|
||||
containing a `session: Box<dyn CryptoSession>` (a keyed `ChaChaSession`).
|
||||
Both call sites discard the session — only `hs.video_codec` is retained.
|
||||
|
||||
All subsequent `send_media` / `recv_media` calls go directly through
|
||||
`Arc<wzp_transport::QuinnTransport>`, which provides QUIC TLS (relay sees
|
||||
plaintext application data after TLS termination at the relay). The WZP
|
||||
application-level AEAD — ChaCha20-Poly1305, keyed per-call, relay-never-sees
|
||||
— is never applied.
|
||||
|
||||
`wzp_client::encrypted_transport::EncryptingTransport` exists
|
||||
(`crates/wzp-client/src/encrypted_transport.rs`) and is fully tested.
|
||||
It wraps any `Arc<dyn MediaTransport>` and intercepts every `send_media` /
|
||||
`recv_media` call with `session.encrypt()` / `session.decrypt()`.
|
||||
|
||||
## Goals
|
||||
|
||||
- The relay-path `HandshakeResult::session` is used to construct an
|
||||
`EncryptingTransport` that wraps the raw `QuinnTransport`.
|
||||
- All `send_media` and `recv_media` calls in the relay path go through the
|
||||
wrapper, not the raw transport.
|
||||
- The direct P2P path (`is_direct_p2p == true`) is left unchanged — QUIC TLS
|
||||
is the encryption layer there.
|
||||
- `cargo check --manifest-path desktop/src-tauri/Cargo.toml` passes.
|
||||
- A `#[cfg(test)]` test verifies that the relay path uses `EncryptingTransport`.
|
||||
|
||||
## Non-goals
|
||||
|
||||
- Rekeying (`SignalMessage::Rekey`) — tracked separately.
|
||||
- Video transport encryption (same mechanism; apply after audio is confirmed working).
|
||||
- Changes to the P2P path, the relay binary, or any crate outside `desktop/src-tauri`.
|
||||
|
||||
## Design
|
||||
|
||||
### `EncryptingTransport` API (read `crates/wzp-client/src/encrypted_transport.rs`)
|
||||
|
||||
```rust
|
||||
pub struct EncryptingTransport { ... }
|
||||
|
||||
impl EncryptingTransport {
|
||||
pub fn new(inner: Arc<dyn MediaTransport>, session: Box<dyn CryptoSession>) -> Self;
|
||||
}
|
||||
|
||||
// Implements MediaTransport:
|
||||
// send_media → session.encrypt(header_bytes, payload) → inner.send_media
|
||||
// recv_media → inner.recv_media → session.decrypt(header_bytes, ciphertext)
|
||||
// send_signal / recv_signal / path_quality / close → forwarded unchanged
|
||||
```
|
||||
|
||||
`EncryptingTransport` is NOT `Arc`-wrapped by the constructor; wrap it in
|
||||
`Arc::new(...)` when storing as `Arc<dyn MediaTransport>`.
|
||||
|
||||
### Two call sites in `desktop/src-tauri/src/engine.rs`
|
||||
|
||||
**Call site 1 — Android path** (`CallEngine::start` around line 575):
|
||||
|
||||
```rust
|
||||
if !is_direct_p2p {
|
||||
let _hs = match wzp_client::handshake::perform_handshake(...).await { Ok(hs) => hs, ... };
|
||||
// hs.session is discarded here — fix this
|
||||
}
|
||||
```
|
||||
|
||||
Change: capture `hs`, then build a wrapped transport:
|
||||
|
||||
```rust
|
||||
if !is_direct_p2p {
|
||||
let hs = match wzp_client::handshake::perform_handshake(...).await { Ok(hs) => hs, ... };
|
||||
info!(video_codec = ?hs.video_codec, "handshake complete");
|
||||
let transport: Arc<dyn wzp_proto::MediaTransport> =
|
||||
Arc::new(wzp_client::encrypted_transport::EncryptingTransport::new(
|
||||
transport.clone(),
|
||||
hs.session,
|
||||
));
|
||||
// use `transport` (the wrapped version) for all subsequent send_t / recv_t clones
|
||||
}
|
||||
```
|
||||
|
||||
The variable `transport` must shadow the raw `Arc<QuinnTransport>` so that
|
||||
every subsequent clone of `transport` picks up the encrypted wrapper.
|
||||
|
||||
**Call site 2 — Desktop path** (`CallEngine::start` around line 1551):
|
||||
|
||||
```rust
|
||||
let _negotiated_video_codec = if !is_direct_p2p {
|
||||
let hs = wzp_client::handshake::perform_handshake(...).await?;
|
||||
info!(video_codec = ?hs.video_codec, "handshake complete");
|
||||
hs.video_codec // session dropped here — fix this
|
||||
} else { None };
|
||||
```
|
||||
|
||||
Change: extract `session` before returning `video_codec`, then shadow
|
||||
`transport` with the wrapped version. Because `transport` is used after this
|
||||
block (cloned into `send_t`, `recv_t`, etc.), the shadow must happen inside
|
||||
the same scope or immediately after:
|
||||
|
||||
```rust
|
||||
let (_negotiated_video_codec, transport): (_, Arc<dyn wzp_proto::MediaTransport>) =
|
||||
if !is_direct_p2p {
|
||||
let hs = wzp_client::handshake::perform_handshake(...).await?;
|
||||
info!(video_codec = ?hs.video_codec, "handshake complete");
|
||||
let enc = Arc::new(wzp_client::encrypted_transport::EncryptingTransport::new(
|
||||
transport.clone(),
|
||||
hs.session,
|
||||
));
|
||||
(hs.video_codec, enc)
|
||||
} else {
|
||||
info!("direct P2P — skipping relay handshake");
|
||||
(None, transport.clone())
|
||||
};
|
||||
```
|
||||
|
||||
All subsequent `transport.clone()` calls then operate on the encrypted wrapper.
|
||||
|
||||
### Import
|
||||
|
||||
Add to the top of `engine.rs` if not already present:
|
||||
|
||||
```rust
|
||||
use wzp_client::encrypted_transport::EncryptingTransport;
|
||||
```
|
||||
|
||||
Or use the fully-qualified path inline (already shown above).
|
||||
|
||||
### Type compatibility
|
||||
|
||||
- `EncryptingTransport` implements `wzp_proto::MediaTransport` (confirmed in the source).
|
||||
- The existing `send_t` / `recv_t` variables are already typed as
|
||||
`Arc<dyn MediaTransport>` (or coerced on first use) — the shadow is a
|
||||
drop-in replacement.
|
||||
- The `vid_transport` for the video path (`line ~2090`) is also cloned from
|
||||
`transport`; it will automatically use the encrypted wrapper if the shadow
|
||||
is placed before those clones.
|
||||
|
||||
## Implementation steps
|
||||
|
||||
1. Read `desktop/src-tauri/src/engine.rs` lines 570–620 (Android path) and
|
||||
1547–1570 (desktop path) to see the exact variable names in each branch.
|
||||
2. **Android path fix** (line ~585): rename `_hs` to `hs`, extract
|
||||
`hs.session`, wrap `transport` with `EncryptingTransport::new`, re-bind
|
||||
`transport` as `Arc<dyn MediaTransport>`.
|
||||
3. **Desktop path fix** (line ~1551): restructure the
|
||||
`if !is_direct_p2p` block to return `(video_codec, wrapped_transport)`
|
||||
and shadow `transport`.
|
||||
4. Confirm that `vid_transport` (line ~2090) is cloned after the shadow — if
|
||||
it is, no further changes are needed for video.
|
||||
5. Run `cargo check --manifest-path desktop/src-tauri/Cargo.toml`. Fix any
|
||||
type-mismatch errors (usually a missing `as Arc<dyn MediaTransport>` cast
|
||||
or a moved value).
|
||||
6. Add a `#[cfg(test)]` module to `engine.rs` (or to a new
|
||||
`engine_tests.rs` included via `#[cfg(test)] mod engine_tests`) with a
|
||||
test that constructs a `LoopbackTransport`, calls `perform_handshake`
|
||||
against a mock relay fixture, and verifies that a received payload is
|
||||
decrypted before returning from `recv_media`. A simpler alternative that
|
||||
avoids a full handshake: assert `is::<EncryptingTransport>()` on the
|
||||
`transport` variable at the test call site using `std::any::Any`.
|
||||
|
||||
## Files to read before implementing
|
||||
|
||||
- `desktop/src-tauri/src/engine.rs` lines 475–625 (Android path) and
|
||||
1480–1570 (desktop path)
|
||||
- `crates/wzp-client/src/encrypted_transport.rs` (full — for the exact
|
||||
constructor signature and trait impl)
|
||||
- `crates/wzp-client/src/handshake.rs` (for `HandshakeResult` struct
|
||||
definition — confirm the `session` field name and type)
|
||||
|
||||
## Verify
|
||||
|
||||
```bash
|
||||
cargo check --manifest-path desktop/src-tauri/Cargo.toml
|
||||
```
|
||||
|
||||
Expected: 0 errors.
|
||||
|
||||
Manual smoke check: both `perform_handshake` call sites in `engine.rs` must
|
||||
use `hs.session` (grep: `hs\.session` should appear twice, once per call site).
|
||||
The string `_hs` must not remain on the relay path (only on the `_hs =` binding if the variable is intentionally unused before wrapping).
|
||||
|
||||
## Done when
|
||||
|
||||
- `cargo check --manifest-path desktop/src-tauri/Cargo.toml` exits 0.
|
||||
- Both relay-path `perform_handshake` call sites build an `EncryptingTransport`
|
||||
from `hs.session`.
|
||||
- The direct-P2P branch (`is_direct_p2p == true`) is unchanged.
|
||||
- A `#[cfg(test)]` test in `engine.rs` verifies that `EncryptingTransport`
|
||||
is used on the relay path (construction proof or decrypt round-trip).
|
||||
220
docs/PRD/PRD-quality-upgrade-flow.md
Normal file
220
docs/PRD/PRD-quality-upgrade-flow.md
Normal file
@@ -0,0 +1,220 @@
|
||||
# PRD: Quality Upgrade Flow — UpgradeProposal / Response / Confirm
|
||||
|
||||
> **Status:** proposed
|
||||
> **Resolves:** Four TODO comments in the signal task of `desktop/src-tauri/src/lib.rs` that leave quality upgrade messages unhandled. Audio quality never upgrades mid-call even when the network improves.
|
||||
> **Depends on:** `wzp_proto::SignalMessage::{UpgradeProposal, UpgradeResponse, UpgradeConfirm, QualityCapability}` (already defined in `crates/wzp-proto/src/packet.rs`).
|
||||
|
||||
## Problem
|
||||
|
||||
The signal receive task in `lib.rs` matches `UpgradeProposal`, `UpgradeResponse`,
|
||||
`UpgradeConfirm`, and `QualityCapability` messages from the peer, logs them,
|
||||
then hits a `// TODO` comment and does nothing. The 4 TODOs are at lines
|
||||
1930, 1949, 1966, and 1985 of `desktop/src-tauri/src/lib.rs`.
|
||||
|
||||
Consequence: audio quality is frozen at the profile negotiated at call start.
|
||||
Even when the network improves, the encoder never upgrades.
|
||||
|
||||
## Goals
|
||||
|
||||
1. `UpgradeProposal` auto-accepts and sends `UpgradeResponse { accepted: true }`.
|
||||
2. Accepted `UpgradeResponse` sends `UpgradeConfirm` and switches the local encoder.
|
||||
3. Received `UpgradeConfirm` switches the local encoder.
|
||||
4. Received `QualityCapability` caps the local encoder to the peer's max profile.
|
||||
5. A unit test verifies the accept/confirm round-trip.
|
||||
6. `cargo check --manifest-path desktop/src-tauri/Cargo.toml` passes.
|
||||
|
||||
## Non-goals
|
||||
|
||||
- UI for manual accept/reject of upgrade proposals (auto-accept only).
|
||||
- Sending `UpgradeProposal` from our side (the outgoing path already exists in
|
||||
`lib.rs`; this PRD only handles receiving).
|
||||
- Downgrade negotiation.
|
||||
- Persisting quality profiles across calls.
|
||||
|
||||
## Design
|
||||
|
||||
### New shared state
|
||||
|
||||
Add the following to `AppState` (or as captured variables in the signal task
|
||||
closure — whichever is cleaner given the existing structure):
|
||||
|
||||
```rust
|
||||
/// Pending outgoing upgrade: (call_id, proposal_id, profile).
|
||||
/// Set when we send an UpgradeProposal, consumed when we receive an accepted UpgradeResponse.
|
||||
pending_upgrade: Arc<Mutex<Option<(String, String, QualityProfile)>>>,
|
||||
|
||||
/// Current quality profile for the encoder. The audio send task reads this
|
||||
/// at the start of each encode cycle.
|
||||
active_quality: Arc<Mutex<QualityProfile>>,
|
||||
|
||||
/// Peer's reported maximum quality cap. The audio send task clamps to min(active, peer_max).
|
||||
peer_max_quality: Arc<Mutex<Option<QualityProfile>>>,
|
||||
```
|
||||
|
||||
If `AppState` already holds these fields (check `lib.rs` for the struct
|
||||
definition), reuse them instead of adding duplicates.
|
||||
|
||||
### Handler implementations
|
||||
|
||||
#### 1. `UpgradeProposal` (line ~1930)
|
||||
|
||||
```rust
|
||||
// Replace the TODO comment with:
|
||||
let response = SignalMessage::UpgradeResponse {
|
||||
version: wzp_proto::default_signal_version(),
|
||||
call_id: call_id.clone(),
|
||||
proposal_id: proposal_id.clone(),
|
||||
accepted: true,
|
||||
reason: None,
|
||||
};
|
||||
if let Err(e) = signal_transport.send_signal(&response).await {
|
||||
tracing::warn!("failed to send UpgradeResponse: {e}");
|
||||
}
|
||||
```
|
||||
|
||||
`signal_transport` is whatever variable holds the signal `Arc<dyn MediaTransport>`
|
||||
in scope at that match arm. Inspect the enclosing task to find the right name.
|
||||
|
||||
#### 2. `UpgradeResponse` (line ~1949)
|
||||
|
||||
```rust
|
||||
// Replace the TODO comment with:
|
||||
if accepted {
|
||||
// Retrieve the pending proposal to get the confirmed_profile.
|
||||
let maybe_proposal = pending_upgrade.lock().unwrap().take();
|
||||
if let Some((_cid, pid, profile)) = maybe_proposal {
|
||||
if pid == proposal_id {
|
||||
// Send UpgradeConfirm.
|
||||
let confirm = SignalMessage::UpgradeConfirm {
|
||||
version: wzp_proto::default_signal_version(),
|
||||
call_id: call_id.clone(),
|
||||
proposal_id: proposal_id.clone(),
|
||||
confirmed_profile: profile.clone(),
|
||||
};
|
||||
if let Err(e) = signal_transport.send_signal(&confirm).await {
|
||||
tracing::warn!("failed to send UpgradeConfirm: {e}");
|
||||
}
|
||||
// Switch our encoder.
|
||||
*active_quality.lock().unwrap() = profile;
|
||||
}
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
If `pending_upgrade` is a captured `Arc<Mutex<...>>` in the task closure, it
|
||||
can be read/written without going through `AppState`.
|
||||
|
||||
#### 3. `UpgradeConfirm` (line ~1966)
|
||||
|
||||
```rust
|
||||
// Replace the TODO comment with:
|
||||
*active_quality.lock().unwrap() = confirmed_profile;
|
||||
```
|
||||
|
||||
The audio send task (in `engine.rs`) reads `active_quality` at the start of
|
||||
each encode cycle and reconfigures the Opus encoder bitrate accordingly.
|
||||
|
||||
#### 4. `QualityCapability` (line ~1985)
|
||||
|
||||
```rust
|
||||
// Replace the TODO comment with:
|
||||
*peer_max_quality.lock().unwrap() = Some(max_profile);
|
||||
```
|
||||
|
||||
#### 5. Audio send task changes (`engine.rs`)
|
||||
|
||||
The audio send task already runs in a loop. Add a quality-check at the top of
|
||||
each encode iteration:
|
||||
|
||||
```rust
|
||||
// At the start of the encode loop body:
|
||||
let effective_profile = {
|
||||
let active = active_quality.lock().unwrap().clone();
|
||||
let peer_cap = peer_max_quality.lock().unwrap().clone();
|
||||
match peer_cap {
|
||||
Some(cap) if cap.opus_bitrate_bps() < active.opus_bitrate_bps() => cap,
|
||||
_ => active,
|
||||
}
|
||||
};
|
||||
// Pass effective_profile to encoder if it changed since last iteration.
|
||||
```
|
||||
|
||||
`QualityProfile::opus_bitrate_bps()` already exists (check
|
||||
`crates/wzp-proto/src/codec_id.rs`). If `QualityProfile` does not have a
|
||||
direct bitrate accessor, compare using the `PartialOrd` impl or a helper that
|
||||
ranks profiles numerically.
|
||||
|
||||
To avoid calling `encoder.set_bitrate()` every single frame, cache the last
|
||||
applied profile and only reconfigure on change:
|
||||
|
||||
```rust
|
||||
let mut last_applied_profile: Option<QualityProfile> = None;
|
||||
|
||||
// Inside loop:
|
||||
if Some(&effective_profile) != last_applied_profile.as_ref() {
|
||||
encoder.set_bitrate(effective_profile.opus_bitrate_bps());
|
||||
last_applied_profile = Some(effective_profile.clone());
|
||||
}
|
||||
```
|
||||
|
||||
`encoder.set_bitrate(bps: u32)` — add this method to `OpusEncoder` in
|
||||
`crates/wzp-codec/src/opus_enc.rs` if it does not exist. It wraps
|
||||
`opus_encoder_ctl(OPUS_SET_BITRATE_REQUEST, bps)`.
|
||||
|
||||
### Unit tests
|
||||
|
||||
Add a `#[cfg(test)]` module in `lib.rs` (or a dedicated test file) that:
|
||||
|
||||
1. Creates a `LoopbackSignalTransport` stub that records sent `SignalMessage`s.
|
||||
2. Calls the `UpgradeProposal` handler logic directly, asserts that an
|
||||
`UpgradeResponse { accepted: true }` was sent.
|
||||
3. Calls the `UpgradeResponse { accepted: true }` handler with a pre-populated
|
||||
`pending_upgrade`, asserts that `UpgradeConfirm` was sent and
|
||||
`active_quality` was updated.
|
||||
|
||||
These can be pure unit tests (no Tauri or audio), since the handlers are
|
||||
pure async functions over captured state.
|
||||
|
||||
## Implementation steps
|
||||
|
||||
1. Read `desktop/src-tauri/src/lib.rs` lines 1910–1990 (the four TODO blocks)
|
||||
and the surrounding signal task structure to identify the variable names
|
||||
for `signal_transport`, `app_state`, and any existing quality-state fields.
|
||||
2. Read `desktop/src-tauri/src/engine.rs` for `CallEngine` struct fields and
|
||||
the audio send task loop.
|
||||
3. Read `crates/wzp-proto/src/codec_id.rs` for `QualityProfile` methods.
|
||||
4. Add `pending_upgrade`, `active_quality`, `peer_max_quality` to the
|
||||
appropriate shared state (or as closure captures in the signal task).
|
||||
5. Replace the 4 TODO comments with the handlers described above.
|
||||
6. Add `set_bitrate` to `OpusEncoder` if missing.
|
||||
7. Update the audio send task to read `active_quality` / `peer_max_quality`
|
||||
each iteration.
|
||||
8. Add unit tests.
|
||||
9. Run `cargo check --manifest-path desktop/src-tauri/Cargo.toml`.
|
||||
|
||||
## Files to read before implementing
|
||||
|
||||
- `desktop/src-tauri/src/lib.rs` — grep for `UpgradeProposal` to find the
|
||||
exact lines; also read the surrounding signal task for variable names.
|
||||
- `crates/wzp-proto/src/packet.rs` lines 1130–1190 — `UpgradeProposal`,
|
||||
`UpgradeResponse`, `UpgradeConfirm`, `QualityCapability` struct layouts.
|
||||
- `desktop/src-tauri/src/engine.rs` — `CallEngine` struct fields, audio
|
||||
send task loop.
|
||||
- `crates/wzp-proto/src/codec_id.rs` — `QualityProfile` methods.
|
||||
- `crates/wzp-codec/src/opus_enc.rs` — `OpusEncoder` API.
|
||||
|
||||
## Verify
|
||||
|
||||
```bash
|
||||
cargo check --manifest-path desktop/src-tauri/Cargo.toml
|
||||
cargo test -p wzp-desktop 2>/dev/null || cargo test --manifest-path desktop/src-tauri/Cargo.toml
|
||||
```
|
||||
|
||||
Expected: 0 errors; unit tests pass.
|
||||
|
||||
## Done when
|
||||
|
||||
- All 4 TODO comments replaced with real logic.
|
||||
- `cargo check --manifest-path desktop/src-tauri/Cargo.toml` exits 0.
|
||||
- Unit test verifies: `UpgradeProposal` → `UpgradeResponse { accepted: true }` sent;
|
||||
`UpgradeResponse { accepted: true }` → `UpgradeConfirm` sent + `active_quality` updated.
|
||||
242
docs/PRD/PRD-wire-format-hardening.md
Normal file
242
docs/PRD/PRD-wire-format-hardening.md
Normal file
@@ -0,0 +1,242 @@
|
||||
# PRD: Wire Format Hardening — FEC block_id u16, SignalMessage version byte, FEC repair index wrap
|
||||
|
||||
> **Status:** proposed
|
||||
> **Resolves:** Three small wire-format defects (H2, M1, M4) that compound over time into silent data corruption or protocol breakage.
|
||||
> **Depends on:** Nothing — purely mechanical changes to `wzp-fec` and `wzp-proto`.
|
||||
|
||||
## Problem
|
||||
|
||||
Three independent issues:
|
||||
|
||||
**H2 — `fec_block_id` u8 wraps too fast.** The `block_id` field in
|
||||
`RaptorQFecEncoder` (and `RaptorQFecDecoder`) is `u8`. At 5 audio frames
|
||||
per block and 50 fps this wraps every ~51 seconds. A slow receiver or a
|
||||
mid-session join can receive packets from two different blocks with the same
|
||||
`block_id`, silently corrupting FEC recovery.
|
||||
|
||||
**M1 — Some `SignalMessage` variants lack a `version` byte.** Most variants
|
||||
have `#[serde(default = "default_signal_version")] version: u8`. The unit
|
||||
variant `Reflect` (and potentially others added recently) does not. Future
|
||||
protocol changes that key on `version` will silently misparse old messages
|
||||
from peers without the field.
|
||||
|
||||
**M4 — FEC repair index can silently wrap at 255.** In
|
||||
`crates/wzp-fec/src/encoder.rs` line 140:
|
||||
|
||||
```rust
|
||||
let idx = (num_source as u16).wrapping_add(i as u16);
|
||||
```
|
||||
|
||||
(The line was already fixed to `u16` — verify it is `u16`, not `u8`. If it
|
||||
is still `u8`, the fix is below.)
|
||||
|
||||
If the line currently reads `(num_source as u8).wrapping_add(i as u8)`, then
|
||||
when `num_source + repair_count > 255` the repair symbol indices wrap silently,
|
||||
producing incorrect ESI values that the decoder cannot correlate to source
|
||||
blocks.
|
||||
|
||||
## Goals
|
||||
|
||||
- **H2**: Widen `block_id` in encoder and decoder from `u8` to `u16`.
|
||||
Update `finalize_block` return type and `current_block_id` return type in
|
||||
the trait (`wzp-proto`) and implementations (`wzp-fec`).
|
||||
- **M1**: Audit every `SignalMessage` variant; add
|
||||
`#[serde(default = "default_signal_version")] version: u8` to any that
|
||||
are missing it.
|
||||
- **M4**: Confirm the repair index uses `u16`; fix it if it is still `u8`.
|
||||
Update the decoder's `add_symbol` call site if the index type changes.
|
||||
- `cargo test -p wzp-fec -p wzp-proto` passes; no existing tests broken.
|
||||
|
||||
## Non-goals
|
||||
|
||||
- Changing the wire encoding of `MediaHeaderV2::fec_block` — it is already
|
||||
`u16` on the wire. This PRD only widens the **internal counter** to match.
|
||||
- Multi-block decode concurrency or block expiry policy.
|
||||
- Any crate outside `wzp-fec` and `wzp-proto`.
|
||||
|
||||
## Design
|
||||
|
||||
### Item A — `fec_block_id` u8 → u16
|
||||
|
||||
**Files**:
|
||||
- `crates/wzp-proto/src/traits.rs` — `FecEncoder` and `FecDecoder` traits
|
||||
- `crates/wzp-fec/src/encoder.rs` — `RaptorQFecEncoder`
|
||||
- `crates/wzp-fec/src/decoder.rs` — `RaptorQFecDecoder`
|
||||
|
||||
**Trait changes** (`traits.rs`):
|
||||
|
||||
```rust
|
||||
// Before:
|
||||
fn finalize_block(&mut self) -> Result<u8, FecError>;
|
||||
fn current_block_id(&self) -> u8;
|
||||
fn add_symbol(&mut self, block_id: u8, ...) -> Result<(), FecError>;
|
||||
fn try_decode(&mut self, block_id: u8) -> Result<...>;
|
||||
fn expire_before(&mut self, block_id: u8);
|
||||
```
|
||||
|
||||
```rust
|
||||
// After:
|
||||
fn finalize_block(&mut self) -> Result<u16, FecError>;
|
||||
fn current_block_id(&self) -> u16;
|
||||
fn add_symbol(&mut self, block_id: u16, ...) -> Result<(), FecError>;
|
||||
fn try_decode(&mut self, block_id: u16) -> Result<...>;
|
||||
fn expire_before(&mut self, block_id: u16);
|
||||
```
|
||||
|
||||
**Encoder changes** (`encoder.rs`):
|
||||
|
||||
- Change `block_id: u8` field to `block_id: u16`.
|
||||
- Update `self.block_id.wrapping_add(1)` (already u16 semantics; keep as is).
|
||||
- Update `finalize_block` to return `u16`.
|
||||
- Update `current_block_id` to return `u16`.
|
||||
- Update all tests that assert `block_id == 0u8` → `== 0u16`, and the
|
||||
wrap test (`block_id_wraps`) to iterate to `u16::MAX` (65535) — or reduce
|
||||
it to 300 iterations to keep it fast, asserting the wrap at 65536.
|
||||
|
||||
The wrap test at 256 iterations (`0..=255u8`) must be updated; a full
|
||||
`u16` wrap test at 65536 iterations is too slow for CI. Change to:
|
||||
|
||||
```rust
|
||||
#[test]
|
||||
fn block_id_wraps_u16() {
|
||||
let mut enc = RaptorQFecEncoder::with_defaults(1);
|
||||
// Advance 300 blocks and verify no panic + monotonic increment.
|
||||
for expected in 0..300u16 {
|
||||
assert_eq!(enc.current_block_id(), expected);
|
||||
enc.add_source_symbol(&[0u8; 10]).unwrap();
|
||||
enc.finalize_block().unwrap();
|
||||
}
|
||||
// Explicitly test wrap at u16 boundary.
|
||||
let mut enc2 = RaptorQFecEncoder::with_defaults(1);
|
||||
enc2.block_id = u16::MAX;
|
||||
enc2.add_source_symbol(&[0u8; 10]).unwrap();
|
||||
let id = enc2.finalize_block().unwrap();
|
||||
assert_eq!(id, u16::MAX);
|
||||
assert_eq!(enc2.current_block_id(), 0);
|
||||
}
|
||||
```
|
||||
|
||||
Note: `block_id` is a private field; expose a test helper or set it in a
|
||||
`#[cfg(test)]` `impl` block.
|
||||
|
||||
**Decoder changes** (`decoder.rs`):
|
||||
|
||||
- Change `blocks: HashMap<u8, BlockState>` to `HashMap<u16, BlockState>`.
|
||||
- Update `get_or_create_block(block_id: u8)` → `get_or_create_block(block_id: u16)`.
|
||||
- Update `add_symbol`, `try_decode`, `expire_before` signatures to `u16`.
|
||||
- The `SourceBlockEncoder::new(self.block_id, ...)` call in `encoder.rs` passes
|
||||
`block_id` to `raptorq`. RaptorQ uses `u8` for source block number internally.
|
||||
Cast it: `(block_id & 0xFF) as u8` or `(block_id % 256) as u8` — the `raptorq`
|
||||
crate's source block ID is a logical identifier within a single object
|
||||
transmission, not a global counter. The u16 is our session counter; truncate
|
||||
to u8 when calling into raptorq.
|
||||
|
||||
### Item B — `SignalMessage` version byte audit
|
||||
|
||||
**File**: `crates/wzp-proto/src/packet.rs`
|
||||
|
||||
Read every variant in the `SignalMessage` enum (lines 555–1241) and check
|
||||
for the presence of:
|
||||
|
||||
```rust
|
||||
#[serde(default = "default_signal_version")]
|
||||
version: u8,
|
||||
```
|
||||
|
||||
The `Reflect` variant at line 974 is a **unit variant** (no fields). Unit
|
||||
variants cannot carry a `version` field without becoming struct variants.
|
||||
Change it to a struct variant:
|
||||
|
||||
```rust
|
||||
// Before:
|
||||
Reflect,
|
||||
|
||||
// After:
|
||||
Reflect {
|
||||
#[serde(default = "default_signal_version")]
|
||||
version: u8,
|
||||
},
|
||||
```
|
||||
|
||||
This is a wire-compatible change: serde JSON struct variants serialize as
|
||||
`{"Reflect": {"version": 1}}` whereas unit variants serialize as
|
||||
`"Reflect"`. These are **not** backward-compatible formats. Since `Reflect`
|
||||
is sent client → relay only and the relay immediately responds, upgrading
|
||||
both sides atomically is acceptable. Add a serde test to confirm round-trip.
|
||||
|
||||
For any other variants missing `version`, follow the same pattern as all
|
||||
existing variants.
|
||||
|
||||
Verify by grepping the enum for variants that do NOT have `version`:
|
||||
|
||||
```bash
|
||||
grep -A3 "^\s*[A-Z][A-Za-z]*\s*{" crates/wzp-proto/src/packet.rs | \
|
||||
grep -B1 -v "serde.*default_signal_version\|version:"
|
||||
```
|
||||
|
||||
### Item C — FEC repair index wrap (M4)
|
||||
|
||||
**File**: `crates/wzp-fec/src/encoder.rs`, line ~140.
|
||||
|
||||
Current code:
|
||||
|
||||
```rust
|
||||
let idx = (num_source as u16).wrapping_add(i as u16);
|
||||
```
|
||||
|
||||
If this line already uses `u16` (as shown in the file at line 140), M4 is
|
||||
already fixed. Verify by reading the current file. If it still reads
|
||||
`u8`, apply:
|
||||
|
||||
```rust
|
||||
let idx = (num_source as u16).wrapping_add(i as u16);
|
||||
```
|
||||
|
||||
**Decoder** (`crates/wzp-fec/src/decoder.rs`): `add_symbol` already accepts
|
||||
`symbol_index: u16` (per the trait). Confirm the parameter flows through to
|
||||
`PayloadId::new(block_id_u8, symbol_index as u32)` without truncation.
|
||||
|
||||
## Implementation steps
|
||||
|
||||
1. Read `crates/wzp-proto/src/traits.rs` lines 60–116 (FecEncoder/FecDecoder
|
||||
trait definitions) to confirm current signatures.
|
||||
2. Read `crates/wzp-fec/src/encoder.rs` and `decoder.rs` (full files).
|
||||
3. Apply Item C fix first (smallest change, easiest to verify).
|
||||
4. Apply Item A: widen `block_id` from u8 to u16 in traits, encoder, decoder.
|
||||
Update all callers by running `cargo check -p wzp-fec -p wzp-proto` and
|
||||
fixing each E0308/E0308 error.
|
||||
5. Apply Item B: read every variant, add missing `version` fields.
|
||||
Change `Reflect` to a struct variant.
|
||||
6. Run tests.
|
||||
|
||||
## Files to read before implementing
|
||||
|
||||
- `crates/wzp-proto/src/traits.rs` lines 60–116 (trait signatures)
|
||||
- `crates/wzp-fec/src/encoder.rs` (full)
|
||||
- `crates/wzp-fec/src/decoder.rs` (full)
|
||||
- `crates/wzp-proto/src/packet.rs` lines 555–1241 (all `SignalMessage` variants)
|
||||
|
||||
## Verify
|
||||
|
||||
```bash
|
||||
cargo test -p wzp-fec -p wzp-proto
|
||||
```
|
||||
|
||||
Expected: all tests pass, 0 failures. Also run:
|
||||
|
||||
```bash
|
||||
cargo check --workspace
|
||||
```
|
||||
|
||||
to catch any call sites outside `wzp-fec` and `wzp-proto` that passed `u8`
|
||||
block IDs to the trait methods.
|
||||
|
||||
## Done when
|
||||
|
||||
- `cargo test -p wzp-fec -p wzp-proto` exits 0.
|
||||
- `block_id` is `u16` in `RaptorQFecEncoder`, `RaptorQFecDecoder`, and the
|
||||
`FecEncoder`/`FecDecoder` traits.
|
||||
- Every non-unit `SignalMessage` variant has a `version: u8` field with
|
||||
`#[serde(default = "default_signal_version")]`.
|
||||
- Repair index in `encoder.rs` is computed with `u16` arithmetic.
|
||||
- No existing tests are broken.
|
||||
Reference in New Issue
Block a user