Remove single window assumption from SceneBuilder #41559

dkwingsmt · 2023-04-27T21:59:15Z

This PR fixes flutter/flutter#112202. Scene is no longer tied to views, but receives method arguments for view properties, clearing the path to multiview Flutter.

Scene no longer creates a LayerTree on construction, but only part of the config. When a output method is called (toImage, toImageSync, or takeLayerTree), the LayerTree is constructed on the spot. In this way, all LayerTrees return to being unique_ptrs instead of shared_ptrs, reverting part of #35608 . And Scene no longer needs to disfunction after one takeLayerTree, since the layer tree config is never really taken away.

A device_pixel_ratio is now added to the parameter list of Engine::Render and Animator::Render.

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 Hixie said 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.

dkwingsmt · 2023-04-28T19:24:06Z

@jonahwilliams This PR should be ready for review, can you take a look?

jonahwilliams · 2023-04-28T19:24:24Z

The dart:ui methods Scene.toImage and Scene.toImageSync receive a new parameter pixelRatio each. This parameter does not change the output dimension but only controls the deprecated physical shape models (PhysicalShapeLayer). We should also modify the framework to pass in the devicePixelRatio argument.

Please don't add parameters we'd need to support indefinitely for functionality we're going to remove in < 1 year. I would instead default the physical shape layer to use a 2.0 DPR.

jonahwilliams · 2023-04-28T19:41:28Z

Here is a draft PR to delete PML: #41593

I'll see if this is landable in the near term.

dkwingsmt · 2023-04-28T20:39:41Z

@jonahwilliams I've removed the extra parameter. Now they fetch the pixel ratio from window 0 with a fallback of 2.0f.

Of course, it would be easier if your change could land. :)

runtime/runtime_controller.cc

lib/ui/compositing/scene.cc

dkwingsmt · 2023-04-28T22:31:43Z

Also cc @goderbauer

jonahwilliams · 2023-04-29T01:43:42Z

Looks like I missed once place , Canvas.cc actually depends on the device pixel ratio in Canvas::drawShadow:

  // Not using SafeNarrow because DPR will always be a relatively small number.
  SkScalar dpr = static_cast<float>(UIDartState::Current()
                                        ->platform_configuration()
                                        ->get_window(0)
                                        ->viewport_metrics()
                                        .device_pixel_ratio);

We'll need to add support for providing the device pixel ratio on the framework side if this is used for real

jonahwilliams · 2023-04-29T01:46:23Z

This is more or less just multiplied with the provided elevation:

https://github.com/flutter/engine/blob/main/impeller/display_list/dl_dispatcher.cc#L1113

The impeller code is based on the Skia code. So technically we could make this work today by multiplying elevation by DPR in the framework, but that would break folks using the canvas interface

dkwingsmt · 2023-05-01T05:13:13Z

@jonahwilliams I've created an issue for Canvas: flutter/flutter#125780

It does not affect this PR though, does it?

goderbauer

Overall this makes sense to me, deferring to the engine experts for details.

goderbauer · 2023-05-02T08:28:23Z

lib/ui/compositing/scene.cc

