34e1480de5beede6c3074895855ac89c01e9bb83
1 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 claude-noether
|
9eae068f11 |
iter2 Phase 5: sonnet review — 3 critical UAPI errors caught, 7 amendments
Phase 5 review run via Plan subagent with model: sonnet per
feedback_dev_process.md Phase 5 discipline. 13 findings: 3 Critical
+ 4 Should-fix + 3 Question + 3 Nit. Reviewer's bottom-line: medium
confidence (vs iter1's medium-high) — lower because the plan had
3 concrete-and-wrong claims about kernel UAPI struct fields that
would have caused compile errors or silent semantic bugs in Phase 6.
Per memory feedback_review_empirical_over_theoretical.md: every
Critical and Should-fix finding was VERIFIED against fresnel's
kernel UAPI before responding. No source-read rebuttals attempted.
Critical resolutions:
C1 (data_byte_offset, not data_bit_offset):
Plan Clause 4 said new API "still requires bit_size + data_bit_
offset, this logic is preserved." Empirical: struct has
data_byte_offset (u32 byte count). FFmpeg uses straight byte
offset, no bit search. Plan amendment: drop bit-search at
h265.c:196-209; replace with byte-offset assignment.
ACCEPTED.
C2 (dpb.rps GONE, pic_order_cnt_val rename, poc_st_curr_*
arrays hold DPB indices):
Plan Clause 6 said "DPB extraction migrates verbatim." Empirical:
- dpb_entry has flags (only LONG_TERM_REFERENCE bit), no .rps
- pic_order_cnt_val (singular s32) replaces pic_order_cnt[0]
- poc_st_curr_before[16]/_after[16]/_lt_curr[16] are u8 DPB
INDICES, not POC values; populate via FFmpeg
get_ref_pic_index() pattern (search dpb[] by timestamp,
return index)
Plan amendment: replace "verbatim migration" claim with explicit
re-spec: classify VAAPI ReferenceFrames into ST_CURR_BEFORE/
AFTER/LT_CURR lists, assign DPB indices, populate arrays with
indices.
ACCEPTED.
C3 (union-aliasing reasoning wrong, claim still right):
Same anti-pattern as iter1 review C1. Plan said reset is benign
because RenderPicture per-buffer copies overwrite byte 17764.
Empirical: byte 17764 lands in num_slices region; non-HEVC
profiles never read that location. Reset is benign because
non-aliasing, NOT because of overwriting. Wording amended.
ACCEPTED.
Should-fix resolutions:
S1 (PPS flags 19+20 missing): empirical confirms
V4L2_HEVC_PPS_FLAG_DEBLOCKING_FILTER_CONTROL_PRESENT (1ULL<<19)
V4L2_HEVC_PPS_FLAG_UNIFORM_SPACING (1ULL<<20)
Plan amended to add both. ACCEPTED.
S2 (3 PPS scalars missing): empirical PPS struct dump confirms
pic_parameter_set_id, num_ref_idx_l0_default_active_minus1,
num_ref_idx_l1_default_active_minus1 all present in modern
struct. Plan amended to populate. ACCEPTED.
S3 (SCALING_MATRIX content divergence FFmpeg vs libva):
FFmpeg sends memset-zero when no scaling list in stream
(BBB has no scaling_list — SPS flags=SAO|STRONG_INTRA only).
Plan said "populate spec defaults when iqmatrix_set==false."
Phase 6 implementer choice; document in commit which path
taken. Phase 7 byte-compare validates. ACCEPTED as choice
rather than mandate.
S4 (FFmpeg function name wrong cite):
Plan cited ff_v4l2_request_query_control_default_value;
actual is ff_v4l2_request_query_control. Cosmetic fix.
ACCEPTED.
Question resolutions:
Q1 (object_heap allocator size handling): VERIFIED safe.
request.c:142-143 uses sizeof(struct object_surface). Adding
slices[64] auto-picks-up the larger size.
Q2 (slice_segment_addr field): VERIFIED present in struct.
Plan amended Clause 4: populate from VAAPI
slice->slice_segment_address. Single-slice BBB safe with
implicit zero; multi-slice would corrupt without this field.
Q3 (VAPictureParameterBufferType per-frame send for HEVC):
Deferred to Phase 7 LIBVA_TRACE capture. iter1+T4 patterns
suggest yes, worth grepping at verification time.
Nits N1+N2+N3: array size [16] not [8]; image-output
directory naming cosmetic; BeginPicture cleanup deferred.
Plan amendments consolidated:
1. Clause 4: data_byte_offset; drop bit-search; add
slice_segment_addr population (C1 + Q2)
2. Clause 6: explicit DPB classification + index-array logic;
pic_order_cnt_val rename; drop dpb.rps (C2)
3. Clause 3: 2 PPS flags + 3 scalars (S1, S2)
4. Clause 5: function name fix (S4); SCALING_MATRIX divergence
deferred to Phase 6 implementer (S3)
5. Clause 10: union-aliasing reasoning corrected (C3)
6. Clause 6: V4L2_HEVC_DPB_ENTRIES_NUM_MAX=16 macro reference (N1)
7. Phase 7 harness: rename png_* → image_* dirs (N2)
Plan re-locks with these amendments. Phase 6 proceeds.
Per global ~/.claude/CLAUDE.md rule: Phase 5 reviews never
skippable. iter2's review was the right path forward — caught
3 concrete UAPI errors (data_bit_offset → data_byte_offset rename;
dpb.rps field gone; pic_order_cnt struct shape) that would have
been Phase 6 compile failures or silent Phase 7 byte-compare
divergences requiring loopback. Outside-look value substantial.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|