Image.toByteData and Picture.toImage implementations (#2) #20326

deakjahn · 2020-08-07T10:11:22Z

Description

Image.toByteData() was not implemented in either DomCanvas or CanvasKit. This PR covers both.
Picture.toImage() was not implemented in either DomCanvas or CanvasKit. This PR covers CanvasKit (I also have a solution for DomCanvas but that would need some discussion, it might clash with functionality somewhere else; details in a comment below).

Related Issues

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.

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.

Implement toByteData()

Null-aware.

Sk* to Ck*

Null-aware

Type arguments

Same for CkAnimatedImage

Implement toByteData()

yjbanov · 2020-08-13T18:11:55Z

lib/web_ui/lib/src/engine/compositor/canvaskit_api.dart

+  external bool isEmpty();
+  external bool isOpaque();
+  external SkRect bounds();
+  external int width();


These should be getters I imagine. However, getters don't need the (), for example external bool get isEmpty; (notice no () at the end).

I don't know any more. The doc I finally found explicitely says they are functions, using ().

Hmm. Interesting. According to the @JS bindings it's a plain object (in JS you'd write { width: x, height: y, aphaType: z, colorSpace: a, colorType: b }). Perhaps there's a difference between SkImageInfo constructed by JS and one returned by CanvasKit?

Why don't you do what works now, and we can clean this up later?

/cc @kjlubick

According to CanvasKit bindings it should be a POJO: https://github.com/google/skia/blob/1d4b92d87806e0d3ed8b4155da98d7d0ac757bd6/modules/canvaskit/canvaskit_bindings.cpp#L1873

That is, there shouldn't be any methods/functions on SkImageInfo, only properties.

That is correct, the SkImageInfo is just a POJO

OK, I do, I do.

yjbanov · 2020-08-13T18:40:04Z

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

+        .build()
+        .webOnlyRootElement);
+
+    await matchGoldenFile('canvas_to_picture.png', region: region, write: true);


For this to pass on CI you'll need to remove write: true. However, you will also need to add the new screenshot file to https://github.com/flutter/goldens (example PR: flutter/goldens#103).

If that's too much ceremony, feel free to comment out this line in this PR, and I can do the dance with the screenshots in a follow up PR.

I simply can't get the screenshot. The test doesn't run on either Windows or Linux for me locally (on Windows by design, on Linux it should work but it doesn't, Chrome starts and then nothing ever happens).

Commenting out renders the test inactive. If it's OK like this, let's do so.

Filed flutter/flutter#63711 to follow up later.

deakjahn · 2020-08-13T18:40:41Z

Same error with Linux Web Engine. I can't find any reference to color spaces in that log.

yjbanov · 2020-08-19T01:42:59Z

@deakjahn Sorry, I renamed the compositor directory to canvaskit today, so this PR needs to be rebased again 😅

It looks like this PR is almost good to go. The only remaining issue is an unused import.

deakjahn · 2020-08-19T08:51:21Z

An import? Did I add it? Which one? I don't remember needing to add any import at all but that doesn't mean a thing. :-)

Never mind, I could have read before I speak...

deakjahn · 2020-08-22T21:19:03Z

You made me comment out the image comparison test at the end but this means that the test itself times out, doesn't it?

yjbanov · 2020-08-22T23:55:38Z

I'm not sure. Would you be able to squash this PR into a single commit so I could try it on my computer? I tried rebasing this PR on top of latest master branch but my git skills turned out insufficient :)

deakjahn · 2020-08-23T09:17:17Z

That's the problem over here, too, I never did anything similar in git. Time to start to read up, I guess.

deakjahn · 2020-08-23T09:43:08Z

I can't even squash two, let alone 70-plus... :-(

deakjahn · 2020-08-23T21:24:21Z

I have a suggestion that might help us. It's only six files, after all. So, if

I close this PR, too,
Delete my fork,
Fork the engine again,
Copy the six modified files and commit them in one go,
Create a third PR

that might fly, I guess.

yjbanov · 2020-08-25T17:51:48Z

That plan SGTM. I'm wondering if Github allows downloading a PR as a patch file that could be applied locally as a single commit 🤔

deakjahn · 2020-08-25T18:19:35Z

It would if I squashed it into one. But then there would be no need...

deakjahn added 30 commits July 28, 2020 20:02

Update image.dart

93e3f0b

Implement toByteData()

Update html_image_codec.dart

cc827d6

Implement toByteData()

rebase

6b9fd73

Update image.dart

352b31a

Null-aware.

Update picture.dart

4ea7ef2

Sk* to Ck*

Update picture.dart

f99c8b2

Update picture.dart

1e7188f

Null-aware

Update html_image_codec.dart

4017e40

Update image.dart

ed811cf

Type arguments

Update image.dart

d6dc410

Same for CkAnimatedImage

Revert

c2c70e4

Tested by local engine

8b1ed1a

Add files via upload

bad4d45

Delete picture_to_image_test.dart

22dbcf7

Test #1

5eb7b8a

Test to read only

1feae02

Finally

4c9001b

Sorry, spaces

a2ff894

Test, finally

0c72496

@js refactoring

91b12ca

@js refactoring

9eeb7e0

SkData and SkImageInfo

6efd763

Unused items

22d2f96

Test to write PNG

f1d49be

Remove comments

6675e13

Add comments

ced934c

CK test

7b78b27

Comment about copying

11c4073

Update image.dart

72ef505

Implement toByteData()

Update html_image_codec.dart

1afe140

Implement toByteData()

deakjahn added 2 commits August 13, 2020 19:37

SkImageInfo correct names

078d6bc

SkImageInfo correct names

5dd02dd

yjbanov reviewed Aug 13, 2020

View reviewed changes

deakjahn added 2 commits August 13, 2020 22:26

SkImageInfo properties

5f4d5e7

Inactive test

107ba3d

yjbanov mentioned this pull request Aug 13, 2020

Add test for Image.toByteData/Picture.toImage flutter/flutter#63711

Closed

Merge remote-tracking branch 'upstream/master'

93cd91d

Unused import

69ec1cc

yjbanov added the platform-web Code specifically for the web engine label Aug 22, 2020

yjbanov self-assigned this Aug 22, 2020

yjbanov mentioned this pull request Aug 22, 2020

How to capture a custom widget having multiple custom painter as an image in web flutter/flutter#63955

Closed

deakjahn added 5 commits August 23, 2020 11:19

Merge remote-tracking branch 'upstream/master'

9d5a611

Squash 2

6e1d95d

Merge branch 'master' of https://github.com/deakjahn/engine

7da3c86

Squash 2.

90d0b69

Merge branch 'master' of https://github.com/deakjahn/engine

4c39264

Merge remote-tracking branch 'upstream/master'

9cf68f8

deakjahn closed this Aug 25, 2020

deakjahn mentioned this pull request Aug 25, 2020

Image.toByteData and Picture.toImage implementations (#3) #20750

Merged

9 tasks

Image.toByteData and Picture.toImage implementations (#2) #20326

Image.toByteData and Picture.toImage implementations (#2) #20326

Uh oh!

Conversation

deakjahn commented Aug 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

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!

deakjahn commented Aug 13, 2020

Uh oh!

yjbanov commented Aug 19, 2020

Uh oh!

deakjahn commented Aug 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deakjahn commented Aug 22, 2020

Uh oh!

yjbanov commented Aug 22, 2020

Uh oh!

deakjahn commented Aug 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deakjahn commented Aug 23, 2020

Uh oh!

deakjahn commented Aug 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yjbanov commented Aug 25, 2020

Uh oh!

deakjahn commented Aug 25, 2020

Uh oh!

Uh oh!

deakjahn commented Aug 7, 2020 •

edited

Loading

deakjahn commented Aug 19, 2020 •

edited

Loading

deakjahn commented Aug 23, 2020 •

edited

Loading

deakjahn commented Aug 23, 2020 •

edited

Loading