Cherry pick PR #8962: cobalt: Move VideoGeometrySetterService on gpu thread#9284
Cherry pick PR #8962: cobalt: Move VideoGeometrySetterService on gpu thread#9284cobalt-github-releaser-bot wants to merge 1 commit into26.androidfrom
Conversation
|
Caution There were merge conflicts while cherry picking! Check out cherry-pick-26.android-8962 and fix the conflicts before proceeding. Check the log at https://github.com/youtube/cobalt/actions/runs/22469574074 for details. |
🤖 Gemini Suggested Commit Message💡 Pro Tips for a Better Commit Message:
|
d974f30 to
54e7ba8
Compare
Relocate the VideoGeometrySetterService from the browser process to the GPU process. This change instantiates the service within the CobaltContentGpuClient, allowing it to be managed directly on the GPU thread. This improves performance and reduces IPC overhead for video geometry updates by co-locating the service closer to the GPU and compositing operations. The StarboardRendererWrapper now directly subscribes to video geometry changes from this local service. Mojo communications: - Before: viz (gpu) --> browser thread --> StarboardRendererClient (media) --> StarboardRendererWrapper (gpu) - After: viz (gpu) --> StarboardRendererWrapper (gpu) Note this is called nearly per UI frame update. Issue: 405424096 (cherry picked from commit d1376b1)
54e7ba8 to
5e9e7a5
Compare
There was a problem hiding this comment.
Code Review
This pull request successfully relocates the VideoGeometrySetterService from the browser process to the GPU process, aligning with the goal of improving performance and reducing IPC overhead for video geometry updates. The changes involve removing the service from CobaltContentBrowserClient and integrating it into CobaltContentGpuClient and StarboardRendererWrapper. Dependencies and interfaces have been updated accordingly across various files. The overall approach seems sound and achieves the stated objective.
| const gfx::RectF& rect_f, | ||
| gfx::OverlayTransform /* transform */) { | ||
| DCHECK_CALLED_ON_VALID_THREAD(thread_checker_); | ||
| gfx::Rect new_bounds = gfx::ToNearestRect(rect_f); |
There was a problem hiding this comment.
The previous implementation in StarboardRendererClient::OnVideoGeometryChange used gfx::ToEnclosingRect (cobalt/renderer/cobalt_content_renderer_client.cc, line 291 in the LEFT side of the diff) to convert the floating-point rectangle to an integer rectangle. This change to gfx::ToNearestRect might subtly alter how video geometry is rounded. ToEnclosingRect ensures the entire float rectangle is contained, while ToNearestRect rounds to the closest integer rectangle, which could potentially clip a small portion of the video if the float rect is not perfectly centered. Please confirm if this change in rounding behavior is intentional and if it has been evaluated for any visual impact on video rendering.
| gfx::Rect new_bounds = gfx::ToNearestRect(rect_f); | |
| gfx::Rect new_bounds = gfx::ToEnclosingRect(rect_f); |

Refer to the original PR: #8962
Relocate the VideoGeometrySetterService from the browser process to
the GPU process. This change instantiates the service within the
CobaltContentGpuClient, allowing it to be managed directly on the GPU
thread.
This improves performance and reduces IPC overhead for video geometry
updates by co-locating the service closer to the GPU and compositing
operations. The StarboardRendererWrapper now directly subscribes to
video geometry changes from this local service.
Mojo communications:
Note this is called nearly per UI frame update.
Issue: 405424096