Test the SurfaceTextureSurfaceProducer-branch in the Android scenario_app (#51061)

matanlurey · web-flow · commit ea6adb071ad3 · 2024-02-28T14:17:21.000-08:00
This PR does the following: - Relands #50993. - Fixes a bug in `SurfaceTextureSurfaceProducer` where it would crash on release (now tested, yay!) Closes flutter/flutter#143539. Closes flutter/flutter#143483. /cc @gaaclarke @chinmaygarde, this PR should (after landed) test/verify #50730.
diff --git a/ci/builders/linux_android_emulator_opengles.json b/ci/builders/linux_android_emulator_opengles.json
@@ -57,6 +57,32 @@
                         "--enable-impeller",
                         "--impeller-backend=opengles"
                     ]
+                },
+                {
+                    "language": "dart",
+                    "name": "Android Scenario App Integration Tests (Impeller/OpenGLES, SurfaceTexture)",
+                    "test_timeout_secs": 900,
+                    "max_attempts": 2,
+                    "test_dependencies": [
+                        {
+                            "dependency": "android_virtual_device",
+                            "version": "android_34_google_apis_x64.textpb"
+                        },
+                        {
+                            "dependency": "avd_cipd_version",
+                            "version": "build_id:8759428741582061553"
+                        }
+                    ],
+                    "contexts": [
+                        "android_virtual_device"
+                    ],
+                    "script": "flutter/testing/scenario_app/bin/run_android_tests.dart",
+                    "parameters": [
+                        "--out-dir=../out/android_emulator_opengles_debug_x64",
+                        "--enable-impeller",
+                        "--impeller-backend=opengles",
+                        "--force-surface-producer-surface-texture"
+                    ]
                 }
             ]
         }
diff --git a/ci/builders/linux_android_emulator_skia.json b/ci/builders/linux_android_emulator_skia.json
@@ -57,6 +57,31 @@
                         "--out-dir=../out/android_emulator_skia_debug_x64",
                         "--no-enable-impeller"
                     ]
+                },
+                {
+                    "language": "dart",
+                    "name": "Android Scenario App Integration Tests (Skia, SurfaceTexture)",
+                    "test_timeout_secs": 900,
+                    "max_attempts": 2,
+                    "test_dependencies": [
+                        {
+                            "dependency": "android_virtual_device",
+                            "version": "android_34_google_apis_x64.textpb"
+                        },
+                        {
+                            "dependency": "avd_cipd_version",
+                            "version": "build_id:8759428741582061553"
+                        }
+                    ],
+                    "contexts": [
+                        "android_virtual_device"
+                    ],
+                    "script": "flutter/testing/scenario_app/bin/run_android_tests.dart",
+                    "parameters": [
+                        "--out-dir=../out/android_emulator_skia_debug_x64",
+                        "--no-enable-impeller",
+                        "--force-surface-producer-surface-texture"
+                    ]
                 }
             ]
         }
diff --git a/shell/platform/android/io/flutter/embedding/engine/renderer/FlutterRenderer.java b/shell/platform/android/io/flutter/embedding/engine/renderer/FlutterRenderer.java
@@ -57,7 +57,7 @@ public class FlutterRenderer implements TextureRegistry {
    * the OpenGLES/{@link SurfaceTextureSurfaceProducer} code branch. This flag has undefined
    * behavior if set to true while running in a Vulkan (Impeller) context.
    */
-  @VisibleForTesting static boolean debugForceSurfaceProducerGlTextures = false;
+  @VisibleForTesting public static boolean debugForceSurfaceProducerGlTextures = false;
 
   private static final String TAG = "FlutterRenderer";
 
@@ -184,19 +184,24 @@ public SurfaceProducer createSurfaceProducer() {
     // Coincidentally, if ImageTexture is available, we are also on an Android
     // version that is
     // running Vulkan, so we don't have to worry about it not being supported.
-    final long id = nextTextureId.getAndIncrement();
     final SurfaceProducer entry;
     if (!debugForceSurfaceProducerGlTextures && Build.VERSION.SDK_INT >= 29) {
+      final long id = nextTextureId.getAndIncrement();
       final ImageReaderSurfaceProducer producer = new ImageReaderSurfaceProducer(id);
       registerImageTexture(id, producer);
       addOnTrimMemoryListener(producer);
       Log.v(TAG, "New ImageReaderSurfaceProducer ID: " + id);
       entry = producer;
     } else {
+      // TODO(matanlurey): Actually have the class named "*Producer" to well, produce
+      // something. This is a code smell, but does guarantee the paths for both
+      // createSurfaceTexture and createSurfaceProducer doesn't diverge. As we get more
+      // confident in this API and any possible bugs (and have tests to check we don't
+      // regress), reconsider this pattern.
+      final SurfaceTextureEntry texture = createSurfaceTexture();
       final SurfaceTextureSurfaceProducer producer =
-          new SurfaceTextureSurfaceProducer(id, handler, flutterJNI);
-      registerSurfaceTexture(id, producer.getSurfaceTexture());
-      Log.v(TAG, "New SurfaceTextureSurfaceProducer ID: " + id);
+          new SurfaceTextureSurfaceProducer(texture.id(), handler, flutterJNI, texture);
+      Log.v(TAG, "New SurfaceTextureSurfaceProducer ID: " + texture.id());
       entry = producer;
     }
     return entry;
@@ -630,8 +635,9 @@ PerImage dequeueImage() {
                       + " closing image="
                       + lastDequeuedImage.image.hashCode());
             }
-            // We must keep the last image dequeued open until we are done presenting it.
-            // We have just dequeued a new image (r). Close the previously dequeued image.
+            // We must keep the last image dequeued open until we are done presenting
+            // it. We have just dequeued a new image (r). Close the previously dequeued
+            // image.
             lastDequeuedImage.image.close();
             lastDequeuedImage = null;
           }
