marfrit
7a1bd8ec0a
Reviewer empirically read rkvdec-hevc.c on boltzmann kernel-agent tree. sps_max_num_reorder_pics is NOT read by rkvdec. α-13 would match kdirect's wire bytes but produce no behavioural change. CRIT-2: num_entry_point_offsets (libva hardcoded 0 at h265.c:356, kdirect 22 from slice header parse) + PPS UNIFORM_SPACING flag are the live candidates. BBB HEVC uses WPP (ENTROPY_CODING_SYNC flag set in PPS) not tiles; 22 entry points = 23 CTB rows for 720p with 32-pixel CTBs. Decision: land α-13 as wire-correctness hygiene (matches kdirect, no regression risk), then α-14 for the actual fix. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
96 lines
4.5 KiB
Markdown
96 lines
4.5 KiB
Markdown
# Iteration 11 — Phase 5 Review
|
||
|
||
Reviewed 2026-05-14. Empirical anchor: Phase 3 strace, rkvdec-hevc.c on boltzmann
|
||
(`build/marfrit-packages/arch/linux-fresnel-fourier/src/linux-7.0`), h265.c in
|
||
`libva-v4l2-request-fourier/src/`.
|
||
|
||
---
|
||
|
||
## CRIT-1 — rkvdec does not read `sps_max_num_reorder_pics` at all
|
||
|
||
**Invalidates the primary mechanism.**
|
||
|
||
Exhaustive grep of `rkvdec-hevc.c`: the field `sps_max_num_reorder_pics` is never
|
||
read. The SPS fields rkvdec actually maps to hardware registers are geometry,
|
||
bitdepth, CTB sizes, PCM config, short/long-term ref counts, temporal-MVP, and flags
|
||
— not the buffering/reorder hints. The field is present in `v4l2_ctrl_hevc_sps` for
|
||
spec completeness but rkvdec ignores it entirely.
|
||
|
||
**Consequence:** α-13 will change wire bytes 10 and produce no behavioural difference
|
||
in rkvdec. Phase 7 YUV hash will remain all-zero.
|
||
|
||
**Required Phase 6b amendment A:** α-13 must be deprioritised to IMP (harmless
|
||
wire-correctness improvement). Bug 5 investigation must shift to the two fields
|
||
rkvdec *does* act on that differ between libva and kdirect.
|
||
|
||
---
|
||
|
||
## CRIT-2 — `num_entry_point_offsets` is the live suspect and needs a Phase 4b plan
|
||
|
||
rkvdec-hevc.c at line 272-284 writes `num_tile_columns_minus1` and
|
||
`num_tile_rows_minus1` from the PPS to hardware. kdirect submits
|
||
`num_entry_point_offsets = 22`; libva submits 0. BBB HEVC is a tiled stream
|
||
(confirmed by 22 entry points from kdirect). Libva's hardcoded 0 tells the kernel
|
||
the bitstream has no tile boundaries, which is false. rkvdec writes tile geometry
|
||
into hardware from PPS fields — if those fields are also wrong, or if the driver
|
||
validates entry-point-offsets count against tile geometry internally, the zero
|
||
produces either a wrong decode or a silent error.
|
||
|
||
Additionally, the PPS `UNIFORM_SPACING` flag (bit 20) is missing from libva. For
|
||
tiled streams this flag controls whether rkvdec computes uniform column/row widths
|
||
or reads them from per-column/row arrays. BBB being tiled makes this non-don't-care.
|
||
|
||
**Required Phase 6b amendment B:** Add Phase 4b scope covering:
|
||
1. `num_entry_point_offsets` — derive from `slice->num_entry_point_offsets` if
|
||
VAAPI exposes it, else from tile geometry (num_columns × num_rows per slice).
|
||
2. PPS `UNIFORM_SPACING` — set flag when `pic_fields.bits.uniform_spacing_flag` is
|
||
set (VAAPI does expose this field in `VAPictureParameterBufferHEVC.pic_fields`).
|
||
|
||
---
|
||
|
||
## IMP-1 — `slice_qp_delta` byte 22 is NOT a hardcoded parallel bug
|
||
|
||
Phase 4 plan raises this as a concern. Empirical check: h265.c line 368 reads
|
||
`slice_params->slice_qp_delta = slice->slice_qp_delta` directly from the VAAPI
|
||
buffer — it is not hardcoded. The -1 value (0xff) observed in Phase 3 strace is
|
||
what VAAPI delivered for frame 1 of BBB. kdirect's 0x00 (0) comes from its own SPS
|
||
NAL parse. This divergence is real but secondary; slice_qp_delta miscalculation
|
||
shifts luma values, it does not suppress entire frame output. Not a parallel
|
||
hardcoded-zero bug.
|
||
|
||
---
|
||
|
||
## IMP-2 — Phase 7 hash check must not be skipped if wire bytes match kdirect's
|
||
|
||
Per `feedback_wire_vs_behavior.md` and the Phase 4 plan's own §Phase 7: matching
|
||
wire bytes alone is insufficient. Phase 7 must SHA the raw NV12 CAPTURE output
|
||
against kdirect's `9340b832…` hash, not just strace-diff the control payloads.
|
||
The plan states this correctly; reviewer flags it to ensure it survives any "close
|
||
enough" shortcut in execution.
|
||
|
||
---
|
||
|
||
## MIN-1 — Comment accuracy in α-13 patch
|
||
|
||
The proposed α-13 comment says "blocked B-frame decode." If CRIT-1 stands and
|
||
rkvdec ignores the field, this causal claim is wrong. The comment should be scoped
|
||
to "wire-level conformance fix; not confirmed to unblock rkvdec output." Amend
|
||
before commit to avoid misleading future readers.
|
||
|
||
---
|
||
|
||
## Summary
|
||
|
||
| # | Severity | Finding | Blocks Phase 6? |
|
||
|---|----------|---------|----------------|
|
||
| CRIT-1 | CRIT | rkvdec never reads `sps_max_num_reorder_pics`; α-13 is wire-only | Yes — replan required |
|
||
| CRIT-2 | CRIT | `num_entry_point_offsets=0` + missing UNIFORM_SPACING are the live suspects for tiled BBB | Yes — Phase 4b needed |
|
||
| IMP-1 | IMP | `slice_qp_delta` is not hardcoded; 0xff is VAAPI-supplied, not a parallel bug | No |
|
||
| IMP-2 | IMP | Phase 7 must hash YUV not just strace | No |
|
||
| MIN-1 | MIN | α-13 comment overclaims causation | No |
|
||
|
||
**Recommended Phase 6b scope:** Land α-13 as a wire-correctness commit with
|
||
corrected comment (MIN-1). Open Phase 4b for `num_entry_point_offsets` +
|
||
`UNIFORM_SPACING` as the primary Bug 5 fix path (CRIT-2). Do not gate Phase 6 on
|
||
α-13 unblocking Bug 5 — it won't.
|