[{"id":20270,"web_url":"https://patchwork.libcamera.org/comment/20270/","msgid":"<20211018154814.ipdwzk22m6ihzce5@uno.localdomain>","date":"2021-10-18T15:48:14","subject":"Re: [libcamera-devel] [PATCH 03/11] android: camera_device: Build\n\tcapture_result dynamically","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Umang,\n\nOn Mon, Oct 18, 2021 at 06:59:15PM +0530, Umang Jain wrote:\n> From: Laurent Pinchart \n>\n> The camera3_capture_result_t is only needed to convey capture results to\n> the camera service through the process_capture_result() callback.\n> There's no need to store it in the Camera3RequestDescriptor. Build it\n> dynamically in CameraDevice::sendCaptureResults() instead.\n>\n> This requires storing the result metadata created in\n> CameraDevice::requestComplete() in the Camera3RequestDescriptor. A side\n> effect of this change is that the request metadata lifetime will match\n> the Camera3RequestDescriptor instead of being destroyed at the end of\n> requestComplete(). This will be needed to support asynchronous\n> post-processing, where the request completion will be signaled to the\n> camera service asynchronously from requestComplete().\n>\n> Signed-off-by: Laurent Pinchart \n> Signed-off-by: Umang Jain \n> ---\n> src/android/camera_device.cpp | 43 ++++++++++++++++-------------------\n> src/android/camera_request.h | 2 +-\n> 2 files changed, 21 insertions(+), 24 deletions(-)\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index b4ab5da1..c6ae8930 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -830,19 +830,11 @@ void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const\n> {\n> \tnotifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_REQUEST);\n>\n> -\tcamera3_capture_result_t &result = descriptor->captureResult_;\n> -\tresult.num_output_buffers = descriptor->buffers_.size();\n> -\tresult.frame_number = descriptor->frameNumber_;\n> -\tresult.partial_result = 0;\n> -\n> -\tstd::vector resultBuffers(result.num_output_buffers);\n> -\tfor (auto [i, buffer] : utils::enumerate(resultBuffers)) {\n> -\t\tbuffer = descriptor->buffers_[i];\n> -\t\tbuffer.release_fence = descriptor->buffers_[i].acquire_fence;\n> +\tfor (auto &buffer : descriptor->buffers_) {\n> +\t\tbuffer.release_fence = buffer.acquire_fence;\n> \t\tbuffer.acquire_fence = -1;\n> \t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> \t}\n> -\tresult.output_buffers = resultBuffers.data();\n>\n> \tdescriptor->status_ = Camera3RequestDescriptor::Status::Error;\n> }\n> @@ -1090,9 +1082,6 @@ void CameraDevice::requestComplete(Request *request)\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 = descriptor->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\tCameraStream *cameraStream =\n> \t\t\tstatic_cast(buffer.stream->priv);\n> @@ -1113,8 +1102,6 @@ void CameraDevice::requestComplete(Request *request)\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> @@ -1128,7 +1115,6 @@ void CameraDevice::requestComplete(Request *request)\n> \t\tnotifyError(descriptor->frameNumber_, nullptr,\n> \t\t\t CAMERA3_MSG_ERROR_REQUEST);\n>\n> -\t\tcaptureResult.partial_result = 0;\n> \t\tfor (camera3_stream_buffer_t &buffer : descriptor->buffers_) {\n> \t\t\t/*\n> \t\t\t * Signal to the framework it has to handle fences that\n> @@ -1165,12 +1151,12 @@ void CameraDevice::requestComplete(Request *request)\n> \t * Notify if the metadata generation has failed, but continue processing\n> \t * buffers and return an empty metadata pack.\n> \t */\n> -\tstd::unique_ptr resultMetadata = getResultMetadata(*descriptor);\n> -\tif (!resultMetadata) {\n> +\tdescriptor->resultMetadata_ = getResultMetadata(*descriptor);\n> +\tif (!descriptor->resultMetadata_) {\n> \t\tnotifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT);\n>\n> \t\t/* The camera framework expects an empty metadata pack on error. */\n> -\t\tresultMetadata = std::make_unique(0, 0);\n> +\t\tdescriptor->resultMetadata_ = std::make_unique(0, 0);\n\nSince we don't sendCaptureResult() and return here we get to\npost-processing with a (0, 0) metadata pack. I wonder if the\npost-processing code is robust enough to cope with that situation\n(even if I suspect I already know the answer :)\n\nIt is my understanding the situation is like this already, so it's\nfine, but maybe we should add a todo.\n\n> \t}\n>\n> \t/* Handle post-processing. */\n> @@ -1192,7 +1178,7 @@ void CameraDevice::requestComplete(Request *request)\n>\n> \t\tint ret = cameraStream->process(*src, buffer,\n> \t\t\t\t\t\tdescriptor->settings_,\n> -\t\t\t\t\t\tresultMetadata.get());\n> +\t\t\t\t\t\tdescriptor->resultMetadata_.get());\n> \t\t/*\n> \t\t * Return the FrameBuffer to the CameraStream now that we're\n> \t\t * done processing it.\n> @@ -1207,7 +1193,6 @@ void CameraDevice::requestComplete(Request *request)\n> \t\t}\n> \t}\n>\n> -\tcaptureResult.result = resultMetadata->get();\n> \tdescriptor->status_ = Camera3RequestDescriptor::Status::Success;\n> \tsendCaptureResults();\n> }\n> @@ -1225,8 +1210,20 @@ void CameraDevice::sendCaptureResults()\n> \t\t * impact on performance which should be measured.\n> \t\t */\n> \t\tlock.unlock();\n> -\t\tcallbacks_->process_capture_result(callbacks_,\n> -\t\t\t\t\t\t &descriptor->captureResult_);\n> +\n> +\t\tcamera3_capture_result_t captureResult = {};\n> +\n> +\t\tcaptureResult.frame_number = descriptor->frameNumber_;\n> +\t\tif (descriptor->resultMetadata_)\n\nDo we need this ? As resultMetadata_ is a unique_ptr<> it is\nconstructed owning nothing, and calling get() on it simnply return\nnullptr, which I think it's what we want.\n\nThanks\n j\n> +\t\t\tcaptureResult.result = descriptor->resultMetadata_->get();\n> +\t\tcaptureResult.num_output_buffers = descriptor->buffers_.size();\n> +\t\tcaptureResult.output_buffers = descriptor->buffers_.data();\n> +\n> +\t\tif (descriptor->status_ == Camera3RequestDescriptor::Status::Success)\n> +\t\t\tcaptureResult.partial_result = 1;\n> +\n> +\t\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n> +\n> \t\tlock.lock();\n> \t}\n> }\n> diff --git a/src/android/camera_request.h b/src/android/camera_request.h\n> index 79dfdb58..db13f624 100644\n> --- a/src/android/camera_request.h\n> +++ b/src/android/camera_request.h\n> @@ -40,8 +40,8 @@ public:\n> \tstd::vector> frameBuffers_;\n> \tCameraMetadata settings_;\n> \tstd::unique_ptr request_;\n> +\tstd::unique_ptr resultMetadata_;\n>\n> -\tcamera3_capture_result_t captureResult_ = {};\n> \tStatus status_ = Status::Pending;\n>\n> private:\n> --\n> 2.31.0\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 8DADEC323E\n\tfor ;\n\tMon, 18 Oct 2021 15:47:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EA64D68F59;\n\tMon, 18 Oct 2021 17:47:27 +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 60CAD68F56\n\tfor ;\n\tMon, 18 Oct 2021 17:47:26 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay4-d.mail.gandi.net (Postfix) with ESMTPSA id E425CE0006;\n\tMon, 18 Oct 2021 15:47:25 +0000 (UTC)"],"Date":"Mon, 18 Oct 2021 17:48:14 +0200","From":"Jacopo Mondi ","To":"Umang Jain ","Message-ID":"<20211018154814.ipdwzk22m6ihzce5@uno.localdomain>","References":"<20211018132923.476242-1-umang.jain@ideasonboard.com>\n\t<20211018132923.476242-4-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211018132923.476242-4-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 03/11] android: camera_device: Build\n\tcapture_result dynamically","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20272,"web_url":"https://patchwork.libcamera.org/comment/20272/","msgid":"<2e12e0d9-1d53-fda5-69fd-3701172ef28c@ideasonboard.com>","date":"2021-10-18T16:12:24","subject":"Re: [libcamera-devel] [PATCH 03/11] android: camera_device: Build\n\tcapture_result dynamically","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Jacopo\n\nOn 10/18/21 9:18 PM, Jacopo Mondi wrote:\n> Hi Umang,\n>\n> On Mon, Oct 18, 2021 at 06:59:15PM +0530, Umang Jain wrote:\n>> From: Laurent Pinchart \n>>\n>> The camera3_capture_result_t is only needed to convey capture results to\n>> the camera service through the process_capture_result() callback.\n>> There's no need to store it in the Camera3RequestDescriptor. Build it\n>> dynamically in CameraDevice::sendCaptureResults() instead.\n>>\n>> This requires storing the result metadata created in\n>> CameraDevice::requestComplete() in the Camera3RequestDescriptor. A side\n>> effect of this change is that the request metadata lifetime will match\n>> the Camera3RequestDescriptor instead of being destroyed at the end of\n>> requestComplete(). This will be needed to support asynchronous\n>> post-processing, where the request completion will be signaled to the\n>> camera service asynchronously from requestComplete().\n>>\n>> Signed-off-by: Laurent Pinchart \n>> Signed-off-by: Umang Jain \n>> ---\n>> src/android/camera_device.cpp | 43 ++++++++++++++++-------------------\n>> src/android/camera_request.h | 2 +-\n>> 2 files changed, 21 insertions(+), 24 deletions(-)\n>>\n>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>> index b4ab5da1..c6ae8930 100644\n>> --- a/src/android/camera_device.cpp\n>> +++ b/src/android/camera_device.cpp\n>> @@ -830,19 +830,11 @@ void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const\n>> {\n>> \tnotifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_REQUEST);\n>>\n>> -\tcamera3_capture_result_t &result = descriptor->captureResult_;\n>> -\tresult.num_output_buffers = descriptor->buffers_.size();\n>> -\tresult.frame_number = descriptor->frameNumber_;\n>> -\tresult.partial_result = 0;\n>> -\n>> -\tstd::vector resultBuffers(result.num_output_buffers);\n>> -\tfor (auto [i, buffer] : utils::enumerate(resultBuffers)) {\n>> -\t\tbuffer = descriptor->buffers_[i];\n>> -\t\tbuffer.release_fence = descriptor->buffers_[i].acquire_fence;\n>> +\tfor (auto &buffer : descriptor->buffers_) {\n>> +\t\tbuffer.release_fence = buffer.acquire_fence;\n>> \t\tbuffer.acquire_fence = -1;\n>> \t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n>> \t}\n>> -\tresult.output_buffers = resultBuffers.data();\n>>\n>> \tdescriptor->status_ = Camera3RequestDescriptor::Status::Error;\n>> }\n>> @@ -1090,9 +1082,6 @@ void CameraDevice::requestComplete(Request *request)\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 = descriptor->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\tCameraStream *cameraStream =\n>> \t\t\tstatic_cast(buffer.stream->priv);\n>> @@ -1113,8 +1102,6 @@ void CameraDevice::requestComplete(Request *request)\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>> @@ -1128,7 +1115,6 @@ void CameraDevice::requestComplete(Request *request)\n>> \t\tnotifyError(descriptor->frameNumber_, nullptr,\n>> \t\t\t CAMERA3_MSG_ERROR_REQUEST);\n>>\n>> -\t\tcaptureResult.partial_result = 0;\n>> \t\tfor (camera3_stream_buffer_t &buffer : descriptor->buffers_) {\n>> \t\t\t/*\n>> \t\t\t * Signal to the framework it has to handle fences that\n>> @@ -1165,12 +1151,12 @@ void CameraDevice::requestComplete(Request *request)\n>> \t * Notify if the metadata generation has failed, but continue processing\n>> \t * buffers and return an empty metadata pack.\n>> \t */\n>> -\tstd::unique_ptr resultMetadata = getResultMetadata(*descriptor);\n>> -\tif (!resultMetadata) {\n>> +\tdescriptor->resultMetadata_ = getResultMetadata(*descriptor);\n>> +\tif (!descriptor->resultMetadata_) {\n>> \t\tnotifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT);\n>>\n>> \t\t/* The camera framework expects an empty metadata pack on error. */\n>> -\t\tresultMetadata = std::make_unique(0, 0);\n>> +\t\tdescriptor->resultMetadata_ = std::make_unique(0, 0);\n> Since we don't sendCaptureResult() and return here we get to\n> post-processing with a (0, 0) metadata pack. I wonder if the\n> post-processing code is robust enough to cope with that situation\n> (even if I suspect I already know the answer :)\n>\n> It is my understanding the situation is like this already, so it's\n> fine, but maybe we should add a todo.\n\n\n\\todo stating what? The situation or the solution. I am not sure of a \npotential solution of this yet\n\n>\n>> \t}\n>>\n>> \t/* Handle post-processing. */\n>> @@ -1192,7 +1178,7 @@ void CameraDevice::requestComplete(Request *request)\n>>\n>> \t\tint ret = cameraStream->process(*src, buffer,\n>> \t\t\t\t\t\tdescriptor->settings_,\n>> -\t\t\t\t\t\tresultMetadata.get());\n>> +\t\t\t\t\t\tdescriptor->resultMetadata_.get());\n>> \t\t/*\n>> \t\t * Return the FrameBuffer to the CameraStream now that we're\n>> \t\t * done processing it.\n>> @@ -1207,7 +1193,6 @@ void CameraDevice::requestComplete(Request *request)\n>> \t\t}\n>> \t}\n>>\n>> -\tcaptureResult.result = resultMetadata->get();\n>> \tdescriptor->status_ = Camera3RequestDescriptor::Status::Success;\n>> \tsendCaptureResults();\n>> }\n>> @@ -1225,8 +1210,20 @@ void CameraDevice::sendCaptureResults()\n>> \t\t * impact on performance which should be measured.\n>> \t\t */\n>> \t\tlock.unlock();\n>> -\t\tcallbacks_->process_capture_result(callbacks_,\n>> -\t\t\t\t\t\t &descriptor->captureResult_);\n>> +\n>> +\t\tcamera3_capture_result_t captureResult = {};\n>> +\n>> +\t\tcaptureResult.frame_number = descriptor->frameNumber_;\n>> +\t\tif (descriptor->resultMetadata_)\n> Do we need this ? As resultMetadata_ is a unique_ptr<> it is\n> constructed owning nothing, and calling get() on it simnply return\n> nullptr, which I think it's what we want.\n\n\nYes we need this. There are two get() here\n\na) one of the unique_ptr one, .get()\n\nb) another is the descriptor->resultMetadata_->get()) one\n\nIf descriptor->resultMetadata_ is nullptr (initialized), ->get() on it,  \nwill segfault\n\nSo I found best to guard it with an if () block for now. Does it make sense?\n\n>\n> Thanks\n> j\n>> +\t\t\tcaptureResult.result = descriptor->resultMetadata_->get();\n>> +\t\tcaptureResult.num_output_buffers = descriptor->buffers_.size();\n>> +\t\tcaptureResult.output_buffers = descriptor->buffers_.data();\n>> +\n>> +\t\tif (descriptor->status_ == Camera3RequestDescriptor::Status::Success)\n>> +\t\t\tcaptureResult.partial_result = 1;\n>> +\n>> +\t\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n>> +\n>> \t\tlock.lock();\n>> \t}\n>> }\n>> diff --git a/src/android/camera_request.h b/src/android/camera_request.h\n>> index 79dfdb58..db13f624 100644\n>> --- a/src/android/camera_request.h\n>> +++ b/src/android/camera_request.h\n>> @@ -40,8 +40,8 @@ public:\n>> \tstd::vector> frameBuffers_;\n>> \tCameraMetadata settings_;\n>> \tstd::unique_ptr request_;\n>> +\tstd::unique_ptr resultMetadata_;\n>>\n>> -\tcamera3_capture_result_t captureResult_ = {};\n>> \tStatus status_ = Status::Pending;\n>>\n>> private:\n>> --\n>> 2.31.0\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 B2DCEC324C\n\tfor ;\n\tMon, 18 Oct 2021 16:12:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1B3FF68F5B;\n\tMon, 18 Oct 2021 18:12:32 +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 EEA9268F56\n\tfor ;\n\tMon, 18 Oct 2021 18:12:29 +0200 (CEST)","from [192.168.1.106] (unknown [103.238.109.14])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 005A68C6;\n\tMon, 18 Oct 2021 18:12:28 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ScAyNFhs\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634573549;\n\tbh=kWLrdNCINIWqIXdaFqKBfVTjA2Sj1wQrr1Qpl3KGUzU=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=ScAyNFhss2hWzNkIBNTxhkHy8URGSbrZ/HZ3rBa7YIlaqUz+u6k62TynqylCbbr+X\n\tma8LFljfOK67z3LFBWsi6cxy/Jl/vK+UrgZ/q4cv6xntiQj52NGpayO2dKnLxUuG48\n\taNJTESINDWy/fM3x6lGX8051UUIXyDOUIkjfBBgs=","To":"Jacopo Mondi ","References":"<20211018132923.476242-1-umang.jain@ideasonboard.com>\n\t<20211018132923.476242-4-umang.jain@ideasonboard.com>\n\t<20211018154814.ipdwzk22m6ihzce5@uno.localdomain>","From":"Umang Jain ","Message-ID":"<2e12e0d9-1d53-fda5-69fd-3701172ef28c@ideasonboard.com>","Date":"Mon, 18 Oct 2021 21:42:24 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<20211018154814.ipdwzk22m6ihzce5@uno.localdomain>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH 03/11] android: camera_device: Build\n\tcapture_result dynamically","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20280,"web_url":"https://patchwork.libcamera.org/comment/20280/","msgid":"<20211018164251.vo56yujq5tosfeup@uno.localdomain>","date":"2021-10-18T16:42:51","subject":"Re: [libcamera-devel] [PATCH 03/11] android: camera_device: Build\n\tcapture_result dynamically","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Umang,\n\nOn Mon, Oct 18, 2021 at 09:42:24PM +0530, Umang Jain wrote:\n> Hi Jacopo\n>\n> On 10/18/21 9:18 PM, Jacopo Mondi wrote:\n> > Hi Umang,\n> >\n> > On Mon, Oct 18, 2021 at 06:59:15PM +0530, Umang Jain wrote:\n> > > From: Laurent Pinchart \n> > >\n> > > The camera3_capture_result_t is only needed to convey capture results to\n> > > the camera service through the process_capture_result() callback.\n> > > There's no need to store it in the Camera3RequestDescriptor. Build it\n> > > dynamically in CameraDevice::sendCaptureResults() instead.\n> > >\n> > > This requires storing the result metadata created in\n> > > CameraDevice::requestComplete() in the Camera3RequestDescriptor. A side\n> > > effect of this change is that the request metadata lifetime will match\n> > > the Camera3RequestDescriptor instead of being destroyed at the end of\n> > > requestComplete(). This will be needed to support asynchronous\n> > > post-processing, where the request completion will be signaled to the\n> > > camera service asynchronously from requestComplete().\n> > >\n> > > Signed-off-by: Laurent Pinchart \n> > > Signed-off-by: Umang Jain \n> > > ---\n> > > src/android/camera_device.cpp | 43 ++++++++++++++++-------------------\n> > > src/android/camera_request.h | 2 +-\n> > > 2 files changed, 21 insertions(+), 24 deletions(-)\n> > >\n> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > index b4ab5da1..c6ae8930 100644\n> > > --- a/src/android/camera_device.cpp\n> > > +++ b/src/android/camera_device.cpp\n> > > @@ -830,19 +830,11 @@ void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const\n> > > {\n> > > \tnotifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_REQUEST);\n> > >\n> > > -\tcamera3_capture_result_t &result = descriptor->captureResult_;\n> > > -\tresult.num_output_buffers = descriptor->buffers_.size();\n> > > -\tresult.frame_number = descriptor->frameNumber_;\n> > > -\tresult.partial_result = 0;\n> > > -\n> > > -\tstd::vector resultBuffers(result.num_output_buffers);\n> > > -\tfor (auto [i, buffer] : utils::enumerate(resultBuffers)) {\n> > > -\t\tbuffer = descriptor->buffers_[i];\n> > > -\t\tbuffer.release_fence = descriptor->buffers_[i].acquire_fence;\n> > > +\tfor (auto &buffer : descriptor->buffers_) {\n> > > +\t\tbuffer.release_fence = buffer.acquire_fence;\n> > > \t\tbuffer.acquire_fence = -1;\n> > > \t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> > > \t}\n> > > -\tresult.output_buffers = resultBuffers.data();\n> > >\n> > > \tdescriptor->status_ = Camera3RequestDescriptor::Status::Error;\n> > > }\n> > > @@ -1090,9 +1082,6 @@ void CameraDevice::requestComplete(Request *request)\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 = descriptor->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\tCameraStream *cameraStream =\n> > > \t\t\tstatic_cast(buffer.stream->priv);\n> > > @@ -1113,8 +1102,6 @@ void CameraDevice::requestComplete(Request *request)\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> > > @@ -1128,7 +1115,6 @@ void CameraDevice::requestComplete(Request *request)\n> > > \t\tnotifyError(descriptor->frameNumber_, nullptr,\n> > > \t\t\t CAMERA3_MSG_ERROR_REQUEST);\n> > >\n> > > -\t\tcaptureResult.partial_result = 0;\n> > > \t\tfor (camera3_stream_buffer_t &buffer : descriptor->buffers_) {\n> > > \t\t\t/*\n> > > \t\t\t * Signal to the framework it has to handle fences that\n> > > @@ -1165,12 +1151,12 @@ void CameraDevice::requestComplete(Request *request)\n> > > \t * Notify if the metadata generation has failed, but continue processing\n> > > \t * buffers and return an empty metadata pack.\n> > > \t */\n> > > -\tstd::unique_ptr resultMetadata = getResultMetadata(*descriptor);\n> > > -\tif (!resultMetadata) {\n> > > +\tdescriptor->resultMetadata_ = getResultMetadata(*descriptor);\n> > > +\tif (!descriptor->resultMetadata_) {\n> > > \t\tnotifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT);\n> > >\n> > > \t\t/* The camera framework expects an empty metadata pack on error. */\n> > > -\t\tresultMetadata = std::make_unique(0, 0);\n> > > +\t\tdescriptor->resultMetadata_ = std::make_unique(0, 0);\n> > Since we don't sendCaptureResult() and return here we get to\n> > post-processing with a (0, 0) metadata pack. I wonder if the\n> > post-processing code is robust enough to cope with that situation\n> > (even if I suspect I already know the answer :)\n> >\n> > It is my understanding the situation is like this already, so it's\n> > fine, but maybe we should add a todo.\n>\n>\n> \\todo stating what? The situation or the solution. I am not sure of a\n> potential solution of this yet\n>\n\nI wish I had a solution. Just recording the potential issue, if you\nthink it's opportune.\n\n> >\n> > > \t}\n> > >\n> > > \t/* Handle post-processing. */\n> > > @@ -1192,7 +1178,7 @@ void CameraDevice::requestComplete(Request *request)\n> > >\n> > > \t\tint ret = cameraStream->process(*src, buffer,\n> > > \t\t\t\t\t\tdescriptor->settings_,\n> > > -\t\t\t\t\t\tresultMetadata.get());\n> > > +\t\t\t\t\t\tdescriptor->resultMetadata_.get());\n> > > \t\t/*\n> > > \t\t * Return the FrameBuffer to the CameraStream now that we're\n> > > \t\t * done processing it.\n> > > @@ -1207,7 +1193,6 @@ void CameraDevice::requestComplete(Request *request)\n> > > \t\t}\n> > > \t}\n> > >\n> > > -\tcaptureResult.result = resultMetadata->get();\n> > > \tdescriptor->status_ = Camera3RequestDescriptor::Status::Success;\n> > > \tsendCaptureResults();\n> > > }\n> > > @@ -1225,8 +1210,20 @@ void CameraDevice::sendCaptureResults()\n> > > \t\t * impact on performance which should be measured.\n> > > \t\t */\n> > > \t\tlock.unlock();\n> > > -\t\tcallbacks_->process_capture_result(callbacks_,\n> > > -\t\t\t\t\t\t &descriptor->captureResult_);\n> > > +\n> > > +\t\tcamera3_capture_result_t captureResult = {};\n> > > +\n> > > +\t\tcaptureResult.frame_number = descriptor->frameNumber_;\n> > > +\t\tif (descriptor->resultMetadata_)\n> > Do we need this ? As resultMetadata_ is a unique_ptr<> it is\n> > constructed owning nothing, and calling get() on it simnply return\n> > nullptr, which I think it's what we want.\n>\n>\n> Yes we need this. There are two get() here\n>\n> a) one of the unique_ptr one, .get()\n>\n> b) another is the descriptor->resultMetadata_->get()) one\n>\n> If descriptor->resultMetadata_ is nullptr (initialized), ->get() on it, \n> will segfault\n>\n> So I found best to guard it with an if () block for now. Does it make sense?\n>\n\nSorry, I confused unique_ptr<>.get() with CameraMetadata::get().\n\n> >\n> > Thanks\n> > j\n> > > +\t\t\tcaptureResult.result = descriptor->resultMetadata_->get();\n> > > +\t\tcaptureResult.num_output_buffers = descriptor->buffers_.size();\n> > > +\t\tcaptureResult.output_buffers = descriptor->buffers_.data();\n> > > +\n> > > +\t\tif (descriptor->status_ == Camera3RequestDescriptor::Status::Success)\n> > > +\t\t\tcaptureResult.partial_result = 1;\n> > > +\n> > > +\t\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n> > > +\n> > > \t\tlock.lock();\n> > > \t}\n> > > }\n> > > diff --git a/src/android/camera_request.h b/src/android/camera_request.h\n> > > index 79dfdb58..db13f624 100644\n> > > --- a/src/android/camera_request.h\n> > > +++ b/src/android/camera_request.h\n> > > @@ -40,8 +40,8 @@ public:\n> > > \tstd::vector> frameBuffers_;\n> > > \tCameraMetadata settings_;\n> > > \tstd::unique_ptr request_;\n> > > +\tstd::unique_ptr resultMetadata_;\n> > >\n> > > -\tcamera3_capture_result_t captureResult_ = {};\n> > > \tStatus status_ = Status::Pending;\n> > >\n> > > private:\n> > > --\n> > > 2.31.0\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 AF979C323E\n\tfor ;\n\tMon, 18 Oct 2021 16:42:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EF05568F59;\n\tMon, 18 Oct 2021 18:42:03 +0200 (CEST)","from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[217.70.183.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5282B68F56\n\tfor ;\n\tMon, 18 Oct 2021 18:42:03 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay3-d.mail.gandi.net (Postfix) with ESMTPSA id D448C60006;\n\tMon, 18 Oct 2021 16:42:02 +0000 (UTC)"],"Date":"Mon, 18 Oct 2021 18:42:51 +0200","From":"Jacopo Mondi ","To":"Umang Jain ","Message-ID":"<20211018164251.vo56yujq5tosfeup@uno.localdomain>","References":"<20211018132923.476242-1-umang.jain@ideasonboard.com>\n\t<20211018132923.476242-4-umang.jain@ideasonboard.com>\n\t<20211018154814.ipdwzk22m6ihzce5@uno.localdomain>\n\t<2e12e0d9-1d53-fda5-69fd-3701172ef28c@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<2e12e0d9-1d53-fda5-69fd-3701172ef28c@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 03/11] android: camera_device: Build\n\tcapture_result dynamically","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20283,"web_url":"https://patchwork.libcamera.org/comment/20283/","msgid":"","date":"2021-10-18T17:39:20","subject":"Re: [libcamera-devel] [PATCH 03/11] android: camera_device: Build\n\tcapture_result dynamically","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, Oct 18, 2021 at 06:42:51PM +0200, Jacopo Mondi wrote:\n> Hi Umang,\n> \n> On Mon, Oct 18, 2021 at 09:42:24PM +0530, Umang Jain wrote:\n> > Hi Jacopo\n> >\n> > On 10/18/21 9:18 PM, Jacopo Mondi wrote:\n> > > Hi Umang,\n> > >\n> > > On Mon, Oct 18, 2021 at 06:59:15PM +0530, Umang Jain wrote:\n> > > > From: Laurent Pinchart \n> > > >\n> > > > The camera3_capture_result_t is only needed to convey capture results to\n> > > > the camera service through the process_capture_result() callback.\n> > > > There's no need to store it in the Camera3RequestDescriptor. Build it\n> > > > dynamically in CameraDevice::sendCaptureResults() instead.\n> > > >\n> > > > This requires storing the result metadata created in\n> > > > CameraDevice::requestComplete() in the Camera3RequestDescriptor. A side\n> > > > effect of this change is that the request metadata lifetime will match\n> > > > the Camera3RequestDescriptor instead of being destroyed at the end of\n> > > > requestComplete(). This will be needed to support asynchronous\n> > > > post-processing, where the request completion will be signaled to the\n> > > > camera service asynchronously from requestComplete().\n> > > >\n> > > > Signed-off-by: Laurent Pinchart \n> > > > Signed-off-by: Umang Jain \n> > > > ---\n> > > > src/android/camera_device.cpp | 43 ++++++++++++++++-------------------\n> > > > src/android/camera_request.h | 2 +-\n> > > > 2 files changed, 21 insertions(+), 24 deletions(-)\n> > > >\n> > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > index b4ab5da1..c6ae8930 100644\n> > > > --- a/src/android/camera_device.cpp\n> > > > +++ b/src/android/camera_device.cpp\n> > > > @@ -830,19 +830,11 @@ void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const\n> > > > {\n> > > > \tnotifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_REQUEST);\n> > > >\n> > > > -\tcamera3_capture_result_t &result = descriptor->captureResult_;\n> > > > -\tresult.num_output_buffers = descriptor->buffers_.size();\n> > > > -\tresult.frame_number = descriptor->frameNumber_;\n> > > > -\tresult.partial_result = 0;\n> > > > -\n> > > > -\tstd::vector resultBuffers(result.num_output_buffers);\n> > > > -\tfor (auto [i, buffer] : utils::enumerate(resultBuffers)) {\n> > > > -\t\tbuffer = descriptor->buffers_[i];\n> > > > -\t\tbuffer.release_fence = descriptor->buffers_[i].acquire_fence;\n> > > > +\tfor (auto &buffer : descriptor->buffers_) {\n> > > > +\t\tbuffer.release_fence = buffer.acquire_fence;\n> > > > \t\tbuffer.acquire_fence = -1;\n> > > > \t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> > > > \t}\n> > > > -\tresult.output_buffers = resultBuffers.data();\n> > > >\n> > > > \tdescriptor->status_ = Camera3RequestDescriptor::Status::Error;\n> > > > }\n> > > > @@ -1090,9 +1082,6 @@ void CameraDevice::requestComplete(Request *request)\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 = descriptor->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\tCameraStream *cameraStream =\n> > > > \t\t\tstatic_cast(buffer.stream->priv);\n> > > > @@ -1113,8 +1102,6 @@ void CameraDevice::requestComplete(Request *request)\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> > > > @@ -1128,7 +1115,6 @@ void CameraDevice::requestComplete(Request *request)\n> > > > \t\tnotifyError(descriptor->frameNumber_, nullptr,\n> > > > \t\t\t CAMERA3_MSG_ERROR_REQUEST);\n> > > >\n> > > > -\t\tcaptureResult.partial_result = 0;\n> > > > \t\tfor (camera3_stream_buffer_t &buffer : descriptor->buffers_) {\n> > > > \t\t\t/*\n> > > > \t\t\t * Signal to the framework it has to handle fences that\n> > > > @@ -1165,12 +1151,12 @@ void CameraDevice::requestComplete(Request *request)\n> > > > \t * Notify if the metadata generation has failed, but continue processing\n> > > > \t * buffers and return an empty metadata pack.\n> > > > \t */\n> > > > -\tstd::unique_ptr resultMetadata = getResultMetadata(*descriptor);\n> > > > -\tif (!resultMetadata) {\n> > > > +\tdescriptor->resultMetadata_ = getResultMetadata(*descriptor);\n> > > > +\tif (!descriptor->resultMetadata_) {\n> > > > \t\tnotifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT);\n> > > >\n> > > > \t\t/* The camera framework expects an empty metadata pack on error. */\n> > > > -\t\tresultMetadata = std::make_unique(0, 0);\n> > > > +\t\tdescriptor->resultMetadata_ = std::make_unique(0, 0);\n> > >\n> > > Since we don't sendCaptureResult() and return here we get to\n> > > post-processing with a (0, 0) metadata pack. I wonder if the\n> > > post-processing code is robust enough to cope with that situation\n> > > (even if I suspect I already know the answer :)\n> > >\n> > > It is my understanding the situation is like this already, so it's\n> > > fine, but maybe we should add a todo.\n> >\n> > \\todo stating what? The situation or the solution. I am not sure of a\n> > potential solution of this yet\n> \n> I wish I had a solution. Just recording the potential issue, if you\n> think it's opportune.\n\n\t* \\todo Check that the post-processor code handles this situation\n\t* correctly.\n\n> > > > \t}\n> > > >\n> > > > \t/* Handle post-processing. */\n> > > > @@ -1192,7 +1178,7 @@ void CameraDevice::requestComplete(Request *request)\n> > > >\n> > > > \t\tint ret = cameraStream->process(*src, buffer,\n> > > > \t\t\t\t\t\tdescriptor->settings_,\n> > > > -\t\t\t\t\t\tresultMetadata.get());\n> > > > +\t\t\t\t\t\tdescriptor->resultMetadata_.get());\n> > > > \t\t/*\n> > > > \t\t * Return the FrameBuffer to the CameraStream now that we're\n> > > > \t\t * done processing it.\n> > > > @@ -1207,7 +1193,6 @@ void CameraDevice::requestComplete(Request *request)\n> > > > \t\t}\n> > > > \t}\n> > > >\n> > > > -\tcaptureResult.result = resultMetadata->get();\n> > > > \tdescriptor->status_ = Camera3RequestDescriptor::Status::Success;\n> > > > \tsendCaptureResults();\n> > > > }\n> > > > @@ -1225,8 +1210,20 @@ void CameraDevice::sendCaptureResults()\n> > > > \t\t * impact on performance which should be measured.\n> > > > \t\t */\n> > > > \t\tlock.unlock();\n> > > > -\t\tcallbacks_->process_capture_result(callbacks_,\n> > > > -\t\t\t\t\t\t &descriptor->captureResult_);\n> > > > +\n> > > > +\t\tcamera3_capture_result_t captureResult = {};\n> > > > +\n> > > > +\t\tcaptureResult.frame_number = descriptor->frameNumber_;\n> > > > +\t\tif (descriptor->resultMetadata_)\n> > >\n> > > Do we need this ? As resultMetadata_ is a unique_ptr<> it is\n> > > constructed owning nothing, and calling get() on it simnply return\n> > > nullptr, which I think it's what we want.\n> >\n> > Yes we need this. There are two get() here\n> >\n> > a) one of the unique_ptr one, .get()\n> >\n> > b) another is the descriptor->resultMetadata_->get()) one\n> >\n> > If descriptor->resultMetadata_ is nullptr (initialized), ->get() on it, \n> > will segfault\n> >\n> > So I found best to guard it with an if () block for now. Does it make sense?\n> \n> Sorry, I confused unique_ptr<>.get() with CameraMetadata::get().\n\nRenaming CameraMetadata::get() to getMetadata() may be a good idea to\navoid future confusion.\n\n> > > > +\t\t\tcaptureResult.result = descriptor->resultMetadata_->get();\n> > > > +\t\tcaptureResult.num_output_buffers = descriptor->buffers_.size();\n> > > > +\t\tcaptureResult.output_buffers = descriptor->buffers_.data();\n> > > > +\n> > > > +\t\tif (descriptor->status_ == Camera3RequestDescriptor::Status::Success)\n> > > > +\t\t\tcaptureResult.partial_result = 1;\n> > > > +\n> > > > +\t\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n> > > > +\n> > > > \t\tlock.lock();\n> > > > \t}\n> > > > }\n> > > > diff --git a/src/android/camera_request.h b/src/android/camera_request.h\n> > > > index 79dfdb58..db13f624 100644\n> > > > --- a/src/android/camera_request.h\n> > > > +++ b/src/android/camera_request.h\n> > > > @@ -40,8 +40,8 @@ public:\n> > > > \tstd::vector> frameBuffers_;\n> > > > \tCameraMetadata settings_;\n> > > > \tstd::unique_ptr request_;\n> > > > +\tstd::unique_ptr resultMetadata_;\n> > > >\n> > > > -\tcamera3_capture_result_t captureResult_ = {};\n> > > > \tStatus status_ = Status::Pending;\n> > > >\n> > > > private:","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 4E32EC323E\n\tfor ;\n\tMon, 18 Oct 2021 17:39:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CC84768F59;\n\tMon, 18 Oct 2021 19:39:40 +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 8F49D68F56\n\tfor ;\n\tMon, 18 Oct 2021 19:39: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 EAFEA8C6;\n\tMon, 18 Oct 2021 19:39: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=\"DNQlq8xa\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634578778;\n\tbh=V21QHw7Q63HurAFHupX4Ud2jIQwWt4BnEiGOpKmRnBI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=DNQlq8xaxoKCX7qaD7TpuAA89J2FM1m0kJPtKaC1Xe/gSy4Dwu9QB1gEvVl8pujJQ\n\tuA/iNWM9agRMQrVG7QwQQFnVnGCQX5AymjPhcQ5JIE0M7X94zFuPGgiJlokAdSTZx7\n\tZK2R1ETubNO4MA5bxIiKtli0lX3IcjqJwafbcRJ0=","Date":"Mon, 18 Oct 2021 20:39:20 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20211018132923.476242-1-umang.jain@ideasonboard.com>\n\t<20211018132923.476242-4-umang.jain@ideasonboard.com>\n\t<20211018154814.ipdwzk22m6ihzce5@uno.localdomain>\n\t<2e12e0d9-1d53-fda5-69fd-3701172ef28c@ideasonboard.com>\n\t<20211018164251.vo56yujq5tosfeup@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20211018164251.vo56yujq5tosfeup@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 03/11] android: camera_device: Build\n\tcapture_result dynamically","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20289,"web_url":"https://patchwork.libcamera.org/comment/20289/","msgid":"","date":"2021-10-19T04:41:04","subject":"Re: [libcamera-devel] [PATCH 03/11] android: camera_device: Build\n\tcapture_result dynamically","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Umang and Laurent, thank you for the patch.\n\nOn Tue, Oct 19, 2021 at 2:39 AM Laurent Pinchart\n wrote:\n>\n> On Mon, Oct 18, 2021 at 06:42:51PM +0200, Jacopo Mondi wrote:\n> > Hi Umang,\n> >\n> > On Mon, Oct 18, 2021 at 09:42:24PM +0530, Umang Jain wrote:\n> > > Hi Jacopo\n> > >\n> > > On 10/18/21 9:18 PM, Jacopo Mondi wrote:\n> > > > Hi Umang,\n> > > >\n> > > > On Mon, Oct 18, 2021 at 06:59:15PM +0530, Umang Jain wrote:\n> > > > > From: Laurent Pinchart \n> > > > >\n> > > > > The camera3_capture_result_t is only needed to convey capture results to\n> > > > > the camera service through the process_capture_result() callback.\n> > > > > There's no need to store it in the Camera3RequestDescriptor. Build it\n> > > > > dynamically in CameraDevice::sendCaptureResults() instead.\n> > > > >\n> > > > > This requires storing the result metadata created in\n> > > > > CameraDevice::requestComplete() in the Camera3RequestDescriptor. A side\n> > > > > effect of this change is that the request metadata lifetime will match\n> > > > > the Camera3RequestDescriptor instead of being destroyed at the end of\n> > > > > requestComplete(). This will be needed to support asynchronous\n> > > > > post-processing, where the request completion will be signaled to the\n> > > > > camera service asynchronously from requestComplete().\n> > > > >\n> > > > > Signed-off-by: Laurent Pinchart \n> > > > > Signed-off-by: Umang Jain \n\nReviewed-by: Hirokazu Honda \n\n> > > > > ---\n> > > > > src/android/camera_device.cpp | 43 ++++++++++++++++-------------------\n> > > > > src/android/camera_request.h | 2 +-\n> > > > > 2 files changed, 21 insertions(+), 24 deletions(-)\n> > > > >\n> > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > > index b4ab5da1..c6ae8930 100644\n> > > > > --- a/src/android/camera_device.cpp\n> > > > > +++ b/src/android/camera_device.cpp\n> > > > > @@ -830,19 +830,11 @@ void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const\n> > > > > {\n> > > > > notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_REQUEST);\n> > > > >\n> > > > > - camera3_capture_result_t &result = descriptor->captureResult_;\n> > > > > - result.num_output_buffers = descriptor->buffers_.size();\n> > > > > - result.frame_number = descriptor->frameNumber_;\n> > > > > - result.partial_result = 0;\n> > > > > -\n> > > > > - std::vector resultBuffers(result.num_output_buffers);\n> > > > > - for (auto [i, buffer] : utils::enumerate(resultBuffers)) {\n> > > > > - buffer = descriptor->buffers_[i];\n> > > > > - buffer.release_fence = descriptor->buffers_[i].acquire_fence;\n> > > > > + for (auto &buffer : descriptor->buffers_) {\n> > > > > + buffer.release_fence = buffer.acquire_fence;\n> > > > > buffer.acquire_fence = -1;\n> > > > > buffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> > > > > }\n> > > > > - result.output_buffers = resultBuffers.data();\n> > > > >\n> > > > > descriptor->status_ = Camera3RequestDescriptor::Status::Error;\n> > > > > }\n> > > > > @@ -1090,9 +1082,6 @@ void CameraDevice::requestComplete(Request *request)\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 = descriptor->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> > > > > CameraStream *cameraStream =\n> > > > > static_cast(buffer.stream->priv);\n> > > > > @@ -1113,8 +1102,6 @@ void CameraDevice::requestComplete(Request *request)\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 error\n> > > > > @@ -1128,7 +1115,6 @@ void CameraDevice::requestComplete(Request *request)\n> > > > > notifyError(descriptor->frameNumber_, nullptr,\n> > > > > CAMERA3_MSG_ERROR_REQUEST);\n> > > > >\n> > > > > - captureResult.partial_result = 0;\n> > > > > for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {\n> > > > > /*\n> > > > > * Signal to the framework it has to handle fences that\n> > > > > @@ -1165,12 +1151,12 @@ void CameraDevice::requestComplete(Request *request)\n> > > > > * Notify if the metadata generation has failed, but continue processing\n> > > > > * buffers and return an empty metadata pack.\n> > > > > */\n> > > > > - std::unique_ptr resultMetadata = getResultMetadata(*descriptor);\n> > > > > - if (!resultMetadata) {\n> > > > > + descriptor->resultMetadata_ = getResultMetadata(*descriptor);\n> > > > > + if (!descriptor->resultMetadata_) {\n> > > > > notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT);\n> > > > >\n> > > > > /* The camera framework expects an empty metadata pack on error. */\n> > > > > - resultMetadata = std::make_unique(0, 0);\n> > > > > + descriptor->resultMetadata_ = std::make_unique(0, 0);\n> > > >\n> > > > Since we don't sendCaptureResult() and return here we get to\n> > > > post-processing with a (0, 0) metadata pack. I wonder if the\n> > > > post-processing code is robust enough to cope with that situation\n> > > > (even if I suspect I already know the answer :)\n> > > >\n> > > > It is my understanding the situation is like this already, so it's\n> > > > fine, but maybe we should add a todo.\n> > >\n> > > \\todo stating what? The situation or the solution. I am not sure of a\n> > > potential solution of this yet\n> >\n> > I wish I had a solution. Just recording the potential issue, if you\n> > think it's opportune.\n>\n> * \\todo Check that the post-processor code handles this situation\n> * correctly.\n>\n> > > > > }\n> > > > >\n> > > > > /* Handle post-processing. */\n> > > > > @@ -1192,7 +1178,7 @@ void CameraDevice::requestComplete(Request *request)\n> > > > >\n> > > > > int ret = cameraStream->process(*src, buffer,\n> > > > > descriptor->settings_,\n> > > > > - resultMetadata.get());\n> > > > > + descriptor->resultMetadata_.get());\n> > > > > /*\n> > > > > * Return the FrameBuffer to the CameraStream now that we're\n> > > > > * done processing it.\n> > > > > @@ -1207,7 +1193,6 @@ void CameraDevice::requestComplete(Request *request)\n> > > > > }\n> > > > > }\n> > > > >\n> > > > > - captureResult.result = resultMetadata->get();\n> > > > > descriptor->status_ = Camera3RequestDescriptor::Status::Success;\n> > > > > sendCaptureResults();\n> > > > > }\n> > > > > @@ -1225,8 +1210,20 @@ void CameraDevice::sendCaptureResults()\n> > > > > * impact on performance which should be measured.\n> > > > > */\n> > > > > lock.unlock();\n> > > > > - callbacks_->process_capture_result(callbacks_,\n> > > > > - &descriptor->captureResult_);\n> > > > > +\n> > > > > + camera3_capture_result_t captureResult = {};\n> > > > > +\n> > > > > + captureResult.frame_number = descriptor->frameNumber_;\n> > > > > + if (descriptor->resultMetadata_)\n> > > >\n> > > > Do we need this ? As resultMetadata_ is a unique_ptr<> it is\n> > > > constructed owning nothing, and calling get() on it simnply return\n> > > > nullptr, which I think it's what we want.\n> > >\n> > > Yes we need this. There are two get() here\n> > >\n> > > a) one of the unique_ptr one, .get()\n> > >\n> > > b) another is the descriptor->resultMetadata_->get()) one\n> > >\n> > > If descriptor->resultMetadata_ is nullptr (initialized), ->get() on it,\n> > > will segfault\n> > >\n> > > So I found best to guard it with an if () block for now. Does it make sense?\n> >\n> > Sorry, I confused unique_ptr<>.get() with CameraMetadata::get().\n>\n> Renaming CameraMetadata::get() to getMetadata() may be a good idea to\n> avoid future confusion.\n>\n> > > > > + captureResult.result = descriptor->resultMetadata_->get();\n> > > > > + captureResult.num_output_buffers = descriptor->buffers_.size();\n> > > > > + captureResult.output_buffers = descriptor->buffers_.data();\n> > > > > +\n> > > > > + if (descriptor->status_ == Camera3RequestDescriptor::Status::Success)\n> > > > > + captureResult.partial_result = 1;\n> > > > > +\n> > > > > + callbacks_->process_capture_result(callbacks_, &captureResult);\n> > > > > +\n> > > > > lock.lock();\n> > > > > }\n> > > > > }\n> > > > > diff --git a/src/android/camera_request.h b/src/android/camera_request.h\n> > > > > index 79dfdb58..db13f624 100644\n> > > > > --- a/src/android/camera_request.h\n> > > > > +++ b/src/android/camera_request.h\n> > > > > @@ -40,8 +40,8 @@ public:\n> > > > > std::vector> frameBuffers_;\n> > > > > CameraMetadata settings_;\n> > > > > std::unique_ptr request_;\n> > > > > + std::unique_ptr resultMetadata_;\n> > > > >\n> > > > > - camera3_capture_result_t captureResult_ = {};\n> > > > > Status status_ = Status::Pending;\n> > > > >\n> > > > > private:\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 B5AD0C324C\n\tfor ;\n\tTue, 19 Oct 2021 04:41:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2B9BF68F58;\n\tTue, 19 Oct 2021 06:41:18 +0200 (CEST)","from mail-ed1-x52c.google.com (mail-ed1-x52c.google.com\n\t[IPv6:2a00:1450:4864:20::52c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 79EB760126\n\tfor ;\n\tTue, 19 Oct 2021 06:41:16 +0200 (CEST)","by mail-ed1-x52c.google.com with SMTP id a25so7720322edx.8\n\tfor ;\n\tMon, 18 Oct 2021 21:41:16 -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=\"Nx8OKj5C\"; 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=8HdEIH6ILNVxoBygjkIpOCnL5HF814tsLPegVzRHQKA=;\n\tb=Nx8OKj5C4qaCcJP+Tr6RpFsFwed2VE7oqqWyD2OipMjBfJBB4RzTAChyLYuU2cfWWE\n\tnTjUGn8K1JletaDLSE2ceRpcy5MTfIP+XdRCwkw8Cjl588O3cy/2d2BdBTO2E7Htiuvr\n\tEYsxVcb0hfyE/nnrCBBB5syiiD/3qXr7rlZT8=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=8HdEIH6ILNVxoBygjkIpOCnL5HF814tsLPegVzRHQKA=;\n\tb=Qd5y/GNGVGWTQIw+AWwP//xqAAjA7ypYa7ZENYkOgYP5IINfJL9oReAay70lG9eT7+\n\txMwHQwc5jsGRkfCquWox0D+h+qtQCQspzDVel/STIF47uzJT9M3t+CTrvI2OCmYycQBR\n\tzPZMCV402yW2AeN6x7tk0n2D0hdgyHFoYty29uOt9XIUv1yqKOoEa3/l8J3K+UWbpQcL\n\tqtxSs4C1XMr0LZgbzqg+r9Bd1MRtwBY1tlc3xMTqHpl3OMPNxcqK225mMoQ9xebYTTLZ\n\tzvdhvRPv6fwCwK+F+XtmcbKppYDweexmoFan8BlEP3jTvgjXyXxHFnUsomJrqSmNIyx+\n\tvbPw==","X-Gm-Message-State":"AOAM532kmOpSGs+l2vj04q3TGDk06A+Vx+HAOXkiryHvx5N4xjTQiA6q\n\tvgiICDxMvZaIa5sEr681dPtzJyOcNm+XWQZf8l4LfOiZfjo=","X-Google-Smtp-Source":"ABdhPJwESiCm59Ja2j1EQnB1rIAGnauex8fCgL9xr/m5+pMvp/ua0ac+nJsgAaxXcsbXDlvNAePb3pQGNfjL7JoujFg=","X-Received":"by 2002:a17:906:4fd6:: with SMTP id\n\ti22mr34663162ejw.92.1634618475992; \n\tMon, 18 Oct 2021 21:41:15 -0700 (PDT)","MIME-Version":"1.0","References":"<20211018132923.476242-1-umang.jain@ideasonboard.com>\n\t<20211018132923.476242-4-umang.jain@ideasonboard.com>\n\t<20211018154814.ipdwzk22m6ihzce5@uno.localdomain>\n\t<2e12e0d9-1d53-fda5-69fd-3701172ef28c@ideasonboard.com>\n\t<20211018164251.vo56yujq5tosfeup@uno.localdomain>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Tue, 19 Oct 2021 13:41:04 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 03/11] android: camera_device: Build\n\tcapture_result dynamically","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]