# Phase 5 — iter2 second-model review (Plan/sonnet)

Reviewed 2026-05-16 evening by `Plan` subagent with `model: sonnet`. Full Phase 0-4 markdown pasted verbatim (no curation per `feedback_dev_process` §5). Same guardrails as ampere-fourier iter1 Phase 5 + the new `feedback_upstream_alignment_over_speed` explicitly flagged.

Reviewer surfaced 4 findings. Per `feedback_review_empirical_over_theoretical`, I test-verified the most consequential before adopting/rebutting.

## Finding #1 — `GstH265SPSEXT` data-source gap (REFUTED empirically)

> Looking at the actual GStreamer reference implementation (`gstv4l2codech265dec.c:940`), `gst_v4l2_codec_h265_dec_fill_ext_sps_rps` uses two distinct data sources: (1) `sps->short_term_ref_pic_set[i]` — in `GstH265ShortTermRefPicSet`, populated by `gsth265parser.c`. (2) `sps_ext->short_term_ref_pic_set_ext[i]` — comes from `GstH265SPSEXT` via `gst_h265_decoder_get_sps_ext(decoder, sps)`. This function is part of `gst-plugins-bad`'s H.265 decoder base class, not from `gsth265parser.c`. […] An additional struct/file beyond the two planned vendor files is needed.

**Verdict**: rebut with empirical evidence.

Direct read of the fetched `gsth265parser.h` shows the vendored header defines TWO structs (not one):

`struct _GstH265ShortTermRefPicSet` (~line 939):
```c
guint8  inter_ref_pic_set_prediction_flag;
guint8  delta_idx_minus1;
guint8  delta_rps_sign;
guint16 abs_delta_rps_minus1;
guint8  NumDeltaPocs;
guint8  NumNegativePics;
guint8  NumPositivePics;
guint8  UsedByCurrPicS0[16];
guint8  UsedByCurrPicS1[16];
gint32  DeltaPocS0[16];
gint32  DeltaPocS1[16];
guint8  NumDeltaPocsOfRefRpsIdx;
```

`struct _GstH265ShortTermRefPicSetExt` (~line 966; documented as `Since: 1.28` — landed alongside the V4L2 EXT_SPS_*_RPS work):
```c
guint8  use_delta_flag[16];
guint8  used_by_curr_pic_flag[16];
guint32 delta_poc_s0_minus1[16];
guint32 delta_poc_s1_minus1[16];
```

The reviewer correctly identified that GStreamer's `fill_ext_sps_rps` uses TWO data sources, but conflated `GstH265SPSEXT` (a separate struct that holds SPS-extension parameters like SCC) with `GstH265ShortTermRefPicSetExt` (the RPS-extended struct that has the four fields the reviewer flagged as missing). Both `GstH265ShortTermRefPicSet` and `GstH265ShortTermRefPicSetExt` are in the SAME vendored gsth265parser.h. So the planned vendoring scope (just gsth265parser.c + .h + gstbitreader.c + .h) IS sufficient.

`gst_h265_decoder_get_sps_ext()` is likely a thin accessor that returns the parsed `GstH265ShortTermRefPicSetExt` array which `gsth265parser.c` writes during SPS parsing. If that accessor lives in GstH265Decoder base class instead of in gsth265parser.c, we just inline-equivalent it in our integration — direct field access on the `GstH265SPS` struct's `short_term_ref_pic_set` and `short_term_ref_pic_set_ext` arrays.

Phase 4 §Step 4 doesn't need an additional vendor file. It DOES need to call out the two-struct usage explicitly so Phase 6's implementation maps both `GstH265ShortTermRefPicSet` fields AND `GstH265ShortTermRefPicSetExt` fields onto the single `struct v4l2_ctrl_hevc_ext_sps_st_rps` entry. Amending Step 4 accordingly.

## Finding #2 — per-fd storage for `has_hevc_ext_sps_rps` (ADOPT)

> The plan should make explicit that `has_hevc_ext_sps_rps` is probed per-device-fd and stored per-device (parallel to how `video_fd_rkvdec` / `video_fd_hantro` are stored separately), not as a single scalar on `driver_data`. Otherwise a future codec routing change could silently misbehave. Adding an explicit `if (kind == 'r' && has_hevc_ext_sps_rps)` guard in `h265_set_controls` also provides a cheap, human-readable explanation of intent — worth the two lines regardless.

**Verdict**: adopt.

The iter38 multi-device-probe pattern (memory `feedback_multi_device_probe_design`) stores rkvdec and hantro fds separately on `struct request_data`. Mirroring that, the probe result should be `bool has_hevc_ext_sps_rps_rkvdec; bool has_hevc_ext_sps_rps_hantro;` — one per fd. h265 path only consults the rkvdec one (HEVC routes through rkvdec); hantro one stays false naturally (hantro doesn't have rkvdec-specific controls).

Plus the explicit `if (kind == 'r' && has_hevc_ext_sps_rps_rkvdec)` guard for human-readable intent regardless of probe correctness.

Phase 4 amendment: §Step 3 storage changes from single bool to per-driver-kind pair on `struct request_data`; §Step 4 control-submission guard becomes `kind == 'r' && has_hevc_ext_sps_rps_rkvdec`.

## Finding #3 — SPS NAL caching strategy (ADOPT, critical)

> This is the most significant open issue the plan glosses over. […] For HEVC decode, ffmpeg-vaapi submits the full "extradata" (SPS + PPS NALs) as additional `VASliceData` buffers before the IDR frame slice. So the SPS NAL is present in `source_data` at frame 0, concatenated before the IDR slice. The NAL walk (look for `nal_unit_type == 33`) will find it — but only when ffmpeg sends it, which is not every frame. For non-IDR frames, `source_data` contains only the slice NALs and the walk will not find an SPS. The plan needs to address this explicitly: parse and cache the SPS at the first frame where it appears, and reuse the cached parser output for subsequent frames.

**Verdict**: adopt.

Phase 4 implicitly treated SPS parsing as per-frame, which would (a) waste work on every frame and (b) silently submit zero-filled RPS arrays for non-IDR frames, which would re-trigger the OOPS.

Storage shape: cache the parsed `GstH265SPS` (which contains both `GstH265ShortTermRefPicSet[]` AND `GstH265ShortTermRefPicSetExt[]`) on `struct request_data` keyed by SPS id, OR on `struct object_surface` since each surface has access to driver_data anyway. Phase 4 §Step 4 amends to:

1. At first `h265_set_controls` call OR at any frame where the SPS NAL is found in `source_data`: parse the SPS, store the parsed `GstH265SPS` on `driver_data->cached_hevc_sps[sps_id]`.
2. On subsequent frames: skip SPS parsing if a cached SPS exists for the seq_parameter_set_id referenced by the current PPS. Reuse the cached parser output to fill the EXT_SPS_*_RPS controls.
3. Cache invalidation: clear cached SPS on context destroy.

This adds ~20 LOC to Phase 4's step-4 surface estimate.

## Finding #4 — C3 prediction caveat (ADOPT, minor)

> The shared-SHA claim requires a caveat. HEVC frame 0 decoded from a different encoder's output than the H.264/VP8/MPEG-2 frame 0 will not necessarily produce identical pixel values even from the same source content, because the HEVC encoder may have used a different quantization step or reference frame structure than the H.264 encoder did. […] Adjust the prediction to "frame 0 HW-decoded SHA matches SW-decoded SHA for the HEVC clip" rather than matching H.264's SHA.

**Verdict**: adopt.

This is a real precision issue in ampere-fourier iter1 Phase 3 too — there I observed sha `3214803d8be74416` was the same across H.264, VP8, MPEG-2 frame 0, but that was lucky empirical convergence (encoders happened to produce bit-identical I-frames at this quality level for THIS source), not guaranteed. For HEVC, the encoder is x265 fast CRF 28, different bitrate target than the other three (5.1 MB vs 13.6/10.5/59.3 MB) — quantization will differ.

Phase 4 §"Phase 7 predictions" C3 row amends from:
> PASS — HEVC I-frame at t=0, no inter-prediction, same sha as ampere-fourier iter1 H.264/VP8/MPEG-2 (`3214803d8be74416`)

to:
> PASS — HEVC frame 0 HW-decode SHA matches HEVC SW-decode SHA from the same encoded clip (anchored per-clip, not cross-codec). The cross-codec convergence observed in iter1 was empirical-not-guaranteed.

## Phase 4 amendments summary

Three concrete changes to phase4_plan_iter2.md (deferred to a brief diff commit after this review file lands):
1. §Step 3 — `has_hevc_ext_sps_rps` storage changes from single bool to per-driver-kind pair (`_rkvdec` / `_hantro`).
2. §Step 4 — explicit two-struct usage (`GstH265ShortTermRefPicSet` + `GstH265ShortTermRefPicSetExt`); SPS parse-and-cache strategy; cache keyed by sps_id on `driver_data`.
3. §"Phase 7 predictions" C3 — anchor SHA per-clip, not cross-codec.

Risk register gets one addition (per reviewer): SPS NAL absent from non-IDR frames if SPS parsing isn't cached. Mitigation IS the caching strategy above; risk #6 in updated register.

## Summary

Reviewer surfaced 1 critical finding that empirical verification refuted (GstH265SPSEXT gap — was actually two structs both in the vendored header), and 3 finding that genuinely tighten the plan (per-fd storage, SPS caching, C3 anchor). All adopted findings result in concrete Phase 4 amendments before Phase 6 starts.

Per `feedback_review_empirical_over_theoretical`: the rebut on Finding #1 was done by reading the actual vendored header content, not by source-reading the reviewer's argument. Empirical evidence won.

The plan is sound with the three amendments. Phase 6 can proceed.