[{"id":16819,"web_url":"https://patchwork.libcamera.org/comment/16819/","msgid":"","date":"2021-05-07T03:50:13","subject":"Re: [libcamera-devel] [PATCH v2] android: Rework request completion\n\tnotification","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Jacopo, thank you for the patch.\n\nOn Fri, May 7, 2021 at 12:00 AM Jacopo Mondi wrote:\n\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 close the camera in case a Fatal\n> error is detected (Fatal does not perform library teardown in\n> production builds)\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> ---\n>\n> v2:\n> - Rework a comment as suggested by Niklas\n> - As Niklas reported, dereferencing a unique_ptr<> which owns a nullptr has\n> an undefined behaviour. Replace that pattern by resetting the\n> resultMetadata\n> unique_ptr<> with a new one holding an empty CameraMetadata, as the\n> framework\n> requires on error.\n>\n> camera3.h:\n> * If there was an error producing the result metadata, result must be\n> an\n> * empty metadata buffer, and notify() must be called with\n> ERROR_RESULT.\n>\n>\n> std::unique_ptr resultMetadata =\n> getResultMetadata(descriptor);\n> if (!resultMetadata) {\n> notifyError(descriptor.frameNumber_,\n> descriptor.buffers_[0].stream,\n> CAMERA3_MSG_ERROR_RESULT);\n>\n> /* The camera framework expects an empy metadata pack on\n> error. */\n> resultMetadata = std::make_unique(0, 0);\n> }\n> captureResult.result = resultMetadata->get();\n>\n> ---\n> src/android/camera_device.cpp | 132 ++++++++++++++++++++--------------\n> src/android/camera_device.h | 3 +-\n> 2 files changed, 80 insertions(+), 55 deletions(-)\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index f1f26c775611..ea7ec8c05f88 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -2025,9 +2025,6 @@ int\n> CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>\n> void CameraDevice::requestComplete(Request *request)\n> {\n> - camera3_buffer_status status = CAMERA3_BUFFER_STATUS_OK;\n> - std::unique_ptr resultMetadata;\n> -\n> decltype(descriptors_)::node_type node;\n> {\n> std::scoped_lock lock(mutex_);\n> @@ -2035,7 +2032,8 @@ void CameraDevice::requestComplete(Request *request)\n> if (it == descriptors_.end()) {\n> LOG(HAL, Fatal)\n> << \"Unknown request: \" <<\n> request->cookie();\n> - status = CAMERA3_BUFFER_STATUS_ERROR;\n> + close();\n>\n\nclose() calls Camera::stop(). Camera::stop() cancels the pending request\nand requestComplete() will be invoked. (this is not done so cf.\nhttps://patchwork.libcamera.org/project/libcamera/list/?series=1954)\nFurthermore, Camera::stop() is a blocking function. So I think calling\nclose() here possibly causes the infinite-loop, e.g. when descriptors_ is\nsomehow empty.\nAh, but LOG(HAL, Fatal) causes the crash libcamera process, hmm..\n\n\n\n> + notifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);\n> return;\n> }\n>\n> @@ -2043,16 +2041,71 @@ void CameraDevice::requestComplete(Request\n> *request)\n> }\n> Camera3RequestDescriptor &descriptor = node.mapped();\n>\n> + /*\n> + * Prepare the capture result for the Android camera stack.\n> + *\n> + * The buffer status is set to OK and later changed to ERROR if\n> + * post-processing/compression fails.\n> + */\n> + camera3_capture_result_t captureResult = {};\n> + captureResult.frame_number = descriptor.frameNumber_;\n> + captureResult.num_output_buffers = descriptor.buffers_.size();\n> + for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> + buffer.acquire_fence = -1;\n> + buffer.release_fence = -1;\n> + buffer.status = CAMERA3_BUFFER_STATUS_OK;\n> + }\n> + captureResult.output_buffers = descriptor.buffers_.data();\n> + captureResult.partial_result = 1;\n> +\n> + /*\n> + * If the Request has failed, abort the request by notifying the\n> error\n> + * and complete the request with all buffers in error state.\n> + */\n> if (request->status() != Request::RequestComplete) {\n> - LOG(HAL, Error) << \"Request not successfully completed: \"\n> + LOG(HAL, Error) << \"Request \" << request->cookie()\n> + << \" not successfully completed: \"\n> << request->status();\n> - status = CAMERA3_BUFFER_STATUS_ERROR;\n> +\n> + notifyError(descriptor.frameNumber_,\n> + descriptor.buffers_[0].stream,\n> + CAMERA3_MSG_ERROR_REQUEST);\n> +\n> + for (camera3_stream_buffer_t &buffer : descriptor.buffers_)\n> + buffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> + callbacks_->process_capture_result(callbacks_,\n> &captureResult);\n> +\n> + return;\n> }\n>\n> + /*\n> + * Notify shutter as soon as we have verified we have a valid\n> request.\n> + *\n> + * \\todo The shutter event notification should be sent to the\n> framework\n> + * as soon as possible, earlier than request completion time.\n> + */\n> + uint64_t sensorTimestamp =\n> static_cast(request->metadata()\n> +\n> .get(controls::SensorTimestamp));\n> + notifyShutter(descriptor.frameNumber_, sensorTimestamp);\n> +\n> LOG(HAL, Debug) << \"Request \" << request->cookie() << \" completed\n> with \"\n> << descriptor.buffers_.size() << \" streams\";\n>\n> - resultMetadata = getResultMetadata(descriptor);\n> + /*\n> + * Generate the metadata associated with the captured buffers.\n> + *\n> + * Notify if the metadata generation has failed, but continue\n> processing\n> + * buffers and return an empty metadata pack.\n> + */\n> + std::unique_ptr resultMetadata =\n> getResultMetadata(descriptor);\n> + if (!resultMetadata) {\n> + notifyError(descriptor.frameNumber_,\n> descriptor.buffers_[0].stream,\n> + CAMERA3_MSG_ERROR_RESULT);\n> +\n> + /* The camera framework expects an empy metadata pack on\n> error. */\n> + resultMetadata = std::make_unique(0, 0);\n> + }\n> + captureResult.result = resultMetadata->get();\n>\n> /* Handle any JPEG compression. */\n> for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> @@ -2065,56 +2118,27 @@ void CameraDevice::requestComplete(Request\n> *request)\n> FrameBuffer *src =\n> request->findBuffer(cameraStream->stream());\n> if (!src) {\n> LOG(HAL, Error) << \"Failed to find a source stream\n> buffer\";\n> + buffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> + notifyError(descriptor.frameNumber_, buffer.stream,\n> + CAMERA3_MSG_ERROR_BUFFER);\n> continue;\n>\n\nIs this still continue? Perhaps return?\n\n\n> }\n>\n> - int ret = cameraStream->process(*src,\n> - *buffer.buffer,\n> + int ret = cameraStream->process(*src, *buffer.buffer,\n> descriptor.settings_,\n> resultMetadata.get());\n> - if (ret) {\n> - status = CAMERA3_BUFFER_STATUS_ERROR;\n> - continue;\n> - }\n> -\n> /*\n> * Return the FrameBuffer to the CameraStream now that\n> we're\n> * done processing it.\n> */\n> if (cameraStream->type() == CameraStream::Type::Internal)\n> cameraStream->putBuffer(src);\n> - }\n>\n> - /* Prepare to call back the Android camera stack. */\n> - camera3_capture_result_t captureResult = {};\n> - captureResult.frame_number = descriptor.frameNumber_;\n> - captureResult.num_output_buffers = descriptor.buffers_.size();\n> - for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> - buffer.acquire_fence = -1;\n> - buffer.release_fence = -1;\n> - buffer.status = status;\n> - }\n> - captureResult.output_buffers = descriptor.buffers_.data();\n> -\n> - if (status == CAMERA3_BUFFER_STATUS_OK) {\n> - uint64_t timestamp =\n> - static_cast(request->metadata()\n> -\n> .get(controls::SensorTimestamp));\n> - notifyShutter(descriptor.frameNumber_, timestamp);\n> -\n> - captureResult.partial_result = 1;\n> - captureResult.result = resultMetadata->get();\n> - }\n> -\n> - if (status == CAMERA3_BUFFER_STATUS_ERROR ||\n> !captureResult.result) {\n> - /* \\todo Improve error handling. In case we notify an error\n> - * because the metadata generation fails, a shutter event\n> has\n> - * already been notified for this frame number before the\n> error\n> - * is here signalled. Make sure the error path plays well\n> with\n> - * the camera stack state machine.\n> - */\n> - notifyError(descriptor.frameNumber_,\n> - descriptor.buffers_[0].stream);\n> + if (ret) {\n> + buffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> + notifyError(descriptor.frameNumber_, buffer.stream,\n> + CAMERA3_MSG_ERROR_BUFFER);\n>\n\nsame here.\n\n-Hiro\n\n+ }\n> }\n>\n> callbacks_->process_capture_result(callbacks_, &captureResult);\n> @@ -2136,23 +2160,23 @@ void CameraDevice::notifyShutter(uint32_t\n> frameNumber, uint64_t timestamp)\n> callbacks_->notify(callbacks_, ¬ify);\n> }\n>\n> -void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t\n> *stream)\n> +void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t\n> *stream,\n> + camera3_error_msg_code code)\n> {\n> camera3_notify_msg_t notify = {};\n>\n> - /*\n> - * \\todo Report and identify the stream number or configuration to\n> - * clarify the stream that failed.\n> - */\n> - LOG(HAL, Error) << \"Error occurred on frame \" << frameNumber << \"\n> (\"\n> - << toPixelFormat(stream->format).toString() << \")\";\n> -\n> notify.type = CAMERA3_MSG_ERROR;\n> notify.message.error.error_stream = stream;\n> notify.message.error.frame_number = frameNumber;\n> - notify.message.error.error_code = CAMERA3_MSG_ERROR_REQUEST;\n> + notify.message.error.error_code = code;\n>\n> callbacks_->notify(callbacks_, ¬ify);\n> +\n> + if (stream)\n> + LOG(HAL, Error) << \"Error occurred on frame \" <<\n> frameNumber << \" (\"\n> + <<\n> toPixelFormat(stream->format).toString() << \")\";\n> + else\n> + LOG(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 23457e47767a..8d5da8bc59e1 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -101,7 +101,8 @@ private:\n> std::tuple calculateStaticMetadataSize();\n> libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t\n> camera3buffer);\n> void notifyShutter(uint32_t frameNumber, uint64_t timestamp);\n> - void notifyError(uint32_t frameNumber, camera3_stream_t *stream);\n> + void notifyError(uint32_t frameNumber, camera3_stream_t *stream,\n> + camera3_error_msg_code code);\n> std::unique_ptr requestTemplatePreview();\n> std::unique_ptr requestTemplateVideo();\n> libcamera::PixelFormat toPixelFormat(int format) const;\n> --\n> 2.31.1\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel\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 7C4B1BF829\n\tfor ;\n\tFri, 7 May 2021 03:50:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DC4356891A;\n\tFri, 7 May 2021 05:50:25 +0200 (CEST)","from mail-ej1-x62d.google.com (mail-ej1-x62d.google.com\n\t[IPv6:2a00:1450:4864:20::62d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CC16B688E4\n\tfor ;\n\tFri, 7 May 2021 05:50:24 +0200 (CEST)","by mail-ej1-x62d.google.com with SMTP id n2so11419679ejy.7\n\tfor ;\n\tThu, 06 May 2021 20:50:24 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"FuVZ2wJR\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=OaB9v6Tp0jfzYQL4AsWndiZq3seSiqDOdEIyeyUBtWs=;\n\tb=FuVZ2wJRb3ALgPV2BMEGU5jPxQSqfZNXjFcLr/AUzgSyq+nPMVOptI8rAFVVUjC1s8\n\t1s4Xx4cgo8s2hxB+z1fOZVY63tUYyPKZtrMgQD7VZ2mV64jms0X6BSykyLpkgV4lKJlO\n\t43G9q77/TlDs29g9et1vVbD8EDbKhzN7odIX0=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=OaB9v6Tp0jfzYQL4AsWndiZq3seSiqDOdEIyeyUBtWs=;\n\tb=VFElo2yE1+b4X/td4Rp1lhaxE+MY8ZKHknyKrjy+JJKYsjvZnSQWIyt9nf7grs2Kb5\n\t/1ZuP+4AdE46P5CAwX/2AmC7z18ixmFziUFuY+MwaH3Ja4Lqm48XEkIlPM8S19kdqeb5\n\tDFjxp6pF6g2CtiVAa6H2gyGVMZS3pJkyQsxZEkJZVrflUMVNPJjwKJ5k0/2ehgElsYe9\n\tJHMVJor1iIwDoTg+h4nB54BoKTk0vRt9/NOY9Lp1gwn7ibgeeehVDE4Ap/B+06buclvs\n\tjmhoBZA4V1OzztTDamr5W0VeImOXCzx2n9q542aa5yDRKoZBSLsOU49EkBaiKtGfP92A\n\ts1xw==","X-Gm-Message-State":"AOAM531GPdu/rTMv0pRg5b/1o3dkhJMUXL1fG4YxNUAunjgeaAvuJsTW\n\twZCjN68Xr3jh1emNvwZW9Mkz6nvwOWh8+MTSQKXgXA==","X-Google-Smtp-Source":"ABdhPJyfZnnbkmtwvsSbmsis/r45cNOhNr3AWIQXudNvwy9/JGB/X2cNuBZCvJ+a0YPqjFrlyevzYSu8CSXUsoIzd/E=","X-Received":"by 2002:a17:906:5acd:: with SMTP id\n\tx13mr7504350ejs.243.1620359424374; \n\tThu, 06 May 2021 20:50:24 -0700 (PDT)","MIME-Version":"1.0","References":"<20210506150106.28470-1-jacopo@jmondi.org>","In-Reply-To":"<20210506150106.28470-1-jacopo@jmondi.org>","From":"Hirokazu Honda ","Date":"Fri, 7 May 2021 12:50:13 +0900","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH v2] android: Rework request completion\n\tnotification","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 ","Content-Type":"multipart/mixed;\n\tboundary=\"===============0235830833234805608==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16820,"web_url":"https://patchwork.libcamera.org/comment/16820/","msgid":"<20210507073739.n27h23o6kpdflkdf@uno.localdomain>","date":"2021-05-07T07:37:39","subject":"Re: [libcamera-devel] [PATCH v2] android: Rework request completion\n\tnotification","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro,\n thanks for review\n\nOn Fri, May 07, 2021 at 12:50:13PM +0900, Hirokazu Honda wrote:\n> Hi Jacopo, thank you for the patch.\n>\n> On Fri, May 7, 2021 at 12:00 AM Jacopo Mondi wrote:\n>\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 close the camera in case a Fatal\n> > error is detected (Fatal does not perform library teardown in\n> > production builds)\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> > ---\n> >\n> > v2:\n> > - Rework a comment as suggested by Niklas\n> > - As Niklas reported, dereferencing a unique_ptr<> which owns a nullptr has\n> > an undefined behaviour. Replace that pattern by resetting the\n> > resultMetadata\n> > unique_ptr<> with a new one holding an empty CameraMetadata, as the\n> > framework\n> > requires on error.\n> >\n> > camera3.h:\n> > * If there was an error producing the result metadata, result must be\n> > an\n> > * empty metadata buffer, and notify() must be called with\n> > ERROR_RESULT.\n> >\n> >\n> > std::unique_ptr resultMetadata =\n> > getResultMetadata(descriptor);\n> > if (!resultMetadata) {\n> > notifyError(descriptor.frameNumber_,\n> > descriptor.buffers_[0].stream,\n> > CAMERA3_MSG_ERROR_RESULT);\n> >\n> > /* The camera framework expects an empy metadata pack on\n> > error. */\n> > resultMetadata = std::make_unique(0, 0);\n> > }\n> > captureResult.result = resultMetadata->get();\n> >\n> > ---\n> > src/android/camera_device.cpp | 132 ++++++++++++++++++++--------------\n> > src/android/camera_device.h | 3 +-\n> > 2 files changed, 80 insertions(+), 55 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index f1f26c775611..ea7ec8c05f88 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -2025,9 +2025,6 @@ int\n> > CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> >\n> > void CameraDevice::requestComplete(Request *request)\n> > {\n> > - camera3_buffer_status status = CAMERA3_BUFFER_STATUS_OK;\n> > - std::unique_ptr resultMetadata;\n> > -\n> > decltype(descriptors_)::node_type node;\n> > {\n> > std::scoped_lock lock(mutex_);\n> > @@ -2035,7 +2032,8 @@ void CameraDevice::requestComplete(Request *request)\n> > if (it == descriptors_.end()) {\n> > LOG(HAL, Fatal)\n> > << \"Unknown request: \" <<\n> > request->cookie();\n> > - status = CAMERA3_BUFFER_STATUS_ERROR;\n> > + close();\n> >\n>\n> close() calls Camera::stop(). Camera::stop() cancels the pending request\n> and requestComplete() will be invoked. (this is not done so cf.\n> https://patchwork.libcamera.org/project/libcamera/list/?series=1954)\n> Furthermore, Camera::stop() is a blocking function. So I think calling\n> close() here possibly causes the infinite-loop, e.g. when descriptors_ is\n> somehow empty.\n\nMmm, you're right, it doesn't seem like an infinite loop but rather a\ndeadlock, as we're in critical section and all new requestComplete()\ncalls due to the Camera::stop() will contend this mutex\n\n> Ah, but LOG(HAL, Fatal) causes the crash libcamera process, hmm..\n\nI recall we had a compiler option to make Fatal not fatal (sorry...)\nin production builds... I don't seem to be able to find it though.\n\nIf Fatal is always fatal, well, I can only notify the ERROR_DEVICE and\nthen crash, so no close is required.\n\nIf Fatal can actually be disabled I feel like close() will need to be\ncalled according to the camera3 API, but I'm not sure if the framework\ndoes that, or the HAL should do so, as I've found two contradictory\nstatements:\n\n\n /**\n * A serious failure occured. No further frames or buffer streams will\n * be produced by the device. Device should be treated as closed. The\n * client must reopen the device to use it again. The frame_number field\n * is unused.\n */\n CAMERA3_MSG_ERROR_DEVICE = 1,\n\nThis seems to imply the device should be re-opened and treated as\nclosed by the framwork, so the HAL has actually to close it.\n\n * S6. Error management:\n *\n * Camera HAL device ops functions that have a return value will all return\n * -ENODEV / NULL in case of a serious error. This means the device cannot\n * continue operation, and must be closed by the framework. Once this error is\n * returned by some method, or if notify() is called with ERROR_DEVICE, only\n * the close() method can be called successfully. All other methods will return\n * -ENODEV / NULL.\n\nThis seems to imply the framework calls close() after it receives\nERROR_DEVICE.\n\nCould this maybe clarified with Ricky's team ?\n\n>\n>\n>\n> > + notifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);\n> > return;\n> > }\n> >\n> > @@ -2043,16 +2041,71 @@ void CameraDevice::requestComplete(Request\n> > *request)\n> > }\n> > Camera3RequestDescriptor &descriptor = node.mapped();\n> >\n> > + /*\n> > + * Prepare the capture result for the Android camera stack.\n> > + *\n> > + * The buffer status is set to OK and later changed to ERROR if\n> > + * post-processing/compression fails.\n> > + */\n> > + camera3_capture_result_t captureResult = {};\n> > + captureResult.frame_number = descriptor.frameNumber_;\n> > + captureResult.num_output_buffers = descriptor.buffers_.size();\n> > + for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> > + buffer.acquire_fence = -1;\n> > + buffer.release_fence = -1;\n> > + buffer.status = CAMERA3_BUFFER_STATUS_OK;\n> > + }\n> > + captureResult.output_buffers = descriptor.buffers_.data();\n> > + captureResult.partial_result = 1;\n> > +\n> > + /*\n> > + * If the Request has failed, abort the request by notifying the\n> > error\n> > + * and complete the request with all buffers in error state.\n> > + */\n> > if (request->status() != Request::RequestComplete) {\n> > - LOG(HAL, Error) << \"Request not successfully completed: \"\n> > + LOG(HAL, Error) << \"Request \" << request->cookie()\n> > + << \" not successfully completed: \"\n> > << request->status();\n> > - status = CAMERA3_BUFFER_STATUS_ERROR;\n> > +\n> > + notifyError(descriptor.frameNumber_,\n> > + descriptor.buffers_[0].stream,\n> > + CAMERA3_MSG_ERROR_REQUEST);\n> > +\n> > + for (camera3_stream_buffer_t &buffer : descriptor.buffers_)\n> > + buffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> > + callbacks_->process_capture_result(callbacks_,\n> > &captureResult);\n> > +\n> > + return;\n> > }\n> >\n> > + /*\n> > + * Notify shutter as soon as we have verified we have a valid\n> > request.\n> > + *\n> > + * \\todo The shutter event notification should be sent to the\n> > framework\n> > + * as soon as possible, earlier than request completion time.\n> > + */\n> > + uint64_t sensorTimestamp =\n> > static_cast(request->metadata()\n> > +\n> > .get(controls::SensorTimestamp));\n> > + notifyShutter(descriptor.frameNumber_, sensorTimestamp);\n> > +\n> > LOG(HAL, Debug) << \"Request \" << request->cookie() << \" completed\n> > with \"\n> > << descriptor.buffers_.size() << \" streams\";\n> >\n> > - resultMetadata = getResultMetadata(descriptor);\n> > + /*\n> > + * Generate the metadata associated with the captured buffers.\n> > + *\n> > + * Notify if the metadata generation has failed, but continue\n> > processing\n> > + * buffers and return an empty metadata pack.\n> > + */\n> > + std::unique_ptr resultMetadata =\n> > getResultMetadata(descriptor);\n> > + if (!resultMetadata) {\n> > + notifyError(descriptor.frameNumber_,\n> > descriptor.buffers_[0].stream,\n> > + CAMERA3_MSG_ERROR_RESULT);\n> > +\n> > + /* The camera framework expects an empy metadata pack on\n> > error. */\n> > + resultMetadata = std::make_unique(0, 0);\n> > + }\n> > + captureResult.result = resultMetadata->get();\n> >\n> > /* Handle any JPEG compression. */\n> > for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> > @@ -2065,56 +2118,27 @@ void CameraDevice::requestComplete(Request\n> > *request)\n> > FrameBuffer *src =\n> > request->findBuffer(cameraStream->stream());\n> > if (!src) {\n> > LOG(HAL, Error) << \"Failed to find a source stream\n> > buffer\";\n> > + buffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> > + notifyError(descriptor.frameNumber_, buffer.stream,\n> > + CAMERA3_MSG_ERROR_BUFFER);\n> > continue;\n> >\n>\n> Is this still continue? Perhaps return?\n\nIs it ? my understandin is that if a single buffer fails, the request\ncan be completed and that single buffer state will be set to\nCAMERA3_BUFFER_STATUS_ERROR and the framework has to be notified about\nthe CAMERA3_MSG_ERROR_BUFFER event. Metadata and other succesfully\ncompleted buffers can be returned correctly if they have been properly\nfilled, hence I think we should continue and process the next\nbuffer...\n\nA question: are there tests in the cros_camera_test package that\nexercize these error paths ?\n\nThanks\n j\n\n>\n>\n> > }\n> >\n> > - int ret = cameraStream->process(*src,\n> > - *buffer.buffer,\n> > + int ret = cameraStream->process(*src, *buffer.buffer,\n> > descriptor.settings_,\n> > resultMetadata.get());\n> > - if (ret) {\n> > - status = CAMERA3_BUFFER_STATUS_ERROR;\n> > - continue;\n> > - }\n> > -\n> > /*\n> > * Return the FrameBuffer to the CameraStream now that\n> > we're\n> > * done processing it.\n> > */\n> > if (cameraStream->type() == CameraStream::Type::Internal)\n> > cameraStream->putBuffer(src);\n> > - }\n> >\n> > - /* Prepare to call back the Android camera stack. */\n> > - camera3_capture_result_t captureResult = {};\n> > - captureResult.frame_number = descriptor.frameNumber_;\n> > - captureResult.num_output_buffers = descriptor.buffers_.size();\n> > - for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> > - buffer.acquire_fence = -1;\n> > - buffer.release_fence = -1;\n> > - buffer.status = status;\n> > - }\n> > - captureResult.output_buffers = descriptor.buffers_.data();\n> > -\n> > - if (status == CAMERA3_BUFFER_STATUS_OK) {\n> > - uint64_t timestamp =\n> > - static_cast(request->metadata()\n> > -\n> > .get(controls::SensorTimestamp));\n> > - notifyShutter(descriptor.frameNumber_, timestamp);\n> > -\n> > - captureResult.partial_result = 1;\n> > - captureResult.result = resultMetadata->get();\n> > - }\n> > -\n> > - if (status == CAMERA3_BUFFER_STATUS_ERROR ||\n> > !captureResult.result) {\n> > - /* \\todo Improve error handling. In case we notify an error\n> > - * because the metadata generation fails, a shutter event\n> > has\n> > - * already been notified for this frame number before the\n> > error\n> > - * is here signalled. Make sure the error path plays well\n> > with\n> > - * the camera stack state machine.\n> > - */\n> > - notifyError(descriptor.frameNumber_,\n> > - descriptor.buffers_[0].stream);\n> > + if (ret) {\n> > + buffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> > + notifyError(descriptor.frameNumber_, buffer.stream,\n> > + CAMERA3_MSG_ERROR_BUFFER);\n> >\n>\n> same here.\n>\n> -Hiro\n>\n> + }\n> > }\n> >\n> > callbacks_->process_capture_result(callbacks_, &captureResult);\n> > @@ -2136,23 +2160,23 @@ void CameraDevice::notifyShutter(uint32_t\n> > frameNumber, uint64_t timestamp)\n> > callbacks_->notify(callbacks_, ¬ify);\n> > }\n> >\n> > -void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t\n> > *stream)\n> > +void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t\n> > *stream,\n> > + camera3_error_msg_code code)\n> > {\n> > camera3_notify_msg_t notify = {};\n> >\n> > - /*\n> > - * \\todo Report and identify the stream number or configuration to\n> > - * clarify the stream that failed.\n> > - */\n> > - LOG(HAL, Error) << \"Error occurred on frame \" << frameNumber << \"\n> > (\"\n> > - << toPixelFormat(stream->format).toString() << \")\";\n> > -\n> > notify.type = CAMERA3_MSG_ERROR;\n> > notify.message.error.error_stream = stream;\n> > notify.message.error.frame_number = frameNumber;\n> > - notify.message.error.error_code = CAMERA3_MSG_ERROR_REQUEST;\n> > + notify.message.error.error_code = code;\n> >\n> > callbacks_->notify(callbacks_, ¬ify);\n> > +\n> > + if (stream)\n> > + LOG(HAL, Error) << \"Error occurred on frame \" <<\n> > frameNumber << \" (\"\n> > + <<\n> > toPixelFormat(stream->format).toString() << \")\";\n> > + else\n> > + LOG(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 23457e47767a..8d5da8bc59e1 100644\n> > --- a/src/android/camera_device.h\n> > +++ b/src/android/camera_device.h\n> > @@ -101,7 +101,8 @@ private:\n> > std::tuple calculateStaticMetadataSize();\n> > libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t\n> > camera3buffer);\n> > void notifyShutter(uint32_t frameNumber, uint64_t timestamp);\n> > - void notifyError(uint32_t frameNumber, camera3_stream_t *stream);\n> > + void notifyError(uint32_t frameNumber, camera3_stream_t *stream,\n> > + camera3_error_msg_code code);\n> > std::unique_ptr requestTemplatePreview();\n> > std::unique_ptr requestTemplateVideo();\n> > libcamera::PixelFormat toPixelFormat(int format) const;\n> > --\n> > 2.31.1\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\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 4E467BF831\n\tfor ;\n\tFri, 7 May 2021 07:36:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C324668919;\n\tFri, 7 May 2021 09:36:56 +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 0F376602BB\n\tfor ;\n\tFri, 7 May 2021 09:36:56 +0200 (CEST)","from uno.localdomain (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay4-d.mail.gandi.net (Postfix) with ESMTPSA id 720E5E0012;\n\tFri, 7 May 2021 07:36:55 +0000 (UTC)"],"X-Originating-IP":"93.61.96.190","Date":"Fri, 7 May 2021 09:37:39 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20210507073739.n27h23o6kpdflkdf@uno.localdomain>","References":"<20210506150106.28470-1-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2] android: Rework request completion\n\tnotification","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16832,"web_url":"https://patchwork.libcamera.org/comment/16832/","msgid":"","date":"2021-05-07T12:27:32","subject":"Re: [libcamera-devel] [PATCH v2] android: Rework request completion\n\tnotification","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, May 07, 2021 at 09:37:39AM +0200, Jacopo Mondi wrote:\n> On Fri, May 07, 2021 at 12:50:13PM +0900, Hirokazu Honda wrote:\n> > On Fri, May 7, 2021 at 12:00 AM Jacopo Mondi wrote:\n> >\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 close the camera in case a Fatal\n> > > error is detected (Fatal does not perform library teardown in\n> > > production builds)\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> > > ---\n> > >\n> > > v2:\n> > > - Rework a comment as suggested by Niklas\n> > > - As Niklas reported, dereferencing a unique_ptr<> which owns a nullptr has\n> > > an undefined behaviour. Replace that pattern by resetting the\n> > > resultMetadata\n> > > unique_ptr<> with a new one holding an empty CameraMetadata, as the\n> > > framework\n> > > requires on error.\n> > >\n> > > camera3.h:\n> > > * If there was an error producing the result metadata, result must be\n> > > an\n> > > * empty metadata buffer, and notify() must be called with\n> > > ERROR_RESULT.\n> > >\n> > >\n> > > std::unique_ptr resultMetadata =\n> > > getResultMetadata(descriptor);\n> > > if (!resultMetadata) {\n> > > notifyError(descriptor.frameNumber_,\n> > > descriptor.buffers_[0].stream,\n> > > CAMERA3_MSG_ERROR_RESULT);\n> > >\n> > > /* The camera framework expects an empy metadata pack on\n> > > error. */\n> > > resultMetadata = std::make_unique(0, 0);\n> > > }\n> > > captureResult.result = resultMetadata->get();\n> > >\n> > > ---\n> > > src/android/camera_device.cpp | 132 ++++++++++++++++++++--------------\n> > > src/android/camera_device.h | 3 +-\n> > > 2 files changed, 80 insertions(+), 55 deletions(-)\n> > >\n> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > index f1f26c775611..ea7ec8c05f88 100644\n> > > --- a/src/android/camera_device.cpp\n> > > +++ b/src/android/camera_device.cpp\n> > > @@ -2025,9 +2025,6 @@ int\n> > > CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > >\n> > > void CameraDevice::requestComplete(Request *request)\n> > > {\n> > > - camera3_buffer_status status = CAMERA3_BUFFER_STATUS_OK;\n> > > - std::unique_ptr resultMetadata;\n> > > -\n> > > decltype(descriptors_)::node_type node;\n> > > {\n> > > std::scoped_lock lock(mutex_);\n> > > @@ -2035,7 +2032,8 @@ void CameraDevice::requestComplete(Request *request)\n> > > if (it == descriptors_.end()) {\n> > > LOG(HAL, Fatal)\n> > > << \"Unknown request: \" <<\n> > > request->cookie();\n> > > - status = CAMERA3_BUFFER_STATUS_ERROR;\n> > > + close();\n> > >\n> >\n> > close() calls Camera::stop(). Camera::stop() cancels the pending request\n> > and requestComplete() will be invoked. (this is not done so cf.\n> > https://patchwork.libcamera.org/project/libcamera/list/?series=1954)\n> > Furthermore, Camera::stop() is a blocking function. So I think calling\n> > close() here possibly causes the infinite-loop, e.g. when descriptors_ is\n> > somehow empty.\n> \n> Mmm, you're right, it doesn't seem like an infinite loop but rather a\n> deadlock, as we're in critical section and all new requestComplete()\n> calls due to the Camera::stop() will contend this mutex\n> \n> > Ah, but LOG(HAL, Fatal) causes the crash libcamera process, hmm..\n> \n> I recall we had a compiler option to make Fatal not fatal (sorry...)\n> in production builds... I don't seem to be able to find it though.\n> \n> If Fatal is always fatal, well, I can only notify the ERROR_DEVICE and\n> then crash, so no close is required.\n\nWe've been toying with the idea of disabling the assertion error for\nFatal in production builds, and I'm still tempted to do so, so it would\nbe best if errors could be handled gracefully.\n\n> If Fatal can actually be disabled I feel like close() will need to be\n> called according to the camera3 API, but I'm not sure if the framework\n> does that, or the HAL should do so, as I've found two contradictory\n> statements:\n> \n> \n> /**\n> * A serious failure occured. No further frames or buffer streams will\n> * be produced by the device. Device should be treated as closed. The\n> * client must reopen the device to use it again. The frame_number field\n> * is unused.\n> */\n> CAMERA3_MSG_ERROR_DEVICE = 1,\n> \n> This seems to imply the device should be re-opened and treated as\n> closed by the framwork, so the HAL has actually to close it.\n> \n> * S6. Error management:\n> *\n> * Camera HAL device ops functions that have a return value will all return\n> * -ENODEV / NULL in case of a serious error. This means the device cannot\n> * continue operation, and must be closed by the framework. Once this error is\n> * returned by some method, or if notify() is called with ERROR_DEVICE, only\n> * the close() method can be called successfully. All other methods will return\n> * -ENODEV / NULL.\n> \n> This seems to imply the framework calls close() after it receives\n> ERROR_DEVICE.\n> \n> Could this maybe clarified with Ricky's team ?\n> \n> > > + notifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);\n> > > return;\n> > > }\n> > >\n> > > @@ -2043,16 +2041,71 @@ void CameraDevice::requestComplete(Request\n> > > *request)\n> > > }\n> > > Camera3RequestDescriptor &descriptor = node.mapped();\n> > >\n> > > + /*\n> > > + * Prepare the capture result for the Android camera stack.\n> > > + *\n> > > + * The buffer status is set to OK and later changed to ERROR if\n> > > + * post-processing/compression fails.\n> > > + */\n> > > + camera3_capture_result_t captureResult = {};\n> > > + captureResult.frame_number = descriptor.frameNumber_;\n> > > + captureResult.num_output_buffers = descriptor.buffers_.size();\n> > > + for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> > > + buffer.acquire_fence = -1;\n> > > + buffer.release_fence = -1;\n> > > + buffer.status = CAMERA3_BUFFER_STATUS_OK;\n> > > + }\n> > > + captureResult.output_buffers = descriptor.buffers_.data();\n> > > + captureResult.partial_result = 1;\n> > > +\n> > > + /*\n> > > + * If the Request has failed, abort the request by notifying the\n> > > error\n> > > + * and complete the request with all buffers in error state.\n> > > + */\n> > > if (request->status() != Request::RequestComplete) {\n> > > - LOG(HAL, Error) << \"Request not successfully completed: \"\n> > > + LOG(HAL, Error) << \"Request \" << request->cookie()\n> > > + << \" not successfully completed: \"\n> > > << request->status();\n> > > - status = CAMERA3_BUFFER_STATUS_ERROR;\n> > > +\n> > > + notifyError(descriptor.frameNumber_,\n> > > + descriptor.buffers_[0].stream,\n> > > + CAMERA3_MSG_ERROR_REQUEST);\n> > > +\n> > > + for (camera3_stream_buffer_t &buffer : descriptor.buffers_)\n> > > + buffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> > > + callbacks_->process_capture_result(callbacks_,\n> > > &captureResult);\n> > > +\n> > > + return;\n> > > }\n> > >\n> > > + /*\n> > > + * Notify shutter as soon as we have verified we have a valid\n> > > request.\n> > > + *\n> > > + * \\todo The shutter event notification should be sent to the\n> > > framework\n> > > + * as soon as possible, earlier than request completion time.\n> > > + */\n> > > + uint64_t sensorTimestamp =\n> > > static_cast(request->metadata()\n> > > +\n> > > .get(controls::SensorTimestamp));\n> > > + notifyShutter(descriptor.frameNumber_, sensorTimestamp);\n> > > +\n> > > LOG(HAL, Debug) << \"Request \" << request->cookie() << \" completed\n> > > with \"\n> > > << descriptor.buffers_.size() << \" streams\";\n> > >\n> > > - resultMetadata = getResultMetadata(descriptor);\n> > > + /*\n> > > + * Generate the metadata associated with the captured buffers.\n> > > + *\n> > > + * Notify if the metadata generation has failed, but continue\n> > > processing\n> > > + * buffers and return an empty metadata pack.\n> > > + */\n> > > + std::unique_ptr resultMetadata =\n> > > getResultMetadata(descriptor);\n> > > + if (!resultMetadata) {\n> > > + notifyError(descriptor.frameNumber_,\n> > > descriptor.buffers_[0].stream,\n> > > + CAMERA3_MSG_ERROR_RESULT);\n> > > +\n> > > + /* The camera framework expects an empy metadata pack on\n> > > error. */\n> > > + resultMetadata = std::make_unique(0, 0);\n> > > + }\n> > > + captureResult.result = resultMetadata->get();\n> > >\n> > > /* Handle any JPEG compression. */\n> > > for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> > > @@ -2065,56 +2118,27 @@ void CameraDevice::requestComplete(Request\n> > > *request)\n> > > FrameBuffer *src =\n> > > request->findBuffer(cameraStream->stream());\n> > > if (!src) {\n> > > LOG(HAL, Error) << \"Failed to find a source stream\n> > > buffer\";\n> > > + buffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> > > + notifyError(descriptor.frameNumber_, buffer.stream,\n> > > + CAMERA3_MSG_ERROR_BUFFER);\n> > > continue;\n> > >\n> >\n> > Is this still continue? Perhaps return?\n> \n> Is it ? my understandin is that if a single buffer fails, the request\n> can be completed and that single buffer state will be set to\n> CAMERA3_BUFFER_STATUS_ERROR and the framework has to be notified about\n> the CAMERA3_MSG_ERROR_BUFFER event. Metadata and other succesfully\n> completed buffers can be returned correctly if they have been properly\n> filled, hence I think we should continue and process the next\n> buffer...\n> \n> A question: are there tests in the cros_camera_test package that\n> exercize these error paths ?\n> \n> > > }\n> > >\n> > > - int ret = cameraStream->process(*src,\n> > > - *buffer.buffer,\n> > > + int ret = cameraStream->process(*src, *buffer.buffer,\n> > > descriptor.settings_,\n> > > resultMetadata.get());\n> > > - if (ret) {\n> > > - status = CAMERA3_BUFFER_STATUS_ERROR;\n> > > - continue;\n> > > - }\n> > > -\n> > > /*\n> > > * Return the FrameBuffer to the CameraStream now that\n> > > we're\n> > > * done processing it.\n> > > */\n> > > if (cameraStream->type() == CameraStream::Type::Internal)\n> > > cameraStream->putBuffer(src);\n> > > - }\n> > >\n> > > - /* Prepare to call back the Android camera stack. */\n> > > - camera3_capture_result_t captureResult = {};\n> > > - captureResult.frame_number = descriptor.frameNumber_;\n> > > - captureResult.num_output_buffers = descriptor.buffers_.size();\n> > > - for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> > > - buffer.acquire_fence = -1;\n> > > - buffer.release_fence = -1;\n> > > - buffer.status = status;\n> > > - }\n> > > - captureResult.output_buffers = descriptor.buffers_.data();\n> > > -\n> > > - if (status == CAMERA3_BUFFER_STATUS_OK) {\n> > > - uint64_t timestamp =\n> > > - static_cast(request->metadata()\n> > > -\n> > > .get(controls::SensorTimestamp));\n> > > - notifyShutter(descriptor.frameNumber_, timestamp);\n> > > -\n> > > - captureResult.partial_result = 1;\n> > > - captureResult.result = resultMetadata->get();\n> > > - }\n> > > -\n> > > - if (status == CAMERA3_BUFFER_STATUS_ERROR ||\n> > > !captureResult.result) {\n> > > - /* \\todo Improve error handling. In case we notify an error\n> > > - * because the metadata generation fails, a shutter event\n> > > has\n> > > - * already been notified for this frame number before the\n> > > error\n> > > - * is here signalled. Make sure the error path plays well\n> > > with\n> > > - * the camera stack state machine.\n> > > - */\n> > > - notifyError(descriptor.frameNumber_,\n> > > - descriptor.buffers_[0].stream);\n> > > + if (ret) {\n> > > + buffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> > > + notifyError(descriptor.frameNumber_, buffer.stream,\n> > > + CAMERA3_MSG_ERROR_BUFFER);\n> > >\n> >\n> > same here.\n> >\n> > + }\n> > > }\n> > >\n> > > callbacks_->process_capture_result(callbacks_, &captureResult);\n> > > @@ -2136,23 +2160,23 @@ void CameraDevice::notifyShutter(uint32_t\n> > > frameNumber, uint64_t timestamp)\n> > > callbacks_->notify(callbacks_, ¬ify);\n> > > }\n> > >\n> > > -void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t\n> > > *stream)\n> > > +void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t\n> > > *stream,\n> > > + camera3_error_msg_code code)\n> > > {\n> > > camera3_notify_msg_t notify = {};\n> > >\n> > > - /*\n> > > - * \\todo Report and identify the stream number or configuration to\n> > > - * clarify the stream that failed.\n> > > - */\n> > > - LOG(HAL, Error) << \"Error occurred on frame \" << frameNumber << \"\n> > > (\"\n> > > - << toPixelFormat(stream->format).toString() << \")\";\n> > > -\n> > > notify.type = CAMERA3_MSG_ERROR;\n> > > notify.message.error.error_stream = stream;\n> > > notify.message.error.frame_number = frameNumber;\n> > > - notify.message.error.error_code = CAMERA3_MSG_ERROR_REQUEST;\n> > > + notify.message.error.error_code = code;\n> > >\n> > > callbacks_->notify(callbacks_, ¬ify);\n> > > +\n> > > + if (stream)\n> > > + LOG(HAL, Error) << \"Error occurred on frame \" <<\n> > > frameNumber << \" (\"\n> > > + <<\n> > > toPixelFormat(stream->format).toString() << \")\";\n> > > + else\n> > > + LOG(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 23457e47767a..8d5da8bc59e1 100644\n> > > --- a/src/android/camera_device.h\n> > > +++ b/src/android/camera_device.h\n> > > @@ -101,7 +101,8 @@ private:\n> > > std::tuple calculateStaticMetadataSize();\n> > > libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t\n> > > camera3buffer);\n> > > void notifyShutter(uint32_t frameNumber, uint64_t timestamp);\n> > > - void notifyError(uint32_t frameNumber, camera3_stream_t *stream);\n> > > + void notifyError(uint32_t frameNumber, camera3_stream_t *stream,\n> > > + camera3_error_msg_code code);\n> > > std::unique_ptr requestTemplatePreview();\n> > > std::unique_ptr requestTemplateVideo();\n> > > libcamera::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 CC129BF829\n\tfor ;\n\tFri, 7 May 2021 12:27:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4E276602BE;\n\tFri, 7 May 2021 14:27:39 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 14FE5602BB\n\tfor ;\n\tFri, 7 May 2021 14:27:38 +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 7FF8A2CF;\n\tFri, 7 May 2021 14:27:37 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"szy3EkXu\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1620390457;\n\tbh=R9twP9jOS4R0wnLQQ6rfhu/w4WfYTLckFkb9YgOt+Pk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=szy3EkXua6O+yiGC91EvjGWS9GHTk26bjHql6kzMYKq5XGpARunabO+s+yYE6yU8O\n\tfb52BLU6ir1FpVrg2m2UWyyM9fSZk2NvcWWf/5qgtsqoFwnU8qdWW53hjA0Z0iBmW2\n\tK2wCb/mivvMzMYJvF9ouLkLzDX5akrCtPcOAauJc=","Date":"Fri, 7 May 2021 15:27:32 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210506150106.28470-1-jacopo@jmondi.org>\n\t\n\t<20210507073739.n27h23o6kpdflkdf@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210507073739.n27h23o6kpdflkdf@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v2] android: Rework request completion\n\tnotification","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16846,"web_url":"https://patchwork.libcamera.org/comment/16846/","msgid":"<20210508063712.yrkd5jn2jd25ymj3@uno.localdomain>","date":"2021-05-08T06:37:12","subject":"Re: [libcamera-devel] [PATCH v2] android: Rework request completion\n\tnotification","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Fri, May 07, 2021 at 03:27:32PM +0300, Laurent Pinchart wrote:\n> On Fri, May 07, 2021 at 09:37:39AM +0200, Jacopo Mondi wrote:\n> > On Fri, May 07, 2021 at 12:50:13PM +0900, Hirokazu Honda wrote:\n> > > On Fri, May 7, 2021 at 12:00 AM Jacopo Mondi wrote:\n> > >\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 close the camera in case a Fatal\n> > > > error is detected (Fatal does not perform library teardown in\n> > > > production builds)\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> > > > ---\n> > > >\n> > > > v2:\n> > > > - Rework a comment as suggested by Niklas\n> > > > - As Niklas reported, dereferencing a unique_ptr<> which owns a nullptr has\n> > > > an undefined behaviour. Replace that pattern by resetting the\n> > > > resultMetadata\n> > > > unique_ptr<> with a new one holding an empty CameraMetadata, as the\n> > > > framework\n> > > > requires on error.\n> > > >\n> > > > camera3.h:\n> > > > * If there was an error producing the result metadata, result must be\n> > > > an\n> > > > * empty metadata buffer, and notify() must be called with\n> > > > ERROR_RESULT.\n> > > >\n> > > >\n> > > > std::unique_ptr resultMetadata =\n> > > > getResultMetadata(descriptor);\n> > > > if (!resultMetadata) {\n> > > > notifyError(descriptor.frameNumber_,\n> > > > descriptor.buffers_[0].stream,\n> > > > CAMERA3_MSG_ERROR_RESULT);\n> > > >\n> > > > /* The camera framework expects an empy metadata pack on\n> > > > error. */\n> > > > resultMetadata = std::make_unique(0, 0);\n> > > > }\n> > > > captureResult.result = resultMetadata->get();\n> > > >\n> > > > ---\n> > > > src/android/camera_device.cpp | 132 ++++++++++++++++++++--------------\n> > > > src/android/camera_device.h | 3 +-\n> > > > 2 files changed, 80 insertions(+), 55 deletions(-)\n> > > >\n> > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > index f1f26c775611..ea7ec8c05f88 100644\n> > > > --- a/src/android/camera_device.cpp\n> > > > +++ b/src/android/camera_device.cpp\n> > > > @@ -2025,9 +2025,6 @@ int\n> > > > CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > > >\n> > > > void CameraDevice::requestComplete(Request *request)\n> > > > {\n> > > > - camera3_buffer_status status = CAMERA3_BUFFER_STATUS_OK;\n> > > > - std::unique_ptr resultMetadata;\n> > > > -\n> > > > decltype(descriptors_)::node_type node;\n> > > > {\n> > > > std::scoped_lock lock(mutex_);\n> > > > @@ -2035,7 +2032,8 @@ void CameraDevice::requestComplete(Request *request)\n> > > > if (it == descriptors_.end()) {\n> > > > LOG(HAL, Fatal)\n> > > > << \"Unknown request: \" <<\n> > > > request->cookie();\n> > > > - status = CAMERA3_BUFFER_STATUS_ERROR;\n> > > > + close();\n> > > >\n> > >\n> > > close() calls Camera::stop(). Camera::stop() cancels the pending request\n> > > and requestComplete() will be invoked. (this is not done so cf.\n> > > https://patchwork.libcamera.org/project/libcamera/list/?series=1954)\n> > > Furthermore, Camera::stop() is a blocking function. So I think calling\n> > > close() here possibly causes the infinite-loop, e.g. when descriptors_ is\n> > > somehow empty.\n> >\n> > Mmm, you're right, it doesn't seem like an infinite loop but rather a\n> > deadlock, as we're in critical section and all new requestComplete()\n> > calls due to the Camera::stop() will contend this mutex\n> >\n> > > Ah, but LOG(HAL, Fatal) causes the crash libcamera process, hmm..\n> >\n> > I recall we had a compiler option to make Fatal not fatal (sorry...)\n> > in production builds... I don't seem to be able to find it though.\n> >\n> > If Fatal is always fatal, well, I can only notify the ERROR_DEVICE and\n> > then crash, so no close is required.\n>\n> We've been toying with the idea of disabling the assertion error for\n> Fatal in production builds, and I'm still tempted to do so, so it would\n> be best if errors could be handled gracefully.\n>\n\nOh, I was confused and I thought it went actually in.. That's why I\ncouldn't find it anywhere!\n\nI'll wait for clarifications about the requirement for the camera to\nbe closed or not in case of CAMERA3_MSG_ERROR_DEVICE and I'll try to\nhandle this gracefully...\n\nThanks\n j\n\n> > If Fatal can actually be disabled I feel like close() will need to be\n> > called according to the camera3 API, but I'm not sure if the framework\n> > does that, or the HAL should do so, as I've found two contradictory\n> > statements:\n> >\n> >\n> > /**\n> > * A serious failure occured. No further frames or buffer streams will\n> > * be produced by the device. Device should be treated as closed. The\n> > * client must reopen the device to use it again. The frame_number field\n> > * is unused.\n> > */\n> > CAMERA3_MSG_ERROR_DEVICE = 1,\n> >\n> > This seems to imply the device should be re-opened and treated as\n> > closed by the framwork, so the HAL has actually to close it.\n> >\n> > * S6. Error management:\n> > *\n> > * Camera HAL device ops functions that have a return value will all return\n> > * -ENODEV / NULL in case of a serious error. This means the device cannot\n> > * continue operation, and must be closed by the framework. Once this error is\n> > * returned by some method, or if notify() is called with ERROR_DEVICE, only\n> > * the close() method can be called successfully. All other methods will return\n> > * -ENODEV / NULL.\n> >\n> > This seems to imply the framework calls close() after it receives\n> > ERROR_DEVICE.\n> >\n> > Could this maybe clarified with Ricky's team ?\n> >\n> > > > + notifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);\n> > > > return;\n> > > > }\n> > > >\n> > > > @@ -2043,16 +2041,71 @@ void CameraDevice::requestComplete(Request\n> > > > *request)\n> > > > }\n> > > > Camera3RequestDescriptor &descriptor = node.mapped();\n> > > >\n> > > > + /*\n> > > > + * Prepare the capture result for the Android camera stack.\n> > > > + *\n> > > > + * The buffer status is set to OK and later changed to ERROR if\n> > > > + * post-processing/compression fails.\n> > > > + */\n> > > > + camera3_capture_result_t captureResult = {};\n> > > > + captureResult.frame_number = descriptor.frameNumber_;\n> > > > + captureResult.num_output_buffers = descriptor.buffers_.size();\n> > > > + for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> > > > + buffer.acquire_fence = -1;\n> > > > + buffer.release_fence = -1;\n> > > > + buffer.status = CAMERA3_BUFFER_STATUS_OK;\n> > > > + }\n> > > > + captureResult.output_buffers = descriptor.buffers_.data();\n> > > > + captureResult.partial_result = 1;\n> > > > +\n> > > > + /*\n> > > > + * If the Request has failed, abort the request by notifying the\n> > > > error\n> > > > + * and complete the request with all buffers in error state.\n> > > > + */\n> > > > if (request->status() != Request::RequestComplete) {\n> > > > - LOG(HAL, Error) << \"Request not successfully completed: \"\n> > > > + LOG(HAL, Error) << \"Request \" << request->cookie()\n> > > > + << \" not successfully completed: \"\n> > > > << request->status();\n> > > > - status = CAMERA3_BUFFER_STATUS_ERROR;\n> > > > +\n> > > > + notifyError(descriptor.frameNumber_,\n> > > > + descriptor.buffers_[0].stream,\n> > > > + CAMERA3_MSG_ERROR_REQUEST);\n> > > > +\n> > > > + for (camera3_stream_buffer_t &buffer : descriptor.buffers_)\n> > > > + buffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> > > > + callbacks_->process_capture_result(callbacks_,\n> > > > &captureResult);\n> > > > +\n> > > > + return;\n> > > > }\n> > > >\n> > > > + /*\n> > > > + * Notify shutter as soon as we have verified we have a valid\n> > > > request.\n> > > > + *\n> > > > + * \\todo The shutter event notification should be sent to the\n> > > > framework\n> > > > + * as soon as possible, earlier than request completion time.\n> > > > + */\n> > > > + uint64_t sensorTimestamp =\n> > > > static_cast(request->metadata()\n> > > > +\n> > > > .get(controls::SensorTimestamp));\n> > > > + notifyShutter(descriptor.frameNumber_, sensorTimestamp);\n> > > > +\n> > > > LOG(HAL, Debug) << \"Request \" << request->cookie() << \" completed\n> > > > with \"\n> > > > << descriptor.buffers_.size() << \" streams\";\n> > > >\n> > > > - resultMetadata = getResultMetadata(descriptor);\n> > > > + /*\n> > > > + * Generate the metadata associated with the captured buffers.\n> > > > + *\n> > > > + * Notify if the metadata generation has failed, but continue\n> > > > processing\n> > > > + * buffers and return an empty metadata pack.\n> > > > + */\n> > > > + std::unique_ptr resultMetadata =\n> > > > getResultMetadata(descriptor);\n> > > > + if (!resultMetadata) {\n> > > > + notifyError(descriptor.frameNumber_,\n> > > > descriptor.buffers_[0].stream,\n> > > > + CAMERA3_MSG_ERROR_RESULT);\n> > > > +\n> > > > + /* The camera framework expects an empy metadata pack on\n> > > > error. */\n> > > > + resultMetadata = std::make_unique(0, 0);\n> > > > + }\n> > > > + captureResult.result = resultMetadata->get();\n> > > >\n> > > > /* Handle any JPEG compression. */\n> > > > for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> > > > @@ -2065,56 +2118,27 @@ void CameraDevice::requestComplete(Request\n> > > > *request)\n> > > > FrameBuffer *src =\n> > > > request->findBuffer(cameraStream->stream());\n> > > > if (!src) {\n> > > > LOG(HAL, Error) << \"Failed to find a source stream\n> > > > buffer\";\n> > > > + buffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> > > > + notifyError(descriptor.frameNumber_, buffer.stream,\n> > > > + CAMERA3_MSG_ERROR_BUFFER);\n> > > > continue;\n> > > >\n> > >\n> > > Is this still continue? Perhaps return?\n> >\n> > Is it ? my understandin is that if a single buffer fails, the request\n> > can be completed and that single buffer state will be set to\n> > CAMERA3_BUFFER_STATUS_ERROR and the framework has to be notified about\n> > the CAMERA3_MSG_ERROR_BUFFER event. Metadata and other succesfully\n> > completed buffers can be returned correctly if they have been properly\n> > filled, hence I think we should continue and process the next\n> > buffer...\n> >\n> > A question: are there tests in the cros_camera_test package that\n> > exercize these error paths ?\n> >\n> > > > }\n> > > >\n> > > > - int ret = cameraStream->process(*src,\n> > > > - *buffer.buffer,\n> > > > + int ret = cameraStream->process(*src, *buffer.buffer,\n> > > > descriptor.settings_,\n> > > > resultMetadata.get());\n> > > > - if (ret) {\n> > > > - status = CAMERA3_BUFFER_STATUS_ERROR;\n> > > > - continue;\n> > > > - }\n> > > > -\n> > > > /*\n> > > > * Return the FrameBuffer to the CameraStream now that\n> > > > we're\n> > > > * done processing it.\n> > > > */\n> > > > if (cameraStream->type() == CameraStream::Type::Internal)\n> > > > cameraStream->putBuffer(src);\n> > > > - }\n> > > >\n> > > > - /* Prepare to call back the Android camera stack. */\n> > > > - camera3_capture_result_t captureResult = {};\n> > > > - captureResult.frame_number = descriptor.frameNumber_;\n> > > > - captureResult.num_output_buffers = descriptor.buffers_.size();\n> > > > - for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> > > > - buffer.acquire_fence = -1;\n> > > > - buffer.release_fence = -1;\n> > > > - buffer.status = status;\n> > > > - }\n> > > > - captureResult.output_buffers = descriptor.buffers_.data();\n> > > > -\n> > > > - if (status == CAMERA3_BUFFER_STATUS_OK) {\n> > > > - uint64_t timestamp =\n> > > > - static_cast(request->metadata()\n> > > > -\n> > > > .get(controls::SensorTimestamp));\n> > > > - notifyShutter(descriptor.frameNumber_, timestamp);\n> > > > -\n> > > > - captureResult.partial_result = 1;\n> > > > - captureResult.result = resultMetadata->get();\n> > > > - }\n> > > > -\n> > > > - if (status == CAMERA3_BUFFER_STATUS_ERROR ||\n> > > > !captureResult.result) {\n> > > > - /* \\todo Improve error handling. In case we notify an error\n> > > > - * because the metadata generation fails, a shutter event\n> > > > has\n> > > > - * already been notified for this frame number before the\n> > > > error\n> > > > - * is here signalled. Make sure the error path plays well\n> > > > with\n> > > > - * the camera stack state machine.\n> > > > - */\n> > > > - notifyError(descriptor.frameNumber_,\n> > > > - descriptor.buffers_[0].stream);\n> > > > + if (ret) {\n> > > > + buffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> > > > + notifyError(descriptor.frameNumber_, buffer.stream,\n> > > > + CAMERA3_MSG_ERROR_BUFFER);\n> > > >\n> > >\n> > > same here.\n> > >\n> > > + }\n> > > > }\n> > > >\n> > > > callbacks_->process_capture_result(callbacks_, &captureResult);\n> > > > @@ -2136,23 +2160,23 @@ void CameraDevice::notifyShutter(uint32_t\n> > > > frameNumber, uint64_t timestamp)\n> > > > callbacks_->notify(callbacks_, ¬ify);\n> > > > }\n> > > >\n> > > > -void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t\n> > > > *stream)\n> > > > +void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t\n> > > > *stream,\n> > > > + camera3_error_msg_code code)\n> > > > {\n> > > > camera3_notify_msg_t notify = {};\n> > > >\n> > > > - /*\n> > > > - * \\todo Report and identify the stream number or configuration to\n> > > > - * clarify the stream that failed.\n> > > > - */\n> > > > - LOG(HAL, Error) << \"Error occurred on frame \" << frameNumber << \"\n> > > > (\"\n> > > > - << toPixelFormat(stream->format).toString() << \")\";\n> > > > -\n> > > > notify.type = CAMERA3_MSG_ERROR;\n> > > > notify.message.error.error_stream = stream;\n> > > > notify.message.error.frame_number = frameNumber;\n> > > > - notify.message.error.error_code = CAMERA3_MSG_ERROR_REQUEST;\n> > > > + notify.message.error.error_code = code;\n> > > >\n> > > > callbacks_->notify(callbacks_, ¬ify);\n> > > > +\n> > > > + if (stream)\n> > > > + LOG(HAL, Error) << \"Error occurred on frame \" <<\n> > > > frameNumber << \" (\"\n> > > > + <<\n> > > > toPixelFormat(stream->format).toString() << \")\";\n> > > > + else\n> > > > + LOG(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 23457e47767a..8d5da8bc59e1 100644\n> > > > --- a/src/android/camera_device.h\n> > > > +++ b/src/android/camera_device.h\n> > > > @@ -101,7 +101,8 @@ private:\n> > > > std::tuple calculateStaticMetadataSize();\n> > > > libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t\n> > > > camera3buffer);\n> > > > void notifyShutter(uint32_t frameNumber, uint64_t timestamp);\n> > > > - void notifyError(uint32_t frameNumber, camera3_stream_t *stream);\n> > > > + void notifyError(uint32_t frameNumber, camera3_stream_t *stream,\n> > > > + camera3_error_msg_code code);\n> > > > std::unique_ptr requestTemplatePreview();\n> > > > std::unique_ptr requestTemplateVideo();\n> > > > libcamera::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 015D7BF831\n\tfor ;\n\tSat, 8 May 2021 06:36:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7BF5668919;\n\tSat, 8 May 2021 08:36:31 +0200 (CEST)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6C31C602B8\n\tfor ;\n\tSat, 8 May 2021 08:36:30 +0200 (CEST)","from uno.localdomain (host-79-53-131-195.retail.telecomitalia.it\n\t[79.53.131.195]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id B38A540003;\n\tSat, 8 May 2021 06:36:28 +0000 (UTC)"],"X-Originating-IP":"79.53.131.195","Date":"Sat, 8 May 2021 08:37:12 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210508063712.yrkd5jn2jd25ymj3@uno.localdomain>","References":"<20210506150106.28470-1-jacopo@jmondi.org>\n\t\n\t<20210507073739.n27h23o6kpdflkdf@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2] android: Rework request completion\n\tnotification","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16850,"web_url":"https://patchwork.libcamera.org/comment/16850/","msgid":"","date":"2021-05-10T06:02:27","subject":"Re: [libcamera-devel] [PATCH v2] android: Rework request completion\n\tnotification","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Jacopo,\n\nOn Fri, May 7, 2021 at 4:36 PM Jacopo Mondi wrote:\n\n> Hi Hiro,\n> thanks for review\n>\n> On Fri, May 07, 2021 at 12:50:13PM +0900, Hirokazu Honda wrote:\n> > Hi Jacopo, thank you for the patch.\n> >\n> > On Fri, May 7, 2021 at 12:00 AM Jacopo Mondi wrote:\n> >\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\n> 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\n> 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 close the camera in case a Fatal\n> > > error is detected (Fatal does not perform library teardown in\n> > > production builds)\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> > > ---\n> > >\n> > > v2:\n> > > - Rework a comment as suggested by Niklas\n> > > - As Niklas reported, dereferencing a unique_ptr<> which owns a\n> nullptr has\n> > > an undefined behaviour. Replace that pattern by resetting the\n> > > resultMetadata\n> > > unique_ptr<> with a new one holding an empty CameraMetadata, as the\n> > > framework\n> > > requires on error.\n> > >\n> > > camera3.h:\n> > > * If there was an error producing the result metadata, result\n> must be\n> > > an\n> > > * empty metadata buffer, and notify() must be called with\n> > > ERROR_RESULT.\n> > >\n> > >\n> > > std::unique_ptr resultMetadata =\n> > > getResultMetadata(descriptor);\n> > > if (!resultMetadata) {\n> > > notifyError(descriptor.frameNumber_,\n> > > descriptor.buffers_[0].stream,\n> > > CAMERA3_MSG_ERROR_RESULT);\n> > >\n> > > /* The camera framework expects an empy metadata pack\n> on\n> > > error. */\n> > > resultMetadata = std::make_unique(0,\n> 0);\n> > > }\n> > > captureResult.result = resultMetadata->get();\n> > >\n> > > ---\n> > > src/android/camera_device.cpp | 132 ++++++++++++++++++++--------------\n> > > src/android/camera_device.h | 3 +-\n> > > 2 files changed, 80 insertions(+), 55 deletions(-)\n> > >\n> > > diff --git a/src/android/camera_device.cpp\n> b/src/android/camera_device.cpp\n> > > index f1f26c775611..ea7ec8c05f88 100644\n> > > --- a/src/android/camera_device.cpp\n> > > +++ b/src/android/camera_device.cpp\n> > > @@ -2025,9 +2025,6 @@ int\n> > > CameraDevice::processCaptureRequest(camera3_capture_request_t\n> *camera3Reques\n> > >\n> > > void CameraDevice::requestComplete(Request *request)\n> > > {\n> > > - camera3_buffer_status status = CAMERA3_BUFFER_STATUS_OK;\n> > > - std::unique_ptr resultMetadata;\n> > > -\n> > > decltype(descriptors_)::node_type node;\n> > > {\n> > > std::scoped_lock lock(mutex_);\n> > > @@ -2035,7 +2032,8 @@ void CameraDevice::requestComplete(Request\n> *request)\n> > > if (it == descriptors_.end()) {\n> > > LOG(HAL, Fatal)\n> > > << \"Unknown request: \" <<\n> > > request->cookie();\n> > > - status = CAMERA3_BUFFER_STATUS_ERROR;\n> > > + close();\n> > >\n> >\n> > close() calls Camera::stop(). Camera::stop() cancels the pending request\n> > and requestComplete() will be invoked. (this is not done so cf.\n> > https://patchwork.libcamera.org/project/libcamera/list/?series=1954)\n> > Furthermore, Camera::stop() is a blocking function. So I think calling\n> > close() here possibly causes the infinite-loop, e.g. when descriptors_ is\n> > somehow empty.\n>\n> Mmm, you're right, it doesn't seem like an infinite loop but rather a\n> deadlock, as we're in critical section and all new requestComplete()\n> calls due to the Camera::stop() will contend this mutex\n>\n> > Ah, but LOG(HAL, Fatal) causes the crash libcamera process, hmm..\n>\n> I recall we had a compiler option to make Fatal not fatal (sorry...)\n> in production builds... I don't seem to be able to find it though.\n>\n> If Fatal is always fatal, well, I can only notify the ERROR_DEVICE and\n> then crash, so no close is required.\n>\n> If Fatal can actually be disabled I feel like close() will need to be\n> called according to the camera3 API, but I'm not sure if the framework\n> does that, or the HAL should do so, as I've found two contradictory\n> statements:\n>\n>\n> /**\n> * A serious failure occured. No further frames or buffer streams will\n> * be produced by the device. Device should be treated as closed. The\n> * client must reopen the device to use it again. The frame_number\n> field\n> * is unused.\n> */\n> CAMERA3_MSG_ERROR_DEVICE = 1,\n>\n> This seems to imply the device should be re-opened and treated as\n> closed by the framwork, so the HAL has actually to close it.\n>\n> * S6. Error management:\n> *\n> * Camera HAL device ops functions that have a return value will all return\n> * -ENODEV / NULL in case of a serious error. This means the device cannot\n> * continue operation, and must be closed by the framework. Once this\n> error is\n> * returned by some method, or if notify() is called with ERROR_DEVICE,\n> only\n> * the close() method can be called successfully. All other methods will\n> return\n> * -ENODEV / NULL.\n>\n> This seems to imply the framework calls close() after it receives\n> ERROR_DEVICE.\n>\n> Could this maybe clarified with Ricky's team ?\n>\n>\n+Ricky Liang , would you answer this question?\n\n\n\n> >\n> >\n> >\n> > > + notifyError(0, nullptr,\n> CAMERA3_MSG_ERROR_DEVICE);\n> > > return;\n> > > }\n> > >\n> > > @@ -2043,16 +2041,71 @@ void CameraDevice::requestComplete(Request\n> > > *request)\n> > > }\n> > > Camera3RequestDescriptor &descriptor = node.mapped();\n> > >\n> > > + /*\n> > > + * Prepare the capture result for the Android camera stack.\n> > > + *\n> > > + * The buffer status is set to OK and later changed to ERROR if\n> > > + * post-processing/compression fails.\n> > > + */\n> > > + camera3_capture_result_t captureResult = {};\n> > > + captureResult.frame_number = descriptor.frameNumber_;\n> > > + captureResult.num_output_buffers = descriptor.buffers_.size();\n> > > + for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> > > + buffer.acquire_fence = -1;\n> > > + buffer.release_fence = -1;\n> > > + buffer.status = CAMERA3_BUFFER_STATUS_OK;\n> > > + }\n> > > + captureResult.output_buffers = descriptor.buffers_.data();\n> > > + captureResult.partial_result = 1;\n> > > +\n> > > + /*\n> > > + * If the Request has failed, abort the request by notifying\n> the\n> > > error\n> > > + * and complete the request with all buffers in error state.\n> > > + */\n> > > if (request->status() != Request::RequestComplete) {\n> > > - LOG(HAL, Error) << \"Request not successfully\n> completed: \"\n> > > + LOG(HAL, Error) << \"Request \" << request->cookie()\n> > > + << \" not successfully completed: \"\n> > > << request->status();\n> > > - status = CAMERA3_BUFFER_STATUS_ERROR;\n> > > +\n> > > + notifyError(descriptor.frameNumber_,\n> > > + descriptor.buffers_[0].stream,\n> > > + CAMERA3_MSG_ERROR_REQUEST);\n> > > +\n> > > + for (camera3_stream_buffer_t &buffer :\n> descriptor.buffers_)\n> > > + buffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> > > + callbacks_->process_capture_result(callbacks_,\n> > > &captureResult);\n> > > +\n> > > + return;\n> > > }\n> > >\n> > > + /*\n> > > + * Notify shutter as soon as we have verified we have a valid\n> > > request.\n> > > + *\n> > > + * \\todo The shutter event notification should be sent to the\n> > > framework\n> > > + * as soon as possible, earlier than request completion time.\n> > > + */\n> > > + uint64_t sensorTimestamp =\n> > > static_cast(request->metadata()\n> > > +\n> > > .get(controls::SensorTimestamp));\n> > > + notifyShutter(descriptor.frameNumber_, sensorTimestamp);\n> > > +\n> > > LOG(HAL, Debug) << \"Request \" << request->cookie() << \"\n> completed\n> > > with \"\n> > > << descriptor.buffers_.size() << \" streams\";\n> > >\n> > > - resultMetadata = getResultMetadata(descriptor);\n> > > + /*\n> > > + * Generate the metadata associated with the captured buffers.\n> > > + *\n> > > + * Notify if the metadata generation has failed, but continue\n> > > processing\n> > > + * buffers and return an empty metadata pack.\n> > > + */\n> > > + std::unique_ptr resultMetadata =\n> > > getResultMetadata(descriptor);\n> > > + if (!resultMetadata) {\n> > > + notifyError(descriptor.frameNumber_,\n> > > descriptor.buffers_[0].stream,\n> > > + CAMERA3_MSG_ERROR_RESULT);\n> > > +\n> > > + /* The camera framework expects an empy metadata pack\n> on\n> > > error. */\n> > > + resultMetadata = std::make_unique(0,\n> 0);\n> > > + }\n> > > + captureResult.result = resultMetadata->get();\n> > >\n> > > /* Handle any JPEG compression. */\n> > > for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> > > @@ -2065,56 +2118,27 @@ void CameraDevice::requestComplete(Request\n> > > *request)\n> > > FrameBuffer *src =\n> > > request->findBuffer(cameraStream->stream());\n> > > if (!src) {\n> > > LOG(HAL, Error) << \"Failed to find a source\n> stream\n> > > buffer\";\n> > > + buffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> > > + notifyError(descriptor.frameNumber_,\n> buffer.stream,\n> > > + CAMERA3_MSG_ERROR_BUFFER);\n> > > continue;\n> > >\n> >\n> > Is this still continue? Perhaps return?\n>\n> Is it ? my understandin is that if a single buffer fails, the request\n> can be completed and that single buffer state will be set to\n> CAMERA3_BUFFER_STATUS_ERROR and the framework has to be notified about\n> the CAMERA3_MSG_ERROR_BUFFER event. Metadata and other succesfully\n> completed buffers can be returned correctly if they have been properly\n> filled, hence I think we should continue and process the next\n> buffer...\n>\n>\nMy question is if it is allowed to call notifyError to the same stream\nmultiple times.\n\n\n\n> A question: are there tests in the cros_camera_test package that\n> exercize these error paths ?\n>\n>\nI don't think so.\n\nThanks,\n-Hiro\n\n\n> Thanks\n> j\n>\n> >\n> >\n> > > }\n> > >\n> > > - int ret = cameraStream->process(*src,\n> > > - *buffer.buffer,\n> > > + int ret = cameraStream->process(*src, *buffer.buffer,\n> > > descriptor.settings_,\n> > > resultMetadata.get());\n> > > - if (ret) {\n> > > - status = CAMERA3_BUFFER_STATUS_ERROR;\n> > > - continue;\n> > > - }\n> > > -\n> > > /*\n> > > * Return the FrameBuffer to the CameraStream now that\n> > > we're\n> > > * done processing it.\n> > > */\n> > > if (cameraStream->type() ==\n> CameraStream::Type::Internal)\n> > > cameraStream->putBuffer(src);\n> > > - }\n> > >\n> > > - /* Prepare to call back the Android camera stack. */\n> > > - camera3_capture_result_t captureResult = {};\n> > > - captureResult.frame_number = descriptor.frameNumber_;\n> > > - captureResult.num_output_buffers = descriptor.buffers_.size();\n> > > - for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> > > - buffer.acquire_fence = -1;\n> > > - buffer.release_fence = -1;\n> > > - buffer.status = status;\n> > > - }\n> > > - captureResult.output_buffers = descriptor.buffers_.data();\n> > > -\n> > > - if (status == CAMERA3_BUFFER_STATUS_OK) {\n> > > - uint64_t timestamp =\n> > > - static_cast(request->metadata()\n> > > -\n> > > .get(controls::SensorTimestamp));\n> > > - notifyShutter(descriptor.frameNumber_, timestamp);\n> > > -\n> > > - captureResult.partial_result = 1;\n> > > - captureResult.result = resultMetadata->get();\n> > > - }\n> > > -\n> > > - if (status == CAMERA3_BUFFER_STATUS_ERROR ||\n> > > !captureResult.result) {\n> > > - /* \\todo Improve error handling. In case we notify an\n> error\n> > > - * because the metadata generation fails, a shutter\n> event\n> > > has\n> > > - * already been notified for this frame number before\n> the\n> > > error\n> > > - * is here signalled. Make sure the error path plays\n> well\n> > > with\n> > > - * the camera stack state machine.\n> > > - */\n> > > - notifyError(descriptor.frameNumber_,\n> > > - descriptor.buffers_[0].stream);\n> > > + if (ret) {\n> > > + buffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> > > + notifyError(descriptor.frameNumber_,\n> buffer.stream,\n> > > + CAMERA3_MSG_ERROR_BUFFER);\n> > >\n> >\n> > same here.\n> >\n> > -Hiro\n> >\n> > + }\n> > > }\n> > >\n> > > callbacks_->process_capture_result(callbacks_, &captureResult);\n> > > @@ -2136,23 +2160,23 @@ void CameraDevice::notifyShutter(uint32_t\n> > > frameNumber, uint64_t timestamp)\n> > > callbacks_->notify(callbacks_, ¬ify);\n> > > }\n> > >\n> > > -void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t\n> > > *stream)\n> > > +void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t\n> > > *stream,\n> > > + camera3_error_msg_code code)\n> > > {\n> > > camera3_notify_msg_t notify = {};\n> > >\n> > > - /*\n> > > - * \\todo Report and identify the stream number or\n> configuration to\n> > > - * clarify the stream that failed.\n> > > - */\n> > > - LOG(HAL, Error) << \"Error occurred on frame \" << frameNumber\n> << \"\n> > > (\"\n> > > - << toPixelFormat(stream->format).toString() <<\n> \")\";\n> > > -\n> > > notify.type = CAMERA3_MSG_ERROR;\n> > > notify.message.error.error_stream = stream;\n> > > notify.message.error.frame_number = frameNumber;\n> > > - notify.message.error.error_code = CAMERA3_MSG_ERROR_REQUEST;\n> > > + notify.message.error.error_code = code;\n> > >\n> > > callbacks_->notify(callbacks_, ¬ify);\n> > > +\n> > > + if (stream)\n> > > + LOG(HAL, Error) << \"Error occurred on frame \" <<\n> > > frameNumber << \" (\"\n> > > + <<\n> > > toPixelFormat(stream->format).toString() << \")\";\n> > > + else\n> > > + LOG(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 23457e47767a..8d5da8bc59e1 100644\n> > > --- a/src/android/camera_device.h\n> > > +++ b/src/android/camera_device.h\n> > > @@ -101,7 +101,8 @@ private:\n> > > std::tuple calculateStaticMetadataSize();\n> > > libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t\n> > > camera3buffer);\n> > > void notifyShutter(uint32_t frameNumber, uint64_t timestamp);\n> > > - void notifyError(uint32_t frameNumber, camera3_stream_t\n> *stream);\n> > > + void notifyError(uint32_t frameNumber, camera3_stream_t\n> *stream,\n> > > + camera3_error_msg_code code);\n> > > std::unique_ptr requestTemplatePreview();\n> > > std::unique_ptr requestTemplateVideo();\n> > > libcamera::PixelFormat toPixelFormat(int format) const;\n> > > --\n> > > 2.31.1\n> > >\n> > > _______________________________________________\n> > > libcamera-devel mailing list\n> > > libcamera-devel@lists.libcamera.org\n> > > https://lists.libcamera.org/listinfo/libcamera-devel\n> > >\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 CD62ABF829\n\tfor ;\n\tMon, 10 May 2021 06:02:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4EED16891B;\n\tMon, 10 May 2021 08:02:41 +0200 (CEST)","from mail-ej1-x633.google.com (mail-ej1-x633.google.com\n\t[IPv6:2a00:1450:4864:20::633])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9F34C6153C\n\tfor ;\n\tMon, 10 May 2021 08:02:39 +0200 (CEST)","by mail-ej1-x633.google.com with SMTP id r9so22686812ejj.3\n\tfor ;\n\tSun, 09 May 2021 23:02:39 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"W/qiIhem\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=RIwtyKvyf6UF3F/y0jLu4AVPm5MNVrzbRd5Autmd4Kk=;\n\tb=W/qiIhemL7tR1v6tGrCwoAfUfzbQQKFoIAXfBrtXJT3gAtCx//U9A7K9V85tolYwQk\n\tZb2N6d+jexdm9ONxP914p9+vwYAqHZxZ0dMzoe8UyXItgKoyeY6knDmXk5SyC4o4Oez1\n\tX6kTB0oSfIrmGr+ROOWpt45PNQ3nnBieng0dI=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=RIwtyKvyf6UF3F/y0jLu4AVPm5MNVrzbRd5Autmd4Kk=;\n\tb=h33IgF50xvohL5DbA3z6kHon1xiK8hI1LlLnFxlYQNOC/HT5eoA9Ann2Q87NaziCwG\n\tPuLZwM0jgcUKNbMFVGI4xBuoMkQZwmPl8T7OvB/pl1lDRkl2z813sGjMg+o80DYf4MI4\n\tt/89eESk0hWC3FzNRriCIK0GEq0QkCJH6nXVih98wiwKhfU8Jpx2OnCJVtCw2u6kTJcM\n\txZAsKbclGX9vipooxATrYGbKmj/EW/1x3G7JK/rWwbxatusqc6RejOrjZzZWwA50lQUi\n\twCVjpg5ok3fLZrZsBvH3XNB3k53XGYqFxkSS9QDTgasz11vziXLB5kVrzeueEKmkS0cg\n\ty4jg==","X-Gm-Message-State":"AOAM5317m7z2Bpxgta/cafXnO0WydZgvDWd5M+SeVkXkctu8MalBQUqL\n\tMOcPM+yJldrSwsGDHxke6UNHr66u+JW4owlNgr61f2qQu+o=","X-Google-Smtp-Source":"ABdhPJxuioyNX91pdfY7F9YXGkoAtK43tHbV3qSSgxZtL7zMiLjz93M/zYmxQPAZ20qjSm/uYyBSRh+J2EwYciVqCuI=","X-Received":"by 2002:a17:906:55c5:: with SMTP id\n\tz5mr23525082ejp.306.1620626558293; \n\tSun, 09 May 2021 23:02:38 -0700 (PDT)","MIME-Version":"1.0","References":"<20210506150106.28470-1-jacopo@jmondi.org>\n\t\n\t<20210507073739.n27h23o6kpdflkdf@uno.localdomain>","In-Reply-To":"<20210507073739.n27h23o6kpdflkdf@uno.localdomain>","From":"Hirokazu Honda ","Date":"Mon, 10 May 2021 15:02:27 +0900","Message-ID":"","To":"Jacopo Mondi , Ricky Liang ","Subject":"Re: [libcamera-devel] [PATCH v2] android: Rework request completion\n\tnotification","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 ","Content-Type":"multipart/mixed;\n\tboundary=\"===============3371817922037555886==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]