Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR refactors the Vulkan psnr_hvs feature extractor to use the shared kernel pipeline template and _add_variant() helper for its per-plane compute pipelines.
Changes:
- Replaces the manually managed Vulkan pipeline state (
dsl, layout, shader, pool, andpipeline[3]) with aVmafVulkanKernelPipelinebundle plus two chroma pipeline variants. - Adds helper logic to build per-plane specialization data and map plane indices to the correct pipeline handle at dispatch time.
- Updates project documentation (
CHANGELOG.md,docs/rebase-notes.md) to describe the migration and its lifecycle invariants.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
libvmaf/src/feature/vulkan/psnr_hvs_vulkan.c |
Refactors pipeline creation/destruction and descriptor/pipeline access to use the shared Vulkan kernel template. |
docs/rebase-notes.md |
Documents the rebase note and destruction-order invariant for the new variant-based pipeline setup. |
CHANGELOG.md |
Records the Vulkan refactor and validation notes in the changelog. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+192
to
+194
| if (err) | ||
| return err; | ||
| return build_pipeline_for_plane(s, /*plane=*/2, &s->pipeline_chroma_v); |
lusoris
force-pushed
the
refactor/vulkan-template-add-variant
branch
from
0cad1f3 to
af40030
Compare
lusoris
force-pushed
the
refactor/psnr-hvs-vulkan-template
branch
from
99556d7 to
7c0aee6
Compare
lusoris
force-pushed
the
refactor/vulkan-template-add-variant
branch
from
af40030 to
2ed2ec8
Compare
lusoris
force-pushed
the
refactor/psnr-hvs-vulkan-template
branch
from
7c0aee6 to
2cfb3dd
Compare
…template First multi-pipeline-via-variant consumer landed on top of PR #272 (which adds the vmaf_vulkan_kernel_pipeline_add_variant() helper). State collapses dsl + pipeline_layout + shader + desc_pool + pipeline[3] to VmafVulkanKernelPipeline pl + VkPipeline pipeline_chroma_u + VkPipeline pipeline_chroma_v. Plane 0 (luma) is the template's base pipeline; planes 1+2 (chroma U/V) are sibling pipelines via _add_variant() — same layout + shader + DSL + pool, different plane spec-constant. New psnr_hvs_plane_pipeline() accessor maps plane index to the right VkPipeline handle. close_fex destroys the chroma U/V variants first, then calls vmaf_vulkan_kernel_pipeline_destroy() on the bundle (same rule as ssim_vulkan in T-GPU-DEDUP-7). Validated against the Netflix-pair smoke (psnr_hvs mean 31.33, psnr_hvs_y 30.58, psnr_hvs_cb 37.26, psnr_hvs_cr 38.20 across 48 frames) + meson test test_vulkan_smoke test_vulkan_async_pending_fence test_vulkan_pic_preallocation (all green). 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 0118) updated per ADR-0108 deep-dive deliverables. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
lusoris
force-pushed
the
refactor/psnr-hvs-vulkan-template
branch
from
2cfb3dd to
9be868e
Compare
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
First multi-pipeline-via-variant consumer of the
vmaf_vulkan_kernel_pipeline_add_variant()helper added byPR #272.
State collapses
dsl + pipeline_layout + shader + desc_pool + pipeline[3]toVmafVulkanKernelPipeline pl + VkPipeline pipeline_chroma_u + VkPipeline pipeline_chroma_v._add_variant()— same layout + shader + DSL + pool, differentplanespec-constant.New
psnr_hvs_plane_pipeline()accessor maps the plane index tothe right
VkPipelinehandle.close_fexdestroys the chromaU/V variants first, then calls
vmaf_vulkan_kernel_pipeline_destroy()on the bundle.Base branch
This PR targets
refactor/vulkan-template-add-variant(#272), notmaster— it depends on the_add_variant()helper. Will retargetmasteronce #272 lands.Test plan
ninja -C build-vulkancleanmeson test test_vulkan_smoke test_vulkan_async_pending_fence test_vulkan_pic_preallocation— all greenpsnr_hvsmean 31.33 (y 30.58 / cb 37.26 / cr 38.20) across 48 framespre-commit run --files <touched>cleanSix deep-dive deliverables (ADR-0108)
no digest needed: routine refactorno alternatives: only-one-way fix(3-pipeline-per-plane shape is exactly what_add_variant()was designed for; cf. ssim_vulkan in PR refactor(vulkan): T-GPU-DEDUP-7 — migrate motion + ssim to kernel_template #272)AGENTS.mdinvariant note: seedocs/rebase-notes.mdentry 0118 — variants destroyed before bundle (same rule as ssim)### Changed🤖 Generated with Claude Code