From c5af7bd1888d8d2e704726090131d66c106ff7ce Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Tue, 16 Apr 2024 14:33:24 -0700 Subject: [PATCH 1/4] [Impeller] added static assert for matching fragment shaders and vertex shaders --- impeller/renderer/pipeline.h | 65 ++++++++++++++++++++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/impeller/renderer/pipeline.h b/impeller/renderer/pipeline.h index eda23adebc5ff..e7d757765d929 100644 --- a/impeller/renderer/pipeline.h +++ b/impeller/renderer/pipeline.h @@ -87,8 +87,73 @@ PipelineFuture CreatePipelineFuture( const Context& context, std::optional desc); +/// This is a classed use to check that the input slots of fragment shaders +/// match the output slots of the vertex shaders. +/// It's not used at runtime. +template +class ShaderBindingComparator { + public: + static constexpr bool CompileTimeStrEqual(const char* str1, + const char* str2) { + return *str1 == *str2 && + (*str1 == '\0' || CompileTimeStrEqual(str1 + 1, str2 + 1)); + } + + static constexpr bool Compare() { + constexpr size_t num_outputs = VertexShaderT::kAllShaderStageOutputs.size(); + constexpr size_t num_inputs = FragmentShaderT::kAllShaderStageInputs.size(); + + if (num_inputs > num_outputs) { + return false; + } + + for (size_t i = 0; i < num_inputs; ++i) { + const ShaderStageIOSlot* input_slot = + FragmentShaderT::kAllShaderStageInputs[i]; + for (size_t j = 0; j < num_outputs; ++j) { + const ShaderStageIOSlot* output_slot = + VertexShaderT::kAllShaderStageOutputs[j]; + if (input_slot->location == output_slot->location) { + if (!CompileTimeStrEqual(input_slot->name, output_slot->name) || + input_slot->set != output_slot->set || + input_slot->binding != output_slot->binding || + input_slot->type != output_slot->type || + input_slot->bit_width != output_slot->bit_width || + input_slot->vec_size != output_slot->vec_size || + input_slot->columns != output_slot->columns || + input_slot->offset != output_slot->offset) { + return false; + } + } + } + } + + return true; + } +}; + +// The following shaders don't define output slots. +// TODO(tbd): Make impellerc emit an empty array for output slots. +struct CheckerboardVertexShader; +struct ClipVertexShader; + +template +class ShaderBindingComparator { + public: + static constexpr bool Compare() { return true; } +}; + +template +class ShaderBindingComparator { + public: + static constexpr bool Compare() { return true; } +}; + template class RenderPipelineT { + static_assert( + ShaderBindingComparator::Compare()); + public: using VertexShader = VertexShader_; using FragmentShader = FragmentShader_; From 5c4cb90718f8d85f354f84d23f1545347412dd9e Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Tue, 16 Apr 2024 15:26:10 -0700 Subject: [PATCH 2/4] chinmay feedback --- impeller/renderer/pipeline.h | 67 +---------------- .../shader_stage_compatibility_checker.h | 75 +++++++++++++++++++ 2 files changed, 79 insertions(+), 63 deletions(-) create mode 100644 impeller/renderer/shader_stage_compatibility_checker.h diff --git a/impeller/renderer/pipeline.h b/impeller/renderer/pipeline.h index e7d757765d929..292f91a30a346 100644 --- a/impeller/renderer/pipeline.h +++ b/impeller/renderer/pipeline.h @@ -13,6 +13,7 @@ #include "impeller/renderer/context.h" #include "impeller/renderer/pipeline_builder.h" #include "impeller/renderer/pipeline_descriptor.h" +#include "impeller/renderer/shader_stage_compatibility_checker.h" namespace impeller { @@ -87,72 +88,12 @@ PipelineFuture CreatePipelineFuture( const Context& context, std::optional desc); -/// This is a classed use to check that the input slots of fragment shaders -/// match the output slots of the vertex shaders. -/// It's not used at runtime. -template -class ShaderBindingComparator { - public: - static constexpr bool CompileTimeStrEqual(const char* str1, - const char* str2) { - return *str1 == *str2 && - (*str1 == '\0' || CompileTimeStrEqual(str1 + 1, str2 + 1)); - } - - static constexpr bool Compare() { - constexpr size_t num_outputs = VertexShaderT::kAllShaderStageOutputs.size(); - constexpr size_t num_inputs = FragmentShaderT::kAllShaderStageInputs.size(); - - if (num_inputs > num_outputs) { - return false; - } - - for (size_t i = 0; i < num_inputs; ++i) { - const ShaderStageIOSlot* input_slot = - FragmentShaderT::kAllShaderStageInputs[i]; - for (size_t j = 0; j < num_outputs; ++j) { - const ShaderStageIOSlot* output_slot = - VertexShaderT::kAllShaderStageOutputs[j]; - if (input_slot->location == output_slot->location) { - if (!CompileTimeStrEqual(input_slot->name, output_slot->name) || - input_slot->set != output_slot->set || - input_slot->binding != output_slot->binding || - input_slot->type != output_slot->type || - input_slot->bit_width != output_slot->bit_width || - input_slot->vec_size != output_slot->vec_size || - input_slot->columns != output_slot->columns || - input_slot->offset != output_slot->offset) { - return false; - } - } - } - } - - return true; - } -}; - -// The following shaders don't define output slots. -// TODO(tbd): Make impellerc emit an empty array for output slots. -struct CheckerboardVertexShader; -struct ClipVertexShader; - -template -class ShaderBindingComparator { - public: - static constexpr bool Compare() { return true; } -}; - -template -class ShaderBindingComparator { - public: - static constexpr bool Compare() { return true; } -}; - template class RenderPipelineT { static_assert( - ShaderBindingComparator::Compare()); + ShaderStageCompatibilityChecker::Check(), + "The output slots for the fragment shader don't have matches in the " + "vertex shader's output slots."); public: using VertexShader = VertexShader_; diff --git a/impeller/renderer/shader_stage_compatibility_checker.h b/impeller/renderer/shader_stage_compatibility_checker.h new file mode 100644 index 0000000000000..b972fec2b7fef --- /dev/null +++ b/impeller/renderer/shader_stage_compatibility_checker.h @@ -0,0 +1,75 @@ +// 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. + +#ifndef FLUTTER_IMPELLER_RENDERER_SHADER_STAGE_COMPATIBILITY_CHECKER_H_ +#define FLUTTER_IMPELLER_RENDERER_SHADER_STAGE_COMPATIBILITY_CHECKER_H_ + +namespace impeller { +/// This is a classed use to check that the input slots of fragment shaders +/// match the output slots of the vertex shaders. +/// It's not used at runtime. +template +class ShaderStageCompatibilityChecker { + public: + static constexpr bool CompileTimeStrEqual(const char* str1, + const char* str2) { + return *str1 == *str2 && + (*str1 == '\0' || CompileTimeStrEqual(str1 + 1, str2 + 1)); + } + + /// Returns `true` if the shader input slots for the fragment shader match the + /// ones declared as outputs in the vertex shader. + static constexpr bool Check() { + constexpr size_t num_outputs = VertexShaderT::kAllShaderStageOutputs.size(); + constexpr size_t num_inputs = FragmentShaderT::kAllShaderStageInputs.size(); + + if (num_inputs > num_outputs) { + return false; + } + + for (size_t i = 0; i < num_inputs; ++i) { + const ShaderStageIOSlot* input_slot = + FragmentShaderT::kAllShaderStageInputs[i]; + for (size_t j = 0; j < num_outputs; ++j) { + const ShaderStageIOSlot* output_slot = + VertexShaderT::kAllShaderStageOutputs[j]; + if (input_slot->location == output_slot->location) { + if (!CompileTimeStrEqual(input_slot->name, output_slot->name) || + input_slot->set != output_slot->set || + input_slot->binding != output_slot->binding || + input_slot->type != output_slot->type || + input_slot->bit_width != output_slot->bit_width || + input_slot->vec_size != output_slot->vec_size || + input_slot->columns != output_slot->columns || + input_slot->offset != output_slot->offset) { + return false; + } + } + } + } + + return true; + } +}; + +// The following shaders don't define output slots. +// TODO(https://github.com/flutter/flutter/issues/146852): Make impellerc emit +// an empty array for output slots. +struct CheckerboardVertexShader; +struct ClipVertexShader; + +template +class ShaderStageCompatibilityChecker { + public: + static constexpr bool Check() { return true; } +}; + +template +class ShaderStageCompatibilityChecker { + public: + static constexpr bool Check() { return true; } +}; +} // namespace impeller +#endif // FLUTTER_IMPELLER_RENDERER_SHADER_STAGE_COMPATIBILITY_CHECKER_H_ From 95bc2ce817578af6e7900f9bb2addd06d4f0afaf Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Tue, 16 Apr 2024 15:40:05 -0700 Subject: [PATCH 3/4] updated comments --- impeller/renderer/pipeline.h | 2 +- impeller/renderer/shader_stage_compatibility_checker.h | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/impeller/renderer/pipeline.h b/impeller/renderer/pipeline.h index 292f91a30a346..7bed0902d2b48 100644 --- a/impeller/renderer/pipeline.h +++ b/impeller/renderer/pipeline.h @@ -93,7 +93,7 @@ class RenderPipelineT { static_assert( ShaderStageCompatibilityChecker::Check(), "The output slots for the fragment shader don't have matches in the " - "vertex shader's output slots."); + "vertex shader's output slots. This will result in a linker error."); public: using VertexShader = VertexShader_; diff --git a/impeller/renderer/shader_stage_compatibility_checker.h b/impeller/renderer/shader_stage_compatibility_checker.h index b972fec2b7fef..5a16e64b6db03 100644 --- a/impeller/renderer/shader_stage_compatibility_checker.h +++ b/impeller/renderer/shader_stage_compatibility_checker.h @@ -8,7 +8,8 @@ namespace impeller { /// This is a classed use to check that the input slots of fragment shaders /// match the output slots of the vertex shaders. -/// It's not used at runtime. +/// If they don't match it will result in linker errors when creating the +/// pipeline. It's not used at runtime. template class ShaderStageCompatibilityChecker { public: From e59d318b71155dbda06fd7fb12a068b901dd6986 Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Tue, 16 Apr 2024 16:00:17 -0700 Subject: [PATCH 4/4] license --- ci/licenses_golden/licenses_flutter | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ci/licenses_golden/licenses_flutter b/ci/licenses_golden/licenses_flutter index d1eb6aefdf937..75de228f6a9be 100644 --- a/ci/licenses_golden/licenses_flutter +++ b/ci/licenses_golden/licenses_flutter @@ -40715,6 +40715,7 @@ ORIGIN: ../../../flutter/impeller/renderer/shader_key.cc + ../../../flutter/LICE ORIGIN: ../../../flutter/impeller/renderer/shader_key.h + ../../../flutter/LICENSE ORIGIN: ../../../flutter/impeller/renderer/shader_library.cc + ../../../flutter/LICENSE ORIGIN: ../../../flutter/impeller/renderer/shader_library.h + ../../../flutter/LICENSE +ORIGIN: ../../../flutter/impeller/renderer/shader_stage_compatibility_checker.h + ../../../flutter/LICENSE ORIGIN: ../../../flutter/impeller/renderer/snapshot.cc + ../../../flutter/LICENSE ORIGIN: ../../../flutter/impeller/renderer/snapshot.h + ../../../flutter/LICENSE ORIGIN: ../../../flutter/impeller/renderer/stroke.comp + ../../../flutter/LICENSE @@ -43596,6 +43597,7 @@ FILE: ../../../flutter/impeller/renderer/shader_key.cc FILE: ../../../flutter/impeller/renderer/shader_key.h FILE: ../../../flutter/impeller/renderer/shader_library.cc FILE: ../../../flutter/impeller/renderer/shader_library.h +FILE: ../../../flutter/impeller/renderer/shader_stage_compatibility_checker.h FILE: ../../../flutter/impeller/renderer/snapshot.cc FILE: ../../../flutter/impeller/renderer/snapshot.h FILE: ../../../flutter/impeller/renderer/stroke.comp