[canvaskit] Detect animated WebP images #54418

harryterkelsen · 2024-08-07T23:02:15Z

Reads the WebP header to determine if the WebP image is animated or not. If it's not animated, we can use <img> tag decoding for less jank.

The WebP half of flutter/flutter#151911

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.

kevmoo · 2024-08-08T00:14:18Z

lib/web_ui/lib/src/engine/image_format_detector.dart

+/// Reads the header of a WebP file to determine if it is animated or not.
+///
+/// See https://developers.google.com/speed/webp/docs/riff_container
+class WebpHeaderReader {


Nit: I kinda like when these types of things are private, in their own library and just the functional thing as a top-level function bool isAnimated(bytes). With the class as private. One should only call isAnimated once, right?

Done. isAnimated isn't even exposed outside the file. It's an implementation detail of detectImageType

kevmoo · 2024-08-08T00:15:10Z

lib/web_ui/test/engine/image_format_detector_test.dart

+      ImageType? expectedImageType;
+      final String testFileExtension =
+          testFile.substring(testFile.lastIndexOf('.') + 1);
+      switch (testFileExtension) {


might could use switch expression here?

Nice! Didn't know about this in the language

kevmoo · 2024-08-08T20:03:01Z

lib/web_ui/test/engine/image_format_detector_test.dart

+        'avif' => ImageType.avif,
+        'bmp' => ImageType.bmp,
+        'png' => ImageType.png,
+        String() => null,


or

Suggested change

String() => null,

_ => null,

🤷

kevmoo · 2024-08-08T20:04:01Z

lib/web_ui/lib/src/engine/image_format_detector.dart

+      final _WebpHeaderReader webpHeaderReader =
+          _WebpHeaderReader(data.buffer.asByteData());
      if (webpHeaderReader.isAnimated()) {


Suggested change

final _WebpHeaderReader webpHeaderReader =

_WebpHeaderReader(data.buffer.asByteData());

if (webpHeaderReader.isAnimated()) {

if (_WebpHeaderReader(data.buffer.asByteData()).isAnimated()) {

compromise?

meh...not a biggy. I'm being picky

…153132) flutter/engine@ef820aa...0520889 2024-08-08 [email protected] [canvaskit] Detect animated WebP images (flutter/engine#54418) 2024-08-08 [email protected] Roll Dart SDK from 067c7cfcbc8c to ff0404c72fc5 (1 revision) (flutter/engine#54455) 2024-08-08 [email protected] [Impeller] move aiks text tests to DL. (flutter/engine#54293) 2024-08-08 [email protected] Roll Skia from 0c6dd1e6ff8e to 4cff580721cf (20 revisions) (flutter/engine#54454) 2024-08-08 [email protected] Add a precision to the fragment shader (flutter/engine#54109) 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],[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

mdebbar

Sorry for the late review, but this LGTM too!

mdebbar · 2024-08-09T15:13:26Z

lib/web_ui/lib/src/engine/image_format_detector.dart

+  bool _readWebpHeader() {
+    final String riffBytes = _readFourCC();
+
+    // Read file size byte.


Only one byte? 🙂

get out of here with your snark! 😜

…lutter#153132) flutter/engine@ef820aa...0520889 2024-08-08 [email protected] [canvaskit] Detect animated WebP images (flutter/engine#54418) 2024-08-08 [email protected] Roll Dart SDK from 067c7cfcbc8c to ff0404c72fc5 (1 revision) (flutter/engine#54455) 2024-08-08 [email protected] [Impeller] move aiks text tests to DL. (flutter/engine#54293) 2024-08-08 [email protected] Roll Skia from 0c6dd1e6ff8e to 4cff580721cf (20 revisions) (flutter/engine#54454) 2024-08-08 [email protected] Add a precision to the fragment shader (flutter/engine#54109) 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],[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 3 commits August 7, 2024 12:54

wip

eaed382

Merge branch 'main' into canvaskit-detect-animated-webp

d046fcb

Add detection for animated WebP images

a28d575

github-actions bot added the platform-web Code specifically for the web engine label Aug 7, 2024

Remove unused import

337f168

kevmoo reviewed Aug 8, 2024

View reviewed changes

kevmoo approved these changes Aug 8, 2024

View reviewed changes

Respond to comments

2919b67

kevmoo reviewed Aug 8, 2024

View reviewed changes

Respond to comments

25034ef

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

auto-submit bot merged commit 0520889 into flutter:main Aug 8, 2024
29 checks passed

engine-flutter-autoroll mentioned this pull request Aug 8, 2024

Roll Flutter Engine from ef820aa74f7a to 05208896830a (5 revisions) flutter/flutter#153132

Merged

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

052088968 [canvaskit] Detect animated WebP images (flutter/engine#54418)

9cbee40

mdebbar reviewed Aug 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[canvaskit] Detect animated WebP images #54418

[canvaskit] Detect animated WebP images #54418

harryterkelsen commented Aug 7, 2024 •

edited by mdebbar

Loading

kevmoo Aug 8, 2024

harryterkelsen Aug 8, 2024

kevmoo Aug 8, 2024

harryterkelsen Aug 8, 2024

kevmoo Aug 8, 2024

harryterkelsen Aug 8, 2024

kevmoo Aug 8, 2024

kevmoo Aug 8, 2024

harryterkelsen Aug 8, 2024

mdebbar left a comment

mdebbar Aug 9, 2024

kevmoo Aug 9, 2024

[canvaskit] Detect animated WebP images #54418

[canvaskit] Detect animated WebP images #54418

Conversation

harryterkelsen commented Aug 7, 2024 • edited by mdebbar Loading

Pre-launch Checklist

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

mdebbar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harryterkelsen commented Aug 7, 2024 •

edited by mdebbar

Loading