[{"id":20529,"web_url":"https://patchwork.libcamera.org/comment/20529/","msgid":"","date":"2021-10-26T10:05:30","subject":"Re: [libcamera-devel] [PATCH v8 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 12:51:48PM +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> streamsProcessMutex_ which helps us to queue all the post-processing\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> Reviewed-by: Hirokazu Honda \n\nReviewed-by: Laurent Pinchart \n\n> ---\n> src/android/camera_device.cpp | 48 +++++++---------\n> src/android/camera_stream.cpp | 101 ++++++++++++++++++++++++++++++++--\n> src/android/camera_stream.h | 38 ++++++++++++-\n> 3 files changed, 153 insertions(+), 34 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index fed539f3..f2e0bdbd 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 streamsProcessMutex_ when post\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> @@ -1242,9 +1227,16 @@ void CameraDevice::streamProcessingComplete(Camera3RequestDescriptor::StreamBuff\n> \t\tstreamBuffer->stream->putBuffer(streamBuffer->internalBuffer);\n> \n> \tCamera3RequestDescriptor *request = streamBuffer->request;\n> -\tMutexLocker locker(request->streamsProcessMutex_);\n> \n> -\trequest->pendingStreamsToProcess_.erase(streamBuffer->stream);\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> +\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..0c402deb 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,9 +24,9 @@\n> #include \n> \n> #include \"camera_request.h\"\n> +#include \"post_processor.h\"\n> \n> class CameraDevice;\n> -class PostProcessor;\n> \n> class CameraStream\n> {\n> @@ -124,8 +128,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 +176,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 10A7FBF415\n\tfor ;\n\tTue, 26 Oct 2021 10:05:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4C3AD60237;\n\tTue, 26 Oct 2021 12:05:54 +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 5E11C60123\n\tfor ;\n\tTue, 26 Oct 2021 12:05:53 +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 C06E23F0;\n\tTue, 26 Oct 2021 12:05:52 +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=\"ZzuIGIt9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1635242753;\n\tbh=nXx1SWFwA2bE59Dk1KYC+OWuIp3Qk9dT2ScDWMnZmQA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ZzuIGIt9lVu0J4+zlQdXWy0cna8rhdya3sdFFOK+IdUvUMOS7hL5fHBBDYXs7Cagm\n\ttF6mzn/mmjbbW4INWd1RBEKkEf756zpCtFy3VOwzqiyuLF6lbUpGaSylMD9+Qx/F6Q\n\tP7r0DrsTrXOS+5tOKuQ4QLe749VjZNl2CjVbBrxs=","Date":"Tue, 26 Oct 2021 13:05:30 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20211026072148.164831-1-umang.jain@ideasonboard.com>\n\t<20211026072148.164831-8-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211026072148.164831-8-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v8 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\" "}}]