From 3734ef3c235da16bfb8c6e0f74e0e809a385ba72 Mon Sep 17 00:00:00 2001 From: Emmanuel Garcia Date: Mon, 3 May 2021 14:50:59 -0700 Subject: [PATCH 1/7] Fix composition --- .../external_view_embedder.cc | 35 ++++++++----------- .../external_view_embedder.h | 2 -- 2 files changed, 15 insertions(+), 22 deletions(-) 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 c22462c02c7ad..caf7da14a17d9 100644 --- a/shell/platform/android/external_view_embedder/external_view_embedder.cc +++ b/shell/platform/android/external_view_embedder/external_view_embedder.cc @@ -104,6 +104,8 @@ void AndroidExternalViewEmbedder::SubmitFrame( overlay_layers.insert({view_id, {}}); sk_sp rtree = view_rtrees_.at(view_id); + SkRect joined_rect; + // Determinate if Flutter UI intersects with any of the previous // platform views stacked by z position. // @@ -116,33 +118,26 @@ void AndroidExternalViewEmbedder::SubmitFrame( // Each rect corresponds to a native view that renders Flutter UI. std::list intersection_rects = rtree->searchNonOverlappingDrawnRects(current_view_rect); - auto allocation_size = intersection_rects.size(); // Limit the number of native views, so it doesn't grow forever. // // In this case, the rects are merged into a single one that is the union // of all the rects. - if (allocation_size > kMaxLayerAllocations) { - SkRect joined_rect; - for (const SkRect& rect : intersection_rects) { - joined_rect.join(rect); - } - intersection_rects.clear(); - intersection_rects.push_back(joined_rect); - } - for (SkRect& intersection_rect : intersection_rects) { - // Subpixels in the platform may not align with the canvas subpixels. - // - // To workaround it, round the floating point bounds and make the rect - // slightly larger. For example, {0.3, 0.5, 3.1, 4.7} becomes {0, 0, 4, - // 5}. - intersection_rect.set(intersection_rect.roundOut()); - overlay_layers.at(view_id).push_back(intersection_rect); - // Clip the background canvas, so it doesn't contain any of the pixels - // drawn on the overlay layer. - background_canvas->clipRect(intersection_rect, SkClipOp::kDifference); + for (const SkRect& rect : intersection_rects) { + joined_rect.join(rect); } } + + // Subpixels in the platform may not align with the canvas subpixels. + // + // To workaround it, round the floating point bounds and make the rect + // slightly larger. For example, {0.3, 0.5, 3.1, 4.7} becomes {0, 0, 4, + // 5}. + joined_rect.set(joined_rect.roundOut()); + overlay_layers.at(view_id).push_back(joined_rect); + // Clip the background canvas, so it doesn't contain any of the pixels + // drawn on the overlay layer. + background_canvas->clipRect(joined_rect, SkClipOp::kDifference); background_canvas->drawPicture(pictures.at(view_id)); } // Submit the background canvas frame before switching the GL context to diff --git a/shell/platform/android/external_view_embedder/external_view_embedder.h b/shell/platform/android/external_view_embedder/external_view_embedder.h index 01b73ed04bac5..f456674be9a94 100644 --- a/shell/platform/android/external_view_embedder/external_view_embedder.h +++ b/shell/platform/android/external_view_embedder/external_view_embedder.h @@ -80,8 +80,6 @@ class AndroidExternalViewEmbedder final : public ExternalViewEmbedder { SkRect GetViewRect(int view_id) const; private: - static const int kMaxLayerAllocations = 2; - // The number of frames the rasterizer task runner will continue // to run on the platform thread after no platform view is rendered. // From 032d030502c13d6284cfc59122a3a59d0049b681 Mon Sep 17 00:00:00 2001 From: Emmanuel Garcia Date: Mon, 3 May 2021 17:46:42 -0700 Subject: [PATCH 2/7] Test --- .../external_view_embedder_unittests.cc | 96 +++++++++++++++++++ 1 file changed, 96 insertions(+) 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 9bbad774d3f1c..95830aee5351e 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 @@ -477,6 +477,102 @@ TEST(AndroidExternalViewEmbedder, SubmitFrame) { } } +TEST(AndroidExternalViewEmbedder, SubmitFrame__overlayComposition) { + auto jni_mock = std::make_shared(); + auto android_context = + std::make_shared(AndroidRenderingAPI::kSoftware); + + auto window = fml::MakeRefCounted(nullptr); + auto gr_context = GrDirectContext::MakeMock(nullptr); + auto frame_size = SkISize::Make(1000, 1000); + auto surface_factory = std::make_shared( + [&android_context, gr_context, window, frame_size]() { + auto surface_frame_1 = std::make_unique( + SkSurface::MakeNull(1000, 1000), false, + [](const SurfaceFrame& surface_frame, SkCanvas* canvas) { + return true; + }); + + auto surface_mock = std::make_unique(); + EXPECT_CALL(*surface_mock, AcquireFrame(frame_size)) + .Times(1 /* frames */) + .WillOnce(Return(ByMove(std::move(surface_frame_1)))); + + auto android_surface_mock = + std::make_unique(android_context); + EXPECT_CALL(*android_surface_mock, IsValid()).WillOnce(Return(true)); + + EXPECT_CALL(*android_surface_mock, CreateGPUSurface(gr_context.get())) + .WillOnce(Return(ByMove(std::move(surface_mock)))); + + EXPECT_CALL(*android_surface_mock, SetNativeWindow(window)); + return android_surface_mock; + }); + auto embedder = std::make_unique( + *android_context, jni_mock, surface_factory); + + auto raster_thread_merger = + GetThreadMergerFromPlatformThread(/*merged=*/true); + + EXPECT_CALL(*jni_mock, FlutterViewBeginFrame()); + embedder->BeginFrame(frame_size, nullptr, 1.5, raster_thread_merger); + + { + // Add first Android view. + SkMatrix matrix; + MutatorsStack stack; + stack.PushTransform(SkMatrix::Translate(0, 0)); + + embedder->PrerollCompositeEmbeddedView( + 0, std::make_unique(matrix, SkSize::Make(200, 200), + stack)); + EXPECT_CALL(*jni_mock, FlutterViewOnDisplayPlatformView(0, 0, 0, 200, 200, + 300, 300, stack)); + } + + auto rect_paint = SkPaint(); + rect_paint.setColor(SkColors::kCyan); + rect_paint.setStyle(SkPaint::Style::kFill_Style); + + // This simulates Flutter UI that intersects with the first Android view. + embedder->CompositeEmbeddedView(0)->drawRect( + SkRect::MakeXYWH(25, 25, 50, 150), rect_paint); + + { + // Add second Android view. + SkMatrix matrix; + MutatorsStack stack; + stack.PushTransform(SkMatrix::Translate(0, 100)); + + embedder->PrerollCompositeEmbeddedView( + 1, std::make_unique(matrix, SkSize::Make(100, 100), + stack)); + EXPECT_CALL(*jni_mock, FlutterViewOnDisplayPlatformView(1, 0, 0, 100, 100, + 150, 150, stack)); + } + // This simulates Flutter UI that intersects with the first and second Android + // views. + embedder->CompositeEmbeddedView(1)->drawRect(SkRect::MakeXYWH(25, 25, 50, 50), + rect_paint); + + EXPECT_CALL(*jni_mock, FlutterViewCreateOverlaySurface()) + .WillRepeatedly([&]() { + return std::make_unique( + 1, window); + }); + + EXPECT_CALL(*jni_mock, FlutterViewDisplayOverlaySurface(1, 25, 25, 50, 150)) + .Times(2); + + auto surface_frame = std::make_unique( + SkSurface::MakeNull(1000, 1000), false, + [](const SurfaceFrame& surface_frame, SkCanvas* canvas) mutable { + return true; + }); + + embedder->SubmitFrame(gr_context.get(), std::move(surface_frame), nullptr); +} + TEST(AndroidExternalViewEmbedder, DoesNotCallJNIPlatformThreadOnlyMethods) { auto jni_mock = std::make_shared(); From c01de95864aa3f3b022ddeb835c16211ac48ba66 Mon Sep 17 00:00:00 2001 From: Emmanuel Garcia Date: Mon, 3 May 2021 17:50:29 -0700 Subject: [PATCH 3/7] Cut sentence better --- .../android/external_view_embedder/external_view_embedder.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) 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 caf7da14a17d9..52151a8a5add0 100644 --- a/shell/platform/android/external_view_embedder/external_view_embedder.cc +++ b/shell/platform/android/external_view_embedder/external_view_embedder.cc @@ -131,8 +131,9 @@ void AndroidExternalViewEmbedder::SubmitFrame( // Subpixels in the platform may not align with the canvas subpixels. // // To workaround it, round the floating point bounds and make the rect - // slightly larger. For example, {0.3, 0.5, 3.1, 4.7} becomes {0, 0, 4, - // 5}. + // slightly larger. + // + // For example, {0.3, 0.5, 3.1, 4.7} becomes {0, 0, 4, 5}. joined_rect.set(joined_rect.roundOut()); overlay_layers.at(view_id).push_back(joined_rect); // Clip the background canvas, so it doesn't contain any of the pixels From 6c427091cda15a402a7bf6a5e022571691e36546 Mon Sep 17 00:00:00 2001 From: Emmanuel Garcia Date: Tue, 4 May 2021 14:39:59 -0700 Subject: [PATCH 4/7] Retrigger build From e82b340382655f6f1a185af1fa9a49986955215a Mon Sep 17 00:00:00 2001 From: Emmanuel Garcia Date: Tue, 11 May 2021 10:13:20 -0700 Subject: [PATCH 5/7] Maybe fixes test --- .../external_view_embedder_unittests.cc | 48 +++++++++---------- 1 file changed, 24 insertions(+), 24 deletions(-) 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 95830aee5351e..d2e64c53d1877 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 @@ -517,18 +517,16 @@ TEST(AndroidExternalViewEmbedder, SubmitFrame__overlayComposition) { EXPECT_CALL(*jni_mock, FlutterViewBeginFrame()); embedder->BeginFrame(frame_size, nullptr, 1.5, raster_thread_merger); - { - // Add first Android view. - SkMatrix matrix; - MutatorsStack stack; - stack.PushTransform(SkMatrix::Translate(0, 0)); - - embedder->PrerollCompositeEmbeddedView( - 0, std::make_unique(matrix, SkSize::Make(200, 200), - stack)); - EXPECT_CALL(*jni_mock, FlutterViewOnDisplayPlatformView(0, 0, 0, 200, 200, - 300, 300, stack)); - } + // Add first Android view. + SkMatrix matrix1; + MutatorsStack stack1; + stack1.PushTransform(SkMatrix::Translate(0, 0)); + + embedder->PrerollCompositeEmbeddedView( + 0, std::make_unique(matrix1, SkSize::Make(200, 200), + stack1)); + EXPECT_CALL(*jni_mock, FlutterViewOnDisplayPlatformView(0, 0, 0, 200, 200, + 300, 300, stack1)); auto rect_paint = SkPaint(); rect_paint.setColor(SkColors::kCyan); @@ -538,18 +536,17 @@ TEST(AndroidExternalViewEmbedder, SubmitFrame__overlayComposition) { embedder->CompositeEmbeddedView(0)->drawRect( SkRect::MakeXYWH(25, 25, 50, 150), rect_paint); - { - // Add second Android view. - SkMatrix matrix; - MutatorsStack stack; - stack.PushTransform(SkMatrix::Translate(0, 100)); - - embedder->PrerollCompositeEmbeddedView( - 1, std::make_unique(matrix, SkSize::Make(100, 100), - stack)); - EXPECT_CALL(*jni_mock, FlutterViewOnDisplayPlatformView(1, 0, 0, 100, 100, - 150, 150, stack)); - } + // Add second Android view. + SkMatrix matrix2; + MutatorsStack stack2; + stack2.PushTransform(SkMatrix::Translate(0, 100)); + + embedder->PrerollCompositeEmbeddedView( + 1, std::make_unique(matrix2, SkSize::Make(100, 100), + stack2)); + EXPECT_CALL(*jni_mock, FlutterViewOnDisplayPlatformView(1, 0, 0, 100, 100, + 150, 150, stack2)); + // This simulates Flutter UI that intersects with the first and second Android // views. embedder->CompositeEmbeddedView(1)->drawRect(SkRect::MakeXYWH(25, 25, 50, 50), @@ -571,6 +568,9 @@ TEST(AndroidExternalViewEmbedder, SubmitFrame__overlayComposition) { }); embedder->SubmitFrame(gr_context.get(), std::move(surface_frame), nullptr); + + EXPECT_CALL(*jni_mock, FlutterViewEndFrame()); + embedder->EndFrame(/*should_resubmit_frame=*/false, raster_thread_merger); } TEST(AndroidExternalViewEmbedder, DoesNotCallJNIPlatformThreadOnlyMethods) { From 389edb1b6ed3199e044079560cef5703eff6df59 Mon Sep 17 00:00:00 2001 From: Emmanuel Garcia Date: Tue, 11 May 2021 12:04:24 -0700 Subject: [PATCH 6/7] Add verbose gmock --- testing/run_tests.py | 1 + 1 file changed, 1 insertion(+) diff --git a/testing/run_tests.py b/testing/run_tests.py index f37904cee616a..dd4ed1428c295 100755 --- a/testing/run_tests.py +++ b/testing/run_tests.py @@ -124,6 +124,7 @@ def RunCCTests(build_dir, filter): ] shuffle_flags = non_repeatable_shuffle_flags + [ "--gtest_repeat=2", + "--gmock_verbose=info", ] RunEngineExecutable(build_dir, 'client_wrapper_glfw_unittests', filter, shuffle_flags) From a5c79b93ab254dfcf4725c54ed76eb1fbf353fe0 Mon Sep 17 00:00:00 2001 From: Emmanuel Garcia Date: Tue, 11 May 2021 13:49:40 -0700 Subject: [PATCH 7/7] Debug test --- .../external_view_embedder.cc | 31 +++++------ .../external_view_embedder_unittests.cc | 54 ++++++++++--------- testing/run_tests.py | 1 - 3 files changed, 46 insertions(+), 40 deletions(-) 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 52151a8a5add0..494b436dce6f4 100644 --- a/shell/platform/android/external_view_embedder/external_view_embedder.cc +++ b/shell/platform/android/external_view_embedder/external_view_embedder.cc @@ -84,7 +84,7 @@ void AndroidExternalViewEmbedder::SubmitFrame( return; } - std::unordered_map> overlay_layers; + std::unordered_map overlay_layers; std::unordered_map> pictures; SkCanvas* background_canvas = frame->SkiaCanvas(); auto current_frame_view_count = composition_order_.size(); @@ -101,10 +101,8 @@ void AndroidExternalViewEmbedder::SubmitFrame( FML_CHECK(picture); pictures.insert({view_id, picture}); - overlay_layers.insert({view_id, {}}); - sk_sp rtree = view_rtrees_.at(view_id); - SkRect joined_rect; + SkRect joined_rect = SkRect::MakeEmpty(); // Determinate if Flutter UI intersects with any of the previous // platform views stacked by z position. @@ -128,6 +126,10 @@ void AndroidExternalViewEmbedder::SubmitFrame( } } + if (joined_rect.isEmpty()) { + continue; + } + // Subpixels in the platform may not align with the canvas subpixels. // // To workaround it, round the floating point bounds and make the rect @@ -135,7 +137,7 @@ void AndroidExternalViewEmbedder::SubmitFrame( // // For example, {0.3, 0.5, 3.1, 4.7} becomes {0, 0, 4, 5}. joined_rect.set(joined_rect.roundOut()); - overlay_layers.at(view_id).push_back(joined_rect); + overlay_layers.insert({view_id, joined_rect}); // Clip the background canvas, so it doesn't contain any of the pixels // drawn on the overlay layer. background_canvas->clipRect(joined_rect, SkClipOp::kDifference); @@ -166,16 +168,15 @@ void AndroidExternalViewEmbedder::SubmitFrame( params.sizePoints().height() * device_pixel_ratio_, params.mutatorsStack() // ); - for (const SkRect& overlay_rect : overlay_layers.at(view_id)) { - std::unique_ptr frame = - CreateSurfaceIfNeeded(context, // - view_id, // - pictures.at(view_id), // - overlay_rect // - ); - if (should_submit_current_frame) { - frame->Submit(); - } + const SkRect& overlay_rect = overlay_layers.at(view_id); + std::unique_ptr frame = + CreateSurfaceIfNeeded(context, // + view_id, // + pictures.at(view_id), // + overlay_rect // + ); + if (should_submit_current_frame) { + frame->Submit(); } } } 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 d2e64c53d1877..f3f36264c71e0 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 @@ -517,16 +517,18 @@ TEST(AndroidExternalViewEmbedder, SubmitFrame__overlayComposition) { EXPECT_CALL(*jni_mock, FlutterViewBeginFrame()); embedder->BeginFrame(frame_size, nullptr, 1.5, raster_thread_merger); - // Add first Android view. - SkMatrix matrix1; - MutatorsStack stack1; - stack1.PushTransform(SkMatrix::Translate(0, 0)); - - embedder->PrerollCompositeEmbeddedView( - 0, std::make_unique(matrix1, SkSize::Make(200, 200), - stack1)); - EXPECT_CALL(*jni_mock, FlutterViewOnDisplayPlatformView(0, 0, 0, 200, 200, - 300, 300, stack1)); + { + // Add first Android view. + SkMatrix matrix; + MutatorsStack stack; + stack.PushTransform(SkMatrix::Translate(0, 0)); + + embedder->PrerollCompositeEmbeddedView( + 0, std::make_unique(matrix, SkSize::Make(200, 200), + stack)); + EXPECT_CALL(*jni_mock, FlutterViewOnDisplayPlatformView(0, 0, 0, 200, 200, + 300, 300, stack)); + } auto rect_paint = SkPaint(); rect_paint.setColor(SkColors::kCyan); @@ -534,31 +536,35 @@ TEST(AndroidExternalViewEmbedder, SubmitFrame__overlayComposition) { // This simulates Flutter UI that intersects with the first Android view. embedder->CompositeEmbeddedView(0)->drawRect( - SkRect::MakeXYWH(25, 25, 50, 150), rect_paint); - - // Add second Android view. - SkMatrix matrix2; - MutatorsStack stack2; - stack2.PushTransform(SkMatrix::Translate(0, 100)); - - embedder->PrerollCompositeEmbeddedView( - 1, std::make_unique(matrix2, SkSize::Make(100, 100), - stack2)); - EXPECT_CALL(*jni_mock, FlutterViewOnDisplayPlatformView(1, 0, 0, 100, 100, - 150, 150, stack2)); + SkRect::MakeXYWH(25, 25, 80, 150), rect_paint); + { + // Add second Android view. + SkMatrix matrix; + MutatorsStack stack; + stack.PushTransform(SkMatrix::Translate(0, 100)); + + embedder->PrerollCompositeEmbeddedView( + 1, std::make_unique(matrix, SkSize::Make(100, 100), + stack)); + EXPECT_CALL(*jni_mock, FlutterViewOnDisplayPlatformView(1, 0, 0, 100, 100, + 150, 150, stack)); + } // This simulates Flutter UI that intersects with the first and second Android // views. - embedder->CompositeEmbeddedView(1)->drawRect(SkRect::MakeXYWH(25, 25, 50, 50), + embedder->CompositeEmbeddedView(1)->drawRect(SkRect::MakeXYWH(25, 25, 80, 50), rect_paint); + embedder->CompositeEmbeddedView(1)->drawRect( + SkRect::MakeXYWH(75, 75, 30, 100), rect_paint); + EXPECT_CALL(*jni_mock, FlutterViewCreateOverlaySurface()) .WillRepeatedly([&]() { return std::make_unique( 1, window); }); - EXPECT_CALL(*jni_mock, FlutterViewDisplayOverlaySurface(1, 25, 25, 50, 150)) + EXPECT_CALL(*jni_mock, FlutterViewDisplayOverlaySurface(1, 25, 25, 80, 150)) .Times(2); auto surface_frame = std::make_unique( diff --git a/testing/run_tests.py b/testing/run_tests.py index dd4ed1428c295..f37904cee616a 100755 --- a/testing/run_tests.py +++ b/testing/run_tests.py @@ -124,7 +124,6 @@ def RunCCTests(build_dir, filter): ] shuffle_flags = non_repeatable_shuffle_flags + [ "--gtest_repeat=2", - "--gmock_verbose=info", ] RunEngineExecutable(build_dir, 'client_wrapper_glfw_unittests', filter, shuffle_flags)