fresnel-fourier/phase5_iter3_review.md

Iteration 3 — Phase 5 (sonnet-architect review)

Second-model review of phase4_iter3_plan.md (commit 2918dda) by sonnet-architect on 2026-05-08. Reviewer ran cold-context source reads of fork files + reference sources + kernel hantro driver + VAAPI headers + verbatim Phase 3 baseline. All Critical findings empirically verified by author before incorporation per memory feedback_review_empirical_over_theoretical.md Direction 2.

Verdict

BLOCK — 4 Critical findings, all blocking iter3 close until amended:

• C1, C2: latent decode-correctness failures (wrong hardware DMA addresses) → criterion-4 byte-compare divergence
• C3, C4: Phase 6 build failures (undefined identifier + non-existent typedef in userspace)

All 4 amendments accepted into the amended Phase 4 plan.

Critical findings

C1 — first_part_header_bits = 0 is a correctness bug, NOT a cosmetic gap

Plan claim (Clause 9): "kernel hantro_vp8.c re-parses the uncompressed header and may not depend on this field." Marked as a known fidelity gap, leaving it 0.

Empirical evidence (verified by author):

references/linux-mainline/drivers/media/platform/verisilicon/hantro_g1_vp8_dec.c:260:
        hdr->first_part_header_bits + 8;

references/linux-mainline/drivers/media/platform/verisilicon/rockchip_vpu2_hw_vp8_dec.c:372:
        hdr->first_part_header_bits + 8;

Both kernel VP8 decoders (G1 on RK3399 + VPU2 on RK3568) read hdr->first_part_header_bits unconditionally to compute the macroblock-data DMA offset:

mb_offset_bits = first_part_offset * 8 + hdr->first_part_header_bits + 8;
mb_offset_bytes = mb_offset_bits / 8;
mb_size = hdr->first_part_size - (mb_offset_bytes - first_part_offset) + ...;

Setting this to 0 places the hardware at the wrong DMA offset for ALL macroblock data. The decode produces garbage or hangs.

Fix (verified by source identity):

frame.first_part_header_bits = slice->macroblock_offset;

Proof (author verified via grep):

references/ffmpeg-kwiboo/libavcodec/vaapi_vp8.c:204:
    .macroblock_offset = (8 * (s->coder_state_at_header_end.input - data) -
                          s->coder_state_at_header_end.bit_count - 8),

references/ffmpeg-kwiboo/libavcodec/v4l2_request_vp8.c:83:
    .first_part_header_bits = (8 * (s->coder_state_at_header_end.input - data) -
                               s->coder_state_at_header_end.bit_count - 8),

Identical formulas, identical inputs. slice->macroblock_offset IS first_part_header_bits by source identity.

Anchor: VAAPI va_dec_vp8.h:186 confirms macroblock_offset field exists in VASliceParameterBufferVP8. Phase 3 keyframe verbatim: first_part_header_bits = 6550.

C2 — frame.first_part_size = slice->partition_size[0] is short by ceil(macroblock_offset/8) bytes

Plan claim (Clause 9): frame.first_part_size = slice->partition_size[0].

Empirical evidence:

references/ffmpeg-kwiboo/libavcodec/vaapi_vp8.c:209:
    sp.partition_size[0] = s->header_partition_size - ((sp.macroblock_offset + 7) / 8);

VAAPI's partition_size[0] is the remaining bytes of the control partition AFTER the header-bits portion (ceil(macroblock_offset / 8) bytes consumed up front). Confirmed by VAAPI spec comment at va_dec_vp8.h:193-196: "remaining bytes of control partition after parsed by application."

But kernel needs the TOTAL control partition size:

hantro_g1_vp8_dec.c:265:  mb_size = hdr->first_part_size - (mb_offset_bytes - first_part_offset) + ...;
hantro_g1_vp8_dec.c:293:  dct_part_offset = first_part_offset + hdr->first_part_size;

