Skwasm scene #40330

eyebrowsoffire · 2023-03-15T22:32:18Z

This implements Scene and SceneBuilder in Skwasm, which lets us have golden tests and also can actually run a flutter app with skwasm, albeit missing quite a few rendering capabilities including text rendering. The subsequent changes to flutter tool that will allow compilation against the skwasm renderer will be following soon after this lands.

lib/web_ui/dev/build.dart

…_scene

lib/web_ui/dev/build.dart

lib/web_ui/dev/utils.dart

…ted.

mdebbar

This is looking good!

I have some comments for the dart code, and I would like someone else to review the C++ code.

lib/web_ui/dev/build.dart

mdebbar · 2023-04-03T20:21:20Z

lib/web_ui/dev/test_runner.dart

@@ -26,7 +26,7 @@ class TestCommand extends Command<bool> with ArgUtils<bool> {
  TestCommand() {
    argParser
      ..addFlag(
-        'debug',
+        'debug-browser',


The flutter tool calls this --start-paused which I think is a more self-explanatory name and keeps names consistent across repos/tools.

Yeah, that is probably a better name. One detail is that this option also causes us to run in a window instead of headless, but that probably is fine to just leave implied.

Yep, that's how it works in the flutter tool too.

mdebbar · 2023-04-03T20:42:13Z

lib/web_ui/lib/src/engine/skwasm/skwasm_impl/renderer.dart

+    domDocument.body!.appendChild(sceneElement);
+    surface = SkwasmSurface('#flt-scene');
+    domDocument.body!.removeChild(sceneElement);


This needs to change to work in element embedding mode. It's fine if you want to do that later, but let's add a TODO.

cc @ditman

Yeah, maybe I should just add a todo. This is just a very rough initial implementation to just get things working in the most basic case. We'll also have to handle multiple surfaces once we start introducing platform views, so this embedding is very much temporary.

mdebbar · 2023-04-03T20:46:29Z

lib/web_ui/lib/src/engine/skwasm/skwasm_impl/surface.dart

  final SurfaceHandle _handle;
+  OnRenderCallbackHandle _callbackHandle = nullptr;
+  final Map<int, Completer<void>> _pendingCallbacks = <int, Completer<void>>{};


Weak suggestion: _pendingCompleters?

Hmm how about _pendingRenders maybe?

lib/web_ui/lib/src/engine/skwasm/skwasm_impl/surface.dart

mdebbar · 2023-04-03T20:53:03Z

lib/web_ui/lib/src/engine/vector_math.dart

+  /// Transforms the input rect and calculates the bounding box of the rect
+  /// after the transform.
+  ui.Rect transformRect(ui.Rect rect) {
+    double? minX;
+    double? minY;
+    double? maxX;
+    double? maxY;
+    final Float32List vector = Float32List(2);
+    for (final ui.Offset point in <ui.Offset>[
+      rect.topLeft,
+      rect.topRight,
+      rect.bottomLeft,
+      rect.bottomRight,
+    ]) {
+      vector[0] = point.dx;
+      vector[1] = point.dy;
+      transform2(vector);
+      minX = minX == null ? vector[0] : math.min(minX, vector[0]);
+      minY = minY == null ? vector[1] : math.min(minY, vector[1]);
+      maxX = maxX == null ? vector[0] : math.max(maxX, vector[0]);
+      maxY = maxY == null ? vector[1] : math.max(maxY, vector[1]);
+    }
+    return ui.Rect.fromLTRB(minX!, minY!, maxX!, maxY!);
+  }


I think you forgot to remove this in favor of the existing:

engine/lib/web_ui/lib/src/engine/util.dart

Lines 233 to 245 in 2e5cdea

ui.Rect transformRect(Matrix4 transform, ui.Rect rect) {

_tempRectData[0] = rect.left;

_tempRectData[1] = rect.top;

_tempRectData[2] = rect.right;

_tempRectData[3] = rect.bottom;

transformLTRB(transform, _tempRectData);

return ui.Rect.fromLTRB(

_tempRectData[0],

_tempRectData[1],

_tempRectData[2],

_tempRectData[3],

);

}

So I went ahead and did some microbenchmarking comparing the two functions. As it turns out, in dart2js, my method is only very slightly slower, about 2.5% in my measurements, but in dart2wasm my method is more than twice as fast:

[chrome-dart2js-html-engine] Running... 00:00 +0: loading bench_transform_rect_test.dart [CHROME STDERR]: [CHROME STDERR]:DevTools listening on ws://127.0.0.1:12345/devtools/browser/207d349b-2b04-4cba-881e-23f0afc4d755 00:02 +0: benchmarks Benchmark transformRect Counter = 1000000 elapsed time = 2197ms 00:05 +1: benchmarks Benchmark matrix.transformRect Counter = 1000000 elapsed time = 2252ms 00:05 +2: All tests passed! [chrome-dart2js-html-engine] All tests passed! [chrome-dart2wasm-html-engine] Running... 00:00 +0: loading bench_transform_rect_test.dart [CHROME STDERR]: [CHROME STDERR]:DevTools listening on ws://127.0.0.1:12345/devtools/browser/c52f7c4a-2c93-4e7a-84a1-b0a9dbb5c146 00:06 +0: benchmarks Benchmark transformRect Counter = 1000000 elapsed time = 5606ms 00:08 +1: benchmarks Benchmark matrix.transformRect Counter = 1000000 elapsed time = 2493ms 00:08 +2: All tests passed! [chrome-dart2wasm-html-engine] All tests passed!

I think maybe we should switch over to my method. I might also be able to make an additional change to try to get that 2.5% to go away on the JS target, let me see.

This is what the benchmark looks like btw:

test('Benchmark transformRect', () { final Matrix4 transform = Matrix4.rotationZ(math.pi / 3.0); const Rect rect = Rect.fromLTRB(-100, -100, 100, 100); final Stopwatch stopwatch = Stopwatch()..start(); int counter = 0; for (int i = 0; i < 1000000; i++) { final Rect transformedRect = transformRect(transform, rect); if (transformedRect.contains(Offset.zero)) { counter++; } } stopwatch.stop(); print('Counter = $counter elapsed time = ${stopwatch.elapsedMilliseconds}ms'); });

And then copy-pasta'd except with matrix.transformRect put in place of transformRect

I made some other minor changes to narrow the gap slightly in the JS case to about 1-1.5%.

So, as it turns out, these are actually slightly different, as the existing one handles perspective transforms and mine does not. I think I'm going to go ahead and roll things back and use the existing implementation, and hope that binaryen can optimize the wasm side enough to make it acceptable. If it doesn't, I can always hand-tune the wasm side later, if needed.

Interesting. Shows you how important benchmarking is.

I'm fine with either direction you want to take.

mdebbar · 2023-04-03T21:07:02Z

lib/web_ui/lib/src/engine/renderer.dart

@@ -213,5 +213,5 @@ abstract class Renderer {

  ui.ParagraphBuilder createParagraphBuilder(ui.ParagraphStyle style);

-  void renderScene(ui.Scene scene);
+  Future<void> renderScene(ui.Scene scene);


This is being called from a synchronous method:

engine/lib/web_ui/lib/src/engine/platform_dispatcher.dart

Lines 708 to 710 in 2e5cdea

void render(ui.Scene scene, [ui.FlutterView? view]) {

renderer.renderScene(scene);

}

I don't think we should change the signature of Renderer.renderScene. Since skwasm is the only one that needs the asynchronicity, let's only change SkwasmRenderer.renderScene's signature? And I would argue that even SkwasmRenderer.renderScene doesn't need to be asynchronous. Nothing is awaiting it, so effectively, we are firing the future and forgetting about it.

One concern that I have here though is race conditions. E.g. what if the previous renderScene call takes a while and we receive a new one? Should we somehow force these calls to go in sequence (by pushing them into a queue for example)? Again, if this is something we plan to handle later, let's add a TODO.

It is fine for this to be called and not awaited from a synchronous method in a "fire and forget" manner, and that's what is still going on right now. Making this method async at the renderer level only makes things a bit more expressive and flexible. The existing renderers which do not asynchronously render can return an immediately resolved future.

The main impetus behind doing this is because the new golden screenshot capabilities in the golden tests need to wait for the rendering to be actually complete before attempting a screenshot. So really, this future is only being awaited in tests.

As for your concern in the last paragraph, I have a lot of the same thoughts. That's sort of the reason why I'd like to expose this as an async method. In situations where the render thread is slower than our target FPS, we may want to keep track of pending futures and stop actually submitting new frames if it starts getting backed up. Otherwise, if we continue pushing more frames, the render thread will get continually more backed up, when it's better to to just drop frames. We can't do this with our current "fire-and-forget" method, but making the method async opens us up to doing this in the future.

Agreed with Mouad, this should be synchronous for consistency with the mobile Flutter engine and our public facing API. The Skwasm renderer can have a way to await for the scene to actually be rendered which can be used in tests. As for the problem with the render thread getting backed up, this is already something that can happen in Flutter mobile and I think they just drop everything but the latest frame.

Is there a suggested alternative? This Renderer.renderScene API is web-specific, so there is nothing in the mobile Flutter engine to keep it consistent with, and I'm also not changing the public facing API (PlatformDispatcher.render is still a synchronous method). Basically the reason I want to return a Future here is:

To allow us to wait for the render to complete in the tests.

If we do want to "drop everything but the latest frame" like the native engine does, the UI thread still needs a way to tell whether we are actually getting backed up.

A Future returned directly from the method seems like the most natural way to achieve this, but if there is something different you all have in mind I'm open to suggestions.

Maybe I could have Renderer.renderScene return FutureOr<void>, so that the subclasses of Renderer that aren't asynchronous can still just return void?

I should've clarified my motivation for keeping things as sync as possible. It's because I want future maintainers to be aware that this is used as a sync API and it's better to stay that way. And if they really need the asynchronicity, then they can make the deliberate decision of converting it to async.

If we make all of them async today, I'm afraid it becomes too easy in the future to add async stuff in there without thinking about the consequences. I want to conversion to async to be a deliberate decision based on a real need (like in the case of skwasm).

Does the FutureOr<void> change not address your issue? SkwasmRenderer's render will return Future<void>, but the other renderers still return void. So it would require us to explicitly change a renderer's return value to make it async

It doesn't address the issue. The developer making change to renderScene will think it's okay to add an await in there because the signature already "supports" async. I want them to have the extra burden of changing the signature, which will make them look around and check whether it's fine to convert it to async or not.

That said, I don't feel strongly about it, and maybe this is all just me doing premature design optimizations :)

Just to clarify, if a developer is editing the renderScene function on one of the existing renderers (whose renderScene method is synchronous), and they want to add an await, they will have the extra burden of changing the signature, since the signature of the existing renderers is staying void renderScene(...). They would have to explicitly change it to Future<void> renderScene(...). The abstract Renderer class has the signature FutureOr<void> renderScene(...), but the actual concrete renderer classes themselves either explicitly return void or explicitly return Future<void>

Ahh okay got it now. I misunderstood your previous comment, thought you were making subclasses FutureOr<void> too (should've looked more closely at the code 🤦‍♂️)

Ok, this actually addresses my concerns, thanks!

Co-authored-by: Mouad Debbar <mdebbar@google.com>

flutter-dashboard · 2023-04-04T17:35:55Z

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #40330 at sha 64d5728

flutter-dashboard · 2023-04-05T21:44:28Z

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #40330 at sha a0dbca2

mdebbar · 2023-04-06T17:28:17Z

I approved the PR but I would love someone more qualified than me to review the C++ changes.

harryterkelsen

LGTM

flutter-dashboard · 2023-04-10T17:01:51Z

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #40330 at sha 2d52b90

Skwasm scene

harryterkelsen and others added 5 commits February 15, 2023 13:46

WIP add screenshot goldens to ui_tests

cfccaa7

Started implementing skwasm scene stuff.

294cff9

Scene builder mostly coded up.

6dc5144

Stuff.

13816e4

Running into TFA issues.

ab7e937

flutter-dashboard bot added the platform-web Code specifically for the web engine label Mar 15, 2023

eyebrowsoffire added 10 commits March 21, 2023 09:51

More scene changes.

6184bec

Merge branch 'main' into skwasm_scene

fd915b3

We drawing now.

aade26b

Copy stuff, but skwasm.worker.js doesn't copy yet.

d49f73d

Add a few flags.

c473192

Bundle skwasm.

bbd206b

Use newer buildroot.

b651d65

Merge branch 'main' into skwasm_scene

6fbb8d0

Merge branch 'main' into skwasm_scene

4796b64

Use the normal toolchain so we get LTO.

d79c143

kevmoo reviewed Mar 29, 2023

View reviewed changes

lib/web_ui/dev/build.dart Outdated Show resolved Hide resolved

kevmoo reviewed Mar 29, 2023

View reviewed changes

lib/web_ui/dev/build.dart Show resolved Hide resolved

eyebrowsoffire added 3 commits March 29, 2023 10:06

Merge remote-tracking branch 'hterkelsen/ui-golden-tests' into skwasm…

8350e48

…_scene

Analyzer cleanup and Kevin's comments.

d8bfb0f

Update license golden.

f7a22db

kevmoo reviewed Mar 29, 2023

View reviewed changes

lib/web_ui/dev/build.dart Outdated Show resolved Hide resolved

eyebrowsoffire added 4 commits March 29, 2023 12:27

Some fixes for goldens, although skwasm renderer has async issues.

ad64278

Some async stuff.

17a8e6c

Merge branch 'main' into skwasm_scene

c89584c

Fixed cull rect calculation.

d446202

kevmoo reviewed Mar 30, 2023

View reviewed changes

lib/web_ui/dev/utils.dart Show resolved Hide resolved

eyebrowsoffire added 3 commits March 30, 2023 16:11

Update licenses and fix an analyzer issue.

4ca5636

Wrote unit tests that cover the remainder of what skwasm has implemen…

4f85df1

…ted.

Merge branch 'main' into skwasm_scene

a722c25

eyebrowsoffire requested a review from harryterkelsen April 3, 2023 18:20

mdebbar reviewed Apr 3, 2023

View reviewed changes

Update lib/web_ui/lib/src/engine/skwasm/skwasm_impl/surface.dart

64d5728

Co-authored-by: Mouad Debbar <mdebbar@google.com>

eyebrowsoffire added 10 commits April 4, 2023 11:46

Address some comments from Mouad.

a239266

Merge branch 'main' into skwasm_scene

3d90da0

Fix formatting.

da3be30

Change name of variable for clarity.

e0892d6

Fix async semantics of skwasm surface.

07f6f75

Reinstate old transformRect algorithm.

0d94512

Merge branch 'main' into skwasm_scene

cd56076

Shift cullRect by offset when using opacity layer.

85674bc

Renderer.renderScene should return FutureOr.

457059f

Merge branch 'main' into skwasm_scene

a0dbca2

mdebbar approved these changes Apr 6, 2023

View reviewed changes

harryterkelsen approved these changes Apr 6, 2023

View reviewed changes

eyebrowsoffire added 4 commits April 6, 2023 10:36

Merge branch 'main' into skwasm_scene

a6f8d1e

Get rid of duplicate skwasm_group definitino from merge.

26a54c0

Merge branch 'main' into skwasm_scene

6a41070

Merge branch 'main' into skwasm_scene

2d52b90

eyebrowsoffire added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 10, 2023

auto-submit bot merged commit 53930ff into main Apr 10, 2023
40 checks passed

auto-submit bot deleted the skwasm_scene branch April 10, 2023 17:39

engine-flutter-autoroll mentioned this pull request Apr 10, 2023

Roll Flutter Engine from 44d165644450 to 53930ff1dcdc (4 revisions) flutter/flutter#124534

Merged

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

53930ff1d Skwasm scene (flutter/engine#40330)

1bff121

gspencergoog pushed a commit to gspencergoog/engine that referenced this pull request Apr 10, 2023

Skwasm scene (flutter#40330)

dd62e92

Skwasm scene

zhongwuzw pushed a commit to zhongwuzw/engine that referenced this pull request Apr 14, 2023

Skwasm scene (flutter#40330)

f164d10

Skwasm scene

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skwasm scene #40330

Skwasm scene #40330

eyebrowsoffire commented Mar 15, 2023 •

edited

mdebbar left a comment

mdebbar Apr 3, 2023

eyebrowsoffire Apr 4, 2023

mdebbar Apr 4, 2023

mdebbar Apr 3, 2023

eyebrowsoffire Apr 4, 2023

mdebbar Apr 3, 2023

eyebrowsoffire Apr 4, 2023

mdebbar Apr 6, 2023

mdebbar Apr 3, 2023

eyebrowsoffire Apr 4, 2023

eyebrowsoffire Apr 4, 2023

eyebrowsoffire Apr 4, 2023

eyebrowsoffire Apr 4, 2023

mdebbar Apr 6, 2023

mdebbar Apr 3, 2023

eyebrowsoffire Apr 4, 2023

harryterkelsen Apr 5, 2023

eyebrowsoffire Apr 5, 2023 •

edited

eyebrowsoffire Apr 5, 2023

mdebbar Apr 6, 2023

eyebrowsoffire Apr 6, 2023

mdebbar Apr 6, 2023

eyebrowsoffire Apr 6, 2023 •

edited

mdebbar Apr 6, 2023 •

edited

flutter-dashboard bot commented Apr 4, 2023

flutter-dashboard bot commented Apr 5, 2023

mdebbar commented Apr 6, 2023

harryterkelsen left a comment

flutter-dashboard bot commented Apr 10, 2023

	ui.Rect transformRect(Matrix4 transform, ui.Rect rect) {
	_tempRectData[0] = rect.left;
	_tempRectData[1] = rect.top;
	_tempRectData[2] = rect.right;
	_tempRectData[3] = rect.bottom;
	transformLTRB(transform, _tempRectData);
	return ui.Rect.fromLTRB(
	_tempRectData[0],
	_tempRectData[1],
	_tempRectData[2],
	_tempRectData[3],
	);
	}

	void render(ui.Scene scene, [ui.FlutterView? view]) {
	renderer.renderScene(scene);
	}

Skwasm scene #40330

Skwasm scene #40330

Conversation

eyebrowsoffire commented Mar 15, 2023 • edited

mdebbar 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eyebrowsoffire Apr 5, 2023 • edited

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

eyebrowsoffire Apr 6, 2023 • edited

Choose a reason for hiding this comment

mdebbar Apr 6, 2023 • edited

Choose a reason for hiding this comment

flutter-dashboard bot commented Apr 4, 2023

flutter-dashboard bot commented Apr 5, 2023

mdebbar commented Apr 6, 2023

harryterkelsen left a comment

Choose a reason for hiding this comment

flutter-dashboard bot commented Apr 10, 2023

eyebrowsoffire commented Mar 15, 2023 •

edited

eyebrowsoffire Apr 5, 2023 •

edited

eyebrowsoffire Apr 6, 2023 •

edited

mdebbar Apr 6, 2023 •

edited