From 290a85e7f247fe57882dbcec0ae9720ef8c17d6f Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Mon, 31 Mar 2025 10:21:04 -0700 Subject: [PATCH] [Impeller] handle shader ordering bug on macOS. (#165937) Fixes https://github.com/flutter/flutter/issues/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> --- .../contents/runtime_effect_contents.cc | 72 ++++++++----------- .../fixtures/shaders/general_shaders/BUILD.gn | 1 + .../general_shaders/uniform_ordering.frag | 20 ++++++ .../testing/dart/fragment_shader_test.dart | 46 +++++------- 4 files changed, 68 insertions(+), 71 deletions(-) create mode 100644 engine/src/flutter/lib/ui/fixtures/shaders/general_shaders/uniform_ordering.frag diff --git a/engine/src/flutter/impeller/entity/contents/runtime_effect_contents.cc b/engine/src/flutter/impeller/entity/contents/runtime_effect_contents.cc index a2e5eb45c6..920efba84e 100644 --- a/engine/src/flutter/impeller/entity/contents/runtime_effect_contents.cc +++ b/engine/src/flutter/impeller/entity/contents/runtime_effect_contents.cc @@ -168,8 +168,9 @@ RuntimeEffectContents::CreatePipeline(const ContentContext& renderer, const std::shared_ptr& context = renderer.GetContext(); const std::shared_ptr& library = context->GetShaderLibrary(); const std::shared_ptr& 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 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 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 metadata = MakeShaderMetadata(uniform); - - switch (uniform.type) { - case kSampledImage: { - FML_DCHECK(sampler_index < texture_inputs_.size()); - auto& input = texture_inputs_[sampler_index]; - - raw_ptr 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; }; diff --git a/engine/src/flutter/lib/ui/fixtures/shaders/general_shaders/BUILD.gn b/engine/src/flutter/lib/ui/fixtures/shaders/general_shaders/BUILD.gn index f6f81f46a6..2f127e5872 100644 --- a/engine/src/flutter/lib/ui/fixtures/shaders/general_shaders/BUILD.gn +++ b/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", diff --git a/engine/src/flutter/lib/ui/fixtures/shaders/general_shaders/uniform_ordering.frag b/engine/src/flutter/lib/ui/fixtures/shaders/general_shaders/uniform_ordering.frag new file mode 100644 index 0000000000..e8dede02f3 --- /dev/null +++ b/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); + } +} \ No newline at end of file diff --git a/engine/src/flutter/testing/dart/fragment_shader_test.dart b/engine/src/flutter/testing/dart/fragment_shader_test.dart index eb518d1cb7..87a1689973 100644 --- a/engine/src/flutter/testing/dart/fragment_shader_test.dart +++ b/engine/src/flutter/testing/dart/fragment_shader_test.dart @@ -195,10 +195,6 @@ 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); @@ -206,10 +202,6 @@ void main() async { }); 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 {