fresnel-fourier/phase5_iter1_review.md

Iteration 1 — Phase 5 (second-model review)

Sonnet-architect review of the iter1 plan, run 2026-05-08 via the Plan subagent with model: sonnet per feedback_dev_process.md Phase 5 discipline. Six findings: 1 Critical (latent), 2 Should-fix (1 valid, 1 based on misreading), 2 Question/clarification, 1 Nit. Reviewer's bottom-line: medium-high confidence in the plan as written.

Per ~/.claude/CLAUDE.md global rule: "An empty review (no findings) is itself a strong signal: high confidence that the plan is sound, not a wasted check. The value of a review is the outside look, regardless of what it finds. Skipping reviews to save time defeats the purpose of having them." This review surfaced 2 amendments worth landing in the plan + 1 latent bug worth following up post-iter1 — material outside-look value.

The full review verbatim is preserved below; my response per finding follows.

Review (verbatim from sonnet-architect Plan agent)

---

Critical

1. VAIQMatrixBufferMPEG2 is always sent by ffmpeg-vaapi; iqmatrix_set is never false in practice, but is also never reset — hiding a surface-reuse latency bug.

Reading references/ffmpeg-kwiboo/libavcodec/vaapi_mpeg2.c:92-107, the VAAPI sender (ffmpeg-vaapi hwaccel) sets all four load_* flags to 1 and unconditionally sends a VAIQMatrixBufferType buffer every frame. This means iqmatrix_set will be true on every frame that the BBB fixture decodes, and the else branch (spec-default matrix synthesis) will never execute for this sender.

That sounds benign for iter1's test fixture — except the plan's Phase 2 "verified not a bug" reasoning about iqmatrix_set has a flaw. The params.h264.matrix_set = false reset at picture.c:287 writes to byte offset 224 of the union (sizeof VAIQMatrixBufferH264 = 224 bytes). params.mpeg2.iqmatrix_set lives at byte offset sizeof(VAPictureParameterBufferMPEG2) + sizeof(VASliceParameterBufferMPEG2) + sizeof(VAIQMatrixBufferMPEG2) ≈ byte 328. These are different bytes — the reset does NOT touch mpeg2.iqmatrix_set. Once a surface's iqmatrix_set is set true on first use, it is never cleared. If the client reuses a surface for a subsequent decode cycle without sending an IQ matrix (legal VAAPI behavior), the backend will use stale matrices from the prior cycle.

This is not a problem for the BBB 10s fixture because ffmpeg-vaapi always sends the matrix. It will not cause a Phase 7 failure. But the plan's stated rationale — "RenderPicture overwrites that byte before mpeg2_set_controls reads anything" — is factually wrong about which byte is being overwritten, and the plan's "Files 6+ — verify only / no changes" conclusion for picture.c may be understating a real latency. Flag this for the Phase 4 author: the picture.c:287 reset should be changed to reset mpeg2.iqmatrix_set conditionally (or use a profile-aware memset of the whole params union), but it will not break iter1 because the sender's behavior masks it.

Assessment: not blocking for iter1 with the BBB fixture + ffmpeg-vaapi sender. Blocking if the claim "picture.c needs no changes" is treated as a clean bill of health for correctness in general.

---

Should-fix

2. sequence.vbv_buffer_size will be SOURCE_SIZE_MAX (1 048 576), not the OUTPUT buffer size at decode time.

Plan Clause 2 maps vbv_buffer_size to surface_object->source_size, citing FFmpeg's controls->pic.output->size. In the FFmpeg path, output->size is the actual V4L2 OUTPUT buffer size, which equals SOURCE_SIZE_MAX (1 MB). In the libva backend, surface_object->source_size is set at picture.c:276 to slot->size, which is the V4L2 buffer's allocated size — also SOURCE_SIZE_MAX. So both FFmpeg and the libva backend use the same value (1 048 576). The plan's comment "FFmpeg uses controls->pic.output->size" is accurate and the equivalence holds.

However, the plan says the old code used SOURCE_SIZE_MAX as a "placeholder" and the new code will use surface_object->source_size instead. These are the same constant value, so the output bytes are identical. The Baseline C strace confirms vbv_buffer_size = 0x00151800 = 1 376 256 which is NOT SOURCE_SIZE_MAX = 1 048 576. This discrepancy is unexplained. Either the fixture's hantro driver has a different buffer size negotiation, or the value in Baseline C comes from a different source than what the plan assumes. The plan will produce 0x00100000 (1 MB) for this field where Baseline C shows 0x00151800 (1.31 MB). Per plan Clause 2's note: "vbv_buffer_size is informational; kernel doesn't reject." This is likely correct — hantro ignores it — but the Phase 7 extra cross-validator byte-comparison check will see a mismatch on this field specifically. The plan should note this expected divergence explicitly so Phase 7 doesn't misread it as a regression.