@@ -817,11 +823,11 @@ private ImageReader createImageReader33() {
       // Allow for double buffering.
       builder.setMaxImages(MAX_IMAGES);
       // Use PRIVATE image format so that we can support video decoding.
-      // TODO(johnmccutchan): Should we always use PRIVATE here? It may impact our ability to read
-      // back texture data. If we don't always want to use it, how do we decide when to use it or
-      // not? Perhaps PlatformViews can indicate if they may contain DRM'd content. I need to
-      // investigate how PRIVATE impacts our ability to take screenshots or capture the output of
-      // Flutter application.
+      // TODO(johnmccutchan): Should we always use PRIVATE here? It may impact our ability to
+      // read back texture data. If we don't always want to use it, how do we decide when to
+      // use it or not? Perhaps PlatformViews can indicate if they may contain DRM'd content.
+      // I need to investigate how PRIVATE impacts our ability to take screenshots or capture
+      // the output of Flutter application.
       builder.setImageFormat(ImageFormat.PRIVATE);
       // Hint that consumed images will only be read by GPU.
       builder.setUsage(HardwareBuffer.USAGE_GPU_SAMPLED_IMAGE);
@@ -1008,10 +1014,10 @@ protected void finalize() throws Throwable {
    */
   public void startRenderingToSurface(@NonNull Surface surface, boolean onlySwap) {
     if (!onlySwap) {
-      // Stop rendering to the surface releases the associated native resources, which causes a
-      // glitch when toggling between rendering to an image view (hybrid composition) and rendering
-      // directly to a Surface or Texture view.
-      // For more, https://github.com/flutter/flutter/issues/95343
+      // Stop rendering to the surface releases the associated native resources, which causes
+      // a glitch when toggling between rendering to an image view (hybrid composition) and
+      // rendering directly to a Surface or Texture view. For more,
+      // https://github.com/flutter/flutter/issues/95343
       stopRenderingToSurface();
     }
 
@@ -1062,10 +1068,10 @@ public void stopRenderingToSurface() {
     if (surface != null) {
       flutterJNI.onSurfaceDestroyed();
 
-      // TODO(mattcarroll): the source of truth for this call should be FlutterJNI, which is where
-      // the call to onFlutterUiDisplayed() comes from. However, no such native callback exists yet,
-      // so until the engine and FlutterJNI are configured to call us back when rendering stops, we
-      // will manually monitor that change here.
+      // TODO(mattcarroll): the source of truth for this call should be FlutterJNI, which is
+      // where the call to onFlutterUiDisplayed() comes from. However, no such native callback
+      // exists yet, so until the engine and FlutterJNI are configured to call us back when
+      // rendering stops, we will manually monitor that change here.
       if (isDisplayingFlutterUi) {
         flutterUiDisplayListener.onFlutterUiNoLongerDisplayed();
       }
diff --git a/shell/platform/android/io/flutter/embedding/engine/renderer/SurfaceTextureSurfaceProducer.java b/shell/platform/android/io/flutter/embedding/engine/renderer/SurfaceTextureSurfaceProducer.java
@@ -16,15 +16,19 @@ final class SurfaceTextureSurfaceProducer
   private int requestedBufferHeight;
   private boolean released;
   @Nullable private Surface surface;
-  @NonNull private final SurfaceTexture texture;
+  @NonNull private final TextureRegistry.SurfaceTextureEntry texture;
   @NonNull private final Handler handler;
   @NonNull private final FlutterJNI flutterJNI;
 
-  SurfaceTextureSurfaceProducer(long id, @NonNull Handler handler, @NonNull FlutterJNI flutterJNI) {
+  SurfaceTextureSurfaceProducer(
+      long id,
+      @NonNull Handler handler,
+      @NonNull FlutterJNI flutterJNI,
+      @NonNull TextureRegistry.SurfaceTextureEntry texture) {
     this.id = id;
     this.handler = handler;
     this.flutterJNI = flutterJNI;
-    this.texture = new SurfaceTexture(0);
+    this.texture = texture;
   }
 
   @Override
@@ -54,7 +58,7 @@ public void release() {
   @Override
   @NonNull
   public SurfaceTexture getSurfaceTexture() {
-    return texture;
+    return texture.surfaceTexture();
   }
 
   @Override
@@ -77,7 +81,7 @@ public int getHeight() {
   @Override
   public Surface getSurface() {
     if (surface == null) {
-      surface = new Surface(texture);
+      surface = new Surface(texture.surfaceTexture());
     }
     return surface;
   }
diff --git a/shell/platform/android/io/flutter/view/FlutterView.java b/shell/platform/android/io/flutter/view/FlutterView.java
@@ -968,6 +968,7 @@ public SurfaceTextureWrapper textureWrapper() {
       return textureWrapper;
     }
 
+    @NonNull
     @Override
     public SurfaceTexture surfaceTexture() {
       return textureWrapper.surfaceTexture();
diff --git a/shell/platform/android/test/io/flutter/embedding/engine/renderer/SurfaceTextureSurfaceProducerTest.java b/shell/platform/android/test/io/flutter/embedding/engine/renderer/SurfaceTextureSurfaceProducerTest.java
@@ -6,6 +6,7 @@
 
 import android.annotation.TargetApi;
 import android.graphics.Canvas;
+import android.graphics.SurfaceTexture;
 import android.os.Handler;
 import android.os.Looper;
 import android.view.Surface;
@@ -22,10 +23,13 @@ public final class SurfaceTextureSurfaceProducerTest {
 
   @Test
   public void createsSurfaceTextureOfGivenSizeAndResizesWhenRequested() {
+    final FlutterRenderer flutterRenderer = new FlutterRenderer(fakeJNI);
+
     // Create a surface and set the initial size.
     final Handler handler = new Handler(Looper.getMainLooper());
     final SurfaceTextureSurfaceProducer producer =
-        new SurfaceTextureSurfaceProducer(0, handler, fakeJNI);
+        new SurfaceTextureSurfaceProducer(
+            0, handler, fakeJNI, flutterRenderer.registerSurfaceTexture(new SurfaceTexture(0)));
     final Surface surface = producer.getSurface();
     AtomicInteger frames = new AtomicInteger();
     producer
diff --git a/shell/platform/android/test/io/flutter/plugin/platform/PlatformViewsControllerTest.java b/shell/platform/android/test/io/flutter/plugin/platform/PlatformViewsControllerTest.java
@@ -26,6 +26,7 @@
 import android.view.ViewParent;
 import android.widget.FrameLayout;
 import android.widget.FrameLayout.LayoutParams;
+import androidx.annotation.NonNull;
 import androidx.test.core.app.ApplicationProvider;
 import androidx.test.ext.junit.runners.AndroidJUnit4;
 import io.flutter.embedding.android.FlutterImageView;
@@ -1555,6 +1556,7 @@ public SurfaceTextureEntry createSurfaceTexture() {
           @Override
           public SurfaceTextureEntry registerSurfaceTexture(SurfaceTexture surfaceTexture) {
             return new SurfaceTextureEntry() {
+              @NonNull
               @Override
               public SurfaceTexture surfaceTexture() {
                 return mock(SurfaceTexture.class);
diff --git a/testing/scenario_app/android/README.md b/testing/scenario_app/android/README.md
@@ -17,6 +17,13 @@ Or for a specific, build, such as `android_debug_unopt_arm64`:
 dart ./testing/scenario_app/bin/run_android_tests.dart --out-dir=../out/android_debug_unopt_arm64
 ```
 
+See also:
+
+- [File an issue][file_issue] with the `e: scenario-app, platform-android`
+  labels.
+
+[file_issue]: https://github.com/flutter/flutter/issues/new?labels=e:%20scenario-app,engine,platform-android,fyi-android,team-engine
+
 ## Debugging
 
 Debugging the tests on CI is not straightforward but is being improved:
@@ -38,31 +45,51 @@ or locally in `out/.../scenario_app/logs`.
 
 You can then view the logs and screenshots on LUCI. [For example](https://ci.chromium.org/ui/p/flutter/builders/try/Linux%20Engine%20Drone/2003164/overview):
 
-![Screenshot of the Logs on LUCI](https://github.com/flutter/engine/assets/168174/79dc864c-c18b-4df9-a733-fd55301cc69c).
+![Screenshot of the Logs on LUCI](https://github.com/flutter/engine/assets/168174/79dc864c-c18b-4df9-a733-fd55301cc69c)
+
+For a full list of flags, see [the runner](../bin/README.md).
 
 ## CI Configuration
 
-See [`ci/builders/linux_android_emulator.json`](../../../ci/builders/linux_android_emulator.json)
-, and grep for `run_android_tests.dart`.
+See [`ci/builders`](../../../ci/builders) and grep for `run_android_tests.dart`.
+
+### Skia
+
+> [!NOTE]
+> As of 2024-02-28, Flutter on Android defaults to the Skia graphics backend.
+
+There are two code branches we test using `scenario_app`:
+
+- Older Android devices, that use `SurfaceTexture`.
+  - CI Configuration (TODO: Link)
+  - CI History (TODO: Link)
+  - Skia Gold (TODO: Link)
+- Newer Android devices, (API 34) that use `ImageReader`.
+  - CI Configuration (TODO: Link)
+  - CI History (TODO: Link)
+  - Skia Gold (TODO: Link)
+
+### Impeller with OpenGLES
+
+There are two code branches we test using `scenario_app`:
 
-The following matrix of configurations is tested on the CI:
+- Older Android devices, that use `SurfaceTexture`.
+  - CI Configuration (TODO: Link)
+  - CI History (TODO: Link)
+  - Skia Gold (TODO: Link)
+- Newer Android devices, (API 34) that use `ImageReader`.
+  - CI Configuration (TODO: Link)
+  - CI History (TODO: Link)
+  - Skia Gold (TODO: Link)
 
-<!-- TODO(matanlurey): Blocked by https://github.com/flutter/flutter/issues/143471.
-| 28          | Skia                | [Android 28 + Skia][skia-gold-skia-28]                           | Older Android devices (without `ImageReader`) on Skia.     |
-| 28          | Impeller (OpenGLES) | [Android 28 + Impeller OpenGLES][skia-gold-impeller-opengles-28] | Older Android devices (without `ImageReader`) on Impeller. |
-[skia-gold-skia-28]: https://flutter-engine-gold.skia.org/search?left_filter=AndroidAPILevel%3D28%26GraphicsBackend%3Dskia&negative=true&positive=true&right_filter=AndroidAPILevel%3D28%26GraphicsBackend%3Dskia
-[skia-gold-impeller-opengles-28]: https://flutter-engine-gold.skia.org/search?left_filter=AndroidAPILevel%3D28%26GraphicsBackend%3Dimpeller-opengles&negative=true&positive=true&right_filter=AndroidAPILevel%3D28%26GraphicsBackend%3Dimpeller-opengles
--->
+### Impeller with Vulkan
 
-| API Version | Graphics Backend    | Skia Gold                                                        | Rationale                                        |
-| ----------- | ------------------- | ---------------------------------------------------------------- | ------------------------------------------------ |
-| 34          | Skia                | [Android 34 + Skia][skia-gold-skia-34]                           | Newer Android devices on Skia.                   |
-| 34          | Impeller (OpenGLES) | [Android 34 + Impeller OpenGLES][skia-gold-impeller-opengles-34] | Newer Android devices on Impeller with OpenGLES. |
-| 34          | Impeller (Vulkan)   | [Android 34 + Impeller Vulkan][skia-gold-impeller-vulkan-34]     | Newer Android devices on Impeller.               |
+There is only a single code branch we test using `scenario_app`:
 
-[skia-gold-skia-34]: https://flutter-engine-gold.skia.org/search?left_filter=AndroidAPILevel%3D34%26GraphicsBackend%3Dskia&negative=true&positive=true&right_filter=AndroidAPILevel%3D34%26GraphicsBackend%3Dskia
-[skia-gold-impeller-opengles-34]: https://flutter-engine-gold.skia.org/search?left_filter=AndroidAPILevel%3D34%26GraphicsBackend%3Dimpeller-opengles&negative=true&positive=true&right_filter=AndroidAPILevel%3D34%26GraphicsBackend%3Dimpeller-opengles
-[skia-gold-impeller-vulkan-34]: https://flutter-engine-gold.skia.org/search?left_filter=AndroidAPILevel%3D34%26GraphicsBackend%3Dimpeller-vulkan&negative=true&positive=true&right_filter=AndroidAPILevel%3D34%26GraphicsBackend%3Dimpeller-vulkan
+- Newer Android devices, (API 34)
+  - CI Configuration (TODO: Link)
+  - CI History (TODO: Link)
+  - Skia Gold (TODO: Link)
 
 ## Updating Gradle dependencies
 
diff --git a/testing/scenario_app/android/app/src/androidTest/java/dev/flutter/TestRunner.java b/testing/scenario_app/android/app/src/androidTest/java/dev/flutter/TestRunner.java
@@ -9,11 +9,13 @@
 import androidx.test.runner.AndroidJUnitRunner;
 import dev.flutter.scenariosui.ScreenshotUtil;
 import io.flutter.FlutterInjector;
+import io.flutter.embedding.engine.renderer.FlutterRenderer;
 
 public class TestRunner extends AndroidJUnitRunner {
   @Override
   public void onCreate(@Nullable Bundle arguments) {
     String[] engineArguments = null;
+    assert arguments != null;
     if ("true".equals(arguments.getString("enable-impeller"))) {
       // Set up the global settings object so that Impeller is enabled for all tests.
       engineArguments =
@@ -22,6 +24,10 @@ public void onCreate(@Nullable Bundle arguments) {
             "--impeller-backend=" + arguments.getString("impeller-backend", "vulkan")
           };
     }
+    if ("true".equals(arguments.getString("force-surface-producer-surface-texture"))) {
+      // Set a test flag to force the SurfaceProducer to use SurfaceTexture.
+      FlutterRenderer.debugForceSurfaceProducerGlTextures = true;
+    }
     // For consistency, just always initilaize FlutterJNI etc.
     FlutterInjector.instance().flutterLoader().startInitialization(getTargetContext());
     FlutterInjector.instance()
diff --git a/testing/scenario_app/android/app/src/main/java/dev/flutter/scenarios/ExternalTextureFlutterActivity.java b/testing/scenario_app/android/app/src/main/java/dev/flutter/scenarios/ExternalTextureFlutterActivity.java
diff --git a/testing/scenario_app/bin/README.md b/testing/scenario_app/bin/README.md
diff --git a/testing/scenario_app/bin/run_android_tests.dart b/testing/scenario_app/bin/run_android_tests.dart
diff --git a/testing/scenario_app/bin/utils/options.dart b/testing/scenario_app/bin/utils/options.dart

Original file line number	Diff line number	Diff line change
`@@ -968,6 +968,7 @@ public SurfaceTextureWrapper textureWrapper() {`
`968`	`968`	`return textureWrapper;`
`969`	`969`	`}`
`970`	`970`
	`971`	`+ @NonNull`
`971`	`972`	`@Override`
`972`	`973`	`public SurfaceTexture surfaceTexture() {`
`973`	`974`	`return textureWrapper.surfaceTexture();`