From 2bd9ecbb73d938ef6537adc0e4992f4423ca7a77 Mon Sep 17 00:00:00 2001 From: Tim Maffett Date: Tue, 2 Dec 2025 13:57:29 -0800 Subject: [PATCH] Adds Impellerc flatbuffer format versioning. (#175470) Fixes #172899 This PR adds a format_version number to the root table of the 3 flat buffer formats that impellerc creates; runtime stages, shader bundles and shader archives. It also adds a 'format_version' key to the root table of the shader bundle json format for completeness (Though it could be argued that the json format does not require this, I feel it may still be useful there). This allows for format compatibility checks with future versions of the table as @chinmaygarde expressed concerns about in #168294 and @jonahwilliams mentions in #166944 . The current code only checks for compatibility with the current version, but this mechanism allows for backwards compatibility in the future and not handling flat buffers with formats with new version number. The big win here is that format_version numbers allow us to abort parsing anything that we may not understand that could possible cause problems, corruptions or crashes. The changes are simple and straightforward and I feel they lay a reasonable foundation for version handling for any future changes. The version numbers themselves are defined within the `runtime_stage_types.fbs`, `shader_bundle.fbs` and `shader_archive.fbs` where I feel they should be as they can directly be incremented if any changes to the flat buffer format are made. Mechanisms for handling of backwards compatibility in the future should be straightforward to make in the corresponding c++ code allowing for the continued support of older (lower) version numbers if so desired. This addresses #172899 and the other concerns mentioned in the issues/pr's mentioned above. I plan on including my tests for exclusion of non-matching versions - Existing test verify that the version checking confirms matching versions. I wanted to get this out there for discussion before spending more time on this in case this approach is rejected outright. I have a companion PR which adds FragmentProgram.fromBytes() to the engine (c++ and canvaskit/wasm). I wanted to separate the impellerc output versioning into it's own (this) PR. ## Pre-launch Checklist - [X] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [X] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [X] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [X] I signed the [CLA]. - [X] I listed at least one issue that this PR fixes in the description above. - [X] I updated/added relevant documentation (doc comments with `///`). - [FORTHCOMING] I added new tests to check the change I am making, or this PR is [test-exempt]. - [X] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [X] All existing and new tests are passing. --------- Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Co-authored-by: gaaclarke <30870216+gaaclarke@users.noreply.github.com> Co-authored-by: Aaron Clarke --- .../impeller/compiler/runtime_stage_data.cc | 6 +++ .../impeller/compiler/shader_bundle.cc | 2 + .../golden_playground_test_mac.cc | 6 ++- .../impeller/playground/playground_test.cc | 6 ++- .../backend/gles/shader_library_gles.cc | 6 +-- .../backend/vulkan/shader_library_vk.cc | 6 +-- .../flutter/impeller/runtime_stage/BUILD.gn | 1 + .../impeller/runtime_stage/runtime_stage.cc | 24 ++++++++-- .../impeller/runtime_stage/runtime_stage.h | 4 +- .../runtime_stage/runtime_stage_types.fbs | 10 +++- .../runtime_stage/runtime_stage_unittests.cc | 37 ++++++++++++--- .../flutter/impeller/shader_archive/BUILD.gn | 1 + .../impeller/shader_archive/shader_archive.cc | 47 +++++++++++-------- .../shader_archive/shader_archive.fbs | 8 ++++ .../impeller/shader_archive/shader_archive.h | 11 +++-- .../shader_archive_unittests.cc | 34 ++++++++++++-- .../shader_archive/shader_archive_writer.cc | 2 + .../impeller/shader_bundle/shader_bundle.fbs | 8 ++++ .../toolkit/interop/fragment_program.cc | 6 ++- engine/src/flutter/lib/gpu/shader_library.cc | 9 ++++ .../lib/ui/painting/fragment_program.cc | 12 +++-- .../testing/dart/fragment_shader_test.dart | 2 +- 22 files changed, 195 insertions(+), 53 deletions(-) diff --git a/engine/src/flutter/impeller/compiler/runtime_stage_data.cc b/engine/src/flutter/impeller/compiler/runtime_stage_data.cc index c7b28a6184f..b745ec94673 100644 --- a/engine/src/flutter/impeller/compiler/runtime_stage_data.cc +++ b/engine/src/flutter/impeller/compiler/runtime_stage_data.cc @@ -181,6 +181,7 @@ static std::optional ToJsonType( FML_UNREACHABLE(); } +static const char* kFormatVersionKey = "format_version"; static const char* kStageKey = "stage"; static const char* kTargetPlatformKey = "target_platform"; static const char* kEntrypointKey = "entrypoint"; @@ -212,6 +213,7 @@ static std::string RuntimeStageBackendToString(RuntimeStageBackend backend) { std::shared_ptr RuntimeStageData::CreateJsonMapping() const { // Runtime Stage Data JSON format // { + // "format_version": 1, // "sksl": { // "stage": 0, // "entrypoint": "", @@ -232,6 +234,8 @@ std::shared_ptr RuntimeStageData::CreateJsonMapping() const { // }, nlohmann::json root; + root[kFormatVersionKey] = + static_cast(fb::RuntimeStagesFormatVersion::kVersion); for (const auto& kvp : data_) { nlohmann::json platform_object; @@ -370,6 +374,8 @@ RuntimeStageData::CreateMultiStageFlatbuffer() const { // The high level object API is used here for writing to the buffer. This is // just a convenience. auto runtime_stages = std::make_unique(); + runtime_stages->format_version = + static_cast(fb::RuntimeStagesFormatVersion::kVersion); for (const auto& kvp : data_) { auto runtime_stage = CreateStageFlatbuffer(kvp.first); diff --git a/engine/src/flutter/impeller/compiler/shader_bundle.cc b/engine/src/flutter/impeller/compiler/shader_bundle.cc index 155029148cc..d178e998178 100644 --- a/engine/src/flutter/impeller/compiler/shader_bundle.cc +++ b/engine/src/flutter/impeller/compiler/shader_bundle.cc @@ -194,6 +194,8 @@ std::optional GenerateShaderBundleFlatbuffer( /// fb::shaderbundle::ShaderBundleT shader_bundle; + shader_bundle.format_version = static_cast( + fb::shaderbundle::ShaderBundleFormatVersion::kVersion); for (const auto& [shader_name, shader_config] : bundle_config.value()) { std::unique_ptr shader = diff --git a/engine/src/flutter/impeller/golden_tests/golden_playground_test_mac.cc b/engine/src/flutter/impeller/golden_tests/golden_playground_test_mac.cc index ae7dae00c15..d8e43e74bd2 100644 --- a/engine/src/flutter/impeller/golden_tests/golden_playground_test_mac.cc +++ b/engine/src/flutter/impeller/golden_tests/golden_playground_test_mac.cc @@ -281,7 +281,11 @@ RuntimeStage::Map GoldenPlaygroundTest::OpenAssetAsRuntimeStage( if (!fixture || fixture->GetSize() == 0) { return {}; } - return RuntimeStage::DecodeRuntimeStages(fixture); + auto stages = RuntimeStage::DecodeRuntimeStages(fixture); + if (!stages.ok()) { + return {}; + } + return stages.value(); } std::shared_ptr GoldenPlaygroundTest::GetContext() const { diff --git a/engine/src/flutter/impeller/playground/playground_test.cc b/engine/src/flutter/impeller/playground/playground_test.cc index e6f9f491d61..8b5be9c8608 100644 --- a/engine/src/flutter/impeller/playground/playground_test.cc +++ b/engine/src/flutter/impeller/playground/playground_test.cc @@ -93,7 +93,11 @@ RuntimeStage::Map PlaygroundTest::OpenAssetAsRuntimeStage( if (!fixture || fixture->GetSize() == 0) { return {}; } - return RuntimeStage::DecodeRuntimeStages(fixture); + auto stages = RuntimeStage::DecodeRuntimeStages(fixture); + if (!stages.ok()) { + return {}; + } + return stages.value(); } // |Playground| diff --git a/engine/src/flutter/impeller/renderer/backend/gles/shader_library_gles.cc b/engine/src/flutter/impeller/renderer/backend/gles/shader_library_gles.cc index 0d52f6c176b..876341e5d6e 100644 --- a/engine/src/flutter/impeller/renderer/backend/gles/shader_library_gles.cc +++ b/engine/src/flutter/impeller/renderer/backend/gles/shader_library_gles.cc @@ -67,12 +67,12 @@ ShaderLibraryGLES::ShaderLibraryGLES( return true; }; for (auto library : shader_libraries) { - auto blob_library = ShaderArchive{std::move(library)}; - if (!blob_library.IsValid()) { + auto blob_library = ShaderArchive::Create(std::move(library)); + if (!blob_library.ok()) { VALIDATION_LOG << "Could not construct blob library for shaders."; return; } - blob_library.IterateAllShaders(iterator); + blob_library->IterateAllShaders(iterator); } functions_ = functions; diff --git a/engine/src/flutter/impeller/renderer/backend/vulkan/shader_library_vk.cc b/engine/src/flutter/impeller/renderer/backend/vulkan/shader_library_vk.cc index 3748328b3ea..2782c65ad1b 100644 --- a/engine/src/flutter/impeller/renderer/backend/vulkan/shader_library_vk.cc +++ b/engine/src/flutter/impeller/renderer/backend/vulkan/shader_library_vk.cc @@ -67,12 +67,12 @@ ShaderLibraryVK::ShaderLibraryVK( return true; }; for (const auto& library_data : shader_libraries_data) { - auto blob_library = ShaderArchive{library_data}; - if (!blob_library.IsValid()) { + auto blob_library = ShaderArchive::Create(library_data); + if (!blob_library.ok()) { VALIDATION_LOG << "Could not construct shader blob library."; return; } - blob_library.IterateAllShaders(iterator); + blob_library->IterateAllShaders(iterator); } if (!success) { diff --git a/engine/src/flutter/impeller/runtime_stage/BUILD.gn b/engine/src/flutter/impeller/runtime_stage/BUILD.gn index 8cc4f3b0a22..ff059a55abd 100644 --- a/engine/src/flutter/impeller/runtime_stage/BUILD.gn +++ b/engine/src/flutter/impeller/runtime_stage/BUILD.gn @@ -38,6 +38,7 @@ impeller_component("runtime_stage") { "../base", "../core", "//flutter/fml", + "//third_party/abseil-cpp/absl/status:statusor", ] } diff --git a/engine/src/flutter/impeller/runtime_stage/runtime_stage.cc b/engine/src/flutter/impeller/runtime_stage/runtime_stage.cc index fb98148dfea..598f24631f7 100644 --- a/engine/src/flutter/impeller/runtime_stage/runtime_stage.cc +++ b/engine/src/flutter/impeller/runtime_stage/runtime_stage.cc @@ -6,6 +6,7 @@ #include #include +#include #include "fml/mapping.h" #include "impeller/base/validation.h" @@ -59,17 +60,32 @@ std::unique_ptr RuntimeStage::RuntimeStageIfPresent( new RuntimeStage(runtime_stage, payload)); } -RuntimeStage::Map RuntimeStage::DecodeRuntimeStages( +absl::StatusOr RuntimeStage::DecodeRuntimeStages( const std::shared_ptr& payload) { if (payload == nullptr || !payload->GetMapping()) { - return {}; + return absl::InvalidArgumentError("Payload is null or empty."); } if (!fb::RuntimeStagesBufferHasIdentifier(payload->GetMapping())) { - return {}; + return absl::InvalidArgumentError( + "Payload does not have valid identifier."); } auto raw_stages = fb::GetRuntimeStages(payload->GetMapping()); - return { + if (!raw_stages) { + return absl::InvalidArgumentError("Failed to get runtime stages."); + } + + const uint32_t version = raw_stages->format_version(); + const auto expected = + static_cast(fb::RuntimeStagesFormatVersion::kVersion); + if (version != expected) { + std::stringstream stream; + stream << "Unsupported runtime stages format version. Expected " << expected + << ", got " << version << "."; + return absl::InvalidArgumentError(stream.str()); + } + + return Map{ {RuntimeStageBackend::kSkSL, RuntimeStageIfPresent(raw_stages->sksl(), payload)}, {RuntimeStageBackend::kMetal, diff --git a/engine/src/flutter/impeller/runtime_stage/runtime_stage.h b/engine/src/flutter/impeller/runtime_stage/runtime_stage.h index 86af3111f84..e8b4d7e090b 100644 --- a/engine/src/flutter/impeller/runtime_stage/runtime_stage.h +++ b/engine/src/flutter/impeller/runtime_stage/runtime_stage.h @@ -14,6 +14,7 @@ #include "flutter/impeller/core/runtime_types.h" #include "impeller/core/shader_types.h" #include "runtime_stage_types_flatbuffers.h" +#include "third_party/abseil-cpp/absl/status/statusor.h" namespace impeller { @@ -22,7 +23,8 @@ class RuntimeStage { static const char* kVulkanUBOName; using Map = std::map>; - static Map DecodeRuntimeStages(const std::shared_ptr& payload); + static absl::StatusOr DecodeRuntimeStages( + const std::shared_ptr& payload); RuntimeStage(const fb::RuntimeStage* runtime_stage, const std::shared_ptr& payload); diff --git a/engine/src/flutter/impeller/runtime_stage/runtime_stage_types.fbs b/engine/src/flutter/impeller/runtime_stage/runtime_stage_types.fbs index dd9f7f5a66d..941184b7d6c 100644 --- a/engine/src/flutter/impeller/runtime_stage/runtime_stage_types.fbs +++ b/engine/src/flutter/impeller/runtime_stage/runtime_stage_types.fbs @@ -4,6 +4,13 @@ namespace impeller.fb; +// This is the runtime stage format version number - it's value should be +// incremented any time there is a change to this file which changes the +// resulting flat buffer format. +enum RuntimeStagesFormatVersion:uint32 { + kVersion = 1 +} + enum Stage:byte { kVertex, kFragment, @@ -12,7 +19,7 @@ enum Stage:byte { // The subset of impeller::ShaderType that may be used for uniform bindings. // kStruct is only supported for Impeller Vulkan. -// kFloat encompases multiple float-based types e.g. vec2. +// kFloat encompasses multiple float-based types e.g. vec2. enum UniformDataType:uint32 { kFloat, kSampledImage, @@ -85,4 +92,5 @@ table RuntimeStages { opengles: RuntimeStage; opengles3: RuntimeStage; vulkan: RuntimeStage; + format_version: uint32; // This is intended to hold RuntimeStagesFormatVersion.kVersion. } 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 8d6e466e04e..c26d8034cbc 100644 --- a/engine/src/flutter/impeller/runtime_stage/runtime_stage_unittests.cc +++ b/engine/src/flutter/impeller/runtime_stage/runtime_stage_unittests.cc @@ -18,7 +18,9 @@ #include "impeller/renderer/pipeline_library.h" #include "impeller/renderer/shader_library.h" #include "impeller/runtime_stage/runtime_stage.h" +#include "impeller/runtime_stage/runtime_stage_flatbuffers.h" #include "impeller/runtime_stage/runtime_stage_playground.h" +#include "runtime_stage_types_flatbuffers.h" namespace impeller { namespace testing { @@ -32,11 +34,26 @@ TEST_P(RuntimeStageTest, CanReadValidBlob) { ASSERT_TRUE(fixture); ASSERT_GT(fixture->GetSize(), 0u); auto stages = RuntimeStage::DecodeRuntimeStages(fixture); - auto stage = stages[PlaygroundBackendToRuntimeStageBackend(GetBackend())]; + ASSERT_TRUE(stages.ok()); + auto stage = + stages.value()[PlaygroundBackendToRuntimeStageBackend(GetBackend())]; ASSERT_TRUE(stage->IsValid()); ASSERT_EQ(stage->GetShaderStage(), RuntimeShaderStage::kFragment); } +TEST_P(RuntimeStageTest, RejectInvalidFormatVersion) { + flatbuffers::FlatBufferBuilder builder; + fb::RuntimeStagesBuilder stages_builder(builder); + stages_builder.add_format_version(0); + auto stages = stages_builder.Finish(); + builder.Finish(stages, fb::RuntimeStagesIdentifier()); + auto mapping = std::make_shared( + builder.GetBufferPointer(), builder.GetSize()); + auto runtime_stages = RuntimeStage::DecodeRuntimeStages(mapping); + EXPECT_FALSE(runtime_stages.ok()); + EXPECT_EQ(runtime_stages.status().code(), absl::StatusCode::kInvalidArgument); +} + TEST_P(RuntimeStageTest, CanRejectInvalidBlob) { ScopedValidationDisable disable_validation; const std::shared_ptr fixture = @@ -50,7 +67,7 @@ TEST_P(RuntimeStageTest, CanRejectInvalidBlob) { junk_allocation->GetLength().GetByteSize()); auto stages = RuntimeStage::DecodeRuntimeStages( CreateMappingFromAllocation(junk_allocation)); - ASSERT_FALSE(stages[PlaygroundBackendToRuntimeStageBackend(GetBackend())]); + ASSERT_FALSE(stages.ok()); } TEST_P(RuntimeStageTest, CanReadUniforms) { @@ -59,7 +76,9 @@ TEST_P(RuntimeStageTest, CanReadUniforms) { ASSERT_TRUE(fixture); ASSERT_GT(fixture->GetSize(), 0u); auto stages = RuntimeStage::DecodeRuntimeStages(fixture); - auto stage = stages[PlaygroundBackendToRuntimeStageBackend(GetBackend())]; + ASSERT_TRUE(stages.ok()); + auto stage = + stages.value()[PlaygroundBackendToRuntimeStageBackend(GetBackend())]; ASSERT_TRUE(stage->IsValid()); switch (GetBackend()) { @@ -237,7 +256,9 @@ TEST_P(RuntimeStageTest, CanReadUniformsSamplerBeforeUBO) { ASSERT_TRUE(fixture); ASSERT_GT(fixture->GetSize(), 0u); auto stages = RuntimeStage::DecodeRuntimeStages(fixture); - auto stage = stages[PlaygroundBackendToRuntimeStageBackend(GetBackend())]; + ASSERT_TRUE(stages.ok()); + auto stage = + stages.value()[PlaygroundBackendToRuntimeStageBackend(GetBackend())]; EXPECT_EQ(stage->GetUniforms().size(), 2u); auto uni = stage->GetUniform(RuntimeStage::kVulkanUBOName); @@ -262,7 +283,9 @@ TEST_P(RuntimeStageTest, CanReadUniformsSamplerAfterUBO) { ASSERT_TRUE(fixture); ASSERT_GT(fixture->GetSize(), 0u); auto stages = RuntimeStage::DecodeRuntimeStages(fixture); - auto stage = stages[PlaygroundBackendToRuntimeStageBackend(GetBackend())]; + ASSERT_TRUE(stages.ok()); + auto stage = + stages.value()[PlaygroundBackendToRuntimeStageBackend(GetBackend())]; EXPECT_EQ(stage->GetUniforms().size(), 2u); auto uni = stage->GetUniform(RuntimeStage::kVulkanUBOName); @@ -283,7 +306,9 @@ TEST_P(RuntimeStageTest, CanRegisterStage) { ASSERT_TRUE(fixture); ASSERT_GT(fixture->GetSize(), 0u); auto stages = RuntimeStage::DecodeRuntimeStages(fixture); - auto stage = stages[PlaygroundBackendToRuntimeStageBackend(GetBackend())]; + ASSERT_TRUE(stages.ok()); + auto stage = + stages.value()[PlaygroundBackendToRuntimeStageBackend(GetBackend())]; ASSERT_TRUE(stage->IsValid()); std::promise registration; auto future = registration.get_future(); diff --git a/engine/src/flutter/impeller/shader_archive/BUILD.gn b/engine/src/flutter/impeller/shader_archive/BUILD.gn index 7e749b93722..dc23d67e6d0 100644 --- a/engine/src/flutter/impeller/shader_archive/BUILD.gn +++ b/engine/src/flutter/impeller/shader_archive/BUILD.gn @@ -29,6 +29,7 @@ impeller_component("shader_archive") { ":shader_archive_flatbuffers", "../base", "//flutter/fml", + "//third_party/abseil-cpp/absl/status:statusor", ] } diff --git a/engine/src/flutter/impeller/shader_archive/shader_archive.cc b/engine/src/flutter/impeller/shader_archive/shader_archive.cc index 4cf2d3e1b56..7c8ee3bcd57 100644 --- a/engine/src/flutter/impeller/shader_archive/shader_archive.cc +++ b/engine/src/flutter/impeller/shader_archive/shader_archive.cc @@ -5,6 +5,7 @@ #include "impeller/shader_archive/shader_archive.h" #include +#include #include #include @@ -25,48 +26,56 @@ constexpr ArchiveShaderType ToShaderType(fb::Stage stage) { FML_UNREACHABLE(); } -ShaderArchive::ShaderArchive(std::shared_ptr payload) - : payload_(std::move(payload)) { - if (!payload_ || payload_->GetMapping() == nullptr) { - VALIDATION_LOG << "Shader mapping was absent."; - return; +absl::StatusOr ShaderArchive::Create( + std::shared_ptr payload) { + if (!payload || payload->GetMapping() == nullptr) { + return absl::InvalidArgumentError("Shader mapping was absent."); } - if (!fb::ShaderArchiveBufferHasIdentifier(payload_->GetMapping())) { - VALIDATION_LOG << "Invalid shader magic."; - return; + if (!fb::ShaderArchiveBufferHasIdentifier(payload->GetMapping())) { + return absl::InvalidArgumentError("Invalid shader magic."); } - auto shader_archive = fb::GetShaderArchive(payload_->GetMapping()); + auto shader_archive = fb::GetShaderArchive(payload->GetMapping()); if (!shader_archive) { - return; + return absl::InvalidArgumentError("Could not read shader archive."); } + const auto version = shader_archive->format_version(); + const auto expected = + static_cast(fb::ShaderArchiveFormatVersion::kVersion); + if (version != expected) { + std::stringstream stream; + stream << "Unsupported shader archive format version. Expected: " + << expected << ", Got: " << version; + return absl::InvalidArgumentError(stream.str()); + } + + Shaders shaders; if (auto items = shader_archive->items()) { for (auto i = items->begin(), end = items->end(); i != end; i++) { ShaderKey key; key.name = i->name()->str(); key.type = ToShaderType(i->stage()); - shaders_[key] = std::make_shared( - i->mapping()->Data(), i->mapping()->size(), - [payload = payload_](auto, auto) { + shaders[key] = std::make_shared( + i->mapping()->Data(), i->mapping()->size(), [payload](auto, auto) { // The pointers are into the base payload. Instead of copying the // data, just hold onto the payload. }); } } - is_valid_ = true; + return ShaderArchive(std::move(payload), std::move(shaders)); } +ShaderArchive::ShaderArchive(std::shared_ptr payload, + Shaders shaders) + : payload_(std::move(payload)), shaders_(std::move(shaders)) {} + ShaderArchive::ShaderArchive(ShaderArchive&&) = default; ShaderArchive::~ShaderArchive() = default; -bool ShaderArchive::IsValid() const { - return is_valid_; -} - size_t ShaderArchive::GetShaderCount() const { return shaders_.size(); } @@ -86,7 +95,7 @@ size_t ShaderArchive::IterateAllShaders( const std::string& name, const std::shared_ptr& mapping)>& callback) const { - if (!IsValid() || !callback) { + if (!callback) { return 0u; } size_t count = 0u; diff --git a/engine/src/flutter/impeller/shader_archive/shader_archive.fbs b/engine/src/flutter/impeller/shader_archive/shader_archive.fbs index 0077f54a353..d8ac2d1d34f 100644 --- a/engine/src/flutter/impeller/shader_archive/shader_archive.fbs +++ b/engine/src/flutter/impeller/shader_archive/shader_archive.fbs @@ -4,6 +4,13 @@ namespace impeller.fb; +// This is the shader archive format version number - it's value should be +// incremented any time there is a change to this file which changes the +// resulting flat buffer format. +enum ShaderArchiveFormatVersion:uint32 { + kVersion = 1 +} + enum Stage:byte { kVertex, kFragment, @@ -18,6 +25,7 @@ table ShaderBlob { table ShaderArchive { items: [ShaderBlob]; + format_version: uint32; // This is intended to hold ShaderArchiveFormatVersion.kVersion. } root_type ShaderArchive; diff --git a/engine/src/flutter/impeller/shader_archive/shader_archive.h b/engine/src/flutter/impeller/shader_archive/shader_archive.h index dc9c243785c..1fcae5cc04f 100644 --- a/engine/src/flutter/impeller/shader_archive/shader_archive.h +++ b/engine/src/flutter/impeller/shader_archive/shader_archive.h @@ -12,19 +12,19 @@ #include "flutter/fml/hash_combine.h" #include "flutter/fml/mapping.h" #include "impeller/shader_archive/shader_archive_types.h" +#include "third_party/abseil-cpp/absl/status/statusor.h" namespace impeller { class ShaderArchive { public: - explicit ShaderArchive(std::shared_ptr payload); + static absl::StatusOr Create( + std::shared_ptr payload); ShaderArchive(ShaderArchive&&); ~ShaderArchive(); - bool IsValid() const; - size_t GetShaderCount() const; std::shared_ptr GetMapping(ArchiveShaderType type, @@ -37,6 +37,8 @@ class ShaderArchive { const; private: + explicit ShaderArchive(std::shared_ptr payload); + struct ShaderKey { ArchiveShaderType type = ArchiveShaderType::kFragment; std::string name; @@ -63,7 +65,8 @@ class ShaderArchive { std::shared_ptr payload_; Shaders shaders_; - bool is_valid_ = false; + + ShaderArchive(std::shared_ptr payload, Shaders shaders); ShaderArchive(const ShaderArchive&) = delete; diff --git a/engine/src/flutter/impeller/shader_archive/shader_archive_unittests.cc b/engine/src/flutter/impeller/shader_archive/shader_archive_unittests.cc index ba4a3072c91..eeaada19744 100644 --- a/engine/src/flutter/impeller/shader_archive/shader_archive_unittests.cc +++ b/engine/src/flutter/impeller/shader_archive/shader_archive_unittests.cc @@ -2,11 +2,13 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +#include #include #include "flutter/fml/mapping.h" #include "flutter/testing/testing.h" #include "impeller/shader_archive/shader_archive.h" +#include "impeller/shader_archive/shader_archive_flatbuffers.h" #include "impeller/shader_archive/shader_archive_writer.h" namespace impeller { @@ -41,17 +43,39 @@ TEST(ShaderArchiveTest, CanReadAndWriteBlobs) { auto mapping = writer.CreateMapping(); ASSERT_NE(mapping, nullptr); - ShaderArchive library(mapping); - ASSERT_TRUE(library.IsValid()); - ASSERT_EQ(library.GetShaderCount(), 5u); + auto library = ShaderArchive::Create(mapping); + ASSERT_TRUE(library.ok()); + ASSERT_EQ(library->GetShaderCount(), 5u); // Wrong type. - ASSERT_EQ(library.GetMapping(ArchiveShaderType::kFragment, "Hello"), nullptr); + ASSERT_EQ(library->GetMapping(ArchiveShaderType::kFragment, "Hello"), + nullptr); - auto hello_vtx = library.GetMapping(ArchiveShaderType::kVertex, "Hello"); + auto hello_vtx = library->GetMapping(ArchiveShaderType::kVertex, "Hello"); ASSERT_NE(hello_vtx, nullptr); ASSERT_EQ(CreateStringFromMapping(*hello_vtx), "World"); } +TEST(ShaderArchiveTest, ReturnsErrorOnInvalidVersion) { + fb::ShaderArchiveT shader_archive; + shader_archive.format_version = -1; + + auto builder = std::make_shared(); + builder->Finish(fb::ShaderArchive::Pack(*builder.get(), &shader_archive), + fb::ShaderArchiveIdentifier()); + auto mapping = std::make_shared( + builder->GetBufferPointer(), builder->GetSize(), + [builder](auto, auto) {}); + + auto library = ShaderArchive::Create(mapping); + ASSERT_FALSE(library.ok()); + ASSERT_EQ(library.status().code(), absl::StatusCode::kInvalidArgument); + std::stringstream stream; + stream << "Unsupported shader archive format version. Expected: " + << static_cast(fb::ShaderArchiveFormatVersion::kVersion) + << ", Got: 4294967295"; + ASSERT_EQ(library.status().message(), stream.str()); +} + } // namespace testing } // namespace impeller diff --git a/engine/src/flutter/impeller/shader_archive/shader_archive_writer.cc b/engine/src/flutter/impeller/shader_archive/shader_archive_writer.cc index 070c5c23d76..95482877d35 100644 --- a/engine/src/flutter/impeller/shader_archive/shader_archive_writer.cc +++ b/engine/src/flutter/impeller/shader_archive/shader_archive_writer.cc @@ -106,6 +106,8 @@ constexpr fb::Stage ToStage(ArchiveShaderType type) { std::shared_ptr ShaderArchiveWriter::CreateMapping() const { fb::ShaderArchiveT shader_archive; + shader_archive.format_version = + static_cast(fb::ShaderArchiveFormatVersion::kVersion); for (const auto& shader_description : shader_descriptions_) { auto mapping = shader_description.mapping; if (!mapping) { diff --git a/engine/src/flutter/impeller/shader_bundle/shader_bundle.fbs b/engine/src/flutter/impeller/shader_bundle/shader_bundle.fbs index 490312223d5..e63876dc76a 100644 --- a/engine/src/flutter/impeller/shader_bundle/shader_bundle.fbs +++ b/engine/src/flutter/impeller/shader_bundle/shader_bundle.fbs @@ -4,6 +4,13 @@ namespace impeller.fb.shaderbundle; +// This is the shader bundle format version number - it's value should be +// incremented any time there is a change to this file which changes the +// resulting flat buffer format. +enum ShaderBundleFormatVersion:uint32 { + kVersion = 1 +} + enum ShaderStage:byte { kVertex, kFragment, @@ -102,6 +109,7 @@ table Shader { table ShaderBundle { shaders: [Shader]; + format_version: uint32; // This is intended to hold ShaderBundleFormatVersion.kVersion. } root_type ShaderBundle; diff --git a/engine/src/flutter/impeller/toolkit/interop/fragment_program.cc b/engine/src/flutter/impeller/toolkit/interop/fragment_program.cc index 593a84c9f99..bdc8c1c7f5f 100644 --- a/engine/src/flutter/impeller/toolkit/interop/fragment_program.cc +++ b/engine/src/flutter/impeller/toolkit/interop/fragment_program.cc @@ -15,8 +15,12 @@ FragmentProgram::FragmentProgram(const std::shared_ptr& data) { } auto stages = RuntimeStage::DecodeRuntimeStages(data); + if (!stages.ok()) { + VALIDATION_LOG << "Failed to decode runtime stages: " << stages.status(); + return; + } - for (const auto& stage : stages) { + for (const auto& stage : stages.value()) { if (auto data = stage.second) { stages_[stage.first] = std::move(data); } diff --git a/engine/src/flutter/lib/gpu/shader_library.cc b/engine/src/flutter/lib/gpu/shader_library.cc index b3fa5ec9969..2387bdaada9 100644 --- a/engine/src/flutter/lib/gpu/shader_library.cc +++ b/engine/src/flutter/lib/gpu/shader_library.cc @@ -185,6 +185,15 @@ fml::RefPtr ShaderLibrary::MakeFromFlatbuffer( return nullptr; } + const auto version = bundle->format_version(); + const auto expected = static_cast( + impeller::fb::shaderbundle::ShaderBundleFormatVersion::kVersion); + if (version != expected) { + VALIDATION_LOG << "Unsupported shader bundle format version: " << version + << ", expected: " << expected; + return nullptr; + } + ShaderLibrary::ShaderMap shader_map; for (const auto* bundled_shader : *bundle->shaders()) { diff --git a/engine/src/flutter/lib/ui/painting/fragment_program.cc b/engine/src/flutter/lib/ui/painting/fragment_program.cc index 379c33a49b8..ae705901f8f 100644 --- a/engine/src/flutter/lib/ui/painting/fragment_program.cc +++ b/engine/src/flutter/lib/ui/painting/fragment_program.cc @@ -112,7 +112,13 @@ std::string FragmentProgram::initFromAsset(const std::string& asset_name) { auto runtime_stages = impeller::RuntimeStage::DecodeRuntimeStages(std::move(data)); - if (runtime_stages.empty()) { + if (!runtime_stages.ok()) { + return std::string("Asset '") + asset_name + + std::string("' manifest could not be decoded: ") + + runtime_stages.status().ToString(); + } + + if (runtime_stages->empty()) { return std::string("Asset '") + asset_name + std::string("' does not contain any shader data."); } @@ -120,7 +126,7 @@ std::string FragmentProgram::initFromAsset(const std::string& asset_name) { impeller::RuntimeStageBackend backend = ui_dart_state->GetRuntimeStageBackend(); std::shared_ptr runtime_stage = - runtime_stages[backend]; + (*runtime_stages)[backend]; if (!runtime_stage) { std::ostringstream stream; stream << "Asset '" << asset_name @@ -128,7 +134,7 @@ std::string FragmentProgram::initFromAsset(const std::string& asset_name) { "backend (" << RuntimeStageBackendToString(backend) << ")." << std::endl << "Found stages: "; - for (const auto& kvp : runtime_stages) { + for (const auto& kvp : *runtime_stages) { if (kvp.second) { stream << RuntimeStageBackendToString(kvp.first) << " "; } diff --git a/engine/src/flutter/testing/dart/fragment_shader_test.dart b/engine/src/flutter/testing/dart/fragment_shader_test.dart index 11b3a1043c4..fabaeaee01c 100644 --- a/engine/src/flutter/testing/dart/fragment_shader_test.dart +++ b/engine/src/flutter/testing/dart/fragment_shader_test.dart @@ -25,7 +25,7 @@ void main() async { expect(rawData is Map, true); final data = rawData! as Map; - expect(data.keys.toList(), ['sksl']); + expect(data.keys.toList(), ['format_version', 'sksl']); expect(data['sksl'] is Map, true); final skslData = data['sksl']! as Map;