[Impeller] adds a plus advanced blend for f16 pixel formats

[gaaclarke]

fixes flutter/flutter#142549

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 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.

[jonahwilliams]

Plus should be an advanced blend only conditionally when we are in a colorspace that does not correctly clamp. There is a loss of performance and rendering fidelity when moving something to an advanced blend that should be avoided if possible.

[gaaclarke]

Plus should be an advanced blend only conditionally when we are in a colorspace that does not correctly clamp. There is a loss of performance and rendering fidelity when moving something to an advanced blend that should be avoided if possible.

I can resurrect the old Plus and make this AdvancedPlus and swap it over in the canvas. The reason I didn't do that is because every branch in logic incurs a maintenance cost. A bug that only shows up in particular setups is more difficult to find and fix. The performance of Plus didn't seem worth that cost.

[jonahwilliams]

If the maintainence cost of branching is too high then we shouldn't have added support for wide gamut imo. Given that we're always going to be an a world we don't know at runtime whether a not a particular device supports wide gamut this is just the cost we've chosen to bear. I don't think its a reasonable argument to say that doing it right isn't worth it.

[gaaclarke]

If the maintainence cost of branching is too high then we shouldn't have added support for wide gamut imo. Given that we're always going to be an a world we don't know at runtime whether a not a particular device supports wide gamut this is just the cost we've chosen to bear. I don't think its a reasonable argument to say that doing it right isn't worth it.

Fair point, in wide gamut's case though we are getting a lot of value for the effort. I'll add an agenda item to the next sync and we can decide as a team. There's no rush, this is hung up on the golden tests question anyway.

[gaaclarke]

If we decide to keep around the old blend, the EntityPass looks like the proper place to map kPlus to kPlusAdvanced. That's where we are setting up advanced blends and branching on framebuffer fetch or not:

--- a/impeller/entity/entity_pass.cc
+++ b/impeller/entity/entity_pass.cc
@@ -996,6 +996,14 @@ bool EntityPass::OnRender(
     /// Setup advanced blends.
     ///
 
+    BlendMode adjusted_blend_mode = result.entity.GetBlendMode();
+    if (adjusted_blend_mode == BlendMode::kPlus &&
+        !IsAlphaClampedToOne(renderer.GetContext()
+                                 ->GetCapabilities()
+                                 ->GetDefaultColorFormat())) {
+      adjusted_blend_mode = BlendMode::kPlusAdvanced;
+    }
+
     if (result.entity.GetBlendMode() > Entity::kLastPipelineBlendMode) {
       if (renderer.GetDeviceCapabilities().SupportsFramebufferFetch()) {
         auto src_contents = result.entity.GetContents();

[jonahwilliams]

That seems like a reasonable way to do it. I think instead of a separate capability state, you'd want a check of the onscreen/offscreen PixelFormat which tells you if you need the advanced blend.

[jonahwilliams]

I think it would be OK to put this in its own shader.

[gaaclarke]

I'm in the middle of still refactoring this. I'll ping you when it's ready to review =)

[gaaclarke]

I see benefits and disadvantages both ways. What we shouldn't have is the code duplicated in 2 different parts. I've unified them with a shared function since this comment was made.

[gaaclarke]

Let's see now if the golden bot malfunctions this time. I haven't force pushed since the last full ci cycle.

[flutter-dashboard]

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

Changes reported for pull request #51589 at sha 202f56d

[auto-submit]

A reason for requesting a revert of flutter/engine/51589 could
not be found or the reason was not properly formatted. Begin a comment with 'Reason for revert:' to tell the bot why
this issue is being reverted.

[jonahwilliams]

Reason for revert: draw vertices devicelab test is crashing due to SIGABRT in blend contents

[eyebrowsoffire]

Reverting due to: flutter/flutter#145899

[gaaclarke]

There is extra state that is getting setup when switching the blend mode that the override isn't catching, namely advanced_blend_proc_. Surprising that the test didn't catch that.

[gaaclarke]

The test failed to catch this because the test is going through the CreateForegroundAdvancedBlend path.