-
Notifications
You must be signed in to change notification settings - Fork 2k
Stream API Vulkan Backend #9164
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
@@ -455,13 +456,18 @@ void VulkanDriver::updateDescriptorSetTexture( | |||
if (UTILS_UNLIKELY(mExternalImageManager.isExternallySampledTexture(texture))) { | |||
mExternalImageManager.bindExternallySampledTexture(set, binding, texture, params); | |||
mAppState.hasBoundExternalImages = true; | |||
} else { | |||
} else if (bool(texture->getStream())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can a texture be external sampled and streamed at the same time? or are they mutually exclusive?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logically yes. But that would mean that a texture that is external was attached to a stream. And that seems like something went wrong, because from the frontend's perspective you can create a texture one of 3 ways:
-
- From a format
-
- From an external image
-
- By setting a stream
Although you could logically do 2) and then set a stream via 3, I question the meaning of that.
- By setting a stream
ExternalImageHandle UTILS_PUBLIC createExternalImage(AHardwareBuffer const* buffer, | ||
bool sRGB) noexcept; | ||
virtual ExternalImageHandle createExternalImageFromRaw(void* image, | ||
bool sRGB) noexcept override; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should just combine these methods into one called createExternalImage(void* image, bool sRGB)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this issue is that Mathias didn't want to do that.
The whole concept of ExternalImageHandleRef exists to avoid using raw pointer. Something that the Stream API totally breaks, it relies (on the GL and VK sides) on void*.
Second issue is we did a ton of changes on the Impress side to create, and send ExternalImageHandleRef (works for all external image types) so undoing this would need many Impress side changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't even understand what the void* is here? This needs to be documented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So that's because in the stream case, we call createExternalImageFromRaw insinde the driver when we process the pending stream. And inside VulkanDriver.cpp we don't know what the platform is, so we can't assume createExternalImage(AHardwareBuffer const*)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I see why you need to do this. This is because AcquiredImage has a void*. I think this is okay for now (for the sake of getting this done). But really, we should change the Acquired Stream API to only speak in terms of ExternalImageRef.
HOWEVER, I think this means that createExternalImageFromRaw() must not be public. It must be protected. And the concrete implementations must be private. This is needed only for internal machinery.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I am ok with that. Frankly, I think I agree, the stream taking void* isn't the right call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do I not make this public?
@@ -36,6 +36,34 @@ namespace filament::backend { | |||
|
|||
struct VulkanTexture; | |||
|
|||
struct VulkanStream : public HwStream, fvkmemory::Resource { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be fvkmemory::ThreadSafeResource
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I've found a way around this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What way did you find?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So first the lifetime of the resource is only managed on the frontend (ie: the backend doesnt hold resources to the stream) so we don;t need to atomic incredment of the ThreadSafeResource. Stream is pretty much like a texture or any other fvkmemory::Resource derived class.
This follows the desing pattern of GL and like GL in updateStreams we just make sure to capture and access parts of the stream that the front end doesnt touch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Backend does in fact hold all the resources. This is due to the ref-counting mechanism of the vk backend. This is not true for the GL backend. This does have to be a ThreadSafeResource
because the resource is accessed in both the backend thread and the frontend thread. And ThreadSafeResource
cannot hold references to other resources due to this
private: | ||
AcquiredImage mAcquired; | ||
AcquiredImage mPrevious; | ||
std::map<void*, fvkmemory::resource_ptr<VulkanTexture>> mTextures; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ThreadSafe resources can't have any resource_ptr
fields unfortunately. Maybe you can keep this map in VulkanStreamedImageManager
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think i've found a way around this.
virtual ImageData createVkImageFromExternal(ExternalImageHandleRef image) const { | ||
return {}; | ||
} | ||
virtual ExternalImageHandle createExternalImageFromRaw(void* image, bool sRGB) noexcept { | ||
return {}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- all virtuals in this file should not be implemented in the header file
- all need documentation, as this is the public API.
ExternalImageHandle UTILS_PUBLIC createExternalImage(AHardwareBuffer const* buffer, | ||
bool sRGB) noexcept; | ||
virtual ExternalImageHandle createExternalImageFromRaw(void* image, | ||
bool sRGB) noexcept override; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't even understand what the void* is here? This needs to be documented.
copyMat3f(bd.buffer, offset, transform); | ||
} | ||
} | ||
mStreamUniformDescriptors.erase(streamDescriptors); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we have to erase this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the logic is that the user has to submit an update to the stream descriptor every time the xform changes. Once we process it, we can remove it from the list.
ExternalImageHandle UTILS_PUBLIC createExternalImage(AHardwareBuffer const* buffer, | ||
bool sRGB) noexcept; | ||
virtual ExternalImageHandle createExternalImageFromRaw(void* image, | ||
bool sRGB) noexcept override; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I see why you need to do this. This is because AcquiredImage has a void*. I think this is okay for now (for the sake of getting this done). But really, we should change the Acquired Stream API to only speak in terms of ExternalImageRef.
HOWEVER, I think this means that createExternalImageFromRaw() must not be public. It must be protected. And the concrete implementations must be private. This is needed only for internal machinery.
// executes on the user thread) Note: stream is captured by copy which is fine, this is | ||
// a copy of a resource_ptr<VulkanStream>. We only need it find the associated stream | ||
// inside the mStreamedImageManager texture bindings | ||
driver->queueCommand([this, stream, s = stream.get(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do the s=stream.get
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it's in the loop, and the stream variable will actually change, so I think we need a unique copy of the stream pointer (access underlying data) per queueCommand
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I think stream
is a copy of the ref-counted pointer to the actual content already. You shouldn't need to do s = stream.get
(unless you pass it with &stream
, then it's a reference. But even this is ok I think).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in which case will change? Now that i look at this again, do you actually need stream and s at the same time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah so this follows the GL design pattern where it gets the underlying raw stream object and sends it to the backend (the backend only manipulates the raw data thereby ensure we don't need to share the actual VulkanStream object betwen FE and BE... I can explain this on the chat, it's a bit tricky (not great) but that's what the GL path does.
in which case will change? Now that i look at this again, do you actually need stream and s at the same time?
This is because we use the VulkanStream (not VulkanStream*)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't understand why you can't just pass stream
by copy. stream
is a pointer. It can be dereferenced to get the content (s
).
VulkanStreamedImageManager mStreamedImageManager; | ||
|
||
// Stream transforms | ||
std::unordered_map<VkBuffer, BufferObjectStreamDescriptor> mStreamUniformDescriptors; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we keep this in VulkanStreamedImageManager
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
// executes on the user thread) Note: stream is captured by copy which is fine, this is | ||
// a copy of a resource_ptr<VulkanStream>. We only need it find the associated stream | ||
// inside the mStreamedImageManager texture bindings | ||
driver->queueCommand([this, stream, s = stream.get(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I think stream
is a copy of the ref-counted pointer to the actual content already. You shouldn't need to do s = stream.get
(unless you pass it with &stream
, then it's a reference. But even this is ok I think).
// Eventually the updateSampler and updateSamplerForExternalSamplerSet | ||
// will call to vkUpdateDescriptorSets with a VkWriteDescriptorSet | ||
// type VK_DESCRIPTOR_TYPE_COMBINED_IMAGE_SAMPLER the | ||
// VkDescriptorImageInfo will contain the view of the new image frame. | ||
mDescriptorSetCache->updateSamplerForExternalSamplerSet(data.set, data.binding, | ||
image); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need this part even. This will be done in ExternalImageManager::prepareBindSets
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, I think this is right. bindExternallySampledTexture pushes into mSetBindings and we allways update all the bindings.
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -36,6 +36,34 @@ namespace filament::backend { | |||
|
|||
struct VulkanTexture; | |||
|
|||
struct VulkanStream : public HwStream, fvkmemory::Resource { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What way did you find?
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.