Refactor BitmapCanvas. Fix image compositing bug #15114

ferhatb · 2020-01-03T18:01:09Z

Added endOfPaint call on recording canvas to allow bitmap canvas to dispose after reuse.
Moved all immediate canvas calls from BitmapCanvas into CanvasPool.
Reduced context updates by creating ContextHandle class.
Added regression tests for drawing on top of image in a single Picture.

Fixes flutter/flutter#44845
Fixes flutter/flutter#48032

googlebot · 2020-01-06T15:48:09Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

googlebot · 2020-01-06T15:48:09Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

yjbanov · 2020-01-03T18:45:13Z

lib/web_ui/lib/src/engine/bitmap_canvas.dart

-      _canvas.style.transform = '';
+      _canvasPool.reuse();
+      _setupInitialTransform();
+      _canvasPool.contextHandle.reset();


It seems the logic for resetting the context handle can move into _CanvasPool.reuse().

Lazy allocation changes this quite a bit where initialTransform becomes setter. Not addressing in this PR.

yjbanov · 2020-01-03T18:45:38Z

lib/web_ui/lib/src/engine/bitmap_canvas.dart

-      -_bounds.top + canvasPositionCorrectionY + kPaddingPixels,
-    );
+  // Returns a data URI containing a representation of the image in this
+  // canvas.


yjbanov · 2020-01-03T18:46:22Z

lib/web_ui/lib/src/engine/canvas_pool.dart

@@ -0,0 +1,694 @@
+part of engine;


This file needs a license header.

yjbanov · 2020-01-03T18:47:11Z

lib/web_ui/lib/src/engine/canvas_pool.dart

@@ -0,0 +1,694 @@
+part of engine;
+
+class _CanvasPool extends _SaveStackTracking {


Although this class is private it's pretty non-trivial, so some docs could help understand what it does in the future.

yjbanov · 2020-01-03T18:52:02Z

lib/web_ui/lib/src/engine/canvas_pool.dart

+    }
+    if (_reusablePool != null && _reusablePool.isNotEmpty) {
+      _canvas = _reusablePool[0];
+      _reusablePool.removeAt(0);


_canvas = _reusablePool.removeAt(0); will save one list look-up.

yjbanov · 2020-01-06T17:11:39Z

lib/web_ui/test/engine/recording_canvas_test.dart

    });

    test('Inner RRect not completely inside Outer RRect', () {
      underTest.drawDRRect(rrect, rrect.deflate(1).shift(const Offset(0.0, 10)), somePaint);
      underTest.apply(mockCanvas);
      // Expect nothing to be called
-      expect(mockCanvas.methodCallLog.length, equals(0));
+      expect(mockCanvas.methodCallLog.length, equals(1));


yjbanov · 2020-01-06T17:11:42Z

lib/web_ui/test/engine/recording_canvas_test.dart

    });

    test('Inner RRect same as Outer RRect', () {
      underTest.drawDRRect(rrect, rrect, somePaint);
      underTest.apply(mockCanvas);
      // Expect nothing to be called
-      expect(mockCanvas.methodCallLog.length, equals(0));
+      expect(mockCanvas.methodCallLog.length, equals(1));


yjbanov · 2020-01-06T17:12:32Z

lib/web_ui/test/golden_tests/engine/canvas_draw_image_golden_test.dart

@@ -50,6 +50,7 @@ void main() async {
    RecordingCanvas(const Rect.fromLTRB(0, 0, 400, 300));
    rc.save();
    rc.drawImage(createTestImage(), Offset(0, 0), new Paint());
+    rc.restore();


Restores are optional in Flutter AFAIK. What happens if you do not call it?

Changed [_CanvasPool] to count save/restore to handle case.

yjbanov · 2020-01-06T17:12:57Z

lib/web_ui/test/golden_tests/engine/canvas_draw_image_golden_test.dart

+  // Circle should draw on top of image not below.
+  test('Paints on top of image', () async {
+    final RecordingCanvas rc =
+    RecordingCanvas(const Rect.fromLTRB(0, 0, 400, 300));


nit: indentation is off (throughout the file)

dartfmt Done.

yjbanov · 2020-01-06T17:16:06Z

lib/web_ui/test/golden_tests/engine/canvas_draw_image_golden_test.dart

+
+  // Regression test for https://github.com/flutter/flutter/issues/44845
+  // Circle should draw on top of image not below.
+  test('Paints on top of image', () async {


We should also have a test that checks that we can still paint under the image :)

Done. flutter/goldens#69

Refactor BitmapCanvas. Fix image compositing bug

e07d553

ferhatb requested a review from yjbanov January 3, 2020 18:01

auto-assign bot requested a review from stuartmorgan-g January 3, 2020 18:01

googlebot added the cla: yes label Jan 3, 2020

yjbanov mentioned this pull request Jan 3, 2020

Flutter application which use canvas to build self-CustomPainter cannot work on browser if i used the release mode by command "flutter run -d chrome --release" or "flutter build web". flutter/flutter#47967

Closed

Add license header. Fix context restore

9ae9063

googlebot added cla: no and removed cla: yes labels Jan 6, 2020

yjbanov reviewed Jan 6, 2020

View reviewed changes

ferhatb added 4 commits January 6, 2020 10:37

Addressed review comments

8bb73a7

dartfmt

67c1fb9

remove golden write

a751fd9

Added TODO to simplify pool

6b1d436

ferhatb closed this Jan 6, 2020

		@@ -0,0 +1,694 @@
		part of engine;

		class _CanvasPool extends _SaveStackTracking {

Refactor BitmapCanvas. Fix image compositing bug #15114

Refactor BitmapCanvas. Fix image compositing bug #15114

Uh oh!

Conversation

ferhatb commented Jan 3, 2020

Uh oh!

googlebot commented Jan 6, 2020

Uh oh!

googlebot commented Jan 6, 2020

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!

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!

Uh oh!