Skip to content

Commit b39a576

Browse files
committed
fix(codec): address PR-195 review — overflow Merge alias + mode-sized pack_leaf
P1 (codex) — overflow δ no longer aliases to Merge: predict_intra previously took `our_delta_u8 = δ as u8` BEFORE checking i8 fit, so e.g. δ=200 wrapped to 0xC8 and could match a neighbour byte 0xC8 (i8=-56), silently corrupting reconstruction. Now the i8 range check gates both the Merge scan and the Delta branch; out-of-range δ falls straight through to Escape (or the documented lossy clamp). + 2 regression tests: - overflow_delta_does_not_alias_to_merge - overflow_delta_with_allocator_takes_escape P2 (coderabbit + codex) — pack_leaf accepts mode-sized buffers: pack_leaf used a 6-byte minimum for all modes; callers pre-sizing by packed_byte_len() got spurious None for Skip(2)/Merge,Delta(3). Length check now gates on packed_byte_len(leaf.mode). + 1 regression test: pack_leaf_accepts_mode_sized_buffers P3 (coderabbit) — doctest examples on the public-API surface: Added /// runnable examples to MAX_BASIN_IDX, BASIN_NONE, unpack_header, pack_merge_dir, unpack_merge_dir, unpack_leaf, packed_byte_len, IntraContext, IntraConfig, is_no_basin. Removed unused LeafCu import from existing predict_intra doctest. Gates: cargo test --features codec --lib hpc::codec → 48 passed cargo test --features codec --doc hpc::codec → 14 passed cargo fmt --all -- --check → clean cargo clippy --features codec --lib -- -D warnings → clean
1 parent 26d987f commit b39a576

2 files changed

Lines changed: 148 additions & 17 deletions

File tree

src/hpc/codec/mode.rs

Lines changed: 51 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -57,10 +57,20 @@ use super::ctu::{CellMode, LeafCu, MergeDir};
5757

5858
/// Maximum encodable `basin_idx`. Stored in the lower 12 bits of the
5959
/// header; values >= this constant overflow the header field.
60+
///
61+
/// ```
62+
/// use ndarray::hpc::codec::MAX_BASIN_IDX;
63+
/// assert_eq!(MAX_BASIN_IDX, (1 << 12) - 1);
64+
/// ```
6065
pub const MAX_BASIN_IDX: u16 = (1 << 12) - 1; // 4095
6166

6267
/// Tag inside the per-frame basin codebook for "no basin assigned"
6368
/// (encoder-side sentinel during mode decision).
69+
///
70+
/// ```
71+
/// use ndarray::hpc::codec::{BASIN_NONE, MAX_BASIN_IDX};
72+
/// assert_eq!(BASIN_NONE, MAX_BASIN_IDX);
73+
/// ```
6474
pub const BASIN_NONE: u16 = MAX_BASIN_IDX;
6575