3. The default intra quantisation matrix in the plan is truncated and explicitly deferred.

Plan Clause 4 / File 2 shows only 57 of 64 entries with /* zigzag, ... */ trailing off, with a note: "remaining 7 from the spec to be transcribed verbatim during Phase 6." The full MPEG-2 default intra matrix from ISO 13818-2 Table 7-3 in zigzag order ends: ..., 83, 97, 112, 121. The plan truncates after 83. This is not a critical path risk (Phase 6 will transcribe them), but if Phase 6 makes a transcription error here the quantisation matrix will be wrong and pixel verify will fail. The review recommends pre-verifying the 7 missing values: 97, 112, 121 fill positions 57-63 (the last 7 entries are the bottom-right corner of the 8×8 matrix traversed in zigzag — all high-frequency, typically {83, 97, 112, 121} in the standard but the table has 64 distinct values). Concretely: cross-check all 64 against Baseline C's QUANTISATION payload before transcription, not after.

---

Question / clarification

4. Criterion 4's mpv-vaapi fallback assumes mpv --hwdec=vaapi (DMA-BUF path) behaves differently from --hwdec=vaapi-copy.

The plan acknowledges (Phase 7 note on criterion 4): "this assumes mpv-vaapi DMA-BUF WILL engage for MPEG-2." But the Phase 3 finding was that mpv's hwdec policy filters MPEG-2 out entirely — that finding was observed with --hwdec=vaapi-copy, but the filtering happens at a layer above the libva driver (mpv's hwdec_codecs list or its ffmpeg hwaccel selection). Whether --hwdec=vaapi (without -copy) follows the same code path or a different one is not verified. The fallback to ffmpeg -hwaccel vaapi -vf hwdownload is sound and should be the primary path for criterion 4, not the contingent fallback. The question: has anyone verified that mpv --hwdec=vaapi (not vaapi-copy) actually reaches the libva decode path for MPEG-2, or is this speculation?

If criterion 4 silently falls back to the ffmpeg path, that's fine — but the Phase 7 harness as written runs the mpv DMA-BUF path first and would need a human judgment call on whether to activate the fallback. Making the ffmpeg path primary removes that ambiguity.

5. The reference timestamp population for B-frames differs from the current code in a subtle way.

Current mpeg2.c:104-118: when forward_reference_picture == VA_INVALID_ID, the code sets forward_reference_surface = surface_object (self-reference) and uses that timestamp. Plan Clause 3 sets forward_ref_ts = 0 when the surface is NULL (i.e., when SURFACE(driver_data, VA_INVALID_ID) returns NULL). These produce different values: the current code sends the surface's own timestamp; the plan sends 0. Baseline C frame 1 shows forward_ref_ts = 0 for an I-frame, which matches the plan's behavior. FFmpeg's v4l2_request_mpeg2.c:98-108 also uses 0 (by only setting the field for P/B). So the plan is correct and the current behavior (self-timestamp) was wrong. But the plan should explicitly call out this behavioral change — it does not — because a reader might expect "same VAAPI field reads" to mean "same output values" and be confused when the timestamps change. The "no semantic change" claim in Clause 3 is therefore incorrect for this field.

---

Nit

6. include/mpeg2-ctrls.h deletion: config.c also includes <hevc-ctrls.h> (line 38).

The plan removes #include <mpeg2-ctrls.h> from config.c and says the diff touches only 3 lines. But config.c:38 also has #include <hevc-ctrls.h>. If the MPEG-2 break is added above the HEVC/default fall-through, the hevc-ctrls.h include remains. The plan correctly identifies git grep "mpeg2-ctrls.h" as the audit step but doesn't mention hevc-ctrls.h. The HEVC header is out of iter1 scope, but leaving it alongside the removed mpeg2-ctrls.h will look inconsistent in review. Worth a comment in the diff or an explicit "intentionally left, deferred to HEVC iteration."

---

Confidence: medium-high. The three core bugs are correctly identified, the fix contract is accurately grounded in the kernel headers and FFmpeg reference, and the Phase 7 harness is specific enough to catch regressions. The plan would produce working code for the BBB fixture. The risk that prevents "high" is the unexplained vbv_buffer_size mismatch between Baseline C and what the plan will generate — if Phase 7's byte-comparison check interprets that as a failure, there will be a loop back to Phase 4 for something that is almost certainly innocuous. The iqmatrix_set never-reset issue is latent but harmless for iter1's sender.

