Move renderer config to EmbedderTestContext #56699

cbracken · 2024-11-19T05:31:02Z

Moves all backend-specific renderer configuration out of EmbedderConfigBuilder and into the backend-specific subclasses of EmbedderTestContext. EmbedderTestContext is already backend-specific and as of recent patches, also houses compositor configuration, making it the natural home of this code.

As a result, we no longer need backend-specific methods such as SetSoftwareRendererConfig, SetMetalRendererConfig, SetOpenGLRendererConfig, SetVulkanRendererConfig. Nor do we need manual backend initialisation in EmbedderConfigBuilder. Nor does that initialisation any longer require relying on internal backend-specific code within EmbedderTestContext, since we now do that initialisation in the EmbedderTestContext constructor.

Since the bulk of the work previously done by this method now occurs in the EmbedderTestContext constructor, the only work remaining in these methods is surface creation. Further, since this is all now implemented in backend-specific EmbedderTestContext subclasses, they have all been renamed to a single method: SetSurface.

Previously, all of these methods took a surface_size parameter, with two exceptions:

SetVulkanRendererConfig also took an optional FlutterVulkanInstanceProcAddressCallback parameter. This has been extracted to a separate method SetVulkanInstanceProcAddressCallback on EmbedderTestContextVulkan.
SetSoftwareRendererConfig defaulted the parameter to a size of (1, 1). For consistency, this is no longer defaulted, and all call sites have been updated, consistent with other backends.

Lastly, one nice benefit is that because the render config is initialised in the EmbedderTestContext constructor, there's no longer a requirement to call Set*RendererConfig prior modifying any specific properties on the config, nor is it problematic to call the (replacement) SetSurface method after modifying the config. Where the renderer config was being customised in embedder unit tests, I've pushed that customisation up to the top of the test where the rest of the test context is configured.

This eliminates nearly all remaining #ifdef SHELL_ENABLE_$BACKEND blocks in the EmbedderTest infrastructure.

Issue: flutter/flutter#158998

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

cbracken · 2024-11-19T06:08:53Z

If there are cases where we specifically want to test with no renderer config set, I’d like to know about them. It’d be a straightforward tweak to this.

cbracken · 2024-11-19T15:21:13Z

shell/platform/embedder/tests/embedder_config_builder.cc


-  InitializeGLRendererConfig();
-  InitializeMetalRendererConfig();
-  InitializeVulkanRendererConfig();


The initialise methods above (and the similar software renderer config initialisation below) moved to ctors of EmbedderTestContextMetal etc.

cbracken · 2024-11-19T15:24:02Z

shell/platform/embedder/tests/embedder_config_builder.cc

-      SetSoftwareRendererConfig(surface_size);
-      break;
-  }
-}


No need for for the type argument anymore since the EmbedderConfigBuilder already took an EmbedderTestContext of that type in its constructor, so we knew the type all along. Renamed all the referenced methods to just EmbedderTestContext{GL,Metal,Sofware,Vulkan}::SetRendererConfig except turns out that now that the initialisation is done in the ctor, all they actually do is create the surface of the right type with the specified size, so renamed them to SetSurface(surface_size).

cbracken · 2024-11-19T15:27:10Z

shell/platform/embedder/tests/embedder_config_builder.cc


-FlutterRendererConfig& EmbedderConfigBuilder::GetRendererConfig() {
-  return renderer_config_;
-}


Unloved and unused now.

cbracken · 2024-11-19T15:28:37Z

shell/platform/embedder/tests/embedder_config_builder.cc

  return UniqueEngine{engine};
 }

-#ifndef SHELL_ENABLE_GL


Everything below here are just methods on the appropriate EmbedderTestContext subclass. People who need GL-specific methods can get them from the GL subclass, etc.

cbracken · 2024-11-19T15:32:06Z

shell/platform/embedder/tests/embedder_a11y_unittests.cc

    EmbedderConfigBuilder builder(
        GetEmbedderContext(EmbedderTestContextType::kSoftwareContext));
-    builder.SetSoftwareRendererConfig();
+    builder.SetSurface(SkISize::Make(1, 1));


Here and below (1, 1) was the default parameter value... but we only ever defaulted it for the software renderer, now we're consistent across the board and make people announce what they want so readers don't need to check the method declaration.

cbracken · 2024-11-19T15:33:29Z

shell/platform/embedder/tests/embedder_test_context.cc


+FlutterRendererConfig& EmbedderTestContext::GetRendererConfig() {
+  return renderer_config_;
+}


Used by EmbedderConfigBuilder::LaunchEngine.

cbracken · 2024-11-19T15:34:29Z

shell/platform/embedder/tests/embedder_test_context.h


-  void SetRenderTargetType(
-      EmbedderTestBackingStoreProducer::RenderTargetType type,
-      FlutterSoftwarePixelFormat software_pixfmt);


Unused method declaration with no definition. I removed it in #56626 but missed cleaning this up.

cbracken · 2024-11-19T15:36:00Z

shell/platform/embedder/tests/embedder_test_context.h


  void SetNextSceneCallback(const NextSceneCallback& next_scene_callback);