6676
/// Pack `(mode, basin_idx)` into a 16-bit header.
@@ -85,6 +95,12 @@ pub fn pack_header(mode: CellMode, basin_idx: u16) -> u16 {
8595
///
8696
/// The 2-bit mode field always decodes (all 4 variants are valid).
8797
/// `basin_idx` is the 12-bit lower field, exactly as packed.
98+
///
99+
/// ```
100+
/// use ndarray::hpc::codec::{pack_header, unpack_header, CellMode};
101+
/// let h = pack_header(CellMode::Escape, 7);
102+
/// assert_eq!(unpack_header(h), (CellMode::Escape, 7));
103+
/// ```
88104
#[inline]
89105
pub fn unpack_header(packed: u16) -> (CellMode, u16) {
90106
let mode_bits = ((packed >> 12) & 0b11) as u8;
@@ -103,6 +119,11 @@ pub fn unpack_header(packed: u16) -> (CellMode, u16) {
103119
// ════════════════════════════════════════════════════════════════════
104120

105121
/// Pack a [`MergeDir`] into the lower 2 bits of a `u8`.
122+
///
123+
/// ```
124+
/// use ndarray::hpc::codec::{pack_merge_dir, MergeDir};
125+
/// assert_eq!(pack_merge_dir(MergeDir::East), 1);
126+
/// ```
106127
#[inline]
107128
pub fn pack_merge_dir(dir: MergeDir) -> u8 {
108129
dir as u8
@@ -112,6 +133,13 @@ pub fn pack_merge_dir(dir: MergeDir) -> u8 {
112133
///
113134
/// All four 2-bit values map to a valid `MergeDir`; bits 2-7 are
114135
/// ignored.
136+
///
137+
/// ```
138+
/// use ndarray::hpc::codec::{pack_merge_dir, unpack_merge_dir, MergeDir};
139+
/// for d in [MergeDir::North, MergeDir::East, MergeDir::West, MergeDir::South] {
140+
/// assert_eq!(unpack_merge_dir(pack_merge_dir(d)), d);
141+
/// }
142+
/// ```
115143
#[inline]
116144
pub fn unpack_merge_dir(byte: u8) -> MergeDir {
117145
match byte & 0b11 {
@@ -133,7 +161,9 @@ pub fn unpack_merge_dir(byte: u8) -> MergeDir {
133161
/// worst case) — callers iterating CTUs typically pre-allocate
134162
/// `6 * cell_count` and trim afterwards.
135163
///
136-
/// Returns `None` if `out.len() < 6` (insufficient capacity).
164+
/// Returns `None` if `out.len() < packed_byte_len(leaf.mode)` (insufficient
165+
/// capacity for the *mode's* width — Skip needs 2, Merge/Delta need 3,
166+
/// Escape needs 6).
137167
///
138168
/// Format:
139169
/// - Bytes 0-1: header (`pack_header(mode, basin_idx)`, LE)
@@ -151,7 +181,8 @@ pub fn unpack_merge_dir(byte: u8) -> MergeDir {
151181
/// assert_eq!(consumed, 3);
152182
/// ```
153183
pub fn pack_leaf(leaf: &LeafCu, out: &mut [u8]) -> Option<usize> {
154-
if out.len() < 6 {
184+
let required = packed_byte_len(leaf.mode);
185+
if out.len() < required {
155186
return None;
156187
}
157188
let header = pack_header(leaf.mode, leaf.basin_idx);
@@ -184,6 +215,16 @@ pub fn pack_leaf(leaf: &LeafCu, out: &mut [u8]) -> Option<usize> {
184215
///
185216
/// Returns `None` if the buffer is shorter than the per-mode width
186217
/// (2 for Skip, 3 for Merge/Delta, 6 for Escape).
218+
///
219+
/// ```
220+
/// use ndarray::hpc::codec::{pack_leaf, unpack_leaf, LeafCu, MergeDir};
221+
/// let leaf = LeafCu::merge(7, MergeDir::West);
222+
/// let mut buf = [0u8; 3];
223+
/// pack_leaf(&leaf, &mut buf).unwrap();
224+
/// let (decoded, n) = unpack_leaf(&buf).unwrap();
225+
/// assert_eq!(decoded, leaf);
226+
/// assert_eq!(n, 3);
227+
/// ```
187228
pub fn unpack_leaf(buf: &[u8]) -> Option<(LeafCu, usize)> {
188229
if buf.len() < 2 {
189230
return None;
@@ -217,6 +258,14 @@ pub fn unpack_leaf(buf: &[u8]) -> Option<(LeafCu, usize)> {
217258

218259
/// Byte cost of packing a leaf in this mode. Useful for pre-sizing
219260
/// a buffer without packing first.
261+
///
262+
/// ```
263+
/// use ndarray::hpc::codec::{packed_byte_len, CellMode};
264+
/// assert_eq!(packed_byte_len(CellMode::Skip), 2);
265+
/// assert_eq!(packed_byte_len(CellMode::Merge), 3);
266+
/// assert_eq!(packed_byte_len(CellMode::Delta), 3);
267+
/// assert_eq!(packed_byte_len(CellMode::Escape), 6);
268+
/// ```
220269
#[inline]
221270
pub const fn packed_byte_len(mode: CellMode) -> usize {
222271
match mode {

src/hpc/codec/predict.rs

Lines changed: 97 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,17 @@ use super::mode::BASIN_NONE;
8787
/// - `neighbours`: NESW (in [`MergeDir`] discriminant order) optional
8888
/// neighbour leaves. `None` for boundary cells; the Merge candidate
8989
/// scan skips `None` entries.
90+
///
91+
/// ```
92+
/// use ndarray::hpc::codec::{IntraContext, LeafCu};
93+
/// let north = LeafCu::delta(5, 17);
94+
/// let ctx = IntraContext {
95+
/// basin_idx: 5,
96+
/// delta_i32: 17,
97+
/// neighbours: [Some(&north), None, None, None],
98+
/// };
99+
/// assert_eq!(ctx.basin_idx, 5);
100+
/// ```
90101
#[derive(Debug, Clone, Copy)]
91102
pub struct IntraContext<'a> {
92103
/// Pre-resolved basin index (12-bit max).
@@ -102,6 +113,14 @@ pub struct IntraContext<'a> {
102113
///
103114
/// Today a single field; the field exists so the API can grow
104115
/// (Merge tolerance, RDO knobs in A6) without a signature break.
116+
///
117+
/// ```
118+
/// use ndarray::hpc::codec::IntraConfig;
119+
/// let default_cfg = IntraConfig::default();
120+
/// assert!(default_cfg.escape_next_idx.is_none());
121+
/// let allocated = IntraConfig { escape_next_idx: Some(42) };
122+
/// assert_eq!(allocated.escape_next_idx, Some(42));
123+
/// ```
105124
#[derive(Debug, Clone, Copy)]
106125
pub struct IntraConfig {
107126
/// Future allocator for the encoder's escape vector — returns the
@@ -139,7 +158,7 @@ impl Default for IntraConfig {
139158
///
140159
/// ```
141160
/// use ndarray::hpc::codec::predict::{predict_intra, IntraContext, IntraConfig};
142-
/// use ndarray::hpc::codec::{CellMode, LeafCu};
161+
/// use ndarray::hpc::codec::CellMode;
143162
/// let ctx = IntraContext {
144163
/// basin_idx: 42,
145164
/// delta_i32: 0,
@@ -170,6 +189,14 @@ pub fn predict_intra(ctx: &IntraContext, cfg: &IntraConfig) -> LeafCu {
170189
return LeafCu::skip(ctx.basin_idx);
171190
}
172191

192+
// i8-fit gates both Merge and Delta. Out-of-range δ must skip
193+
// Merge entirely — wrapping `200_i32 as u8` aliases to `0xC8`,
194+
// which could spuriously match a neighbour whose byte equals
195+
// `0xC8` (i8 = -56), producing a leaf the decoder reconstructs as
196+
// -56 instead of 200.
197+
let fits_i8 = (-128..=127).contains(&ctx.delta_i32);
198+
let our_delta_u8 = ctx.delta_i32 as u8; // wrapping cast matches A2 pack
199+
173200
// ── 2. Merge ─────────────────────────────────────────────────────
174201
//
175202
// A neighbour is a Merge candidate iff:
@@ -186,20 +213,21 @@ pub fn predict_intra(ctx: &IntraContext, cfg: &IntraConfig) -> LeafCu {
186213
// Multiple matches all collapse to the same coded leaf, so the
187214
// first-hit policy is order-deterministic without affecting
188215
// bitstream length.
189-
let our_delta_u8 = ctx.delta_i32 as u8; // wrapping cast matches A2 pack
190-
for (i, nb_slot) in ctx.neighbours.iter().enumerate() {
191-
let Some(nb) = nb_slot else { continue };
192-
if nb.mode != CellMode::Delta {
193-
continue;
216+
if fits_i8 {
217+
for (i, nb_slot) in ctx.neighbours.iter().enumerate() {
218+
let Some(nb) = nb_slot else { continue };
219+
if nb.mode != CellMode::Delta {
220+
continue;
221+
}
222+
if nb.basin_idx != ctx.basin_idx {
223+
continue;
224+
}
225+
if nb.delta != Some(our_delta_u8) {
226+
continue;
227+
}
228+
let dir = merge_dir_from_index(i);
229+
return LeafCu::merge(ctx.basin_idx, dir);
194230
}
195-
if nb.basin_idx != ctx.basin_idx {
196-
continue;
197-
}
198-
if nb.delta != Some(our_delta_u8) {
199-
continue;
200-
}
201-
let dir = merge_dir_from_index(i);
202-
return LeafCu::merge(ctx.basin_idx, dir);
203231
}
204232

205233
// ── 3. Delta ─────────────────────────────────────────────────────
@@ -208,7 +236,7 @@ pub fn predict_intra(ctx: &IntraContext, cfg: &IntraConfig) -> LeafCu {
208236
// the encoder's reconstruction must read the byte back as i8 to
209237
// recover the sign. This matches how `LeafCu::delta` stores it and
210238
// how `super::mode::pack_leaf` writes it.
211-
if (-128..=127).contains(&ctx.delta_i32) {
239+
if fits_i8 {
212240
return LeafCu::delta(ctx.basin_idx, our_delta_u8);
213241
}
214242

@@ -244,6 +272,12 @@ fn merge_dir_from_index(i: usize) -> MergeDir {
244272
/// is the "no basin" marker. Encoders that compute basins lazily can
245273
/// short-circuit Skip/Merge/Delta and emit Escape directly when this
246274
/// fires.
275+
///
276+
/// ```
277+
/// use ndarray::hpc::codec::{is_no_basin, BASIN_NONE};
278+
/// assert!(is_no_basin(BASIN_NONE));
279+
/// assert!(!is_no_basin(0));
280+
/// ```
247281
#[inline]
248282
pub fn is_no_basin(basin_idx: u16) -> bool {
249283
basin_idx == BASIN_NONE
@@ -394,4 +428,52 @@ mod tests {
394428
assert!(!is_no_basin(0));
395429
assert!(!is_no_basin(100));
396430
}
431+
432+
#[test]
433+
fn overflow_delta_does_not_alias_to_merge() {
434+
// Regression for the wrapping-cast Merge alias bug:
435+
// δ = 200 (overflows i8) must NOT match a neighbour whose
436+
// u8 byte equals (200 as u8) = 0xC8 (= -56 in i8). The
437+
// encoder must take the Escape path (or, here, the lossy
438+
// clamp fallback because no allocator is wired).
439+
let nb_alias = LeafCu::delta(100, 0xC8);
440+
let neighbours = [Some(&nb_alias), None, None, None];
441+
let leaf = predict_intra(&ctx_with_neighbours(100, 200, neighbours), &IntraConfig::default());
442+
assert_ne!(leaf.mode, CellMode::Merge, "overflow δ must not Merge");
443+
// With no allocator the encoder clamps to +127 (lossy Delta).
444+
assert_eq!(leaf.mode, CellMode::Delta);
445+
assert_eq!(leaf.delta, Some(127));
446+
}
447+
448+
#[test]
449+
fn overflow_delta_with_allocator_takes_escape() {
450+
let nb_alias = LeafCu::delta(100, 0xC8);
451+
let neighbours = [Some(&nb_alias), None, None, None];
452+
let cfg = IntraConfig {
453+
escape_next_idx: Some(7),
454+
};
455+
let leaf = predict_intra(&ctx_with_neighbours(100, 200, neighbours), &cfg);
456+
assert_eq!(leaf.mode, CellMode::Escape);
457+
assert_eq!(leaf.escape_idx, Some(7));
458+
}
459+
460+
#[test]
461+
fn pack_leaf_accepts_mode_sized_buffers() {
462+
// Regression for the P2 6-byte-minimum bug: Skip should pack
463+
// into a 2-byte buffer, Merge/Delta into a 3-byte buffer.
464+
use super::super::mode::{pack_leaf, packed_byte_len};
465+
let skip = LeafCu::skip(10);
466+
let mut buf2 = [0u8; 2];
467+
assert_eq!(pack_leaf(&skip, &mut buf2), Some(2));
468+
assert_eq!(packed_byte_len(CellMode::Skip), 2);
469+
470+
let delta = LeafCu::delta(10, 7);
471+
let mut buf3 = [0u8; 3];
472+
assert_eq!(pack_leaf(&delta, &mut buf3), Some(3));
473+
474+
// Escape still needs 6 bytes; a 3-byte buffer is rejected.
475+
let esc = LeafCu::escape(10, 99);
476+
let mut buf3b = [0u8; 3];
477+
assert_eq!(pack_leaf(&esc, &mut buf3b), None);
478+
}
397479
}

0 commit comments

Comments
 (0)