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.

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

Made the warning about downgrading wide gamut happen at the correct time #46064

Merged
merged 1 commit into from
Sep 19, 2023
Merged

Made the warning about downgrading wide gamut happen at the correct time #46064

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 16 additions & 5 deletions shell/platform/darwin/ios/framework/Source/FlutterView.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@ - (BOOL)isWideGamutSupported {
return NO;
}

FML_DCHECK(self.screen);

// 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.
Expand All @@ -68,10 +70,6 @@ - (instancetype)initWithDelegate:(id<FlutterViewEngineDelegate>)delegate
if (self) {
_delegate = delegate;
_isWideGamutEnabled = isWideGamutEnabled;
if (_isWideGamutEnabled && !self.isWideGamutSupported) {
FML_DLOG(WARNING) << "Rendering wide gamut colors is turned on but isn't "
"supported, downgrading the color gamut to sRGB.";
}
self.layer.opaque = opaque;

// This line is necessary. CoreAnimation(or UIKit) may take this to do
Expand All @@ -84,6 +82,16 @@ - (instancetype)initWithDelegate:(id<FlutterViewEngineDelegate>)delegate
return self;
}

static void PrintWideGamutWarningOnce() {
static BOOL did_print = NO;
Copy link
Contributor

@cyanglaz cyanglaz Sep 19, 2023 •
edited

Choose a reason for hiding this comment

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

nits:
maybe use dispatch_once_t?

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 had that at first but then decided the table stakes are so low that threading doesn't need to happen. Worst case scenario is that the print statement happens twice.

Copy link
Contributor

Choose a reason for hiding this comment

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

make sense. LGTM!

if (did_print) {
return;
}
FML_DLOG(WARNING) << "Rendering wide gamut colors is turned on but isn't "
"supported, downgrading the color gamut to sRGB.";
did_print = YES;
}

- (void)layoutSubviews {
if ([self.layer isKindOfClass:NSClassFromString(@"CAMetalLayer")]) {
// It is a known Apple bug that CAMetalLayer incorrectly reports its supported
Expand All @@ -97,7 +105,8 @@ - (void)layoutSubviews {
layer.contentsScale = screenScale;
layer.rasterizationScale = screenScale;
layer.framebufferOnly = flutter::Settings::kSurfaceDataAccessible ? NO : YES;
Copy link
Member

Choose a reason for hiding this comment

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

drive by comment: shouldn't this be NO always? We set it to NO here:

engine/shell/platform/darwin/ios/ios_surface_metal_impeller.mm

Line 57 in fdb33e4

layer.framebufferOnly = NO;

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 don't think so. YES is the preferable setting since it has better performance. I think I may have added this switch that allows you to turn it to NO so we can do wide gamut tests looking extended range pixels. Its possible that NO is needed in that layer, but not on this one. I don't know off the top of my head the relationship between these 2 layers.

Copy link
Member

Choose a reason for hiding this comment

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

This needs to be NO because of backdropfilters and platform views.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, for that layer. But is that layer just placed on top of this one? Or is it the same layer and that is just overriding the setting?

Copy link
Member

Choose a reason for hiding this comment

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

They're the same layer, so you're setting to YES and then its getting blown away. So just set it to NO here.

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 verified they are the same layer. This seems messy if we have setup in 2 different places for the layer. I guess this setting might be meaningful for the skia backend though? That could explain why the setup is in 2 places.

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'm not going to touch it since it could potentially mess up the skia backend and I think touching it would mean cleaning it up even further to unify the setup.

if (_isWideGamutEnabled && self.isWideGamutSupported) {
BOOL isWideGamutSupported = self.isWideGamutSupported;
if (_isWideGamutEnabled && isWideGamutSupported) {
CGColorSpaceRef srgb = CGColorSpaceCreateWithName(kCGColorSpaceExtendedSRGB);
layer.colorspace = srgb;
CFRelease(srgb);
Expand All @@ -106,6 +115,8 @@ - (void)layoutSubviews {
// F16 was chosen over BGRA10_XR since Skia does not support decoding
// BGRA10_XR.
layer.pixelFormat = MTLPixelFormatRGBA16Float;
} else if (_isWideGamutEnabled && !isWideGamutSupported) {
PrintWideGamutWarningOnce();
}
}

Expand Down