---

Author response per finding

C1 — Phase 2 union-aliasing reasoning was wrong; iter1 unaffected; latent bug for follow-up

Verdict: ✅ accepted (partial). Reviewer is correct that Phase 2's "RenderPicture overwrites that byte before mpeg2_set_controls reads anything" was wrong about which byte. The actual safety reasoning is different: ffmpeg-vaapi sends VAIQMatrixBufferType every frame, so codec_store_buffer:140-146 writes the entire VAIQMatrixBufferMPEG2 payload into params.mpeg2.iqmatrix and sets iqmatrix_set = true freshly on every RenderPicture cycle.

Verified offsets (compiled on fresnel against <va/va.h>):

h264.matrix_set offset    = 240   (sizeof(VAIQMatrixBufferH264) = 240)
mpeg2.iqmatrix_set offset = 376
mpeg2.picture  range = [0   .. 40)   size=40
mpeg2.slice    range = [40  .. 88)   size=48
mpeg2.iqmatrix range = [88  .. 376)  size=288

Setting params.h264.matrix_set = false writes byte 240 of the union, which is inside mpeg2.iqmatrix (range 88..376), specifically at offset 152 within VAIQMatrixBufferMPEG2. Layout inside VAIQMatrixBufferMPEG2: 4 load_* flags (offsets 0..3), then four 64-byte matrices (offsets 4..67, 68..131, 132..195, 196..259). Byte 152 lands inside chroma_intra_quantiser_matrix at byte 20.

So the actual corruption is: byte 20 of chroma_intra_quantiser_matrix is zeroed in BeginPicture, then overwritten by the ffmpeg-vaapi-sent IQMatrix in RenderPicture (which copies all 260 bytes), then read by mpeg2_set_controls in EndPicture. Net safe for iter1's ffmpeg-vaapi sender.

Latent bug confirmed: a VAAPI client that calls vaBeginPicture → vaRenderPicture(VASliceData) → vaEndPicture without sending a new VAIQMatrixBufferType (legal VAAPI for surfaces with prior IQMatrix state) would see byte 20 of chroma_intra_quantiser_matrix reset to 0 — corrupting the matrix on a subsequent decode of the same surface. Combined with iqmatrix_set never being cleared, the backend would submit a partially-corrupted matrix.

Action for iter1: no code change (out of scope per Phase 1 lock; ffmpeg-vaapi sender masks it). Update Phase 2 + Phase 4 wording to:

The params.h264.matrix_set = false reset at src/picture.c:287 writes byte 240 of the union, which lands inside mpeg2.iqmatrix.chroma_intra_quantiser_matrix (specifically byte 20). For iter1's ffmpeg-vaapi-driven path, RenderPicture's codec_store_buffer:140-146 overwrites this corrupted byte by copying the full VAIQMatrixBufferMPEG2 payload before mpeg2_set_controls reads it. Latent bug for clients that reuse a surface without re-sending the IQMatrix; logged as a follow-up cleanup task post-iter1 (separate iteration scope, e.g. iter2 or after; involves making the BeginPicture reset profile-aware or replacing it with memset(&params, 0, sizeof params)).

Follow-up task created: post-iter1 cleanup — make params reset profile-aware in BeginPicture to address the latent surface-reuse corruption. Goes on the iter2+ backlog when locked.

S2 — vbv_buffer_size source: reviewer was working from a wrong assumption

Verdict: ❌ reviewer's concern based on misreading. Plan is correct as-is, no amendment needed. But documenting the reasoning so the next reader doesn't repeat the trip.

Reviewer's claim: slot->size = SOURCE_SIZE_MAX = 1 MB; Baseline C's vbv_buffer_size = 0x151800 = 1.31 MB; therefore the plan will produce 1 MB and Phase 7 byte-compare will see a mismatch.

Actual chain, traced through the source:

• src/request_pool.c:48-72: request_pool_init calls v4l2_create_buffers for the OUTPUT pool, then for each slot calls v4l2_query_buffer(video_fd, output_type, index, &length, &offset, 1) and stores pool->slots[i].size = length (line 71).
• length is the V4L2-reported buffer length from VIDIOC_QUERYBUF, which is the negotiated sizeimage from VIDIOC_S_FMT.
• Phase 3 Baseline C strace shows S_FMT(OUTPUT_MPLANE) for the MPEG-2 fixture returning sizeimage=1382400 = 0x151800.
• Therefore slot->size = 1382400, so surface_object->source_size = 1382400, so the plan's sequence.vbv_buffer_size = surface_object->source_size = 1382400 = 0x151800.
• This matches Baseline C's verbatim payload exactly. No discrepancy.

