From d5acf20125820a90c3abb31843eb3fcaafeaccaa Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Wed, 7 Jun 2023 10:56:20 -0700 Subject: [PATCH 1/7] [Impeller] make interleaved layout (more) explicit in generated headers --- impeller/compiler/code_gen_template.h | 17 +++++- impeller/compiler/reflector.cc | 39 ++++++++++++-- impeller/compiler/reflector.h | 7 ++- impeller/core/shader_types.h | 18 ++++++- .../contents/runtime_effect_contents.cc | 5 +- .../backend/gles/buffer_bindings_gles.cc | 22 +++----- .../backend/gles/buffer_bindings_gles.h | 6 ++- .../renderer/backend/gles/pipeline_gles.cc | 3 +- .../backend/metal/pipeline_library_mtl.mm | 5 +- .../backend/metal/vertex_descriptor_mtl.h | 21 ++------ .../backend/metal/vertex_descriptor_mtl.mm | 53 ++++++++----------- .../backend/vulkan/pipeline_library_vk.cc | 26 ++++----- impeller/renderer/pipeline_builder.h | 29 +++------- impeller/renderer/vertex_descriptor.cc | 24 ++++++--- impeller/renderer/vertex_descriptor.h | 23 +++++--- .../runtime_stage/runtime_stage_unittests.cc | 4 +- 16 files changed, 172 insertions(+), 130 deletions(-) diff --git a/impeller/compiler/code_gen_template.h b/impeller/compiler/code_gen_template.h index 3bd6be04d97ee..61f442d824dd4 100644 --- a/impeller/compiler/code_gen_template.h +++ b/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 kInterleavedBufferLayout = { + &kInterleavedLayout + }; +{% endif %} + {% if length(sampled_images) > 0 %} // =========================================================================== // Sampled Images ============================================================ diff --git a/impeller/compiler/reflector.cc b/impeller/compiler/reflector.cc index 50078efe1d306..d16cdceebcd73 100644 --- a/impeller/compiler/reflector.cc +++ b/impeller/compiler/reflector.cc @@ -226,8 +226,8 @@ std::optional 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=*/true); stage_inputs_json.has_value()) { stage_inputs = std::move(stage_inputs_json.value()); } else { @@ -402,8 +402,22 @@ std::shared_ptr Reflector::InflateTemplate( inflated_template->size(), [inflated_template](auto, auto) {}); } +std::vector Reflector::ComputeOffsets( + const spirv_cross::SmallVector& resources) const { + std::vector offsets(resources.size()); + size_t offset = 0u; + for (auto i = 0u; i < resources.size(); i++) { + offsets[i] = offset; + const auto resource = resources[i]; + const auto type = compiler_->get_type(resource.type_id); + offset += (type.width * type.vecsize) / 8; + } + return offsets; +} + std::optional Reflector::ReflectResource( - const spirv_cross::Resource& resource) const { + const spirv_cross::Resource& resource, + std::optional offset) const { nlohmann::json::object_t result; result["name"] = resource.name; @@ -426,6 +440,9 @@ std::optional Reflector::ReflectResource( return std::nullopt; } result["type"] = std::move(type.value()); + if (offset.has_value()) { + result["offset"] = offset.value(); + } return result; } @@ -462,12 +479,24 @@ std::optional Reflector::ReflectType( } std::optional Reflector::ReflectResources( - const spirv_cross::SmallVector& resources) const { + const spirv_cross::SmallVector& resources, + bool compute_offsets) const { nlohmann::json::array_t result; result.reserve(resources.size()); + std::vector offsets; + if (compute_offsets) { + offsets = ComputeOffsets(resources); + } + size_t index = 0u; for (const auto& resource : resources) { - if (auto reflected = ReflectResource(resource); reflected.has_value()) { + std::optional maybe_offset = std::nullopt; + if (compute_offsets) { + maybe_offset = offsets[index]; + } + if (auto reflected = ReflectResource(resource, maybe_offset); + reflected.has_value()) { result.emplace_back(std::move(reflected.value())); + index++; } else { return std::nullopt; } diff --git a/impeller/compiler/reflector.h b/impeller/compiler/reflector.h index 69b550c7b9697..61cbce9ca06f0 100644 --- a/impeller/compiler/reflector.h +++ b/impeller/compiler/reflector.h @@ -113,9 +113,14 @@ class Reflector { std::shared_ptr InflateTemplate(std::string_view tmpl) const; std::optional ReflectResource( - const spirv_cross::Resource& resource) const; + const spirv_cross::Resource& resource, + std::optional offset) const; std::optional ReflectResources( + const spirv_cross::SmallVector& resources, + bool compute_offsets = false) const; + + std::vector ComputeOffsets( const spirv_cross::SmallVector& resources) const; std::optional ReflectType( diff --git a/impeller/core/shader_types.h b/impeller/core/shader_types.h index 5cd35f12d0bcd..9ce7954998e0c 100644 --- a/impeller/core/shader_types.h +++ b/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; } }; diff --git a/impeller/entity/contents/runtime_effect_contents.cc b/impeller/entity/contents/runtime_effect_contents.cc index bd021af3a76d7..c3798abdb0ae8 100644 --- a/impeller/entity/contents/runtime_effect_contents.cc +++ b/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(); - 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}); diff --git a/impeller/renderer/backend/gles/buffer_bindings_gles.cc b/impeller/renderer/backend/gles/buffer_bindings_gles.cc index a21140c433cc5..c72c34f807aeb 100644 --- a/impeller/renderer/backend/gles/buffer_bindings_gles.cc +++ b/impeller/renderer/backend/gles/buffer_bindings_gles.cc @@ -23,17 +23,12 @@ BufferBindingsGLES::~BufferBindingsGLES() = default; bool BufferBindingsGLES::RegisterVertexStageInput( const ProcTableGLES& gl, - const std::vector& 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& p_inputs, + const std::vector& layouts) { std::vector 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; } diff --git a/impeller/renderer/backend/gles/buffer_bindings_gles.h b/impeller/renderer/backend/gles/buffer_bindings_gles.h index 5dc3536f3f685..b556364b9ea86 100644 --- a/impeller/renderer/backend/gles/buffer_bindings_gles.h +++ b/impeller/renderer/backend/gles/buffer_bindings_gles.h @@ -25,8 +25,10 @@ class BufferBindingsGLES { ~BufferBindingsGLES(); - bool RegisterVertexStageInput(const ProcTableGLES& gl, - const std::vector& inputs); + bool RegisterVertexStageInput( + const ProcTableGLES& gl, + const std::vector& inputs, + const std::vector& layouts); bool ReadUniformsBindings(const ProcTableGLES& gl, GLuint program); diff --git a/impeller/renderer/backend/gles/pipeline_gles.cc b/impeller/renderer/backend/gles/pipeline_gles.cc index 31063ce43d64f..79efe7454d5ae 100644 --- a/impeller/renderer/backend/gles/pipeline_gles.cc +++ b/impeller/renderer/backend/gles/pipeline_gles.cc @@ -46,7 +46,8 @@ bool PipelineGLES::BuildVertexDescriptor(const ProcTableGLES& gl, } auto vtx_desc = std::make_unique(); if (!vtx_desc->RegisterVertexStageInput( - gl, GetDescriptor().GetVertexDescriptor()->GetStageInputs())) { + gl, GetDescriptor().GetVertexDescriptor()->GetStageInputs(), + GetDescriptor().GetVertexDescriptor()->GetStageLayouts())) { return false; } if (!vtx_desc->ReadUniformsBindings(gl, program)) { diff --git a/impeller/renderer/backend/metal/pipeline_library_mtl.mm b/impeller/renderer/backend/metal/pipeline_library_mtl.mm index c65e31f1e7a43..a4b56568cf0df 100644 --- a/impeller/renderer/backend/metal/pipeline_library_mtl.mm +++ b/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(); } diff --git a/impeller/renderer/backend/metal/vertex_descriptor_mtl.h b/impeller/renderer/backend/metal/vertex_descriptor_mtl.h index 030d36fe8570f..22c4f71542c1c 100644 --- a/impeller/renderer/backend/metal/vertex_descriptor_mtl.h +++ b/impeller/renderer/backend/metal/vertex_descriptor_mtl.h @@ -20,27 +20,14 @@ class VertexDescriptorMTL { ~VertexDescriptorMTL(); - bool SetStageInputs(const std::vector& inputs); + bool SetStageInputsAndLayout( + const std::vector& inputs, + const std::vector& 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 stage_inputs_; + MTLVertexDescriptor* descriptor_; FML_DISALLOW_COPY_AND_ASSIGN(VertexDescriptorMTL); }; diff --git a/impeller/renderer/backend/metal/vertex_descriptor_mtl.mm b/impeller/renderer/backend/metal/vertex_descriptor_mtl.mm index a49a227324b26..b5948830f9abf 100644 --- a/impeller/renderer/backend/metal/vertex_descriptor_mtl.mm +++ b/impeller/renderer/backend/metal/vertex_descriptor_mtl.mm @@ -169,10 +169,14 @@ static MTLVertexFormat ReadStageInputFormat(const ShaderStageIOSlot& input) { } } -bool VertexDescriptorMTL::SetStageInputs( - const std::vector& inputs) { - stage_inputs_.clear(); - +bool VertexDescriptorMTL::SetStageInputsAndLayout( + const std::vector& inputs, + const std::vector& 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); @@ -180,38 +184,27 @@ static MTLVertexFormat ReadStageInputFormat(const ShaderStageIOSlot& input) { 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 diff --git a/impeller/renderer/backend/vulkan/pipeline_library_vk.cc b/impeller/renderer/backend/vulkan/pipeline_library_vk.cc index 201f88e32a3ab..2cf6a0fed89e4 100644 --- a/impeller/renderer/backend/vulkan/pipeline_library_vk.cc +++ b/impeller/renderer/backend/vulkan/pipeline_library_vk.cc @@ -243,31 +243,31 @@ std::unique_ptr PipelineLibraryVK::CreatePipeline( //---------------------------------------------------------------------------- /// Vertex Input Setup /// - vk::VertexInputBindingDescription binding_description; - // Only 1 stream of data is supported for now. - binding_description.setBinding(0); - binding_description.setInputRate(vk::VertexInputRate::eVertex); - std::vector attr_descs; - uint32_t offset = 0; + std::vector buffer_descs; + const auto& stage_inputs = desc.GetVertexDescriptor()->GetStageInputs(); + const auto& stage_buffer_layouts = + desc.GetVertexDescriptor()->GetStageLayouts(); for (const ShaderStageIOSlot& stage_in : stage_inputs) { vk::VertexInputAttributeDescription attr_desc; attr_desc.setBinding(stage_in.binding); attr_desc.setLocation(stage_in.location); attr_desc.setFormat(ToVertexDescriptorFormat(stage_in)); - attr_desc.setOffset(offset); + attr_desc.setOffset(stage_in.offset); attr_descs.push_back(attr_desc); - uint32_t len = (stage_in.bit_width * stage_in.vec_size) / 8; - offset += len; } - - binding_description.setStride(offset); + for (const ShaderStageBufferLayout& layout : stage_buffer_layouts) { + vk::VertexInputBindingDescription binding_description; + binding_description.setBinding(layout.binding); + binding_description.setInputRate(vk::VertexInputRate::eVertex); + binding_description.setStride(layout.stride); + buffer_descs.push_back(binding_description); + } vk::PipelineVertexInputStateCreateInfo vertex_input_state; vertex_input_state.setVertexAttributeDescriptions(attr_descs); - vertex_input_state.setVertexBindingDescriptionCount(1); - vertex_input_state.setPVertexBindingDescriptions(&binding_description); + vertex_input_state.setVertexBindingDescriptions(buffer_descs); pipeline_info.setPVertexInputState(&vertex_input_state); diff --git a/impeller/renderer/pipeline_builder.h b/impeller/renderer/pipeline_builder.h index ccc9e9b83a9f9..593df8057ff75 100644 --- a/impeller/renderer/pipeline_builder.h +++ b/impeller/renderer/pipeline_builder.h @@ -85,29 +85,12 @@ struct PipelineBuilder { // Setup the vertex descriptor from reflected information. { auto vertex_descriptor = std::make_shared(); - if (!vertex_descriptor->SetStageInputs( - VertexShader::kAllShaderStageInputs)) { - VALIDATION_LOG - << "Could not configure vertex descriptor for pipeline named '" - << VertexShader::kLabel << "'."; - return false; - } - if (!vertex_descriptor->RegisterDescriptorSetLayouts( - VertexShader::kDescriptorSetLayouts)) { - VALIDATION_LOG << "Cound not configure vertex descriptor set layout for" - " pipeline named '" - << VertexShader::kLabel << "'."; - return false; - } - - if (!vertex_descriptor->RegisterDescriptorSetLayouts( - FragmentShader::kDescriptorSetLayouts)) { - VALIDATION_LOG << "Cound not configure vertex descriptor set layout for" - " pipeline named '" - << VertexShader::kLabel << "'."; - return false; - } - + vertex_descriptor->SetStageInputs(VertexShader::kAllShaderStageInputs, + VertexShader::kInterleavedBufferLayout); + vertex_descriptor->RegisterDescriptorSetLayouts( + VertexShader::kDescriptorSetLayouts); + vertex_descriptor->RegisterDescriptorSetLayouts( + FragmentShader::kDescriptorSetLayouts); desc.SetVertexDescriptor(std::move(vertex_descriptor)); } diff --git a/impeller/renderer/vertex_descriptor.cc b/impeller/renderer/vertex_descriptor.cc index 97dff83913a66..a16ff1309e94a 100644 --- a/impeller/renderer/vertex_descriptor.cc +++ b/impeller/renderer/vertex_descriptor.cc @@ -10,24 +10,28 @@ VertexDescriptor::VertexDescriptor() = default; VertexDescriptor::~VertexDescriptor() = default; -bool VertexDescriptor::SetStageInputs( +void VertexDescriptor::SetStageInputs( const ShaderStageIOSlot* const stage_inputs[], - size_t count) { + size_t count, + const ShaderStageBufferLayout* const stage_layout[], + size_t layout_count) { inputs_.reserve(inputs_.size() + count); + layouts_.reserve(layouts_.size() + layout_count); for (size_t i = 0; i < count; i++) { inputs_.emplace_back(*stage_inputs[i]); } - return true; + for (size_t i = 0; i < layout_count; i++) { + layouts_.emplace_back(*stage_layout[i]); + } } -bool VertexDescriptor::RegisterDescriptorSetLayouts( +void VertexDescriptor::RegisterDescriptorSetLayouts( const DescriptorSetLayout desc_set_layout[], size_t count) { desc_set_layouts_.reserve(desc_set_layouts_.size() + count); for (size_t i = 0; i < count; i++) { desc_set_layouts_.emplace_back(desc_set_layout[i]); } - return true; } // |Comparable| @@ -36,18 +40,26 @@ size_t VertexDescriptor::GetHash() const { for (const auto& input : inputs_) { fml::HashCombineSeed(seed, input.GetHash()); } + for (const auto& layout : layouts_) { + fml::HashCombineSeed(seed, layout.GetHash()); + } return seed; } // |Comparable| bool VertexDescriptor::IsEqual(const VertexDescriptor& other) const { - return inputs_ == other.inputs_; + return inputs_ == other.inputs_ && layouts_ == other.layouts_; } const std::vector& VertexDescriptor::GetStageInputs() const { return inputs_; } +const std::vector& VertexDescriptor::GetStageLayouts() + const { + return layouts_; +} + const std::vector& VertexDescriptor::GetDescriptorSetLayouts() const { return desc_set_layouts_; diff --git a/impeller/renderer/vertex_descriptor.h b/impeller/renderer/vertex_descriptor.h index e62b4d2552e2f..43e2f00d193e3 100644 --- a/impeller/renderer/vertex_descriptor.h +++ b/impeller/renderer/vertex_descriptor.h @@ -30,26 +30,32 @@ class VertexDescriptor final : public Comparable { // |Comparable| virtual ~VertexDescriptor(); - template - bool SetStageInputs( - const std::array& inputs) { - return SetStageInputs(inputs.data(), inputs.size()); + template + void SetStageInputs( + const std::array& inputs, + const std::array& layout) { + return SetStageInputs(inputs.data(), inputs.size(), layout.data(), + layout.size()); } template - bool RegisterDescriptorSetLayouts( + void RegisterDescriptorSetLayouts( const std::array& inputs) { return RegisterDescriptorSetLayouts(inputs.data(), inputs.size()); } - bool SetStageInputs(const ShaderStageIOSlot* const stage_inputs[], - size_t count); + void SetStageInputs(const ShaderStageIOSlot* const stage_inputs[], + size_t count, + const ShaderStageBufferLayout* const stage_layout[], + size_t layout_count); - bool RegisterDescriptorSetLayouts(const DescriptorSetLayout desc_set_layout[], + void RegisterDescriptorSetLayouts(const DescriptorSetLayout desc_set_layout[], size_t count); const std::vector& GetStageInputs() const; + const std::vector& GetStageLayouts() const; + const std::vector& GetDescriptorSetLayouts() const; // |Comparable| @@ -60,6 +66,7 @@ class VertexDescriptor final : public Comparable { private: std::vector inputs_; + std::vector layouts_; std::vector desc_set_layouts_; FML_DISALLOW_COPY_AND_ASSIGN(VertexDescriptor); diff --git a/impeller/runtime_stage/runtime_stage_unittests.cc b/impeller/runtime_stage/runtime_stage_unittests.cc index fbe4d5f447eee..9fd6fe2f1c406 100644 --- a/impeller/runtime_stage/runtime_stage_unittests.cc +++ b/impeller/runtime_stage/runtime_stage_unittests.cc @@ -239,7 +239,9 @@ TEST_P(RuntimeStageTest, CanCreatePipelineFromRuntimeStage) { desc.AddStageEntrypoint( library->GetFunction(stage->GetEntrypoint(), ShaderStage::kFragment)); auto vertex_descriptor = std::make_shared(); - ASSERT_TRUE(vertex_descriptor->SetStageInputs(VS::kAllShaderStageInputs)); + vertex_descriptor->SetStageInputs(VS::kAllShaderStageInputs, + VS::kInterleavedBufferLayout); + desc.SetVertexDescriptor(std::move(vertex_descriptor)); ColorAttachmentDescriptor color0; color0.format = GetContext()->GetCapabilities()->GetDefaultColorFormat(); From bce747d8ec1c9f1d8b359f6d4feaf3abd3c1ca44 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Wed, 7 Jun 2023 14:46:13 -0700 Subject: [PATCH 2/7] ++ --- impeller/compiler/reflector.cc | 35 +++++++++++++++++++++------------- 1 file changed, 22 insertions(+), 13 deletions(-) diff --git a/impeller/compiler/reflector.cc b/impeller/compiler/reflector.cc index d16cdceebcd73..0258267922248 100644 --- a/impeller/compiler/reflector.cc +++ b/impeller/compiler/reflector.cc @@ -10,6 +10,7 @@ #include #include #include +#include #include "flutter/fml/closure.h" #include "flutter/fml/logging.h" @@ -226,8 +227,9 @@ std::optional Reflector::GenerateTemplateArguments() const { { auto& stage_inputs = root["stage_inputs"] = nlohmann::json::array_t{}; - if (auto stage_inputs_json = ReflectResources(shader_resources.stage_inputs, - /*compute_offsets=*/true); + 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 { @@ -405,13 +407,22 @@ std::shared_ptr Reflector::InflateTemplate( std::vector Reflector::ComputeOffsets( const spirv_cross::SmallVector& resources) const { std::vector offsets(resources.size()); - size_t offset = 0u; - for (auto i = 0u; i < resources.size(); i++) { - offsets[i] = offset; - const auto resource = resources[i]; + + for (const auto& resource : resources) { const auto type = compiler_->get_type(resource.type_id); - offset += (type.width * type.vecsize) / 8; + auto location = compiler_->get_decoration( + resource.id, spv::Decoration::DecorationLocation); + if (location < 0 || location >= resources.size()) { + std::cerr << "Bad size: " << location << std::endl; + } else { + offsets[location] = (type.width * type.vecsize) / 8; + } } + for (size_t i = 1; i < resources.size(); i++) { + offsets[i] = offsets[i - 1]; + } + offsets[0] = 0; + return offsets; } @@ -440,9 +451,7 @@ std::optional Reflector::ReflectResource( return std::nullopt; } result["type"] = std::move(type.value()); - if (offset.has_value()) { - result["offset"] = offset.value(); - } + result["offset"] = offset.value_or(0u); return result; } @@ -487,16 +496,16 @@ std::optional Reflector::ReflectResources( if (compute_offsets) { offsets = ComputeOffsets(resources); } - size_t index = 0u; for (const auto& resource : resources) { std::optional maybe_offset = std::nullopt; if (compute_offsets) { - maybe_offset = offsets[index]; + 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())); - index++; } else { return std::nullopt; } From 7cd97928505b067cdde83ec5153283a9238b5f6d Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Tue, 13 Jun 2023 10:32:26 -0700 Subject: [PATCH 3/7] check for size --- impeller/compiler/reflector.cc | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/impeller/compiler/reflector.cc b/impeller/compiler/reflector.cc index 0258267922248..7013c8f511129 100644 --- a/impeller/compiler/reflector.cc +++ b/impeller/compiler/reflector.cc @@ -10,7 +10,6 @@ #include #include #include -#include #include "flutter/fml/closure.h" #include "flutter/fml/logging.h" @@ -407,21 +406,18 @@ std::shared_ptr Reflector::InflateTemplate( std::vector Reflector::ComputeOffsets( const spirv_cross::SmallVector& resources) const { std::vector offsets(resources.size()); - for (const auto& resource : resources) { const auto type = compiler_->get_type(resource.type_id); auto location = compiler_->get_decoration( resource.id, spv::Decoration::DecorationLocation); - if (location < 0 || location >= resources.size()) { - std::cerr << "Bad size: " << location << std::endl; - } else { - offsets[location] = (type.width * type.vecsize) / 8; - } + offsets[location] = (type.width * type.vecsize) / 8; } for (size_t i = 1; i < resources.size(); i++) { offsets[i] = offsets[i - 1]; } - offsets[0] = 0; + if (resources.size() > 0) { + offsets[0] = 0; + } return offsets; } From f5b66276b5df116f5d9d5d3129f2b1d8092a36fd Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Tue, 13 Jun 2023 10:56:25 -0700 Subject: [PATCH 4/7] ++ --- impeller/compiler/reflector.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/impeller/compiler/reflector.cc b/impeller/compiler/reflector.cc index 7013c8f511129..56dfcd3488611 100644 --- a/impeller/compiler/reflector.cc +++ b/impeller/compiler/reflector.cc @@ -413,6 +413,9 @@ std::vector Reflector::ComputeOffsets( 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]; } if (resources.size() > 0) { From 0e251f76018d18b5e6230e2e2723a00051e2497f Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Tue, 13 Jun 2023 11:03:09 -0700 Subject: [PATCH 5/7] ++ --- impeller/compiler/reflector.cc | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/impeller/compiler/reflector.cc b/impeller/compiler/reflector.cc index 56dfcd3488611..7cd8d70885e23 100644 --- a/impeller/compiler/reflector.cc +++ b/impeller/compiler/reflector.cc @@ -406,6 +406,9 @@ std::shared_ptr Reflector::InflateTemplate( std::vector Reflector::ComputeOffsets( const spirv_cross::SmallVector& resources) const { std::vector offsets(resources.size()); + 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( @@ -418,9 +421,7 @@ std::vector Reflector::ComputeOffsets( for (size_t i = resources.size() - 1; i > 0; i--) { offsets[i] = offsets[i - 1]; } - if (resources.size() > 0) { - offsets[0] = 0; - } + offsets[0] = 0; return offsets; } From 9cc0bc5f2ab8ada3346fe6539f2dfe24f4898acf Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Tue, 13 Jun 2023 13:22:36 -0700 Subject: [PATCH 6/7] dont assume locations are valid --- impeller/compiler/reflector.cc | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/impeller/compiler/reflector.cc b/impeller/compiler/reflector.cc index 7cd8d70885e23..a1bcb2203f9d3 100644 --- a/impeller/compiler/reflector.cc +++ b/impeller/compiler/reflector.cc @@ -405,7 +405,7 @@ std::shared_ptr Reflector::InflateTemplate( std::vector Reflector::ComputeOffsets( const spirv_cross::SmallVector& resources) const { - std::vector offsets(resources.size()); + std::vector offsets(resources.size(), 0); if (resources.size() == 0) { return offsets; } @@ -413,15 +413,19 @@ std::vector Reflector::ComputeOffsets( 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; + 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; } From 154b63b4b608a1d4ea4bb3f68ac8692f22930954 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Tue, 13 Jun 2023 13:25:12 -0700 Subject: [PATCH 7/7] + --- impeller/compiler/reflector.cc | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/impeller/compiler/reflector.cc b/impeller/compiler/reflector.cc index a1bcb2203f9d3..5787048bc26b7 100644 --- a/impeller/compiler/reflector.cc +++ b/impeller/compiler/reflector.cc @@ -419,13 +419,13 @@ std::vector Reflector::ComputeOffsets( } 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; + 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; }