[Impeller] Adds test to verify wide gamut indexed png decompression fix for Skia. #45399

gaaclarke · 2023-09-01T21:18:09Z

fixes flutter/flutter#133013
depends on skia fix: https://skia-review.googlesource.com/c/skia/+/751696

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.

gaaclarke · 2023-09-08T23:46:05Z

lib/ui/painting/image_decoder_no_gl_unittests.cc

+namespace flutter {
+namespace testing {
+
+// Tests are disabled for fuchsia.


One of these tests just got moved over here, so it wasn't running for fuchsia anyway.

gaaclarke · 2023-09-08T23:53:57Z

lib/ui/painting/image_decoder_no_gl_unittests.cc

+  return (x * slope) + intercept;
+}
+
+TEST(ImageDecoderNoGLTest, ImpellerWideGamutDisplayP3) {


This test was moved to its own file because it wasn't running on macos because other tests in image_decoder_unittests.cc need opengl, not this one.

…kia patch lands

jason-simmons · 2023-09-12T21:36:22Z

lib/ui/painting/image_decoder_no_gl_unittests.cc

+    return false;
+  }
+
+  if (memcmp(bytes, "\x89PNG\x0d\x0a\x1a\x0a", 8) != 0) {


use constexpr std::string_view to represent the PNG format string constants and their lengths

jason-simmons · 2023-09-12T21:46:54Z

lib/ui/painting/image_decoder_no_gl_unittests.cc

+  }
+
+  const uint8_t* end = bytes + size;
+  const uint8_t* loc = bytes + 8;


Define a constant for the length of the PNG chunk header.

Or a packed struct for the header (like https://github.com/flutter/engine/blob/main/lib/ui/painting/image_generator_apng.h#L69)

jason-simmons · 2023-09-12T21:47:15Z

lib/ui/painting/image_decoder_no_gl_unittests.cc

+  const uint8_t* end = bytes + size;
+  const uint8_t* loc = bytes + 8;
+  while (loc + 8 <= end) {
+    uint32_t chunk_length =


Use fml::BigEndianToArch

jason-simmons · 2023-09-12T21:54:13Z

lib/ui/painting/image_decoder_no_gl_unittests.cc

+  return false;
+}
+
+sk_sp<SkData> OpenFixtureAsSkData(const char* name) {


There are several copies of OpenFixtureAsSkData in various tests.

Can they be consolidated into someplace like https://github.com/flutter/engine/blob/main/testing/testing.cc?

(I'd be fine with doing this in a separate PR if it's nontrivial)

I didn't mean to add one more, removed the duplication I introduced.

flutter-dashboard · 2023-09-12T23:24:14Z

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 #45399 at sha 54a2586

jason-simmons · 2023-09-12T23:37:46Z

lib/ui/painting/image_decoder_no_gl_unittests.cc

+
+  const uint8_t* end = bytes + size;
+  const uint8_t* loc = bytes + kPngMagic.size();
+  while (loc + 8 <= end) {


Replace 8 with kLengthBytes + kTypeBytes

…pression fix for Skia. (flutter/engine#45399)

…134676) flutter/engine@5e671d5...b71b366 2023-09-13 matanlurey@users.noreply.github.com Do not run real processes in `clang_tidy_test.dart` (flutter/engine#45748) 2023-09-13 30870216+gaaclarke@users.noreply.github.com [Impeller] Adds test to verify wide gamut indexed png decompression fix for Skia. (flutter/engine#45399) 2023-09-13 skia-flutter-autoroll@skia.org Roll Skia from 284c333d7eb2 to 78d18d509475 (2 revisions) (flutter/engine#45769) 2023-09-13 skia-flutter-autoroll@skia.org Roll Skia from 3ff43577d04b to 284c333d7eb2 (1 revision) (flutter/engine#45768) 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 bdero@google.com,rmistry@google.com,zra@google.com 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…ix for Skia. (flutter#45399) fixes flutter/flutter#133013 depends on skia fix: https://skia-review.googlesource.com/c/skia/+/751696 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] 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. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I signed the [CLA]. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord].  [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [test-exempt]: https://github.com/flutter/flutter/wiki/Tree-hygiene#tests [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style [testing the engine]: https://github.com/flutter/flutter/wiki/Testing-the-engine [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat

…lutter#134676) flutter/engine@5e671d5...b71b366 2023-09-13 matanlurey@users.noreply.github.com Do not run real processes in `clang_tidy_test.dart` (flutter/engine#45748) 2023-09-13 30870216+gaaclarke@users.noreply.github.com [Impeller] Adds test to verify wide gamut indexed png decompression fix for Skia. (flutter/engine#45399) 2023-09-13 skia-flutter-autoroll@skia.org Roll Skia from 284c333d7eb2 to 78d18d509475 (2 revisions) (flutter/engine#45769) 2023-09-13 skia-flutter-autoroll@skia.org Roll Skia from 3ff43577d04b to 284c333d7eb2 (1 revision) (flutter/engine#45768) 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 bdero@google.com,rmistry@google.com,zra@google.com 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…on fix for Skia. (#45399) (#46101) cherry pick of #45399

gaaclarke mentioned this pull request Sep 1, 2023

[Impeller] App crashes on wide-gamut indexed PNG decompression in Skia with wide-gamut enabled. flutter/flutter#133013

Closed

2 tasks

gaaclarke changed the title ~~[Impeller] started blocking wide gamut indexed pngs~~ [Impeller] Adds test to verify wide gamut indexed png decompression fix for skia Sep 5, 2023

gaaclarke force-pushed the no-wide-gamut-palette-pngs branch from d3635b0 to b2c89dc Compare September 5, 2023 21:21

chinmaygarde added the e: impeller label Sep 6, 2023

chinmaygarde assigned gaaclarke Sep 6, 2023

chinmaygarde changed the title ~~[Impeller] Adds test to verify wide gamut indexed png decompression fix for skia~~ [Impeller] Adds test to verify wide gamut indexed png decompression fix for Skia. Sep 6, 2023

github-actions bot removed the e: impeller label Sep 6, 2023

chinmaygarde added the e: impeller label Sep 8, 2023

gaaclarke force-pushed the no-wide-gamut-palette-pngs branch from e9cc968 to f3b5330 Compare September 8, 2023 21:04

github-actions bot removed the e: impeller label Sep 8, 2023

gaaclarke force-pushed the no-wide-gamut-palette-pngs branch 2 times, most recently from 65e4f13 to 89900ac Compare September 8, 2023 23:16

gaaclarke marked this pull request as ready for review September 8, 2023 23:45

gaaclarke requested review from jason-simmons and jonahwilliams September 8, 2023 23:45

gaaclarke commented Sep 8, 2023

View reviewed changes

chinmaygarde added the e: impeller label Sep 12, 2023

gaaclarke added 6 commits September 12, 2023 14:32

[Impeller] started blocking wide gamut indexed pngs

f78b4f9

switched from blocking plte images to asserting they work after the s…

da84500

…kia patch lands

removed dup code

1af4390

Swapped test back to old image

544d272

updated license file

c381f49

disabled the tests for fuchsia

7d8fbae

gaaclarke force-pushed the no-wide-gamut-palette-pngs branch from 89900ac to 7d8fbae Compare September 12, 2023 21:32

github-actions bot removed the e: impeller label Sep 12, 2023

jason-simmons reviewed Sep 12, 2023

View reviewed changes

gaaclarke added 2 commits September 12, 2023 15:47

jason feedback

ed89c50

jason feedback2

54a2586

gaaclarke requested a review from jason-simmons September 12, 2023 22:54

flutter-dashboard bot added the will affect goldens label Sep 12, 2023

jason-simmons approved these changes Sep 12, 2023

View reviewed changes

jason feedback 3

6feb452

gaaclarke added autosubmit Merge PR when tree becomes green via auto submit App and removed will affect goldens labels Sep 13, 2023

gaaclarke merged commit 3b6d0ea into flutter:main Sep 13, 2023

engine-flutter-autoroll mentioned this pull request Sep 13, 2023

Roll Flutter Engine from 5e671d5c90f9 to 3b6d0ea43824 (3 revisions) flutter/flutter#134673

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 13, 2023

3b6d0ea43 [Impeller] Adds test to verify wide gamut indexed png decom…

66f1f2e

…pression fix for Skia. (flutter/engine#45399)

engine-flutter-autoroll mentioned this pull request Sep 13, 2023

Roll Flutter Engine from 5e671d5c90f9 to b71b366e3de3 (4 revisions) flutter/flutter#134676

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 13, 2023

3b6d0ea43 [Impeller] Adds test to verify wide gamut indexed png decom…

470c4b9

…pression fix for Skia. (flutter/engine#45399)

gaaclarke mentioned this pull request Sep 20, 2023

[CP][Impeller] Adds test to verify wide gamut indexed png decompression fix for Skia. (#45399) #46101

Merged

chinmaygarde added the e: impeller label Sep 21, 2023

auto-submit bot pushed a commit that referenced this pull request Oct 12, 2023

[CP][Impeller] Adds test to verify wide gamut indexed png decompressi…

050b907

…on fix for Skia. (#45399) (#46101) cherry pick of #45399

[Impeller] Adds test to verify wide gamut indexed png decompression fix for Skia. #45399

[Impeller] Adds test to verify wide gamut indexed png decompression fix for Skia. #45399

Uh oh!

Conversation

gaaclarke commented Sep 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

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!

flutter-dashboard bot commented Sep 12, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gaaclarke commented Sep 1, 2023 •

edited

Loading