Skip to content
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.

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

[Impeller] Made toByteData work on wide gamut images (always downscaling to sRGB). #39932

Merged
merged 10 commits into from Mar 2, 2023

Conversation

gaaclarke
Copy link
Member

@gaaclarke gaaclarke commented Feb 28, 2023

fixes flutter/flutter#121563

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.

@chinmaygarde chinmaygarde changed the title Made toByteData work on wide gamut images (always downscaling to sRGB). [Impeller] Made toByteData work on wide gamut images (always downscaling to sRGB). Feb 28, 2023
@gaaclarke gaaclarke marked this pull request as ready for review March 1, 2023 01:51

} // namespace

void ImageEncodingImpeller::ConvertDlImageToSkImage(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made this public for easier testing. With a little bit of work I could make the tests work on the existing public api. There is just a bit of threading work required to do that.

@gaaclarke gaaclarke requested a review from dnfield March 1, 2023 01:54
@@ -1481,6 +1481,7 @@ ORIGIN: ../../../flutter/impeller/renderer/gpu_tracer.cc + ../../../flutter/LICE
ORIGIN: ../../../flutter/impeller/renderer/gpu_tracer.h + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/impeller/renderer/host_buffer.cc + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/impeller/renderer/host_buffer.h + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/impeller/renderer/mocks.h + ../../../flutter/LICENSE
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: maybe this belongs in the excluded one instead? I think we're doing that with at least some test-only files.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have a pointer to an example? I can ask Hixie.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved it over, I think this makes sense

@gaaclarke gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 1, 2023
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Mar 1, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented Mar 1, 2023

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

@gaaclarke gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 2, 2023
@auto-submit auto-submit bot merged commit 0562901 into flutter:main Mar 2, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 2, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 2, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 2, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 2, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 2, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 2, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 2, 2023
chinmaygarde pushed a commit to chinmaygarde/flutter_engine that referenced this pull request Mar 3, 2023
…ing to sRGB). (flutter#39932)

[Impeller] Made toByteData work on wide gamut images (always downscaling to sRGB).
@armandojimenez
Copy link

I am getting this error: Failed to get color type from pixel format when using the palette_generator package with wide color gamut enabled in Flutter 3.10, is this the issue?

@gaaclarke
Copy link
Member Author

I am getting this error: Failed to get color type from pixel format when using the palette_generator package with wide color gamut enabled in Flutter 3.10, is this the issue?

Could be. There was no intentional breaking change. Can you file an issue please with reproduction code? Bonus points if the reproduction code uses the ui package directly instead of relying on a plugin.

@armandojimenez
Copy link

Also just tried using ColorSheme.fromImageProvider(), announced in the What's new in Flutter, does not work with wide color gamut enabled, so a simple code like this:

final colorSheme = await ColorScheme.fromImageProvider(
              provider: Image.file(File(savedPath)).image);
)

Should give the error, I don't have the time right now to create the issue, but should be easy to reproduce just using ColorSheme.fromImageProvider()

@armandojimenez
Copy link

armandojimenez commented May 11, 2023

NVM, looks like the problem is with the Screenshot package with Wide gamut enabled

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App e: impeller
Projects
No open projects
Archived in project
4 participants