New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Impeller] Return image decoder error messages to the Dart API #42175

Merged

auto-submit merged 1 commit into flutter:main from jason-simmons:bug_127061

May 22, 2023

Member

jason-simmons commented May 19, 2023

Fixes flutter/flutter#127061

See flutter/flutter#126768

jason-simmons requested review from chinmaygarde and dnfield

May 19, 2023 21:51

flutter-dashboard bot commented May 19, 2023

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.

flutter-dashboard bot added the needs tests label

jason-simmons force-pushed the bug_127061 branch 3 times, most recently from bd5ec97 to 387f07d Compare

May 19, 2023 23:42

dnfield approved these changes

View reviewed changes

Contributor

dnfield left a comment

Some suggested improvements but LGTM with or without them

lib/ui/painting/image_decoder_impeller.cc Outdated

                 const auto pixel_format =
                     impeller::skia_conversions::ToPixelFormat(image_info.colorType());
                 if (!pixel_format.has_value()) {
-                  FML_DLOG(ERROR) << "Codec pixel format not supported by Impeller.";
-                  return std::nullopt;
+                  std::string decode_error("Codec pixel format not supported by Impeller.");

Contributor

dnfield May 20, 2023

Can we include anything here about what the color type was listed as according to Skia?

Member

chinmaygarde May 21, 2023

I'd get rid of the Impeller bit. There aren't pixel formats that are unique to either Skia or Impeller that are supported by Flutter.

lib/ui/painting/image_decoder_impeller.cc Outdated

                   ImageDescriptor* descriptor,
                   SkISize target_size,
                   impeller::ISize max_texture_size,
                   bool supports_wide_gamut,
                   const std::shared_ptr<impeller::Allocator>& allocator) {
                 TRACE_EVENT0("impeller", __FUNCTION__);
                 if (!descriptor) {
-                  FML_DLOG(ERROR) << "Invalid descriptor.";
-                  return std::nullopt;
+                  std::string decode_error("Invalid descriptor");

Contributor

dnfield May 20, 2023

This is a "this should never happen and is a bug in the engine code." I'm not sure if we need to say that here or not.

lib/ui/painting/image_decoder_impeller.cc Outdated

                   const std::shared_ptr<impeller::Context>& context,
                   const std::shared_ptr<impeller::DeviceBuffer>& buffer,
                   const SkImageInfo& image_info) {
                 TRACE_EVENT0("impeller", __FUNCTION__);
                 if (!context || !buffer) {
-                  return nullptr;
+                  return std::make_pair(nullptr, "No Impeller context or buffer available");

Contributor

dnfield May 20, 2023

It would be helpful to split these into separate messages - for example if there is no context there are differnet causes then if the buffer is invalid.

lib/ui/painting/image_decoder_impeller.cc Outdated

                 }
                 const auto pixel_format =
                     impeller::skia_conversions::ToPixelFormat(image_info.colorType());
                 if (!pixel_format) {
-                  FML_DLOG(ERROR) << "Pixel format unsupported by Impeller.";
-                  return nullptr;
+                  std::string decode_error("Pixel format unsupported by Impeller.");

Contributor

dnfield May 20, 2023

Same comment as above, if we can include what the unsupported color type is we should.

lib/ui/painting/image_decoder_impeller.cc Outdated

                   const std::shared_ptr<impeller::Context>& context,
                   std::shared_ptr<SkBitmap> bitmap,
                   bool create_mips) {
                 TRACE_EVENT0("impeller", __FUNCTION__);
                 if (!context || !bitmap) {
-                  return nullptr;
+                  return std::make_pair(nullptr, "No Impeller context or buffer available");

Contributor

dnfield May 20, 2023

Same comment as above about splitting context/buffer message.

dnfield approved these changes

View reviewed changes

Contributor

dnfield left a comment

Some suggested improvements but LGTM with or without them

chinmaygarde added the e: impeller label

chinmaygarde approved these changes

View reviewed changes

lib/ui/painting/image_decoder_impeller.cc Outdated

                 const auto pixel_format =
                     impeller::skia_conversions::ToPixelFormat(image_info.colorType());
                 if (!pixel_format.has_value()) {
-                  FML_DLOG(ERROR) << "Codec pixel format not supported by Impeller.";
-                  return std::nullopt;
+                  std::string decode_error("Codec pixel format not supported by Impeller.");

Member

chinmaygarde May 21, 2023

I'd get rid of the Impeller bit. There aren't pixel formats that are unique to either Skia or Impeller that are supported by Flutter.

chinmaygarde assigned jason-simmons


          [Impeller] Return image decoder error messages to the Dart API

Fixes flutter/flutter#127061

See flutter/flutter#126768

jason-simmons force-pushed the bug_127061 branch from 387f07d to 3850057 Compare

May 22, 2023 17:06

Member Author

jason-simmons commented May 22, 2023

Updated the PR with the requested changes

jason-simmons added the autosubmit label

auto-submit bot merged commit 1095102 into flutter:main

engine-flutter-autoroll mentioned this pull request

Roll Flutter Engine from a342a9186e69 to 10951022cc27 (6 revisions) flutter/flutter#127359

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request


          10951022c [Impeller] Return image decoder error messages to the Dart …

ace1f8e

…API (flutter/engine#42175)

engine-flutter-autoroll mentioned this pull request

Roll Flutter Engine from a342a9186e69 to 2a325eed77d0 (7 revisions) flutter/flutter#127364

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request


          10951022c [Impeller] Return image decoder error messages to the Dart …

62aa2d8

…API (flutter/engine#42175)

auto-submit bot pushed a commit to flutter/flutter that referenced this pull request


          Roll Flutter Engine from a342a9186e69 to 2a325eed77d0 (7 revisions) (#…

194d30a

…127364)

flutter/engine@a342a91...2a325ee

2023-05-22 skia-flutter-autoroll@skia.org Roll Dart SDK from b3e1eeda4918 to 1ca8f8368ecc (5 revisions) (flutter/engine#42224)
2023-05-22 jason-simmons@users.noreply.github.com [Impeller] Return image decoder error messages to the Dart API (flutter/engine#42175)
2023-05-22 5236035+fzyzcjy@users.noreply.github.com Again a two-word super tiny typo (flutter/engine#42181)
2023-05-22 ychris@google.com Reland "[ios_platform_view] only recycle maskView when the view is applying mutators #41573" (flutter/engine#42115)
2023-05-22 jason-simmons@users.noreply.github.com [Impeller] Use untransformed text bounds to calculate the size of ColorSourceTextContents (flutter/engine#42142)
2023-05-22 jonahwilliams@google.com [Impeller] Add UV compute shader. (flutter/engine#42192)
2023-05-22 jonahwilliams@google.com [Impeller] remove final cmd buffer waitUntilScheduled on physical iOS (flutter/engine#42160)

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 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

engine-flutter-autoroll mentioned this pull request

Manual roll Flutter Engine from a342a9186e69 to 2586cbeeae37 (14 revisions) flutter/flutter#127369

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request


          10951022c [Impeller] Return image decoder error messages to the Dart …

72f52b8

…API (flutter/engine#42175)

zanderso pushed a commit to flutter/flutter that referenced this pull request


          Manual roll Flutter Engine from a342a9186e69 to 2586cbeeae37 (14 revi…

dd07c61

…sions) (#127369)

Manual roll requested by zra@google.com

flutter/engine@a342a91...2586cbe

2023-05-23 zanderso@users.noreply.github.com Revert "[ios_platform_view]
only recycle maskView when the view is applying mutators #41573"
(flutter/engine#42231)
2023-05-23 skia-flutter-autoroll@skia.org Roll Skia from ac87929b3d2e to
6a57876d0e44 (2 revisions) (flutter/engine#42230)
2023-05-23 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from
QAwORJOkyNl4J3x4Y... to DzmjiSg6XC0JUfbKP... (flutter/engine#42227)
2023-05-23 skia-flutter-autoroll@skia.org Manual roll Dart SDK from
b3e1eeda4918 to 1ca8f8368ecc (5 revisions) (flutter/engine#42229)
2023-05-23 skia-flutter-autoroll@skia.org Roll Skia from d448fe07ea46 to
ac87929b3d2e (8 revisions) (flutter/engine#42226)
2023-05-23 dnfield@google.com Make FML_LOG safe from static
initialization (flutter/engine#42219)
2023-05-23 uysalere@gmail.com [fuchsia] Bind ChildViewWatcher on
platform thread (flutter/engine#42222)
2023-05-22 skia-flutter-autoroll@skia.org Roll Dart SDK from
b3e1eeda4918 to 1ca8f8368ecc (5 revisions) (flutter/engine#42224)
2023-05-22 jason-simmons@users.noreply.github.com [Impeller] Return
image decoder error messages to the Dart API (flutter/engine#42175)
2023-05-22 5236035+fzyzcjy@users.noreply.github.com Again a two-word
super tiny typo (flutter/engine#42181)
2023-05-22 ychris@google.com Reland "[ios_platform_view] only recycle
maskView when the view is applying mutators #41573"
(flutter/engine#42115)
2023-05-22 jason-simmons@users.noreply.github.com [Impeller] Use
untransformed text bounds to calculate the size of
ColorSourceTextContents (flutter/engine#42142)
2023-05-22 jonahwilliams@google.com [Impeller] Add UV compute shader.
(flutter/engine#42192)
2023-05-22 jonahwilliams@google.com [Impeller] remove final cmd buffer
waitUntilScheduled on physical iOS (flutter/engine#42160)

Also rolling transitive DEPS:
  fuchsia/sdk/core/mac-amd64 from QAwORJOkyNl4 to DzmjiSg6XC0J

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 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

CaseyHillers pushed a commit to CaseyHillers/flutter that referenced this pull request


          Roll Flutter Engine from a342a9186e69 to 2a325eed77d0 (7 revisions) (f…

802c4a1

…lutter#127364)

flutter/engine@a342a91...2a325ee

2023-05-22 skia-flutter-autoroll@skia.org Roll Dart SDK from b3e1eeda4918 to 1ca8f8368ecc (5 revisions) (flutter/engine#42224)
2023-05-22 jason-simmons@users.noreply.github.com [Impeller] Return image decoder error messages to the Dart API (flutter/engine#42175)
2023-05-22 5236035+fzyzcjy@users.noreply.github.com Again a two-word super tiny typo (flutter/engine#42181)
2023-05-22 ychris@google.com Reland "[ios_platform_view] only recycle maskView when the view is applying mutators flutter#41573" (flutter/engine#42115)
2023-05-22 jason-simmons@users.noreply.github.com [Impeller] Use untransformed text bounds to calculate the size of ColorSourceTextContents (flutter/engine#42142)
2023-05-22 jonahwilliams@google.com [Impeller] Add UV compute shader. (flutter/engine#42192)
2023-05-22 jonahwilliams@google.com [Impeller] remove final cmd buffer waitUntilScheduled on physical iOS (flutter/engine#42160)

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 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

CaseyHillers pushed a commit to CaseyHillers/flutter that referenced this pull request


          Manual roll Flutter Engine from a342a9186e69 to 2586cbeeae37 (14 revi…

22e1b6d

…sions) (flutter#127369)

Manual roll requested by zra@google.com

flutter/engine@a342a91...2586cbe

2023-05-23 zanderso@users.noreply.github.com Revert "[ios_platform_view]
only recycle maskView when the view is applying mutators flutter#41573"
(flutter/engine#42231)
2023-05-23 skia-flutter-autoroll@skia.org Roll Skia from ac87929b3d2e to
6a57876d0e44 (2 revisions) (flutter/engine#42230)
2023-05-23 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from
QAwORJOkyNl4J3x4Y... to DzmjiSg6XC0JUfbKP... (flutter/engine#42227)
2023-05-23 skia-flutter-autoroll@skia.org Manual roll Dart SDK from
b3e1eeda4918 to 1ca8f8368ecc (5 revisions) (flutter/engine#42229)
2023-05-23 skia-flutter-autoroll@skia.org Roll Skia from d448fe07ea46 to
ac87929b3d2e (8 revisions) (flutter/engine#42226)
2023-05-23 dnfield@google.com Make FML_LOG safe from static
initialization (flutter/engine#42219)
2023-05-23 uysalere@gmail.com [fuchsia] Bind ChildViewWatcher on
platform thread (flutter/engine#42222)
2023-05-22 skia-flutter-autoroll@skia.org Roll Dart SDK from
b3e1eeda4918 to 1ca8f8368ecc (5 revisions) (flutter/engine#42224)
2023-05-22 jason-simmons@users.noreply.github.com [Impeller] Return
image decoder error messages to the Dart API (flutter/engine#42175)
2023-05-22 5236035+fzyzcjy@users.noreply.github.com Again a two-word
super tiny typo (flutter/engine#42181)
2023-05-22 ychris@google.com Reland "[ios_platform_view] only recycle
maskView when the view is applying mutators flutter#41573"
(flutter/engine#42115)
2023-05-22 jason-simmons@users.noreply.github.com [Impeller] Use
untransformed text bounds to calculate the size of
ColorSourceTextContents (flutter/engine#42142)
2023-05-22 jonahwilliams@google.com [Impeller] Add UV compute shader.
(flutter/engine#42192)
2023-05-22 jonahwilliams@google.com [Impeller] remove final cmd buffer
waitUntilScheduled on physical iOS (flutter/engine#42160)

Also rolling transitive DEPS:
  fuchsia/sdk/core/mac-amd64 from QAwORJOkyNl4 to DzmjiSg6XC0J

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 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

dnfield mentioned this pull request

Cherry picks for image decoding in Impeller #42362

Merged

auto-submit bot pushed a commit that referenced this pull request


          Cherry picks for image decoding in Impeller (#42362)

c47096f

Issue: flutter/flutter#126878

Picks in #42349 and #42175

The reason for the second is that the first creates many more conflicts without it, and resolving them was a lot easier. The second PR is also very low risk and improves error messaging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit e: impeller needs tests