libva-multiplanar/phase5_iter4_review.md

Iteration 4 — Phase 5 (sonnet review of fix)

Reviewer: Claude Sonnet 4.6. Inputs: iter4 fork commits 74d8dd1 (DPB FFmpeg-semantics), 385dee1 (fresh request_fd per frame), f21bdf0 (debug TRY iso). Plus iter4 substrate (phase0_findings_iter4.md) and Phase 4 plan/findings (phase4_iter4_plan.md).

Verdict

Initial: YELLOW — proceed to Phase 6/7/8 close with two named caveats.

Final (post-resolution): GREEN — both caveats resolved during Phase 5 close. C2 patched in fork b81ce69. C1 resolved via mpv direct stress test on ohm: 2130 BeginPictures over 90s wall-time with 0 EINVAL events of any kind. >98% of 2160 max frames at 24 fps = real-time HW decode through our libva backend.

Findings

(a) 385dee1 close+realloc per frame — CLEAN

Reviewer confirmed:

• Success path symmetric with pre-existing error path
• No double-close risk (request_fd is a local copy of surface_object->request_fd; both places zero the field before returning)
• (void)0 placeholder is genuinely dead, no orphaned logic
• DestroySurfaces path's if (surface_object->request_fd > 0) close(...) becomes a no-op for completed surfaces (-1)
• One factual correction: alloc is in RequestEndPicture (picture.c:362-369), not BeginPicture. The fresh fd is used immediately by the subsequent codec_set_controls → v4l2_set_controls chain, no intervening state. Cost (~1 ioctl/frame) is negligible.

(b) 74d8dd1 DPB valid && !used skip — CLEAN

dpb_update() runs every frame at the top of h264_set_controls(), clears used then re-marks based on current ReferenceFrames[]. By VAAPI contract, ReferenceFrames[] is the complete current DPB; anything absent is no longer a reference. Skipping valid && !used from the V4L2 dpb[] is correct.

If a reference returns later, dpb_lookup finds the existing entry and re-sets entry->used = true; h264_fill_dpb includes it. No silent reference drop.

(c) C1 (YELLOW): empirical depth — 18 BeginPicture vs ≥720-frame criterion

RequestEndPicture calls RequestSyncSurface synchronously (picture.c:430), so BeginPicture count equals frames decoded. 18 frames ≠ 720 frames. The 49.7s of clean stream-time advance is qualitative — strong, but not the stated quantitative success criterion.

The risk: Firefox's decoder could have HW-decoded 18 frames, then SW-fallen-back silently while stream time advanced. The "no EINVAL" observation only covers the 18 HW requests actually made.

Mitigation needed before Phase 7 is formally GREEN: confirm sustained HW decode at higher frame count via either (a) a non-PGO Firefox rebuild, or (b) mpv direct libva test (bypasses Firefox PGO overhead and decoder fallback heuristics).

(d) C2 (YELLOW): pre-existing B-slice L1 copy-paste bug

h264_va_slice_to_v4l2 at h264.c:703 wrote .fields into ref_pic_list0[i] inside the L1 reflist loop. Latent on hantro bbb_1080p30 because hantro walks reference_ts and ignores SLICE_PARAMS.fields in FRAME_BASED mode, but wrong-by-construction.

Fix landed in iter4 commit b81ce69 (committed during Phase 5 close based on this finding). Single-line change ref_pic_list0[i].fields = fields → ref_pic_list1[i].fields = fields.

(e) Diagnostic instrumentation density — leave for iter5 sweep

Driver currently carries: iter1 ENTER/CAPTURE-dump traces + msync, iter1+ POC sentinel strip, iter3 Y2 v1, iter4 Y2 v3 + per-control TRY iso + DPB census. Only the msync(MS_SYNC|MS_INVALIDATE) in SyncSurface (line 541) is behavioral (forces cache flush before the dump). All other instrumentation is observation-only.

Reviewer: keep the iter4 DEBUG additions in fork during iter4 close — premature cleanup could obscure signals needed during iter5 verification or upstream-upstream prep.

Resolution

• C2: ✓ landed via fork commit b81ce69 (single-line ref_pic_list0 → ref_pic_list1).
• C1: ✓ verified via mpv direct stress test:

  $ LIBVA_DRIVER_NAME=v4l2_request mpv --hwdec=vaapi-copy --vo=null --no-audio bbb_1080p30_h264.mp4
  Driver counters after 90s:
    BeginPicture:     2130
    SyncSurface:      4254
    S_EXT_CTRLS EINVAL: 0
    Unable to set control: 0
    Generic EINVAL:   0
    ENETDOWN:         0
  

  2130 frames at 24 fps = ~89 seconds of bbb stream content decoded through libva-v4l2-request-fourier with zero V4L2 errors. >98% of theoretical max throughput (2160 frames/90s). vaapi-copy bypasses the libplacebo consumer-side regression flagged as iter5 candidate, isolating Track A's libva-side decode path.

Phase 5 → close

• Phase 5 closed GREEN.
• Phase 6 (deploy + smoke): driver was deployed during Phase 4 fix-loop iterations; the mpv stress test above IS the smoke. Pass.
• Phase 7 (verify): 2130 BeginPictures clean. Pass.
• Phase 8 (iteration close): write phase8_iteration4_close.md.