From 1969110cb680a3a0f6449b76d3d5caea7bc28cf4 Mon Sep 17 00:00:00 2001 From: Gray Mackall <34871572+gmackall@users.noreply.github.com> Date: Fri, 10 Oct 2025 00:00:11 -0700 Subject: [PATCH] [HCPP] Properly remove hcpp views that are no longer visible (#176742) Fixes https://github.com/flutter/flutter/issues/175882. This ended up being a very similar bug to https://github.com/flutter/flutter/pull/173881, and in my understanding comes from the same incorrect assumption. In old PV land these views would stop displaying if we stopped sending updates to them. But in HCPP the underlying implementation ends up using an Android SurfaceFlinger which continues displaying what it was last told to. So again we have a bug here were we aren't taking the new responsibility of clearing that we need to. See https://github.com/flutter/flutter/pull/173881#discussion_r2283287104 Other solutions would be to just always call hide on the set of (keys of view_params which are not in composition order) on every frame (which would avoid this mirroring of state), or to modify the jni here to pass over one big bundle with all the information required to do all the existing calls as well as determine which views need to be hidden. But we are already doing this same mirroring of visibility state for the overlay layer (see Hide/ShowOverlayLayerIfNeeded()), so I think it is reasonable to take the same approach here. This is sort of a follow up to https://github.com/flutter/flutter/pull/162908 as well, this PR properly removes the view when it is the last platform view, but does not fix the case that this PR attempts to more generally fix where we go from 2->1 (or n->n-1 > 0) pvs. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed. [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md --------- Co-authored-by: Gray Mackall --- .../lib/hcpp/platform_view_clear_hidden.dart | 122 ++++++++++++++++++ .../hcpp/platform_view_clear_hidden_test.dart | 63 +++++++++ .../android/android_shell_holder_unittests.cc | 1 + .../android/external_view_embedder/BUILD.gn | 3 + .../external_view_embedder_2.cc | 43 ++++-- .../external_view_embedder_2.h | 7 +- .../flutter/embedding/engine/FlutterJNI.java | 12 ++ .../platform/PlatformViewsController2.java | 9 ++ .../shell/platform/android/jni/jni_mock.h | 1 + .../android/jni/platform_view_android_jni.h | 2 + .../android/platform_view_android_jni_impl.cc | 11 ++ .../android/platform_view_android_jni_impl.h | 2 + 12 files changed, 265 insertions(+), 11 deletions(-) create mode 100644 dev/integration_tests/android_engine_test/lib/hcpp/platform_view_clear_hidden.dart create mode 100644 dev/integration_tests/android_engine_test/test_driver/hcpp/platform_view_clear_hidden_test.dart diff --git a/dev/integration_tests/android_engine_test/lib/hcpp/platform_view_clear_hidden.dart b/dev/integration_tests/android_engine_test/lib/hcpp/platform_view_clear_hidden.dart new file mode 100644 index 00000000000..4dcbbd8a931 --- /dev/null +++ b/dev/integration_tests/android_engine_test/lib/hcpp/platform_view_clear_hidden.dart @@ -0,0 +1,122 @@ +// Copyright 2014 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. + +import 'dart:convert'; + +import 'package:android_driver_extensions/extension.dart'; +import 'package:flutter/foundation.dart'; +import 'package:flutter/gestures.dart'; +import 'package:flutter/material.dart'; +import 'package:flutter/rendering.dart'; +import 'package:flutter/services.dart'; +import 'package:flutter_driver/driver_extension.dart'; + +import '../src/allow_list_devices.dart'; + +void main() { + ensureAndroidDevice(); + enableFlutterDriverExtension( + handler: (String? command) async { + return json.encode({ + 'supported': await HybridAndroidViewController.checkIfSupported(), + }); + }, + commands: [nativeDriverCommands], + ); + + runApp(const MyApp()); +} + +/// A reusable widget that encapsulates the creation of the platform view. +class PlatformViewWidget extends StatelessWidget { + const PlatformViewWidget({super.key}); + + @override + Widget build(BuildContext context) { + return Center( + child: SizedBox( + width: 200, + height: 200, + child: PlatformViewLink( + viewType: 'changing_color_button_platform_view', + surfaceFactory: (BuildContext context, PlatformViewController controller) { + return AndroidViewSurface( + controller: controller as AndroidViewController, + hitTestBehavior: PlatformViewHitTestBehavior.transparent, + gestureRecognizers: const >{}, + ); + }, + onCreatePlatformView: (PlatformViewCreationParams params) { + return PlatformViewsService.initHybridAndroidView( + id: params.id, + viewType: 'changing_color_button_platform_view', + layoutDirection: TextDirection.ltr, + creationParamsCodec: const StandardMessageCodec(), + ) + ..addOnPlatformViewCreatedListener(params.onPlatformViewCreated) + ..create(); + }, + ), + ), + ); + } +} + +class MyApp extends StatefulWidget { + const MyApp({super.key}); + + @override + State createState() => _MyAppState(); +} + +class _MyAppState extends State { + bool _showRightView = true; + + void _toggleRightView() { + setState(() { + _showRightView = !_showRightView; + }); + } + + @override + Widget build(BuildContext context) { + return MaterialApp( + home: Scaffold( + appBar: AppBar(title: const Text('Platform View Toggle Demo')), + body: Column( + children: [ + Padding( + padding: const EdgeInsets.all(16.0), + child: ElevatedButton( + key: const ValueKey('ToggleRightView'), + onPressed: _toggleRightView, + child: Text(_showRightView ? 'Hide Right View' : 'Show Right View'), + ), + ), + Expanded( + child: Row( + children: [ + // The left platform view is always visible. + const Expanded(child: PlatformViewWidget()), + + // The right platform view's visibility is toggled, + // but it always occupies space in the layout. + Expanded( + child: Visibility( + visible: _showRightView, + maintainState: true, + maintainSize: true, + maintainAnimation: true, + child: const PlatformViewWidget(), + ), + ), + ], + ), + ), + ], + ), + ), + ); + } +} diff --git a/dev/integration_tests/android_engine_test/test_driver/hcpp/platform_view_clear_hidden_test.dart b/dev/integration_tests/android_engine_test/test_driver/hcpp/platform_view_clear_hidden_test.dart new file mode 100644 index 00000000000..671dfe3376c --- /dev/null +++ b/dev/integration_tests/android_engine_test/test_driver/hcpp/platform_view_clear_hidden_test.dart @@ -0,0 +1,63 @@ +// Copyright 2014 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. + +import 'dart:convert'; + +import 'package:android_driver_extensions/native_driver.dart'; +import 'package:android_driver_extensions/skia_gold.dart'; +import 'package:flutter_driver/flutter_driver.dart'; +import 'package:test/test.dart'; + +import '../_luci_skia_gold_prelude.dart'; + +/// For local debugging, a (local) golden-file is required as a baseline: +/// +/// ```sh +/// # Checkout HEAD, i.e. *before* changes you want to test. +/// UPDATE_GOLDENS=1 flutter drive lib/hcpp/platform_view_clear_hidden.dart +/// +/// # Make your changes. +/// +/// # Run the test against baseline. +/// flutter drive lib/hcpp/platform_view_clear_hidden.dart +/// ``` +/// +/// For a convenient way to deflake a test, see `tool/deflake.dart`. +void main() async { + const String goldenPrefix = 'hybrid_composition_pp_platform_view'; + + late final FlutterDriver flutterDriver; + late final NativeDriver nativeDriver; + + setUpAll(() async { + if (isLuci) { + await enableSkiaGoldComparator(namePrefix: 'android_engine_test$goldenVariant'); + } + flutterDriver = await FlutterDriver.connect(); + nativeDriver = await AndroidNativeDriver.connect(flutterDriver); + await nativeDriver.configureForScreenshotTesting(); + await flutterDriver.waitUntilFirstFrameRasterized(); + }); + + tearDownAll(() async { + await nativeDriver.close(); + await flutterDriver.close(); + }); + + test('verify that HCPP is supported and enabled', () async { + final Map response = + json.decode(await flutterDriver.requestData('')) as Map; + + expect(response['supported'], true); + }, timeout: Timeout.none); + + test('should start with texture, and toggle to no texture', () async { + await expectLater(nativeDriver.screenshot(), matchesGoldenFile('$goldenPrefix.two_boxes.png')); + await flutterDriver.tap(find.byValueKey('ToggleRightView')); + await expectLater( + nativeDriver.screenshot(), + matchesGoldenFile('$goldenPrefix.only_one_box.png'), + ); + }, timeout: Timeout.none); +} diff --git a/engine/src/flutter/shell/platform/android/android_shell_holder_unittests.cc b/engine/src/flutter/shell/platform/android/android_shell_holder_unittests.cc index 9664cc06cc5..abfc80a0cf9 100644 --- a/engine/src/flutter/shell/platform/android/android_shell_holder_unittests.cc +++ b/engine/src/flutter/shell/platform/android/android_shell_holder_unittests.cc @@ -114,6 +114,7 @@ class MockPlatformViewAndroidJNI : public PlatformViewAndroidJNI { int32_t viewHeight, MutatorsStack mutators_stack), (override)); + MOCK_METHOD(void, hidePlatformView2, (int32_t view_id), (override)); MOCK_METHOD(void, onEndFrame2, (), (override)); MOCK_METHOD(void, showOverlaySurface2, (), (override)); MOCK_METHOD(void, hideOverlaySurface2, (), (override)); diff --git a/engine/src/flutter/shell/platform/android/external_view_embedder/BUILD.gn b/engine/src/flutter/shell/platform/android/external_view_embedder/BUILD.gn index 4702b9a7fdf..27bf19ef502 100644 --- a/engine/src/flutter/shell/platform/android/external_view_embedder/BUILD.gn +++ b/engine/src/flutter/shell/platform/android/external_view_embedder/BUILD.gn @@ -29,6 +29,9 @@ source_set("external_view_embedder") { "//flutter/shell/platform/android/surface", "//flutter/skia", ] + + public_deps = + [ "//flutter/third_party/abseil-cpp/absl/container:flat_hash_set" ] } test_fixtures("external_view_embedder_fixtures") { diff --git a/engine/src/flutter/shell/platform/android/external_view_embedder/external_view_embedder_2.cc b/engine/src/flutter/shell/platform/android/external_view_embedder/external_view_embedder_2.cc index f8e16cabdbf..21588320ae1 100644 --- a/engine/src/flutter/shell/platform/android/external_view_embedder/external_view_embedder_2.cc +++ b/engine/src/flutter/shell/platform/android/external_view_embedder/external_view_embedder_2.cc @@ -72,9 +72,20 @@ void AndroidExternalViewEmbedder2::SubmitFlutterView( if (!FrameHasPlatformLayers()) { frame->Submit(); - // If the previous frame had platform views, hide the overlay surface. - HideOverlayLayerIfNeeded(); - jni_facade_->applyTransaction(); + task_runners_.GetPlatformTaskRunner()->PostTask(fml::MakeCopyable( + [this, jni_facade = jni_facade_, + views_visible_last_frame = views_visible_last_frame_]() { + // This pointer is guaranteed to not be dangling as long as + // DestroySurfaces is called before the embedder is deleted. See + // https://github.com/flutter/flutter/pull/176742#discussion_r2415229396. + this->HideOverlayLayerIfNeeded(); + for (int64_t view_id : views_visible_last_frame) { + jni_facade->hidePlatformView2(view_id); + } + + jni_facade->applyTransaction(); + })); + views_visible_last_frame_.clear(); return; } @@ -151,13 +162,14 @@ void AndroidExternalViewEmbedder2::SubmitFlutterView( } else { overlay_layer_has_content_this_frame_ = false; } - frame->Submit(); + frame->Submit(); task_runners_.GetPlatformTaskRunner()->PostTask(fml::MakeCopyable( [&, composition_order = composition_order_, view_params = view_params_, jni_facade = jni_facade_, device_pixel_ratio = device_pixel_ratio_, slices = std::move(slices_), - overlay_layer_has_content_this_frame_]() -> void { + views_visible_last_frame = views_visible_last_frame_, + overlay_layer_has_content_this_frame_]() mutable -> void { jni_facade->swapTransaction(); if (overlay_layer_has_content_this_frame_) { @@ -179,9 +191,20 @@ void AndroidExternalViewEmbedder2::SubmitFlutterView( params.sizePoints().height * device_pixel_ratio, params.mutatorsStack() // ); + // Remove from views visible last frame, so we can hide the rest. + views_visible_last_frame.erase(view_id); } + // Hide views that were visible last frame, but not in this frame. + for (int64_t view_id : views_visible_last_frame) { + jni_facade->hidePlatformView2(view_id); + } + jni_facade_->onEndFrame2(); })); + + views_visible_last_frame_.clear(); + views_visible_last_frame_.insert(composition_order_.begin(), + composition_order_.end()); } // |ExternalViewEmbedder| @@ -259,20 +282,20 @@ void AndroidExternalViewEmbedder2::DestroySurfaces() { latch.Signal(); }); latch.Wait(); - overlay_layer_is_shown_ = false; + overlay_layer_is_shown_.store(false); } void AndroidExternalViewEmbedder2::ShowOverlayLayerIfNeeded() { - if (!overlay_layer_is_shown_) { + if (!overlay_layer_is_shown_.load()) { jni_facade_->showOverlaySurface2(); - overlay_layer_is_shown_ = true; + overlay_layer_is_shown_.store(true); } } void AndroidExternalViewEmbedder2::HideOverlayLayerIfNeeded() { - if (overlay_layer_is_shown_) { + if (overlay_layer_is_shown_.load()) { jni_facade_->hideOverlaySurface2(); - overlay_layer_is_shown_ = false; + overlay_layer_is_shown_.store(false); } } diff --git a/engine/src/flutter/shell/platform/android/external_view_embedder/external_view_embedder_2.h b/engine/src/flutter/shell/platform/android/external_view_embedder/external_view_embedder_2.h index 988198fc761..4f2a1a3794e 100644 --- a/engine/src/flutter/shell/platform/android/external_view_embedder/external_view_embedder_2.h +++ b/engine/src/flutter/shell/platform/android/external_view_embedder/external_view_embedder_2.h @@ -5,6 +5,7 @@ #ifndef FLUTTER_SHELL_PLATFORM_ANDROID_EXTERNAL_VIEW_EMBEDDER_EXTERNAL_VIEW_EMBEDDER_2_H_ #define FLUTTER_SHELL_PLATFORM_ANDROID_EXTERNAL_VIEW_EMBEDDER_EXTERNAL_VIEW_EMBEDDER_2_H_ +#include #include #include "flutter/common/task_runners.h" @@ -13,6 +14,7 @@ #include "flutter/shell/platform/android/external_view_embedder/surface_pool.h" #include "flutter/shell/platform/android/jni/platform_view_android_jni.h" #include "flutter/shell/platform/android/surface/android_surface.h" +#include "flutter/third_party/abseil-cpp/absl/container/flat_hash_set.h" namespace flutter { @@ -113,7 +115,7 @@ class AndroidExternalViewEmbedder2 final : public ExternalViewEmbedder { const TaskRunners task_runners_; // If the overlay layer is currently shown. - bool overlay_layer_is_shown_ = false; + std::atomic_bool overlay_layer_is_shown_{false}; // The size of the root canvas. DlISize frame_size_; @@ -135,6 +137,9 @@ class AndroidExternalViewEmbedder2 final : public ExternalViewEmbedder { // mutation stack. std::unordered_map view_params_; + // The set of platform views that were visible in the last frame. + absl::flat_hash_set views_visible_last_frame_; + // Destroys the surfaces created from the surface factory. // This method schedules a task on the platform thread, and waits for // the task until it completes. diff --git a/engine/src/flutter/shell/platform/android/io/flutter/embedding/engine/FlutterJNI.java b/engine/src/flutter/shell/platform/android/io/flutter/embedding/engine/FlutterJNI.java index 2f97368b6e3..b7d3f3e24c6 100644 --- a/engine/src/flutter/shell/platform/android/io/flutter/embedding/engine/FlutterJNI.java +++ b/engine/src/flutter/shell/platform/android/io/flutter/embedding/engine/FlutterJNI.java @@ -1387,6 +1387,18 @@ public class FlutterJNI { viewId, x, y, width, height, viewWidth, viewHeight, mutatorsStack); } + @UiThread + @SuppressWarnings("unused") + @SuppressLint("NewApi") + public void hidePlatformView2(int viewId) { + ensureRunningOnMainThread(); + if (platformViewsController2 == null) { + throw new RuntimeException( + "platformViewsController must be set before attempting to hide a platform view"); + } + platformViewsController2.hidePlatformView(viewId); + } + // ----- Start Localization Support ---- /** Sets the localization plugin that is used in various localization methods. */ diff --git a/engine/src/flutter/shell/platform/android/io/flutter/plugin/platform/PlatformViewsController2.java b/engine/src/flutter/shell/platform/android/io/flutter/plugin/platform/PlatformViewsController2.java index 711a1a4577d..a5b4cbad9b7 100644 --- a/engine/src/flutter/shell/platform/android/io/flutter/plugin/platform/PlatformViewsController2.java +++ b/engine/src/flutter/shell/platform/android/io/flutter/plugin/platform/PlatformViewsController2.java @@ -526,6 +526,15 @@ public class PlatformViewsController2 implements PlatformViewsAccessibilityDeleg } } + public void hidePlatformView(int viewId) { + if (!initializePlatformViewIfNeeded(viewId)) { + return; + } + + final FlutterMutatorView parentView = platformViewParent.get(viewId); + parentView.setVisibility(View.GONE); + } + @RequiresApi(API_LEVELS.API_34) public void onEndFrame() { SurfaceControl.Transaction tx = new SurfaceControl.Transaction(); diff --git a/engine/src/flutter/shell/platform/android/jni/jni_mock.h b/engine/src/flutter/shell/platform/android/jni/jni_mock.h index 17939a68c89..8b0651c8fb8 100644 --- a/engine/src/flutter/shell/platform/android/jni/jni_mock.h +++ b/engine/src/flutter/shell/platform/android/jni/jni_mock.h @@ -143,6 +143,7 @@ class JNIMock final : public PlatformViewAndroidJNI { MutatorsStack mutators_stack), (override)); + MOCK_METHOD(void, hidePlatformView2, (int32_t view_id), (override)); MOCK_METHOD(void, onEndFrame2, (), (override)); MOCK_METHOD(void, hideOverlaySurface2, (), (override)); MOCK_METHOD(void, showOverlaySurface2, (), (override)); diff --git a/engine/src/flutter/shell/platform/android/jni/platform_view_android_jni.h b/engine/src/flutter/shell/platform/android/jni/platform_view_android_jni.h index 0598507d6cc..70d1070c441 100644 --- a/engine/src/flutter/shell/platform/android/jni/platform_view_android_jni.h +++ b/engine/src/flutter/shell/platform/android/jni/platform_view_android_jni.h @@ -244,6 +244,8 @@ class PlatformViewAndroidJNI { int32_t viewHeight, MutatorsStack mutators_stack) = 0; + virtual void hidePlatformView2(int32_t view_id) = 0; + virtual void showOverlaySurface2() = 0; virtual void hideOverlaySurface2() = 0; diff --git a/engine/src/flutter/shell/platform/android/platform_view_android_jni_impl.cc b/engine/src/flutter/shell/platform/android/platform_view_android_jni_impl.cc index cb88853bbe2..ed3324fefb8 100644 --- a/engine/src/flutter/shell/platform/android/platform_view_android_jni_impl.cc +++ b/engine/src/flutter/shell/platform/android/platform_view_android_jni_impl.cc @@ -87,6 +87,7 @@ static jfieldID g_jni_shell_holder_field = nullptr; V(g_on_display_platform_view2_method, onDisplayPlatformView2, \ "(IIIIIIILio/flutter/embedding/engine/mutatorsstack/" \ "FlutterMutatorsStack;)V") \ + V(g_hide_platform_view2_method, hidePlatformView2, "(I)V") \ V(g_on_end_frame2_method, endFrame2, "()V") \ V(g_show_overlay_surface2_method, showOverlaySurface2, "()V") \ V(g_hide_overlay_surface2_method, hideOverlaySurface2, "()V") \ @@ -2305,6 +2306,16 @@ void PlatformViewAndroidJNIImpl::onDisplayPlatformView2( FML_CHECK(fml::jni::CheckException(env)); } +void PlatformViewAndroidJNIImpl::hidePlatformView2(int32_t view_id) { + JNIEnv* env = fml::jni::AttachCurrentThread(); + auto java_object = java_object_.get(env); + if (java_object.is_null()) { + return; + } + + env->CallVoidMethod(java_object.obj(), g_hide_platform_view2_method, view_id); +} + void PlatformViewAndroidJNIImpl::onEndFrame2() { JNIEnv* env = fml::jni::AttachCurrentThread(); diff --git a/engine/src/flutter/shell/platform/android/platform_view_android_jni_impl.h b/engine/src/flutter/shell/platform/android/platform_view_android_jni_impl.h index bae55e608fb..e97f5fd6d86 100644 --- a/engine/src/flutter/shell/platform/android/platform_view_android_jni_impl.h +++ b/engine/src/flutter/shell/platform/android/platform_view_android_jni_impl.h @@ -126,6 +126,8 @@ class PlatformViewAndroidJNIImpl final : public PlatformViewAndroidJNI { int32_t viewHeight, MutatorsStack mutators_stack) override; + void hidePlatformView2(int32_t view_id) override; + void showOverlaySurface2() override; void hideOverlaySurface2() override;