diff --git a/flow/layers/physical_shape_layer.cc b/flow/layers/physical_shape_layer.cc index 06661898cb5ba..e4d7954eb6f2a 100644 --- a/flow/layers/physical_shape_layer.cc +++ b/flow/layers/physical_shape_layer.cc @@ -56,18 +56,24 @@ void PhysicalShapeLayer::Preroll(PrerollContext* context, Layer::AutoPrerollSaveLayerState save = Layer::AutoPrerollSaveLayerState::Create(context, UsesSaveLayer()); - SkRect child_paint_bounds; + SkRect child_paint_bounds = SkRect::MakeEmpty(); PrerollChildren(context, matrix, &child_paint_bounds); + SkRect paint_bounds; if (elevation_ == 0) { - set_paint_bounds(path_.getBounds()); + paint_bounds = path_.getBounds(); } else { // We will draw the shadow in Paint(), so add some margin to the paint - // bounds to leave space for the shadow. We fill this whole region and clip - // children to it so we don't need to join the child paint bounds. - set_paint_bounds(DisplayListCanvasDispatcher::ComputeShadowBounds( - path_, elevation_, context->frame_device_pixel_ratio, matrix)); + // bounds to leave space for the shadow. + paint_bounds = DisplayListCanvasDispatcher::ComputeShadowBounds( + path_, elevation_, context->frame_device_pixel_ratio, matrix); } + + if (clip_behavior_ == Clip::none) { + paint_bounds.join(child_paint_bounds); + } + + set_paint_bounds(paint_bounds); } void PhysicalShapeLayer::Paint(PaintContext& context) const { diff --git a/flow/layers/physical_shape_layer_unittests.cc b/flow/layers/physical_shape_layer_unittests.cc index 133ce35f33b85..9c78cf272bb78 100644 --- a/flow/layers/physical_shape_layer_unittests.cc +++ b/flow/layers/physical_shape_layer_unittests.cc @@ -66,7 +66,7 @@ TEST_F(PhysicalShapeLayerTest, NonEmptyLayer) { 0, MockCanvas::DrawPathData{layer_path, layer_paint}}})); } -TEST_F(PhysicalShapeLayerTest, ChildrenLargerThanPath) { +TEST_F(PhysicalShapeLayerTest, ChildrenLargerThanPathClip) { SkPath layer_path; layer_path.addRect(5.0f, 6.0f, 20.5f, 21.5f); SkPath child1_path; @@ -83,11 +83,11 @@ TEST_F(PhysicalShapeLayerTest, ChildrenLargerThanPath) { auto layer = std::make_shared(SK_ColorGREEN, SK_ColorBLACK, 0.0f, // elevation - layer_path, Clip::none); + layer_path, Clip::hardEdge); layer->Add(child1); layer->Add(child2); - SkRect child_paint_bounds; + SkRect child_paint_bounds = SkRect::MakeEmpty(); layer->Preroll(preroll_context(), SkMatrix()); child_paint_bounds.join(child1->paint_bounds()); child_paint_bounds.join(child2->paint_bounds()); @@ -95,6 +95,62 @@ TEST_F(PhysicalShapeLayerTest, ChildrenLargerThanPath) { EXPECT_NE(layer->paint_bounds(), child_paint_bounds); EXPECT_TRUE(layer->needs_painting(paint_context())); + SkPaint layer_paint; + layer_paint.setColor(SK_ColorGREEN); + layer_paint.setAntiAlias(true); + SkPaint child1_paint; + child1_paint.setColor(SK_ColorRED); + child1_paint.setAntiAlias(true); + SkPaint child2_paint; + child2_paint.setColor(SK_ColorBLUE); + child2_paint.setAntiAlias(true); + layer->Paint(paint_context()); + EXPECT_EQ(mock_canvas().draw_calls(), + std::vector({ + MockCanvas::DrawCall{ + 0, MockCanvas::DrawPathData{layer_path, layer_paint}}, + MockCanvas::DrawCall{0, MockCanvas::SaveData{1}}, + MockCanvas::DrawCall{ + 1, MockCanvas::ClipRectData{layer_path.getBounds(), + SkClipOp::kIntersect}}, + MockCanvas::DrawCall{ + 1, MockCanvas::DrawPathData{child1_path, child1_paint}}, + MockCanvas::DrawCall{ + 1, MockCanvas::DrawPathData{child2_path, child2_paint}}, + MockCanvas::DrawCall{1, MockCanvas::RestoreData{0}}, + })); +} + +TEST_F(PhysicalShapeLayerTest, ChildrenLargerThanPathNoClip) { + SkPath layer_path; + layer_path.addRect(5.0f, 6.0f, 20.5f, 21.5f); + SkPath child1_path; + child1_path.addRect(4, 0, 12, 12).close(); + SkPath child2_path; + child2_path.addRect(3, 2, 5, 15).close(); + auto child1 = std::make_shared(SK_ColorRED, SK_ColorBLACK, + 0.0f, // elevation + child1_path, Clip::none); + auto child2 = + std::make_shared(SK_ColorBLUE, SK_ColorBLACK, + 0.0f, // elevation + child2_path, Clip::none); + auto layer = + std::make_shared(SK_ColorGREEN, SK_ColorBLACK, + 0.0f, // elevation + layer_path, Clip::none); + layer->Add(child1); + layer->Add(child2); + + SkRect total_bounds = SkRect::MakeEmpty(); + layer->Preroll(preroll_context(), SkMatrix()); + total_bounds.join(child1->paint_bounds()); + total_bounds.join(child2->paint_bounds()); + total_bounds.join(layer_path.getBounds()); + EXPECT_NE(layer->paint_bounds(), layer_path.getBounds()); + EXPECT_EQ(layer->paint_bounds(), total_bounds); + EXPECT_TRUE(layer->needs_painting(paint_context())); + SkPaint layer_paint; layer_paint.setColor(SK_ColorGREEN); layer_paint.setAntiAlias(true); @@ -174,10 +230,14 @@ TEST_F(PhysicalShapeLayerTest, ElevationComplex) { // On Fuchsia, the system compositor handles all elevated // PhysicalShapeLayers and their shadows , so we do not do any painting // there. - EXPECT_EQ(layers[i]->paint_bounds(), - (DisplayListCanvasDispatcher::ComputeShadowBounds( - layer_path, initial_elevations[i], 1.0f /* pixel_ratio */, - SkMatrix()))); + SkRect paint_bounds = DisplayListCanvasDispatcher::ComputeShadowBounds( + layer_path, initial_elevations[i], 1.0f /* pixel_ratio */, SkMatrix()); + + // Without clipping the children will be painted as well + for (auto layer : layers[i]->layers()) { + paint_bounds.join(layer->paint_bounds()); + } + EXPECT_EQ(layers[i]->paint_bounds(), paint_bounds); EXPECT_TRUE(layers[i]->needs_painting(paint_context())); } diff --git a/shell/common/shell.cc b/shell/common/shell.cc index b0a7966327601..fae4b3fe182c0 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -878,10 +878,13 @@ void Shell::OnPlatformViewDestroyed() { fml::TaskRunner::RunNowOrPostTask(task_runners_.GetRasterTaskRunner(), raster_task); latch.Wait(); - // On Android, the external view embedder posts a task to the platform thread, - // and waits until it completes. - // As a result, the platform thread must not be blocked prior to calling - // this method. + // On Android, the external view embedder may post a task to the platform + // thread, and wait until it completes if overlay surfaces must be released. + // However, the platform thread might be blocked when Dart is initializing. + // In this situation, calling TeardownExternalViewEmbedder is safe because no + // platform views have been created before Flutter renders the first frame. + // Overall, the longer term plan is to remove this implementation once + // https://github.com/flutter/flutter/issues/96679 is fixed. rasterizer_->TeardownExternalViewEmbedder(); } diff --git a/shell/platform/android/external_view_embedder/external_view_embedder.cc b/shell/platform/android/external_view_embedder/external_view_embedder.cc index 84f2ab9378125..b9ee53bc5ab2d 100644 --- a/shell/platform/android/external_view_embedder/external_view_embedder.cc +++ b/shell/platform/android/external_view_embedder/external_view_embedder.cc @@ -309,6 +309,9 @@ void AndroidExternalViewEmbedder::Teardown() { // |ExternalViewEmbedder| void AndroidExternalViewEmbedder::DestroySurfaces() { + if (!surface_pool_->HasLayers()) { + return; + } fml::AutoResetWaitableEvent latch; fml::TaskRunner::RunNowOrPostTask(task_runners_.GetPlatformTaskRunner(), [&]() { diff --git a/shell/platform/android/external_view_embedder/external_view_embedder_unittests.cc b/shell/platform/android/external_view_embedder/external_view_embedder_unittests.cc index 57966440d0d05..7ff2a86592455 100644 --- a/shell/platform/android/external_view_embedder/external_view_embedder_unittests.cc +++ b/shell/platform/android/external_view_embedder/external_view_embedder_unittests.cc @@ -970,5 +970,16 @@ TEST(AndroidExternalViewEmbedder, Teardown) { embedder->Teardown(); } +TEST(AndroidExternalViewEmbedder, TeardownDoesNotCallJNIMethod) { + auto jni_mock = std::make_shared(); + auto android_context = + std::make_shared(AndroidRenderingAPI::kSoftware); + auto embedder = std::make_unique( + *android_context, jni_mock, nullptr, GetTaskRunnersForFixture()); + + EXPECT_CALL(*jni_mock, FlutterViewDestroyOverlaySurfaces()).Times(0); + embedder->Teardown(); +} + } // namespace testing } // namespace flutter diff --git a/shell/platform/android/external_view_embedder/surface_pool.cc b/shell/platform/android/external_view_embedder/surface_pool.cc index 27426c982869f..18afb1afb12e0 100644 --- a/shell/platform/android/external_view_embedder/surface_pool.cc +++ b/shell/platform/android/external_view_embedder/surface_pool.cc @@ -24,11 +24,11 @@ std::shared_ptr SurfacePool::GetLayer( const AndroidContext& android_context, std::shared_ptr jni_facade, std::shared_ptr surface_factory) { + std::lock_guard lock(mutex_); // Destroy current layers in the pool if the frame size has changed. if (requested_frame_size_ != current_frame_size_) { - DestroyLayers(jni_facade); + DestroyLayersLocked(jni_facade); } - intptr_t gr_context_key = reinterpret_cast(gr_context); // Allocate a new surface if there isn't one available. if (available_layer_index_ >= layers_.size()) { @@ -56,6 +56,7 @@ std::shared_ptr SurfacePool::GetLayer( layer->gr_context_key = gr_context_key; layers_.push_back(layer); } + std::shared_ptr layer = layers_[available_layer_index_]; // Since the surfaces are recycled, it's possible that the GrContext is // different. @@ -73,19 +74,33 @@ std::shared_ptr SurfacePool::GetLayer( } void SurfacePool::RecycleLayers() { + std::lock_guard lock(mutex_); available_layer_index_ = 0; } +bool SurfacePool::HasLayers() { + std::lock_guard lock(mutex_); + return layers_.size() > 0; +} + void SurfacePool::DestroyLayers( std::shared_ptr jni_facade) { - if (layers_.size() > 0) { - jni_facade->FlutterViewDestroyOverlaySurfaces(); + std::lock_guard lock(mutex_); + DestroyLayersLocked(jni_facade); +} + +void SurfacePool::DestroyLayersLocked( + std::shared_ptr jni_facade) { + if (layers_.size() == 0) { + return; } + jni_facade->FlutterViewDestroyOverlaySurfaces(); layers_.clear(); available_layer_index_ = 0; } std::vector> SurfacePool::GetUnusedLayers() { + std::lock_guard lock(mutex_); std::vector> results; for (size_t i = available_layer_index_; i < layers_.size(); i++) { results.push_back(layers_[i]); @@ -94,6 +109,7 @@ std::vector> SurfacePool::GetUnusedLayers() { } void SurfacePool::SetFrameSize(SkISize frame_size) { + std::lock_guard lock(mutex_); requested_frame_size_ = frame_size; } diff --git a/shell/platform/android/external_view_embedder/surface_pool.h b/shell/platform/android/external_view_embedder/surface_pool.h index 7f32104589dad..341cc819a4af3 100644 --- a/shell/platform/android/external_view_embedder/surface_pool.h +++ b/shell/platform/android/external_view_embedder/surface_pool.h @@ -5,6 +5,8 @@ #ifndef FLUTTER_SHELL_PLATFORM_ANDROID_EXTERNAL_VIEW_EMBEDDER_SURFACE_POOL_H_ #define FLUTTER_SHELL_PLATFORM_ANDROID_EXTERNAL_VIEW_EMBEDDER_SURFACE_POOL_H_ +#include + #include "flutter/flow/surface.h" #include "flutter/shell/platform/android/context/android_context.h" #include "flutter/shell/platform/android/surface/android_surface.h" @@ -41,7 +43,6 @@ struct OverlayLayer { intptr_t gr_context_key; }; -// This class isn't thread safe. class SurfacePool { public: SurfacePool(); @@ -72,6 +73,9 @@ class SurfacePool { // then they are deallocated as soon as |GetLayer| is called. void SetFrameSize(SkISize frame_size); + // Returns true if the current pool has layers in use. + bool HasLayers(); + private: // The index of the entry in the layers_ vector that determines the beginning // of the unused layers. For example, consider the following vector: @@ -95,6 +99,11 @@ class SurfacePool { // The frame size to be used by future layers. SkISize requested_frame_size_; + + // Used to guard public methods. + std::mutex mutex_; + + void DestroyLayersLocked(std::shared_ptr jni_facade); }; } // namespace flutter diff --git a/shell/platform/android/external_view_embedder/surface_pool_unittests.cc b/shell/platform/android/external_view_embedder/surface_pool_unittests.cc index caebb97f4fd03..ecf4e1a6c78e9 100644 --- a/shell/platform/android/external_view_embedder/surface_pool_unittests.cc +++ b/shell/platform/android/external_view_embedder/surface_pool_unittests.cc @@ -62,6 +62,7 @@ TEST(SurfacePool, GetLayerAllocateOneLayer) { auto layer = pool->GetLayer(gr_context.get(), *android_context, jni_mock, surface_factory); + ASSERT_TRUE(pool->HasLayers()); ASSERT_NE(nullptr, layer); ASSERT_EQ(reinterpret_cast(gr_context.get()), layer->gr_context_key); @@ -96,6 +97,7 @@ TEST(SurfacePool, GetUnusedLayers) { pool->RecycleLayers(); + ASSERT_TRUE(pool->HasLayers()); ASSERT_EQ(1UL, pool->GetUnusedLayers().size()); ASSERT_EQ(layer, pool->GetUnusedLayers()[0]); } @@ -137,6 +139,7 @@ TEST(SurfacePool, GetLayerRecycle) { auto layer_2 = pool->GetLayer(gr_context_2.get(), *android_context, jni_mock, surface_factory); + ASSERT_TRUE(pool->HasLayers()); ASSERT_NE(nullptr, layer_1); ASSERT_EQ(layer_1, layer_2); ASSERT_EQ(reinterpret_cast(gr_context_2.get()), @@ -176,6 +179,8 @@ TEST(SurfacePool, GetLayerAllocateTwoLayers) { surface_factory); auto layer_2 = pool->GetLayer(gr_context.get(), *android_context, jni_mock, surface_factory); + + ASSERT_TRUE(pool->HasLayers()); ASSERT_NE(nullptr, layer_1); ASSERT_NE(nullptr, layer_2); ASSERT_NE(layer_1, layer_2); @@ -213,9 +218,11 @@ TEST(SurfacePool, DestroyLayers) { pool->GetLayer(gr_context.get(), *android_context, jni_mock, surface_factory); EXPECT_CALL(*jni_mock, FlutterViewDestroyOverlaySurfaces()); + + ASSERT_TRUE(pool->HasLayers()); pool->DestroyLayers(jni_mock); - pool->RecycleLayers(); + ASSERT_FALSE(pool->HasLayers()); ASSERT_TRUE(pool->GetUnusedLayers().empty()); } @@ -246,8 +253,12 @@ TEST(SurfacePool, DestroyLayersFrameSizeChanged) { ByMove(std::make_unique( 0, window)))); + ASSERT_FALSE(pool->HasLayers()); + pool->GetLayer(gr_context.get(), *android_context, jni_mock, surface_factory); + ASSERT_TRUE(pool->HasLayers()); + pool->SetFrameSize(SkISize::Make(20, 20)); EXPECT_CALL(*jni_mock, FlutterViewDestroyOverlaySurfaces()).Times(1); EXPECT_CALL(*jni_mock, FlutterViewCreateOverlaySurface()) @@ -258,6 +269,7 @@ TEST(SurfacePool, DestroyLayersFrameSizeChanged) { pool->GetLayer(gr_context.get(), *android_context, jni_mock, surface_factory); ASSERT_TRUE(pool->GetUnusedLayers().empty()); + ASSERT_TRUE(pool->HasLayers()); } } // namespace testing