This repository was archived by the owner on Feb 25, 2025. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 6k
[Impeller] Fix hot reload for shaders on Impeller #49739
Merged
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,139 @@ | ||
| // 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 <cstdint> | ||
|
|
||
| #include "gtest/gtest.h" | ||
|
|
||
| #include "impeller/core/allocator.h" | ||
| #include "impeller/core/device_buffer_descriptor.h" | ||
| #include "impeller/entity/contents/content_context.h" | ||
| #include "impeller/geometry/color.h" | ||
| #include "impeller/renderer/capabilities.h" | ||
| #include "impeller/renderer/pipeline.h" | ||
| #include "impeller/renderer/pipeline_descriptor.h" | ||
|
|
||
| namespace impeller { | ||
| namespace testing { | ||
|
|
||
| class FakeAllocator : public Allocator { | ||
| public: | ||
| FakeAllocator() : Allocator() {} | ||
|
|
||
| uint16_t MinimumBytesPerRow(PixelFormat format) const override { return 0; } | ||
| ISize GetMaxTextureSizeSupported() const override { return ISize(); } | ||
|
|
||
| std::shared_ptr<DeviceBuffer> OnCreateBuffer( | ||
| const DeviceBufferDescriptor& desc) override { | ||
| return nullptr; | ||
| } | ||
| std::shared_ptr<Texture> OnCreateTexture( | ||
| const TextureDescriptor& desc) override { | ||
| return nullptr; | ||
| } | ||
| }; | ||
|
|
||
| class FakeContext : public Context { | ||
| public: | ||
| FakeContext() : Context(), allocator_(std::make_shared<FakeAllocator>()) {} | ||
|
|
||
| BackendType GetBackendType() const override { return BackendType::kVulkan; } | ||
| std::string DescribeGpuModel() const override { return ""; } | ||
| bool IsValid() const override { return false; } | ||
| const std::shared_ptr<const Capabilities>& GetCapabilities() const override { | ||
| return capabilities_; | ||
| } | ||
| std::shared_ptr<Allocator> GetResourceAllocator() const override { | ||
| return allocator_; | ||
| } | ||
| std::shared_ptr<ShaderLibrary> GetShaderLibrary() const { return nullptr; } | ||
| std::shared_ptr<SamplerLibrary> GetSamplerLibrary() const { return nullptr; } | ||
| std::shared_ptr<PipelineLibrary> GetPipelineLibrary() const { | ||
| return nullptr; | ||
| } | ||
| std::shared_ptr<CommandBuffer> CreateCommandBuffer() const { return nullptr; } | ||
| void Shutdown() {} | ||
|
|
||
| private: | ||
| std::shared_ptr<Allocator> allocator_; | ||
| std::shared_ptr<const Capabilities> capabilities_; | ||
| }; | ||
|
|
||
| class FakePipeline : public Pipeline<PipelineDescriptor> { | ||
| public: | ||
| FakePipeline() : Pipeline({}, PipelineDescriptor{}) {} | ||
|
|
||
| bool IsValid() const override { return false; } | ||
| }; | ||
|
|
||
| static std::shared_ptr<FakePipeline> CreateFakePipelineCallback() { | ||
| return std::make_shared<FakePipeline>(); | ||
| } | ||
|
|
||
| TEST(ContentContext, CachesPipelines) { | ||
| auto context = std::make_shared<FakeContext>(); | ||
| ContentContext content_context(context, nullptr); | ||
| ContentContextOptions optionsA{.blend_mode = BlendMode::kSourceOver}; | ||
| ContentContextOptions optionsB{.blend_mode = BlendMode::kSource}; | ||
|
|
||
| auto pipelineA = content_context.GetCachedRuntimeEffectPipeline( | ||
| "A", optionsA, CreateFakePipelineCallback); | ||
|
|
||
| auto pipelineA2 = content_context.GetCachedRuntimeEffectPipeline( | ||
| "A", optionsA, CreateFakePipelineCallback); | ||
|
|
||
| auto pipelineA3 = content_context.GetCachedRuntimeEffectPipeline( | ||
| "A", optionsB, CreateFakePipelineCallback); | ||
|
|
||
| auto pipelineB = content_context.GetCachedRuntimeEffectPipeline( | ||
| "B", optionsB, CreateFakePipelineCallback); | ||
|
|
||
| ASSERT_EQ(pipelineA.get(), pipelineA2.get()); | ||
| ASSERT_NE(pipelineA.get(), pipelineA3.get()); | ||
| ASSERT_NE(pipelineB.get(), pipelineA.get()); | ||
| } | ||
|
|
||
| TEST(ContentContext, InvalidatesAllPipelinesWithSameUniqueNameOnClear) { | ||
| auto context = std::make_shared<FakeContext>(); | ||
| ContentContext content_context(context, nullptr); | ||
| ContentContextOptions optionsA{.blend_mode = BlendMode::kSourceOver}; | ||
| ContentContextOptions optionsB{.blend_mode = BlendMode::kSource}; | ||
|
|
||
| auto pipelineA = content_context.GetCachedRuntimeEffectPipeline( | ||
| "A", optionsA, CreateFakePipelineCallback); | ||
|
|
||
| auto pipelineA2 = content_context.GetCachedRuntimeEffectPipeline( | ||
| "A", optionsB, CreateFakePipelineCallback); | ||
|
|
||
| auto pipelineB = content_context.GetCachedRuntimeEffectPipeline( | ||
| "B", optionsB, CreateFakePipelineCallback); | ||
|
|
||
| ASSERT_TRUE(pipelineA); | ||
| ASSERT_TRUE(pipelineA2); | ||
| ASSERT_TRUE(pipelineB); | ||
|
|
||
| ASSERT_EQ(pipelineA, content_context.GetCachedRuntimeEffectPipeline( | ||
| "A", optionsA, CreateFakePipelineCallback)); | ||
| ASSERT_EQ(pipelineA2, content_context.GetCachedRuntimeEffectPipeline( | ||
| "A", optionsB, CreateFakePipelineCallback)); | ||
| ASSERT_EQ(pipelineB, content_context.GetCachedRuntimeEffectPipeline( | ||
| "B", optionsB, CreateFakePipelineCallback)); | ||
|
|
||
| content_context.ClearCachedRuntimeEffectPipeline("A"); | ||
|
|
||
| ASSERT_NE(pipelineA, content_context.GetCachedRuntimeEffectPipeline( | ||
| "A", optionsA, CreateFakePipelineCallback)); | ||
| ASSERT_NE(pipelineA2, content_context.GetCachedRuntimeEffectPipeline( | ||
| "A", optionsB, CreateFakePipelineCallback)); | ||
| ASSERT_EQ(pipelineB, content_context.GetCachedRuntimeEffectPipeline( | ||
| "B", optionsB, CreateFakePipelineCallback)); | ||
|
|
||
| content_context.ClearCachedRuntimeEffectPipeline("B"); | ||
|
|
||
| ASSERT_NE(pipelineB, content_context.GetCachedRuntimeEffectPipeline( | ||
| "B", optionsB, CreateFakePipelineCallback)); | ||
| } | ||
|
|
||
| } // namespace testing | ||
| } // namespace impeller |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its possible that from frame to frame you will get different values for options, which means you may not invalidate all of the pipeline descriptors associated with this shader consider this scenario:
Frame 1: Use and cache shader with Rect geometry, triangle geometry is used.
Frame 2: Hot reload, Use shader with tessellated geometry, triangle strip is used. Pipeline descripor matching shader name + triangle strip is removed.
Frame 3: Hot reload, use shader with rect geometry, old pipeline descriptor from frame 1 is recycled.
You'd need to clear out all possible pipelines for a given shader variant. Since this is a debug only feature, you could iterate the cache and evict everything that has a name match. Or switch to a two level cache.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The shader comes in as dirty initially. That would mean evicting everything with the same name every time we got a new geometry, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its a one to many mapping of shaders to pipelines. So if the shader is dirty, then all pipelines (regardless of the ContentContextOptions must be marked dirty). You'd probably need to special case the first time you see a shader?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we really are worried about this case I think we need to just plain have a service extension that kills the cache. It's just hard to get that right with rendering so that the cache gets killed and some frame isn't in flight that puts the shader back into the cache before the new shader version comes through.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace frame numbers with two kinds of draws in the same frame. If you don't invalidate all pipeline descriptors marching the name, then only the first will get updated when you got reload
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah you're right, I have a sample program now that uses different blend mode.
Invalidating all the ones with the same name ... seems to work. Let's just do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I've updated the evict method to just evict by name rather than including the options, and written some tests for it. This works locally for hot reload with a simple app where the blend modes are different on the same shader.