Stream API Vulkan Backend #9164
Conversation
| 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)
In filament, once a descriptor set is bound, we no longer make updates to it. However, this guarrantee was broken in the external sampler/image because a colorspace change might necessitate a layout change, and thereby a new descriptor needs to be generated with the appropriate externally sampled image updated in the new set. Another use case is the Stream API, where each frame we might be getting a different AHardwareBuffer, meaning we need to update one or multiple sampler bindings in a set that might have been bound in a previous frame. In this change, we always create a new set when there's a change in the image currently bound to an existing set (while accounting for whether we need to use a new externally sampled layout or not).
… the streamed texture manager. The issue is that all textures are external, not all are using external format. But it's more robust to let the external format image manager rely on the AHB* to decide if the format is external or not. Regardless (because the memory is external) we need to send this to the external image manager.
phoenixxxx
force-pushed
the
streamed_texture
branch
from
078441d to
44a85a6
Compare
4 participants
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.