From cd333fe74677da852cc05dcb24c9e7e4614377ec Mon Sep 17 00:00:00 2001 From: Yin-Chia Yeh Date: Fri, 8 Feb 2019 13:45:41 -0800 Subject: [PATCH 1/2] Camera: bug fixes for HAL buffer manager Test: GCA smoke test + Camera CTS Bug: 120986771 Change-Id: I946fb95d8e685995ebb8cf3d36b0373958bfad09 --- .../device3/Camera3Device.cpp | 38 ++++++++++++------- .../libcameraservice/device3/Camera3Device.h | 2 +- .../device3/Camera3Stream.cpp | 6 ++- 3 files changed, 30 insertions(+), 16 deletions(-) diff --git a/services/camera/libcameraservice/device3/Camera3Device.cpp b/services/camera/libcameraservice/device3/Camera3Device.cpp index 918dcf775d..2b39254d08 100644 --- a/services/camera/libcameraservice/device3/Camera3Device.cpp +++ b/services/camera/libcameraservice/device3/Camera3Device.cpp @@ -1027,11 +1027,22 @@ hardware::Return Camera3Device::requestStreamBuffers( return hardware::Void(); } + if (outputStream->isAbandoned()) { + bufRet.val.error(StreamBufferRequestError::STREAM_DISCONNECTED); + allReqsSucceeds = false; + continue; + } + bufRet.streamId = streamId; + size_t handOutBufferCount = outputStream->getOutstandingBuffersCount(); uint32_t numBuffersRequested = bufReq.numBuffersRequested; - size_t totalHandout = outputStream->getOutstandingBuffersCount() + numBuffersRequested; - if (totalHandout > outputStream->asHalStream()->max_buffers) { + size_t totalHandout = handOutBufferCount + numBuffersRequested; + uint32_t maxBuffers = outputStream->asHalStream()->max_buffers; + if (totalHandout > maxBuffers) { // Not able to allocate enough buffer. Exit early for this stream + ALOGE("%s: request too much buffers for stream %d: at HAL: %zu + requesting: %d" + " > max: %d", __FUNCTION__, streamId, handOutBufferCount, + numBuffersRequested, maxBuffers); bufRet.val.error(StreamBufferRequestError::MAX_BUFFER_EXCEEDED); allReqsSucceeds = false; continue; @@ -2186,12 +2197,11 @@ status_t Camera3Device::waitUntilStateThenRelock(bool active, nsecs_t timeout) { mStatusWaiters++; - // Notify HAL to start draining. We need to notify the HalInterface layer - // even when the device is already IDLE, so HalInterface can reject incoming - // requestStreamBuffers call. if (!active && mUseHalBufManager) { auto streamIds = mOutputStreams.getStreamIds(); - mRequestThread->signalPipelineDrain(streamIds); + if (mStatus == STATUS_ACTIVE) { + mRequestThread->signalPipelineDrain(streamIds); + } mRequestBufferSM.onWaitUntilIdle(); } @@ -5251,6 +5261,11 @@ bool Camera3Device::RequestThread::threadLoop() { ALOGVV("%s: %d: submitting %zu requests in a batch.", __FUNCTION__, __LINE__, mNextRequests.size()); + sp parent = mParent.promote(); + if (parent != nullptr) { + parent->mRequestBufferSM.onSubmittingRequest(); + } + bool submitRequestSuccess = false; nsecs_t tRequestStart = systemTime(SYSTEM_TIME_MONOTONIC); if (mInterface->supportBatchRequest()) { @@ -5261,13 +5276,6 @@ bool Camera3Device::RequestThread::threadLoop() { nsecs_t tRequestEnd = systemTime(SYSTEM_TIME_MONOTONIC); mRequestLatency.add(tRequestStart, tRequestEnd); - if (submitRequestSuccess) { - sp parent = mParent.promote(); - if (parent != nullptr) { - parent->mRequestBufferSM.onRequestSubmitted(); - } - } - if (useFlushLock) { mFlushLock.unlock(); } @@ -6429,9 +6437,11 @@ void Camera3Device::RequestBufferStateMachine::onStreamsConfigured() { return; } -void Camera3Device::RequestBufferStateMachine::onRequestSubmitted() { +void Camera3Device::RequestBufferStateMachine::onSubmittingRequest() { std::lock_guard lock(mLock); mRequestThreadPaused = false; + // inflight map register actually happens in prepareHalRequest now, but it is close enough + // approximation. mInflightMapEmpty = false; if (mStatus == RB_STATUS_STOPPED) { mStatus = RB_STATUS_READY; diff --git a/services/camera/libcameraservice/device3/Camera3Device.h b/services/camera/libcameraservice/device3/Camera3Device.h index e5a38bbf61..51a8fd51da 100644 --- a/services/camera/libcameraservice/device3/Camera3Device.h +++ b/services/camera/libcameraservice/device3/Camera3Device.h @@ -1317,7 +1317,7 @@ class Camera3Device : void onInflightMapEmpty(); // Events triggered by RequestThread - void onRequestSubmitted(); + void onSubmittingRequest(); void onRequestThreadPaused(); private: diff --git a/services/camera/libcameraservice/device3/Camera3Stream.cpp b/services/camera/libcameraservice/device3/Camera3Stream.cpp index d29e5c083d..0571741ba5 100644 --- a/services/camera/libcameraservice/device3/Camera3Stream.cpp +++ b/services/camera/libcameraservice/device3/Camera3Stream.cpp @@ -588,7 +588,11 @@ status_t Camera3Stream::getBuffer(camera3_stream_buffer *buffer, if (mState != STATE_CONFIGURED) { ALOGE("%s: Stream %d: Can't get buffers if stream is not in CONFIGURED state %d", __FUNCTION__, mId, mState); - return INVALID_OPERATION; + if (mState == STATE_ABANDONED) { + return DEAD_OBJECT; + } else { + return INVALID_OPERATION; + } } // Wait for new buffer returned back if we are running into the limit. From 7e5a2042659b0659be82397202261732a797c216 Mon Sep 17 00:00:00 2001 From: Yin-Chia Yeh Date: Wed, 6 Feb 2019 16:01:06 -0800 Subject: [PATCH 2/2] Camera: rework API1 shim takePicture retry logic onBufferReleased is no longer reliable indicator of capture error due to HAL buffer manager feature. Switch to listen to error callback from HAL directly. Test: API1 CTS + Pixel 3 Bug: 123952355 Change-Id: I7362942f19356583ec66f259b01e963a1af3a205 --- .../libcameraservice/api1/Camera2Client.cpp | 5 ++- .../api1/client2/CaptureSequencer.cpp | 25 +++++++++++++++ .../api1/client2/CaptureSequencer.h | 3 ++ .../api1/client2/JpegProcessor.cpp | 32 ------------------- .../api1/client2/JpegProcessor.h | 9 +----- .../device3/Camera3Device.cpp | 4 +-- 6 files changed, 33 insertions(+), 45 deletions(-) diff --git a/services/camera/libcameraservice/api1/Camera2Client.cpp b/services/camera/libcameraservice/api1/Camera2Client.cpp index c9c216b3ce..e002e186fb 100644 --- a/services/camera/libcameraservice/api1/Camera2Client.cpp +++ b/services/camera/libcameraservice/api1/Camera2Client.cpp @@ -1773,6 +1773,8 @@ void Camera2Client::notifyError(int32_t errorCode, break; } + mCaptureSequencer->notifyError(errorCode, resultExtras); + ALOGE("%s: Error condition %d reported by HAL, requestId %" PRId32, __FUNCTION__, errorCode, resultExtras.requestId); @@ -1927,9 +1929,6 @@ void Camera2Client::notifyAutoExposure(uint8_t newState, int triggerId) { void Camera2Client::notifyShutter(const CaptureResultExtras& resultExtras, nsecs_t timestamp) { - (void)resultExtras; - (void)timestamp; - ALOGV("%s: Shutter notification for request id %" PRId32 " at time %" PRId64, __FUNCTION__, resultExtras.requestId, timestamp); mCaptureSequencer->notifyShutter(resultExtras, timestamp); diff --git a/services/camera/libcameraservice/api1/client2/CaptureSequencer.cpp b/services/camera/libcameraservice/api1/client2/CaptureSequencer.cpp index 5029d4babb..88799f969e 100644 --- a/services/camera/libcameraservice/api1/client2/CaptureSequencer.cpp +++ b/services/camera/libcameraservice/api1/client2/CaptureSequencer.cpp @@ -117,6 +117,31 @@ void CaptureSequencer::notifyShutter(const CaptureResultExtras& resultExtras, } } +void CaptureSequencer::notifyError(int32_t errorCode, const CaptureResultExtras& resultExtras) { + ATRACE_CALL(); + bool jpegBufferLost = false; + if (errorCode == hardware::camera2::ICameraDeviceCallbacks::ERROR_CAMERA_BUFFER) { + sp client = mClient.promote(); + if (client == nullptr) { + return; + } + int captureStreamId = client->getCaptureStreamId(); + if (captureStreamId == resultExtras.errorStreamId) { + jpegBufferLost = true; + } + } else if (errorCode == + hardware::camera2::ICameraDeviceCallbacks::ERROR_CAMERA_REQUEST) { + if (resultExtras.requestId == mShutterCaptureId) { + jpegBufferLost = true; + } + } + + if (jpegBufferLost) { + sp emptyBuffer; + onCaptureAvailable(/*timestamp*/0, emptyBuffer, /*captureError*/true); + } +} + void CaptureSequencer::onResultAvailable(const CaptureResult &result) { ATRACE_CALL(); ALOGV("%s: New result available.", __FUNCTION__); diff --git a/services/camera/libcameraservice/api1/client2/CaptureSequencer.h b/services/camera/libcameraservice/api1/client2/CaptureSequencer.h index c23b12da8a..727dd5310b 100644 --- a/services/camera/libcameraservice/api1/client2/CaptureSequencer.h +++ b/services/camera/libcameraservice/api1/client2/CaptureSequencer.h @@ -65,6 +65,9 @@ class CaptureSequencer: void notifyShutter(const CaptureResultExtras& resultExtras, nsecs_t timestamp); + // Notifications about shutter (capture start) + void notifyError(int32_t errorCode, const CaptureResultExtras& resultExtras); + // Notification from the frame processor virtual void onResultAvailable(const CaptureResult &result); diff --git a/services/camera/libcameraservice/api1/client2/JpegProcessor.cpp b/services/camera/libcameraservice/api1/client2/JpegProcessor.cpp index 36395f3442..ddfe5e31dc 100755 --- a/services/camera/libcameraservice/api1/client2/JpegProcessor.cpp +++ b/services/camera/libcameraservice/api1/client2/JpegProcessor.cpp @@ -62,31 +62,6 @@ void JpegProcessor::onFrameAvailable(const BufferItem& /*item*/) { } } -void JpegProcessor::onBufferRequestForFrameNumber(uint64_t /*frameNumber*/, - int /*streamId*/, const CameraMetadata& /*settings*/) { - // Intentionally left empty -} - -void JpegProcessor::onBufferAcquired(const BufferInfo& /*bufferInfo*/) { - // Intentionally left empty -} - -void JpegProcessor::onBufferReleased(const BufferInfo& bufferInfo) { - ALOGV("%s", __FUNCTION__); - if (bufferInfo.mError) { - // Only lock in case of error, since we get one of these for each - // onFrameAvailable as well, and scheduling may delay this call late - // enough to run into later preview restart operations, for non-error - // cases. - // b/29524651 - ALOGV("%s: JPEG buffer lost", __FUNCTION__); - Mutex::Autolock l(mInputMutex); - mCaptureDone = true; - mCaptureSuccess = false; - mCaptureDoneSignal.signal(); - } -} - status_t JpegProcessor::updateStream(const Parameters ¶ms) { ATRACE_CALL(); ALOGV("%s", __FUNCTION__); @@ -181,13 +156,6 @@ status_t JpegProcessor::updateStream(const Parameters ¶ms) { strerror(-res), res); return res; } - - res = device->addBufferListenerForStream(mCaptureStreamId, this); - if (res != OK) { - ALOGE("%s: Camera %d: Can't add buffer listeneri: %s (%d)", - __FUNCTION__, mId, strerror(-res), res); - return res; - } } return OK; } diff --git a/services/camera/libcameraservice/api1/client2/JpegProcessor.h b/services/camera/libcameraservice/api1/client2/JpegProcessor.h index 53e6836017..977f11d26d 100644 --- a/services/camera/libcameraservice/api1/client2/JpegProcessor.h +++ b/services/camera/libcameraservice/api1/client2/JpegProcessor.h @@ -42,8 +42,7 @@ struct Parameters; * Still image capture output image processing */ class JpegProcessor: - public Thread, public CpuConsumer::FrameAvailableListener, - public camera3::Camera3StreamBufferListener { + public Thread, public CpuConsumer::FrameAvailableListener { public: JpegProcessor(sp client, wp sequencer); ~JpegProcessor(); @@ -51,12 +50,6 @@ class JpegProcessor: // CpuConsumer listener implementation void onFrameAvailable(const BufferItem& item); - // Camera3StreamBufferListener implementation - void onBufferAcquired(const BufferInfo& bufferInfo) override; - void onBufferReleased(const BufferInfo& bufferInfo) override; - void onBufferRequestForFrameNumber(uint64_t frameNumber, int streamId, - const CameraMetadata& settings) override; - status_t updateStream(const Parameters ¶ms); status_t deleteStream(); int getStreamId() const; diff --git a/services/camera/libcameraservice/device3/Camera3Device.cpp b/services/camera/libcameraservice/device3/Camera3Device.cpp index 2b39254d08..7850e70f1e 100644 --- a/services/camera/libcameraservice/device3/Camera3Device.cpp +++ b/services/camera/libcameraservice/device3/Camera3Device.cpp @@ -885,14 +885,14 @@ status_t Camera3Device::convertMetadataListToRequestListLocked( return OK; } -status_t Camera3Device::capture(CameraMetadata &request, int64_t* /*lastFrameNumber*/) { +status_t Camera3Device::capture(CameraMetadata &request, int64_t* lastFrameNumber) { ATRACE_CALL(); List requestsList; std::list surfaceMaps; convertToRequestList(requestsList, surfaceMaps, request); - return captureList(requestsList, surfaceMaps, /*lastFrameNumber*/NULL); + return captureList(requestsList, surfaceMaps, lastFrameNumber); } void Camera3Device::convertToRequestList(List& requestsList,