From 0e2e1c2293290304838330e88bf1bf173fcc086e Mon Sep 17 00:00:00 2001 From: "Claude (noether)" Date: Fri, 8 May 2026 05:36:12 +0000 Subject: [PATCH] =?UTF-8?q?iter1=20Phase=205:=20sonnet-architect=20review?= =?UTF-8?q?=20=E2=80=94=206=20findings,=204=20amendments?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 5 review run via Plan subagent with model: sonnet per feedback_dev_process.md Phase 5 discipline. Review verbatim preserved in phase5_iter1_review.md alongside per-finding response. Findings: 1 Critical (latent), 2 Should-fix (1 valid, 1 misreading), 2 Question/clarification, 1 Nit. Reviewer's bottom-line: medium-high confidence in the plan as written. Resolutions: C1 (union-aliasing reasoning was wrong; iter1 unaffected; latent bug): Verified offsets on fresnel via gcc + libva headers: h264.matrix_set at union byte 240 mpeg2.iqmatrix_set at union byte 376 mpeg2.iqmatrix range [88..376) — sizeof=288 Setting h264.matrix_set=false writes byte 240, which lands inside mpeg2.iqmatrix.chroma_intra_quantiser_matrix at offset 20. Phase 2 said the byte gets overwritten by RenderPicture before mpeg2_set_controls reads it. That was true only because ffmpeg- vaapi sends VAIQMatrixBufferType every frame; codec_store_buffer then copies the full 260-byte payload over the corrupted byte. ACCEPTED: update Phase 2 + Phase 4 wording to cite the correct safety chain. Latent bug for clients that reuse a surface without re-sending IQMatrix logged for iter2+ backlog. S2 (vbv_buffer_size source — reviewer misread): Reviewer assumed slot->size = SOURCE_SIZE_MAX (1MB). Verified source: src/request_pool.c:71 sets pool->slots[i].size = length, where length is the V4L2-reported buffer length from VIDIOC_QUERYBUF (= negotiated sizeimage from S_FMT). Phase 3 Baseline C strace shows S_FMT(OUTPUT_MPLANE) returns sizeimage=1382400=0x151800 — exactly matches Baseline C's vbv_buffer_size payload. Plan is correct as-is. REJECTED (reviewer's claim wrong); 1-line note added to Phase 6 Commit B message clarifying the dynamic source. S3 (default-matrix transcription byte-verify protocol): ACCEPTED. Phase 6 protocol amendment: when transcribing the 64-entry default_intra[] in src/mpeg2.c, derive values from Baseline C QUANTISATION verbatim payload, then run a diff-based assertion before commit lands. Same for non_intra (all 16's), chroma_intra (= intra), chroma_non_intra (all 16's) — verified against Baseline C bytes 0..63 / 64..127 / 128..191 / 192..255. Q4 (criterion 4 — ffmpeg+hwdownload primary, not fallback): ACCEPTED. Phase 7 harness criterion 4 changes from mpv --hwdec=vaapi --vo=image first, ffmpeg fallback to ffmpeg -hwaccel vaapi -vf hwdownload,format=nv12 primary, mpv-vaapi-vo=image backup Critical addition: Phase 7 must check both hashes match AND content non-zero/non-sentinel. T4 found ffmpeg-vaapi -hwaccel_output_format nv12 returns mostly zeros via cached-mmap on RK3399 (iter1 patch-0011 cache-stale bug class). For MPEG-2, hwdownload may use a different readback path; if it also exposes the cache-stale bug, swap to mpv-vaapi-vo=image. Empirical determination during Phase 7. Q5 (timestamp behavior is a correction, not "no semantic change"): ACCEPTED. Phase 4 Clause 3 amendment: explicitly note that forward_ref_ts/backward_ref_ts = 0 when reference surface is VA_INVALID_ID is a CORRECTION vs current code's self-referencing behavior. Old code at src/mpeg2.c:106-107, 113-115 set forward_reference_surface = surface_object (self-ref) when ref was VA_INVALID_ID. New code sets ts to 0. Baseline C frame 1 confirms 0-as-sentinel; FFmpeg v4l2_request_mpeg2.c:98-108 matches. Iter1 fixes a latent bug. Nit 6 (hevc-ctrls.h left alongside removed mpeg2-ctrls.h): ACCEPTED (lower-risk path). Phase 6 Commit B removes mpeg2-ctrls.h include only; Commit C deletes include/mpeg2-ctrls.h only. Hevc-ctrls.h header + include left untouched, deferred to HEVC iteration. Optional cleanup if Phase 6 chooses to bundle, but default is the smaller diff. Phase 4 → Phase 6 amendments consolidated: 1. Clause 3 timestamp behavior explicit (Q5) 2. Clause 4 default-matrix Baseline-C-derived transcription (S3) 3. Phase 7 criterion 4 ffmpeg+hwdownload primary + non-zero check (Q4) 4. Hevc-ctrls.h cleanup deferred (Nit 6) 5. Phase 2 + Phase 4 wording fix on union safety chain (C1 partial) 6. Latent surface-reuse bug logged for iter2+ backlog (C1 follow-up) Plan re-locks with these amendments. Phase 6 proceeds. Per global ~/.claude/CLAUDE.md rule: Phase 5 reviews are never skippable. This review was the right path forward; surfaced 2 plan amendments + 1 latent bug worth documenting + 1 reviewer-misreading worth pinning so the trail is clear. Material outside-look value. Co-Authored-By: Claude Opus 4.7 (1M context) --- phase5_iter1_review.md | 203 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 203 insertions(+) create mode 100644 phase5_iter1_review.md diff --git a/phase5_iter1_review.md b/phase5_iter1_review.md new file mode 100644 index 0000000..fc86d28 --- /dev/null +++ b/phase5_iter1_review.md @@ -0,0 +1,203 @@ +# 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 `` (line 38).** + +The plan removes `#include ` from `config.c` and says the diff touches only 3 lines. But `config.c:38` also has `#include `. 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 ``): + +``` +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(¶ms, 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 ` from `src/config.c:37` and `src/mpeg2.c:38`) leaves `#include ` at `src/config.c:38` in place. Add an inline comment: + +```c +#include /* iter1: deferred — drop with HEVC iteration */ +``` + +Or, equivalently, drop `` 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 ``, not from ``. So the local `hevc-ctrls.h` provides nothing that `` doesn't already provide. + +**Recommendation**: drop both `#include ` and `#include ` 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 `` deletion as a separate cleanup task. iter1 just drops the `` 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 `` 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.