fresnel-fourier/phase5_iter2_review.md

Iteration 2 — Phase 5 (second-model review)

Sonnet-architect review of the iter2 plan, run 2026-05-08 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 has more concrete-and-wrong claims about kernel UAPI struct field names (C1, C2 in particular).

Per memory/feedback_review_empirical_over_theoretical.md: every finding the reviewer cited concretely was verified empirically against fresnel's kernel UAPI before responding. Reviewer was right on every verifiable claim. Lesson held.

The full review verbatim is preserved in the agent transcript; my response per finding follows.

Critical findings — all 3 confirmed

C1 — data_byte_offset not data_bit_offset; drop the bit-search logic

Verdict: ✅ accepted, plan is wrong. Empirical verification (ssh fresnel 'awk "/^struct v4l2_ctrl_hevc_slice_params/,/^};/" /usr/include/linux/v4l2-controls.h'):

struct v4l2_ctrl_hevc_slice_params {
    __u32 bit_size;
    __u32 data_byte_offset;        /* NOT data_bit_offset! */
    __u32 num_entry_point_offsets;
    ...

Plan Clause 4 said: "the new V4L2 API still requires per-slice bit_size and data_bit_offset. This logic is preserved — the new struct keeps these fields (they're per-slice metadata, not per-frame)."

That claim is incorrect. The new struct keeps bit_size but renames data_bit_offset to data_byte_offset. The current src/h265.c:184-209 performs an 8-bit search to compute a bit-granularity offset (the slice-segment-header NALU start bit position), then stores it in slice_params->data_bit_offset. That field name doesn't compile against the new struct.

FFmpeg's reference at libavcodec/v4l2_request_hevc.c:190 sets .data_byte_offset = controls->pic.output->bytesused + sh->data_offset — a plain byte offset, no bit search.

Plan amendment to Clause 4:

Drop the bit-search logic at h265.c:196-209. Replace with byte-offset assignment:

slice_params->data_byte_offset = surface_object->slices_size + slice->slice_data_byte_offset;

(or slice->slice_data_offset + slice->slice_data_byte_offset, depending on libva semantics — Phase 6 source-read of how slice_data_byte_offset is computed by VAAPI consumers will clarify; FFmpeg's pattern is the authority).

The bit-granularity start-bit search is no longer needed — kernel HEVC decoder finds the slice header start from the byte offset and standard NAL unit boundaries.

C2 — dpb[].rps field GONE; pic_order_cnt_val rename; poc_st_curr_* arrays hold DPB indices

Verdict: ✅ accepted, plan is wrong on three sub-claims. Empirical verification:

struct v4l2_hevc_dpb_entry {
    __u64 timestamp;
    __u8  flags;          /* only V4L2_HEVC_DPB_ENTRY_LONG_TERM_REFERENCE = 0x01 */
    __u8  field_pic;
    __u16 reserved;
    __s32 pic_order_cnt_val;   /* singular, NOT pic_order_cnt[0] */
};

#define V4L2_HEVC_DPB_ENTRY_LONG_TERM_REFERENCE  0x01
/* No V4L2_HEVC_DPB_ENTRY_RPS_ST_CURR_BEFORE/AFTER/LT_CURR */

The current src/h265.c:292-304 writes:

slice_params->dpb[i].rps = V4L2_HEVC_DPB_ENTRY_RPS_ST_CURR_BEFORE;  /* etc. */
...
slice_params->dpb[i].pic_order_cnt[0] = hevc_picture->pic_order_cnt;

Three sub-claims wrong:

1. .rps field is gone. The new v4l2_hevc_dpb_entry has only flags (with LONG_TERM_REFERENCE as the sole defined bit). The per-entry classification ST_CURR_BEFORE / ST_CURR_AFTER / LT_CURR is now expressed via the poc_st_curr_before[16], poc_st_curr_after[16], poc_lt_curr[16] arrays in DECODE_PARAMS.
2. .pic_order_cnt[0] is renamed pic_order_cnt_val, singular __s32.
3. The poc_st_curr_* arrays hold DPB INDICES (u8), not POC values. FFmpeg's reference v4l2_request_hevc.c:168-175 calls get_ref_pic_index() which searches dpb[] by timestamp and returns the index, then stores that index in poc_st_curr_before[i].

So the logic must be:

(a) Classify each VAAPI picture->ReferenceFrames[i] as ST_CURR_BEFORE/AFTER/LT_CURR via flags & VA_PICTURE_HEVC_RPS_*. (b) Set dpb[j].flags = V4L2_HEVC_DPB_ENTRY_LONG_TERM_REFERENCE IFF the ref is a long-term ref. (c) Set dpb[j].pic_order_cnt_val = hevc_picture->pic_order_cnt. (d) For each ref, compute its DPB index j, append to the right array (poc_st_curr_before[k++] = j, etc.) based on classification.

Plan claim "DPB extraction logic migrates verbatim" is wrong. The migration is more nuanced — FFmpeg pattern via get_ref_pic_index() is the canonical implementation.

Plan amendment to Clause 6: replace "preserves existing extraction logic from current src/h265.c lines 269-315" with explicit re-spec:

Reference frames are classified into three lists by their VA_PICTURE_HEVC flags: ST_CURR_BEFORE (forward refs in display order), ST_CURR_AFTER (backward refs), LT_CURR (long-term refs). For each VAAPI ReferenceFrame[i] that's valid (not VA_PICTURE_HEVC_INVALID, not VA_INVALID_SURFACE):

1. Lookup its surface_object via SURFACE(driver_data, hevc_picture->picture_id).
2. Determine its DPB-array index j (the position it occupies in the kernel-side dpb[] array — assign sequentially).
3. Set decode_params.dpb[j].timestamp = v4l2_timeval_to_ns(&surface->timestamp).
4. Set decode_params.dpb[j].pic_order_cnt_val = hevc_picture->pic_order_cnt.
5. Set decode_params.dpb[j].flags = (hevc_picture->flags & VA_PICTURE_HEVC_RPS_LT_CURR) ? V4L2_HEVC_DPB_ENTRY_LONG_TERM_REFERENCE : 0.
6. Set decode_params.dpb[j].field_pic = (hevc_picture->flags & VA_PICTURE_HEVC_FIELD_PIC) ? 1 : 0.
7. Append j to poc_st_curr_before[k]++ if VA_PICTURE_HEVC_RPS_ST_CURR_BEFORE; _after[k]++ if RPS_ST_CURR_AFTER; _lt_curr[k]++ if RPS_LT_CURR.

decode_params.num_active_dpb_entries, num_poc_st_curr_before, num_poc_st_curr_after, num_poc_lt_curr are then the counts.

This is a new piece of code, not a verbatim migration. Phase 6 implementer notes the rewrite scope.

C3 — Union-aliasing reasoning wrong (again, like iter1 C1)

Verdict: ✅ accepted, my reasoning was wrong; the safety claim itself holds.

Plan said the unconditional params.h265.num_slices = 0 reset at BeginPicture is "benign for non-HEVC since the union aliasing puts num_slices in a region overwritten by RenderPicture's per-buffer copies." The reviewer's verification (and my offsetof check during this phase) confirms num_slices lands at byte ~17764 of the union after the slices[64] insertion — well past any H.264 (max 240) or MPEG-2 (max 376) field. Non-H.264/MPEG-2 paths never touch byte 17764; the reset is benign because of non-aliasing, not because of overwriting.

Same anti-pattern as iter1 review C1: I wrote a hand-wavy union-aliasing argument that turned out to be wrong-mechanism but right-conclusion. Lesson reinforced (but already in feedback_review_empirical_over_theoretical.md — both reviews now have an example of the same trap).

Plan amendment to Clause 10: replace the wording with "after slices[64] insertion, num_slices lands at byte ~17764, well past any H.264 (≤240) or MPEG-2 (≤376) field. The reset is benign for non-HEVC because no other profile reads union byte 17764, NOT because RenderPicture per-buffer copies overwrite that location."

Should-fix findings — all 4 confirmed

S1 — Two PPS flags missing from Clause 3 mapping

Verdict: ✅ accepted. Empirical verification:

#define V4L2_HEVC_PPS_FLAG_DEBLOCKING_FILTER_CONTROL_PRESENT  (1ULL << 19)
#define V4L2_HEVC_PPS_FLAG_UNIFORM_SPACING                    (1ULL << 20)

Plan Clause 3 listed bits 0-18, missed 19+20. BBB likely doesn't trigger them (single-tile, default deblocking) but the mapping is incomplete.

Plan amendment to Clause 3: add to the PPS flags table:

Flag bit	New name	VAAPI source
19	_DEBLOCKING_FILTER_CONTROL_PRESENT	picture->slice_parsing_fields.bits.deblocking_filter_control_present_flag (or implied — VAAPI may not expose directly; in that case set 1 if PPS was decoded with deblocking-filter overrides allowed)
20	_UNIFORM_SPACING	picture->pic_fields.bits.uniform_spacing_flag

Phase 7 byte-compare on PPS bytes will catch silent zeroing.

S2 — Three PPS scalar fields missing from source mapping

Verdict: ✅ accepted. PPS struct dump on fresnel:

struct v4l2_ctrl_hevc_pps {
    __u8 pic_parameter_set_id;                         /* iter1 plan missed */
    __u8 num_extra_slice_header_bits;
    __u8 num_ref_idx_l0_default_active_minus1;          /* iter1 plan missed */
    __u8 num_ref_idx_l1_default_active_minus1;          /* iter1 plan missed */
    ...

The old staging-era v4l2_ctrl_hevc_pps didn't have these fields; new one does. Without explicit fill, the memset(&pps, 0, ...) will leave them zero — probably correct for BBB but not guaranteed.

Plan amendment to Clause 3: add to source mapping:

New PPS field	Source
pic_parameter_set_id	0 (single-PPS streams; VAAPI doesn't expose)
num_ref_idx_l0_default_active_minus1	picture->num_ref_idx_l0_default_active_minus1
num_ref_idx_l1_default_active_minus1	picture->num_ref_idx_l1_default_active_minus1

S3 — SCALING_MATRIX content divergence between FFmpeg and libva paths

Verdict: ✅ accepted as Phase 6 implementer's choice. FFmpeg at v4l2_request_hevc.c:384-403 populates from the active scaling list pointer sl; if sl == NULL (BBB has no scaling list per SPS flags), FFmpeg's struct stays memset-zero. Plan's "populate spec defaults" branch would produce non-zero default matrices instead.

Phase 7 byte-compare consequence: if Phase 6 implements "spec defaults when iqmatrix_set==false," Baseline B compare will diverge on SCALING_MATRIX bytes. Two response paths:

(a) Implement matching FFmpeg: send zeros when no scaling list (preserves byte-equality vs cross-validator).

(b) Implement spec defaults (semantically purer; kernel may handle either).

Plan amendment to Clause 5: defer to Phase 6 implementer; flag in commit message which path was chosen and why. Phase 7 byte-compare will validate via decode-correctness of the result regardless of which path matches Baseline B exactly.

S4 — Wrong FFmpeg function name cited

Verdict: ✅ accepted, cosmetic fix.

Plan Clause 5 cited ff_v4l2_request_query_control_default_value. The actual FFmpeg call at v4l2_request_hevc.c:677 is ff_v4l2_request_query_control(avctx, &scaling_matrix). Same intent (probe control existence), different function name.

Plan amendment to Clause 5: fix the citation. (No semantic change.)

Question / clarification — all 3 verified

Q1 — object_heap allocator handles new union size automatically

Verdict: ✅ verified. src/request.c:142-143:

object_heap_init(&driver_data->surface_heap,
                 sizeof(struct object_surface), SURFACE_ID_OFFSET);

object_heap_init is parameterized on sizeof(struct object_surface) — automatic pickup of the larger size after slices[64] insertion. No hardcoded constant to update.

Q2 — slice_segment_addr field present in new API; must be populated

Verdict: ✅ verified. slice_segment_addr (u32) is in the new v4l2_ctrl_hevc_slice_params struct dump. Current src/h265.c::h265_fill_slice_params does NOT populate it. For multi-slice frames, omission would cause kernel decode error or wrong pixel output. For single-slice BBB, the implicit zero is correct (first slice is at address 0).

Plan amendment to Clause 4: add field to source mapping:

| slice_params.slice_segment_addr | slice->slice_segment_address (VAAPI exposes this directly) |

Q3 — VAPictureParameterBufferType per-frame send for HEVC unverified

Verdict: deferred to Phase 7 LIBVA_TRACE capture. iter1+T4 patterns suggest yes, but worth grepping the trace for va_TraceRenderPicture.*VAPictureParameterBufferType per frame at Phase 7 verification.

Nits — all 3 noted

N1 — Array size description wrong

Plan Clause 6 said poc_st_curr_before[8] etc. Actual kernel UAPI uses [V4L2_HEVC_DPB_ENTRIES_NUM_MAX] = [16]. Plan amendment: use the macro reference, value 16.

N2 — Phase 7 harness directory naming inconsistent

/tmp/iter2_phase7/png_hw/*.jpg — the directory is named "png" but mpv writes JPEG. Cosmetic; plan amendment: rename png_hw → image_hw (or similar) in Phase 7 commands. Functionally no-op.

N3 — BeginPicture unconditional reset cleanup deferred

Plan Clause 10 adds params.h265.num_slices = 0 unconditional. Same pattern as the existing params.h264.matrix_set = false. Future cleanup: profile-aware reset (per Phase 4 cross-cutting backlog item B3 from iter1 close). No iter2 action.

Plan amendments consolidated

After Phase 5, the iter2 plan stands as authored in phase4_iter2_plan.md, with the following amendments:

1. Clause 4 (slice_params): drop bit-search; use data_byte_offset (NOT data_bit_offset); add slice_segment_addr field population from slice->slice_segment_address (C1 + Q2).
2. Clause 6 (decode_params): replace "verbatim migration" with explicit DPB classification + index-array population logic; rename pic_order_cnt[0] → pic_order_cnt_val; drop dpb[].rps (use dpb[].flags & LONG_TERM_REFERENCE for long-term classification only); populate poc_st_curr_before/after/lt_curr arrays with DPB indices via FFmpeg get_ref_pic_index() pattern (C2).
3. Clause 3 (PPS): add 2 missing flags (bits 19, 20) and 3 missing scalars (pic_parameter_set_id, num_ref_idx_l0/l1_default_active_minus1) (S1, S2).
4. Clause 5 (SCALING_MATRIX): fix function name ff_v4l2_request_query_control (S4); flag the FFmpeg-vs-libva default-fill divergence as a Phase 6 implementer choice (S3).
5. Clause 10 (per-slice accumulation): fix the union-aliasing reasoning wording — benign because non-HEVC profiles don't read byte 17764, NOT because RenderPicture overwrites it (C3).
6. Clause 6 (array sizes): use V4L2_HEVC_DPB_ENTRIES_NUM_MAX (= 16) macro reference, not hardcoded [8] (N1).
7. Phase 7 harness: rename png_* directories → image_* for cosmetic clarity (N2).
8. Q3 deferred: Phase 7 LIBVA_TRACE capture confirms VAPictureParameterBufferType per-frame send for HEVC.

Plan re-locks with these amendments. Phase 6 proceeds.

Phase 5 close

Per feedback_dev_process.md Phase 5: review handed over to sonnet-architect (Plan agent), 13 findings surfaced, 7 substantive amendments accepted. Critical findings C1 + C2 would have been compile errors (data_bit_offset undefined in struct) or silent semantic bugs (dpb.rps undefined; pic_order_cnt struct shape wrong). The reviewer's outside-look saved Phase 6/7 from at least 2 loopback cycles.

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. Lesson held this iteration. Phase 6 implementer can now proceed against an amended plan that compiles against the actual kernel UAPI on first try.