Revert "[Impeller] adds a plus advanced blend for f16 pixel formats (…

…#51589)" This reverts commit 467b801.
flutter · Mar 28, 2024 · 452b24c · 452b24c
1 parent cd4b8cc
commit 452b24c
Show file tree

Hide file tree

Showing 30 changed files with 179 additions and 364 deletions.
diff --git a/display_list/testing/dl_test_surface_metal.cc b/display_list/testing/dl_test_surface_metal.cc
@@ -115,7 +115,7 @@ sk_sp<DlImage> DlMetalSurfaceProvider::MakeImpellerImage(
 
 void DlMetalSurfaceProvider::InitScreenShotter() const {
   if (!snapshotter_) {
-    snapshotter_.reset(new MetalScreenshotter(/*enable_wide_gamut=*/false));
+    snapshotter_.reset(new MetalScreenshotter());
     auto typographer = impeller::TypographerContextSkia::Make();
     aiks_context_.reset(new impeller::AiksContext(
         snapshotter_->GetPlayground().GetContext(), typographer));

diff --git a/impeller/aiks/aiks_unittests.cc b/impeller/aiks/aiks_unittests.cc
@@ -1102,76 +1102,6 @@ TEST_P(AiksTest, PaintBlendModeIsRespected) {
   ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture()));
 }
 
-// This makes sure the WideGamut named tests use 16bit float pixel format.
-TEST_P(AiksTest, F16WideGamut) {
-  if (GetParam() != PlaygroundBackend::kMetal) {
-    GTEST_SKIP_("This backend doesn't yet support wide gamut.");
-  }
-  EXPECT_EQ(GetContext()->GetCapabilities()->GetDefaultColorFormat(),
-            PixelFormat::kR16G16B16A16Float);
-  EXPECT_FALSE(IsAlphaClampedToOne(
-      GetContext()->GetCapabilities()->GetDefaultColorFormat()));
-}
-
-TEST_P(AiksTest, NotF16) {
-  EXPECT_TRUE(IsAlphaClampedToOne(
-      GetContext()->GetCapabilities()->GetDefaultColorFormat()));
-}
-
-// Bug: https://github.com/flutter/flutter/issues/142549
-TEST_P(AiksTest, BlendModePlusAlphaWideGamut) {
-  if (GetParam() != PlaygroundBackend::kMetal) {
-    GTEST_SKIP_("This backend doesn't yet support wide gamut.");
-  }
-  EXPECT_EQ(GetContext()->GetCapabilities()->GetDefaultColorFormat(),
-            PixelFormat::kR16G16B16A16Float);
-  auto texture = CreateTextureForFixture("airplane.jpg",
-                                         /*enable_mipmapping=*/true);
-
-  Canvas canvas;
-  canvas.Scale(GetContentScale());
-  canvas.DrawPaint({.color = Color(0.9, 1.0, 0.9, 1.0)});
-  canvas.SaveLayer({});
-  Paint paint;
-  paint.blend_mode = BlendMode::kPlus;
-  paint.color = Color::Red();
-  canvas.DrawRect(Rect::MakeXYWH(100, 100, 400, 400), paint);
-  paint.color = Color::White();
-  canvas.DrawImageRect(
-      std::make_shared<Image>(texture), Rect::MakeSize(texture->GetSize()),
-      Rect::MakeXYWH(100, 100, 400, 400).Expand(-100, -100), paint);
-  canvas.Restore();
-  ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture()));
-}
-
-// Bug: https://github.com/flutter/flutter/issues/142549
-TEST_P(AiksTest, BlendModePlusAlphaColorFilterWideGamut) {
-  if (GetParam() != PlaygroundBackend::kMetal) {
-    GTEST_SKIP_("This backend doesn't yet support wide gamut.");
-  }
-  EXPECT_EQ(GetContext()->GetCapabilities()->GetDefaultColorFormat(),
-            PixelFormat::kR16G16B16A16Float);
-  auto texture = CreateTextureForFixture("airplane.jpg",
-                                         /*enable_mipmapping=*/true);
-
-  Canvas canvas;
-  canvas.Scale(GetContentScale());
-  canvas.DrawPaint({.color = Color(0.1, 0.2, 0.1, 1.0)});
-  canvas.SaveLayer({
-      .color_filter =
-          ColorFilter::MakeBlend(BlendMode::kPlus, Color(Vector4{1, 0, 0, 1})),
-  });
-  Paint paint;
-  paint.color = Color::Red();
-  canvas.DrawRect(Rect::MakeXYWH(100, 100, 400, 400), paint);
-  paint.color = Color::White();
-  canvas.DrawImageRect(
-      std::make_shared<Image>(texture), Rect::MakeSize(texture->GetSize()),
-      Rect::MakeXYWH(100, 100, 400, 400).Expand(-100, -100), paint);
-  canvas.Restore();
-  ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture()));
-}
-
 TEST_P(AiksTest, ColorWheel) {
   // Compare with https://fiddle.skia.org/c/@BlendModes
 

diff --git a/impeller/compiler/shader_lib/impeller/color.glsl b/impeller/compiler/shader_lib/impeller/color.glsl
@@ -46,12 +46,4 @@ f16vec4 IPHalfPremultiply(f16vec4 color) {
   return f16vec4(color.rgb * color.a, color.a);
 }
 
-/// Performs the plus blend on `src` and `dst` which are premultiplied colors.
-//`max` determines the values the results are clamped to.
-f16vec4 IPHalfPlusBlend(f16vec4 src, f16vec4 dst) {
-  float16_t min = 0.0hf;
-  float16_t max = 1.0hf;
-  return clamp(dst + src, min, max);
-}
-
 #endif
diff --git a/impeller/core/formats.h b/impeller/core/formats.h
@@ -138,14 +138,6 @@ constexpr bool IsStencilWritable(PixelFormat format) {
   }
 }
 
