[{"id":16796,"web_url":"https://patchwork.libcamera.org/comment/16796/","msgid":"<YJOpTqH5gvOIn057@oden.dyn.berto.se>","date":"2021-05-06T08:31:10","subject":"Re: [libcamera-devel] [PATCH] android: Rework request completion\n\tnotification","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-04 17:58:08 +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 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 <jacopo@jmondi.org>\n> ---\n>  src/android/camera_device.cpp | 128 ++++++++++++++++++++--------------\n>  src/android/camera_device.h   |   3 +-\n>  2 files changed, 76 insertions(+), 55 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index f1f26c775611..5588e99b6eba 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -2025,9 +2025,6 @@ 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<CameraMetadata> resultMetadata;\n> -\n>  \tdecltype(descriptors_)::node_type node;\n>  \t{\n>  \t\tstd::scoped_lock<std::mutex> lock(mutex_);\n> @@ -2035,7 +2032,8 @@ void CameraDevice::requestComplete(Request *request)\n>  \t\tif (it == descriptors_.end()) {\n>  \t\t\tLOG(HAL, Fatal)\n>  \t\t\t\t<< \"Unknown request: \" << request->cookie();\n> -\t\t\tstatus = CAMERA3_BUFFER_STATUS_ERROR;\n> +\t\t\tclose();\n> +\t\t\tnotifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);\n>  \t\t\treturn;\n>  \t\t}\n>  \n> @@ -2043,16 +2041,67 @@ void CameraDevice::requestComplete(Request *request)\n>  \t}\n>  \tCamera3RequestDescriptor &descriptor = node.mapped();\n>  \n> +\t/*\n> +\t * Prepare the capture result to call back the Android camera stack with.\n\nnit: Prepare the capture result for the Android camera stack ?\n\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\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<uint64_t>(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<CameraMetadata> resultMetadata = getResultMetadata(descriptor);\n> +\tif (!resultMetadata)\n> +\t\tnotifyError(descriptor.frameNumber_, descriptor.buffers_[0].stream,\n> +\t\t\t    CAMERA3_MSG_ERROR_RESULT);\n> +\tcaptureResult.result = resultMetadata->get();\n\nHum if !resultMetadata will not resultMetadata->get() give us a nullptr \nexception here?\n\n>  \n>  \t/* Handle any JPEG compression. */\n>  \tfor (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> @@ -2065,56 +2114,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<uint64_t>(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> @@ -2136,23 +2156,23 @@ void CameraDevice::notifyShutter(uint32_t frameNumber, uint64_t timestamp)\n>  \tcallbacks_->notify(callbacks_, &notify);\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_, &notify);\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 23457e47767a..8d5da8bc59e1 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -101,7 +101,8 @@ private:\n>  \tstd::tuple<uint32_t, uint32_t> calculateStaticMetadataSize();\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<CameraMetadata> requestTemplatePreview();\n>  \tstd::unique_ptr<CameraMetadata> requestTemplateVideo();\n>  \tlibcamera::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","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 C4BC3BF81D\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu,  6 May 2021 08:31:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1EA276891B;\n\tThu,  6 May 2021 10:31:14 +0200 (CEST)","from mail-lf1-x136.google.com (mail-lf1-x136.google.com\n\t[IPv6:2a00:1450:4864:20::136])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B11F468911\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu,  6 May 2021 10:31:12 +0200 (CEST)","by mail-lf1-x136.google.com with SMTP id c11so6588881lfi.9\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 06 May 2021 01:31:12 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tu13sm495093lfk.193.2021.05.06.01.31.11\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 06 May 2021 01:31:11 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"I1ZTkBGJ\"; 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=Ge9pXUNcX5xnEW9KPaxpw/bJ0LaLHDxYt/x37UZYvYU=;\n\tb=I1ZTkBGJJQqijnrh/RoHSH6RyXvEglHK8P5opzJ+pSg66VLJhttRwwvmv/inwkRpSE\n\twjLYw3M0C94eTR/Al+xdC+OvP1shM9srILnV8pc9vvNx1fyROr8hSAUY/9rynaWvmG4T\n\tQ5Cz/WYJlPwaxYV+rX/hh/dtiyzXED0iL8cB0p2uxtgGaPSzIJMvqf2IhNBZhT0yTSLf\n\teNhnCzdJFW3qss8A6VdnHGvcGY0nbdkFc6QmgFqKT1dvmAO+bEUmHkrjcIuEmV5zFhYW\n\tZVFjLunTg/U+HUN0DI/3Rd+EBS9/bPAKsRy3If9KSH2sGVYt6OQeq624ugxhAyQ6U5cA\n\tsyJQ==","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=Ge9pXUNcX5xnEW9KPaxpw/bJ0LaLHDxYt/x37UZYvYU=;\n\tb=DY+hRuAP9TKXQy9bzB/Uki68yW5PeJUejHDZ4sMqRjWWD1uKKUpvmvmAmm8UJJyeAG\n\t/TgtcP/bGpReTbPEZF5kV2zxFfDMoTAbf6XUCQmD5HqD7AM+diJF4EOloIaOYmMLFlf3\n\tB2glsr7of3h+DXozPmDl5E+ILfw0LHIHYCADnrAkSOHKr5dan+pHPApbNyObGf7HOh8j\n\tsvsc+9I5ZQJ/afxnnuMXUqXRVdyUhDM2Vig5FY72RKSEFiRMaQVOD1bFBCh4a1ZgKLvw\n\ty+ipsvWrnVr49GPPuvnUAWhVi1ro/lXltpsHUuAuN0MLZV0dNInvTPFmzl9oDoItpLgP\n\tlIRg==","X-Gm-Message-State":"AOAM531HKHBvZZuzxad3bt9b9A2Onbd59vGye+6WCy4RnruaBdwcgozx\n\tFw8AB3+wa1VkJCuYyLfiB8E3hggeoTcS8g==","X-Google-Smtp-Source":"ABdhPJzKBLiggPOgLNqfRIhhFhMcKCUcZInNMYAb1mavOw4rfl7LMxdjrj3m0ccVzJ+NWrfHv0D9BQ==","X-Received":"by 2002:a05:6512:31c1:: with SMTP id\n\tj1mr2012295lfe.461.1620289871908; \n\tThu, 06 May 2021 01:31:11 -0700 (PDT)","Date":"Thu, 6 May 2021 10:31:10 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= <niklas.soderlund@ragnatech.se>","To":"Jacopo Mondi <jacopo@jmondi.org>","Message-ID":"<YJOpTqH5gvOIn057@oden.dyn.berto.se>","References":"<20210504155808.67191-1-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210504155808.67191-1-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH] android: Rework request completion\n\tnotification","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":16799,"web_url":"https://patchwork.libcamera.org/comment/16799/","msgid":"<20210506085302.z6mjhuaezdr5qdvh@uno.localdomain>","date":"2021-05-06T08:53:02","subject":"Re: [libcamera-devel] [PATCH] 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 Niklas,\n\nOn Thu, May 06, 2021 at 10:31:10AM +0200, Niklas Söderlund wrote:\n> Hi Jacopo,\n>\n> Thanks for your work.\n>\n> On 2021-05-04 17:58:08 +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 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 <jacopo@jmondi.org>\n> > ---\n> >  src/android/camera_device.cpp | 128 ++++++++++++++++++++--------------\n> >  src/android/camera_device.h   |   3 +-\n> >  2 files changed, 76 insertions(+), 55 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index f1f26c775611..5588e99b6eba 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -2025,9 +2025,6 @@ 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<CameraMetadata> resultMetadata;\n> > -\n> >  \tdecltype(descriptors_)::node_type node;\n> >  \t{\n> >  \t\tstd::scoped_lock<std::mutex> lock(mutex_);\n> > @@ -2035,7 +2032,8 @@ void CameraDevice::requestComplete(Request *request)\n> >  \t\tif (it == descriptors_.end()) {\n> >  \t\t\tLOG(HAL, Fatal)\n> >  \t\t\t\t<< \"Unknown request: \" << request->cookie();\n> > -\t\t\tstatus = CAMERA3_BUFFER_STATUS_ERROR;\n> > +\t\t\tclose();\n> > +\t\t\tnotifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);\n> >  \t\t\treturn;\n> >  \t\t}\n> >\n> > @@ -2043,16 +2041,67 @@ void CameraDevice::requestComplete(Request *request)\n> >  \t}\n> >  \tCamera3RequestDescriptor &descriptor = node.mapped();\n> >\n> > +\t/*\n> > +\t * Prepare the capture result to call back the Android camera stack with.\n>\n> nit: Prepare the capture result for the Android camera stack ?\n>\n\nAck, it's simpler\n\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\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<uint64_t>(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<CameraMetadata> resultMetadata = getResultMetadata(descriptor);\n> > +\tif (!resultMetadata)\n> > +\t\tnotifyError(descriptor.frameNumber_, descriptor.buffers_[0].stream,\n> > +\t\t\t    CAMERA3_MSG_ERROR_RESULT);\n> > +\tcaptureResult.result = resultMetadata->get();\n>\n> Hum if !resultMetadata will not resultMetadata->get() give us a nullptr\n> exception here?\n>\n\nAccording to the std::unique_ptr<>::get() documentation:\n\n\"Returns a pointer to the managed object or nullptr if no object is owned.\"\n\nas getResultMetadata() returns nullptr on error, and the function\nsignature is\n        std::unique_ptr<CameraMetadata> CameraDevice::getResultMetadata()\n\nmy understanding is that the returned unique_ptr<> will own no object,\nhence get() should return nullptr. Or have I missed something about\nthe \"no object is owned\" vs \"a unique_ptr<> that owns nullptr\" ?\n\n> >\n> >  \t/* Handle any JPEG compression. */\n> >  \tfor (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> > @@ -2065,56 +2114,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<uint64_t>(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> > @@ -2136,23 +2156,23 @@ void CameraDevice::notifyShutter(uint32_t frameNumber, uint64_t timestamp)\n> >  \tcallbacks_->notify(callbacks_, &notify);\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_, &notify);\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 23457e47767a..8d5da8bc59e1 100644\n> > --- a/src/android/camera_device.h\n> > +++ b/src/android/camera_device.h\n> > @@ -101,7 +101,8 @@ private:\n> >  \tstd::tuple<uint32_t, uint32_t> calculateStaticMetadataSize();\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<CameraMetadata> requestTemplatePreview();\n> >  \tstd::unique_ptr<CameraMetadata> requestTemplateVideo();\n> >  \tlibcamera::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> --\n> Regards,\n> Niklas Söderlund","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 4C79FBF81D\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu,  6 May 2021 08:52:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 11AA968921;\n\tThu,  6 May 2021 10:52:20 +0200 (CEST)","from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B71E56891B\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu,  6 May 2021 10:52:18 +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 relay6-d.mail.gandi.net (Postfix) with ESMTPSA id 39C44C0013;\n\tThu,  6 May 2021 08:52:17 +0000 (UTC)"],"X-Originating-IP":"93.61.96.190","Date":"Thu, 6 May 2021 10:53:02 +0200","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Message-ID":"<20210506085302.z6mjhuaezdr5qdvh@uno.localdomain>","References":"<20210504155808.67191-1-jacopo@jmondi.org>\n\t<YJOpTqH5gvOIn057@oden.dyn.berto.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<YJOpTqH5gvOIn057@oden.dyn.berto.se>","Subject":"Re: [libcamera-devel] [PATCH] android: Rework request completion\n\tnotification","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":16800,"web_url":"https://patchwork.libcamera.org/comment/16800/","msgid":"<YJOwzbC7jTqU4ysi@oden.dyn.berto.se>","date":"2021-05-06T09:03:09","subject":"Re: [libcamera-devel] [PATCH] android: Rework request completion\n\tnotification","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nOn 2021-05-06 10:53:02 +0200, Jacopo Mondi wrote:\n\n<snip>\n\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<CameraMetadata> resultMetadata = getResultMetadata(descriptor);\n> > > +\tif (!resultMetadata)\n> > > +\t\tnotifyError(descriptor.frameNumber_, descriptor.buffers_[0].stream,\n> > > +\t\t\t    CAMERA3_MSG_ERROR_RESULT);\n> > > +\tcaptureResult.result = resultMetadata->get();\n> >\n> > Hum if !resultMetadata will not resultMetadata->get() give us a nullptr\n> > exception here?\n> >\n> \n> According to the std::unique_ptr<>::get() documentation:\n> \n> \"Returns a pointer to the managed object or nullptr if no object is owned.\"\n> \n> as getResultMetadata() returns nullptr on error, and the function\n> signature is\n>         std::unique_ptr<CameraMetadata> CameraDevice::getResultMetadata()\n> \n> my understanding is that the returned unique_ptr<> will own no object,\n> hence get() should return nullptr. Or have I missed something about\n> the \"no object is owned\" vs \"a unique_ptr<> that owns nullptr\" ?\n\nI share your understanding of std::unique_ptr<>::get() but that would be \n\n    resultMetadata.get()\n\nand not\n\n    resultMetadata->get()\n\nThe later calls CameraMetadata::get() which returns camera_metadata_t * \nwhich is the correct datatype for the captureResult.result right? So I \nthink the code calls the correct get(), only the manual protection for \nthe nullptr is missing.","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 A8390BDE7F\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu,  6 May 2021 09:03:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1DA7468921;\n\tThu,  6 May 2021 11:03:13 +0200 (CEST)","from mail-lj1-x233.google.com (mail-lj1-x233.google.com\n\t[IPv6:2a00:1450:4864:20::233])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6310D6891B\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu,  6 May 2021 11:03:11 +0200 (CEST)","by mail-lj1-x233.google.com with SMTP id v5so6085256ljg.12\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 06 May 2021 02:03:11 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\ty24sm635521ljm.106.2021.05.06.02.03.10\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 06 May 2021 02:03:10 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"qpRmfeLT\"; 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=5ZqZJYARZAEEv8RYVaJYP46ThaLVFQ+oFiCaBOxqwWg=;\n\tb=qpRmfeLTTw+wPdVCKWh4igBxbQrT4x/3yePzOYPblhOaS6c0hNZDL6JdsIKStbUYui\n\tYf5KVcnoMusUQtgLrm4o+wgL/N2/KOQLbGBq20d8gDcOp8kfDNpxHa7ivHgWbgQITfuj\n\tlAdHwEm1c+dvxMb6pLU2GKCrhhRWZxe0beDXmOiGWYxGtlushYKpSgCWrzP/nGpml/X+\n\tVD3YhBnvIM7cioyW4b3/dFR3w4CiCniCiJEZqjSpTD2DBOphXJ4LskMrajbIFmA9M2JK\n\tSizxvMT1OnMKl5OLIna1CAkbyLNG+YursrERgBjF9Xsc3VJuv+tCdMJESxJhnA9Wda+h\n\trSXA==","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=5ZqZJYARZAEEv8RYVaJYP46ThaLVFQ+oFiCaBOxqwWg=;\n\tb=p8ftIQWVjMDDz/TXSZkyvH8x2uPSykJJP+FC+9Ym3bn5jnRm+Rgt4epm7wNZ+X9w8a\n\tGalBV0VcDHZvNVAATqihTy3+9Mnjo5gu74ovH3386/qDcXBqtEXjK0BGtlysjbpFL61s\n\t3E/BvMSD+JwlAG6jYaL5iCOnZJPFN+M9S1HLcibXsLdA7qsZFrIAPUz4vhurA/oryooV\n\t+J9ZVHDlMmx92cU6LlKSIFkpgsvLDTXjM1czaZPYeFMQ0N3KxHVesFEoakd583nlO6DS\n\tSwSbZpF1/FkHb65Sywit6iZ9c6/Hou3jZSYS9l4xyXyCqxKHXg6xWKhT1P0q+LFTLFVW\n\tB+gA==","X-Gm-Message-State":"AOAM532q6tR3WqBX6rDd6eJfJFy6zZznrtxaz0jydc7aXe9cTOnjzl/e\n\t3dHDr7kdSOrfZx+DwWuVYTFjqw==","X-Google-Smtp-Source":"ABdhPJzMyiCli6MawxWGylTMLn0kpMouomw/JK7gd+2A9OPs2ll7cTnD7r1dSWUBTu/pfGZfU6SFCg==","X-Received":"by 2002:a2e:8e21:: with SMTP id r1mr2522431ljk.166.1620291790860;\n\tThu, 06 May 2021 02:03:10 -0700 (PDT)","Date":"Thu, 6 May 2021 11:03:09 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= <niklas.soderlund@ragnatech.se>","To":"Jacopo Mondi <jacopo@jmondi.org>","Message-ID":"<YJOwzbC7jTqU4ysi@oden.dyn.berto.se>","References":"<20210504155808.67191-1-jacopo@jmondi.org>\n\t<YJOpTqH5gvOIn057@oden.dyn.berto.se>\n\t<20210506085302.z6mjhuaezdr5qdvh@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210506085302.z6mjhuaezdr5qdvh@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH] android: Rework request completion\n\tnotification","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":16802,"web_url":"https://patchwork.libcamera.org/comment/16802/","msgid":"<20210506091937.wlmmugavgzz3clrx@uno.localdomain>","date":"2021-05-06T09:19:37","subject":"Re: [libcamera-devel] [PATCH] 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 Niklas,\n\nOn Thu, May 06, 2021 at 11:03:09AM +0200, Niklas Söderlund wrote:\n> Hi Jacopo,\n>\n> On 2021-05-06 10:53:02 +0200, Jacopo Mondi wrote:\n>\n> <snip>\n>\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<CameraMetadata> resultMetadata = getResultMetadata(descriptor);\n> > > > +\tif (!resultMetadata)\n> > > > +\t\tnotifyError(descriptor.frameNumber_, descriptor.buffers_[0].stream,\n> > > > +\t\t\t    CAMERA3_MSG_ERROR_RESULT);\n> > > > +\tcaptureResult.result = resultMetadata->get();\n> > >\n> > > Hum if !resultMetadata will not resultMetadata->get() give us a nullptr\n> > > exception here?\n> > >\n> >\n> > According to the std::unique_ptr<>::get() documentation:\n> >\n> > \"Returns a pointer to the managed object or nullptr if no object is owned.\"\n> >\n> > as getResultMetadata() returns nullptr on error, and the function\n> > signature is\n> >         std::unique_ptr<CameraMetadata> CameraDevice::getResultMetadata()\n> >\n> > my understanding is that the returned unique_ptr<> will own no object,\n> > hence get() should return nullptr. Or have I missed something about\n> > the \"no object is owned\" vs \"a unique_ptr<> that owns nullptr\" ?\n>\n> I share your understanding of std::unique_ptr<>::get() but that would be\n>\n>     resultMetadata.get()\n>\n> and not\n>\n>     resultMetadata->get()\n>\n> The later calls CameraMetadata::get() which returns camera_metadata_t *\n> which is the correct datatype for the captureResult.result right? So I\n> think the code calls the correct get(), only the manual protection for\n> the nullptr is missing.\n\nAAAA! very good catch! thanks, I'm a bit slow\n\nYes, unique_ptr<>::operator-> of course says\n        The behavior is undefined if get() == nullptr.\n\nso yes, I should put a check there. Sorry, I overlooked this! thanks\nfor spotting, I'll fix\n\nThanks\n   j\n\n>\n> --\n> Regards,\n> Niklas Söderlund","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 5DEAABDE7F\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu,  6 May 2021 09:18:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C1DD568920;\n\tThu,  6 May 2021 11:18:54 +0200 (CEST)","from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 545A268911\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu,  6 May 2021 11:18:54 +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 relay11.mail.gandi.net (Postfix) with ESMTPSA id C8ED610001A;\n\tThu,  6 May 2021 09:18:53 +0000 (UTC)"],"Date":"Thu, 6 May 2021 11:19:37 +0200","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Message-ID":"<20210506091937.wlmmugavgzz3clrx@uno.localdomain>","References":"<20210504155808.67191-1-jacopo@jmondi.org>\n\t<YJOpTqH5gvOIn057@oden.dyn.berto.se>\n\t<20210506085302.z6mjhuaezdr5qdvh@uno.localdomain>\n\t<YJOwzbC7jTqU4ysi@oden.dyn.berto.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<YJOwzbC7jTqU4ysi@oden.dyn.berto.se>","Subject":"Re: [libcamera-devel] [PATCH] android: Rework request completion\n\tnotification","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]