From 9eb8dc90f338331fb3cb24138f1e2e0380cadde8 Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Wed, 6 Dec 2023 10:00:07 -0800 Subject: [PATCH] [Impeller] disable entity culling by default. (flutter/engine#48717) From local testing across both flutter galleries, wonderous, some other test apps, the only time this code successfully culls is during the stretch overscroll (and we cull 1 or so entries). The cost of this culling is approximately 20% of entity rendering time, and about 5% of overall raster time. ![image](https://github.com/flutter/engine/assets/8975114/ef85629c-48a8-457f-8385-0c8136404571) --- .../impeller/entity/contents/clip_contents.cc | 7 +++---- .../impeller/entity/contents/clip_contents.h | 4 ++-- .../flutter/impeller/entity/contents/contents.cc | 3 +-- .../flutter/impeller/entity/contents/contents.h | 2 +- engine/src/flutter/impeller/entity/entity.cc | 4 ++++ .../flutter/impeller/entity/entity_unittests.cc | 14 ++++++++++++++ 6 files changed, 25 insertions(+), 9 deletions(-) diff --git a/engine/src/flutter/impeller/entity/contents/clip_contents.cc b/engine/src/flutter/impeller/entity/contents/clip_contents.cc index 8779ab9b44..514d42f5f0 100644 --- a/engine/src/flutter/impeller/entity/contents/clip_contents.cc +++ b/engine/src/flutter/impeller/entity/contents/clip_contents.cc @@ -62,9 +62,8 @@ Contents::ClipCoverage ClipContents::GetClipCoverage( FML_UNREACHABLE(); } -bool ClipContents::ShouldRender( - const Entity& entity, - const std::optional& clip_coverage) const { +bool ClipContents::ShouldRender(const Entity& entity, + const std::optional clip_coverage) const { return true; } @@ -161,7 +160,7 @@ Contents::ClipCoverage ClipRestoreContents::GetClipCoverage( bool ClipRestoreContents::ShouldRender( const Entity& entity, - const std::optional& clip_coverage) const { + const std::optional clip_coverage) const { return true; } diff --git a/engine/src/flutter/impeller/entity/contents/clip_contents.h b/engine/src/flutter/impeller/entity/contents/clip_contents.h index 410e1a7c2a..eaf156b018 100644 --- a/engine/src/flutter/impeller/entity/contents/clip_contents.h +++ b/engine/src/flutter/impeller/entity/contents/clip_contents.h @@ -35,7 +35,7 @@ class ClipContents final : public Contents { // |Contents| bool ShouldRender(const Entity& entity, - const std::optional& clip_coverage) const override; + const std::optional clip_coverage) const override; // |Contents| bool Render(const ContentContext& renderer, @@ -78,7 +78,7 @@ class ClipRestoreContents final : public Contents { // |Contents| bool ShouldRender(const Entity& entity, - const std::optional& clip_coverage) const override; + const std::optional clip_coverage) const override; // |Contents| bool Render(const ContentContext& renderer, diff --git a/engine/src/flutter/impeller/entity/contents/contents.cc b/engine/src/flutter/impeller/entity/contents/contents.cc index 06178fb601..58efcf7798 100644 --- a/engine/src/flutter/impeller/entity/contents/contents.cc +++ b/engine/src/flutter/impeller/entity/contents/contents.cc @@ -131,11 +131,10 @@ bool Contents::ApplyColorFilter( } bool Contents::ShouldRender(const Entity& entity, - const std::optional& clip_coverage) const { + const std::optional clip_coverage) const { if (!clip_coverage.has_value()) { return false; } - auto coverage = GetCoverage(entity); if (!coverage.has_value()) { return false; diff --git a/engine/src/flutter/impeller/entity/contents/contents.h b/engine/src/flutter/impeller/entity/contents/contents.h index 9c40846cff..d1dbb3349b 100644 --- a/engine/src/flutter/impeller/entity/contents/contents.h +++ b/engine/src/flutter/impeller/entity/contents/contents.h @@ -123,7 +123,7 @@ class Contents { const std::string& label = "Snapshot") const; virtual bool ShouldRender(const Entity& entity, - const std::optional& clip_coverage) const; + const std::optional clip_coverage) const; //---------------------------------------------------------------------------- /// @brief Return the color source's intrinsic size, if available. diff --git a/engine/src/flutter/impeller/entity/entity.cc b/engine/src/flutter/impeller/entity/entity.cc index 24bf0efa84..99f74727f0 100644 --- a/engine/src/flutter/impeller/entity/entity.cc +++ b/engine/src/flutter/impeller/entity/entity.cc @@ -71,7 +71,11 @@ Contents::ClipCoverage Entity::GetClipCoverage( } bool Entity::ShouldRender(const std::optional& clip_coverage) const { +#ifdef IMPELLER_CONTENT_CULLING return contents_->ShouldRender(*this, clip_coverage); +#else + return true; +#endif // IMPELLER_CONTENT_CULLING } void Entity::SetContents(std::shared_ptr contents) { diff --git a/engine/src/flutter/impeller/entity/entity_unittests.cc b/engine/src/flutter/impeller/entity/entity_unittests.cc index 520f7a1c20..be77915983 100644 --- a/engine/src/flutter/impeller/entity/entity_unittests.cc +++ b/engine/src/flutter/impeller/entity/entity_unittests.cc @@ -1605,6 +1605,20 @@ TEST_P(EntityTest, SolidFillShouldRenderIsCorrect) { } } +TEST_P(EntityTest, DoesNotCullEntitiesByDefault) { + auto fill = std::make_shared(); + fill->SetColor(Color::CornflowerBlue()); + fill->SetGeometry( + Geometry::MakeRect(Rect::MakeLTRB(-1000, -1000, -900, -900))); + + Entity entity; + entity.SetContents(fill); + + // Even though the entity is offscreen, this should still render because we do + // not compute the coverage intersection by default. + EXPECT_TRUE(entity.ShouldRender(Rect::MakeLTRB(0, 0, 100, 100))); +} + TEST_P(EntityTest, ClipContentsShouldRenderIsCorrect) { // For clip ops, `ShouldRender` should always return true.