[{"id":19761,"web_url":"https://patchwork.libcamera.org/comment/19761/","msgid":"","date":"2021-09-21T23:58:56","subject":"Re: [libcamera-devel] [PATCH v3 10/10] android: camera_stream: Run\n\tpost processor in a 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 Mon, Sep 20, 2021 at 11:07:52PM +0530, Umang Jain wrote:\n> This commit makes the post processor run in a separate thread.\n> To enable the move to a separate thread, he post processor\n\ns/he post/the post/\n\n> needs to be inherited from libcamera::Object and use the\n\ns/to be inherited/to inherit/\n\n> Object::moveToThread() API to execute the move.\n> \n> A user defined destructor is introduced to stop the thread for cleanup.\n\ns/user defined/user-defined/\n\n> Since CameraStream is move-constructible and compiler implicitly\n> generates its constructor, defining a destructor will prevent the\n> compiler from adding an implicitly-declared move constructor. Therefore,\n> we need to explicitly add a move constructor as well.\n> \n> Also, the destination buffer for post-processing needs to be alive and\n> stored as a part of overall context, hence a CameraBuffer unique-pointer\n> member is introduced in the Camera3RequestDescriptor struct.\n> \n> ---\n> /* \\todo Use ConnectionTypeQueued instead of ConnectionTypeBlocking */\n\nIndeed, that defeates the point of a thread a little bit :-) What's\nblocking the switch to the queued connection type ?\n\n> ---\n> \n> Signed-off-by: Umang Jain \n> ---\n> src/android/camera_device.h | 2 ++\n> src/android/camera_stream.cpp | 29 +++++++++++++++++++++--------\n> src/android/camera_stream.h | 5 +++++\n> src/android/post_processor.h | 3 ++-\n> 4 files changed, 30 insertions(+), 9 deletions(-)\n> \n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index 0bd570a1..e3f3fe7c 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -27,6 +27,7 @@\n> #include \n> #include \n> \n> +#include \"camera_buffer.h\"\n> #include \"camera_capabilities.h\"\n> #include \"camera_metadata.h\"\n> #include \"camera_stream.h\"\n> @@ -55,6 +56,7 @@ struct Camera3RequestDescriptor {\n> \tCameraMetadata settings_;\n> \tstd::unique_ptr request_;\n> \tstd::unique_ptr resultMetadata_;\n> +\tstd::unique_ptr destBuffer_;\n> \n> \tcamera3_capture_result_t captureResult_ = {};\n> \tlibcamera::FrameBuffer *internalBuffer_;\n> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> index c18c7041..6578ce09 100644\n> --- a/src/android/camera_stream.cpp\n> +++ b/src/android/camera_stream.cpp\n> @@ -55,6 +55,9 @@ 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\tthread_ = std::make_unique();\n\nI don't think you need to allocate the thread dynamically, you can have\na\n\n\tlibcamera::Thread thread_;\n\nin CameraStream.\n\n> +\t\tpostProcessor_->moveToThread(thread_.get());\n> +\t\tthread_->start();\n> \t}\n> \n> \tif (type == Type::Internal) {\n> @@ -63,6 +66,14 @@ CameraStream::CameraStream(CameraDevice *const cameraDevice,\n> \t}\n> }\n\nPlease add\n\nCameraStream::CameraStream(CameraStream &&other) = default;\n\nhere and drop the ' = default' in the header file, we don't need to\ninline the move-constructor.\n\n> +CameraStream::~CameraStream()\n> +{\n> +\tif (thread_) {\n> +\t\tthread_->exit();\n> +\t\tthread_->wait();\n> +\t}\n\nThis makes me realize that we're missing support for being able to flush\nthe requests queued to the post-processor. That will require some extra\nwork, and I'm tempted to say we should get this series merged with\nConnectionTypeBlocking first as an intermediate step. I'm fairly\nconfident this is going in the right direction and that implementing\nflush support won't require a complete redesign, but there's still this\nlittle voice that tells me the risk is not zero. Feel free to agree or\ndisagree :-)\n\nOne thing that will likely need to be reworked more extensively is this\npatch though. The most naive implementation would be to add start() and\nstop() functions to CameraStream. start() will start the thread, stop()\nwill stop it (calling exit and wait). You'll have to call create a\ncustom class inheriting from Thread (really sorry for going back and\nforth on this topic :-S) to reimplement run() and call\n\n\tdispatchMessages(Message::Type::InvokeMessage);\n\nafter calling exec(). Otherwise you'll have queued process() calls that\nwill be left dangling. Performance won't be great, as *all* queued\nrequests will be processed, even the ones whose processing hasn't\nstarted yet when stop() is called.\n\nTo make it better, we should complete the ongoing processing, and then\ncomplete the other requests that have been queued but haven't been\nprocessed yet. This will require keeping a queue of pending requests in\nthe camera stream. Requests will be added to the queue before they get\npassed to the post-processor, and removed when the post-processor\nsignals completion. You'll be able to remove the dispatchMessages() call\n(and won't have to reimplement Thread::run(), so no need to subclass it\nafter all \\o/), and after the thread stops (after wait() returns),\nyou'll have to go through the queue and signal completion of all\nrequests still stored there. This shouldn't be too complex, I'd aim for\nthis option directly as you won't need to subclass Thread.\n\nIdeally, to avoid delays when starting the camera and when flushing it,\nwe should keep the thread running at all times (when no work will be\nqueued to it, it will not consume any CPU time). There's no way to\ninstruct a running thread to finish the ongoing processing and drop the\npending invokeMethod() calls, so you'll need to subclass Thread (did I\nmention going back and forth ? :-)) and reimplement run(). This time you\nwon't be able to rely on exec() and invokeMethod(), so you'll have to\nimplement a queue of request in the thread subclass, a custom loop in\nrun() that will process the requests from the queue, and add a condition\nvariable to signal to the run() function that a new request has been\nadded to the queue, or that a flush() has been requested. That way\nyou'll be able to get rid of start() and stop(), and only add flush().\n\n> +}\n> +\n> const StreamConfiguration &CameraStream::configuration() const\n> {\n> \treturn config_->at(index_);\n> @@ -111,21 +122,23 @@ void CameraStream::process(const FrameBuffer *source,\n> \t\treturn;\n> \t}\n> \n> -\t/*\n> -\t * \\todo Buffer mapping and processing should be moved to a\n> -\t * separate thread.\n> -\t */\n> \tconst StreamConfiguration &output = configuration();\n> -\tCameraBuffer dest(camera3Dest, formats::MJPEG, output.size,\n> -\t\t\t PROT_READ | PROT_WRITE);\n> -\tif (!dest.isValid()) {\n> +\tstd::unique_ptr dest =\n> +\t\tstd::make_unique(camera3Dest, formats::MJPEG,\n> +\t\t\t\t\t output.size, PROT_READ | PROT_WRITE);\n> +\n> +\tif (!dest->isValid()) {\n> \t\tLOG(HAL, Error) << \"Failed to map android blob buffer\";\n> \t\trequest->status_ = Camera3RequestDescriptor::ProcessStatus::Error;\n> \t\thandleProcessComplete(request);\n> \t\treturn;\n> \t}\n\nBlank line.\n\n> +\trequest->destBuffer_ = std::move(dest);\n> \n> -\tpostProcessor_->process(source, &dest, request);\n> +\t/* \\todo Use ConnectionTypeQueued instead of ConnectionTypeBlocking */\n> +\tpostProcessor_->invokeMethod(&PostProcessor::process,\n> +\t\t\t\t ConnectionTypeBlocking, source,\n> +\t\t\t\t request->destBuffer_.get(), request);\n> }\n> \n> void CameraStream::handleProcessComplete(Camera3RequestDescriptor *request)\n> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n> index d4ec5c25..42db72d8 100644\n> --- a/src/android/camera_stream.h\n> +++ b/src/android/camera_stream.h\n> @@ -13,6 +13,8 @@\n> \n> #include \n> \n> +#include \n> +\n> #include \n> #include \n> #include \n> @@ -113,6 +115,8 @@ public:\n> \tCameraStream(CameraDevice *const cameraDevice,\n> \t\t libcamera::CameraConfiguration *config, Type type,\n> \t\t camera3_stream_t *camera3Stream, unsigned int index);\n> +\tCameraStream(CameraStream &&other) = default;\n> +\t~CameraStream();\n> \n> \tType type() const { return type_; }\n> \tconst camera3_stream_t &camera3Stream() const { return *camera3Stream_; }\n> @@ -143,6 +147,7 @@ private:\n> \t */\n> \tstd::unique_ptr mutex_;\n> \tstd::unique_ptr postProcessor_;\n> +\tstd::unique_ptr thread_;\n> };\n> \n> #endif /* __ANDROID_CAMERA_STREAM__ */\n> diff --git a/src/android/post_processor.h b/src/android/post_processor.h\n> index 48ddd8ac..fdfd52d3 100644\n> --- a/src/android/post_processor.h\n> +++ b/src/android/post_processor.h\n> @@ -7,6 +7,7 @@\n> #ifndef __ANDROID_POST_PROCESSOR_H__\n> #define __ANDROID_POST_PROCESSOR_H__\n> \n> +#include \n> #include \n> \n> #include \n> @@ -18,7 +19,7 @@ class CameraMetadata;\n> \n> struct Camera3RequestDescriptor;\n> \n> -class PostProcessor\n> +class PostProcessor : public libcamera::Object\n> {\n> public:\n> \tvirtual ~PostProcessor() = default;","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 8BFD2BDC71\n\tfor ;\n\tTue, 21 Sep 2021 23:59:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D4F0D6918B;\n\tWed, 22 Sep 2021 01:59:28 +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 73E5169186\n\tfor ;\n\tWed, 22 Sep 2021 01:59:27 +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 DA493291;\n\tWed, 22 Sep 2021 01:59:26 +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=\"eCAGdgPo\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632268767;\n\tbh=/pPi8TLST2sCxyrWgTOe4JcvPAQ2sH60wbxajgSnJH8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=eCAGdgPo8GsdgBicbDpgNQ3lfvUjHQu7RSRqxdbhS8sP7mTXgegVPAjN2jNH+itiw\n\t3KucapCPlRoYEBiifPDyKZW0HoRvzrsemyMV6uzycble+EfcK+N1/onaCKpjJ1hu5J\n\tO7+86EPh1bTOCuqJgGAXYEYWGfaTJJAzv1H1/DjA=","Date":"Wed, 22 Sep 2021 02:58:56 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20210920173752.1346190-1-umang.jain@ideasonboard.com>\n\t<20210920173752.1346190-11-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210920173752.1346190-11-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 10/10] android: camera_stream: Run\n\tpost processor in a 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":19788,"web_url":"https://patchwork.libcamera.org/comment/19788/","msgid":"<7d652cf9-5199-8b93-7f7a-e56bd3730158@ideasonboard.com>","date":"2021-09-22T11:44:55","subject":"Re: [libcamera-devel] [PATCH v3 10/10] android: camera_stream: Run\n\tpost processor in a thread","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hello Laurent,\n\nI'll defer the reply to this review. Reason is, I believe some \nconversations happened on the IRC and evolved a bit. I'll try to \nimplement and see what works and how it shapes up overall and write my \nthoughts here (implementation-point-of-view) that will be done as Part \nIII. I hope that's ok!\n\nOn 9/22/21 5:28 AM, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> Thank you for the patch.\n>\n> On Mon, Sep 20, 2021 at 11:07:52PM +0530, Umang Jain wrote:\n>> This commit makes the post processor run in a separate thread.\n>> To enable the move to a separate thread, he post processor\n> s/he post/the post/\n>\n>> needs to be inherited from libcamera::Object and use the\n> s/to be inherited/to inherit/\n>\n>> Object::moveToThread() API to execute the move.\n>>\n>> A user defined destructor is introduced to stop the thread for cleanup.\n> s/user defined/user-defined/\n>\n>> Since CameraStream is move-constructible and compiler implicitly\n>> generates its constructor, defining a destructor will prevent the\n>> compiler from adding an implicitly-declared move constructor. Therefore,\n>> we need to explicitly add a move constructor as well.\n>>\n>> Also, the destination buffer for post-processing needs to be alive and\n>> stored as a part of overall context, hence a CameraBuffer unique-pointer\n>> member is introduced in the Camera3RequestDescriptor struct.\n>>\n>> ---\n>> /* \\todo Use ConnectionTypeQueued instead of ConnectionTypeBlocking */\n> Indeed, that defeates the point of a thread a little bit :-) What's\n> blocking the switch to the queued connection type ?\n>\n>> ---\n>>\n>> Signed-off-by: Umang Jain \n>> ---\n>> src/android/camera_device.h | 2 ++\n>> src/android/camera_stream.cpp | 29 +++++++++++++++++++++--------\n>> src/android/camera_stream.h | 5 +++++\n>> src/android/post_processor.h | 3 ++-\n>> 4 files changed, 30 insertions(+), 9 deletions(-)\n>>\n>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n>> index 0bd570a1..e3f3fe7c 100644\n>> --- a/src/android/camera_device.h\n>> +++ b/src/android/camera_device.h\n>> @@ -27,6 +27,7 @@\n>> #include \n>> #include \n>> \n>> +#include \"camera_buffer.h\"\n>> #include \"camera_capabilities.h\"\n>> #include \"camera_metadata.h\"\n>> #include \"camera_stream.h\"\n>> @@ -55,6 +56,7 @@ struct Camera3RequestDescriptor {\n>> \tCameraMetadata settings_;\n>> \tstd::unique_ptr request_;\n>> \tstd::unique_ptr resultMetadata_;\n>> +\tstd::unique_ptr destBuffer_;\n>> \n>> \tcamera3_capture_result_t captureResult_ = {};\n>> \tlibcamera::FrameBuffer *internalBuffer_;\n>> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n>> index c18c7041..6578ce09 100644\n>> --- a/src/android/camera_stream.cpp\n>> +++ b/src/android/camera_stream.cpp\n>> @@ -55,6 +55,9 @@ 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\tthread_ = std::make_unique();\n> I don't think you need to allocate the thread dynamically, you can have\n> a\n>\n> \tlibcamera::Thread thread_;\n>\n> in CameraStream.\n>\n>> +\t\tpostProcessor_->moveToThread(thread_.get());\n>> +\t\tthread_->start();\n>> \t}\n>> \n>> \tif (type == Type::Internal) {\n>> @@ -63,6 +66,14 @@ CameraStream::CameraStream(CameraDevice *const cameraDevice,\n>> \t}\n>> }\n> Please add\n>\n> CameraStream::CameraStream(CameraStream &&other) = default;\n>\n> here and drop the ' = default' in the header file, we don't need to\n> inline the move-constructor.\n>\n>> +CameraStream::~CameraStream()\n>> +{\n>> +\tif (thread_) {\n>> +\t\tthread_->exit();\n>> +\t\tthread_->wait();\n>> +\t}\n> This makes me realize that we're missing support for being able to flush\n> the requests queued to the post-processor. That will require some extra\n> work, and I'm tempted to say we should get this series merged with\n> ConnectionTypeBlocking first as an intermediate step. I'm fairly\n> confident this is going in the right direction and that implementing\n> flush support won't require a complete redesign, but there's still this\n> little voice that tells me the risk is not zero. Feel free to agree or\n> disagree :-)\n>\n> One thing that will likely need to be reworked more extensively is this\n> patch though. The most naive implementation would be to add start() and\n> stop() functions to CameraStream. start() will start the thread, stop()\n> will stop it (calling exit and wait). You'll have to call create a\n> custom class inheriting from Thread (really sorry for going back and\n> forth on this topic :-S) to reimplement run() and call\n>\n> \tdispatchMessages(Message::Type::InvokeMessage);\n>\n> after calling exec(). Otherwise you'll have queued process() calls that\n> will be left dangling. Performance won't be great, as *all* queued\n> requests will be processed, even the ones whose processing hasn't\n> started yet when stop() is called.\n>\n> To make it better, we should complete the ongoing processing, and then\n> complete the other requests that have been queued but haven't been\n> processed yet. This will require keeping a queue of pending requests in\n> the camera stream. Requests will be added to the queue before they get\n> passed to the post-processor, and removed when the post-processor\n> signals completion. You'll be able to remove the dispatchMessages() call\n> (and won't have to reimplement Thread::run(), so no need to subclass it\n> after all \\o/), and after the thread stops (after wait() returns),\n> you'll have to go through the queue and signal completion of all\n> requests still stored there. This shouldn't be too complex, I'd aim for\n> this option directly as you won't need to subclass Thread.\n>\n> Ideally, to avoid delays when starting the camera and when flushing it,\n> we should keep the thread running at all times (when no work will be\n> queued to it, it will not consume any CPU time). There's no way to\n> instruct a running thread to finish the ongoing processing and drop the\n> pending invokeMethod() calls, so you'll need to subclass Thread (did I\n> mention going back and forth ? :-)) and reimplement run(). This time you\n> won't be able to rely on exec() and invokeMethod(), so you'll have to\n> implement a queue of request in the thread subclass, a custom loop in\n> run() that will process the requests from the queue, and add a condition\n> variable to signal to the run() function that a new request has been\n> added to the queue, or that a flush() has been requested. That way\n> you'll be able to get rid of start() and stop(), and only add flush().\n>\n>> +}\n>> +\n>> const StreamConfiguration &CameraStream::configuration() const\n>> {\n>> \treturn config_->at(index_);\n>> @@ -111,21 +122,23 @@ void CameraStream::process(const FrameBuffer *source,\n>> \t\treturn;\n>> \t}\n>> \n>> -\t/*\n>> -\t * \\todo Buffer mapping and processing should be moved to a\n>> -\t * separate thread.\n>> -\t */\n>> \tconst StreamConfiguration &output = configuration();\n>> -\tCameraBuffer dest(camera3Dest, formats::MJPEG, output.size,\n>> -\t\t\t PROT_READ | PROT_WRITE);\n>> -\tif (!dest.isValid()) {\n>> +\tstd::unique_ptr dest =\n>> +\t\tstd::make_unique(camera3Dest, formats::MJPEG,\n>> +\t\t\t\t\t output.size, PROT_READ | PROT_WRITE);\n>> +\n>> +\tif (!dest->isValid()) {\n>> \t\tLOG(HAL, Error) << \"Failed to map android blob buffer\";\n>> \t\trequest->status_ = Camera3RequestDescriptor::ProcessStatus::Error;\n>> \t\thandleProcessComplete(request);\n>> \t\treturn;\n>> \t}\n> Blank line.\n>\n>> +\trequest->destBuffer_ = std::move(dest);\n>> \n>> -\tpostProcessor_->process(source, &dest, request);\n>> +\t/* \\todo Use ConnectionTypeQueued instead of ConnectionTypeBlocking */\n>> +\tpostProcessor_->invokeMethod(&PostProcessor::process,\n>> +\t\t\t\t ConnectionTypeBlocking, source,\n>> +\t\t\t\t request->destBuffer_.get(), request);\n>> }\n>> \n>> void CameraStream::handleProcessComplete(Camera3RequestDescriptor *request)\n>> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n>> index d4ec5c25..42db72d8 100644\n>> --- a/src/android/camera_stream.h\n>> +++ b/src/android/camera_stream.h\n>> @@ -13,6 +13,8 @@\n>> \n>> #include \n>> \n>> +#include \n>> +\n>> #include \n>> #include \n>> #include \n>> @@ -113,6 +115,8 @@ public:\n>> \tCameraStream(CameraDevice *const cameraDevice,\n>> \t\t libcamera::CameraConfiguration *config, Type type,\n>> \t\t camera3_stream_t *camera3Stream, unsigned int index);\n>> +\tCameraStream(CameraStream &&other) = default;\n>> +\t~CameraStream();\n>> \n>> \tType type() const { return type_; }\n>> \tconst camera3_stream_t &camera3Stream() const { return *camera3Stream_; }\n>> @@ -143,6 +147,7 @@ private:\n>> \t */\n>> \tstd::unique_ptr mutex_;\n>> \tstd::unique_ptr postProcessor_;\n>> +\tstd::unique_ptr thread_;\n>> };\n>> \n>> #endif /* __ANDROID_CAMERA_STREAM__ */\n>> diff --git a/src/android/post_processor.h b/src/android/post_processor.h\n>> index 48ddd8ac..fdfd52d3 100644\n>> --- a/src/android/post_processor.h\n>> +++ b/src/android/post_processor.h\n>> @@ -7,6 +7,7 @@\n>> #ifndef __ANDROID_POST_PROCESSOR_H__\n>> #define __ANDROID_POST_PROCESSOR_H__\n>> \n>> +#include \n>> #include \n>> \n>> #include \n>> @@ -18,7 +19,7 @@ class CameraMetadata;\n>> \n>> struct Camera3RequestDescriptor;\n>> \n>> -class PostProcessor\n>> +class PostProcessor : public libcamera::Object\n>> {\n>> public:\n>> \tvirtual ~PostProcessor() = default;","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 4EFE2BDC71\n\tfor ;\n\tWed, 22 Sep 2021 11:45:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 646086918E;\n\tWed, 22 Sep 2021 13:45:03 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2E6026917F\n\tfor ;\n\tWed, 22 Sep 2021 13:45:02 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.124])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 16F3DF1;\n\tWed, 22 Sep 2021 13:45:00 +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=\"tG4k8O6O\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632311101;\n\tbh=xhtUmnKnsgCq0k/jd+5YBWrFnnEmnvoVMnUArKRfpP4=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=tG4k8O6Oup3CfGLt5v+hCaACI6WyUwUdJM/NA/SEKSs6nIuz7WU5jPiwHF6y4K0oC\n\tpG9KibXNBRZcwDAEyWdglCeH6nfnca8uzUIw0JRd0GqEKu0iEGP/giVpn07AIS85rU\n\tf5pm6q35cMirmmb6n9JwINQv2E3hEr74eqefn0PE=","To":"Laurent Pinchart ","References":"<20210920173752.1346190-1-umang.jain@ideasonboard.com>\n\t<20210920173752.1346190-11-umang.jain@ideasonboard.com>\n\t","From":"Umang Jain ","Message-ID":"<7d652cf9-5199-8b93-7f7a-e56bd3730158@ideasonboard.com>","Date":"Wed, 22 Sep 2021 17:14:55 +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 v3 10/10] android: camera_stream: Run\n\tpost processor in a 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":20104,"web_url":"https://patchwork.libcamera.org/comment/20104/","msgid":"<34968300-49ea-39ff-0674-e6ea96335d36@ideasonboard.com>","date":"2021-10-11T09:45:52","subject":"Re: [libcamera-devel] [PATCH v3 10/10] android: camera_stream: Run\n\tpost processor in a 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/22/21 5:28 AM, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> Thank you for the patch.\n>\n> On Mon, Sep 20, 2021 at 11:07:52PM +0530, Umang Jain wrote:\n>> This commit makes the post processor run in a separate thread.\n>> To enable the move to a separate thread, he post processor\n> s/he post/the post/\n>\n>> needs to be inherited from libcamera::Object and use the\n> s/to be inherited/to inherit/\n>\n>> Object::moveToThread() API to execute the move.\n>>\n>> A user defined destructor is introduced to stop the thread for cleanup.\n> s/user defined/user-defined/\n>\n>> Since CameraStream is move-constructible and compiler implicitly\n>> generates its constructor, defining a destructor will prevent the\n>> compiler from adding an implicitly-declared move constructor. Therefore,\n>> we need to explicitly add a move constructor as well.\n>>\n>> Also, the destination buffer for post-processing needs to be alive and\n>> stored as a part of overall context, hence a CameraBuffer unique-pointer\n>> member is introduced in the Camera3RequestDescriptor struct.\n>>\n>> ---\n>> /* \\todo Use ConnectionTypeQueued instead of ConnectionTypeBlocking */\n> Indeed, that defeates the point of a thread a little bit :-) What's\n> blocking the switch to the queued connection type ?\n>\n>> ---\n>>\n>> Signed-off-by: Umang Jain \n>> ---\n>> src/android/camera_device.h | 2 ++\n>> src/android/camera_stream.cpp | 29 +++++++++++++++++++++--------\n>> src/android/camera_stream.h | 5 +++++\n>> src/android/post_processor.h | 3 ++-\n>> 4 files changed, 30 insertions(+), 9 deletions(-)\n>>\n>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n>> index 0bd570a1..e3f3fe7c 100644\n>> --- a/src/android/camera_device.h\n>> +++ b/src/android/camera_device.h\n>> @@ -27,6 +27,7 @@\n>> #include \n>> #include \n>> \n>> +#include \"camera_buffer.h\"\n>> #include \"camera_capabilities.h\"\n>> #include \"camera_metadata.h\"\n>> #include \"camera_stream.h\"\n>> @@ -55,6 +56,7 @@ struct Camera3RequestDescriptor {\n>> \tCameraMetadata settings_;\n>> \tstd::unique_ptr request_;\n>> \tstd::unique_ptr resultMetadata_;\n>> +\tstd::unique_ptr destBuffer_;\n>> \n>> \tcamera3_capture_result_t captureResult_ = {};\n>> \tlibcamera::FrameBuffer *internalBuffer_;\n>> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n>> index c18c7041..6578ce09 100644\n>> --- a/src/android/camera_stream.cpp\n>> +++ b/src/android/camera_stream.cpp\n>> @@ -55,6 +55,9 @@ 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\tthread_ = std::make_unique();\n> I don't think you need to allocate the thread dynamically, you can have\n> a\n>\n> \tlibcamera::Thread thread_;\n>\n> in CameraStream.\n>\n>> +\t\tpostProcessor_->moveToThread(thread_.get());\n>> +\t\tthread_->start();\n>> \t}\n>> \n>> \tif (type == Type::Internal) {\n>> @@ -63,6 +66,14 @@ CameraStream::CameraStream(CameraDevice *const cameraDevice,\n>> \t}\n>> }\n> Please add\n>\n> CameraStream::CameraStream(CameraStream &&other) = default;\n>\n> here and drop the ' = default' in the header file, we don't need to\n> inline the move-constructor.\n>\n>> +CameraStream::~CameraStream()\n>> +{\n>> +\tif (thread_) {\n>> +\t\tthread_->exit();\n>> +\t\tthread_->wait();\n>> +\t}\n> This makes me realize that we're missing support for being able to flush\n> the requests queued to the post-processor. That will require some extra\n> work, and I'm tempted to say we should get this series merged with\n> ConnectionTypeBlocking first as an intermediate step. I'm fairly\n> confident this is going in the right direction and that implementing\n> flush support won't require a complete redesign, but there's still this\n> little voice that tells me the risk is not zero. Feel free to agree or\n> disagree :-)\n>\n> One thing that will likely need to be reworked more extensively is this\n> patch though. The most naive implementation would be to add start() and\n> stop() functions to CameraStream. start() will start the thread, stop()\n> will stop it (calling exit and wait). You'll have to call create a\n> custom class inheriting from Thread (really sorry for going back and\n> forth on this topic :-S) to reimplement run() and call\n>\n> \tdispatchMessages(Message::Type::InvokeMessage);\n>\n> after calling exec(). Otherwise you'll have queued process() calls that\n> will be left dangling. Performance won't be great, as *all* queued\n> requests will be processed, even the ones whose processing hasn't\n> started yet when stop() is called.\n>\n> To make it better, we should complete the ongoing processing, and then\n> complete the other requests that have been queued but haven't been\n> processed yet. This will require keeping a queue of pending requests in\n> the camera stream. Requests will be added to the queue before they get\n> passed to the post-processor, and removed when the post-processor\n> signals completion. You'll be able to remove the dispatchMessages() call\n> (and won't have to reimplement Thread::run(), so no need to subclass it\n> after all \\o/), and after the thread stops (after wait() returns),\n> you'll have to go through the queue and signal completion of all\n> requests still stored there. This shouldn't be too complex, I'd aim for\n> this option directly as you won't need to subclass Thread.\n>\n> Ideally, to avoid delays when starting the camera and when flushing it,\n> we should keep the thread running at all times (when no work will be\n> queued to it, it will not consume any CPU time). There's no way to\n> instruct a running thread to finish the ongoing processing and drop the\n> pending invokeMethod() calls, so you'll need to subclass Thread (did I\n> mention going back and forth ? :-)) and reimplement run(). This time you\n> won't be able to rely on exec() and invokeMethod(), so you'll have to\n> implement a queue of request in the thread subclass, a custom loop in\n> run() that will process the requests from the queue, and add a condition\n> variable to signal to the run() function that a new request has been\n> added to the queue, or that a flush() has been requested. That way\n> you'll be able to get rid of start() and stop(), and only add flush().\n\n\nI went ahead with using the condition variable along with maintaining a \nqueue in v4 as you stated out the design here (in the latter parts). All \nthe Object inheritence and ::invokeMethod() call have be dropped.\n\nA queue  in the worker class helps to purge the requests on-demand. As \nper my understand around flush(), I have designed a solution in Patch \n7/7 v4. To test it, I would have used CTS, but it is not playing well as \nof now (I am looking into it), but a assert:\n\n@@ -479,6 +479,7 @@ void CameraDevice::stop()\n\n         stopCamera();\n\n+       ASSERT (descriptors_.empty());\n         descriptors_ = {};\n\n\n\ntells me all the descriptors are getting cleared as expected on camera \nstop. Complying with CTS will be a double confirmation on this, hopefully.\n\n\n>\n>> +}\n>> +\n>> const StreamConfiguration &CameraStream::configuration() const\n>> {\n>> \treturn config_->at(index_);\n>> @@ -111,21 +122,23 @@ void CameraStream::process(const FrameBuffer *source,\n>> \t\treturn;\n>> \t}\n>> \n>> -\t/*\n>> -\t * \\todo Buffer mapping and processing should be moved to a\n>> -\t * separate thread.\n>> -\t */\n>> \tconst StreamConfiguration &output = configuration();\n>> -\tCameraBuffer dest(camera3Dest, formats::MJPEG, output.size,\n>> -\t\t\t PROT_READ | PROT_WRITE);\n>> -\tif (!dest.isValid()) {\n>> +\tstd::unique_ptr dest =\n>> +\t\tstd::make_unique(camera3Dest, formats::MJPEG,\n>> +\t\t\t\t\t output.size, PROT_READ | PROT_WRITE);\n>> +\n>> +\tif (!dest->isValid()) {\n>> \t\tLOG(HAL, Error) << \"Failed to map android blob buffer\";\n>> \t\trequest->status_ = Camera3RequestDescriptor::ProcessStatus::Error;\n>> \t\thandleProcessComplete(request);\n>> \t\treturn;\n>> \t}\n> Blank line.\n>\n>> +\trequest->destBuffer_ = std::move(dest);\n>> \n>> -\tpostProcessor_->process(source, &dest, request);\n>> +\t/* \\todo Use ConnectionTypeQueued instead of ConnectionTypeBlocking */\n>> +\tpostProcessor_->invokeMethod(&PostProcessor::process,\n>> +\t\t\t\t ConnectionTypeBlocking, source,\n>> +\t\t\t\t request->destBuffer_.get(), request);\n>> }\n>> \n>> void CameraStream::handleProcessComplete(Camera3RequestDescriptor *request)\n>> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n>> index d4ec5c25..42db72d8 100644\n>> --- a/src/android/camera_stream.h\n>> +++ b/src/android/camera_stream.h\n>> @@ -13,6 +13,8 @@\n>> \n>> #include \n>> \n>> +#include \n>> +\n>> #include \n>> #include \n>> #include \n>> @@ -113,6 +115,8 @@ public:\n>> \tCameraStream(CameraDevice *const cameraDevice,\n>> \t\t libcamera::CameraConfiguration *config, Type type,\n>> \t\t camera3_stream_t *camera3Stream, unsigned int index);\n>> +\tCameraStream(CameraStream &&other) = default;\n>> +\t~CameraStream();\n>> \n>> \tType type() const { return type_; }\n>> \tconst camera3_stream_t &camera3Stream() const { return *camera3Stream_; }\n>> @@ -143,6 +147,7 @@ private:\n>> \t */\n>> \tstd::unique_ptr mutex_;\n>> \tstd::unique_ptr postProcessor_;\n>> +\tstd::unique_ptr thread_;\n>> };\n>> \n>> #endif /* __ANDROID_CAMERA_STREAM__ */\n>> diff --git a/src/android/post_processor.h b/src/android/post_processor.h\n>> index 48ddd8ac..fdfd52d3 100644\n>> --- a/src/android/post_processor.h\n>> +++ b/src/android/post_processor.h\n>> @@ -7,6 +7,7 @@\n>> #ifndef __ANDROID_POST_PROCESSOR_H__\n>> #define __ANDROID_POST_PROCESSOR_H__\n>> \n>> +#include \n>> #include \n>> \n>> #include \n>> @@ -18,7 +19,7 @@ class CameraMetadata;\n>> \n>> struct Camera3RequestDescriptor;\n>> \n>> -class PostProcessor\n>> +class PostProcessor : public libcamera::Object\n>> {\n>> public:\n>> \tvirtual ~PostProcessor() = default;","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 E0238C323E\n\tfor ;\n\tMon, 11 Oct 2021 09:45:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4102068F4D;\n\tMon, 11 Oct 2021 11:45:58 +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 67E9260501\n\tfor ;\n\tMon, 11 Oct 2021 11:45:57 +0200 (CEST)","from [192.168.1.106] (unknown [103.251.226.107])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7E5022BD;\n\tMon, 11 Oct 2021 11:45:56 +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=\"WPdNXyLj\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1633945557;\n\tbh=Ow9V34fieAUAlDqL43V9wNzjcvTJuuDhBMuARhLZ+1s=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=WPdNXyLjnMeNXtjoyJweOKc+Bb1Dnn81zhuVwtPEjkh6y1fic3k35i1y6pgo2BRpw\n\tWraZM2+aYJplyyEVgXRY4witp2UHFBDHO4hvb9+cZYlnQ6QdE+EVxXduW+yVZA2oCi\n\t+0r9h3fON8QmD2ir8FyG33IUfH6UaLHnxEpGEzb0=","To":"Laurent Pinchart ","References":"<20210920173752.1346190-1-umang.jain@ideasonboard.com>\n\t<20210920173752.1346190-11-umang.jain@ideasonboard.com>\n\t","From":"Umang Jain ","Message-ID":"<34968300-49ea-39ff-0674-e6ea96335d36@ideasonboard.com>","Date":"Mon, 11 Oct 2021 15:15:52 +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":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v3 10/10] android: camera_stream: Run\n\tpost processor in a 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\" "}}]