[//gpu] Have SIFactory create and own GMBFactory [chromium/src : main]
Vasiliy Telezhnikov (Gerrit)
Vasiliy Telezhnikov added 2 comments![]( "Open in Gerrit")
Sorry for the delay on this, it's kinda tricky.
To keep this rolling, maybe we could:
- Land the follow-up CL first (essentially inline creation into SharedImageManager
- Do something wonky where we move GMBFactory to SIFactory only for non-windows
- Merge non-windows factories out
It's not that clean, but kinda moves us in the right direction while we're sorting out windows part. Essentially we need to sort out copy-native-to-shmem. What do you think?
gpu_memory_buffer_factory_(gpu::GpuMemoryBufferFactory::CreateNativeType(The side effect of this is that we'd create new d3d device for each SharedImageFactory, which could be quite a bit, especially because we never destroy staging textures.
We might need to either add code to clean those up or to extract device/staging texture in a separate helper class that is shared across GMB factories. Or find a way to use main D3D device, but that's a bit complicated due to threads.
Related details
- Colin Blundell
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Colin Blundell (Gerrit)
Colin Blundell added 3 comments![]( "Open in Gerrit")
Thanks! I figured that you were wrestling with a nuance about the CL here, which is why I didn't invest in building further on it today :).
Sorry for the delay on this, it's kinda tricky.
To keep this rolling, maybe we could:
- Land the follow-up CL first (essentially inline creation into SharedImageManager
- Do something wonky where we move GMBFactory to SIFactory only for non-windows
- Merge non-windows factories out
It's not that clean, but kinda moves us in the right direction while we're sorting out windows part. Essentially we need to sort out copy-native-to-shmem. What do you think?
Your point in the comment makes a ton of sense. I would add to it that as you're mentioning the non-Windows code is essentially trivial and IMO there's no real reason it needs to be abstracted through the backing factories via virtual methods (that kind of creates a misleading impression that there's some sort of dynamic choice going on, which is not actually what's happening). What if we did this, building on your suggestion:
- Extracted the GMBFactoryNativePixmap and GMBFactoryIOSurface method bodies into static methods called by those factory methods
- Added static methods on the respective SI backing factories that call those methods
- Changed SIFactory to call those static methods and use GMBFactory only on Windows
- Mutate GMBFactoryDXGI into a helper class that is still created/owned by SIManager (or alternatively a singleton internal to SIFactory) and have SIFactory delegate to that for implementing the GMBHandle-related methods on Windows?
I'm open to other suggestions.
gpu_memory_buffer_factory_(gpu::GpuMemoryBufferFactory::CreateNativeType(The side effect of this is that we'd create new d3d device for each SharedImageFactory, which could be quite a bit, especially because we never destroy staging textures.
We might need to either add code to clean those up or to extract device/staging texture in a separate helper class that is shared across GMB factories. Or find a way to use main D3D device, but that's a bit complicated due to threads.
Thanks, that makes a ton of sense and indeed I hadn't thought of that nuance.
Related details
- Vasiliy Telezhnikov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vasiliy Telezhnikov (Gerrit)
Vasiliy Telezhnikov added 1 comment![]( "Open in Gerrit")
Colin BlundellSorry for the delay on this, it's kinda tricky.
To keep this rolling, maybe we could:
- Land the follow-up CL first (essentially inline creation into SharedImageManager
- Do something wonky where we move GMBFactory to SIFactory only for non-windows
- Merge non-windows factories out
It's not that clean, but kinda moves us in the right direction while we're sorting out windows part. Essentially we need to sort out copy-native-to-shmem. What do you think?
Your point in the comment makes a ton of sense. I would add to it that as you're mentioning the non-Windows code is essentially trivial and IMO there's no real reason it needs to be abstracted through the backing factories via virtual methods (that kind of creates a misleading impression that there's some sort of dynamic choice going on, which is not actually what's happening). What if we did this, building on your suggestion:
- Extracted the GMBFactoryNativePixmap and GMBFactoryIOSurface method bodies into static methods called by those factory methods
- Added static methods on the respective SI backing factories that call those methods
- Changed SIFactory to call those static methods and use GMBFactory only on Windows
- Mutate GMBFactoryDXGI into a helper class that is still created/owned by SIManager (or alternatively a singleton internal to SIFactory) and have SIFactory delegate to that for implementing the GMBHandle-related methods on Windows?
I'm open to other suggestions.
Something like that sounds good to me.
We generally need a plan copy-native-to-shmem, especially as we're about to add it to android as it heavily intersects with CopyToGpuMemoryBuffer functionality on SII and with CompoundImageBacking.
As far as I know creating D3D textures is fine on any thread (D3D11Device is thread-safe in that manner unless created with SINGLETHREADED flag, which we do only for this GMB thingy), but doing copy would require access to ImmediateContext which is not thread-safe.
It's also debatable how much it makes sense to make a gpu readback on the IO-thread, as the thread is supposed to be never busy to handle IPC in a timely manner.
Related details
- Colin Blundell
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |