[web] Fixes canvas pixelation and overallocation due to transforms. #22160

ferhatb · 2020-10-28T03:20:34Z

Description

This PR introduces density to PersistedPicture. After Scene preroll, the picture uses the absolute transform to compute best scalar to use to match physical pixels to canvas pixels and rescales recording canvas operations.
The scalar (density) is clamped to make sure we don't exceed webkit allocation limits.
Added code to free up canvas pool when canvas.getContext('2d') fails due to insufficient memory.

Related Issues

Tests

Added SceneBuilder test that verifies when using a 0.5,0.5 scale transform, a smaller canvas is allocated instead of bounds.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the [contributor guide] and followed the process outlined there for submitting PRs.
I signed the [CLA].
I read and followed the [C++, Objective-C, Java style guides] for the engine.
I read the [tree hygiene] wiki page, which explains my responsibilities.
I updated/added relevant documentation.
All existing and new tests are passing.
I am willing to follow-up on review comments in a timely manner.

Reviewer Checklist

I have submitted any presubmit flakes in this PR using the [engine presubmit flakes form] before re-triggering the failure.

Breaking Change

Did any tests fail when you ran them? Please read [handling breaking changes].

No, no existing tests failed, so this is not a breaking change.
Yes, this is a breaking change. If not, delete the remainder of this section.
- I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
- I got input from the developer relations team, specifically from: Replace with the names of who gave advice
- I wrote a migration guide: https://flutter.dev/go/breaking-changes-template Replace this with a link to a pull request that adds the migration guide to https://flutter.dev/docs/release/breaking-changes

…moryUsage

yjbanov · 2020-10-28T23:22:34Z

lib/web_ui/lib/src/engine/html/surface.dart

+        }
+        if (newChild.rootElement == null) {
+          print(newChild);
+        }


Yes. Removed.

yjbanov · 2020-10-28T23:22:49Z

lib/web_ui/test/engine/surface/scene_builder_test.dart

  });

