From 902d6c17ba81fb86af7c624346125acf35b8bb1b Mon Sep 17 00:00:00 2001 From: claude-noether Date: Sun, 17 May 2026 12:19:19 +0000 Subject: [PATCH] ampere-av1 Phase 5 review: stale linked_decode_surface_id clear; remap fix REVERTED MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/av1.c | 16 +++++++++++++++- src/picture.c | 12 ++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/src/av1.c b/src/av1.c index e47fb5d..727ffd8 100644 --- a/src/av1.c +++ b/src/av1.c @@ -293,7 +293,21 @@ static void av1_fill_frame(VADecPictureParameterBufferAV1 *picture, 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] = { V4L2_AV1_FRAME_RESTORE_NONE, diff --git a/src/picture.c b/src/picture.c index 785b4a4..ef09064 100644 --- a/src/picture.c +++ b/src/picture.c @@ -390,6 +390,18 @@ VAStatus RequestBeginPicture(VADriverContextP context, VAContextID context_id, */ if (surface_object->current_slot != NULL) 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 = cap_pool_acquire(&driver_data->capture_pool, surface_id);