[{"id":17110,"web_url":"https://patchwork.libcamera.org/comment/17110/","msgid":"","date":"2021-05-22T09:21:39","subject":"Re: [libcamera-devel] [PATCH v3 1/8] android: Rework request\n\tcompletion notification","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your work.\n\nOn 2021-05-21 17:42:20 +0200, Jacopo Mondi wrote:\n> The current implementation of CameraDevice::requestComplete() which\n> handles event notification and calls the framework capture result\n> callback does not handle error notification precisely enough.\n> \n> In detail:\n> - Error notification is an asynchronous callback that has to be notified\n> to the framework as soon as an error condition is detected, and it\n> independent from the process_capture_result() callback\n> \n> - Error notification requires the HAL to report the precise error cause,\n> by specifying the correct CAMERA3_MSG_ERROR_* error code.\n> \n> The current implementation only notifies errors of type\n> CAMERA3_MSG_ERROR_REQUEST at the end of the procedure, before the\n> callback invocation.\n> \n> Rework the procedure to:\n> \n> - Notify CAMERA3_MSG_ERROR_DEVICE and perform library tear-down in case\n> a Fatal error is detected\n> \n> - Notify CAMERA3_MSG_ERROR_REQUEST if the libcamera::Request::status is\n> different than RequestCompleted and immediately call\n> process_capture_result() with all buffers in error state.\n> \n> - Notify the shutter event as soon as possible\n> \n> - Notify CAMERA3_MSG_ERROR_RESULT in case the metadata cannot be\n> generated correctly and call process_capture_result() with the right\n> buffer state regardless of metadata availability.\n> \n> - Notify CAMERA3_MSG_ERROR_BUFFER for buffers whose post-processing\n> failed\n> \n> While at it, return the CameraStream buffer by calling\n> cameraStream->putBuffer() regardless of the post-processing result.\n> \n> No regression detected when running CTS in LIMITED mode.\n> \n> Signed-off-by: Jacopo Mondi \n> Reviewed-by: Hirokazu Honda \n\nI can't build master as it's broken for my environment but I trust this \ncompiles :-)\n\nReviewed-by: Niklas Söderlund \n\n> ---\n> src/android/camera_device.cpp | 138 +++++++++++++++++++++-------------\n> src/android/camera_device.h | 3 +-\n> 2 files changed, 86 insertions(+), 55 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index b32e8be59134..bd96b355ea92 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -1956,17 +1956,20 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \n> void CameraDevice::requestComplete(Request *request)\n> {\n> -\tcamera3_buffer_status status = CAMERA3_BUFFER_STATUS_OK;\n> -\tstd::unique_ptr resultMetadata;\n> -\n> \tdecltype(descriptors_)::node_type node;\n> \t{\n> \t\tstd::scoped_lock lock(mutex_);\n> \t\tauto it = descriptors_.find(request->cookie());\n> \t\tif (it == descriptors_.end()) {\n> +\t\t\t/*\n> +\t\t\t * \\todo Clarify if the Camera has to be closed on\n> +\t\t\t * ERROR_DEVICE and possibly demote the Fatal to simple\n> +\t\t\t * Error.\n> +\t\t\t */\n> +\t\t\tnotifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);\n> \t\t\tLOG(HAL, Fatal)\n> \t\t\t\t<< \"Unknown request: \" << request->cookie();\n> -\t\t\tstatus = CAMERA3_BUFFER_STATUS_ERROR;\n> +\n> \t\t\treturn;\n> \t\t}\n> \n> @@ -1974,16 +1977,72 @@ void CameraDevice::requestComplete(Request *request)\n> \t}\n> \tCamera3RequestDescriptor &descriptor = node.mapped();\n> \n> +\t/*\n> +\t * Prepare the capture result for the Android camera stack.\n> +\t *\n> +\t * The buffer status is set to OK and later changed to ERROR if\n> +\t * post-processing/compression fails.\n> +\t */\n> +\tcamera3_capture_result_t captureResult = {};\n> +\tcaptureResult.frame_number = descriptor.frameNumber_;\n> +\tcaptureResult.num_output_buffers = descriptor.buffers_.size();\n> +\tfor (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> +\t\tbuffer.acquire_fence = -1;\n> +\t\tbuffer.release_fence = -1;\n> +\t\tbuffer.status = CAMERA3_BUFFER_STATUS_OK;\n> +\t}\n> +\tcaptureResult.output_buffers = descriptor.buffers_.data();\n> +\tcaptureResult.partial_result = 1;\n> +\n> +\t/*\n> +\t * If the Request has failed, abort the request by notifying the error\n> +\t * and complete the request with all buffers in error state.\n> +\t */\n> \tif (request->status() != Request::RequestComplete) {\n> -\t\tLOG(HAL, Error) << \"Request not successfully completed: \"\n> +\t\tLOG(HAL, Error) << \"Request \" << request->cookie()\n> +\t\t\t\t<< \" not successfully completed: \"\n> \t\t\t\t<< request->status();\n> -\t\tstatus = CAMERA3_BUFFER_STATUS_ERROR;\n> +\n> +\t\tnotifyError(descriptor.frameNumber_,\n> +\t\t\t descriptor.buffers_[0].stream,\n> +\t\t\t CAMERA3_MSG_ERROR_REQUEST);\n> +\n> +\t\tcaptureResult.partial_result = 0;\n> +\t\tfor (camera3_stream_buffer_t &buffer : descriptor.buffers_)\n> +\t\t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> +\t\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n> +\n> +\t\treturn;\n> \t}\n> \n> +\t/*\n> +\t * Notify shutter as soon as we have verified we have a valid request.\n> +\t *\n> +\t * \\todo The shutter event notification should be sent to the framework\n> +\t * as soon as possible, earlier than request completion time.\n> +\t */\n> +\tuint64_t sensorTimestamp = static_cast(request->metadata()\n> +\t\t\t\t\t\t\t .get(controls::SensorTimestamp));\n> +\tnotifyShutter(descriptor.frameNumber_, sensorTimestamp);\n> +\n> \tLOG(HAL, Debug) << \"Request \" << request->cookie() << \" completed with \"\n> \t\t\t<< descriptor.buffers_.size() << \" streams\";\n> \n> -\tresultMetadata = getResultMetadata(descriptor);\n> +\t/*\n> +\t * Generate the metadata associated with the captured buffers.\n> +\t *\n> +\t * Notify if the metadata generation has failed, but continue processing\n> +\t * buffers and return an empty metadata pack.\n> +\t */\n> +\tstd::unique_ptr resultMetadata = getResultMetadata(descriptor);\n> +\tif (!resultMetadata) {\n> +\t\tnotifyError(descriptor.frameNumber_, descriptor.buffers_[0].stream,\n> +\t\t\t CAMERA3_MSG_ERROR_RESULT);\n> +\n> +\t\t/* The camera framework expects an empy metadata pack on error. */\n> +\t\tresultMetadata = std::make_unique(0, 0);\n> +\t}\n> +\tcaptureResult.result = resultMetadata->get();\n> \n> \t/* Handle any JPEG compression. */\n> \tfor (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> @@ -1996,56 +2055,27 @@ void CameraDevice::requestComplete(Request *request)\n> \t\tFrameBuffer *src = request->findBuffer(cameraStream->stream());\n> \t\tif (!src) {\n> \t\t\tLOG(HAL, Error) << \"Failed to find a source stream buffer\";\n> +\t\t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> +\t\t\tnotifyError(descriptor.frameNumber_, buffer.stream,\n> +\t\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n> \t\t\tcontinue;\n> \t\t}\n> \n> -\t\tint ret = cameraStream->process(*src,\n> -\t\t\t\t\t\t*buffer.buffer,\n> +\t\tint ret = cameraStream->process(*src, *buffer.buffer,\n> \t\t\t\t\t\tdescriptor.settings_,\n> \t\t\t\t\t\tresultMetadata.get());\n> -\t\tif (ret) {\n> -\t\t\tstatus = CAMERA3_BUFFER_STATUS_ERROR;\n> -\t\t\tcontinue;\n> -\t\t}\n> -\n> \t\t/*\n> \t\t * Return the FrameBuffer to the CameraStream now that we're\n> \t\t * done processing it.\n> \t\t */\n> \t\tif (cameraStream->type() == CameraStream::Type::Internal)\n> \t\t\tcameraStream->putBuffer(src);\n> -\t}\n> \n> -\t/* Prepare to call back the Android camera stack. */\n> -\tcamera3_capture_result_t captureResult = {};\n> -\tcaptureResult.frame_number = descriptor.frameNumber_;\n> -\tcaptureResult.num_output_buffers = descriptor.buffers_.size();\n> -\tfor (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> -\t\tbuffer.acquire_fence = -1;\n> -\t\tbuffer.release_fence = -1;\n> -\t\tbuffer.status = status;\n> -\t}\n> -\tcaptureResult.output_buffers = descriptor.buffers_.data();\n> -\n> -\tif (status == CAMERA3_BUFFER_STATUS_OK) {\n> -\t\tuint64_t timestamp =\n> -\t\t\tstatic_cast(request->metadata()\n> -\t\t\t\t\t .get(controls::SensorTimestamp));\n> -\t\tnotifyShutter(descriptor.frameNumber_, timestamp);\n> -\n> -\t\tcaptureResult.partial_result = 1;\n> -\t\tcaptureResult.result = resultMetadata->get();\n> -\t}\n> -\n> -\tif (status == CAMERA3_BUFFER_STATUS_ERROR || !captureResult.result) {\n> -\t\t/* \\todo Improve error handling. In case we notify an error\n> -\t\t * because the metadata generation fails, a shutter event has\n> -\t\t * already been notified for this frame number before the error\n> -\t\t * is here signalled. Make sure the error path plays well with\n> -\t\t * the camera stack state machine.\n> -\t\t */\n> -\t\tnotifyError(descriptor.frameNumber_,\n> -\t\t\t descriptor.buffers_[0].stream);\n> +\t\tif (ret) {\n> +\t\t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> +\t\t\tnotifyError(descriptor.frameNumber_, buffer.stream,\n> +\t\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n> +\t\t}\n> \t}\n> \n> \tcallbacks_->process_capture_result(callbacks_, &captureResult);\n> @@ -2067,23 +2097,23 @@ void CameraDevice::notifyShutter(uint32_t frameNumber, uint64_t timestamp)\n> \tcallbacks_->notify(callbacks_, ¬ify);\n> }\n> \n> -void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream)\n> +void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream,\n> +\t\t\t camera3_error_msg_code code)\n> {\n> \tcamera3_notify_msg_t notify = {};\n> \n> -\t/*\n> -\t * \\todo Report and identify the stream number or configuration to\n> -\t * clarify the stream that failed.\n> -\t */\n> -\tLOG(HAL, Error) << \"Error occurred on frame \" << frameNumber << \" (\"\n> -\t\t\t<< toPixelFormat(stream->format).toString() << \")\";\n> -\n> \tnotify.type = CAMERA3_MSG_ERROR;\n> \tnotify.message.error.error_stream = stream;\n> \tnotify.message.error.frame_number = frameNumber;\n> -\tnotify.message.error.error_code = CAMERA3_MSG_ERROR_REQUEST;\n> +\tnotify.message.error.error_code = code;\n> \n> \tcallbacks_->notify(callbacks_, ¬ify);\n> +\n> +\tif (stream)\n> +\t\tLOG(HAL, Error) << \"Error occurred on frame \" << frameNumber << \" (\"\n> +\t\t\t\t<< toPixelFormat(stream->format).toString() << \")\";\n> +\telse\n> +\t\tLOG(HAL, Error) << \"Fatal error occurred on device\";\n> }\n> \n> /*\n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index ae162a452499..a34e8a2cd900 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -100,7 +100,8 @@ private:\n> \n> \tlibcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer);\n> \tvoid notifyShutter(uint32_t frameNumber, uint64_t timestamp);\n> -\tvoid notifyError(uint32_t frameNumber, camera3_stream_t *stream);\n> +\tvoid notifyError(uint32_t frameNumber, camera3_stream_t *stream,\n> +\t\t\t camera3_error_msg_code code);\n> \tstd::unique_ptr requestTemplatePreview();\n> \tstd::unique_ptr requestTemplateVideo();\n> \tlibcamera::PixelFormat toPixelFormat(int format) const;\n> -- \n> 2.31.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 701BBC3200\n\tfor ;\n\tSat, 22 May 2021 09:21:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B022668921;\n\tSat, 22 May 2021 11:21:43 +0200 (CEST)","from mail-lj1-x22e.google.com (mail-lj1-x22e.google.com\n\t[IPv6:2a00:1450:4864:20::22e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 94B6D602B0\n\tfor ;\n\tSat, 22 May 2021 11:21:42 +0200 (CEST)","by mail-lj1-x22e.google.com with SMTP id p20so26847819ljj.8\n\tfor ;\n\tSat, 22 May 2021 02:21:42 -0700 (PDT)","from localhost (h-155-4-209-203.A463.priv.bahnhof.se.\n\t[155.4.209.203]) by smtp.gmail.com with ESMTPSA id\n\ts1sm779740ljj.69.2021.05.22.02.21.40\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tSat, 22 May 2021 02:21:40 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"za9exv8h\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=EpnusL6v1xQYo0XNqBAm6kt9XUMoehl5gw515qo42Fo=;\n\tb=za9exv8hAjIHRGUFH6kCqPE6O5ucXzScBpiWL9C+CXUsLASRyUw0c1xhnuSAXHcR/2\n\trUiBk03fOvCiDrcePsDSwaq9yTH1mnnifdNEhJjHuAdMaCkRcYWp3Zr/x9nU3QdNANLL\n\tJtc+mHO7Yf2ahpkvbHEu8RsHPyDTnLcEMAKTh4A+QOd67Gubn5YqdjsBcGcaSOWxGkoO\n\t9g7iyVa9ssDuW3T+qXkJB2IjBELPWwJwSig+pJv1U+hJMAe58OlHIopG0py7s5IfEbb0\n\t5MlteKNgXwG79GjAT5pS0JlEP34lEOj0rRSI8LJQVgimtFWHDCOVLgvskj/OYNUueUWi\n\t+AWw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=EpnusL6v1xQYo0XNqBAm6kt9XUMoehl5gw515qo42Fo=;\n\tb=K0/5VUBKtGfaZ45yx6A5rcO5wI6BvBj5TCwkmXVW6sA3xHf7DNFjPBjF3/bDVUMUF1\n\tjpzcDRjwps8oXIFFV7WiEnoiJZFMQoMAclvPxl0mi6mlJm4Qa7U+B+Hsa7MVsFwLiCkG\n\tzDVQ1rvF6wcy2acinyBj4atzqiLjv+S8KIMbX0NHFms2WMUc2vVWM8a0WZnQUspGkryq\n\tkNNQ3LYI/13Gc6RTpNp+T+AKCePOZikoRN8QcaN4NGkK+qhKnrYw/cWPYSZdJQPdmoIs\n\t0vrYfNMFBZF07Mlvx4lPhU/QVE8Fv6U21CBsA7UThvt+jWBoVTO61Lb8UPSXr+xPsqr0\n\tBT4g==","X-Gm-Message-State":"AOAM533Wq4ER5buJgDpdZjh18Dct0mcr0nBsSi2I75fxmdGYRIjMWOud\n\tN6EHKRuPO1sxeYkb0q7C5Wp5aw==","X-Google-Smtp-Source":"ABdhPJznFKiV6WT7J/8/g0LgGD4pjeqgnldd5D9uGgvjCk4T2PauL5nsKzqvGjroYlZFGZCXWqsi4Q==","X-Received":"by 2002:a05:651c:b12:: with SMTP id\n\tb18mr9663168ljr.24.1621675301527; \n\tSat, 22 May 2021 02:21:41 -0700 (PDT)","Date":"Sat, 22 May 2021 11:21:39 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210521154227.60186-1-jacopo@jmondi.org>\n\t<20210521154227.60186-2-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20210521154227.60186-2-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v3 1/8] android: Rework request\n\tcompletion notification","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17134,"web_url":"https://patchwork.libcamera.org/comment/17134/","msgid":"","date":"2021-05-23T17:50:32","subject":"Re: [libcamera-devel] [PATCH v3 1/8] android: Rework request\n\tcompletion notification","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Fri, May 21, 2021 at 05:42:20PM +0200, Jacopo Mondi wrote:\n> The current implementation of CameraDevice::requestComplete() which\n> handles event notification and calls the framework capture result\n> callback does not handle error notification precisely enough.\n> \n> In detail:\n> - Error notification is an asynchronous callback that has to be notified\n> to the framework as soon as an error condition is detected, and it\n> independent from the process_capture_result() callback\n> \n> - Error notification requires the HAL to report the precise error cause,\n> by specifying the correct CAMERA3_MSG_ERROR_* error code.\n> \n> The current implementation only notifies errors of type\n> CAMERA3_MSG_ERROR_REQUEST at the end of the procedure, before the\n> callback invocation.\n> \n> Rework the procedure to:\n> \n> - Notify CAMERA3_MSG_ERROR_DEVICE and perform library tear-down in case\n> a Fatal error is detected\n> \n> - Notify CAMERA3_MSG_ERROR_REQUEST if the libcamera::Request::status is\n> different than RequestCompleted and immediately call\n> process_capture_result() with all buffers in error state.\n> \n> - Notify the shutter event as soon as possible\n> \n> - Notify CAMERA3_MSG_ERROR_RESULT in case the metadata cannot be\n> generated correctly and call process_capture_result() with the right\n> buffer state regardless of metadata availability.\n> \n> - Notify CAMERA3_MSG_ERROR_BUFFER for buffers whose post-processing\n> failed\n> \n> While at it, return the CameraStream buffer by calling\n> cameraStream->putBuffer() regardless of the post-processing result.\n> \n> No regression detected when running CTS in LIMITED mode.\n> \n> Signed-off-by: Jacopo Mondi \n> Reviewed-by: Hirokazu Honda \n> ---\n> src/android/camera_device.cpp | 138 +++++++++++++++++++++-------------\n> src/android/camera_device.h | 3 +-\n> 2 files changed, 86 insertions(+), 55 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index b32e8be59134..bd96b355ea92 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -1956,17 +1956,20 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \n> void CameraDevice::requestComplete(Request *request)\n> {\n> -\tcamera3_buffer_status status = CAMERA3_BUFFER_STATUS_OK;\n> -\tstd::unique_ptr resultMetadata;\n> -\n> \tdecltype(descriptors_)::node_type node;\n> \t{\n> \t\tstd::scoped_lock lock(mutex_);\n> \t\tauto it = descriptors_.find(request->cookie());\n> \t\tif (it == descriptors_.end()) {\n> +\t\t\t/*\n> +\t\t\t * \\todo Clarify if the Camera has to be closed on\n> +\t\t\t * ERROR_DEVICE and possibly demote the Fatal to simple\n> +\t\t\t * Error.\n> +\t\t\t */\n> +\t\t\tnotifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);\n> \t\t\tLOG(HAL, Fatal)\n> \t\t\t\t<< \"Unknown request: \" << request->cookie();\n\nFor the same reason as explained below, I'd move the LOG() before\nnotifyError().\n\n> -\t\t\tstatus = CAMERA3_BUFFER_STATUS_ERROR;\n> +\n> \t\t\treturn;\n> \t\t}\n> \n> @@ -1974,16 +1977,72 @@ void CameraDevice::requestComplete(Request *request)\n> \t}\n> \tCamera3RequestDescriptor &descriptor = node.mapped();\n> \n> +\t/*\n> +\t * Prepare the capture result for the Android camera stack.\n> +\t *\n> +\t * The buffer status is set to OK and later changed to ERROR if\n> +\t * post-processing/compression fails.\n> +\t */\n> +\tcamera3_capture_result_t captureResult = {};\n> +\tcaptureResult.frame_number = descriptor.frameNumber_;\n> +\tcaptureResult.num_output_buffers = descriptor.buffers_.size();\n> +\tfor (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> +\t\tbuffer.acquire_fence = -1;\n> +\t\tbuffer.release_fence = -1;\n> +\t\tbuffer.status = CAMERA3_BUFFER_STATUS_OK;\n> +\t}\n> +\tcaptureResult.output_buffers = descriptor.buffers_.data();\n> +\tcaptureResult.partial_result = 1;\n> +\n> +\t/*\n> +\t * If the Request has failed, abort the request by notifying the error\n> +\t * and complete the request with all buffers in error state.\n> +\t */\n> \tif (request->status() != Request::RequestComplete) {\n> -\t\tLOG(HAL, Error) << \"Request not successfully completed: \"\n> +\t\tLOG(HAL, Error) << \"Request \" << request->cookie()\n> +\t\t\t\t<< \" not successfully completed: \"\n> \t\t\t\t<< request->status();\n> -\t\tstatus = CAMERA3_BUFFER_STATUS_ERROR;\n> +\n> +\t\tnotifyError(descriptor.frameNumber_,\n> +\t\t\t descriptor.buffers_[0].stream,\n\nShouldn't you pass nullptr for the stream pointer in this case ?\n\n> +\t\t\t CAMERA3_MSG_ERROR_REQUEST);\n> +\n> +\t\tcaptureResult.partial_result = 0;\n> +\t\tfor (camera3_stream_buffer_t &buffer : descriptor.buffers_)\n> +\t\t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> +\t\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n> +\n> +\t\treturn;\n> \t}\n> \n> +\t/*\n> +\t * Notify shutter as soon as we have verified we have a valid request.\n> +\t *\n> +\t * \\todo The shutter event notification should be sent to the framework\n> +\t * as soon as possible, earlier than request completion time.\n> +\t */\n> +\tuint64_t sensorTimestamp = static_cast(request->metadata()\n> +\t\t\t\t\t\t\t .get(controls::SensorTimestamp));\n> +\tnotifyShutter(descriptor.frameNumber_, sensorTimestamp);\n> +\n> \tLOG(HAL, Debug) << \"Request \" << request->cookie() << \" completed with \"\n> \t\t\t<< descriptor.buffers_.size() << \" streams\";\n> \n> -\tresultMetadata = getResultMetadata(descriptor);\n> +\t/*\n> +\t * Generate the metadata associated with the captured buffers.\n> +\t *\n> +\t * Notify if the metadata generation has failed, but continue processing\n> +\t * buffers and return an empty metadata pack.\n> +\t */\n> +\tstd::unique_ptr resultMetadata = getResultMetadata(descriptor);\n> +\tif (!resultMetadata) {\n> +\t\tnotifyError(descriptor.frameNumber_, descriptor.buffers_[0].stream,\n\nSame here, nullptr for the stream ?\n\n> +\t\t\t CAMERA3_MSG_ERROR_RESULT);\n> +\n> +\t\t/* The camera framework expects an empy metadata pack on error. */\n> +\t\tresultMetadata = std::make_unique(0, 0);\n> +\t}\n> +\tcaptureResult.result = resultMetadata->get();\n> \n> \t/* Handle any JPEG compression. */\n> \tfor (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> @@ -1996,56 +2055,27 @@ void CameraDevice::requestComplete(Request *request)\n> \t\tFrameBuffer *src = request->findBuffer(cameraStream->stream());\n> \t\tif (!src) {\n> \t\t\tLOG(HAL, Error) << \"Failed to find a source stream buffer\";\n> +\t\t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> +\t\t\tnotifyError(descriptor.frameNumber_, buffer.stream,\n> +\t\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n> \t\t\tcontinue;\n> \t\t}\n> \n> -\t\tint ret = cameraStream->process(*src,\n> -\t\t\t\t\t\t*buffer.buffer,\n> +\t\tint ret = cameraStream->process(*src, *buffer.buffer,\n> \t\t\t\t\t\tdescriptor.settings_,\n> \t\t\t\t\t\tresultMetadata.get());\n> -\t\tif (ret) {\n> -\t\t\tstatus = CAMERA3_BUFFER_STATUS_ERROR;\n> -\t\t\tcontinue;\n> -\t\t}\n> -\n> \t\t/*\n> \t\t * Return the FrameBuffer to the CameraStream now that we're\n> \t\t * done processing it.\n> \t\t */\n> \t\tif (cameraStream->type() == CameraStream::Type::Internal)\n> \t\t\tcameraStream->putBuffer(src);\n> -\t}\n> \n> -\t/* Prepare to call back the Android camera stack. */\n> -\tcamera3_capture_result_t captureResult = {};\n> -\tcaptureResult.frame_number = descriptor.frameNumber_;\n> -\tcaptureResult.num_output_buffers = descriptor.buffers_.size();\n> -\tfor (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> -\t\tbuffer.acquire_fence = -1;\n> -\t\tbuffer.release_fence = -1;\n> -\t\tbuffer.status = status;\n> -\t}\n> -\tcaptureResult.output_buffers = descriptor.buffers_.data();\n> -\n> -\tif (status == CAMERA3_BUFFER_STATUS_OK) {\n> -\t\tuint64_t timestamp =\n> -\t\t\tstatic_cast(request->metadata()\n> -\t\t\t\t\t .get(controls::SensorTimestamp));\n> -\t\tnotifyShutter(descriptor.frameNumber_, timestamp);\n> -\n> -\t\tcaptureResult.partial_result = 1;\n> -\t\tcaptureResult.result = resultMetadata->get();\n> -\t}\n> -\n> -\tif (status == CAMERA3_BUFFER_STATUS_ERROR || !captureResult.result) {\n> -\t\t/* \\todo Improve error handling. In case we notify an error\n> -\t\t * because the metadata generation fails, a shutter event has\n> -\t\t * already been notified for this frame number before the error\n> -\t\t * is here signalled. Make sure the error path plays well with\n> -\t\t * the camera stack state machine.\n> -\t\t */\n> -\t\tnotifyError(descriptor.frameNumber_,\n> -\t\t\t descriptor.buffers_[0].stream);\n> +\t\tif (ret) {\n> +\t\t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> +\t\t\tnotifyError(descriptor.frameNumber_, buffer.stream,\n> +\t\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n> +\t\t}\n> \t}\n> \n> \tcallbacks_->process_capture_result(callbacks_, &captureResult);\n> @@ -2067,23 +2097,23 @@ void CameraDevice::notifyShutter(uint32_t frameNumber, uint64_t timestamp)\n> \tcallbacks_->notify(callbacks_, ¬ify);\n> }\n> \n> -void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream)\n> +void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream,\n> +\t\t\t camera3_error_msg_code code)\n> {\n> \tcamera3_notify_msg_t notify = {};\n> \n> -\t/*\n> -\t * \\todo Report and identify the stream number or configuration to\n> -\t * clarify the stream that failed.\n> -\t */\n> -\tLOG(HAL, Error) << \"Error occurred on frame \" << frameNumber << \" (\"\n> -\t\t\t<< toPixelFormat(stream->format).toString() << \")\";\n> -\n> \tnotify.type = CAMERA3_MSG_ERROR;\n> \tnotify.message.error.error_stream = stream;\n> \tnotify.message.error.frame_number = frameNumber;\n> -\tnotify.message.error.error_code = CAMERA3_MSG_ERROR_REQUEST;\n> +\tnotify.message.error.error_code = code;\n> \n> \tcallbacks_->notify(callbacks_, ¬ify);\n> +\n> +\tif (stream)\n> +\t\tLOG(HAL, Error) << \"Error occurred on frame \" << frameNumber << \" (\"\n> +\t\t\t\t<< toPixelFormat(stream->format).toString() << \")\";\n> +\telse\n> +\t\tLOG(HAL, Error) << \"Fatal error occurred on device\";\n\nI'd keep this at the top of the function. Otherwise, notifying the\ncamera server first, we could get a call from the camera service\nsynchronously that would result in other messages being logged before\nour error message. For instance, we could have\n\n\tClosing device\n\tFatal error occurred on device\n\nThe other way around would be better.\n\nVery nice refactoring overall.\n\nReviewed-by: Laurent Pinchart \n\n> }\n> \n> /*\n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index ae162a452499..a34e8a2cd900 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -100,7 +100,8 @@ private:\n> \n> \tlibcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer);\n> \tvoid notifyShutter(uint32_t frameNumber, uint64_t timestamp);\n> -\tvoid notifyError(uint32_t frameNumber, camera3_stream_t *stream);\n> +\tvoid notifyError(uint32_t frameNumber, camera3_stream_t *stream,\n> +\t\t\t camera3_error_msg_code code);\n> \tstd::unique_ptr requestTemplatePreview();\n> \tstd::unique_ptr requestTemplateVideo();\n> \tlibcamera::PixelFormat toPixelFormat(int format) const;","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 373A0C3200\n\tfor ;\n\tSun, 23 May 2021 17:50:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 90E89602B1;\n\tSun, 23 May 2021 19:50:37 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EF452601A9\n\tfor ;\n\tSun, 23 May 2021 19:50:35 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7D45D2A8;\n\tSun, 23 May 2021 19:50:35 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"SpiwKWv+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1621792235;\n\tbh=FCbuMfx0I6aLFDjmK2bm879IqbmfuyJpByJXepdwXE0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=SpiwKWv+Aaogde67KXsX5kpAXcO9skgO2yqA4I4ZzZIrP4+MdRCx2RbnzDFhYvY8d\n\tj7cJKVVJXNN/d4CBzgXUY0VyTb/oWMlvYiVtWr7EWJ4YzRLR2VwpFzFTf+thX4bT54\n\tPeFTmCTtGznImZGbQy7oYa97kkHxm0cztRJKheEM=","Date":"Sun, 23 May 2021 20:50:32 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210521154227.60186-1-jacopo@jmondi.org>\n\t<20210521154227.60186-2-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210521154227.60186-2-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v3 1/8] android: Rework request\n\tcompletion notification","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17316,"web_url":"https://patchwork.libcamera.org/comment/17316/","msgid":"<20210527082041.pkmx5dtvbwj6kj64@uno.localdomain>","date":"2021-05-27T08:20:41","subject":"Re: [libcamera-devel] [PATCH v3 1/8] android: Rework request\n\tcompletion notification","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Sun, May 23, 2021 at 08:50:32PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Fri, May 21, 2021 at 05:42:20PM +0200, Jacopo Mondi wrote:\n> > The current implementation of CameraDevice::requestComplete() which\n> > handles event notification and calls the framework capture result\n> > callback does not handle error notification precisely enough.\n> >\n> > In detail:\n> > - Error notification is an asynchronous callback that has to be notified\n> > to the framework as soon as an error condition is detected, and it\n> > independent from the process_capture_result() callback\n> >\n> > - Error notification requires the HAL to report the precise error cause,\n> > by specifying the correct CAMERA3_MSG_ERROR_* error code.\n> >\n> > The current implementation only notifies errors of type\n> > CAMERA3_MSG_ERROR_REQUEST at the end of the procedure, before the\n> > callback invocation.\n> >\n> > Rework the procedure to:\n> >\n> > - Notify CAMERA3_MSG_ERROR_DEVICE and perform library tear-down in case\n> > a Fatal error is detected\n> >\n> > - Notify CAMERA3_MSG_ERROR_REQUEST if the libcamera::Request::status is\n> > different than RequestCompleted and immediately call\n> > process_capture_result() with all buffers in error state.\n> >\n> > - Notify the shutter event as soon as possible\n> >\n> > - Notify CAMERA3_MSG_ERROR_RESULT in case the metadata cannot be\n> > generated correctly and call process_capture_result() with the right\n> > buffer state regardless of metadata availability.\n> >\n> > - Notify CAMERA3_MSG_ERROR_BUFFER for buffers whose post-processing\n> > failed\n> >\n> > While at it, return the CameraStream buffer by calling\n> > cameraStream->putBuffer() regardless of the post-processing result.\n> >\n> > No regression detected when running CTS in LIMITED mode.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > Reviewed-by: Hirokazu Honda \n> > ---\n> > src/android/camera_device.cpp | 138 +++++++++++++++++++++-------------\n> > src/android/camera_device.h | 3 +-\n> > 2 files changed, 86 insertions(+), 55 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index b32e8be59134..bd96b355ea92 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -1956,17 +1956,20 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> >\n> > void CameraDevice::requestComplete(Request *request)\n> > {\n> > -\tcamera3_buffer_status status = CAMERA3_BUFFER_STATUS_OK;\n> > -\tstd::unique_ptr resultMetadata;\n> > -\n> > \tdecltype(descriptors_)::node_type node;\n> > \t{\n> > \t\tstd::scoped_lock lock(mutex_);\n> > \t\tauto it = descriptors_.find(request->cookie());\n> > \t\tif (it == descriptors_.end()) {\n> > +\t\t\t/*\n> > +\t\t\t * \\todo Clarify if the Camera has to be closed on\n> > +\t\t\t * ERROR_DEVICE and possibly demote the Fatal to simple\n> > +\t\t\t * Error.\n> > +\t\t\t */\n> > +\t\t\tnotifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);\n> > \t\t\tLOG(HAL, Fatal)\n> > \t\t\t\t<< \"Unknown request: \" << request->cookie();\n>\n> For the same reason as explained below, I'd move the LOG() before\n> notifyError().\n\nI've taken all other suggestions in but this one. As the log is Fatal,\nI would notify the framework first.\n\nThanks\n j\n\n>\n> > -\t\t\tstatus = CAMERA3_BUFFER_STATUS_ERROR;\n> > +\n> > \t\t\treturn;\n> > \t\t}\n> >\n> > @@ -1974,16 +1977,72 @@ void CameraDevice::requestComplete(Request *request)\n> > \t}\n> > \tCamera3RequestDescriptor &descriptor = node.mapped();\n> >\n> > +\t/*\n> > +\t * Prepare the capture result for the Android camera stack.\n> > +\t *\n> > +\t * The buffer status is set to OK and later changed to ERROR if\n> > +\t * post-processing/compression fails.\n> > +\t */\n> > +\tcamera3_capture_result_t captureResult = {};\n> > +\tcaptureResult.frame_number = descriptor.frameNumber_;\n> > +\tcaptureResult.num_output_buffers = descriptor.buffers_.size();\n> > +\tfor (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> > +\t\tbuffer.acquire_fence = -1;\n> > +\t\tbuffer.release_fence = -1;\n> > +\t\tbuffer.status = CAMERA3_BUFFER_STATUS_OK;\n> > +\t}\n> > +\tcaptureResult.output_buffers = descriptor.buffers_.data();\n> > +\tcaptureResult.partial_result = 1;\n> > +\n> > +\t/*\n> > +\t * If the Request has failed, abort the request by notifying the error\n> > +\t * and complete the request with all buffers in error state.\n> > +\t */\n> > \tif (request->status() != Request::RequestComplete) {\n> > -\t\tLOG(HAL, Error) << \"Request not successfully completed: \"\n> > +\t\tLOG(HAL, Error) << \"Request \" << request->cookie()\n> > +\t\t\t\t<< \" not successfully completed: \"\n> > \t\t\t\t<< request->status();\n> > -\t\tstatus = CAMERA3_BUFFER_STATUS_ERROR;\n> > +\n> > +\t\tnotifyError(descriptor.frameNumber_,\n> > +\t\t\t descriptor.buffers_[0].stream,\n>\n> Shouldn't you pass nullptr for the stream pointer in this case ?\n>\n> > +\t\t\t CAMERA3_MSG_ERROR_REQUEST);\n> > +\n> > +\t\tcaptureResult.partial_result = 0;\n> > +\t\tfor (camera3_stream_buffer_t &buffer : descriptor.buffers_)\n> > +\t\t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> > +\t\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n> > +\n> > +\t\treturn;\n> > \t}\n> >\n> > +\t/*\n> > +\t * Notify shutter as soon as we have verified we have a valid request.\n> > +\t *\n> > +\t * \\todo The shutter event notification should be sent to the framework\n> > +\t * as soon as possible, earlier than request completion time.\n> > +\t */\n> > +\tuint64_t sensorTimestamp = static_cast(request->metadata()\n> > +\t\t\t\t\t\t\t .get(controls::SensorTimestamp));\n> > +\tnotifyShutter(descriptor.frameNumber_, sensorTimestamp);\n> > +\n> > \tLOG(HAL, Debug) << \"Request \" << request->cookie() << \" completed with \"\n> > \t\t\t<< descriptor.buffers_.size() << \" streams\";\n> >\n> > -\tresultMetadata = getResultMetadata(descriptor);\n> > +\t/*\n> > +\t * Generate the metadata associated with the captured buffers.\n> > +\t *\n> > +\t * Notify if the metadata generation has failed, but continue processing\n> > +\t * buffers and return an empty metadata pack.\n> > +\t */\n> > +\tstd::unique_ptr resultMetadata = getResultMetadata(descriptor);\n> > +\tif (!resultMetadata) {\n> > +\t\tnotifyError(descriptor.frameNumber_, descriptor.buffers_[0].stream,\n>\n> Same here, nullptr for the stream ?\n>\n> > +\t\t\t CAMERA3_MSG_ERROR_RESULT);\n> > +\n> > +\t\t/* The camera framework expects an empy metadata pack on error. */\n> > +\t\tresultMetadata = std::make_unique(0, 0);\n> > +\t}\n> > +\tcaptureResult.result = resultMetadata->get();\n> >\n> > \t/* Handle any JPEG compression. */\n> > \tfor (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> > @@ -1996,56 +2055,27 @@ void CameraDevice::requestComplete(Request *request)\n> > \t\tFrameBuffer *src = request->findBuffer(cameraStream->stream());\n> > \t\tif (!src) {\n> > \t\t\tLOG(HAL, Error) << \"Failed to find a source stream buffer\";\n> > +\t\t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> > +\t\t\tnotifyError(descriptor.frameNumber_, buffer.stream,\n> > +\t\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n> > \t\t\tcontinue;\n> > \t\t}\n> >\n> > -\t\tint ret = cameraStream->process(*src,\n> > -\t\t\t\t\t\t*buffer.buffer,\n> > +\t\tint ret = cameraStream->process(*src, *buffer.buffer,\n> > \t\t\t\t\t\tdescriptor.settings_,\n> > \t\t\t\t\t\tresultMetadata.get());\n> > -\t\tif (ret) {\n> > -\t\t\tstatus = CAMERA3_BUFFER_STATUS_ERROR;\n> > -\t\t\tcontinue;\n> > -\t\t}\n> > -\n> > \t\t/*\n> > \t\t * Return the FrameBuffer to the CameraStream now that we're\n> > \t\t * done processing it.\n> > \t\t */\n> > \t\tif (cameraStream->type() == CameraStream::Type::Internal)\n> > \t\t\tcameraStream->putBuffer(src);\n> > -\t}\n> >\n> > -\t/* Prepare to call back the Android camera stack. */\n> > -\tcamera3_capture_result_t captureResult = {};\n> > -\tcaptureResult.frame_number = descriptor.frameNumber_;\n> > -\tcaptureResult.num_output_buffers = descriptor.buffers_.size();\n> > -\tfor (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> > -\t\tbuffer.acquire_fence = -1;\n> > -\t\tbuffer.release_fence = -1;\n> > -\t\tbuffer.status = status;\n> > -\t}\n> > -\tcaptureResult.output_buffers = descriptor.buffers_.data();\n> > -\n> > -\tif (status == CAMERA3_BUFFER_STATUS_OK) {\n> > -\t\tuint64_t timestamp =\n> > -\t\t\tstatic_cast(request->metadata()\n> > -\t\t\t\t\t .get(controls::SensorTimestamp));\n> > -\t\tnotifyShutter(descriptor.frameNumber_, timestamp);\n> > -\n> > -\t\tcaptureResult.partial_result = 1;\n> > -\t\tcaptureResult.result = resultMetadata->get();\n> > -\t}\n> > -\n> > -\tif (status == CAMERA3_BUFFER_STATUS_ERROR || !captureResult.result) {\n> > -\t\t/* \\todo Improve error handling. In case we notify an error\n> > -\t\t * because the metadata generation fails, a shutter event has\n> > -\t\t * already been notified for this frame number before the error\n> > -\t\t * is here signalled. Make sure the error path plays well with\n> > -\t\t * the camera stack state machine.\n> > -\t\t */\n> > -\t\tnotifyError(descriptor.frameNumber_,\n> > -\t\t\t descriptor.buffers_[0].stream);\n> > +\t\tif (ret) {\n> > +\t\t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> > +\t\t\tnotifyError(descriptor.frameNumber_, buffer.stream,\n> > +\t\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n> > +\t\t}\n> > \t}\n> >\n> > \tcallbacks_->process_capture_result(callbacks_, &captureResult);\n> > @@ -2067,23 +2097,23 @@ void CameraDevice::notifyShutter(uint32_t frameNumber, uint64_t timestamp)\n> > \tcallbacks_->notify(callbacks_, ¬ify);\n> > }\n> >\n> > -void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream)\n> > +void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream,\n> > +\t\t\t camera3_error_msg_code code)\n> > {\n> > \tcamera3_notify_msg_t notify = {};\n> >\n> > -\t/*\n> > -\t * \\todo Report and identify the stream number or configuration to\n> > -\t * clarify the stream that failed.\n> > -\t */\n> > -\tLOG(HAL, Error) << \"Error occurred on frame \" << frameNumber << \" (\"\n> > -\t\t\t<< toPixelFormat(stream->format).toString() << \")\";\n> > -\n> > \tnotify.type = CAMERA3_MSG_ERROR;\n> > \tnotify.message.error.error_stream = stream;\n> > \tnotify.message.error.frame_number = frameNumber;\n> > -\tnotify.message.error.error_code = CAMERA3_MSG_ERROR_REQUEST;\n> > +\tnotify.message.error.error_code = code;\n> >\n> > \tcallbacks_->notify(callbacks_, ¬ify);\n> > +\n> > +\tif (stream)\n> > +\t\tLOG(HAL, Error) << \"Error occurred on frame \" << frameNumber << \" (\"\n> > +\t\t\t\t<< toPixelFormat(stream->format).toString() << \")\";\n> > +\telse\n> > +\t\tLOG(HAL, Error) << \"Fatal error occurred on device\";\n>\n> I'd keep this at the top of the function. Otherwise, notifying the\n> camera server first, we could get a call from the camera service\n> synchronously that would result in other messages being logged before\n> our error message. For instance, we could have\n>\n> \tClosing device\n> \tFatal error occurred on device\n>\n> The other way around would be better.\n>\n> Very nice refactoring overall.\n>\n> Reviewed-by: Laurent Pinchart \n>\n> > }\n> >\n> > /*\n> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > index ae162a452499..a34e8a2cd900 100644\n> > --- a/src/android/camera_device.h\n> > +++ b/src/android/camera_device.h\n> > @@ -100,7 +100,8 @@ private:\n> >\n> > \tlibcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer);\n> > \tvoid notifyShutter(uint32_t frameNumber, uint64_t timestamp);\n> > -\tvoid notifyError(uint32_t frameNumber, camera3_stream_t *stream);\n> > +\tvoid notifyError(uint32_t frameNumber, camera3_stream_t *stream,\n> > +\t\t\t camera3_error_msg_code code);\n> > \tstd::unique_ptr requestTemplatePreview();\n> > \tstd::unique_ptr requestTemplateVideo();\n> > \tlibcamera::PixelFormat toPixelFormat(int format) const;\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 76155C3203\n\tfor ;\n\tThu, 27 May 2021 08:19:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BA05268925;\n\tThu, 27 May 2021 10:19:57 +0200 (CEST)","from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2CF89602AA\n\tfor ;\n\tThu, 27 May 2021 10:19:56 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay4-d.mail.gandi.net (Postfix) with ESMTPSA id 118A7E0004;\n\tThu, 27 May 2021 08:19:54 +0000 (UTC)"],"Date":"Thu, 27 May 2021 10:20:41 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210527082041.pkmx5dtvbwj6kj64@uno.localdomain>","References":"<20210521154227.60186-1-jacopo@jmondi.org>\n\t<20210521154227.60186-2-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 1/8] android: Rework request\n\tcompletion notification","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]