From patchwork Wed Oct 20 10:42:11 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Umang Jain X-Patchwork-Id: 14191 Return-Path: X-Original-To: parsemail@patchwork.libcamera.org Delivered-To: parsemail@patchwork.libcamera.org Received: from lancelot.ideasonboard.com (lancelot.ideasonboard.com [92.243.16.209]) by patchwork.libcamera.org (Postfix) with ESMTPS id 7BF09BF415 for ; Wed, 20 Oct 2021 10:42:27 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 3902E68F5E; Wed, 20 Oct 2021 12:42:27 +0200 (CEST) Authentication-Results: lancelot.ideasonboard.com; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="HS27UytA"; dkim-atps=neutral Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 26F0868F59 for ; Wed, 20 Oct 2021 12:42:25 +0200 (CEST) Received: from perceval.ideasonboard.com (unknown [103.251.226.185]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 284DA3F6; Wed, 20 Oct 2021 12:42:23 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1634726544; bh=ogsSJjRL0Tw4mCch/KO7enP8OtimsXDMgdkD3ke7dZc=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=HS27UytAgOXJv8LvpNsf0MFsl5aRTWrTpIM5S/nWLdcRsjJtVBUcMR7jrah4a1x4v NKbak4SWxsUCkJhv66nCk6SEbCfxR57yPH+PyaZGzAtbgX+tg4/6hrhcG316mhypwZ OH8oPhS8/6vVjRNXL6oDczBJhyr32N549YNdYSEs= From: Umang Jain To: libcamera-devel@lists.libcamera.org Date: Wed, 20 Oct 2021 16:12:11 +0530 Message-Id: <20211020104212.121743-4-umang.jain@ideasonboard.com> X-Mailer: git-send-email 2.31.1 In-Reply-To: <20211020104212.121743-1-umang.jain@ideasonboard.com> References: <20211020104212.121743-1-umang.jain@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH v5 3/4] android: post_processor: Make post processing async X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" Introduce a dedicated worker class derived from libcamera::Thread. The worker class maintains a queue for post-processing requests and waits for a post-processing request to become available. It will process them as per FIFO before de-queuing it from the queue. To get access to the source and destination buffers in the worker thread, we also need to save a pointer to them in the Camera3RequestDescriptor. This patch also implements a flush() for the PostProcessorWorker class which is responsible to purge post-processing requests queued up while a camera is stopping/flushing. It is hooked with CameraStream::flush(), which isn't used currently but will be used when we handle flush/stop scnearios in greater detail subsequently (in a different patchset). The libcamera request completion handler CameraDevice::requestComplete() assumes that the request that has just completed is at the front of the queue. Now that the post-processor runs asynchronously, this isn't true anymore, a request being post-processed will stay in the queue and a new libcamera request may complete. Remove that assumption, and use the request cookie to obtain the Camera3RequestDescriptor. Signed-off-by: Umang Jain Signed-off-by: Laurent Pinchart --- src/android/camera_device.cpp | 46 +++++---------- src/android/camera_request.h | 4 ++ src/android/camera_stream.cpp | 107 +++++++++++++++++++++++++++++++--- src/android/camera_stream.h | 38 +++++++++++- 4 files changed, 156 insertions(+), 39 deletions(-) diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index 541c2c81..b14416ce 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -1020,29 +1020,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques void CameraDevice::requestComplete(Request *request) { - Camera3RequestDescriptor *descriptor; - { - MutexLocker descriptorsLock(descriptorsMutex_); - ASSERT(!descriptors_.empty()); - descriptor = descriptors_.front().get(); - } - - if (descriptor->request_->cookie() != request->cookie()) { - /* - * \todo Clarify if the Camera has to be closed on - * ERROR_DEVICE. - */ - LOG(HAL, Error) - << "Out-of-order completion for request " - << utils::hex(request->cookie()); - - MutexLocker descriptorsLock(descriptorsMutex_); - descriptors_.pop(); - - notifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE); - - return; - } + Camera3RequestDescriptor *descriptor = + reinterpret_cast(request->cookie()); /* * Prepare the capture result for the Android camera stack. @@ -1120,12 +1099,7 @@ void CameraDevice::requestComplete(Request *request) bool needsPostProcessing = false; Camera3RequestDescriptor::Status processingStatus = Camera3RequestDescriptor::Status::Pending; - /* - * \todo Apply streamsProcessMutex_ when post-processing is adapted to run - * asynchronously. If we introduce the streamsProcessMutex_ right away, the - * lock will be held forever since it is synchronous at this point - * (see streamProcessingComplete). - */ + for (auto &buffer : descriptor->buffers_) { CameraStream *stream = buffer.stream; @@ -1145,7 +1119,13 @@ void CameraDevice::requestComplete(Request *request) buffer.internalBuffer = src; needsPostProcessing = true; - int ret = stream->process(*src, buffer, descriptor); + + int ret; + { + MutexLocker locker(descriptor->streamsProcessMutex_); + ret = stream->process(*src, buffer, descriptor); + } + if (ret) { setBufferStatus(stream, buffer, descriptor, Camera3RequestDescriptor::Status::Error); @@ -1156,6 +1136,12 @@ void CameraDevice::requestComplete(Request *request) if (needsPostProcessing) { if (processingStatus == Camera3RequestDescriptor::Status::Error) { descriptor->status_ = processingStatus; + /* + * \todo This is problematic when some streams are + * queued successfully, but some fail to get queued. + * We might end up with use-after-free situation for + * descriptor in streamProcessingComplete(). + */ sendCaptureResults(); } diff --git a/src/android/camera_request.h b/src/android/camera_request.h index 3a2774e0..85274414 100644 --- a/src/android/camera_request.h +++ b/src/android/camera_request.h @@ -18,6 +18,7 @@ #include +#include "camera_buffer.h" #include "camera_metadata.h" #include "camera_worker.h" @@ -39,6 +40,9 @@ public: int fence; Status status; libcamera::FrameBuffer *internalBuffer = nullptr; + std::unique_ptr destBuffer; + const libcamera::FrameBuffer *srcBuffer; + Camera3RequestDescriptor *request = nullptr; }; Camera3RequestDescriptor(libcamera::Camera *camera, diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp index b3cb06a2..a29ce33b 100644 --- a/src/android/camera_stream.cpp +++ b/src/android/camera_stream.cpp @@ -99,6 +99,7 @@ int CameraStream::configure() if (ret) return ret; + worker_ = std::make_unique(postProcessor_.get()); postProcessor_->processComplete.connect( this, [&](Camera3RequestDescriptor *request, PostProcessor::Status status) { Camera3RequestDescriptor::Status bufferStatus = @@ -110,6 +111,7 @@ int CameraStream::configure() cameraDevice_->streamProcessingComplete(this, request, bufferStatus); }); + worker_->start(); } if (type_ == Type::Internal) { @@ -179,23 +181,31 @@ int CameraStream::process(const FrameBuffer &source, if (!postProcessor_) return 0; - /* - * \todo Buffer mapping and processing should be moved to a - * separate thread. - */ const StreamConfiguration &output = configuration(); - CameraBuffer destBuffer(*dest.camera3Buffer, output.pixelFormat, - output.size, PROT_READ | PROT_WRITE); - if (!destBuffer.isValid()) { + dest.destBuffer = std::make_unique( + *dest.camera3Buffer, output.pixelFormat, output.size, PROT_READ | PROT_WRITE); + if (!dest.destBuffer->isValid()) { LOG(HAL, Error) << "Failed to create destination buffer"; return -EINVAL; } - postProcessor_->process(source, &destBuffer, request); + dest.srcBuffer = &source; + dest.request = request; + + /* Push the postProcessor request to the worker queue. */ + worker_->queueRequest(&dest); return 0; } +void CameraStream::flush() +{ + if (!postProcessor_) + return; + + worker_->flush(); +} + FrameBuffer *CameraStream::getBuffer() { if (!allocator_) @@ -223,3 +233,84 @@ void CameraStream::putBuffer(FrameBuffer *buffer) buffers_.push_back(buffer); } + +CameraStream::PostProcessorWorker::PostProcessorWorker(PostProcessor *postProcessor) + : postProcessor_(postProcessor) +{ +} + +CameraStream::PostProcessorWorker::~PostProcessorWorker() +{ + { + libcamera::MutexLocker lock(mutex_); + state_ = State::Stopped; + } + + cv_.notify_one(); + wait(); +} + +void CameraStream::PostProcessorWorker::start() +{ + { + libcamera::MutexLocker lock(mutex_); + state_ = State::Running; + } + + Thread::start(); +} + +void CameraStream::PostProcessorWorker::queueRequest(Camera3RequestDescriptor::StreamBuffer *dest) +{ + { + MutexLocker lock(mutex_); + ASSERT(state_ == State::Running); + requests_.push(dest); + } + + cv_.notify_one(); +} + +void CameraStream::PostProcessorWorker::run() +{ + MutexLocker locker(mutex_); + + while (1) { + cv_.wait(locker, [&] { + return state_ != State::Running || !requests_.empty(); + }); + + if (state_ != State::Running) + break; + + Camera3RequestDescriptor::StreamBuffer *stream = requests_.front(); + requests_.pop(); + locker.unlock(); + + postProcessor_->process(*stream->srcBuffer, stream->destBuffer.get(), + stream->request); + + locker.lock(); + } + + if (state_ == State::Flushing) { + while (!requests_.empty()) { + postProcessor_->processComplete.emit( + requests_.front()->request, + PostProcessor::Status::Error); + requests_.pop(); + } + + state_ = State::Stopped; + locker.unlock(); + } +} + +void CameraStream::PostProcessorWorker::flush() +{ + libcamera::MutexLocker lock(mutex_); + state_ = State::Flushing; + lock.unlock(); + + cv_.notify_one(); +} diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h index f242336e..64e32c77 100644 --- a/src/android/camera_stream.h +++ b/src/android/camera_stream.h @@ -7,12 +7,16 @@ #ifndef __ANDROID_CAMERA_STREAM_H__ #define __ANDROID_CAMERA_STREAM_H__ +#include #include #include +#include #include #include +#include + #include #include #include @@ -20,9 +24,9 @@ #include #include "camera_request.h" +#include "post_processor.h" class CameraDevice; -class PostProcessor; class CameraStream { @@ -126,8 +130,38 @@ public: Camera3RequestDescriptor *request); libcamera::FrameBuffer *getBuffer(); void putBuffer(libcamera::FrameBuffer *buffer); + void flush(); private: + class PostProcessorWorker : public libcamera::Thread + { + public: + enum class State { + Stopped, + Running, + Flushing, + }; + + PostProcessorWorker(PostProcessor *postProcessor); + ~PostProcessorWorker(); + + void start(); + void queueRequest(Camera3RequestDescriptor::StreamBuffer *request); + void flush(); + + protected: + void run() override; + + private: + PostProcessor *postProcessor_; + + libcamera::Mutex mutex_; + std::condition_variable cv_; + + std::queue requests_; + State state_; + }; + int waitFence(int fence); CameraDevice *const cameraDevice_; @@ -144,6 +178,8 @@ private: */ std::unique_ptr mutex_; std::unique_ptr postProcessor_; + + std::unique_ptr worker_; }; #endif /* __ANDROID_CAMERA_STREAM__ */