Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Made sure not to turn on wide gamut support without impeller. #41460

Merged
merged 3 commits into from
Apr 25, 2023

Conversation

gaaclarke
Copy link
Member

@gaaclarke gaaclarke commented Apr 24, 2023

fixes flutter/flutter#125430

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.

@gaaclarke gaaclarke marked this pull request as ready for review April 24, 2023 21:25
@@ -1402,6 +1402,10 @@ - (FlutterDartProject*)project {
return _dartProject.get();
}

- (BOOL)isUsingImpeller {
return self.project.isImpellerEnabled;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is available via the shell, but there's nothing inherently wrong with exposing it on the project either.

// This predicates the decision on the capabilities of the iOS device's
// display. This means external displays will not support wide gamut if the
// device's display doesn't support it. It practice that should be never.
return UIScreen.mainScreen.traitCollection.displayGamut != UIDisplayGamutSRGB;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: This should be using availability checks and then checking the UIScene API on newer iOS and falling back to this deprecated property. That would also solve the problem of not knowing whether a connected display is wide gamut or not.

Copy link
Member Author

Choose a reason for hiding this comment

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

Filed issue: flutter/flutter#125496

Copy link
Member

@jmagman jmagman Apr 25, 2023

Choose a reason for hiding this comment

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

Can you instead use self.window.windowScene.screen.traitCollection.displayGamut? See mainScreenIfViewLoaded in the FlutterViewController for a fallback example.

- (UIScreen*)mainScreenIfViewLoaded {
if (@available(iOS 13.0, *)) {
if (self.viewIfLoaded == nil) {
FML_LOG(WARNING) << "Trying to access the view before it is loaded.";
}
return self.viewIfLoaded.window.windowScene.screen;
}
return UIScreen.mainScreen;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

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

LGTM with nits. We should file a bug to follow up on using UIScene here

@gaaclarke gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 25, 2023
@auto-submit auto-submit bot merged commit b9aa2b7 into flutter:main Apr 25, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 25, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 26, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Apr 26, 2023
…125529)

flutter/engine@34ece7a...b3cbf06

2023-04-25 [email protected] Migrate windows host engine to engine v2. (flutter/engine#41487)
2023-04-25 [email protected] Roll Skia from f6f0c4b5ee98 to 809bf518d4ab (3 revisions) (flutter/engine#41494)
2023-04-25 [email protected] Made sure not to turn on wide gamut support without impeller. (flutter/engine#41460)
2023-04-25 [email protected] Roll Fuchsia Mac SDK from 5bc9_eyVcLoMrWvdO... to JKHSUjf-qEr0ZMdEi... (flutter/engine#41493)
2023-04-25 [email protected] Roll Skia from 1bed4228ea3b to f6f0c4b5ee98 (7 revisions) (flutter/engine#41492)
2023-04-25 [email protected] [Impeller] Fixed blit command missing tracking and added mock vulkan for tests (flutter/engine#41408)

Also rolling transitive DEPS:
  fuchsia/sdk/core/mac-amd64 from 5bc9_eyVcLoM to JKHSUjf-qEr0

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://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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
autosubmit Merge PR when tree becomes green via auto submit App platform-ios
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Skia crash due to an unexpected Metal pixel format while wrapping an onscreen surface.
3 participants