[ozone] Avoid gating plane EGLImage pixmap attrs for per-plane textures [chromium/src : main]
Saifuddin Hitawala (Gerrit)
Saifuddin Hitawala added 2 comments![]( "Open in Gerrit")
if (pixmap_plane > 1) {Related details
- Colin Blundell
- 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. |
Vasiliy Telezhnikov (Gerrit)
Vasiliy Telezhnikov voted and added 2 comments![]( "Open in Gerrit")
Votes added by Vasiliy Telezhnikov
| Code-Review | +1 |
2 comments
lgtm, thanks.
if (pixmap_plane > 1) {I do believe it's no op and so it's okay to remove. On CrOS we use per plane access only to write and we only ever write to [NV12](https://source.chromium.org/chromium/chromium/src/+/main:media/video/renderable_mappable_shared_image_video_frame_pool.cc;drc=a8525686784b4ea1794e537d6e025cca88c7d76f;l=191) from multiplane formats.
Whether it will work for other formats or not is a good question, I don't know implementation details inside the driver out of my head, but it was added just for this specific use-case [here](https://chromium-review.googlesource.com/c/chromium/src/+/3194113), so I'd assume it will work, we just didn't want to write more code for conversion from BufferPlane to an index.
Related details
- Colin Blundell
- 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. |
Colin Blundell (Gerrit)
Colin Blundell voted and added 2 comments![]( "Open in Gerrit")
Votes added by Colin Blundell
| Code-Review | +1 |
2 comments
Thanks!
attrs.push_back(EGL_DMA_BUF_PLANE0_FD_EXT);definitely outside of scope of this CL, but is this code correct? Naively it would seem like it should be PLANE<n> throughout.
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. |
Vasiliy Telezhnikov (Gerrit)
Vasiliy Telezhnikov added 1 comment![]( "Open in Gerrit")
attrs.push_back(EGL_DMA_BUF_PLANE0_FD_EXT);definitely outside of scope of this CL, but is this code correct? Naively it would seem like it should be PLANE<n> throughout.
That's how it works, on the driver level we just pretend it's a single plane (so we pass only plane 0), but we path dma-buf+offset of the plane we want.
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 reviews!
attrs.push_back(EGL_DMA_BUF_PLANE0_FD_EXT);Vasiliy Telezhnikovdefinitely outside of scope of this CL, but is this code correct? Naively it would seem like it should be PLANE<n> throughout.
That's how it works, on the driver level we just pretend it's a single plane (so we pass only plane 0), but we path dma-buf+offset of the plane we want.
That makes sense, thanks for clarifying!
if (pixmap_plane > 1) {Vasiliy Telezhnikov@vas...@chromium.org Do you think its okay remove this conditional?
I do believe it's no op and so it's okay to remove. On CrOS we use per plane access only to write and we only ever write to [NV12](https://source.chromium.org/chromium/chromium/src/+/main:media/video/renderable_mappable_shared_image_video_frame_pool.cc;drc=a8525686784b4ea1794e537d6e025cca88c7d76f;l=191) from multiplane formats.
Whether it will work for other formats or not is a good question, I don't know implementation details inside the driver out of my head, but it was added just for this specific use-case [here](https://chromium-review.googlesource.com/c/chromium/src/+/3194113), so I'd assume it will work, we just didn't want to write more code for conversion from BufferPlane to an index.
Okay, so it was introduced for NV12 and from current code paths it can also still be NV12. Sounds good, I'll remove it since it is no-op and thanks for clarifying!
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![]( "Open in Gerrit")
Change information
[ozone] Avoid gating plane EGLImage pixmap attrs for per-plane textures
Avoid gating NativePixmap attributes for per-plane textures to 0/1
when creating an EGLImage in NativePixmapEGLBinding. This is a
follow-up from [1] where earlier it was always gated to be 0/1 based
on assumption that it is always Y_UV plane config. This change
updates it so that other plane configs also work as expected, possibly
coming from OzoneImageBacking.
[1] https://chromium-review.googlesource.com/c/chromium/src/+/7613869/6
- M ui/ozone/common/native_pixmap_egl_binding.cc
Code-Review: +1 by Vasiliy Telezhnikov, +1 by Colin Blundell
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Colin Blundell (Gerrit)
Colin Blundell added 1 comment![]( "Open in Gerrit")
attrs.push_back(EGL_DMA_BUF_PLANE0_FD_EXT);Vasiliy Telezhnikovdefinitely outside of scope of this CL, but is this code correct? Naively it would seem like it should be PLANE<n> throughout.
Saifuddin HitawalaThat's how it works, on the driver level we just pretend it's a single plane (so we pass only plane 0), but we path dma-buf+offset of the plane we want.
That makes sense, thanks for clarifying!
Thanks! I couldn't immediately find out: Where in the codebase do we put the data into the pixmaps that get processed here?
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 added 1 comment![]( "Open in Gerrit")
attrs.push_back(EGL_DMA_BUF_PLANE0_FD_EXT);Vasiliy Telezhnikovdefinitely outside of scope of this CL, but is this code correct? Naively it would seem like it should be PLANE<n> throughout.
Saifuddin HitawalaThat's how it works, on the driver level we just pretend it's a single plane (so we pass only plane 0), but we path dma-buf+offset of the plane we want.
Colin BlundellThat makes sense, thanks for clarifying!
Thanks! I couldn't immediately find out: Where in the codebase do we put the data into the pixmaps that get processed here?
It's created on the clients like for OzoneImageBackingFactory over [here](https://source.chromium.org/chromium/chromium/src/+/main:gpu/command_buffer/service/shared_image/ozone_image_backing_factory.cc;drc=84e9f8cb8ba9621be941c81af04d33df743f7de4;l=143) but not sure where we put data into it. Maybe some other parts of ozone?
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |