<div dir="ltr"><div>Hi Jacopo, thank you for the patch.</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, May 7, 2021 at 12:00 AM Jacopo Mondi <<a href="mailto:jacopo@jmondi.org">jacopo@jmondi.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">The current implementation of CameraDevice::requestComplete() which<br>
handles event notification and calls the framework capture result<br>
callback does not handle error notification precisely enough.<br>
<br>
In detail:<br>
- Error notification is an asynchronous callback that has to be notified<br>
to the framework as soon as an error condition is detected, and it<br>
independent from the process_capture_result() callback<br>
<br>
- Error notification requires the HAL to report the precise error cause,<br>
by specifying the correct CAMERA3_MSG_ERROR_* error code.<br>
<br>
The current implementation only notifies errors of type<br>
CAMERA3_MSG_ERROR_REQUEST at the end of the procedure, before the<br>
callback invocation.<br>
<br>
Rework the procedure to:<br>
<br>
- Notify CAMERA3_MSG_ERROR_DEVICE and close the camera in case a Fatal<br>
error is detected (Fatal does not perform library teardown in<br>
production builds)<br>
<br>
- Notify CAMERA3_MSG_ERROR_REQUEST if the libcamera::Request::status is<br>
different than RequestCompleted and immediately call<br>
process_capture_result() with all buffers in error state.<br>
<br>
- Notify the shutter event as soon as possible<br>
<br>
- Notify CAMERA3_MSG_ERROR_RESULT in case the metadata cannot be<br>
generated correctly and call process_capture_result() with the right<br>
buffer state regardless of metadata availability.<br>
<br>
- Notify CAMERA3_MSG_ERROR_BUFFER for buffers whose post-processing<br>
failed<br>
<br>
While at it, return the CameraStream buffer by calling<br>
cameraStream->putBuffer() regardless of the post-processing result.<br>
<br>
No regression detected when running CTS in LIMITED mode.<br>
<br>
Signed-off-by: Jacopo Mondi <<a href="mailto:jacopo@jmondi.org" target="_blank">jacopo@jmondi.org</a>><br>
---<br>
<br>
v2:<br>
- Rework a comment as suggested by Niklas<br>
- As Niklas reported, dereferencing a unique_ptr<> which owns a nullptr has<br>
an undefined behaviour. Replace that pattern by resetting the resultMetadata<br>
unique_ptr<> with a new one holding an empty CameraMetadata, as the framework<br>
requires on error.<br>
<br>
camera3.h:<br>
* If there was an error producing the result metadata, result must be an<br>
* empty metadata buffer, and notify() must be called with ERROR_RESULT.<br>
<br>
<br>
std::unique_ptr<CameraMetadata> resultMetadata = getResultMetadata(descriptor);<br>
if (!resultMetadata) {<br>
notifyError(descriptor.frameNumber_, descriptor.buffers_[0].stream,<br>
CAMERA3_MSG_ERROR_RESULT);<br>
<br>
/* The camera framework expects an empy metadata pack on error. */<br>
resultMetadata = std::make_unique<CameraMetadata>(0, 0);<br>
}<br>
captureResult.result = resultMetadata->get();<br>
<br>
---<br>
src/android/camera_device.cpp | 132 ++++++++++++++++++++--------------<br>
src/android/camera_device.h | 3 +-<br>
2 files changed, 80 insertions(+), 55 deletions(-)<br>
<br>
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp<br>
index f1f26c775611..ea7ec8c05f88 100644<br>
--- a/src/android/camera_device.cpp<br>
+++ b/src/android/camera_device.cpp<br>
@@ -2025,9 +2025,6 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques<br>
<br>
void CameraDevice::requestComplete(Request *request)<br>
{<br>
- camera3_buffer_status status = CAMERA3_BUFFER_STATUS_OK;<br>
- std::unique_ptr<CameraMetadata> resultMetadata;<br>
-<br>
decltype(descriptors_)::node_type node;<br>
{<br>
std::scoped_lock<std::mutex> lock(mutex_);<br>
@@ -2035,7 +2032,8 @@ void CameraDevice::requestComplete(Request *request)<br>
if (it == descriptors_.end()) {<br>
LOG(HAL, Fatal)<br>
<< "Unknown request: " << request->cookie();<br>
- status = CAMERA3_BUFFER_STATUS_ERROR;<br>
+ close();<br></blockquote><div><br></div><div>close() calls Camera::stop(). Camera::stop() cancels the pending request and requestComplete() will be invoked. (this is not done so cf. <a href="https://patchwork.libcamera.org/project/libcamera/list/?series=1954">https://patchwork.libcamera.org/project/libcamera/list/?series=1954</a>)</div><div>Furthermore, Camera::stop() is a blocking function. So I think calling close() here possibly causes the infinite-loop, e.g. when descriptors_ is somehow empty.</div><div>Ah, but LOG(HAL, Fatal) causes the crash libcamera process, hmm..</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ notifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);<br>
return;<br>
}<br>
<br>
@@ -2043,16 +2041,71 @@ void CameraDevice::requestComplete(Request *request)<br>
}<br>
Camera3RequestDescriptor &descriptor = node.mapped();<br>
<br>
+ /*<br>
+ * Prepare the capture result for the Android camera stack.<br>
+ *<br>
+ * The buffer status is set to OK and later changed to ERROR if<br>
+ * post-processing/compression fails.<br>
+ */<br>
+ camera3_capture_result_t captureResult = {};<br>
+ captureResult.frame_number = descriptor.frameNumber_;<br>
+ captureResult.num_output_buffers = descriptor.buffers_.size();<br>
+ for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {<br>
+ buffer.acquire_fence = -1;<br>
+ buffer.release_fence = -1;<br>
+ buffer.status = CAMERA3_BUFFER_STATUS_OK;<br>
+ }<br>
+ captureResult.output_buffers = descriptor.buffers_.data();<br>
+ captureResult.partial_result = 1;<br>
+<br>
+ /*<br>
+ * If the Request has failed, abort the request by notifying the error<br>
+ * and complete the request with all buffers in error state.<br>
+ */<br>
if (request->status() != Request::RequestComplete) {<br>
- LOG(HAL, Error) << "Request not successfully completed: "<br>
+ LOG(HAL, Error) << "Request " << request->cookie()<br>
+ << " not successfully completed: "<br>
<< request->status();<br>
- status = CAMERA3_BUFFER_STATUS_ERROR;<br>
+<br>
+ notifyError(descriptor.frameNumber_,<br>
+ descriptor.buffers_[0].stream,<br>
+ CAMERA3_MSG_ERROR_REQUEST);<br>
+<br>
+ for (camera3_stream_buffer_t &buffer : descriptor.buffers_)<br>
+ buffer.status = CAMERA3_BUFFER_STATUS_ERROR;<br>
+ callbacks_->process_capture_result(callbacks_, &captureResult);<br>
+<br>
+ return;<br>
}<br>
<br>
+ /*<br>
+ * Notify shutter as soon as we have verified we have a valid request.<br>
+ *<br>
+ * \todo The shutter event notification should be sent to the framework<br>
+ * as soon as possible, earlier than request completion time.<br>
+ */<br>
+ uint64_t sensorTimestamp = static_cast<uint64_t>(request->metadata()<br>
+ .get(controls::SensorTimestamp));<br>
+ notifyShutter(descriptor.frameNumber_, sensorTimestamp);<br>
+<br>
LOG(HAL, Debug) << "Request " << request->cookie() << " completed with "<br>
<< descriptor.buffers_.size() << " streams";<br>
<br>
- resultMetadata = getResultMetadata(descriptor);<br>
+ /*<br>
+ * Generate the metadata associated with the captured buffers.<br>
+ *<br>
+ * Notify if the metadata generation has failed, but continue processing<br>
+ * buffers and return an empty metadata pack.<br>
+ */<br>
+ std::unique_ptr<CameraMetadata> resultMetadata = getResultMetadata(descriptor);<br>
+ if (!resultMetadata) {<br>
+ notifyError(descriptor.frameNumber_, descriptor.buffers_[0].stream,<br>
+ CAMERA3_MSG_ERROR_RESULT);<br>
+<br>
+ /* The camera framework expects an empy metadata pack on error. */<br>
+ resultMetadata = std::make_unique<CameraMetadata>(0, 0);<br>
+ }<br>
+ captureResult.result = resultMetadata->get();<br>
<br>
/* Handle any JPEG compression. */<br>
for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {<br>
@@ -2065,56 +2118,27 @@ void CameraDevice::requestComplete(Request *request)<br>
FrameBuffer *src = request->findBuffer(cameraStream->stream());<br>
if (!src) {<br>
LOG(HAL, Error) << "Failed to find a source stream buffer";<br>
+ buffer.status = CAMERA3_BUFFER_STATUS_ERROR;<br>
+ notifyError(descriptor.frameNumber_, buffer.stream,<br>
+ CAMERA3_MSG_ERROR_BUFFER);<br>
continue;<br></blockquote><div><br></div><div>Is this still continue? Perhaps return?</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
}<br>
<br>
- int ret = cameraStream->process(*src,<br>
- *buffer.buffer,<br>
+ int ret = cameraStream->process(*src, *buffer.buffer,<br>
descriptor.settings_,<br>
resultMetadata.get());<br>
- if (ret) {<br>
- status = CAMERA3_BUFFER_STATUS_ERROR;<br>
- continue;<br>
- }<br>
-<br>
/*<br>
* Return the FrameBuffer to the CameraStream now that we're<br>
* done processing it.<br>
*/<br>
if (cameraStream->type() == CameraStream::Type::Internal)<br>
cameraStream->putBuffer(src);<br>
- }<br>
<br>
- /* Prepare to call back the Android camera stack. */<br>
- camera3_capture_result_t captureResult = {};<br>
- captureResult.frame_number = descriptor.frameNumber_;<br>
- captureResult.num_output_buffers = descriptor.buffers_.size();<br>
- for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {<br>
- buffer.acquire_fence = -1;<br>
- buffer.release_fence = -1;<br>
- buffer.status = status;<br>
- }<br>
- captureResult.output_buffers = descriptor.buffers_.data();<br>
-<br>
- if (status == CAMERA3_BUFFER_STATUS_OK) {<br>
- uint64_t timestamp =<br>
- static_cast<uint64_t>(request->metadata()<br>
- .get(controls::SensorTimestamp));<br>
- notifyShutter(descriptor.frameNumber_, timestamp);<br>
-<br>
- captureResult.partial_result = 1;<br>
- captureResult.result = resultMetadata->get();<br>
- }<br>
-<br>
- if (status == CAMERA3_BUFFER_STATUS_ERROR || !captureResult.result) {<br>
- /* \todo Improve error handling. In case we notify an error<br>
- * because the metadata generation fails, a shutter event has<br>
- * already been notified for this frame number before the error<br>
- * is here signalled. Make sure the error path plays well with<br>
- * the camera stack state machine.<br>
- */<br>
- notifyError(descriptor.frameNumber_,<br>
- descriptor.buffers_[0].stream);<br>
+ if (ret) {<br>
+ buffer.status = CAMERA3_BUFFER_STATUS_ERROR;<br>
+ notifyError(descriptor.frameNumber_, buffer.stream,<br>
+ CAMERA3_MSG_ERROR_BUFFER);<br></blockquote><div><br></div><div>same here.</div><div><br></div><div>-Hiro</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ }<br>
}<br>
<br>
callbacks_->process_capture_result(callbacks_, &captureResult);<br>
@@ -2136,23 +2160,23 @@ void CameraDevice::notifyShutter(uint32_t frameNumber, uint64_t timestamp)<br>
callbacks_->notify(callbacks_, ¬ify);<br>
}<br>
<br>
-void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream)<br>
+void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream,<br>
+ camera3_error_msg_code code)<br>
{<br>
camera3_notify_msg_t notify = {};<br>
<br>
- /*<br>
- * \todo Report and identify the stream number or configuration to<br>
- * clarify the stream that failed.<br>
- */<br>
- LOG(HAL, Error) << "Error occurred on frame " << frameNumber << " ("<br>
- << toPixelFormat(stream->format).toString() << ")";<br>
-<br>
notify.type = CAMERA3_MSG_ERROR;<br>
notify.message.error.error_stream = stream;<br>
notify.message.error.frame_number = frameNumber;<br>
- notify.message.error.error_code = CAMERA3_MSG_ERROR_REQUEST;<br>
+ notify.message.error.error_code = code;<br>
<br>
callbacks_->notify(callbacks_, ¬ify);<br>
+<br>
+ if (stream)<br>
+ LOG(HAL, Error) << "Error occurred on frame " << frameNumber << " ("<br>
+ << toPixelFormat(stream->format).toString() << ")";<br>
+ else<br>
+ LOG(HAL, Error) << "Fatal error occurred on device";<br>
}<br>
<br>
/*<br>
diff --git a/src/android/camera_device.h b/src/android/camera_device.h<br>
index 23457e47767a..8d5da8bc59e1 100644<br>
--- a/src/android/camera_device.h<br>
+++ b/src/android/camera_device.h<br>
@@ -101,7 +101,8 @@ private:<br>
std::tuple<uint32_t, uint32_t> calculateStaticMetadataSize();<br>
libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer);<br>
void notifyShutter(uint32_t frameNumber, uint64_t timestamp);<br>
- void notifyError(uint32_t frameNumber, camera3_stream_t *stream);<br>
+ void notifyError(uint32_t frameNumber, camera3_stream_t *stream,<br>
+ camera3_error_msg_code code);<br>
std::unique_ptr<CameraMetadata> requestTemplatePreview();<br>
std::unique_ptr<CameraMetadata> requestTemplateVideo();<br>
libcamera::PixelFormat toPixelFormat(int format) const;<br>
--<br>
2.31.1<br>
<br>
_______________________________________________<br>
libcamera-devel mailing list<br>
<a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a><br>
<a href="https://lists.libcamera.org/listinfo/libcamera-devel" rel="noreferrer" target="_blank">https://lists.libcamera.org/listinfo/libcamera-devel</a><br>
</blockquote></div></div>