[{"id":20126,"web_url":"https://patchwork.libcamera.org/comment/20126/","msgid":"","date":"2021-10-12T03:31:03","subject":"Re: [libcamera-devel] [PATCH v4 3/7] android: Notify post\n\tprocessing completion via a signal","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 Mon, Oct 11, 2021 at 01:05:01PM +0530, Umang Jain wrote:\n> Notify that the post processing for a request has been completed,\n> via a signal. A pointer to the descriptor which is tracking the\n> capture request is emitted along with the status of post processing.\n> The function CameraDevice::streamProcessingComplete() will finally set\n> the status on the descriptor by reading the status of the post-processor\n> completion (passed as an argument to the signal) and call\n> sendCaptureResults().\n> \n> We also need to save a pointer to any internal buffers that might have\n> been allocated by CameraStream. The buffer should be returned back to\n> CameraStream just before capture results are sent.\n> \n> Signed-off-by: Umang Jain \n> ---\n> src/android/camera_device.cpp | 49 +++++++++++++++++-------\n> src/android/camera_device.h | 5 +++\n> src/android/camera_stream.cpp | 5 +++\n> src/android/jpeg/post_processor_jpeg.cpp | 2 +\n> src/android/post_processor.h | 9 +++++\n> src/android/yuv/post_processor_yuv.cpp | 12 +++++-\n> 6 files changed, 67 insertions(+), 15 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index b52bdc8f..9f26c36d 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -8,7 +8,6 @@\n> #include \"camera_device.h\"\n> #include \"camera_hal_config.h\"\n> #include \"camera_ops.h\"\n> -#include \"post_processor.h\"\n> \n> #include \n> #include \n> @@ -1226,20 +1225,12 @@ void CameraDevice::requestComplete(Request *request)\n> \t\t\tcontinue;\n> \t\t}\n> \n> -\t\tint ret = cameraStream->process(*src, buffer, descriptor);\n> -\n> -\t\t/*\n> -\t\t * Return the FrameBuffer to the CameraStream now that we're\n> -\t\t * done processing it.\n> -\t\t */\n> \t\tif (cameraStream->type() == CameraStream::Type::Internal)\n> -\t\t\tcameraStream->putBuffer(src);\n> +\t\t\tdescriptor->internalBuffer_ = src;\n> \n> -\t\tif (ret) {\n> -\t\t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> -\t\t\tnotifyError(descriptor->frameNumber_, buffer.stream,\n> -\t\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n> -\t\t}\n> +\t\tint ret = cameraStream->process(*src, buffer, descriptor);\n> +\n> +\t\treturn;\n> \t}\n> \n> \tcaptureResult.result = descriptor->resultMetadata_->get();\n> @@ -1266,6 +1257,38 @@ void CameraDevice::sendCaptureResults()\n> \t}\n> }\n> \n> +void CameraDevice::streamProcessingComplete(CameraStream *cameraStream,\n> +\t\t\t\t\t Camera3RequestDescriptor *request,\n> +\t\t\t\t\t PostProcessor::Status status)\n> +{\n\nYou'll have a problem here if multiple streams need to be produced with\npost-processing, the first stream to complete will complete the request.\n\n> +\tif (status == PostProcessor::Status::Error) {\n> +\t\tfor (camera3_stream_buffer_t &buffer : request->buffers_) {\n> +\t\t\tCameraStream *cs = static_cast(buffer.stream->priv);\n> +\n> +\t\t\tif (cs->type() == CameraStream::Type::Direct)\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> +\t\t\tnotifyError(request->frameNumber_, buffer.stream,\n> +\t\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n> +\t\t}\n> +\t}\n\nThis is right but shouldn't be done on all buffers in the request, only\nthe buffer for the stream that has completed.\n\n> +\n> +\t/*\n> +\t * Return the FrameBuffer to the CameraStream now that we're\n> +\t * done processing it.\n> +\t */\n> +\tif (cameraStream->type() == CameraStream::Type::Internal)\n> +\t\tcameraStream->putBuffer(request->internalBuffer_);\n\nSame here, and note how you could have one internal buffer per stream,\nso it has to be stored accordingly, not as a single pointer in the\nrequest.\n\n> +\n> +\tif (status == PostProcessor::Status::Success)\n> +\t\trequest->status_ = Camera3RequestDescriptor::Status::Success;\n> +\telse\n> +\t\trequest->status_ = Camera3RequestDescriptor::Status::Error;\n> +\n> +\tsendCaptureResults();\n\nThen you need to check if all post-processors have completed, and only\ncomplete the request at that point.\n\n> +}\n> +\n> std::string CameraDevice::logPrefix() const\n> {\n> \treturn \"'\" + camera_->id() + \"'\";\n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index 3da8dffa..eee97516 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -32,6 +32,7 @@\n> #include \"camera_stream.h\"\n> #include \"camera_worker.h\"\n> #include \"jpeg/encoder.h\"\n> +#include \"post_processor.h\"\n> \n> struct CameraConfigData;\n> \n> @@ -56,6 +57,7 @@ struct Camera3RequestDescriptor {\n> \tCameraMetadata settings_;\n> \tstd::unique_ptr request_;\n> \tstd::unique_ptr resultMetadata_;\n> +\tlibcamera::FrameBuffer *internalBuffer_;\n> \n> \tcamera3_capture_result_t captureResult_ = {};\n> \tStatus status_ = Status::Pending;\n> @@ -91,6 +93,9 @@ public:\n> \tint configureStreams(camera3_stream_configuration_t *stream_list);\n> \tint processCaptureRequest(camera3_capture_request_t *request);\n> \tvoid requestComplete(libcamera::Request *request);\n> +\tvoid streamProcessingComplete(CameraStream *cameraStream,\n> +\t\t\t\t Camera3RequestDescriptor *request,\n> +\t\t\t\t PostProcessor::Status status);\n> \n> protected:\n> \tstd::string logPrefix() const override;\n> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> index 8f47e4d8..d91e7dee 100644\n> --- a/src/android/camera_stream.cpp\n> +++ b/src/android/camera_stream.cpp\n> @@ -93,6 +93,11 @@ int CameraStream::configure()\n> \t\tint ret = postProcessor_->configure(configuration(), output);\n> \t\tif (ret)\n> \t\t\treturn ret;\n> +\n> +\t\tpostProcessor_->processComplete.connect(\n> +\t\t\tthis, [&](Camera3RequestDescriptor *request, PostProcessor::Status status) {\n> +\t\t\t\tcameraDevice_->streamProcessingComplete(this, request, status);\n> +\t\t\t});\n> \t}\n> \n> \tif (type_ == Type::Internal) {\n> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n> index 656c48b2..81d1efe6 100644\n> --- a/src/android/jpeg/post_processor_jpeg.cpp\n> +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> @@ -197,6 +197,7 @@ int PostProcessorJpeg::process(const FrameBuffer &source,\n> \t\t\t\t\t exif.data(), quality);\n> \tif (jpeg_size < 0) {\n> \t\tLOG(JPEG, Error) << \"Failed to encode stream image\";\n> +\t\tprocessComplete.emit(request, PostProcessor::Status::Error);\n> \t\treturn jpeg_size;\n> \t}\n> \n> @@ -210,6 +211,7 @@ int PostProcessorJpeg::process(const FrameBuffer &source,\n> \n> \t/* Update the JPEG result Metadata. */\n> \tresultMetadata->addEntry(ANDROID_JPEG_SIZE, jpeg_size);\n> +\tprocessComplete.emit(request, PostProcessor::Status::Success);\n> \n> \treturn 0;\n> }\n> diff --git a/src/android/post_processor.h b/src/android/post_processor.h\n> index fa13c7e0..6e67bcba 100644\n> --- a/src/android/post_processor.h\n> +++ b/src/android/post_processor.h\n> @@ -7,6 +7,8 @@\n> #ifndef __ANDROID_POST_PROCESSOR_H__\n> #define __ANDROID_POST_PROCESSOR_H__\n> \n> +#include \n> +\n> #include \n> #include \n> \n> @@ -19,6 +21,11 @@ struct Camera3RequestDescriptor;\n> class PostProcessor\n> {\n> public:\n> +\tenum class Status {\n> +\t\tError,\n> +\t\tSuccess\n> +\t};\n> +\n> \tvirtual ~PostProcessor() = default;\n> \n> \tvirtual int configure(const libcamera::StreamConfiguration &inCfg,\n> @@ -26,6 +33,8 @@ public:\n> \tvirtual int process(const libcamera::FrameBuffer &source,\n> \t\t\t CameraBuffer *destination,\n> \t\t\t Camera3RequestDescriptor *request) = 0;\n> +\n> +\tlibcamera::Signal processComplete;\n> };\n> \n> #endif /* __ANDROID_POST_PROCESSOR_H__ */\n> diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp\n> index 8110a1f1..b34b389d 100644\n> --- a/src/android/yuv/post_processor_yuv.cpp\n> +++ b/src/android/yuv/post_processor_yuv.cpp\n> @@ -18,6 +18,8 @@\n> #include \"libcamera/internal/formats.h\"\n> #include \"libcamera/internal/mapped_framebuffer.h\"\n> \n> +#include \"../camera_device.h\"\n> +\n> using namespace libcamera;\n> \n> LOG_DEFINE_CATEGORY(YUV)\n> @@ -51,14 +53,17 @@ int PostProcessorYuv::configure(const StreamConfiguration &inCfg,\n> \n> int PostProcessorYuv::process(const FrameBuffer &source,\n> \t\t\t CameraBuffer *destination,\n> -\t\t\t [[maybe_unused]] Camera3RequestDescriptor *request)\n> +\t\t\t Camera3RequestDescriptor *request)\n> {\n> -\tif (!isValidBuffers(source, *destination))\n> +\tif (!isValidBuffers(source, *destination)) {\n> +\t\tprocessComplete.emit(request, PostProcessor::Status::Error);\n> \t\treturn -EINVAL;\n> +\t}\n> \n> \tconst MappedFrameBuffer sourceMapped(&source, MappedFrameBuffer::MapFlag::Read);\n> \tif (!sourceMapped.isValid()) {\n> \t\tLOG(YUV, Error) << \"Failed to mmap camera frame buffer\";\n> +\t\tprocessComplete.emit(request, PostProcessor::Status::Error);\n> \t\treturn -EINVAL;\n> \t}\n> \n> @@ -76,9 +81,12 @@ int PostProcessorYuv::process(const FrameBuffer &source,\n> \t\t\t\t libyuv::FilterMode::kFilterBilinear);\n> \tif (ret) {\n> \t\tLOG(YUV, Error) << \"Failed NV12 scaling: \" << ret;\n> +\t\tprocessComplete.emit(request, PostProcessor::Status::Error);\n> \t\treturn -EINVAL;\n> \t}\n> \n> +\tprocessComplete.emit(request, PostProcessor::Status::Success);\n> +\n> \treturn 0;\n> }\n>","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 52079C323E\n\tfor ;\n\tTue, 12 Oct 2021 03:31:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 633BA68F4C;\n\tTue, 12 Oct 2021 05:31:18 +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 CA2AB6023A\n\tfor ;\n\tTue, 12 Oct 2021 05:31:16 +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 40B92F1;\n\tTue, 12 Oct 2021 05:31:16 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"JF8C49zs\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634009476;\n\tbh=6Y31n/WPS+Jh8MTSxQgfWPxniyWTCnPyh62xway9fro=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=JF8C49zsgfOfyJMY3OF8Gbe6R7HuPg+anF5kjasOGWStL1niTiihF7CamiU0rpgYR\n\tqRFka6RnGkHHby5bTZ1kKvYM0w1LzNKOzbM8G/Nsv7aPM5fI1QhMfYcpaHqYCRNFOc\n\t7rXDV0+HZKFuUaUQLgFj+6AVGFF5vzahcUrPPHXA=","Date":"Tue, 12 Oct 2021 06:31:03 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20211011073505.243864-1-umang.jain@ideasonboard.com>\n\t<20211011073505.243864-4-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211011073505.243864-4-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 3/7] android: Notify post\n\tprocessing completion via a signal","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\" "}}]