-  group('SceneBuilder', () {
+ group('SceneBuilder', () {


yjbanov · 2020-10-28T23:23:10Z

lib/web_ui/test/engine/surface/scene_builder_test.dart

+  test('Canvas should allocate fewer pixels if scaled', () async {
+    final SurfaceSceneBuilder builder = SurfaceSceneBuilder();
+    final Picture picture1 = _drawPicture();
+    builder.pushClipRect(const Rect.fromLTRB(10, 10, 300, 300),);


unnecessary trailing comma

yjbanov · 2020-10-28T23:24:02Z

lib/web_ui/test/engine/surface/scene_builder_test.dart

    await testCase('', 'remove all', deletions: 2);
  });
+
+  test('Canvas should allocate fewer pixels if scaled', () async {


scaled as in "zoomed in" or "zoomed out"? (I know the answer now, but will need to think longer in a few months)

Updated comment. thx.

yjbanov · 2020-10-28T23:26:11Z

lib/web_ui/test/engine/surface/scene_builder_test.dart

+    // Force update to scene which will utilize reuse code path.
+    final SurfaceSceneBuilder builder2 = SurfaceSceneBuilder();
+    builder2.pushOffset(0, 0);
+    builder2.pushTransform(Matrix4.identity().scaled(0.5, 0.5).toFloat64());


we also need a test the zooms the picture in and makes sure that the growth in pixel count stops at screen size (i.e. that we're not allocating enormous canvases)

When you zoom out, the big canvas becomes small ie scale factor < 1. I added another test case for zoom in.

yjbanov · 2020-10-28T23:28:24Z

lib/web_ui/lib/src/engine/html/picture.dart

+  if (scale < kEpsilon || scale == 1) {
+    // Handle local paint bounds scaled to 0, typical when using
+    // transform animations.
+    return 1.0;


Shouldn't this be if ((scale - 1).abs() < kEpsilon)? Also scale 0 probably means "empty picture".

Added and updated comment. It guards the code below from divide by zero.

yjbanov · 2020-10-28T23:30:12Z

lib/web_ui/lib/src/engine/html/picture.dart

+    //
+    // On a fullscreen high dpi device dpi*density*resolution will demand
+    // too much memory, so clamp at 4.
+    scale = math.min(4.0, ((scale / 2.0).ceil() * 2.0));


Should we normalize before the previous if statement? This way if scale snaps to 1x we just return 1.

I like the early exit. For many apps most of the time scale is just 1.0 unless you use boxfit.

yjbanov

…forms. (flutter/engine#22160)

* c597333 Roll Skia from f548a028ce70 to c21902c0d3cc (46 revisions) (flutter/engine#22224) * 37d766c Fix includes to start with shell (flutter/engine#22227) * 1ad6765 [web] Fixes canvas pixelation and overallocation due to transforms. (flutter/engine#22160) * 9f9fc1f Roll Skia from c21902c0d3cc to 9615bcf71f2a (1 revision) (flutter/engine#22226) * 0faa72e Roll Dart SDK from fed66f60a3bc to 25ef5dc559cf (1 revision) (flutter/engine#22225) * 172a393 Report image diff status for iOS scenario golden tests (flutter/engine#22230) * fddabca updating integration tests version. (flutter/engine#22235) * a36bcdc Roll Fuchsia Mac SDK from mhak7e_o6... to 8SkbMXJJ9... (flutter/engine#22231) * 6a331d3 Roll Skia from 9615bcf71f2a to d5e6368fffd0 (8 revisions) (flutter/engine#22234) * 9b34207 Fixing semantics borders on mobile web (flutter/engine#21856) * 4e9459e Refactored the FlutterEngine to make it easier to implement spawn functionality (flutter/engine#21890) * fa77e68 disable AppLifecycleTests (flutter/engine#22236) * 153775b update golden (flutter/engine#22247) * 50dbe85 [web] fix hot restart type error (flutter/engine#22248) * c8cf09a Roll Skia from d5e6368fffd0 to 7585a65ac709 (7 revisions) (flutter/engine#22237) * 68e2e46 Roll Fuchsia Mac SDK from 8SkbMXJJ9... to Pz4ZHZrUp... (flutter/engine#22246) * d3182bc Roll Dart SDK from 25ef5dc559cf to 5acb5fcf84cb (4 revisions) (flutter/engine#22243) * 9b4bb20 makes android semanticsnode to ignore hittest if it is not focusable (flutter/engine#22205) * 3c7a54e Roll Fuchsia Linux SDK from sNx8qabBn... to QqGvMWaYk... (flutter/engine#22244) * eea98b2 Roll Skia from 7585a65ac709 to dffd20efe95c (14 revisions) (flutter/engine#22250) * 46e3bba Defer Windows arrow key and delete handling (flutter/engine#22207) * 14437d6 fix _getArrayBuffer signature (flutter/engine#22251) * 8defec6 Fix nullability issue with Image.network (flutter/engine#22252) * 67d55ed Roll Dart SDK from 5acb5fcf84cb to a9d583383410 (4 revisions) (flutter/engine#22255) * 7c8f57c Report error when instantiating CanvasKit network image (flutter/engine#22159) * 9945db3 Remove the metrics task from cirrus. (flutter/engine#22240) * bd19181 Add braces on if statements to match linter style (flutter/engine#22130) * e9c62e7 do not print in _computePixelDensity (flutter/engine#22257) * 2617101 Roll Dart SDK from a9d583383410 to d2577410a501 (1 revision) (flutter/engine#22258) * 3d194fa Switch macOS embedding to proc table embedder API (flutter/engine#21811) * 31b6f0b Roll Fuchsia Mac SDK from Pz4ZHZrUp... to 6yEx5GNGG... (flutter/engine#22262) * 78a0181 Roll Fuchsia Linux SDK from QqGvMWaYk... to oLF1FW-gC... (flutter/engine#22264) * ccdb681 WeakPersistentHandle migration (flutter/engine#19843) * ce0a30c Roll Dart SDK from 52783837369d to b43baaaa477d (723 revisions) (flutter/engine#22265) * 59b01e0 [web] Fix repaint logic for cullrect,transform changes (flutter/engine#22273)

…lutter#22160)

ferhatb added 4 commits October 23, 2020 12:40

Protect against canvas constructor NNBD optimizing out reduceCanvasMe…

cbf8134

…moryUsage

Reduce canvas memory usage when allocation fails

c058364

Merge remote-tracking branch 'upstream/master' into path_exception

5ff86cb

Implement canvas density based on transform stack

01e4843

ferhatb requested review from harryterkelsen and yjbanov October 28, 2020 03:20

google-cla bot added the cla: yes label Oct 28, 2020

remove logging code, fix analyzer warnings

3a0f756

yjbanov reviewed Oct 28, 2020

View reviewed changes

ferhatb added the platform-web Code specifically for the web engine label Oct 29, 2020

ferhatb added 4 commits October 29, 2020 11:15

Merge remote-tracking branch 'upstream/master' into path_exception

2004fda

Address review comments

8d9fe57

fix computePixelDensity to use 4 corners

541e321

fix computeDensity

b44cb99

yjbanov approved these changes Oct 29, 2020

View reviewed changes

ferhatb added 3 commits October 29, 2020 14:32

fix transform y max/min

971b3ec

Only check against limit if scaling up

ae30c96

Update shadow maxdiff due to rendering change

86be729

ferhatb merged commit 1ad6765 into flutter:master Oct 30, 2020

engine-flutter-autoroll mentioned this pull request Nov 2, 2020

Roll Engine from 346051939814 to 14437d6c0006 (22 revisions) flutter/flutter#69599

Closed

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

1ad6765 [web] Fixes canvas pixelation and overallocation due to trans…

5b1ec93

…forms. (flutter/engine#22160)

engine-flutter-autoroll mentioned this pull request Nov 2, 2020

Roll Engine from 346051939814 to 8defec6e8ca1 (23 revisions) flutter/flutter#69608

Closed

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

1ad6765 [web] Fixes canvas pixelation and overallocation due to trans…

1f6d533

…forms. (flutter/engine#22160)

engine-flutter-autoroll mentioned this pull request Nov 2, 2020

Roll Engine from 346051939814 to 67d55ed42b25 (24 revisions) flutter/flutter#69613

Closed

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

1ad6765 [web] Fixes canvas pixelation and overallocation due to trans…

b146cd0

…forms. (flutter/engine#22160)

jonahwilliams mentioned this pull request Nov 2, 2020

Remove usage of --enable-experiment to analysis server flutter/flutter#69614

Merged

engine-flutter-autoroll mentioned this pull request Nov 3, 2020

Roll Engine from 346051939814 to 7c8f57c07d24 (25 revisions) flutter/flutter#69623

Closed

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

1ad6765 [web] Fixes canvas pixelation and overallocation due to trans…

b7a40fc

…forms. (flutter/engine#22160)

engine-flutter-autoroll mentioned this pull request Nov 3, 2020

Roll Engine from 346051939814 to 26171019992a (29 revisions) flutter/flutter#69637

Closed

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

1ad6765 [web] Fixes canvas pixelation and overallocation due to trans…

711f2d0

…forms. (flutter/engine#22160)

engine-flutter-autoroll mentioned this pull request Nov 3, 2020

Roll Engine from 346051939814 to 31b6f0b31a48 (31 revisions) flutter/flutter#69648

Closed

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

1ad6765 [web] Fixes canvas pixelation and overallocation due to trans…

45a3079

…forms. (flutter/engine#22160)

engine-flutter-autoroll mentioned this pull request Nov 3, 2020

Roll Engine from 346051939814 to 78a018174088 (32 revisions) flutter/flutter#69657

Closed

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

1ad6765 [web] Fixes canvas pixelation and overallocation due to trans…

0fe9660

…forms. (flutter/engine#22160)

engine-flutter-autoroll mentioned this pull request Nov 3, 2020

Roll Engine from 346051939814 to ccdb6819ce53 (33 revisions) flutter/flutter#69672

Closed

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

1ad6765 [web] Fixes canvas pixelation and overallocation due to trans…

843d88e

…forms. (flutter/engine#22160)

engine-flutter-autoroll mentioned this pull request Nov 3, 2020

Roll Engine from 346051939814 to ccdb6819ce53 (33 revisions) flutter/flutter#69692

Closed

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

1ad6765 [web] Fixes canvas pixelation and overallocation due to trans…

5ec55f7

…forms. (flutter/engine#22160)

engine-flutter-autoroll mentioned this pull request Nov 3, 2020

Roll Engine from 346051939814 to 59b01e0e5a3f (35 revisions) flutter/flutter#69709

Merged

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

1ad6765 [web] Fixes canvas pixelation and overallocation due to trans…

ac0f619

…forms. (flutter/engine#22160)

chaselatta pushed a commit to chaselatta/engine that referenced this pull request Nov 30, 2020

[web] Fixes canvas pixelation and overallocation due to transforms. (f…

7e2ff5a

…lutter#22160)

ferhatb mentioned this pull request Feb 2, 2021

InteractiveView to cause pixilated CustomPaint flutter/flutter#63162

Closed

[web] Fixes canvas pixelation and overallocation due to transforms. #22160

[web] Fixes canvas pixelation and overallocation due to transforms. #22160

Uh oh!

Conversation

ferhatb commented Oct 28, 2020

Description

Related Issues

Tests

Checklist

Reviewer Checklist

Breaking Change

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!

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!

yjbanov left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants