Merge Ozone prototype commits into main #4727

madhurajayaraman · 2025-01-21T19:32:36Z

Had weird merge conflicts in #4726 so cherry-picked relevant changes instead.

b/391414243

…outube#4654) This reverts commit cc55e87.

b/371272304

…ube#4688) Move ownership of SbWindow to PlatformWindowStarboard, allowing us to get the proper initialization size and assign it as the platform window in the platform window delegate. b/388348504

Swap to using Angle instead of native GLES to avoid devel/debug builds crashing. b/390499263

Mappings from kSbKeyFoo to ui::DomCode::Bar .

andrewsavage1 · 2025-01-22T18:18:21Z

gpu/config/gpu_util.cc

This is the only thing I see here that we should really avoid (and looks avoidable considering it's feature-gated)

TyHolc

There's a few hacks left in there. Some might be removable with the move to Angle, but do we want to merge the others over?

Also in a couple spots we should put TODO comments to make sure we revisit those sections.

TyHolc · 2025-01-22T18:16:57Z

gpu/config/gpu_util.cc

-  if (blocklisted_features.count(GPU_FEATURE_TYPE_ACCELERATED_GL))
-    return kGpuFeatureStatusBlocklisted;
+  // TODO(cobalt, b/371272304): Re-enable
+  // if (blocklisted_features.count(GPU_FEATURE_TYPE_ACCELERATED_GL))


This was one of the hacks we didn't really want to pull in. We might not need this anymore with the switch over to using Angle, but we'll need to check.

Let's change it to #if !BUILDFLAG(IS_STARBOARD) instead of commenting it out.

Let's please remove it entirely instead of using a buildflag—it's already gated by feature flags, we should just use those if we still need to (which we should be able to do from cobalt-owned code)

I dug into this today. I couldn't find a flag to target GPU_FEATURE_TYPE_ACCELERATED_GL specifically, but one might exist. There is a switch to ignore the gpu blocklist but that caused a separate crash (likely we do want the other features to remain blocked). In that same file there's a switch for blocklist test groups that enable/disable different sets of features. There may be a specific group we could set but I didn't find the test group definitions.

While backtracking from that feature setting I found that the blocklist is created here using default values. The default values are values for software rendering and from a generated file, out/linux-x64x11_devel/gen/gpu/config/software_rendering_list_autogen.cc, which is based on a template file. So maybe we can configure it there, but it seems to target devices very specifically and I don't know if we can apply a blanket rule of if_starboard -> enable accelerated gl/disable software compositing.

I did find that the only place it seems to matter for the crash is in the gpu_init. Setting the feature to enabled immediately after that compute call (with the line gpu_feature_info_.status_values[GPU_FEATURE_TYPE_ACCELERATED_GL] = kGpuFeatureStatusEnabled;) stopped the crash for me. But there are also a lot of calls to that same function in that file gated behind a whole host of #if directives, so there may be some build where we would hit a different one than that.

IIRC the Chrome GPU/Graphics team always had little bandwidth to support the multitude of GPU + driver combinations on Linux so in general I'd expect Linux == all GPU features disabled. With that I'd +1 the blanket rule approach (if_starboard -> enable GPU accelerated stuff). Hopefully we'll never have to maintain our own blocklist! :)

TyHolc · 2025-01-22T18:17:36Z

gpu/config/gpu_util.cc

-    return true;
-  }
+  // TODO(cobalt, b/371272304): Re-enable
+  // if (gpu_feature_info.status_values[GPU_FEATURE_TYPE_ACCELERATED_WEBGL] !=


Same comment here for hacks. In this case, swiftshader should ideally be fully disabled.

same:
#if !BUILDFLAG(IS_STARBOARD)

TyHolc · 2025-01-22T18:18:31Z

starboard/shared/linux/system_network_status.cc

