Replace deprecated and internal Skia EncodedImage calls with public versions #41368

kjlubick · 2023-04-20T15:07:57Z

Skia would like to remove SkImageGenerator::MakeFromEncoded and this appears to be the only remaining usage. It appears to be easily swapped out for the SkImages::DeferredFromEncodedData. (skbug.com/13052)

This also removes the use of the internal SkCodecImageGenerator for the public SkCodec API (which the image generator had just been deferring to anyway).

While unbreaking some unit tests, I made a few assertions easier to debug and produce nicer error messages.

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 Hixie said 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.

…keFromEncoded

flutter-dashboard · 2023-04-20T15:08:00Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

kjlubick · 2023-04-20T18:32:35Z

Looks like Skia will need to expose https://github.com/google/skia/blob/main/src/codec/SkPixmapUtils.h#L37 or something similar to make some of the tests like ImageDecoderTest.VerifySubpixelDecodingPreservesExifOrientation continue to pass, as that was something the SkCodecImageGenerator handled

…ge-generator

kjlubick · 2023-04-24T20:34:04Z

Hopefully I've found all the places where the SkCodecImageGenerator was transparently applying Exif metadata rotations and made them explicit on the Flutter-side.

dnfield

LGTM with a couple style nits.

impeller/image/backends/skia/compressed_image_skia.cc

lib/ui/painting/image_generator.cc

chinmaygarde · 2023-04-27T20:20:23Z

I believe this is good to go. Rebasing to kick off CI which tripped on a timeout.

auto-submit · 2023-04-28T15:18:33Z

auto label is removed for flutter/engine, pr: 41368, due to - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

dnfield · 2023-04-28T16:10:23Z

The google failures are not because of this changed, filed b/280066465 and manually marked status.

…public versions" (#41595) Reverts #41368 This is blocking rolls into the framework. Let's revert until we sort out whether there's a workaround for the change to the unpremultiplied alpha issue.

…h public versions (flutter/engine#41368)

…125714) flutter/engine@98b6fab...2a84ea5 2023-04-28 zanderso@users.noreply.github.com Revert "Replace deprecated and internal Skia EncodedImage calls with public versions" (flutter/engine#41595) 2023-04-28 maruel@gmail.com Roll buildroot to c96c9d4641714301fab450a5f3b0f3c42712e1e3 (flutter/engine#41589) 2023-04-28 ychris@google.com [platform_view] Only dispose view when it is removed from the composition order (flutter/engine#41521) 2023-04-28 gspencergoog@users.noreply.github.com Determine lifecycle by looking at window focus also (flutter/engine#41094) 2023-04-28 zanderso@users.noreply.github.com Increase timeout for clang-tidy on mac (flutter/engine#41588) 2023-04-28 bdero@google.com [Impeller] Always enable validation for goldens (flutter/engine#41574) 2023-04-28 maruel@gmail.com fuchsia: do not read version_history.json (flutter/engine#41585) 2023-04-28 skia-flutter-autoroll@skia.org Roll Dart SDK from c7160d4ea0b7 to 8e9bf2bb9878 (5 revisions) (flutter/engine#41586) 2023-04-28 kjlubick@users.noreply.github.com Replace deprecated and internal Skia EncodedImage calls with public versions (flutter/engine#41368) 2023-04-28 skia-flutter-autoroll@skia.org Roll Skia from 05d09f28250a to 9867fa253064 (18 revisions) (flutter/engine#41584) 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 jsimmons@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

This is a reland of #41368 with fix for transparent images and a unit test to verify it. Skia would like to remove `SkImageGenerator::MakeFromEncoded` and this appears to be the only remaining usage. It appears to be easily swapped out for the `SkImages::DeferredFromEncodedData`. (skbug.com/13052) This also removes the use of the internal `SkCodecImageGenerator` for the public `SkCodec` API (which the image generator had just been deferring to anyway). While unbreaking some unit tests, I made a few assertions easier to debug and produce nicer error messages. ## 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 Hixie said 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]. - [ ] 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 [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

Chromium change: chromium/chromium@1cd1998 commit 1cd1998bdcf00f30ea443f0c1a9b91a298f571d4 Author: Kevin Lubick <kjlubick@google.com> Date: Fri Apr 21 09:59:05 2023 -0400 Expose SkPixmapUtils SkPixmapUtils::Orient is a useful utility when dealing with exif data. See also flutter/engine#41368 Bug: skia:13983

This is a reland of flutter#41368 with fix for transparent images and a unit test to verify it. Skia would like to remove `SkImageGenerator::MakeFromEncoded` and this appears to be the only remaining usage. It appears to be easily swapped out for the `SkImages::DeferredFromEncodedData`. (skbug.com/13052) This also removes the use of the internal `SkCodecImageGenerator` for the public `SkCodec` API (which the image generator had just been deferring to anyway). While unbreaking some unit tests, I made a few assertions easier to debug and produce nicer error messages. ## 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 Hixie said 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]. - [ ] 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 [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

Chromium change: chromium/chromium@1cd1998 commit 1cd1998bdcf00f30ea443f0c1a9b91a298f571d4 Author: Kevin Lubick <kjlubick@google.com> Date: Fri Apr 21 09:59:05 2023 -0400 Expose SkPixmapUtils SkPixmapUtils::Orient is a useful utility when dealing with exif data. See also flutter/engine#41368 Bug: skia:13983

kjlubick added 2 commits April 20, 2023 11:04

Remove use of internal SkCodecImageGenerator and SkImageGenerator::Ma…

3797924

…keFromEncoded

format

314a2c7

flutter-dashboard bot added the needs tests label Apr 20, 2023

handle success better

8161047

kjlubick and others added 8 commits April 24, 2023 08:45

trying skpixmaputils

3f277da

Use SkPixmapUtils to orient image if necessary

361587c

Merge branch 'main' into image-generator

94d3987

debugging

598f3a3

more asserts

671860e

try swapping

b992608

tests pass

c6f1166

Merge branch 'image-generator' of github.com:kjlubick/engine into ima…

155d076

…ge-generator

clang tidy

1a0be70

zanderso requested review from chinmaygarde and dnfield April 27, 2023 17:00

dnfield approved these changes Apr 27, 2023

View reviewed changes

impeller/image/backends/skia/compressed_image_skia.cc Outdated Show resolved Hide resolved

lib/ui/painting/image_generator.cc Outdated Show resolved Hide resolved

kjlubick and others added 3 commits April 27, 2023 13:33

address feedback

7696cd6

one more img

db7c21a

Merge branch 'main' into image-generator

c3ed41f

chinmaygarde approved these changes Apr 27, 2023

View reviewed changes

chinmaygarde added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 27, 2023

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Apr 28, 2023

dnfield merged commit fe4a6f2 into flutter:main Apr 28, 2023

engine-flutter-autoroll mentioned this pull request Apr 28, 2023

Roll Flutter Engine from 98b6fabc66bb to fe4a6f2060df (2 revisions) flutter/flutter#125694

Closed

zanderso mentioned this pull request Apr 28, 2023

Revert "Replace deprecated and internal Skia EncodedImage calls with public versions" #41595

Merged

engine-flutter-autoroll mentioned this pull request Apr 28, 2023

Roll Flutter Engine from 98b6fabc66bb to 2a84ea55e4ef (10 revisions) flutter/flutter#125714

Merged

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

fe4a6f206 Replace deprecated and internal Skia EncodedImage calls wit…

1d0be63

…h public versions (flutter/engine#41368)

kjlubick mentioned this pull request May 1, 2023

Reland image encoder #41632

Merged

8 tasks

kjlubick deleted the image-generator branch August 3, 2023 18:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Replace deprecated and internal Skia EncodedImage calls with public versions #41368

Replace deprecated and internal Skia EncodedImage calls with public versions #41368

Uh oh!

kjlubick commented Apr 20, 2023 •

edited

Loading

Uh oh!

flutter-dashboard bot commented Apr 20, 2023

Uh oh!

kjlubick commented Apr 20, 2023

Uh oh!

kjlubick commented Apr 24, 2023

Uh oh!

dnfield left a comment

Uh oh!

Uh oh!

Uh oh!

chinmaygarde commented Apr 27, 2023

Uh oh!

auto-submit bot commented Apr 28, 2023

Uh oh!

dnfield commented Apr 28, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Replace deprecated and internal Skia EncodedImage calls with public versions #41368

Replace deprecated and internal Skia EncodedImage calls with public versions #41368

Uh oh!

Conversation

kjlubick commented Apr 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Apr 20, 2023

Uh oh!

kjlubick commented Apr 20, 2023

Uh oh!

kjlubick commented Apr 24, 2023

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

chinmaygarde commented Apr 27, 2023

Uh oh!

auto-submit bot commented Apr 28, 2023

Uh oh!

dnfield commented Apr 28, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kjlubick commented Apr 20, 2023 •

edited

Loading