[{"id":19629,"web_url":"https://patchwork.libcamera.org/comment/19629/","msgid":"","date":"2021-09-13T01:21:20","subject":"Re: [libcamera-devel] [PATCH v2 8/9] android: camera_stream: Run\n\tpost-processor in a separate thread","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:37PM +0530, Umang Jain wrote:\n> In CameraStream, introduce a new private PostProcessorWorker\n> class derived from Object. A instance of PostProcessorWorker\n> is moved to a separate thread instance which will be responsible\n> to run the post-processor.\n> \n> Running PostProcessor asynchronously should entail that all the\n> data context needed by the PostProcessor should remain valid for\n> the entire duration of its run. Most of the context preserving\n> part has been addressed in the previous commits, we just need to\n> ensure the source framebuffer data that comes via Camera::Request,\n> should remain valid for the entire duration of post-processing\n> running asynchronously. In order to so, we maintain a separate\n> copy of the framebuffer data and add it to the Camera3RequestDescriptor\n> structure in which we preserve rest of the context.\n> \n> Signed-off-by: Umang Jain \n> ---\n> src/android/camera_device.cpp | 19 ++++++++++++++++++-\n> src/android/camera_device.h | 4 ++++\n> src/android/camera_stream.cpp | 16 ++++++++++++++--\n> src/android/camera_stream.h | 28 ++++++++++++++++++++++++++++\n> 4 files changed, 64 insertions(+), 3 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 988d4232..73eb5758 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -1174,13 +1174,29 @@ void CameraDevice::requestComplete(Request *request)\n> \t\t\treturn;\n> \t\t}\n> \n> +\t\tFrameBuffer *source = src;\n> +\t\tif (cameraStream->type() != CameraStream::Type::Internal) {\n> +\t\t\t/*\n> +\t\t\t * The source buffer is owned by Request object which\n> +\t\t\t * can be reused by libcamera. Since post-processor will\n> +\t\t\t * run asynchrnously, we need to copy the request's\n> +\t\t\t * frame buffer and use that as the source buffer for\n> +\t\t\t * post processing.\n\nI don't think this is right. The request can be reused indeed, but with\nnew FrameBuffer instances every time. The frame buffers are owned by\nCamera3RequestDescriptor, they're stored in a vector there.\n\n> +\t\t\t */\n> +\t\t\tfor (const auto &plane : src->planes())\n> +\t\t\t\tdescriptor.srcPlanes_.push_back(plane);\n> +\t\t\tdescriptor.srcFramebuffer_ =\n> +\t\t\t\tstd::make_unique(descriptor.srcPlanes_);\n> +\t\t\tsource = descriptor.srcFramebuffer_.get();\n> +\t\t}\n> +\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\tCamera3RequestDescriptor *currentDescriptor = queuedDescriptor_.back().get();\n> -\t\tint ret = cameraStream->process(src,\n> +\t\tint ret = cameraStream->process(source,\n> \t\t\t\t\t\tcurrentDescriptor->destBuffer_.get(),\n> \t\t\t\t\t\tcurrentDescriptor->settings_,\n> \t\t\t\t\t\tcurrentDescriptor->resultMetadata_.get(),\n> @@ -1255,6 +1271,7 @@ void CameraDevice::streamProcessingComplete(CameraStream *cameraStream,\n> \n> void CameraDevice::sendQueuedCaptureResults()\n> {\n> +\tMutexLocker lock(queuedDescriptorsMutex_);\n> \twhile (!queuedDescriptor_.empty()) {\n> \t\tstd::unique_ptr &d = queuedDescriptor_.front();\n> \t\tif (d->status_ == Camera3RequestDescriptor::Pending)\n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index b62d373c..ecdda06e 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -62,6 +62,9 @@ struct Camera3RequestDescriptor {\n> \tcamera3_capture_result_t captureResult_;\n> \tlibcamera::FrameBuffer *internalBuffer_;\n> \tcompletionStatus status_;\n> +\n> +\tstd::unique_ptr srcFramebuffer_;\n> +\tstd::vector srcPlanes_;\n> };\n> \n> class CameraDevice : protected libcamera::Loggable\n> @@ -147,6 +150,7 @@ private:\n> \tlibcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */\n> \tstd::map descriptors_;\n> \n> +\tlibcamera::Mutex queuedDescriptorsMutex_; /* Protects queuedDescriptor_. */\n\nThis belongs to the previous patch.\n\n> \tstd::deque> queuedDescriptor_;\n> \n> \tstd::string maker_;\n> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> index 5fd04bbf..845e2462 100644\n> --- a/src/android/camera_stream.cpp\n> +++ b/src/android/camera_stream.cpp\n> @@ -55,6 +55,15 @@ CameraStream::CameraStream(CameraDevice *const cameraDevice,\n> \t\t * is what we instantiate here.\n> \t\t */\n> \t\tpostProcessor_ = std::make_unique(cameraDevice_);\n> +\t\tppWorker_ = std::make_unique(postProcessor_.get());\n> +\n> +\t\tthread_ = std::make_unique();\n> +\t\tppWorker_->moveToThread(thread_.get());\n> +\t\t/*\n> +\t\t * \\todo: Class is MoveConstructible, so where to stop thread\n> +\t\t * if we don't user-defined destructor? See RFC patch at the end.\n> +\t\t */\n> +\t\tthread_->start();\n> \t}\n> \n> \tif (type == Type::Internal) {\n> @@ -110,8 +119,11 @@ int CameraStream::process(const FrameBuffer *source,\n> \tif (!postProcessor_)\n> \t\treturn 0;\n> \n> -\treturn postProcessor_->process(source, destBuffer, requestMetadata,\n> -\t\t\t\t resultMetadata, context);\n> +\tppWorker_->invokeMethod(&PostProcessorWorker::process,\n> +\t\t\t\tConnectionTypeQueued, source, destBuffer,\n> +\t\t\t\trequestMetadata, resultMetadata, context);\n> +\n> +\treturn 0;\n> }\n> \n> void CameraStream::postProcessingComplete(PostProcessor::Status status,\n> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n> index 8097ddbc..dbb7eee3 100644\n> --- a/src/android/camera_stream.h\n> +++ b/src/android/camera_stream.h\n> @@ -13,7 +13,9 @@\n> \n> #include \n> \n> +#include \n> #include \n> +#include \n> \n> #include \n> #include \n> @@ -23,6 +25,7 @@\n> \n> #include \"post_processor.h\"\n> \n> +class CameraBuffer;\n> class CameraDevice;\n> class CameraMetadata;\n> \n> @@ -137,6 +140,29 @@ public:\n> \t};\n> \n> private:\n> +\tclass PostProcessorWorker : public libcamera::Object\n> +\t{\n> +\tpublic:\n> +\t\tPostProcessorWorker(PostProcessor *postProcessor)\n> +\t\t{\n> +\t\t\tpostProcessor_ = postProcessor;\n> +\t\t}\n> +\n> +\t\tvoid process(const libcamera::FrameBuffer *source,\n> +\t\t\t CameraBuffer *destination,\n> +\t\t\t const CameraMetadata &requestMetadata,\n> +\t\t\t CameraMetadata *resultMetadata,\n> +\t\t\t const Camera3RequestDescriptor *context)\n> +\t\t{\n> +\t\t\tpostProcessor_->process(source, destination,\n> +\t\t\t\t\t\trequestMetadata, resultMetadata,\n> +\t\t\t\t\t\tcontext);\n> +\t\t}\n> +\n> +\tprivate:\n> +\t\tPostProcessor *postProcessor_;\n> +\t};\n\nIf it wasn't for the fact that I mentioned in the review of v1 that it\nwould be best to keep the PostProcessor class not thread-aware, I'd be\ntempted to say we could make PostProcessor inherit from Object and drop\nthis class completely :-) Would you be tempted to do so too ? Compared\nto v1 (the RFC) it would resurect patch 1/5, but we wouldn't need the\nPostProcessorWorker class at all.\n\nOn the other hand, given that I've mentioned in the review of a 7/9 that\nmapping the destination buffer could be done in the post-processor\nthread, maybe PostProcessorWorker::process() is the right place to do\nso. And now that I've written that, I now realize we've split the\nconstruction of CameraBuffer from the actual mapping, which is performed\non the first call to CameraBuffer::plane(), so the mapping is already\ndone in the post-processor thread. It's thus likely fine to have the\nconstruction of CameraBuffer in CameraStream or CameraDevice.\n\n> +\n> \tvoid postProcessingComplete(PostProcessor::Status status,\n> \t\t\t\t const Camera3RequestDescriptor *context);\n> \n> @@ -154,6 +180,8 @@ private:\n> \t */\n> \tstd::unique_ptr mutex_;\n> \tstd::unique_ptr postProcessor_;\n> +\tstd::unique_ptr ppWorker_;\n> +\tstd::unique_ptr thread_;\n> };\n> \n> #endif /* __ANDROID_CAMERA_STREAM__ */","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 0A338BDB1D\n\tfor ;\n\tMon, 13 Sep 2021 01:21:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 75FCB69184;\n\tMon, 13 Sep 2021 03:21:46 +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 C709960250\n\tfor ;\n\tMon, 13 Sep 2021 03:21:44 +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 353489E;\n\tMon, 13 Sep 2021 03:21:44 +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+5MO1pL\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1631496104;\n\tbh=N1SXJWr5XErqEmnB3fB1haffIxn6G4+VJ3kEE+0JPFc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=l+5MO1pLyv/OSIgiNAcigLqQRtjw+BHeUjEpSLr31Donwne7tgGeuoqn3y7n68bLe\n\tY/sKuNtd0vlOZSFnaA9NTOIuNZQqNCqTC5lrDTJKx6ytayB/1sppwajJvpsdXUnADe\n\tQ8IJf4fZVUsaHUigbSGvU134+iz3v+HXeOv8UmHM=","Date":"Mon, 13 Sep 2021 04:21:20 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20210910070638.467294-1-umang.jain@ideasonboard.com>\n\t<20210910070638.467294-9-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210910070638.467294-9-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 8/9] android: camera_stream: Run\n\tpost-processor in a separate thread","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":19636,"web_url":"https://patchwork.libcamera.org/comment/19636/","msgid":"<832740db-14aa-c0c8-1714-2d105c4f9db9@ideasonboard.com>","date":"2021-09-13T07:09:11","subject":"Re: [libcamera-devel] [PATCH v2 8/9] android: camera_stream: Run\n\tpost-processor in a separate thread","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:51 AM, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> Thank you for the patch.\n>\n> On Fri, Sep 10, 2021 at 12:36:37PM +0530, Umang Jain wrote:\n>> In CameraStream, introduce a new private PostProcessorWorker\n>> class derived from Object. A instance of PostProcessorWorker\n>> is moved to a separate thread instance which will be responsible\n>> to run the post-processor.\n>>\n>> Running PostProcessor asynchronously should entail that all the\n>> data context needed by the PostProcessor should remain valid for\n>> the entire duration of its run. Most of the context preserving\n>> part has been addressed in the previous commits, we just need to\n>> ensure the source framebuffer data that comes via Camera::Request,\n>> should remain valid for the entire duration of post-processing\n>> running asynchronously. In order to so, we maintain a separate\n>> copy of the framebuffer data and add it to the Camera3RequestDescriptor\n>> structure in which we preserve rest of the context.\n>>\n>> Signed-off-by: Umang Jain \n>> ---\n>> src/android/camera_device.cpp | 19 ++++++++++++++++++-\n>> src/android/camera_device.h | 4 ++++\n>> src/android/camera_stream.cpp | 16 ++++++++++++++--\n>> src/android/camera_stream.h | 28 ++++++++++++++++++++++++++++\n>> 4 files changed, 64 insertions(+), 3 deletions(-)\n>>\n>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>> index 988d4232..73eb5758 100644\n>> --- a/src/android/camera_device.cpp\n>> +++ b/src/android/camera_device.cpp\n>> @@ -1174,13 +1174,29 @@ void CameraDevice::requestComplete(Request *request)\n>> \t\t\treturn;\n>> \t\t}\n>> \n>> +\t\tFrameBuffer *source = src;\n>> +\t\tif (cameraStream->type() != CameraStream::Type::Internal) {\n>> +\t\t\t/*\n>> +\t\t\t * The source buffer is owned by Request object which\n>> +\t\t\t * can be reused by libcamera. Since post-processor will\n>> +\t\t\t * run asynchrnously, we need to copy the request's\n>> +\t\t\t * frame buffer and use that as the source buffer for\n>> +\t\t\t * post processing.\n> I don't think this is right. The request can be reused indeed, but with\n> new FrameBuffer instances every time. The frame buffers are owned by\n> Camera3RequestDescriptor, they're stored in a vector there.\n\n\nYes, I realized by discussion with Jacopo. I think we don't need a copy \nhere, working on it as we speak.\n\n>\n>> +\t\t\t */\n>> +\t\t\tfor (const auto &plane : src->planes())\n>> +\t\t\t\tdescriptor.srcPlanes_.push_back(plane);\n>> +\t\t\tdescriptor.srcFramebuffer_ =\n>> +\t\t\t\tstd::make_unique(descriptor.srcPlanes_);\n>> +\t\t\tsource = descriptor.srcFramebuffer_.get();\n>> +\t\t}\n>> +\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\tCamera3RequestDescriptor *currentDescriptor = queuedDescriptor_.back().get();\n>> -\t\tint ret = cameraStream->process(src,\n>> +\t\tint ret = cameraStream->process(source,\n>> \t\t\t\t\t\tcurrentDescriptor->destBuffer_.get(),\n>> \t\t\t\t\t\tcurrentDescriptor->settings_,\n>> \t\t\t\t\t\tcurrentDescriptor->resultMetadata_.get(),\n>> @@ -1255,6 +1271,7 @@ void CameraDevice::streamProcessingComplete(CameraStream *cameraStream,\n>> \n>> void CameraDevice::sendQueuedCaptureResults()\n>> {\n>> +\tMutexLocker lock(queuedDescriptorsMutex_);\n>> \twhile (!queuedDescriptor_.empty()) {\n>> \t\tstd::unique_ptr &d = queuedDescriptor_.front();\n>> \t\tif (d->status_ == Camera3RequestDescriptor::Pending)\n>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n>> index b62d373c..ecdda06e 100644\n>> --- a/src/android/camera_device.h\n>> +++ b/src/android/camera_device.h\n>> @@ -62,6 +62,9 @@ struct Camera3RequestDescriptor {\n>> \tcamera3_capture_result_t captureResult_;\n>> \tlibcamera::FrameBuffer *internalBuffer_;\n>> \tcompletionStatus status_;\n>> +\n>> +\tstd::unique_ptr srcFramebuffer_;\n>> +\tstd::vector srcPlanes_;\n>> };\n>> \n>> class CameraDevice : protected libcamera::Loggable\n>> @@ -147,6 +150,7 @@ private:\n>> \tlibcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */\n>> \tstd::map descriptors_;\n>> \n>> +\tlibcamera::Mutex queuedDescriptorsMutex_; /* Protects queuedDescriptor_. */\n> This belongs to the previous patch.\n\n\nNo, I think as this patch is the one which introduces the thread, it \nshould introduce the necessary locking too. Correct me if I am wrong.\n\n>\n>> \tstd::deque> queuedDescriptor_;\n>> \n>> \tstd::string maker_;\n>> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n>> index 5fd04bbf..845e2462 100644\n>> --- a/src/android/camera_stream.cpp\n>> +++ b/src/android/camera_stream.cpp\n>> @@ -55,6 +55,15 @@ CameraStream::CameraStream(CameraDevice *const cameraDevice,\n>> \t\t * is what we instantiate here.\n>> \t\t */\n>> \t\tpostProcessor_ = std::make_unique(cameraDevice_);\n>> +\t\tppWorker_ = std::make_unique(postProcessor_.get());\n>> +\n>> +\t\tthread_ = std::make_unique();\n>> +\t\tppWorker_->moveToThread(thread_.get());\n>> +\t\t/*\n>> +\t\t * \\todo: Class is MoveConstructible, so where to stop thread\n>> +\t\t * if we don't user-defined destructor? See RFC patch at the end.\n>> +\t\t */\n>> +\t\tthread_->start();\n>> \t}\n>> \n>> \tif (type == Type::Internal) {\n>> @@ -110,8 +119,11 @@ int CameraStream::process(const FrameBuffer *source,\n>> \tif (!postProcessor_)\n>> \t\treturn 0;\n>> \n>> -\treturn postProcessor_->process(source, destBuffer, requestMetadata,\n>> -\t\t\t\t resultMetadata, context);\n>> +\tppWorker_->invokeMethod(&PostProcessorWorker::process,\n>> +\t\t\t\tConnectionTypeQueued, source, destBuffer,\n>> +\t\t\t\trequestMetadata, resultMetadata, context);\n>> +\n>> +\treturn 0;\n>> }\n>> \n>> void CameraStream::postProcessingComplete(PostProcessor::Status status,\n>> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n>> index 8097ddbc..dbb7eee3 100644\n>> --- a/src/android/camera_stream.h\n>> +++ b/src/android/camera_stream.h\n>> @@ -13,7 +13,9 @@\n>> \n>> #include \n>> \n>> +#include \n>> #include \n>> +#include \n>> \n>> #include \n>> #include \n>> @@ -23,6 +25,7 @@\n>> \n>> #include \"post_processor.h\"\n>> \n>> +class CameraBuffer;\n>> class CameraDevice;\n>> class CameraMetadata;\n>> \n>> @@ -137,6 +140,29 @@ public:\n>> \t};\n>> \n>> private:\n>> +\tclass PostProcessorWorker : public libcamera::Object\n>> +\t{\n>> +\tpublic:\n>> +\t\tPostProcessorWorker(PostProcessor *postProcessor)\n>> +\t\t{\n>> +\t\t\tpostProcessor_ = postProcessor;\n>> +\t\t}\n>> +\n>> +\t\tvoid process(const libcamera::FrameBuffer *source,\n>> +\t\t\t CameraBuffer *destination,\n>> +\t\t\t const CameraMetadata &requestMetadata,\n>> +\t\t\t CameraMetadata *resultMetadata,\n>> +\t\t\t const Camera3RequestDescriptor *context)\n>> +\t\t{\n>> +\t\t\tpostProcessor_->process(source, destination,\n>> +\t\t\t\t\t\trequestMetadata, resultMetadata,\n>> +\t\t\t\t\t\tcontext);\n>> +\t\t}\n>> +\n>> +\tprivate:\n>> +\t\tPostProcessor *postProcessor_;\n>> +\t};\n> If it wasn't for the fact that I mentioned in the review of v1 that it\n> would be best to keep the PostProcessor class not thread-aware, I'd be\n> tempted to say we could make PostProcessor inherit from Object and drop\n> this class completely :-) Would you be tempted to do so too ? Compared\n\n\nI was tempted that from the start which was RFC v1. I should start to \ntrust my gut more....\n\n> to v1 (the RFC) it would resurect patch 1/5, but we wouldn't need the\n> PostProcessorWorker class at all.\n\n\nhmmm....\n\n>\n> On the other hand, given that I've mentioned in the review of a 7/9 that\n> mapping the destination buffer could be done in the post-processor\n> thread, maybe PostProcessorWorker::process() is the right place to do\n> so. And now that I've written that, I now realize we've split the\n> construction of CameraBuffer from the actual mapping, which is performed\n> on the first call to CameraBuffer::plane(), so the mapping is already\n> done in the post-processor thread. It's thus likely fine to have the\n> construction of CameraBuffer in CameraStream or CameraDevice.\n\n\nAha, very interesting. So I think this brings one more \\todo down in the \ncode base\n\n>\n>> +\n>> \tvoid postProcessingComplete(PostProcessor::Status status,\n>> \t\t\t\t const Camera3RequestDescriptor *context);\n>> \n>> @@ -154,6 +180,8 @@ private:\n>> \t */\n>> \tstd::unique_ptr mutex_;\n>> \tstd::unique_ptr postProcessor_;\n>> +\tstd::unique_ptr ppWorker_;\n>> +\tstd::unique_ptr thread_;\n>> };\n>> \n>> #endif /* __ANDROID_CAMERA_STREAM__ */","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 EAB62BDC71\n\tfor ;\n\tMon, 13 Sep 2021 07:09:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6211969187;\n\tMon, 13 Sep 2021 09:09:17 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7019D60132\n\tfor ;\n\tMon, 13 Sep 2021 09:09:16 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.152])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 87E749E;\n\tMon, 13 Sep 2021 09:09:15 +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=\"NeBhXvxm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1631516956;\n\tbh=ZLkg6R/stjoB4VcTKutVO9U9rqgVhdyojylCXV11zNc=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=NeBhXvxmayYu0d6bozhP2sfA7DlaWEYmdBdS6ZMxpYb5H9MQ1o6VmfygZp1EOLOtV\n\t7JX/rPZYZhgKUXQhAAGJh3cQgMocEwP0/GpQMSY/YSdl7ED8ew+FH2l3G+imCjCl9l\n\tO1s6iKCg4RKjqj/c+iR9QEjoTZofUdAtiQk6RctM=","To":"Laurent Pinchart ","References":"<20210910070638.467294-1-umang.jain@ideasonboard.com>\n\t<20210910070638.467294-9-umang.jain@ideasonboard.com>\n\t","From":"Umang Jain ","Message-ID":"<832740db-14aa-c0c8-1714-2d105c4f9db9@ideasonboard.com>","Date":"Mon, 13 Sep 2021 12:39:11 +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 8/9] android: camera_stream: Run\n\tpost-processor in a separate thread","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":19694,"web_url":"https://patchwork.libcamera.org/comment/19694/","msgid":"","date":"2021-09-15T01:35:04","subject":"Re: [libcamera-devel] [PATCH v2 8/9] android: camera_stream: Run\n\tpost-processor in a separate thread","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:39:11PM +0530, Umang Jain wrote:\n> On 9/13/21 6:51 AM, Laurent Pinchart wrote:\n> > On Fri, Sep 10, 2021 at 12:36:37PM +0530, Umang Jain wrote:\n> >> In CameraStream, introduce a new private PostProcessorWorker\n> >> class derived from Object. A instance of PostProcessorWorker\n> >> is moved to a separate thread instance which will be responsible\n> >> to run the post-processor.\n> >>\n> >> Running PostProcessor asynchronously should entail that all the\n> >> data context needed by the PostProcessor should remain valid for\n> >> the entire duration of its run. Most of the context preserving\n> >> part has been addressed in the previous commits, we just need to\n> >> ensure the source framebuffer data that comes via Camera::Request,\n> >> should remain valid for the entire duration of post-processing\n> >> running asynchronously. In order to so, we maintain a separate\n> >> copy of the framebuffer data and add it to the Camera3RequestDescriptor\n> >> structure in which we preserve rest of the context.\n> >>\n> >> Signed-off-by: Umang Jain \n> >> ---\n> >> src/android/camera_device.cpp | 19 ++++++++++++++++++-\n> >> src/android/camera_device.h | 4 ++++\n> >> src/android/camera_stream.cpp | 16 ++++++++++++++--\n> >> src/android/camera_stream.h | 28 ++++++++++++++++++++++++++++\n> >> 4 files changed, 64 insertions(+), 3 deletions(-)\n> >>\n> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> >> index 988d4232..73eb5758 100644\n> >> --- a/src/android/camera_device.cpp\n> >> +++ b/src/android/camera_device.cpp\n> >> @@ -1174,13 +1174,29 @@ void CameraDevice::requestComplete(Request *request)\n> >> \t\t\treturn;\n> >> \t\t}\n> >> \n> >> +\t\tFrameBuffer *source = src;\n> >> +\t\tif (cameraStream->type() != CameraStream::Type::Internal) {\n> >> +\t\t\t/*\n> >> +\t\t\t * The source buffer is owned by Request object which\n> >> +\t\t\t * can be reused by libcamera. Since post-processor will\n> >> +\t\t\t * run asynchrnously, we need to copy the request's\n> >> +\t\t\t * frame buffer and use that as the source buffer for\n> >> +\t\t\t * post processing.\n> >\n> > I don't think this is right. The request can be reused indeed, but with\n> > new FrameBuffer instances every time. The frame buffers are owned by\n> > Camera3RequestDescriptor, they're stored in a vector there.\n> \n> Yes, I realized by discussion with Jacopo. I think we don't need a copy \n> here, working on it as we speak.\n> \n> >> +\t\t\t */\n> >> +\t\t\tfor (const auto &plane : src->planes())\n> >> +\t\t\t\tdescriptor.srcPlanes_.push_back(plane);\n> >> +\t\t\tdescriptor.srcFramebuffer_ =\n> >> +\t\t\t\tstd::make_unique(descriptor.srcPlanes_);\n> >> +\t\t\tsource = descriptor.srcFramebuffer_.get();\n> >> +\t\t}\n> >> +\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\tCamera3RequestDescriptor *currentDescriptor = queuedDescriptor_.back().get();\n> >> -\t\tint ret = cameraStream->process(src,\n> >> +\t\tint ret = cameraStream->process(source,\n> >> \t\t\t\t\t\tcurrentDescriptor->destBuffer_.get(),\n> >> \t\t\t\t\t\tcurrentDescriptor->settings_,\n> >> \t\t\t\t\t\tcurrentDescriptor->resultMetadata_.get(),\n> >> @@ -1255,6 +1271,7 @@ void CameraDevice::streamProcessingComplete(CameraStream *cameraStream,\n> >> \n> >> void CameraDevice::sendQueuedCaptureResults()\n> >> {\n> >> +\tMutexLocker lock(queuedDescriptorsMutex_);\n> >> \twhile (!queuedDescriptor_.empty()) {\n> >> \t\tstd::unique_ptr &d = queuedDescriptor_.front();\n> >> \t\tif (d->status_ == Camera3RequestDescriptor::Pending)\n> >> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> >> index b62d373c..ecdda06e 100644\n> >> --- a/src/android/camera_device.h\n> >> +++ b/src/android/camera_device.h\n> >> @@ -62,6 +62,9 @@ struct Camera3RequestDescriptor {\n> >> \tcamera3_capture_result_t captureResult_;\n> >> \tlibcamera::FrameBuffer *internalBuffer_;\n> >> \tcompletionStatus status_;\n> >> +\n> >> +\tstd::unique_ptr srcFramebuffer_;\n> >> +\tstd::vector srcPlanes_;\n> >> };\n> >> \n> >> class CameraDevice : protected libcamera::Loggable\n> >> @@ -147,6 +150,7 @@ private:\n> >> \tlibcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */\n> >> \tstd::map descriptors_;\n> >> \n> >> +\tlibcamera::Mutex queuedDescriptorsMutex_; /* Protects queuedDescriptor_. */\n> >\n> > This belongs to the previous patch.\n> \n> No, I think as this patch is the one which introduces the thread, it \n> should introduce the necessary locking too. Correct me if I am wrong.\n\nI prefer reviewing the locking scheme with the data classes as it's\neasier to ensure the locking is right. If the lock was moved to the\nprevious patch I could check the introduced new usages of the data along\nwith the lock, while with locking in this patch I need to look at both\npatches together.\n\nThis being said, if moving the lock to the previous patch causes issues,\nit's not a very strong rule.\n\n> >> \tstd::deque> queuedDescriptor_;\n> >> \n> >> \tstd::string maker_;\n> >> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> >> index 5fd04bbf..845e2462 100644\n> >> --- a/src/android/camera_stream.cpp\n> >> +++ b/src/android/camera_stream.cpp\n> >> @@ -55,6 +55,15 @@ CameraStream::CameraStream(CameraDevice *const cameraDevice,\n> >> \t\t * is what we instantiate here.\n> >> \t\t */\n> >> \t\tpostProcessor_ = std::make_unique(cameraDevice_);\n> >> +\t\tppWorker_ = std::make_unique(postProcessor_.get());\n> >> +\n> >> +\t\tthread_ = std::make_unique();\n> >> +\t\tppWorker_->moveToThread(thread_.get());\n> >> +\t\t/*\n> >> +\t\t * \\todo: Class is MoveConstructible, so where to stop thread\n> >> +\t\t * if we don't user-defined destructor? See RFC patch at the end.\n> >> +\t\t */\n> >> +\t\tthread_->start();\n> >> \t}\n> >> \n> >> \tif (type == Type::Internal) {\n> >> @@ -110,8 +119,11 @@ int CameraStream::process(const FrameBuffer *source,\n> >> \tif (!postProcessor_)\n> >> \t\treturn 0;\n> >> \n> >> -\treturn postProcessor_->process(source, destBuffer, requestMetadata,\n> >> -\t\t\t\t resultMetadata, context);\n> >> +\tppWorker_->invokeMethod(&PostProcessorWorker::process,\n> >> +\t\t\t\tConnectionTypeQueued, source, destBuffer,\n> >> +\t\t\t\trequestMetadata, resultMetadata, context);\n> >> +\n> >> +\treturn 0;\n> >> }\n> >> \n> >> void CameraStream::postProcessingComplete(PostProcessor::Status status,\n> >> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n> >> index 8097ddbc..dbb7eee3 100644\n> >> --- a/src/android/camera_stream.h\n> >> +++ b/src/android/camera_stream.h\n> >> @@ -13,7 +13,9 @@\n> >> \n> >> #include \n> >> \n> >> +#include \n> >> #include \n> >> +#include \n> >> \n> >> #include \n> >> #include \n> >> @@ -23,6 +25,7 @@\n> >> \n> >> #include \"post_processor.h\"\n> >> \n> >> +class CameraBuffer;\n> >> class CameraDevice;\n> >> class CameraMetadata;\n> >> \n> >> @@ -137,6 +140,29 @@ public:\n> >> \t};\n> >> \n> >> private:\n> >> +\tclass PostProcessorWorker : public libcamera::Object\n> >> +\t{\n> >> +\tpublic:\n> >> +\t\tPostProcessorWorker(PostProcessor *postProcessor)\n> >> +\t\t{\n> >> +\t\t\tpostProcessor_ = postProcessor;\n> >> +\t\t}\n> >> +\n> >> +\t\tvoid process(const libcamera::FrameBuffer *source,\n> >> +\t\t\t CameraBuffer *destination,\n> >> +\t\t\t const CameraMetadata &requestMetadata,\n> >> +\t\t\t CameraMetadata *resultMetadata,\n> >> +\t\t\t const Camera3RequestDescriptor *context)\n> >> +\t\t{\n> >> +\t\t\tpostProcessor_->process(source, destination,\n> >> +\t\t\t\t\t\trequestMetadata, resultMetadata,\n> >> +\t\t\t\t\t\tcontext);\n> >> +\t\t}\n> >> +\n> >> +\tprivate:\n> >> +\t\tPostProcessor *postProcessor_;\n> >> +\t};\n> >\n> > If it wasn't for the fact that I mentioned in the review of v1 that it\n> > would be best to keep the PostProcessor class not thread-aware, I'd be\n> > tempted to say we could make PostProcessor inherit from Object and drop\n> > this class completely :-) Would you be tempted to do so too ? Compared\n> \n> I was tempted that from the start which was RFC v1. I should start to \n> trust my gut more....\n> \n> > to v1 (the RFC) it would resurect patch 1/5, but we wouldn't need the\n> > PostProcessorWorker class at all.\n> \n> hmmm....\n> \n> > On the other hand, given that I've mentioned in the review of a 7/9 that\n> > mapping the destination buffer could be done in the post-processor\n> > thread, maybe PostProcessorWorker::process() is the right place to do\n> > so. And now that I've written that, I now realize we've split the\n> > construction of CameraBuffer from the actual mapping, which is performed\n> > on the first call to CameraBuffer::plane(), so the mapping is already\n> > done in the post-processor thread. It's thus likely fine to have the\n> > construction of CameraBuffer in CameraStream or CameraDevice.\n> \n> Aha, very interesting. So I think this brings one more \\todo down in the \n> code base\n> \n> >> +\n> >> \tvoid postProcessingComplete(PostProcessor::Status status,\n> >> \t\t\t\t const Camera3RequestDescriptor *context);\n> >> \n> >> @@ -154,6 +180,8 @@ private:\n> >> \t */\n> >> \tstd::unique_ptr mutex_;\n> >> \tstd::unique_ptr postProcessor_;\n> >> +\tstd::unique_ptr ppWorker_;\n> >> +\tstd::unique_ptr thread_;\n> >> };\n> >> \n> >> #endif /* __ANDROID_CAMERA_STREAM__ */","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 600A5BDB1D\n\tfor ;\n\tWed, 15 Sep 2021 01:35:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D3D1A69189;\n\tWed, 15 Sep 2021 03:35:30 +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 8DE4F60249\n\tfor ;\n\tWed, 15 Sep 2021 03:35:29 +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 F285B24F;\n\tWed, 15 Sep 2021 03:35:28 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Ysr7tYd8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1631669729;\n\tbh=/PZVoei6eznactSqolM8ZsDgYkZ1HfOxiwmJ2NHW0gc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Ysr7tYd87byYOVXLpYS2rakkodJGekqDPi6+VvY1HLCkFs27LB5ZjkjVgup4Y+56n\n\tETxNcZ/ibArpYTd01ZMrSFKACOHVVF80hnvHCZLnEyHQfL8VKBTgqnmXO5ES3qvpZO\n\t6jynGD+o6gk7vQZdaVyPO1EMxRvvKy9Y6vGlmY8Y=","Date":"Wed, 15 Sep 2021 04:35:04 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20210910070638.467294-1-umang.jain@ideasonboard.com>\n\t<20210910070638.467294-9-umang.jain@ideasonboard.com>\n\t\n\t<832740db-14aa-c0c8-1714-2d105c4f9db9@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<832740db-14aa-c0c8-1714-2d105c4f9db9@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 8/9] android: camera_stream: Run\n\tpost-processor in a separate thread","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\" "}}]