[{"id":20338,"web_url":"https://patchwork.libcamera.org/comment/20338/","msgid":"","date":"2021-10-21T01:33:48","subject":"Re: [libcamera-devel] [PATCH v5 3/4] 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 Wed, Oct 20, 2021 at 04:12:11PM +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\ns/a pointer/pointers/\n\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. It is hooked with\n> CameraStream::flush(), which isn't used currently but will be\n> used when we handle flush/stop scnearios 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 | 46 +++++----------\n> src/android/camera_request.h | 4 ++\n> src/android/camera_stream.cpp | 107 +++++++++++++++++++++++++++++++---\n> src/android/camera_stream.h | 38 +++++++++++-\n> 4 files changed, 156 insertions(+), 39 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 541c2c81..b14416ce 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -1020,29 +1020,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> @@ -1120,12 +1099,7 @@ void CameraDevice::requestComplete(Request *request)\n> \tbool needsPostProcessing = false;\n> \tCamera3RequestDescriptor::Status processingStatus =\n> \t\tCamera3RequestDescriptor::Status::Pending;\n> -\t/*\n> -\t * \\todo Apply streamsProcessMutex_ when post-processing is adapted to run\n> -\t * asynchronously. If we introduce the streamsProcessMutex_ right away, the\n> -\t * lock will be held forever since it is synchronous at this point\n> -\t * (see streamProcessingComplete).\n> -\t */\n> +\n> \tfor (auto &buffer : descriptor->buffers_) {\n> \t\tCameraStream *stream = buffer.stream;\n> \n> @@ -1145,7 +1119,13 @@ void CameraDevice::requestComplete(Request *request)\n> \t\t\tbuffer.internalBuffer = src;\n> \n> \t\tneedsPostProcessing = true;\n> -\t\tint ret = stream->process(*src, buffer, descriptor);\n> +\n> +\t\tint ret;\n> +\t\t{\n> +\t\t\tMutexLocker locker(descriptor->streamsProcessMutex_);\n\nIt may be possible (see my review of 2/4) to only lock access to the\ndata structures and not cover the process() call. Let's see how it turns\nout.\n\n> +\t\t\tret = stream->process(*src, buffer, descriptor);\n> +\t\t}\n> +\n> \t\tif (ret) {\n> \t\t\tsetBufferStatus(stream, buffer, descriptor,\n> \t\t\t\t\tCamera3RequestDescriptor::Status::Error);\n> @@ -1156,6 +1136,12 @@ void CameraDevice::requestComplete(Request *request)\n> \tif (needsPostProcessing) {\n> \t\tif (processingStatus == Camera3RequestDescriptor::Status::Error) {\n> \t\t\tdescriptor->status_ = processingStatus;\n> +\t\t\t/*\n> +\t\t\t * \\todo This is problematic when some streams are\n> +\t\t\t * queued successfully, but some fail to get queued.\n> +\t\t\t * We might end up with use-after-free situation for\n> +\t\t\t * descriptor in streamProcessingComplete().\n> +\t\t\t */\n\nHopefully this will be fixed in v6 of 2/4 :-)\n\n> \t\t\tsendCaptureResults();\n> \t\t}\n> \n> diff --git a/src/android/camera_request.h b/src/android/camera_request.h\n> index 3a2774e0..85274414 100644\n> --- a/src/android/camera_request.h\n> +++ b/src/android/camera_request.h\n> @@ -18,6 +18,7 @@\n> \n> #include \n> \n> +#include \"camera_buffer.h\"\n> #include \"camera_metadata.h\"\n> #include \"camera_worker.h\"\n> \n> @@ -39,6 +40,9 @@ public:\n> \t\tint fence;\n> \t\tStatus status;\n> \t\tlibcamera::FrameBuffer *internalBuffer = nullptr;\n> +\t\tstd::unique_ptr destBuffer;\n> +\t\tconst libcamera::FrameBuffer *srcBuffer;\n> +\t\tCamera3RequestDescriptor *request = nullptr;\n> \t};\n> \n> \tCamera3RequestDescriptor(libcamera::Camera *camera,\n> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> index b3cb06a2..a29ce33b 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 *request, PostProcessor::Status status) {\n> \t\t\t\tCamera3RequestDescriptor::Status bufferStatus =\n> @@ -110,6 +111,7 @@ int CameraStream::configure()\n> \t\t\t\tcameraDevice_->streamProcessingComplete(this, request,\n> \t\t\t\t\t\t\t\t\tbufferStatus);\n> \t\t\t});\n> +\t\tworker_->start();\n> \t}\n> \n> \tif (type_ == Type::Internal) {\n> @@ -179,23 +181,31 @@ int CameraStream::process(const FrameBuffer &source,\n> \tif (!postProcessor_)\n> \t\treturn 0;\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 destBuffer(*dest.camera3Buffer, output.pixelFormat,\n> -\t\t\t\toutput.size, PROT_READ | PROT_WRITE);\n> -\tif (!destBuffer.isValid()) {\n> +\tdest.destBuffer = std::make_unique(\n> +\t\t*dest.camera3Buffer, output.pixelFormat, output.size, PROT_READ | PROT_WRITE);\n\n\tdest.destBuffer = std::make_unique(\n\t\t*dest.camera3Buffer, output.pixelFormat, output.size,\n\t\tPROT_READ | PROT_WRITE);\n\n> +\tif (!dest.destBuffer->isValid()) {\n> \t\tLOG(HAL, Error) << \"Failed to create destination buffer\";\n> \t\treturn -EINVAL;\n> \t}\n> \n> -\tpostProcessor_->process(source, &destBuffer, request);\n> +\tdest.srcBuffer = &source;\n> +\tdest.request = request;\n> +\n> +\t/* Push the postProcessor request to the worker queue. */\n> +\tworker_->queueRequest(&dest);\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 +233,84 @@ 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\nCould you add a\n\n\t\tASSERT(state_ != State::Running);\n\nhere ? I'm worried that a future refactoring will call\nCameraStream::configure() multiple times, and that would result in the\nworker being start multiple times too.\n\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 *stream = requests_.front();\n> +\t\trequests_.pop();\n> +\t\tlocker.unlock();\n> +\n> +\t\tpostProcessor_->process(*stream->srcBuffer, stream->destBuffer.get(),\n> +\t\t\t\t\tstream->request);\n> +\n> +\t\tlocker.lock();\n> +\t}\n> +\n> +\tif (state_ == State::Flushing) {\n> +\t\twhile (!requests_.empty()) {\n> +\t\t\tpostProcessor_->processComplete.emit(\n> +\t\t\t\trequests_.front()->request,\n> +\t\t\t\tPostProcessor::Status::Error);\n\nI'm also a tiny bit concerned here that we're emitting the signal with\nthe lock held, while for requests that complete normally we don't. The\nbehaviour isn't consistent and could cause issues in the CameraStream or\nCameraDevice classes. I think it's harmless for now, but maybe we could\ndrop the lock around the emit(à call ? One option to avoid dropping and\ntaking the lock in every iteration would be\n\n\t\tstd::queue requests = std::move(requests_);\n\n\t\tlocker.unlock();\n\n\t\twhile (!requests.empty()) {\n\t\t\tpostProcessor_->processComplete.emit(\n\t\t\t\trequests.front()->request,\n\t\t\t\tPostProcessor::Status::Error);\n\t\t\trequests.pop();\n\t\t}\n\n\t\tlocker.lock();\n\nAnd now that I think about it, given that we now have a StreamBuffer\nobject, wouldn't it be simpler to pass the StreamBuffer instead of the\nCamera3RequestDescriptor to the signal ?\n\n> +\t\t\trequests_.pop();\n> +\t\t}\n> +\n> +\t\tstate_ = State::Stopped;\n> +\t\tlocker.unlock();\n\nYou can drop the unlock() call here.\n\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 f242336e..64e32c77 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> @@ -126,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::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_;\n> +\t};\n> +\n> \tint waitFence(int fence);\n> \n> \tCameraDevice *const cameraDevice_;\n> @@ -144,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 2F64BBDB1C\n\tfor ;\n\tThu, 21 Oct 2021 01:34:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7A22768F57;\n\tThu, 21 Oct 2021 03:34:09 +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 CAA2C68F57\n\tfor ;\n\tThu, 21 Oct 2021 03:34:07 +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 3AA8C93;\n\tThu, 21 Oct 2021 03:34:07 +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=\"dmR5YHpy\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634780047;\n\tbh=W+61U1P2ZSy+1Ja+o3PRSZiA5K8KdwcxMJ9//NMZ95U=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=dmR5YHpyBMHWTrkfXmXsVT/5wUdiQT10jP0VyFDWYxupvPpoEWI0U5Ymcqf6mYc9D\n\tlobfbRbfoXNCQxMKEC19YAF9sH8x4t9rRsIt8g3W5pYlThOvRTQfUyESawftLHspNa\n\trb3e21PTSv5kj3iSfWNb39fLfIqjiNN3RtPpQ1Tk=","Date":"Thu, 21 Oct 2021 04:33:48 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20211020104212.121743-1-umang.jain@ideasonboard.com>\n\t<20211020104212.121743-4-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20211020104212.121743-4-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v5 3/4] 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":20353,"web_url":"https://patchwork.libcamera.org/comment/20353/","msgid":"<8dfa9709-2e17-db3d-ab5d-70352e84c085@ideasonboard.com>","date":"2021-10-21T05:53:04","subject":"Re: [libcamera-devel] [PATCH v5 3/4] 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/21/21 7:03 AM, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> Thank you for the patch.\n>\n> On Wed, Oct 20, 2021 at 04:12:11PM +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> s/a pointer/pointers/\n>\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. It is hooked with\n>> CameraStream::flush(), which isn't used currently but will be\n>> used when we handle flush/stop scnearios 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 | 46 +++++----------\n>> src/android/camera_request.h | 4 ++\n>> src/android/camera_stream.cpp | 107 +++++++++++++++++++++++++++++++---\n>> src/android/camera_stream.h | 38 +++++++++++-\n>> 4 files changed, 156 insertions(+), 39 deletions(-)\n>>\n>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>> index 541c2c81..b14416ce 100644\n>> --- a/src/android/camera_device.cpp\n>> +++ b/src/android/camera_device.cpp\n>> @@ -1020,29 +1020,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>> @@ -1120,12 +1099,7 @@ void CameraDevice::requestComplete(Request *request)\n>> \tbool needsPostProcessing = false;\n>> \tCamera3RequestDescriptor::Status processingStatus =\n>> \t\tCamera3RequestDescriptor::Status::Pending;\n>> -\t/*\n>> -\t * \\todo Apply streamsProcessMutex_ when post-processing is adapted to run\n>> -\t * asynchronously. If we introduce the streamsProcessMutex_ right away, the\n>> -\t * lock will be held forever since it is synchronous at this point\n>> -\t * (see streamProcessingComplete).\n>> -\t */\n>> +\n>> \tfor (auto &buffer : descriptor->buffers_) {\n>> \t\tCameraStream *stream = buffer.stream;\n>> \n>> @@ -1145,7 +1119,13 @@ void CameraDevice::requestComplete(Request *request)\n>> \t\t\tbuffer.internalBuffer = src;\n>> \n>> \t\tneedsPostProcessing = true;\n>> -\t\tint ret = stream->process(*src, buffer, descriptor);\n>> +\n>> +\t\tint ret;\n>> +\t\t{\n>> +\t\t\tMutexLocker locker(descriptor->streamsProcessMutex_);\n> It may be possible (see my review of 2/4) to only lock access to the\n> data structures and not cover the process() call. Let's see how it turns\n> out.\n\n\nYes, I need to draft the code and then only I'll be able to confirm. My \ndeep-code visual skills  are kinda limited.\n\n>\n>> +\t\t\tret = stream->process(*src, buffer, descriptor);\n>> +\t\t}\n>> +\n>> \t\tif (ret) {\n>> \t\t\tsetBufferStatus(stream, buffer, descriptor,\n>> \t\t\t\t\tCamera3RequestDescriptor::Status::Error);\n>> @@ -1156,6 +1136,12 @@ void CameraDevice::requestComplete(Request *request)\n>> \tif (needsPostProcessing) {\n>> \t\tif (processingStatus == Camera3RequestDescriptor::Status::Error) {\n>> \t\t\tdescriptor->status_ = processingStatus;\n>> +\t\t\t/*\n>> +\t\t\t * \\todo This is problematic when some streams are\n>> +\t\t\t * queued successfully, but some fail to get queued.\n>> +\t\t\t * We might end up with use-after-free situation for\n>> +\t\t\t * descriptor in streamProcessingComplete().\n>> +\t\t\t */\n> Hopefully this will be fixed in v6 of 2/4 :-)\n>\n>> \t\t\tsendCaptureResults();\n>> \t\t}\n>> \n>> diff --git a/src/android/camera_request.h b/src/android/camera_request.h\n>> index 3a2774e0..85274414 100644\n>> --- a/src/android/camera_request.h\n>> +++ b/src/android/camera_request.h\n>> @@ -18,6 +18,7 @@\n>> \n>> #include \n>> \n>> +#include \"camera_buffer.h\"\n>> #include \"camera_metadata.h\"\n>> #include \"camera_worker.h\"\n>> \n>> @@ -39,6 +40,9 @@ public:\n>> \t\tint fence;\n>> \t\tStatus status;\n>> \t\tlibcamera::FrameBuffer *internalBuffer = nullptr;\n>> +\t\tstd::unique_ptr destBuffer;\n>> +\t\tconst libcamera::FrameBuffer *srcBuffer;\n>> +\t\tCamera3RequestDescriptor *request = nullptr;\n>> \t};\n>> \n>> \tCamera3RequestDescriptor(libcamera::Camera *camera,\n>> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n>> index b3cb06a2..a29ce33b 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 *request, PostProcessor::Status status) {\n>> \t\t\t\tCamera3RequestDescriptor::Status bufferStatus =\n>> @@ -110,6 +111,7 @@ int CameraStream::configure()\n>> \t\t\t\tcameraDevice_->streamProcessingComplete(this, request,\n>> \t\t\t\t\t\t\t\t\tbufferStatus);\n>> \t\t\t});\n>> +\t\tworker_->start();\n>> \t}\n>> \n>> \tif (type_ == Type::Internal) {\n>> @@ -179,23 +181,31 @@ int CameraStream::process(const FrameBuffer &source,\n>> \tif (!postProcessor_)\n>> \t\treturn 0;\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 destBuffer(*dest.camera3Buffer, output.pixelFormat,\n>> -\t\t\t\toutput.size, PROT_READ | PROT_WRITE);\n>> -\tif (!destBuffer.isValid()) {\n>> +\tdest.destBuffer = std::make_unique(\n>> +\t\t*dest.camera3Buffer, output.pixelFormat, output.size, PROT_READ | PROT_WRITE);\n> \tdest.destBuffer = std::make_unique(\n> \t\t*dest.camera3Buffer, output.pixelFormat, output.size,\n> \t\tPROT_READ | PROT_WRITE);\n>\n>> +\tif (!dest.destBuffer->isValid()) {\n>> \t\tLOG(HAL, Error) << \"Failed to create destination buffer\";\n>> \t\treturn -EINVAL;\n>> \t}\n>> \n>> -\tpostProcessor_->process(source, &destBuffer, request);\n>> +\tdest.srcBuffer = &source;\n>> +\tdest.request = request;\n>> +\n>> +\t/* Push the postProcessor request to the worker queue. */\n>> +\tworker_->queueRequest(&dest);\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 +233,84 @@ 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> Could you add a\n>\n> \t\tASSERT(state_ != State::Running);\n>\n> here ? I'm worried that a future refactoring will call\n> CameraStream::configure() multiple times, and that would result in the\n> worker being start multiple times too.\n\n\nAck\n\n>\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 *stream = requests_.front();\n>> +\t\trequests_.pop();\n>> +\t\tlocker.unlock();\n>> +\n>> +\t\tpostProcessor_->process(*stream->srcBuffer, stream->destBuffer.get(),\n>> +\t\t\t\t\tstream->request);\n>> +\n>> +\t\tlocker.lock();\n>> +\t}\n>> +\n>> +\tif (state_ == State::Flushing) {\n>> +\t\twhile (!requests_.empty()) {\n>> +\t\t\tpostProcessor_->processComplete.emit(\n>> +\t\t\t\trequests_.front()->request,\n>> +\t\t\t\tPostProcessor::Status::Error);\n> I'm also a tiny bit concerned here that we're emitting the signal with\n> the lock held, while for requests that complete normally we don't. The\n> behaviour isn't consistent and could cause issues in the CameraStream or\n> CameraDevice classes. I think it's harmless for now, but maybe we could\n> drop the lock around the emit(à call ? One option to avoid dropping and\n> taking the lock in every iteration would be\n>\n> \t\tstd::queue requests = std::move(requests_);\n>\n> \t\tlocker.unlock();\n>\n> \t\twhile (!requests.empty()) {\n> \t\t\tpostProcessor_->processComplete.emit(\n> \t\t\t\trequests.front()->request,\n> \t\t\t\tPostProcessor::Status::Error);\n> \t\t\trequests.pop();\n> \t\t}\n>\n> \t\tlocker.lock();\n>\n> And now that I think about it, given that we now have a StreamBuffer\n> object, wouldn't it be simpler to pass the StreamBuffer instead of the\n> Camera3RequestDescriptor to the signal ?\n\n\nYesteday, Jacopo might have suggested the same. I'll try to accomodate this.\n\n>> +\t\t}\n>> +\n>> +\t\tstate_ = State::Stopped;\n>> +\t\tlocker.unlock();\n> You can drop the unlock() call here.\n\n\nAck. Shouldn't be here\n\n>\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 f242336e..64e32c77 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>> @@ -126,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::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_;\n>> +\t};\n>> +\n>> \tint waitFence(int fence);\n>> \n>> \tCameraDevice *const cameraDevice_;\n>> @@ -144,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 94C88BDB1C\n\tfor ;\n\tThu, 21 Oct 2021 05:53:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F0F9868F5B;\n\tThu, 21 Oct 2021 07:53:09 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0D6D6604FE\n\tfor ;\n\tThu, 21 Oct 2021 07:53:09 +0200 (CEST)","from [192.168.1.106] (unknown [103.251.226.185])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DCFF32BA;\n\tThu, 21 Oct 2021 07:53:07 +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=\"SEWVb1Eh\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634795588;\n\tbh=d/QxxQ+Ov6i/74Yww8fYyqsx65AN3OCov4pO6WX2vuQ=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=SEWVb1Eh79U6+hkcki0yKG1aSvOdNHq9ODzK8zeaVdNaQOmbem1P5GoXzdKntaAIi\n\tM9N8XY+q5cRvLKOlm5jhIJe8eIscLwiSVFclVI1ujdF6g7XQ2jmU6+SiFmki44wKtE\n\t+zzgx2cPqNfvZDss0z9E746prezAjeN+KRibOils=","To":"Laurent Pinchart ","References":"<20211020104212.121743-1-umang.jain@ideasonboard.com>\n\t<20211020104212.121743-4-umang.jain@ideasonboard.com>\n\t","From":"Umang Jain ","Message-ID":"<8dfa9709-2e17-db3d-ab5d-70352e84c085@ideasonboard.com>","Date":"Thu, 21 Oct 2021 11:23:04 +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 v5 3/4] 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":20356,"web_url":"https://patchwork.libcamera.org/comment/20356/","msgid":"<1a22527c-793e-7da9-c60c-33ae4ae6a158@ideasonboard.com>","date":"2021-10-21T07:37:50","subject":"Re: [libcamera-devel] [PATCH v5 3/4] 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":"Hello,\n\nOn 10/20/21 4:12 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. It is hooked with\n> CameraStream::flush(), which isn't used currently but will be\n> used when we handle flush/stop scnearios 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 | 46 +++++----------\n> src/android/camera_request.h | 4 ++\n> src/android/camera_stream.cpp | 107 +++++++++++++++++++++++++++++++---\n> src/android/camera_stream.h | 38 +++++++++++-\n> 4 files changed, 156 insertions(+), 39 deletions(-)\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 541c2c81..b14416ce 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -1020,29 +1020,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> @@ -1120,12 +1099,7 @@ void CameraDevice::requestComplete(Request *request)\n> \tbool needsPostProcessing = false;\n> \tCamera3RequestDescriptor::Status processingStatus =\n> \t\tCamera3RequestDescriptor::Status::Pending;\n> -\t/*\n> -\t * \\todo Apply streamsProcessMutex_ when post-processing is adapted to run\n> -\t * asynchronously. If we introduce the streamsProcessMutex_ right away, the\n> -\t * lock will be held forever since it is synchronous at this point\n> -\t * (see streamProcessingComplete).\n> -\t */\n> +\n> \tfor (auto &buffer : descriptor->buffers_) {\n> \t\tCameraStream *stream = buffer.stream;\n> \n> @@ -1145,7 +1119,13 @@ void CameraDevice::requestComplete(Request *request)\n> \t\t\tbuffer.internalBuffer = src;\n> \n> \t\tneedsPostProcessing = true;\n> -\t\tint ret = stream->process(*src, buffer, descriptor);\n> +\n> +\t\tint ret;\n> +\t\t{\n> +\t\t\tMutexLocker locker(descriptor->streamsProcessMutex_);\n> +\t\t\tret = stream->process(*src, buffer, descriptor);\n> +\t\t}\n> +\n> \t\tif (ret) {\n> \t\t\tsetBufferStatus(stream, buffer, descriptor,\n> \t\t\t\t\tCamera3RequestDescriptor::Status::Error);\n> @@ -1156,6 +1136,12 @@ void CameraDevice::requestComplete(Request *request)\n> \tif (needsPostProcessing) {\n> \t\tif (processingStatus == Camera3RequestDescriptor::Status::Error) {\n> \t\t\tdescriptor->status_ = processingStatus;\n> +\t\t\t/*\n> +\t\t\t * \\todo This is problematic when some streams are\n> +\t\t\t * queued successfully, but some fail to get queued.\n> +\t\t\t * We might end up with use-after-free situation for\n> +\t\t\t * descriptor in streamProcessingComplete().\n> +\t\t\t */\n> \t\t\tsendCaptureResults();\n> \t\t}\n> \n> diff --git a/src/android/camera_request.h b/src/android/camera_request.h\n> index 3a2774e0..85274414 100644\n> --- a/src/android/camera_request.h\n> +++ b/src/android/camera_request.h\n> @@ -18,6 +18,7 @@\n> \n> #include \n> \n> +#include \"camera_buffer.h\"\n> #include \"camera_metadata.h\"\n> #include \"camera_worker.h\"\n> \n> @@ -39,6 +40,9 @@ public:\n> \t\tint fence;\n> \t\tStatus status;\n> \t\tlibcamera::FrameBuffer *internalBuffer = nullptr;\n> +\t\tstd::unique_ptr destBuffer;\n> +\t\tconst libcamera::FrameBuffer *srcBuffer;\n> +\t\tCamera3RequestDescriptor *request = nullptr;\n> \t};\n> \n> \tCamera3RequestDescriptor(libcamera::Camera *camera,\n> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> index b3cb06a2..a29ce33b 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 *request, PostProcessor::Status status) {\n> \t\t\t\tCamera3RequestDescriptor::Status bufferStatus =\n> @@ -110,6 +111,7 @@ int CameraStream::configure()\n> \t\t\t\tcameraDevice_->streamProcessingComplete(this, request,\n> \t\t\t\t\t\t\t\t\tbufferStatus);\n> \t\t\t});\n> +\t\tworker_->start();\n> \t}\n> \n> \tif (type_ == Type::Internal) {\n> @@ -179,23 +181,31 @@ int CameraStream::process(const FrameBuffer &source,\n> \tif (!postProcessor_)\n> \t\treturn 0;\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 destBuffer(*dest.camera3Buffer, output.pixelFormat,\n> -\t\t\t\toutput.size, PROT_READ | PROT_WRITE);\n> -\tif (!destBuffer.isValid()) {\n> +\tdest.destBuffer = std::make_unique(\n> +\t\t*dest.camera3Buffer, output.pixelFormat, output.size, PROT_READ | PROT_WRITE);\n> +\tif (!dest.destBuffer->isValid()) {\n> \t\tLOG(HAL, Error) << \"Failed to create destination buffer\";\n> \t\treturn -EINVAL;\n> \t}\n> \n> -\tpostProcessor_->process(source, &destBuffer, request);\n> +\tdest.srcBuffer = &source;\n> +\tdest.request = request;\n> +\n> +\t/* Push the postProcessor request to the worker queue. */\n> +\tworker_->queueRequest(&dest);\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 +233,84 @@ 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::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 *stream = requests_.front();\n> +\t\trequests_.pop();\n> +\t\tlocker.unlock();\n> +\n> +\t\tpostProcessor_->process(*stream->srcBuffer, stream->destBuffer.get(),\n> +\t\t\t\t\tstream->request);\n\n\nAs I have received in reviews, processComplete should emit a \nCamera3RequestDescriptor::StreamBuffer pointer, instead of having \nemitted currently a pointer to Camera3RequestDescriptor\n\nNow looking postProcessor_->process() call above, I think I should \nfurther migrate its signature to:\n\n     PostProcessor::process(Camera3RequestDescriptor::StreamBuffer *stream)\n\nand all the three elements can be accessed from there.\n\nLaurent, do you agree with the design decision?\n\n> +\n> +\t\tlocker.lock();\n> +\t}\n> +\n> +\tif (state_ == State::Flushing) {\n> +\t\twhile (!requests_.empty()) {\n> +\t\t\tpostProcessor_->processComplete.emit(\n> +\t\t\t\trequests_.front()->request,\n> +\t\t\t\tPostProcessor::Status::Error);\n> +\t\t\trequests_.pop();\n> +\t\t}\n> +\n> +\t\tstate_ = State::Stopped;\n> +\t\tlocker.unlock();\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 f242336e..64e32c77 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> @@ -126,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::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_;\n> +\t};\n> +\n> \tint waitFence(int fence);\n> \n> \tCameraDevice *const cameraDevice_;\n> @@ -144,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 EFBB5BF415\n\tfor ;\n\tThu, 21 Oct 2021 07:37:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 550C768F58;\n\tThu, 21 Oct 2021 09:37:58 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CB24D60129\n\tfor ;\n\tThu, 21 Oct 2021 09:37:56 +0200 (CEST)","from [IPv6:2409:4041:2e8d:6fbd:274d:ab38:fc35:b0cb] (unknown\n\t[IPv6:2409:4041:2e8d:6fbd:274d:ab38:fc35:b0cb])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CD0822BA;\n\tThu, 21 Oct 2021 09:37:54 +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=\"qtS0foX9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634801876;\n\tbh=+szCCnFIj9A19PI7LXaF9md7/203u6Pmtf62NYBDK4M=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=qtS0foX9TqSzWbycYgETRaTjN8wSgaGgAm0aX+zC3md5lGswYpVGhwjkm4LN0EacM\n\tlD9mH16kz9ZrC6cQYyLR0pp6loPmgowOErkOfEHaqT7REMwqmQPpLnRSO9wID7NVyT\n\t1EwGL2PvuOA719ZCTrvnhycwe5Qdb4LeTRQu4/h4=","To":"libcamera-devel@lists.libcamera.org","References":"<20211020104212.121743-1-umang.jain@ideasonboard.com>\n\t<20211020104212.121743-4-umang.jain@ideasonboard.com>","From":"Umang Jain ","Message-ID":"<1a22527c-793e-7da9-c60c-33ae4ae6a158@ideasonboard.com>","Date":"Thu, 21 Oct 2021 13:07:50 +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":"<20211020104212.121743-4-umang.jain@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v5 3/4] 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":20358,"web_url":"https://patchwork.libcamera.org/comment/20358/","msgid":"","date":"2021-10-21T13:10:11","subject":"Re: [libcamera-devel] [PATCH v5 3/4] 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 Thu, Oct 21, 2021 at 01:07:50PM +0530, Umang Jain wrote:\n> On 10/20/21 4:12 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. It is hooked with\n> > CameraStream::flush(), which isn't used currently but will be\n> > used when we handle flush/stop scnearios 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 | 46 +++++----------\n> > src/android/camera_request.h | 4 ++\n> > src/android/camera_stream.cpp | 107 +++++++++++++++++++++++++++++++---\n> > src/android/camera_stream.h | 38 +++++++++++-\n> > 4 files changed, 156 insertions(+), 39 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index 541c2c81..b14416ce 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -1020,29 +1020,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> > @@ -1120,12 +1099,7 @@ void CameraDevice::requestComplete(Request *request)\n> > \tbool needsPostProcessing = false;\n> > \tCamera3RequestDescriptor::Status processingStatus =\n> > \t\tCamera3RequestDescriptor::Status::Pending;\n> > -\t/*\n> > -\t * \\todo Apply streamsProcessMutex_ when post-processing is adapted to run\n> > -\t * asynchronously. If we introduce the streamsProcessMutex_ right away, the\n> > -\t * lock will be held forever since it is synchronous at this point\n> > -\t * (see streamProcessingComplete).\n> > -\t */\n> > +\n> > \tfor (auto &buffer : descriptor->buffers_) {\n> > \t\tCameraStream *stream = buffer.stream;\n> > \n> > @@ -1145,7 +1119,13 @@ void CameraDevice::requestComplete(Request *request)\n> > \t\t\tbuffer.internalBuffer = src;\n> > \n> > \t\tneedsPostProcessing = true;\n> > -\t\tint ret = stream->process(*src, buffer, descriptor);\n> > +\n> > +\t\tint ret;\n> > +\t\t{\n> > +\t\t\tMutexLocker locker(descriptor->streamsProcessMutex_);\n> > +\t\t\tret = stream->process(*src, buffer, descriptor);\n> > +\t\t}\n> > +\n> > \t\tif (ret) {\n> > \t\t\tsetBufferStatus(stream, buffer, descriptor,\n> > \t\t\t\t\tCamera3RequestDescriptor::Status::Error);\n> > @@ -1156,6 +1136,12 @@ void CameraDevice::requestComplete(Request *request)\n> > \tif (needsPostProcessing) {\n> > \t\tif (processingStatus == Camera3RequestDescriptor::Status::Error) {\n> > \t\t\tdescriptor->status_ = processingStatus;\n> > +\t\t\t/*\n> > +\t\t\t * \\todo This is problematic when some streams are\n> > +\t\t\t * queued successfully, but some fail to get queued.\n> > +\t\t\t * We might end up with use-after-free situation for\n> > +\t\t\t * descriptor in streamProcessingComplete().\n> > +\t\t\t */\n> > \t\t\tsendCaptureResults();\n> > \t\t}\n> > \n> > diff --git a/src/android/camera_request.h b/src/android/camera_request.h\n> > index 3a2774e0..85274414 100644\n> > --- a/src/android/camera_request.h\n> > +++ b/src/android/camera_request.h\n> > @@ -18,6 +18,7 @@\n> > \n> > #include \n> > \n> > +#include \"camera_buffer.h\"\n> > #include \"camera_metadata.h\"\n> > #include \"camera_worker.h\"\n> > \n> > @@ -39,6 +40,9 @@ public:\n> > \t\tint fence;\n> > \t\tStatus status;\n> > \t\tlibcamera::FrameBuffer *internalBuffer = nullptr;\n> > +\t\tstd::unique_ptr destBuffer;\n> > +\t\tconst libcamera::FrameBuffer *srcBuffer;\n> > +\t\tCamera3RequestDescriptor *request = nullptr;\n> > \t};\n> > \n> > \tCamera3RequestDescriptor(libcamera::Camera *camera,\n> > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> > index b3cb06a2..a29ce33b 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 *request, PostProcessor::Status status) {\n> > \t\t\t\tCamera3RequestDescriptor::Status bufferStatus =\n> > @@ -110,6 +111,7 @@ int CameraStream::configure()\n> > \t\t\t\tcameraDevice_->streamProcessingComplete(this, request,\n> > \t\t\t\t\t\t\t\t\tbufferStatus);\n> > \t\t\t});\n> > +\t\tworker_->start();\n> > \t}\n> > \n> > \tif (type_ == Type::Internal) {\n> > @@ -179,23 +181,31 @@ int CameraStream::process(const FrameBuffer &source,\n> > \tif (!postProcessor_)\n> > \t\treturn 0;\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 destBuffer(*dest.camera3Buffer, output.pixelFormat,\n> > -\t\t\t\toutput.size, PROT_READ | PROT_WRITE);\n> > -\tif (!destBuffer.isValid()) {\n> > +\tdest.destBuffer = std::make_unique(\n> > +\t\t*dest.camera3Buffer, output.pixelFormat, output.size, PROT_READ | PROT_WRITE);\n> > +\tif (!dest.destBuffer->isValid()) {\n> > \t\tLOG(HAL, Error) << \"Failed to create destination buffer\";\n> > \t\treturn -EINVAL;\n> > \t}\n> > \n> > -\tpostProcessor_->process(source, &destBuffer, request);\n> > +\tdest.srcBuffer = &source;\n> > +\tdest.request = request;\n> > +\n> > +\t/* Push the postProcessor request to the worker queue. */\n> > +\tworker_->queueRequest(&dest);\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 +233,84 @@ 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::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 *stream = requests_.front();\n> > +\t\trequests_.pop();\n> > +\t\tlocker.unlock();\n> > +\n> > +\t\tpostProcessor_->process(*stream->srcBuffer, stream->destBuffer.get(),\n> > +\t\t\t\t\tstream->request);\n> \n> As I have received in reviews, processComplete should emit a \n> Camera3RequestDescriptor::StreamBuffer pointer, instead of having \n> emitted currently a pointer to Camera3RequestDescriptor\n> \n> Now looking postProcessor_->process() call above, I think I should \n> further migrate its signature to:\n> \n>     PostProcessor::process(Camera3RequestDescriptor::StreamBuffer *stream)\n> \n> and all the three elements can be accessed from there.\n> \n> Laurent, do you agree with the design decision?\n\nYes, that looks good to me. Maybe the variable could also be renamed to\nstreamBuffer here, to avoid confusing it with a stream.\n\n> > +\n> > +\t\tlocker.lock();\n> > +\t}\n> > +\n> > +\tif (state_ == State::Flushing) {\n> > +\t\twhile (!requests_.empty()) {\n> > +\t\t\tpostProcessor_->processComplete.emit(\n> > +\t\t\t\trequests_.front()->request,\n> > +\t\t\t\tPostProcessor::Status::Error);\n> > +\t\t\trequests_.pop();\n> > +\t\t}\n> > +\n> > +\t\tstate_ = State::Stopped;\n> > +\t\tlocker.unlock();\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 f242336e..64e32c77 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> > @@ -126,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::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_;\n> > +\t};\n> > +\n> > \tint waitFence(int fence);\n> > \n> > \tCameraDevice *const cameraDevice_;\n> > @@ -144,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 30407BDB1C\n\tfor ;\n\tThu, 21 Oct 2021 13:10:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9B0E568F58;\n\tThu, 21 Oct 2021 15:10: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 E068E604FE\n\tfor ;\n\tThu, 21 Oct 2021 15:10:30 +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 610BB8B6;\n\tThu, 21 Oct 2021 15:10:30 +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=\"YwvhC9o8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634821830;\n\tbh=0T1bHGn5AFUuoKHoizKbvSOarl5jJzNKbkevBxGr05w=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=YwvhC9o87zwiVZuAnlHS4Ty2ICta4wakLKKDovwZ8r1U18zJHUlMaCkl52K+fEMxX\n\t8dwxQrU9a9vyJOX9b+7YRUJQWw3NpfhZYvyIzFeJNkOhScpukZ42o8YHufmzFBa54D\n\t5IeKfy2FDhzihXNNdCQHHxtqIrF/xq/oAcdR3DyU=","Date":"Thu, 21 Oct 2021 16:10:11 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20211020104212.121743-1-umang.jain@ideasonboard.com>\n\t<20211020104212.121743-4-umang.jain@ideasonboard.com>\n\t<1a22527c-793e-7da9-c60c-33ae4ae6a158@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<1a22527c-793e-7da9-c60c-33ae4ae6a158@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v5 3/4] 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\" "}}]