@@ -43,7 +43,8 @@ bool GetOnlineStatus(bool* is_online_ptr, int netlink_fd) {
  sa.nl_groups = RTMGRP_IPV4_IFADDR;
  sa.nl_pid = getpid();
  int bind_result = bind(netlink_fd, (struct sockaddr*)&sa, sizeof(sa));
-  SB_DCHECK(bind_result == 0);
+  // TODO(cobalt, b/371272304): Re-enable
+  // SB_DCHECK(bind_result == 0);


This one we really want to solve since it's in starboard, but this is also a hack. I'm not sure what the solution is here, we might want to work with @jellefoks to figure out why this check fails when using Ozone.

Let's get it in for now.

TyHolc · 2025-01-22T18:19:34Z

ui/ozone/platform/starboard/BUILD.gn

+  sources = [
+    "test/platform_window_starboard_unittest.cc",
+    "test/starboard_test_helper.cc",
+    "test/starboard_test_helper.h",


This will change with an incoming change of mine, but I can just merge that straight to main so don't let it block merging this.

TyHolc · 2025-01-22T18:21:22Z

ui/ozone/platform/starboard/ozone_platform_starboard.cc

+    if (!surface_factory_) {
+      surface_factory_ = std::make_unique<SurfaceFactoryStarboard>();
+    }
+    // Not thread safe. This is just for prototyping.


Looks like some changes came in for events but this comment remained. Is it still true that this isn't thread safe?

TyHolc · 2025-01-22T18:23:48Z

ui/ozone/platform/starboard/platform_event_source_starboard.cc

+    {kSbKeyTab, ui::DomCode::TAB},
+    {kSbKeyEscape, ui::DomCode::ESCAPE},
+});
+// Hack: replace this when implementing event handling


Can we add a TODO here instead of this old comment? We want to make sure we have an easy way to find things like this. Also @y4vor do we still need the task runner with the new approach or will we be dropping this at some point?

Yeah, please add TODO comment

TyHolc · 2025-01-22T18:26:02Z

ui/ozone/platform/starboard/platform_screen_starboard.cc

@@ -65,10 +65,12 @@ PlatformScreenStarboard::GetAcceleratedWidgetAtScreenPoint(

 void PlatformScreenStarboard::AddObserver(display::DisplayObserver* observer) {
  // TODO(b/371272304): Add Observer to display::DisplayList.


Non-blocking, but I thought I removed these TODOs when I actually added and removed the observers to the display list? Once this is on main I can go in and remove these comments again.

TyHolc and others added 5 commits January 21, 2025 11:28

Revert "ui/ozone/starboard: introduce ScopedSbWindow (youtube#4590)" (y…

bef9f3c

…outube#4654) This reverts commit cc55e87.

Prototype for using starboard as an ozone platform (youtube#4655)

dd9571a

b/371272304

[Ozone] Move ownership of SbWindow to the platform window class (yout…

52f7d01

…ube#4688) Move ownership of SbWindow to PlatformWindowStarboard, allowing us to get the proper initialization size and assign it as the platform window in the platform window delegate. b/388348504

[Ozone] Use Angle instead of native GLES (youtube#4717)

184a263

Swap to using Angle instead of native GLES to avoid devel/debug builds crashing. b/390499263

Initial pass at event mapping (youtube#4678)

6765c67

Mappings from kSbKeyFoo to ui::DomCode::Bar .

madhurajayaraman requested a review from a team as a code owner January 21, 2025 19:32

madhurajayaraman requested a review from yell0wd0g January 21, 2025 19:32

andrewsavage1 reviewed Jan 22, 2025

View reviewed changes

TyHolc requested changes Jan 22, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge Ozone prototype commits into main #4727

Merge Ozone prototype commits into main #4727

madhurajayaraman commented Jan 21, 2025 •

edited

Loading

andrewsavage1 Jan 22, 2025

TyHolc left a comment

TyHolc Jan 22, 2025

y4vor Jan 22, 2025

andrewsavage1 Jan 22, 2025

TyHolc Jan 23, 2025

yell0wd0g Jan 23, 2025

TyHolc Jan 22, 2025

y4vor Jan 22, 2025

TyHolc Jan 22, 2025

y4vor Jan 22, 2025

TyHolc Jan 22, 2025

TyHolc Jan 22, 2025

TyHolc Jan 22, 2025

y4vor Jan 22, 2025

TyHolc Jan 22, 2025

		@@ -65,10 +65,12 @@ PlatformScreenStarboard::GetAcceleratedWidgetAtScreenPoint(

		void PlatformScreenStarboard::AddObserver(display::DisplayObserver* observer) {
		// TODO(b/371272304): Add Observer to display::DisplayList.

Merge Ozone prototype commits into main #4727

Are you sure you want to change the base?

Merge Ozone prototype commits into main #4727

Conversation

madhurajayaraman commented Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

TyHolc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madhurajayaraman commented Jan 21, 2025 •

edited

Loading