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`](../../.claude/projects/-home-mfritsche-src/memory/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:

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

to:

```bash
# 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:

```c
#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`](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.