diff options
| author | 2020-05-13 16:01:19 -0700 | |
|---|---|---|
| committer | 2020-06-04 18:28:44 -0700 | |
| commit | db97350aa110c2fc888de04d9c3c98c1994e368f (patch) | |
| tree | 5f1cfd737b08bbb5a404f3db90b8558b00618839 | |
| parent | a38a553c1617312433a9c0e27fdaafee27e3a359 (diff) | |
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
5 files changed, 276 insertions, 61 deletions
diff --git a/core/java/android/hardware/camera2/impl/CameraDeviceImpl.java b/core/java/android/hardware/camera2/impl/CameraDeviceImpl.java index 6d49add65c5b..23c86029f3be 100644 --- a/core/java/android/hardware/camera2/impl/CameraDeviceImpl.java +++ b/core/java/android/hardware/camera2/impl/CameraDeviceImpl.java @@ -1072,7 +1072,7 @@ public class CameraDeviceImpl extends CameraDevice * @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 @@ public class CameraDeviceImpl extends CameraDevice if (repeating) { if (mRepeatingRequestId != REQUEST_ID_NONE) { - checkEarlyTriggerSequenceComplete(mRepeatingRequestId, + checkEarlyTriggerSequenceCompleteLocked(mRepeatingRequestId, requestInfo.getLastFrameNumber(), mRepeatingRequestTypes); } @@ -1269,7 +1269,7 @@ public class CameraDeviceImpl extends CameraDevice return; } - checkEarlyTriggerSequenceComplete(requestId, lastFrameNumber, requestTypes); + checkEarlyTriggerSequenceCompleteLocked(requestId, lastFrameNumber, requestTypes); } } } @@ -1302,7 +1302,7 @@ public class CameraDeviceImpl extends CameraDevice 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 @@ public class CameraDeviceImpl extends CameraDevice 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 @@ public class CameraDeviceImpl extends CameraDevice 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 @@ public class CameraDeviceImpl extends CameraDevice 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 class CameraDeviceImpl extends CameraDevice 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 @@ public class CameraDeviceImpl extends CameraDevice 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 1d9d644c9306..413caf5e22e0 100644 --- a/core/java/android/hardware/camera2/impl/CameraOfflineSessionImpl.java +++ b/core/java/android/hardware/camera2/impl/CameraOfflineSessionImpl.java @@ -182,6 +182,12 @@ public class CameraOfflineSessionImpl extends CameraOfflineSession 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 class CameraOfflineSessionImpl extends CameraOfflineSession 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 @@ public class CameraOfflineSessionImpl extends CameraOfflineSession } } + 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 1ff5bd562f2e..5d9da73fd5c0 100644 --- a/core/java/android/hardware/camera2/impl/CaptureResultExtras.java +++ b/core/java/android/hardware/camera2/impl/CaptureResultExtras.java @@ -30,6 +30,9 @@ public class CaptureResultExtras implements Parcelable { 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 class CaptureResultExtras implements Parcelable { 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 @@ public class CaptureResultExtras implements Parcelable { this.partialResultCount = partialResultCount; this.errorStreamId = errorStreamId; this.errorPhysicalCameraId = errorPhysicalCameraId; + this.lastCompletedRegularFrameNumber = lastCompletedRegularFrameNumber; + this.lastCompletedReprocessFrameNumber = lastCompletedReprocessFrameNumber; + this.lastCompletedZslFrameNumber = lastCompletedZslFrameNumber; } @Override @@ -82,6 +90,9 @@ public class CaptureResultExtras implements Parcelable { } else { dest.writeBoolean(false); } + dest.writeLong(lastCompletedRegularFrameNumber); + dest.writeLong(lastCompletedReprocessFrameNumber); + dest.writeLong(lastCompletedZslFrameNumber); } public void readFromParcel(Parcel in) { @@ -96,6 +107,9 @@ public class CaptureResultExtras implements Parcelable { if (errorPhysicalCameraIdPresent) { errorPhysicalCameraId = in.readString(); } + lastCompletedRegularFrameNumber = in.readLong(); + lastCompletedReprocessFrameNumber = in.readLong(); + lastCompletedZslFrameNumber = in.readLong(); } public String getErrorPhysicalCameraId() { @@ -129,4 +143,16 @@ public class CaptureResultExtras implements Parcelable { 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 bd1df9e1ac7d..0ee4ebc1aa87 100644 --- a/core/java/android/hardware/camera2/impl/RequestLastFrameNumbersHolder.java +++ b/core/java/android/hardware/camera2/impl/RequestLastFrameNumbersHolder.java @@ -38,6 +38,10 @@ public class RequestLastFrameNumbersHolder { // 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 @@ public class RequestLastFrameNumbersHolder { mLastReprocessFrameNumber = lastReprocessFrameNumber; mLastZslStillFrameNumber = lastZslStillFrameNumber; mRequestId = requestInfo.getRequestId(); + mSequenceCompleted = false; + mInflightCompleted = false; } /** @@ -137,6 +143,8 @@ public class RequestLastFrameNumbersHolder { mLastZslStillFrameNumber = lastZslStillFrameNumber; mLastReprocessFrameNumber = CameraCaptureSession.CaptureCallback.NO_FRAMES_CAPTURED; mRequestId = requestId; + mSequenceCompleted = false; + mInflightCompleted = false; } /** @@ -177,5 +185,34 @@ public class RequestLastFrameNumbersHolder { 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 fbc9ac3229c3..fdd578c419d8 100644 --- a/core/java/android/hardware/camera2/legacy/LegacyCameraDevice.java +++ b/core/java/android/hardware/camera2/legacy/LegacyCameraDevice.java @@ -109,11 +109,12 @@ public class LegacyCameraDevice implements AutoCloseable { } 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); } /** |