[{"id":19627,"web_url":"https://patchwork.libcamera.org/comment/19627/","msgid":"","date":"2021-09-13T00:59:31","subject":"Re: [libcamera-devel] [PATCH v2 6/9] android: camera_device: Add a\n\tqueue for sending capture results","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nThank you for the patch.\n\nOn Fri, Sep 10, 2021 at 12:36:35PM +0530, Umang Jain wrote:\n> When a camera capture request completes, the next step is to send the\n> capture results to the framework via process_capture_results(). All\n> the capture associated result metadata is prepared and populated. If\n> any post-processing is required, it will also take place in the same\n> thread (which can be blocking for a subtaintial amount of time) before\n> the results can be sent back to the framework.\n> \n> Subsequent patches will move the post-processing to run in a separate\n> thread. In order to do so, there is few bits of groundwork which this\n> patch entails. Mainly, we need to preserve the order of sending\n> the capture results back in which they were queued by the framework\n> to the HAL (i.e. the order is sequential). Hence, we need to add a queue\n> in which capture results can be queued with context.\n> \n> As per this patch, the post-processor still runs synchronously as\n> before, but it will queue up the current capture results with context.\n> Later down the line, the capture results are dequeud in order and\n> sent back to the framework via sendQueuedCaptureResults(). If the queue\n> is empty or unused, the current behaviour is to skip the queue and\n> send capture results directly.\n> \n> The context is preserved using Camera3RequestDescriptor utility\n> structure. It has been expanded accordingly to address the needs of\n> the functionality.\n> ---\n> Check if we can drop unique_ptr to camera3descriptor\n> ---\n> \n> Signed-off-by: Umang Jain \n> ---\n> src/android/camera_device.cpp | 106 ++++++++++++++++++++++++++++++----\n> src/android/camera_device.h | 22 +++++++\n> src/android/camera_stream.cpp | 2 +\n> 3 files changed, 119 insertions(+), 11 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 84549d04..7f04d225 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -240,6 +240,8 @@ Camera3RequestDescriptor::Camera3RequestDescriptor(\n> \t/* Clone the controls associated with the camera3 request. */\n> \tsettings_ = CameraMetadata(camera3Request->settings);\n> \n> +\tinternalBuffer_ = nullptr;\n> +\n> \t/*\n> \t * Create the CaptureRequest, stored as a unique_ptr<> to tie its\n> \t * lifetime to the descriptor.\n> @@ -989,6 +991,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \t\t\t * once it has been processed.\n> \t\t\t */\n> \t\t\tbuffer = cameraStream->getBuffer();\n> +\t\t\tdescriptor.internalBuffer_ = buffer;\n> \t\t\tLOG(HAL, Debug) << ss.str() << \" (internal)\";\n> \t\t\tbreak;\n> \t\t}\n> @@ -1148,26 +1151,107 @@ void CameraDevice::requestComplete(Request *request)\n> \t\t\tcontinue;\n> \t\t}\n> \n> +\n> +\t\t/*\n> +\t\t * Save the current context of capture result and queue the\n> +\t\t * descriptor before processing the camera stream.\n> +\t\t *\n> +\t\t * When the processing completes, the descriptor will be\n> +\t\t * dequeued and capture results will be sent to the framework.\n> +\t\t */\n> +\t\tdescriptor.status_ = Camera3RequestDescriptor::Pending;\n\nThe status should also be initialized to Pending in the\nCamera3RequestDescriptor constructor.\n\n> +\t\tdescriptor.resultMetadata_ = std::move(resultMetadata);\n\nLet's move the resultMetadata to the descriptor right when creating it,\nit will be cleaner.\n\n> +\t\tdescriptor.captureResult_ = captureResult;\n\nThis causes a copy of a fairly large structure. You can prevent this by\nreplacing\n\n\tcamera3_capture_result_t captureResult = {};\n\nabove with\n\n\tcamera3_capture_result_t &captureResult = descriptor.captureResult_;\n\tcaptureResult = {};\n\nor better, in the definition of Camera3RequestDescriptor, replace\n\n\tcamera3_capture_result_t captureResult_;\n\nwith\n\n\tcamera3_capture_result_t captureResult_ = {};\n\nand drop the initialization in this function. This will ensure that the\ncapture results are correctly initialized as soon as\nCamera3RequestDescriptor is constructed, avoiding possibly\nuse-before-init issues.\n\n> +\n> +\t\tstd::unique_ptr reqDescriptor =\n> +\t\t\tstd::make_unique();\n> +\t\t*reqDescriptor = std::move(descriptor);\n\nThis will copy the whole class as there's no move constructor for\nCamera3RequestDescriptor. I don't think that's what you want. If the\ndescriptor needs to be moved out of the map to the queue, without a\ncopy, then they should be stored in the map as unique_ptr already. This\nshould be done as a separate patch before this one.\n\nI wonder if we shouldn't keep the descriptor in the map until we finish\npost-processing, as that would help implementing partial completion\nsupport in the future (where we would connect the bufferComplete signal\nand start post-processing of buffers before libcamera completes the\nwhole request). Maybe that will be best handled in the future, not now,\nso I think I'm fine moving descriptors from the map to the queue for\nnow.\n\n> +\t\tqueuedDescriptor_.push_back(std::move(reqDescriptor));\n> +\n> +\t\tCamera3RequestDescriptor *currentDescriptor = queuedDescriptor_.back().get();\n> \t\tint ret = cameraStream->process(src, *buffer.buffer,\n\nret is unused, this causes a compilation failure. Could you please\ncompile-test individual patches in the series ?\n\nNow that camera stream processing completion is signaled, the process()\nfunction should become void. This can go in a separate patch.\n\n> -\t\t\t\t\t\tdescriptor.settings_,\n> -\t\t\t\t\t\tresultMetadata.get(),\n> -\t\t\t\t\t\t&descriptor);\n> +\t\t\t\t\t\tcurrentDescriptor->settings_,\n> +\t\t\t\t\t\tcurrentDescriptor->resultMetadata_.get(),\n\nYou can drop the resultMetadata parameter from the process() function\nand access it through the descriptor instead.\n\n> +\t\t\t\t\t\tcurrentDescriptor);\n> +\t\treturn;\n> +\t}\n> +\n> +\tif (queuedDescriptor_.empty()) {\n> +\t\tcaptureResult.result = resultMetadata->get();\n> +\t\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n> +\t} else {\n> +\t\t/*\n> +\t\t * Previous capture results waiting to be sent to framework\n> +\t\t * hence, queue the current capture results as well. After that,\n> +\t\t * check if any results are ready to be sent back to the\n> +\t\t * framework.\n> +\t\t */\n> +\t\tdescriptor.status_ = Camera3RequestDescriptor::Succeeded;\n> +\t\tdescriptor.resultMetadata_ = std::move(resultMetadata);\n> +\t\tdescriptor.captureResult_ = captureResult;\n> +\t\tstd::unique_ptr reqDescriptor =\n> +\t\t\tstd::make_unique();\n> +\t\t*reqDescriptor = std::move(descriptor);\n> +\t\tqueuedDescriptor_.push_back(std::move(reqDescriptor));\n> +\n> +\t\tsendQueuedCaptureResults();\n> +\t}\n> +}\n> +\n> +void CameraDevice::streamProcessingComplete(CameraStream *cameraStream,\n> +\t\t\t\t\t CameraStream::ProcessStatus status,\n> +\t\t\t\t\t const Camera3RequestDescriptor *context)\n> +{\n> +\tfor (auto &d : queuedDescriptor_) {\n\nThis function is called from the post-processing completion thread, so\nit will race with CameraDevice::requestComplete(). Both access\nqueuedDescriptor_ with a lock, it won't end well.\n\n> +\t\tif (d.get() != context)\n> +\t\t\tcontinue;\n\nIterating over a queue to find the element we need sounds like we're not\nusing the right type of container. I think you should keep the\nCamera3RequestDescriptor in the map, and look it up from the map here.\n\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> -\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\tcameraStream->putBuffer(d->internalBuffer_);\n> +\n> +\t\tif (status == CameraStream::ProcessStatus::Succeeded) {\n> +\t\t\td->status_ = Camera3RequestDescriptor::Succeeded;\n> +\t\t} else {\n> +\t\t\td->status_ = Camera3RequestDescriptor::Failed;\n> +\n> +\t\t\td->captureResult_.partial_result = 0;\n> +\t\t\tfor (camera3_stream_buffer_t &buffer : d->buffers_) {\n> +\t\t\t\tCameraStream *cs = static_cast(buffer.stream->priv);\n> +\n> +\t\t\t\tif (cs->camera3Stream().format != HAL_PIXEL_FORMAT_BLOB)\n> +\t\t\t\t\tcontinue;\n> +\t\t\t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> +\t\t\t\tnotifyError(d->frameNumber_, buffer.stream,\n> +\t\t\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n> +\t\t\t}\n> \t\t}\n> +\t\tbreak;\n> \t}\n> \n> -\tcaptureResult.result = resultMetadata->get();\n> -\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n> +\t/*\n> +\t * Send back capture results to the framework by inspecting the queue.\n> +\t * The framework can defer queueing further requests to the HAL (via\n> +\t * process_capture_request) unless until it receives the capture results\n> +\t * for already queued requests.\n> +\t */\n> +\tsendQueuedCaptureResults();\n> +}\n> +\n> +void CameraDevice::sendQueuedCaptureResults()\n> +{\n> +\twhile (!queuedDescriptor_.empty()) {\n> +\t\tstd::unique_ptr &d = queuedDescriptor_.front();\n> +\t\tif (d->status_ == Camera3RequestDescriptor::Pending)\n> +\t\t\treturn;\n> +\n> +\t\td->captureResult_.result = d->resultMetadata_->get();\n> +\t\tcallbacks_->process_capture_result(callbacks_, &(d->captureResult_));\n> +\t\tqueuedDescriptor_.pop_front();\n> +\t}\n> }\n> \n> std::string CameraDevice::logPrefix() const\n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index b59ac3e7..e7318358 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -7,6 +7,7 @@\n> #ifndef __ANDROID_CAMERA_DEVICE_H__\n> #define __ANDROID_CAMERA_DEVICE_H__\n> \n> +#include \n> #include \n> #include \n> #include \n> @@ -46,6 +47,20 @@ struct Camera3RequestDescriptor {\n> \tstd::vector> frameBuffers_;\n> \tCameraMetadata settings_;\n> \tstd::unique_ptr request_;\n> +\n> +\t/*\n> +\t * Below are utility placeholders used when a capture result\n> +\t * needs to be queued before completion via process_capture_result()\n> +\t */\n> +\tenum completionStatus {\n> +\t\tPending,\n> +\t\tSucceeded,\n> +\t\tFailed,\n> +\t};\n> +\tstd::unique_ptr resultMetadata_;\n> +\tcamera3_capture_result_t captureResult_;\n\nThis patch is a bit hard to review. Could you split the addition of\nresultMetadata_ and captureResult_ to Camera3RequestDescriptor to a\nseparate patch (including addressing the comments above) ?\n\n> +\tlibcamera::FrameBuffer *internalBuffer_;\n> +\tcompletionStatus status_;\n> };\n> \n> class CameraDevice : protected libcamera::Loggable\n> @@ -78,6 +93,9 @@ public:\n> \tint processCaptureRequest(camera3_capture_request_t *request);\n> \tvoid requestComplete(libcamera::Request *request);\n> \n> +\tvoid streamProcessingComplete(CameraStream *stream,\n> +\t\t\t\t CameraStream::ProcessStatus status,\n> +\t\t\t\t const Camera3RequestDescriptor *context);\n> protected:\n> \tstd::string logPrefix() const override;\n> \n> @@ -106,6 +124,8 @@ private:\n> \tstd::unique_ptr getResultMetadata(\n> \t\tconst Camera3RequestDescriptor &descriptor) const;\n> \n> +\tvoid sendQueuedCaptureResults();\n> +\n> \tunsigned int id_;\n> \tcamera3_device_t camera3Device_;\n> \n> @@ -126,6 +146,8 @@ private:\n> \tlibcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */\n> \tstd::map descriptors_;\n> \n> +\tstd::deque> queuedDescriptor_;\n> +\n> \tstd::string maker_;\n> \tstd::string model_;\n> \n> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> index 996779c4..c7d874b2 100644\n> --- a/src/android/camera_stream.cpp\n> +++ b/src/android/camera_stream.cpp\n> @@ -134,6 +134,8 @@ void CameraStream::postProcessingComplete(PostProcessor::Status status,\n> \t\tprocessStatus = ProcessStatus::Succeeded;\n> \telse\n> \t\tprocessStatus = ProcessStatus::Failed;\n> +\n> +\tcameraDevice_->streamProcessingComplete(this, processStatus, context);\n> }\n> \n> FrameBuffer *CameraStream::getBuffer()","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 9E2A9BDB1D\n\tfor ;\n\tMon, 13 Sep 2021 00:59:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DE66069186;\n\tMon, 13 Sep 2021 02:59:56 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D2B9B60250\n\tfor ;\n\tMon, 13 Sep 2021 02:59:55 +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 416489E;\n\tMon, 13 Sep 2021 02:59:55 +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=\"LDENT60Q\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1631494795;\n\tbh=5qVDIzgXOp/jXeXAAS8s/EhzURlB6eq7sHWGDBRlBS4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LDENT60Q+58mrs5COjFH6I7EjYSCCac7kjrpmS6HacAaVyaam1s/bjrV8f/FbxUjs\n\tT4NLWuTUM9PggvZ7a00XcyCG14Y1hsCO1mr0abEj9S6xrNLPnhfvTK3M7zK/h6suyD\n\tcWRID7HNj6b+sG+3cm0NQ6zUB+NyYqJH0VJJ/N1Q=","Date":"Mon, 13 Sep 2021 03:59:31 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20210910070638.467294-1-umang.jain@ideasonboard.com>\n\t<20210910070638.467294-7-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210910070638.467294-7-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 6/9] android: camera_device: Add a\n\tqueue for sending capture results","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":19634,"web_url":"https://patchwork.libcamera.org/comment/19634/","msgid":"","date":"2021-09-13T07:01:46","subject":"Re: [libcamera-devel] [PATCH v2 6/9] android: camera_device: Add a\n\tqueue for sending capture results","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 9/13/21 6:29 AM, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> Thank you for the patch.\n>\n> On Fri, Sep 10, 2021 at 12:36:35PM +0530, Umang Jain wrote:\n>> When a camera capture request completes, the next step is to send the\n>> capture results to the framework via process_capture_results(). All\n>> the capture associated result metadata is prepared and populated. If\n>> any post-processing is required, it will also take place in the same\n>> thread (which can be blocking for a subtaintial amount of time) before\n>> the results can be sent back to the framework.\n>>\n>> Subsequent patches will move the post-processing to run in a separate\n>> thread. In order to do so, there is few bits of groundwork which this\n>> patch entails. Mainly, we need to preserve the order of sending\n>> the capture results back in which they were queued by the framework\n>> to the HAL (i.e. the order is sequential). Hence, we need to add a queue\n>> in which capture results can be queued with context.\n>>\n>> As per this patch, the post-processor still runs synchronously as\n>> before, but it will queue up the current capture results with context.\n>> Later down the line, the capture results are dequeud in order and\n>> sent back to the framework via sendQueuedCaptureResults(). If the queue\n>> is empty or unused, the current behaviour is to skip the queue and\n>> send capture results directly.\n>>\n>> The context is preserved using Camera3RequestDescriptor utility\n>> structure. It has been expanded accordingly to address the needs of\n>> the functionality.\n>> ---\n>> Check if we can drop unique_ptr to camera3descriptor\n>> ---\n>>\n>> Signed-off-by: Umang Jain \n>> ---\n>> src/android/camera_device.cpp | 106 ++++++++++++++++++++++++++++++----\n>> src/android/camera_device.h | 22 +++++++\n>> src/android/camera_stream.cpp | 2 +\n>> 3 files changed, 119 insertions(+), 11 deletions(-)\n>>\n>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>> index 84549d04..7f04d225 100644\n>> --- a/src/android/camera_device.cpp\n>> +++ b/src/android/camera_device.cpp\n>> @@ -240,6 +240,8 @@ Camera3RequestDescriptor::Camera3RequestDescriptor(\n>> \t/* Clone the controls associated with the camera3 request. */\n>> \tsettings_ = CameraMetadata(camera3Request->settings);\n>> \n>> +\tinternalBuffer_ = nullptr;\n>> +\n>> \t/*\n>> \t * Create the CaptureRequest, stored as a unique_ptr<> to tie its\n>> \t * lifetime to the descriptor.\n>> @@ -989,6 +991,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>> \t\t\t * once it has been processed.\n>> \t\t\t */\n>> \t\t\tbuffer = cameraStream->getBuffer();\n>> +\t\t\tdescriptor.internalBuffer_ = buffer;\n>> \t\t\tLOG(HAL, Debug) << ss.str() << \" (internal)\";\n>> \t\t\tbreak;\n>> \t\t}\n>> @@ -1148,26 +1151,107 @@ void CameraDevice::requestComplete(Request *request)\n>> \t\t\tcontinue;\n>> \t\t}\n>> \n>> +\n>> +\t\t/*\n>> +\t\t * Save the current context of capture result and queue the\n>> +\t\t * descriptor before processing the camera stream.\n>> +\t\t *\n>> +\t\t * When the processing completes, the descriptor will be\n>> +\t\t * dequeued and capture results will be sent to the framework.\n>> +\t\t */\n>> +\t\tdescriptor.status_ = Camera3RequestDescriptor::Pending;\n> The status should also be initialized to Pending in the\n> Camera3RequestDescriptor constructor.\n>\n>> +\t\tdescriptor.resultMetadata_ = std::move(resultMetadata);\n> Let's move the resultMetadata to the descriptor right when creating it,\n> it will be cleaner.\n>\n>> +\t\tdescriptor.captureResult_ = captureResult;\n> This causes a copy of a fairly large structure. You can prevent this by\n> replacing\n>\n> \tcamera3_capture_result_t captureResult = {};\n>\n> above with\n>\n> \tcamera3_capture_result_t &captureResult = descriptor.captureResult_;\n> \tcaptureResult = {};\n>\n> or better, in the definition of Camera3RequestDescriptor, replace\n>\n> \tcamera3_capture_result_t captureResult_;\n>\n> with\n>\n> \tcamera3_capture_result_t captureResult_ = {};\n>\n> and drop the initialization in this function. This will ensure that the\n> capture results are correctly initialized as soon as\n> Camera3RequestDescriptor is constructed, avoiding possibly\n> use-before-init issues.\n>\n>> +\n>> +\t\tstd::unique_ptr reqDescriptor =\n>> +\t\t\tstd::make_unique();\n>> +\t\t*reqDescriptor = std::move(descriptor);\n> This will copy the whole class as there's no move constructor for\n> Camera3RequestDescriptor. I don't think that's what you want. If the\n> descriptor needs to be moved out of the map to the queue, without a\n> copy, then they should be stored in the map as unique_ptr already. This\n> should be done as a separate patch before this one.\n\n\nSo, you mean descriptors in the map should be unique_ptr beforehand? \nThat will avoid the copy on std::move?\n\n>\n> I wonder if we shouldn't keep the descriptor in the map until we finish\n> post-processing, as that would help implementing partial completion\n> support in the future (where we would connect the bufferComplete signal\n> and start post-processing of buffers before libcamera completes the\n> whole request). Maybe that will be best handled in the future, not now,\n> so I think I'm fine moving descriptors from the map to the queue for\n> now.\n\n\nAck.\n\n>\n>> +\t\tqueuedDescriptor_.push_back(std::move(reqDescriptor));\n>> +\n>> +\t\tCamera3RequestDescriptor *currentDescriptor = queuedDescriptor_.back().get();\n>> \t\tint ret = cameraStream->process(src, *buffer.buffer,\n> ret is unused, this causes a compilation failure. Could you please\n> compile-test individual patches in the series ?\n>\n> Now that camera stream processing completion is signaled, the process()\n> function should become void. This can go in a separate patch.\n>\n>> -\t\t\t\t\t\tdescriptor.settings_,\n>> -\t\t\t\t\t\tresultMetadata.get(),\n>> -\t\t\t\t\t\t&descriptor);\n>> +\t\t\t\t\t\tcurrentDescriptor->settings_,\n>> +\t\t\t\t\t\tcurrentDescriptor->resultMetadata_.get(),\n> You can drop the resultMetadata parameter from the process() function\n> and access it through the descriptor instead.\n\n\nAs I mentioned in the previous replies, I think fields that are modified \nas part of process(), should be passed explitcity. The descriptor I pass \ndown the line, is a const.\n\n>\n>> +\t\t\t\t\t\tcurrentDescriptor);\n>> +\t\treturn;\n>> +\t}\n>> +\n>> +\tif (queuedDescriptor_.empty()) {\n>> +\t\tcaptureResult.result = resultMetadata->get();\n>> +\t\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n>> +\t} else {\n>> +\t\t/*\n>> +\t\t * Previous capture results waiting to be sent to framework\n>> +\t\t * hence, queue the current capture results as well. After that,\n>> +\t\t * check if any results are ready to be sent back to the\n>> +\t\t * framework.\n>> +\t\t */\n>> +\t\tdescriptor.status_ = Camera3RequestDescriptor::Succeeded;\n>> +\t\tdescriptor.resultMetadata_ = std::move(resultMetadata);\n>> +\t\tdescriptor.captureResult_ = captureResult;\n>> +\t\tstd::unique_ptr reqDescriptor =\n>> +\t\t\tstd::make_unique();\n>> +\t\t*reqDescriptor = std::move(descriptor);\n>> +\t\tqueuedDescriptor_.push_back(std::move(reqDescriptor));\n>> +\n>> +\t\tsendQueuedCaptureResults();\n>> +\t}\n>> +}\n>> +\n>> +void CameraDevice::streamProcessingComplete(CameraStream *cameraStream,\n>> +\t\t\t\t\t CameraStream::ProcessStatus status,\n>> +\t\t\t\t\t const Camera3RequestDescriptor *context)\n>> +{\n>> +\tfor (auto &d : queuedDescriptor_) {\n> This function is called from the post-processing completion thread, so\n> it will race with CameraDevice::requestComplete(). Both access\n> queuedDescriptor_ with a lock, it won't end well.\n\n\nThere is no thread here in this patch. It's all synchronous as mentioned \nin the commit message. The threading only appears 8/9 patch and there we \nneed to start looking at racing issues.\n\n\n>\n>> +\t\tif (d.get() != context)\n>> +\t\t\tcontinue;\n> Iterating over a queue to find the element we need sounds like we're not\n> using the right type of container. I think you should keep the\n> Camera3RequestDescriptor in the map, and look it up from the map here.\n\n\nNo, The descriptor is already getting cleaned up in requestComplete().\n\nLet me explain:\n\nAs per master right now, the descriptor is extracted from the map at the \nstart of requestComplete. The lifetime of the descriptor is now only in \nthe requestComplete() function. Once it returns, the descriptor is \ndestroyed.\n\nWhat am I doing through this series is, adding a descriptor to a queue. \nThis happens because we don't want to destroy it, rather preserve it \nuntil post-processing completes(well, it's going to happen async, so it \nmakes more sense). But still it should be removed from the map, just \nthat, not let it get destroyed.\n\nNow, why a queue? Because we should be able to send capture results as \nper ordering. The descriptors are queued up in order.\n\nNot removing the descriptor from the map might have it's own \nimplications for e.g. you will need to Request around as well, as the \ndescriptors_ map is looked with with request->cookie(). I think it's \nslightly invasive changes, no? Plus I think the \"android: Fix \ndescriptors_ clean up\" will also conflict to some extend.\n\n\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>> -\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\tcameraStream->putBuffer(d->internalBuffer_);\n>> +\n>> +\t\tif (status == CameraStream::ProcessStatus::Succeeded) {\n>> +\t\t\td->status_ = Camera3RequestDescriptor::Succeeded;\n>> +\t\t} else {\n>> +\t\t\td->status_ = Camera3RequestDescriptor::Failed;\n>> +\n>> +\t\t\td->captureResult_.partial_result = 0;\n>> +\t\t\tfor (camera3_stream_buffer_t &buffer : d->buffers_) {\n>> +\t\t\t\tCameraStream *cs = static_cast(buffer.stream->priv);\n>> +\n>> +\t\t\t\tif (cs->camera3Stream().format != HAL_PIXEL_FORMAT_BLOB)\n>> +\t\t\t\t\tcontinue;\n>> +\t\t\t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n>> +\t\t\t\tnotifyError(d->frameNumber_, buffer.stream,\n>> +\t\t\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n>> +\t\t\t}\n>> \t\t}\n>> +\t\tbreak;\n>> \t}\n>> \n>> -\tcaptureResult.result = resultMetadata->get();\n>> -\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n>> +\t/*\n>> +\t * Send back capture results to the framework by inspecting the queue.\n>> +\t * The framework can defer queueing further requests to the HAL (via\n>> +\t * process_capture_request) unless until it receives the capture results\n>> +\t * for already queued requests.\n>> +\t */\n>> +\tsendQueuedCaptureResults();\n>> +}\n>> +\n>> +void CameraDevice::sendQueuedCaptureResults()\n>> +{\n>> +\twhile (!queuedDescriptor_.empty()) {\n>> +\t\tstd::unique_ptr &d = queuedDescriptor_.front();\n>> +\t\tif (d->status_ == Camera3RequestDescriptor::Pending)\n>> +\t\t\treturn;\n>> +\n>> +\t\td->captureResult_.result = d->resultMetadata_->get();\n>> +\t\tcallbacks_->process_capture_result(callbacks_, &(d->captureResult_));\n>> +\t\tqueuedDescriptor_.pop_front();\n>> +\t}\n>> }\n>> \n>> std::string CameraDevice::logPrefix() const\n>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n>> index b59ac3e7..e7318358 100644\n>> --- a/src/android/camera_device.h\n>> +++ b/src/android/camera_device.h\n>> @@ -7,6 +7,7 @@\n>> #ifndef __ANDROID_CAMERA_DEVICE_H__\n>> #define __ANDROID_CAMERA_DEVICE_H__\n>> \n>> +#include \n>> #include \n>> #include \n>> #include \n>> @@ -46,6 +47,20 @@ struct Camera3RequestDescriptor {\n>> \tstd::vector> frameBuffers_;\n>> \tCameraMetadata settings_;\n>> \tstd::unique_ptr request_;\n>> +\n>> +\t/*\n>> +\t * Below are utility placeholders used when a capture result\n>> +\t * needs to be queued before completion via process_capture_result()\n>> +\t */\n>> +\tenum completionStatus {\n>> +\t\tPending,\n>> +\t\tSucceeded,\n>> +\t\tFailed,\n>> +\t};\n>> +\tstd::unique_ptr resultMetadata_;\n>> +\tcamera3_capture_result_t captureResult_;\n> This patch is a bit hard to review. Could you split the addition of\n> resultMetadata_ and captureResult_ to Camera3RequestDescriptor to a\n> separate patch (including addressing the comments above) ?\n\n\nYes, it's hard since it setups with the ground work for post-processor. \nIt has many pieces linked together giving very little room for splitting up.\n\n>\n>> +\tlibcamera::FrameBuffer *internalBuffer_;\n>> +\tcompletionStatus status_;\n>> };\n>> \n>> class CameraDevice : protected libcamera::Loggable\n>> @@ -78,6 +93,9 @@ public:\n>> \tint processCaptureRequest(camera3_capture_request_t *request);\n>> \tvoid requestComplete(libcamera::Request *request);\n>> \n>> +\tvoid streamProcessingComplete(CameraStream *stream,\n>> +\t\t\t\t CameraStream::ProcessStatus status,\n>> +\t\t\t\t const Camera3RequestDescriptor *context);\n>> protected:\n>> \tstd::string logPrefix() const override;\n>> \n>> @@ -106,6 +124,8 @@ private:\n>> \tstd::unique_ptr getResultMetadata(\n>> \t\tconst Camera3RequestDescriptor &descriptor) const;\n>> \n>> +\tvoid sendQueuedCaptureResults();\n>> +\n>> \tunsigned int id_;\n>> \tcamera3_device_t camera3Device_;\n>> \n>> @@ -126,6 +146,8 @@ private:\n>> \tlibcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */\n>> \tstd::map descriptors_;\n>> \n>> +\tstd::deque> queuedDescriptor_;\n>> +\n>> \tstd::string maker_;\n>> \tstd::string model_;\n>> \n>> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n>> index 996779c4..c7d874b2 100644\n>> --- a/src/android/camera_stream.cpp\n>> +++ b/src/android/camera_stream.cpp\n>> @@ -134,6 +134,8 @@ void CameraStream::postProcessingComplete(PostProcessor::Status status,\n>> \t\tprocessStatus = ProcessStatus::Succeeded;\n>> \telse\n>> \t\tprocessStatus = ProcessStatus::Failed;\n>> +\n>> +\tcameraDevice_->streamProcessingComplete(this, processStatus, context);\n>> }\n>> \n>> FrameBuffer *CameraStream::getBuffer()","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 7AF23BDB1D\n\tfor ;\n\tMon, 13 Sep 2021 07:01:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A51C269186;\n\tMon, 13 Sep 2021 09:01:53 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B88DB69181\n\tfor ;\n\tMon, 13 Sep 2021 09:01:51 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.152])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 99E129E;\n\tMon, 13 Sep 2021 09:01:50 +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=\"njunbvAa\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1631516511;\n\tbh=k1HweJ8gr1En4H5YuqHElhsLgD9sybgjtVlD3ICkAbo=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=njunbvAaZrb2xJfv4Ofp5qjcFTW5auvuFCEmf6+4beRUQU3DAzDkuLLNuDKsPgq2J\n\tzqBw2ZxG8Hew4L2MB6bOmjF7JUZ0sUdS/NFWC8Dr23NyTcS4y6oVDyGhLvllSPZt7u\n\tZs6CMbAOslC8hSnY4IrTEuQG7doKRC56SmETrJrc=","To":"Laurent Pinchart ","References":"<20210910070638.467294-1-umang.jain@ideasonboard.com>\n\t<20210910070638.467294-7-umang.jain@ideasonboard.com>\n\t","From":"Umang Jain ","Message-ID":"","Date":"Mon, 13 Sep 2021 12:31:46 +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":"","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v2 6/9] android: camera_device: Add a\n\tqueue for sending capture results","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":19693,"web_url":"https://patchwork.libcamera.org/comment/19693/","msgid":"","date":"2021-09-15T01:28:56","subject":"Re: [libcamera-devel] [PATCH v2 6/9] android: camera_device: Add a\n\tqueue for sending capture results","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nOn Mon, Sep 13, 2021 at 12:31:46PM +0530, Umang Jain wrote:\n> On 9/13/21 6:29 AM, Laurent Pinchart wrote:\n> > On Fri, Sep 10, 2021 at 12:36:35PM +0530, Umang Jain wrote:\n> >> When a camera capture request completes, the next step is to send the\n> >> capture results to the framework via process_capture_results(). All\n> >> the capture associated result metadata is prepared and populated. If\n> >> any post-processing is required, it will also take place in the same\n> >> thread (which can be blocking for a subtaintial amount of time) before\n> >> the results can be sent back to the framework.\n> >>\n> >> Subsequent patches will move the post-processing to run in a separate\n> >> thread. In order to do so, there is few bits of groundwork which this\n> >> patch entails. Mainly, we need to preserve the order of sending\n> >> the capture results back in which they were queued by the framework\n> >> to the HAL (i.e. the order is sequential). Hence, we need to add a queue\n> >> in which capture results can be queued with context.\n> >>\n> >> As per this patch, the post-processor still runs synchronously as\n> >> before, but it will queue up the current capture results with context.\n> >> Later down the line, the capture results are dequeud in order and\n> >> sent back to the framework via sendQueuedCaptureResults(). If the queue\n> >> is empty or unused, the current behaviour is to skip the queue and\n> >> send capture results directly.\n> >>\n> >> The context is preserved using Camera3RequestDescriptor utility\n> >> structure. It has been expanded accordingly to address the needs of\n> >> the functionality.\n> >> ---\n> >> Check if we can drop unique_ptr to camera3descriptor\n> >> ---\n> >>\n> >> Signed-off-by: Umang Jain \n> >> ---\n> >> src/android/camera_device.cpp | 106 ++++++++++++++++++++++++++++++----\n> >> src/android/camera_device.h | 22 +++++++\n> >> src/android/camera_stream.cpp | 2 +\n> >> 3 files changed, 119 insertions(+), 11 deletions(-)\n> >>\n> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> >> index 84549d04..7f04d225 100644\n> >> --- a/src/android/camera_device.cpp\n> >> +++ b/src/android/camera_device.cpp\n> >> @@ -240,6 +240,8 @@ Camera3RequestDescriptor::Camera3RequestDescriptor(\n> >> \t/* Clone the controls associated with the camera3 request. */\n> >> \tsettings_ = CameraMetadata(camera3Request->settings);\n> >> \n> >> +\tinternalBuffer_ = nullptr;\n> >> +\n> >> \t/*\n> >> \t * Create the CaptureRequest, stored as a unique_ptr<> to tie its\n> >> \t * lifetime to the descriptor.\n> >> @@ -989,6 +991,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> >> \t\t\t * once it has been processed.\n> >> \t\t\t */\n> >> \t\t\tbuffer = cameraStream->getBuffer();\n> >> +\t\t\tdescriptor.internalBuffer_ = buffer;\n> >> \t\t\tLOG(HAL, Debug) << ss.str() << \" (internal)\";\n> >> \t\t\tbreak;\n> >> \t\t}\n> >> @@ -1148,26 +1151,107 @@ void CameraDevice::requestComplete(Request *request)\n> >> \t\t\tcontinue;\n> >> \t\t}\n> >> \n> >> +\n> >> +\t\t/*\n> >> +\t\t * Save the current context of capture result and queue the\n> >> +\t\t * descriptor before processing the camera stream.\n> >> +\t\t *\n> >> +\t\t * When the processing completes, the descriptor will be\n> >> +\t\t * dequeued and capture results will be sent to the framework.\n> >> +\t\t */\n> >> +\t\tdescriptor.status_ = Camera3RequestDescriptor::Pending;\n> >\n> > The status should also be initialized to Pending in the\n> > Camera3RequestDescriptor constructor.\n> >\n> >> +\t\tdescriptor.resultMetadata_ = std::move(resultMetadata);\n> >\n> > Let's move the resultMetadata to the descriptor right when creating it,\n> > it will be cleaner.\n> >\n> >> +\t\tdescriptor.captureResult_ = captureResult;\n> >\n> > This causes a copy of a fairly large structure. You can prevent this by\n> > replacing\n> >\n> > \tcamera3_capture_result_t captureResult = {};\n> >\n> > above with\n> >\n> > \tcamera3_capture_result_t &captureResult = descriptor.captureResult_;\n> > \tcaptureResult = {};\n> >\n> > or better, in the definition of Camera3RequestDescriptor, replace\n> >\n> > \tcamera3_capture_result_t captureResult_;\n> >\n> > with\n> >\n> > \tcamera3_capture_result_t captureResult_ = {};\n> >\n> > and drop the initialization in this function. This will ensure that the\n> > capture results are correctly initialized as soon as\n> > Camera3RequestDescriptor is constructed, avoiding possibly\n> > use-before-init issues.\n> >\n> >> +\n> >> +\t\tstd::unique_ptr reqDescriptor =\n> >> +\t\t\tstd::make_unique();\n> >> +\t\t*reqDescriptor = std::move(descriptor);\n> >\n> > This will copy the whole class as there's no move constructor for\n> > Camera3RequestDescriptor. I don't think that's what you want. If the\n> > descriptor needs to be moved out of the map to the queue, without a\n> > copy, then they should be stored in the map as unique_ptr already. This\n> > should be done as a separate patch before this one.\n> \n> So, you mean descriptors in the map should be unique_ptr beforehand? \n> That will avoid the copy on std::move?\n\nThe only thing that std::move() does is to return an rvalue reference to\nthe parameter passed to it. That's all. What it implies is that the\nassignment will then pick the version of operator=() that takes an\nrvalue reference (called the move assignment operator). It's really just\na way to pick one implementation of operator=() over the other.\n\nFor classes such as std::unique_ptr<>, the move assignment operator will\ntransfer ownership of the pointer. For classes such as std::vector<>, it\nwill transfer ownership of the internal data, so it's cheap. For\nCamera3RequestDescriptor, we don't define any move assignement operator,\nso all members will be moved one by one, and for most members, that will\njust be a copy.\n\nBottom line, moving a std::unique_ptr<> makes sense, moving a\nCamera3RequestDescriptor less so.\n\nAfter reviewing the whole series, and thinking some more about it, I\nthink we should introduce a\nstd::queue> that will own the\nCamera3RequestDescriptor instances, and a std::map to lookup the Camera3RequestDescriptor in\nCameraDevice::requestComplete(). The queue will have ownership of the\nrequest descriptor, the map will store pointers to the instance stored\nin the queue. You'll need to\n\n- add an entry to the queue in CameraDevice::processCaptureRequest(),\n and add it to the map there too, right before queuing the request to\n the worker\n\n- look up the entry from the map in CameraDevice::requestComplete(), and\n remove it from the map there\n\n- handle completion in order using the queue, with a function like\n CameraDevice::sendQueuedCaptureResults()\n\nIt would be best, I think, if this rework could be done early in the\nseries to make it easier to review, and then add the async\npost-processor. I haven't looked at it in details though, so maybe\nthat's not practical.\n\n> > I wonder if we shouldn't keep the descriptor in the map until we finish\n> > post-processing, as that would help implementing partial completion\n> > support in the future (where we would connect the bufferComplete signal\n> > and start post-processing of buffers before libcamera completes the\n> > whole request). Maybe that will be best handled in the future, not now,\n> > so I think I'm fine moving descriptors from the map to the queue for\n> > now.\n> \n> Ack.\n> \n> >> +\t\tqueuedDescriptor_.push_back(std::move(reqDescriptor));\n> >> +\n> >> +\t\tCamera3RequestDescriptor *currentDescriptor = queuedDescriptor_.back().get();\n> >> \t\tint ret = cameraStream->process(src, *buffer.buffer,\n> >\n> > ret is unused, this causes a compilation failure. Could you please\n> > compile-test individual patches in the series ?\n> >\n> > Now that camera stream processing completion is signaled, the process()\n> > function should become void. This can go in a separate patch.\n> >\n> >> -\t\t\t\t\t\tdescriptor.settings_,\n> >> -\t\t\t\t\t\tresultMetadata.get(),\n> >> -\t\t\t\t\t\t&descriptor);\n> >> +\t\t\t\t\t\tcurrentDescriptor->settings_,\n> >> +\t\t\t\t\t\tcurrentDescriptor->resultMetadata_.get(),\n> >\n> > You can drop the resultMetadata parameter from the process() function\n> > and access it through the descriptor instead.\n> \n> As I mentioned in the previous replies, I think fields that are modified \n> as part of process(), should be passed explitcity. The descriptor I pass \n> down the line, is a const.\n> \n> >> +\t\t\t\t\t\tcurrentDescriptor);\n> >> +\t\treturn;\n> >> +\t}\n> >> +\n> >> +\tif (queuedDescriptor_.empty()) {\n> >> +\t\tcaptureResult.result = resultMetadata->get();\n> >> +\t\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n> >> +\t} else {\n> >> +\t\t/*\n> >> +\t\t * Previous capture results waiting to be sent to framework\n> >> +\t\t * hence, queue the current capture results as well. After that,\n> >> +\t\t * check if any results are ready to be sent back to the\n> >> +\t\t * framework.\n> >> +\t\t */\n> >> +\t\tdescriptor.status_ = Camera3RequestDescriptor::Succeeded;\n> >> +\t\tdescriptor.resultMetadata_ = std::move(resultMetadata);\n> >> +\t\tdescriptor.captureResult_ = captureResult;\n> >> +\t\tstd::unique_ptr reqDescriptor =\n> >> +\t\t\tstd::make_unique();\n> >> +\t\t*reqDescriptor = std::move(descriptor);\n> >> +\t\tqueuedDescriptor_.push_back(std::move(reqDescriptor));\n> >> +\n> >> +\t\tsendQueuedCaptureResults();\n> >> +\t}\n> >> +}\n> >> +\n> >> +void CameraDevice::streamProcessingComplete(CameraStream *cameraStream,\n> >> +\t\t\t\t\t CameraStream::ProcessStatus status,\n> >> +\t\t\t\t\t const Camera3RequestDescriptor *context)\n> >> +{\n> >> +\tfor (auto &d : queuedDescriptor_) {\n> >\n> > This function is called from the post-processing completion thread, so\n> > it will race with CameraDevice::requestComplete(). Both access\n> > queuedDescriptor_ with a lock, it won't end well.\n> \n> There is no thread here in this patch. It's all synchronous as mentioned \n> in the commit message. The threading only appears 8/9 patch and there we \n> need to start looking at racing issues.\n\nYou're right, but could we introduce the required locking in this patch\nalready ? It's easier to review if the new data structures and the\nlocking come together.\n\n> >> +\t\tif (d.get() != context)\n> >> +\t\t\tcontinue;\n> >\n> > Iterating over a queue to find the element we need sounds like we're not\n> > using the right type of container. I think you should keep the\n> > Camera3RequestDescriptor in the map, and look it up from the map here.\n> \n> No, The descriptor is already getting cleaned up in requestComplete().\n> \n> Let me explain:\n> \n> As per master right now, the descriptor is extracted from the map at the \n> start of requestComplete. The lifetime of the descriptor is now only in \n> the requestComplete() function. Once it returns, the descriptor is \n> destroyed.\n> \n> What am I doing through this series is, adding a descriptor to a queue. \n> This happens because we don't want to destroy it, rather preserve it \n> until post-processing completes(well, it's going to happen async, so it \n> makes more sense). But still it should be removed from the map, just \n> that, not let it get destroyed.\n> \n> Now, why a queue? Because we should be able to send capture results as \n> per ordering. The descriptors are queued up in order.\n\nThat's a very clear explanation, I agree with all of this.\n\nMy comment about looking it up from the map indeed doesn't make too much\nsense. Reading this function again, can't you just access the\nCamera3RequestDescriptor from the context parameter ? You'll need to\nconst_cast<> to remove the const, but that's fine, we know here that we\ncan do so because we own the Camera3RequestDescriptor.\n\n> Not removing the descriptor from the map might have it's own \n> implications for e.g. you will need to Request around as well, as the \n> descriptors_ map is looked with with request->cookie(). I think it's \n> slightly invasive changes, no?\n\nYes, not a good idea.\n\n> Plus I think the \"android: Fix \n> descriptors_ clean up\" will also conflict to some extend.\n\nThere could be some conflicts, yes :-S\n\nDoes my above proposal with a queue owning the request and a map to look\nit up make sense ? We should ideally even be able to get rid of the map\nby storing the pointer to the Camera3RequestDescriptor in the Request\ncookie, but it's currently used to store the CaptureRequest pointer.\nCaptureRequest should really be dropped as fence handling should go to\nthe libcamera core, but I don't want to include that in this series,\nit's big enough already. A map is fine as an interim solution.\n\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> >> -\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\tcameraStream->putBuffer(d->internalBuffer_);\n> >> +\n> >> +\t\tif (status == CameraStream::ProcessStatus::Succeeded) {\n> >> +\t\t\td->status_ = Camera3RequestDescriptor::Succeeded;\n> >> +\t\t} else {\n> >> +\t\t\td->status_ = Camera3RequestDescriptor::Failed;\n> >> +\n> >> +\t\t\td->captureResult_.partial_result = 0;\n> >> +\t\t\tfor (camera3_stream_buffer_t &buffer : d->buffers_) {\n> >> +\t\t\t\tCameraStream *cs = static_cast(buffer.stream->priv);\n> >> +\n> >> +\t\t\t\tif (cs->camera3Stream().format != HAL_PIXEL_FORMAT_BLOB)\n> >> +\t\t\t\t\tcontinue;\n> >> +\t\t\t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> >> +\t\t\t\tnotifyError(d->frameNumber_, buffer.stream,\n> >> +\t\t\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n> >> +\t\t\t}\n> >> \t\t}\n> >> +\t\tbreak;\n> >> \t}\n> >> \n> >> -\tcaptureResult.result = resultMetadata->get();\n> >> -\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n> >> +\t/*\n> >> +\t * Send back capture results to the framework by inspecting the queue.\n> >> +\t * The framework can defer queueing further requests to the HAL (via\n> >> +\t * process_capture_request) unless until it receives the capture results\n> >> +\t * for already queued requests.\n> >> +\t */\n> >> +\tsendQueuedCaptureResults();\n> >> +}\n> >> +\n> >> +void CameraDevice::sendQueuedCaptureResults()\n> >> +{\n> >> +\twhile (!queuedDescriptor_.empty()) {\n> >> +\t\tstd::unique_ptr &d = queuedDescriptor_.front();\n> >> +\t\tif (d->status_ == Camera3RequestDescriptor::Pending)\n> >> +\t\t\treturn;\n> >> +\n> >> +\t\td->captureResult_.result = d->resultMetadata_->get();\n> >> +\t\tcallbacks_->process_capture_result(callbacks_, &(d->captureResult_));\n> >> +\t\tqueuedDescriptor_.pop_front();\n> >> +\t}\n> >> }\n> >> \n> >> std::string CameraDevice::logPrefix() const\n> >> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> >> index b59ac3e7..e7318358 100644\n> >> --- a/src/android/camera_device.h\n> >> +++ b/src/android/camera_device.h\n> >> @@ -7,6 +7,7 @@\n> >> #ifndef __ANDROID_CAMERA_DEVICE_H__\n> >> #define __ANDROID_CAMERA_DEVICE_H__\n> >> \n> >> +#include \n> >> #include \n> >> #include \n> >> #include \n> >> @@ -46,6 +47,20 @@ struct Camera3RequestDescriptor {\n> >> \tstd::vector> frameBuffers_;\n> >> \tCameraMetadata settings_;\n> >> \tstd::unique_ptr request_;\n> >> +\n> >> +\t/*\n> >> +\t * Below are utility placeholders used when a capture result\n> >> +\t * needs to be queued before completion via process_capture_result()\n> >> +\t */\n> >> +\tenum completionStatus {\n> >> +\t\tPending,\n> >> +\t\tSucceeded,\n> >> +\t\tFailed,\n> >> +\t};\n> >> +\tstd::unique_ptr resultMetadata_;\n> >> +\tcamera3_capture_result_t captureResult_;\n> >\n> > This patch is a bit hard to review. Could you split the addition of\n> > resultMetadata_ and captureResult_ to Camera3RequestDescriptor to a\n> > separate patch (including addressing the comments above) ?\n> \n> Yes, it's hard since it setups with the ground work for post-processor. \n> It has many pieces linked together giving very little room for splitting up.\n> \n> >> +\tlibcamera::FrameBuffer *internalBuffer_;\n> >> +\tcompletionStatus status_;\n> >> };\n> >> \n> >> class CameraDevice : protected libcamera::Loggable\n> >> @@ -78,6 +93,9 @@ public:\n> >> \tint processCaptureRequest(camera3_capture_request_t *request);\n> >> \tvoid requestComplete(libcamera::Request *request);\n> >> \n> >> +\tvoid streamProcessingComplete(CameraStream *stream,\n> >> +\t\t\t\t CameraStream::ProcessStatus status,\n> >> +\t\t\t\t const Camera3RequestDescriptor *context);\n> >> protected:\n> >> \tstd::string logPrefix() const override;\n> >> \n> >> @@ -106,6 +124,8 @@ private:\n> >> \tstd::unique_ptr getResultMetadata(\n> >> \t\tconst Camera3RequestDescriptor &descriptor) const;\n> >> \n> >> +\tvoid sendQueuedCaptureResults();\n> >> +\n> >> \tunsigned int id_;\n> >> \tcamera3_device_t camera3Device_;\n> >> \n> >> @@ -126,6 +146,8 @@ private:\n> >> \tlibcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */\n> >> \tstd::map descriptors_;\n> >> \n> >> +\tstd::deque> queuedDescriptor_;\n> >> +\n> >> \tstd::string maker_;\n> >> \tstd::string model_;\n> >> \n> >> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> >> index 996779c4..c7d874b2 100644\n> >> --- a/src/android/camera_stream.cpp\n> >> +++ b/src/android/camera_stream.cpp\n> >> @@ -134,6 +134,8 @@ void CameraStream::postProcessingComplete(PostProcessor::Status status,\n> >> \t\tprocessStatus = ProcessStatus::Succeeded;\n> >> \telse\n> >> \t\tprocessStatus = ProcessStatus::Failed;\n> >> +\n> >> +\tcameraDevice_->streamProcessingComplete(this, processStatus, context);\n> >> }\n> >> \n> >> FrameBuffer *CameraStream::getBuffer()","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 0ABB0BDC71\n\tfor ;\n\tWed, 15 Sep 2021 01:29:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5B8E56918B;\n\tWed, 15 Sep 2021 03:29:22 +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 4526F60249\n\tfor ;\n\tWed, 15 Sep 2021 03:29:21 +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 A15CA24F;\n\tWed, 15 Sep 2021 03:29:20 +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=\"ViAM1FkX\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1631669360;\n\tbh=zuGtskGHCI5Vs7apu4TSQZe+ja3anXefeTbSgzf3V9Y=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ViAM1FkXAvchOmJ0+3gefjEr23ZSpnGPJzzIapEdEdxJer5CeWpr7Q1XekadV87Yv\n\thcW76q8DHmY/xURi/xJopTRi3SDDUEo2cFm7KfYGgY8l6w5EoBQDt5+94tzADb5ElB\n\tMyqORPkRJ/xcMUVJaoHFvE8g7mTT1bxf0XgNycNI=","Date":"Wed, 15 Sep 2021 04:28:56 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20210910070638.467294-1-umang.jain@ideasonboard.com>\n\t<20210910070638.467294-7-umang.jain@ideasonboard.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 6/9] android: camera_device: Add a\n\tqueue for sending capture results","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":19713,"web_url":"https://patchwork.libcamera.org/comment/19713/","msgid":"","date":"2021-09-17T08:53:28","subject":"Re: [libcamera-devel] [PATCH v2 6/9] android: camera_device: Add a\n\tqueue for sending capture results","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 9/15/21 6:58 AM, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> On Mon, Sep 13, 2021 at 12:31:46PM +0530, Umang Jain wrote:\n>> On 9/13/21 6:29 AM, Laurent Pinchart wrote:\n>>> On Fri, Sep 10, 2021 at 12:36:35PM +0530, Umang Jain wrote:\n>>>> When a camera capture request completes, the next step is to send the\n>>>> capture results to the framework via process_capture_results(). All\n>>>> the capture associated result metadata is prepared and populated. If\n>>>> any post-processing is required, it will also take place in the same\n>>>> thread (which can be blocking for a subtaintial amount of time) before\n>>>> the results can be sent back to the framework.\n>>>>\n>>>> Subsequent patches will move the post-processing to run in a separate\n>>>> thread. In order to do so, there is few bits of groundwork which this\n>>>> patch entails. Mainly, we need to preserve the order of sending\n>>>> the capture results back in which they were queued by the framework\n>>>> to the HAL (i.e. the order is sequential). Hence, we need to add a queue\n>>>> in which capture results can be queued with context.\n>>>>\n>>>> As per this patch, the post-processor still runs synchronously as\n>>>> before, but it will queue up the current capture results with context.\n>>>> Later down the line, the capture results are dequeud in order and\n>>>> sent back to the framework via sendQueuedCaptureResults(). If the queue\n>>>> is empty or unused, the current behaviour is to skip the queue and\n>>>> send capture results directly.\n>>>>\n>>>> The context is preserved using Camera3RequestDescriptor utility\n>>>> structure. It has been expanded accordingly to address the needs of\n>>>> the functionality.\n>>>> ---\n>>>> Check if we can drop unique_ptr to camera3descriptor\n>>>> ---\n>>>>\n>>>> Signed-off-by: Umang Jain \n>>>> ---\n>>>> src/android/camera_device.cpp | 106 ++++++++++++++++++++++++++++++----\n>>>> src/android/camera_device.h | 22 +++++++\n>>>> src/android/camera_stream.cpp | 2 +\n>>>> 3 files changed, 119 insertions(+), 11 deletions(-)\n>>>>\n>>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>>>> index 84549d04..7f04d225 100644\n>>>> --- a/src/android/camera_device.cpp\n>>>> +++ b/src/android/camera_device.cpp\n>>>> @@ -240,6 +240,8 @@ Camera3RequestDescriptor::Camera3RequestDescriptor(\n>>>> \t/* Clone the controls associated with the camera3 request. */\n>>>> \tsettings_ = CameraMetadata(camera3Request->settings);\n>>>> \n>>>> +\tinternalBuffer_ = nullptr;\n>>>> +\n>>>> \t/*\n>>>> \t * Create the CaptureRequest, stored as a unique_ptr<> to tie its\n>>>> \t * lifetime to the descriptor.\n>>>> @@ -989,6 +991,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>>>> \t\t\t * once it has been processed.\n>>>> \t\t\t */\n>>>> \t\t\tbuffer = cameraStream->getBuffer();\n>>>> +\t\t\tdescriptor.internalBuffer_ = buffer;\n>>>> \t\t\tLOG(HAL, Debug) << ss.str() << \" (internal)\";\n>>>> \t\t\tbreak;\n>>>> \t\t}\n>>>> @@ -1148,26 +1151,107 @@ void CameraDevice::requestComplete(Request *request)\n>>>> \t\t\tcontinue;\n>>>> \t\t}\n>>>> \n>>>> +\n>>>> +\t\t/*\n>>>> +\t\t * Save the current context of capture result and queue the\n>>>> +\t\t * descriptor before processing the camera stream.\n>>>> +\t\t *\n>>>> +\t\t * When the processing completes, the descriptor will be\n>>>> +\t\t * dequeued and capture results will be sent to the framework.\n>>>> +\t\t */\n>>>> +\t\tdescriptor.status_ = Camera3RequestDescriptor::Pending;\n>>> The status should also be initialized to Pending in the\n>>> Camera3RequestDescriptor constructor.\n>>>\n>>>> +\t\tdescriptor.resultMetadata_ = std::move(resultMetadata);\n>>> Let's move the resultMetadata to the descriptor right when creating it,\n>>> it will be cleaner.\n>>>\n>>>> +\t\tdescriptor.captureResult_ = captureResult;\n>>> This causes a copy of a fairly large structure. You can prevent this by\n>>> replacing\n>>>\n>>> \tcamera3_capture_result_t captureResult = {};\n>>>\n>>> above with\n>>>\n>>> \tcamera3_capture_result_t &captureResult = descriptor.captureResult_;\n>>> \tcaptureResult = {};\n>>>\n>>> or better, in the definition of Camera3RequestDescriptor, replace\n>>>\n>>> \tcamera3_capture_result_t captureResult_;\n>>>\n>>> with\n>>>\n>>> \tcamera3_capture_result_t captureResult_ = {};\n>>>\n>>> and drop the initialization in this function. This will ensure that the\n>>> capture results are correctly initialized as soon as\n>>> Camera3RequestDescriptor is constructed, avoiding possibly\n>>> use-before-init issues.\n>>>\n>>>> +\n>>>> +\t\tstd::unique_ptr reqDescriptor =\n>>>> +\t\t\tstd::make_unique();\n>>>> +\t\t*reqDescriptor = std::move(descriptor);\n>>> This will copy the whole class as there's no move constructor for\n>>> Camera3RequestDescriptor. I don't think that's what you want. If the\n>>> descriptor needs to be moved out of the map to the queue, without a\n>>> copy, then they should be stored in the map as unique_ptr already. This\n>>> should be done as a separate patch before this one.\n>> So, you mean descriptors in the map should be unique_ptr beforehand?\n>> That will avoid the copy on std::move?\n> The only thing that std::move() does is to return an rvalue reference to\n> the parameter passed to it. That's all. What it implies is that the\n> assignment will then pick the version of operator=() that takes an\n> rvalue reference (called the move assignment operator). It's really just\n> a way to pick one implementation of operator=() over the other.\n>\n> For classes such as std::unique_ptr<>, the move assignment operator will\n> transfer ownership of the pointer. For classes such as std::vector<>, it\n> will transfer ownership of the internal data, so it's cheap. For\n> Camera3RequestDescriptor, we don't define any move assignement operator,\n> so all members will be moved one by one, and for most members, that will\n> just be a copy.\n>\n> Bottom line, moving a std::unique_ptr<> makes sense, moving a\n> Camera3RequestDescriptor less so.\n\n\nI agree with this particular pointer, that storing \nCamera3RequestDescriptor can be made via std::unique<> to essentially \nleverage std::move.\n\nBut I am not too sure of your strategy below.\n\n>\n> After reviewing the whole series, and thinking some more about it, I\n> think we should introduce a\n> std::queue> that will own the\n> Camera3RequestDescriptor instances, and a std::map Camera3RequestDescriptor *> to lookup the Camera3RequestDescriptor in\n> CameraDevice::requestComplete(). The queue will have ownership of the\n> request descriptor, the map will store pointers to the instance stored\n> in the queue. You'll need to\n>\n> - add an entry to the queue in CameraDevice::processCaptureRequest(),\n> and add it to the map there too, right before queuing the request to\n> the worker\n>\n> - look up the entry from the map in CameraDevice::requestComplete(), and\n> remove it from the map there\n>\n> - handle completion in order using the queue, with a function like\n> CameraDevice::sendQueuedCaptureResults()\n>\n> It would be best, I think, if this rework could be done early in the\n> series to make it easier to review, and then add the async\n> post-processor. I haven't looked at it in details though, so maybe\n> that's not practical.\n\n\nThis particular bit is getting complicated here. I understand you want \nthat the ownership is held by a queue but all look-ups happen via a map \nfrom the start, but there are error paths that will clean up the \ndescriptors be removing it from the map and free the buffers held by it \nright away. If the ownership is held with the queue, we need to clean it \nup from the queue, to actually free the buffers right? .... And queue \nmight need to get re-adjusted with remaining elements which might not be \nthat great. It's adding up complexity (at least from error-ed requests \npoint of view), that when iterating over the queue, a descriptor is \npresent in the queue, but has been removed from the map, which the queue \nis supposed to do with it?\n\nI am still pondering over it (and also looking at \"android: Fix \ndescriptors_ clean up\" series), trying to answer my questions but it's \nmaking me a bit scared.\n\n>\n>>> I wonder if we shouldn't keep the descriptor in the map until we finish\n>>> post-processing, as that would help implementing partial completion\n>>> support in the future (where we would connect the bufferComplete signal\n>>> and start post-processing of buffers before libcamera completes the\n>>> whole request). Maybe that will be best handled in the future, not now,\n>>> so I think I'm fine moving descriptors from the map to the queue for\n>>> now.\n>> Ack.\n>>\n>>>> +\t\tqueuedDescriptor_.push_back(std::move(reqDescriptor));\n>>>> +\n>>>> +\t\tCamera3RequestDescriptor *currentDescriptor = queuedDescriptor_.back().get();\n>>>> \t\tint ret = cameraStream->process(src, *buffer.buffer,\n>>> ret is unused, this causes a compilation failure. Could you please\n>>> compile-test individual patches in the series ?\n>>>\n>>> Now that camera stream processing completion is signaled, the process()\n>>> function should become void. This can go in a separate patch.\n>>>\n>>>> -\t\t\t\t\t\tdescriptor.settings_,\n>>>> -\t\t\t\t\t\tresultMetadata.get(),\n>>>> -\t\t\t\t\t\t&descriptor);\n>>>> +\t\t\t\t\t\tcurrentDescriptor->settings_,\n>>>> +\t\t\t\t\t\tcurrentDescriptor->resultMetadata_.get(),\n>>> You can drop the resultMetadata parameter from the process() function\n>>> and access it through the descriptor instead.\n>> As I mentioned in the previous replies, I think fields that are modified\n>> as part of process(), should be passed explitcity. The descriptor I pass\n>> down the line, is a const.\n>>\n>>>> +\t\t\t\t\t\tcurrentDescriptor);\n>>>> +\t\treturn;\n>>>> +\t}\n>>>> +\n>>>> +\tif (queuedDescriptor_.empty()) {\n>>>> +\t\tcaptureResult.result = resultMetadata->get();\n>>>> +\t\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n>>>> +\t} else {\n>>>> +\t\t/*\n>>>> +\t\t * Previous capture results waiting to be sent to framework\n>>>> +\t\t * hence, queue the current capture results as well. After that,\n>>>> +\t\t * check if any results are ready to be sent back to the\n>>>> +\t\t * framework.\n>>>> +\t\t */\n>>>> +\t\tdescriptor.status_ = Camera3RequestDescriptor::Succeeded;\n>>>> +\t\tdescriptor.resultMetadata_ = std::move(resultMetadata);\n>>>> +\t\tdescriptor.captureResult_ = captureResult;\n>>>> +\t\tstd::unique_ptr reqDescriptor =\n>>>> +\t\t\tstd::make_unique();\n>>>> +\t\t*reqDescriptor = std::move(descriptor);\n>>>> +\t\tqueuedDescriptor_.push_back(std::move(reqDescriptor));\n>>>> +\n>>>> +\t\tsendQueuedCaptureResults();\n>>>> +\t}\n>>>> +}\n>>>> +\n>>>> +void CameraDevice::streamProcessingComplete(CameraStream *cameraStream,\n>>>> +\t\t\t\t\t CameraStream::ProcessStatus status,\n>>>> +\t\t\t\t\t const Camera3RequestDescriptor *context)\n>>>> +{\n>>>> +\tfor (auto &d : queuedDescriptor_) {\n>>> This function is called from the post-processing completion thread, so\n>>> it will race with CameraDevice::requestComplete(). Both access\n>>> queuedDescriptor_ with a lock, it won't end well.\n>> There is no thread here in this patch. It's all synchronous as mentioned\n>> in the commit message. The threading only appears 8/9 patch and there we\n>> need to start looking at racing issues.\n> You're right, but could we introduce the required locking in this patch\n> already ? It's easier to review if the new data structures and the\n> locking come together.\n>\n>>>> +\t\tif (d.get() != context)\n>>>> +\t\t\tcontinue;\n>>> Iterating over a queue to find the element we need sounds like we're not\n>>> using the right type of container. I think you should keep the\n>>> Camera3RequestDescriptor in the map, and look it up from the map here.\n>> No, The descriptor is already getting cleaned up in requestComplete().\n>>\n>> Let me explain:\n>>\n>> As per master right now, the descriptor is extracted from the map at the\n>> start of requestComplete. The lifetime of the descriptor is now only in\n>> the requestComplete() function. Once it returns, the descriptor is\n>> destroyed.\n>>\n>> What am I doing through this series is, adding a descriptor to a queue.\n>> This happens because we don't want to destroy it, rather preserve it\n>> until post-processing completes(well, it's going to happen async, so it\n>> makes more sense). But still it should be removed from the map, just\n>> that, not let it get destroyed.\n>>\n>> Now, why a queue? Because we should be able to send capture results as\n>> per ordering. The descriptors are queued up in order.\n> That's a very clear explanation, I agree with all of this.\n>\n> My comment about looking it up from the map indeed doesn't make too much\n> sense. Reading this function again, can't you just access the\n> Camera3RequestDescriptor from the context parameter ? You'll need to\n> const_cast<> to remove the const, but that's fine, we know here that we\n> can do so because we own the Camera3RequestDescriptor.\n>\n>> Not removing the descriptor from the map might have it's own\n>> implications for e.g. you will need to Request around as well, as the\n>> descriptors_ map is looked with with request->cookie(). I think it's\n>> slightly invasive changes, no?\n> Yes, not a good idea.\n>\n>> Plus I think the \"android: Fix\n>> descriptors_ clean up\" will also conflict to some extend.\n> There could be some conflicts, yes :-S\n>\n> Does my above proposal with a queue owning the request and a map to look\n> it up make sense ? We should ideally even be able to get rid of the map\n> by storing the pointer to the Camera3RequestDescriptor in the Request\n> cookie, but it's currently used to store the CaptureRequest pointer.\n> CaptureRequest should really be dropped as fence handling should go to\n> the libcamera core, but I don't want to include that in this series,\n> it's big enough already. A map is fine as an interim solution.\n>\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>>>> -\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\tcameraStream->putBuffer(d->internalBuffer_);\n>>>> +\n>>>> +\t\tif (status == CameraStream::ProcessStatus::Succeeded) {\n>>>> +\t\t\td->status_ = Camera3RequestDescriptor::Succeeded;\n>>>> +\t\t} else {\n>>>> +\t\t\td->status_ = Camera3RequestDescriptor::Failed;\n>>>> +\n>>>> +\t\t\td->captureResult_.partial_result = 0;\n>>>> +\t\t\tfor (camera3_stream_buffer_t &buffer : d->buffers_) {\n>>>> +\t\t\t\tCameraStream *cs = static_cast(buffer.stream->priv);\n>>>> +\n>>>> +\t\t\t\tif (cs->camera3Stream().format != HAL_PIXEL_FORMAT_BLOB)\n>>>> +\t\t\t\t\tcontinue;\n>>>> +\t\t\t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n>>>> +\t\t\t\tnotifyError(d->frameNumber_, buffer.stream,\n>>>> +\t\t\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n>>>> +\t\t\t}\n>>>> \t\t}\n>>>> +\t\tbreak;\n>>>> \t}\n>>>> \n>>>> -\tcaptureResult.result = resultMetadata->get();\n>>>> -\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n>>>> +\t/*\n>>>> +\t * Send back capture results to the framework by inspecting the queue.\n>>>> +\t * The framework can defer queueing further requests to the HAL (via\n>>>> +\t * process_capture_request) unless until it receives the capture results\n>>>> +\t * for already queued requests.\n>>>> +\t */\n>>>> +\tsendQueuedCaptureResults();\n>>>> +}\n>>>> +\n>>>> +void CameraDevice::sendQueuedCaptureResults()\n>>>> +{\n>>>> +\twhile (!queuedDescriptor_.empty()) {\n>>>> +\t\tstd::unique_ptr &d = queuedDescriptor_.front();\n>>>> +\t\tif (d->status_ == Camera3RequestDescriptor::Pending)\n>>>> +\t\t\treturn;\n>>>> +\n>>>> +\t\td->captureResult_.result = d->resultMetadata_->get();\n>>>> +\t\tcallbacks_->process_capture_result(callbacks_, &(d->captureResult_));\n>>>> +\t\tqueuedDescriptor_.pop_front();\n>>>> +\t}\n>>>> }\n>>>> \n>>>> std::string CameraDevice::logPrefix() const\n>>>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n>>>> index b59ac3e7..e7318358 100644\n>>>> --- a/src/android/camera_device.h\n>>>> +++ b/src/android/camera_device.h\n>>>> @@ -7,6 +7,7 @@\n>>>> #ifndef __ANDROID_CAMERA_DEVICE_H__\n>>>> #define __ANDROID_CAMERA_DEVICE_H__\n>>>> \n>>>> +#include \n>>>> #include \n>>>> #include \n>>>> #include \n>>>> @@ -46,6 +47,20 @@ struct Camera3RequestDescriptor {\n>>>> \tstd::vector> frameBuffers_;\n>>>> \tCameraMetadata settings_;\n>>>> \tstd::unique_ptr request_;\n>>>> +\n>>>> +\t/*\n>>>> +\t * Below are utility placeholders used when a capture result\n>>>> +\t * needs to be queued before completion via process_capture_result()\n>>>> +\t */\n>>>> +\tenum completionStatus {\n>>>> +\t\tPending,\n>>>> +\t\tSucceeded,\n>>>> +\t\tFailed,\n>>>> +\t};\n>>>> +\tstd::unique_ptr resultMetadata_;\n>>>> +\tcamera3_capture_result_t captureResult_;\n>>> This patch is a bit hard to review. Could you split the addition of\n>>> resultMetadata_ and captureResult_ to Camera3RequestDescriptor to a\n>>> separate patch (including addressing the comments above) ?\n>> Yes, it's hard since it setups with the ground work for post-processor.\n>> It has many pieces linked together giving very little room for splitting up.\n>>\n>>>> +\tlibcamera::FrameBuffer *internalBuffer_;\n>>>> +\tcompletionStatus status_;\n>>>> };\n>>>> \n>>>> class CameraDevice : protected libcamera::Loggable\n>>>> @@ -78,6 +93,9 @@ public:\n>>>> \tint processCaptureRequest(camera3_capture_request_t *request);\n>>>> \tvoid requestComplete(libcamera::Request *request);\n>>>> \n>>>> +\tvoid streamProcessingComplete(CameraStream *stream,\n>>>> +\t\t\t\t CameraStream::ProcessStatus status,\n>>>> +\t\t\t\t const Camera3RequestDescriptor *context);\n>>>> protected:\n>>>> \tstd::string logPrefix() const override;\n>>>> \n>>>> @@ -106,6 +124,8 @@ private:\n>>>> \tstd::unique_ptr getResultMetadata(\n>>>> \t\tconst Camera3RequestDescriptor &descriptor) const;\n>>>> \n>>>> +\tvoid sendQueuedCaptureResults();\n>>>> +\n>>>> \tunsigned int id_;\n>>>> \tcamera3_device_t camera3Device_;\n>>>> \n>>>> @@ -126,6 +146,8 @@ private:\n>>>> \tlibcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */\n>>>> \tstd::map descriptors_;\n>>>> \n>>>> +\tstd::deque> queuedDescriptor_;\n>>>> +\n>>>> \tstd::string maker_;\n>>>> \tstd::string model_;\n>>>> \n>>>> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n>>>> index 996779c4..c7d874b2 100644\n>>>> --- a/src/android/camera_stream.cpp\n>>>> +++ b/src/android/camera_stream.cpp\n>>>> @@ -134,6 +134,8 @@ void CameraStream::postProcessingComplete(PostProcessor::Status status,\n>>>> \t\tprocessStatus = ProcessStatus::Succeeded;\n>>>> \telse\n>>>> \t\tprocessStatus = ProcessStatus::Failed;\n>>>> +\n>>>> +\tcameraDevice_->streamProcessingComplete(this, processStatus, context);\n>>>> }\n>>>> \n>>>> FrameBuffer *CameraStream::getBuffer()","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 AD4CFBDC71\n\tfor ;\n\tFri, 17 Sep 2021 08:53:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A05EA6918B;\n\tFri, 17 Sep 2021 10:53:35 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 74EDD69180\n\tfor ;\n\tFri, 17 Sep 2021 10:53:33 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.174])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 562E8510;\n\tFri, 17 Sep 2021 10:53:32 +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=\"L/r5H1BF\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1631868813;\n\tbh=12FD7o9sitUK402C7rniIy/9nOfO/ey8PnbkRjTVlM8=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=L/r5H1BFH3CJPaQoJu/tCnGsNrMif9kABMHvQeUKniPDYFIvJGNHconhVMdlYLpRs\n\tWgex6Gl9wYYQ2BoWbYSYV3GdrCtFrERENvf7Vg76G2OMUIHZS13iAlQxARyGMO0Ad0\n\t8HNZxAATFBteKO9U0Y1oSSz2AU3l/879zTspo19Y=","To":"Laurent Pinchart ","References":"<20210910070638.467294-1-umang.jain@ideasonboard.com>\n\t<20210910070638.467294-7-umang.jain@ideasonboard.com>\n\t\n\t\n\t","From":"Umang Jain ","Message-ID":"","Date":"Fri, 17 Sep 2021 14:23:28 +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":"","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v2 6/9] android: camera_device: Add a\n\tqueue for sending capture results","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":19714,"web_url":"https://patchwork.libcamera.org/comment/19714/","msgid":"","date":"2021-09-17T11:55:13","subject":"Re: [libcamera-devel] [PATCH v2 6/9] android: camera_device: Add a\n\tqueue for sending capture results","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nOn Fri, Sep 17, 2021 at 02:23:28PM +0530, Umang Jain wrote:\n> On 9/15/21 6:58 AM, Laurent Pinchart wrote:\n> > On Mon, Sep 13, 2021 at 12:31:46PM +0530, Umang Jain wrote:\n> >> On 9/13/21 6:29 AM, Laurent Pinchart wrote:\n> >>> On Fri, Sep 10, 2021 at 12:36:35PM +0530, Umang Jain wrote:\n> >>>> When a camera capture request completes, the next step is to send the\n> >>>> capture results to the framework via process_capture_results(). All\n> >>>> the capture associated result metadata is prepared and populated. If\n> >>>> any post-processing is required, it will also take place in the same\n> >>>> thread (which can be blocking for a subtaintial amount of time) before\n> >>>> the results can be sent back to the framework.\n> >>>>\n> >>>> Subsequent patches will move the post-processing to run in a separate\n> >>>> thread. In order to do so, there is few bits of groundwork which this\n> >>>> patch entails. Mainly, we need to preserve the order of sending\n> >>>> the capture results back in which they were queued by the framework\n> >>>> to the HAL (i.e. the order is sequential). Hence, we need to add a queue\n> >>>> in which capture results can be queued with context.\n> >>>>\n> >>>> As per this patch, the post-processor still runs synchronously as\n> >>>> before, but it will queue up the current capture results with context.\n> >>>> Later down the line, the capture results are dequeud in order and\n> >>>> sent back to the framework via sendQueuedCaptureResults(). If the queue\n> >>>> is empty or unused, the current behaviour is to skip the queue and\n> >>>> send capture results directly.\n> >>>>\n> >>>> The context is preserved using Camera3RequestDescriptor utility\n> >>>> structure. It has been expanded accordingly to address the needs of\n> >>>> the functionality.\n> >>>> ---\n> >>>> Check if we can drop unique_ptr to camera3descriptor\n> >>>> ---\n> >>>>\n> >>>> Signed-off-by: Umang Jain \n> >>>> ---\n> >>>> src/android/camera_device.cpp | 106 ++++++++++++++++++++++++++++++----\n> >>>> src/android/camera_device.h | 22 +++++++\n> >>>> src/android/camera_stream.cpp | 2 +\n> >>>> 3 files changed, 119 insertions(+), 11 deletions(-)\n> >>>>\n> >>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> >>>> index 84549d04..7f04d225 100644\n> >>>> --- a/src/android/camera_device.cpp\n> >>>> +++ b/src/android/camera_device.cpp\n> >>>> @@ -240,6 +240,8 @@ Camera3RequestDescriptor::Camera3RequestDescriptor(\n> >>>> \t/* Clone the controls associated with the camera3 request. */\n> >>>> \tsettings_ = CameraMetadata(camera3Request->settings);\n> >>>> \n> >>>> +\tinternalBuffer_ = nullptr;\n> >>>> +\n> >>>> \t/*\n> >>>> \t * Create the CaptureRequest, stored as a unique_ptr<> to tie its\n> >>>> \t * lifetime to the descriptor.\n> >>>> @@ -989,6 +991,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> >>>> \t\t\t * once it has been processed.\n> >>>> \t\t\t */\n> >>>> \t\t\tbuffer = cameraStream->getBuffer();\n> >>>> +\t\t\tdescriptor.internalBuffer_ = buffer;\n> >>>> \t\t\tLOG(HAL, Debug) << ss.str() << \" (internal)\";\n> >>>> \t\t\tbreak;\n> >>>> \t\t}\n> >>>> @@ -1148,26 +1151,107 @@ void CameraDevice::requestComplete(Request *request)\n> >>>> \t\t\tcontinue;\n> >>>> \t\t}\n> >>>> \n> >>>> +\n> >>>> +\t\t/*\n> >>>> +\t\t * Save the current context of capture result and queue the\n> >>>> +\t\t * descriptor before processing the camera stream.\n> >>>> +\t\t *\n> >>>> +\t\t * When the processing completes, the descriptor will be\n> >>>> +\t\t * dequeued and capture results will be sent to the framework.\n> >>>> +\t\t */\n> >>>> +\t\tdescriptor.status_ = Camera3RequestDescriptor::Pending;\n> >>>\n> >>> The status should also be initialized to Pending in the\n> >>> Camera3RequestDescriptor constructor.\n> >>>\n> >>>> +\t\tdescriptor.resultMetadata_ = std::move(resultMetadata);\n> >>>\n> >>> Let's move the resultMetadata to the descriptor right when creating it,\n> >>> it will be cleaner.\n> >>>\n> >>>> +\t\tdescriptor.captureResult_ = captureResult;\n> >>>\n> >>> This causes a copy of a fairly large structure. You can prevent this by\n> >>> replacing\n> >>>\n> >>> \tcamera3_capture_result_t captureResult = {};\n> >>>\n> >>> above with\n> >>>\n> >>> \tcamera3_capture_result_t &captureResult = descriptor.captureResult_;\n> >>> \tcaptureResult = {};\n> >>>\n> >>> or better, in the definition of Camera3RequestDescriptor, replace\n> >>>\n> >>> \tcamera3_capture_result_t captureResult_;\n> >>>\n> >>> with\n> >>>\n> >>> \tcamera3_capture_result_t captureResult_ = {};\n> >>>\n> >>> and drop the initialization in this function. This will ensure that the\n> >>> capture results are correctly initialized as soon as\n> >>> Camera3RequestDescriptor is constructed, avoiding possibly\n> >>> use-before-init issues.\n> >>>\n> >>>> +\n> >>>> +\t\tstd::unique_ptr reqDescriptor =\n> >>>> +\t\t\tstd::make_unique();\n> >>>> +\t\t*reqDescriptor = std::move(descriptor);\n> >>>\n> >>> This will copy the whole class as there's no move constructor for\n> >>> Camera3RequestDescriptor. I don't think that's what you want. If the\n> >>> descriptor needs to be moved out of the map to the queue, without a\n> >>> copy, then they should be stored in the map as unique_ptr already. This\n> >>> should be done as a separate patch before this one.\n> >>\n> >> So, you mean descriptors in the map should be unique_ptr beforehand?\n> >> That will avoid the copy on std::move?\n> >\n> > The only thing that std::move() does is to return an rvalue reference to\n> > the parameter passed to it. That's all. What it implies is that the\n> > assignment will then pick the version of operator=() that takes an\n> > rvalue reference (called the move assignment operator). It's really just\n> > a way to pick one implementation of operator=() over the other.\n> >\n> > For classes such as std::unique_ptr<>, the move assignment operator will\n> > transfer ownership of the pointer. For classes such as std::vector<>, it\n> > will transfer ownership of the internal data, so it's cheap. For\n> > Camera3RequestDescriptor, we don't define any move assignement operator,\n> > so all members will be moved one by one, and for most members, that will\n> > just be a copy.\n> >\n> > Bottom line, moving a std::unique_ptr<> makes sense, moving a\n> > Camera3RequestDescriptor less so.\n> \n> I agree with this particular pointer, that storing \n> Camera3RequestDescriptor can be made via std::unique<> to essentially \n> leverage std::move.\n> \n> But I am not too sure of your strategy below.\n> \n> > After reviewing the whole series, and thinking some more about it, I\n> > think we should introduce a\n> > std::queue> that will own the\n> > Camera3RequestDescriptor instances, and a std::map > Camera3RequestDescriptor *> to lookup the Camera3RequestDescriptor in\n> > CameraDevice::requestComplete(). The queue will have ownership of the\n> > request descriptor, the map will store pointers to the instance stored\n> > in the queue. You'll need to\n> >\n> > - add an entry to the queue in CameraDevice::processCaptureRequest(),\n> > and add it to the map there too, right before queuing the request to\n> > the worker\n> >\n> > - look up the entry from the map in CameraDevice::requestComplete(), and\n> > remove it from the map there\n> >\n> > - handle completion in order using the queue, with a function like\n> > CameraDevice::sendQueuedCaptureResults()\n> >\n> > It would be best, I think, if this rework could be done early in the\n> > series to make it easier to review, and then add the async\n> > post-processor. I haven't looked at it in details though, so maybe\n> > that's not practical.\n> \n> This particular bit is getting complicated here. I understand you want \n> that the ownership is held by a queue but all look-ups happen via a map \n> from the start, but there are error paths that will clean up the \n> descriptors be removing it from the map and free the buffers held by it \n> right away. If the ownership is held with the queue, we need to clean it \n> up from the queue, to actually free the buffers right? .... And queue \n> might need to get re-adjusted with remaining elements which might not be \n> that great. It's adding up complexity (at least from error-ed requests \n> point of view), that when iterating over the queue, a descriptor is \n> present in the queue, but has been removed from the map, which the queue \n> is supposed to do with it?\n> \n> I am still pondering over it (and also looking at \"android: Fix \n> descriptors_ clean up\" series), trying to answer my questions but it's \n> making me a bit scared.\n\nMy rationale is that requests are queued by Android and are expected by\nAndroid to complete in the same order. Camera3RequestDescriptor is the\ncounterpart of camera3_capture_request_t, I think it should have the\nsame lifetime as camera3_capture_request_t, and follow the same rules. A\nqueue thus seems to be the best storage.\n\nThe map is there only to look up the Camera3RequestDescriptor when the\nlibcamera Request completes. This would normally be done using\nRequest::cookie(), but the request cookie is handled by the\nCameraWorker. Looking at it, CameraWorker sets the cookie to a pointer\nto CameraRequest, but nobody actually depends on that. I think you could\npass a uint64_t cookie to the CameraRequest constructor, and pass\nreinterpret_cast(this) to it from the Camera3RequestDescriptor\nconstructor. We should then be able to get rid of the map.\n\n> >>> I wonder if we shouldn't keep the descriptor in the map until we finish\n> >>> post-processing, as that would help implementing partial completion\n> >>> support in the future (where we would connect the bufferComplete signal\n> >>> and start post-processing of buffers before libcamera completes the\n> >>> whole request). Maybe that will be best handled in the future, not now,\n> >>> so I think I'm fine moving descriptors from the map to the queue for\n> >>> now.\n> >>\n> >> Ack.\n> >>\n> >>>> +\t\tqueuedDescriptor_.push_back(std::move(reqDescriptor));\n> >>>> +\n> >>>> +\t\tCamera3RequestDescriptor *currentDescriptor = queuedDescriptor_.back().get();\n> >>>> \t\tint ret = cameraStream->process(src, *buffer.buffer,\n> >>>\n> >>> ret is unused, this causes a compilation failure. Could you please\n> >>> compile-test individual patches in the series ?\n> >>>\n> >>> Now that camera stream processing completion is signaled, the process()\n> >>> function should become void. This can go in a separate patch.\n> >>>\n> >>>> -\t\t\t\t\t\tdescriptor.settings_,\n> >>>> -\t\t\t\t\t\tresultMetadata.get(),\n> >>>> -\t\t\t\t\t\t&descriptor);\n> >>>> +\t\t\t\t\t\tcurrentDescriptor->settings_,\n> >>>> +\t\t\t\t\t\tcurrentDescriptor->resultMetadata_.get(),\n> >>>\n> >>> You can drop the resultMetadata parameter from the process() function\n> >>> and access it through the descriptor instead.\n> >>\n> >> As I mentioned in the previous replies, I think fields that are modified\n> >> as part of process(), should be passed explitcity. The descriptor I pass\n> >> down the line, is a const.\n> >>\n> >>>> +\t\t\t\t\t\tcurrentDescriptor);\n> >>>> +\t\treturn;\n> >>>> +\t}\n> >>>> +\n> >>>> +\tif (queuedDescriptor_.empty()) {\n> >>>> +\t\tcaptureResult.result = resultMetadata->get();\n> >>>> +\t\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n> >>>> +\t} else {\n> >>>> +\t\t/*\n> >>>> +\t\t * Previous capture results waiting to be sent to framework\n> >>>> +\t\t * hence, queue the current capture results as well. After that,\n> >>>> +\t\t * check if any results are ready to be sent back to the\n> >>>> +\t\t * framework.\n> >>>> +\t\t */\n> >>>> +\t\tdescriptor.status_ = Camera3RequestDescriptor::Succeeded;\n> >>>> +\t\tdescriptor.resultMetadata_ = std::move(resultMetadata);\n> >>>> +\t\tdescriptor.captureResult_ = captureResult;\n> >>>> +\t\tstd::unique_ptr reqDescriptor =\n> >>>> +\t\t\tstd::make_unique();\n> >>>> +\t\t*reqDescriptor = std::move(descriptor);\n> >>>> +\t\tqueuedDescriptor_.push_back(std::move(reqDescriptor));\n> >>>> +\n> >>>> +\t\tsendQueuedCaptureResults();\n> >>>> +\t}\n> >>>> +}\n> >>>> +\n> >>>> +void CameraDevice::streamProcessingComplete(CameraStream *cameraStream,\n> >>>> +\t\t\t\t\t CameraStream::ProcessStatus status,\n> >>>> +\t\t\t\t\t const Camera3RequestDescriptor *context)\n> >>>> +{\n> >>>> +\tfor (auto &d : queuedDescriptor_) {\n> >>>\n> >>> This function is called from the post-processing completion thread, so\n> >>> it will race with CameraDevice::requestComplete(). Both access\n> >>> queuedDescriptor_ with a lock, it won't end well.\n> >>\n> >> There is no thread here in this patch. It's all synchronous as mentioned\n> >> in the commit message. The threading only appears 8/9 patch and there we\n> >> need to start looking at racing issues.\n> >\n> > You're right, but could we introduce the required locking in this patch\n> > already ? It's easier to review if the new data structures and the\n> > locking come together.\n> >\n> >>>> +\t\tif (d.get() != context)\n> >>>> +\t\t\tcontinue;\n> >>>\n> >>> Iterating over a queue to find the element we need sounds like we're not\n> >>> using the right type of container. I think you should keep the\n> >>> Camera3RequestDescriptor in the map, and look it up from the map here.\n> >>\n> >> No, The descriptor is already getting cleaned up in requestComplete().\n> >>\n> >> Let me explain:\n> >>\n> >> As per master right now, the descriptor is extracted from the map at the\n> >> start of requestComplete. The lifetime of the descriptor is now only in\n> >> the requestComplete() function. Once it returns, the descriptor is\n> >> destroyed.\n> >>\n> >> What am I doing through this series is, adding a descriptor to a queue.\n> >> This happens because we don't want to destroy it, rather preserve it\n> >> until post-processing completes(well, it's going to happen async, so it\n> >> makes more sense). But still it should be removed from the map, just\n> >> that, not let it get destroyed.\n> >>\n> >> Now, why a queue? Because we should be able to send capture results as\n> >> per ordering. The descriptors are queued up in order.\n> >\n> > That's a very clear explanation, I agree with all of this.\n> >\n> > My comment about looking it up from the map indeed doesn't make too much\n> > sense. Reading this function again, can't you just access the\n> > Camera3RequestDescriptor from the context parameter ? You'll need to\n> > const_cast<> to remove the const, but that's fine, we know here that we\n> > can do so because we own the Camera3RequestDescriptor.\n> >\n> >> Not removing the descriptor from the map might have it's own\n> >> implications for e.g. you will need to Request around as well, as the\n> >> descriptors_ map is looked with with request->cookie(). I think it's\n> >> slightly invasive changes, no?\n> >\n> > Yes, not a good idea.\n> >\n> >> Plus I think the \"android: Fix\n> >> descriptors_ clean up\" will also conflict to some extend.\n> >\n> > There could be some conflicts, yes :-S\n> >\n> > Does my above proposal with a queue owning the request and a map to look\n> > it up make sense ? We should ideally even be able to get rid of the map\n> > by storing the pointer to the Camera3RequestDescriptor in the Request\n> > cookie, but it's currently used to store the CaptureRequest pointer.\n> > CaptureRequest should really be dropped as fence handling should go to\n> > the libcamera core, but I don't want to include that in this series,\n> > it's big enough already. A map is fine as an interim solution.\n> >\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> >>>> -\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\tcameraStream->putBuffer(d->internalBuffer_);\n> >>>> +\n> >>>> +\t\tif (status == CameraStream::ProcessStatus::Succeeded) {\n> >>>> +\t\t\td->status_ = Camera3RequestDescriptor::Succeeded;\n> >>>> +\t\t} else {\n> >>>> +\t\t\td->status_ = Camera3RequestDescriptor::Failed;\n> >>>> +\n> >>>> +\t\t\td->captureResult_.partial_result = 0;\n> >>>> +\t\t\tfor (camera3_stream_buffer_t &buffer : d->buffers_) {\n> >>>> +\t\t\t\tCameraStream *cs = static_cast(buffer.stream->priv);\n> >>>> +\n> >>>> +\t\t\t\tif (cs->camera3Stream().format != HAL_PIXEL_FORMAT_BLOB)\n> >>>> +\t\t\t\t\tcontinue;\n> >>>> +\t\t\t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> >>>> +\t\t\t\tnotifyError(d->frameNumber_, buffer.stream,\n> >>>> +\t\t\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n> >>>> +\t\t\t}\n> >>>> \t\t}\n> >>>> +\t\tbreak;\n> >>>> \t}\n> >>>> \n> >>>> -\tcaptureResult.result = resultMetadata->get();\n> >>>> -\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n> >>>> +\t/*\n> >>>> +\t * Send back capture results to the framework by inspecting the queue.\n> >>>> +\t * The framework can defer queueing further requests to the HAL (via\n> >>>> +\t * process_capture_request) unless until it receives the capture results\n> >>>> +\t * for already queued requests.\n> >>>> +\t */\n> >>>> +\tsendQueuedCaptureResults();\n> >>>> +}\n> >>>> +\n> >>>> +void CameraDevice::sendQueuedCaptureResults()\n> >>>> +{\n> >>>> +\twhile (!queuedDescriptor_.empty()) {\n> >>>> +\t\tstd::unique_ptr &d = queuedDescriptor_.front();\n> >>>> +\t\tif (d->status_ == Camera3RequestDescriptor::Pending)\n> >>>> +\t\t\treturn;\n> >>>> +\n> >>>> +\t\td->captureResult_.result = d->resultMetadata_->get();\n> >>>> +\t\tcallbacks_->process_capture_result(callbacks_, &(d->captureResult_));\n> >>>> +\t\tqueuedDescriptor_.pop_front();\n> >>>> +\t}\n> >>>> }\n> >>>> \n> >>>> std::string CameraDevice::logPrefix() const\n> >>>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> >>>> index b59ac3e7..e7318358 100644\n> >>>> --- a/src/android/camera_device.h\n> >>>> +++ b/src/android/camera_device.h\n> >>>> @@ -7,6 +7,7 @@\n> >>>> #ifndef __ANDROID_CAMERA_DEVICE_H__\n> >>>> #define __ANDROID_CAMERA_DEVICE_H__\n> >>>> \n> >>>> +#include \n> >>>> #include \n> >>>> #include \n> >>>> #include \n> >>>> @@ -46,6 +47,20 @@ struct Camera3RequestDescriptor {\n> >>>> \tstd::vector> frameBuffers_;\n> >>>> \tCameraMetadata settings_;\n> >>>> \tstd::unique_ptr request_;\n> >>>> +\n> >>>> +\t/*\n> >>>> +\t * Below are utility placeholders used when a capture result\n> >>>> +\t * needs to be queued before completion via process_capture_result()\n> >>>> +\t */\n> >>>> +\tenum completionStatus {\n> >>>> +\t\tPending,\n> >>>> +\t\tSucceeded,\n> >>>> +\t\tFailed,\n> >>>> +\t};\n> >>>> +\tstd::unique_ptr resultMetadata_;\n> >>>> +\tcamera3_capture_result_t captureResult_;\n> >>>\n> >>> This patch is a bit hard to review. Could you split the addition of\n> >>> resultMetadata_ and captureResult_ to Camera3RequestDescriptor to a\n> >>> separate patch (including addressing the comments above) ?\n> >>\n> >> Yes, it's hard since it setups with the ground work for post-processor.\n> >> It has many pieces linked together giving very little room for splitting up.\n> >>\n> >>>> +\tlibcamera::FrameBuffer *internalBuffer_;\n> >>>> +\tcompletionStatus status_;\n> >>>> };\n> >>>> \n> >>>> class CameraDevice : protected libcamera::Loggable\n> >>>> @@ -78,6 +93,9 @@ public:\n> >>>> \tint processCaptureRequest(camera3_capture_request_t *request);\n> >>>> \tvoid requestComplete(libcamera::Request *request);\n> >>>> \n> >>>> +\tvoid streamProcessingComplete(CameraStream *stream,\n> >>>> +\t\t\t\t CameraStream::ProcessStatus status,\n> >>>> +\t\t\t\t const Camera3RequestDescriptor *context);\n> >>>> protected:\n> >>>> \tstd::string logPrefix() const override;\n> >>>> \n> >>>> @@ -106,6 +124,8 @@ private:\n> >>>> \tstd::unique_ptr getResultMetadata(\n> >>>> \t\tconst Camera3RequestDescriptor &descriptor) const;\n> >>>> \n> >>>> +\tvoid sendQueuedCaptureResults();\n> >>>> +\n> >>>> \tunsigned int id_;\n> >>>> \tcamera3_device_t camera3Device_;\n> >>>> \n> >>>> @@ -126,6 +146,8 @@ private:\n> >>>> \tlibcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */\n> >>>> \tstd::map descriptors_;\n> >>>> \n> >>>> +\tstd::deque> queuedDescriptor_;\n> >>>> +\n> >>>> \tstd::string maker_;\n> >>>> \tstd::string model_;\n> >>>> \n> >>>> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> >>>> index 996779c4..c7d874b2 100644\n> >>>> --- a/src/android/camera_stream.cpp\n> >>>> +++ b/src/android/camera_stream.cpp\n> >>>> @@ -134,6 +134,8 @@ void CameraStream::postProcessingComplete(PostProcessor::Status status,\n> >>>> \t\tprocessStatus = ProcessStatus::Succeeded;\n> >>>> \telse\n> >>>> \t\tprocessStatus = ProcessStatus::Failed;\n> >>>> +\n> >>>> +\tcameraDevice_->streamProcessingComplete(this, processStatus, context);\n> >>>> }\n> >>>> \n> >>>> FrameBuffer *CameraStream::getBuffer()","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 C9081BF01C\n\tfor ;\n\tFri, 17 Sep 2021 11:55:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BFF5E69189;\n\tFri, 17 Sep 2021 13:55:43 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3226369186\n\tfor ;\n\tFri, 17 Sep 2021 13:55:42 +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 9ADE61B3F;\n\tFri, 17 Sep 2021 13:55:41 +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=\"J1sShToW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1631879741;\n\tbh=L4uqFwQBFvI/6k0w0LVDNrKetW8TF1gaFu7xMhT6Y9g=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=J1sShToWQEWeC6Xkk6p7uhmuSEJ32OcUD1dEl9XMX4cPc3N/WlvWAsjE+NI200p1P\n\tBf21NCpUTz1u4R97Al/rwyi6UrJewTFfuAXi29sZ9VfVqng8LsFhr9uwbSvuJg2ytN\n\t3XJOgZ4vba6rVJoAoUUYr/m6Mh+y3c6eQHT/gq3c=","Date":"Fri, 17 Sep 2021 14:55:13 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20210910070638.467294-1-umang.jain@ideasonboard.com>\n\t<20210910070638.467294-7-umang.jain@ideasonboard.com>\n\t\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 6/9] android: camera_device: Add a\n\tqueue for sending capture results","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\" "}}]