[Impeller] handle shader ordering bug on macOS. (#165937)

Fixes flutter/flutter#165740

if a user declares interleaved `float` and `sampler2D` uniforms, then we
can screw up the binding order.

This only manifests on platforms where we don't combine all float/scalar
uniform values into a single struct. We should probably start doing that
for metal, but refactoring there is out of scope for this patch.

I enabled a large number of skipped tests we probably should have turned
on by now. oops.

---------

Co-authored-by: gaaclarke <30870216+gaaclarke@users.noreply.github.com>

Author: Jonah Williams

engine/src/flutter/impeller/entity/contents/runtime_effect_contents.cc

@@ -168,8 +168,9 @@ RuntimeEffectContents::CreatePipeline(const ContentContext& renderer,
   const std::shared_ptr<Context>& context = renderer.GetContext();
   const std::shared_ptr<ShaderLibrary>& library = context->GetShaderLibrary();
   const std::shared_ptr<const Capabilities>& caps = context->GetCapabilities();
-  const auto color_attachment_format = caps->GetDefaultColorFormat();
-  const auto stencil_attachment_format = caps->GetDefaultDepthStencilFormat();
+  const PixelFormat color_attachment_format = caps->GetDefaultColorFormat();
+  const PixelFormat stencil_attachment_format =
+      caps->GetDefaultDepthStencilFormat();
 
   using VS = RuntimeEffectVertexShader;
 
@@ -233,25 +234,37 @@ bool RuntimeEffectContents::Render(const ContentContext& renderer,
   ///
   BindFragmentCallback bind_callback = [this, &renderer,
                                         &context](RenderPass& pass) {
-    size_t minimum_sampler_index = 100000000;
     size_t buffer_index = 0;
     size_t buffer_offset = 0;
-
+    size_t sampler_location = 0;
+    size_t buffer_location = 0;
+
+    // Uniforms are ordered in the IPLR according to their
+    // declaration and the uniform location reflects the correct offset to
+    // be mapped to - except that it may include all proceeding
+    // uniforms of a different type. For example, a texture sampler that comes
+    // after 4 float uniforms may have a location of 4. Since we know that
+    // the declarations are already ordered, we can track the uniform location
+    // ourselves.
     for (const auto& uniform : runtime_stage_->GetUniforms()) {
       std::unique_ptr<ShaderMetadata> metadata = MakeShaderMetadata(uniform);
       switch (uniform.type) {
         case kSampledImage: {
-          // Sampler uniforms are ordered in the IPLR according to their
-          // declaration and the uniform location reflects the correct offset to
-          // be mapped to - except that it may include all proceeding float
-          // uniforms. For example, a float sampler that comes after 4 float
-          // uniforms may have a location of 4. To convert to the actual offset
-          // we need to find the largest location assigned to a float uniform
-          // and then subtract this from all uniform locations. This is more or
-          // less the same operation we previously performed in the shader
-          // compiler.
-          minimum_sampler_index =
-              std::min(minimum_sampler_index, uniform.location);
+          FML_DCHECK(sampler_location < texture_inputs_.size());
+          auto& input = texture_inputs_[sampler_location];
+
+          raw_ptr<const Sampler> sampler =
+              context->GetSamplerLibrary()->GetSampler(
+                  input.sampler_descriptor);
+
+          SampledImageSlot image_slot;
+          image_slot.name = uniform.name.c_str();
+          image_slot.binding = uniform.binding;
+          image_slot.texture_index = sampler_location;
+          pass.BindDynamicResource(ShaderStage::kFragment,
+                                   DescriptorType::kSampledImage, image_slot,
+                                   std::move(metadata), input.texture, sampler);
+          sampler_location++;
           break;
         }
         case kFloat: {
@@ -268,12 +281,13 @@ bool RuntimeEffectContents::Render(const ContentContext& renderer,
 
           ShaderUniformSlot uniform_slot;
           uniform_slot.name = uniform.name.c_str();
-          uniform_slot.ext_res_0 = uniform.location;
+          uniform_slot.ext_res_0 = buffer_location;
           pass.BindDynamicResource(ShaderStage::kFragment,
                                    DescriptorType::kUniformBuffer, uniform_slot,
                                    std::move(metadata), std::move(buffer_view));
           buffer_index++;
           buffer_offset += uniform.GetSize();
+          buffer_location++;
           break;
         }
         case kStruct: {
@@ -292,34 +306,6 @@ bool RuntimeEffectContents::Render(const ContentContext& renderer,
       }
     }
 
-    size_t sampler_index = 0;
-    for (const auto& uniform : runtime_stage_->GetUniforms()) {
-      std::unique_ptr<ShaderMetadata> metadata = MakeShaderMetadata(uniform);
-
-      switch (uniform.type) {
-        case kSampledImage: {
-          FML_DCHECK(sampler_index < texture_inputs_.size());
-          auto& input = texture_inputs_[sampler_index];
-
-          raw_ptr<const Sampler> sampler =
-              context->GetSamplerLibrary()->GetSampler(
-                  input.sampler_descriptor);
-
-          SampledImageSlot image_slot;
-          image_slot.name = uniform.name.c_str();
-          image_slot.binding = uniform.binding;
-          image_slot.texture_index = uniform.location - minimum_sampler_index;
-          pass.BindDynamicResource(ShaderStage::kFragment,
-                                   DescriptorType::kSampledImage, image_slot,
-                                   std::move(metadata), input.texture, sampler);
-
-          sampler_index++;
-          break;
-        }
-        default:
-          continue;
-      }
-    }
     return true;
   };
 

engine/src/flutter/lib/ui/fixtures/shaders/general_shaders/BUILD.gn

@@ -16,6 +16,7 @@ if (enable_unittests) {
     "simple.frag",
     "uniforms.frag",
     "uniforms_sorted.frag",
+    "uniform_ordering.frag",
     "uniform_arrays.frag",
     "filter_shader.frag",
     "missing_size.frag",

engine/src/flutter/lib/ui/fixtures/shaders/general_shaders/uniform_ordering.frag

@@ -0,0 +1,20 @@
+#version 300 es
+
+// Copyright 2013 The Flutter Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+uniform float a;
+uniform sampler2D u_texture;
+uniform float b;
+uniform float c;
+
+out vec4 frag_color;
+
+void main() {
+  if (a == 1.0 && b == 2.0 && c == 3.0) {
+    frag_color = vec4(0, 1, 0, 1);
+  } else {
+    frag_color = vec4(1, 0, 0, 1);
+  }
+}

engine/src/flutter/testing/dart/fragment_shader_test.dart

@@ -195,21 +195,13 @@ void main() async {
   });
 
   test('FragmentShader simple shader renders correctly', () async {
-    if (impellerEnabled) {
-      print('Skipped for Impeller - https://github.com/flutter/flutter/issues/122823');
-      return;
-    }
     final FragmentProgram program = await FragmentProgram.fromAsset('functions.frag.iplr');
     final FragmentShader shader = program.fragmentShader()..setFloat(0, 1.0);
     await _expectShaderRendersGreen(shader);
     shader.dispose();
   });
 
   test('Reused FragmentShader simple shader renders correctly', () async {
-    if (impellerEnabled) {
-      print('Skipped for Impeller - https://github.com/flutter/flutter/issues/122823');
-      return;
-    }
     final FragmentProgram program = await FragmentProgram.fromAsset('functions.frag.iplr');
     final FragmentShader shader = program.fragmentShader()..setFloat(0, 1.0);
     await _expectShaderRendersGreen(shader);
@@ -221,10 +213,6 @@ void main() async {
   });
 
   test('FragmentShader blue-green image renders green', () async {
-    if (impellerEnabled) {
-      print('Skipped for Impeller - https://github.com/flutter/flutter/issues/122823');
-      return;
-    }
     final FragmentProgram program = await FragmentProgram.fromAsset('blue_green_sampler.frag.iplr');
     final Image blueGreenImage = await _createBlueGreenImage();
     final FragmentShader shader = program.fragmentShader()..setImageSampler(0, blueGreenImage);
@@ -234,10 +222,6 @@ void main() async {
   });
 
   test('FragmentShader blue-green image renders green - GPU image', () async {
-    if (impellerEnabled) {
-      print('Skipped for Impeller - https://github.com/flutter/flutter/issues/122823');
-      return;
-    }
     final FragmentProgram program = await FragmentProgram.fromAsset('blue_green_sampler.frag.iplr');
     final Image blueGreenImage = _createBlueGreenImageSync();
     final FragmentShader shader = program.fragmentShader()..setImageSampler(0, blueGreenImage);
@@ -247,10 +231,6 @@ void main() async {
   });
 
   test('FragmentShader with uniforms renders correctly', () async {
-    if (impellerEnabled) {
-      print('Skipped for Impeller - https://github.com/flutter/flutter/issues/122823');
-      return;
-    }
     final FragmentProgram program = await FragmentProgram.fromAsset('uniforms.frag.iplr');
 
     final FragmentShader shader =
@@ -274,10 +254,6 @@ void main() async {
   });
 
   test('FragmentShader shader with array uniforms renders correctly', () async {
-    if (impellerEnabled) {
-      print('Skipped for Impeller - https://github.com/flutter/flutter/issues/122823');
-      return;
-    }
     final FragmentProgram program = await FragmentProgram.fromAsset('uniform_arrays.frag.iplr');
 
     final FragmentShader shader = program.fragmentShader();
@@ -305,10 +281,6 @@ void main() async {
   });
 
   test('FragmentShader Uniforms are sorted correctly', () async {
-    if (impellerEnabled) {
-      print('Skipped for Impeller - https://github.com/flutter/flutter/issues/122823');
-      return;
-    }
     final FragmentProgram program = await FragmentProgram.fromAsset('uniforms_sorted.frag.iplr');
 
     // The shader will not render green if the compiler doesn't keep the
@@ -323,6 +295,24 @@ void main() async {
     shader.dispose();
   });
 
+  test('FragmentShader Uniforms with interleaved textures are sorted ', () async {
+    final FragmentProgram program = await FragmentProgram.fromAsset('uniform_ordering.frag.iplr');
+
+    // The shader will not render green if the compiler doesn't keep the
+    // uniforms in the right order.
+    final FragmentShader shader = program.fragmentShader();
+    shader.setFloat(0, 1);
+    shader.setFloat(1, 2);
+    shader.setFloat(2, 3);
+
+    final Image blueGreenImage = _createBlueGreenImageSync();
+    shader.setImageSampler(0, blueGreenImage);
+
+    await _expectShaderRendersGreen(shader);
+
+    shader.dispose();
+  });
+
   test('fromAsset throws an exception on invalid assetKey', () async {
     bool throws = false;
     try {