ampere-kernel-decoders/phase5_review_iter2.md

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):

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):

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.