diff --git a/impeller/compiler/compiler.cc b/impeller/compiler/compiler.cc index 0d7af022cc36f..795f9962a7479 100644 --- a/impeller/compiler/compiler.cc +++ b/impeller/compiler/compiler.cc @@ -464,13 +464,24 @@ const std::vector& Compiler::GetIncludedFileNames() const { return included_file_names_; } +// Escape `%` and `#` characters according to doc comment at +// https://github.com/ninja-build/ninja/blob/master/src/depfile_parser.cc#L28 +static void EscapeString(std::string& str, std::stringstream& stream) { + for (auto it = str.begin(); it != str.end(); it++) { + if (*it == '%' || *it == '#') { + stream << '\\'; + } + stream << *it; + } +} + static std::string JoinStrings(std::vector items, std::string separator) { std::stringstream stream; for (size_t i = 0, count = items.size(); i < count; i++) { const auto is_last = (i == count - 1); - stream << items[i]; + EscapeString(items[i], stream); if (!is_last) { stream << separator; } @@ -486,6 +497,7 @@ std::string Compiler::GetDependencyNames(std::string separator) const { std::unique_ptr Compiler::CreateDepfileContents( std::initializer_list targets_names) const { + // https://github.com/ninja-build/ninja/blob/master/src/depfile_parser.cc#L28 const auto targets = JoinStrings(targets_names, " "); const auto dependencies = GetDependencyNames(" "); diff --git a/impeller/compiler/compiler_test.cc b/impeller/compiler/compiler_test.cc index 85b3abaa5f9c5..79285051f1f81 100644 --- a/impeller/compiler/compiler_test.cc +++ b/impeller/compiler/compiler_test.cc @@ -58,6 +58,37 @@ static std::string SLFileName(const char* fixture_name, return stream.str(); } +static std::string DepfileName(const char* fixture_name) { + std::stringstream stream; + stream << fixture_name << ".d"; + return stream.str(); +} + +bool CompilerTest::ValidateDepfileEscaped(const char* fixture_name) const { + auto depfile_name = DepfileName(fixture_name); + auto mapping = std::make_unique( + fml::OpenFile(intermediates_directory_, depfile_name.c_str(), false, + fml::FilePermission::kRead)); + + std::string contents(reinterpret_cast(mapping->GetMapping()), + mapping->GetSize()); + bool escaped = false; + for (auto it = contents.begin(); it != contents.end(); it++) { + if (*it == '\\') { + escaped = true; + } else if (*it == '%' || *it == '#') { + if (!escaped) { + VALIDATION_LOG << "Unescaped character " << *it << " in depfile."; + return false; + } + escaped = false; + } else { + escaped = false; + } + } + return true; +} + bool CompilerTest::CanCompileAndReflect(const char* fixture_name, SourceType source_type) const { auto fixture = flutter::testing::OpenFixtureAsMapping(fixture_name); @@ -160,6 +191,12 @@ bool CompilerTest::CanCompileAndReflect(const char* fixture_name, } } + auto mapping = compiler.CreateDepfileContents({fixture_name}); + if (!fml::WriteAtomically(intermediates_directory_, + DepfileName(fixture_name).c_str(), *mapping)) { + VALIDATION_LOG << "Could not write depfile."; + return false; + } return true; } diff --git a/impeller/compiler/compiler_test.h b/impeller/compiler/compiler_test.h index a7be1c688b198..be9504de15dac 100644 --- a/impeller/compiler/compiler_test.h +++ b/impeller/compiler/compiler_test.h @@ -25,6 +25,8 @@ class CompilerTest : public ::testing::TestWithParam { const char* fixture_name, SourceType source_type = SourceType::kUnknown) const; + bool ValidateDepfileEscaped(const char* fixture_name) const; + private: fml::UniqueFD intermediates_directory_; diff --git a/impeller/compiler/compiler_unittests.cc b/impeller/compiler/compiler_unittests.cc index 23101e313f90a..68084d62d35ca 100644 --- a/impeller/compiler/compiler_unittests.cc +++ b/impeller/compiler/compiler_unittests.cc @@ -65,6 +65,11 @@ TEST_P(CompilerTest, MustFailDueToMultipleLocationPerStructMember) { ASSERT_FALSE(CanCompileAndReflect("struct_def_bug.vert")); } +TEST_P(CompilerTest, ShaderWithSpecialCharactersHasEscapedDepfile) { + ASSERT_TRUE(CanCompileAndReflect("sa\%m#ple.vert")); + ASSERT_TRUE(ValidateDepfileEscaped("sa\%m#ple.vert")); +} + #define INSTANTIATE_TARGET_PLATFORM_TEST_SUITE_P(suite_name) \ INSTANTIATE_TEST_SUITE_P( \ suite_name, CompilerTest, \ diff --git a/impeller/fixtures/BUILD.gn b/impeller/fixtures/BUILD.gn index 3a62e149c864d..88a828942649a 100644 --- a/impeller/fixtures/BUILD.gn +++ b/impeller/fixtures/BUILD.gn @@ -47,6 +47,7 @@ test_fixtures("file_fixtures") { "sample.tesc", "sample.tese", "sample.vert", + "sa%m#ple.vert", "struct_def_bug.vert", "table_mountain_nx.png", "table_mountain_ny.png", diff --git a/impeller/fixtures/sa%m#ple.vert b/impeller/fixtures/sa%m#ple.vert new file mode 100644 index 0000000000000..fc7e9d1122000 --- /dev/null +++ b/impeller/fixtures/sa%m#ple.vert @@ -0,0 +1,23 @@ +// 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. + +#include "types.h" + +uniform UniformBufferObject { + Uniforms uniforms; +} ubo; + +uniform sampler2D world; + +in vec2 inPosition; +in vec3 inPosition22; +in vec4 inAnotherPosition; +in float stuff; + +out vec4 outStuff; + +void main() { + gl_Position = ubo.uniforms.projection * ubo.uniforms.view * ubo.uniforms.model * vec4(inPosition22, 1.0) * inAnotherPosition; + outStuff = texture(world, inPosition); +} diff --git a/lib/ui/BUILD.gn b/lib/ui/BUILD.gn index 81433a10e754e..5be9029295f03 100644 --- a/lib/ui/BUILD.gn +++ b/lib/ui/BUILD.gn @@ -187,6 +187,7 @@ if (enable_unittests) { dart_main = "fixtures/ui_test.dart" fixtures = [ "fixtures/DashInNooglerHat.jpg", + "fixtures/DashInNooglerHat%20WithSpace.jpg", "fixtures/Horizontal.jpg", "fixtures/Horizontal.png", "fixtures/hello_loop_2.gif", diff --git a/lib/ui/fixtures/DashInNooglerHat%20WithSpace.jpg b/lib/ui/fixtures/DashInNooglerHat%20WithSpace.jpg new file mode 100644 index 0000000000000..488fdb4d5215c Binary files /dev/null and b/lib/ui/fixtures/DashInNooglerHat%20WithSpace.jpg differ diff --git a/lib/ui/fixtures/shaders/general_shaders/BUILD.gn b/lib/ui/fixtures/shaders/general_shaders/BUILD.gn index 640823715e8f2..eb14c9ca2f859 100644 --- a/lib/ui/fixtures/shaders/general_shaders/BUILD.gn +++ b/lib/ui/fixtures/shaders/general_shaders/BUILD.gn @@ -11,6 +11,7 @@ if (enable_unittests) { "blue_green_sampler.frag", "children_and_uniforms.frag", "functions.frag", + "functions%20with_space.frag", "no_builtin_redefinition.frag", "no_uniforms.frag", "simple.frag", diff --git a/lib/ui/fixtures/shaders/general_shaders/functions%20with_space.frag b/lib/ui/fixtures/shaders/general_shaders/functions%20with_space.frag new file mode 100644 index 0000000000000..52bf92dbcc1cb --- /dev/null +++ b/lib/ui/fixtures/shaders/general_shaders/functions%20with_space.frag @@ -0,0 +1,34 @@ +#version 320 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. + +precision highp float; + +layout ( location = 0 ) out vec4 oColor; + +layout ( location = 0 ) uniform float a; // should be 1.0 + +float addA(float x) { + return x + a; +} + +vec2 pairWithA(float x) { + return vec2(x, a); +} + +vec3 composedFunction(float x) { + return vec3(addA(x), pairWithA(x)); +} + +float multiParam(float x, float y, float z) { + return x * y * z * a; +} + +void main() { + float x = addA(0.0); // x = 0 + 1; + vec3 v3 = composedFunction(x); // v3 = vec3(2, 1, 1); + x = multiParam(v3.x, v3.y, v3.z); // x = 2 * 1 * 1 * 1; + oColor = vec4(0.0, x / 2.0, 0.0, 1.0); // vec4(0, 1, 0, 1); +} diff --git a/lib/ui/painting.dart b/lib/ui/painting.dart index 8be9426168858..90bb63654b396 100644 --- a/lib/ui/painting.dart +++ b/lib/ui/painting.dart @@ -4077,13 +4077,18 @@ class FragmentProgram extends NativeFieldWrapperClass1 { /// compiler. The constructed object should then be reused via the [shader] /// method to create [Shader] objects that can be used by [Shader.paint]. static Future fromAsset(String assetKey) { - final FragmentProgram? program = _shaderRegistry[assetKey]?.target; + // The flutter tool converts all asset keys with spaces into URI + // encoded paths (replacing ' ' with '%20', for example). We perform + // the same encoding here so that users can load assets with the same + // key they have written in the pubspec. + final String encodedKey = Uri(path: Uri.encodeFull(assetKey)).path; + final FragmentProgram? program = _shaderRegistry[encodedKey]?.target; if (program != null) { return Future.value(program); } return Future.microtask(() { - final FragmentProgram program = FragmentProgram._fromAsset(assetKey); - _shaderRegistry[assetKey] = WeakReference(program); + final FragmentProgram program = FragmentProgram._fromAsset(encodedKey); + _shaderRegistry[encodedKey] = WeakReference(program); return program; }); } @@ -5955,9 +5960,14 @@ class ImmutableBuffer extends NativeFieldWrapperClass1 { /// /// Throws an [Exception] if the asset does not exist. static Future fromAsset(String assetKey) { + // The flutter tool converts all asset keys with spaces into URI + // encoded paths (replacing ' ' with '%20', for example). We perform + // the same encoding here so that users can load assets with the same + // key they have written in the pubspec. + final String encodedKey = Uri(path: Uri.encodeFull(assetKey)).path; final ImmutableBuffer instance = ImmutableBuffer._(0); return _futurize((_Callback callback) { - return instance._initFromAsset(assetKey, callback); + return instance._initFromAsset(encodedKey, callback); }).then((int length) => instance.._length = length); } diff --git a/testing/dart/assets_test.dart b/testing/dart/assets_test.dart index 7a2522c399fc5..64943c2179a91 100644 --- a/testing/dart/assets_test.dart +++ b/testing/dart/assets_test.dart @@ -27,6 +27,13 @@ void main() { expect(buffer.length == 354679, true); }); + test('Can load an asset with a space in the key', () async { + // This assets actual path is "fixtures/DashInNooglerHat%20WithSpace.jpg" + final ImmutableBuffer buffer = await ImmutableBuffer.fromAsset('DashInNooglerHat WithSpace.jpg'); + + expect(buffer.length == 354679, true); + }); + test('can dispose immutable buffer', () async { final ImmutableBuffer buffer = await ImmutableBuffer.fromAsset('DashInNooglerHat.jpg'); diff --git a/testing/dart/fragment_shader_test.dart b/testing/dart/fragment_shader_test.dart index 7fdcdf2742654..d3b1b33521145 100644 --- a/testing/dart/fragment_shader_test.dart +++ b/testing/dart/fragment_shader_test.dart @@ -24,6 +24,16 @@ void main() async { _expectShaderRendersGreen(shader); }); + test('simple shader with space in key renders correctly', () async { + final FragmentProgram program = await FragmentProgram.fromAsset( + 'functions with_space.frag.iplr', + ); + final Shader shader = program.shader( + floatUniforms: Float32List.fromList([1]), + ); + _expectShaderRendersGreen(shader); + }); + test('blue-green image renders green', () async { final FragmentProgram program = await FragmentProgram.fromAsset( 'blue_green_sampler.frag.iplr',