[ozone][gpu] Use SharedImageFormat in DrmModifiersFilter [chromium/src : main]
Saifuddin Hitawala (Gerrit)
Saifuddin Hitawala added 1 comment![]( "Open in Gerrit")
Related details
- Kramer Ge
- Vasiliy Telezhnikov
Code-Coverage
Code-Owners
Code-Review
| 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
- Kramer Ge
- Saifuddin Hitawala
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kramer Ge (Gerrit)
Kramer Ge added 3 comments![]( "Open in Gerrit")
Sorry for the delay.
#include "components/viz/common/resources/shared_image_format.h"nit: this is a dup incl of header
class SharedImageFormat;this should not be a forward declaration.
Related details
- Saifuddin Hitawala
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Saifuddin Hitawala (Gerrit)
Saifuddin Hitawala added 3 comments![]( "Open in Gerrit")
Address some feedback, also fixed setting PrefersExternalSampler as that was missing earlier.
#include "components/viz/common/resources/shared_image_format.h"nit: this is a dup incl of header
Sounds good, done.
class SharedImageFormat;this should not be a forward declaration.
There's some chromium [guidance](https://chromium.googlesource.com/chromium/src/+/HEAD/styleguide/c++/c++.md#forward-declarations-vs_includes) that states forward declarations are preferred although there was some work done around IWYU so I'm not sure.
I'll keep it as is for now, as adding the header leads to adding to DEPS.
Related details
- Kramer Ge
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kramer Ge (Gerrit)
Kramer Ge voted and added 2 comments![]( "Open in Gerrit")
Votes added by Kramer Ge
| Code-Review | +1 |
2 comments
specific_include_rules = {Wondering if there are motives to make `shared_image_format.h` more widely-available.
class SharedImageFormat;Saifuddin Hitawalathis should not be a forward declaration.
There's some chromium [guidance](https://chromium.googlesource.com/chromium/src/+/HEAD/styleguide/c++/c++.md#forward-declarations-vs_includes) that states forward declarations are preferred although there was some work done around IWYU so I'm not sure.
I'll keep it as is for now, as adding the header leads to adding to DEPS.
Yes, although it is a pass-by-value in declaration so the compiler needs to know the size of `SharedImageFormat` when it sees a `Filter()` definition, which forces every definition to perform the `include`. In reality, the compiler didn't complain is because the actual definitions always include `shared_image_format.h`.
I find no clear instructions on whether this is a good/bad practice, doing this declaration is just the same spirit to using [callback_forward.h](https://chromium.googlesource.com/chromium/src/+/HEAD/base/functional/callback_forward.h) to avoid inclusion headaches.
Related details
- Saifuddin Hitawala
- 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. |
Saifuddin Hitawala (Gerrit)
Saifuddin Hitawala voted and added 3 comments![]( "Open in Gerrit")
Votes added by Saifuddin Hitawala
| Commit-Queue | +2 |
3 comments
Thanks for reviewing!
specific_include_rules = {Wondering if there are motives to make `shared_image_format.h` more widely-available.
Eventually we kinda want to use it in ui/ozone directly but trying to tackle usages other than ui/ozone for now. Or we might want to have NativePixmapFormat which will be scoped to ui/ozone similar to NativePixmapUsage, if we cannot use SharedImageFormat in ui/ozone for some reason. The primary motivation is to get rid of BufferFormat as that still represents something related to GMBs and there can be confusion on client-end since GMBs are handled by shared images now because of MappableSI.
I did add it to BUILD file but for some reason gn check complained that include rules were missing and needed this.
class SharedImageFormat;Saifuddin Hitawalathis should not be a forward declaration.
Kramer GeThere's some chromium [guidance](https://chromium.googlesource.com/chromium/src/+/HEAD/styleguide/c++/c++.md#forward-declarations-vs_includes) that states forward declarations are preferred although there was some work done around IWYU so I'm not sure.
I'll keep it as is for now, as adding the header leads to adding to DEPS.
Yes, although it is a pass-by-value in declaration so the compiler needs to know the size of `SharedImageFormat` when it sees a `Filter()` definition, which forces every definition to perform the `include`. In reality, the compiler didn't complain is because the actual definitions always include `shared_image_format.h`.
I find no clear instructions on whether this is a good/bad practice, doing this declaration is just the same spirit to using [callback_forward.h](https://chromium.googlesource.com/chromium/src/+/HEAD/base/functional/callback_forward.h) to avoid inclusion headaches.
Right yeah, I guess in this case doing forward declaration doesn't make much sense.
I can look more into it in follow-ups as I will likely need to handle ui/ozone DEPs again as most future conversions will involve ozone.
Related details
- Vasiliy Telezhnikov
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[ozone][gpu] Use SharedImageFormat in DrmModifiersFilter
As part of BufferFormat to SharedImageFormat conversions, convert
DrmModifiersFilter and its subclasses to use SharedImageFormat instead
of BufferFormat. Also update relevant callsites to use
SharedImageFormat.
- M gpu/command_buffer/service/drm_modifiers_filter_dawn.cc
- M gpu/command_buffer/service/drm_modifiers_filter_dawn.h
- M gpu/command_buffer/service/drm_modifiers_filter_vulkan.cc
- M gpu/command_buffer/service/drm_modifiers_filter_vulkan.h
- M gpu/command_buffer/service/drm_modifiers_filter_vulkan_unittest.cc
- M ui/ozone/platform/drm/BUILD.gn
- M ui/ozone/platform/drm/gpu/DEPS
- M ui/ozone/platform/drm/gpu/hardware_display_controller.cc
- M ui/ozone/platform/drm/gpu/mock_drm_modifiers_filter.cc
- M ui/ozone/platform/drm/gpu/mock_drm_modifiers_filter.h
- M ui/ozone/platform/wayland/gpu/wayland_buffer_manager_gpu.cc
- M ui/ozone/public/drm_modifiers_filter.h
Code-Review: +1 by Kramer Ge, +1 by Vasiliy Telezhnikov
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |