[canvaskit] Decode images using <img> tag decoding #53201

harryterkelsen · 2024-06-04T19:58:29Z

Prefer to decode images using the browser API rather than with CanvasKit to avoid jank when decoding.

Part of deprecating the HTML renderer: flutter/flutter#145954

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C , Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

flutter-dashboard · 2024-06-24T20:04:36Z

This pull request executed golden file tests, but it has not been updated in a while (20 days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

harryterkelsen · 2024-07-11T20:42:47Z

This change causes the CanvasKit renderer to default to decoding using ImageDecoder when it's available, or using an ImageElement otherwise. To facilitate this, some helper methods were added which can read the pixels and resize ImageElement images using a DOM canvas, as a workaround to a bug in CanvasKit where getting pixel data from images created from textures returns an array of all 0s.

lib/web_ui/lib/src/engine/canvaskit/image.dart

yjbanov · 2024-07-16T19:51:55Z

lib/web_ui/lib/src/engine/canvaskit/image.dart

+ }
+
+ final int scaledWidth = scaledSize.width.toInt();
+ final int scaledHeight = scaledSize.height.toInt();


Maybe scaledImageSize should switch to BitmapSize to avoid rounding? There's also the issue that we check for allowUpscaling before calling toInt, which may end up upscaling anyway.

Switched to BitmapSize to avoid ugly .toInt() and .toDouble() calls where possible. Checking for allowUpscaling before calling toInt() should be okay here since we're only working with ints for the width and height.

yjbanov · 2024-07-16T19:55:22Z

lib/web_ui/lib/src/engine/canvaskit/image.dart

+ scaledHeight,
+ );
+ final DomImageData imageData =
+ ctx.getImageData(0, 0, scaledWidth, scaledHeight);


Safari can run out 2D canvas memory easily. To prevent that set the size of the htmlCanvas to 0x0. That forces Safari to reclaim the underlying image buffer eagerly. Alternatively, maybe the htmlCanvas could be reused. If the app scaled an image once, it may do it again, but that's speculative.

yjbanov · 2024-07-16T19:59:20Z

lib/web_ui/lib/src/engine/canvaskit/image.dart

+ height: scaledHeight,
+ );
+ final DomCanvasRenderingContext2D ctx =
+ htmlCanvas.getContext('2d')! as DomCanvasRenderingContext2D;


I'm guessing that despite the slowness (due to https://bugs.webkit.org/show_bug.cgi?id=267291) a 2d OffscreenCanvas with transferToImageBitmap would still be faster than an on-screen 2d canvas getImageData. The latter is guaranteed to always copy, in addition to other copying that happens in canvasKit.MakeImage.

However, if use ImageBitmap as the source, then we'd have to use MakeLazyImageFromTextureSource, which has been a bit unreliable.

Unreliable how? I think we should try to avoid explicitly pulling the bytes out of the image. Is there a reason we can't just create an ImageBitmap and use that, instead of grabbing the image data? I also should mention that createImageBitmap actually allows you to scale the image while extracting it from the image source: https://developer.mozilla.org/en-US/docs/Web/API/createImageBitmap

yjbanov · 2024-07-16T20:19:40Z

lib/web_ui/lib/src/engine/canvaskit/image.dart

+ /// For images which are decoded via an HTML Image element, this field holds
+ /// the image element from which this image was created.
+ ///
+ /// Skia owns the image element and will close it when it's no longer used.


How does Skia close the image element? VideoFrame and ImageBitmap provide the close() method, but is there something equivalent for <img> tag?

I checked the Skia code and it only automatically disposes the VideoFrame. We should manually dispose of the ImageBitmap when dispose is called. As for the <img> tag, I think the best we can do is let it be garbage collected with this Image.

yjbanov · 2024-07-16T20:24:17Z

lib/web_ui/lib/src/engine/canvaskit/image.dart

+ return CkImage.cloneOf(
+ box,
+ videoFrame: videoFrame?.clone(),
+ imageElement: imageElement,


Does this need to do anything with bitmapImage? If not, it would be good to explain why not.

Yes, it should. I overlooked the clone call when adding ImageBitmap here.

lib/web_ui/lib/src/engine/canvaskit/image.dart

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

lib/web_ui/lib/src/engine/html_image_element_codec.dart

…erkelsen/engine into pr/harryterkelsen/53201

eyebrowsoffire · 2024-07-18T21:05:49Z

lib/web_ui/lib/src/engine/canvaskit/image.dart

+ final int scaledWidth = scaledSize.width;
+ final int scaledHeight = scaledSize.height;
+
+ final DomOffscreenCanvas offscreenCanvas = createDomOffscreenCanvas(


I think you can avoid creating an offscreen canvas and so on here by just using createImageBitmap instead. https://developer.mozilla.org/en-US/docs/Web/API/createImageBitmap It has resizeWidth and resizeHeight options which will to the rescaling for you.

Harry pointed out that createImageBitmap is async, which makes this much less clean. So we should leave it as is.

eyebrowsoffire

LGTM!

eyebrowsoffire · 2024-07-18T21:21:48Z

lib/web_ui/lib/src/engine/canvaskit/image.dart

+ final int scaledWidth = scaledSize.width;
+ final int scaledHeight = scaledSize.height;
+
+ final DomOffscreenCanvas offscreenCanvas = createDomOffscreenCanvas(


Harry pointed out that createImageBitmap is async, which makes this much less clean. So we should leave it as is.

flutter-dashboard · 2024-07-18T22:16:23Z

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

Changes reported for pull request #53201 at sha e7aff46

…/engine#53201)

…152046) flutter/engine@9739d92...6312dfc 2024-07-19 1961493 [email protected] [canvaskit] Decode images using <img> tag decoding (flutter/engine#53201) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/ doc/main/autoroll/README.md

harryterkelsen added 7 commits May 24, 2024 16:18

Use HtmlImageElement for image decoding in skia

49ed8d4

start touching up tests

6b248d1

Merge branch 'main' into canvaskit-html-img-element-decode

d780fe3

eagerly decode

4e43312

Merge branch 'main' into canvaskit-html-img-element-decode

9b1a448

wip

e6d4d44

Merge branch 'main' into canvaskit-html-img-element-decode

8ea4865

harryterkelsen added the Work in progress (WIP) Not ready (yet) for review! label Jun 4, 2024

github-actions bot added the platform-web Code specifically for the web engine label Jun 4, 2024

harryterkelsen added 7 commits July 2, 2024 10:16

wip

88d685d

Merge branch 'main' into canvaskit-html-img-element-decode

23cb86b

Merge branch 'main' into canvaskit-html-img-element-decode

7883559

WIP

8a1a7ce

Merge branch 'main' into canvaskit-html-img-element-decode

389369d

Fix ImageBitmap toByteData case

d900ded

Fall back to Skia decoding for GIF and WEBP images

355891b

harryterkelsen removed the Work in progress (WIP) Not ready (yet) for review! label Jul 11, 2024

harryterkelsen added 2 commits July 11, 2024 13:39

Merge branch 'main' into canvaskit-html-img-element-decode

cfc1c4a

Undo semantics change

d317302

harryterkelsen requested a review from yjbanov July 11, 2024 20:42

harryterkelsen added 5 commits July 11, 2024 13:47

Remove unused readImageElementPixelsUnmodified method

c579c8e

remove outdated comment in test

dd5c8d6

delete old unused tests

f3997c1

fix analysis errors

52b8330

Merge branch 'main' into canvaskit-html-img-element-decode

b207ee7

yjbanov reviewed Jul 16, 2024

View reviewed changes

Merge branch 'main' into pr/harryterkelsen/53201

4f5f863

harryterkelsen mentioned this pull request Jul 17, 2024

[canvaskit] Determine if image is animated by reading its header and use Skia to decode in that case flutter/flutter#151911

Open

harryterkelsen and others added 3 commits July 17, 2024 16:10

Respond to review comments

7c6ab13

Merge branch 'main' into canvaskit-html-img-element-decode

0b426fe

Merge branch 'canvaskit-html-img-element-decode' of github.com:harryt…

c06797c

…erkelsen/engine into pr/harryterkelsen/53201

harryterkelsen force-pushed the canvaskit-html-img-element-decode branch from 0b426fe to b207ee7 Compare July 18, 2024 21:01

eyebrowsoffire requested changes Jul 18, 2024

View reviewed changes

eyebrowsoffire approved these changes Jul 18, 2024

View reviewed changes

Remove unnecessary await

e7aff46

flutter-dashboard bot added the will affect goldens label Jul 18, 2024

harryterkelsen added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 19, 2024

auto-submit bot merged commit 6312dfc into flutter:main Jul 19, 2024
32 checks passed

engine-flutter-autoroll mentioned this pull request Jul 19, 2024

Roll Flutter Engine from 9739d92f2696 to 6312dfc492cd (1 revision) flutter/flutter#152046

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 19, 2024

6312dfc49 [canvaskit] Decode images using <img> tag decoding (flutter…

ffd26b8

…/engine#53201)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[canvaskit] Decode images using <img> tag decoding #53201

[canvaskit] Decode images using <img> tag decoding #53201

harryterkelsen commented Jun 4, 2024 •

edited

Loading

flutter-dashboard bot commented Jun 24, 2024

harryterkelsen commented Jul 11, 2024

yjbanov Jul 16, 2024

harryterkelsen Jul 17, 2024

yjbanov Jul 16, 2024

harryterkelsen Jul 17, 2024

yjbanov Jul 16, 2024

harryterkelsen Jul 17, 2024

eyebrowsoffire Jul 18, 2024

yjbanov Jul 16, 2024

harryterkelsen Jul 17, 2024

yjbanov Jul 16, 2024

harryterkelsen Jul 17, 2024

eyebrowsoffire Jul 18, 2024

eyebrowsoffire Jul 18, 2024

eyebrowsoffire left a comment

eyebrowsoffire Jul 18, 2024

flutter-dashboard bot commented Jul 18, 2024

[canvaskit] Decode images using <img> tag decoding #53201

[canvaskit] Decode images using <img> tag decoding #53201

Conversation

harryterkelsen commented Jun 4, 2024 • edited Loading

Pre-launch Checklist

flutter-dashboard bot commented Jun 24, 2024

harryterkelsen commented Jul 11, 2024

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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flutter-dashboard bot commented Jul 18, 2024

harryterkelsen commented Jun 4, 2024 •

edited

Loading