Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[media] Set the bounds of Sbplayer via video_painter.cc #4385

Merged
merged 1 commit into from
Nov 15, 2024
Merged

[media] Set the bounds of Sbplayer via video_painter.cc #4385

merged 1 commit into from
Nov 15, 2024

Conversation

borongc
Copy link
Contributor

@borongc borongc commented Nov 6, 2024 •
edited
Loading

SbPlayer needs SbPlayerSetBounds to update the bounds. This PR updates the bounds in blink via video_painter.cc for SbPlayer.

Unlike Cobalt, Chromium updates the bounds of video frequently, resulting in multiple calls of SbPlayerSetBounds per video session for SbPlayer. We should revisit it if the multiple calls of SbPlayerSetBounds cause performance issues.

b/376320224

@borongc borongc requested a review from a team as a code owner November 6, 2024 22:25
@borongc borongc requested a review from briantting November 6, 2024 22:25
@borongc borongc force-pushed the android_sbplayer_bound branch 3 times, most recently from f3c2698 to 36d51af Compare November 6, 2024 22:42
@borongc borongc requested review from sideb0ard, jasonzhangxx, xiaomings and osagie98 and removed request for briantting November 6, 2024 22:43
@borongc borongc force-pushed the android_sbplayer_bound branch 8 times, most recently from fba14f7 to 7402087 Compare November 11, 2024 19:40
@borongc borongc force-pushed the android_sbplayer_bound branch 2 times, most recently from 001b361 to bb12930 Compare November 15, 2024 00:13
@borongc borongc force-pushed the android_sbplayer_bound branch from bb12930 to 7111333 Compare November 15, 2024 00:26
@borongc borongc enabled auto-merge (squash) November 15, 2024 00:26
kaidokert
kaidokert approved these changes Nov 15, 2024
View reviewed changes
Copy link
Member

@kaidokert kaidokert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm from 3p / upstream modifications perspective

@borongc
[media] Set the bounds of Sbplayer via video_painter.cc
b00e647 
SbPlayer needs SbPlayerSetBounds to update the bounds. This PR updates the bounds in blink via video_painter.cc for SbPlayer.

Unlike Cobalt, Chromium upupdates the bounds of video frequently, resulting in multiple calls per video session for SbPlayer. We should revisit it if SbPlayerSetBounds causes performance issues.

