From c2d552f6988aa2606386b3e42c9c1fde3a73adc2 Mon Sep 17 00:00:00 2001 From: Benson Luk <97480502+b-luk@users.noreply.github.com> Date: Tue, 17 Feb 2026 16:34:34 -0800 Subject: [PATCH 1/4] Don't compile shaders to SkSL unless --sksl arg is present --- .../src/flutter/impeller/compiler/compiler.cc | 4 +- .../flutter/impeller/compiler/compiler_test.h | 4 +- .../impeller/compiler/compiler_unittests.cc | 116 +++++++++++------- .../impeller/compiler/impellerc_main.cc | 59 +-------- .../impeller/compiler/shader_bundle.cc | 6 +- .../src/flutter/impeller/compiler/switches.cc | 26 ++-- .../src/flutter/impeller/compiler/switches.h | 6 +- .../impeller/compiler/switches_unittests.cc | 3 +- engine/src/flutter/impeller/compiler/types.cc | 19 --- engine/src/flutter/impeller/compiler/types.h | 2 - engine/src/flutter/impeller/fixtures/BUILD.gn | 23 +++- .../runtime_stage_simple_no_sksl.frag | 9 ++ .../runtime_stage/runtime_stage_unittests.cc | 14 ++- 13 files changed, 139 insertions(+), 152 deletions(-) create mode 100644 engine/src/flutter/impeller/fixtures/runtime_stage_simple_no_sksl.frag diff --git a/engine/src/flutter/impeller/compiler/compiler.cc b/engine/src/flutter/impeller/compiler/compiler.cc index ee6e84afafe..89d9ab20dfb 100644 --- a/engine/src/flutter/impeller/compiler/compiler.cc +++ b/engine/src/flutter/impeller/compiler/compiler.cc @@ -242,7 +242,6 @@ static CompilerBackend CreateCompiler(const spirv_cross::ParsedIR& ir, case TargetPlatform::kRuntimeStageVulkan: compiler = CreateVulkanCompiler(ir, source_options); break; - case TargetPlatform::kUnknown: case TargetPlatform::kOpenGLES: case TargetPlatform::kOpenGLDesktop: case TargetPlatform::kRuntimeStageGLES: @@ -251,6 +250,9 @@ static CompilerBackend CreateCompiler(const spirv_cross::ParsedIR& ir, break; case TargetPlatform::kSkSL: compiler = CreateSkSLCompiler(ir, source_options); + break; + case TargetPlatform::kUnknown: + FML_UNREACHABLE(); } if (!compiler) { return {}; diff --git a/engine/src/flutter/impeller/compiler/compiler_test.h b/engine/src/flutter/impeller/compiler/compiler_test.h index 6e3f507f950..a8da01cc55d 100644 --- a/engine/src/flutter/impeller/compiler/compiler_test.h +++ b/engine/src/flutter/impeller/compiler/compiler_test.h @@ -45,9 +45,11 @@ class CompilerTestBase : public ::testing::TestWithParam { class CompilerTest : public CompilerTestBase {}; +class CompilerTestRuntime : public CompilerTestBase {}; + class CompilerTestSkSL : public CompilerTestBase {}; -class CompilerTestRuntime : public CompilerTestBase {}; +class CompilerTestUnknownPlatform : public CompilerTestBase {}; } // namespace testing } // namespace compiler diff --git a/engine/src/flutter/impeller/compiler/compiler_unittests.cc b/engine/src/flutter/impeller/compiler/compiler_unittests.cc index 71750ba41f2..67da3ab3bf1 100644 --- a/engine/src/flutter/impeller/compiler/compiler_unittests.cc +++ b/engine/src/flutter/impeller/compiler/compiler_unittests.cc @@ -15,6 +15,52 @@ namespace impeller { namespace compiler { namespace testing { +#define INSTANTIATE_TARGET_PLATFORM_TEST_SUITE_P(suite_name) \ + INSTANTIATE_TEST_SUITE_P( \ + suite_name, CompilerTest, \ + ::testing::Values(TargetPlatform::kOpenGLES, \ + TargetPlatform::kOpenGLDesktop, \ + TargetPlatform::kMetalDesktop, \ + TargetPlatform::kMetalIOS, TargetPlatform::kVulkan), \ + [](const ::testing::TestParamInfo& info) { \ + return TargetPlatformToString(info.param); \ + }); + +INSTANTIATE_TARGET_PLATFORM_TEST_SUITE_P(CompilerSuite); + +#define INSTANTIATE_RUNTIME_TARGET_PLATFORM_TEST_SUITE_P(suite_name) \ + INSTANTIATE_TEST_SUITE_P( \ + suite_name, CompilerTestRuntime, \ + ::testing::Values(TargetPlatform::kRuntimeStageMetal, \ + TargetPlatform::kRuntimeStageGLES, \ + TargetPlatform::kRuntimeStageGLES3, \ + TargetPlatform::kRuntimeStageVulkan, \ + TargetPlatform::kSkSL), \ + [](const ::testing::TestParamInfo& info) { \ + return TargetPlatformToString(info.param); \ + }); + +INSTANTIATE_RUNTIME_TARGET_PLATFORM_TEST_SUITE_P(CompilerSuite); + +#define INSTANTIATE_SKSL_TARGET_PLATFORM_TEST_SUITE_P(suite_name) \ + INSTANTIATE_TEST_SUITE_P( \ + suite_name, CompilerTestSkSL, ::testing::Values(TargetPlatform::kSkSL), \ + [](const ::testing::TestParamInfo& info) { \ + return TargetPlatformToString(info.param); \ + }); + +INSTANTIATE_SKSL_TARGET_PLATFORM_TEST_SUITE_P(CompilerSuite); + +#define INSTANTIATE_UNKNOWN_TARGET_PLATFORM_TEST_SUITE_P(suite_name) \ + INSTANTIATE_TEST_SUITE_P( \ + suite_name, CompilerTestUnknownPlatform, \ + ::testing::Values(TargetPlatform::kUnknown), \ + [](const ::testing::TestParamInfo& info) { \ + return TargetPlatformToString(info.param); \ + }); + +INSTANTIATE_UNKNOWN_TARGET_PLATFORM_TEST_SUITE_P(CompilerSuite); + TEST(CompilerTest, Defines) { std::shared_ptr fixture = flutter::testing::OpenFixtureAsMapping("check_gles_definition.frag"); @@ -48,9 +94,6 @@ TEST(CompilerTest, ShaderKindMatchingIsSuccessful) { } TEST_P(CompilerTest, CanCompile) { - if (GetParam() == TargetPlatform::kSkSL) { - GTEST_SKIP() << "Not supported with SkSL"; - } ASSERT_TRUE(CanCompileAndReflect("sample.vert")); ASSERT_TRUE(CanCompileAndReflect("sample.vert", SourceType::kVertexShader)); ASSERT_TRUE(CanCompileAndReflect("sample.vert", SourceType::kVertexShader, @@ -58,17 +101,11 @@ TEST_P(CompilerTest, CanCompile) { } TEST_P(CompilerTest, CanCompileHLSL) { - if (GetParam() == TargetPlatform::kSkSL) { - GTEST_SKIP() << "Not supported with SkSL"; - } ASSERT_TRUE(CanCompileAndReflect( "simple.vert.hlsl", SourceType::kVertexShader, SourceLanguage::kHLSL)); } TEST_P(CompilerTest, CanCompileHLSLWithMultipleStages) { - if (GetParam() == TargetPlatform::kSkSL) { - GTEST_SKIP() << "Not supported with SkSL"; - } ASSERT_TRUE(CanCompileAndReflect("multiple_stages.hlsl", SourceType::kVertexShader, SourceLanguage::kHLSL, "VertexShader")); @@ -87,18 +124,12 @@ TEST_P(CompilerTest, CanCompileComputeShader) { } TEST_P(CompilerTest, MustFailDueToExceedingResourcesLimit) { - if (GetParam() == TargetPlatform::kSkSL) { - GTEST_SKIP() << "Not supported with SkSL"; - } ScopedValidationDisable disable_validation; ASSERT_FALSE( CanCompileAndReflect("resources_limit.vert", SourceType::kVertexShader)); } TEST_P(CompilerTest, MustFailDueToMultipleLocationPerStructMember) { - if (GetParam() == TargetPlatform::kSkSL) { - GTEST_SKIP() << "Not supported with SkSL"; - } ScopedValidationDisable disable_validation; ASSERT_FALSE(CanCompileAndReflect("struct_def_bug.vert")); } @@ -202,6 +233,13 @@ inline std::ostream& operator<<(std::ostream& out, const UniformInfo& info) { } // namespace TEST_P(CompilerTestRuntime, UniformsAppearInJson) { + if (GetParam() == TargetPlatform::kSkSL) { + GTEST_SKIP() << "Not supported with SkSL"; + } + if (GetParam() == TargetPlatform::kRuntimeStageVulkan) { + GTEST_SKIP() << "Not supported with Vulkan"; + } + ASSERT_TRUE(CanCompileAndReflect("sample_with_uniforms.frag", SourceType::kFragmentShader, SourceLanguage::kGLSL)); @@ -248,6 +286,13 @@ TEST_P(CompilerTestRuntime, UniformsAppearInJson) { } TEST_P(CompilerTestRuntime, PositionedUniformsAppearInJson) { + if (GetParam() == TargetPlatform::kSkSL) { + GTEST_SKIP() << "Not supported with SkSL"; + } + if (GetParam() == TargetPlatform::kRuntimeStageVulkan) { + GTEST_SKIP() << "Not supported with Vulkan"; + } + ASSERT_TRUE(CanCompileAndReflect("sample_with_positioned_uniforms.frag", SourceType::kFragmentShader, SourceLanguage::kGLSL)); @@ -377,39 +422,11 @@ TEST_P(CompilerTestSkSL, CompilesWithValidArrayInitialization) { SourceType::kFragmentShader)); } -#define INSTANTIATE_TARGET_PLATFORM_TEST_SUITE_P(suite_name) \ - INSTANTIATE_TEST_SUITE_P( \ - suite_name, CompilerTest, \ - ::testing::Values(TargetPlatform::kOpenGLES, \ - TargetPlatform::kOpenGLDesktop, \ - TargetPlatform::kMetalDesktop, \ - TargetPlatform::kMetalIOS, TargetPlatform::kSkSL), \ - [](const ::testing::TestParamInfo& info) { \ - return TargetPlatformToString(info.param); \ - }); - -INSTANTIATE_TARGET_PLATFORM_TEST_SUITE_P(CompilerSuite); - -#define INSTANTIATE_RUNTIME_TARGET_PLATFORM_TEST_SUITE_P(suite_name) \ - INSTANTIATE_TEST_SUITE_P( \ - suite_name, CompilerTestRuntime, \ - ::testing::Values(TargetPlatform::kRuntimeStageMetal), \ - [](const ::testing::TestParamInfo& info) { \ - return TargetPlatformToString(info.param); \ - }); - -INSTANTIATE_RUNTIME_TARGET_PLATFORM_TEST_SUITE_P(CompilerSuite); - -#define INSTANTIATE_SKSL_TARGET_PLATFORM_TEST_SUITE_P(suite_name) \ - INSTANTIATE_TEST_SUITE_P( \ - suite_name, CompilerTestSkSL, ::testing::Values(TargetPlatform::kSkSL), \ - [](const ::testing::TestParamInfo& info) { \ - return TargetPlatformToString(info.param); \ - }); - -INSTANTIATE_SKSL_TARGET_PLATFORM_TEST_SUITE_P(CompilerSuite); - TEST_P(CompilerTestRuntime, Mat2Reflection) { + if (GetParam() == TargetPlatform::kSkSL) { + GTEST_SKIP() << "Not supported with SkSL"; + } + ASSERT_TRUE(CanCompileAndReflect( "mat2_test.frag", SourceType::kFragmentShader, SourceLanguage::kGLSL)); @@ -448,6 +465,11 @@ TEST_P(CompilerTestRuntime, Mat2Reflection) { EXPECT_EQ(mat2Member["offset"], 0u); } +TEST_P(CompilerTestUnknownPlatform, MustFailDueToUnknownPlatform) { + ASSERT_FALSE( + CanCompileAndReflect("sample.frag", SourceType::kFragmentShader)); +} + } // namespace testing } // namespace compiler } // namespace impeller diff --git a/engine/src/flutter/impeller/compiler/impellerc_main.cc b/engine/src/flutter/impeller/compiler/impellerc_main.cc index 1e838c65cdb..37fdf29c030 100644 --- a/engine/src/flutter/impeller/compiler/impellerc_main.cc +++ b/engine/src/flutter/impeller/compiler/impellerc_main.cc @@ -32,47 +32,13 @@ static Reflector::Options CreateReflectorOptions(const SourceOptions& options, return reflector_options; } -/// Run the shader compiler to geneate SkSL reflection data. -/// If there is an error, prints error text and returns `nullptr`. -static std::shared_ptr CompileSkSL( - std::shared_ptr source_file_mapping, - const Switches& switches) { - auto options = switches.CreateSourceOptions(TargetPlatform::kSkSL); - - Reflector::Options sksl_reflector_options = - CreateReflectorOptions(options, switches); - sksl_reflector_options.target_platform = TargetPlatform::kSkSL; - - Compiler sksl_compiler = - Compiler(std::move(source_file_mapping), options, sksl_reflector_options); - if (!sksl_compiler.IsValid()) { - std::cerr << "Compilation to SkSL failed." << std::endl; - std::cerr << sksl_compiler.GetErrorMessages() << std::endl; - return nullptr; - } - return sksl_compiler.GetReflector()->GetRuntimeStageShaderData(); -} - static bool OutputIPLR( const Switches& switches, const std::shared_ptr& source_file_mapping) { FML_DCHECK(switches.iplr); RuntimeStageData stages; - std::shared_ptr sksl_shader; - if (TargetPlatformBundlesSkSL(switches.SelectDefaultTargetPlatform())) { - sksl_shader = CompileSkSL(source_file_mapping, switches); - if (!sksl_shader) { - return false; - } - stages.AddShader(sksl_shader); - } - for (const auto& platform : switches.PlatformsToCompile()) { - if (platform == TargetPlatform::kSkSL) { - // Already handled above. - continue; - } SourceOptions options = switches.CreateSourceOptions(platform); // Invoke the compiler and generate reflection data for a single shader. @@ -81,7 +47,8 @@ static bool OutputIPLR( CreateReflectorOptions(options, switches); Compiler compiler(source_file_mapping, options, reflector_options); if (!compiler.IsValid()) { - std::cerr << "Compilation failed." << std::endl; + std::cerr << "Compilation failed for target: " + << TargetPlatformToString(platform) << std::endl; std::cerr << compiler.GetErrorMessages() << std::endl; return false; } @@ -197,24 +164,7 @@ static bool OutputDepfile(const Compiler& compiler, const Switches& switches) { /// if (!switches.depfile_path.empty()) { - std::string result_file; - switch (switches.SelectDefaultTargetPlatform()) { - case TargetPlatform::kMetalDesktop: - case TargetPlatform::kMetalIOS: - case TargetPlatform::kOpenGLES: - case TargetPlatform::kOpenGLDesktop: - case TargetPlatform::kRuntimeStageMetal: - case TargetPlatform::kRuntimeStageGLES: - case TargetPlatform::kRuntimeStageGLES3: - case TargetPlatform::kRuntimeStageVulkan: - case TargetPlatform::kSkSL: - case TargetPlatform::kVulkan: - result_file = Utf8FromPath(switches.sl_file_name); - break; - case TargetPlatform::kUnknown: - result_file = Utf8FromPath(switches.spirv_file_name); - break; - } + std::string result_file = Utf8FromPath(switches.sl_file_name); auto depfile_path = std::filesystem::absolute( std::filesystem::current_path() / switches.depfile_path); if (!fml::WriteAtomically(*switches.working_directory, @@ -263,7 +213,8 @@ bool Main(const fml::CommandLine& command_line) { // Create at least one compiler to output the SL file, reflection data, and a // depfile. - SourceOptions options = switches.CreateSourceOptions(); + SourceOptions options = + switches.CreateSourceOptions(switches.PlatformsToCompile().front()); // Invoke the compiler and generate reflection data for a single shader. diff --git a/engine/src/flutter/impeller/compiler/shader_bundle.cc b/engine/src/flutter/impeller/compiler/shader_bundle.cc index d178e998178..c4324192a91 100644 --- a/engine/src/flutter/impeller/compiler/shader_bundle.cc +++ b/engine/src/flutter/impeller/compiler/shader_bundle.cc @@ -215,7 +215,11 @@ bool GenerateShaderBundle(Switches& switches) { /// auto shader_bundle = GenerateShaderBundleFlatbuffer( - switches.shader_bundle, switches.CreateSourceOptions()); + switches.shader_bundle, + switches.CreateSourceOptions( + // This TargetPlatform does not matter because it is overridden by + // GenerateShaderBackendFB(). + TargetPlatform::kUnknown)); if (!shader_bundle.has_value()) { // Specific error messages are already handled by // GenerateShaderBundleFlatbuffer. diff --git a/engine/src/flutter/impeller/compiler/switches.cc b/engine/src/flutter/impeller/compiler/switches.cc index 8c42598ad47..810c902aee7 100644 --- a/engine/src/flutter/impeller/compiler/switches.cc +++ b/engine/src/flutter/impeller/compiler/switches.cc @@ -25,12 +25,13 @@ static const std::map kKnownPlatforms = { {"opengl-desktop", TargetPlatform::kOpenGLDesktop}, }; -static const std::map kKnownRuntimeStages = { - {"sksl", TargetPlatform::kSkSL}, - {"runtime-stage-metal", TargetPlatform::kRuntimeStageMetal}, - {"runtime-stage-gles", TargetPlatform::kRuntimeStageGLES}, - {"runtime-stage-gles3", TargetPlatform::kRuntimeStageGLES3}, - {"runtime-stage-vulkan", TargetPlatform::kRuntimeStageVulkan}, +static const std::vector> + kKnownRuntimeStages = { + {"sksl", TargetPlatform::kSkSL}, + {"runtime-stage-metal", TargetPlatform::kRuntimeStageMetal}, + {"runtime-stage-gles", TargetPlatform::kRuntimeStageGLES}, + {"runtime-stage-gles3", TargetPlatform::kRuntimeStageGLES3}, + {"runtime-stage-vulkan", TargetPlatform::kRuntimeStageVulkan}, }; static const std::map kKnownSourceTypes = { @@ -309,19 +310,10 @@ std::vector Switches::PlatformsToCompile() const { return {target_platform_}; } -TargetPlatform Switches::SelectDefaultTargetPlatform() const { - if (target_platform_ == TargetPlatform::kUnknown && - !runtime_stages_.empty()) { - return runtime_stages_.front(); - } - return target_platform_; -} - SourceOptions Switches::CreateSourceOptions( - std::optional target_platform) const { + TargetPlatform target_platform) const { SourceOptions options; - options.target_platform = - target_platform.value_or(SelectDefaultTargetPlatform()); + options.target_platform = target_platform; options.source_language = source_language; if (input_type == SourceType::kUnknown) { options.type = SourceTypeFromFileName(source_file_name); diff --git a/engine/src/flutter/impeller/compiler/switches.h b/engine/src/flutter/impeller/compiler/switches.h index 689bd265358..7bb3aed6ee8 100644 --- a/engine/src/flutter/impeller/compiler/switches.h +++ b/engine/src/flutter/impeller/compiler/switches.h @@ -56,12 +56,10 @@ class Switches { /// A vector containing at least one valid platform. std::vector PlatformsToCompile() const; - TargetPlatform SelectDefaultTargetPlatform() const; // Creates source options from these switches for the specified - // TargetPlatform. Uses SelectDefaultTargetPlatform if not specified. - SourceOptions CreateSourceOptions( - std::optional target_platform = std::nullopt) const; + // TargetPlatform. + SourceOptions CreateSourceOptions(TargetPlatform target_platform) const; static void PrintHelp(std::ostream& stream); diff --git a/engine/src/flutter/impeller/compiler/switches_unittests.cc b/engine/src/flutter/impeller/compiler/switches_unittests.cc index e8a8da1d902..4a3d3f05ab7 100644 --- a/engine/src/flutter/impeller/compiler/switches_unittests.cc +++ b/engine/src/flutter/impeller/compiler/switches_unittests.cc @@ -10,6 +10,7 @@ #include "flutter/fml/string_conversion.h" #include "flutter/testing/testing.h" #include "impeller/compiler/switches.h" +#include "impeller/compiler/types.h" #include "impeller/compiler/utilities.h" namespace impeller { @@ -101,7 +102,7 @@ TEST(SwitchesTest, EntryPointPrefixIsApplied) { EXPECT_EQ(switches.entry_point_prefix, "my_prefix_"); switches.source_file_name = "test.frag"; - auto options = switches.CreateSourceOptions(); + auto options = switches.CreateSourceOptions(TargetPlatform::kUnknown); EXPECT_EQ(options.entry_point_name, "my_prefix_test_fragment_main"); } diff --git a/engine/src/flutter/impeller/compiler/types.cc b/engine/src/flutter/impeller/compiler/types.cc index c570e1244ca..761c7a6b2cd 100644 --- a/engine/src/flutter/impeller/compiler/types.cc +++ b/engine/src/flutter/impeller/compiler/types.cc @@ -312,24 +312,5 @@ bool TargetPlatformIsVulkan(TargetPlatform platform) { FML_UNREACHABLE(); } -bool TargetPlatformBundlesSkSL(TargetPlatform platform) { - switch (platform) { - case TargetPlatform::kSkSL: - case TargetPlatform::kRuntimeStageMetal: - case TargetPlatform::kRuntimeStageGLES: - case TargetPlatform::kRuntimeStageGLES3: - case TargetPlatform::kRuntimeStageVulkan: - return true; - case TargetPlatform::kMetalDesktop: - case TargetPlatform::kMetalIOS: - case TargetPlatform::kUnknown: - case TargetPlatform::kOpenGLES: - case TargetPlatform::kOpenGLDesktop: - case TargetPlatform::kVulkan: - return false; - } - FML_UNREACHABLE(); -} - } // namespace compiler } // namespace impeller diff --git a/engine/src/flutter/impeller/compiler/types.h b/engine/src/flutter/impeller/compiler/types.h index 02ef699d14f..37f2fdc9db1 100644 --- a/engine/src/flutter/impeller/compiler/types.h +++ b/engine/src/flutter/impeller/compiler/types.h @@ -124,8 +124,6 @@ std::string EntryPointFunctionNameFromSourceName( bool TargetPlatformNeedsReflection(TargetPlatform platform); -bool TargetPlatformBundlesSkSL(TargetPlatform platform); - std::string ShaderCErrorToString(shaderc_compilation_status status); shaderc_shader_kind ToShaderCShaderKind(SourceType type); diff --git a/engine/src/flutter/impeller/fixtures/BUILD.gn b/engine/src/flutter/impeller/fixtures/BUILD.gn index 73309350e13..cba95658798 100644 --- a/engine/src/flutter/impeller/fixtures/BUILD.gn +++ b/engine/src/flutter/impeller/fixtures/BUILD.gn @@ -104,6 +104,22 @@ impellerc("runtime_stages") { ] sl_file_extension = "iplr" + shader_target_flags = [ + "--sksl", + "--runtime-stage-metal", + "--runtime-stage-gles", + "--runtime-stage-gles3", + "--runtime-stage-vulkan", + ] + + iplr = true +} + +impellerc("runtime_stages_non_sksl") { + mnemonic = "IMPELLERC_IPLR" + shaders = [ "runtime_stage_simple_no_sksl.frag" ] + sl_file_extension = "iplr" + shader_target_flags = [ "--runtime-stage-metal", "--runtime-stage-gles", @@ -172,7 +188,12 @@ test_fixtures("file_fixtures") { } fixtures += filter_include(get_target_outputs(":runtime_stages"), [ "*.iplr" ]) - deps = [ ":runtime_stages" ] + fixtures += filter_include(get_target_outputs(":runtime_stages_non_sksl"), + [ "*.iplr" ]) + deps = [ + ":runtime_stages", + ":runtime_stages_non_sksl", + ] } impellerc("flutter_gpu_shaders") { diff --git a/engine/src/flutter/impeller/fixtures/runtime_stage_simple_no_sksl.frag b/engine/src/flutter/impeller/fixtures/runtime_stage_simple_no_sksl.frag new file mode 100644 index 00000000000..38b00be8d32 --- /dev/null +++ b/engine/src/flutter/impeller/fixtures/runtime_stage_simple_no_sksl.frag @@ -0,0 +1,9 @@ +// 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. + +out vec4 frag_color; + +void main() { + frag_color = vec4(1.0, 0.0, 0.0, 1.0); +} diff --git a/engine/src/flutter/impeller/runtime_stage/runtime_stage_unittests.cc b/engine/src/flutter/impeller/runtime_stage/runtime_stage_unittests.cc index 36e7ad4c76d..456c56470b5 100644 --- a/engine/src/flutter/impeller/runtime_stage/runtime_stage_unittests.cc +++ b/engine/src/flutter/impeller/runtime_stage/runtime_stage_unittests.cc @@ -471,12 +471,18 @@ TEST_P(RuntimeStageTest, ContainsExpectedShaderTypes) { auto stages_result = OpenAssetAsRuntimeStage("ink_sparkle.frag.iplr"); ABSL_ASSERT_OK(stages_result); auto stages = stages_result.value(); - // Right now, SkSL gets implicitly bundled regardless of what the build rule - // for this test requested. After - // https://github.com/flutter/flutter/issues/138919, this may require a build - // rule change or a new test. EXPECT_TRUE(stages[RuntimeStageBackend::kSkSL]); + EXPECT_TRUE(stages[RuntimeStageBackend::kOpenGLES]); + EXPECT_TRUE(stages[RuntimeStageBackend::kMetal]); + EXPECT_TRUE(stages[RuntimeStageBackend::kVulkan]); +} +TEST_P(RuntimeStageTest, ContainsExpectedShaderTypesNoSksl) { + auto stages_result = + OpenAssetAsRuntimeStage("runtime_stage_simple_no_sksl.frag.iplr"); + ABSL_ASSERT_OK(stages_result); + auto stages = stages_result.value(); + EXPECT_FALSE(stages[RuntimeStageBackend::kSkSL]); EXPECT_TRUE(stages[RuntimeStageBackend::kOpenGLES]); EXPECT_TRUE(stages[RuntimeStageBackend::kMetal]); EXPECT_TRUE(stages[RuntimeStageBackend::kVulkan]); From d8d6d161d7a853f85129e4fdcfbf384b0225cc6f Mon Sep 17 00:00:00 2001 From: Benson Luk <97480502+b-luk@users.noreply.github.com> Date: Wed, 18 Feb 2026 15:37:01 -0800 Subject: [PATCH 2/4] Fix compiler.gni logic around when to generate reflection state. - This used to incorrectly generate reflection state whenever the last shader_target_flags is not "--sksl". - Instead, generate reflection state when any non-runtime target is in shader_target_flags. - Consolidate some of the if/else logic to reduce duplicate code. Remove the TargetPlatformNeedsReflection check in impeller_main.cc. - Instead, whether reflection state is generated depends only on the presence or absense of "reflection_{json|header|cc}_name" flags. - The logic of whether to include these flags is already in compiler.gni. So it's redundant to also have logic for whether to generate the reflection state here. - The TargetPlatformNeedsReflection method had faulty logic. - It returned true for everything except SkSL, even though reflection state isn't needed for runtime targets. - It was called on the target from Switches::SelectDefaultTargetPlatform. When impellerc is used with multiple runtime targets, this would return the runtime target that is first alphabetically by flag name. So if --sksl is provided along with any other --runtime-stage-* target, SelectDefaultTargetPlatform returns the non-sksl runtime target. Effectively this meant that TargetPlatformNeedsReflection returns true except for when --sksl is the only provided runtime target. - Removes the TargetPlatformNeedsReflection function entirely. --- .../impeller/compiler/compiler_test.cc | 77 +++++++++---------- .../impeller/compiler/impellerc_main.cc | 63 ++++++++------- engine/src/flutter/impeller/compiler/types.cc | 19 ----- engine/src/flutter/impeller/compiler/types.h | 2 - .../src/flutter/impeller/tools/compiler.gni | 70 ++++++++--------- 5 files changed, 97 insertions(+), 134 deletions(-) diff --git a/engine/src/flutter/impeller/compiler/compiler_test.cc b/engine/src/flutter/impeller/compiler/compiler_test.cc index 683e6bea578..5dc1d566ad4 100644 --- a/engine/src/flutter/impeller/compiler/compiler_test.cc +++ b/engine/src/flutter/impeller/compiler/compiler_test.cc @@ -142,53 +142,50 @@ bool CompilerTestBase::CanCompileAndReflect( return false; } - if (TargetPlatformNeedsReflection(GetParam())) { - auto reflector = compiler.GetReflector(); - if (!reflector) { - VALIDATION_LOG - << "No reflector was found for target platform SL compiler."; - return false; - } + auto reflector = compiler.GetReflector(); + if (!reflector) { + VALIDATION_LOG << "No reflector was found for target platform SL compiler."; + return false; + } - auto reflection_json = reflector->GetReflectionJSON(); - auto reflection_header = reflector->GetReflectionHeader(); - auto reflection_source = reflector->GetReflectionCC(); + auto reflection_json = reflector->GetReflectionJSON(); + auto reflection_header = reflector->GetReflectionHeader(); + auto reflection_source = reflector->GetReflectionCC(); - if (!reflection_json) { - VALIDATION_LOG << "Reflection JSON was not found."; - return false; - } + if (!reflection_json) { + VALIDATION_LOG << "Reflection JSON was not found."; + return false; + } - if (!reflection_header) { - VALIDATION_LOG << "Reflection header was not found."; - return false; - } + if (!reflection_header) { + VALIDATION_LOG << "Reflection header was not found."; + return false; + } - if (!reflection_source) { - VALIDATION_LOG << "Reflection source was not found."; - return false; - } + if (!reflection_source) { + VALIDATION_LOG << "Reflection source was not found."; + return false; + } - if (!fml::WriteAtomically(intermediates_directory_, - ReflectionHeaderName(fixture_name).c_str(), - *reflection_header)) { - VALIDATION_LOG << "Could not write reflection header intermediates."; - return false; - } + if (!fml::WriteAtomically(intermediates_directory_, + ReflectionHeaderName(fixture_name).c_str(), + *reflection_header)) { + VALIDATION_LOG << "Could not write reflection header intermediates."; + return false; + } - if (!fml::WriteAtomically(intermediates_directory_, - ReflectionCCName(fixture_name).c_str(), - *reflection_source)) { - VALIDATION_LOG << "Could not write reflection CC intermediates."; - return false; - } + if (!fml::WriteAtomically(intermediates_directory_, + ReflectionCCName(fixture_name).c_str(), + *reflection_source)) { + VALIDATION_LOG << "Could not write reflection CC intermediates."; + return false; + } - if (!fml::WriteAtomically(intermediates_directory_, - ReflectionJSONName(fixture_name).c_str(), - *reflection_json)) { - VALIDATION_LOG << "Could not write reflection json intermediates."; - return false; - } + if (!fml::WriteAtomically(intermediates_directory_, + ReflectionJSONName(fixture_name).c_str(), + *reflection_json)) { + VALIDATION_LOG << "Could not write reflection json intermediates."; + return false; } return true; } diff --git a/engine/src/flutter/impeller/compiler/impellerc_main.cc b/engine/src/flutter/impeller/compiler/impellerc_main.cc index 37fdf29c030..13eb1a37296 100644 --- a/engine/src/flutter/impeller/compiler/impellerc_main.cc +++ b/engine/src/flutter/impeller/compiler/impellerc_main.cc @@ -116,43 +116,40 @@ static bool OutputReflectionData(const Compiler& compiler, /// May include a JSON file, a C++ header, and/or a C++ TU. /// - if (TargetPlatformNeedsReflection(options.target_platform)) { - if (!switches.reflection_json_name.empty()) { - auto reflection_json_name = std::filesystem::absolute( - std::filesystem::current_path() / switches.reflection_json_name); - if (!fml::WriteAtomically( - *switches.working_directory, - Utf8FromPath(reflection_json_name).c_str(), - *compiler.GetReflector()->GetReflectionJSON())) { - std::cerr << "Could not write reflection json to " - << switches.reflection_json_name << std::endl; - return false; - } + if (!switches.reflection_json_name.empty()) { + auto reflection_json_name = std::filesystem::absolute( + std::filesystem::current_path() / switches.reflection_json_name); + if (!fml::WriteAtomically(*switches.working_directory, + Utf8FromPath(reflection_json_name).c_str(), + *compiler.GetReflector()->GetReflectionJSON())) { + std::cerr << "Could not write reflection json to " + << switches.reflection_json_name << std::endl; + return false; } + } - if (!switches.reflection_header_name.empty()) { - auto reflection_header_name = std::filesystem::absolute( - std::filesystem::current_path() / switches.reflection_header_name); - if (!fml::WriteAtomically( - *switches.working_directory, - Utf8FromPath(reflection_header_name).c_str(), - *compiler.GetReflector()->GetReflectionHeader())) { - std::cerr << "Could not write reflection header to " - << switches.reflection_header_name << std::endl; - return false; - } + if (!switches.reflection_header_name.empty()) { + auto reflection_header_name = std::filesystem::absolute( + std::filesystem::current_path() / switches.reflection_header_name); + if (!fml::WriteAtomically( + *switches.working_directory, + Utf8FromPath(reflection_header_name).c_str(), + *compiler.GetReflector()->GetReflectionHeader())) { + std::cerr << "Could not write reflection header to " + << switches.reflection_header_name << std::endl; + return false; } + } - if (!switches.reflection_cc_name.empty()) { - auto reflection_cc_name = std::filesystem::absolute( - std::filesystem::current_path() / switches.reflection_cc_name); - if (!fml::WriteAtomically(*switches.working_directory, - Utf8FromPath(reflection_cc_name).c_str(), - *compiler.GetReflector()->GetReflectionCC())) { - std::cerr << "Could not write reflection CC to " - << switches.reflection_cc_name << std::endl; - return false; - } + if (!switches.reflection_cc_name.empty()) { + auto reflection_cc_name = std::filesystem::absolute( + std::filesystem::current_path() / switches.reflection_cc_name); + if (!fml::WriteAtomically(*switches.working_directory, + Utf8FromPath(reflection_cc_name).c_str(), + *compiler.GetReflector()->GetReflectionCC())) { + std::cerr << "Could not write reflection CC to " + << switches.reflection_cc_name << std::endl; + return false; } } return true; diff --git a/engine/src/flutter/impeller/compiler/types.cc b/engine/src/flutter/impeller/compiler/types.cc index 761c7a6b2cd..57723c372a4 100644 --- a/engine/src/flutter/impeller/compiler/types.cc +++ b/engine/src/flutter/impeller/compiler/types.cc @@ -127,25 +127,6 @@ std::string EntryPointFunctionNameFromSourceName( return stream.str(); } -bool TargetPlatformNeedsReflection(TargetPlatform platform) { - switch (platform) { - case TargetPlatform::kMetalIOS: - case TargetPlatform::kMetalDesktop: - case TargetPlatform::kOpenGLES: - case TargetPlatform::kOpenGLDesktop: - case TargetPlatform::kRuntimeStageMetal: - case TargetPlatform::kRuntimeStageGLES: - case TargetPlatform::kRuntimeStageGLES3: - case TargetPlatform::kRuntimeStageVulkan: - case TargetPlatform::kVulkan: - return true; - case TargetPlatform::kUnknown: - case TargetPlatform::kSkSL: - return false; - } - FML_UNREACHABLE(); -} - std::string ShaderCErrorToString(shaderc_compilation_status status) { using Status = shaderc_compilation_status; switch (status) { diff --git a/engine/src/flutter/impeller/compiler/types.h b/engine/src/flutter/impeller/compiler/types.h index 37f2fdc9db1..7adddff9e26 100644 --- a/engine/src/flutter/impeller/compiler/types.h +++ b/engine/src/flutter/impeller/compiler/types.h @@ -122,8 +122,6 @@ std::string EntryPointFunctionNameFromSourceName( SourceLanguage source_language, const std::string& entry_point_name); -bool TargetPlatformNeedsReflection(TargetPlatform platform); - std::string ShaderCErrorToString(shaderc_compilation_status status); shaderc_shader_kind ToShaderCShaderKind(SourceType type); diff --git a/engine/src/flutter/impeller/tools/compiler.gni b/engine/src/flutter/impeller/tools/compiler.gni index edaa4f9b65a..7435a7308ca 100644 --- a/engine/src/flutter/impeller/tools/compiler.gni +++ b/engine/src/flutter/impeller/tools/compiler.gni @@ -75,10 +75,6 @@ template("impellerc") { shader_target_flags = [] } - sksl = false - foreach(shader_target_flag, shader_target_flags) { - sksl = shader_target_flag == "--sksl" - } iplr = false if (defined(invoker.iplr) && invoker.iplr) { iplr = invoker.iplr @@ -91,7 +87,6 @@ template("impellerc") { # Not needed on every path. not_needed([ "iplr", - "sksl", "shader_bundle", "shader_bundle_output", ]) @@ -181,11 +176,7 @@ template("impellerc") { ] outputs = [ sl_output ] - } else if (sksl) { - # When SkSL is selected as a `shader_target_flags`, don't generate - # C++ reflection state. Nothing needs to use it and it's likely invalid - # given the special cases when generating SkSL. - # Note that this configuration is orthogonal to the "--iplr" flag + } else { sl_output = "$generated_dir/{{source_file_part}}.${invoker.sl_file_extension}" sl_output_path = rebase_path(sl_output, root_build_dir) @@ -198,40 +189,39 @@ template("impellerc") { ] outputs = [ sl_output ] - } else { - # The default branch. Here we just generate one shader along with all of - # its C++ reflection state. - sl_output = - "$generated_dir/{{source_file_part}}.${invoker.sl_file_extension}" - sl_output_path = rebase_path(sl_output, root_build_dir) + # Generate reflection state only if a non-runtime target is provided. + if (filter_include(shader_target_flags, + [ + "--metal-ios", + "--metal-desktop", + "--opengl-es", + "--opengl-desktop", + "--vulkan", + ]) != []) { + reflection_json_intermediate = + "$generated_dir/{{source_file_part}}.json" + reflection_header_intermediate = "$generated_dir/{{source_file_part}}.h" + reflection_cc_intermediate = "$generated_dir/{{source_file_part}}.cc" - reflection_json_intermediate = "$generated_dir/{{source_file_part}}.json" - reflection_header_intermediate = "$generated_dir/{{source_file_part}}.h" - reflection_cc_intermediate = "$generated_dir/{{source_file_part}}.cc" + reflection_json_path = + rebase_path(reflection_json_intermediate, root_build_dir) + reflection_header_path = + rebase_path(reflection_header_intermediate, root_build_dir) + reflection_cc_path = + rebase_path(reflection_cc_intermediate, root_build_dir) - spirv_intermediate = "$generated_dir/{{source_file_part}}.spirv" - spirv_intermediate_path = rebase_path(spirv_intermediate, root_build_dir) - reflection_json_path = - rebase_path(reflection_json_intermediate, root_build_dir) - reflection_header_path = - rebase_path(reflection_header_intermediate, root_build_dir) - reflection_cc_path = - rebase_path(reflection_cc_intermediate, root_build_dir) + args += [ + "--reflection-json=$reflection_json_path", + "--reflection-header=$reflection_header_path", + "--reflection-cc=$reflection_cc_path", + ] - args += [ - "--sl=$sl_output_path", - "--spirv=$spirv_intermediate_path", - "--reflection-json=$reflection_json_path", - "--reflection-header=$reflection_header_path", - "--reflection-cc=$reflection_cc_path", - ] - - outputs = [ - sl_output, - reflection_header_intermediate, - reflection_cc_intermediate, - ] + outputs += [ + reflection_header_intermediate, + reflection_cc_intermediate, + ] + } } if (defined(invoker.defines)) { From cd747fdab1ab53dd1a8be4ad8d48a885e6cfc36f Mon Sep 17 00:00:00 2001 From: Benson Luk <97480502+b-luk@users.noreply.github.com> Date: Wed, 18 Feb 2026 16:40:52 -0800 Subject: [PATCH 3/4] Address comments --- .../impeller/compiler/compiler_unittests.cc | 88 +++++++++---------- .../impeller/compiler/impellerc_main.cc | 11 ++- .../impeller/compiler/shader_bundle.cc | 6 +- .../src/flutter/impeller/compiler/switches.cc | 4 +- .../src/flutter/impeller/compiler/switches.h | 8 +- .../impeller/compiler/switches_unittests.cc | 2 +- 6 files changed, 56 insertions(+), 63 deletions(-) diff --git a/engine/src/flutter/impeller/compiler/compiler_unittests.cc b/engine/src/flutter/impeller/compiler/compiler_unittests.cc index 67da3ab3bf1..d0d9fa15195 100644 --- a/engine/src/flutter/impeller/compiler/compiler_unittests.cc +++ b/engine/src/flutter/impeller/compiler/compiler_unittests.cc @@ -15,51 +15,45 @@ namespace impeller { namespace compiler { namespace testing { -#define INSTANTIATE_TARGET_PLATFORM_TEST_SUITE_P(suite_name) \ - INSTANTIATE_TEST_SUITE_P( \ - suite_name, CompilerTest, \ - ::testing::Values(TargetPlatform::kOpenGLES, \ - TargetPlatform::kOpenGLDesktop, \ - TargetPlatform::kMetalDesktop, \ - TargetPlatform::kMetalIOS, TargetPlatform::kVulkan), \ - [](const ::testing::TestParamInfo& info) { \ - return TargetPlatformToString(info.param); \ - }); +INSTANTIATE_TEST_SUITE_P( + CompilerSuite, + CompilerTest, + ::testing::Values(TargetPlatform::kOpenGLES, + TargetPlatform::kOpenGLDesktop, + TargetPlatform::kMetalDesktop, + TargetPlatform::kMetalIOS, + TargetPlatform::kVulkan), + [](const ::testing::TestParamInfo& info) { + return TargetPlatformToString(info.param); + }); -INSTANTIATE_TARGET_PLATFORM_TEST_SUITE_P(CompilerSuite); +INSTANTIATE_TEST_SUITE_P( + CompilerSuite, + CompilerTestRuntime, + ::testing::Values(TargetPlatform::kRuntimeStageMetal, + TargetPlatform::kRuntimeStageGLES, + TargetPlatform::kRuntimeStageGLES3, + TargetPlatform::kRuntimeStageVulkan, + TargetPlatform::kSkSL), + [](const ::testing::TestParamInfo& info) { + return TargetPlatformToString(info.param); + }); -#define INSTANTIATE_RUNTIME_TARGET_PLATFORM_TEST_SUITE_P(suite_name) \ - INSTANTIATE_TEST_SUITE_P( \ - suite_name, CompilerTestRuntime, \ - ::testing::Values(TargetPlatform::kRuntimeStageMetal, \ - TargetPlatform::kRuntimeStageGLES, \ - TargetPlatform::kRuntimeStageGLES3, \ - TargetPlatform::kRuntimeStageVulkan, \ - TargetPlatform::kSkSL), \ - [](const ::testing::TestParamInfo& info) { \ - return TargetPlatformToString(info.param); \ - }); +INSTANTIATE_TEST_SUITE_P( + CompilerSuite, + CompilerTestSkSL, + ::testing::Values(TargetPlatform::kSkSL), + [](const ::testing::TestParamInfo& info) { + return TargetPlatformToString(info.param); + }); -INSTANTIATE_RUNTIME_TARGET_PLATFORM_TEST_SUITE_P(CompilerSuite); - -#define INSTANTIATE_SKSL_TARGET_PLATFORM_TEST_SUITE_P(suite_name) \ - INSTANTIATE_TEST_SUITE_P( \ - suite_name, CompilerTestSkSL, ::testing::Values(TargetPlatform::kSkSL), \ - [](const ::testing::TestParamInfo& info) { \ - return TargetPlatformToString(info.param); \ - }); - -INSTANTIATE_SKSL_TARGET_PLATFORM_TEST_SUITE_P(CompilerSuite); - -#define INSTANTIATE_UNKNOWN_TARGET_PLATFORM_TEST_SUITE_P(suite_name) \ - INSTANTIATE_TEST_SUITE_P( \ - suite_name, CompilerTestUnknownPlatform, \ - ::testing::Values(TargetPlatform::kUnknown), \ - [](const ::testing::TestParamInfo& info) { \ - return TargetPlatformToString(info.param); \ - }); - -INSTANTIATE_UNKNOWN_TARGET_PLATFORM_TEST_SUITE_P(CompilerSuite); +INSTANTIATE_TEST_SUITE_P( + CompilerSuite, + CompilerTestUnknownPlatform, + ::testing::Values(TargetPlatform::kUnknown), + [](const ::testing::TestParamInfo& info) { + return TargetPlatformToString(info.param); + }); TEST(CompilerTest, Defines) { std::shared_ptr fixture = @@ -233,10 +227,9 @@ inline std::ostream& operator<<(std::ostream& out, const UniformInfo& info) { } // namespace TEST_P(CompilerTestRuntime, UniformsAppearInJson) { - if (GetParam() == TargetPlatform::kSkSL) { - GTEST_SKIP() << "Not supported with SkSL"; - } if (GetParam() == TargetPlatform::kRuntimeStageVulkan) { + // TODO: Investigate why does not pass: + // https://github.com/flutter/flutter/issues/182578 GTEST_SKIP() << "Not supported with Vulkan"; } @@ -286,10 +279,9 @@ TEST_P(CompilerTestRuntime, UniformsAppearInJson) { } TEST_P(CompilerTestRuntime, PositionedUniformsAppearInJson) { - if (GetParam() == TargetPlatform::kSkSL) { - GTEST_SKIP() << "Not supported with SkSL"; - } if (GetParam() == TargetPlatform::kRuntimeStageVulkan) { + // TODO: Investigate why does not pass: + // https://github.com/flutter/flutter/issues/182578 GTEST_SKIP() << "Not supported with Vulkan"; } diff --git a/engine/src/flutter/impeller/compiler/impellerc_main.cc b/engine/src/flutter/impeller/compiler/impellerc_main.cc index 13eb1a37296..6316affa7af 100644 --- a/engine/src/flutter/impeller/compiler/impellerc_main.cc +++ b/engine/src/flutter/impeller/compiler/impellerc_main.cc @@ -39,7 +39,8 @@ static bool OutputIPLR( RuntimeStageData stages; for (const auto& platform : switches.PlatformsToCompile()) { - SourceOptions options = switches.CreateSourceOptions(platform); + SourceOptions options = switches.CreateSourceOptions(); + options.target_platform = platform; // Invoke the compiler and generate reflection data for a single shader. @@ -210,8 +211,12 @@ bool Main(const fml::CommandLine& command_line) { // Create at least one compiler to output the SL file, reflection data, and a // depfile. - SourceOptions options = - switches.CreateSourceOptions(switches.PlatformsToCompile().front()); + SourceOptions options = switches.CreateSourceOptions(); + // If there are multiple platform compile targets, the specific target + // platform that is used does not matter because the output files won't depend + // on the target platform. Arbitrarily choose the first one from + // PlatformsToCompile(). + options.target_platform = switches.PlatformsToCompile().front(); // Invoke the compiler and generate reflection data for a single shader. diff --git a/engine/src/flutter/impeller/compiler/shader_bundle.cc b/engine/src/flutter/impeller/compiler/shader_bundle.cc index c4324192a91..d178e998178 100644 --- a/engine/src/flutter/impeller/compiler/shader_bundle.cc +++ b/engine/src/flutter/impeller/compiler/shader_bundle.cc @@ -215,11 +215,7 @@ bool GenerateShaderBundle(Switches& switches) { /// auto shader_bundle = GenerateShaderBundleFlatbuffer( - switches.shader_bundle, - switches.CreateSourceOptions( - // This TargetPlatform does not matter because it is overridden by - // GenerateShaderBackendFB(). - TargetPlatform::kUnknown)); + switches.shader_bundle, switches.CreateSourceOptions()); if (!shader_bundle.has_value()) { // Specific error messages are already handled by // GenerateShaderBundleFlatbuffer. diff --git a/engine/src/flutter/impeller/compiler/switches.cc b/engine/src/flutter/impeller/compiler/switches.cc index 810c902aee7..057bc86aa39 100644 --- a/engine/src/flutter/impeller/compiler/switches.cc +++ b/engine/src/flutter/impeller/compiler/switches.cc @@ -310,10 +310,8 @@ std::vector Switches::PlatformsToCompile() const { return {target_platform_}; } -SourceOptions Switches::CreateSourceOptions( - TargetPlatform target_platform) const { +SourceOptions Switches::CreateSourceOptions() const { SourceOptions options; - options.target_platform = target_platform; options.source_language = source_language; if (input_type == SourceType::kUnknown) { options.type = SourceTypeFromFileName(source_file_name); diff --git a/engine/src/flutter/impeller/compiler/switches.h b/engine/src/flutter/impeller/compiler/switches.h index 7bb3aed6ee8..044659d02f5 100644 --- a/engine/src/flutter/impeller/compiler/switches.h +++ b/engine/src/flutter/impeller/compiler/switches.h @@ -57,9 +57,11 @@ class Switches { /// A vector containing at least one valid platform. std::vector PlatformsToCompile() const; - // Creates source options from these switches for the specified - // TargetPlatform. - SourceOptions CreateSourceOptions(TargetPlatform target_platform) const; + // Creates source options from these switches. The returned options does not + // have a set TargetPlatform because that cannot be determined based purely + // on the switches. Clients must set a valid TargetPlatform on the returned + // options before before it is used. + SourceOptions CreateSourceOptions() const; static void PrintHelp(std::ostream& stream); diff --git a/engine/src/flutter/impeller/compiler/switches_unittests.cc b/engine/src/flutter/impeller/compiler/switches_unittests.cc index 4a3d3f05ab7..6372809a764 100644 --- a/engine/src/flutter/impeller/compiler/switches_unittests.cc +++ b/engine/src/flutter/impeller/compiler/switches_unittests.cc @@ -102,7 +102,7 @@ TEST(SwitchesTest, EntryPointPrefixIsApplied) { EXPECT_EQ(switches.entry_point_prefix, "my_prefix_"); switches.source_file_name = "test.frag"; - auto options = switches.CreateSourceOptions(TargetPlatform::kUnknown); + auto options = switches.CreateSourceOptions(); EXPECT_EQ(options.entry_point_name, "my_prefix_test_fragment_main"); } From 57e30bdc1716ec11f0e2478ca6fb77ef7aa1c35a Mon Sep 17 00:00:00 2001 From: Benson Luk <97480502+b-luk@users.noreply.github.com> Date: Wed, 18 Feb 2026 18:23:26 -0800 Subject: [PATCH 4/4] Update TODO format to not fail lint --- .../src/flutter/impeller/compiler/compiler_unittests.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/engine/src/flutter/impeller/compiler/compiler_unittests.cc b/engine/src/flutter/impeller/compiler/compiler_unittests.cc index d0d9fa15195..c680558d0d6 100644 --- a/engine/src/flutter/impeller/compiler/compiler_unittests.cc +++ b/engine/src/flutter/impeller/compiler/compiler_unittests.cc @@ -228,8 +228,8 @@ inline std::ostream& operator<<(std::ostream& out, const UniformInfo& info) { TEST_P(CompilerTestRuntime, UniformsAppearInJson) { if (GetParam() == TargetPlatform::kRuntimeStageVulkan) { - // TODO: Investigate why does not pass: - // https://github.com/flutter/flutter/issues/182578 + // TODO(https://github.com/flutter/flutter/issues/182578): Investigate why + // this does not pass. GTEST_SKIP() << "Not supported with Vulkan"; } @@ -280,8 +280,8 @@ TEST_P(CompilerTestRuntime, UniformsAppearInJson) { TEST_P(CompilerTestRuntime, PositionedUniformsAppearInJson) { if (GetParam() == TargetPlatform::kRuntimeStageVulkan) { - // TODO: Investigate why does not pass: - // https://github.com/flutter/flutter/issues/182578 + // TODO(https://github.com/flutter/flutter/issues/182578): Investigate why + // this does not pass. GTEST_SKIP() << "Not supported with Vulkan"; }