From db97350aa110c2fc888de04d9c3c98c1994e368f Mon Sep 17 00:00:00 2001 From: Shuzhen Wang Date: Wed, 13 May 2020 16:01:19 -0700 Subject: [PATCH] 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 --- .../camera2/impl/CameraDeviceImpl.java | 194 ++++++++++++------ .../impl/CameraOfflineSessionImpl.java | 73 +++++++ .../camera2/impl/CaptureResultExtras.java | 28 ++- .../impl/RequestLastFrameNumbersHolder.java | 37 ++++ .../camera2/legacy/LegacyCameraDevice.java | 5 +- 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 6d49add65c5b1..23c86029f3be2 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 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 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 1d9d644c9306e..413caf5e22e04 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 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 1ff5bd562f2e7..5d9da73fd5c06 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 CREATOR = new Parcelable.Creator() { @@ -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 bd1df9e1ac7d4..0ee4ebc1aa87b 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 fbc9ac3229c34..fdd578c419d83 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); } /**