Skip to content

Commit c7362b4

Browse files
authored
[Android] Add a way to request new Surfaces from SurfaceProducer and avoid SurfaceProducer returning invalid Surface (flutter#169899)
> [!NOTE] > For anyone reviewing this PR, see flutter/packages#9360 for how I'd use the `getSurface(boolean forceNewSurface)` method. > [!NOTE] > For anyone coming across this PR post-landing, in the code review process, I renamed `getSurface(boolean forceNewSurface)` to `getForcedNewSurface()`. ### What this does (1) Adds method `getSurface(boolean forceNewSurface)` to `SurfaceProducer` that will force the creation of a new `Surface` when `SurfaceProducer.getSurface()` is called. (2) Fixes `SurfaceProducer` to avoid returning invalid `Surface`s when `SurfaceProducer.getSurface()`/`getSurface(boolean forceNewSurface)` is called. ### Why we should... #### (1) Add `getSurface(boolean forceNewSurface)` My motivation for adding this is directly tied to flutter#155294. The `camera_android_camerax` plugin supports a camera preview use case that requires providing a `Surface` to in order to render the preview. It does so via `SurfaceProducer`; we provide a `Surface` retrieved from `SurfaceProducer.getSurface()` to the CameraX library to render a camera preview, and when the camera preview is done, a callback that we provide is called, reporting that `Surface` as used and that it now should be released/invalidated (see [`SurfaceRequest`](https://developer.android.com/reference/androidx/camera/core/SurfaceRequest), [`SurfaceRequest.Result`](https://developer.android.com/reference/androidx/camera/core/SurfaceRequest#provideSurface(android.view.Surface,java.util.concurrent.Executor,androidx.core.util.Consumer%3Candroidx.camera.core.SurfaceRequest.Result%3E))). However, the CameraX library [makes no guarantees](https://developer.android.com/reference/androidx/camera/core/Preview.SurfaceProvider#onSurfaceRequested(androidx.camera.core.SurfaceRequest):~:text=The%20camera%20may%20repeatedly%20request%20surfaces%20throughout%20usage%20of%20a%20Preview%20use%20case%2C%20but%20only%20a%20single%20request%20will%20be%20active%20at%20a%20time.) about when requests for new `Surface`s are made, so the following race condition can happen: 1. CameraX requests a `Surface` 2. We provide `Surface` A from `SurfaceProducer.getSurface()` 3. The camera preview was paused; CameraX requests a new `Surface` 4. The camera preview is resumed 5. We provide `Surface` A from `SurfaceProducer.getSurface()` because it is still technically valid 6. CameraX calls our callback and now `Surface` A is released/invalid I believe `SurfaceProducer` currently has no way to handle the level of complexity of `Surface` handling required for use cases like the `camera_android_camerax` camera preview, where a `Surface` may be invalidated between calls to `SurfaceProducer.getSurface()`. So, `getSurface(boolean forceNewSurface)` can support these use cases.* `getSurface(boolean forceNewSurface)` simply will force `SurfaceProducer` to return a new `Surface` when `SurfaceProducer.getSurface` is called versus potentially returning the same `Surface` as the previous invocation. For `camera_android_camerax`, see flutter/packages#9360 for an example of how it would be used. *I'd like to note that I also played around with creating new `SurfaceProducer`s to solve the camera problem, which _was_ successful and probably could be generally for that use case, but I think it's cleaner to support this functionality from within the same `SurfaceProducer` since we can 🤷‍♀️ It also helps avoid the user from needing to create/manage multiple Flutter `Texture`s, from my understanding. #### (2) Fix `SurfaceProducer.getSurface()`/`getSurface(boolean forceNewSurface)` to never return an invalid `Surface`s I noticed that `SurfaceProducer.getSurface()`/`getSurface(boolean forceNewSurface)` does not check for `Surface` validity before returning them while I was working on (1). Honestly, this just feels right? We should ensure that a `Surface` is valid and can be used before returning it and potentially confusing the user. ## 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]. <!-- Links --> [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
1 parent 9a8cbb8 commit c7362b4

File tree

6 files changed

+215
-14
lines changed

6 files changed

+215
-14
lines changed

engine/src/flutter/shell/platform/android/io/flutter/embedding/engine/renderer/FlutterRenderer.java

Lines changed: 30 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -555,10 +555,10 @@ void close() {
555555
return (double) deltaNanos / 1000000.0;
556556
}
557557

558-
PerImageReader getOrCreatePerImageReader(ImageReader reader) {
558+
private PerImageReader getOrCreatePerImageReader(ImageReader reader) {
559559
PerImageReader r = perImageReaders.get(reader);
560560
if (r == null) {
561-
r = new PerImageReader(reader);
561+
r = createPerImageReader(reader);
562562
perImageReaders.put(reader, r);
563563
imageReaderQueue.add(r);
564564
if (VERBOSE_LOGS) {
@@ -568,6 +568,11 @@ PerImageReader getOrCreatePerImageReader(ImageReader reader) {
568568
return r;
569569
}
570570

571+
@VisibleForTesting
572+
public PerImageReader createPerImageReader(ImageReader reader) {
573+
return new PerImageReader(reader);
574+
}
575+
571576
void pruneImageReaderQueue() {
572577
boolean change = false;
573578
// Prune nodes from the head of the ImageReader queue.
@@ -829,6 +834,12 @@ public Surface getSurface() {
829834
return pir.reader.getSurface();
830835
}
831836

837+
@Override
838+
public Surface getForcedNewSurface() {
839+
createNewReader = true;
840+
return getSurface();
841+
}
842+
832843
@Override
833844
public void scheduleFrame() {
834845
if (VERBOSE_LOGS) {
@@ -855,17 +866,23 @@ public Image acquireLatestImage() {
855866

856867
private PerImageReader getActiveReader() {
857868
synchronized (lock) {
858-
if (createNewReader) {
859-
createNewReader = false;
860-
// Create a new ImageReader and add it to the queue.
861-
ImageReader reader = createImageReader();
862-
if (VERBOSE_LOGS) {
863-
Log.i(
864-
TAG, reader.hashCode() + " created w=" + requestedWidth + " h=" + requestedHeight);
869+
if (!createNewReader) {
870+
// Verify we don't need a new ImageReader anyway because its Surface has been invalidated.
871+
PerImageReader lastPerImageReader = imageReaderQueue.peekLast();
872+
Surface lastImageReaderSurface = lastPerImageReader.reader.getSurface();
873+
boolean lastImageReaderHasValidSurface = lastImageReaderSurface.isValid();
874+
if (lastImageReaderHasValidSurface) {
875+
return lastPerImageReader;
865876
}
866-
return getOrCreatePerImageReader(reader);
867877
}
868-
return imageReaderQueue.peekLast();
878+
879+
createNewReader = false;
880+
// Create a new ImageReader and add it to the queue.
881+
ImageReader reader = createImageReader();
882+
if (VERBOSE_LOGS) {
883+
Log.i(TAG, reader.hashCode() + " created w=" + requestedWidth + " h=" + requestedHeight);
884+
}
885+
return getOrCreatePerImageReader(reader);
869886
}
870887
}
871888

@@ -909,7 +926,8 @@ private ImageReader createImageReader29() {
909926
HardwareBuffer.USAGE_GPU_SAMPLED_IMAGE);
910927
}
911928

912-
private ImageReader createImageReader() {
929+
@VisibleForTesting
930+
public ImageReader createImageReader() {
913931
if (Build.VERSION.SDK_INT >= API_LEVELS.API_33) {
914932
return createImageReader33();
915933
} else if (Build.VERSION.SDK_INT >= API_LEVELS.API_29) {

engine/src/flutter/shell/platform/android/io/flutter/embedding/engine/renderer/SurfaceTextureSurfaceProducer.java

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
import android.view.Surface;
66
import androidx.annotation.NonNull;
77
import androidx.annotation.Nullable;
8+
import androidx.annotation.VisibleForTesting;
89
import io.flutter.embedding.engine.FlutterJNI;
910
import io.flutter.view.TextureRegistry;
1011

@@ -92,12 +93,23 @@ public int getHeight() {
9293

9394
@Override
9495
public Surface getSurface() {
95-
if (surface == null) {
96-
surface = new Surface(texture.surfaceTexture());
96+
if (surface == null || !surface.isValid()) {
97+
surface = createSurface(texture.surfaceTexture());
9798
}
9899
return surface;
99100
}
100101

102+
@Override
103+
public Surface getForcedNewSurface() {
104+
surface = null;
105+
return getSurface();
106+
}
107+
108+
@VisibleForTesting
109+
public Surface createSurface(SurfaceTexture surfaceTexture) {
110+
return new Surface(surfaceTexture);
111+
}
112+
101113
@Override
102114
public void scheduleFrame() {
103115
flutterJNI.markTextureFrameAvailable(id);

engine/src/flutter/shell/platform/android/io/flutter/view/TextureRegistry.java

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -138,6 +138,24 @@ interface SurfaceProducer extends TextureEntry {
138138
*/
139139
Surface getSurface();
140140

141+
/**
142+
* Direct access to a surface, which will be newly created (and thus, different from any surface
143+
* objects returned from previous calls to {@link #getSurface()} or {@link
144+
* #getForcedNewSurface()}.
145+
*
146+
* <p>When using this API, you will usually need to implement {@link SurfaceProducer.Callback}
147+
* and provide it to {@link #setCallback(Callback)} in order to be notified when an existing
148+
* surface has been destroyed (such as when the application goes to the background) or a new
149+
* surface has been created (such as when the application is resumed back to the foreground).
150+
*
151+
* <p>NOTE: You should not cache the returned surface but instead invoke {@code getSurface} each
152+
* time you need to draw. The surface may change when the texture is resized or has its format
153+
* changed.
154+
*
155+
* @return a Surface to use for a drawing target for various APIs.
156+
*/
157+
Surface getForcedNewSurface();
158+
141159
/**
142160
* Sets a callback that is notified when a previously created {@link Surface} returned by {@link
143161
* SurfaceProducer#getSurface()} is no longer valid due to being destroyed, or a new surface is

engine/src/flutter/shell/platform/android/test/io/flutter/embedding/engine/renderer/FlutterRendererTest.java

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
import static org.junit.Assert.assertArrayEquals;
1010
import static org.junit.Assert.assertEquals;
1111
import static org.junit.Assert.assertFalse;
12+
import static org.junit.Assert.assertNotEquals;
1213
import static org.junit.Assert.assertNotNull;
1314
import static org.junit.Assert.assertNull;
1415
import static org.junit.Assert.assertTrue;
@@ -21,12 +22,14 @@
2122
import static org.mockito.Mockito.spy;
2223
import static org.mockito.Mockito.times;
2324
import static org.mockito.Mockito.verify;
25+
import static org.mockito.Mockito.when;
2426
import static org.robolectric.Shadows.shadowOf;
2527

2628
import android.graphics.Canvas;
2729
import android.graphics.Rect;
2830
import android.graphics.SurfaceTexture;
2931
import android.media.Image;
32+
import android.media.ImageReader;
3033
import android.os.Looper;
3134
import android.view.Surface;
3235
import androidx.lifecycle.Lifecycle;
@@ -991,4 +994,72 @@ public void ImageReaderSurfaceProducerSchedulesFrameIfQueueNotEmpty() throws Exc
991994
// is now empty.
992995
verify(flutterRenderer, times(3)).scheduleEngineFrame();
993996
}
997+
998+
@Test
999+
public void getSurface_doesNotReturnInvalidSurface() {
1000+
FlutterRenderer flutterRenderer = spy(engineRule.getFlutterEngine().getRenderer());
1001+
TextureRegistry.SurfaceProducer producer = flutterRenderer.createSurfaceProducer();
1002+
FlutterRenderer.ImageReaderSurfaceProducer spyImageReaderSurfaceProducer =
1003+
spy((FlutterRenderer.ImageReaderSurfaceProducer) producer);
1004+
ImageReader mockImageReader = mock(ImageReader.class);
1005+
ImageReader mockSecondImageReader = mock(ImageReader.class);
1006+
Surface firstMockSurface = mock(Surface.class);
1007+
Surface secondMockSurface = mock(Surface.class);
1008+
1009+
when(mockImageReader.getSurface()).thenReturn(firstMockSurface);
1010+
when(mockSecondImageReader.getSurface()).thenReturn(secondMockSurface);
1011+
when(firstMockSurface.isValid()).thenReturn(false);
1012+
when(spyImageReaderSurfaceProducer.createImageReader())
1013+
.thenReturn(mockImageReader)
1014+
.thenReturn(mockSecondImageReader);
1015+
1016+
Surface firstSurface = spyImageReaderSurfaceProducer.getSurface();
1017+
Surface secondSurface = spyImageReaderSurfaceProducer.getSurface();
1018+
1019+
assertNotEquals(firstSurface, secondSurface);
1020+
assertEquals(firstSurface, firstMockSurface);
1021+
assertEquals(secondSurface, secondMockSurface);
1022+
}
1023+
1024+
@Test
1025+
public void getSurface_consecutiveCallsReturnSameSurfaceIfStillValid() {
1026+
FlutterRenderer flutterRenderer = spy(engineRule.getFlutterEngine().getRenderer());
1027+
TextureRegistry.SurfaceProducer producer = flutterRenderer.createSurfaceProducer();
1028+
FlutterRenderer.ImageReaderSurfaceProducer spyImageReaderSurfaceProducer =
1029+
spy((FlutterRenderer.ImageReaderSurfaceProducer) producer);
1030+
ImageReader mockImageReader = mock(ImageReader.class);
1031+
Surface mockSurface = mock(Surface.class);
1032+
1033+
when(mockSurface.isValid()).thenReturn(true);
1034+
when(mockImageReader.getSurface()).thenReturn(mockSurface);
1035+
when(spyImageReaderSurfaceProducer.createImageReader()).thenReturn(mockImageReader);
1036+
1037+
Surface firstSurface = spyImageReaderSurfaceProducer.getSurface();
1038+
Surface secondSurface = spyImageReaderSurfaceProducer.getSurface();
1039+
1040+
assertEquals(firstSurface, secondSurface);
1041+
assertEquals(firstSurface, mockSurface);
1042+
}
1043+
1044+
@Test
1045+
public void getForcedNewSurface_returnsNewSurface() {
1046+
FlutterRenderer flutterRenderer = spy(engineRule.getFlutterEngine().getRenderer());
1047+
TextureRegistry.SurfaceProducer producer = flutterRenderer.createSurfaceProducer();
1048+
1049+
Surface firstSurface = producer.getSurface();
1050+
Surface secondSurface = producer.getForcedNewSurface();
1051+
1052+
assertNotEquals(firstSurface, secondSurface);
1053+
}
1054+
1055+
@Test
1056+
public void getSurface_doesNotReturnNewSurface() {
1057+
FlutterRenderer flutterRenderer = spy(engineRule.getFlutterEngine().getRenderer());
1058+
TextureRegistry.SurfaceProducer producer = flutterRenderer.createSurfaceProducer();
1059+
1060+
Surface firstSurface = producer.getSurface();
1061+
Surface secondSurface = producer.getSurface();
1062+
1063+
assertEquals(firstSurface, secondSurface);
1064+
}
9941065
}

engine/src/flutter/shell/platform/android/test/io/flutter/embedding/engine/renderer/SurfaceTextureSurfaceProducerTest.java

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,11 @@
77
import static io.flutter.Build.API_LEVELS;
88
import static org.junit.Assert.assertEquals;
99
import static org.junit.Assert.assertFalse;
10+
import static org.junit.Assert.assertNotEquals;
1011
import static org.junit.Assert.assertTrue;
1112
import static org.mockito.Mockito.mock;
13+
import static org.mockito.Mockito.spy;
14+
import static org.mockito.Mockito.when;
1215
import static org.robolectric.Shadows.shadowOf;
1316

1417
import android.annotation.TargetApi;
@@ -19,6 +22,7 @@
1922
import android.view.Surface;
2023
import androidx.test.ext.junit.runners.AndroidJUnit4;
2124
import io.flutter.embedding.engine.FlutterJNI;
25+
import io.flutter.view.TextureRegistry;
2226
import java.util.concurrent.atomic.AtomicInteger;
2327
import org.junit.Test;
2428
import org.junit.runner.RunWith;
@@ -84,4 +88,77 @@ public void releaseWillReleaseSurface() {
8488
producer.release();
8589
assertFalse(surface.isValid());
8690
}
91+
92+
@Test
93+
public void getSurface_doesNotReturnInvalidSurface() {
94+
final FlutterRenderer flutterRenderer = new FlutterRenderer(fakeJNI);
95+
final Handler handler = new Handler(Looper.getMainLooper());
96+
final SurfaceTexture mockSurfaceTexture = mock(SurfaceTexture.class);
97+
final TextureRegistry.SurfaceTextureEntry spyTexture =
98+
spy(flutterRenderer.registerSurfaceTexture(mockSurfaceTexture));
99+
final SurfaceTextureSurfaceProducer producerSpy =
100+
spy(new SurfaceTextureSurfaceProducer(0, handler, fakeJNI, spyTexture));
101+
final Surface firstMockSurface = mock(Surface.class);
102+
final Surface secondMockSurface = mock(Surface.class);
103+
104+
when(spyTexture.surfaceTexture()).thenReturn(mockSurfaceTexture);
105+
when(firstMockSurface.isValid()).thenReturn(false);
106+
when(producerSpy.createSurface(mockSurfaceTexture))
107+
.thenReturn(firstMockSurface)
108+
.thenReturn(secondMockSurface);
109+
110+
final Surface firstSurface = producerSpy.getSurface();
111+
final Surface secondSurface = producerSpy.getSurface();
112+
113+
assertNotEquals(firstSurface, secondSurface);
114+
}
115+
116+
@Test
117+
public void getSurface_consecutiveCallsReturnSameSurfaceIfStillValid() {
118+
final FlutterRenderer flutterRenderer = new FlutterRenderer(fakeJNI);
119+
final Handler handler = new Handler(Looper.getMainLooper());
120+
final SurfaceTexture mockSurfaceTexture = mock(SurfaceTexture.class);
121+
final TextureRegistry.SurfaceTextureEntry spyTexture =
122+
spy(flutterRenderer.registerSurfaceTexture(mockSurfaceTexture));
123+
final SurfaceTextureSurfaceProducer producerSpy =
124+
spy(new SurfaceTextureSurfaceProducer(0, handler, fakeJNI, spyTexture));
125+
final Surface mockSurface = mock(Surface.class);
126+
127+
when(spyTexture.surfaceTexture()).thenReturn(mockSurfaceTexture);
128+
when(mockSurface.isValid()).thenReturn(true);
129+
when(producerSpy.createSurface(mockSurfaceTexture)).thenReturn(mockSurface);
130+
131+
final Surface firstSurface = producerSpy.getSurface();
132+
final Surface secondSurface = producerSpy.getSurface();
133+
134+
assertEquals(firstSurface, secondSurface);
135+
}
136+
137+
@Test
138+
public void getForcedNewSurface_returnsNewSurface() {
139+
final FlutterRenderer flutterRenderer = new FlutterRenderer(fakeJNI);
140+
final Handler handler = new Handler(Looper.getMainLooper());
141+
final SurfaceTextureSurfaceProducer producer =
142+
new SurfaceTextureSurfaceProducer(
143+
0, handler, fakeJNI, flutterRenderer.registerSurfaceTexture(new SurfaceTexture(0)));
144+
145+
final Surface firstSurface = producer.getSurface();
146+
final Surface secondSurface = producer.getForcedNewSurface();
147+
148+
assertNotEquals(firstSurface, secondSurface);
149+
}
150+
151+
@Test
152+
public void getSurface_doesNotReturnNewSurface() {
153+
final FlutterRenderer flutterRenderer = new FlutterRenderer(fakeJNI);
154+
final Handler handler = new Handler(Looper.getMainLooper());
155+
final SurfaceTextureSurfaceProducer producer =
156+
new SurfaceTextureSurfaceProducer(
157+
0, handler, fakeJNI, flutterRenderer.registerSurfaceTexture(new SurfaceTexture(0)));
158+
159+
Surface firstSurface = producer.getSurface();
160+
Surface secondSurface = producer.getSurface();
161+
162+
assertEquals(firstSurface, secondSurface);
163+
}
87164
}

engine/src/flutter/shell/platform/android/test/io/flutter/plugin/platform/PlatformViewsControllerTest.java

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1689,6 +1689,11 @@ public Surface getSurface() {
16891689
return null;
16901690
}
16911691

1692+
@Override
1693+
public Surface getForcedNewSurface() {
1694+
return null;
1695+
}
1696+
16921697
@Override
16931698
public boolean handlesCropAndRotation() {
16941699
return false;

0 commit comments

Comments
 (0)