-  virtual void SetupSurface(SkISize surface_size) = 0;


These methods are no longer necessary because it turns out that all that the Set*RenderConfig methods did was initialise the render config (done in the ctors of this subclass now) and call this method.

cbracken · 2024-11-19T15:38:08Z

shell/platform/embedder/tests/embedder_test_context_gl.cc

+      },
+      .populate_existing_damage = nullptr,
+  };
+}


Here and in the other EmbedderTestContext subclass ctors: this is just the code from the old Initialize*FooRenderConfig methods except using designated initialiser notation for readability, which also forces the members to be initialised in the order they're declared in embedder.h, hence the reordering.

cbracken · 2024-11-19T15:39:16Z

shell/platform/embedder/tests/embedder_test_context_gl.cc

+        FlutterPresentInfo{
+            .fbo_id = 0,
+        });
+  };


Moved here from ifdef'ed code on EmbedderConfigBuilder.

cbracken · 2024-11-19T15:39:58Z

shell/platform/embedder/tests/embedder_test_context_gl.h

+  // |EmbedderTestContext|
+  void SetSurface(SkISize surface_size) override;

-  void SetupCompositorUsingGLSurfaces();


Went looking and this method was never used and had no definition. 🤷‍♂️

cbracken · 2024-11-19T15:40:43Z

shell/platform/embedder/tests/embedder_test_context_gl.h

+  // |EmbedderTestContext|
+  void SetupCompositor() override;

- private:


This stuff is all private now instead of protected since there are no subclasses.

jonahwilliams

RSLGTM

cbracken · 2024-11-19T15:48:18Z

shell/platform/embedder/tests/embedder_test_context_gl.h

-
 private:
-  // This allows the builder to access the hooks.
-  friend class EmbedderConfigBuilder;


Friendship ended with EmbedderConfigBuilder.

cbracken · 2024-11-19T15:49:18Z

shell/platform/embedder/tests/embedder_test_context_metal.mm

+  FML_CHECK(metal_surface_) << "Set up the Metal surface before setting up a compositor.";
+  compositor_ =
+      std::make_unique<EmbedderTestCompositorMetal>(surface_size_, metal_surface_->GetGrContext());
+}


For consistency with header declaration order, moved these guys down to the bottom given that they're private.

Moves all backend-specific renderer configuration out of EmbedderConfigBuilder and into the backend-specific subclass of EmbedderTestContext. EmbedderTestContext is already backend-specific and as of recent patches, also houses compositor configuration, making it the natural home of this code. As a result, we no longer need backend-specific methods such as SetSoftwareRendererConfig, SetMetalRendererConfig, etc. Nor do we need manual backend initialization in EmbedderConfigBuilder. Nor does that initialization any longer require relying on internal backend-specific code within EmbedderTestContext, since we now do that initialization in the EmbedderTestContext constructor. Since the bulk of the work previously done by this method now occurs in the EmbedderTestContext constructor, the only work remaining in these methods is surface creation. Further, since theses are now implemented in backend-specific EmbedderTestContext subclasses, they have all been renamed to a single method: SetSurface. Previously, all of these methods took a surface_size parameter with two exceptions: * SetVulkanRendererConfig also took an optional FlutterVulkanInstanceProcAddressCallback parameter. This has been extracted to a separate method SetVulkanInstanceProcAddressCallback on EmbedderTestContextVulkan. * SetSoftwareRendererConfig defaulted the parameter to a size of (1, 1). For consistency, this is no longer defaulted, and all call sites have been updated, consistent with other backends. Lastly, one nice benefit is that because the render config is initialized in the EmbedderTestContext constructor, there's no longer a requirement to call Set*RendererConfig prior modifying any specific properties on the config, nor is it problematic to call the updated SetSurface method after modifying the config. Where the renderer config was being customised in embedder unittests, I've pushed that customisation up to the top of the test where the rest of the test context is configured. Issue: flutter/flutter#158998

cbracken · 2024-11-19T15:52:36Z

shell/platform/embedder/tests/embedder_vk_unittests.cc

