Fix CaptureFailedRetryQuirk failing for previous request info
Relnote: "Fixed retry not triggering properly in case of capture failure in problematic devices mentioned in CaptureFailedRetryQuirk."
Bug: 288209032
Test: TakePictureManagerTest, ImageCaptureTest and other related tests
Change-Id: I7b589a19bf5ee8f316c009e6c69dd21b7cf6a0c3
diff --git a/camera/camera-core/src/androidTest/java/androidx/camera/core/ImageCaptureTest.java b/camera/camera-core/src/androidTest/java/androidx/camera/core/ImageCaptureTest.java
index dba781b..6c21ada 100644
--- a/camera/camera-core/src/androidTest/java/androidx/camera/core/ImageCaptureTest.java
+++ b/camera/camera-core/src/androidTest/java/androidx/camera/core/ImageCaptureTest.java
@@ -49,6 +49,7 @@
import androidx.camera.core.impl.UseCaseConfigFactory;
import androidx.camera.core.impl.utils.executor.CameraXExecutors;
import androidx.camera.core.internal.CameraUseCaseAdapter;
+import androidx.camera.core.internal.compat.workaround.CaptureFailedRetryEnabler;
import androidx.camera.testing.CoreAppTestUtil;
import androidx.camera.testing.fakes.FakeCamera;
import androidx.camera.testing.fakes.FakeCameraCaptureResult;
@@ -536,6 +537,9 @@
// Act.
// Complete the picture taken, then new flash mode should be applied.
+ CaptureFailedRetryEnabler retryEnabler = new CaptureFailedRetryEnabler();
+ // Because of retry in some devices, we may need to notify capture failures multiple times.
+ addExtraFailureNotificationsForRetry(fakeCameraControl, retryEnabler.getRetryCount());
fakeCameraControl.notifyAllRequestsOnCaptureFailed();
// Assert.
@@ -543,6 +547,16 @@
assertThat(fakeCameraControl.getFlashMode()).isEqualTo(ImageCapture.FLASH_MODE_ON);
}
+ private void addExtraFailureNotificationsForRetry(FakeCameraControl cameraControl,
+ int retryCount) {
+ if (retryCount > 0) {
+ cameraControl.setOnNewCaptureRequestListener(captureConfigs -> {
+ addExtraFailureNotificationsForRetry(cameraControl, retryCount - 1);
+ cameraControl.notifyAllRequestsOnCaptureFailed();
+ });
+ }
+ }
+
@Test
public void correctViewPortRectInResolutionInfo_withCropAspectRatioSetting() {
ImageCapture imageCapture = new ImageCapture.Builder()
diff --git a/camera/camera-core/src/main/java/androidx/camera/core/imagecapture/RequestWithCallback.java b/camera/camera-core/src/main/java/androidx/camera/core/imagecapture/RequestWithCallback.java
index 58ff19d..4903752 100644
--- a/camera/camera-core/src/main/java/androidx/camera/core/imagecapture/RequestWithCallback.java
+++ b/camera/camera-core/src/main/java/androidx/camera/core/imagecapture/RequestWithCallback.java
@@ -149,13 +149,19 @@
// Fail silently if the request has been aborted.
return;
}
- if (mTakePictureRequest.decrementRetryCounter()) {
- mRetryControl.retryRequest(mTakePictureRequest);
- } else {
+
+ boolean isRetryAllowed = mTakePictureRequest.decrementRetryCounter();
+ if (!isRetryAllowed) {
onFailure(imageCaptureException);
}
markComplete();
mCaptureCompleter.setException(imageCaptureException);
+
+ if (isRetryAllowed) {
+ // retry after all the cleaning up works are done via mCaptureCompleter.setException,
+ // e.g. removing previous request from CaptureNode, SingleBundlingNode etc.
+ mRetryControl.retryRequest(mTakePictureRequest);
+ }
}
@MainThread
diff --git a/camera/camera-core/src/main/java/androidx/camera/core/imagecapture/TakePictureManager.java b/camera/camera-core/src/main/java/androidx/camera/core/imagecapture/TakePictureManager.java
index 3ba10d1..e2c6ae5 100644
--- a/camera/camera-core/src/main/java/androidx/camera/core/imagecapture/TakePictureManager.java
+++ b/camera/camera-core/src/main/java/androidx/camera/core/imagecapture/TakePictureManager.java
@@ -37,6 +37,7 @@
import androidx.camera.core.ImageCapture;
import androidx.camera.core.ImageCaptureException;
import androidx.camera.core.ImageProxy;
+import androidx.camera.core.Logger;
import androidx.camera.core.impl.utils.futures.FutureCallback;
import androidx.camera.core.impl.utils.futures.Futures;
import androidx.core.util.Pair;
@@ -120,8 +121,11 @@
@Override
public void retryRequest(@NonNull TakePictureRequest request) {
checkMainThread();
+ Logger.d(TAG, "Add a new request for retrying.");
// Insert the request to the front of the queue.
mNewRequests.addFirst(request);
+ // Try to issue the newly added request in case condition allows.
+ issueNextRequest();
}
/**
diff --git a/camera/camera-core/src/test/java/androidx/camera/core/imagecapture/FakeImageCaptureControl.kt b/camera/camera-core/src/test/java/androidx/camera/core/imagecapture/FakeImageCaptureControl.kt
index d0b5089..85043d9 100644
--- a/camera/camera-core/src/test/java/androidx/camera/core/imagecapture/FakeImageCaptureControl.kt
+++ b/camera/camera-core/src/test/java/androidx/camera/core/imagecapture/FakeImageCaptureControl.kt
@@ -39,11 +39,17 @@
// Flip this flag to return a custom result using pendingResultCompleter.
var shouldUsePendingResult = false
lateinit var pendingResultCompleter: CallbackToFutureAdapter.Completer<Void>
- var pendingResult = CallbackToFutureAdapter.getFuture { completer ->
+ var pendingResult = createPendingResult()
+
+ private fun createPendingResult() = CallbackToFutureAdapter.getFuture { completer ->
pendingResultCompleter = completer
"FakeImageCaptureControl's pendingResult"
}
+ fun resetPendingResult() {
+ pendingResult = createPendingResult()
+ }
+
override fun lockFlashMode() {
actions.add(Action.LOCK_FLASH)
}
diff --git a/camera/camera-core/src/test/java/androidx/camera/core/imagecapture/TakePictureManagerTest.kt b/camera/camera-core/src/test/java/androidx/camera/core/imagecapture/TakePictureManagerTest.kt
index 2a6cf328..2f8d74f 100644
--- a/camera/camera-core/src/test/java/androidx/camera/core/imagecapture/TakePictureManagerTest.kt
+++ b/camera/camera-core/src/test/java/androidx/camera/core/imagecapture/TakePictureManagerTest.kt
@@ -24,10 +24,14 @@
import androidx.camera.core.ImageCapture.ERROR_CAPTURE_FAILED
import androidx.camera.core.ImageCapture.OutputFileResults
import androidx.camera.core.ImageCaptureException
+import androidx.camera.core.imagecapture.FakeImageCaptureControl.Action.SUBMIT_REQUESTS
import androidx.camera.core.impl.CaptureConfig
+import androidx.camera.core.internal.compat.quirk.CaptureFailedRetryQuirk
+import androidx.camera.core.internal.compat.quirk.DeviceQuirks
import androidx.camera.testing.fakes.FakeImageInfo
import androidx.camera.testing.fakes.FakeImageProxy
import com.google.common.truth.Truth.assertThat
+import com.google.common.truth.Truth.assertWithMessage
import org.junit.After
import org.junit.Test
import org.junit.runner.RunWith
@@ -35,6 +39,7 @@
import org.robolectric.Shadows.shadowOf
import org.robolectric.annotation.Config
import org.robolectric.annotation.internal.DoNotInstrument
+import org.robolectric.shadows.ShadowBuild
/**
* Unit tests for [TakePictureManager].
@@ -133,7 +138,7 @@
// Assert: one request is sent.
assertThat(imageCaptureControl.actions).containsExactly(
FakeImageCaptureControl.Action.LOCK_FLASH,
- FakeImageCaptureControl.Action.SUBMIT_REQUESTS,
+ SUBMIT_REQUESTS,
FakeImageCaptureControl.Action.UNLOCK_FLASH,
).inOrder()
// Both request are aborted.
@@ -184,7 +189,7 @@
// Assert: only one request is sent.
assertThat(imageCaptureControl.actions).containsExactly(
FakeImageCaptureControl.Action.LOCK_FLASH,
- FakeImageCaptureControl.Action.SUBMIT_REQUESTS,
+ SUBMIT_REQUESTS,
FakeImageCaptureControl.Action.UNLOCK_FLASH,
).inOrder()
@@ -195,10 +200,10 @@
// Assert: 2nd request is sent too.
assertThat(imageCaptureControl.actions).containsExactly(
FakeImageCaptureControl.Action.LOCK_FLASH,
- FakeImageCaptureControl.Action.SUBMIT_REQUESTS,
+ SUBMIT_REQUESTS,
FakeImageCaptureControl.Action.UNLOCK_FLASH,
FakeImageCaptureControl.Action.LOCK_FLASH,
- FakeImageCaptureControl.Action.SUBMIT_REQUESTS,
+ SUBMIT_REQUESTS,
FakeImageCaptureControl.Action.UNLOCK_FLASH,
).inOrder()
}
@@ -275,7 +280,7 @@
// Assert:
assertThat(imageCaptureControl.actions).containsExactly(
FakeImageCaptureControl.Action.LOCK_FLASH,
- FakeImageCaptureControl.Action.SUBMIT_REQUESTS,
+ SUBMIT_REQUESTS,
FakeImageCaptureControl.Action.UNLOCK_FLASH,
).inOrder()
assertThat(imageCaptureControl.latestCaptureConfigs).isEqualTo(response1)
@@ -288,10 +293,10 @@
// Assert: imageCaptureControl was invoked in the exact given order.
assertThat(imageCaptureControl.actions).containsExactly(
FakeImageCaptureControl.Action.LOCK_FLASH,
- FakeImageCaptureControl.Action.SUBMIT_REQUESTS,
+ SUBMIT_REQUESTS,
FakeImageCaptureControl.Action.UNLOCK_FLASH,
FakeImageCaptureControl.Action.LOCK_FLASH,
- FakeImageCaptureControl.Action.SUBMIT_REQUESTS,
+ SUBMIT_REQUESTS,
FakeImageCaptureControl.Action.UNLOCK_FLASH,
).inOrder()
assertThat(imageCaptureControl.latestCaptureConfigs).isEqualTo(response2)
@@ -399,7 +404,7 @@
assertThat(takePictureManager.mNewRequests.size).isEqualTo(0)
assertThat(imageCaptureControl.actions).containsExactly(
FakeImageCaptureControl.Action.LOCK_FLASH,
- FakeImageCaptureControl.Action.SUBMIT_REQUESTS,
+ SUBMIT_REQUESTS,
FakeImageCaptureControl.Action.UNLOCK_FLASH,
).inOrder()
}
@@ -443,4 +448,74 @@
// Assert. new request can be issued after the capture failure of the first request
takePictureManager.offerRequest(request2)
}
-}
\ No newline at end of file
+
+ @Test
+ fun requestFailure_failureReportedIfQuirkDisabled() {
+ // Arrange: use the real ImagePipeline implementation to do the test
+ takePictureManager.mImagePipeline =
+ ImagePipeline(Utils.createEmptyImageCaptureConfig(), Size(640, 480))
+
+ // Create a request and offer it to the manager.
+ imageCaptureControl.shouldUsePendingResult = true
+ val request = FakeTakePictureRequest(FakeTakePictureRequest.Type.IN_MEMORY)
+ takePictureManager.offerRequest(request)
+
+ // Act: make the request fail once.
+ imageCaptureControl.pendingResultCompleter.setException(
+ ImageCaptureException(
+ ERROR_CAPTURE_FAILED, "", null
+ )
+ )
+ shadowOf(getMainLooper()).idle()
+
+ // Assert: failure exception received and no more retry possible.
+ assertThat((request.exceptionReceived as ImageCaptureException).imageCaptureError)
+ .isEqualTo(ERROR_CAPTURE_FAILED)
+ assertThat(request.remainingRetries).isEqualTo(0)
+ // Only 1 request submitted to camera
+ assertThat(imageCaptureControl.actions.count { it == SUBMIT_REQUESTS }).isEqualTo(1)
+ }
+
+ @Test
+ fun requestFailure_retriedIfQuirkEnabled() {
+ // Arrange: enable retry related quirk.
+ ShadowBuild.setBrand("SAMSUNG")
+ ShadowBuild.setModel("SM-G981U1")
+
+ val captureFailedRetryQuirk = DeviceQuirks.get(CaptureFailedRetryQuirk::class.java)
+
+ assertWithMessage("CaptureFailedRetryQuirk not enabled!")
+ .that(captureFailedRetryQuirk).isNotNull()
+
+ // Use the real ImagePipeline implementation to do the test
+ takePictureManager.mImagePipeline =
+ ImagePipeline(Utils.createEmptyImageCaptureConfig(), Size(640, 480))
+
+ // Create a request and offer it to the manager.
+ imageCaptureControl.shouldUsePendingResult = true
+ val request = FakeTakePictureRequest(FakeTakePictureRequest.Type.IN_MEMORY)
+ takePictureManager.offerRequest(request)
+
+ // Act: make the request fail once and then successful if retried later.
+ imageCaptureControl.pendingResultCompleter.setException(
+ ImageCaptureException(
+ ERROR_CAPTURE_FAILED, "", null
+ )
+ )
+ imageCaptureControl.resetPendingResult()
+
+ // complete the new capture successfully
+ imageCaptureControl.pendingResultCompleter.set(null)
+ shadowOf(getMainLooper()).idle()
+
+ // Assert: retry count decremented without any failure.
+ assertThat(request.remainingRetries).isEqualTo(captureFailedRetryQuirk!!.retryCount - 1)
+ assertThat(request.exceptionReceived).isNull()
+ // 2 requests submitted to camera
+ assertThat(imageCaptureControl.actions.count { it == SUBMIT_REQUESTS }).isEqualTo(2)
+
+ // Clean-up brands and models set for enabling quirk.
+ ShadowBuild.setBrand("")
+ ShadowBuild.setModel("")
+ }
+}