The reviewer assumed slot->size = SOURCE_SIZE_MAX (which is a constant defined elsewhere in the codebase, used for buffer allocation upper bounds, not as the actual slot size). That assumption was wrong; in reality slot->size is dynamically set from the kernel's negotiated sizeimage.

Action: no change to Phase 4 plan. Add a 1-line note in Phase 6's Commit B message explaining the dynamic source so future readers don't repeat the misreading.

S3 — Default-matrix transcription must be byte-verified against Baseline C

Verdict: ✅ accepted. Reviewer's concrete recommendation is sound: "cross-check all 64 against Baseline C's QUANTISATION payload before transcription, not after."

Phase 6 protocol amendment: when transcribing the 64-entry default_intra array in src/mpeg2.c, the value list must come from Baseline C's QUANTISATION verbatim payload (which empirically shows the BBB fixture using spec defaults). After transcription, run diff <(printf '%d\n' ...64 values...) <(extract first 64 bytes from Baseline C QUANTISATION as decimal) — must produce empty diff. If the diff is non-empty, the transcription is wrong.

Same protocol for the other 3 matrices: non_intra must be all 16's; chroma_intra must equal intra (per VAAPI/MPEG-2 4:2:0 spec); chroma_non_intra must be all 16's. Verify against Baseline C bytes 64..127 (non_intra), 128..191 (chroma_intra), 192..255 (chroma_non_intra).

The reviewer's claimed last 4 values {83, 97, 112, 121} are not authoritative (they didn't have the spec table in hand); Phase 6 transcription will derive from Baseline C bytes directly.

Action: amend Phase 6 protocol — bytes-from-Baseline-C transcription, not spec-from-memory.

Q4 — Make ffmpeg+hwdownload primary for criterion 4, not the contingent fallback

Verdict: ✅ accepted. Reviewer's reasoning is valid: making the ffmpeg path primary removes the speculation step and the human judgment call about whether to activate the fallback. Phase 7 harness should anchor on what we know works.

Plan amendment: Phase 7 criterion 4 incantation changes from:

# OLD (Phase 4 plan): mpv --hwdec=vaapi --vo=image first, then ffmpeg fallback
mpv --hwdec=vaapi --vo=image ... bbb_720p10s_mpeg2.ts

to:

# NEW (Phase 5 amendment): ffmpeg-vaapi + hwdownload primary
ffmpeg -hide_banner -loglevel error \
  -hwaccel vaapi -hwaccel_output_format nv12 \
  -i ~/fourier-test/bbb_720p10s_mpeg2.ts \
  -ss 2 -frames:v 2 -vf hwdownload,format=nv12 \
  -f rawvideo -y /tmp/iter1_phase7/hw.nv12

ffmpeg -hide_banner -loglevel error \
  -i ~/fourier-test/bbb_720p10s_mpeg2.ts \
  -ss 2 -frames:v 2 -pix_fmt nv12 \
  -f rawvideo -y /tmp/iter1_phase7/sw.nv12

sha256sum /tmp/iter1_phase7/hw.nv12 /tmp/iter1_phase7/sw.nv12

Pass criterion: hw.nv12 and sw.nv12 hash-equal AND non-zero/non-sentinel content (run a quick non-zero count). The non-zero check is critical — recall T4 found that ffmpeg-vaapi -hwaccel_output_format nv12 returns mostly zeros via the cached-mmap path on RK3399 (the iter1-patch-0011 cache-stale bug class). For MPEG-2, hopefully the same path is OK because the read happens after the kernel's request completes, and hwdownload may use a different readback than direct nv12 export. Phase 7 must explicitly check both hashes match AND content is plausible — if the hashes match but bytes are mostly zero, the cache-stale bug is fooling us into a false PASS.

Backup criterion-4 path (if ffmpeg+hwdownload exposes the cache-stale bug): use mpv --hwdec=vaapi --vo=image directly. Phase 3 Baseline D proved this path works for H.264; if mpv-vaapi-MPEG-2 reaches the libva path, it'll work for MPEG-2 too. Empirical determination during Phase 7 — run both incantations, take whichever is cache-coherency-safe.

Q5 — Reference timestamp behavior is a correction; document explicitly

Verdict: ✅ accepted. Reviewer is right that the plan called this "no semantic change" when in fact the plan FIXES a bug in the current mpeg2.c:104-118 code (self-reference for missing forward/backward refs).

Plan amendment to Clause 3:

