Repository navigation
feat: adopted device encoding with fixes - #45
Merged
Merged
Conversation
A context adopted from a caller's device took queue 0 of every family it needed. On most hardware queue 0 of the transfer and compute families is the caller's own graphics queue, and a VkQueue may not be submitted to from two threads at once. A renderer that kept drawing while pixelforge decoded on another thread was racing it on its own queue, with no way to avoid it short of a lock around every submission on both sides. The builder now takes a DeviceQueue (family and index) per role, so the caller can create spare queues and hand those over. Each choice is checked against what the role needs and against how many queues the family offers; whether the caller actually created that many is not something Vulkan reports, so that part stays a documented requirement. DeviceRequirements now also says which family serves which role, so the caller knows where a spare queue is worth creating, and whether the device supports VK_KHR_internally_synchronized_queues. That extension is the other way out, on hardware with no queue to spare: a queue created with its flag may be shared, the driver doing the locking. Pixelforge needs no part in it, so it is reported rather than required. decode_adopted now keeps queue 0 to itself wherever a family has a second queue, which also puts the non-zero-index path under test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Pixelforge called vkQueueSubmit2, vkCmdPipelineBarrier2 and vkCmdCopyImage2 through their core entry points, which exist only when the instance asked for Vulkan 1.3. A device adopted from an application that asked for 1.1 or 1.2 has them null, and ash turns the first call into a panic. Nothing in the adopt API said 1.3 was required, and an application sharing its device has no reason to raise its API version for a library. The sync2 commands now go through VK_KHR_synchronization2's own table, which is valid on any device with that extension enabled; pixelforge required the extension already. The one image copy uses vkCmdCopyImage, since it needs nothing the newer entry point adds. The decode requirements now list VK_KHR_timeline_semaphore as well, for the same reason: on a 1.1 device the timeline semaphores the decoder relies on do not exist without it. The new adopted-device test builds a Vulkan 1.1 instance and device the way such an application would, hands pixelforge spare queues, and checks every decoded frame against a context pixelforge created itself on the same GPU. It needs no ffmpeg. Without this change it panics on the first barrier. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Decoding could already run on a device the caller created; encoding could not, so an application that renders on its own device and wants to encode the result had to give pixelforge a device of its own and move every frame across. That means exporting each image as external memory, importing it on the other device, and waiting on the CPU between the two, since two devices share no timeline a semaphore could use. encode_device_requirements() and build_from_existing_encode() are the encode counterparts of the decode pair. The requirements cover the encoder and the colour converter: the video encode extensions for the codecs asked for (or every codec the device encodes, if none were), push descriptors for the converter, and the 2-plane 4:4:4 formats where the device has them. Features are now reported too, in DeviceRequirements::features, as booleans rather than a pNext chain. A caller merging them into its own device may already chain VkPhysicalDeviceVulkan13Features, and Vulkan forbids chaining that alongside the per-feature structs it contains, so pixelforge names the bits and leaves the structs to the caller. The queue-family scan the decode path used is shared with encode rather than copied, and the adopted-device test gains an encode case: a Vulkan 1.1 device, spare queues, RGB frames through the converter into the encoder, and the stream checked against one encoded by a context pixelforge created itself. encode_adopted is the example counterpart of decode_adopted. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ColorConverter::convert submitted its dispatch and then waited on a fence, and Encoder::encode had no way to wait for anything the caller submitted. That was harmless when every frame crossed from another device anyway, since that crossing needed a CPU wait of its own. On a shared device it is the last CPU round trip left in the frame: the caller's copy, the conversion and the encode can all be ordered by semaphores instead. TimelinePoint names a timeline semaphore value. convert_async() waits for the points it is given, submits, and returns the point its own timeline reaches when the conversion is done; encode_after() makes the frame's first submission, the upload copy if there is one and otherwise the encode, wait for the points it is given. So a frame goes caller -> convert -> encode with each step waiting on the previous on the GPU. convert() and encode() are now those with nothing to wait for, and convert() still blocks until done, so existing callers see no difference. External waits hold back ALL_COMMANDS rather than a narrower stage: pixelforge's submissions open with layout transitions the caller cannot see, and a wait that let those run early would race the caller's writes. The converter used to wait for its fence right after submitting; it now waits at the start of the next conversion instead, before the command buffer, output buffer or source view are reused or destroyed. The adopted-device encode test now takes this path: each conversion waits on a point the host signals only after submitting it, and each encode on the conversion. On RADV the result is bit-identical to the synchronous path on a context pixelforge created itself. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
An adopted context takes every feature DeviceRequirements::features sets as enabled, since Vulkan cannot be asked afterwards which were. encode_adopted built its device by hand and left out videoEncodeAV1, so on a device that encodes AV1 the context claimed a feature the device never turned on: the kind of mismatch where the driver parses the structs and ignores them, which surfaces as wrong output rather than an error. The example only encodes H.264, which is why nothing failed. It now sets every feature the requirements ask for, and says why that matters. The adopted-device test already did. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Both adopted-device tests now call common::init_logging(), so with validation on their messages reach the test output instead of nowhere. The encode test also describes its source image as GENERAL, the layout the shared test source starts in and the converter leaves it in. It still said SHADER_READ_ONLY_OPTIMAL, from before the colour conversion tests moved to GENERAL, which the validation layer reports for every conversion now that the converter uses push descriptors. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The crate docs gain a section on encoding on the caller's own device: what to create it with, why pixelforge should get queues of its own, and how a frame is ordered from the caller's rendering through conversion to encode. The features list mentions shared devices, and the examples list gains encode_adopted. README.md is regenerated from them. CHANGELOG covers the additions and the lower Vulkan version an adopted device may use. AGENTS.md says what the adopted-device test checks and when to run it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
one_shot, which uploads the synthetic source image and reads back what the converter wrote, ran on the transfer queue. Both images are the converter's, used on its compute queue, and created for one queue family. On RADV and ANV the transfer and compute queues the tests get share a family, so this went unnoticed. NVIDIA has a dedicated transfer family: there the upload's final barrier asked for shader read access on a queue with no shader stages, which the validation layer reports for every source image, and the image then moved to the compute family with no ownership handover. The tests passed anyway, but on no guarantee. one_shot now records and submits on the compute queue, the family every image it touches belongs to. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A ColorConverter destroyed its timeline semaphore when dropped, after waiting for its own last conversion. But the points convert_async hands out are waited on by other submissions, an encode_after first of all, and those can still be pending when the conversion they wait for is done. Dropping the converter between submitting such an encode and the encode running destroyed a semaphore a queue was still waiting on, which Vulkan leaves undefined. The converter now hands its semaphore to the context, which destroys it when it goes itself. Every encoder holds the context and waits for its queues when dropped, so by then nothing of pixelforge's waits on it. A caller's own submission waiting on such a point has the same rule, which TimelinePoint now states. The adopted-device encode test now drops its converter while the last encode is held back on a point signalled only afterwards. Without this change that hangs the GPU on ANV; with it the test passes and validation is quiet. The in_flight comment also stops mentioning a cached source view, which the converter no longer has. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Two ways the encode requirements could disagree with the device the caller then builds. Codecs were checked on the encode family the scan picked, while a caller passing with_encode_queue may choose another. The encode queue is now settled first, the caller's choice included, and codecs and timestamp support are checked on its family. ycbcr2plane444Formats and videoEncodeAV1 were reported as features to enable whenever their extensions were present. A driver may offer an extension with its feature off, and a caller enabling it as told gets VK_ERROR_FEATURE_NOT_PRESENT. Both bits are now queried, AV1 counts as supported only with its feature on, and the 4:4:4 formats only with theirs. Checking for an extension is now one helper, DeviceExtensions, which enumerates once rather than per question. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
DeviceRequirements, DeviceFeatures and QueueRoles are only ever built by pixelforge, and each gains fields as pixelforge learns to use more of a device. Without #[non_exhaustive] every such field is a breaking change for a caller that builds or exhaustively destructures one. BREAKING CHANGE: DeviceRequirements, DeviceFeatures and QueueRoles can no longer be built with a struct literal or matched without `..` outside pixelforge. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
encode_after hands its waits to the frame's first submission. For the encoder's own input image that is the encode, and nothing waits on the CPU. For any other image it is the copy into the input image, which the encoder waits for on the CPU before recording, so the call blocks until the caller's work is done. The docs promised no CPU wait without saying which case that held for. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
decode_adopted handed pixelforge spare transfer and compute queues but left decode on queue 0 of its family, while encode_adopted hands over a spare encode queue. An application that also uses the decode family and copied the example would share that queue with pixelforge across threads. It now hands over a spare decode queue as well, which also puts with_decode_queue under test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
hgaiser
approved these changes
Oct 19, 2026
hgaiser
left a comment
hgaiser
left a comment
Owner
There was a problem hiding this comment.
Thanks! Looks good to me. I removed the adopted examples since I don't consider it necessary to demonstrate the core usage of this crate.
Other than that, looks good 👍
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 2/3 of the #41 split, again with fixes and improvements compared to original PR.
encode_adoptedshows usage example.with_xxx_queuemethods to allow applications to choose where sbmits go, can help with performance when rendering queue is separate from encoding/decoding given ones (so they won't race each-other or slow each-other down).ColorConverter::convert_asyncandEncoder::encode_aftermethods for more efficient GPU-driven work rather waiting on CPU. Useful in cases where adopted device comes from application that can't be controlled (i.e. hooking into a game as a Vulkan layer).Resolves #35
Tested on RX 9060 XT, Arc A310 and additionally cloud-rented RTX 6000 Ada with both 580 and 615 drivers.