From patchwork Wed Oct 20 10:42:09 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Umang Jain X-Patchwork-Id: 14189 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 AE842BF415 for ; Wed, 20 Oct 2021 10:42:23 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 3C3A168F5C; Wed, 20 Oct 2021 12:42:23 +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="sKOZbJlI"; 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 450D568F58 for ; Wed, 20 Oct 2021 12:42:21 +0200 (CEST) Received: from perceval.ideasonboard.com (unknown [103.251.226.185]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 40DE62A5; Wed, 20 Oct 2021 12:42:20 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1634726541; bh=1n5eo1ItDKoPwgz5CdzMWKGu7I6QstwOrPYxpoX+msc=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=sKOZbJlIMLwmUqFzjrLxI6fa11HD4iGK7RJpkVK95eVBJJGhnmWCfRCxWU7AFTcbL FP4D9S565JnugL6JV7i+3N/2A2xWzq63aFyXAzCwB0b79RIjaaRnTP/lHZ+dTG0Ych 8/akaMJDIro4O4dP9udRQi2UD5kdlfaa4fiARmmI= From: Umang Jain To: libcamera-devel@lists.libcamera.org Date: Wed, 20 Oct 2021 16:12:09 +0530 Message-Id: <20211020104212.121743-2-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 1/4] android: Notify post processing completion via a signal 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" Notify that the post processing for a request has been completed, via a signal. A pointer to the descriptor which is tracking the capture request is emitted along with the status of post processed buffer. The function CameraDevice::streamProcessingComplete() will finally set the status on the request descriptor and send capture results back to the framework accordingly. We also need to save a pointer to any internal buffers that might have been allocated by CameraStream. The buffer should be returned back to CameraStream just before capture results are sent. A streamProcessMutex_ has been introduced here itself, which will be applicable to guard access to descriptor->buffers_ when post-processing is moved to be asynchronous in subsequent commits. Signed-off-by: Umang Jain --- src/android/camera_device.cpp | 92 +++++++++++++++++++++--- src/android/camera_device.h | 7 ++ src/android/camera_request.h | 4 ++ src/android/camera_stream.cpp | 13 ++++ src/android/jpeg/post_processor_jpeg.cpp | 2 + src/android/post_processor.h | 9 +++ src/android/yuv/post_processor_yuv.cpp | 10 ++- 7 files changed, 125 insertions(+), 12 deletions(-) diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index 806b4090..541c2c81 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -1117,6 +1117,15 @@ void CameraDevice::requestComplete(Request *request) } /* Handle post-processing. */ + 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; @@ -1132,22 +1141,27 @@ void CameraDevice::requestComplete(Request *request) continue; } - int ret = stream->process(*src, buffer, descriptor); - - /* - * Return the FrameBuffer to the CameraStream now that we're - * done processing it. - */ if (stream->type() == CameraStream::Type::Internal) - stream->putBuffer(src); + buffer.internalBuffer = src; + needsPostProcessing = true; + int ret = stream->process(*src, buffer, descriptor); if (ret) { - buffer.status = Camera3RequestDescriptor::Status::Error; - notifyError(descriptor->frameNumber_, stream->camera3Stream(), - CAMERA3_MSG_ERROR_BUFFER); + setBufferStatus(stream, buffer, descriptor, + Camera3RequestDescriptor::Status::Error); + processingStatus = Camera3RequestDescriptor::Status::Error; } } + if (needsPostProcessing) { + if (processingStatus == Camera3RequestDescriptor::Status::Error) { + descriptor->status_ = processingStatus; + sendCaptureResults(); + } + + return; + } + descriptor->status_ = Camera3RequestDescriptor::Status::Success; sendCaptureResults(); } @@ -1206,6 +1220,64 @@ void CameraDevice::sendCaptureResults() } } +void CameraDevice::setBufferStatus(CameraStream *cameraStream, + Camera3RequestDescriptor::StreamBuffer &buffer, + Camera3RequestDescriptor *request, + Camera3RequestDescriptor::Status status) +{ + /* + * Return the FrameBuffer to the CameraStream now that we're + * done processing it. + */ + if (cameraStream->type() == CameraStream::Type::Internal) + cameraStream->putBuffer(buffer.internalBuffer); + + buffer.status = status; + if (status != Camera3RequestDescriptor::Status::Success) + notifyError(request->frameNumber_, buffer.stream->camera3Stream(), + CAMERA3_MSG_ERROR_BUFFER); +} + +void CameraDevice::streamProcessingComplete(CameraStream *cameraStream, + Camera3RequestDescriptor *request, + Camera3RequestDescriptor::Status status) +{ + MutexLocker locker(request->streamsProcessMutex_); + for (auto &buffer : request->buffers_) { + if (buffer.stream != cameraStream) + continue; + + setBufferStatus(cameraStream, buffer, request, status); + } + + bool hasPostProcessingErrors = false; + for (auto &buffer : request->buffers_) { + if (cameraStream->type() == CameraStream::Type::Direct) + continue; + + /* + * Other eligible buffers might be waiting to get post-processed. + * So wait for their turn before sendCaptureResults() for the + * descriptor. + */ + if (buffer.status == Camera3RequestDescriptor::Status::Pending) + return; + + if (!hasPostProcessingErrors && + buffer.status == Camera3RequestDescriptor::Status::Error) + hasPostProcessingErrors = true; + } + + if (hasPostProcessingErrors) + request->status_ = Camera3RequestDescriptor::Status::Error; + else + request->status_ = Camera3RequestDescriptor::Status::Success; + + locker.unlock(); + + sendCaptureResults(); +} + std::string CameraDevice::logPrefix() const { return "'" + camera_->id() + "'"; diff --git a/src/android/camera_device.h b/src/android/camera_device.h index 863cf414..1ef933da 100644 --- a/src/android/camera_device.h +++ b/src/android/camera_device.h @@ -66,6 +66,9 @@ public: int configureStreams(camera3_stream_configuration_t *stream_list); int processCaptureRequest(camera3_capture_request_t *request); void requestComplete(libcamera::Request *request); + void streamProcessingComplete(CameraStream *cameraStream, + Camera3RequestDescriptor *request, + Camera3RequestDescriptor::Status status); protected: std::string logPrefix() const override; @@ -94,6 +97,10 @@ private: camera3_error_msg_code code) const; int processControls(Camera3RequestDescriptor *descriptor); void sendCaptureResults(); + void setBufferStatus(CameraStream *cameraStream, + Camera3RequestDescriptor::StreamBuffer &buffer, + Camera3RequestDescriptor *request, + Camera3RequestDescriptor::Status status); std::unique_ptr getResultMetadata( const Camera3RequestDescriptor &descriptor) const; diff --git a/src/android/camera_request.h b/src/android/camera_request.h index 05dabf89..3a2774e0 100644 --- a/src/android/camera_request.h +++ b/src/android/camera_request.h @@ -8,6 +8,7 @@ #define __ANDROID_CAMERA_REQUEST_H__ #include +#include #include #include @@ -37,6 +38,7 @@ public: std::unique_ptr frameBuffer; int fence; Status status; + libcamera::FrameBuffer *internalBuffer = nullptr; }; Camera3RequestDescriptor(libcamera::Camera *camera, @@ -47,6 +49,8 @@ public: uint32_t frameNumber_ = 0; + /* Protects buffers_ for post-processing streams. */ + std::mutex streamsProcessMutex_; std::vector buffers_; CameraMetadata settings_; diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp index f44a2717..04cbef8c 100644 --- a/src/android/camera_stream.cpp +++ b/src/android/camera_stream.cpp @@ -22,6 +22,7 @@ #include "camera_capabilities.h" #include "camera_device.h" #include "camera_metadata.h" +#include "post_processor.h" using namespace libcamera; @@ -97,6 +98,18 @@ int CameraStream::configure() int ret = postProcessor_->configure(configuration(), output); if (ret) return ret; + + postProcessor_->processComplete.connect( + this, [&](Camera3RequestDescriptor *request, PostProcessor::Status status) { + Camera3RequestDescriptor::Status bufferStatus = + Camera3RequestDescriptor::Status::Error; + + if (status == PostProcessor::Status::Success) + bufferStatus = Camera3RequestDescriptor::Status::Success; + + cameraDevice_->streamProcessingComplete(this, request, + bufferStatus); + }); } if (type_ == Type::Internal) { diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp index 699576ef..a001fede 100644 --- a/src/android/jpeg/post_processor_jpeg.cpp +++ b/src/android/jpeg/post_processor_jpeg.cpp @@ -198,6 +198,7 @@ int PostProcessorJpeg::process(const FrameBuffer &source, exif.data(), quality); if (jpeg_size < 0) { LOG(JPEG, Error) << "Failed to encode stream image"; + processComplete.emit(request, PostProcessor::Status::Error); return jpeg_size; } @@ -211,6 +212,7 @@ int PostProcessorJpeg::process(const FrameBuffer &source, /* Update the JPEG result Metadata. */ resultMetadata->addEntry(ANDROID_JPEG_SIZE, jpeg_size); + processComplete.emit(request, PostProcessor::Status::Success); return 0; } diff --git a/src/android/post_processor.h b/src/android/post_processor.h index 27eaef88..14f5e8c7 100644 --- a/src/android/post_processor.h +++ b/src/android/post_processor.h @@ -7,6 +7,8 @@ #ifndef __ANDROID_POST_PROCESSOR_H__ #define __ANDROID_POST_PROCESSOR_H__ +#include + #include #include @@ -17,6 +19,11 @@ class Camera3RequestDescriptor; class PostProcessor { public: + enum class Status { + Error, + Success + }; + virtual ~PostProcessor() = default; virtual int configure(const libcamera::StreamConfiguration &inCfg, @@ -24,6 +31,8 @@ public: virtual int process(const libcamera::FrameBuffer &source, CameraBuffer *destination, Camera3RequestDescriptor *request) = 0; + + libcamera::Signal processComplete; }; #endif /* __ANDROID_POST_PROCESSOR_H__ */ diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp index 8110a1f1..fd364741 100644 --- a/src/android/yuv/post_processor_yuv.cpp +++ b/src/android/yuv/post_processor_yuv.cpp @@ -51,14 +51,17 @@ int PostProcessorYuv::configure(const StreamConfiguration &inCfg, int PostProcessorYuv::process(const FrameBuffer &source, CameraBuffer *destination, - [[maybe_unused]] Camera3RequestDescriptor *request) + Camera3RequestDescriptor *request) { - if (!isValidBuffers(source, *destination)) + if (!isValidBuffers(source, *destination)) { + processComplete.emit(request, PostProcessor::Status::Error); return -EINVAL; + } const MappedFrameBuffer sourceMapped(&source, MappedFrameBuffer::MapFlag::Read); if (!sourceMapped.isValid()) { LOG(YUV, Error) << "Failed to mmap camera frame buffer"; + processComplete.emit(request, PostProcessor::Status::Error); return -EINVAL; } @@ -76,9 +79,12 @@ int PostProcessorYuv::process(const FrameBuffer &source, libyuv::FilterMode::kFilterBilinear); if (ret) { LOG(YUV, Error) << "Failed NV12 scaling: " << ret; + processComplete.emit(request, PostProcessor::Status::Error); return -EINVAL; } + processComplete.emit(request, PostProcessor::Status::Success); + return 0; } From patchwork Wed Oct 20 10:42:10 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Umang Jain X-Patchwork-Id: 14190 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 16AE0BF415 for ; Wed, 20 Oct 2021 10:42:25 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id CC86968F5B; Wed, 20 Oct 2021 12:42:24 +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="OYZlDAQZ"; dkim-atps=neutral Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 3C5AB68F5D for ; Wed, 20 Oct 2021 12:42:23 +0200 (CEST) Received: from perceval.ideasonboard.com (unknown [103.251.226.185]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 28B1A2A5; Wed, 20 Oct 2021 12:42:22 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1634726543; bh=SXg77ETBzTMM3GAp3jgvqdWLWi1O1H4D42TKSyFB45w=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=OYZlDAQZTmHbrnyhEonkMdUBifcEBHY3dX8phlMXsCF4oWHwH1kVaUrSjLm4NSeXS WLWOwDhZFk2CaH5qxxEnmLaWWgNrCylSht/MFmxwmlaNLZqSxs5bJzC6zu3FrKTykm kfMj13vHIt+ybDZPzBUEU3lACY6N7hl9yOzxSprw= From: Umang Jain To: libcamera-devel@lists.libcamera.org Date: Wed, 20 Oct 2021 16:12:10 +0530 Message-Id: <20211020104212.121743-3-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 2/4] android: post_processor: Drop return value for process() 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" PostProcessor::process() is invoked by CameraStream class in case any post-processing is required for the camera stream. The failure or success is checked via the value returned by CameraStream::process(). Now that the post-processor notifies about the post-processing completion operation, we can drop the return value of PostProcessor::process(). The status of post-processing is passed to CameraDevice::streamProcessingComplete() by the PostProcessor::processComplete signal's slot. Signed-off-by: Umang Jain Reviewed-by: Laurent Pinchart --- src/android/camera_stream.cpp | 4 +++- src/android/jpeg/post_processor_jpeg.cpp | 12 +++++------- src/android/jpeg/post_processor_jpeg.h | 6 +++--- src/android/post_processor.h | 6 +++--- src/android/yuv/post_processor_yuv.cpp | 14 ++++++-------- src/android/yuv/post_processor_yuv.h | 6 +++--- 6 files changed, 23 insertions(+), 25 deletions(-) diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp index 04cbef8c..b3cb06a2 100644 --- a/src/android/camera_stream.cpp +++ b/src/android/camera_stream.cpp @@ -191,7 +191,9 @@ int CameraStream::process(const FrameBuffer &source, return -EINVAL; } - return postProcessor_->process(source, &destBuffer, request); + postProcessor_->process(source, &destBuffer, request); + + return 0; } FrameBuffer *CameraStream::getBuffer() diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp index a001fede..955ea21d 100644 --- a/src/android/jpeg/post_processor_jpeg.cpp +++ b/src/android/jpeg/post_processor_jpeg.cpp @@ -98,12 +98,12 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source, } } -int PostProcessorJpeg::process(const FrameBuffer &source, - CameraBuffer *destination, - Camera3RequestDescriptor *request) +void PostProcessorJpeg::process(const FrameBuffer &source, + CameraBuffer *destination, + Camera3RequestDescriptor *request) { if (!encoder_) - return 0; + return; ASSERT(destination->numPlanes() == 1); @@ -199,7 +199,7 @@ int PostProcessorJpeg::process(const FrameBuffer &source, if (jpeg_size < 0) { LOG(JPEG, Error) << "Failed to encode stream image"; processComplete.emit(request, PostProcessor::Status::Error); - return jpeg_size; + return; } /* Fill in the JPEG blob header. */ @@ -213,6 +213,4 @@ int PostProcessorJpeg::process(const FrameBuffer &source, /* Update the JPEG result Metadata. */ resultMetadata->addEntry(ANDROID_JPEG_SIZE, jpeg_size); processComplete.emit(request, PostProcessor::Status::Success); - - return 0; } diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h index 0184d77e..5dab14e1 100644 --- a/src/android/jpeg/post_processor_jpeg.h +++ b/src/android/jpeg/post_processor_jpeg.h @@ -22,9 +22,9 @@ public: int configure(const libcamera::StreamConfiguration &incfg, const libcamera::StreamConfiguration &outcfg) override; - int process(const libcamera::FrameBuffer &source, - CameraBuffer *destination, - Camera3RequestDescriptor *request) override; + void process(const libcamera::FrameBuffer &source, + CameraBuffer *destination, + Camera3RequestDescriptor *request) override; private: void generateThumbnail(const libcamera::FrameBuffer &source, diff --git a/src/android/post_processor.h b/src/android/post_processor.h index 14f5e8c7..7e78cfec 100644 --- a/src/android/post_processor.h +++ b/src/android/post_processor.h @@ -28,9 +28,9 @@ public: virtual int configure(const libcamera::StreamConfiguration &inCfg, const libcamera::StreamConfiguration &outCfg) = 0; - virtual int process(const libcamera::FrameBuffer &source, - CameraBuffer *destination, - Camera3RequestDescriptor *request) = 0; + virtual void process(const libcamera::FrameBuffer &source, + CameraBuffer *destination, + Camera3RequestDescriptor *request) = 0; libcamera::Signal processComplete; }; diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp index fd364741..b167f057 100644 --- a/src/android/yuv/post_processor_yuv.cpp +++ b/src/android/yuv/post_processor_yuv.cpp @@ -49,20 +49,20 @@ int PostProcessorYuv::configure(const StreamConfiguration &inCfg, return 0; } -int PostProcessorYuv::process(const FrameBuffer &source, - CameraBuffer *destination, - Camera3RequestDescriptor *request) +void PostProcessorYuv::process(const FrameBuffer &source, + CameraBuffer *destination, + Camera3RequestDescriptor *request) { if (!isValidBuffers(source, *destination)) { processComplete.emit(request, PostProcessor::Status::Error); - return -EINVAL; + return; } const MappedFrameBuffer sourceMapped(&source, MappedFrameBuffer::MapFlag::Read); if (!sourceMapped.isValid()) { LOG(YUV, Error) << "Failed to mmap camera frame buffer"; processComplete.emit(request, PostProcessor::Status::Error); - return -EINVAL; + return; } int ret = libyuv::NV12Scale(sourceMapped.planes()[0].data(), @@ -80,12 +80,10 @@ int PostProcessorYuv::process(const FrameBuffer &source, if (ret) { LOG(YUV, Error) << "Failed NV12 scaling: " << ret; processComplete.emit(request, PostProcessor::Status::Error); - return -EINVAL; + return; } processComplete.emit(request, PostProcessor::Status::Success); - - return 0; } bool PostProcessorYuv::isValidBuffers(const FrameBuffer &source, diff --git a/src/android/yuv/post_processor_yuv.h b/src/android/yuv/post_processor_yuv.h index a4e0ff5d..1278843b 100644 --- a/src/android/yuv/post_processor_yuv.h +++ b/src/android/yuv/post_processor_yuv.h @@ -18,9 +18,9 @@ public: int configure(const libcamera::StreamConfiguration &incfg, const libcamera::StreamConfiguration &outcfg) override; - int process(const libcamera::FrameBuffer &source, - CameraBuffer *destination, - Camera3RequestDescriptor *request) override; + void process(const libcamera::FrameBuffer &source, + CameraBuffer *destination, + Camera3RequestDescriptor *request) override; private: bool isValidBuffers(const libcamera::FrameBuffer &source, 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__ */ From patchwork Wed Oct 20 10:42:12 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Umang Jain X-Patchwork-Id: 14192 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 72700BF415 for ; Wed, 20 Oct 2021 10:42:29 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 2CDD568F5B; Wed, 20 Oct 2021 12:42:29 +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="fKAxxH5u"; dkim-atps=neutral Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 400E968F61 for ; Wed, 20 Oct 2021 12:42:27 +0200 (CEST) Received: from perceval.ideasonboard.com (unknown [103.251.226.185]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 0ACEB2A5; Wed, 20 Oct 2021 12:42:25 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1634726547; bh=8wevhsEeDoT7GH/zuB8hQFildf6Fb6V/BSAK+h27oGE=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=fKAxxH5ugzGtiSUHzP1iFLnMAZp7f9E4fPD1jINyGTA/xJuMFqexCvq+GTKuZIDD1 3Skri/rRBOQaRNq13TXH5eN00/cI8qVCx9Kx3PoXlXDclWRIQwS3pK7d5fLqtnahey bwxbzdyRyE0EV4uszTVUvnGLcchAc4pLa0moauQc= From: Umang Jain To: libcamera-devel@lists.libcamera.org Date: Wed, 20 Oct 2021 16:12:12 +0530 Message-Id: <20211020104212.121743-5-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 4/4] android: camera_device: Protect descriptor status_ with lock 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" From: Laurent Pinchart The Camera3RequestDescriptor::status_ is checked as a stop condition for the sendCaptureResults() loop, protected by the descriptorsMutex_. The status is however not set with the mutex locked, which can cause a race condition with a concurrent sendCaptureResults() call (from the post-processor thread for instance). This should be harmless in practice, as the reader thread will either see the old status (Pending) and stop iterating over descriptors, or the new status and continue. Still, if the Camera3RequestDescriptor state machine were to change in the future, this could introduce hard to debug issues. Close the race window by always setting the status with the lock taken. Signed-off-by: Laurent Pinchart Signed-off-by: Umang Jain --- src/android/camera_device.cpp | 29 +++++++++++++++++++---------- src/android/camera_device.h | 5 ++++- 2 files changed, 23 insertions(+), 11 deletions(-) diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index b14416ce..4e8fb2ee 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -803,8 +803,6 @@ void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const for (auto &buffer : descriptor->buffers_) buffer.status = Camera3RequestDescriptor::Status::Error; - - descriptor->status_ = Camera3RequestDescriptor::Status::Error; } bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const @@ -988,7 +986,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques descriptors_.push(std::move(descriptor)); } - sendCaptureResults(); + completeDescriptor(descriptor.get(), + Camera3RequestDescriptor::Status::Error); return 0; } @@ -1058,7 +1057,8 @@ void CameraDevice::requestComplete(Request *request) << request->status(); abortRequest(descriptor); - sendCaptureResults(); + completeDescriptor(descriptor, + Camera3RequestDescriptor::Status::Error); return; } @@ -1135,20 +1135,28 @@ 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(); + completeDescriptor(descriptor, processingStatus); } return; } - descriptor->status_ = Camera3RequestDescriptor::Status::Success; + completeDescriptor(descriptor, Camera3RequestDescriptor::Status::Success); +} + +void CameraDevice::completeDescriptor(Camera3RequestDescriptor *descriptor, + Camera3RequestDescriptor::Status status) +{ + MutexLocker lock(descriptorsMutex_); + descriptor->status_ = status; + lock.unlock(); + sendCaptureResults(); } @@ -1254,14 +1262,15 @@ void CameraDevice::streamProcessingComplete(CameraStream *cameraStream, hasPostProcessingErrors = true; } + Camera3RequestDescriptor::Status descriptorStatus; if (hasPostProcessingErrors) - request->status_ = Camera3RequestDescriptor::Status::Error; + descriptorStatus = Camera3RequestDescriptor::Status::Error; else - request->status_ = Camera3RequestDescriptor::Status::Success; + descriptorStatus = Camera3RequestDescriptor::Status::Success; locker.unlock(); - sendCaptureResults(); + completeDescriptor(request, descriptorStatus); } std::string CameraDevice::logPrefix() const diff --git a/src/android/camera_device.h b/src/android/camera_device.h index 1ef933da..46fb93ee 100644 --- a/src/android/camera_device.h +++ b/src/android/camera_device.h @@ -96,6 +96,8 @@ private: void notifyError(uint32_t frameNumber, camera3_stream_t *stream, camera3_error_msg_code code) const; int processControls(Camera3RequestDescriptor *descriptor); + void completeDescriptor(Camera3RequestDescriptor *descriptor, + Camera3RequestDescriptor::Status status); void sendCaptureResults(); void setBufferStatus(CameraStream *cameraStream, Camera3RequestDescriptor::StreamBuffer &buffer, @@ -121,7 +123,8 @@ private: std::vector streams_; - libcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */ + /* Protects descriptors_ and Camera3RequestDescriptor::status_. */ + libcamera::Mutex descriptorsMutex_; std::queue> descriptors_; std::string maker_;