ampere-av1 Phase 5 review: stale linked_decode_surface_id clear; remap fix REVERTED

Two of three Phase 5 sonnet-architect review amendments addressed.

Amendment 4 (kept): clear surface_object->linked_decode_surface_id at
BeginPicture after the iter2 Fix 3 release. Prevents stale-link
borrows in copy_surface_to_image when ffmpeg-vaapi recycles a former
display surface as a decode target. No-op for non-AV1 codecs (link
field stays VA_INVALID_SURFACE for them throughout).

Amendment 1 (reverted): reviewer proposed remap_lr_type table
{NONE, SWITCHABLE, WIENER, SGRPROJ} per Kwiboo's permutation,
arguing AV1 spec FrameRestoreType wire encoding differs from
V4L2_AV1_FRAME_RESTORE_* enum order. Applied the proposed table
empirically → regressed ALL tests (allintra 10/10 → 0/10, test_av1
bit-exact → DIFF). Reverted to identity mapping. Either VAAPI's
yframe_restoration_type is already in V4L2-enum order, or vpu981
interprets the V4L2 enum values via a mapping that differs from the
uAPI header documentation. Per [[feedback_review_empirical_over_theoretical]]
empirical PASS wins; updated the code comment to capture the
investigation outcome so the next session has the context.

Amendment 5 (SEPARATE_UV_DELTA_Q sequence flag missing): noted but
not actionable — VAAPI doesn't expose color_config.separate_uv_delta_q.
Will need bitstream-side info to surface. Not blocking current tests.

Verification on ampere:
  test_av1.ivf:             bit-exact PASS sha 029ee72c214b37c1
  av1-1-b8-02-allintra.ivf: 10/10 PASS (no regression)
  av1_larger.ivf:           3/10 PASS (no regression)

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
2026-05-17 12:19:19 +00:00
parent c839b9456e
commit 902d6c17ba
2 changed files with 27 additions and 1 deletions
+15 -1
View File
@@ -293,7 +293,21 @@ static void av1_fill_frame(VADecPictureParameterBufferAV1 *picture,
ctrl->cdef.uv_sec_strength[i] = uv & 0x03; ctrl->cdef.uv_sec_strength[i] = uv & 0x03;
} }
/* ---- loop_restoration ---- (F3) */ /* ---- loop_restoration ---- (F3)
* Phase 5 review Amendment 1 was REVERTED. The reviewer proposed
* remap = {NONE, SWITCHABLE, WIENER, SGRPROJ} (Kwiboo's table)
* based on AV1 spec FrameRestoreType wire encoding
* {NONE=0, SWITCHABLE=1, WIENER=2, SGRPROJ=3} differing from V4L2's
* {NONE=0, WIENER=1, SGRPROJ=2, SWITCHABLE=3}. Empirically applying
* that permutation regressed ALL tests (allintra 10/10 → 0/10) —
* so either VAAPI's yframe_restoration_type is NOT the raw spec
* value (already-remapped to V4L2 enum semantics?), or vpu981
* interprets the V4L2 enum values via a different mapping than
* the V4L2 uAPI header documents. Per
* [[feedback_review_empirical_over_theoretical]] keep the
* identity mapping that empirically works; revisit if a
* restoration-using fixture surfaces a real decode bug.
*/
{ {
uint8_t remap[4] = { uint8_t remap[4] = {
V4L2_AV1_FRAME_RESTORE_NONE, V4L2_AV1_FRAME_RESTORE_NONE,
+12
View File
@@ -390,6 +390,18 @@ VAStatus RequestBeginPicture(VADriverContextP context, VAContextID context_id,
*/ */
if (surface_object->current_slot != NULL) if (surface_object->current_slot != NULL)
surface_unbind_slot(driver_data, surface_object); surface_unbind_slot(driver_data, surface_object);
/*
* AV1 Phase 5 review Amendment 4: clear any stale
* linked_decode_surface_id from a prior film_grain display→decode
* link. If ffmpeg-vaapi recycles a former display surface as a
* decode target, BeginPicture binds a fresh slot — but without
* this reset, copy_surface_to_image's link-follow would still
* borrow from the now-stale linked surface and serve wrong data.
* Cleared unconditionally (cheap) so the next AV1 grain frame
* re-establishes the link if needed.
*/
surface_object->linked_decode_surface_id = VA_INVALID_SURFACE;
{ {
struct cap_pool_slot *cap_slot = struct cap_pool_slot *cap_slot =
cap_pool_acquire(&driver_data->capture_pool, surface_id); cap_pool_acquire(&driver_data->capture_pool, surface_id);