afb9b1450fa9e6c48002827dbd2d3788ede08c0f
1 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 claude-noether
|
0e2e1c2293 |
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>
|