[{"id":20122,"web_url":"https://patchwork.libcamera.org/comment/20122/","msgid":"","date":"2021-10-11T18:17:20","subject":"Re: [libcamera-devel] [PATCH v4 5/7] android: post_processor: Make\n\tpost processing async","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi me,\n\nOn 10/11/21 1:05 PM, Umang Jain wrote:\n> Introduce a dedicated worker class derived from libcamera::Thread.\n> The worker class maintains a queue for post-processing requests\n> and waits for a post-processing request to become available.\n> It will process them as per FIFO before de-queuing it from the\n> queue.\n>\n> To get access to the source and destination buffers in the worker\n> thread, we also need to save a pointer to them in the\n> Camera3RequestDescriptor.\n>\n> This patch also implements a flush() for the PostProcessorWorker\n> class which is responsible to purge post-processing requests\n> queued up while a camera is stopping/flushing.\n>\n> The libcamera request completion handler CameraDevice::requestComplete()\n> assumes that the request that has just completed is at the front of the\n> queue. Now that the post-processor runs asynchronously, this isn't true\n> anymore, a request being post-processed will stay in the queue and a new\n> libcamera request may complete. Remove that assumption, and use the\n> request cookie to obtain the Camera3RequestDescriptor.\n>\n> Signed-off-by: Umang Jain \n> Signed-off-by: Laurent Pinchart \n> ---\n> src/android/camera_device.cpp | 25 +-------\n> src/android/camera_device.h | 3 +\n> src/android/camera_stream.cpp | 108 +++++++++++++++++++++++++++++++---\n> src/android/camera_stream.h | 39 +++++++++++-\n> 4 files changed, 144 insertions(+), 31 deletions(-)\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index eba370ea..61b902ad 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -239,6 +239,7 @@ Camera3RequestDescriptor::Camera3RequestDescriptor(\n> \t/* Clone the controls associated with the camera3 request. */\n> \tsettings_ = CameraMetadata(camera3Request->settings);\n> \n> +\tdest_.reset();\n> \t/*\n> \t * Create the CaptureRequest, stored as a unique_ptr<> to tie its\n> \t * lifetime to the descriptor.\n> @@ -1094,28 +1095,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \n> void CameraDevice::requestComplete(Request *request)\n> {\n> -\tCamera3RequestDescriptor *descriptor;\n> -\t{\n> -\t\tMutexLocker descriptorsLock(descriptorsMutex_);\n> -\t\tASSERT(!descriptors_.empty());\n> -\t\tdescriptor = descriptors_.front().get();\n> -\t}\n> -\n> -\tif (descriptor->request_->cookie() != request->cookie()) {\n> -\t\t/*\n> -\t\t * \\todo Clarify if the Camera has to be closed on\n> -\t\t * ERROR_DEVICE and possibly demote the Fatal to simple\n> -\t\t * Error.\n> -\t\t */\n> -\t\tnotifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);\n> -\t\tLOG(HAL, Fatal)\n> -\t\t\t<< \"Out-of-order completion for request \"\n> -\t\t\t<< utils::hex(request->cookie());\n> -\n> -\t\tMutexLocker descriptorsLock(descriptorsMutex_);\n> -\t\tdescriptors_.pop();\n> -\t\treturn;\n> -\t}\n> +\tCamera3RequestDescriptor *descriptor =\n> +\t\treinterpret_cast(request->cookie());\n> \n> \t/*\n> \t * Prepare the capture result for the Android camera stack.\n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index eee97516..725a0618 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -59,6 +59,9 @@ struct Camera3RequestDescriptor {\n> \tstd::unique_ptr resultMetadata_;\n> \tlibcamera::FrameBuffer *internalBuffer_;\n> \n> +\tstd::unique_ptr dest_;\n> +\tconst libcamera::FrameBuffer *src_;\n> +\n> \tcamera3_capture_result_t captureResult_ = {};\n> \tStatus status_ = Status::Pending;\n> };\n> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> index cec07269..818ef948 100644\n> --- a/src/android/camera_stream.cpp\n> +++ b/src/android/camera_stream.cpp\n> @@ -94,10 +94,12 @@ int CameraStream::configure()\n> \t\tif (ret)\n> \t\t\treturn ret;\n> \n> +\t\tworker_ = std::make_unique(postProcessor_.get());\n> \t\tpostProcessor_->processComplete.connect(\n> \t\t\tthis, [&](Camera3RequestDescriptor *request, PostProcessor::Status status) {\n> \t\t\t\tcameraDevice_->streamProcessingComplete(this, request, status);\n> \t\t\t});\n> +\t\tworker_->start();\n> \t}\n> \n> \tif (type_ == Type::Internal) {\n> @@ -167,19 +169,26 @@ void CameraStream::process(const FrameBuffer &source,\n> \tif (!postProcessor_)\n> \t\treturn;\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.buffer, output.pixelFormat, output.size,\n> -\t\t\t PROT_READ | PROT_WRITE);\n> -\tif (!dest.isValid()) {\n> +\trequest->dest_ = std::make_unique(\n> +\t\t*camera3Dest.buffer, output.pixelFormat, output.size, PROT_READ | PROT_WRITE);\n> +\tif (!request->dest_->isValid()) {\n> \t\tLOG(HAL, Error) << \"Failed to create destination buffer\";\n> \t\treturn;\n> \t}\n> \n> -\tpostProcessor_->process(source, &dest, request);\n> +\trequest->src_ = &source;\n> +\n> +\t/* Push the postProcessor request to the worker queue. */\n> +\tworker_->queueRequest(request);\n> +}\n> +\n> +void CameraStream::flush()\n> +{\n> +\tif (!postProcessor_)\n> +\t\treturn;\n> +\n> +\tworker_->flush();\n> }\n> \n> FrameBuffer *CameraStream::getBuffer()\n> @@ -209,3 +218,86 @@ void CameraStream::putBuffer(FrameBuffer *buffer)\n> \n> \tbuffers_.push_back(buffer);\n> }\n> +\n> +CameraStream::PostProcessorWorker::PostProcessorWorker(PostProcessor *postProcessor)\n> +\t: postProcessor_(postProcessor)\n> +{\n> +}\n> +\n> +CameraStream::PostProcessorWorker::~PostProcessorWorker()\n> +{\n> +\t{\n> +\t\tlibcamera::MutexLocker lock(mutex_);\n> +\t\tstate_ = State::Stopped;\n> +\t}\n> +\n> +\tcv_.notify_one();\n> +\twait();\n> +}\n> +\n> +void CameraStream::PostProcessorWorker::start()\n> +{\n> +\t{\n> +\t\tlibcamera::MutexLocker lock(mutex_);\n> +\t\tstate_ = State::Running;\n> +\t}\n> +\n> +\tThread::start();\n> +}\n> +\n> +void CameraStream::PostProcessorWorker::queueRequest(Camera3RequestDescriptor *request)\n> +{\n> +\t{\n> +\t\tMutexLocker lock(mutex_);\n> +\t\tASSERT(state_ == State::Running);\n> +\t\trequests_.push(request);\n> +\t}\n> +\tcv_.notify_one();\n> +}\n> +\n> +void CameraStream::PostProcessorWorker::run()\n> +{\n> +\tMutexLocker locker(mutex_);\n> +\n> +\twhile (1) {\n> +\t\tcv_.wait(locker, [&] {\n> +\t\t\treturn state_ != State::Running || !requests_.empty();\n> +\t\t});\n> +\n> +\t\tif (state_ != State::Running)\n> +\t\t\tbreak;\n> +\n> +\t\tCamera3RequestDescriptor *descriptor = requests_.front();\n> +\t\trequests_.pop();\n> +\t\tlocker.unlock();\n> +\n> +\t\tpostProcessor_->process(*descriptor->src_, descriptor->dest_.get(),\n> +\t\t\t\t\tdescriptor);\n> +\n> +\t\tlocker.lock();\n> +\t}\n> +\n> +\tif (state_ == State::Flushing) {\n> +\t\twhile (!requests_.empty()) {\n> +\t\t\tpostProcessor_->processComplete.emit(requests_.front(),\n> +\t\t\t\t\t\t\t PostProcessor::Status::Error);\n> +\t\t\trequests_.pop();\n> +\t\t}\n> +\t\tstate_ = State::Stopped;\n> +\t\tlocker.unlock();\n> +\t\tcv_.notify_one();\n> +\t}\n> +}\n> +\n> +void CameraStream::PostProcessorWorker::flush()\n> +{\n> +\tlibcamera::MutexLocker lock(mutex_);\n> +\tstate_ = State::Flushing;\n> +\tlock.unlock();\n> +\tcv_.notify_one();\n> +\n> +\tlock.lock();\n> +\tcv_.wait(lock, [&] {\n\n\nMental note:\n\nThis can add a lot of latency to the flush op since it's happening on \nthe main thread. I think the queues (post-processing one and \ndescriptors) can be flushed out independently without waiting for \none-another.\n\nOne still needs to set error state on descriptors/buffers, so that will \nrequire some thinking (since you can't iterate over a std::queue but \nset-pop() only, even for error setting). This might introduce another \nprocess_capture_results() callback outside sendCaptureResults (which is \na divergence from ideal but maybe need to bite that bullet). The goal \nhere is to be as fast as possible on flush op, to clear the queues. As \nan experiment, one can measure asctual latency on flush() as in master \nto have a common base figure can compare with the latency of this series\n\n\n> +\t\treturn state_ == State::Stopped;\n> +\t});\n> +}\n> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n> index a0c5f166..e410f35d 100644\n> --- a/src/android/camera_stream.h\n> +++ b/src/android/camera_stream.h\n> @@ -7,21 +7,26 @@\n> #ifndef __ANDROID_CAMERA_STREAM_H__\n> #define __ANDROID_CAMERA_STREAM_H__\n> \n> +#include \n> #include \n> #include \n> +#include \n> #include \n> \n> #include \n> \n> +#include \n> +\n> #include \n> #include \n> #include \n> #include \n> #include \n> \n> +#include \"post_processor.h\"\n> +\n> class CameraDevice;\n> class CameraMetadata;\n> -class PostProcessor;\n> \n> struct Camera3RequestDescriptor;\n> \n> @@ -125,8 +130,38 @@ public:\n> \t\t Camera3RequestDescriptor *request);\n> \tlibcamera::FrameBuffer *getBuffer();\n> \tvoid putBuffer(libcamera::FrameBuffer *buffer);\n> +\tvoid flush();\n> \n> private:\n> +\tclass PostProcessorWorker : public libcamera::Thread\n> +\t{\n> +\tpublic:\n> +\t\tenum class State {\n> +\t\t\tStopped,\n> +\t\t\tRunning,\n> +\t\t\tFlushing,\n> +\t\t};\n> +\n> +\t\tPostProcessorWorker(PostProcessor *postProcessor);\n> +\t\t~PostProcessorWorker();\n> +\n> +\t\tvoid start();\n> +\t\tvoid queueRequest(Camera3RequestDescriptor *request);\n> +\t\tvoid flush();\n> +\n> +\tprotected:\n> +\t\tvoid run() override;\n> +\n> +\tprivate:\n> +\t\tPostProcessor *postProcessor_;\n> +\n> +\t\tlibcamera::Mutex mutex_;\n> +\t\tstd::condition_variable cv_;\n> +\n> +\t\tstd::queue requests_;\n> +\t\tState state_;\n> +\t};\n> +\n> \tint waitFence(int fence);\n> \n> \tCameraDevice *const cameraDevice_;\n> @@ -143,6 +178,8 @@ private:\n> \t */\n> \tstd::unique_ptr mutex_;\n> \tstd::unique_ptr postProcessor_;\n> +\n> +\tstd::unique_ptr worker_;\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 0345FBDC71\n\tfor ;\n\tMon, 11 Oct 2021 18:17:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 52D0468F4C;\n\tMon, 11 Oct 2021 20:17:27 +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 007456012B\n\tfor ;\n\tMon, 11 Oct 2021 20:17:25 +0200 (CEST)","from [192.168.1.106] (unknown [103.251.226.107])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id F3DD1FD3;\n\tMon, 11 Oct 2021 20:17:24 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"rwh03hK9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1633976245;\n\tbh=qYltZRpR8j+It10B5pniDHgJ6ac0379onvoPa1Mo1KQ=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=rwh03hK9MOEtSnem81XlxVy7yGxTOleM+AbbMQzFeso1bZRnnCWTlVkfqnfymZH+6\n\tJHUBvPssTObZf0sXAc9kYxxh/zLUEl9FNj1ZWbMMlz++YjeYscQjO8sIThezl3L1wd\n\tTrcZliw6FavRWxaqxffa31CdpNyaVIJMtPruFUEg=","To":"libcamera-devel@lists.libcamera.org","References":"<20211011073505.243864-1-umang.jain@ideasonboard.com>\n\t<20211011073505.243864-6-umang.jain@ideasonboard.com>","From":"Umang Jain ","Message-ID":"","Date":"Mon, 11 Oct 2021 23:47:20 +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":"<20211011073505.243864-6-umang.jain@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v4 5/7] android: post_processor: Make\n\tpost processing async","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20154,"web_url":"https://patchwork.libcamera.org/comment/20154/","msgid":"","date":"2021-10-12T23:57:15","subject":"Re: [libcamera-devel] [PATCH v4 5/7] android: post_processor: Make\n\tpost processing async","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\ns/async/asynchronous/ in the subject line.\n\nOn Mon, Oct 11, 2021 at 01:05:03PM +0530, Umang Jain wrote:\n> Introduce a dedicated worker class derived from libcamera::Thread.\n> The worker class maintains a queue for post-processing requests\n> and waits for a post-processing request to become available.\n> It will process them as per FIFO before de-queuing it from the\n> queue.\n> \n> To get access to the source and destination buffers in the worker\n> thread, we also need to save a pointer to them in the\n> Camera3RequestDescriptor.\n> \n> This patch also implements a flush() for the PostProcessorWorker\n> class which is responsible to purge post-processing requests\n> queued up while a camera is stopping/flushing.\n> \n> The libcamera request completion handler CameraDevice::requestComplete()\n> assumes that the request that has just completed is at the front of the\n> queue. Now that the post-processor runs asynchronously, this isn't true\n> anymore, a request being post-processed will stay in the queue and a new\n> libcamera request may complete. Remove that assumption, and use the\n> request cookie to obtain the Camera3RequestDescriptor.\n> \n> Signed-off-by: Umang Jain \n> Signed-off-by: Laurent Pinchart \n> ---\n> src/android/camera_device.cpp | 25 +-------\n> src/android/camera_device.h | 3 +\n> src/android/camera_stream.cpp | 108 +++++++++++++++++++++++++++++++---\n> src/android/camera_stream.h | 39 +++++++++++-\n> 4 files changed, 144 insertions(+), 31 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index eba370ea..61b902ad 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -239,6 +239,7 @@ Camera3RequestDescriptor::Camera3RequestDescriptor(\n> \t/* Clone the controls associated with the camera3 request. */\n> \tsettings_ = CameraMetadata(camera3Request->settings);\n> \n> +\tdest_.reset();\n\ndest_ is a std::unique_ptr<>, its constructor will do the right thing,\nyou don't need to initialize it here.\n\n> \t/*\n> \t * Create the CaptureRequest, stored as a unique_ptr<> to tie its\n> \t * lifetime to the descriptor.\n> @@ -1094,28 +1095,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \n> void CameraDevice::requestComplete(Request *request)\n> {\n> -\tCamera3RequestDescriptor *descriptor;\n> -\t{\n> -\t\tMutexLocker descriptorsLock(descriptorsMutex_);\n> -\t\tASSERT(!descriptors_.empty());\n> -\t\tdescriptor = descriptors_.front().get();\n> -\t}\n> -\n> -\tif (descriptor->request_->cookie() != request->cookie()) {\n> -\t\t/*\n> -\t\t * \\todo Clarify if the Camera has to be closed on\n> -\t\t * ERROR_DEVICE and possibly demote the Fatal to simple\n> -\t\t * Error.\n> -\t\t */\n> -\t\tnotifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);\n> -\t\tLOG(HAL, Fatal)\n> -\t\t\t<< \"Out-of-order completion for request \"\n> -\t\t\t<< utils::hex(request->cookie());\n> -\n> -\t\tMutexLocker descriptorsLock(descriptorsMutex_);\n> -\t\tdescriptors_.pop();\n> -\t\treturn;\n> -\t}\n> +\tCamera3RequestDescriptor *descriptor =\n> +\t\treinterpret_cast(request->cookie());\n> \n> \t/*\n> \t * Prepare the capture result for the Android camera stack.\n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index eee97516..725a0618 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -59,6 +59,9 @@ struct Camera3RequestDescriptor {\n> \tstd::unique_ptr resultMetadata_;\n> \tlibcamera::FrameBuffer *internalBuffer_;\n> \n> +\tstd::unique_ptr dest_;\n> +\tconst libcamera::FrameBuffer *src_;\n\nAs mentioned in the review of the previous patch, you can have more than\none post-processed stream per request, so this won't be enough.\n\nI'd recomment first refactoring the Camera3RequestDescriptor class and\nadd an internal\n\n\tstruct Stream {\n\t\tcamera3_stream_buffer_t buffer;\n\t\tstd::unique_ptr frameBuffer;\n\t};\n\nwith the buffers_ and frameBuffers members replaced with\n\n\tstd::vector streams_;\n\nThen you can extend the Stream structure in this patch to add the\nnecessary fields.\n\nThinking some more about it, src_ is likely not needed, as it's a\npointer to the FrameBuffer already stored in struct Stream. What you'll\nneed will be the ability to find the Stream instance corresponding to a\ngiven libcamera stream, so maybe a map would be better than a vector.\n\nIt also seems like the PostProcessorWorker should move from processing\nrequests to processing streams, as there's one PostProcessorWorker for\neach CameraStream. Maybe the struct Stream should contain a pointer to\nits Camera3RequestDescriptor, that way you could pass the\nCamera3RequestDescriptor::Stream pointer to CameraStream::process() and\nto the post-processors, and then find the corresponding\nCamera3RequestDescriptor in the completion handler.\n\n> +\n> \tcamera3_capture_result_t captureResult_ = {};\n> \tStatus status_ = Status::Pending;\n> };\n> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> index cec07269..818ef948 100644\n> --- a/src/android/camera_stream.cpp\n> +++ b/src/android/camera_stream.cpp\n> @@ -94,10 +94,12 @@ int CameraStream::configure()\n> \t\tif (ret)\n> \t\t\treturn ret;\n> \n> +\t\tworker_ = std::make_unique(postProcessor_.get());\n> \t\tpostProcessor_->processComplete.connect(\n> \t\t\tthis, [&](Camera3RequestDescriptor *request, PostProcessor::Status status) {\n> \t\t\t\tcameraDevice_->streamProcessingComplete(this, request, status);\n> \t\t\t});\n> +\t\tworker_->start();\n> \t}\n> \n> \tif (type_ == Type::Internal) {\n> @@ -167,19 +169,26 @@ void CameraStream::process(const FrameBuffer &source,\n> \tif (!postProcessor_)\n> \t\treturn;\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.buffer, output.pixelFormat, output.size,\n> -\t\t\t PROT_READ | PROT_WRITE);\n> -\tif (!dest.isValid()) {\n> +\trequest->dest_ = std::make_unique(\n> +\t\t*camera3Dest.buffer, output.pixelFormat, output.size, PROT_READ | PROT_WRITE);\n> +\tif (!request->dest_->isValid()) {\n> \t\tLOG(HAL, Error) << \"Failed to create destination buffer\";\n> \t\treturn;\n> \t}\n> \n> -\tpostProcessor_->process(source, &dest, request);\n> +\trequest->src_ = &source;\n> +\n> +\t/* Push the postProcessor request to the worker queue. */\n> +\tworker_->queueRequest(request);\n> +}\n> +\n> +void CameraStream::flush()\n> +{\n> +\tif (!postProcessor_)\n> +\t\treturn;\n> +\n> +\tworker_->flush();\n> }\n> \n> FrameBuffer *CameraStream::getBuffer()\n> @@ -209,3 +218,86 @@ void CameraStream::putBuffer(FrameBuffer *buffer)\n> \n> \tbuffers_.push_back(buffer);\n> }\n> +\n> +CameraStream::PostProcessorWorker::PostProcessorWorker(PostProcessor *postProcessor)\n> +\t: postProcessor_(postProcessor)\n> +{\n> +}\n> +\n> +CameraStream::PostProcessorWorker::~PostProcessorWorker()\n> +{\n> +\t{\n> +\t\tlibcamera::MutexLocker lock(mutex_);\n> +\t\tstate_ = State::Stopped;\n> +\t}\n> +\n> +\tcv_.notify_one();\n> +\twait();\n> +}\n> +\n> +void CameraStream::PostProcessorWorker::start()\n> +{\n> +\t{\n> +\t\tlibcamera::MutexLocker lock(mutex_);\n> +\t\tstate_ = State::Running;\n> +\t}\n> +\n> +\tThread::start();\n> +}\n> +\n> +void CameraStream::PostProcessorWorker::queueRequest(Camera3RequestDescriptor *request)\n> +{\n> +\t{\n> +\t\tMutexLocker lock(mutex_);\n> +\t\tASSERT(state_ == State::Running);\n> +\t\trequests_.push(request);\n> +\t}\n> +\tcv_.notify_one();\n> +}\n> +\n> +void CameraStream::PostProcessorWorker::run()\n> +{\n> +\tMutexLocker locker(mutex_);\n> +\n> +\twhile (1) {\n> +\t\tcv_.wait(locker, [&] {\n> +\t\t\treturn state_ != State::Running || !requests_.empty();\n> +\t\t});\n> +\n> +\t\tif (state_ != State::Running)\n> +\t\t\tbreak;\n> +\n> +\t\tCamera3RequestDescriptor *descriptor = requests_.front();\n> +\t\trequests_.pop();\n> +\t\tlocker.unlock();\n> +\n> +\t\tpostProcessor_->process(*descriptor->src_, descriptor->dest_.get(),\n> +\t\t\t\t\tdescriptor);\n> +\n> +\t\tlocker.lock();\n> +\t}\n> +\n> +\tif (state_ == State::Flushing) {\n> +\t\twhile (!requests_.empty()) {\n> +\t\t\tpostProcessor_->processComplete.emit(requests_.front(),\n> +\t\t\t\t\t\t\t PostProcessor::Status::Error);\n> +\t\t\trequests_.pop();\n> +\t\t}\n> +\t\tstate_ = State::Stopped;\n> +\t\tlocker.unlock();\n> +\t\tcv_.notify_one();\n> +\t}\n> +}\n> +\n> +void CameraStream::PostProcessorWorker::flush()\n> +{\n> +\tlibcamera::MutexLocker lock(mutex_);\n> +\tstate_ = State::Flushing;\n> +\tlock.unlock();\n> +\tcv_.notify_one();\n> +\n> +\tlock.lock();\n> +\tcv_.wait(lock, [&] {\n> +\t\treturn state_ == State::Stopped;\n> +\t});\n> +}\n> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n> index a0c5f166..e410f35d 100644\n> --- a/src/android/camera_stream.h\n> +++ b/src/android/camera_stream.h\n> @@ -7,21 +7,26 @@\n> #ifndef __ANDROID_CAMERA_STREAM_H__\n> #define __ANDROID_CAMERA_STREAM_H__\n> \n> +#include \n> #include \n> #include \n> +#include \n> #include \n> \n> #include \n> \n> +#include \n> +\n> #include \n> #include \n> #include \n> #include \n> #include \n> \n> +#include \"post_processor.h\"\n> +\n> class CameraDevice;\n> class CameraMetadata;\n> -class PostProcessor;\n> \n> struct Camera3RequestDescriptor;\n> \n> @@ -125,8 +130,38 @@ public:\n> \t\t Camera3RequestDescriptor *request);\n> \tlibcamera::FrameBuffer *getBuffer();\n> \tvoid putBuffer(libcamera::FrameBuffer *buffer);\n> +\tvoid flush();\n> \n> private:\n> +\tclass PostProcessorWorker : public libcamera::Thread\n> +\t{\n> +\tpublic:\n> +\t\tenum class State {\n> +\t\t\tStopped,\n> +\t\t\tRunning,\n> +\t\t\tFlushing,\n> +\t\t};\n> +\n> +\t\tPostProcessorWorker(PostProcessor *postProcessor);\n> +\t\t~PostProcessorWorker();\n> +\n> +\t\tvoid start();\n> +\t\tvoid queueRequest(Camera3RequestDescriptor *request);\n> +\t\tvoid flush();\n> +\n> +\tprotected:\n> +\t\tvoid run() override;\n> +\n> +\tprivate:\n> +\t\tPostProcessor *postProcessor_;\n> +\n> +\t\tlibcamera::Mutex mutex_;\n> +\t\tstd::condition_variable cv_;\n> +\n> +\t\tstd::queue requests_;\n> +\t\tState state_;\n> +\t};\n> +\n> \tint waitFence(int fence);\n> \n> \tCameraDevice *const cameraDevice_;\n> @@ -143,6 +178,8 @@ private:\n> \t */\n> \tstd::unique_ptr mutex_;\n> \tstd::unique_ptr postProcessor_;\n> +\n> +\tstd::unique_ptr worker_;\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 D102FC323E\n\tfor ;\n\tTue, 12 Oct 2021 23:57:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0B9F368F4F;\n\tWed, 13 Oct 2021 01:57:32 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E51936012B\n\tfor ;\n\tWed, 13 Oct 2021 01:57: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 4FF0BE7;\n\tWed, 13 Oct 2021 01:57:29 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"oge7W7dV\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634083049;\n\tbh=8gyKbmAv1ByTfxNiFyM7FeHtOpPivQIs/bCQbtkBFkE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=oge7W7dV3DLVxZQjhTGmmLp7SI75xMFZLdiA54oVOhWEKdtICSEpIBfCCRcIQeluA\n\tSwlHJlWh7VZWcXTugAaJyRMSju72ZxfwhLDgcatHxUzyKdnJ7/+Rs9YxCKzB+c8WWf\n\tKax5vFDfqdWiIh023u+9oAO7jJeYTi9jh6rLP9lY=","Date":"Wed, 13 Oct 2021 02:57:15 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20211011073505.243864-1-umang.jain@ideasonboard.com>\n\t<20211011073505.243864-6-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211011073505.243864-6-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 5/7] android: post_processor: Make\n\tpost processing async","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":20155,"web_url":"https://patchwork.libcamera.org/comment/20155/","msgid":"","date":"2021-10-13T00:02:25","subject":"Re: [libcamera-devel] [PATCH v4 5/7] android: post_processor: Make\n\tpost processing async","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Oct 13, 2021 at 02:57:16AM +0300, Laurent Pinchart wrote:\n> Hi Umang,\n> \n> Thank you for the patch.\n> \n> s/async/asynchronous/ in the subject line.\n> \n> On Mon, Oct 11, 2021 at 01:05:03PM +0530, Umang Jain wrote:\n> > Introduce a dedicated worker class derived from libcamera::Thread.\n> > The worker class maintains a queue for post-processing requests\n> > and waits for a post-processing request to become available.\n> > It will process them as per FIFO before de-queuing it from the\n> > queue.\n> > \n> > To get access to the source and destination buffers in the worker\n> > thread, we also need to save a pointer to them in the\n> > Camera3RequestDescriptor.\n> > \n> > This patch also implements a flush() for the PostProcessorWorker\n> > class which is responsible to purge post-processing requests\n> > queued up while a camera is stopping/flushing.\n> > \n> > The libcamera request completion handler CameraDevice::requestComplete()\n> > assumes that the request that has just completed is at the front of the\n> > queue. Now that the post-processor runs asynchronously, this isn't true\n> > anymore, a request being post-processed will stay in the queue and a new\n> > libcamera request may complete. Remove that assumption, and use the\n> > request cookie to obtain the Camera3RequestDescriptor.\n> > \n> > Signed-off-by: Umang Jain \n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > src/android/camera_device.cpp | 25 +-------\n> > src/android/camera_device.h | 3 +\n> > src/android/camera_stream.cpp | 108 +++++++++++++++++++++++++++++++---\n> > src/android/camera_stream.h | 39 +++++++++++-\n> > 4 files changed, 144 insertions(+), 31 deletions(-)\n> > \n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index eba370ea..61b902ad 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -239,6 +239,7 @@ Camera3RequestDescriptor::Camera3RequestDescriptor(\n> > \t/* Clone the controls associated with the camera3 request. */\n> > \tsettings_ = CameraMetadata(camera3Request->settings);\n> > \n> > +\tdest_.reset();\n> \n> dest_ is a std::unique_ptr<>, its constructor will do the right thing,\n> you don't need to initialize it here.\n> \n> > \t/*\n> > \t * Create the CaptureRequest, stored as a unique_ptr<> to tie its\n> > \t * lifetime to the descriptor.\n> > @@ -1094,28 +1095,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > \n> > void CameraDevice::requestComplete(Request *request)\n> > {\n> > -\tCamera3RequestDescriptor *descriptor;\n> > -\t{\n> > -\t\tMutexLocker descriptorsLock(descriptorsMutex_);\n> > -\t\tASSERT(!descriptors_.empty());\n> > -\t\tdescriptor = descriptors_.front().get();\n> > -\t}\n> > -\n> > -\tif (descriptor->request_->cookie() != request->cookie()) {\n> > -\t\t/*\n> > -\t\t * \\todo Clarify if the Camera has to be closed on\n> > -\t\t * ERROR_DEVICE and possibly demote the Fatal to simple\n> > -\t\t * Error.\n> > -\t\t */\n> > -\t\tnotifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);\n> > -\t\tLOG(HAL, Fatal)\n> > -\t\t\t<< \"Out-of-order completion for request \"\n> > -\t\t\t<< utils::hex(request->cookie());\n> > -\n> > -\t\tMutexLocker descriptorsLock(descriptorsMutex_);\n> > -\t\tdescriptors_.pop();\n> > -\t\treturn;\n> > -\t}\n> > +\tCamera3RequestDescriptor *descriptor =\n> > +\t\treinterpret_cast(request->cookie());\n> > \n> > \t/*\n> > \t * Prepare the capture result for the Android camera stack.\n> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > index eee97516..725a0618 100644\n> > --- a/src/android/camera_device.h\n> > +++ b/src/android/camera_device.h\n> > @@ -59,6 +59,9 @@ struct Camera3RequestDescriptor {\n> > \tstd::unique_ptr resultMetadata_;\n> > \tlibcamera::FrameBuffer *internalBuffer_;\n> > \n> > +\tstd::unique_ptr dest_;\n> > +\tconst libcamera::FrameBuffer *src_;\n> \n> As mentioned in the review of the previous patch, you can have more than\n> one post-processed stream per request, so this won't be enough.\n> \n> I'd recomment first refactoring the Camera3RequestDescriptor class and\n> add an internal\n> \n> \tstruct Stream {\n> \t\tcamera3_stream_buffer_t buffer;\n> \t\tstd::unique_ptr frameBuffer;\n> \t};\n> \n> with the buffers_ and frameBuffers members replaced with\n> \n> \tstd::vector streams_;\n> \n> Then you can extend the Stream structure in this patch to add the\n> necessary fields.\n> \n> Thinking some more about it, src_ is likely not needed, as it's a\n> pointer to the FrameBuffer already stored in struct Stream. What you'll\n> need will be the ability to find the Stream instance corresponding to a\n> given libcamera stream, so maybe a map would be better than a vector.\n> \n> It also seems like the PostProcessorWorker should move from processing\n> requests to processing streams, as there's one PostProcessorWorker for\n> each CameraStream. Maybe the struct Stream should contain a pointer to\n> its Camera3RequestDescriptor, that way you could pass the\n> Camera3RequestDescriptor::Stream pointer to CameraStream::process() and\n> to the post-processors, and then find the corresponding\n> Camera3RequestDescriptor in the completion handler.\n\nBy the way, another option may be to move the PostProcessorWorker to\nCameraDevice and still give it a Camera3RequestDescriptor, and\ninternally in the thread run the post-processors sequentially for each\npost-processed stream. If we had a lot of post-processed streams I would\nsay that would be a better design, as we could then create a threads\npool (with N threads for M streams, and N < M) and dispatch the jobs to\nthose threads, but that's overkill I think. Still, maybe a single thread\ndesign would be easier and look cleaner, I'm not sure.\n\n> > +\n> > \tcamera3_capture_result_t captureResult_ = {};\n> > \tStatus status_ = Status::Pending;\n> > };\n> > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> > index cec07269..818ef948 100644\n> > --- a/src/android/camera_stream.cpp\n> > +++ b/src/android/camera_stream.cpp\n> > @@ -94,10 +94,12 @@ int CameraStream::configure()\n> > \t\tif (ret)\n> > \t\t\treturn ret;\n> > \n> > +\t\tworker_ = std::make_unique(postProcessor_.get());\n> > \t\tpostProcessor_->processComplete.connect(\n> > \t\t\tthis, [&](Camera3RequestDescriptor *request, PostProcessor::Status status) {\n> > \t\t\t\tcameraDevice_->streamProcessingComplete(this, request, status);\n> > \t\t\t});\n> > +\t\tworker_->start();\n> > \t}\n> > \n> > \tif (type_ == Type::Internal) {\n> > @@ -167,19 +169,26 @@ void CameraStream::process(const FrameBuffer &source,\n> > \tif (!postProcessor_)\n> > \t\treturn;\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.buffer, output.pixelFormat, output.size,\n> > -\t\t\t PROT_READ | PROT_WRITE);\n> > -\tif (!dest.isValid()) {\n> > +\trequest->dest_ = std::make_unique(\n> > +\t\t*camera3Dest.buffer, output.pixelFormat, output.size, PROT_READ | PROT_WRITE);\n> > +\tif (!request->dest_->isValid()) {\n> > \t\tLOG(HAL, Error) << \"Failed to create destination buffer\";\n> > \t\treturn;\n> > \t}\n> > \n> > -\tpostProcessor_->process(source, &dest, request);\n> > +\trequest->src_ = &source;\n> > +\n> > +\t/* Push the postProcessor request to the worker queue. */\n> > +\tworker_->queueRequest(request);\n> > +}\n> > +\n> > +void CameraStream::flush()\n> > +{\n> > +\tif (!postProcessor_)\n> > +\t\treturn;\n> > +\n> > +\tworker_->flush();\n> > }\n> > \n> > FrameBuffer *CameraStream::getBuffer()\n> > @@ -209,3 +218,86 @@ void CameraStream::putBuffer(FrameBuffer *buffer)\n> > \n> > \tbuffers_.push_back(buffer);\n> > }\n> > +\n> > +CameraStream::PostProcessorWorker::PostProcessorWorker(PostProcessor *postProcessor)\n> > +\t: postProcessor_(postProcessor)\n> > +{\n> > +}\n> > +\n> > +CameraStream::PostProcessorWorker::~PostProcessorWorker()\n> > +{\n> > +\t{\n> > +\t\tlibcamera::MutexLocker lock(mutex_);\n> > +\t\tstate_ = State::Stopped;\n> > +\t}\n> > +\n> > +\tcv_.notify_one();\n> > +\twait();\n> > +}\n> > +\n> > +void CameraStream::PostProcessorWorker::start()\n> > +{\n> > +\t{\n> > +\t\tlibcamera::MutexLocker lock(mutex_);\n> > +\t\tstate_ = State::Running;\n> > +\t}\n> > +\n> > +\tThread::start();\n> > +}\n> > +\n> > +void CameraStream::PostProcessorWorker::queueRequest(Camera3RequestDescriptor *request)\n> > +{\n> > +\t{\n> > +\t\tMutexLocker lock(mutex_);\n> > +\t\tASSERT(state_ == State::Running);\n> > +\t\trequests_.push(request);\n> > +\t}\n> > +\tcv_.notify_one();\n> > +}\n> > +\n> > +void CameraStream::PostProcessorWorker::run()\n> > +{\n> > +\tMutexLocker locker(mutex_);\n> > +\n> > +\twhile (1) {\n> > +\t\tcv_.wait(locker, [&] {\n> > +\t\t\treturn state_ != State::Running || !requests_.empty();\n> > +\t\t});\n> > +\n> > +\t\tif (state_ != State::Running)\n> > +\t\t\tbreak;\n> > +\n> > +\t\tCamera3RequestDescriptor *descriptor = requests_.front();\n> > +\t\trequests_.pop();\n> > +\t\tlocker.unlock();\n> > +\n> > +\t\tpostProcessor_->process(*descriptor->src_, descriptor->dest_.get(),\n> > +\t\t\t\t\tdescriptor);\n> > +\n> > +\t\tlocker.lock();\n> > +\t}\n> > +\n> > +\tif (state_ == State::Flushing) {\n> > +\t\twhile (!requests_.empty()) {\n> > +\t\t\tpostProcessor_->processComplete.emit(requests_.front(),\n> > +\t\t\t\t\t\t\t PostProcessor::Status::Error);\n> > +\t\t\trequests_.pop();\n> > +\t\t}\n> > +\t\tstate_ = State::Stopped;\n> > +\t\tlocker.unlock();\n> > +\t\tcv_.notify_one();\n> > +\t}\n> > +}\n> > +\n> > +void CameraStream::PostProcessorWorker::flush()\n> > +{\n> > +\tlibcamera::MutexLocker lock(mutex_);\n> > +\tstate_ = State::Flushing;\n> > +\tlock.unlock();\n> > +\tcv_.notify_one();\n> > +\n> > +\tlock.lock();\n> > +\tcv_.wait(lock, [&] {\n> > +\t\treturn state_ == State::Stopped;\n> > +\t});\n> > +}\n> > diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n> > index a0c5f166..e410f35d 100644\n> > --- a/src/android/camera_stream.h\n> > +++ b/src/android/camera_stream.h\n> > @@ -7,21 +7,26 @@\n> > #ifndef __ANDROID_CAMERA_STREAM_H__\n> > #define __ANDROID_CAMERA_STREAM_H__\n> > \n> > +#include \n> > #include \n> > #include \n> > +#include \n> > #include \n> > \n> > #include \n> > \n> > +#include \n> > +\n> > #include \n> > #include \n> > #include \n> > #include \n> > #include \n> > \n> > +#include \"post_processor.h\"\n> > +\n> > class CameraDevice;\n> > class CameraMetadata;\n> > -class PostProcessor;\n> > \n> > struct Camera3RequestDescriptor;\n> > \n> > @@ -125,8 +130,38 @@ public:\n> > \t\t Camera3RequestDescriptor *request);\n> > \tlibcamera::FrameBuffer *getBuffer();\n> > \tvoid putBuffer(libcamera::FrameBuffer *buffer);\n> > +\tvoid flush();\n> > \n> > private:\n> > +\tclass PostProcessorWorker : public libcamera::Thread\n> > +\t{\n> > +\tpublic:\n> > +\t\tenum class State {\n> > +\t\t\tStopped,\n> > +\t\t\tRunning,\n> > +\t\t\tFlushing,\n> > +\t\t};\n> > +\n> > +\t\tPostProcessorWorker(PostProcessor *postProcessor);\n> > +\t\t~PostProcessorWorker();\n> > +\n> > +\t\tvoid start();\n> > +\t\tvoid queueRequest(Camera3RequestDescriptor *request);\n> > +\t\tvoid flush();\n> > +\n> > +\tprotected:\n> > +\t\tvoid run() override;\n> > +\n> > +\tprivate:\n> > +\t\tPostProcessor *postProcessor_;\n> > +\n> > +\t\tlibcamera::Mutex mutex_;\n> > +\t\tstd::condition_variable cv_;\n> > +\n> > +\t\tstd::queue requests_;\n> > +\t\tState state_;\n> > +\t};\n> > +\n> > \tint waitFence(int fence);\n> > \n> > \tCameraDevice *const cameraDevice_;\n> > @@ -143,6 +178,8 @@ private:\n> > \t */\n> > \tstd::unique_ptr mutex_;\n> > \tstd::unique_ptr postProcessor_;\n> > +\n> > +\tstd::unique_ptr worker_;\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 95D11BDC71\n\tfor ;\n\tWed, 13 Oct 2021 00:02:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 05E7068F4F;\n\tWed, 13 Oct 2021 02:02:41 +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 BF1646012B\n\tfor ;\n\tWed, 13 Oct 2021 02:02:39 +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 2C2F1E7;\n\tWed, 13 Oct 2021 02:02:39 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"vtodqc3I\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634083359;\n\tbh=0coMp1R+97ERv9TJsaKn1suygDt6bm/RUmbC20pAL2A=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=vtodqc3I93HawvOnNBgeDUaEcXo3Uh0zdDfN0s1375bciXRp3SwcL56TQ1r2uWhhu\n\tiSLyhVt5L//BEeRdEefo6x7eeczvZ9wbjeUNotZnRiyD5jmi+h3u31niLngJMjwuX2\n\t7G/n59YtzyHrSb5+TfVBoc1KA/4R6RAfBJiTrQPs=","Date":"Wed, 13 Oct 2021 03:02:25 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20211011073505.243864-1-umang.jain@ideasonboard.com>\n\t<20211011073505.243864-6-umang.jain@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v4 5/7] android: post_processor: Make\n\tpost processing async","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":20169,"web_url":"https://patchwork.libcamera.org/comment/20169/","msgid":"<6744f251-bec3-19e4-545a-8a148940d872@ideasonboard.com>","date":"2021-10-13T09:44:21","subject":"Re: [libcamera-devel] [PATCH v4 5/7] android: post_processor: Make\n\tpost processing async","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent,\n\nThank you for your thoughts\n\nOn 10/13/21 5:27 AM, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> Thank you for the patch.\n>\n> s/async/asynchronous/ in the subject line.\n>\n> On Mon, Oct 11, 2021 at 01:05:03PM +0530, Umang Jain wrote:\n>> Introduce a dedicated worker class derived from libcamera::Thread.\n>> The worker class maintains a queue for post-processing requests\n>> and waits for a post-processing request to become available.\n>> It will process them as per FIFO before de-queuing it from the\n>> queue.\n>>\n>> To get access to the source and destination buffers in the worker\n>> thread, we also need to save a pointer to them in the\n>> Camera3RequestDescriptor.\n>>\n>> This patch also implements a flush() for the PostProcessorWorker\n>> class which is responsible to purge post-processing requests\n>> queued up while a camera is stopping/flushing.\n>>\n>> The libcamera request completion handler CameraDevice::requestComplete()\n>> assumes that the request that has just completed is at the front of the\n>> queue. Now that the post-processor runs asynchronously, this isn't true\n>> anymore, a request being post-processed will stay in the queue and a new\n>> libcamera request may complete. Remove that assumption, and use the\n>> request cookie to obtain the Camera3RequestDescriptor.\n>>\n>> Signed-off-by: Umang Jain \n>> Signed-off-by: Laurent Pinchart \n>> ---\n>> src/android/camera_device.cpp | 25 +-------\n>> src/android/camera_device.h | 3 +\n>> src/android/camera_stream.cpp | 108 +++++++++++++++++++++++++++++++---\n>> src/android/camera_stream.h | 39 +++++++++++-\n>> 4 files changed, 144 insertions(+), 31 deletions(-)\n>>\n>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>> index eba370ea..61b902ad 100644\n>> --- a/src/android/camera_device.cpp\n>> +++ b/src/android/camera_device.cpp\n>> @@ -239,6 +239,7 @@ Camera3RequestDescriptor::Camera3RequestDescriptor(\n>> \t/* Clone the controls associated with the camera3 request. */\n>> \tsettings_ = CameraMetadata(camera3Request->settings);\n>> \n>> +\tdest_.reset();\n> dest_ is a std::unique_ptr<>, its constructor will do the right thing,\n> you don't need to initialize it here.\n>\n>> \t/*\n>> \t * Create the CaptureRequest, stored as a unique_ptr<> to tie its\n>> \t * lifetime to the descriptor.\n>> @@ -1094,28 +1095,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>> \n>> void CameraDevice::requestComplete(Request *request)\n>> {\n>> -\tCamera3RequestDescriptor *descriptor;\n>> -\t{\n>> -\t\tMutexLocker descriptorsLock(descriptorsMutex_);\n>> -\t\tASSERT(!descriptors_.empty());\n>> -\t\tdescriptor = descriptors_.front().get();\n>> -\t}\n>> -\n>> -\tif (descriptor->request_->cookie() != request->cookie()) {\n>> -\t\t/*\n>> -\t\t * \\todo Clarify if the Camera has to be closed on\n>> -\t\t * ERROR_DEVICE and possibly demote the Fatal to simple\n>> -\t\t * Error.\n>> -\t\t */\n>> -\t\tnotifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);\n>> -\t\tLOG(HAL, Fatal)\n>> -\t\t\t<< \"Out-of-order completion for request \"\n>> -\t\t\t<< utils::hex(request->cookie());\n>> -\n>> -\t\tMutexLocker descriptorsLock(descriptorsMutex_);\n>> -\t\tdescriptors_.pop();\n>> -\t\treturn;\n>> -\t}\n>> +\tCamera3RequestDescriptor *descriptor =\n>> +\t\treinterpret_cast(request->cookie());\n>> \n>> \t/*\n>> \t * Prepare the capture result for the Android camera stack.\n>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n>> index eee97516..725a0618 100644\n>> --- a/src/android/camera_device.h\n>> +++ b/src/android/camera_device.h\n>> @@ -59,6 +59,9 @@ struct Camera3RequestDescriptor {\n>> \tstd::unique_ptr resultMetadata_;\n>> \tlibcamera::FrameBuffer *internalBuffer_;\n>> \n>> +\tstd::unique_ptr dest_;\n>> +\tconst libcamera::FrameBuffer *src_;\n> As mentioned in the review of the previous patch, you can have more than\n> one post-processed stream per request, so this won't be enough.\n\n\nDo we actually know if we have such requests coming from, that require \nmultiple post-processed streams?\n\nOr is the answer, \"We might, from the CTS framework\" ?\n\n>\n> I'd recomment first refactoring the Camera3RequestDescriptor class and\n> add an internal\n>\n> \tstruct Stream {\n> \t\tcamera3_stream_buffer_t buffer;\n> \t\tstd::unique_ptr frameBuffer;\n> \t};\n>\n> with the buffers_ and frameBuffers members replaced with\n>\n> \tstd::vector streams_;\n>\n> Then you can extend the Stream structure in this patch to add the\n> necessary fields.\n\n\nMakes sense.\n\n>\n> Thinking some more about it, src_ is likely not needed, as it's a\n> pointer to the FrameBuffer already stored in struct Stream. What you'll\n> need will be the ability to find the Stream instance corresponding to a\n> given libcamera stream, so maybe a map would be better than a vector.\n\nYes, we should be able to associate it from the start otherwise we will \nneed to introduce needless iterations on finding it just queuing it to \npost-processor.\n\n\n>\n> It also seems like the PostProcessorWorker should move from processing\n> requests to processing streams, as there's one PostProcessorWorker for\n> each CameraStream. Maybe the struct Stream should contain a pointer to\n> its Camera3RequestDescriptor, that way you could pass the\n> Camera3RequestDescriptor::Stream pointer to CameraStream::process() and\n> to the post-processors, and then find the corresponding\n> Camera3RequestDescriptor in the completion handler.\n\n\nOne potential issue here post-processing streams might require to know \nthe statuses of other (post-processing) streams too, belonging to the \nsame request? Because we need to complete the request only after all \nstreams have finished post-processing,\n\nCurrently if we only maintain one queue, that can end up with \npost-processing requests of streams from multiple successive requests. \nNeed to think a bit but surely, we might need to containerize this \nfurther I think (container for streams of one request + container for \npost-processing requests). We have the latter, but I think we also need \nto have a former but avaibility to check container.empty(). Error \nhandling paths also might can get tricky in such a scenario. Let's see.\n\n>\n>> +\n>> \tcamera3_capture_result_t captureResult_ = {};\n>> \tStatus status_ = Status::Pending;\n>> };\n>> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n>> index cec07269..818ef948 100644\n>> --- a/src/android/camera_stream.cpp\n>> +++ b/src/android/camera_stream.cpp\n>> @@ -94,10 +94,12 @@ int CameraStream::configure()\n>> \t\tif (ret)\n>> \t\t\treturn ret;\n>> \n>> +\t\tworker_ = std::make_unique(postProcessor_.get());\n>> \t\tpostProcessor_->processComplete.connect(\n>> \t\t\tthis, [&](Camera3RequestDescriptor *request, PostProcessor::Status status) {\n>> \t\t\t\tcameraDevice_->streamProcessingComplete(this, request, status);\n>> \t\t\t});\n>> +\t\tworker_->start();\n>> \t}\n>> \n>> \tif (type_ == Type::Internal) {\n>> @@ -167,19 +169,26 @@ void CameraStream::process(const FrameBuffer &source,\n>> \tif (!postProcessor_)\n>> \t\treturn;\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.buffer, output.pixelFormat, output.size,\n>> -\t\t\t PROT_READ | PROT_WRITE);\n>> -\tif (!dest.isValid()) {\n>> +\trequest->dest_ = std::make_unique(\n>> +\t\t*camera3Dest.buffer, output.pixelFormat, output.size, PROT_READ | PROT_WRITE);\n>> +\tif (!request->dest_->isValid()) {\n>> \t\tLOG(HAL, Error) << \"Failed to create destination buffer\";\n>> \t\treturn;\n>> \t}\n>> \n>> -\tpostProcessor_->process(source, &dest, request);\n>> +\trequest->src_ = &source;\n>> +\n>> +\t/* Push the postProcessor request to the worker queue. */\n>> +\tworker_->queueRequest(request);\n>> +}\n>> +\n>> +void CameraStream::flush()\n>> +{\n>> +\tif (!postProcessor_)\n>> +\t\treturn;\n>> +\n>> +\tworker_->flush();\n>> }\n>> \n>> FrameBuffer *CameraStream::getBuffer()\n>> @@ -209,3 +218,86 @@ void CameraStream::putBuffer(FrameBuffer *buffer)\n>> \n>> \tbuffers_.push_back(buffer);\n>> }\n>> +\n>> +CameraStream::PostProcessorWorker::PostProcessorWorker(PostProcessor *postProcessor)\n>> +\t: postProcessor_(postProcessor)\n>> +{\n>> +}\n>> +\n>> +CameraStream::PostProcessorWorker::~PostProcessorWorker()\n>> +{\n>> +\t{\n>> +\t\tlibcamera::MutexLocker lock(mutex_);\n>> +\t\tstate_ = State::Stopped;\n>> +\t}\n>> +\n>> +\tcv_.notify_one();\n>> +\twait();\n>> +}\n>> +\n>> +void CameraStream::PostProcessorWorker::start()\n>> +{\n>> +\t{\n>> +\t\tlibcamera::MutexLocker lock(mutex_);\n>> +\t\tstate_ = State::Running;\n>> +\t}\n>> +\n>> +\tThread::start();\n>> +}\n>> +\n>> +void CameraStream::PostProcessorWorker::queueRequest(Camera3RequestDescriptor *request)\n>> +{\n>> +\t{\n>> +\t\tMutexLocker lock(mutex_);\n>> +\t\tASSERT(state_ == State::Running);\n>> +\t\trequests_.push(request);\n>> +\t}\n>> +\tcv_.notify_one();\n>> +}\n>> +\n>> +void CameraStream::PostProcessorWorker::run()\n>> +{\n>> +\tMutexLocker locker(mutex_);\n>> +\n>> +\twhile (1) {\n>> +\t\tcv_.wait(locker, [&] {\n>> +\t\t\treturn state_ != State::Running || !requests_.empty();\n>> +\t\t});\n>> +\n>> +\t\tif (state_ != State::Running)\n>> +\t\t\tbreak;\n>> +\n>> +\t\tCamera3RequestDescriptor *descriptor = requests_.front();\n>> +\t\trequests_.pop();\n>> +\t\tlocker.unlock();\n>> +\n>> +\t\tpostProcessor_->process(*descriptor->src_, descriptor->dest_.get(),\n>> +\t\t\t\t\tdescriptor);\n>> +\n>> +\t\tlocker.lock();\n>> +\t}\n>> +\n>> +\tif (state_ == State::Flushing) {\n>> +\t\twhile (!requests_.empty()) {\n>> +\t\t\tpostProcessor_->processComplete.emit(requests_.front(),\n>> +\t\t\t\t\t\t\t PostProcessor::Status::Error);\n>> +\t\t\trequests_.pop();\n>> +\t\t}\n>> +\t\tstate_ = State::Stopped;\n>> +\t\tlocker.unlock();\n>> +\t\tcv_.notify_one();\n>> +\t}\n>> +}\n>> +\n>> +void CameraStream::PostProcessorWorker::flush()\n>> +{\n>> +\tlibcamera::MutexLocker lock(mutex_);\n>> +\tstate_ = State::Flushing;\n>> +\tlock.unlock();\n>> +\tcv_.notify_one();\n>> +\n>> +\tlock.lock();\n>> +\tcv_.wait(lock, [&] {\n>> +\t\treturn state_ == State::Stopped;\n>> +\t});\n>> +}\n>> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n>> index a0c5f166..e410f35d 100644\n>> --- a/src/android/camera_stream.h\n>> +++ b/src/android/camera_stream.h\n>> @@ -7,21 +7,26 @@\n>> #ifndef __ANDROID_CAMERA_STREAM_H__\n>> #define __ANDROID_CAMERA_STREAM_H__\n>> \n>> +#include \n>> #include \n>> #include \n>> +#include \n>> #include \n>> \n>> #include \n>> \n>> +#include \n>> +\n>> #include \n>> #include \n>> #include \n>> #include \n>> #include \n>> \n>> +#include \"post_processor.h\"\n>> +\n>> class CameraDevice;\n>> class CameraMetadata;\n>> -class PostProcessor;\n>> \n>> struct Camera3RequestDescriptor;\n>> \n>> @@ -125,8 +130,38 @@ public:\n>> \t\t Camera3RequestDescriptor *request);\n>> \tlibcamera::FrameBuffer *getBuffer();\n>> \tvoid putBuffer(libcamera::FrameBuffer *buffer);\n>> +\tvoid flush();\n>> \n>> private:\n>> +\tclass PostProcessorWorker : public libcamera::Thread\n>> +\t{\n>> +\tpublic:\n>> +\t\tenum class State {\n>> +\t\t\tStopped,\n>> +\t\t\tRunning,\n>> +\t\t\tFlushing,\n>> +\t\t};\n>> +\n>> +\t\tPostProcessorWorker(PostProcessor *postProcessor);\n>> +\t\t~PostProcessorWorker();\n>> +\n>> +\t\tvoid start();\n>> +\t\tvoid queueRequest(Camera3RequestDescriptor *request);\n>> +\t\tvoid flush();\n>> +\n>> +\tprotected:\n>> +\t\tvoid run() override;\n>> +\n>> +\tprivate:\n>> +\t\tPostProcessor *postProcessor_;\n>> +\n>> +\t\tlibcamera::Mutex mutex_;\n>> +\t\tstd::condition_variable cv_;\n>> +\n>> +\t\tstd::queue requests_;\n>> +\t\tState state_;\n>> +\t};\n>> +\n>> \tint waitFence(int fence);\n>> \n>> \tCameraDevice *const cameraDevice_;\n>> @@ -143,6 +178,8 @@ private:\n>> \t */\n>> \tstd::unique_ptr mutex_;\n>> \tstd::unique_ptr postProcessor_;\n>> +\n>> +\tstd::unique_ptr worker_;\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 E2986BDC71\n\tfor ;\n\tWed, 13 Oct 2021 09:44:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4166A68F51;\n\tWed, 13 Oct 2021 11:44:29 +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 8413268546\n\tfor ;\n\tWed, 13 Oct 2021 11:44:27 +0200 (CEST)","from [192.168.1.106] (unknown [103.251.226.115])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 818B718FF;\n\tWed, 13 Oct 2021 11:44:26 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"DUaufn8t\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634118267;\n\tbh=tpUfvpRtES/t6k2EmCiBjNanNytg0x5xmCO9vmkuZHM=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=DUaufn8t+XE2eYnG0jYE2ILl6M95zmeZqlcw9u+l2Y52oKY6xrsBBsKiZGRgnvugn\n\t7RzALucCIgojOY+lRw/fE1r1L/oGJwaCEJJbvkZvJzo8UD1pbf46jEFQJBwgTrSWRk\n\tZzCBlug/kYOrs0OzoWPSC1U0RUfXASblGVm0aTv8=","To":"Laurent Pinchart ","References":"<20211011073505.243864-1-umang.jain@ideasonboard.com>\n\t<20211011073505.243864-6-umang.jain@ideasonboard.com>\n\t","From":"Umang Jain ","Message-ID":"<6744f251-bec3-19e4-545a-8a148940d872@ideasonboard.com>","Date":"Wed, 13 Oct 2021 15:14:21 +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 v4 5/7] android: post_processor: Make\n\tpost processing async","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":20171,"web_url":"https://patchwork.libcamera.org/comment/20171/","msgid":"","date":"2021-10-13T09:51:28","subject":"Re: [libcamera-devel] [PATCH v4 5/7] android: post_processor: Make\n\tpost processing async","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 10/13/21 5:32 AM, Laurent Pinchart wrote:\n> On Wed, Oct 13, 2021 at 02:57:16AM +0300, Laurent Pinchart wrote:\n>> Hi Umang,\n>>\n>> Thank you for the patch.\n>>\n>> s/async/asynchronous/ in the subject line.\n>>\n>> On Mon, Oct 11, 2021 at 01:05:03PM +0530, Umang Jain wrote:\n>>> Introduce a dedicated worker class derived from libcamera::Thread.\n>>> The worker class maintains a queue for post-processing requests\n>>> and waits for a post-processing request to become available.\n>>> It will process them as per FIFO before de-queuing it from the\n>>> queue.\n>>>\n>>> To get access to the source and destination buffers in the worker\n>>> thread, we also need to save a pointer to them in the\n>>> Camera3RequestDescriptor.\n>>>\n>>> This patch also implements a flush() for the PostProcessorWorker\n>>> class which is responsible to purge post-processing requests\n>>> queued up while a camera is stopping/flushing.\n>>>\n>>> The libcamera request completion handler CameraDevice::requestComplete()\n>>> assumes that the request that has just completed is at the front of the\n>>> queue. Now that the post-processor runs asynchronously, this isn't true\n>>> anymore, a request being post-processed will stay in the queue and a new\n>>> libcamera request may complete. Remove that assumption, and use the\n>>> request cookie to obtain the Camera3RequestDescriptor.\n>>>\n>>> Signed-off-by: Umang Jain \n>>> Signed-off-by: Laurent Pinchart \n>>> ---\n>>> src/android/camera_device.cpp | 25 +-------\n>>> src/android/camera_device.h | 3 +\n>>> src/android/camera_stream.cpp | 108 +++++++++++++++++++++++++++++++---\n>>> src/android/camera_stream.h | 39 +++++++++++-\n>>> 4 files changed, 144 insertions(+), 31 deletions(-)\n>>>\n>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>>> index eba370ea..61b902ad 100644\n>>> --- a/src/android/camera_device.cpp\n>>> +++ b/src/android/camera_device.cpp\n>>> @@ -239,6 +239,7 @@ Camera3RequestDescriptor::Camera3RequestDescriptor(\n>>> \t/* Clone the controls associated with the camera3 request. */\n>>> \tsettings_ = CameraMetadata(camera3Request->settings);\n>>> \n>>> +\tdest_.reset();\n>> dest_ is a std::unique_ptr<>, its constructor will do the right thing,\n>> you don't need to initialize it here.\n>>\n>>> \t/*\n>>> \t * Create the CaptureRequest, stored as a unique_ptr<> to tie its\n>>> \t * lifetime to the descriptor.\n>>> @@ -1094,28 +1095,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>>> \n>>> void CameraDevice::requestComplete(Request *request)\n>>> {\n>>> -\tCamera3RequestDescriptor *descriptor;\n>>> -\t{\n>>> -\t\tMutexLocker descriptorsLock(descriptorsMutex_);\n>>> -\t\tASSERT(!descriptors_.empty());\n>>> -\t\tdescriptor = descriptors_.front().get();\n>>> -\t}\n>>> -\n>>> -\tif (descriptor->request_->cookie() != request->cookie()) {\n>>> -\t\t/*\n>>> -\t\t * \\todo Clarify if the Camera has to be closed on\n>>> -\t\t * ERROR_DEVICE and possibly demote the Fatal to simple\n>>> -\t\t * Error.\n>>> -\t\t */\n>>> -\t\tnotifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);\n>>> -\t\tLOG(HAL, Fatal)\n>>> -\t\t\t<< \"Out-of-order completion for request \"\n>>> -\t\t\t<< utils::hex(request->cookie());\n>>> -\n>>> -\t\tMutexLocker descriptorsLock(descriptorsMutex_);\n>>> -\t\tdescriptors_.pop();\n>>> -\t\treturn;\n>>> -\t}\n>>> +\tCamera3RequestDescriptor *descriptor =\n>>> +\t\treinterpret_cast(request->cookie());\n>>> \n>>> \t/*\n>>> \t * Prepare the capture result for the Android camera stack.\n>>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n>>> index eee97516..725a0618 100644\n>>> --- a/src/android/camera_device.h\n>>> +++ b/src/android/camera_device.h\n>>> @@ -59,6 +59,9 @@ struct Camera3RequestDescriptor {\n>>> \tstd::unique_ptr resultMetadata_;\n>>> \tlibcamera::FrameBuffer *internalBuffer_;\n>>> \n>>> +\tstd::unique_ptr dest_;\n>>> +\tconst libcamera::FrameBuffer *src_;\n>> As mentioned in the review of the previous patch, you can have more than\n>> one post-processed stream per request, so this won't be enough.\n>>\n>> I'd recomment first refactoring the Camera3RequestDescriptor class and\n>> add an internal\n>>\n>> \tstruct Stream {\n>> \t\tcamera3_stream_buffer_t buffer;\n>> \t\tstd::unique_ptr frameBuffer;\n>> \t};\n>>\n>> with the buffers_ and frameBuffers members replaced with\n>>\n>> \tstd::vector streams_;\n>>\n>> Then you can extend the Stream structure in this patch to add the\n>> necessary fields.\n>>\n>> Thinking some more about it, src_ is likely not needed, as it's a\n>> pointer to the FrameBuffer already stored in struct Stream. What you'll\n>> need will be the ability to find the Stream instance corresponding to a\n>> given libcamera stream, so maybe a map would be better than a vector.\n>>\n>> It also seems like the PostProcessorWorker should move from processing\n>> requests to processing streams, as there's one PostProcessorWorker for\n>> each CameraStream. Maybe the struct Stream should contain a pointer to\n>> its Camera3RequestDescriptor, that way you could pass the\n>> Camera3RequestDescriptor::Stream pointer to CameraStream::process() and\n>> to the post-processors, and then find the corresponding\n>> Camera3RequestDescriptor in the completion handler.\n> By the way, another option may be to move the PostProcessorWorker to\n> CameraDevice and still give it a Camera3RequestDescriptor, and\n> internally in the thread run the post-processors sequentially for each\n> post-processed stream. If we had a lot of post-processed streams I would\n> say that would be a better design, as we could then create a threads\n> pool (with N threads for M streams, and N < M) and dispatch the jobs to\n> those threads, but that's overkill I think. Still, maybe a single thread\n> design would be easier and look cleaner, I'm not sure.\n\n\nPostProcessorWorker can be a self sustaining but looking at the things \nright now, I would leave it in camera-stream itself.\n\nIf we end up with thread pools in the future, I will happy to rework it, \nis that okay?\n\n>\n>>> +\n>>> \tcamera3_capture_result_t captureResult_ = {};\n>>> \tStatus status_ = Status::Pending;\n>>> };\n>>> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n>>> index cec07269..818ef948 100644\n>>> --- a/src/android/camera_stream.cpp\n>>> +++ b/src/android/camera_stream.cpp\n>>> @@ -94,10 +94,12 @@ int CameraStream::configure()\n>>> \t\tif (ret)\n>>> \t\t\treturn ret;\n>>> \n>>> +\t\tworker_ = std::make_unique(postProcessor_.get());\n>>> \t\tpostProcessor_->processComplete.connect(\n>>> \t\t\tthis, [&](Camera3RequestDescriptor *request, PostProcessor::Status status) {\n>>> \t\t\t\tcameraDevice_->streamProcessingComplete(this, request, status);\n>>> \t\t\t});\n>>> +\t\tworker_->start();\n>>> \t}\n>>> \n>>> \tif (type_ == Type::Internal) {\n>>> @@ -167,19 +169,26 @@ void CameraStream::process(const FrameBuffer &source,\n>>> \tif (!postProcessor_)\n>>> \t\treturn;\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.buffer, output.pixelFormat, output.size,\n>>> -\t\t\t PROT_READ | PROT_WRITE);\n>>> -\tif (!dest.isValid()) {\n>>> +\trequest->dest_ = std::make_unique(\n>>> +\t\t*camera3Dest.buffer, output.pixelFormat, output.size, PROT_READ | PROT_WRITE);\n>>> +\tif (!request->dest_->isValid()) {\n>>> \t\tLOG(HAL, Error) << \"Failed to create destination buffer\";\n>>> \t\treturn;\n>>> \t}\n>>> \n>>> -\tpostProcessor_->process(source, &dest, request);\n>>> +\trequest->src_ = &source;\n>>> +\n>>> +\t/* Push the postProcessor request to the worker queue. */\n>>> +\tworker_->queueRequest(request);\n>>> +}\n>>> +\n>>> +void CameraStream::flush()\n>>> +{\n>>> +\tif (!postProcessor_)\n>>> +\t\treturn;\n>>> +\n>>> +\tworker_->flush();\n>>> }\n>>> \n>>> FrameBuffer *CameraStream::getBuffer()\n>>> @@ -209,3 +218,86 @@ void CameraStream::putBuffer(FrameBuffer *buffer)\n>>> \n>>> \tbuffers_.push_back(buffer);\n>>> }\n>>> +\n>>> +CameraStream::PostProcessorWorker::PostProcessorWorker(PostProcessor *postProcessor)\n>>> +\t: postProcessor_(postProcessor)\n>>> +{\n>>> +}\n>>> +\n>>> +CameraStream::PostProcessorWorker::~PostProcessorWorker()\n>>> +{\n>>> +\t{\n>>> +\t\tlibcamera::MutexLocker lock(mutex_);\n>>> +\t\tstate_ = State::Stopped;\n>>> +\t}\n>>> +\n>>> +\tcv_.notify_one();\n>>> +\twait();\n>>> +}\n>>> +\n>>> +void CameraStream::PostProcessorWorker::start()\n>>> +{\n>>> +\t{\n>>> +\t\tlibcamera::MutexLocker lock(mutex_);\n>>> +\t\tstate_ = State::Running;\n>>> +\t}\n>>> +\n>>> +\tThread::start();\n>>> +}\n>>> +\n>>> +void CameraStream::PostProcessorWorker::queueRequest(Camera3RequestDescriptor *request)\n>>> +{\n>>> +\t{\n>>> +\t\tMutexLocker lock(mutex_);\n>>> +\t\tASSERT(state_ == State::Running);\n>>> +\t\trequests_.push(request);\n>>> +\t}\n>>> +\tcv_.notify_one();\n>>> +}\n>>> +\n>>> +void CameraStream::PostProcessorWorker::run()\n>>> +{\n>>> +\tMutexLocker locker(mutex_);\n>>> +\n>>> +\twhile (1) {\n>>> +\t\tcv_.wait(locker, [&] {\n>>> +\t\t\treturn state_ != State::Running || !requests_.empty();\n>>> +\t\t});\n>>> +\n>>> +\t\tif (state_ != State::Running)\n>>> +\t\t\tbreak;\n>>> +\n>>> +\t\tCamera3RequestDescriptor *descriptor = requests_.front();\n>>> +\t\trequests_.pop();\n>>> +\t\tlocker.unlock();\n>>> +\n>>> +\t\tpostProcessor_->process(*descriptor->src_, descriptor->dest_.get(),\n>>> +\t\t\t\t\tdescriptor);\n>>> +\n>>> +\t\tlocker.lock();\n>>> +\t}\n>>> +\n>>> +\tif (state_ == State::Flushing) {\n>>> +\t\twhile (!requests_.empty()) {\n>>> +\t\t\tpostProcessor_->processComplete.emit(requests_.front(),\n>>> +\t\t\t\t\t\t\t PostProcessor::Status::Error);\n>>> +\t\t\trequests_.pop();\n>>> +\t\t}\n>>> +\t\tstate_ = State::Stopped;\n>>> +\t\tlocker.unlock();\n>>> +\t\tcv_.notify_one();\n>>> +\t}\n>>> +}\n>>> +\n>>> +void CameraStream::PostProcessorWorker::flush()\n>>> +{\n>>> +\tlibcamera::MutexLocker lock(mutex_);\n>>> +\tstate_ = State::Flushing;\n>>> +\tlock.unlock();\n>>> +\tcv_.notify_one();\n>>> +\n>>> +\tlock.lock();\n>>> +\tcv_.wait(lock, [&] {\n>>> +\t\treturn state_ == State::Stopped;\n>>> +\t});\n>>> +}\n>>> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n>>> index a0c5f166..e410f35d 100644\n>>> --- a/src/android/camera_stream.h\n>>> +++ b/src/android/camera_stream.h\n>>> @@ -7,21 +7,26 @@\n>>> #ifndef __ANDROID_CAMERA_STREAM_H__\n>>> #define __ANDROID_CAMERA_STREAM_H__\n>>> \n>>> +#include \n>>> #include \n>>> #include \n>>> +#include \n>>> #include \n>>> \n>>> #include \n>>> \n>>> +#include \n>>> +\n>>> #include \n>>> #include \n>>> #include \n>>> #include \n>>> #include \n>>> \n>>> +#include \"post_processor.h\"\n>>> +\n>>> class CameraDevice;\n>>> class CameraMetadata;\n>>> -class PostProcessor;\n>>> \n>>> struct Camera3RequestDescriptor;\n>>> \n>>> @@ -125,8 +130,38 @@ public:\n>>> \t\t Camera3RequestDescriptor *request);\n>>> \tlibcamera::FrameBuffer *getBuffer();\n>>> \tvoid putBuffer(libcamera::FrameBuffer *buffer);\n>>> +\tvoid flush();\n>>> \n>>> private:\n>>> +\tclass PostProcessorWorker : public libcamera::Thread\n>>> +\t{\n>>> +\tpublic:\n>>> +\t\tenum class State {\n>>> +\t\t\tStopped,\n>>> +\t\t\tRunning,\n>>> +\t\t\tFlushing,\n>>> +\t\t};\n>>> +\n>>> +\t\tPostProcessorWorker(PostProcessor *postProcessor);\n>>> +\t\t~PostProcessorWorker();\n>>> +\n>>> +\t\tvoid start();\n>>> +\t\tvoid queueRequest(Camera3RequestDescriptor *request);\n>>> +\t\tvoid flush();\n>>> +\n>>> +\tprotected:\n>>> +\t\tvoid run() override;\n>>> +\n>>> +\tprivate:\n>>> +\t\tPostProcessor *postProcessor_;\n>>> +\n>>> +\t\tlibcamera::Mutex mutex_;\n>>> +\t\tstd::condition_variable cv_;\n>>> +\n>>> +\t\tstd::queue requests_;\n>>> +\t\tState state_;\n>>> +\t};\n>>> +\n>>> \tint waitFence(int fence);\n>>> \n>>> \tCameraDevice *const cameraDevice_;\n>>> @@ -143,6 +178,8 @@ private:\n>>> \t */\n>>> \tstd::unique_ptr mutex_;\n>>> \tstd::unique_ptr postProcessor_;\n>>> +\n>>> +\tstd::unique_ptr worker_;\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 72026BDC71\n\tfor ;\n\tWed, 13 Oct 2021 09:51:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 038B268F4D;\n\tWed, 13 Oct 2021 11:51: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 9F5ED68546\n\tfor ;\n\tWed, 13 Oct 2021 11:51:33 +0200 (CEST)","from [192.168.1.106] (unknown [103.251.226.115])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B0260291;\n\tWed, 13 Oct 2021 11:51:32 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"c5YgSQlk\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634118693;\n\tbh=yBh2p9UNo0ChbNha2sigLC6+huK6IcQzppE6e7GQIGQ=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=c5YgSQlk7FpavMJDPtOlH8CZ9Lwg4+g/IvaXm7HFSMLPltWnQpeTY2KKR18/REfAy\n\tCo4F5T9VEelDna41dOnyAPobfQorK57W0uc+Keb17nI2o00/WFAf0R2vSttxynhJMW\n\t1jCyi6aZ1q2Quw4/BMdgUFfQ3IEZ4V4j0nD4RyOI=","To":"Laurent Pinchart ","References":"<20211011073505.243864-1-umang.jain@ideasonboard.com>\n\t<20211011073505.243864-6-umang.jain@ideasonboard.com>\n\t\n\t","From":"Umang Jain ","Message-ID":"","Date":"Wed, 13 Oct 2021 15:21: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 v4 5/7] android: post_processor: Make\n\tpost processing async","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":20176,"web_url":"https://patchwork.libcamera.org/comment/20176/","msgid":"","date":"2021-10-13T10:18:19","subject":"Re: [libcamera-devel] [PATCH v4 5/7] android: post_processor: Make\n\tpost processing async","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nOn Wed, Oct 13, 2021 at 03:14:21PM +0530, Umang Jain wrote:\n> On 10/13/21 5:27 AM, Laurent Pinchart wrote:\n> > Hi Umang,\n> >\n> > Thank you for the patch.\n> >\n> > s/async/asynchronous/ in the subject line.\n> >\n> > On Mon, Oct 11, 2021 at 01:05:03PM +0530, Umang Jain wrote:\n> >> Introduce a dedicated worker class derived from libcamera::Thread.\n> >> The worker class maintains a queue for post-processing requests\n> >> and waits for a post-processing request to become available.\n> >> It will process them as per FIFO before de-queuing it from the\n> >> queue.\n> >>\n> >> To get access to the source and destination buffers in the worker\n> >> thread, we also need to save a pointer to them in the\n> >> Camera3RequestDescriptor.\n> >>\n> >> This patch also implements a flush() for the PostProcessorWorker\n> >> class which is responsible to purge post-processing requests\n> >> queued up while a camera is stopping/flushing.\n> >>\n> >> The libcamera request completion handler CameraDevice::requestComplete()\n> >> assumes that the request that has just completed is at the front of the\n> >> queue. Now that the post-processor runs asynchronously, this isn't true\n> >> anymore, a request being post-processed will stay in the queue and a new\n> >> libcamera request may complete. Remove that assumption, and use the\n> >> request cookie to obtain the Camera3RequestDescriptor.\n> >>\n> >> Signed-off-by: Umang Jain \n> >> Signed-off-by: Laurent Pinchart \n> >> ---\n> >> src/android/camera_device.cpp | 25 +-------\n> >> src/android/camera_device.h | 3 +\n> >> src/android/camera_stream.cpp | 108 +++++++++++++++++++++++++++++++---\n> >> src/android/camera_stream.h | 39 +++++++++++-\n> >> 4 files changed, 144 insertions(+), 31 deletions(-)\n> >>\n> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> >> index eba370ea..61b902ad 100644\n> >> --- a/src/android/camera_device.cpp\n> >> +++ b/src/android/camera_device.cpp\n> >> @@ -239,6 +239,7 @@ Camera3RequestDescriptor::Camera3RequestDescriptor(\n> >> \t/* Clone the controls associated with the camera3 request. */\n> >> \tsettings_ = CameraMetadata(camera3Request->settings);\n> >> \n> >> +\tdest_.reset();\n> >\n> > dest_ is a std::unique_ptr<>, its constructor will do the right thing,\n> > you don't need to initialize it here.\n> >\n> >> \t/*\n> >> \t * Create the CaptureRequest, stored as a unique_ptr<> to tie its\n> >> \t * lifetime to the descriptor.\n> >> @@ -1094,28 +1095,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> >> \n> >> void CameraDevice::requestComplete(Request *request)\n> >> {\n> >> -\tCamera3RequestDescriptor *descriptor;\n> >> -\t{\n> >> -\t\tMutexLocker descriptorsLock(descriptorsMutex_);\n> >> -\t\tASSERT(!descriptors_.empty());\n> >> -\t\tdescriptor = descriptors_.front().get();\n> >> -\t}\n> >> -\n> >> -\tif (descriptor->request_->cookie() != request->cookie()) {\n> >> -\t\t/*\n> >> -\t\t * \\todo Clarify if the Camera has to be closed on\n> >> -\t\t * ERROR_DEVICE and possibly demote the Fatal to simple\n> >> -\t\t * Error.\n> >> -\t\t */\n> >> -\t\tnotifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);\n> >> -\t\tLOG(HAL, Fatal)\n> >> -\t\t\t<< \"Out-of-order completion for request \"\n> >> -\t\t\t<< utils::hex(request->cookie());\n> >> -\n> >> -\t\tMutexLocker descriptorsLock(descriptorsMutex_);\n> >> -\t\tdescriptors_.pop();\n> >> -\t\treturn;\n> >> -\t}\n> >> +\tCamera3RequestDescriptor *descriptor =\n> >> +\t\treinterpret_cast(request->cookie());\n> >> \n> >> \t/*\n> >> \t * Prepare the capture result for the Android camera stack.\n> >> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> >> index eee97516..725a0618 100644\n> >> --- a/src/android/camera_device.h\n> >> +++ b/src/android/camera_device.h\n> >> @@ -59,6 +59,9 @@ struct Camera3RequestDescriptor {\n> >> \tstd::unique_ptr resultMetadata_;\n> >> \tlibcamera::FrameBuffer *internalBuffer_;\n> >> \n> >> +\tstd::unique_ptr dest_;\n> >> +\tconst libcamera::FrameBuffer *src_;\n> > As mentioned in the review of the previous patch, you can have more than\n> > one post-processed stream per request, so this won't be enough.\n> \n> Do we actually know if we have such requests coming from, that require \n> multiple post-processed streams?\n> \n> Or is the answer, \"We might, from the CTS framework\" ?\n\nIt can happen, for instance with both one YUV downscaled stream produced\nin software and a JPEG stream in the same request.\n\n> > I'd recomment first refactoring the Camera3RequestDescriptor class and\n> > add an internal\n> >\n> > \tstruct Stream {\n> > \t\tcamera3_stream_buffer_t buffer;\n> > \t\tstd::unique_ptr frameBuffer;\n> > \t};\n> >\n> > with the buffers_ and frameBuffers members replaced with\n> >\n> > \tstd::vector streams_;\n> >\n> > Then you can extend the Stream structure in this patch to add the\n> > necessary fields.\n> \n> Makes sense.\n> \n> > Thinking some more about it, src_ is likely not needed, as it's a\n> > pointer to the FrameBuffer already stored in struct Stream. What you'll\n> > need will be the ability to find the Stream instance corresponding to a\n> > given libcamera stream, so maybe a map would be better than a vector.\n> \n> Yes, we should be able to associate it from the start otherwise we will \n> need to introduce needless iterations on finding it just queuing it to \n> post-processor.\n> \n> > It also seems like the PostProcessorWorker should move from processing\n> > requests to processing streams, as there's one PostProcessorWorker for\n> > each CameraStream. Maybe the struct Stream should contain a pointer to\n> > its Camera3RequestDescriptor, that way you could pass the\n> > Camera3RequestDescriptor::Stream pointer to CameraStream::process() and\n> > to the post-processors, and then find the corresponding\n> > Camera3RequestDescriptor in the completion handler.\n> \n> One potential issue here post-processing streams might require to know \n> the statuses of other (post-processing) streams too, belonging to the \n> same request? Because we need to complete the request only after all \n> streams have finished post-processing,\n\nYou need to keep track of the pending post-processing calls in the\ndescriptor, with a list of streams being post-processed, or possibly\njust a counter. Make sure to pay attention to race conditions and\nlocking.\n\n> Currently if we only maintain one queue, that can end up with \n> post-processing requests of streams from multiple successive requests. \n> Need to think a bit but surely, we might need to containerize this \n> further I think (container for streams of one request + container for \n> post-processing requests). We have the latter, but I think we also need \n> to have a former but avaibility to check container.empty(). Error \n> handling paths also might can get tricky in such a scenario. Let's see.\n> \n> >> +\n> >> \tcamera3_capture_result_t captureResult_ = {};\n> >> \tStatus status_ = Status::Pending;\n> >> };\n> >> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> >> index cec07269..818ef948 100644\n> >> --- a/src/android/camera_stream.cpp\n> >> +++ b/src/android/camera_stream.cpp\n> >> @@ -94,10 +94,12 @@ int CameraStream::configure()\n> >> \t\tif (ret)\n> >> \t\t\treturn ret;\n> >> \n> >> +\t\tworker_ = std::make_unique(postProcessor_.get());\n> >> \t\tpostProcessor_->processComplete.connect(\n> >> \t\t\tthis, [&](Camera3RequestDescriptor *request, PostProcessor::Status status) {\n> >> \t\t\t\tcameraDevice_->streamProcessingComplete(this, request, status);\n> >> \t\t\t});\n> >> +\t\tworker_->start();\n> >> \t}\n> >> \n> >> \tif (type_ == Type::Internal) {\n> >> @@ -167,19 +169,26 @@ void CameraStream::process(const FrameBuffer &source,\n> >> \tif (!postProcessor_)\n> >> \t\treturn;\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.buffer, output.pixelFormat, output.size,\n> >> -\t\t\t PROT_READ | PROT_WRITE);\n> >> -\tif (!dest.isValid()) {\n> >> +\trequest->dest_ = std::make_unique(\n> >> +\t\t*camera3Dest.buffer, output.pixelFormat, output.size, PROT_READ | PROT_WRITE);\n> >> +\tif (!request->dest_->isValid()) {\n> >> \t\tLOG(HAL, Error) << \"Failed to create destination buffer\";\n> >> \t\treturn;\n> >> \t}\n> >> \n> >> -\tpostProcessor_->process(source, &dest, request);\n> >> +\trequest->src_ = &source;\n> >> +\n> >> +\t/* Push the postProcessor request to the worker queue. */\n> >> +\tworker_->queueRequest(request);\n> >> +}\n> >> +\n> >> +void CameraStream::flush()\n> >> +{\n> >> +\tif (!postProcessor_)\n> >> +\t\treturn;\n> >> +\n> >> +\tworker_->flush();\n> >> }\n> >> \n> >> FrameBuffer *CameraStream::getBuffer()\n> >> @@ -209,3 +218,86 @@ void CameraStream::putBuffer(FrameBuffer *buffer)\n> >> \n> >> \tbuffers_.push_back(buffer);\n> >> }\n> >> +\n> >> +CameraStream::PostProcessorWorker::PostProcessorWorker(PostProcessor *postProcessor)\n> >> +\t: postProcessor_(postProcessor)\n> >> +{\n> >> +}\n> >> +\n> >> +CameraStream::PostProcessorWorker::~PostProcessorWorker()\n> >> +{\n> >> +\t{\n> >> +\t\tlibcamera::MutexLocker lock(mutex_);\n> >> +\t\tstate_ = State::Stopped;\n> >> +\t}\n> >> +\n> >> +\tcv_.notify_one();\n> >> +\twait();\n> >> +}\n> >> +\n> >> +void CameraStream::PostProcessorWorker::start()\n> >> +{\n> >> +\t{\n> >> +\t\tlibcamera::MutexLocker lock(mutex_);\n> >> +\t\tstate_ = State::Running;\n> >> +\t}\n> >> +\n> >> +\tThread::start();\n> >> +}\n> >> +\n> >> +void CameraStream::PostProcessorWorker::queueRequest(Camera3RequestDescriptor *request)\n> >> +{\n> >> +\t{\n> >> +\t\tMutexLocker lock(mutex_);\n> >> +\t\tASSERT(state_ == State::Running);\n> >> +\t\trequests_.push(request);\n> >> +\t}\n> >> +\tcv_.notify_one();\n> >> +}\n> >> +\n> >> +void CameraStream::PostProcessorWorker::run()\n> >> +{\n> >> +\tMutexLocker locker(mutex_);\n> >> +\n> >> +\twhile (1) {\n> >> +\t\tcv_.wait(locker, [&] {\n> >> +\t\t\treturn state_ != State::Running || !requests_.empty();\n> >> +\t\t});\n> >> +\n> >> +\t\tif (state_ != State::Running)\n> >> +\t\t\tbreak;\n> >> +\n> >> +\t\tCamera3RequestDescriptor *descriptor = requests_.front();\n> >> +\t\trequests_.pop();\n> >> +\t\tlocker.unlock();\n> >> +\n> >> +\t\tpostProcessor_->process(*descriptor->src_, descriptor->dest_.get(),\n> >> +\t\t\t\t\tdescriptor);\n> >> +\n> >> +\t\tlocker.lock();\n> >> +\t}\n> >> +\n> >> +\tif (state_ == State::Flushing) {\n> >> +\t\twhile (!requests_.empty()) {\n> >> +\t\t\tpostProcessor_->processComplete.emit(requests_.front(),\n> >> +\t\t\t\t\t\t\t PostProcessor::Status::Error);\n> >> +\t\t\trequests_.pop();\n> >> +\t\t}\n> >> +\t\tstate_ = State::Stopped;\n> >> +\t\tlocker.unlock();\n> >> +\t\tcv_.notify_one();\n> >> +\t}\n> >> +}\n> >> +\n> >> +void CameraStream::PostProcessorWorker::flush()\n> >> +{\n> >> +\tlibcamera::MutexLocker lock(mutex_);\n> >> +\tstate_ = State::Flushing;\n> >> +\tlock.unlock();\n> >> +\tcv_.notify_one();\n> >> +\n> >> +\tlock.lock();\n> >> +\tcv_.wait(lock, [&] {\n> >> +\t\treturn state_ == State::Stopped;\n> >> +\t});\n> >> +}\n> >> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n> >> index a0c5f166..e410f35d 100644\n> >> --- a/src/android/camera_stream.h\n> >> +++ b/src/android/camera_stream.h\n> >> @@ -7,21 +7,26 @@\n> >> #ifndef __ANDROID_CAMERA_STREAM_H__\n> >> #define __ANDROID_CAMERA_STREAM_H__\n> >> \n> >> +#include \n> >> #include \n> >> #include \n> >> +#include \n> >> #include \n> >> \n> >> #include \n> >> \n> >> +#include \n> >> +\n> >> #include \n> >> #include \n> >> #include \n> >> #include \n> >> #include \n> >> \n> >> +#include \"post_processor.h\"\n> >> +\n> >> class CameraDevice;\n> >> class CameraMetadata;\n> >> -class PostProcessor;\n> >> \n> >> struct Camera3RequestDescriptor;\n> >> \n> >> @@ -125,8 +130,38 @@ public:\n> >> \t\t Camera3RequestDescriptor *request);\n> >> \tlibcamera::FrameBuffer *getBuffer();\n> >> \tvoid putBuffer(libcamera::FrameBuffer *buffer);\n> >> +\tvoid flush();\n> >> \n> >> private:\n> >> +\tclass PostProcessorWorker : public libcamera::Thread\n> >> +\t{\n> >> +\tpublic:\n> >> +\t\tenum class State {\n> >> +\t\t\tStopped,\n> >> +\t\t\tRunning,\n> >> +\t\t\tFlushing,\n> >> +\t\t};\n> >> +\n> >> +\t\tPostProcessorWorker(PostProcessor *postProcessor);\n> >> +\t\t~PostProcessorWorker();\n> >> +\n> >> +\t\tvoid start();\n> >> +\t\tvoid queueRequest(Camera3RequestDescriptor *request);\n> >> +\t\tvoid flush();\n> >> +\n> >> +\tprotected:\n> >> +\t\tvoid run() override;\n> >> +\n> >> +\tprivate:\n> >> +\t\tPostProcessor *postProcessor_;\n> >> +\n> >> +\t\tlibcamera::Mutex mutex_;\n> >> +\t\tstd::condition_variable cv_;\n> >> +\n> >> +\t\tstd::queue requests_;\n> >> +\t\tState state_;\n> >> +\t};\n> >> +\n> >> \tint waitFence(int fence);\n> >> \n> >> \tCameraDevice *const cameraDevice_;\n> >> @@ -143,6 +178,8 @@ private:\n> >> \t */\n> >> \tstd::unique_ptr mutex_;\n> >> \tstd::unique_ptr postProcessor_;\n> >> +\n> >> +\tstd::unique_ptr worker_;\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 16B39BDC71\n\tfor ;\n\tWed, 13 Oct 2021 10:18:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D886668F4F;\n\tWed, 13 Oct 2021 12:18: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 E7C6F60501\n\tfor ;\n\tWed, 13 Oct 2021 12:18:33 +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 708C518FF;\n\tWed, 13 Oct 2021 12:18:33 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"DIGXi62t\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634120313;\n\tbh=srXYUYgwuw/Uw2D9nga+xkjdqdU88rxOw6CUA2ED9zc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=DIGXi62tt2hbz0QcojhZSCo7AUg29VrwBEI18g/DgfyaIijya8Ve5Ww/CKXL5hGlH\n\tcp7J4qbIuwe8J9P31EmIIPztJBfUDMtYwpabQjthPj4MmsFFtQfAW7DvtOJkALdsZC\n\teZFtN8LLEfsWyjm7opmM/5nu6n8tQn15aL1y7cu8=","Date":"Wed, 13 Oct 2021 13:18:19 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20211011073505.243864-1-umang.jain@ideasonboard.com>\n\t<20211011073505.243864-6-umang.jain@ideasonboard.com>\n\t\n\t<6744f251-bec3-19e4-545a-8a148940d872@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<6744f251-bec3-19e4-545a-8a148940d872@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 5/7] android: post_processor: Make\n\tpost processing async","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":20178,"web_url":"https://patchwork.libcamera.org/comment/20178/","msgid":"","date":"2021-10-13T10:21:36","subject":"Re: [libcamera-devel] [PATCH v4 5/7] android: post_processor: Make\n\tpost processing async","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nOn Wed, Oct 13, 2021 at 03:21:28PM +0530, Umang Jain wrote:\n> On 10/13/21 5:32 AM, Laurent Pinchart wrote:\n> > On Wed, Oct 13, 2021 at 02:57:16AM +0300, Laurent Pinchart wrote:\n> >> Hi Umang,\n> >>\n> >> Thank you for the patch.\n> >>\n> >> s/async/asynchronous/ in the subject line.\n> >>\n> >> On Mon, Oct 11, 2021 at 01:05:03PM +0530, Umang Jain wrote:\n> >>> Introduce a dedicated worker class derived from libcamera::Thread.\n> >>> The worker class maintains a queue for post-processing requests\n> >>> and waits for a post-processing request to become available.\n> >>> It will process them as per FIFO before de-queuing it from the\n> >>> queue.\n> >>>\n> >>> To get access to the source and destination buffers in the worker\n> >>> thread, we also need to save a pointer to them in the\n> >>> Camera3RequestDescriptor.\n> >>>\n> >>> This patch also implements a flush() for the PostProcessorWorker\n> >>> class which is responsible to purge post-processing requests\n> >>> queued up while a camera is stopping/flushing.\n> >>>\n> >>> The libcamera request completion handler CameraDevice::requestComplete()\n> >>> assumes that the request that has just completed is at the front of the\n> >>> queue. Now that the post-processor runs asynchronously, this isn't true\n> >>> anymore, a request being post-processed will stay in the queue and a new\n> >>> libcamera request may complete. Remove that assumption, and use the\n> >>> request cookie to obtain the Camera3RequestDescriptor.\n> >>>\n> >>> Signed-off-by: Umang Jain \n> >>> Signed-off-by: Laurent Pinchart \n> >>> ---\n> >>> src/android/camera_device.cpp | 25 +-------\n> >>> src/android/camera_device.h | 3 +\n> >>> src/android/camera_stream.cpp | 108 +++++++++++++++++++++++++++++++---\n> >>> src/android/camera_stream.h | 39 +++++++++++-\n> >>> 4 files changed, 144 insertions(+), 31 deletions(-)\n> >>>\n> >>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> >>> index eba370ea..61b902ad 100644\n> >>> --- a/src/android/camera_device.cpp\n> >>> +++ b/src/android/camera_device.cpp\n> >>> @@ -239,6 +239,7 @@ Camera3RequestDescriptor::Camera3RequestDescriptor(\n> >>> \t/* Clone the controls associated with the camera3 request. */\n> >>> \tsettings_ = CameraMetadata(camera3Request->settings);\n> >>> \n> >>> +\tdest_.reset();\n> >> dest_ is a std::unique_ptr<>, its constructor will do the right thing,\n> >> you don't need to initialize it here.\n> >>\n> >>> \t/*\n> >>> \t * Create the CaptureRequest, stored as a unique_ptr<> to tie its\n> >>> \t * lifetime to the descriptor.\n> >>> @@ -1094,28 +1095,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> >>> \n> >>> void CameraDevice::requestComplete(Request *request)\n> >>> {\n> >>> -\tCamera3RequestDescriptor *descriptor;\n> >>> -\t{\n> >>> -\t\tMutexLocker descriptorsLock(descriptorsMutex_);\n> >>> -\t\tASSERT(!descriptors_.empty());\n> >>> -\t\tdescriptor = descriptors_.front().get();\n> >>> -\t}\n> >>> -\n> >>> -\tif (descriptor->request_->cookie() != request->cookie()) {\n> >>> -\t\t/*\n> >>> -\t\t * \\todo Clarify if the Camera has to be closed on\n> >>> -\t\t * ERROR_DEVICE and possibly demote the Fatal to simple\n> >>> -\t\t * Error.\n> >>> -\t\t */\n> >>> -\t\tnotifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);\n> >>> -\t\tLOG(HAL, Fatal)\n> >>> -\t\t\t<< \"Out-of-order completion for request \"\n> >>> -\t\t\t<< utils::hex(request->cookie());\n> >>> -\n> >>> -\t\tMutexLocker descriptorsLock(descriptorsMutex_);\n> >>> -\t\tdescriptors_.pop();\n> >>> -\t\treturn;\n> >>> -\t}\n> >>> +\tCamera3RequestDescriptor *descriptor =\n> >>> +\t\treinterpret_cast(request->cookie());\n> >>> \n> >>> \t/*\n> >>> \t * Prepare the capture result for the Android camera stack.\n> >>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> >>> index eee97516..725a0618 100644\n> >>> --- a/src/android/camera_device.h\n> >>> +++ b/src/android/camera_device.h\n> >>> @@ -59,6 +59,9 @@ struct Camera3RequestDescriptor {\n> >>> \tstd::unique_ptr resultMetadata_;\n> >>> \tlibcamera::FrameBuffer *internalBuffer_;\n> >>> \n> >>> +\tstd::unique_ptr dest_;\n> >>> +\tconst libcamera::FrameBuffer *src_;\n> >> As mentioned in the review of the previous patch, you can have more than\n> >> one post-processed stream per request, so this won't be enough.\n> >>\n> >> I'd recomment first refactoring the Camera3RequestDescriptor class and\n> >> add an internal\n> >>\n> >> \tstruct Stream {\n> >> \t\tcamera3_stream_buffer_t buffer;\n> >> \t\tstd::unique_ptr frameBuffer;\n> >> \t};\n> >>\n> >> with the buffers_ and frameBuffers members replaced with\n> >>\n> >> \tstd::vector streams_;\n> >>\n> >> Then you can extend the Stream structure in this patch to add the\n> >> necessary fields.\n> >>\n> >> Thinking some more about it, src_ is likely not needed, as it's a\n> >> pointer to the FrameBuffer already stored in struct Stream. What you'll\n> >> need will be the ability to find the Stream instance corresponding to a\n> >> given libcamera stream, so maybe a map would be better than a vector.\n> >>\n> >> It also seems like the PostProcessorWorker should move from processing\n> >> requests to processing streams, as there's one PostProcessorWorker for\n> >> each CameraStream. Maybe the struct Stream should contain a pointer to\n> >> its Camera3RequestDescriptor, that way you could pass the\n> >> Camera3RequestDescriptor::Stream pointer to CameraStream::process() and\n> >> to the post-processors, and then find the corresponding\n> >> Camera3RequestDescriptor in the completion handler.\n> >\n> > By the way, another option may be to move the PostProcessorWorker to\n> > CameraDevice and still give it a Camera3RequestDescriptor, and\n> > internally in the thread run the post-processors sequentially for each\n> > post-processed stream. If we had a lot of post-processed streams I would\n> > say that would be a better design, as we could then create a threads\n> > pool (with N threads for M streams, and N < M) and dispatch the jobs to\n> > those threads, but that's overkill I think. Still, maybe a single thread\n> > design would be easier and look cleaner, I'm not sure.\n> \n> PostProcessorWorker can be a self sustaining but looking at the things \n> right now, I would leave it in camera-stream itself.\n> \n> If we end up with thread pools in the future, I will happy to rework it, \n> is that okay?\n\nI don't think we'll end up with thread pools, it will likely be one\nthread per stream, or a single thread. Either way works for me, with\nbonus points if the implementation can compartiment the bits and pieces\nnicely to make a rework not too difficult later (and to keep the code\nand data structures readable in any case).\n\n> >>> +\n> >>> \tcamera3_capture_result_t captureResult_ = {};\n> >>> \tStatus status_ = Status::Pending;\n> >>> };\n> >>> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> >>> index cec07269..818ef948 100644\n> >>> --- a/src/android/camera_stream.cpp\n> >>> +++ b/src/android/camera_stream.cpp\n> >>> @@ -94,10 +94,12 @@ int CameraStream::configure()\n> >>> \t\tif (ret)\n> >>> \t\t\treturn ret;\n> >>> \n> >>> +\t\tworker_ = std::make_unique(postProcessor_.get());\n> >>> \t\tpostProcessor_->processComplete.connect(\n> >>> \t\t\tthis, [&](Camera3RequestDescriptor *request, PostProcessor::Status status) {\n> >>> \t\t\t\tcameraDevice_->streamProcessingComplete(this, request, status);\n> >>> \t\t\t});\n> >>> +\t\tworker_->start();\n> >>> \t}\n> >>> \n> >>> \tif (type_ == Type::Internal) {\n> >>> @@ -167,19 +169,26 @@ void CameraStream::process(const FrameBuffer &source,\n> >>> \tif (!postProcessor_)\n> >>> \t\treturn;\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.buffer, output.pixelFormat, output.size,\n> >>> -\t\t\t PROT_READ | PROT_WRITE);\n> >>> -\tif (!dest.isValid()) {\n> >>> +\trequest->dest_ = std::make_unique(\n> >>> +\t\t*camera3Dest.buffer, output.pixelFormat, output.size, PROT_READ | PROT_WRITE);\n> >>> +\tif (!request->dest_->isValid()) {\n> >>> \t\tLOG(HAL, Error) << \"Failed to create destination buffer\";\n> >>> \t\treturn;\n> >>> \t}\n> >>> \n> >>> -\tpostProcessor_->process(source, &dest, request);\n> >>> +\trequest->src_ = &source;\n> >>> +\n> >>> +\t/* Push the postProcessor request to the worker queue. */\n> >>> +\tworker_->queueRequest(request);\n> >>> +}\n> >>> +\n> >>> +void CameraStream::flush()\n> >>> +{\n> >>> +\tif (!postProcessor_)\n> >>> +\t\treturn;\n> >>> +\n> >>> +\tworker_->flush();\n> >>> }\n> >>> \n> >>> FrameBuffer *CameraStream::getBuffer()\n> >>> @@ -209,3 +218,86 @@ void CameraStream::putBuffer(FrameBuffer *buffer)\n> >>> \n> >>> \tbuffers_.push_back(buffer);\n> >>> }\n> >>> +\n> >>> +CameraStream::PostProcessorWorker::PostProcessorWorker(PostProcessor *postProcessor)\n> >>> +\t: postProcessor_(postProcessor)\n> >>> +{\n> >>> +}\n> >>> +\n> >>> +CameraStream::PostProcessorWorker::~PostProcessorWorker()\n> >>> +{\n> >>> +\t{\n> >>> +\t\tlibcamera::MutexLocker lock(mutex_);\n> >>> +\t\tstate_ = State::Stopped;\n> >>> +\t}\n> >>> +\n> >>> +\tcv_.notify_one();\n> >>> +\twait();\n> >>> +}\n> >>> +\n> >>> +void CameraStream::PostProcessorWorker::start()\n> >>> +{\n> >>> +\t{\n> >>> +\t\tlibcamera::MutexLocker lock(mutex_);\n> >>> +\t\tstate_ = State::Running;\n> >>> +\t}\n> >>> +\n> >>> +\tThread::start();\n> >>> +}\n> >>> +\n> >>> +void CameraStream::PostProcessorWorker::queueRequest(Camera3RequestDescriptor *request)\n> >>> +{\n> >>> +\t{\n> >>> +\t\tMutexLocker lock(mutex_);\n> >>> +\t\tASSERT(state_ == State::Running);\n> >>> +\t\trequests_.push(request);\n> >>> +\t}\n> >>> +\tcv_.notify_one();\n> >>> +}\n> >>> +\n> >>> +void CameraStream::PostProcessorWorker::run()\n> >>> +{\n> >>> +\tMutexLocker locker(mutex_);\n> >>> +\n> >>> +\twhile (1) {\n> >>> +\t\tcv_.wait(locker, [&] {\n> >>> +\t\t\treturn state_ != State::Running || !requests_.empty();\n> >>> +\t\t});\n> >>> +\n> >>> +\t\tif (state_ != State::Running)\n> >>> +\t\t\tbreak;\n> >>> +\n> >>> +\t\tCamera3RequestDescriptor *descriptor = requests_.front();\n> >>> +\t\trequests_.pop();\n> >>> +\t\tlocker.unlock();\n> >>> +\n> >>> +\t\tpostProcessor_->process(*descriptor->src_, descriptor->dest_.get(),\n> >>> +\t\t\t\t\tdescriptor);\n> >>> +\n> >>> +\t\tlocker.lock();\n> >>> +\t}\n> >>> +\n> >>> +\tif (state_ == State::Flushing) {\n> >>> +\t\twhile (!requests_.empty()) {\n> >>> +\t\t\tpostProcessor_->processComplete.emit(requests_.front(),\n> >>> +\t\t\t\t\t\t\t PostProcessor::Status::Error);\n> >>> +\t\t\trequests_.pop();\n> >>> +\t\t}\n> >>> +\t\tstate_ = State::Stopped;\n> >>> +\t\tlocker.unlock();\n> >>> +\t\tcv_.notify_one();\n> >>> +\t}\n> >>> +}\n> >>> +\n> >>> +void CameraStream::PostProcessorWorker::flush()\n> >>> +{\n> >>> +\tlibcamera::MutexLocker lock(mutex_);\n> >>> +\tstate_ = State::Flushing;\n> >>> +\tlock.unlock();\n> >>> +\tcv_.notify_one();\n> >>> +\n> >>> +\tlock.lock();\n> >>> +\tcv_.wait(lock, [&] {\n> >>> +\t\treturn state_ == State::Stopped;\n> >>> +\t});\n> >>> +}\n> >>> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n> >>> index a0c5f166..e410f35d 100644\n> >>> --- a/src/android/camera_stream.h\n> >>> +++ b/src/android/camera_stream.h\n> >>> @@ -7,21 +7,26 @@\n> >>> #ifndef __ANDROID_CAMERA_STREAM_H__\n> >>> #define __ANDROID_CAMERA_STREAM_H__\n> >>> \n> >>> +#include \n> >>> #include \n> >>> #include \n> >>> +#include \n> >>> #include \n> >>> \n> >>> #include \n> >>> \n> >>> +#include \n> >>> +\n> >>> #include \n> >>> #include \n> >>> #include \n> >>> #include \n> >>> #include \n> >>> \n> >>> +#include \"post_processor.h\"\n> >>> +\n> >>> class CameraDevice;\n> >>> class CameraMetadata;\n> >>> -class PostProcessor;\n> >>> \n> >>> struct Camera3RequestDescriptor;\n> >>> \n> >>> @@ -125,8 +130,38 @@ public:\n> >>> \t\t Camera3RequestDescriptor *request);\n> >>> \tlibcamera::FrameBuffer *getBuffer();\n> >>> \tvoid putBuffer(libcamera::FrameBuffer *buffer);\n> >>> +\tvoid flush();\n> >>> \n> >>> private:\n> >>> +\tclass PostProcessorWorker : public libcamera::Thread\n> >>> +\t{\n> >>> +\tpublic:\n> >>> +\t\tenum class State {\n> >>> +\t\t\tStopped,\n> >>> +\t\t\tRunning,\n> >>> +\t\t\tFlushing,\n> >>> +\t\t};\n> >>> +\n> >>> +\t\tPostProcessorWorker(PostProcessor *postProcessor);\n> >>> +\t\t~PostProcessorWorker();\n> >>> +\n> >>> +\t\tvoid start();\n> >>> +\t\tvoid queueRequest(Camera3RequestDescriptor *request);\n> >>> +\t\tvoid flush();\n> >>> +\n> >>> +\tprotected:\n> >>> +\t\tvoid run() override;\n> >>> +\n> >>> +\tprivate:\n> >>> +\t\tPostProcessor *postProcessor_;\n> >>> +\n> >>> +\t\tlibcamera::Mutex mutex_;\n> >>> +\t\tstd::condition_variable cv_;\n> >>> +\n> >>> +\t\tstd::queue requests_;\n> >>> +\t\tState state_;\n> >>> +\t};\n> >>> +\n> >>> \tint waitFence(int fence);\n> >>> \n> >>> \tCameraDevice *const cameraDevice_;\n> >>> @@ -143,6 +178,8 @@ private:\n> >>> \t */\n> >>> \tstd::unique_ptr mutex_;\n> >>> \tstd::unique_ptr postProcessor_;\n> >>> +\n> >>> +\tstd::unique_ptr worker_;\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 CCE52C323E\n\tfor ;\n\tWed, 13 Oct 2021 10:21:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A0A4468F51;\n\tWed, 13 Oct 2021 12:21:52 +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 7DB0D60501\n\tfor ;\n\tWed, 13 Oct 2021 12:21:51 +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 0709C1AD5;\n\tWed, 13 Oct 2021 12:21:50 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"VLwqOy+u\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634120511;\n\tbh=KaDeZlCayFk/v2/x/BHCCEf5gM4/QzZFuKGXEYRVE3k=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=VLwqOy+uh1hULY4j2WXUR499sxGiRdDeC7Nx+a/m3gJE67TJzKp0vClZUtzKanT+c\n\tPR9lNfBOYzjt6w56fQIeklgB4mr9iYNCFmm6khYPyf0ihti7GJ+rEmWvhX2SOkbnNQ\n\t7Dtbp2W9k7YE2Fauhco2xbTUS8SCaeQ8JTwfSQjY=","Date":"Wed, 13 Oct 2021 13:21:36 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20211011073505.243864-1-umang.jain@ideasonboard.com>\n\t<20211011073505.243864-6-umang.jain@ideasonboard.com>\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 v4 5/7] android: post_processor: Make\n\tpost processing async","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\" "}}]