Camera: Fix race for onCaptureBufferLost callback
The callback holder was removed when the capture sequence is
completed, which is too soon because the buffer loss callback could
potentially arrives later than the capture sequence completion.
Defer the deletion of the callback holder to when the native inflight
request is removed, which takes into consideration of error
notifications.
Test: Camera CTS
Bug: 155353799
Change-Id: I12b716acc9fbaa9791f0498ac77d4470a7448d5a
diff --git a/core/java/android/hardware/camera2/impl/CameraDeviceImpl.java b/core/java/android/hardware/camera2/impl/CameraDeviceImpl.java
index 6d49add..23c8602 100644
--- a/core/java/android/hardware/camera2/impl/CameraDeviceImpl.java
+++ b/core/java/android/hardware/camera2/impl/CameraDeviceImpl.java
@@ -1072,7 +1072,7 @@
* @param lastFrameNumber last frame number returned from binder.
* @param repeatingRequestTypes the repeating requests' types.
*/
- private void checkEarlyTriggerSequenceComplete(
+ private void checkEarlyTriggerSequenceCompleteLocked(
final int requestId, final long lastFrameNumber,
final int[] repeatingRequestTypes) {
// lastFrameNumber being equal to NO_FRAMES_CAPTURED means that the request
@@ -1212,7 +1212,7 @@
if (repeating) {
if (mRepeatingRequestId != REQUEST_ID_NONE) {
- checkEarlyTriggerSequenceComplete(mRepeatingRequestId,
+ checkEarlyTriggerSequenceCompleteLocked(mRepeatingRequestId,
requestInfo.getLastFrameNumber(),
mRepeatingRequestTypes);
}
@@ -1269,7 +1269,7 @@
return;
}
- checkEarlyTriggerSequenceComplete(requestId, lastFrameNumber, requestTypes);
+ checkEarlyTriggerSequenceCompleteLocked(requestId, lastFrameNumber, requestTypes);
}
}
}
@@ -1302,7 +1302,7 @@
long lastFrameNumber = mRemoteDevice.flush();
if (mRepeatingRequestId != REQUEST_ID_NONE) {
- checkEarlyTriggerSequenceComplete(mRepeatingRequestId, lastFrameNumber,
+ checkEarlyTriggerSequenceCompleteLocked(mRepeatingRequestId, lastFrameNumber,
mRepeatingRequestTypes);
mRepeatingRequestId = REQUEST_ID_NONE;
mRepeatingRequestTypes = null;
@@ -1442,78 +1442,135 @@
long completedFrameNumber = mFrameNumberTracker.getCompletedFrameNumber();
long completedReprocessFrameNumber = mFrameNumberTracker.getCompletedReprocessFrameNumber();
long completedZslStillFrameNumber = mFrameNumberTracker.getCompletedZslStillFrameNumber();
- boolean isReprocess = false;
+
Iterator<RequestLastFrameNumbersHolder> iter = mRequestLastFrameNumbersList.iterator();
while (iter.hasNext()) {
final RequestLastFrameNumbersHolder requestLastFrameNumbers = iter.next();
- boolean sequenceCompleted = false;
final int requestId = requestLastFrameNumbers.getRequestId();
final CaptureCallbackHolder holder;
- synchronized(mInterfaceLock) {
- if (mRemoteDevice == null) {
- Log.w(TAG, "Camera closed while checking sequences");
- return;
+ if (mRemoteDevice == null) {
+ Log.w(TAG, "Camera closed while checking sequences");
+ return;
+ }
+ if (!requestLastFrameNumbers.isSequenceCompleted()) {
+ long lastRegularFrameNumber =
+ requestLastFrameNumbers.getLastRegularFrameNumber();
+ long lastReprocessFrameNumber =
+ requestLastFrameNumbers.getLastReprocessFrameNumber();
+ long lastZslStillFrameNumber =
+ requestLastFrameNumbers.getLastZslStillFrameNumber();
+ if (lastRegularFrameNumber <= completedFrameNumber
+ && lastReprocessFrameNumber <= completedReprocessFrameNumber
+ && lastZslStillFrameNumber <= completedZslStillFrameNumber) {
+ Log.v(TAG, String.format(
+ "Mark requestId %d as completed, because lastRegularFrame %d "
+ + "is <= %d, lastReprocessFrame %d is <= %d, "
+ + "lastZslStillFrame %d is <= %d", requestId,
+ lastRegularFrameNumber, completedFrameNumber,
+ lastReprocessFrameNumber, completedReprocessFrameNumber,
+ lastZslStillFrameNumber, completedZslStillFrameNumber));
+ requestLastFrameNumbers.markSequenceCompleted();
}
+ // Call onCaptureSequenceCompleted
int index = mCaptureCallbackMap.indexOfKey(requestId);
holder = (index >= 0) ?
mCaptureCallbackMap.valueAt(index) : null;
- if (holder != null) {
- long lastRegularFrameNumber =
- requestLastFrameNumbers.getLastRegularFrameNumber();
- long lastReprocessFrameNumber =
- requestLastFrameNumbers.getLastReprocessFrameNumber();
- long lastZslStillFrameNumber =
- requestLastFrameNumbers.getLastZslStillFrameNumber();
- // check if it's okay to remove request from mCaptureCallbackMap
- if (lastRegularFrameNumber <= completedFrameNumber
- && lastReprocessFrameNumber <= completedReprocessFrameNumber
- && lastZslStillFrameNumber <= completedZslStillFrameNumber) {
- sequenceCompleted = true;
- mCaptureCallbackMap.removeAt(index);
- if (DEBUG) {
- Log.v(TAG, String.format(
- "Remove holder for requestId %d, because lastRegularFrame %d "
- + "is <= %d, lastReprocessFrame %d is <= %d, "
- + "lastZslStillFrame %d is <= %d", requestId,
- lastRegularFrameNumber, completedFrameNumber,
- lastReprocessFrameNumber, completedReprocessFrameNumber,
- lastZslStillFrameNumber, completedZslStillFrameNumber));
+ if (holder != null && requestLastFrameNumbers.isSequenceCompleted()) {
+ Runnable resultDispatch = new Runnable() {
+ @Override
+ public void run() {
+ if (!CameraDeviceImpl.this.isClosed()){
+ if (DEBUG) {
+ Log.d(TAG, String.format(
+ "fire sequence complete for request %d",
+ requestId));
+ }
+
+ holder.getCallback().onCaptureSequenceCompleted(
+ CameraDeviceImpl.this,
+ requestId,
+ requestLastFrameNumbers.getLastFrameNumber());
+ }
}
+ };
+ final long ident = Binder.clearCallingIdentity();
+ try {
+ holder.getExecutor().execute(resultDispatch);
+ } finally {
+ Binder.restoreCallingIdentity(ident);
}
}
}
- // If no callback is registered for this requestId or sequence completed, remove it
- // from the frame number->request pair because it's not needed anymore.
- if (holder == null || sequenceCompleted) {
+ if (requestLastFrameNumbers.isSequenceCompleted() &&
+ requestLastFrameNumbers.isInflightCompleted()) {
+ int index = mCaptureCallbackMap.indexOfKey(requestId);
+ if (index >= 0) {
+ mCaptureCallbackMap.removeAt(index);
+ }
+ if (DEBUG) {
+ Log.v(TAG, String.format(
+ "Remove holder for requestId %d", requestId));
+ }
iter.remove();
}
+ }
+ }
- // Call onCaptureSequenceCompleted
- if (sequenceCompleted) {
- Runnable resultDispatch = new Runnable() {
- @Override
- public void run() {
- if (!CameraDeviceImpl.this.isClosed()){
- if (DEBUG) {
- Log.d(TAG, String.format(
- "fire sequence complete for request %d",
- requestId));
- }
+ private void removeCompletedCallbackHolderLocked(long lastCompletedRegularFrameNumber,
+ long lastCompletedReprocessFrameNumber, long lastCompletedZslStillFrameNumber) {
+ if (DEBUG) {
+ Log.v(TAG, String.format("remove completed callback holders for "
+ + "lastCompletedRegularFrameNumber %d, "
+ + "lastCompletedReprocessFrameNumber %d, "
+ + "lastCompletedZslStillFrameNumber %d",
+ lastCompletedRegularFrameNumber,
+ lastCompletedReprocessFrameNumber,
+ lastCompletedZslStillFrameNumber));
+ }
- holder.getCallback().onCaptureSequenceCompleted(
- CameraDeviceImpl.this,
- requestId,
- requestLastFrameNumbers.getLastFrameNumber());
- }
+ Iterator<RequestLastFrameNumbersHolder> iter = mRequestLastFrameNumbersList.iterator();
+ while (iter.hasNext()) {
+ final RequestLastFrameNumbersHolder requestLastFrameNumbers = iter.next();
+ final int requestId = requestLastFrameNumbers.getRequestId();
+ final CaptureCallbackHolder holder;
+ if (mRemoteDevice == null) {
+ Log.w(TAG, "Camera closed while removing completed callback holders");
+ return;
+ }
+
+ long lastRegularFrameNumber =
+ requestLastFrameNumbers.getLastRegularFrameNumber();
+ long lastReprocessFrameNumber =
+ requestLastFrameNumbers.getLastReprocessFrameNumber();
+ long lastZslStillFrameNumber =
+ requestLastFrameNumbers.getLastZslStillFrameNumber();
+
+ if (lastRegularFrameNumber <= lastCompletedRegularFrameNumber
+ && lastReprocessFrameNumber <= lastCompletedReprocessFrameNumber
+ && lastZslStillFrameNumber <= lastCompletedZslStillFrameNumber) {
+
+ if (requestLastFrameNumbers.isSequenceCompleted()) {
+ int index = mCaptureCallbackMap.indexOfKey(requestId);
+ if (index >= 0) {
+ mCaptureCallbackMap.removeAt(index);
}
- };
- final long ident = Binder.clearCallingIdentity();
- try {
- holder.getExecutor().execute(resultDispatch);
- } finally {
- Binder.restoreCallingIdentity(ident);
+ if (DEBUG) {
+ Log.v(TAG, String.format(
+ "Remove holder for requestId %d, because lastRegularFrame %d "
+ + "is <= %d, lastReprocessFrame %d is <= %d, "
+ + "lastZslStillFrame %d is <= %d", requestId,
+ lastRegularFrameNumber, lastCompletedRegularFrameNumber,
+ lastReprocessFrameNumber, lastCompletedReprocessFrameNumber,
+ lastZslStillFrameNumber, lastCompletedZslStillFrameNumber));
+ }
+ iter.remove();
+ } else {
+ if (DEBUG) {
+ Log.v(TAG, "Sequence not yet completed for request id " + requestId);
+ }
+ requestLastFrameNumbers.markInflightCompleted();
}
}
}
@@ -1702,6 +1759,12 @@
return;
}
+ // Remove all capture callbacks now that device has gone to IDLE state.
+ removeCompletedCallbackHolderLocked(
+ Long.MAX_VALUE, /*lastCompletedRegularFrameNumber*/
+ Long.MAX_VALUE, /*lastCompletedReprocessFrameNumber*/
+ Long.MAX_VALUE /*lastCompletedZslStillFrameNumber*/);
+
if (!CameraDeviceImpl.this.mIdle) {
final long ident = Binder.clearCallingIdentity();
try {
@@ -1747,7 +1810,7 @@
return;
}
- checkEarlyTriggerSequenceComplete(mRepeatingRequestId, lastFrameNumber,
+ checkEarlyTriggerSequenceCompleteLocked(mRepeatingRequestId, lastFrameNumber,
mRepeatingRequestTypes);
// Check if there is already a new repeating request
if (mRepeatingRequestId == repeatingRequestId) {
@@ -1766,9 +1829,18 @@
public void onCaptureStarted(final CaptureResultExtras resultExtras, final long timestamp) {
int requestId = resultExtras.getRequestId();
final long frameNumber = resultExtras.getFrameNumber();
+ final long lastCompletedRegularFrameNumber =
+ resultExtras.getLastCompletedRegularFrameNumber();
+ final long lastCompletedReprocessFrameNumber =
+ resultExtras.getLastCompletedReprocessFrameNumber();
+ final long lastCompletedZslFrameNumber =
+ resultExtras.getLastCompletedZslFrameNumber();
if (DEBUG) {
- Log.d(TAG, "Capture started for id " + requestId + " frame number " + frameNumber);
+ Log.d(TAG, "Capture started for id " + requestId + " frame number " + frameNumber
+ + ": completedRegularFrameNumber " + lastCompletedRegularFrameNumber
+ + ", completedReprocessFrameNUmber " + lastCompletedReprocessFrameNumber
+ + ", completedZslFrameNumber " + lastCompletedZslFrameNumber);
}
final CaptureCallbackHolder holder;
@@ -1784,6 +1856,12 @@
return;
}
+ // Check if it's okay to remove completed callbacks from mCaptureCallbackMap.
+ // A callback is completed if the corresponding inflight request has been removed
+ // from the inflight queue in cameraservice.
+ removeCompletedCallbackHolderLocked(lastCompletedRegularFrameNumber,
+ lastCompletedReprocessFrameNumber, lastCompletedZslFrameNumber);
+
// Get the callback for this frame ID, if there is one
holder = CameraDeviceImpl.this.mCaptureCallbackMap.get(requestId);
diff --git a/core/java/android/hardware/camera2/impl/CameraOfflineSessionImpl.java b/core/java/android/hardware/camera2/impl/CameraOfflineSessionImpl.java
index 1d9d644..413caf5 100644
--- a/core/java/android/hardware/camera2/impl/CameraOfflineSessionImpl.java
+++ b/core/java/android/hardware/camera2/impl/CameraOfflineSessionImpl.java
@@ -182,6 +182,12 @@
return;
}
+ // Remove all capture callbacks now that device has gone to IDLE state.
+ removeCompletedCallbackHolderLocked(
+ Long.MAX_VALUE, /*lastCompletedRegularFrameNumber*/
+ Long.MAX_VALUE, /*lastCompletedReprocessFrameNumber*/
+ Long.MAX_VALUE /*lastCompletedZslStillFrameNumber*/);
+
Runnable idleDispatch = new Runnable() {
@Override
public void run() {
@@ -204,10 +210,22 @@
public void onCaptureStarted(final CaptureResultExtras resultExtras, final long timestamp) {
int requestId = resultExtras.getRequestId();
final long frameNumber = resultExtras.getFrameNumber();
+ final long lastCompletedRegularFrameNumber =
+ resultExtras.getLastCompletedRegularFrameNumber();
+ final long lastCompletedReprocessFrameNumber =
+ resultExtras.getLastCompletedReprocessFrameNumber();
+ final long lastCompletedZslFrameNumber =
+ resultExtras.getLastCompletedZslFrameNumber();
final CaptureCallbackHolder holder;
synchronized(mInterfaceLock) {
+ // Check if it's okay to remove completed callbacks from mCaptureCallbackMap.
+ // A callback is completed if the corresponding inflight request has been removed
+ // from the inflight queue in cameraservice.
+ removeCompletedCallbackHolderLocked(lastCompletedRegularFrameNumber,
+ lastCompletedReprocessFrameNumber, lastCompletedZslFrameNumber);
+
// Get the callback for this frame ID, if there is one
holder = CameraOfflineSessionImpl.this.mCaptureCallbackMap.get(requestId);
@@ -601,6 +619,61 @@
}
}
+ private void removeCompletedCallbackHolderLocked(long lastCompletedRegularFrameNumber,
+ long lastCompletedReprocessFrameNumber, long lastCompletedZslStillFrameNumber) {
+ if (DEBUG) {
+ Log.v(TAG, String.format("remove completed callback holders for "
+ + "lastCompletedRegularFrameNumber %d, "
+ + "lastCompletedReprocessFrameNumber %d, "
+ + "lastCompletedZslStillFrameNumber %d",
+ lastCompletedRegularFrameNumber,
+ lastCompletedReprocessFrameNumber,
+ lastCompletedZslStillFrameNumber));
+ }
+
+ boolean isReprocess = false;
+ Iterator<RequestLastFrameNumbersHolder> iter =
+ mOfflineRequestLastFrameNumbersList.iterator();
+ while (iter.hasNext()) {
+ final RequestLastFrameNumbersHolder requestLastFrameNumbers = iter.next();
+ final int requestId = requestLastFrameNumbers.getRequestId();
+ final CaptureCallbackHolder holder;
+
+ int index = mCaptureCallbackMap.indexOfKey(requestId);
+ holder = (index >= 0) ?
+ mCaptureCallbackMap.valueAt(index) : null;
+ if (holder != null) {
+ long lastRegularFrameNumber =
+ requestLastFrameNumbers.getLastRegularFrameNumber();
+ long lastReprocessFrameNumber =
+ requestLastFrameNumbers.getLastReprocessFrameNumber();
+ long lastZslStillFrameNumber =
+ requestLastFrameNumbers.getLastZslStillFrameNumber();
+ if (lastRegularFrameNumber <= lastCompletedRegularFrameNumber
+ && lastReprocessFrameNumber <= lastCompletedReprocessFrameNumber
+ && lastZslStillFrameNumber <= lastCompletedZslStillFrameNumber) {
+ if (requestLastFrameNumbers.isSequenceCompleted()) {
+ mCaptureCallbackMap.removeAt(index);
+ if (DEBUG) {
+ Log.v(TAG, String.format(
+ "Remove holder for requestId %d, because lastRegularFrame %d "
+ + "is <= %d, lastReprocessFrame %d is <= %d, "
+ + "lastZslStillFrame %d is <= %d", requestId,
+ lastRegularFrameNumber, lastCompletedRegularFrameNumber,
+ lastReprocessFrameNumber, lastCompletedReprocessFrameNumber,
+ lastZslStillFrameNumber, lastCompletedZslStillFrameNumber));
+ }
+
+ iter.remove();
+ } else {
+ Log.e(TAG, "Sequence not yet completed for request id " + requestId);
+ continue;
+ }
+ }
+ }
+ }
+ }
+
public void notifyFailedSwitch() {
synchronized(mInterfaceLock) {
Runnable switchFailDispatch = new Runnable() {
diff --git a/core/java/android/hardware/camera2/impl/CaptureResultExtras.java b/core/java/android/hardware/camera2/impl/CaptureResultExtras.java
index 1ff5bd5..5d9da73 100644
--- a/core/java/android/hardware/camera2/impl/CaptureResultExtras.java
+++ b/core/java/android/hardware/camera2/impl/CaptureResultExtras.java
@@ -30,6 +30,9 @@
private int partialResultCount;
private int errorStreamId;
private String errorPhysicalCameraId;
+ private long lastCompletedRegularFrameNumber;
+ private long lastCompletedReprocessFrameNumber;
+ private long lastCompletedZslFrameNumber;
public static final @android.annotation.NonNull Parcelable.Creator<CaptureResultExtras> CREATOR =
new Parcelable.Creator<CaptureResultExtras>() {
@@ -51,7 +54,9 @@
public CaptureResultExtras(int requestId, int subsequenceId, int afTriggerId,
int precaptureTriggerId, long frameNumber,
int partialResultCount, int errorStreamId,
- String errorPhysicalCameraId) {
+ String errorPhysicalCameraId, long lastCompletedRegularFrameNumber,
+ long lastCompletedReprocessFrameNumber,
+ long lastCompletedZslFrameNumber) {
this.requestId = requestId;
this.subsequenceId = subsequenceId;
this.afTriggerId = afTriggerId;
@@ -60,6 +65,9 @@
this.partialResultCount = partialResultCount;
this.errorStreamId = errorStreamId;
this.errorPhysicalCameraId = errorPhysicalCameraId;
+ this.lastCompletedRegularFrameNumber = lastCompletedRegularFrameNumber;
+ this.lastCompletedReprocessFrameNumber = lastCompletedReprocessFrameNumber;
+ this.lastCompletedZslFrameNumber = lastCompletedZslFrameNumber;
}
@Override
@@ -82,6 +90,9 @@
} else {
dest.writeBoolean(false);
}
+ dest.writeLong(lastCompletedRegularFrameNumber);
+ dest.writeLong(lastCompletedReprocessFrameNumber);
+ dest.writeLong(lastCompletedZslFrameNumber);
}
public void readFromParcel(Parcel in) {
@@ -96,6 +107,9 @@
if (errorPhysicalCameraIdPresent) {
errorPhysicalCameraId = in.readString();
}
+ lastCompletedRegularFrameNumber = in.readLong();
+ lastCompletedReprocessFrameNumber = in.readLong();
+ lastCompletedZslFrameNumber = in.readLong();
}
public String getErrorPhysicalCameraId() {
@@ -129,4 +143,16 @@
public int getErrorStreamId() {
return errorStreamId;
}
+
+ public long getLastCompletedRegularFrameNumber() {
+ return lastCompletedRegularFrameNumber;
+ }
+
+ public long getLastCompletedReprocessFrameNumber() {
+ return lastCompletedReprocessFrameNumber;
+ }
+
+ public long getLastCompletedZslFrameNumber() {
+ return lastCompletedZslFrameNumber;
+ }
}
diff --git a/core/java/android/hardware/camera2/impl/RequestLastFrameNumbersHolder.java b/core/java/android/hardware/camera2/impl/RequestLastFrameNumbersHolder.java
index bd1df9e..0ee4ebc 100644
--- a/core/java/android/hardware/camera2/impl/RequestLastFrameNumbersHolder.java
+++ b/core/java/android/hardware/camera2/impl/RequestLastFrameNumbersHolder.java
@@ -38,6 +38,10 @@
// The last ZSL still capture frame number for this request ID. It's
// CaptureCallback.NO_FRAMES_CAPTURED if the request ID has no zsl request.
private final long mLastZslStillFrameNumber;
+ // Whether the sequence is completed. (only consider capture result)
+ private boolean mSequenceCompleted;
+ // Whether the inflight request is completed. (consider result, buffers, and notifies)
+ private boolean mInflightCompleted;
/**
* Create a request-last-frame-numbers holder with a list of requests, request ID, and
@@ -89,6 +93,8 @@
mLastReprocessFrameNumber = lastReprocessFrameNumber;
mLastZslStillFrameNumber = lastZslStillFrameNumber;
mRequestId = requestInfo.getRequestId();
+ mSequenceCompleted = false;
+ mInflightCompleted = false;
}
/**
@@ -137,6 +143,8 @@
mLastZslStillFrameNumber = lastZslStillFrameNumber;
mLastReprocessFrameNumber = CameraCaptureSession.CaptureCallback.NO_FRAMES_CAPTURED;
mRequestId = requestId;
+ mSequenceCompleted = false;
+ mInflightCompleted = false;
}
/**
@@ -177,5 +185,34 @@
public int getRequestId() {
return mRequestId;
}
+
+ /**
+ * Return whether the capture sequence is completed.
+ */
+ public boolean isSequenceCompleted() {
+ return mSequenceCompleted;
+ }
+
+ /**
+ * Mark the capture sequence as completed.
+ */
+ public void markSequenceCompleted() {
+ mSequenceCompleted = true;
+ }
+
+ /**
+ * Return whether the inflight capture is completed.
+ */
+ public boolean isInflightCompleted() {
+ return mInflightCompleted;
+ }
+
+ /**
+ * Mark the inflight capture as completed.
+ */
+ public void markInflightCompleted() {
+ mInflightCompleted = true;
+ }
+
}
diff --git a/core/java/android/hardware/camera2/legacy/LegacyCameraDevice.java b/core/java/android/hardware/camera2/legacy/LegacyCameraDevice.java
index fbc9ac32..fdd578c 100644
--- a/core/java/android/hardware/camera2/legacy/LegacyCameraDevice.java
+++ b/core/java/android/hardware/camera2/legacy/LegacyCameraDevice.java
@@ -109,11 +109,12 @@
}
if (holder == null) {
return new CaptureResultExtras(ILLEGAL_VALUE, ILLEGAL_VALUE, ILLEGAL_VALUE,
- ILLEGAL_VALUE, ILLEGAL_VALUE, ILLEGAL_VALUE, ILLEGAL_VALUE, null);
+ ILLEGAL_VALUE, ILLEGAL_VALUE, ILLEGAL_VALUE, ILLEGAL_VALUE, null,
+ ILLEGAL_VALUE, ILLEGAL_VALUE, ILLEGAL_VALUE);
}
return new CaptureResultExtras(holder.getRequestId(), holder.getSubsequeceId(),
/*afTriggerId*/0, /*precaptureTriggerId*/0, holder.getFrameNumber(),
- /*partialResultCount*/1, errorStreamId, null);
+ /*partialResultCount*/1, errorStreamId, null, holder.getFrameNumber(), -1, -1);
}
/**