Do not trigger onInvalidate callback if the SurfacEdge is closed.
Currently post-processing pipeline causes an infinite loop on SurfaceView. The root cause is that the SurfaceRequest.invalidate is called on a closed edge, which makes Preview sending a new SurfaceRequest for the closed edge.
- Separate disconnect() from close(). Disconnected edge can be connected again.
- Closed edge should never be called again. Added checks to enforce that.
- Once closed, invalidating SurfaceRequest won't trigger onInvalidateCallback.
Bug:266743420
Test: manual test and ./gradlew bOS
Change-Id: I6b6c1f9aba65c2e8af492943499482ef3b6c50db
diff --git a/camera/camera-core/src/main/java/androidx/camera/core/processing/SurfaceEdge.java b/camera/camera-core/src/main/java/androidx/camera/core/processing/SurfaceEdge.java
index ca3c2df..fa70c2e 100644
--- a/camera/camera-core/src/main/java/androidx/camera/core/processing/SurfaceEdge.java
+++ b/camera/camera-core/src/main/java/androidx/camera/core/processing/SurfaceEdge.java
@@ -41,6 +41,7 @@
import androidx.annotation.NonNull;
import androidx.annotation.Nullable;
import androidx.annotation.RequiresApi;
+import androidx.annotation.VisibleForTesting;
import androidx.camera.core.CameraEffect;
import androidx.camera.core.SurfaceOutput;
import androidx.camera.core.SurfaceProcessor;
@@ -124,6 +125,11 @@
@NonNull
private final Set<Runnable> mOnInvalidatedListeners = new HashSet<>();
+ // Guarded by main thread.
+ // Tombstone flag indicates whether the edge has been closed. Once closed, the edge should
+ // never be used again.
+ private boolean mIsClosed = false;
+
/**
* Please see the getters to understand the parameters.
*/
@@ -156,6 +162,7 @@
@MainThread
public void addOnInvalidatedListener(@NonNull Runnable onInvalidated) {
checkMainThread();
+ checkNotClosed();
mOnInvalidatedListeners.add(onInvalidated);
}
@@ -170,6 +177,7 @@
@MainThread
public DeferrableSurface getDeferrableSurface() {
checkMainThread();
+ checkNotClosed();
checkAndSetHasConsumer();
return mSettableSurface;
}
@@ -209,6 +217,7 @@
public void setProvider(@NonNull DeferrableSurface provider)
throws DeferrableSurface.SurfaceClosedException {
checkMainThread();
+ checkNotClosed();
mSettableSurface.setProvider(provider);
}
@@ -248,10 +257,15 @@
public SurfaceRequest createSurfaceRequest(@NonNull CameraInternal cameraInternal,
@Nullable Range<Integer> expectedFpsRange) {
checkMainThread();
+ checkNotClosed();
// TODO(b/238230154) figure out how to support HDR.
SurfaceRequest surfaceRequest = new SurfaceRequest(mStreamSpec.getResolution(),
cameraInternal, expectedFpsRange,
- () -> mainThreadExecutor().execute(this::invalidate));
+ () -> mainThreadExecutor().execute(() -> {
+ if (!mIsClosed) {
+ invalidate();
+ }
+ }));
try {
DeferrableSurface deferrableSurface = surfaceRequest.getDeferrableSurface();
if (mSettableSurface.setProvider(deferrableSurface)) {
@@ -299,6 +313,7 @@
public ListenableFuture<SurfaceOutput> createSurfaceOutputFuture(@NonNull Size inputSize,
@NonNull Rect cropRect, int rotationDegrees, boolean mirroring) {
checkMainThread();
+ checkNotClosed();
checkAndSetHasConsumer();
SettableSurface settableSurface = mSettableSurface;
return transformAsync(mSettableSurface.getSurface(),
@@ -321,7 +336,7 @@
}
/**
- * Closes the current connection and notifies that a new connection is ready.
+ * Resets connection and notifies that a new connection is ready.
*
* <p>Call this method to notify that the {@link Surface} previously provided via
* {@link #createSurfaceRequest} or {@link #setProvider} should no longer be used. The
@@ -337,7 +352,12 @@
@MainThread
public void invalidate() {
checkMainThread();
- close();
+ checkNotClosed();
+ if (mSettableSurface.canSetProvider()) {
+ // If the edge is still connectable, no-ops.
+ return;
+ }
+ disconnectWithoutCheckingClosed();
mHasConsumer = false;
mSettableSurface = new SettableSurface(mStreamSpec.getResolution());
for (Runnable onInvalidated : mOnInvalidatedListeners) {
@@ -346,20 +366,41 @@
}
/**
- * Closes the current connection.
+ * Closes the edge.
*
- * <p>This method uses the mechanism in {@link DeferrableSurface} and/or
- * {@link SurfaceOutputImpl} to notify the upstream pipeline that the {@link Surface}
- * previously provided via {@link #createSurfaceRequest} or {@link #setProvider} should no
- * longer be used. The upstream pipeline will stops writing to the {@link Surface}, and the
- * downstream pipeline can choose to release the {@link Surface} once the writing stops.
+ * <p> Disconnects the edge and sets a tombstone so it will never be used again. This method
+ * is idempotent.
+ *
+ * @see #disconnect()
+ */
+ @MainThread
+ public final void close() {
+ checkMainThread();
+ disconnectWithoutCheckingClosed();
+ mIsClosed = true;
+ }
+
+ /**
+ * Disconnects the edge.
+ *
+ * <p> Once disconnected, upstream should stop sending images to the edge, and downstream
+ * should stop expecting images from the edge.
+ *
+ * <p> This method notifies the upstream via {@link SettableSurface#close()}/
+ * {@link SurfaceOutputImpl}. By calling {@link SettableSurface#close()}, it also decrements the
+ * ref-count on downstream Surfaces so they can be released.
*
* @see DeferrableSurface#close().
* @see #invalidate()
*/
@MainThread
- public final void close() {
+ public final void disconnect() {
checkMainThread();
+ checkNotClosed();
+ disconnectWithoutCheckingClosed();
+ }
+
+ private void disconnectWithoutCheckingClosed() {
mSettableSurface.close();
if (mConsumerToNotify != null) {
mConsumerToNotify.requestClose();
@@ -476,6 +517,16 @@
return mStreamSpec;
}
+ private void checkNotClosed() {
+ checkState(!mIsClosed, "Edge is already closed.");
+ }
+
+ @VisibleForTesting
+ @NonNull
+ DeferrableSurface getDeferrableSurfaceForTesting() {
+ return mSettableSurface;
+ }
+
/**
* A {@link DeferrableSurface} that sets another {@link DeferrableSurface} as the source.
*
@@ -504,6 +555,12 @@
return mSurfaceFuture;
}
+ @MainThread
+ boolean canSetProvider() {
+ checkMainThread();
+ return mProvider == null && !isClosed();
+ }
+
/**
* Sets the {@link DeferrableSurface} that provides the surface.
*
diff --git a/camera/camera-core/src/test/java/androidx/camera/core/processing/SurfaceEdgeTest.kt b/camera/camera-core/src/test/java/androidx/camera/core/processing/SurfaceEdgeTest.kt
index 60e066b..6aca2c6 100644
--- a/camera/camera-core/src/test/java/androidx/camera/core/processing/SurfaceEdgeTest.kt
+++ b/camera/camera-core/src/test/java/androidx/camera/core/processing/SurfaceEdgeTest.kt
@@ -93,6 +93,49 @@
}
@Test
+ fun closeEdgeThenInvalidate_callbackNotInvoked() {
+ // Arrange.
+ var invalidated = false
+ surfaceEdge.addOnInvalidatedListener {
+ invalidated = true
+ }
+ val surfaceRequest = surfaceEdge.createSurfaceRequest(FakeCamera())
+ // Act.
+ surfaceEdge.close()
+ surfaceRequest.invalidate()
+ shadowOf(getMainLooper()).idle()
+ // Assert.
+ assertThat(invalidated).isFalse()
+ }
+
+ @Test
+ fun invalidateWithoutProvider_settableSurfaceNotRecreated() {
+ // Arrange.
+ val deferrableSurface = surfaceEdge.deferrableSurfaceForTesting
+ // Act.
+ surfaceEdge.invalidate()
+ // Assert.
+ assertThat(surfaceEdge.deferrableSurfaceForTesting).isSameInstanceAs(deferrableSurface)
+ }
+
+ @Test
+ fun invalidateWithProvider_settableSurfaceNotRecreated() {
+ // Arrange.
+ val deferrableSurface = surfaceEdge.deferrableSurfaceForTesting
+ surfaceEdge.createSurfaceRequest(FakeCamera())
+ // Act.
+ surfaceEdge.invalidate()
+ // Assert.
+ assertThat(surfaceEdge.deferrableSurfaceForTesting).isNotSameInstanceAs(deferrableSurface)
+ }
+
+ @Test
+ fun callCloseTwice_noException() {
+ surfaceEdge.close()
+ surfaceEdge.close()
+ }
+
+ @Test
fun createWithStreamSpec_canGetStreamSpec() {
val edge = SurfaceEdge(
PREVIEW, FRAME_SPEC, Matrix(), true, Rect(), 0, false
@@ -106,6 +149,24 @@
surfaceEdge.setProvider(provider)
}
+ @Test(expected = IllegalStateException::class)
+ fun getDeferrableSurfaceOnClosedEdge_throwsException() {
+ surfaceEdge.close()
+ surfaceEdge.deferrableSurface
+ }
+
+ @Test(expected = IllegalStateException::class)
+ fun getSurfaceOutputOnClosedEdge_throwsException() {
+ surfaceEdge.close()
+ createSurfaceOutputFuture(surfaceEdge)
+ }
+
+ @Test(expected = IllegalStateException::class)
+ fun createSurfaceRequest_throwsException() {
+ surfaceEdge.close()
+ surfaceEdge.createSurfaceRequest(FakeCamera())
+ }
+
@Test
fun provideSurfaceThenImmediatelyInvalidate_surfaceOutputFails() {
// Arrange: create SurfaceOutput and set provider.
@@ -189,10 +250,10 @@
}
@Test
- fun createSurfaceRequestThenClose_surfaceRequestCancelled() {
+ fun createSurfaceRequestThenDisconnect_surfaceRequestCancelled() {
// Arrange: create a SurfaceRequest then close.
val surfaceRequest = surfaceEdge.createSurfaceRequest(FakeCamera())
- surfaceEdge.close()
+ surfaceEdge.disconnect()
// Act: provide a Surface and get the result.
var result: SurfaceRequest.Result? = null
@@ -206,9 +267,9 @@
}
@Test
- fun createSurfaceOutputWithClosedInstance_surfaceOutputNotCreated() {
+ fun createSurfaceOutputWithDisconnectedEdge_surfaceOutputNotCreated() {
// Arrange: create a SurfaceOutput future from a closed LinkableSurface
- surfaceEdge.close()
+ surfaceEdge.disconnect()
val surfaceOutput = createSurfaceOutputFuture(surfaceEdge)
// Act: wait for the SurfaceOutput to return.
@@ -338,7 +399,7 @@
assertThat(isSurfaceReleased).isEqualTo(false)
// Act: close the LinkableSurface, signaling the intention to close the Surface.
- surfaceEdge.close()
+ surfaceEdge.disconnect()
shadowOf(getMainLooper()).idle()
// Assert: The close is propagated to the SurfaceRequest.
