From 1754351d9199721e7e7943461689e399ef015260 Mon Sep 17 00:00:00 2001 From: Eino-Ville Talvala Date: Wed, 6 Aug 2014 14:32:02 -0700 Subject: CameraService: Correct API2 error handling - Add more error codes to the binder camera2 callbacks - Translate HAL errors to callback errors - When flushing, report failures for queued requests - Treat stream config failure as nonfatal - Send request errors when buffers aren't available for captures Bug: 15524101 Bug: 14448494 Bug: 11272459 Bug: 17160301 Change-Id: I81aa54e805a9cce1cb8a6a9374549daa7666deb2 --- include/camera/camera2/ICameraDeviceCallbacks.h | 6 +- .../libcameraservice/device3/Camera3Device.cpp | 286 +++++++++++++++------ .../libcameraservice/device3/Camera3Device.h | 13 +- .../libcameraservice/device3/Camera3Stream.cpp | 29 +++ .../libcameraservice/device3/Camera3Stream.h | 7 + .../device3/Camera3StreamInterface.h | 7 + 6 files changed, 265 insertions(+), 83 deletions(-) diff --git a/include/camera/camera2/ICameraDeviceCallbacks.h b/include/camera/camera2/ICameraDeviceCallbacks.h index f059b3d..670480b 100644 --- a/include/camera/camera2/ICameraDeviceCallbacks.h +++ b/include/camera/camera2/ICameraDeviceCallbacks.h @@ -42,9 +42,13 @@ public: * Error codes for CAMERA_MSG_ERROR */ enum CameraErrorCode { + ERROR_CAMERA_INVALID_ERROR = -1, // To indicate all invalid error codes ERROR_CAMERA_DISCONNECTED = 0, ERROR_CAMERA_DEVICE = 1, - ERROR_CAMERA_SERVICE = 2 + ERROR_CAMERA_SERVICE = 2, + ERROR_CAMERA_REQUEST = 3, + ERROR_CAMERA_RESULT = 4, + ERROR_CAMERA_BUFFER = 5, }; // One way diff --git a/services/camera/libcameraservice/device3/Camera3Device.cpp b/services/camera/libcameraservice/device3/Camera3Device.cpp index 0d33406..9b51b99 100644 --- a/services/camera/libcameraservice/device3/Camera3Device.cpp +++ b/services/camera/libcameraservice/device3/Camera3Device.cpp @@ -1145,6 +1145,7 @@ status_t Camera3Device::setNotifyCallback(NotificationListener *listener) { ALOGW("%s: Replacing old callback listener", __FUNCTION__); } mListener = listener; + mRequestThread->setNotifyCallback(listener); return OK; } @@ -1268,9 +1269,15 @@ status_t Camera3Device::flush(int64_t *frameNumber) { ALOGV("%s: Camera %d: Flushing all requests", __FUNCTION__, mId); Mutex::Autolock il(mInterfaceLock); + NotificationListener* listener; + { + Mutex::Autolock l(mOutputLock); + listener = mListener; + } + { Mutex::Autolock l(mLock); - mRequestThread->clear(/*out*/frameNumber); + mRequestThread->clear(listener, /*out*/frameNumber); } status_t res; @@ -1458,7 +1465,42 @@ status_t Camera3Device::configureStreamsLocked() { res = mHal3Device->ops->configure_streams(mHal3Device, &config); ATRACE_END(); - if (res != OK) { + if (res == BAD_VALUE) { + // HAL rejected this set of streams as unsupported, clean up config + // attempt and return to unconfigured state + if (mInputStream != NULL && mInputStream->isConfiguring()) { + res = mInputStream->cancelConfiguration(); + if (res != OK) { + SET_ERR_L("Can't cancel configuring input stream %d: %s (%d)", + mInputStream->getId(), strerror(-res), res); + return res; + } + } + + for (size_t i = 0; i < mOutputStreams.size(); i++) { + sp outputStream = + mOutputStreams.editValueAt(i); + if (outputStream->isConfiguring()) { + res = outputStream->cancelConfiguration(); + if (res != OK) { + SET_ERR_L( + "Can't cancel configuring output stream %d: %s (%d)", + outputStream->getId(), strerror(-res), res); + return res; + } + } + } + + // Return state to that at start of call, so that future configures + // properly clean things up + mStatus = STATUS_UNCONFIGURED; + mNeedConfig = true; + + ALOGV("%s: Camera %d: Stream configuration failed", __FUNCTION__, mId); + return BAD_VALUE; + } else if (res != OK) { + // Some other kind of error from configure_streams - this is not + // expected SET_ERR_L("Unable to configure streams with HAL: %s (%d)", strerror(-res), res); return res; @@ -1544,14 +1586,20 @@ void Camera3Device::setErrorStateLockedV(const char *fmt, va_list args) { // But only do error state transition steps for the first error if (mStatus == STATUS_ERROR || mStatus == STATUS_UNINITIALIZED) return; - // Save stack trace. View by dumping it later. - CameraTraces::saveTrace(); - // TODO: consider adding errorCause and client pid/procname - mErrorCause = errorCause; mRequestThread->setPaused(true); mStatus = STATUS_ERROR; + + // Notify upstream about a device error + if (mListener != NULL) { + mListener->notifyError(ICameraDeviceCallbacks::ERROR_CAMERA_DEVICE, + CaptureResultExtras()); + } + + // Save stack trace. View by dumping it later. + CameraTraces::saveTrace(); + // TODO: consider adding errorCause and client pid/procname } /** @@ -2022,92 +2070,134 @@ void Camera3Device::notify(const camera3_notify_msg *msg) { switch (msg->type) { case CAMERA3_MSG_ERROR: { - int streamId = 0; - if (msg->message.error.error_stream != NULL) { - Camera3Stream *stream = - Camera3Stream::cast( - msg->message.error.error_stream); - streamId = stream->getId(); - } - ALOGV("Camera %d: %s: HAL error, frame %d, stream %d: %d", - mId, __FUNCTION__, msg->message.error.frame_number, - streamId, msg->message.error.error_code); + notifyError(msg->message.error, listener); + break; + } + case CAMERA3_MSG_SHUTTER: { + notifyShutter(msg->message.shutter, listener); + break; + } + default: + SET_ERR("Unknown notify message from HAL: %d", + msg->type); + } +} + +void Camera3Device::notifyError(const camera3_error_msg_t &msg, + NotificationListener *listener) { + + // Map camera HAL error codes to ICameraDeviceCallback error codes + // Index into this with the HAL error code + static const ICameraDeviceCallbacks::CameraErrorCode + halErrorMap[CAMERA3_MSG_NUM_ERRORS] = { + // 0 = Unused error code + ICameraDeviceCallbacks::ERROR_CAMERA_INVALID_ERROR, + // 1 = CAMERA3_MSG_ERROR_DEVICE + ICameraDeviceCallbacks::ERROR_CAMERA_DEVICE, + // 2 = CAMERA3_MSG_ERROR_REQUEST + ICameraDeviceCallbacks::ERROR_CAMERA_REQUEST, + // 3 = CAMERA3_MSG_ERROR_RESULT + ICameraDeviceCallbacks::ERROR_CAMERA_RESULT, + // 4 = CAMERA3_MSG_ERROR_BUFFER + ICameraDeviceCallbacks::ERROR_CAMERA_BUFFER + }; + + ICameraDeviceCallbacks::CameraErrorCode errorCode = + ((msg.error_code >= 0) && + (msg.error_code < CAMERA3_MSG_NUM_ERRORS)) ? + halErrorMap[msg.error_code] : + ICameraDeviceCallbacks::ERROR_CAMERA_INVALID_ERROR; + + int streamId = 0; + if (msg.error_stream != NULL) { + Camera3Stream *stream = + Camera3Stream::cast(msg.error_stream); + streamId = stream->getId(); + } + ALOGV("Camera %d: %s: HAL error, frame %d, stream %d: %d", + mId, __FUNCTION__, msg.frame_number, + streamId, msg.error_code); - CaptureResultExtras resultExtras; - // Set request error status for the request in the in-flight tracking + CaptureResultExtras resultExtras; + switch (errorCode) { + case ICameraDeviceCallbacks::ERROR_CAMERA_DEVICE: + // SET_ERR calls notifyError + SET_ERR("Camera HAL reported serious device error"); + break; + case ICameraDeviceCallbacks::ERROR_CAMERA_REQUEST: + case ICameraDeviceCallbacks::ERROR_CAMERA_RESULT: + case ICameraDeviceCallbacks::ERROR_CAMERA_BUFFER: { Mutex::Autolock l(mInFlightLock); - ssize_t idx = mInFlightMap.indexOfKey(msg->message.error.frame_number); + ssize_t idx = mInFlightMap.indexOfKey(msg.frame_number); if (idx >= 0) { InFlightRequest &r = mInFlightMap.editValueAt(idx); - r.requestStatus = msg->message.error.error_code; + r.requestStatus = msg.error_code; resultExtras = r.resultExtras; } else { - resultExtras.frameNumber = msg->message.error.frame_number; - ALOGE("Camera %d: %s: cannot find in-flight request on frame %" PRId64 - " error", mId, __FUNCTION__, resultExtras.frameNumber); + resultExtras.frameNumber = msg.frame_number; + ALOGE("Camera %d: %s: cannot find in-flight request on " + "frame %" PRId64 " error", mId, __FUNCTION__, + resultExtras.frameNumber); } } - if (listener != NULL) { - if (msg->message.error.error_code == CAMERA3_MSG_ERROR_DEVICE) { - listener->notifyError(ICameraDeviceCallbacks::ERROR_CAMERA_DEVICE, - resultExtras); - } + listener->notifyError(errorCode, resultExtras); } else { ALOGE("Camera %d: %s: no listener available", mId, __FUNCTION__); } break; + default: + // SET_ERR calls notifyError + SET_ERR("Unknown error message from HAL: %d", msg.error_code); + break; + } +} + +void Camera3Device::notifyShutter(const camera3_shutter_msg_t &msg, + NotificationListener *listener) { + ssize_t idx; + // Verify ordering of shutter notifications + { + Mutex::Autolock l(mOutputLock); + // TODO: need to track errors for tighter bounds on expected frame number. + if (msg.frame_number < mNextShutterFrameNumber) { + SET_ERR("Shutter notification out-of-order. Expected " + "notification for frame %d, got frame %d", + mNextShutterFrameNumber, msg.frame_number); + return; } - case CAMERA3_MSG_SHUTTER: { - ssize_t idx; - uint32_t frameNumber = msg->message.shutter.frame_number; - nsecs_t timestamp = msg->message.shutter.timestamp; - // Verify ordering of shutter notifications - { - Mutex::Autolock l(mOutputLock); - // TODO: need to track errors for tighter bounds on expected frame number. - if (frameNumber < mNextShutterFrameNumber) { - SET_ERR("Shutter notification out-of-order. Expected " - "notification for frame %d, got frame %d", - mNextShutterFrameNumber, frameNumber); - break; - } - mNextShutterFrameNumber = frameNumber + 1; - } + mNextShutterFrameNumber = msg.frame_number + 1; + } - CaptureResultExtras resultExtras; + CaptureResultExtras resultExtras; - // Set timestamp for the request in the in-flight tracking - // and get the request ID to send upstream - { - Mutex::Autolock l(mInFlightLock); - idx = mInFlightMap.indexOfKey(frameNumber); - if (idx >= 0) { - InFlightRequest &r = mInFlightMap.editValueAt(idx); - r.captureTimestamp = timestamp; - resultExtras = r.resultExtras; - } - } - if (idx < 0) { - SET_ERR("Shutter notification for non-existent frame number %d", - frameNumber); - break; - } - ALOGVV("Camera %d: %s: Shutter fired for frame %d (id %d) at %" PRId64, - mId, __FUNCTION__, frameNumber, resultExtras.requestId, timestamp); - // Call listener, if any - if (listener != NULL) { - listener->notifyShutter(resultExtras, timestamp); - } - break; + // Set timestamp for the request in the in-flight tracking + // and get the request ID to send upstream + { + Mutex::Autolock l(mInFlightLock); + idx = mInFlightMap.indexOfKey(msg.frame_number); + if (idx >= 0) { + InFlightRequest &r = mInFlightMap.editValueAt(idx); + r.captureTimestamp = msg.timestamp; + resultExtras = r.resultExtras; } - default: - SET_ERR("Unknown notify message from HAL: %d", - msg->type); + } + if (idx < 0) { + SET_ERR("Shutter notification for non-existent frame number %d", + msg.frame_number); + return; + } + ALOGVV("Camera %d: %s: Shutter fired for frame %d (id %d) at %" PRId64, + mId, __FUNCTION__, + msg.frame_number, resultExtras.requestId, msg.timestamp); + // Call listener, if any + if (listener != NULL) { + listener->notifyShutter(resultExtras, msg.timestamp); } } + CameraMetadata Camera3Device::getLatestRequestLocked() { ALOGV("%s", __FUNCTION__); @@ -2144,6 +2234,12 @@ Camera3Device::RequestThread::RequestThread(wp parent, mStatusId = statusTracker->addComponent(); } +void Camera3Device::RequestThread::setNotifyCallback( + NotificationListener *listener) { + Mutex::Autolock l(mRequestLock); + mListener = listener; +} + void Camera3Device::RequestThread::configurationComplete() { Mutex::Autolock l(mRequestLock); mReconfigured = true; @@ -2266,20 +2362,26 @@ status_t Camera3Device::RequestThread::clearRepeatingRequests(/*out*/int64_t *la return OK; } -status_t Camera3Device::RequestThread::clear(/*out*/int64_t *lastFrameNumber) { +status_t Camera3Device::RequestThread::clear( + NotificationListener *listener, + /*out*/int64_t *lastFrameNumber) { Mutex::Autolock l(mRequestLock); ALOGV("RequestThread::%s:", __FUNCTION__); + mRepeatingRequests.clear(); - // Decrement repeating frame count for those requests never sent to device - // TODO: Remove this after we have proper error handling so these requests - // will generate an error callback. This might be the only place calling - // isRepeatingRequestLocked. If so, isRepeatingRequestLocked should also be removed. - const RequestList &requests = mRequestQueue; - for (RequestList::const_iterator it = requests.begin(); - it != requests.end(); ++it) { - if (isRepeatingRequestLocked(*it)) { - mRepeatingLastFrameNumber--; + // Send errors for all requests pending in the request queue, including + // pending repeating requests + if (listener != NULL) { + for (RequestList::iterator it = mRequestQueue.begin(); + it != mRequestQueue.end(); ++it) { + // Set the frame number this request would have had, if it + // had been submitted; this frame number will not be reused. + // The requestId and burstId fields were set when the request was + // submitted originally (in convertMetadataListToRequestListLocked) + (*it)->mResultExtras.frameNumber = mFrameNumber++; + listener->notifyError(ICameraDeviceCallbacks::ERROR_CAMERA_REQUEST, + (*it)->mResultExtras); } } mRequestQueue.clear(); @@ -2421,8 +2523,17 @@ bool Camera3Device::RequestThread::threadLoop() { request.input_buffer = &inputBuffer; res = nextRequest->mInputStream->getInputBuffer(&inputBuffer); if (res != OK) { + // Can't get input buffer from gralloc queue - this could be due to + // disconnected queue or other producer misbehavior, so not a fatal + // error ALOGE("RequestThread: Can't get input buffer, skipping request:" " %s (%d)", strerror(-res), res); + Mutex::Autolock l(mRequestLock); + if (mListener != NULL) { + mListener->notifyError( + ICameraDeviceCallbacks::ERROR_CAMERA_REQUEST, + nextRequest->mResultExtras); + } cleanUpFailedRequest(request, nextRequest, outputBuffers); return true; } @@ -2438,8 +2549,17 @@ bool Camera3Device::RequestThread::threadLoop() { res = nextRequest->mOutputStreams.editItemAt(i)-> getBuffer(&outputBuffers.editItemAt(i)); if (res != OK) { + // Can't get output buffer from gralloc queue - this could be due to + // abandoned queue or other consumer misbehavior, so not a fatal + // error ALOGE("RequestThread: Can't get output buffer, skipping request:" " %s (%d)", strerror(-res), res); + Mutex::Autolock l(mRequestLock); + if (mListener != NULL) { + mListener->notifyError( + ICameraDeviceCallbacks::ERROR_CAMERA_REQUEST, + nextRequest->mResultExtras); + } cleanUpFailedRequest(request, nextRequest, outputBuffers); return true; } @@ -2450,6 +2570,7 @@ bool Camera3Device::RequestThread::threadLoop() { // Log request in the in-flight queue sp parent = mParent.promote(); if (parent == NULL) { + // Should not happen, and nowhere to send errors to, so just log it CLOGE("RequestThread: Parent is gone"); cleanUpFailedRequest(request, nextRequest, outputBuffers); return false; @@ -2485,6 +2606,9 @@ bool Camera3Device::RequestThread::threadLoop() { ATRACE_END(); if (res != OK) { + // Should only get a failure here for malformed requests or device-level + // errors, so consider all errors fatal. Bad metadata failures should + // come through notify. SET_ERR("RequestThread: Unable to submit capture request %d to HAL" " device: %s (%d)", request.frame_number, strerror(-res), res); cleanUpFailedRequest(request, nextRequest, outputBuffers); diff --git a/services/camera/libcameraservice/device3/Camera3Device.h b/services/camera/libcameraservice/device3/Camera3Device.h index 915c024..e3c98ef 100644 --- a/services/camera/libcameraservice/device3/Camera3Device.h +++ b/services/camera/libcameraservice/device3/Camera3Device.h @@ -346,6 +346,8 @@ class Camera3Device : sp statusTracker, camera3_device_t *hal3Device); + void setNotifyCallback(NotificationListener *listener); + /** * Call after stream (re)-configuration is completed. */ @@ -369,7 +371,8 @@ class Camera3Device : /** * Remove all queued and repeating requests, and pending triggers */ - status_t clear(/*out*/ + status_t clear(NotificationListener *listener, + /*out*/ int64_t *lastFrameNumber = NULL); /** @@ -452,6 +455,8 @@ class Camera3Device : wp mStatusTracker; camera3_device_t *mHal3Device; + NotificationListener *mListener; + const int mId; // The camera ID int mStatusId; // The RequestThread's component ID for // status tracking @@ -611,6 +616,12 @@ class Camera3Device : void notify(const camera3_notify_msg *msg); + // Specific notify handlers + void notifyError(const camera3_error_msg_t &msg, + NotificationListener *listener); + void notifyShutter(const camera3_shutter_msg_t &msg, + NotificationListener *listener); + /** * Static callback forwarding methods from HAL to instance */ diff --git a/services/camera/libcameraservice/device3/Camera3Stream.cpp b/services/camera/libcameraservice/device3/Camera3Stream.cpp index 3f6254f..29ce38c 100644 --- a/services/camera/libcameraservice/device3/Camera3Stream.cpp +++ b/services/camera/libcameraservice/device3/Camera3Stream.cpp @@ -209,6 +209,35 @@ status_t Camera3Stream::finishConfiguration(camera3_device *hal3Device) { return res; } +status_t Camera3Stream::cancelConfiguration() { + ATRACE_CALL(); + Mutex::Autolock l(mLock); + switch (mState) { + case STATE_ERROR: + ALOGE("%s: In error state", __FUNCTION__); + return INVALID_OPERATION; + case STATE_IN_CONFIG: + case STATE_IN_RECONFIG: + // OK + break; + case STATE_CONSTRUCTED: + case STATE_CONFIGURED: + ALOGE("%s: Cannot cancel configuration that hasn't been started", + __FUNCTION__); + return INVALID_OPERATION; + default: + ALOGE("%s: Unknown state", __FUNCTION__); + return INVALID_OPERATION; + } + + camera3_stream::usage = oldUsage; + camera3_stream::max_buffers = oldMaxBuffers; + + mState = STATE_CONSTRUCTED; + + return OK; +} + status_t Camera3Stream::getBuffer(camera3_stream_buffer *buffer) { ATRACE_CALL(); Mutex::Autolock l(mLock); diff --git a/services/camera/libcameraservice/device3/Camera3Stream.h b/services/camera/libcameraservice/device3/Camera3Stream.h index a77f27c..d0e1337 100644 --- a/services/camera/libcameraservice/device3/Camera3Stream.h +++ b/services/camera/libcameraservice/device3/Camera3Stream.h @@ -159,6 +159,13 @@ class Camera3Stream : status_t finishConfiguration(camera3_device *hal3Device); /** + * Cancels the stream configuration process. This returns the stream to the + * initial state, allowing it to be configured again later. + * This is done if the HAL rejects the proposed combined stream configuration + */ + status_t cancelConfiguration(); + + /** * Fill in the camera3_stream_buffer with the next valid buffer for this * stream, to hand over to the HAL. * diff --git a/services/camera/libcameraservice/device3/Camera3StreamInterface.h b/services/camera/libcameraservice/device3/Camera3StreamInterface.h index c93ae15..da989cd 100644 --- a/services/camera/libcameraservice/device3/Camera3StreamInterface.h +++ b/services/camera/libcameraservice/device3/Camera3StreamInterface.h @@ -82,6 +82,13 @@ class Camera3StreamInterface : public virtual RefBase { virtual status_t finishConfiguration(camera3_device *hal3Device) = 0; /** + * Cancels the stream configuration process. This returns the stream to the + * initial state, allowing it to be configured again later. + * This is done if the HAL rejects the proposed combined stream configuration + */ + virtual status_t cancelConfiguration() = 0; + + /** * Fill in the camera3_stream_buffer with the next valid buffer for this * stream, to hand over to the HAL. * -- cgit v1.1