Skwasm Renderer - initial implementation #39072

eyebrowsoffire · 2023-01-23T19:35:14Z

Implements a handful of renderer APIs, including paths, canvas, picture, paint.

flutter-dashboard · 2023-01-23T19:35:17Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

…e name. Change-Id: I5e3644a904917a375486acd47830f311fe19a5ce Tested: Running unit tests in this repo as well as running against coming web engine changes that use the @Native annotation: flutter/engine#39072 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/279479 Reviewed-by: Ömer Ağacan <omersa@google.com> Commit-Queue: Jackson Gardner <jacksongardner@google.com> Reviewed-by: Aske Simon Christensen <askesc@google.com>

mdebbar

LGTM! But someone else should take a look at the C++ files.

mdebbar · 2023-02-27T23:04:03Z

lib/web_ui/skwasm/BUILD.gn

@@ -0,0 +1,63 @@
+# Copyright 2019 The Flutter Authors. All rights reserved.


I agree it's orthogonal. It's a bit weird that some of our build files live in web_sdk at the root of the repo, while others live inside web_ui.

harryterkelsen

LGTM with nits

harryterkelsen · 2023-02-28T19:27:03Z

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

+
+  @override
+  void saveLayer(ui.Rect? bounds, ui.Paint uiPaint) {
+    assert(uiPaint is SkwasmPaint);


nit: unnecessary since the next line will crash anyways if uiPaint is not a SkwasmPaint

harryterkelsen · 2023-02-28T19:38:50Z

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

+  SkwasmImage clone() => this;
+
+  @override
+  bool isCloneOf(ui.Image other) => other == this;


nit: change to identical(this, other). The way it's written now is potentially problematic if later on we add == operator to SkwasmImage in a way where == is not necessarily the same as being a clone

harryterkelsen · 2023-02-28T21:29:29Z

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

@@ -17,160 +18,182 @@ import '../../renderer.dart';
 class SkwasmRenderer implements Renderer {
  @override
  ui.Path combinePaths(ui.PathOperation op, ui.Path path1, ui.Path path2) {
-    throw UnimplementedError('Not yet implemented');
+    assert(path1 is SkwasmPath);


nit: here and below, same issue as above re the assertions. since the as checks do the same thing

mdebbar · 2023-03-01T23:17:01Z

web_sdk/BUILD.gn

      {
        source = rebase_path("$root_out_dir/canvaskit_chromium/canvaskit.js")
-        destination = "flutter_web_sdk/canvaskit_chromium/canvaskit.js"
+        destination = "canvaskit/chromium/canvaskit.js"
      },
      {
        source = rebase_path("$root_out_dir/canvaskit_chromium/canvaskit.wasm")
-        destination = "flutter_web_sdk/canvaskit_chromium/canvaskit.wasm"
+        destination = "canvaskit/chromium/canvaskit.wasm"
      },


Thanks for doing the chromium change as well! I guess I'm not gonna need my restructuring PR after all (#39796).

Actually though, the copy steps from that PR might still be pertinent. I don't have any copy steps here.

You are right. I still need out/wasm_release/canvaskit/chromium/... so that the flutter web server can find the locally built canvaskit(s) when --local-web-sdk is specified.

Although the PR will be smaller after your change.

)

yjbanov · 2023-02-27T02:05:28Z

lib/web_ui/skwasm/BUILD.gn

+    ]
+  } else {
+    ldflags += [
+      "-O1",


Should we use something higher for release builds?

yjbanov · 2023-02-27T02:13:59Z

lib/web_ui/skwasm/helpers.h

+}
+
+inline SkRRect createRRect(const SkScalar* f) {
+  const SkScalar* twelveFloats = reinterpret_cast<const SkScalar*>(f);


This cast looks like an noop.

yjbanov · 2023-02-27T02:20:29Z

lib/web_ui/skwasm/paint.cpp

+}
+
+SKWASM_EXPORT void paint_destroy(SkPaint* paint) {
+  delete paint;


What's the ownership model for SkPaint? Is it safe to delete the object if it's been used in a picture?

Skwasm Renderer - initial implementation

eyebrowsoffire added 20 commits November 3, 2022 13:28

Started porting prototype over.

f2be860

A few fixes.

b5a42f4

Converted wasm pragmas to FfiNative annotations.

313a055

Implement a few methods of the renderer.

5c953e6

Some little lint errors.

1badfec

A bit of wiring up of the test harness.

7cb9a08

Fix some formatting issues.

73ee925

Minor changes to skwasm renderer.

cb65c7b

Merge branch 'main' into skwasm_renderer

1b6f39b

A few fixes to get things compiling via dart2wasm.

f3192dd

Use futureToPromise.

dbe87a1

Some more fixes for dart2wasm.

8397290

Allow promises to have nullable objects.

69c4c86

Merge branch 'main' into wasm_fixes

7d61204

Merge branch 'wasm_fixes' into skwasm_renderer

ea59128

Some changes to skwasm.

831be2e

Merge branch 'main' into skwasm_renderer

ace664b

Fix up some signatures.

5db2a4a

More expressive warnings.

bfbb993

Merge branch 'main' into skwasm_renderer

adef70b

flutter-dashboard bot added the platform-web Code specifically for the web engine label Jan 23, 2023

flutter-dashboard bot added the needs tests label Jan 23, 2023

eyebrowsoffire marked this pull request as draft January 23, 2023 19:35

eyebrowsoffire added 4 commits January 23, 2023 13:13

Merge branch 'main' into skwasm_renderer

1927e0c

Merge branch 'main' into skwasm_renderer

8fc257c

Switch @FfiNative to @Native.

6e4ddd1

Unit tests partially working.

0461799

License headers.

f45f7ff

mdebbar approved these changes Feb 27, 2023

View reviewed changes

eyebrowsoffire requested a review from harryterkelsen February 28, 2023 18:44

eyebrowsoffire added 2 commits February 28, 2023 13:39

Merge branch 'main' into skwasm_renderer

fd91011

Merge branch 'main' into skwasm_renderer

68110f9

harryterkelsen approved these changes Mar 1, 2023

View reviewed changes

Address Harry's comments and change the archive structure.

9b43948

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

mdebbar reviewed Mar 1, 2023

View reviewed changes

auto-submit bot merged commit a749820 into flutter:main Mar 2, 2023

engine-flutter-autoroll mentioned this pull request Mar 2, 2023

Roll Flutter Engine from f85e4c667575 to 77bc5449e20d (4 revisions) flutter/flutter#121763

Closed

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

a749820e5 Skwasm Renderer - initial implementation (flutter/engine#39072

2923edf

)

engine-flutter-autoroll mentioned this pull request Mar 2, 2023

Roll Flutter Engine from f85e4c667575 to 0c9323ec5a2b (5 revisions) flutter/flutter#121764

Closed

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

a749820e5 Skwasm Renderer - initial implementation (flutter/engine#39072

7f48d4f

)

engine-flutter-autoroll mentioned this pull request Mar 2, 2023

Roll Flutter Engine from f85e4c667575 to 056290127820 (6 revisions) flutter/flutter#121765

Closed

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

a749820e5 Skwasm Renderer - initial implementation (flutter/engine#39072

e5f0c89

)

engine-flutter-autoroll mentioned this pull request Mar 2, 2023

Roll Flutter Engine from f85e4c667575 to 0661e2717559 (7 revisions) flutter/flutter#121766

Closed

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

a749820e5 Skwasm Renderer - initial implementation (flutter/engine#39072

4e4efdb

)

engine-flutter-autoroll mentioned this pull request Mar 2, 2023

Roll Flutter Engine from f85e4c667575 to 23d2931f4896 (8 revisions) flutter/flutter#121767

Closed

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

a749820e5 Skwasm Renderer - initial implementation (flutter/engine#39072

ad11165

)

engine-flutter-autoroll mentioned this pull request Mar 2, 2023

Roll Flutter Engine from f85e4c667575 to f73ac7d27f1b (9 revisions) flutter/flutter#121771

Closed

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

a749820e5 Skwasm Renderer - initial implementation (flutter/engine#39072

b61020c

)

engine-flutter-autoroll mentioned this pull request Mar 2, 2023

Roll Flutter Engine from f85e4c667575 to 24e69c8fb1fd (10 revisions) flutter/flutter#121772

Closed

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

a749820e5 Skwasm Renderer - initial implementation (flutter/engine#39072

377e8fd

)

engine-flutter-autoroll mentioned this pull request Mar 2, 2023

Roll Flutter Engine from f85e4c667575 to 24e69c8fb1fd (10 revisions) flutter/flutter#121776

Closed

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

a749820e5 Skwasm Renderer - initial implementation (flutter/engine#39072

78314f7

)

engine-flutter-autoroll mentioned this pull request Mar 2, 2023

Roll Flutter Engine from f85e4c667575 to 385d02891638 (11 revisions) flutter/flutter#121779

Merged

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

a749820e5 Skwasm Renderer - initial implementation (flutter/engine#39072

d0c333b

)

yjbanov reviewed Mar 2, 2023

View reviewed changes

chinmaygarde pushed a commit to chinmaygarde/flutter_engine that referenced this pull request Mar 3, 2023

Skwasm Renderer - initial implementation (flutter#39072)

c0bca01

Skwasm Renderer - initial implementation

mdebbar mentioned this pull request Mar 7, 2023

Revert "[web:tools] always use CanvasKit from the cache when building web apps (#93002)" flutter/flutter#117693

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skwasm Renderer - initial implementation #39072

Skwasm Renderer - initial implementation #39072

eyebrowsoffire commented Jan 23, 2023

flutter-dashboard bot commented Jan 23, 2023

mdebbar left a comment

mdebbar Feb 27, 2023

harryterkelsen left a comment

harryterkelsen Feb 28, 2023

harryterkelsen Feb 28, 2023

harryterkelsen Feb 28, 2023

mdebbar Mar 1, 2023

eyebrowsoffire Mar 1, 2023

mdebbar Mar 1, 2023

yjbanov Feb 27, 2023

yjbanov Feb 27, 2023

yjbanov Feb 27, 2023

		@@ -0,0 +1,63 @@
		# Copyright 2019 The Flutter Authors. All rights reserved.

Skwasm Renderer - initial implementation #39072

Skwasm Renderer - initial implementation #39072

Conversation

eyebrowsoffire commented Jan 23, 2023

flutter-dashboard bot commented Jan 23, 2023

mdebbar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harryterkelsen 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