[ozone] Remove gfx::BufferPlane and use std::optional<int> plane_index [chromium/src : main]
Saifuddin Hitawala (Gerrit)
Saifuddin Hitawala voted and added 1 comment![]( "Open in Gerrit")
Votes added by Saifuddin Hitawala
| Commit-Queue | +1 |
1 comment
Related details
- 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. |
Vasiliy Telezhnikov (Gerrit)
Vasiliy Telezhnikov voted and added 1 comment![]( "Open in Gerrit")
Votes added by Vasiliy Telezhnikov
| Code-Review | +1 |
1 comment
- Colin Blundell
- Saifuddin Hitawala
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 5 comments![]( "Open in Gerrit")
Thanks! It's a little tricky to reason 100% that this CL doesn't have unintended side-effects - it might be better to break it up into a couple steps if feasible?
gfx::BufferPlane GetBufferPlane(viz::SharedImageFormat format,Am I seeing correctly from the rest of this CL that the code actually implicitly assumes that the config here will be kY_UV, or are there things I'm missing?
// The plane is DEFAULT for single planar formats and multi planar withnit: eliminate or rephrase this comment
std::move(native_pixmap), viz::MultiPlaneFormat::kNV12, std::nullopt,nit: /buffer_plane_index=/std::nullopt
This could also use a comment on why we're passing nullopt with NV12 - is it because we're using external sampling since this is ChromeOS? I realize that this is pre-existing.
size_t pixmap_plane = plane_ == gfx::BufferPlane::Y ? 0 : 1;Am I seeing correctly that this is the only place where we actually process the BufferPlane value, or are there other places I'm missing?
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 6 comments![]( "Open in Gerrit")
Thanks! It's a little tricky to reason 100% that this CL doesn't have unintended side-effects - it might be better to break it up into a couple steps if feasible?
There's only meaningful changes to few files as rest are plumbing. I had tried splitting it when working on it, but the nature of ImportNativePixmap being the only method using it made it so that it was easier to do it all together.
gfx::BufferPlane GetBufferPlane(viz::SharedImageFormat format,Am I seeing correctly from the rest of this CL that the code actually implicitly assumes that the config here will be kY_UV, or are there things I'm missing?
I'm not sure it is always kY_UV here, I believe OzoneImageBacking can support other plane configs like YV12 (which we had a BufferFormat for as well).
// The plane is DEFAULT for single planar formats and multi planar withnit: eliminate or rephrase this comment
Done
std::move(native_pixmap), viz::MultiPlaneFormat::kNV12, std::nullopt,nit: /buffer_plane_index=/std::nullopt
This could also use a comment on why we're passing nullopt with NV12 - is it because we're using external sampling since this is ChromeOS? I realize that this is pre-existing.
Sounds good, updated to add the comments here and in other places.
size_t pixmap_plane = plane_ == gfx::BufferPlane::Y ? 0 : 1;Am I seeing correctly that this is the only place where we actually process the BufferPlane value, or are there other places I'm missing?
Yes, this is the only place that processes BufferPlane value. I'm actually not sure why this DCHECK was there. The value here can come from GLImageProcessBackend (which is Y/UV), but also from OzoneImageGLTextureHolders which I think can be formats other than Y_UV config, like YV12.
But yes this has the unintended side-effect of earlier always being 0/1 but now it could be 2/3 as well. I think if anything it is more correct now, but open to suggestions.
Related details
- Colin Blundell
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 3 comments![]( "Open in Gerrit")
Votes added by Colin Blundell
| Code-Review | +1 |
Thanks!
gfx::BufferPlane GetBufferPlane(viz::SharedImageFormat format,Saifuddin HitawalaAm I seeing correctly from the rest of this CL that the code actually implicitly assumes that the config here will be kY_UV, or are there things I'm missing?
I'm not sure it is always kY_UV here, I believe OzoneImageBacking can support other plane configs like YV12 (which we had a BufferFormat for as well).
The comment about the usage point is what led to me making the comment here - it seems like the one actualy consumer of the `BufferPlane` value just wouldn't work correctly with non Y_UV layouts unless I'm missing something?
size_t pixmap_plane = plane_ == gfx::BufferPlane::Y ? 0 : 1;Saifuddin HitawalaAm I seeing correctly that this is the only place where we actually process the BufferPlane value, or are there other places I'm missing?
Yes, this is the only place that processes BufferPlane value. I'm actually not sure why this DCHECK was there. The value here can come from GLImageProcessBackend (which is Y/UV), but also from OzoneImageGLTextureHolders which I think can be formats other than Y_UV config, like YV12.
But yes this has the unintended side-effect of earlier always being 0/1 but now it could be 2/3 as well. I think if anything it is more correct now, but open to suggestions.
Hmm, I wonder if we want to separate that out into followup and preserve the current behavior in this CL by "gating" the plane index to 1 if it's >= 1? Just to ensure that this CL is a behavioral no-op.
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 voted and added 3 comments![]( "Open in Gerrit")
Votes added by Saifuddin Hitawala
| Commit-Queue | +2 |
3 comments
gfx::BufferPlane GetBufferPlane(viz::SharedImageFormat format,Saifuddin HitawalaAm I seeing correctly from the rest of this CL that the code actually implicitly assumes that the config here will be kY_UV, or are there things I'm missing?
Colin BlundellI'm not sure it is always kY_UV here, I believe OzoneImageBacking can support other plane configs like YV12 (which we had a BufferFormat for as well).
The comment about the usage point is what led to me making the comment here - it seems like the one actualy consumer of the `BufferPlane` value just wouldn't work correctly with non Y_UV layouts unless I'm missing something?
That makes sense. I'll make the plane_index changes in a follow-up.
size_t pixmap_plane = plane_ == gfx::BufferPlane::Y ? 0 : 1;Saifuddin HitawalaAm I seeing correctly that this is the only place where we actually process the BufferPlane value, or are there other places I'm missing?
Colin BlundellYes, this is the only place that processes BufferPlane value. I'm actually not sure why this DCHECK was there. The value here can come from GLImageProcessBackend (which is Y/UV), but also from OzoneImageGLTextureHolders which I think can be formats other than Y_UV config, like YV12.
But yes this has the unintended side-effect of earlier always being 0/1 but now it could be 2/3 as well. I think if anything it is more correct now, but open to suggestions.
Hmm, I wonder if we want to separate that out into followup and preserve the current behavior in this CL by "gating" the plane index to 1 if it's >= 1? Just to ensure that this CL is a behavioral no-op.
Sounds good, I'll keep this behaviourally consistent here, and change it in a follow-up.
Related details
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 Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
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 voted and added 2 comments![]( "Open in Gerrit")
Votes added by Colin Blundell
| Code-Review | +1 |
2 comments
Thanks!
// Gate the pixmap_plane to be either 0 or 1.nit: Expand this comment to note that we're doing this to preserve historical behavior with a TODO and bugref to eliminate it and/or verify that the flow here actually can only be Y_UV and enforce that?
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 voted and added 2 comments![]( "Open in Gerrit")
Votes added by Saifuddin Hitawala
| Commit-Queue | +2 |
Thanks for reviews!
nit: Expand this comment to note that we're doing this to preserve historical behavior with a TODO and bugref to eliminate it and/or verify that the flow here actually can only be Y_UV and enforce that?
Sounds good, updated the comment with a TODO.
Related details
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 Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
6 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: ui/ozone/common/native_pixmap_egl_binding.cc
Insertions: 5, Deletions: 1.
The diff is too large to show. Please review the diff.
```
Change information
[ozone] Remove gfx::BufferPlane and use std::optional<int> plane_index
Remove gfx::BufferPlane now that BufferFormats are gone, and update
NativePixmapGLBinding and ImportNativePixmap to use std::optional<int>
plane_index, which is unset if there is single texture (single-planar
or external sampled multiplanar) and set only for texture per plane.
- M gpu/command_buffer/service/shared_image/ozone_image_gl_textures_holder.cc
- M media/gpu/chromeos/gl_image_processor_backend.cc
- M ui/gfx/buffer_types.h
- M ui/ozone/common/gl_ozone_egl.cc
- M ui/ozone/common/gl_ozone_egl.h
- M ui/ozone/common/native_pixmap_egl_binding.cc
- M ui/ozone/common/native_pixmap_egl_binding.h
- M ui/ozone/demo/skia/skia_surfaceless_gl_renderer.cc
- M ui/ozone/demo/surfaceless_gl_renderer.cc
- M ui/ozone/gl/native_pixmap_gl_binding_unittest.cc
- M ui/ozone/platform/drm/gpu/gbm_surface_factory.cc
- M ui/ozone/platform/wayland/gpu/wayland_surface_factory.cc
- M ui/ozone/platform/x11/x11_surface_factory.cc
- M ui/ozone/public/gl_ozone.h
- M ui/ozone/public/native_pixmap_gl_binding.h
Code-Review: +1 by Colin Blundell, +1 by Vasiliy Telezhnikov
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |