Stream API Vulkan Backend #9164
Conversation
| VulkanStreamedImageManager mStreamedImageManager; | ||
|
|
||
| // Stream transforms | ||
| std::unordered_map<VkBuffer, BufferObjectStreamDescriptor> mStreamUniformDescriptors; |
There was a problem hiding this comment.
can we keep this in VulkanStreamedImageManager ?
There was a problem hiding this comment.
I would rather not. It feels more nature for the stream to hold this data. It also means that once we have the stream we can access all the stream images. Since this object is not a Thread safe resource, it seems sensible?
But if you feel strongly about it, I can move it to the VulkanStreamedImageManager, that's one more level of indirection where we need a to map<VkBuffer, BufferObjectStreamDescriptor> inside the VulkanStreamedImageManager
| // For some reason, some of the frames coming to us, are on streams where the | ||
| // descriptor set isn't external... | ||
| if (data.set->getExternalSamplerVkSet()) { | ||
| if (newImage) { |
There was a problem hiding this comment.
I think we can always just call bindExternallySampledTexture even if it's not a newImage ?
But we should probably add some logic in bindExternallSampledTexture to remove an entry if it shares the same data.set and data.binding. (Basically it's overwriting the existing binding for a descriptor set).
There was a problem hiding this comment.
I don't think so, we get these calls every frame, which means we will keep pushing int the binding vector and it will grow unbounded.
There was a problem hiding this comment.
You'll need to add logic in bindExternallySampledTexture to remove bindings to the same set and set binding.
| // For some reason, some of the frames coming to us, are on streams where the | ||
| // descriptor set isn't external... | ||
| if (data.set->getExternalSamplerVkSet()) { | ||
| if (newImage) { |
There was a problem hiding this comment.
You'll need to add logic in bindExternallySampledTexture to remove bindings to the same set and set binding.
| if (data.image->getStream() == stream) { | ||
| // For some reason, some of the frames coming to us, are on streams where the | ||
| // descriptor set isn't external... | ||
| if (data.set->getExternalSamplerVkSet()) { |
There was a problem hiding this comment.
I think this needs to be image->getVkFormat() == VK_FORMAT_UNDEFINED that indicates the texture has a externally sampled format. The ExternalSamplerVkSet will only be created if the bindExternallySampledTexture goes through.
There was a problem hiding this comment.
Nice catch!!
You are right, if the new acquiredImage needs an external sampler then bind it, otherwise just update the sampler
| commands.acquire(bo); | ||
| bo->loadFromCpu(commands, bd.buffer, byteOffset, bd.size); | ||
|
|
||
| if (UTILS_UNLIKELY(!mStreamUniformDescriptors.empty())) { |
There was a problem hiding this comment.
shouldnt the copyMat3f happen before bo->loadFromCpu?
right now its copying the current bd.buffer to the gpu memory and then its updating the transform
There was a problem hiding this comment.
Yes that's right, good catch.
| if (data.image->getStream() == stream) { | ||
| // For some reason, some of the frames coming to us, are on streams where the | ||
| // descriptor set isn't external... | ||
| if (data.image->getVkFormat() == VK_FORMAT_UNDEFINED) { |
There was a problem hiding this comment.
so im pretty sure here, you have to create a new descriptor set and copy the contents otherwise you might update one currently in flight
like every time, you acquire a new image, we need a new descriptor set regardless if the the sampler changed or not
There was a problem hiding this comment.
and that's actually what this PR is doing. So you might want to wait for it to land and then rebase.
There was a problem hiding this comment.
but even if the stream is not using an external sampler, you would still have to create a new descriptor set, which from what I understand, that PR wont help
Also in the case its also using the same colorspace but the texture is different, you would also require a new descriptor set because you want to update the entry but not change the one currently in flight.
There was a problem hiding this comment.
actually, you're right. You'll need to make a new descriptor for the non-externally-sampled case.
There was a problem hiding this comment.
OK so I think I might have lost track of all the temporary DS.... And I think a lot of cracks were found in the current ext sampler implementations. I am fine focusing on #9251 and then re-evaluating how to manage this for the stream path (in this PR)
This is the implemenation of the Stream API on the Vulkan Backend. We leverage most of the work that was done by the external image manager, but we created a different manager to contain the code.
Tested on Moohan (manual merge, so some things could have gotten lost in the process). Also working on validating this on Android.