From ae36dac40884c7c3d82a0f6c7576c2517eeb3e9a Mon Sep 17 00:00:00 2001 From: Emmanuel Garcia Date: Tue, 25 Aug 2020 08:44:15 -0700 Subject: [PATCH] Revert "Enable hybrid composition by default on Android (#20722)" This reverts commit d16ba48e1d79cf0197e96c51c38300ee921baeb0. --- common/settings.h | 9 +++ flow/surface.cc | 4 -- flow/surface.h | 2 - shell/common/rasterizer.cc | 4 -- shell/common/switches.cc | 3 + shell/common/switches.h | 10 +++- shell/gpu/gpu_surface_gl.cc | 5 -- shell/gpu/gpu_surface_gl.h | 3 - .../platform/android/android_shell_holder.cc | 58 ++++++++++++++----- shell/platform/android/android_shell_holder.h | 8 +++ shell/platform/android/android_surface_gl.cc | 3 + .../android/android_surface_software.cc | 3 + .../engine/loader/ApplicationInfoLoader.java | 8 ++- .../engine/loader/FlutterApplicationInfo.java | 7 ++- .../engine/loader/FlutterLoader.java | 3 + .../embedding/engine/PluginComponentTest.java | 2 +- .../loader/ApplicationInfoLoaderTest.java | 3 + .../android/app/src/main/AndroidManifest.xml | 3 + 18 files changed, 101 insertions(+), 37 deletions(-) diff --git a/common/settings.h b/common/settings.h index 6d7b07a2ce1ea..ae52e4a7342ba 100644 --- a/common/settings.h +++ b/common/settings.h @@ -223,6 +223,15 @@ struct Settings { /// to log a timeline event that tracks the latency of engine startup. std::chrono::microseconds engine_start_timestamp = {}; + /// Whether the application claims that it uses the android embedded view for + /// platform views. + /// + /// A `true` value will result the raster task runner always run on the + /// platform thread. + // TODO(cyanlaz): Remove this when dynamic thread merging is done. + // https://github.com/flutter/flutter/issues/59930 + bool use_embedded_view = false; + std::string ToString() const; }; diff --git a/flow/surface.cc b/flow/surface.cc index 7dbdfb06753cf..a19ab9afec860 100644 --- a/flow/surface.cc +++ b/flow/surface.cc @@ -18,8 +18,4 @@ std::unique_ptr Surface::MakeRenderContextCurrent() { return std::make_unique(true); } -bool Surface::ClearRenderContext() { - return false; -} - } // namespace flutter diff --git a/flow/surface.h b/flow/surface.h index 610cc43232bba..3009af6507d33 100644 --- a/flow/surface.h +++ b/flow/surface.h @@ -33,8 +33,6 @@ class Surface { virtual std::unique_ptr MakeRenderContextCurrent(); - virtual bool ClearRenderContext(); - private: FML_DISALLOW_COPY_AND_ASSIGN(Surface); }; diff --git a/shell/common/rasterizer.cc b/shell/common/rasterizer.cc index e6abdbd155a22..682d3a6d2672f 100644 --- a/shell/common/rasterizer.cc +++ b/shell/common/rasterizer.cc @@ -143,10 +143,6 @@ void Rasterizer::Draw(fml::RefPtr> pipeline) { consume_result = PipelineConsumeResult::MoreAvailable; } - if (surface_ != nullptr) { - surface_->ClearRenderContext(); - } - // Merging the thread as we know the next `Draw` should be run on the platform // thread. if (surface_ != nullptr && surface_->GetExternalViewEmbedder() != nullptr) { diff --git a/shell/common/switches.cc b/shell/common/switches.cc index de76aa1a1471a..16494204ddf81 100644 --- a/shell/common/switches.cc +++ b/shell/common/switches.cc @@ -229,6 +229,9 @@ Settings SettingsFromCommandLine(const fml::CommandLine& command_line) { : "127.0.0.1"; } + settings.use_embedded_view = + command_line.HasOption(FlagForSwitch(Switch::UseEmbeddedView)); + // Set Observatory Port if (command_line.HasOption(FlagForSwitch(Switch::DeviceObservatoryPort))) { if (!GetSwitchValue(command_line, Switch::DeviceObservatoryPort, diff --git a/shell/common/switches.h b/shell/common/switches.h index f33e2722403df..1110261f767c3 100644 --- a/shell/common/switches.h +++ b/shell/common/switches.h @@ -196,7 +196,15 @@ DEF_SWITCH( "Uses separate threads for the platform, UI, GPU and IO task runners. " "By default, a single thread is used for all task runners. Only available " "in the flutter_tester.") - +// TODO(cyanlaz): Remove this when dynamic thread merging is done. +// https://github.com/flutter/flutter/issues/59930 +DEF_SWITCH(UseEmbeddedView, + "use-embedded-view", + "Whether an android application uses embedded views." + "This is a temporary flag to make the raster task runner runs on " + "the platform thread." + "This flag should be removed once the dynamic thread merging is " + "enabled on android.") DEF_SWITCHES_END void PrintUsage(const std::string& executable_name); diff --git a/shell/gpu/gpu_surface_gl.cc b/shell/gpu/gpu_surface_gl.cc index 58d69951d8c18..5386683a27474 100644 --- a/shell/gpu/gpu_surface_gl.cc +++ b/shell/gpu/gpu_surface_gl.cc @@ -341,9 +341,4 @@ std::unique_ptr GPUSurfaceGL::MakeRenderContextCurrent() { return delegate_->GLContextMakeCurrent(); } -// |Surface| -bool GPUSurfaceGL::ClearRenderContext() { - return delegate_->GLContextClearCurrent(); -} - } // namespace flutter diff --git a/shell/gpu/gpu_surface_gl.h b/shell/gpu/gpu_surface_gl.h index 09898bfc5a182..11ecfb9ee3e32 100644 --- a/shell/gpu/gpu_surface_gl.h +++ b/shell/gpu/gpu_surface_gl.h @@ -48,9 +48,6 @@ class GPUSurfaceGL : public Surface { // |Surface| std::unique_ptr MakeRenderContextCurrent() override; - // |Surface| - bool ClearRenderContext() override; - private: GPUSurfaceGLDelegate* delegate_; sk_sp context_; diff --git a/shell/platform/android/android_shell_holder.cc b/shell/platform/android/android_shell_holder.cc index d58ad703f695a..7272571d71a4f 100644 --- a/shell/platform/android/android_shell_holder.cc +++ b/shell/platform/android/android_shell_holder.cc @@ -28,6 +28,8 @@ static PlatformData GetDefaultPlatformData() { return platform_data; } +bool AndroidShellHolder::use_embedded_view; + AndroidShellHolder::AndroidShellHolder( flutter::Settings settings, std::shared_ptr jni_facade, @@ -100,21 +102,47 @@ AndroidShellHolder::AndroidShellHolder( ui_runner = thread_host_.ui_thread->GetTaskRunner(); io_runner = thread_host_.io_thread->GetTaskRunner(); } - - flutter::TaskRunners task_runners(thread_label, // label - platform_runner, // platform - gpu_runner, // raster - ui_runner, // ui - io_runner // io - ); - - shell_ = - Shell::Create(task_runners, // task runners - GetDefaultPlatformData(), // window data - settings_, // settings - on_create_platform_view, // platform view create callback - on_create_rasterizer // rasterizer create callback - ); + if (settings.use_embedded_view) { + use_embedded_view = true; + // Embedded views requires the gpu and the platform views to be the same. + // The plan is to eventually dynamically merge the threads when there's a + // platform view in the layer tree. + // For now we use a fixed thread configuration with the same thread used as + // the gpu and platform task runner. + // TODO(amirh/chinmaygarde): remove this, and dynamically change the thread + // configuration. https://github.com/flutter/flutter/issues/23975 + // https://github.com/flutter/flutter/issues/59930 + flutter::TaskRunners task_runners(thread_label, // label + platform_runner, // platform + platform_runner, // raster + ui_runner, // ui + io_runner // io + ); + + shell_ = + Shell::Create(task_runners, // task runners + GetDefaultPlatformData(), // window data + settings_, // settings + on_create_platform_view, // platform view create callback + on_create_rasterizer // rasterizer create callback + ); + } else { + use_embedded_view = false; + flutter::TaskRunners task_runners(thread_label, // label + platform_runner, // platform + gpu_runner, // raster + ui_runner, // ui + io_runner // io + ); + + shell_ = + Shell::Create(task_runners, // task runners + GetDefaultPlatformData(), // window data + settings_, // settings + on_create_platform_view, // platform view create callback + on_create_rasterizer // rasterizer create callback + ); + } platform_view_ = weak_platform_view; FML_DCHECK(platform_view_); diff --git a/shell/platform/android/android_shell_holder.h b/shell/platform/android/android_shell_holder.h index 932f12ff7fdfb..28ad8610882f7 100644 --- a/shell/platform/android/android_shell_holder.h +++ b/shell/platform/android/android_shell_holder.h @@ -21,6 +21,14 @@ namespace flutter { class AndroidShellHolder { public: + // Whether the application sets to use embedded_view view + // `io.flutter.embedded_views_preview` flag. This can be static because it is + // determined by the application and it is safe when there are multiple + // `AndroidSurface`s. + // TODO(cyanglaz): remove this when dynamic thread merging is enabled on + // android. https://github.com/flutter/flutter/issues/59930 + static bool use_embedded_view; + AndroidShellHolder(flutter::Settings settings, std::shared_ptr jni_facade, bool is_background_view); diff --git a/shell/platform/android/android_surface_gl.cc b/shell/platform/android/android_surface_gl.cc index 7087f6643a686..7f692f007634a 100644 --- a/shell/platform/android/android_surface_gl.cc +++ b/shell/platform/android/android_surface_gl.cc @@ -123,6 +123,9 @@ intptr_t AndroidSurfaceGL::GLContextFBO(GLFrameInfo frame_info) const { // |GPUSurfaceGLDelegate| ExternalViewEmbedder* AndroidSurfaceGL::GetExternalViewEmbedder() { + if (!AndroidShellHolder::use_embedded_view) { + return nullptr; + } return external_view_embedder_.get(); } diff --git a/shell/platform/android/android_surface_software.cc b/shell/platform/android/android_surface_software.cc index 126f127bd2b8c..1f99b967b9a00 100644 --- a/shell/platform/android/android_surface_software.cc +++ b/shell/platform/android/android_surface_software.cc @@ -146,6 +146,9 @@ bool AndroidSurfaceSoftware::PresentBackingStore( // |GPUSurfaceSoftwareDelegate| ExternalViewEmbedder* AndroidSurfaceSoftware::GetExternalViewEmbedder() { + if (!AndroidShellHolder::use_embedded_view) { + return nullptr; + } return external_view_embedder_.get(); } diff --git a/shell/platform/android/io/flutter/embedding/engine/loader/ApplicationInfoLoader.java b/shell/platform/android/io/flutter/embedding/engine/loader/ApplicationInfoLoader.java index bc1f71af2b3fa..c29ddd9f21a9c 100644 --- a/shell/platform/android/io/flutter/embedding/engine/loader/ApplicationInfoLoader.java +++ b/shell/platform/android/io/flutter/embedding/engine/loader/ApplicationInfoLoader.java @@ -79,6 +79,11 @@ private static String getNetworkPolicy(ApplicationInfo appInfo, Context context) return output.toString(); } + private static boolean getUseEmbeddedView(ApplicationInfo appInfo) { + Bundle bundle = appInfo.metaData; + return bundle != null && bundle.getBoolean("io.flutter.embedded_views_preview"); + } + private static void parseDomainConfig( XmlResourceParser xrp, JSONArray output, boolean inheritedCleartextPermitted) throws IOException, XmlPullParserException { @@ -150,6 +155,7 @@ public static FlutterApplicationInfo load(@NonNull Context applicationContext) { getString(appInfo.metaData, PUBLIC_FLUTTER_ASSETS_DIR_KEY), getNetworkPolicy(appInfo, applicationContext), appInfo.nativeLibraryDir, - clearTextPermitted); + clearTextPermitted, + getUseEmbeddedView(appInfo)); } } diff --git a/shell/platform/android/io/flutter/embedding/engine/loader/FlutterApplicationInfo.java b/shell/platform/android/io/flutter/embedding/engine/loader/FlutterApplicationInfo.java index 33a9e4488f18d..3d5c2b10c40d6 100644 --- a/shell/platform/android/io/flutter/embedding/engine/loader/FlutterApplicationInfo.java +++ b/shell/platform/android/io/flutter/embedding/engine/loader/FlutterApplicationInfo.java @@ -18,6 +18,9 @@ public final class FlutterApplicationInfo { final String domainNetworkPolicy; final String nativeLibraryDir; final boolean clearTextPermitted; + // TODO(cyanlaz): Remove this when dynamic thread merging is done. + // https://github.com/flutter/flutter/issues/59930 + final boolean useEmbeddedView; public FlutterApplicationInfo( String aotSharedLibraryName, @@ -26,7 +29,8 @@ public FlutterApplicationInfo( String flutterAssetsDir, String domainNetworkPolicy, String nativeLibraryDir, - boolean clearTextPermitted) { + boolean clearTextPermitted, + boolean useEmbeddedView) { this.aotSharedLibraryName = aotSharedLibraryName == null ? DEFAULT_AOT_SHARED_LIBRARY_NAME : aotSharedLibraryName; this.vmSnapshotData = vmSnapshotData == null ? DEFAULT_VM_SNAPSHOT_DATA : vmSnapshotData; @@ -37,5 +41,6 @@ public FlutterApplicationInfo( this.nativeLibraryDir = nativeLibraryDir; this.domainNetworkPolicy = domainNetworkPolicy == null ? "" : domainNetworkPolicy; this.clearTextPermitted = clearTextPermitted; + this.useEmbeddedView = useEmbeddedView; } } diff --git a/shell/platform/android/io/flutter/embedding/engine/loader/FlutterLoader.java b/shell/platform/android/io/flutter/embedding/engine/loader/FlutterLoader.java index c69d6a3fb9b36..2aee56dee94e3 100644 --- a/shell/platform/android/io/flutter/embedding/engine/loader/FlutterLoader.java +++ b/shell/platform/android/io/flutter/embedding/engine/loader/FlutterLoader.java @@ -223,6 +223,9 @@ public void ensureInitializationComplete( if (flutterApplicationInfo.domainNetworkPolicy != null) { shellArgs.add("--domain-network-policy=" + flutterApplicationInfo.domainNetworkPolicy); } + if (flutterApplicationInfo.useEmbeddedView) { + shellArgs.add("--use-embedded-view"); + } if (settings.getLogTag() != null) { shellArgs.add("--log-tag=" + settings.getLogTag()); } diff --git a/shell/platform/android/test/io/flutter/embedding/engine/PluginComponentTest.java b/shell/platform/android/test/io/flutter/embedding/engine/PluginComponentTest.java index de766e74a735e..01d012c38b83a 100644 --- a/shell/platform/android/test/io/flutter/embedding/engine/PluginComponentTest.java +++ b/shell/platform/android/test/io/flutter/embedding/engine/PluginComponentTest.java @@ -28,7 +28,7 @@ public void pluginsCanAccessFlutterAssetPaths() { FlutterJNI flutterJNI = mock(FlutterJNI.class); when(flutterJNI.isAttached()).thenReturn(true); FlutterApplicationInfo emptyInfo = - new FlutterApplicationInfo(null, null, null, null, null, null, false); + new FlutterApplicationInfo(null, null, null, null, null, null, false, false); // FlutterLoader is the object to which the PluginRegistry defers for obtaining // the path to a Flutter asset. Ideally in this component test we would use a diff --git a/shell/platform/android/test/io/flutter/embedding/engine/loader/ApplicationInfoLoaderTest.java b/shell/platform/android/test/io/flutter/embedding/engine/loader/ApplicationInfoLoaderTest.java index 9a3dda5321856..769525f7db3c9 100644 --- a/shell/platform/android/test/io/flutter/embedding/engine/loader/ApplicationInfoLoaderTest.java +++ b/shell/platform/android/test/io/flutter/embedding/engine/loader/ApplicationInfoLoaderTest.java @@ -43,6 +43,7 @@ public void itGeneratesCorrectApplicationInfoWithDefaultManifest() { assertEquals("", info.domainNetworkPolicy); assertNull(info.nativeLibraryDir); assertEquals(true, info.clearTextPermitted); + assertEquals(false, info.useEmbeddedView); } @Config(shadows = {ApplicationInfoLoaderTest.ShadowNetworkSecurityPolicy.class}) @@ -87,6 +88,7 @@ public void itGeneratesCorrectApplicationInfoWithCustomValues() throws Exception bundle.putString(ApplicationInfoLoader.PUBLIC_VM_SNAPSHOT_DATA_KEY, "testvmsnapshot"); bundle.putString(ApplicationInfoLoader.PUBLIC_ISOLATE_SNAPSHOT_DATA_KEY, "testisolatesnapshot"); bundle.putString(ApplicationInfoLoader.PUBLIC_FLUTTER_ASSETS_DIR_KEY, "testassets"); + bundle.putBoolean("io.flutter.embedded_views_preview", true); Context context = generateMockContext(bundle, null); FlutterApplicationInfo info = ApplicationInfoLoader.load(context); assertNotNull(info); @@ -96,6 +98,7 @@ public void itGeneratesCorrectApplicationInfoWithCustomValues() throws Exception assertEquals("testassets", info.flutterAssetsDir); assertNull(info.nativeLibraryDir); assertEquals("", info.domainNetworkPolicy); + assertEquals(true, info.useEmbeddedView); } @Test diff --git a/testing/scenario_app/android/app/src/main/AndroidManifest.xml b/testing/scenario_app/android/app/src/main/AndroidManifest.xml index 1ddceaf516d8b..a580f17b7a523 100644 --- a/testing/scenario_app/android/app/src/main/AndroidManifest.xml +++ b/testing/scenario_app/android/app/src/main/AndroidManifest.xml @@ -35,6 +35,9 @@ +