b/376320224
@borongc borongc force-pushed the android_sbplayer_bound branch from 7111333 to b00e647 Compare November 15, 2024 17:24
@borongc borongc merged commit 9b6a247 into youtube:main Nov 15, 2024
54 of 55 checks passed
@borongc borongc deleted the android_sbplayer_bound branch December 9, 2024 19:47
@borongc borongc mentioned this pull request Feb 4, 2025
Merged
borongc added a commit that referenced this pull request Feb 15, 2025
@borongc
Add VideoGeometrySetter Service for Cobalt (#4810)
10d4851 
StarboardRender needs to be informed with the video geometry information
from the display compositor. VideoGeometrySetter provides the IPC
between the StarobardRenderer and the display compositor so the video
geometry information can reach StarboardRenderer.

This refers to
https://chromium-review.googlesource.com/c/chromium/src/+/1799692 with
the following modifications:
- VideoGeometrySetterService is in Renderer thread, and it is exposed to
ContentBrowserClient.
- ContentBrowserClient binds VideoGeometrySetterService after receiving
mojo::PendingRemote\<cobalt::media::mojom::VideoGeometrySetter\> from
compositing thread (viz) to Renderer thread.
- ContentRendererClient creates a custom StarboardRendererFactory, which
allows to bind |overlay_plane_id| for each StarboardRenderer with
VideoGeometrySetterService.

This CL also cleans up the old implementations for setting video bounds:
#4385, because it is unnecessary
with this PR.

b/391938746
borongc added a commit to borongc/cobalt that referenced this pull request Feb 18, 2025
@borongc
Add VideoGeometrySetter Service for Cobalt (youtube#4810)
691284e 
StarboardRender needs to be informed with the video geometry information
from the display compositor. VideoGeometrySetter provides the IPC
between the StarobardRenderer and the display compositor so the video
geometry information can reach StarboardRenderer.

This refers to
https://chromium-review.googlesource.com/c/chromium/src/+/1799692 with
the following modifications:
- VideoGeometrySetterService is in Renderer thread, and it is exposed to
ContentBrowserClient.
- ContentBrowserClient binds VideoGeometrySetterService after receiving
mojo::PendingRemote\<cobalt::media::mojom::VideoGeometrySetter\> from
compositing thread (viz) to Renderer thread.
- ContentRendererClient creates a custom StarboardRendererFactory, which
allows to bind |overlay_plane_id| for each StarboardRenderer with
VideoGeometrySetterService.

This CL also cleans up the old implementations for setting video bounds:
youtube#4385, because it is unnecessary
with this PR.

b/391938746
@borongc borongc mentioned this pull request Feb 18, 2025
Closed
borongc added a commit to borongc/cobalt that referenced this pull request Feb 18, 2025
@borongc
Reland "Add VideoGeometrySetter Service for Cobalt (youtube#4810)"
76d4507 
This is a reland of 10d4851

This CL fixed DCHECK failed caused by the PR (youtube#4810) and list the fixes below (b/397413189):
- Removed `DCHECK(content::RenderThread::IsMainThread());` in CobaltContentRendererClient due to CobaltContentRendererClient is not in Render Thread.
- Moved the order of `DCHECK(video_geometry_change_subcriber_remote_);` after `video_geometry_setter_service_->GetVideoGeometryChangeSubscriber` in StarboardRenderer.
- Changed `gfx::ToNearestRect()` to `gfx::ToEnclosingRect()` in StarboardRenderer.

Original change's description:
> Add VideoGeometrySetter Service for Cobalt (youtube#4810)
>
> StarboardRender needs to be informed with the video geometry information
> from the display compositor. VideoGeometrySetter provides the IPC
> between the StarobardRenderer and the display compositor so the video
> geometry information can reach StarboardRenderer.
>
> This refers to
> https://chromium-review.googlesource.com/c/chromium/src/+/1799692 with
> the following modifications:
> - VideoGeometrySetterService is in Renderer thread, and it is exposed to
> ContentBrowserClient.
> - ContentBrowserClient binds VideoGeometrySetterService after receiving
> mojo::PendingRemote\<cobalt::media::mojom::VideoGeometrySetter\> from
> compositing thread (viz) to Renderer thread.
> - ContentRendererClient creates a custom StarboardRendererFactory, which
> allows to bind |overlay_plane_id| for each StarboardRenderer with
> VideoGeometrySetterService.
>
> This CL also cleans up the old implementations for setting video bounds:
> youtube#4385, because it is unnecessary
> with this PR.
>
> b/391938746
@borongc borongc mentioned this pull request Feb 18, 2025
Merged
borongc added a commit to borongc/cobalt that referenced this pull request Feb 18, 2025
@borongc
Reland "Add VideoGeometrySetter Service for Cobalt (youtube#4810)"
78a1e9d 
This is a reland of 10d4851

This CL fixed DCHECK failed caused by the PR (youtube#4810) and list the fixes below (b/397413189):
- Removed `DCHECK(content::RenderThread::IsMainThread());` in CobaltContentRendererClient due to CobaltContentRendererClient is not in Render Thread.
- Moved the order of `DCHECK(video_geometry_change_subcriber_remote_);` after `video_geometry_setter_service_->GetVideoGeometryChangeSubscriber` in StarboardRenderer.
- Changed `gfx::ToNearestRect()` to `gfx::ToEnclosingRect()` in StarboardRenderer.

Original change's description:
> Add VideoGeometrySetter Service for Cobalt (youtube#4810)
>
> StarboardRender needs to be informed with the video geometry information
> from the display compositor. VideoGeometrySetter provides the IPC
> between the StarobardRenderer and the display compositor so the video
> geometry information can reach StarboardRenderer.
>
> This refers to
> https://chromium-review.googlesource.com/c/chromium/src/+/1799692 with
> the following modifications:
> - VideoGeometrySetterService is in Renderer thread, and it is exposed to
> ContentBrowserClient.
> - ContentBrowserClient binds VideoGeometrySetterService after receiving
> mojo::PendingRemote\<cobalt::media::mojom::VideoGeometrySetter\> from
> compositing thread (viz) to Renderer thread.
> - ContentRendererClient creates a custom StarboardRendererFactory, which
> allows to bind |overlay_plane_id| for each StarboardRenderer with
> VideoGeometrySetterService.
>
> This CL also cleans up the old implementations for setting video bounds:
> youtube#4385, because it is unnecessary
> with this PR.
>
> b/391938746
borongc added a commit to borongc/cobalt that referenced this pull request Feb 18, 2025
@borongc
Reland "Add VideoGeometrySetter Service for Cobalt (youtube#4810)"
d165e88 
This is a reland of 10d4851

This CL fixed DCHECK failed caused by the PR (youtube#4810) and list the fixes below (b/397413189):
- Removed `DCHECK(content::RenderThread::IsMainThread());` in CobaltContentRendererClient due to CobaltContentRendererClient is not in Render Thread.
- Moved the order of `DCHECK(video_geometry_change_subcriber_remote_);` after `video_geometry_setter_service_->GetVideoGeometryChangeSubscriber` in StarboardRenderer.
- Changed `gfx::ToNearestRect()` to `gfx::ToEnclosingRect()` in StarboardRenderer.

Original change's description:
> Add VideoGeometrySetter Service for Cobalt (youtube#4810)
>
> StarboardRender needs to be informed with the video geometry information
> from the display compositor. VideoGeometrySetter provides the IPC
> between the StarobardRenderer and the display compositor so the video
> geometry information can reach StarboardRenderer.
>
> This refers to
> https://chromium-review.googlesource.com/c/chromium/src/+/1799692 with
> the following modifications:
> - VideoGeometrySetterService is in Renderer thread, and it is exposed to
> ContentBrowserClient.
> - ContentBrowserClient binds VideoGeometrySetterService after receiving
> mojo::PendingRemote\<cobalt::media::mojom::VideoGeometrySetter\> from
> compositing thread (viz) to Renderer thread.
> - ContentRendererClient creates a custom StarboardRendererFactory, which
> allows to bind |overlay_plane_id| for each StarboardRenderer with
> VideoGeometrySetterService.
>
> This CL also cleans up the old implementations for setting video bounds:
> youtube#4385, because it is unnecessary
> with this PR.
>
> b/391938746
borongc added a commit to borongc/cobalt that referenced this pull request Feb 19, 2025
@borongc
Reland "Add VideoGeometrySetter Service for Cobalt (youtube#4810)"
464300d 
This is a reland of 10d4851

This CL fixed DCHECK failed caused by the PR (youtube#4810) and list the fixes below (b/397413189):
- Removed `DCHECK(content::RenderThread::IsMainThread());` in CobaltContentRendererClient due to CobaltContentRendererClient is not in Render Thread.
- Moved the order of `DCHECK(video_geometry_change_subcriber_remote_);` after `video_geometry_setter_service_->GetVideoGeometryChangeSubscriber` in StarboardRenderer.
- Changed `gfx::ToNearestRect()` to `gfx::ToEnclosingRect()` in StarboardRenderer.

Original change's description:
> Add VideoGeometrySetter Service for Cobalt (youtube#4810)
>
> StarboardRender needs to be informed with the video geometry information
> from the display compositor. VideoGeometrySetter provides the IPC
> between the StarobardRenderer and the display compositor so the video
> geometry information can reach StarboardRenderer.
>
> This refers to
> https://chromium-review.googlesource.com/c/chromium/src/+/1799692 with
> the following modifications:
> - VideoGeometrySetterService is in Renderer thread, and it is exposed to
> ContentBrowserClient.
> - ContentBrowserClient binds VideoGeometrySetterService after receiving
> mojo::PendingRemote\<cobalt::media::mojom::VideoGeometrySetter\> from
> compositing thread (viz) to Renderer thread.
> - ContentRendererClient creates a custom StarboardRendererFactory, which
> allows to bind |overlay_plane_id| for each StarboardRenderer with
> VideoGeometrySetterService.
>
> This CL also cleans up the old implementations for setting video bounds:
> youtube#4385, because it is unnecessary
> with this PR.
>
> b/391938746
borongc added a commit that referenced this pull request Feb 19, 2025
@borongc
Reland "Add VideoGeometrySetter Service for Cobalt (#4810)" (#4921)
f19998d 
This is a reland of 10d4851

This CL fixed DCHECK failed caused by the PR
(#4810) and list the fixes below
(b/397413189):
- Removed `DCHECK(content::RenderThread::IsMainThread());` in
CobaltContentRendererClient due to CobaltContentRendererClient is not in
Render Thread.
- Moved the order of `DCHECK(video_geometry_change_subcriber_remote_);`
after `video_geometry_setter_service_->GetVideoGeometryChangeSubscriber`
in StarboardRenderer.
- Changed `gfx::ToNearestRect()` to `gfx::ToEnclosingRect()` in
StarboardRenderer.

Original change's description:
> Add VideoGeometrySetter Service for Cobalt (#4810)
>
> StarboardRender needs to be informed with the video geometry
information
> from the display compositor. VideoGeometrySetter provides the IPC
> between the StarobardRenderer and the display compositor so the video
> geometry information can reach StarboardRenderer.
>
> This refers to
> https://chromium-review.googlesource.com/c/chromium/src/+/1799692 with
> the following modifications:
> - VideoGeometrySetterService is in Renderer thread, and it is exposed
to
> ContentBrowserClient.
> - ContentBrowserClient binds VideoGeometrySetterService after
receiving
> mojo::PendingRemote\<cobalt::media::mojom::VideoGeometrySetter\> from
> compositing thread (viz) to Renderer thread.
> - ContentRendererClient creates a custom StarboardRendererFactory,
which
> allows to bind |overlay_plane_id| for each StarboardRenderer with
> VideoGeometrySetterService.
>
> This CL also cleans up the old implementations for setting video
bounds:
> #4385, because it is unnecessary
> with this PR.
>
> b/391938746
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasonzhangxx jasonzhangxx jasonzhangxx approved these changes

@kaidokert kaidokert kaidokert approved these changes

@osagie98 osagie98 osagie98 approved these changes

@johnxwork johnxwork johnxwork approved these changes

@sideb0ard sideb0ard Awaiting requested review from sideb0ard

@xiaomings xiaomings Awaiting requested review from xiaomings

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@borongc @jasonzhangxx @kaidokert @osagie98 @johnxwork