<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_, &notify);<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_, &notify);<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>