[{"id":20489,"web_url":"https://patchwork.libcamera.org/comment/20489/","msgid":"","date":"2021-10-25T22:05:13","subject":"Re: [libcamera-devel] [PATCH v7 7/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\nOn Tue, Oct 26, 2021 at 02:08:33AM +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> The entire post-processing handling iteration is locked under\n> streamProcessMutex_ which helps us to queue all the post-processing\n\ns/streamProcessMutex_/streamsProcessMutex_/\n\n> request at once, before any of the post-processing completion slot\n> (streamProcessingComplete()) is allowed to run for post-processing\n> requests completing in parallel. This helps us to manage both\n> synchronous and asynchronous errors encountered during the entire\n> post processing operation. Since a post-processing operation can\n> even complete after CameraDevice::requestComplete() has returned,\n> we need to check and complete the descriptor from\n> streamProcessingComplete() running in the PostProcessorWorker's\n> thread.\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. It is hooked with\n> CameraStream::flush(), which isn't used currently but will be\n> used when we handle flush/stop scenarios in greater detail\n> subsequently (in a different patchset).\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 | 44 ++++++---------\n> src/android/camera_stream.cpp | 101 ++++++++++++++++++++++++++++++++--\n> src/android/camera_stream.h | 37 +++++++++++++\n> 3 files changed, 151 insertions(+), 31 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 3ded0f7e..53ebe0ea 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -1027,29 +1027,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.\n> -\t\t */\n> -\t\tLOG(HAL, Error)\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> -\n> -\t\tnotifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);\n> -\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> @@ -1124,9 +1103,13 @@ void CameraDevice::requestComplete(Request *request)\n> \t}\n> \n> \t/* Handle post-processing. */\n> +\tMutexLocker locker(descriptor->streamsProcessMutex_);\n> +\n> \t/*\n> -\t * \\todo Protect the loop below with streamProcessMutex_ when post\n\nIn the patch that introduced this,\n\ns/streamProcessMutex_/streamsProcessMutex_/\n\n> -\t * processor runs asynchronously.\n> +\t * Queue all the post-processing streams request at once. The completion\n> +\t * slot streamProcessingComplete() can only execute when we are out\n> +\t * this critical section. This helps to handle synchronous errors here\n> +\t * itself.\n> \t */\n> \tauto iter = descriptor->pendingStreamsToProcess_.begin();\n> \twhile (iter != descriptor->pendingStreamsToProcess_.end()) {\n> @@ -1158,8 +1141,10 @@ void CameraDevice::requestComplete(Request *request)\n> \t\t}\n> \t}\n> \n> -\tif (descriptor->pendingStreamsToProcess_.empty())\n> +\tif (descriptor->pendingStreamsToProcess_.empty()) {\n> +\t\tlocker.unlock();\n> \t\tcompleteDescriptor(descriptor);\n> +\t}\n> }\n> \n> void CameraDevice::completeDescriptor(Camera3RequestDescriptor *descriptor)\n> @@ -1245,6 +1230,13 @@ void CameraDevice::streamProcessingComplete(Camera3RequestDescriptor::StreamBuff\n> \tMutexLocker locker(request->streamsProcessMutex_);\n> \n> \trequest->pendingStreamsToProcess_.erase(streamBuffer->stream);\n> +\n> +\tif (!request->pendingStreamsToProcess_.empty())\n> +\t\treturn;\n> +\n> +\tlocker.unlock();\n\nMaybe\n\n\t{\n\t\tMutexLocker locker(request->streamsProcessMutex_);\n\n\t\trequest->pendingStreamsToProcess_.erase(streamBuffer->stream);\n\t\tif (!request->pendingStreamsToProcess_.empty())\n\t\t\treturn;\n\t}\n\nup to you.\n\n> +\n> +\tcompleteDescriptor(streamBuffer->request);\n> }\n> \n> std::string CameraDevice::logPrefix() const\n> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> index fed99022..9023c13c 100644\n> --- a/src/android/camera_stream.cpp\n> +++ b/src/android/camera_stream.cpp\n> @@ -99,6 +99,7 @@ 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::StreamBuffer *streamBuffer,\n> \t\t\t\t PostProcessor::Status status) {\n> @@ -112,6 +113,8 @@ int CameraStream::configure()\n> \t\t\t\tcameraDevice_->streamProcessingComplete(streamBuffer,\n> \t\t\t\t\t\t\t\t\tbufferStatus);\n> \t\t\t});\n> +\n> +\t\tworker_->start();\n> \t}\n> \n> \tif (type_ == Type::Internal) {\n> @@ -178,10 +181,6 @@ int CameraStream::process(Camera3RequestDescriptor::StreamBuffer *streamBuffer)\n> \t\tstreamBuffer->fence = -1;\n> \t}\n> \n> -\t/*\n> -\t * \\todo Buffer mapping and processing should be moved to a\n> -\t * separate thread.\n> -\t */\n> \tconst StreamConfiguration &output = configuration();\n> \tstreamBuffer->dstBuffer = std::make_unique(\n> \t\t*streamBuffer->camera3Buffer, output.pixelFormat, output.size,\n> @@ -191,11 +190,19 @@ int CameraStream::process(Camera3RequestDescriptor::StreamBuffer *streamBuffer)\n> \t\treturn -EINVAL;\n> \t}\n> \n> -\tpostProcessor_->process(streamBuffer);\n> +\tworker_->queueRequest(streamBuffer);\n> \n> \treturn 0;\n> }\n> \n> +void CameraStream::flush()\n> +{\n> +\tif (!postProcessor_)\n> +\t\treturn;\n> +\n> +\tworker_->flush();\n> +}\n> +\n> FrameBuffer *CameraStream::getBuffer()\n> {\n> \tif (!allocator_)\n> @@ -223,3 +230,87 @@ 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\tASSERT(state_ != State::Running);\n> +\t\tstate_ = State::Running;\n> +\t}\n> +\n> +\tThread::start();\n> +}\n> +\n> +void CameraStream::PostProcessorWorker::queueRequest(Camera3RequestDescriptor::StreamBuffer *dest)\n> +{\n> +\t{\n> +\t\tMutexLocker lock(mutex_);\n> +\t\tASSERT(state_ == State::Running);\n> +\t\trequests_.push(dest);\n> +\t}\n> +\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::StreamBuffer *streamBuffer = requests_.front();\n> +\t\trequests_.pop();\n> +\t\tlocker.unlock();\n> +\n> +\t\tpostProcessor_->process(streamBuffer);\n> +\n> +\t\tlocker.lock();\n> +\t}\n> +\n> +\tif (state_ == State::Flushing) {\n> +\t\tstd::queue requests =\n> +\t\t\tstd::move(requests_);\n> +\t\tlocker.unlock();\n> +\n> +\t\twhile (!requests.empty()) {\n> +\t\t\tpostProcessor_->processComplete.emit(\n> +\t\t\t\trequests.front(), PostProcessor::Status::Error);\n> +\t\t\trequests.pop();\n> +\t\t}\n> +\n> +\t\tlocker.lock();\n> +\t\tstate_ = State::Stopped;\n> +\t}\n> +}\n> +\n> +void CameraStream::PostProcessorWorker::flush()\n> +{\n> +\tlibcamera::MutexLocker lock(mutex_);\n> +\tstate_ = State::Flushing;\n> +\tlock.unlock();\n> +\n> +\tcv_.notify_one();\n> +}\n> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n> index e74a9a3b..1588938a 100644\n> --- a/src/android/camera_stream.h\n> +++ b/src/android/camera_stream.h\n> @@ -7,12 +7,16 @@\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> @@ -20,6 +24,7 @@\n> #include \n> \n> #include \"camera_request.h\"\n> +#include \"post_processor.h\"\n> \n> class CameraDevice;\n> class PostProcessor;\n\nYou can drop the forward declaration.\n\n> @@ -124,8 +129,38 @@ public:\n> \tint process(Camera3RequestDescriptor::StreamBuffer *streamBuffer);\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::StreamBuffer *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_ = State::Stopped;\n> +\t};\n> +\n> \tint waitFence(int fence);\n> \n> \tCameraDevice *const cameraDevice_;\n> @@ -142,6 +177,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 54531BDB1C\n\tfor ;\n\tMon, 25 Oct 2021 22:05:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0C71C6487A;\n\tTue, 26 Oct 2021 00:05:38 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9F2166486D\n\tfor ;\n\tTue, 26 Oct 2021 00:05:35 +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 1EDC2292;\n\tTue, 26 Oct 2021 00:05:35 +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=\"Sbfy0vIo\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1635199535;\n\tbh=iw7/uhDzFJW/3Sd0tw9uGDPMz00D4x9zb/svQ/jVYpQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Sbfy0vIoVf+6msLNFEh/MrZ+gsBkWLoxQcZkvuqbklPQ6J+wbzv8MB/IpV2EHd5kD\n\tmxCKm55z+Vhu0Akdeu8rs13zBEse6h6dI8+pTosb6GKc1k6FHIZPKJ9e80zxFz/s57\n\tCgIsA8l7LiPhPZesXjGzdvEQ4+ZKiPRjxTNk6d1Y=","Date":"Tue, 26 Oct 2021 01:05:13 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20211025203833.122460-1-umang.jain@ideasonboard.com>\n\t<20211025203833.122460-8-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211025203833.122460-8-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v7 7/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":20491,"web_url":"https://patchwork.libcamera.org/comment/20491/","msgid":"","date":"2021-10-26T01:12:54","subject":"Re: [libcamera-devel] [PATCH v7 7/7] android: post_processor: Make\n\tpost processing async","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Umang, thank you for the patch.\n\nOn Tue, Oct 26, 2021 at 7:05 AM Laurent Pinchart\n wrote:\n>\n> Hi Umang,\n>\n> Thank you for the patch.\n>\n> On Tue, Oct 26, 2021 at 02:08:33AM +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> > The entire post-processing handling iteration is locked under\n> > streamProcessMutex_ which helps us to queue all the post-processing\n>\n> s/streamProcessMutex_/streamsProcessMutex_/\n>\n> > request at once, before any of the post-processing completion slot\n> > (streamProcessingComplete()) is allowed to run for post-processing\n> > requests completing in parallel. This helps us to manage both\n> > synchronous and asynchronous errors encountered during the entire\n> > post processing operation. Since a post-processing operation can\n> > even complete after CameraDevice::requestComplete() has returned,\n> > we need to check and complete the descriptor from\n> > streamProcessingComplete() running in the PostProcessorWorker's\n> > thread.\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. It is hooked with\n> > CameraStream::flush(), which isn't used currently but will be\n> > used when we handle flush/stop scenarios in greater detail\n> > subsequently (in a different patchset).\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\nReviewed-by: Hirokazu Honda \n> > ---\n> > src/android/camera_device.cpp | 44 ++++++---------\n> > src/android/camera_stream.cpp | 101 ++++++++++++++++++++++++++++++++--\n> > src/android/camera_stream.h | 37 +++++++++++++\n> > 3 files changed, 151 insertions(+), 31 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index 3ded0f7e..53ebe0ea 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -1027,29 +1027,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> >\n> > void CameraDevice::requestComplete(Request *request)\n> > {\n> > - Camera3RequestDescriptor *descriptor;\n> > - {\n> > - MutexLocker descriptorsLock(descriptorsMutex_);\n> > - ASSERT(!descriptors_.empty());\n> > - descriptor = descriptors_.front().get();\n> > - }\n> > -\n> > - if (descriptor->request_->cookie() != request->cookie()) {\n> > - /*\n> > - * \\todo Clarify if the Camera has to be closed on\n> > - * ERROR_DEVICE.\n> > - */\n> > - LOG(HAL, Error)\n> > - << \"Out-of-order completion for request \"\n> > - << utils::hex(request->cookie());\n> > -\n> > - MutexLocker descriptorsLock(descriptorsMutex_);\n> > - descriptors_.pop();\n> > -\n> > - notifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);\n> > -\n> > - return;\n> > - }\n> > + Camera3RequestDescriptor *descriptor =\n> > + reinterpret_cast(request->cookie());\n> >\n> > /*\n> > * Prepare the capture result for the Android camera stack.\n> > @@ -1124,9 +1103,13 @@ void CameraDevice::requestComplete(Request *request)\n> > }\n> >\n> > /* Handle post-processing. */\n> > + MutexLocker locker(descriptor->streamsProcessMutex_);\n> > +\n> > /*\n> > - * \\todo Protect the loop below with streamProcessMutex_ when post\n>\n> In the patch that introduced this,\n>\n> s/streamProcessMutex_/streamsProcessMutex_/\n>\n> > - * processor runs asynchronously.\n> > + * Queue all the post-processing streams request at once. The completion\n> > + * slot streamProcessingComplete() can only execute when we are out\n> > + * this critical section. This helps to handle synchronous errors here\n> > + * itself.\n> > */\n> > auto iter = descriptor->pendingStreamsToProcess_.begin();\n> > while (iter != descriptor->pendingStreamsToProcess_.end()) {\n> > @@ -1158,8 +1141,10 @@ void CameraDevice::requestComplete(Request *request)\n> > }\n> > }\n> >\n> > - if (descriptor->pendingStreamsToProcess_.empty())\n> > + if (descriptor->pendingStreamsToProcess_.empty()) {\n> > + locker.unlock();\n> > completeDescriptor(descriptor);\n> > + }\n> > }\n> >\n> > void CameraDevice::completeDescriptor(Camera3RequestDescriptor *descriptor)\n> > @@ -1245,6 +1230,13 @@ void CameraDevice::streamProcessingComplete(Camera3RequestDescriptor::StreamBuff\n> > MutexLocker locker(request->streamsProcessMutex_);\n> >\n> > request->pendingStreamsToProcess_.erase(streamBuffer->stream);\n> > +\n> > + if (!request->pendingStreamsToProcess_.empty())\n> > + return;\n> > +\n> > + locker.unlock();\n>\n> Maybe\n>\n> {\n> MutexLocker locker(request->streamsProcessMutex_);\n>\n> request->pendingStreamsToProcess_.erase(streamBuffer->stream);\n> if (!request->pendingStreamsToProcess_.empty())\n> return;\n> }\n>\n> up to you.\n>\n> > +\n> > + completeDescriptor(streamBuffer->request);\n> > }\n> >\n> > std::string CameraDevice::logPrefix() const\n> > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> > index fed99022..9023c13c 100644\n> > --- a/src/android/camera_stream.cpp\n> > +++ b/src/android/camera_stream.cpp\n> > @@ -99,6 +99,7 @@ int CameraStream::configure()\n> > if (ret)\n> > return ret;\n> >\n> > + worker_ = std::make_unique(postProcessor_.get());\n> > postProcessor_->processComplete.connect(\n> > this, [&](Camera3RequestDescriptor::StreamBuffer *streamBuffer,\n> > PostProcessor::Status status) {\n> > @@ -112,6 +113,8 @@ int CameraStream::configure()\n> > cameraDevice_->streamProcessingComplete(streamBuffer,\n> > bufferStatus);\n> > });\n> > +\n> > + worker_->start();\n> > }\n> >\n> > if (type_ == Type::Internal) {\n> > @@ -178,10 +181,6 @@ int CameraStream::process(Camera3RequestDescriptor::StreamBuffer *streamBuffer)\n> > streamBuffer->fence = -1;\n> > }\n> >\n> > - /*\n> > - * \\todo Buffer mapping and processing should be moved to a\n> > - * separate thread.\n> > - */\n> > const StreamConfiguration &output = configuration();\n> > streamBuffer->dstBuffer = std::make_unique(\n> > *streamBuffer->camera3Buffer, output.pixelFormat, output.size,\n> > @@ -191,11 +190,19 @@ int CameraStream::process(Camera3RequestDescriptor::StreamBuffer *streamBuffer)\n> > return -EINVAL;\n> > }\n> >\n> > - postProcessor_->process(streamBuffer);\n> > + worker_->queueRequest(streamBuffer);\n> >\n> > return 0;\n> > }\n> >\n> > +void CameraStream::flush()\n> > +{\n> > + if (!postProcessor_)\n> > + return;\n> > +\n> > + worker_->flush();\n> > +}\n> > +\n> > FrameBuffer *CameraStream::getBuffer()\n> > {\n> > if (!allocator_)\n> > @@ -223,3 +230,87 @@ void CameraStream::putBuffer(FrameBuffer *buffer)\n> >\n> > buffers_.push_back(buffer);\n> > }\n> > +\n> > +CameraStream::PostProcessorWorker::PostProcessorWorker(PostProcessor *postProcessor)\n> > + : postProcessor_(postProcessor)\n> > +{\n> > +}\n> > +\n> > +CameraStream::PostProcessorWorker::~PostProcessorWorker()\n> > +{\n> > + {\n> > + libcamera::MutexLocker lock(mutex_);\n> > + state_ = State::Stopped;\n> > + }\n> > +\n> > + cv_.notify_one();\n> > + wait();\n> > +}\n> > +\n> > +void CameraStream::PostProcessorWorker::start()\n> > +{\n> > + {\n> > + libcamera::MutexLocker lock(mutex_);\n> > + ASSERT(state_ != State::Running);\n> > + state_ = State::Running;\n> > + }\n> > +\n> > + Thread::start();\n> > +}\n> > +\n> > +void CameraStream::PostProcessorWorker::queueRequest(Camera3RequestDescriptor::StreamBuffer *dest)\n> > +{\n> > + {\n> > + MutexLocker lock(mutex_);\n> > + ASSERT(state_ == State::Running);\n> > + requests_.push(dest);\n> > + }\n> > +\n> > + cv_.notify_one();\n> > +}\n> > +\n> > +void CameraStream::PostProcessorWorker::run()\n> > +{\n> > + MutexLocker locker(mutex_);\n> > +\n> > + while (1) {\n> > + cv_.wait(locker, [&] {\n> > + return state_ != State::Running || !requests_.empty();\n> > + });\n> > +\n> > + if (state_ != State::Running)\n> > + break;\n> > +\n> > + Camera3RequestDescriptor::StreamBuffer *streamBuffer = requests_.front();\n> > + requests_.pop();\n> > + locker.unlock();\n> > +\n> > + postProcessor_->process(streamBuffer);\n> > +\n> > + locker.lock();\n> > + }\n> > +\n> > + if (state_ == State::Flushing) {\n> > + std::queue requests =\n> > + std::move(requests_);\n> > + locker.unlock();\n> > +\n> > + while (!requests.empty()) {\n> > + postProcessor_->processComplete.emit(\n> > + requests.front(), PostProcessor::Status::Error);\n> > + requests.pop();\n> > + }\n> > +\n> > + locker.lock();\n> > + state_ = State::Stopped;\n> > + }\n> > +}\n> > +\n> > +void CameraStream::PostProcessorWorker::flush()\n> > +{\n> > + libcamera::MutexLocker lock(mutex_);\n> > + state_ = State::Flushing;\n> > + lock.unlock();\n> > +\n> > + cv_.notify_one();\n> > +}\n> > diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n> > index e74a9a3b..1588938a 100644\n> > --- a/src/android/camera_stream.h\n> > +++ b/src/android/camera_stream.h\n> > @@ -7,12 +7,16 @@\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> > @@ -20,6 +24,7 @@\n> > #include \n> >\n> > #include \"camera_request.h\"\n> > +#include \"post_processor.h\"\n> >\n> > class CameraDevice;\n> > class PostProcessor;\n>\n> You can drop the forward declaration.\n>\n> > @@ -124,8 +129,38 @@ public:\n> > int process(Camera3RequestDescriptor::StreamBuffer *streamBuffer);\n> > libcamera::FrameBuffer *getBuffer();\n> > void putBuffer(libcamera::FrameBuffer *buffer);\n> > + void flush();\n> >\n> > private:\n> > + class PostProcessorWorker : public libcamera::Thread\n> > + {\n> > + public:\n> > + enum class State {\n> > + Stopped,\n> > + Running,\n> > + Flushing,\n> > + };\n> > +\n> > + PostProcessorWorker(PostProcessor *postProcessor);\n> > + ~PostProcessorWorker();\n> > +\n> > + void start();\n> > + void queueRequest(Camera3RequestDescriptor::StreamBuffer *request);\n> > + void flush();\n> > +\n> > + protected:\n> > + void run() override;\n> > +\n> > + private:\n> > + PostProcessor *postProcessor_;\n> > +\n> > + libcamera::Mutex mutex_;\n> > + std::condition_variable cv_;\n> > +\n> > + std::queue requests_;\n> > + State state_ = State::Stopped;\n> > + };\n> > +\n> > int waitFence(int fence);\n> >\n> > CameraDevice *const cameraDevice_;\n> > @@ -142,6 +177,8 @@ private:\n> > */\n> > std::unique_ptr mutex_;\n> > std::unique_ptr postProcessor_;\n> > +\n> > + std::unique_ptr worker_;\n> > };\n> >\n> > #endif /* __ANDROID_CAMERA_STREAM__ */\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 1290CBDB1C\n\tfor ;\n\tTue, 26 Oct 2021 01:13:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C64A26487A;\n\tTue, 26 Oct 2021 03:13:06 +0200 (CEST)","from mail-ed1-x52d.google.com (mail-ed1-x52d.google.com\n\t[IPv6:2a00:1450:4864:20::52d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 64BFA6486D\n\tfor ;\n\tTue, 26 Oct 2021 03:13:05 +0200 (CEST)","by mail-ed1-x52d.google.com with SMTP id l13so6516311edi.8\n\tfor ;\n\tMon, 25 Oct 2021 18:13:05 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"YJonpHv5\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=UoYSrGTLuBOWS4bfjyYk+BEksv35bZ2H98yj9O9jl8E=;\n\tb=YJonpHv5WDKmp7/efXQI1NTkPrOQx74OaAknbsgCJER9rQa5QroAJah0xejbZU8N6R\n\tjz/H3CAZJ50nW91PhXQhwD6IA8v6rcZQ1GFqSK3AauBQayRcgGSLwXokh5YMWq2gKqAd\n\t+3O4KGgx+uIQliEq3vXHYEQuCFleCJNZYbFj8=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=UoYSrGTLuBOWS4bfjyYk+BEksv35bZ2H98yj9O9jl8E=;\n\tb=X/gmrUZD1YBuc1ClTLcqqPs2tdk8EuY56wgKBrW4U3O5b2qrIkJz2SmEmVl9WPevYZ\n\tNVtk711rrb5FnFlm5DlwdsgMrZLpQoKc3HKR8bZkrOaS+sjTXGCgRRI7KkaIx28YZl9p\n\tEPZIY8JLp4jV6HZ4PxlO3kMxn91ql57HJ/95MRaVJmIE6OuxBNShK3lAekZ9dYOdUK+R\n\tU/ZAgNB5IJQqEIWNsTWuc1OZgCTtmMYdIwobIFkHdLRWEKDExxW/uGGco1YSdGiZN2PM\n\t3EVvotycogUaVbknTs2ZJmyrAT2bdLtygWmQ14WHsSUcfN+/8W6GZgs5WN8oQudm2cBJ\n\tT8Ww==","X-Gm-Message-State":"AOAM5338f43SDvVhRhkoXoa6z5gIijYqHkMfXEN67cOw6IhGbmk+1ZuP\n\tP7yHQsTuqg6qQElLAVP9SByQKhUdYB6/lk0hFdJy7w==","X-Google-Smtp-Source":"ABdhPJzIYejghPtXzM3+RbNIfe2NxWRKB2YGQByVyCdnnf2SCdF+heYUeDQAUV2VvYTG71nny5RIPvfjk7aFTt1Trl0=","X-Received":"by 2002:a17:906:26ce:: with SMTP id\n\tu14mr26753916ejc.559.1635210784701; \n\tMon, 25 Oct 2021 18:13:04 -0700 (PDT)","MIME-Version":"1.0","References":"<20211025203833.122460-1-umang.jain@ideasonboard.com>\n\t<20211025203833.122460-8-umang.jain@ideasonboard.com>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Tue, 26 Oct 2021 10:12:54 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v7 7/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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20493,"web_url":"https://patchwork.libcamera.org/comment/20493/","msgid":"","date":"2021-10-26T04:39:22","subject":"Re: [libcamera-devel] [PATCH v7 7/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/26/21 3:35 AM, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> Thank you for the patch.\n>\n> On Tue, Oct 26, 2021 at 02:08:33AM +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>> The entire post-processing handling iteration is locked under\n>> streamProcessMutex_ which helps us to queue all the post-processing\n> s/streamProcessMutex_/streamsProcessMutex_/\n>\n>> request at once, before any of the post-processing completion slot\n>> (streamProcessingComplete()) is allowed to run for post-processing\n>> requests completing in parallel. This helps us to manage both\n>> synchronous and asynchronous errors encountered during the entire\n>> post processing operation. Since a post-processing operation can\n>> even complete after CameraDevice::requestComplete() has returned,\n>> we need to check and complete the descriptor from\n>> streamProcessingComplete() running in the PostProcessorWorker's\n>> thread.\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. It is hooked with\n>> CameraStream::flush(), which isn't used currently but will be\n>> used when we handle flush/stop scenarios in greater detail\n>> subsequently (in a different patchset).\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 | 44 ++++++---------\n>> src/android/camera_stream.cpp | 101 ++++++++++++++++++++++++++++++++--\n>> src/android/camera_stream.h | 37 +++++++++++++\n>> 3 files changed, 151 insertions(+), 31 deletions(-)\n>>\n>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>> index 3ded0f7e..53ebe0ea 100644\n>> --- a/src/android/camera_device.cpp\n>> +++ b/src/android/camera_device.cpp\n>> @@ -1027,29 +1027,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.\n>> -\t\t */\n>> -\t\tLOG(HAL, Error)\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>> -\n>> -\t\tnotifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);\n>> -\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>> @@ -1124,9 +1103,13 @@ void CameraDevice::requestComplete(Request *request)\n>> \t}\n>> \n>> \t/* Handle post-processing. */\n>> +\tMutexLocker locker(descriptor->streamsProcessMutex_);\n>> +\n>> \t/*\n>> -\t * \\todo Protect the loop below with streamProcessMutex_ when post\n> In the patch that introduced this,\n>\n> s/streamProcessMutex_/streamsProcessMutex_/\n>\n>> -\t * processor runs asynchronously.\n>> +\t * Queue all the post-processing streams request at once. The completion\n>> +\t * slot streamProcessingComplete() can only execute when we are out\n>> +\t * this critical section. This helps to handle synchronous errors here\n>> +\t * itself.\n>> \t */\n>> \tauto iter = descriptor->pendingStreamsToProcess_.begin();\n>> \twhile (iter != descriptor->pendingStreamsToProcess_.end()) {\n>> @@ -1158,8 +1141,10 @@ void CameraDevice::requestComplete(Request *request)\n>> \t\t}\n>> \t}\n>> \n>> -\tif (descriptor->pendingStreamsToProcess_.empty())\n>> +\tif (descriptor->pendingStreamsToProcess_.empty()) {\n>> +\t\tlocker.unlock();\n>> \t\tcompleteDescriptor(descriptor);\n>> +\t}\n>> }\n>> \n>> void CameraDevice::completeDescriptor(Camera3RequestDescriptor *descriptor)\n>> @@ -1245,6 +1230,13 @@ void CameraDevice::streamProcessingComplete(Camera3RequestDescriptor::StreamBuff\n>> \tMutexLocker locker(request->streamsProcessMutex_);\n>> \n>> \trequest->pendingStreamsToProcess_.erase(streamBuffer->stream);\n>> +\n>> +\tif (!request->pendingStreamsToProcess_.empty())\n>> +\t\treturn;\n>> +\n>> +\tlocker.unlock();\n> Maybe\n>\n> \t{\n> \t\tMutexLocker locker(request->streamsProcessMutex_);\n>\n> \t\trequest->pendingStreamsToProcess_.erase(streamBuffer->stream);\n> \t\tif (!request->pendingStreamsToProcess_.empty())\n> \t\t\treturn;\n> \t}\n>\n> up to you.\n\n\nOk, yes, this looks cleaner\n\n>\n>> +\n>> +\tcompleteDescriptor(streamBuffer->request);\n>> }\n>> \n>> std::string CameraDevice::logPrefix() const\n>> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n>> index fed99022..9023c13c 100644\n>> --- a/src/android/camera_stream.cpp\n>> +++ b/src/android/camera_stream.cpp\n>> @@ -99,6 +99,7 @@ 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::StreamBuffer *streamBuffer,\n>> \t\t\t\t PostProcessor::Status status) {\n>> @@ -112,6 +113,8 @@ int CameraStream::configure()\n>> \t\t\t\tcameraDevice_->streamProcessingComplete(streamBuffer,\n>> \t\t\t\t\t\t\t\t\tbufferStatus);\n>> \t\t\t});\n>> +\n>> +\t\tworker_->start();\n>> \t}\n>> \n>> \tif (type_ == Type::Internal) {\n>> @@ -178,10 +181,6 @@ int CameraStream::process(Camera3RequestDescriptor::StreamBuffer *streamBuffer)\n>> \t\tstreamBuffer->fence = -1;\n>> \t}\n>> \n>> -\t/*\n>> -\t * \\todo Buffer mapping and processing should be moved to a\n>> -\t * separate thread.\n>> -\t */\n>> \tconst StreamConfiguration &output = configuration();\n>> \tstreamBuffer->dstBuffer = std::make_unique(\n>> \t\t*streamBuffer->camera3Buffer, output.pixelFormat, output.size,\n>> @@ -191,11 +190,19 @@ int CameraStream::process(Camera3RequestDescriptor::StreamBuffer *streamBuffer)\n>> \t\treturn -EINVAL;\n>> \t}\n>> \n>> -\tpostProcessor_->process(streamBuffer);\n>> +\tworker_->queueRequest(streamBuffer);\n>> \n>> \treturn 0;\n>> }\n>> \n>> +void CameraStream::flush()\n>> +{\n>> +\tif (!postProcessor_)\n>> +\t\treturn;\n>> +\n>> +\tworker_->flush();\n>> +}\n>> +\n>> FrameBuffer *CameraStream::getBuffer()\n>> {\n>> \tif (!allocator_)\n>> @@ -223,3 +230,87 @@ 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\tASSERT(state_ != State::Running);\n>> +\t\tstate_ = State::Running;\n>> +\t}\n>> +\n>> +\tThread::start();\n>> +}\n>> +\n>> +void CameraStream::PostProcessorWorker::queueRequest(Camera3RequestDescriptor::StreamBuffer *dest)\n>> +{\n>> +\t{\n>> +\t\tMutexLocker lock(mutex_);\n>> +\t\tASSERT(state_ == State::Running);\n>> +\t\trequests_.push(dest);\n>> +\t}\n>> +\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::StreamBuffer *streamBuffer = requests_.front();\n>> +\t\trequests_.pop();\n>> +\t\tlocker.unlock();\n>> +\n>> +\t\tpostProcessor_->process(streamBuffer);\n>> +\n>> +\t\tlocker.lock();\n>> +\t}\n>> +\n>> +\tif (state_ == State::Flushing) {\n>> +\t\tstd::queue requests =\n>> +\t\t\tstd::move(requests_);\n>> +\t\tlocker.unlock();\n>> +\n>> +\t\twhile (!requests.empty()) {\n>> +\t\t\tpostProcessor_->processComplete.emit(\n>> +\t\t\t\trequests.front(), PostProcessor::Status::Error);\n>> +\t\t\trequests.pop();\n>> +\t\t}\n>> +\n>> +\t\tlocker.lock();\n>> +\t\tstate_ = State::Stopped;\n>> +\t}\n>> +}\n>> +\n>> +void CameraStream::PostProcessorWorker::flush()\n>> +{\n>> +\tlibcamera::MutexLocker lock(mutex_);\n>> +\tstate_ = State::Flushing;\n>> +\tlock.unlock();\n>> +\n>> +\tcv_.notify_one();\n>> +}\n>> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n>> index e74a9a3b..1588938a 100644\n>> --- a/src/android/camera_stream.h\n>> +++ b/src/android/camera_stream.h\n>> @@ -7,12 +7,16 @@\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>> @@ -20,6 +24,7 @@\n>> #include \n>> \n>> #include \"camera_request.h\"\n>> +#include \"post_processor.h\"\n>> \n>> class CameraDevice;\n>> class PostProcessor;\n> You can drop the forward declaration.\n\n\nOuch, What happens when you manually cherry-pick patches, because \nconflicts were too hard to deal with :-/\n\nI will address these changes locally, should  I re-post a new version v8 \nwith those changes?\n\n>\n>> @@ -124,8 +129,38 @@ public:\n>> \tint process(Camera3RequestDescriptor::StreamBuffer *streamBuffer);\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::StreamBuffer *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_ = State::Stopped;\n>> +\t};\n>> +\n>> \tint waitFence(int fence);\n>> \n>> \tCameraDevice *const cameraDevice_;\n>> @@ -142,6 +177,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 BF6CABDB1C\n\tfor ;\n\tTue, 26 Oct 2021 04:39:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0AA5664878;\n\tTue, 26 Oct 2021 06:39: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 831CF6486B\n\tfor ;\n\tTue, 26 Oct 2021 06:39:27 +0200 (CEST)","from [192.168.1.106] (unknown [103.251.226.211])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6011D3F0;\n\tTue, 26 Oct 2021 06:39: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=\"Y/j401NR\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1635223167;\n\tbh=SOATqbUsjlPCy1pwLrp/9ACPloKQ21ywhEbxxtalcWg=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=Y/j401NRdKkJN075JYcq2/rDOInXWusc7X5OveBuQbDwyafA58Th6GJJIPkSxkZxE\n\tIf0BHIwNCTGG7+7yCPMTj9V2568KY350KH9XQYOBjfEAzwEDhcVmuMiZgi3360O5uu\n\tbdbgV0jU3BIZF81CnR0+lTT5shdxTyckUIMuhPdA=","To":"Laurent Pinchart ","References":"<20211025203833.122460-1-umang.jain@ideasonboard.com>\n\t<20211025203833.122460-8-umang.jain@ideasonboard.com>\n\t","From":"Umang Jain ","Message-ID":"","Date":"Tue, 26 Oct 2021 10:09:22 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v7 7/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":20504,"web_url":"https://patchwork.libcamera.org/comment/20504/","msgid":"","date":"2021-10-26T07:20:42","subject":"Re: [libcamera-devel] [PATCH v7 7/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 Tue, Oct 26, 2021 at 10:09:22AM +0530, Umang Jain wrote:\n> On 10/26/21 3:35 AM, Laurent Pinchart wrote:\n> > On Tue, Oct 26, 2021 at 02:08:33AM +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> >> The entire post-processing handling iteration is locked under\n> >> streamProcessMutex_ which helps us to queue all the post-processing\n> > s/streamProcessMutex_/streamsProcessMutex_/\n> >\n> >> request at once, before any of the post-processing completion slot\n> >> (streamProcessingComplete()) is allowed to run for post-processing\n> >> requests completing in parallel. This helps us to manage both\n> >> synchronous and asynchronous errors encountered during the entire\n> >> post processing operation. Since a post-processing operation can\n> >> even complete after CameraDevice::requestComplete() has returned,\n> >> we need to check and complete the descriptor from\n> >> streamProcessingComplete() running in the PostProcessorWorker's\n> >> thread.\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. It is hooked with\n> >> CameraStream::flush(), which isn't used currently but will be\n> >> used when we handle flush/stop scenarios in greater detail\n> >> subsequently (in a different patchset).\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 | 44 ++++++---------\n> >> src/android/camera_stream.cpp | 101 ++++++++++++++++++++++++++++++++--\n> >> src/android/camera_stream.h | 37 +++++++++++++\n> >> 3 files changed, 151 insertions(+), 31 deletions(-)\n> >>\n> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> >> index 3ded0f7e..53ebe0ea 100644\n> >> --- a/src/android/camera_device.cpp\n> >> +++ b/src/android/camera_device.cpp\n> >> @@ -1027,29 +1027,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.\n> >> -\t\t */\n> >> -\t\tLOG(HAL, Error)\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> >> -\n> >> -\t\tnotifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);\n> >> -\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> >> @@ -1124,9 +1103,13 @@ void CameraDevice::requestComplete(Request *request)\n> >> \t}\n> >> \n> >> \t/* Handle post-processing. */\n> >> +\tMutexLocker locker(descriptor->streamsProcessMutex_);\n> >> +\n> >> \t/*\n> >> -\t * \\todo Protect the loop below with streamProcessMutex_ when post\n> > In the patch that introduced this,\n> >\n> > s/streamProcessMutex_/streamsProcessMutex_/\n> >\n> >> -\t * processor runs asynchronously.\n> >> +\t * Queue all the post-processing streams request at once. The completion\n> >> +\t * slot streamProcessingComplete() can only execute when we are out\n> >> +\t * this critical section. This helps to handle synchronous errors here\n> >> +\t * itself.\n> >> \t */\n> >> \tauto iter = descriptor->pendingStreamsToProcess_.begin();\n> >> \twhile (iter != descriptor->pendingStreamsToProcess_.end()) {\n> >> @@ -1158,8 +1141,10 @@ void CameraDevice::requestComplete(Request *request)\n> >> \t\t}\n> >> \t}\n> >> \n> >> -\tif (descriptor->pendingStreamsToProcess_.empty())\n> >> +\tif (descriptor->pendingStreamsToProcess_.empty()) {\n> >> +\t\tlocker.unlock();\n> >> \t\tcompleteDescriptor(descriptor);\n> >> +\t}\n> >> }\n> >> \n> >> void CameraDevice::completeDescriptor(Camera3RequestDescriptor *descriptor)\n> >> @@ -1245,6 +1230,13 @@ void CameraDevice::streamProcessingComplete(Camera3RequestDescriptor::StreamBuff\n> >> \tMutexLocker locker(request->streamsProcessMutex_);\n> >> \n> >> \trequest->pendingStreamsToProcess_.erase(streamBuffer->stream);\n> >> +\n> >> +\tif (!request->pendingStreamsToProcess_.empty())\n> >> +\t\treturn;\n> >> +\n> >> +\tlocker.unlock();\n> > Maybe\n> >\n> > \t{\n> > \t\tMutexLocker locker(request->streamsProcessMutex_);\n> >\n> > \t\trequest->pendingStreamsToProcess_.erase(streamBuffer->stream);\n> > \t\tif (!request->pendingStreamsToProcess_.empty())\n> > \t\t\treturn;\n> > \t}\n> >\n> > up to you.\n> \n> \n> Ok, yes, this looks cleaner\n> \n> >\n> >> +\n> >> +\tcompleteDescriptor(streamBuffer->request);\n> >> }\n> >> \n> >> std::string CameraDevice::logPrefix() const\n> >> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> >> index fed99022..9023c13c 100644\n> >> --- a/src/android/camera_stream.cpp\n> >> +++ b/src/android/camera_stream.cpp\n> >> @@ -99,6 +99,7 @@ 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::StreamBuffer *streamBuffer,\n> >> \t\t\t\t PostProcessor::Status status) {\n> >> @@ -112,6 +113,8 @@ int CameraStream::configure()\n> >> \t\t\t\tcameraDevice_->streamProcessingComplete(streamBuffer,\n> >> \t\t\t\t\t\t\t\t\tbufferStatus);\n> >> \t\t\t});\n> >> +\n> >> +\t\tworker_->start();\n> >> \t}\n> >> \n> >> \tif (type_ == Type::Internal) {\n> >> @@ -178,10 +181,6 @@ int CameraStream::process(Camera3RequestDescriptor::StreamBuffer *streamBuffer)\n> >> \t\tstreamBuffer->fence = -1;\n> >> \t}\n> >> \n> >> -\t/*\n> >> -\t * \\todo Buffer mapping and processing should be moved to a\n> >> -\t * separate thread.\n> >> -\t */\n> >> \tconst StreamConfiguration &output = configuration();\n> >> \tstreamBuffer->dstBuffer = std::make_unique(\n> >> \t\t*streamBuffer->camera3Buffer, output.pixelFormat, output.size,\n> >> @@ -191,11 +190,19 @@ int CameraStream::process(Camera3RequestDescriptor::StreamBuffer *streamBuffer)\n> >> \t\treturn -EINVAL;\n> >> \t}\n> >> \n> >> -\tpostProcessor_->process(streamBuffer);\n> >> +\tworker_->queueRequest(streamBuffer);\n> >> \n> >> \treturn 0;\n> >> }\n> >> \n> >> +void CameraStream::flush()\n> >> +{\n> >> +\tif (!postProcessor_)\n> >> +\t\treturn;\n> >> +\n> >> +\tworker_->flush();\n> >> +}\n> >> +\n> >> FrameBuffer *CameraStream::getBuffer()\n> >> {\n> >> \tif (!allocator_)\n> >> @@ -223,3 +230,87 @@ 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\tASSERT(state_ != State::Running);\n> >> +\t\tstate_ = State::Running;\n> >> +\t}\n> >> +\n> >> +\tThread::start();\n> >> +}\n> >> +\n> >> +void CameraStream::PostProcessorWorker::queueRequest(Camera3RequestDescriptor::StreamBuffer *dest)\n> >> +{\n> >> +\t{\n> >> +\t\tMutexLocker lock(mutex_);\n> >> +\t\tASSERT(state_ == State::Running);\n> >> +\t\trequests_.push(dest);\n> >> +\t}\n> >> +\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::StreamBuffer *streamBuffer = requests_.front();\n> >> +\t\trequests_.pop();\n> >> +\t\tlocker.unlock();\n> >> +\n> >> +\t\tpostProcessor_->process(streamBuffer);\n> >> +\n> >> +\t\tlocker.lock();\n> >> +\t}\n> >> +\n> >> +\tif (state_ == State::Flushing) {\n> >> +\t\tstd::queue requests =\n> >> +\t\t\tstd::move(requests_);\n> >> +\t\tlocker.unlock();\n> >> +\n> >> +\t\twhile (!requests.empty()) {\n> >> +\t\t\tpostProcessor_->processComplete.emit(\n> >> +\t\t\t\trequests.front(), PostProcessor::Status::Error);\n> >> +\t\t\trequests.pop();\n> >> +\t\t}\n> >> +\n> >> +\t\tlocker.lock();\n> >> +\t\tstate_ = State::Stopped;\n> >> +\t}\n> >> +}\n> >> +\n> >> +void CameraStream::PostProcessorWorker::flush()\n> >> +{\n> >> +\tlibcamera::MutexLocker lock(mutex_);\n> >> +\tstate_ = State::Flushing;\n> >> +\tlock.unlock();\n> >> +\n> >> +\tcv_.notify_one();\n> >> +}\n> >> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n> >> index e74a9a3b..1588938a 100644\n> >> --- a/src/android/camera_stream.h\n> >> +++ b/src/android/camera_stream.h\n> >> @@ -7,12 +7,16 @@\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> >> @@ -20,6 +24,7 @@\n> >> #include \n> >> \n> >> #include \"camera_request.h\"\n> >> +#include \"post_processor.h\"\n> >> \n> >> class CameraDevice;\n> >> class PostProcessor;\n> >\n> > You can drop the forward declaration.\n> \n> Ouch, What happens when you manually cherry-pick patches, because \n> conflicts were too hard to deal with :-/\n> \n> I will address these changes locally, should  I re-post a new version v8 \n> with those changes?\n\nThey're small enough, if you're confident you got them right, I don't\nneed a v8 on the list.\n\n> >> @@ -124,8 +129,38 @@ public:\n> >> \tint process(Camera3RequestDescriptor::StreamBuffer *streamBuffer);\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::StreamBuffer *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_ = State::Stopped;\n> >> +\t};\n> >> +\n> >> \tint waitFence(int fence);\n> >> \n> >> \tCameraDevice *const cameraDevice_;\n> >> @@ -142,6 +177,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 C2CD7BF415\n\tfor ;\n\tTue, 26 Oct 2021 07:21:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2F45E64878;\n\tTue, 26 Oct 2021 09:21:06 +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 2476960123\n\tfor ;\n\tTue, 26 Oct 2021 09:21:05 +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 8DF3E3F0;\n\tTue, 26 Oct 2021 09:21:04 +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=\"ZcnYtpkZ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1635232864;\n\tbh=q+nEImbbi17D6E+hTqdBOnwejMi91+2yazbeWurudk4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ZcnYtpkZPFcgpp8FbRJ2C7bQeTeCrDYohQCDvDc/q2dUydfiEM242wW9FEmfXx/py\n\ta+Sh7ut41DhjAquxpWn/dwLgnDLlpLsJcNEX6g3Pb/sKlaijMFFxsEIQOUNLnlyecs\n\twl1+NuixWyJRC87dtmDuTxjLIXgeqITrgZ16QStQ=","Date":"Tue, 26 Oct 2021 10:20:42 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20211025203833.122460-1-umang.jain@ideasonboard.com>\n\t<20211025203833.122460-8-umang.jain@ideasonboard.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v7 7/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":20506,"web_url":"https://patchwork.libcamera.org/comment/20506/","msgid":"<7e2fed52-e7b9-f6b6-3489-ac89e398c6d0@ideasonboard.com>","date":"2021-10-26T07:42:11","subject":"Re: [libcamera-devel] [PATCH v7 7/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/26/21 12:50 PM, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> On Tue, Oct 26, 2021 at 10:09:22AM +0530, Umang Jain wrote:\n>> On 10/26/21 3:35 AM, Laurent Pinchart wrote:\n>>> On Tue, Oct 26, 2021 at 02:08:33AM +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>>>> The entire post-processing handling iteration is locked under\n>>>> streamProcessMutex_ which helps us to queue all the post-processing\n>>> s/streamProcessMutex_/streamsProcessMutex_/\n>>>\n>>>> request at once, before any of the post-processing completion slot\n>>>> (streamProcessingComplete()) is allowed to run for post-processing\n>>>> requests completing in parallel. This helps us to manage both\n>>>> synchronous and asynchronous errors encountered during the entire\n>>>> post processing operation. Since a post-processing operation can\n>>>> even complete after CameraDevice::requestComplete() has returned,\n>>>> we need to check and complete the descriptor from\n>>>> streamProcessingComplete() running in the PostProcessorWorker's\n>>>> thread.\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. It is hooked with\n>>>> CameraStream::flush(), which isn't used currently but will be\n>>>> used when we handle flush/stop scenarios in greater detail\n>>>> subsequently (in a different patchset).\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 | 44 ++++++---------\n>>>> src/android/camera_stream.cpp | 101 ++++++++++++++++++++++++++++++++--\n>>>> src/android/camera_stream.h | 37 +++++++++++++\n>>>> 3 files changed, 151 insertions(+), 31 deletions(-)\n>>>>\n>>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>>>> index 3ded0f7e..53ebe0ea 100644\n>>>> --- a/src/android/camera_device.cpp\n>>>> +++ b/src/android/camera_device.cpp\n>>>> @@ -1027,29 +1027,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.\n>>>> -\t\t */\n>>>> -\t\tLOG(HAL, Error)\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>>>> -\n>>>> -\t\tnotifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);\n>>>> -\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>>>> @@ -1124,9 +1103,13 @@ void CameraDevice::requestComplete(Request *request)\n>>>> \t}\n>>>> \n>>>> \t/* Handle post-processing. */\n>>>> +\tMutexLocker locker(descriptor->streamsProcessMutex_);\n>>>> +\n>>>> \t/*\n>>>> -\t * \\todo Protect the loop below with streamProcessMutex_ when post\n>>> In the patch that introduced this,\n>>>\n>>> s/streamProcessMutex_/streamsProcessMutex_/\n>>>\n>>>> -\t * processor runs asynchronously.\n>>>> +\t * Queue all the post-processing streams request at once. The completion\n>>>> +\t * slot streamProcessingComplete() can only execute when we are out\n>>>> +\t * this critical section. This helps to handle synchronous errors here\n>>>> +\t * itself.\n>>>> \t */\n>>>> \tauto iter = descriptor->pendingStreamsToProcess_.begin();\n>>>> \twhile (iter != descriptor->pendingStreamsToProcess_.end()) {\n>>>> @@ -1158,8 +1141,10 @@ void CameraDevice::requestComplete(Request *request)\n>>>> \t\t}\n>>>> \t}\n>>>> \n>>>> -\tif (descriptor->pendingStreamsToProcess_.empty())\n>>>> +\tif (descriptor->pendingStreamsToProcess_.empty()) {\n>>>> +\t\tlocker.unlock();\n>>>> \t\tcompleteDescriptor(descriptor);\n>>>> +\t}\n>>>> }\n>>>> \n>>>> void CameraDevice::completeDescriptor(Camera3RequestDescriptor *descriptor)\n>>>> @@ -1245,6 +1230,13 @@ void CameraDevice::streamProcessingComplete(Camera3RequestDescriptor::StreamBuff\n>>>> \tMutexLocker locker(request->streamsProcessMutex_);\n>>>> \n>>>> \trequest->pendingStreamsToProcess_.erase(streamBuffer->stream);\n>>>> +\n>>>> +\tif (!request->pendingStreamsToProcess_.empty())\n>>>> +\t\treturn;\n>>>> +\n>>>> +\tlocker.unlock();\n>>> Maybe\n>>>\n>>> \t{\n>>> \t\tMutexLocker locker(request->streamsProcessMutex_);\n>>>\n>>> \t\trequest->pendingStreamsToProcess_.erase(streamBuffer->stream);\n>>> \t\tif (!request->pendingStreamsToProcess_.empty())\n>>> \t\t\treturn;\n>>> \t}\n>>>\n>>> up to you.\n>>\n>> Ok, yes, this looks cleaner\n>>\n>>>> +\n>>>> +\tcompleteDescriptor(streamBuffer->request);\n>>>> }\n>>>> \n>>>> std::string CameraDevice::logPrefix() const\n>>>> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n>>>> index fed99022..9023c13c 100644\n>>>> --- a/src/android/camera_stream.cpp\n>>>> +++ b/src/android/camera_stream.cpp\n>>>> @@ -99,6 +99,7 @@ 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::StreamBuffer *streamBuffer,\n>>>> \t\t\t\t PostProcessor::Status status) {\n>>>> @@ -112,6 +113,8 @@ int CameraStream::configure()\n>>>> \t\t\t\tcameraDevice_->streamProcessingComplete(streamBuffer,\n>>>> \t\t\t\t\t\t\t\t\tbufferStatus);\n>>>> \t\t\t});\n>>>> +\n>>>> +\t\tworker_->start();\n>>>> \t}\n>>>> \n>>>> \tif (type_ == Type::Internal) {\n>>>> @@ -178,10 +181,6 @@ int CameraStream::process(Camera3RequestDescriptor::StreamBuffer *streamBuffer)\n>>>> \t\tstreamBuffer->fence = -1;\n>>>> \t}\n>>>> \n>>>> -\t/*\n>>>> -\t * \\todo Buffer mapping and processing should be moved to a\n>>>> -\t * separate thread.\n>>>> -\t */\n>>>> \tconst StreamConfiguration &output = configuration();\n>>>> \tstreamBuffer->dstBuffer = std::make_unique(\n>>>> \t\t*streamBuffer->camera3Buffer, output.pixelFormat, output.size,\n>>>> @@ -191,11 +190,19 @@ int CameraStream::process(Camera3RequestDescriptor::StreamBuffer *streamBuffer)\n>>>> \t\treturn -EINVAL;\n>>>> \t}\n>>>> \n>>>> -\tpostProcessor_->process(streamBuffer);\n>>>> +\tworker_->queueRequest(streamBuffer);\n>>>> \n>>>> \treturn 0;\n>>>> }\n>>>> \n>>>> +void CameraStream::flush()\n>>>> +{\n>>>> +\tif (!postProcessor_)\n>>>> +\t\treturn;\n>>>> +\n>>>> +\tworker_->flush();\n>>>> +}\n>>>> +\n>>>> FrameBuffer *CameraStream::getBuffer()\n>>>> {\n>>>> \tif (!allocator_)\n>>>> @@ -223,3 +230,87 @@ 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\tASSERT(state_ != State::Running);\n>>>> +\t\tstate_ = State::Running;\n>>>> +\t}\n>>>> +\n>>>> +\tThread::start();\n>>>> +}\n>>>> +\n>>>> +void CameraStream::PostProcessorWorker::queueRequest(Camera3RequestDescriptor::StreamBuffer *dest)\n>>>> +{\n>>>> +\t{\n>>>> +\t\tMutexLocker lock(mutex_);\n>>>> +\t\tASSERT(state_ == State::Running);\n>>>> +\t\trequests_.push(dest);\n>>>> +\t}\n>>>> +\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::StreamBuffer *streamBuffer = requests_.front();\n>>>> +\t\trequests_.pop();\n>>>> +\t\tlocker.unlock();\n>>>> +\n>>>> +\t\tpostProcessor_->process(streamBuffer);\n>>>> +\n>>>> +\t\tlocker.lock();\n>>>> +\t}\n>>>> +\n>>>> +\tif (state_ == State::Flushing) {\n>>>> +\t\tstd::queue requests =\n>>>> +\t\t\tstd::move(requests_);\n>>>> +\t\tlocker.unlock();\n>>>> +\n>>>> +\t\twhile (!requests.empty()) {\n>>>> +\t\t\tpostProcessor_->processComplete.emit(\n>>>> +\t\t\t\trequests.front(), PostProcessor::Status::Error);\n>>>> +\t\t\trequests.pop();\n>>>> +\t\t}\n>>>> +\n>>>> +\t\tlocker.lock();\n>>>> +\t\tstate_ = State::Stopped;\n>>>> +\t}\n>>>> +}\n>>>> +\n>>>> +void CameraStream::PostProcessorWorker::flush()\n>>>> +{\n>>>> +\tlibcamera::MutexLocker lock(mutex_);\n>>>> +\tstate_ = State::Flushing;\n>>>> +\tlock.unlock();\n>>>> +\n>>>> +\tcv_.notify_one();\n>>>> +}\n>>>> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n>>>> index e74a9a3b..1588938a 100644\n>>>> --- a/src/android/camera_stream.h\n>>>> +++ b/src/android/camera_stream.h\n>>>> @@ -7,12 +7,16 @@\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>>>> @@ -20,6 +24,7 @@\n>>>> #include \n>>>> \n>>>> #include \"camera_request.h\"\n>>>> +#include \"post_processor.h\"\n>>>> \n>>>> class CameraDevice;\n>>>> class PostProcessor;\n>>> You can drop the forward declaration.\n>> Ouch, What happens when you manually cherry-pick patches, because\n>> conflicts were too hard to deal with :-/\n>>\n>> I will address these changes locally, should  I re-post a new version v8\n>> with those changes?\n> They're small enough, if you're confident you got them right, I don't\n> need a v8 on the list.\n\n\nAahh.. Saw this email right now. Already posted v8 on the list.\n\nAnyway, we are missing one R-B tag on last one patch, so it would be \ngood for someone to look at cleaned up patches.\n\n>\n>>>> @@ -124,8 +129,38 @@ public:\n>>>> \tint process(Camera3RequestDescriptor::StreamBuffer *streamBuffer);\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::StreamBuffer *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_ = State::Stopped;\n>>>> +\t};\n>>>> +\n>>>> \tint waitFence(int fence);\n>>>> \n>>>> \tCameraDevice *const cameraDevice_;\n>>>> @@ -142,6 +177,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 8AE06BDB1C\n\tfor ;\n\tTue, 26 Oct 2021 07:42:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C14DA6487A;\n\tTue, 26 Oct 2021 09:42:17 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0114260123\n\tfor ;\n\tTue, 26 Oct 2021 09:42:16 +0200 (CEST)","from [192.168.1.106] (unknown [103.251.226.211])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 35F76292;\n\tTue, 26 Oct 2021 09:42:16 +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=\"nljdqDHE\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1635234136;\n\tbh=+zG9CpMvlsFGOUY0tywiGL5ctI7QnFCs7c8Mhf4c3Ss=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=nljdqDHEZbIbzsGMGZguOI2ORq51yRbvWdy1SnUgHitZn12VqsY79sTKr3Gqrlwam\n\tBPEULKNG28zKIZJVODi/R160It7WFNp9+whHOY0D7rdiiPrBGrSMo9VCx17qhEU/Nq\n\tgJx9TSDQRRTxWHSD0OnSW04QuIfRNUrhMyny/gtA=","To":"Laurent Pinchart ","References":"<20211025203833.122460-1-umang.jain@ideasonboard.com>\n\t<20211025203833.122460-8-umang.jain@ideasonboard.com>\n\t\n\t\n\t","From":"Umang Jain ","Message-ID":"<7e2fed52-e7b9-f6b6-3489-ac89e398c6d0@ideasonboard.com>","Date":"Tue, 26 Oct 2021 13:12:11 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v7 7/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\" "}}]