<div dir="ltr"><div dir="ltr">Hi Jacopo,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, May 7, 2021 at 4:36 PM 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">Hi Hiro,<br>
thanks for review<br>
<br>
On Fri, May 07, 2021 at 12:50:13PM +0900, Hirokazu Honda wrote:<br>
> Hi Jacopo, thank you for the patch.<br>
><br>
> On Fri, May 7, 2021 at 12:00 AM Jacopo Mondi <<a href="mailto:jacopo@jmondi.org" target="_blank">jacopo@jmondi.org</a>> wrote:<br>
><br>
> > 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<br>
> > resultMetadata<br>
> > unique_ptr<> with a new one holding an empty CameraMetadata, as the<br>
> > framework<br>
> > requires on error.<br>
> ><br>
> > camera3.h:<br>
> > * If there was an error producing the result metadata, result must be<br>
> > an<br>
> > * empty metadata buffer, and notify() must be called with<br>
> > ERROR_RESULT.<br>
> ><br>
> ><br>
> > std::unique_ptr<CameraMetadata> resultMetadata =<br>
> > getResultMetadata(descriptor);<br>
> > if (!resultMetadata) {<br>
> > notifyError(descriptor.frameNumber_,<br>
> > descriptor.buffers_[0].stream,<br>
> > CAMERA3_MSG_ERROR_RESULT);<br>
> ><br>
> > /* The camera framework expects an empy metadata pack on<br>
> > 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<br>
> > 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: " <<<br>
> > request->cookie();<br>
> > - status = CAMERA3_BUFFER_STATUS_ERROR;<br>
> > + close();<br>
> ><br>
><br>
> close() calls Camera::stop(). Camera::stop() cancels the pending request<br>
> and requestComplete() will be invoked. (this is not done so cf.<br>
> <a href="https://patchwork.libcamera.org/project/libcamera/list/?series=1954" rel="noreferrer" target="_blank">https://patchwork.libcamera.org/project/libcamera/list/?series=1954</a>)<br>
> Furthermore, Camera::stop() is a blocking function. So I think calling<br>
> close() here possibly causes the infinite-loop, e.g. when descriptors_ is<br>
> somehow empty.<br>
<br>
Mmm, you're right, it doesn't seem like an infinite loop but rather a<br>
deadlock, as we're in critical section and all new requestComplete()<br>
calls due to the Camera::stop() will contend this mutex<br>
<br>
> Ah, but LOG(HAL, Fatal) causes the crash libcamera process, hmm..<br>
<br>
I recall we had a compiler option to make Fatal not fatal (sorry...)<br>
in production builds... I don't seem to be able to find it though.<br>
<br>
If Fatal is always fatal, well, I can only notify the ERROR_DEVICE and<br>
then crash, so no close is required.<br>
<br>
If Fatal can actually be disabled I feel like close() will need to be<br>
called according to the camera3 API, but I'm not sure if the framework<br>
does that, or the HAL should do so, as I've found two contradictory<br>
statements:<br>
<br>
<br>
/**<br>
* A serious failure occured. No further frames or buffer streams will<br>
* be produced by the device. Device should be treated as closed. The<br>
* client must reopen the device to use it again. The frame_number field<br>
* is unused.<br>
*/<br>
CAMERA3_MSG_ERROR_DEVICE = 1,<br>
<br>
This seems to imply the device should be re-opened and treated as<br>
closed by the framwork, so the HAL has actually to close it.<br>
<br>
* S6. Error management:<br>
*<br>
* Camera HAL device ops functions that have a return value will all return<br>
* -ENODEV / NULL in case of a serious error. This means the device cannot<br>
* continue operation, and must be closed by the framework. Once this error is<br>
* returned by some method, or if notify() is called with ERROR_DEVICE, only<br>
* the close() method can be called successfully. All other methods will return<br>
* -ENODEV / NULL.<br>
<br>
This seems to imply the framework calls close() after it receives<br>
ERROR_DEVICE.<br>
<br>
Could this maybe clarified with Ricky's team ?<br>
<br></blockquote><div><br></div><div><a class="gmail_plusreply" id="plusReplyChip-0" href="mailto:jcliang@chromium.org" tabindex="-1">+Ricky Liang</a>, would you answer this question?</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">
><br>
><br>
><br>
> > + notifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);<br>
> > return;<br>
> > }<br>
> ><br>
> > @@ -2043,16 +2041,71 @@ void CameraDevice::requestComplete(Request<br>
> > *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<br>
> > 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_,<br>
> > &captureResult);<br>
> > +<br>
> > + return;<br>
> > }<br>
> ><br>
> > + /*<br>
> > + * Notify shutter as soon as we have verified we have a valid<br>
> > request.<br>
> > + *<br>
> > + * \todo The shutter event notification should be sent to the<br>
> > framework<br>
> > + * as soon as possible, earlier than request completion time.<br>
> > + */<br>
> > + uint64_t sensorTimestamp =<br>
> > static_cast<uint64_t>(request->metadata()<br>
> > +<br>
> > .get(controls::SensorTimestamp));<br>
> > + notifyShutter(descriptor.frameNumber_, sensorTimestamp);<br>
> > +<br>
> > LOG(HAL, Debug) << "Request " << request->cookie() << " completed<br>
> > 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<br>
> > processing<br>
> > + * buffers and return an empty metadata pack.<br>
> > + */<br>
> > + std::unique_ptr<CameraMetadata> resultMetadata =<br>
> > getResultMetadata(descriptor);<br>
> > + if (!resultMetadata) {<br>
> > + notifyError(descriptor.frameNumber_,<br>
> > descriptor.buffers_[0].stream,<br>
> > + CAMERA3_MSG_ERROR_RESULT);<br>
> > +<br>
> > + /* The camera framework expects an empy metadata pack on<br>
> > 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<br>
> > *request)<br>
> > FrameBuffer *src =<br>
> > request->findBuffer(cameraStream->stream());<br>
> > if (!src) {<br>
> > LOG(HAL, Error) << "Failed to find a source stream<br>
> > buffer";<br>
> > + buffer.status = CAMERA3_BUFFER_STATUS_ERROR;<br>
> > + notifyError(descriptor.frameNumber_, buffer.stream,<br>
> > + CAMERA3_MSG_ERROR_BUFFER);<br>
> > continue;<br>
> ><br>
><br>
> Is this still continue? Perhaps return?<br>
<br>
Is it ? my understandin is that if a single buffer fails, the request<br>
can be completed and that single buffer state will be set to<br>
CAMERA3_BUFFER_STATUS_ERROR and the framework has to be notified about<br>
the CAMERA3_MSG_ERROR_BUFFER event. Metadata and other succesfully<br>
completed buffers can be returned correctly if they have been properly<br>
filled, hence I think we should continue and process the next<br>
buffer...<br>
<br></blockquote><div><br></div><div>My question is if it is allowed to call notifyError to the same stream multiple times.</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">
A question: are there tests in the cros_camera_test package that<br>
exercize these error paths ?<br>
<br></blockquote><div><br></div><div>I don't think so.</div><div><br></div><div>Thanks,</div><div>-Hiro</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">
Thanks<br>
j<br>
<br>
><br>
><br>
> > }<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<br>
> > 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>
> > -<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 ||<br>
> > !captureResult.result) {<br>
> > - /* \todo Improve error handling. In case we notify an error<br>
> > - * because the metadata generation fails, a shutter event<br>
> > has<br>
> > - * already been notified for this frame number before the<br>
> > error<br>
> > - * is here signalled. Make sure the error path plays well<br>
> > 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>
> ><br>
><br>
> same here.<br>
><br>
> -Hiro<br>
><br>
> + }<br>
> > }<br>
> ><br>
> > callbacks_->process_capture_result(callbacks_, &captureResult);<br>
> > @@ -2136,23 +2160,23 @@ void CameraDevice::notifyShutter(uint32_t<br>
> > frameNumber, uint64_t timestamp)<br>
> > callbacks_->notify(callbacks_, ¬ify);<br>
> > }<br>
> ><br>
> > -void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t<br>
> > *stream)<br>
> > +void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t<br>
> > *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>
> > ("<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 " <<<br>
> > frameNumber << " ("<br>
> > + <<<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<br>
> > 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>
> ><br>
</blockquote></div></div>