Behavioral change vs. current code: when picture->forward_reference_picture == VA_INVALID_ID (or backward_reference_picture == VA_INVALID_ID for B-frames), the new code sets the corresponding forward_ref_ts / backward_ref_ts to 0. The old code at src/mpeg2.c:106-107, 113-115 self-referenced the current surface's timestamp (forward_reference_surface = surface_object), which was wrong: the kernel uses forward_ref_ts == 0 as the sentinel for "no forward reference" (Phase 3 Baseline C frame 1 confirms — I-frame has forward_ref_ts = backward_ref_ts = 0). FFmpeg's libavcodec/v4l2_request_mpeg2.c:98-108 uses the same 0-as-sentinel convention. Iter1 corrects this to match the kernel + FFmpeg contract.

This was a latent bug in the old code that nobody hit because nobody tested MPEG-2 end-to-end on this fork. Net for iter1: corrected behavior; no functional regression vs. "what the old code claimed to do."

Nit 6 — hevc-ctrls.h left in place; comment in the diff

Verdict: ✅ accepted. Trivial.

Plan amendment: Phase 6 Commit B (which removes #include <mpeg2-ctrls.h> from src/config.c:37 and src/mpeg2.c:38) leaves #include <hevc-ctrls.h> at src/config.c:38 in place. Add an inline comment:

#include <hevc-ctrls.h>  /* iter1: deferred — drop with HEVC iteration */

Or, equivalently, drop <hevc-ctrls.h> simultaneously since it's also a staging-era local header (the same architectural smell). Decide in Phase 6 based on whether RequestQueryConfigProfiles still needs V4L2_PIX_FMT_HEVC_SLICE from somewhere: per Phase 2 source-read, RequestQueryConfigProfiles references V4L2_PIX_FMT_HEVC_SLICE (line 146, 149) — which is a kernel UAPI macro from <linux/videodev2.h>, not from <hevc-ctrls.h>. So the local hevc-ctrls.h provides nothing that <linux/videodev2.h> doesn't already provide.

Recommendation: drop both #include <mpeg2-ctrls.h> and #include <hevc-ctrls.h> in Commit B, and delete both include/mpeg2-ctrls.h AND include/hevc-ctrls.h in Commit C. Verify the build still passes; if HEVC iteration later needs anything from those headers, kernel UAPI provides it. (This expands iter1's deletion scope by one file but reduces the architectural smell; the tradeoff is small and clean.)

Alternative: keep <hevc-ctrls.h> deletion as a separate cleanup task. iter1 just drops the <mpeg2-ctrls.h> include from MPEG-2 callsites and deletes include/mpeg2-ctrls.h. The HEVC header stays untouched. Lower risk; smaller diff.

Phase 6 implementor's choice — I'll default to the lower-risk path (drop only mpeg2-ctrls.h for iter1) unless Phase 6 surfaces a reason to bundle.

Phase 4 → Phase 6 plan amendments (consolidated)

After Phase 5, the iter1 plan stands as authored in phase4_iter1_plan.md, with the following amendments:

1. Clause 3 — note the timestamp behavior change explicitly (Q5 accepted).
2. Clause 4 — Phase 6 transcription of default_intra[64] etc. derives from Baseline C bytes verbatim, with a diff-based assertion before the commit lands (S3 accepted).
3. Phase 7 criterion 4 — anchor on ffmpeg -hwaccel vaapi ... -vf hwdownload,format=nv12 ... | sha256sum as primary, with non-zero content check (Q4 accepted). mpv-vaapi-vo=image becomes a backup path.
4. Files 6+ — keep <hevc-ctrls.h> deletion out of iter1 by default (Nit 6 lower-risk path).
5. Phase 2 + Phase 4 wording — fix the union-aliasing reasoning to cite the actual safety chain (RenderPicture overwrites the corrupted byte via codec_store_buffer copy, not the hand-wavy "before mpeg2_set_controls reads it" claim) (C1 partial accept).
6. Post-iter1 follow-up backlog — log the latent surface-reuse bug for an iter2+ cleanup iteration (C1 follow-up).

Plan re-locks with these amendments. Phase 6 proceeds.

Phase 5 close

Per feedback_dev_process.md Phase 5: "Markus reviews and redacts, then initiates the handover to a fresh model instance." The handover-to-reviewer step happened (sonnet-architect Plan agent); Markus is the final arbiter on the amendments and may redact or extend before Phase 6.

If Markus accepts the amendments above, Phase 6 (implementation) can start. If any amendment is rejected, Phase 5 → Phase 4 loopback to re-lock plan first.