-/// Returns `true` if the pixel format has an implicit `clamp(x, 0, 1)` in the
-/// pixel format. This is important for example when performing the `Plus` blend
-/// where we don't want alpha values over 1.0.
-constexpr bool IsAlphaClampedToOne(PixelFormat pixel_format) {
-  return !(pixel_format == PixelFormat::kR32G32B32A32Float ||
-           pixel_format == PixelFormat::kR16G16B16A16Float);
-}
-
 constexpr const char* PixelFormatToString(PixelFormat format) {
   switch (format) {
     case PixelFormat::kUnknown:

diff --git a/impeller/entity/contents/content_context.cc b/impeller/entity/contents/content_context.cc
@@ -124,8 +124,6 @@ void ContentContextOptions::ApplyToPipelineDescriptor(
       color0.src_color_blend_factor = BlendFactor::kOneMinusDestinationAlpha;
       break;
     case BlendMode::kPlus:
-      // The kPlusAdvanced should be used instead.
-      FML_DCHECK(IsAlphaClampedToOne(color_attachment_pixel_format));
       color0.dst_alpha_blend_factor = BlendFactor::kOne;
       color0.dst_color_blend_factor = BlendFactor::kOne;
       color0.src_alpha_blend_factor = BlendFactor::kOne;
@@ -326,10 +324,6 @@ ContentContext::ContentContext(
     framebuffer_blend_lighten_pipelines_.CreateDefault(
         *context_, options_trianglestrip,
         {static_cast<Scalar>(BlendSelectValues::kLighten), supports_decal});
-    framebuffer_blend_plus_advanced_pipelines_.CreateDefault(
-        *context_, options_trianglestrip,
-        {static_cast<Scalar>(BlendSelectValues::kPlusAdvanced),
-         supports_decal});
     framebuffer_blend_luminosity_pipelines_.CreateDefault(
         *context_, options_trianglestrip,
         {static_cast<Scalar>(BlendSelectValues::kLuminosity), supports_decal});
@@ -377,9 +371,6 @@ ContentContext::ContentContext(
   blend_lighten_pipelines_.CreateDefault(
       *context_, options_trianglestrip,
       {static_cast<Scalar>(BlendSelectValues::kLighten), supports_decal});
-  blend_plus_advanced_pipelines_.CreateDefault(
-      *context_, options_trianglestrip,
-      {static_cast<Scalar>(BlendSelectValues::kPlusAdvanced), supports_decal});
   blend_luminosity_pipelines_.CreateDefault(
       *context_, options_trianglestrip,
       {static_cast<Scalar>(BlendSelectValues::kLuminosity), supports_decal});

diff --git a/impeller/entity/contents/content_context.h b/impeller/entity/contents/content_context.h
@@ -197,8 +197,6 @@ using BlendHuePipeline =
     RenderPipelineT<AdvancedBlendVertexShader, AdvancedBlendFragmentShader>;
 using BlendLightenPipeline =
     RenderPipelineT<AdvancedBlendVertexShader, AdvancedBlendFragmentShader>;
-using BlendPlusAdvancedPipeline =
-    RenderPipelineT<AdvancedBlendVertexShader, AdvancedBlendFragmentShader>;
 using BlendLuminosityPipeline =
     RenderPipelineT<AdvancedBlendVertexShader, AdvancedBlendFragmentShader>;
 using BlendMultiplyPipeline =
@@ -239,9 +237,6 @@ using FramebufferBlendHuePipeline =
 using FramebufferBlendLightenPipeline =
     RenderPipelineT<FramebufferBlendVertexShader,
                     FramebufferBlendFragmentShader>;
-using FramebufferBlendPlusAdvancedPipeline =
-    RenderPipelineT<FramebufferBlendVertexShader,
-                    FramebufferBlendFragmentShader>;
 using FramebufferBlendLuminosityPipeline =
     RenderPipelineT<FramebufferBlendVertexShader,
                     FramebufferBlendFragmentShader>;
@@ -645,11 +640,6 @@ class ContentContext {
     return GetPipeline(blend_lighten_pipelines_, opts);
   }
 
-  std::shared_ptr<Pipeline<PipelineDescriptor>> GetBlendPlusAdvancedPipeline(
-      ContentContextOptions opts) const {
-    return GetPipeline(blend_plus_advanced_pipelines_, opts);
-  }
-
   std::shared_ptr<Pipeline<PipelineDescriptor>> GetBlendLuminosityPipeline(
       ContentContextOptions opts) const {
     return GetPipeline(blend_luminosity_pipelines_, opts);
@@ -735,12 +725,6 @@ class ContentContext {
     return GetPipeline(framebuffer_blend_lighten_pipelines_, opts);
   }
 
-  std::shared_ptr<Pipeline<PipelineDescriptor>>
-  GetFramebufferBlendPlusAdvancedPipeline(ContentContextOptions opts) const {
-    FML_DCHECK(GetDeviceCapabilities().SupportsFramebufferFetch());
-    return GetPipeline(framebuffer_blend_plus_advanced_pipelines_, opts);
-  }
-
   std::shared_ptr<Pipeline<PipelineDescriptor>>
   GetFramebufferBlendLuminosityPipeline(ContentContextOptions opts) const {
     FML_DCHECK(GetDeviceCapabilities().SupportsFramebufferFetch());
@@ -1005,7 +989,6 @@ class ContentContext {
   mutable Variants<BlendHardLightPipeline> blend_hardlight_pipelines_;
   mutable Variants<BlendHuePipeline> blend_hue_pipelines_;
   mutable Variants<BlendLightenPipeline> blend_lighten_pipelines_;
-  mutable Variants<BlendPlusAdvancedPipeline> blend_plus_advanced_pipelines_;
   mutable Variants<BlendLuminosityPipeline> blend_luminosity_pipelines_;
   mutable Variants<BlendMultiplyPipeline> blend_multiply_pipelines_;
   mutable Variants<BlendOverlayPipeline> blend_overlay_pipelines_;
@@ -1031,8 +1014,6 @@ class ContentContext {
       framebuffer_blend_hue_pipelines_;
   mutable Variants<FramebufferBlendLightenPipeline>
       framebuffer_blend_lighten_pipelines_;
-  mutable Variants<FramebufferBlendPlusAdvancedPipeline>
-      framebuffer_blend_plus_advanced_pipelines_;
   mutable Variants<FramebufferBlendLuminosityPipeline>
       framebuffer_blend_luminosity_pipelines_;
   mutable Variants<FramebufferBlendMultiplyPipeline>

diff --git a/impeller/entity/contents/filters/blend_filter_contents.cc b/impeller/entity/contents/filters/blend_filter_contents.cc
@@ -338,9 +338,6 @@ std::optional<Entity> BlendFilterContents::CreateForegroundAdvancedBlend(
       case BlendMode::kColor:
         pass.SetPipeline(renderer.GetBlendColorPipeline(options));
         break;
-      case BlendMode::kPlusAdvanced:
-        pass.SetPipeline(renderer.GetBlendPlusAdvancedPipeline(options));
-        break;
       case BlendMode::kLuminosity:
         pass.SetPipeline(renderer.GetBlendLuminosityPipeline(options));
         break;
@@ -586,7 +583,6 @@ void BlendFilterContents::SetBlendMode(BlendMode blend_mode) {
       BLEND_CASE(Hue)
       BLEND_CASE(Saturation)
       BLEND_CASE(Color)
-      BLEND_CASE(PlusAdvanced)
       BLEND_CASE(Luminosity)
       default:
         FML_UNREACHABLE();
@@ -615,26 +611,19 @@ std::optional<Entity> BlendFilterContents::RenderFilter(
                          std::nullopt, GetAbsorbOpacity(), GetAlpha());
   }
 
-  BlendMode blend_mode = blend_mode_;
-  if (blend_mode == BlendMode::kPlus &&
-      !IsAlphaClampedToOne(
-          renderer.GetContext()->GetCapabilities()->GetDefaultColorFormat())) {
-    blend_mode = BlendMode::kPlusAdvanced;
-  }
-
-  if (blend_mode <= Entity::kLastPipelineBlendMode) {
-    return PipelineBlend(inputs, renderer, entity, coverage, blend_mode,
+  if (blend_mode_ <= Entity::kLastPipelineBlendMode) {
+    return PipelineBlend(inputs, renderer, entity, coverage, blend_mode_,
                          foreground_color_, GetAbsorbOpacity(), GetAlpha());
   }
 
-  if (blend_mode <= Entity::kLastAdvancedBlendMode) {
+  if (blend_mode_ <= Entity::kLastAdvancedBlendMode) {
     if (inputs.size() == 1 && foreground_color_.has_value() &&
         GetAbsorbOpacity() == ColorFilterContents::AbsorbOpacity::kYes) {
       return CreateForegroundAdvancedBlend(
           inputs[0], renderer, entity, coverage, foreground_color_.value(),
-          blend_mode, GetAlpha(), GetAbsorbOpacity());
+          blend_mode_, GetAlpha(), GetAbsorbOpacity());
     }
-    return advanced_blend_proc_(inputs, renderer, entity, coverage, blend_mode,
+    return advanced_blend_proc_(inputs, renderer, entity, coverage, blend_mode_,
                                 foreground_color_, GetAbsorbOpacity(),
                                 GetAlpha());
   }

diff --git a/impeller/entity/contents/framebuffer_blend_contents.cc b/impeller/entity/contents/framebuffer_blend_contents.cc
@@ -118,10 +118,6 @@ bool FramebufferBlendContents::Render(const ContentContext& renderer,
     case BlendMode::kColor:
       pass.SetPipeline(renderer.GetFramebufferBlendColorPipeline(options));
       break;
-    case BlendMode::kPlusAdvanced:
-      pass.SetPipeline(
-          renderer.GetFramebufferBlendPlusAdvancedPipeline(options));
-      break;
     case BlendMode::kLuminosity:
       pass.SetPipeline(renderer.GetFramebufferBlendLuminosityPipeline(options));
       break;

diff --git a/impeller/entity/contents/framebuffer_blend_contents.h b/impeller/entity/contents/framebuffer_blend_contents.h
@@ -27,7 +27,6 @@ enum class BlendSelectValues {
   kHue,
   kSaturation,
   kColor,
-  kPlusAdvanced,
   kLuminosity,
 };
 

diff --git a/impeller/entity/entity_pass.cc b/impeller/entity/entity_pass.cc
@@ -970,13 +970,6 @@ bool EntityPass::OnRender(
     /// Setup advanced blends.
     ///
 
-    if (result.entity.GetBlendMode() == BlendMode::kPlus &&
-        !IsAlphaClampedToOne(pass_context.GetPassTarget()
-                                 .GetRenderTarget()
-                                 .GetRenderTargetPixelFormat())) {
-      result.entity.SetBlendMode(BlendMode::kPlusAdvanced);
-    }
-
     if (result.entity.GetBlendMode() > Entity::kLastPipelineBlendMode) {
       if (renderer.GetDeviceCapabilities().SupportsFramebufferFetch()) {
         auto src_contents = result.entity.GetContents();

diff --git a/impeller/entity/shaders/blending/advanced_blend.frag b/impeller/entity/shaders/blending/advanced_blend.frag
@@ -36,33 +36,22 @@ f16vec4 Sample(f16sampler2D texture_sampler, vec2 texture_coords) {
 }
 
 void main() {
-  f16vec4 premultiplied_dst =
-      Sample(texture_sampler_dst,  // sampler
-             v_dst_texture_coords  // texture coordinates
-      );
-  int nblend_type = int(blend_type);
-
-  if (nblend_type == /*BlendSelectValues::kPlusAdvanced*/ 14) {
-    f16vec4 premultiplied_src =
-        Sample(texture_sampler_src,  // sampler
-               v_src_texture_coords  // texture coordinates
-        );
-    frag_color = IPHalfPlusBlend(premultiplied_src, premultiplied_dst);
-  } else {
-    f16vec4 dst = IPHalfUnpremultiply(premultiplied_dst);
-    dst *= blend_info.dst_input_alpha;
+  f16vec4 dst =
+      IPHalfUnpremultiply(Sample(texture_sampler_dst,  // sampler
+                                 v_dst_texture_coords  // texture coordinates
+                                 ));
+  dst *= blend_info.dst_input_alpha;
+  f16vec4 src = blend_info.color_factor > 0.0hf
+                    ? blend_info.color
+                    : IPHalfUnpremultiply(Sample(
+                          texture_sampler_src,  // sampler
+                          v_src_texture_coords  // texture coordinates
+                          ));
+  if (blend_info.color_factor == 0.0hf) {
+    src.a *= blend_info.src_input_alpha;
+  }
 
-    f16vec4 src = blend_info.color_factor > 0.0hf
-                      ? blend_info.color
-                      : IPHalfUnpremultiply(Sample(
-                            texture_sampler_src,  // sampler
-                            v_src_texture_coords  // texture coordinates
-                            ));
-    if (blend_info.color_factor == 0.0hf) {
-      src.a *= blend_info.src_input_alpha;
-    }
+  f16vec3 blend_result = AdvancedBlend(dst.rgb, src.rgb, int(blend_type));
 
-    f16vec3 blend_result = AdvancedBlend(dst.rgb, src.rgb, nblend_type);
-    frag_color = IPApplyBlendedColor(dst, src, blend_result);
-  }
+  frag_color = IPApplyBlendedColor(dst, src, blend_result);
 }
diff --git a/impeller/entity/shaders/blending/blend_select.glsl b/impeller/entity/shaders/blending/blend_select.glsl
@@ -20,7 +20,6 @@
 // kHue,
 // kSaturation,
 // kColor,
-// kPlusAdvanced,
 // kLuminosity,
 // Note, this isn't a switch as GLSL ES 1.0 does not support them.
 f16vec3 AdvancedBlend(f16vec3 dst, f16vec3 src, int blend_type) {
@@ -66,9 +65,7 @@ f16vec3 AdvancedBlend(f16vec3 dst, f16vec3 src, int blend_type) {
   if (blend_type == 13) {
     return IPBlendColor(dst, src);
   }
-  // 14 is `PlusAdvanced`, it's handled in advanced_blend.frag and
-  // framebuffer_blend.frag.
-  if (blend_type == 15) {
+  if (blend_type == 14) {
     return IPBlendLuminosity(dst, src);
   }
   return f16vec3(0.0hf);

diff --git a/impeller/entity/shaders/blending/framebuffer_blend.frag b/impeller/entity/shaders/blending/framebuffer_blend.frag
@@ -43,21 +43,14 @@ vec4 Sample(sampler2D texture_sampler, vec2 texture_coords) {
 }
 
 void main() {
-  f16vec4 premultiplied_dst = f16vec4(ReadDestination());
-  f16vec4 premultiplied_src =
+  f16vec4 dst = IPHalfUnpremultiply(f16vec4(ReadDestination()));
+  f16vec4 src = IPHalfUnpremultiply(
       f16vec4(Sample(texture_sampler_src,  // sampler
                      v_src_texture_coords  // texture coordinates
-                     ));
-  int nblend_type = int(blend_type);
-
-  if (nblend_type == /*BlendSelectValues::kPlusAdvanced*/ 14) {
-    frag_color = IPHalfPlusBlend(premultiplied_src, premultiplied_dst);
-  } else {
-    f16vec4 dst = IPHalfUnpremultiply(premultiplied_dst);
-    f16vec4 src = IPHalfUnpremultiply(premultiplied_src);
-    src.a *= frag_info.src_input_alpha;
-
-    f16vec3 blend_result = AdvancedBlend(dst.rgb, src.rgb, nblend_type);
-    frag_color = IPApplyBlendedColor(dst, src, blend_result);
-  }
+                     )));
+  src.a *= frag_info.src_input_alpha;
+
+  f16vec3 blend_result = AdvancedBlend(dst.rgb, src.rgb, int(blend_type));
+
+  frag_color = IPApplyBlendedColor(dst, src, blend_result);
 }
diff --git a/impeller/geometry/color.cc b/impeller/geometry/color.cc
@@ -276,8 +276,6 @@ Color Color::Blend(Color src, BlendMode blend_mode) const {
       return (src.Premultiply() * (1 - dst.alpha) +
               dst.Premultiply() * (1 - src.alpha))
           .Unpremultiply();
-    case BlendMode::kPlusAdvanced:
-      [[fallthrough]];
     case BlendMode::kPlus:
       // r = min(s + d, 1)
       return (Min(src.Premultiply() + dst.Premultiply(), 1)).Unpremultiply();