From 43599a528afb681eff407c5fb1d0d9ef981c5064 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Wed, 24 Jan 2024 12:15:11 -0800 Subject: [PATCH] Do not double-increment `nextId` when using `createSurfaceProducer` with `SurfaceTexture`s (flutter/engine#50011) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The change is _very_ tiny, but I added some documentation as well. After this PR, it appears we're "ready" to start using this API once it lands in stable. Hoorah! Here are local tests using `video_player_android/example`: ## Skia ![Screenshot 2024-01-24 at 10 59 17 AM](https://github.com/flutter/engine/assets/168174/0cc890ee-4c43-47d3-8d3e-4503f5b1a545) ## Impeller ![Screenshot 2024-01-24 at 10 59 46 AM](https://github.com/flutter/engine/assets/168174/bf13a0f0-bcc7-40bf-a521-b61720dab0d9) --- .../flutter/shell/platform/android/README.md | 78 +++++++++++++++++++ .../engine/renderer/FlutterRenderer.java | 17 +++- .../engine/renderer/FlutterRendererTest.java | 16 ++++ 3 files changed, 109 insertions(+), 2 deletions(-) diff --git a/engine/src/flutter/shell/platform/android/README.md b/engine/src/flutter/shell/platform/android/README.md index 09eeb869ad..3bfcec6bbf 100644 --- a/engine/src/flutter/shell/platform/android/README.md +++ b/engine/src/flutter/shell/platform/android/README.md @@ -23,6 +23,84 @@ See also: - [Testing Android Changes in the Devicelab on an Emulator](https://github.com/flutter/flutter/wiki/Testing-Android-Changes-in-the-Devicelab-on-an-Emulator) - [Texture Layer Hybrid Composition](https://github.com/flutter/flutter/wiki/Texture-Layer-Hybrid-Composition) +## Testing + +There are two classes of tests for the Android embedder: unit tests that tend +to test _contracts_ within the embedder, and integration tests that test the +engine and embedder together, typically coupled with a Flutter app (that's how +our users interact with the engine and embedder). + +### Unit tests + +Unit tests for the Android embedder are located in: + +- [`test`](test): Java unit tests. +- C++ files that end in `_unittests.cc` in [`shell/platform/android`](./). + +The easiest way (though not the quickest) is to use `run_tests.py`: + +```shell +# Assuming you're at the root of the engine repo where `run_tests.py` is located. +./testing/run_tests.py --type java + +# Confusingly, these are C++ tests for Android. +./testing/run_tests.py --type android + +# If you're using android_debug_unopt_arm64 builds: +./testing/run_tests.py --type android --android-variant android_debug_unopt_arm64 +./testing/run_tests.py --type java --android-variant android_debug_unopt_arm64 +``` + +You may also be able to run the tests directly from Android Studio. + +### Integration tests + +Integration tests for the Android embedder mostly exist outside of the engine +for dubious historical reasons. + +To run these tests, you'll need to have a Flutter checkout and a working Android +emulator or device: + +```shell +# Build an up-to-date Flutter engine for Android. +cd $ENGINE/src + +# Or, use your favorite arguments and build variant. +ninja -j1000 -C ../out/android_debug_unopt_arm64 android_jar + +# Run the tests. Here is *1* example: +cd $FLUTTER/dev/integration_tests/external_textures +flutter drive \ + --local-engine-host=$ENGINE/out/host_debug_unopt_arm64 \ + --local-engine=$ENGINE/out/android_debug_unopt_arm64 +``` + +Another good source of (unfortunately, manual) testing is `flutter/packages`: + +```shell +cd $PACKAGES/packages/video_player/video_player_android/example +flutter run \ + --local-engine-host=$ENGINE/out/host_debug_unopt_arm64 \ + --local-engine=$ENGINE/out/android_debug_unopt_arm64 +``` + +> ![NOTE] +> External texture rendering on Android is based on the device API level. For +> example to test the OpenGLES branch (which uses `SurfaceTexture`), you'll +> typically need an older device or emulator with an API version 29 or lower. +> +> You can also (locally) "force" the engine to use `SurfaceTextures`: +> +> ```diff +> // shell/platform/android/io/flutter/embedding/engine/renderer/FlutterRenderer.java +> - @VisibleForTesting static boolean debugForceSurfaceProducerGlTextures = false; +> + @VisibleForTesting static boolean debugForceSurfaceProducerGlTextures = true; +> ``` +> +> ... and rebuild the engine. + +See [our wiki](https://github.com/flutter/flutter/wiki/Testing-the-engine#java---android-embedding) also. + ## Developing How to edit and contribute to the Android embedder. diff --git a/engine/src/flutter/shell/platform/android/io/flutter/embedding/engine/renderer/FlutterRenderer.java b/engine/src/flutter/shell/platform/android/io/flutter/embedding/engine/renderer/FlutterRenderer.java index dd67c7f718..e6dacdf6fa 100644 --- a/engine/src/flutter/shell/platform/android/io/flutter/embedding/engine/renderer/FlutterRenderer.java +++ b/engine/src/flutter/shell/platform/android/io/flutter/embedding/engine/renderer/FlutterRenderer.java @@ -198,7 +198,7 @@ public class FlutterRenderer implements TextureRegistry { } else { final SurfaceTextureSurfaceProducer producer = new SurfaceTextureSurfaceProducer(id, handler, flutterJNI); - registerSurfaceTexture(producer.getSurfaceTexture()); + registerSurfaceTexture(id, producer.getSurfaceTexture()); Log.v(TAG, "New SurfaceTextureSurfaceProducer ID: " + id); entry = producer; } @@ -224,9 +224,22 @@ public class FlutterRenderer implements TextureRegistry { @NonNull @Override public SurfaceTextureEntry registerSurfaceTexture(@NonNull SurfaceTexture surfaceTexture) { + return registerSurfaceTexture(nextTextureId.getAndIncrement(), surfaceTexture); + } + + /** + * Similar to {@link FlutterRenderer#registerSurfaceTexture} but with an existing @{code + * textureId}. + * + * @param surfaceTexture Surface texture to wrap. + * @param textureId A texture ID already created that should be assigned to the surface texture. + */ + @NonNull + private SurfaceTextureEntry registerSurfaceTexture( + long textureId, @NonNull SurfaceTexture surfaceTexture) { surfaceTexture.detachFromGLContext(); final SurfaceTextureRegistryEntry entry = - new SurfaceTextureRegistryEntry(nextTextureId.getAndIncrement(), surfaceTexture); + new SurfaceTextureRegistryEntry(textureId, surfaceTexture); Log.v(TAG, "New SurfaceTexture ID: " + entry.id()); registerTexture(entry.id(), entry.textureWrapper()); addOnTrimMemoryListener(entry); diff --git a/engine/src/flutter/shell/platform/android/test/io/flutter/embedding/engine/renderer/FlutterRendererTest.java b/engine/src/flutter/shell/platform/android/test/io/flutter/embedding/engine/renderer/FlutterRendererTest.java index 3330d34f82..b2f2043e21 100644 --- a/engine/src/flutter/shell/platform/android/test/io/flutter/embedding/engine/renderer/FlutterRendererTest.java +++ b/engine/src/flutter/shell/platform/android/test/io/flutter/embedding/engine/renderer/FlutterRendererTest.java @@ -5,6 +5,7 @@ import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; +import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyFloat; import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.ArgumentMatchers.eq; @@ -599,4 +600,19 @@ public class FlutterRendererTest { // We will have no pending readers to close. assertEquals(0, texture.readersToCloseSize()); } + + @Test + public void SurfaceTextureSurfaceProducerCreatesAConnectedTexture() { + // Force creating a SurfaceTextureSurfaceProducer regardless of Android API version. + FlutterRenderer.debugForceSurfaceProducerGlTextures = true; + + FlutterRenderer flutterRenderer = new FlutterRenderer(fakeFlutterJNI); + TextureRegistry.SurfaceProducer producer = flutterRenderer.createSurfaceProducer(); + + flutterRenderer.startRenderingToSurface(fakeSurface, false); + + // Verify behavior under test. + assertEquals(producer.id(), 0); + verify(fakeFlutterJNI, times(1)).registerTexture(eq(producer.id()), any()); + } }