@@ -23,6 +23,8 @@

 namespace flutter {

+static constexpr float kFallbackPixelRatio = 2.0f;


This should probably have a comment answering questions like: Why is this 2.0? When do we fallback to this? Is this just meant to support legacy behavior and new logic should not use this constant? Or when should new logic use this constant?

(I see that this is kinda explained on defaultViewPixelRatio, might be worthwhile to link to that here)

goderbauer · 2023-05-02T08:32:00Z

lib/ui/compositing/scene.h

+  // {Preroll,Paint}Context.frame_device_pixel_ratio, remove this method and its
+  // related logic.
+  // https://github.com/flutter/flutter/issues/125720
+  static float defaultViewPixelRatio();


Can we give this a name that clearly indicated that this is legacy behavior and new code should not depend on this? Maybe we can replace default with legacy or something like that... The "default" should be to not assume a specific view or DPR.

goderbauer · 2023-05-02T08:34:12Z

runtime/runtime_controller.cc

@@ -20,6 +20,8 @@

 namespace flutter {

+const uint64_t kFlutterDefaultViewId = 0llu;


Should this be "implicitView" instead of default view? We are trying to remove the annotation that flutter has a default view. In some modes, it may run with an implicitly created view, though...

Engine PR: flutter/engine#41593 This must land first We should remove these, as they've been deprecated for a while. On the engine side of things, the physical model layer is the only one which requires the device pixel ratio, so deleting it will allow us to simplify the layer tree code in flutter/engine#41559

jonahwilliams

LGTM

…engine#41559)

…126175) flutter/engine@6b467df...758cbad 2023-05-05 jason-simmons@users.noreply.github.com [Impeller] Do not free a Vulkan command buffer if its command pool has already been destroyed (flutter/engine#41761) 2023-05-05 dkwingsmt@users.noreply.github.com Remove single window assumption from SceneBuilder (flutter/engine#41559) 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 jimgraham@google.com,rmistry@google.com,zra@google.com 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

dkwingsmt added 8 commits April 27, 2023 14:58

Create layer tree on taking

d934818

Let takeLayerTree remove config for now

474b357

Format

39a3a09

lint

ba4b01a

Compute layer tree size in RuntimeConfiguration

c8bd69b

Resolve toImageSync

6a03556

Convert toImage

30b95c6

Right before dart_ui.cc

ae2c065

dkwingsmt force-pushed the layer-tree-content-2 branch from 337f35a to ae2c065 Compare April 28, 2023 03:53

dkwingsmt added 3 commits April 28, 2023 00:58

Fix crash

d7fa9d6

To dart:ui

2ccc869

config is no longer a ptr

99bfd24

dkwingsmt requested a review from jonahwilliams April 28, 2023 19:22

dkwingsmt added 2 commits April 28, 2023 12:56

Implicit ratio

768aa5e

An easier way

332f86a

loic-sharma reviewed Apr 28, 2023

View reviewed changes

runtime/runtime_controller.cc Outdated Show resolved Hide resolved

loic-sharma reviewed Apr 28, 2023

View reviewed changes

lib/ui/compositing/scene.cc Outdated Show resolved Hide resolved

Add TODO and comment

c829f37

jonahwilliams mentioned this pull request Apr 28, 2023

[framework] delete physical model layer. flutter/flutter#125719

Merged

goderbauer reviewed May 2, 2023

View reviewed changes

Merge remote-tracking branch 'origin/main' into layer-tree-content-2

8b793a4

Move device pixel ratio

3917b10

jonahwilliams approved these changes May 5, 2023

View reviewed changes

dkwingsmt merged commit 67d18ed into flutter:main May 5, 2023

engine-flutter-autoroll mentioned this pull request May 5, 2023

Roll Flutter Engine from 6b467df16e11 to 758cbadfac1f (2 revisions) flutter/flutter#126175

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 5, 2023

67d18ed61 Remove single window assumption from SceneBuilder (flutter/…

6feafb7

…engine#41559)

dkwingsmt mentioned this pull request May 9, 2023

Multiview prototype (May 9) loic-sharma/flutter-engine#43

Merged

8 tasks

dkwingsmt deleted the layer-tree-content-2 branch July 11, 2023 17:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Remove single window assumption from SceneBuilder #41559

Remove single window assumption from SceneBuilder #41559

Uh oh!

dkwingsmt commented Apr 27, 2023 •

edited

Loading

Uh oh!

dkwingsmt commented Apr 28, 2023

Uh oh!

jonahwilliams commented Apr 28, 2023

Uh oh!

jonahwilliams commented Apr 28, 2023

Uh oh!

dkwingsmt commented Apr 28, 2023 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

dkwingsmt commented Apr 28, 2023

Uh oh!

jonahwilliams commented Apr 29, 2023

Uh oh!

jonahwilliams commented Apr 29, 2023

Uh oh!

dkwingsmt commented May 1, 2023 •

edited

Loading

Uh oh!

goderbauer left a comment

Uh oh!

goderbauer May 2, 2023

Uh oh!

goderbauer May 2, 2023

Uh oh!

goderbauer May 2, 2023

Uh oh!

goderbauer May 2, 2023

Uh oh!

jonahwilliams left a comment

Uh oh!

Uh oh!

		@@ -23,6 +23,8 @@

		namespace flutter {

		static constexpr float kFallbackPixelRatio = 2.0f;

		@@ -20,6 +20,8 @@

		namespace flutter {

		const uint64_t kFlutterDefaultViewId = 0llu;

Remove single window assumption from SceneBuilder #41559

Remove single window assumption from SceneBuilder #41559

Uh oh!

Conversation

dkwingsmt commented Apr 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

dkwingsmt commented Apr 28, 2023

Uh oh!

jonahwilliams commented Apr 28, 2023

Uh oh!

jonahwilliams commented Apr 28, 2023

Uh oh!

dkwingsmt commented Apr 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dkwingsmt commented Apr 28, 2023

Uh oh!

jonahwilliams commented Apr 29, 2023

Uh oh!

jonahwilliams commented Apr 29, 2023

Uh oh!

dkwingsmt commented May 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

goderbauer May 2, 2023

Choose a reason for hiding this comment

Uh oh!

goderbauer May 2, 2023

Choose a reason for hiding this comment

Uh oh!

goderbauer May 2, 2023

Choose a reason for hiding this comment

Uh oh!

goderbauer May 2, 2023

Choose a reason for hiding this comment

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dkwingsmt commented Apr 27, 2023 •

edited

Loading

dkwingsmt commented Apr 28, 2023 •

edited

Loading

dkwingsmt commented May 1, 2023 •

edited

Loading