[Impeller] Compute correct-ish binding order for Vulkan.

impeller/compiler/reflector.cc

@@ -376,6 +376,8 @@ std::shared_ptr<RuntimeStageData::Shader> Reflector::GenerateRuntimeStageData()
 
     const auto& ubo = ubos[0];
 
+    size_t binding =
+        compiler_->get_decoration(ubo.id, spv::Decoration::DecorationBinding);
     auto members = ReadStructMembers(ubo.type_id);
     std::vector<uint8_t> struct_layout;
     size_t float_count = 0;
@@ -410,9 +412,8 @@ std::shared_ptr<RuntimeStageData::Shader> Reflector::GenerateRuntimeStageData()
     }
     data->uniforms.emplace_back(UniformDescription{
         .name = ubo.name,
-        .location = 64,  // Magic constant that must match the descriptor set
-                         // location for fragment programs.
-        .binding = 64,
+        .location = binding,
+        .binding = binding,
         .type = spirv_cross::SPIRType::Struct,
         .struct_layout = std::move(struct_layout),
         .struct_float_count = float_count,

impeller/fixtures/BUILD.gn

@@ -70,6 +70,8 @@ impellerc("runtime_stages") {
     "ink_sparkle.frag",
     "runtime_stage_example.frag",
     "gradient.frag",
+    "uniforms_and_sampler_1.frag",
+    "uniforms_and_sampler_2.frag",
   ]
   sl_file_extension = "iplr"
 

impeller/fixtures/uniforms_and_sampler_1.frag

@@ -0,0 +1,13 @@
+// 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.
+
+// sampler is specifically ordered before color.
+uniform sampler2D u_texture;
+uniform vec4 u_color;
+
+out vec4 frag_color;
+
+void main() {
+  frag_color = u_color + texture(u_texture, vec2(0.5, 0.5));
+}

impeller/fixtures/uniforms_and_sampler_2.frag

@@ -0,0 +1,13 @@
+// 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 vec4 u_color;
+// sampler is specifically ordered after color.
+uniform sampler2D u_texture;
+
+out vec4 frag_color;
+
+void main() {
+  frag_color = u_color + texture(u_texture, vec2(0.5, 0.5));
+}

impeller/runtime_stage/runtime_stage.cc

@@ -89,13 +89,23 @@ RuntimeStage::RuntimeStage(const fb::RuntimeStage* runtime_stage,
   entrypoint_ = runtime_stage->entrypoint()->str();
 
   auto* uniforms = runtime_stage->uniforms();
+
+  // Note: image bindings are screwy and will always have the same offset.
+  // track the binding of the UBO to determine where the image bindings go.
+  // This is only guaranteed to give us the correct bindings if there is a
+  // single sampler2D.
+  std::optional<size_t> ubo_id;
   if (uniforms) {
     for (auto i = uniforms->begin(), end = uniforms->end(); i != end; i++) {
       RuntimeUniformDescription desc;
       desc.name = i->name()->str();
       desc.location = i->location();
       desc.binding = i->binding();
       desc.type = ToType(i->type());
+      if (desc.type == kStruct) {
+        ubo_id = desc.location;
+        desc.binding = desc.location;
+      }
       desc.dimensions = RuntimeUniformDimensions{
           static_cast<size_t>(i->rows()), static_cast<size_t>(i->columns())};
       desc.bit_width = i->bit_width();
@@ -105,7 +115,6 @@ RuntimeStage::RuntimeStage(const fb::RuntimeStage* runtime_stage,
           desc.struct_layout.push_back(static_cast<uint8_t>(byte_type));
         }
       }
-      desc.binding = i->binding();
       desc.struct_float_count = i->struct_float_count();
       uniforms_.push_back(std::move(desc));
     }
@@ -117,6 +126,20 @@ RuntimeStage::RuntimeStage(const fb::RuntimeStage* runtime_stage,
       [payload = payload_](auto, auto) {}  //
   );
 
+  size_t binding = 64;
+  if (ubo_id.has_value() && ubo_id.value() == binding) {
+    binding++;
+  }
+  for (auto& uniform : uniforms_) {
+    if (uniform.type == kSampledImage) {
+      uniform.binding = binding;
+      binding++;
+      if (ubo_id.has_value() && ubo_id.value() == binding) {
+        binding++;
+      }
+    }
+  }
+
   for (const auto& uniform : GetUniforms()) {
     if (uniform.type == kStruct) {
       descriptor_set_layouts_.push_back(DescriptorSetLayout{
@@ -132,7 +155,6 @@ RuntimeStage::RuntimeStage(const fb::RuntimeStage* runtime_stage,
       });
     }
   }
-
   is_valid_ = true;
 }
 

impeller/runtime_stage/runtime_stage_unittests.cc

@@ -7,6 +7,7 @@
 #include "flutter/fml/make_copyable.h"
 #include "flutter/testing/testing.h"
 #include "gmock/gmock.h"
+#include "gtest/gtest.h"
 #include "impeller/base/allocation.h"
 #include "impeller/base/validation.h"
 #include "impeller/core/runtime_types.h"
@@ -225,6 +226,56 @@ TEST_P(RuntimeStageTest, CanReadUniforms) {
   }
 }
 
+TEST_P(RuntimeStageTest, CanReadUniformsSamplerBeforeUBO) {
+  if (GetBackend() != PlaygroundBackend::kVulkan) {
+    GTEST_SKIP() << "Test only relevant for Vulkan";
+  }
+  const std::shared_ptr<fml::Mapping> fixture =
+      flutter::testing::OpenFixtureAsMapping(
+          "uniforms_and_sampler_1.frag.iplr");
+  ASSERT_TRUE(fixture);
+  ASSERT_GT(fixture->GetSize(), 0u);
+  auto stages = RuntimeStage::DecodeRuntimeStages(fixture);
+  auto stage = stages[PlaygroundBackendToRuntimeStageBackend(GetBackend())];
+
+  EXPECT_EQ(stage->GetUniforms().size(), 2u);
+  auto uni = stage->GetUniform(RuntimeStage::kVulkanUBOName);
+  ASSERT_TRUE(uni);
+  // Struct must be offset at 65.
+  EXPECT_EQ(uni->type, RuntimeUniformType::kStruct);
+  EXPECT_EQ(uni->binding, 65u);
+  // Sampler should be offset at 64 but due to current bug
+  // has offset of 0, the correct offset is computed at runtime.
+  auto sampler_uniform = stage->GetUniform("u_texture");
+  EXPECT_EQ(sampler_uniform->type, RuntimeUniformType::kSampledImage);
+  EXPECT_EQ(sampler_uniform->binding, 64u);
+}
+
+TEST_P(RuntimeStageTest, CanReadUniformsSamplerAfterUBO) {
+  if (GetBackend() != PlaygroundBackend::kVulkan) {
+    GTEST_SKIP() << "Test only relevant for Vulkan";
+  }
+  const std::shared_ptr<fml::Mapping> fixture =
+      flutter::testing::OpenFixtureAsMapping(
+          "uniforms_and_sampler_2.frag.iplr");
+  ASSERT_TRUE(fixture);
+  ASSERT_GT(fixture->GetSize(), 0u);
+  auto stages = RuntimeStage::DecodeRuntimeStages(fixture);
+  auto stage = stages[PlaygroundBackendToRuntimeStageBackend(GetBackend())];
+
+  EXPECT_EQ(stage->GetUniforms().size(), 2u);
+  auto uni = stage->GetUniform(RuntimeStage::kVulkanUBOName);
+  ASSERT_TRUE(uni);
+  // Struct must be offset at 45.
+  EXPECT_EQ(uni->type, RuntimeUniformType::kStruct);
+  EXPECT_EQ(uni->binding, 64u);
+  // Sampler should be offset at 64 but due to current bug
+  // has offset of 0, the correct offset is computed at runtime.
+  auto sampler_uniform = stage->GetUniform("u_texture");
+  EXPECT_EQ(sampler_uniform->type, RuntimeUniformType::kSampledImage);
+  EXPECT_EQ(sampler_uniform->binding, 65u);
+}
+
 TEST_P(RuntimeStageTest, CanRegisterStage) {
   const std::shared_ptr<fml::Mapping> fixture =
       flutter::testing::OpenFixtureAsMapping("ink_sparkle.frag.iplr");