From 599a2be1459bccdb9db60cf80835c51f3756df1b Mon Sep 17 00:00:00 2001 From: Brandon DeRosier Date: Wed, 13 Mar 2024 17:27:23 -0700 Subject: [PATCH 1/2] Revert "[Impeller] stencil buffer record/replay instead of MSAA storage. (#47397)" This reverts commit 04329c5d00d7c36509c0f2bfaa8b7393fc9a08d7. --- impeller/entity/entity_pass.cc | 68 +++++++++----------------- impeller/entity/entity_pass.h | 23 --------- impeller/entity/inline_pass_context.cc | 13 ++++- impeller/entity/inline_pass_context.h | 2 +- 4 files changed, 35 insertions(+), 71 deletions(-) diff --git a/impeller/entity/entity_pass.cc b/impeller/entity/entity_pass.cc index 495b038273b68..061b38d6de0a9 100644 --- a/impeller/entity/entity_pass.cc +++ b/impeller/entity/entity_pass.cc @@ -238,15 +238,18 @@ void EntityPass::AddSubpassInline(std::unique_ptr pass) { pass->advanced_blend_reads_from_pass_texture_; } -static const constexpr RenderTarget::AttachmentConfig kDefaultStencilConfig = - RenderTarget::AttachmentConfig{ - .storage_mode = StorageMode::kDeviceTransient, - .load_action = LoadAction::kDontCare, - .store_action = StoreAction::kDontCare, - }; +static RenderTarget::AttachmentConfig GetDefaultStencilConfig(bool readable) { + return RenderTarget::AttachmentConfig{ + .storage_mode = readable ? StorageMode::kDevicePrivate + : StorageMode::kDeviceTransient, + .load_action = LoadAction::kDontCare, + .store_action = StoreAction::kDontCare, + }; +} static EntityPassTarget CreateRenderTarget(ContentContext& renderer, ISize size, + bool readable, const Color& clear_color) { auto context = renderer.GetContext(); @@ -267,8 +270,8 @@ static EntityPassTarget CreateRenderTarget(ContentContext& renderer, .resolve_storage_mode = StorageMode::kDevicePrivate, .load_action = LoadAction::kDontCare, .store_action = StoreAction::kMultisampleResolve, - .clear_color = clear_color}, // color_attachment_config - kDefaultStencilConfig // stencil_attachment_config + .clear_color = clear_color}, // color_attachment_config + GetDefaultStencilConfig(readable) // stencil_attachment_config ); } else { target = RenderTarget::CreateOffscreen( @@ -281,8 +284,8 @@ static EntityPassTarget CreateRenderTarget(ContentContext& renderer, .load_action = LoadAction::kDontCare, .store_action = StoreAction::kDontCare, .clear_color = clear_color, - }, // color_attachment_config - kDefaultStencilConfig // stencil_attachment_config + }, // color_attachment_config + GetDefaultStencilConfig(readable) // stencil_attachment_config ); } @@ -340,7 +343,7 @@ bool EntityPass::Render(ContentContext& renderer, // there's no need to set up a stencil attachment on the root render target. if (reads_from_onscreen_backdrop) { auto offscreen_target = CreateRenderTarget( - renderer, root_render_target.GetRenderTargetSize(), + renderer, root_render_target.GetRenderTargetSize(), true, GetClearColorOrDefault(render_target.GetRenderTargetSize())); if (!OnRender(renderer, // renderer @@ -442,7 +445,8 @@ bool EntityPass::Render(ContentContext& renderer, *renderer.GetContext(), *renderer.GetRenderTargetCache(), color0.texture->GetSize(), renderer.GetContext()->GetCapabilities()->SupportsOffscreenMSAA(), - "ImpellerOnscreen", kDefaultStencilConfig); + "ImpellerOnscreen", + GetDefaultStencilConfig(reads_from_onscreen_backdrop)); } // Set up the clear color of the root pass. @@ -479,10 +483,10 @@ EntityPass::EntityResult EntityPass::GetEntityForElement( //-------------------------------------------------------------------------- /// Setup entity element. /// + if (const auto& entity = std::get_if(&element)) { Entity element_entity = entity->Clone(); element_entity.SetCapture(capture.CreateChild("Entity")); - if (!global_pass_position.IsZero()) { // If the pass image is going to be rendered with a non-zero position, // apply the negative translation to entity copies before rendering them @@ -497,9 +501,11 @@ EntityPass::EntityResult EntityPass::GetEntityForElement( //-------------------------------------------------------------------------- /// Setup subpass element. /// - if (const auto& subpass_ptr = - std::get_if>(&element)) { + + else if (const auto& subpass_ptr = + std::get_if>(&element)) { auto subpass = subpass_ptr->get(); + if (subpass->delegate_->CanElide()) { return EntityPass::EntityResult::Skip(); } @@ -602,6 +608,7 @@ EntityPass::EntityResult EntityPass::GetEntityForElement( auto subpass_target = CreateRenderTarget( renderer, // renderer subpass_size, // size + subpass->GetTotalPassReads(renderer) > 0, // readable subpass->GetClearColorOrDefault(subpass_size)); // clear_color if (!subpass_target.IsValid()) { @@ -675,6 +682,7 @@ EntityPass::EntityResult EntityPass::GetEntityForElement( return EntityPass::EntityResult::Success(std::move(element_entity)); } + FML_UNREACHABLE(); } @@ -699,15 +707,6 @@ bool EntityPass::RenderElement(Entity& element_entity, // blit the non-MSAA resolve texture of the previous pass to MSAA textures // (let alone a transient one). if (result.backdrop_texture) { - // Restore any clips that were recorded before the backdrop filter was - // applied. - auto& replay_entities = clip_replay_->GetReplayEntities(); - for (const auto& entity : replay_entities) { - if (!entity.Render(renderer, *result.pass)) { - VALIDATION_LOG << "Failed to render entity for clip restore."; - } - } - auto size_rect = Rect::MakeSize(result.pass->GetRenderTargetSize()); auto msaa_backdrop_contents = TextureContents::MakeRect(size_rect); msaa_backdrop_contents->SetStencilEnabled(false); @@ -815,7 +814,6 @@ bool EntityPass::RenderElement(Entity& element_entity, #endif element_entity.SetClipDepth(element_entity.GetClipDepth() - clip_depth_floor); - clip_replay_->RecordEntity(element_entity, clip_coverage.type); if (!element_entity.Render(renderer, *result.pass)) { VALIDATION_LOG << "Failed to render entity."; return false; @@ -1167,24 +1165,4 @@ void EntityPass::SetEnableOffscreenCheckerboard(bool enabled) { enable_offscreen_debug_checkerboard_ = enabled; } -EntityPassClipRecorder::EntityPassClipRecorder() {} - -void EntityPassClipRecorder::RecordEntity(const Entity& entity, - Contents::ClipCoverage::Type type) { - switch (type) { - case Contents::ClipCoverage::Type::kNoChange: - return; - case Contents::ClipCoverage::Type::kAppend: - rendered_clip_entities_.push_back(entity.Clone()); - break; - case Contents::ClipCoverage::Type::kRestore: - rendered_clip_entities_.pop_back(); - break; - } -} - -const std::vector& EntityPassClipRecorder::GetReplayEntities() const { - return rendered_clip_entities_; -} - } // namespace impeller diff --git a/impeller/entity/entity_pass.h b/impeller/entity/entity_pass.h index 39f5449ff323b..37f7ea074a24f 100644 --- a/impeller/entity/entity_pass.h +++ b/impeller/entity/entity_pass.h @@ -21,7 +21,6 @@ namespace impeller { class ContentContext; -class EntityPassClipRecorder; class EntityPass { public: @@ -295,8 +294,6 @@ class EntityPass { bool flood_clip_ = false; bool enable_offscreen_debug_checkerboard_ = false; std::optional bounds_limit_; - std::unique_ptr clip_replay_ = - std::make_unique(); /// These values are incremented whenever something is added to the pass that /// requires reading from the backdrop texture. Currently, this can happen in @@ -321,26 +318,6 @@ class EntityPass { EntityPass& operator=(const EntityPass&) = delete; }; -/// @brief A class that tracks all clips that have been recorded in the current -/// entity pass stencil. -/// -/// These clips are replayed when restoring the backdrop so that the -/// stencil buffer is left in an identical state. -class EntityPassClipRecorder { - public: - EntityPassClipRecorder(); - - ~EntityPassClipRecorder() = default; - - /// @brief Record the entity based on the provided coverage [type]. - void RecordEntity(const Entity& entity, Contents::ClipCoverage::Type type); - - const std::vector& GetReplayEntities() const; - - private: - std::vector rendered_clip_entities_; -}; - } // namespace impeller #endif // FLUTTER_IMPELLER_ENTITY_ENTITY_PASS_H_ diff --git a/impeller/entity/inline_pass_context.cc b/impeller/entity/inline_pass_context.cc index a1904b743bce8..64227abe663f4 100644 --- a/impeller/entity/inline_pass_context.cc +++ b/impeller/entity/inline_pass_context.cc @@ -23,6 +23,7 @@ InlinePassContext::InlinePassContext( : context_(std::move(context)), pass_target_(pass_target), entity_count_(entity_count), + total_pass_reads_(pass_texture_reads), is_collapsed_(collapsed_parent_pass.has_value()) { if (collapsed_parent_pass.has_value()) { pass_ = collapsed_parent_pass.value().pass; @@ -141,9 +142,17 @@ InlinePassContext::RenderPassResult InlinePassContext::GetRenderPass( return {}; } - stencil->load_action = LoadAction::kClear; - stencil->store_action = StoreAction::kDontCare; + // Only clear the stencil if this is the very first pass of the + // layer. + stencil->load_action = + pass_count_ > 0 ? LoadAction::kLoad : LoadAction::kClear; + // If we're on the last pass of the layer, there's no need to store the + // stencil because nothing needs to read it. + stencil->store_action = pass_count_ == total_pass_reads_ + ? StoreAction::kDontCare + : StoreAction::kStore; pass_target_.target_.SetStencilAttachment(stencil.value()); + pass_target_.target_.SetColorAttachment(color0, 0); pass_ = command_buffer_->CreateRenderPass(pass_target_.GetRenderTarget()); diff --git a/impeller/entity/inline_pass_context.h b/impeller/entity/inline_pass_context.h index 595d291b5a1c8..7f49ba637fa61 100644 --- a/impeller/entity/inline_pass_context.h +++ b/impeller/entity/inline_pass_context.h @@ -51,7 +51,7 @@ class InlinePassContext { std::shared_ptr pass_; uint32_t pass_count_ = 0; uint32_t entity_count_ = 0; - + uint32_t total_pass_reads_ = 0; // Whether this context is collapsed into a parent entity pass. bool is_collapsed_ = false; From 86c02bdd38558b61610ddc3e61978f6242ef8407 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Thu, 14 Mar 2024 15:19:05 -0700 Subject: [PATCH 2/2] test --- testing/dart/painting_test.dart | 43 +++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/testing/dart/painting_test.dart b/testing/dart/painting_test.dart index 680f639a8481f..8c5061772f541 100644 --- a/testing/dart/painting_test.dart +++ b/testing/dart/painting_test.dart @@ -7,6 +7,8 @@ import 'dart:ui'; import 'package:litetest/litetest.dart'; +typedef CanvasCallback = void Function(Canvas canvas); + void main() { test('Vertices checks', () { try { @@ -60,4 +62,45 @@ void main() { indices: Uint16List.fromList(const [0, 2, 1, 2, 0, 1, 2, 0]), ).dispose(); }); + + test('BackdropFilter with multiple clips', () async { + // Regression test for https://github.com/flutter/flutter/issues/144211 + Picture makePicture(CanvasCallback callback) { + final PictureRecorder recorder = PictureRecorder(); + final Canvas canvas = Canvas(recorder); + callback(canvas); + return recorder.endRecording(); + } + final SceneBuilder sceneBuilder = SceneBuilder(); + + final Picture redClippedPicture = makePicture((Canvas canvas) { + canvas.clipRect(const Rect.fromLTRB(10, 10, 200, 200)); + canvas.clipRect(const Rect.fromLTRB(11, 10, 300, 200)); + canvas.drawPaint(Paint()..color = const Color(0xFFFF0000)); + }); + sceneBuilder.addPicture(Offset.zero, redClippedPicture); + + final Float64List matrix = Float64List(16); + sceneBuilder.pushBackdropFilter(ImageFilter.matrix(matrix)); + + final Picture whitePicture = makePicture((Canvas canvas) { + canvas.drawPaint(Paint()..color = const Color(0xFFFFFFFF)); + }); + sceneBuilder.addPicture(Offset.zero, whitePicture); + + final Scene scene = sceneBuilder.build(); + final Image image = scene.toImageSync(20, 20); + + final ByteData data = (await image.toByteData())!; + expect(data.buffer.asUint32List().length, 20 * 20); + // If clipping went wrong as in the linked issue, there will be red pixels. + for (final int color in data.buffer.asUint32List()) { + expect(color, 0xFFFFFFFF); + } + + scene.dispose(); + image.dispose(); + whitePicture.dispose(); + redClippedPicture.dispose(); + }); }