-  EmbedderConfigBuilder builder(context);
-  builder.SetVulkanRendererConfig(
-      SkISize::Make(1024, 1024),
+  context.SetVulkanInstanceProcAddressCallback(


Vulkan-specific context configuration now occurs on the Vulkan-specific context.

cbracken · 2024-11-19T15:55:10Z

shell/platform/embedder/tests/embedder_gl_unittests.cc

-
-  EmbedderConfigBuilder builder(context);
-  builder.SetOpenGLRendererConfig(SkISize::Make(600, 1024));
-  builder.SetDartEntrypoint("push_frames_over_and_over");


Here and below: There used to be an unobvious ordering requirement wherein any backend-specific context configuration could only happen after the EmbedderConfigBuilder::Set*RendererConfig() call (otherwise the Set*RendererConfig would stomp it. We no longer have that ordering dependency, so we can now group context configuration together at the top, and builder configuration together afterwards.

Further, a few of the backend-specific calls that used to be exposed on the non-backend-specific EmbedderConfigBuilder are now exposed on the backend-specific EmbedderTestContext, in order to avoid nonsensical calls like setting GL callbacks on a Metal test context.

cbracken · 2024-11-19T16:02:36Z

shell/platform/embedder/tests/embedder_gl_unittests.cc

-  builder.SetDartEntrypoint("push_frames_over_and_over");
-
+  auto& context = static_cast<EmbedderTestContextGL&>(
+      GetEmbedderContext(EmbedderTestContextType::kOpenGLContext));


In a followup patch I'm going to try rewriting this method to be templatised so we can do this and just get the right type out automatically like you can do with std::variant:

auto& context GetEmbedderContext<EmbedderTestContextGL>();

…#56699)

…159155) flutter/engine@cff1e75...d5820a6 2024-11-19 [email protected] Roll Skia from 0b74a1bb1b55 to 78ef6b7a574f (6 revisions) (flutter/engine#56707) 2024-11-19 [email protected] Move renderer config to EmbedderTestContext (flutter/engine#56699) 2024-11-19 [email protected] Roll Skia from 8a1a84509501 to 0b74a1bb1b55 (1 revision) (flutter/engine#56702) 2024-11-19 [email protected] Roll Dart SDK from 05d58364e92f to b01654fa26c7 (1 revision) (flutter/engine#56694) 2024-11-19 [email protected] EmbedderTest: Eliminate unused include (flutter/engine#56698) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

Moves all backend-specific renderer configuration out of `EmbedderConfigBuilder` and into the backend-specific subclasses of `EmbedderTestContext`. `EmbedderTestContext` is already backend-specific and as of recent patches, also houses compositor configuration, making it the natural home of this code. As a result, we no longer need backend-specific methods such as `SetSoftwareRendererConfig`, `SetMetalRendererConfig`, `SetOpenGLRendererConfig`, `SetVulkanRendererConfig`. Nor do we need manual backend initialisation in `EmbedderConfigBuilder`. Nor does that initialisation any longer require relying on internal backend-specific code within `EmbedderTestContext`, since we now do that initialisation in the `EmbedderTestContext` constructor. Since the bulk of the work previously done by this method now occurs in the `EmbedderTestContext` constructor, the only work remaining in these methods is surface creation. Further, since this is all now implemented in backend-specific `EmbedderTestContext` subclasses, they have all been renamed to a single method: `SetSurface`. Previously, all of these methods took a surface_size parameter, with two exceptions: * `SetVulkanRendererConfig` also took an optional `FlutterVulkanInstanceProcAddressCallback` parameter. This has been extracted to a separate method `SetVulkanInstanceProcAddressCallback` on `EmbedderTestContextVulkan`. * `SetSoftwareRendererConfig` defaulted the parameter to a size of (1, 1). For consistency, this is no longer defaulted, and all call sites have been updated, consistent with other backends. Lastly, one nice benefit is that because the render config is initialised in the `EmbedderTestContext` constructor, there's no longer a requirement to call `Set*RendererConfig` prior modifying any specific properties on the config, nor is it problematic to call the (replacement) `SetSurface` method after modifying the config. Where the renderer config was being customised in embedder unit tests, I've pushed that customisation up to the top of the test where the rest of the test context is configured. This eliminates nearly all remaining `#ifdef SHELL_ENABLE_$BACKEND` blocks in the EmbedderTest infrastructure. Issue: flutter#158998 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

cbracken requested review from bdero, chinmaygarde and jonahwilliams November 19, 2024 05:31

cbracken commented Nov 19, 2024

View reviewed changes

jonahwilliams approved these changes Nov 19, 2024

View reviewed changes

cbracken commented Nov 19, 2024

View reviewed changes

cbracken added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 19, 2024

auto-submit bot merged commit 3e96ceb into flutter:main Nov 19, 2024
31 checks passed

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 19, 2024

cbracken deleted the refactor-render-context branch November 19, 2024 16:55

engine-flutter-autoroll mentioned this pull request Nov 19, 2024

Roll Flutter Engine from cff1e751f853 to 3e96cebf0d1d (4 revisions) flutter/flutter#159152

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 19, 2024

3e96cebf0 Move renderer config to EmbedderTestContext (flutter/engine…

1d50503

…#56699)

engine-flutter-autoroll mentioned this pull request Nov 19, 2024

Roll Flutter Engine from cff1e751f853 to d5820a638885 (5 revisions) flutter/flutter#159155

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 19, 2024

3e96cebf0 Move renderer config to EmbedderTestContext (flutter/engine…

bc42bf9

…#56699)


		void SetNextSceneCallback(const NextSceneCallback& next_scene_callback);

		virtual void SetupSurface(SkISize surface_size) = 0;

Move renderer config to EmbedderTestContext #56699

Move renderer config to EmbedderTestContext #56699

Uh oh!

Conversation

cbracken commented Nov 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

cbracken commented Nov 19, 2024

Uh oh!

cbracken Nov 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbracken Nov 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbracken Nov 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cbracken commented Nov 19, 2024 •

edited

Loading

cbracken Nov 19, 2024 •

edited

Loading

cbracken Nov 19, 2024 •

edited

Loading

cbracken Nov 19, 2024 •

edited

Loading