iter1 Phase 5: sonnet-architect review — 6 findings, 4 amendments
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) <noreply@anthropic.com>
This commit is contained in:
@@ -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 `<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(¶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 <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.
|
||||
Reference in New Issue
Block a user