Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Refactors the Vulkan float_adm feature extractor to use the shared kernel_template infrastructure, deduplicating pipeline/layout/shader/descriptor-pool management and creating the 16 stage/scale pipelines via template variants.
Changes:
- Migrates
float_adm_vulkanpipeline creation/destruction toVmafVulkanKernelPipeline+vmaf_vulkan_kernel_pipeline_add_variant(). - Keeps the existing 16-entry
[stage][scale]pipeline table while making[0][0]alias the template base pipeline. - Documents the migration and invariants in
CHANGELOG.mdanddocs/rebase-notes.md.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| libvmaf/src/feature/vulkan/float_adm_vulkan.c | Replaces manual Vulkan pipeline/DSL/pool management with kernel_template + variant pipelines. |
| docs/rebase-notes.md | Adds entry 0122 describing the migration and the destroy-skip invariant for (0,0). |
| CHANGELOG.md | Records the refactor under “Changed” with scope and test validation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+269
to
+276
| VkComputePipelineCreateInfo cpci = { | ||
| .stage = | ||
| { | ||
| .pName = "main", | ||
| .pSpecializationInfo = &spec_info, | ||
| }, | ||
| }; | ||
| if (vkCreatePipelineLayout(dev, &plci, NULL, &s->pipeline_layout) != VK_SUCCESS) | ||
| return -ENOMEM; | ||
| return vmaf_vulkan_kernel_pipeline_add_variant(s->ctx, &s->pl, &cpci, out_pipeline); |
Comment on lines
+248
to
+252
| map[0] = (VkSpecializationMapEntry){.constantID = 0, .offset = 0, .size = sizeof(int32_t)}; | ||
| map[1] = (VkSpecializationMapEntry){.constantID = 1, .offset = 4, .size = sizeof(int32_t)}; | ||
| map[2] = (VkSpecializationMapEntry){.constantID = 2, .offset = 8, .size = sizeof(int32_t)}; | ||
| map[3] = (VkSpecializationMapEntry){.constantID = 3, .offset = 12, .size = sizeof(int32_t)}; | ||
| map[4] = (VkSpecializationMapEntry){.constantID = 4, .offset = 16, .size = sizeof(int32_t)}; |
0cad1f3 to
af40030
Compare
d0d9a19 to
6981c64
Compare
af40030 to
2ed2ec8
Compare
6981c64 to
5c5b553
Compare
c67bddb to
500f6a9
Compare
…_template Twin migration to adm_vulkan (T-GPU-DEDUP-21); same 16-pipeline 2-D [stage][scale] shape (4 stages * 4 scales). State collapses dsl + pipeline_layout + shader + desc_pool to VmafVulkanKernelPipeline pl. The VkPipeline pipelines[4][4] 2-D lookup is preserved so the per-stage dispatch path stays clean, but pipelines[0][0] aliases s->pl.pipeline (template's base) and the other 15 entries are sibling pipelines via _add_variant(). close_fex destroys the 15 sibling variants before calling vmaf_vulkan_kernel_pipeline_destroy() on the bundle. The (0, 0) skip is essential because pipelines[0][0] aliases s->pl.pipeline. Validated by meson test test_vulkan_smoke test_vulkan_async_pending_fence test_vulkan_pic_preallocation (all green) + clean compile. Numerical contract unchanged. Based on refactor/vulkan-template-add-variant (PR #272); will rebase clean once #272 lands. CHANGELOG.md + docs/rebase-notes.md (entry 0122) updated per ADR-0108 deep-dive deliverables. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
float_adm_vulkan (T-GPU-DEDUP-22) binds 9 SSBOs (src + 2× dwt_tmp + 2× band + 2× csf + accum + dis) and was tripping the template's max-8 guard at init() time, causing every extract() call to fail with "problem reading pictures" / "problem flushing context". The new cap of 16 is purely additive: existing consumers (PSNR 3 SSBOs, moment 4, ciede 3, motion 2, ssim 6, vif 6, etc.) are unaffected. Vulkan's portability minimum for maxDescriptorSetStorageBuffers is 96 on conformant devices, so 16 is well within the safe range. Smoke: float_adm Netflix-pair (576×324×48f) now reports adm2 mean 0.934515; smoke tests + lint clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
500f6a9 to
2353f2d
Compare
8 tasks
lusoris
pushed a commit
that referenced
this pull request
May 3, 2026
Per CLAUDE.md §12 r13, every PR that closes a bug, opens a bug, or rules a Netflix upstream report not-affecting-the-fork updates docs/state.md in the same PR. Today's session shipped 7 such PRs without state.md updates (most were drafted in subagent-driven runs where the bookkeeping slipped). This is the catch-up. Recently closed (added): - vmaf_tiny_v1.onnx external-data ref fix (PR #296) - kernel_template SSBO cap 8->16 (PR #288 + named constant in #292) - deliverables-check.sh backslash strip (PR #292) - CI workflows draft-skip guard (PR #300) - CLAUDE.md §12 r14 ffmpeg-patches gate fix (PR #297) - ADR slug-drift cleanup (PR #304) - 1.07e-3 CPU vmaf_v0.6.1 score drift bisect (PR #305) — confirmed inherited from upstream a44e5e6 motion edge-mirror fix; not a fork regression. Snapshot regen in separate PR aligns with fork. No code changes. State.md only. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
lusoris
added a commit
that referenced
this pull request
May 3, 2026
…307) * docs(state): record 2026-05-02 session bug-status changes (7 closes) Per CLAUDE.md §12 r13, every PR that closes a bug, opens a bug, or rules a Netflix upstream report not-affecting-the-fork updates docs/state.md in the same PR. Today's session shipped 7 such PRs without state.md updates (most were drafted in subagent-driven runs where the bookkeeping slipped). This is the catch-up. Recently closed (added): - vmaf_tiny_v1.onnx external-data ref fix (PR #296) - kernel_template SSBO cap 8->16 (PR #288 + named constant in #292) - deliverables-check.sh backslash strip (PR #292) - CI workflows draft-skip guard (PR #300) - CLAUDE.md §12 r14 ffmpeg-patches gate fix (PR #297) - ADR slug-drift cleanup (PR #304) - 1.07e-3 CPU vmaf_v0.6.1 score drift bisect (PR #305) — confirmed inherited from upstream a44e5e6 motion edge-mirror fix; not a fork regression. Snapshot regen in separate PR aligns with fork. No code changes. State.md only. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * ci: re-trigger after (6) opt-out fix --------- Co-authored-by: Lusoris <lusoris@pm.me> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
13 tasks
lusoris
pushed a commit
that referenced
this pull request
May 4, 2026
Per ADR-0165 / CLAUDE.md §12 r13. Bookkeeping refresh of `docs/state.md` to close out the busy 2026-05-02/03 session: - Header date bumped 2026-04-29 → 2026-05-03. - Closed Issue #239 (FFmpeg `libvmaf_vulkan` wall-clock serialisation): row moved Open → Recently closed; cited PR #241 / commit `e266bf8e` and ADR-0251 (renumbered from 0235 by PR #310 dedup sweep). Closure verified by the v2 async pending-fence ring's `v2 ≤ 0.7 × v1` measurement gate flipping ADR-0251 to Accepted. - New Open-bugs row: `y4m_convert_411_422jpeg` heap-buffer-overflow on 4:1:1 with `dst_c_w == 1` (PR #348 fuzz). Reproducer parked at `libvmaf/test/fuzz/y4m_input_known_crashes/y4m_411_w2_h4_oob_dst.y4m`; fix follow-up PR TBD. - Recently closed audited for stale drafts: six rows updated with merged commit SHAs (#288, #292, #296, #297, #300, #304, #305) and the kernel-template citation corrected to ADR-0246 (post-dedup; ADR-0221 is now `changelog-adr-fragment-pattern.md`). - Netflix#955 deferred row: last-checked stamp refreshed to 2026-05-03; Netflix#1494 still `state=OPEN` per gh API. - "Update protocol" untouched; no row removed below its closure threshold; "Confirmed not-affected" entries unchanged. Six ADR-0108 deliverables: 1. Research digest — no digest needed: state-md bookkeeping. 2. Decision matrix — no alternatives: only-one-way fix per ADR-0165 / CLAUDE.md §12 r13. 3. AGENTS.md invariant note — no rebase-sensitive invariants. 4. Reproducer / smoke command — `mkdocs build --strict` + view the file; PR #348's harness reproduces the new Open bug. 5. CHANGELOG fragment — `changelog.d/changed/state-md-refresh-2026-05-03.md`. 6. Rebase note — no rebase impact: fork-local doc bookkeeping. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
lusoris
added a commit
that referenced
this pull request
May 4, 2026
#352) Per ADR-0165 / CLAUDE.md §12 r13. Bookkeeping refresh of `docs/state.md` to close out the busy 2026-05-02/03 session: - Header date bumped 2026-04-29 → 2026-05-03. - Closed Issue #239 (FFmpeg `libvmaf_vulkan` wall-clock serialisation): row moved Open → Recently closed; cited PR #241 / commit `e266bf8e` and ADR-0251 (renumbered from 0235 by PR #310 dedup sweep). Closure verified by the v2 async pending-fence ring's `v2 ≤ 0.7 × v1` measurement gate flipping ADR-0251 to Accepted. - New Open-bugs row: `y4m_convert_411_422jpeg` heap-buffer-overflow on 4:1:1 with `dst_c_w == 1` (PR #348 fuzz). Reproducer parked at `libvmaf/test/fuzz/y4m_input_known_crashes/y4m_411_w2_h4_oob_dst.y4m`; fix follow-up PR TBD. - Recently closed audited for stale drafts: six rows updated with merged commit SHAs (#288, #292, #296, #297, #300, #304, #305) and the kernel-template citation corrected to ADR-0246 (post-dedup; ADR-0221 is now `changelog-adr-fragment-pattern.md`). - Netflix#955 deferred row: last-checked stamp refreshed to 2026-05-03; Netflix#1494 still `state=OPEN` per gh API. - "Update protocol" untouched; no row removed below its closure threshold; "Confirmed not-affected" entries unchanged. Six ADR-0108 deliverables: 1. Research digest — no digest needed: state-md bookkeeping. 2. Decision matrix — no alternatives: only-one-way fix per ADR-0165 / CLAUDE.md §12 r13. 3. AGENTS.md invariant note — no rebase-sensitive invariants. 4. Reproducer / smoke command — `mkdocs build --strict` + view the file; PR #348's harness reproduces the new Open bug. 5. CHANGELOG fragment — `changelog.d/changed/state-md-refresh-2026-05-03.md`. 6. Rebase note — no rebase impact: fork-local doc bookkeeping. Co-authored-by: Lusoris <lusoris@pm.me> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Twin migration to
adm_vulkan(T-GPU-DEDUP-21 / PR #287);same 16-pipeline 2-D
[stage][scale]array (4 stages × 4 scales).State collapses
dsl + pipeline_layout + shader + desc_pooltoVmafVulkanKernelPipeline pl.pipelines[0][0]aliasess->pl.pipeline(template's base); the other 15 entries aresibling pipelines via
_add_variant().Test plan
ninja -C build-vulkancleanmeson test test_vulkan_smoke test_vulkan_async_pending_fence test_vulkan_pic_preallocation— all greenpre-commit run --files <touched>cleanBase branch
This PR targets
refactor/vulkan-template-add-variant(#272).Six deep-dive deliverables (ADR-0108)
no digest needed: routine refactorno alternatives: only-one-way fixAGENTS.mdinvariant note: seedocs/rebase-notes.mdentry 0122 — variants destroyed before bundle + the (0,0) destroy-skip invariant (pipeline aliasing)### Changed🤖 Generated with Claude Code