[Impeller] Convert to BGRAXR_10 for iOS wide gamut surface/offscreen buffers

gaaclarke commented 1 month ago

Today we use f16 as the pixel format for the surface and offscreen buffers if wide gamut is enabled. We found a bug in the Plus blend where you could get alpha values greater than 1. That was fixed by moving the plus blend to a fragment shader in https://github.com/flutter/engine/pull/51589.

Using a fragment shader for Plus blending is not ideal because: 1) It's slower 1) It requires more resources (vram for sure, sampler?) 1) Its code is more complicated

We may be able to eliminate the need for the advanced shader if we instead switch to using apples BGRA10_XR pixel format. That format does clamp the alpha channel to 1.0, so it would have eliminated the bug that we saw that predicated the advanced blend.

Here are some concerns that need to be sorted out to implement this: 1) Skia would need to be patched to implement bgra10_xr for layer.toImage to work. We have patched Skia already to support BGR10_XR, so presumably they would be open to this and it wouldn't be that much more work. 1) BGR10_XR clamps the color channels to 1.25098, that's outside the range of the displays we are rendering to. Could that cause odd outputs when stacking blends? Enough to care?

I was concerned about maybe a clash with our texture formats, but I verified we already use BGR10_XR for opaque images, f16 for transparent images, so there shouldn't be any dependency on the surface pixel formats for those to work.

Using BGRA10_XR also has the added benefit of hiding any errors in shaders that are being covered up with the implicit clamps of sRGB. This has happened in the past.

This is complicated but I'm 80% sure this would work. It will require some attention and testing to make sure. It should be 30% (1 - 5 / 3.8) faster.

Jonah already started drafting a PR in https://github.com/flutter/engine/pull/51748

cc @jonahwilliams

jonahwilliams commented 1 month ago

BGR10_XR clamps the color channels to 1.25098, that's outside the range of the displays we are rendering to. Could that cause odd outputs when stacking blends? Enough to care?

Does it? I thought the point of wide gamut was that it was only enabled if the screen supported it

gaaclarke commented 1 month ago

BGR10_XR clamps the color channels to 1.25098, that's outside the range of the displays we are rendering to. Could that cause odd outputs when stacking blends? Enough to care?

Does it? I thought the point of wide gamut was that it was only enabled if the screen supported it

BGRA_XR supports even wider gamuts than displayp3 which is the colorspace apple hardware uses. DisplayP3 will never get close to 1.25 limit for any color channel.

Wide gamut really isn't a boolean thing, so while we talk about it that way things like BGRA10_XR aren't built that way.

I don't think there is a way to get 100% accurate wide gamut plus blend without a fragment shader. BGRA10_XR with extended sRGB will make you have to clamp at the limits for your devices color gamut. BGRA10_XR (or f16) with displayp3 will require you to clamp at 1.0 for all the color components.

jonahwilliams commented 1 month ago

If we need a fragment shader to do all blends, then we should not have any wide gamut support - period. Its a completely unreasonable performance burdent to re-implement fixed function hardware in software.

If we're going to make an attempt to support it, then we should use a color format that behaves kind of correctly (With clamped alpha rather than completely incorrectly with compensations like making everything an advanced blends.

Next week I will start a patch to skia to add support for the BGRA extended range format. If switching to the clamped format is infeasible for whatever reason then we'll need to evaluate if we can afford to keep this feature.

gaaclarke commented 1 month ago

Looks like Jonah found some more places where the alpha overflow could be an issue ( https://github.com/flutter/engine/pull/51792 )

Here are the PR's for BGR10XR: