[gpu] Remove HasEquivalentBufferFormat and its checks [chromium/src : main]
Saifuddin Hitawala (Gerrit)
Saifuddin Hitawala added 1 comment![]( "Open in Gerrit")
Related details
- Chromium IPC Reviews
- Colin Blundell
- Vasiliy Telezhnikov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: joenot...@google.com
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): joenot...@google.com
Reviewer source(s):
joenot...@google.com is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Colin Blundell
- Joe Mason
- Vasiliy Telezhnikov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joe Mason (Gerrit)
Joe Mason voted Code-Review+1![]( "Open in Gerrit")
- Colin Blundell
- Saifuddin Hitawala
- Vasiliy Telezhnikov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vasiliy Telezhnikov (Gerrit)
Vasiliy Telezhnikov voted and added 1 comment![]( "Open in Gerrit")
Votes added by Vasiliy Telezhnikov
| Code-Review | +1 |
1 comment
lgtm, thanks.
Related details
- Colin Blundell
- Saifuddin Hitawala
Colin Blundell (Gerrit)
Colin Blundell voted and added 2 comments![]( "Open in Gerrit")
Votes added by Colin Blundell
| Code-Review | +1 |
2 comments
Thanks!
// The format must be mappable shared image format at the least.I think we can just leave this as it was? i.e. the current setup here is clearly assuming that subclasses are going to be the ones meaningfully implementing this method, which means they should all either be (a) overriding it or (b) content with it always returning true. I think it makes sense to leave that in place (and not take the risk of changing behavior for any subclasses that are in the case of (b)).
Related details
- Saifuddin Hitawala
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Saifuddin Hitawala (Gerrit)
Saifuddin Hitawala added 1 comment![]( "Open in Gerrit")
// The format must be mappable shared image format at the least.I think we can just leave this as it was? i.e. the current setup here is clearly assuming that subclasses are going to be the ones meaningfully implementing this method, which means they should all either be (a) overriding it or (b) content with it always returning true. I think it makes sense to leave that in place (and not take the risk of changing behavior for any subclasses that are in the case of (b)).
This was used from OzoneImageBackingFactory::IsSupported (only caller) which also checked for HasEquivalentBufferFormat. I was chatting with Vasiliy about this, and since that check is now removed from IsSupported we thought it okay to move it here (also since there was a convenient list of formats here :P). It is only overridden for X11SurfaceFactory and no other ozone platforms. I guess X11surfaceFactory probably does the right thing by querying it from system so it does not need this check.
I'm also open to leaving it as it was and just return true as well. I think `gl_ozone->CanImportNativePixmap(format)` in OzoneImageBackingFactory should do the right thing and check if it can import a native pixmap of this format? CanImportNativePixmap eventually ends up [here](https://source.chromium.org/chromium/chromium/src/+/main:ui/ozone/common/native_pixmap_egl_binding.cc;drc=5a0f078b076a30e11d492e3babe4af0d9680d31d;l=54) which basically checks for same format list through FourCC format conversion. In which case we can then remove `CanImportNativePixmap` completely as it is only callsite here, maybe in a follow-up.
Related details
- Colin Blundell
Saifuddin Hitawala (Gerrit)
Saifuddin Hitawala added 2 comments![]( "Open in Gerrit")
Addressed feedback, but lost +1s. Please take another look.
// The format must be mappable shared image format at the least.Saifuddin HitawalaI think we can just leave this as it was? i.e. the current setup here is clearly assuming that subclasses are going to be the ones meaningfully implementing this method, which means they should all either be (a) overriding it or (b) content with it always returning true. I think it makes sense to leave that in place (and not take the risk of changing behavior for any subclasses that are in the case of (b)).
This was used from OzoneImageBackingFactory::IsSupported (only caller) which also checked for HasEquivalentBufferFormat. I was chatting with Vasiliy about this, and since that check is now removed from IsSupported we thought it okay to move it here (also since there was a convenient list of formats here :P). It is only overridden for X11SurfaceFactory and no other ozone platforms. I guess X11surfaceFactory probably does the right thing by querying it from system so it does not need this check.
I'm also open to leaving it as it was and just return true as well. I think `gl_ozone->CanImportNativePixmap(format)` in OzoneImageBackingFactory should do the right thing and check if it can import a native pixmap of this format? CanImportNativePixmap eventually ends up [here](https://source.chromium.org/chromium/chromium/src/+/main:ui/ozone/common/native_pixmap_egl_binding.cc;drc=5a0f078b076a30e11d492e3babe4af0d9680d31d;l=54) which basically checks for same format list through FourCC format conversion. In which case we can then remove `CanImportNativePixmap` completely as it is only callsite here, maybe in a follow-up.
I like the idea of keeping it as is and then just removing `CanCreateNativePixmapForFormat` as `CanImportNativePixmap` should handle it correctly. Updated to remove changes in this file.
Related details
- Colin Blundell
- Joe Mason
- Vasiliy Telezhnikov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Colin Blundell (Gerrit)
Colin Blundell added 2 comments![]( "Open in Gerrit")
if (!factory->CanCreateNativePixmapForFormat(format)) {Hmm, is this not a subtly different check now? The current code is checking the result of this method *only if* `HasEquivalentBufferFormat(format)` is true.
// The format must be mappable shared image format at the least.Saifuddin HitawalaI think we can just leave this as it was? i.e. the current setup here is clearly assuming that subclasses are going to be the ones meaningfully implementing this method, which means they should all either be (a) overriding it or (b) content with it always returning true. I think it makes sense to leave that in place (and not take the risk of changing behavior for any subclasses that are in the case of (b)).
Saifuddin HitawalaThis was used from OzoneImageBackingFactory::IsSupported (only caller) which also checked for HasEquivalentBufferFormat. I was chatting with Vasiliy about this, and since that check is now removed from IsSupported we thought it okay to move it here (also since there was a convenient list of formats here :P). It is only overridden for X11SurfaceFactory and no other ozone platforms. I guess X11surfaceFactory probably does the right thing by querying it from system so it does not need this check.
I'm also open to leaving it as it was and just return true as well. I think `gl_ozone->CanImportNativePixmap(format)` in OzoneImageBackingFactory should do the right thing and check if it can import a native pixmap of this format? CanImportNativePixmap eventually ends up [here](https://source.chromium.org/chromium/chromium/src/+/main:ui/ozone/common/native_pixmap_egl_binding.cc;drc=5a0f078b076a30e11d492e3babe4af0d9680d31d;l=54) which basically checks for same format list through FourCC format conversion. In which case we can then remove `CanImportNativePixmap` completely as it is only callsite here, maybe in a follow-up.
I like the idea of keeping it as is and then just removing `CanCreateNativePixmapForFormat` as `CanImportNativePixmap` should handle it correctly. Updated to remove changes in this file.
Ah I see, thanks for explaining the full context! My apologies, I had only looked at //ui. I actually now have another question (see the other comment).
Related details
- Joe Mason
- Saifuddin Hitawala
- Vasiliy Telezhnikov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Saifuddin Hitawala (Gerrit)
Saifuddin Hitawala added 1 comment![]( "Open in Gerrit")
if (!factory->CanCreateNativePixmapForFormat(format)) {Hmm, is this not a subtly different check now? The current code is checking the result of this method *only if* `HasEquivalentBufferFormat(format)` is true.
Yeah, now that I think about it `CanImportNativePixmap` is only with `used_by_gl` and we will still need `CanCreateNativePixmapForFormat` at least for X11SurfaceFactory. I'll add back the check in surface_factory_ozone.
Related details
- Colin Blundell
- Joe Mason
- Vasiliy Telezhnikov
Colin Blundell (Gerrit)
Colin Blundell added 1 comment![]( "Open in Gerrit")
if (!factory->CanCreateNativePixmapForFormat(format)) {Saifuddin HitawalaHmm, is this not a subtly different check now? The current code is checking the result of this method *only if* `HasEquivalentBufferFormat(format)` is true.
Yeah, now that I think about it `CanImportNativePixmap` is only with `used_by_gl` and we will still need `CanCreateNativePixmapForFormat` at least for X11SurfaceFactory. I'll add back the check in surface_factory_ozone.
I'm not sure that's quite correct though, unless I'm missing something - it seems to me that the current code will run through this check very happily if the SI format doesn't have an equivalent BufferFormat, whereas the new code would fail out in that case? Am I reading this wrong?
Related details
- Joe Mason
- Saifuddin Hitawala
- Vasiliy Telezhnikov
Saifuddin Hitawala (Gerrit)
Saifuddin Hitawala added 1 comment![]( "Open in Gerrit")
if (!factory->CanCreateNativePixmapForFormat(format)) {Saifuddin HitawalaHmm, is this not a subtly different check now? The current code is checking the result of this method *only if* `HasEquivalentBufferFormat(format)` is true.
Colin BlundellYeah, now that I think about it `CanImportNativePixmap` is only with `used_by_gl` and we will still need `CanCreateNativePixmapForFormat` at least for X11SurfaceFactory. I'll add back the check in surface_factory_ozone.
I'm not sure that's quite correct though, unless I'm missing something - it seems to me that the current code will run through this check very happily if the SI format doesn't have an equivalent BufferFormat, whereas the new code would fail out in that case? Am I reading this wrong?
So currently on TOT, we have check for `HasEquivalentBufferFormat` in OzoneImageBackingFactory::IsSupported. This CL initially removed that check from IsSupported, but also added that check to `CanCreateNativePixmapForFormat` which is used alongside HasEquivalentBufferFormat in IsSupported.
It's a bit complex because of the negations/bools, but earlier it meant "does this format have an equivalent buffer format but we cannot create native pixmap for format (for x11) then return false" and now "if we cannot create native pixmap for this format based on list (or same check for x11) then return false". So it should mean the same thing (once I add back check removed in PS4) I believe unless ... I also missed something.
Related details
- Colin Blundell
- Joe Mason
- Vasiliy Telezhnikov
Colin Blundell (Gerrit)
Colin Blundell added 1 comment![]( "Open in Gerrit")
if (!factory->CanCreateNativePixmapForFormat(format)) {Saifuddin HitawalaHmm, is this not a subtly different check now? The current code is checking the result of this method *only if* `HasEquivalentBufferFormat(format)` is true.
Colin BlundellYeah, now that I think about it `CanImportNativePixmap` is only with `used_by_gl` and we will still need `CanCreateNativePixmapForFormat` at least for X11SurfaceFactory. I'll add back the check in surface_factory_ozone.
Saifuddin HitawalaI'm not sure that's quite correct though, unless I'm missing something - it seems to me that the current code will run through this check very happily if the SI format doesn't have an equivalent BufferFormat, whereas the new code would fail out in that case? Am I reading this wrong?
So currently on TOT, we have check for `HasEquivalentBufferFormat` in OzoneImageBackingFactory::IsSupported. This CL initially removed that check from IsSupported, but also added that check to `CanCreateNativePixmapForFormat` which is used alongside HasEquivalentBufferFormat in IsSupported.
It's a bit complex because of the negations/bools, but earlier it meant "does this format have an equivalent buffer format but we cannot create native pixmap for format (for x11) then return false" and now "if we cannot create native pixmap for this format based on list (or same check for x11) then return false". So it should mean the same thing (once I add back check removed in PS4) I believe unless ... I also missed something.
What I'm seeing is that before the check would succeed if the format didn't have an equivalent buffer format whereas now it will fail in that case (as of PS3), no?
Related details
- Joe Mason
- Saifuddin Hitawala
- Vasiliy Telezhnikov
Saifuddin Hitawala (Gerrit)
Saifuddin Hitawala voted and added 2 comments![]( "Open in Gerrit")
Votes added by Saifuddin Hitawala
| Commit-Queue | +1 |
2 comments
Updated the patch to keep OzoneBacking as it is and look to removing the check there in a follow-up. Please take another look, thanks.
if (!factory->CanCreateNativePixmapForFormat(format)) {Saifuddin HitawalaHmm, is this not a subtly different check now? The current code is checking the result of this method *only if* `HasEquivalentBufferFormat(format)` is true.
Colin BlundellYeah, now that I think about it `CanImportNativePixmap` is only with `used_by_gl` and we will still need `CanCreateNativePixmapForFormat` at least for X11SurfaceFactory. I'll add back the check in surface_factory_ozone.
Saifuddin HitawalaI'm not sure that's quite correct though, unless I'm missing something - it seems to me that the current code will run through this check very happily if the SI format doesn't have an equivalent BufferFormat, whereas the new code would fail out in that case? Am I reading this wrong?
Colin BlundellSo currently on TOT, we have check for `HasEquivalentBufferFormat` in OzoneImageBackingFactory::IsSupported. This CL initially removed that check from IsSupported, but also added that check to `CanCreateNativePixmapForFormat` which is used alongside HasEquivalentBufferFormat in IsSupported.
It's a bit complex because of the negations/bools, but earlier it meant "does this format have an equivalent buffer format but we cannot create native pixmap for format (for x11) then return false" and now "if we cannot create native pixmap for this format based on list (or same check for x11) then return false". So it should mean the same thing (once I add back check removed in PS4) I believe unless ... I also missed something.
What I'm seeing is that before the check would succeed if the format didn't have an equivalent buffer format whereas now it will fail in that case (as of PS3), no?
You are very much correct. This would just go down to other checks if there is no equivalent buffer format. This should have been `!viz::HasEquivalentBufferFormat || !factory->CanCreateNativePixmapForFormat(format)` and then if we plumbed it down to SurfaceFactoryOzone it would mean the same. Vasiliy and I looked at blame and I added it for some reason [here](http://chromium-review.googlesource.com/c/chromium/src/+/5199133) :D
To decouple this mess from this CL, I've added back the check and `HasEquivalentBufferFormat(format)` in anonymous namespace and look to removing this check in a follow-up.
Related details
- Colin Blundell
- Joe Mason
- Vasiliy Telezhnikov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joe Mason (Gerrit)
Joe Mason voted Code-Review+1![]( "Open in Gerrit")
- Colin Blundell
- Saifuddin Hitawala
- Vasiliy Telezhnikov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Colin Blundell (Gerrit)
Colin Blundell voted and added 1 comment![]( "Open in Gerrit")
Votes added by Colin Blundell
| Code-Review | +1 |
1 comment
if (!factory->CanCreateNativePixmapForFormat(format)) {Saifuddin HitawalaHmm, is this not a subtly different check now? The current code is checking the result of this method *only if* `HasEquivalentBufferFormat(format)` is true.
Colin BlundellYeah, now that I think about it `CanImportNativePixmap` is only with `used_by_gl` and we will still need `CanCreateNativePixmapForFormat` at least for X11SurfaceFactory. I'll add back the check in surface_factory_ozone.
Saifuddin HitawalaI'm not sure that's quite correct though, unless I'm missing something - it seems to me that the current code will run through this check very happily if the SI format doesn't have an equivalent BufferFormat, whereas the new code would fail out in that case? Am I reading this wrong?
Colin BlundellSo currently on TOT, we have check for `HasEquivalentBufferFormat` in OzoneImageBackingFactory::IsSupported. This CL initially removed that check from IsSupported, but also added that check to `CanCreateNativePixmapForFormat` which is used alongside HasEquivalentBufferFormat in IsSupported.
It's a bit complex because of the negations/bools, but earlier it meant "does this format have an equivalent buffer format but we cannot create native pixmap for format (for x11) then return false" and now "if we cannot create native pixmap for this format based on list (or same check for x11) then return false". So it should mean the same thing (once I add back check removed in PS4) I believe unless ... I also missed something.
Saifuddin HitawalaWhat I'm seeing is that before the check would succeed if the format didn't have an equivalent buffer format whereas now it will fail in that case (as of PS3), no?
You are very much correct. This would just go down to other checks if there is no equivalent buffer format. This should have been `!viz::HasEquivalentBufferFormat || !factory->CanCreateNativePixmapForFormat(format)` and then if we plumbed it down to SurfaceFactoryOzone it would mean the same. Vasiliy and I looked at blame and I added it for some reason [here](http://chromium-review.googlesource.com/c/chromium/src/+/5199133) :D
To decouple this mess from this CL, I've added back the check and `HasEquivalentBufferFormat(format)` in anonymous namespace and look to removing this check in a follow-up.
- Saifuddin Hitawala
- Vasiliy Telezhnikov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Saifuddin Hitawala (Gerrit)
Saifuddin Hitawala voted and added 1 comment![]( "Open in Gerrit")
Votes added by Saifuddin Hitawala
| Commit-Queue | +2 |
1 comment
- Vasiliy Telezhnikov
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[gpu] Remove HasEquivalentBufferFormat and its checks
The CHECKs and some of the conditionals are mostly remnants of
BufferFormat conversions and can be removed.
Note: The only place left is OzoneImageBackingFactory
which I'll handle in a follow-up.
- M components/exo/buffer.cc
- M components/viz/common/resources/shared_image_format_utils.cc
- M components/viz/common/resources/shared_image_format_utils.h
- M gpu/command_buffer/client/internal/mappable_buffer_dxgi.cc
- M gpu/command_buffer/client/internal/mappable_buffer_native_pixmap.cc
- M gpu/command_buffer/client/internal/mappable_buffer_shared_memory.cc
- M gpu/command_buffer/client/shared_image_interface.cc
- M gpu/command_buffer/service/drm_modifiers_filter_vulkan.cc
- M gpu/command_buffer/service/shared_image/compound_image_backing.cc
- M gpu/command_buffer/service/shared_image/ozone_image_backing_factory.cc
- M gpu/command_buffer/service/shared_image/shared_image_factory.cc
- M gpu/command_buffer/service/shared_image/shared_memory_image_backing_factory.cc
- M gpu/command_buffer/service/shared_memory_region_wrapper.cc
- M gpu/ipc/client/client_shared_image_interface.cc
- M gpu/ipc/service/gpu_channel.cc
- M media/capture/video/chromeos/camera_buffer_factory.cc
- M ui/gfx/native_pixmap_handle.cc
Code-Review: +1 by Joe Mason, +1 by Colin Blundell
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |