Repository navigation
feat: Converter push descriptors and fixes - #43
Merged
hgaiser merged 7 commits intoOct 18, 2026
Merged
Conversation
The converter binds two descriptors, a sampled source image and an output buffer, and rewrites both on every dispatch. It did that through VK_EXT_descriptor_buffer, which is now deprecated in favour of VK_EXT_descriptor_heap, and which was a heavy tool for two descriptors: it needed bufferDeviceAddress, descriptorBufferCaptureReplay, a mapped host buffer, per-binding offset queries, device-address memory, and a pipeline flag that was missing until hgaiser#40 added it. Push descriptors write both straight into the command buffer. There is no descriptor memory, no device addresses and nothing to enable beyond VK_KHR_push_descriptor, which every desktop driver has had for years and which is core in Vulkan 1.4. Descriptor heaps would be the replacement for a large, long-lived descriptor set; this is not one. The shorter feature list matters most for a caller that hands pixelforge its own device, which would otherwise have to turn on descriptor buffers and capture-replay just so the converter can bind two descriptors. Removing the descriptor-buffer block also fixes two features that had ended up inside it by accident: samplerYcbcrConversion and the 2-plane 4:4:4 formats were only enabled on devices that also had descriptor buffers, even though the 4:4:4 extension itself was enabled regardless. BREAKING CHANGE: VideoContext::has_descriptor_buffer is replaced by VideoContext::has_push_descriptor. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The colour conversion tests built their BGRA source image, and the one-shot submission and host buffer helpers behind it, inline. A test of the converter replacing its source image needs that image built from pixels of its own choosing, so these move to tests/common, with the frame size and pixels passed in rather than taken from the conversion test's constants. No behaviour change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The converter kept one image view and reused it whenever the next source had the same VkImage handle. A handle only identifies an image while it exists: drivers hand a new image a destroyed one's handle, and RADV does so readily. A caller that destroyed its source after each frame and created the next got every frame after the first converted from a view of the first, freed image. On RADV that meant a garbage picture from the second frame on, with nothing reporting an error. The validation layer gives every object a unique handle, so it hides this entirely, and none of the existing tests replaced a source image. The new conversion test does, and fails on RADV without this change. A view is now made for each conversion and destroyed once that conversion has finished. Creating a view is cheap next to a frame's dispatch and copy, and this way nothing survives the image it was made from. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The barrier moving the setup slot into VIDEO_ENCODE_DPB_KHR had an empty destination access mask. Its execution dependency held, but no memory dependency did: the encode's write of the reconstructed picture was outside the barrier's second scope, so the transition and that write were unordered. Synchronization validation reports it as a write-after-write hazard in vkCmdEncodeVideoKHR, once per DPB slot on first use, on RADV and ANV alike. The DPB barriers now use synchronization2, which has video encode stages and access flags, in place of the original barrier API, which has neither and could only say ALL_COMMANDS and MEMORY_*. Every one is from the encode stage to the encode stage. The setup slot's destination is encode read and write, which is the fix. Its source is now the encode's write as well, where it was empty. When a slot is reused, the earlier encode's write to it has to be ordered before the new one. Today the queue's signal operation between the two submissions already does that, and synchronization validation, with submit-time checks on, reports nothing either way on RADV or ANV. Naming the write keeps the barrier correct without leaning on that. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
pixelforge sends the validation layer's findings to tracing. roundtrip, rfi and verify_all install a subscriber; color_conversion did not, so with validation on its messages went nowhere, and a run of it printing nothing proved nothing. tests/common gains init_logging(), a tracing subscriber at RUST_LOG's level (warn when unset) writing through the test harness, and color_conversion calls it before building a context. Run with --nocapture to see a passing test's output. The first run with it shows what had been hidden: the source image is described to the converter in a layout it is no longer in, fixed in the next commit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The converter returns its source image to GENERAL after every conversion. The tests created their source in SHADER_READ_ONLY_OPTIMAL and kept saying so on every call, so every conversion after the first read through a descriptor written for a layout the image was no longer in. Descriptor buffers were never tracked for that; push descriptors are, and the validation layer said so 12 times in color_conversion. The source now starts in GENERAL and every call says GENERAL. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
CHANGELOG covers the move to push descriptors, the features that had been enabled only alongside descriptor buffers, the converter's stale source view and the DPB barrier. AGENTS.md says that integration tests need common::init_logging() for their validation messages to go anywhere, and why the ignored tests need a run without validation too: the layer gives every object a unique handle, which hid the converter's stale view. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Owner
|
I tried it on my system, it mostly seems to work except for Regardless, this PR looks good to me, so thanks! |
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.
Part 1/3 of the #41 split, with some small refinements and fixes.
Tested on RX 9060 XT and Arc A310. @hgaiser if you got time to test on your NVIDIA GPU, I'd appreciate that 🙏 - all the usable cloud GPUs were taken so couldn't test myself 😅
This split PR doesn't touch decoding, so if it's still broken, that's a separate bug I'll have to fix, maybe NVIDIA drivers made some changes in that case or became more strict? 🤔