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 vif_vulkan to use the shared Vulkan kernel-template pipeline abstraction and _add_variant() helper, reducing duplicated pipeline-management state while preserving the existing per-scale specialization behavior.
Changes:
- Replaced manually managed Vulkan pipeline/layout/shader/descriptor-pool state in
vif_vulkan.cwithVmafVulkanKernelPipelineplus three sibling scale variants. - Added a
vif_scale_pipeline()accessor and updated descriptor set / pipeline binding sites to use the new template-owned resources. - Documented the migration and destruction-order invariant in
docs/rebase-notes.mdandCHANGELOG.md.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
libvmaf/src/feature/vulkan/vif_vulkan.c |
Migrates VIF Vulkan pipeline creation, variant handling, binding, and teardown to the shared kernel-template abstraction. |
docs/rebase-notes.md |
Records the rebase/migration details and the destroy-variants-before-bundle invariant. |
CHANGELOG.md |
Adds a changelog entry describing the Vulkan refactor and expected unchanged numerical behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
0cad1f3 to
af40030
Compare
b9d69cd to
3207dff
Compare
af40030 to
2ed2ec8
Compare
Multi-pipeline-via-variant migration on top of PR #272. State collapses dsl + pipeline_layout + shader + desc_pool + pipelines[4] to VmafVulkanKernelPipeline pl + VkPipeline scale_variants[3]. Scale 0 (luma) is the template's base pipeline; scales 1, 2, 3 are sibling pipelines via _add_variant() — same layout, shader, DSL, pool, different SCALE spec-constant. New vif_scale_pipeline() accessor maps scale index to the right VkPipeline handle. close_fex destroys the 3 scale variants first, then calls vmaf_vulkan_kernel_pipeline_destroy() on the bundle (same rule as ssim_vulkan in T-GPU-DEDUP-7 and psnr_hvs_vulkan in T-GPU-DEDUP-18). Validated against the Netflix-pair smoke (integer_vif_scale0..3 means 0.364, 0.767, 0.863, 0.916, integer_vif mean 0.446 across 48 frames). 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 0119) updated per ADR-0108 deep-dive deliverables. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
3207dff to
34eb584
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
Multi-pipeline-via-variant migration on top of PR #272 (which
adds the
vmaf_vulkan_kernel_pipeline_add_variant()helper).State collapses
dsl + pipeline_layout + shader + desc_pool + pipelines[4]toVmafVulkanKernelPipeline pl + VkPipeline scale_variants[3]._add_variant()—same layout, shader, DSL, pool, different
SCALEspec-constant.
New
vif_scale_pipeline()accessor maps scale index to theright
VkPipelinehandle.close_fexdestroys the 3 scalevariants first, then calls
vmaf_vulkan_kernel_pipeline_destroy()on the bundle (sameinvariant as ssim_vulkan in T-GPU-DEDUP-7 and psnr_hvs_vulkan in
T-GPU-DEDUP-18).
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-vulkancleaninteger_vif_scale0..3means 0.364, 0.767, 0.863, 0.916;integer_vifmean 0.446 across 48 framespre-commit run --files <touched>cleanSix deep-dive deliverables (ADR-0108)
no digest needed: routine refactorno alternatives: only-one-way fixAGENTS.mdinvariant note: seedocs/rebase-notes.mdentry 0119 — variants destroyed before bundle (same rule as ssim/psnr_hvs)### Changed🤖 Generated with Claude Code