[Impeller] Use MTLPixelFormatBGRA10_XR for wide gamut. #51748
Conversation
|
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 "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use 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. |
jonahwilliams
changed the title
| layer.pixelFormat = MTLPixelFormatRGBA16Float; | ||
| // MTLPixelFormatBGR10_XR was chosen since it is compatible with | ||
| // impeller's offscreen buffers which need to have transparency. | ||
| layer.pixelFormat = MTLPixelFormatBGR10_XR; |
There was a problem hiding this comment.
Notes from chat: We need an alpha channel, so bgra10_xr.
| } else if (pixelFormat == MTLPixelFormatBGR10_XR) { | ||
| self->_colorSpaceRef = fml::CFRef(CGColorSpaceCreateWithName(kCGColorSpaceExtendedSRGB)); |
There was a problem hiding this comment.
Notes from chat: I'm not sure how Metal interprets this combination, extended srgb with BGRA_XR. Specifically what does a red channel with the 0x3ff value interpret as? If it is 1.0, we can't represent wide gamut colors. If it's 1.0931, it will work since it's the max for displayp3, but does that mean all channels are clamped to that value? That would be a problem, or are they all interpreted different for each channel?
What we'd need for this to work is for 0x3ff to be interpreted as the maximum value for displayp3 for each channel (it's different for each one [1.09, 1.01, 1.04, 1.0]). We aren't communicating displayp3 anywhere so that's unlikely the case.
If I had to guess they have specified some arbitrary cut off of 0x3ff to represent that can handle most (all?) possible colorspaces, same thing for interpreting zero.
There was a problem hiding this comment.
Something like, [0x200, 0x20, 0x20, 0x3ff] = srgb(1, 0, 0, 1)? Do they treat alpha different, where 0x3ff is one?
We also have to consider that BGRA10XR isn't available on android. We'll need f16 support at some point anyway.
There was a problem hiding this comment.
We're not going to do wide gamut on Android this year at least, it shouldn't be a consideration.
|
According to the docs:
|
|
The missing capability in this PR is that we use Skias image codecs on iOS, which can't encode to/from MTLPixelFormatBGRA10_XR, so as is this breaks layer.toImage. The easiest short term solution is to blit the wide gamut texture to a skia supported format, and then clean that up when we remove the skia image codecs for slimpeller. |
Oh, regardless of the colorspace? Interesting. That sounds like it would solve our alpha channel problems, which is our biggest issue. In the advanced blend plus implementation I ended up clamping all channels to 1 out of caution. That means we don't get displayp3(1,0,0) drawn when |
Skia is open to PR's that expand this. I already added BGR10_XR support. BGRA10_XR shouldn't be that much different. |
Fixes flutter/flutter#142549
MTLPixelFormatBGRA10_XR is a clamped alpha format. While Skia can't decode images to this format, that doesn't matter since we're only ever sampling images and never attaching them as render targets.