first_part_size here is the TOTAL control partition size (= FFmpeg's s->header_partition_size).

Numerical verification (Phase 3 frame-1 keyframe verbatim):

• Kernel first_part_size = 22742 bytes
• macroblock_offset = first_part_header_bits = 6550 bits → ceil(6550/8) = 819 bytes
• VAAPI's partition_size[0] = 22742 - 819 = 21923 bytes
• Plan-as-written would set first_part_size = 21923 → DCT partitions begin 819 bytes too early → garbage output

Fix:

frame.first_part_size = slice->partition_size[0] +
                        ((uint32_t)slice->macroblock_offset + 7) / 8;

C3 — VAProbabilityDataBufferType does not exist; will fail Phase 6 compile

Plan refers to (Phase 2 B7, Phase 4 Commit C): case VAProbabilityDataBufferType: in codec_store_buffer outer switch.

Empirical evidence (verified by grep):

reference/libva/va/va.h:2058:           VAProbabilityBufferType             = 13,
reference/libva/va/va_dec_vp8.h:218:    typedef struct _VAProbabilityDataBufferVP8 {

The plan confused the buffer-type enum constant (VAProbabilityBufferType) with the buffer-data struct name (VAProbabilityDataBufferVP8). Switch case must use the enum constant.

Fix: replace VAProbabilityDataBufferType → VAProbabilityBufferType throughout (Phase 2 B7 + Phase 4 Commit C site list).

Cross-check: vaapi_vp8.c:147 confirms FFmpeg's VAAPI encoder uses VAProbabilityBufferType correctly.

C4 — (s8) cast undefined in userspace

Plan code (Clauses 6, 7):

frame.quant.y_dc_delta = (s8)(iqmatrix->quantization_index[0][1] -
                              iqmatrix->quantization_index[0][0]);
frame.segment.quant_update[i] = (s8)(...);

Empirical evidence (verified by grep):

/usr/include/asm-generic/int-ll64.h:20:  typedef __signed__ char __s8;

The kernel UAPI exposes __s8 (with leading double-underscore prefix). Bare s8 is a kernel-internal typedef from linux/types.h — NOT exposed to userspace. Compile fails: undefined identifier.

Fix: replace (s8) with (int8_t) from <stdint.h>. Or drop the explicit cast entirely — implicit conversion to __s8 is well-defined for values in [-128, 127] which the BBB content stays well within.

Suggested improvements

S1 — Quantization derivation correctness for non-BBB content (author's Q1)

The plan's iqmatrix[0][N] - iqmatrix[0][0] subtraction is correct for streams where no clip saturation occurs. Verified via vaapi_vp8.c:157-162: VAAPI fills quantization_index[0][N] = clip(base_quant[0] + yac_qi + delta_N).

Edge case: when yac_qi + delta_N saturates at 0 or 127 in VAAPI, the subtraction can't recover the original signed delta. BBB does not exercise this path. Document as known fidelity gap; not blocking iter3 close.

S2 — segment.quant_update[s] semantics (author's Q5)

VAAPI: iqmatrix[s][0] = clip(base_quant[s] + yac_qi). Plan: quant_update[s] = iqmatrix[s][0] - iqmatrix[0][0] = base_quant[s] - base_quant[0] (when no clip).

When base_quant[0] = 0 (typical), this matches FFmpeg V4L2's quant_update[i] = s->segmentation.base_quant[i]. When base_quant[0] != 0, plan's formula produces base_quant[s] - base_quant[0] not base_quant[s]. BBB has segmentation disabled; flag as known fidelity gap for non-standard streams.

S3 — Probability buffer fallback comment is factually wrong

Plan Clause 8 comment: "if not, kernel hantro driver re-derives from default tables."

This is incorrect. hantro_vp8.c::hantro_vp8_prob_update (lines 49-143) reads entropy->coeff_probs unconditionally — there is no default-table fallback in the driver. If probability_set==false and coeff_probs is zero, decode produces garbage.

Practical risk is low (vaapi_vp8.c:146-148 shows FFmpeg VAAPI always sends VAProbabilityBufferType per frame, so mpv-vaapi via FFmpeg should always set probability_set), but the plan comment should be corrected and an assert(probability_set) added (or a logged warning) so any miss surfaces immediately during Phase 6 runtime.

Recommended amendment: change the comment, add the runtime assertion.

S4 — lf.level source for segmentation-enabled streams

Plan: frame.lf.level = picture->loop_filter_level[0].

vaapi_vp8.c:87-95: VAAPI fills pp.loop_filter_level[i] = filter_level[i] + filter.level (effective level for segment i). For segmentation-disabled BBB, loop_filter_level[0] = filter.level (the base) — plan is correct. For segmentation-enabled streams in DELTA mode, kernel computes clamp(lf.level + lf_update[i]); using effective level for segment 0 as the base produces wrong results for segments 1-3. BBB out-of-scope; document fidelity gap.

S5 — segment.lf_update[0] is implicitly correct via self-subtraction

Plan Clause 7 fills lf_update[i] for i = 0..3 as loop_filter_level[i] - loop_filter_level[0]. For i=0: yields 0 regardless. Kernel computes clamp(lf.level + lf_update[0]) = lf.level. Correct. No issue.

Questions

Q1 — Does mpv-vaapi consumer always send VAProbabilityBufferType?

vaapi_vp8.c:146-148 shows FFmpeg's VAAPI encoder always sends it. mpv's libavcodec VP8 VAAPI path uses ff_vp8_vaapi_hwaccel which is the same code, so yes — probability_set should always be true.

Recommended: add assert(probability_set); debug guard so any miss surfaces in Phase 6 (this is the runtime check from S3).

Q2 — first_part_size recovery formula for non-byte-aligned macroblock_offset

((macroblock_offset + 7) / 8) rounds up. Phase 3 keyframe: macroblock_offset = 6550 bits → ceil(6550/8) = 819 bytes. Reconstructed first_part_size = 21923 + 819 = 22742 ✓. Author should verify this also holds for inter frames in Phase 7 (Phase 3 captured inter first_part_size = 1218 etc.; Phase 7 byte-compare will validate empirically).

Q3 — Which kernel VP8 driver runs on RK3399 (G1 vs VPU2)?

Both G1 (hantro_g1_vp8_dec.c) and VPU2 (rockchip_vpu2_hw_vp8_dec.c) use first_part_header_bits identically. RK3399 uses G1 (per hantro_drv.c machine-data table). C1 + C2 apply regardless. No action.

Empirical verification log

Claim	Verification	Result
first_part_header_bits used by hantro G1	grep -rn first_part_header_bits references/.../verisilicon/	Read at hantro_g1_vp8_dec.c:260 + rockchip_vpu2_hw_vp8_dec.c:372
macroblock_offset == first_part_header_bits formula identity	Source compare vaapi_vp8.c:204-205 vs v4l2_request_vp8.c:83-84	Identical formulas
partition_size[0] != first_part_size	vaapi_vp8.c:209 derivation + Phase 3 numerics	Differ by ceil(macroblock_offset/8) = 819 bytes
VAProbabilityDataBufferType non-existent	grep VAProbability* va.h va_dec_vp8.h	Only VAProbabilityBufferType=13 exists; struct is VAProbabilityDataBufferVP8
s8 userspace typedef	grep typedef.*s8 /usr/include/	Only __s8 (double-underscore) defined; bare s8 kernel-only
VAAPI quantization_index is absolute	vaapi_vp8.c:152-163	Confirmed clip(base_quant[s] + yac_qi + delta_c)
coeff_probs direct memcpy safe	Both paths apply coeff_bands_inverse reordering	vaapi_vp8.c:133-143 and v4l2_request_vp8.c:141-153 use identical reordering — direct memcpy correct
num_dct_parts off-by-one direction	vaapi_vp8.c:206: num_of_partitions = num_coeff_partitions + 1; v4l2_request_vp8.c:80: num_dct_parts = num_coeff_partitions	Plan's num_of_partitions - 1 confirmed correct
VAAPI mb_mode_delta offset baked in	vaapi_vp8.c:100: [i + 4]; v4l2_request_vp8.c:109: [i + MODE_I4x4] (= 4)	Same offset; plan's direct copy correct
KEY_FRAME inversion direction	vaapi_vp8.c:58: key_frame = !s->keyframe	Plan's !pic_fields.bits.key_frame correct
hantro_vp8_prob_update no fallback	Read hantro_vp8.c:49-143	Confirmed — comment in plan Clause 8 was wrong
All VAAPI field names valid	Line-by-line grep against va_dec_vp8.h:71-241	All present

Author's 7 review questions — direct answers

Q	Answer
Q1 quantization derivation	Correct for typical content (no clip saturation, base_quant[0]=0). See S1+S2 for edge cases.
Q2 first_part_header_bits=0 safety	UNSAFE → C1
Q3 num_dct_parts off-by-one	num_of_partitions - 1 correct
Q4 field availability	All present; (s8) fails compile (C4); VAProbabilityDataBufferType fails compile (C3)
Q5 quant_update[s] semantics	Signed delta; plan's subtraction correct when base_quant[0]=0
Q6 SHOW_FRAME unconditional	Safe for BBB scope; documented fidelity gap for invisible-frame content
Q7 buffer order independence	Confirmed — each codec_store_buffer writes a distinct field

Required Phase 4 plan amendments

ID	Location	Change
C1	Clause 9, plan-comment + code	frame.first_part_header_bits = slice->macroblock_offset;
C2	Clause 9, code	frame.first_part_size = slice->partition_size[0] + ((uint32_t)slice->macroblock_offset + 7) / 8;
C3	Phase 2 B7 + Phase 4 Commit C	case VAProbabilityBufferType: (drop "Data")
C4	Clauses 6+7	(s8) → (int8_t) (or drop cast)
S3	Clause 8 comment + add runtime check	Correct comment; add assert(probability_set) or warn-log

C1 + C2 affect Clause 9 jointly; rewrite as a single block. C3 affects 5 places (Phase 2 B7 + Phase 4 Commit C site list + B7 patch shape). C4 affects ~6 places (Clauses 6+7). S3 affects 2 places.

Process discipline note

This review was conducted with empirical verification of every Critical claim BEFORE inclusion (memory feedback_review_empirical_over_theoretical.md Direction 2). Every C1-C4 finding was verified by grep / source-read of multiple files, and the proposed amendments were cross-checked against working code (FFmpeg VAAPI ref, FFmpeg V4L2 ref). Author independently verified each before incorporating into the amended plan.

The review caught what would have been a 2-loopback Phase 6 failure cycle (compile failures C3+C4) plus a Phase 7 byte-compare divergence (C1+C2 hardware-DMA-offset bug). Without this review pass, iter3 Phase 6 would have failed first compile attempt, then after fix-forward would have produced visible output but with wrong DMA addresses → Phase 7 → Phase 4 loopback.