[Impeller] Make interleaved layout (more) explicit in generated headers.

impeller/compiler/code_gen_template.h

@@ -86,7 +86,8 @@ struct {{camel_case(shader_name)}}{{camel_case(shader_stage)}}Shader {
     {{stage_input.type.type_name}},     // type
     {{stage_input.type.bit_width}}u,    // bit width of type
     {{stage_input.type.vec_size}}u,     // vec size
-    {{stage_input.type.columns}}u       // number of columns
+    {{stage_input.type.columns}}u,      // number of columns
+    {{stage_input.offset}}u,            // offset for interleaved layout
   };
 {% endfor %}
 {% endif %}
@@ -97,6 +98,20 @@ struct {{camel_case(shader_name)}}{{camel_case(shader_stage)}}Shader {
 {% endfor %}
   };
 
+{% if shader_stage == "vertex" %}
+  static constexpr auto kInterleavedLayout = ShaderStageBufferLayout {
+{% if length(stage_inputs) > 0 %}
+    sizeof(PerVertexData),                 // stride for interleaved layout
+{% else %}
+    0u,
+{% endif %}
+    0u,                                    // attribute binding
+  };
+  static constexpr std::array<const ShaderStageBufferLayout*, 1> kInterleavedBufferLayout = {
+    &kInterleavedLayout
+  };
+{% endif %}
+
 {% if length(sampled_images) > 0 %}
   // ===========================================================================
   // Sampled Images ============================================================

impeller/compiler/reflector.cc

@@ -226,8 +226,9 @@ std::optional<nlohmann::json> Reflector::GenerateTemplateArguments() const {
 
   {
     auto& stage_inputs = root["stage_inputs"] = nlohmann::json::array_t{};
-    if (auto stage_inputs_json =
-            ReflectResources(shader_resources.stage_inputs);
+    if (auto stage_inputs_json = ReflectResources(
+            shader_resources.stage_inputs,
+            /*compute_offsets=*/execution_model == spv::ExecutionModelVertex);
         stage_inputs_json.has_value()) {
       stage_inputs = std::move(stage_inputs_json.value());
     } else {
@@ -402,8 +403,36 @@ std::shared_ptr<fml::Mapping> Reflector::InflateTemplate(
       inflated_template->size(), [inflated_template](auto, auto) {});
 }
 
+std::vector<size_t> Reflector::ComputeOffsets(
+    const spirv_cross::SmallVector<spirv_cross::Resource>& resources) const {
+  std::vector<size_t> offsets(resources.size(), 0);
+  if (resources.size() == 0) {
+    return offsets;
+  }
+  for (const auto& resource : resources) {
+    const auto type = compiler_->get_type(resource.type_id);
+    auto location = compiler_->get_decoration(
+        resource.id, spv::Decoration::DecorationLocation);
+    // Malformed shader, will be caught later on.
+    if (location >= resources.size() || location < 0) {
+      location = 0;
+    }
+    offsets[location] = (type.width * type.vecsize) / 8;
+  }
+  for (size_t i = 1; i < resources.size(); i++) {
+    offsets[i] += offsets[i - 1];
+  }
+  for (size_t i = resources.size() - 1; i > 0; i--) {
+    offsets[i] = offsets[i - 1];
+  }
+  offsets[0] = 0;
+
+  return offsets;
+}
+
 std::optional<nlohmann::json::object_t> Reflector::ReflectResource(
-    const spirv_cross::Resource& resource) const {
+    const spirv_cross::Resource& resource,
+    std::optional<size_t> offset) const {
   nlohmann::json::object_t result;
 
   result["name"] = resource.name;
@@ -426,6 +455,7 @@ std::optional<nlohmann::json::object_t> Reflector::ReflectResource(
     return std::nullopt;
   }
   result["type"] = std::move(type.value());
+  result["offset"] = offset.value_or(0u);
   return result;
 }
 
@@ -462,11 +492,23 @@ std::optional<nlohmann::json::object_t> Reflector::ReflectType(
 }
 
 std::optional<nlohmann::json::array_t> Reflector::ReflectResources(
-    const spirv_cross::SmallVector<spirv_cross::Resource>& resources) const {
+    const spirv_cross::SmallVector<spirv_cross::Resource>& resources,
+    bool compute_offsets) const {
   nlohmann::json::array_t result;
   result.reserve(resources.size());
+  std::vector<size_t> offsets;
+  if (compute_offsets) {
+    offsets = ComputeOffsets(resources);
+  }
   for (const auto& resource : resources) {
-    if (auto reflected = ReflectResource(resource); reflected.has_value()) {
+    std::optional<size_t> maybe_offset = std::nullopt;
+    if (compute_offsets) {
+      auto location = compiler_->get_decoration(
+          resource.id, spv::Decoration::DecorationLocation);
+      maybe_offset = offsets[location];
+    }
+    if (auto reflected = ReflectResource(resource, maybe_offset);
+        reflected.has_value()) {
       result.emplace_back(std::move(reflected.value()));
     } else {
       return std::nullopt;

impeller/compiler/reflector.h

@@ -113,9 +113,14 @@ class Reflector {
   std::shared_ptr<fml::Mapping> InflateTemplate(std::string_view tmpl) const;
 
   std::optional<nlohmann::json::object_t> ReflectResource(
-      const spirv_cross::Resource& resource) const;
+      const spirv_cross::Resource& resource,
+      std::optional<size_t> offset) const;
 
   std::optional<nlohmann::json::array_t> ReflectResources(
+      const spirv_cross::SmallVector<spirv_cross::Resource>& resources,
+      bool compute_offsets = false) const;
+
+  std::vector<size_t> ComputeOffsets(
       const spirv_cross::SmallVector<spirv_cross::Resource>& resources) const;
 
   std::optional<nlohmann::json::object_t> ReflectType(

impeller/core/shader_types.h

@@ -94,10 +94,11 @@ struct ShaderStageIOSlot {
   size_t bit_width;
   size_t vec_size;
   size_t columns;
+  size_t offset;
 
   constexpr size_t GetHash() const {
     return fml::HashCombine(name, location, set, binding, type, bit_width,
-                            vec_size, columns);
+                            vec_size, columns, offset);
   }
 
   constexpr bool operator==(const ShaderStageIOSlot& other) const {
@@ -108,7 +109,20 @@ struct ShaderStageIOSlot {
            type == other.type &&            //
            bit_width == other.bit_width &&  //
            vec_size == other.vec_size &&    //
-           columns == other.columns;
+           columns == other.columns &&      //
+           offset == other.offset;
+  }
+};
+
+struct ShaderStageBufferLayout {
+  size_t stride;
+  size_t binding;
+
+  constexpr size_t GetHash() const { return fml::HashCombine(stride, binding); }
+
+  constexpr bool operator==(const ShaderStageBufferLayout& other) const {
+    return stride == other.stride &&  //
+           binding == other.binding;
   }
 };
 

impeller/entity/contents/runtime_effect_contents.cc

@@ -118,9 +118,8 @@ bool RuntimeEffectContents::Render(const ContentContext& renderer,
   desc.AddStageEntrypoint(library->GetFunction(runtime_stage_->GetEntrypoint(),
                                                ShaderStage::kFragment));
   auto vertex_descriptor = std::make_shared<VertexDescriptor>();
-  if (!vertex_descriptor->SetStageInputs(VS::kAllShaderStageInputs)) {
-    VALIDATION_LOG << "Failed to set stage inputs for runtime effect pipeline.";
-  }
+  vertex_descriptor->SetStageInputs(VS::kAllShaderStageInputs,
+                                    VS::kInterleavedBufferLayout);
   desc.SetVertexDescriptor(std::move(vertex_descriptor));
   desc.SetColorAttachmentDescriptor(
       0u, {.format = color_attachment_format, .blending_enabled = true});

impeller/renderer/backend/gles/buffer_bindings_gles.cc

@@ -23,17 +23,12 @@ BufferBindingsGLES::~BufferBindingsGLES() = default;
 
 bool BufferBindingsGLES::RegisterVertexStageInput(
     const ProcTableGLES& gl,
-    const std::vector<ShaderStageIOSlot>& p_inputs) {
-  // Attrib locations have to be iterated over in order of location because we
-  // will be calculating offsets later.
-  auto inputs = p_inputs;
-  std::sort(inputs.begin(), inputs.end(), [](const auto& lhs, const auto& rhs) {
-    return lhs.location < rhs.location;
-  });
-
+    const std::vector<ShaderStageIOSlot>& p_inputs,
+    const std::vector<ShaderStageBufferLayout>& layouts) {
   std::vector<VertexAttribPointer> vertex_attrib_arrays;
-  size_t offset = 0u;
-  for (const auto& input : inputs) {
+  for (auto i = 0u; i < p_inputs.size(); i++) {
+    const auto& input = p_inputs[i];
+    const auto& layout = layouts[input.binding];
     VertexAttribPointer attrib;
     attrib.index = input.location;
     // Component counts must be 1, 2, 3 or 4. Do that validation now.
@@ -47,13 +42,10 @@ bool BufferBindingsGLES::RegisterVertexStageInput(
     }
     attrib.type = type.value();
     attrib.normalized = GL_FALSE;
-    attrib.offset = offset;
-    offset += (input.bit_width * input.vec_size) / 8;
+    attrib.offset = input.offset;
+    attrib.stride = layout.stride;
     vertex_attrib_arrays.emplace_back(attrib);
   }
-  for (auto& array : vertex_attrib_arrays) {
-    array.stride = offset;
-  }
   vertex_attrib_arrays_ = std::move(vertex_attrib_arrays);
   return true;
 }

impeller/renderer/backend/gles/buffer_bindings_gles.h

@@ -25,8 +25,10 @@ class BufferBindingsGLES {
 
   ~BufferBindingsGLES();
 
-  bool RegisterVertexStageInput(const ProcTableGLES& gl,
-                                const std::vector<ShaderStageIOSlot>& inputs);
+  bool RegisterVertexStageInput(
+      const ProcTableGLES& gl,
+      const std::vector<ShaderStageIOSlot>& inputs,
+      const std::vector<ShaderStageBufferLayout>& layouts);
 
   bool ReadUniformsBindings(const ProcTableGLES& gl, GLuint program);
 

impeller/renderer/backend/gles/pipeline_gles.cc

@@ -46,7 +46,8 @@ bool PipelineGLES::BuildVertexDescriptor(const ProcTableGLES& gl,
   }
   auto vtx_desc = std::make_unique<BufferBindingsGLES>();
   if (!vtx_desc->RegisterVertexStageInput(
-          gl, GetDescriptor().GetVertexDescriptor()->GetStageInputs())) {
+          gl, GetDescriptor().GetVertexDescriptor()->GetStageInputs(),
+          GetDescriptor().GetVertexDescriptor()->GetStageLayouts())) {
     return false;
   }
   if (!vtx_desc->ReadUniformsBindings(gl, program)) {

impeller/renderer/backend/metal/pipeline_library_mtl.mm

@@ -41,8 +41,9 @@
 
   if (const auto& vertex_descriptor = desc.GetVertexDescriptor()) {
     VertexDescriptorMTL vertex_descriptor_mtl;
-    if (vertex_descriptor_mtl.SetStageInputs(
-            vertex_descriptor->GetStageInputs())) {
+    if (vertex_descriptor_mtl.SetStageInputsAndLayout(
+            vertex_descriptor->GetStageInputs(),
+            vertex_descriptor->GetStageLayouts())) {
       descriptor.vertexDescriptor =
           vertex_descriptor_mtl.GetMTLVertexDescriptor();
     }

impeller/renderer/backend/metal/vertex_descriptor_mtl.h

@@ -20,27 +20,14 @@ class VertexDescriptorMTL {
 
   ~VertexDescriptorMTL();
 
-  bool SetStageInputs(const std::vector<ShaderStageIOSlot>& inputs);
+  bool SetStageInputsAndLayout(
+      const std::vector<ShaderStageIOSlot>& inputs,
+      const std::vector<ShaderStageBufferLayout>& layouts);
 
   MTLVertexDescriptor* GetMTLVertexDescriptor() const;
 
  private:
-  struct StageInput {
-    size_t location;
-    MTLVertexFormat format;
-    size_t length;
-
-    StageInput(size_t p_location, MTLVertexFormat p_format, size_t p_length)
-        : location(p_location), format(p_format), length(p_length) {}
-
-    struct Compare {
-      constexpr bool operator()(const StageInput& lhs,
-                                const StageInput& rhs) const {
-        return lhs.location < rhs.location;
-      }
-    };
-  };
-  std::set<StageInput, StageInput::Compare> stage_inputs_;
+  MTLVertexDescriptor* descriptor_;
 
   FML_DISALLOW_COPY_AND_ASSIGN(VertexDescriptorMTL);
 };

impeller/renderer/backend/metal/vertex_descriptor_mtl.mm

@@ -169,49 +169,42 @@ static MTLVertexFormat ReadStageInputFormat(const ShaderStageIOSlot& input) {
   }
 }
 
-bool VertexDescriptorMTL::SetStageInputs(
-    const std::vector<ShaderStageIOSlot>& inputs) {
-  stage_inputs_.clear();
-
+bool VertexDescriptorMTL::SetStageInputsAndLayout(
+    const std::vector<ShaderStageIOSlot>& inputs,
+    const std::vector<ShaderStageBufferLayout>& layouts) {
+  auto descriptor = descriptor_ = [MTLVertexDescriptor vertexDescriptor];
+
+  // TODO: its odd that we offset buffers from the max index on metal
+  // but not on GLES or Vulkan. We should probably consistently start
+  // these at zero?
   for (size_t i = 0; i < inputs.size(); i++) {
     const auto& input = inputs[i];
     auto vertex_format = ReadStageInputFormat(input);
     if (vertex_format == MTLVertexFormatInvalid) {
       VALIDATION_LOG << "Format for input " << input.name << " not supported.";
       return false;
     }
-
-    stage_inputs_.insert(StageInput{input.location, vertex_format,
-                                    (input.bit_width * input.vec_size) / 8});
+    auto attrib = descriptor.attributes[input.location];
+    attrib.format = vertex_format;
+    attrib.offset = input.offset;
+    attrib.bufferIndex =
+        VertexDescriptor::kReservedVertexBufferIndex - input.binding;
   }
 
+  for (size_t i = 0; i < layouts.size(); i++) {
+    const auto& layout = layouts[i];
+    auto vertex_layout =
+        descriptor.layouts[VertexDescriptor::kReservedVertexBufferIndex -
+                           layout.binding];
+    vertex_layout.stride = layout.stride;
+    vertex_layout.stepRate = 1u;
+    vertex_layout.stepFunction = MTLVertexStepFunctionPerVertex;
+  }
   return true;
 }
 
 MTLVertexDescriptor* VertexDescriptorMTL::GetMTLVertexDescriptor() const {
-  auto descriptor = [MTLVertexDescriptor vertexDescriptor];
-
-  const size_t vertex_buffer_index =
-      VertexDescriptor::kReservedVertexBufferIndex;
-
-  size_t offset = 0u;
-  for (const auto& input : stage_inputs_) {
-    auto attrib = descriptor.attributes[input.location];
-    attrib.format = input.format;
-    attrib.offset = offset;
-    // All vertex inputs are interleaved and tightly packed in one buffer at a
-    // reserved index.
-    attrib.bufferIndex = vertex_buffer_index;
-    offset += input.length;
-  }
-
-  // Since it's all in one buffer, indicate its layout.
-  auto vertex_layout = descriptor.layouts[vertex_buffer_index];
-  vertex_layout.stride = offset;
-  vertex_layout.stepRate = 1u;
-  vertex_layout.stepFunction = MTLVertexStepFunctionPerVertex;
-
-  return descriptor;
+  return descriptor_;
 }
 
 }  // namespace impeller