[{"id":19732,"web_url":"https://patchwork.libcamera.org/comment/19732/","msgid":"<20210921112928.ru35uv6eittpmbxm@uno.localdomain>","date":"2021-09-21T11:29:28","subject":"Re: [libcamera-devel] [PATCH v3 07/10] android: post_processor:\n\tNotify post processing completion status","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Umang,\n\nOn Mon, Sep 20, 2021 at 11:07:49PM +0530, Umang Jain wrote:\n> We should be able to know if post-processing has been completed\n> successfully or encountered some errors. This commit introduces a\n> Signal<> that will notify that the post-processing has been completed.\n> If the post processing was successful, status on the request descriptor\n> will be set to Camera3RequestDescriptor::ProcessStatus::Success.\n> The signal will be required when the post-processor is meant to\n> run asynchronously (in subsequent commits) and capture results need\n> to be sent back to the framework from the signal's slot instead.\n>\n> Signed-off-by: Umang Jain \n> ---\n> src/android/camera_device.cpp | 18 ++++++++++++++++++\n> src/android/camera_device.h | 11 +++++++++++\n> src/android/camera_stream.cpp | 15 ++++++++++++++-\n> src/android/camera_stream.h | 2 ++\n> src/android/jpeg/post_processor_jpeg.cpp | 10 +++++++++-\n> src/android/post_processor.h | 4 ++++\n> src/android/yuv/post_processor_yuv.cpp | 16 ++++++++++++++--\n> 7 files changed, 72 insertions(+), 4 deletions(-)\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index fa462368..e2de4012 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -246,6 +246,12 @@ Camera3RequestDescriptor::Camera3RequestDescriptor(\n> \t */\n> \trequest_ = std::make_unique(camera,\n> \t\t\t\t\t\t reinterpret_cast(this));\n> +\n> +\t/*\n> +\t * Denotes the post-processing status required by any stream. It is set\n> +\t * to ProcessStatus::Success if processing is successful.\n> +\t */\n> +\tstatus_ = ProcessStatus::None;\n\nLooking at how the status is used in CameraDevice, I would rather make\nit a paramter of CameraDevice::streamProcessingComplete() and in the\nfollowing patch of sendCaptureResults().\n\nThis way you could avoid there\n\tif (d->status_ == Camera3RequestDescriptor::ProcessStatus::Processing ||\n\t d->status_ == Camera3RequestDescriptor::ProcessStatus::None)\n\t\treturn;\n\n\n> }\n>\n> /*\n> @@ -1150,6 +1156,8 @@ void CameraDevice::requestComplete(Request *request)\n> \t\t\tcontinue;\n> \t\t}\n>\n> +\t\tdescriptor->status_ = Camera3RequestDescriptor::ProcessStatus::Processing;\n> +\n> \t\tint ret = cameraStream->process(src, *buffer.buffer, descriptor);\n>\n> \t\t/*\n> @@ -1172,6 +1180,16 @@ void CameraDevice::requestComplete(Request *request)\n> \tdescriptors_.pop_front();\n> }\n>\n> +\n> +void CameraDevice::streamProcessingComplete([[maybe_unused]] CameraStream *cameraStream,\n> +\t\t\t\t\t [[maybe_unused]] Camera3RequestDescriptor *request)\n> +{\n> +\t/*\n> +\t * \\todo Stream processing is completed hence, check for errors and\n> +\t * if any, mark the corresponding buffer with CAMERA3_BUFFER_STATUS_ERROR.\n> +\t */\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 b2871e52..60c134dc 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -36,6 +36,13 @@\n> struct CameraConfigData;\n>\n> struct Camera3RequestDescriptor {\n> +\tenum class ProcessStatus {\n> +\t\tNone,\n> +\t\tProcessing,\n> +\t\tSuccess,\n> +\t\tError\n> +\t};\n> +\n> \tCamera3RequestDescriptor() = default;\n> \t~Camera3RequestDescriptor() = default;\n> \tCamera3RequestDescriptor(libcamera::Camera *camera,\n> @@ -48,6 +55,8 @@ struct Camera3RequestDescriptor {\n> \tCameraMetadata settings_;\n> \tstd::unique_ptr request_;\n> \tstd::unique_ptr resultMetadata_;\n> +\n> +\tProcessStatus status_;\n> };\n>\n> class CameraDevice : protected libcamera::Loggable\n> @@ -79,6 +88,8 @@ 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>\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 d494f5d5..70471959 100644\n> --- a/src/android/camera_stream.cpp\n> +++ b/src/android/camera_stream.cpp\n> @@ -81,6 +81,9 @@ 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, &CameraStream::handleProcessComplete);\n> \t}\n>\n> \tif (allocator_) {\n> @@ -102,8 +105,11 @@ int CameraStream::process(const FrameBuffer *source,\n> \t\t\t buffer_handle_t camera3Dest,\n> \t\t\t Camera3RequestDescriptor *request)\n> {\n> -\tif (!postProcessor_)\n> +\tif (!postProcessor_) {\n> +\t\trequest->status_ = Camera3RequestDescriptor::ProcessStatus::Error;\n> +\t\thandleProcessComplete(request);\n> \t\treturn 0;\n> +\t}\n\nThat was like this before, but I feel like this is a development\nmistaken and should be better caught with Fatal\n\nsame for the below !encoder and the YUV post-processor\n\n>\n> \t/*\n> \t * \\todo Buffer mapping and processing should be moved to a\n> @@ -114,12 +120,19 @@ int CameraStream::process(const FrameBuffer *source,\n> \t\t\t PROT_READ | PROT_WRITE);\n> \tif (!dest.isValid()) {\n> \t\tLOG(HAL, Error) << \"Failed to map android blob buffer\";\n> +\t\trequest->status_ = Camera3RequestDescriptor::ProcessStatus::Error;\n> +\t\thandleProcessComplete(request);\n> \t\treturn -EINVAL;\n> \t}\n>\n> \treturn postProcessor_->process(source, &dest, request);\n> }\n>\n> +void CameraStream::handleProcessComplete(Camera3RequestDescriptor *request)\n> +{\n> +\tcameraDevice_->streamProcessingComplete(this, request);\n> +}\n\nCan't CameraStream connect CameraDevice::streamProcessingComplete with\nthe post-processor's processComplete signal ?\n\n> +\n> FrameBuffer *CameraStream::getBuffer()\n> {\n> \tif (!allocator_)\n> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n> index 68789700..f8ad6245 100644\n> --- a/src/android/camera_stream.h\n> +++ b/src/android/camera_stream.h\n> @@ -127,6 +127,8 @@ public:\n> \tvoid putBuffer(libcamera::FrameBuffer *buffer);\n>\n> private:\n> +\tvoid handleProcessComplete(Camera3RequestDescriptor *request);\n> +\n> \tCameraDevice *const cameraDevice_;\n> \tconst libcamera::CameraConfiguration *config_;\n> \tconst Type type_;\n> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n> index 31f68330..87252acd 100644\n> --- a/src/android/jpeg/post_processor_jpeg.cpp\n> +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> @@ -101,8 +101,11 @@ int PostProcessorJpeg::process(const FrameBuffer *source,\n> \t\t\t CameraBuffer *destination,\n> \t\t\t Camera3RequestDescriptor *request)\n> {\n> -\tif (!encoder_)\n> +\tif (!encoder_) {\n> +\t\trequest->status_ = Camera3RequestDescriptor::ProcessStatus::Error;\n> +\t\tprocessComplete.emit(request);\n> \t\treturn 0;\n> +\t}\n>\n> \tASSERT(destination->numPlanes() == 1);\n>\n> @@ -197,6 +200,8 @@ 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\trequest->status_ = Camera3RequestDescriptor::ProcessStatus::Error;\n> +\t\tprocessComplete.emit(request);\n> \t\treturn jpeg_size;\n> \t}\n>\n> @@ -211,5 +216,8 @@ int PostProcessorJpeg::process(const FrameBuffer *source,\n> \t/* Update the JPEG result Metadata. */\n> \tresultMetadata->addEntry(ANDROID_JPEG_SIZE, jpeg_size);\n>\n> +\trequest->status_ = Camera3RequestDescriptor::ProcessStatus::Success;\n> +\tprocessComplete.emit(request);\n> +\n> \treturn 0;\n> }\n> diff --git a/src/android/post_processor.h b/src/android/post_processor.h\n> index bdd6afe7..f639b237 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> @@ -26,6 +28,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 5e18caee..b144649a 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,19 @@ 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\trequest->status_ = Camera3RequestDescriptor::ProcessStatus::Error;\n> +\t\tprocessComplete.emit(request);\n\nIf my first suggestion will be considered, the post processor will\nsend the status in the signal, and you can reduce the number of status\nto Success or Error maybe ?\n\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\trequest->status_ = Camera3RequestDescriptor::ProcessStatus::Error;\n> +\t\tprocessComplete.emit(request);\n> \t\treturn -EINVAL;\n> \t}\n>\n> @@ -76,9 +83,14 @@ 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\trequest->status_ = Camera3RequestDescriptor::ProcessStatus::Error;\n> +\t\tprocessComplete.emit(request);\n> \t\treturn -EINVAL;\n> \t}\n>\n> +\trequest->status_ = Camera3RequestDescriptor::ProcessStatus::Success;\n> +\tprocessComplete.emit(request);\n> +\n> \treturn 0;\n> }\n>\n> --\n> 2.31.1\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 47F6ABF01C\n\tfor ;\n\tTue, 21 Sep 2021 11:28:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id ACF496918A;\n\tTue, 21 Sep 2021 13:28:43 +0200 (CEST)","from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[217.70.183.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C2EFD60247\n\tfor ;\n\tTue, 21 Sep 2021 13:28:41 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 3B58860010;\n\tTue, 21 Sep 2021 11:28:41 +0000 (UTC)"],"Date":"Tue, 21 Sep 2021 13:29:28 +0200","From":"Jacopo Mondi ","To":"Umang Jain ","Message-ID":"<20210921112928.ru35uv6eittpmbxm@uno.localdomain>","References":"<20210920173752.1346190-1-umang.jain@ideasonboard.com>\n\t<20210920173752.1346190-8-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210920173752.1346190-8-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 07/10] android: post_processor:\n\tNotify post processing completion status","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":19735,"web_url":"https://patchwork.libcamera.org/comment/19735/","msgid":"<20210921114847.yih52hd6lh337llg@uno.localdomain>","date":"2021-09-21T11:48:47","subject":"Re: [libcamera-devel] [PATCH v3 07/10] android: post_processor:\n\tNotify post processing completion status","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"On Mon, Sep 20, 2021 at 11:07:49PM +0530, Umang Jain wrote:\n> We should be able to know if post-processing has been completed\n> successfully or encountered some errors. This commit introduces a\n> Signal<> that will notify that the post-processing has been completed.\n> If the post processing was successful, status on the request descriptor\n> will be set to Camera3RequestDescriptor::ProcessStatus::Success.\n> The signal will be required when the post-processor is meant to\n> run asynchronously (in subsequent commits) and capture results need\n> to be sent back to the framework from the signal's slot instead.\n>\n> Signed-off-by: Umang Jain \n> ---\n> src/android/camera_device.cpp | 18 ++++++++++++++++++\n> src/android/camera_device.h | 11 +++++++++++\n> src/android/camera_stream.cpp | 15 ++++++++++++++-\n> src/android/camera_stream.h | 2 ++\n> src/android/jpeg/post_processor_jpeg.cpp | 10 +++++++++-\n> src/android/post_processor.h | 4 ++++\n> src/android/yuv/post_processor_yuv.cpp | 16 ++++++++++++++--\n> 7 files changed, 72 insertions(+), 4 deletions(-)\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index fa462368..e2de4012 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -246,6 +246,12 @@ Camera3RequestDescriptor::Camera3RequestDescriptor(\n> \t */\n> \trequest_ = std::make_unique(camera,\n> \t\t\t\t\t\t reinterpret_cast(this));\n> +\n> +\t/*\n> +\t * Denotes the post-processing status required by any stream. It is set\n> +\t * to ProcessStatus::Success if processing is successful.\n> +\t */\n> +\tstatus_ = ProcessStatus::None;\n> }\n>\n> /*\n> @@ -1150,6 +1156,8 @@ void CameraDevice::requestComplete(Request *request)\n> \t\t\tcontinue;\n> \t\t}\n>\n> +\t\tdescriptor->status_ = Camera3RequestDescriptor::ProcessStatus::Processing;\n> +\n> \t\tint ret = cameraStream->process(src, *buffer.buffer, descriptor);\n>\n> \t\t/*\n> @@ -1172,6 +1180,16 @@ void CameraDevice::requestComplete(Request *request)\n> \tdescriptors_.pop_front();\n> }\n>\n> +\n\nAlso double empty line\nweird that checkstyle does not complain\n\n> +void CameraDevice::streamProcessingComplete([[maybe_unused]] CameraStream *cameraStream,\n> +\t\t\t\t\t [[maybe_unused]] Camera3RequestDescriptor *request)\n> +{\n> +\t/*\n> +\t * \\todo Stream processing is completed hence, check for errors and\n> +\t * if any, mark the corresponding buffer with CAMERA3_BUFFER_STATUS_ERROR.\n> +\t */\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 b2871e52..60c134dc 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -36,6 +36,13 @@\n> struct CameraConfigData;\n>\n> struct Camera3RequestDescriptor {\n> +\tenum class ProcessStatus {\n> +\t\tNone,\n> +\t\tProcessing,\n> +\t\tSuccess,\n> +\t\tError\n> +\t};\n> +\n> \tCamera3RequestDescriptor() = default;\n> \t~Camera3RequestDescriptor() = default;\n> \tCamera3RequestDescriptor(libcamera::Camera *camera,\n> @@ -48,6 +55,8 @@ struct Camera3RequestDescriptor {\n> \tCameraMetadata settings_;\n> \tstd::unique_ptr request_;\n> \tstd::unique_ptr resultMetadata_;\n> +\n> +\tProcessStatus status_;\n> };\n>\n> class CameraDevice : protected libcamera::Loggable\n> @@ -79,6 +88,8 @@ 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>\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 d494f5d5..70471959 100644\n> --- a/src/android/camera_stream.cpp\n> +++ b/src/android/camera_stream.cpp\n> @@ -81,6 +81,9 @@ 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, &CameraStream::handleProcessComplete);\n> \t}\n>\n> \tif (allocator_) {\n> @@ -102,8 +105,11 @@ int CameraStream::process(const FrameBuffer *source,\n> \t\t\t buffer_handle_t camera3Dest,\n> \t\t\t Camera3RequestDescriptor *request)\n> {\n> -\tif (!postProcessor_)\n> +\tif (!postProcessor_) {\n> +\t\trequest->status_ = Camera3RequestDescriptor::ProcessStatus::Error;\n> +\t\thandleProcessComplete(request);\n> \t\treturn 0;\n> +\t}\n>\n> \t/*\n> \t * \\todo Buffer mapping and processing should be moved to a\n> @@ -114,12 +120,19 @@ int CameraStream::process(const FrameBuffer *source,\n> \t\t\t PROT_READ | PROT_WRITE);\n> \tif (!dest.isValid()) {\n> \t\tLOG(HAL, Error) << \"Failed to map android blob buffer\";\n> +\t\trequest->status_ = Camera3RequestDescriptor::ProcessStatus::Error;\n> +\t\thandleProcessComplete(request);\n> \t\treturn -EINVAL;\n> \t}\n>\n> \treturn postProcessor_->process(source, &dest, request);\n> }\n>\n> +void CameraStream::handleProcessComplete(Camera3RequestDescriptor *request)\n> +{\n> +\tcameraDevice_->streamProcessingComplete(this, request);\n> +}\n> +\n> FrameBuffer *CameraStream::getBuffer()\n> {\n> \tif (!allocator_)\n> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n> index 68789700..f8ad6245 100644\n> --- a/src/android/camera_stream.h\n> +++ b/src/android/camera_stream.h\n> @@ -127,6 +127,8 @@ public:\n> \tvoid putBuffer(libcamera::FrameBuffer *buffer);\n>\n> private:\n> +\tvoid handleProcessComplete(Camera3RequestDescriptor *request);\n> +\n> \tCameraDevice *const cameraDevice_;\n> \tconst libcamera::CameraConfiguration *config_;\n> \tconst Type type_;\n> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n> index 31f68330..87252acd 100644\n> --- a/src/android/jpeg/post_processor_jpeg.cpp\n> +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> @@ -101,8 +101,11 @@ int PostProcessorJpeg::process(const FrameBuffer *source,\n> \t\t\t CameraBuffer *destination,\n> \t\t\t Camera3RequestDescriptor *request)\n> {\n> -\tif (!encoder_)\n> +\tif (!encoder_) {\n> +\t\trequest->status_ = Camera3RequestDescriptor::ProcessStatus::Error;\n> +\t\tprocessComplete.emit(request);\n> \t\treturn 0;\n> +\t}\n>\n> \tASSERT(destination->numPlanes() == 1);\n>\n> @@ -197,6 +200,8 @@ 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\trequest->status_ = Camera3RequestDescriptor::ProcessStatus::Error;\n> +\t\tprocessComplete.emit(request);\n> \t\treturn jpeg_size;\n> \t}\n>\n> @@ -211,5 +216,8 @@ int PostProcessorJpeg::process(const FrameBuffer *source,\n> \t/* Update the JPEG result Metadata. */\n> \tresultMetadata->addEntry(ANDROID_JPEG_SIZE, jpeg_size);\n>\n> +\trequest->status_ = Camera3RequestDescriptor::ProcessStatus::Success;\n> +\tprocessComplete.emit(request);\n> +\n> \treturn 0;\n> }\n> diff --git a/src/android/post_processor.h b/src/android/post_processor.h\n> index bdd6afe7..f639b237 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> @@ -26,6 +28,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 5e18caee..b144649a 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,19 @@ 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\trequest->status_ = Camera3RequestDescriptor::ProcessStatus::Error;\n> +\t\tprocessComplete.emit(request);\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\trequest->status_ = Camera3RequestDescriptor::ProcessStatus::Error;\n> +\t\tprocessComplete.emit(request);\n> \t\treturn -EINVAL;\n> \t}\n>\n> @@ -76,9 +83,14 @@ 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\trequest->status_ = Camera3RequestDescriptor::ProcessStatus::Error;\n> +\t\tprocessComplete.emit(request);\n> \t\treturn -EINVAL;\n> \t}\n>\n> +\trequest->status_ = Camera3RequestDescriptor::ProcessStatus::Success;\n> +\tprocessComplete.emit(request);\n> +\n> \treturn 0;\n> }\n>\n> --\n> 2.31.1\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 E322ABDC71\n\tfor ;\n\tTue, 21 Sep 2021 11:48:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 69C946918C;\n\tTue, 21 Sep 2021 13:48:02 +0200 (CEST)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2942360247\n\tfor ;\n\tTue, 21 Sep 2021 13:48:01 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id ADCBA40003;\n\tTue, 21 Sep 2021 11:48:00 +0000 (UTC)"],"Date":"Tue, 21 Sep 2021 13:48:47 +0200","From":"Jacopo Mondi ","To":"Umang Jain ","Message-ID":"<20210921114847.yih52hd6lh337llg@uno.localdomain>","References":"<20210920173752.1346190-1-umang.jain@ideasonboard.com>\n\t<20210920173752.1346190-8-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210920173752.1346190-8-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 07/10] android: post_processor:\n\tNotify post processing completion status","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":19759,"web_url":"https://patchwork.libcamera.org/comment/19759/","msgid":"","date":"2021-09-21T23:26:43","subject":"Re: [libcamera-devel] [PATCH v3 07/10] android: post_processor:\n\tNotify post processing completion status","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Tue, Sep 21, 2021 at 01:29:28PM +0200, Jacopo Mondi wrote:\n> On Mon, Sep 20, 2021 at 11:07:49PM +0530, Umang Jain wrote:\n> > We should be able to know if post-processing has been completed\n> > successfully or encountered some errors. This commit introduces a\n> > Signal<> that will notify that the post-processing has been completed.\n> > If the post processing was successful, status on the request descriptor\n> > will be set to Camera3RequestDescriptor::ProcessStatus::Success.\n> > The signal will be required when the post-processor is meant to\n> > run asynchronously (in subsequent commits) and capture results need\n> > to be sent back to the framework from the signal's slot instead.\n> >\n> > Signed-off-by: Umang Jain \n> > ---\n> > src/android/camera_device.cpp | 18 ++++++++++++++++++\n> > src/android/camera_device.h | 11 +++++++++++\n> > src/android/camera_stream.cpp | 15 ++++++++++++++-\n> > src/android/camera_stream.h | 2 ++\n> > src/android/jpeg/post_processor_jpeg.cpp | 10 +++++++++-\n> > src/android/post_processor.h | 4 ++++\n> > src/android/yuv/post_processor_yuv.cpp | 16 ++++++++++++++--\n> > 7 files changed, 72 insertions(+), 4 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index fa462368..e2de4012 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -246,6 +246,12 @@ Camera3RequestDescriptor::Camera3RequestDescriptor(\n> > \t */\n> > \trequest_ = std::make_unique(camera,\n> > \t\t\t\t\t\t reinterpret_cast(this));\n> > +\n> > +\t/*\n> > +\t * Denotes the post-processing status required by any stream. It is set\n> > +\t * to ProcessStatus::Success if processing is successful.\n> > +\t */\n> > +\tstatus_ = ProcessStatus::None;\n> \n> Looking at how the status is used in CameraDevice, I would rather make\n> it a paramter of CameraDevice::streamProcessingComplete() and in the\n> following patch of sendCaptureResults().\n> \n> This way you could avoid there\n> \tif (d->status_ == Camera3RequestDescriptor::ProcessStatus::Processing ||\n> \t d->status_ == Camera3RequestDescriptor::ProcessStatus::None)\n> \t\treturn;\n\nI'm in two minds about this. For the PostProcessor::processComplete()\nsignal, I agree, we could pass a status back as a signal parameter. It\nwould have the advantage of not having to set the request status in\nquite a few locations in the post-processors. In that case, the status\nshould be a\n\n\tenum class Status {\n\t\tSuccess,\n\t\tError,\n\t};\n\nin the PostProcessor class, as we don't need more than that.\n\nHowever, once we reach CameraStream::handleProcessComplete(), I think it\nwould make sense to store the status in Camera3RequestDescriptor, to\nkeep all request-related data together. This will be needed when\niterating over all descriptors in the queue to find out which ones are\ncomplete. We would need a different Camera3RequestDescriptor::Status\nthere, and I think it should indicate the status of the request, not the\nstatus of the post-processing. We would thus have\n\n\tenum class Status {\n\t\tPending,\n\t\tSuccess,\n\t\tError,\n\t};\n\nin Camera3RequestDescriptor.\n\n> > }\n> >\n> > /*\n> > @@ -1150,6 +1156,8 @@ void CameraDevice::requestComplete(Request *request)\n> > \t\t\tcontinue;\n> > \t\t}\n> >\n> > +\t\tdescriptor->status_ = Camera3RequestDescriptor::ProcessStatus::Processing;\n> > +\n> > \t\tint ret = cameraStream->process(src, *buffer.buffer, descriptor);\n> >\n> > \t\t/*\n> > @@ -1172,6 +1180,16 @@ void CameraDevice::requestComplete(Request *request)\n> > \tdescriptors_.pop_front();\n> > }\n> >\n> > +\n> > +void CameraDevice::streamProcessingComplete([[maybe_unused]] CameraStream *cameraStream,\n> > +\t\t\t\t\t [[maybe_unused]] Camera3RequestDescriptor *request)\n> > +{\n> > +\t/*\n> > +\t * \\todo Stream processing is completed hence, check for errors and\n> > +\t * if any, mark the corresponding buffer with CAMERA3_BUFFER_STATUS_ERROR.\n> > +\t */\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 b2871e52..60c134dc 100644\n> > --- a/src/android/camera_device.h\n> > +++ b/src/android/camera_device.h\n> > @@ -36,6 +36,13 @@\n> > struct CameraConfigData;\n> >\n> > struct Camera3RequestDescriptor {\n> > +\tenum class ProcessStatus {\n> > +\t\tNone,\n> > +\t\tProcessing,\n> > +\t\tSuccess,\n> > +\t\tError\n> > +\t};\n> > +\n> > \tCamera3RequestDescriptor() = default;\n> > \t~Camera3RequestDescriptor() = default;\n> > \tCamera3RequestDescriptor(libcamera::Camera *camera,\n> > @@ -48,6 +55,8 @@ struct Camera3RequestDescriptor {\n> > \tCameraMetadata settings_;\n> > \tstd::unique_ptr request_;\n> > \tstd::unique_ptr resultMetadata_;\n> > +\n> > +\tProcessStatus status_;\n> > };\n> >\n> > class CameraDevice : protected libcamera::Loggable\n> > @@ -79,6 +88,8 @@ 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> >\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 d494f5d5..70471959 100644\n> > --- a/src/android/camera_stream.cpp\n> > +++ b/src/android/camera_stream.cpp\n> > @@ -81,6 +81,9 @@ 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, &CameraStream::handleProcessComplete);\n> > \t}\n> >\n> > \tif (allocator_) {\n> > @@ -102,8 +105,11 @@ int CameraStream::process(const FrameBuffer *source,\n> > \t\t\t buffer_handle_t camera3Dest,\n> > \t\t\t Camera3RequestDescriptor *request)\n> > {\n> > -\tif (!postProcessor_)\n> > +\tif (!postProcessor_) {\n> > +\t\trequest->status_ = Camera3RequestDescriptor::ProcessStatus::Error;\n> > +\t\thandleProcessComplete(request);\n> > \t\treturn 0;\n> > +\t}\n> \n> That was like this before, but I feel like this is a development\n> mistaken and should be better caught with Fatal\n> \n> same for the below !encoder and the YUV post-processor\n\nOr\n\n\tASSERT(postProcessor_);\n\n?\n\n> >\n> > \t/*\n> > \t * \\todo Buffer mapping and processing should be moved to a\n> > @@ -114,12 +120,19 @@ int CameraStream::process(const FrameBuffer *source,\n> > \t\t\t PROT_READ | PROT_WRITE);\n> > \tif (!dest.isValid()) {\n> > \t\tLOG(HAL, Error) << \"Failed to map android blob buffer\";\n> > +\t\trequest->status_ = Camera3RequestDescriptor::ProcessStatus::Error;\n> > +\t\thandleProcessComplete(request);\n> > \t\treturn -EINVAL;\n> > \t}\n> >\n> > \treturn postProcessor_->process(source, &dest, request);\n> > }\n> >\n> > +void CameraStream::handleProcessComplete(Camera3RequestDescriptor *request)\n> > +{\n> > +\tcameraDevice_->streamProcessingComplete(this, request);\n> > +}\n> \n> Can't CameraStream connect CameraDevice::streamProcessingComplete with\n> the post-processor's processComplete signal ?\n\nWe can do so with a lambda function, now supported by the signal class:\n\n\t\tpostProcessor_->processComplete.connect(\n\t\t\t[&](Camera3RequestDescriptor *request) {\n\t\t\t\tcameraDevice_->streamProcessingComplete(this, request);\n\t\t\t}\n\t\t);\n\n> > +\n> > FrameBuffer *CameraStream::getBuffer()\n> > {\n> > \tif (!allocator_)\n> > diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n> > index 68789700..f8ad6245 100644\n> > --- a/src/android/camera_stream.h\n> > +++ b/src/android/camera_stream.h\n> > @@ -127,6 +127,8 @@ public:\n> > \tvoid putBuffer(libcamera::FrameBuffer *buffer);\n> >\n> > private:\n> > +\tvoid handleProcessComplete(Camera3RequestDescriptor *request);\n> > +\n> > \tCameraDevice *const cameraDevice_;\n> > \tconst libcamera::CameraConfiguration *config_;\n> > \tconst Type type_;\n> > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n> > index 31f68330..87252acd 100644\n> > --- a/src/android/jpeg/post_processor_jpeg.cpp\n> > +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> > @@ -101,8 +101,11 @@ int PostProcessorJpeg::process(const FrameBuffer *source,\n> > \t\t\t CameraBuffer *destination,\n> > \t\t\t Camera3RequestDescriptor *request)\n> > {\n> > -\tif (!encoder_)\n> > +\tif (!encoder_) {\n> > +\t\trequest->status_ = Camera3RequestDescriptor::ProcessStatus::Error;\n> > +\t\tprocessComplete.emit(request);\n> > \t\treturn 0;\n> > +\t}\n> >\n> > \tASSERT(destination->numPlanes() == 1);\n> >\n> > @@ -197,6 +200,8 @@ 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\trequest->status_ = Camera3RequestDescriptor::ProcessStatus::Error;\n> > +\t\tprocessComplete.emit(request);\n> > \t\treturn jpeg_size;\n> > \t}\n> >\n> > @@ -211,5 +216,8 @@ int PostProcessorJpeg::process(const FrameBuffer *source,\n> > \t/* Update the JPEG result Metadata. */\n> > \tresultMetadata->addEntry(ANDROID_JPEG_SIZE, jpeg_size);\n> >\n> > +\trequest->status_ = Camera3RequestDescriptor::ProcessStatus::Success;\n> > +\tprocessComplete.emit(request);\n> > +\n> > \treturn 0;\n> > }\n> > diff --git a/src/android/post_processor.h b/src/android/post_processor.h\n> > index bdd6afe7..f639b237 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> > @@ -26,6 +28,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 5e18caee..b144649a 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,19 @@ 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\trequest->status_ = Camera3RequestDescriptor::ProcessStatus::Error;\n> > +\t\tprocessComplete.emit(request);\n> \n> If my first suggestion will be considered, the post processor will\n> send the status in the signal, and you can reduce the number of status\n> to Success or Error maybe ?\n> \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\trequest->status_ = Camera3RequestDescriptor::ProcessStatus::Error;\n> > +\t\tprocessComplete.emit(request);\n> > \t\treturn -EINVAL;\n> > \t}\n> >\n> > @@ -76,9 +83,14 @@ 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\trequest->status_ = Camera3RequestDescriptor::ProcessStatus::Error;\n> > +\t\tprocessComplete.emit(request);\n> > \t\treturn -EINVAL;\n> > \t}\n> >\n> > +\trequest->status_ = Camera3RequestDescriptor::ProcessStatus::Success;\n> > +\tprocessComplete.emit(request);\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 A0344BF01C\n\tfor ;\n\tTue, 21 Sep 2021 23:27:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B65E76918E;\n\tWed, 22 Sep 2021 01:27:15 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 640C969186\n\tfor ;\n\tWed, 22 Sep 2021 01:27:14 +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 C49DD291;\n\tWed, 22 Sep 2021 01:27:13 +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=\"alHIGWyn\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632266834;\n\tbh=q2nkrmlJs2OzVHgPUcLXOBcfQEoocJLaIK+Nb5kzgOk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=alHIGWynjzEzYmNI09qw5RXZhjSQDHF7HlvJRGCrHeysTxfO0v+GDv5MeBleqi9Jl\n\taQTjFIJO/9As+dZiwsWlSgxlIBOqd3CL7WoomkLJDY3RYWl/lDIj/0ClmvdQu6jWc3\n\tKyPjE+Gu6kePRbauJKKXdTJ/ximSCkMLHex/m87Q=","Date":"Wed, 22 Sep 2021 02:26:43 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210920173752.1346190-1-umang.jain@ideasonboard.com>\n\t<20210920173752.1346190-8-umang.jain@ideasonboard.com>\n\t<20210921112928.ru35uv6eittpmbxm@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210921112928.ru35uv6eittpmbxm@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3 07/10] android: post_processor:\n\tNotify post processing completion status","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":19781,"web_url":"https://patchwork.libcamera.org/comment/19781/","msgid":"","date":"2021-09-22T08:43:56","subject":"Re: [libcamera-devel] [PATCH v3 07/10] android: post_processor:\n\tNotify post processing completion status","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 9/22/21 4:56 AM, Laurent Pinchart wrote:\n> Hello,\n>\n> On Tue, Sep 21, 2021 at 01:29:28PM +0200, Jacopo Mondi wrote:\n>> On Mon, Sep 20, 2021 at 11:07:49PM +0530, Umang Jain wrote:\n>>> We should be able to know if post-processing has been completed\n>>> successfully or encountered some errors. This commit introduces a\n>>> Signal<> that will notify that the post-processing has been completed.\n>>> If the post processing was successful, status on the request descriptor\n>>> will be set to Camera3RequestDescriptor::ProcessStatus::Success.\n>>> The signal will be required when the post-processor is meant to\n>>> run asynchronously (in subsequent commits) and capture results need\n>>> to be sent back to the framework from the signal's slot instead.\n>>>\n>>> Signed-off-by: Umang Jain \n>>> ---\n>>> src/android/camera_device.cpp | 18 ++++++++++++++++++\n>>> src/android/camera_device.h | 11 +++++++++++\n>>> src/android/camera_stream.cpp | 15 ++++++++++++++-\n>>> src/android/camera_stream.h | 2 ++\n>>> src/android/jpeg/post_processor_jpeg.cpp | 10 +++++++++-\n>>> src/android/post_processor.h | 4 ++++\n>>> src/android/yuv/post_processor_yuv.cpp | 16 ++++++++++++++--\n>>> 7 files changed, 72 insertions(+), 4 deletions(-)\n>>>\n>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>>> index fa462368..e2de4012 100644\n>>> --- a/src/android/camera_device.cpp\n>>> +++ b/src/android/camera_device.cpp\n>>> @@ -246,6 +246,12 @@ Camera3RequestDescriptor::Camera3RequestDescriptor(\n>>> \t */\n>>> \trequest_ = std::make_unique(camera,\n>>> \t\t\t\t\t\t reinterpret_cast(this));\n>>> +\n>>> +\t/*\n>>> +\t * Denotes the post-processing status required by any stream. It is set\n>>> +\t * to ProcessStatus::Success if processing is successful.\n>>> +\t */\n>>> +\tstatus_ = ProcessStatus::None;\n>> Looking at how the status is used in CameraDevice, I would rather make\n>> it a paramter of CameraDevice::streamProcessingComplete() and in the\n>> following patch of sendCaptureResults().\n>>\n>> This way you could avoid there\n>> \tif (d->status_ == Camera3RequestDescriptor::ProcessStatus::Processing ||\n>> \t d->status_ == Camera3RequestDescriptor::ProcessStatus::None)\n>> \t\treturn;\n> I'm in two minds about this. For the PostProcessor::processComplete()\n> signal, I agree, we could pass a status back as a signal parameter. It\n> would have the advantage of not having to set the request status in\n> quite a few locations in the post-processors. In that case, the status\n> should be a\n>\n> \tenum class Status {\n> \t\tSuccess,\n> \t\tError,\n> \t};\n>\n> in the PostProcessor class, as we don't need more than that.\n>\n> However, once we reach CameraStream::handleProcessComplete(), I think it\n> would make sense to store the status in Camera3RequestDescriptor, to\n> keep all request-related data together. This will be needed when\n> iterating over all descriptors in the queue to find out which ones are\n> complete. We would need a different Camera3RequestDescriptor::Status\n> there, and I think it should indicate the status of the request, not the\n> status of the post-processing. We would thus have\n>\n> \tenum class Status {\n> \t\tPending,\n> \t\tSuccess,\n> \t\tError,\n> \t};\n>\n> in Camera3RequestDescriptor.\n\n\nOk so I need to split status variables out into two separate enums?\n\n- One would target PostProcessor Status and sit in CameraStream\n\n- Other would target the descriptor's status and sit in \nCamera3RequestDesriptor\n\nI think we agreed upon a centralized location of status variables hence, \nit made sense to keep them in the descriptor itself\n\nI find the 2 split approach decent as well, so it should be fine.\n\n>\n>>> }\n>>>\n>>> /*\n>>> @@ -1150,6 +1156,8 @@ void CameraDevice::requestComplete(Request *request)\n>>> \t\t\tcontinue;\n>>> \t\t}\n>>>\n>>> +\t\tdescriptor->status_ = Camera3RequestDescriptor::ProcessStatus::Processing;\n>>> +\n>>> \t\tint ret = cameraStream->process(src, *buffer.buffer, descriptor);\n>>>\n>>> \t\t/*\n>>> @@ -1172,6 +1180,16 @@ void CameraDevice::requestComplete(Request *request)\n>>> \tdescriptors_.pop_front();\n>>> }\n>>>\n>>> +\n>>> +void CameraDevice::streamProcessingComplete([[maybe_unused]] CameraStream *cameraStream,\n>>> +\t\t\t\t\t [[maybe_unused]] Camera3RequestDescriptor *request)\n>>> +{\n>>> +\t/*\n>>> +\t * \\todo Stream processing is completed hence, check for errors and\n>>> +\t * if any, mark the corresponding buffer with CAMERA3_BUFFER_STATUS_ERROR.\n>>> +\t */\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 b2871e52..60c134dc 100644\n>>> --- a/src/android/camera_device.h\n>>> +++ b/src/android/camera_device.h\n>>> @@ -36,6 +36,13 @@\n>>> struct CameraConfigData;\n>>>\n>>> struct Camera3RequestDescriptor {\n>>> +\tenum class ProcessStatus {\n>>> +\t\tNone,\n>>> +\t\tProcessing,\n>>> +\t\tSuccess,\n>>> +\t\tError\n>>> +\t};\n>>> +\n>>> \tCamera3RequestDescriptor() = default;\n>>> \t~Camera3RequestDescriptor() = default;\n>>> \tCamera3RequestDescriptor(libcamera::Camera *camera,\n>>> @@ -48,6 +55,8 @@ struct Camera3RequestDescriptor {\n>>> \tCameraMetadata settings_;\n>>> \tstd::unique_ptr request_;\n>>> \tstd::unique_ptr resultMetadata_;\n>>> +\n>>> +\tProcessStatus status_;\n>>> };\n>>>\n>>> class CameraDevice : protected libcamera::Loggable\n>>> @@ -79,6 +88,8 @@ 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>>>\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 d494f5d5..70471959 100644\n>>> --- a/src/android/camera_stream.cpp\n>>> +++ b/src/android/camera_stream.cpp\n>>> @@ -81,6 +81,9 @@ 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, &CameraStream::handleProcessComplete);\n>>> \t}\n>>>\n>>> \tif (allocator_) {\n>>> @@ -102,8 +105,11 @@ int CameraStream::process(const FrameBuffer *source,\n>>> \t\t\t buffer_handle_t camera3Dest,\n>>> \t\t\t Camera3RequestDescriptor *request)\n>>> {\n>>> -\tif (!postProcessor_)\n>>> +\tif (!postProcessor_) {\n>>> +\t\trequest->status_ = Camera3RequestDescriptor::ProcessStatus::Error;\n>>> +\t\thandleProcessComplete(request);\n>>> \t\treturn 0;\n>>> +\t}\n>> That was like this before, but I feel like this is a development\n>> mistaken and should be better caught with Fatal\n>>\n>> same for the below !encoder and the YUV post-processor\n> Or\n>\n> \tASSERT(postProcessor_);\n>\n> ?\n>\n>>> \t/*\n>>> \t * \\todo Buffer mapping and processing should be moved to a\n>>> @@ -114,12 +120,19 @@ int CameraStream::process(const FrameBuffer *source,\n>>> \t\t\t PROT_READ | PROT_WRITE);\n>>> \tif (!dest.isValid()) {\n>>> \t\tLOG(HAL, Error) << \"Failed to map android blob buffer\";\n>>> +\t\trequest->status_ = Camera3RequestDescriptor::ProcessStatus::Error;\n>>> +\t\thandleProcessComplete(request);\n>>> \t\treturn -EINVAL;\n>>> \t}\n>>>\n>>> \treturn postProcessor_->process(source, &dest, request);\n>>> }\n>>>\n>>> +void CameraStream::handleProcessComplete(Camera3RequestDescriptor *request)\n>>> +{\n>>> +\tcameraDevice_->streamProcessingComplete(this, request);\n>>> +}\n>> Can't CameraStream connect CameraDevice::streamProcessingComplete with\n>> the post-processor's processComplete signal ?\n> We can do so with a lambda function, now supported by the signal class:\n>\n> \t\tpostProcessor_->processComplete.connect(\n> \t\t\t[&](Camera3RequestDescriptor *request) {\n> \t\t\t\tcameraDevice_->streamProcessingComplete(this, request);\n> \t\t\t}\n> \t\t);\n>\n>>> +\n>>> FrameBuffer *CameraStream::getBuffer()\n>>> {\n>>> \tif (!allocator_)\n>>> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n>>> index 68789700..f8ad6245 100644\n>>> --- a/src/android/camera_stream.h\n>>> +++ b/src/android/camera_stream.h\n>>> @@ -127,6 +127,8 @@ public:\n>>> \tvoid putBuffer(libcamera::FrameBuffer *buffer);\n>>>\n>>> private:\n>>> +\tvoid handleProcessComplete(Camera3RequestDescriptor *request);\n>>> +\n>>> \tCameraDevice *const cameraDevice_;\n>>> \tconst libcamera::CameraConfiguration *config_;\n>>> \tconst Type type_;\n>>> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n>>> index 31f68330..87252acd 100644\n>>> --- a/src/android/jpeg/post_processor_jpeg.cpp\n>>> +++ b/src/android/jpeg/post_processor_jpeg.cpp\n>>> @@ -101,8 +101,11 @@ int PostProcessorJpeg::process(const FrameBuffer *source,\n>>> \t\t\t CameraBuffer *destination,\n>>> \t\t\t Camera3RequestDescriptor *request)\n>>> {\n>>> -\tif (!encoder_)\n>>> +\tif (!encoder_) {\n>>> +\t\trequest->status_ = Camera3RequestDescriptor::ProcessStatus::Error;\n>>> +\t\tprocessComplete.emit(request);\n>>> \t\treturn 0;\n>>> +\t}\n>>>\n>>> \tASSERT(destination->numPlanes() == 1);\n>>>\n>>> @@ -197,6 +200,8 @@ 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\trequest->status_ = Camera3RequestDescriptor::ProcessStatus::Error;\n>>> +\t\tprocessComplete.emit(request);\n>>> \t\treturn jpeg_size;\n>>> \t}\n>>>\n>>> @@ -211,5 +216,8 @@ int PostProcessorJpeg::process(const FrameBuffer *source,\n>>> \t/* Update the JPEG result Metadata. */\n>>> \tresultMetadata->addEntry(ANDROID_JPEG_SIZE, jpeg_size);\n>>>\n>>> +\trequest->status_ = Camera3RequestDescriptor::ProcessStatus::Success;\n>>> +\tprocessComplete.emit(request);\n>>> +\n>>> \treturn 0;\n>>> }\n>>> diff --git a/src/android/post_processor.h b/src/android/post_processor.h\n>>> index bdd6afe7..f639b237 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>>> @@ -26,6 +28,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 5e18caee..b144649a 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,19 @@ 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\trequest->status_ = Camera3RequestDescriptor::ProcessStatus::Error;\n>>> +\t\tprocessComplete.emit(request);\n>> If my first suggestion will be considered, the post processor will\n>> send the status in the signal, and you can reduce the number of status\n>> to Success or Error maybe ?\n>>\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\trequest->status_ = Camera3RequestDescriptor::ProcessStatus::Error;\n>>> +\t\tprocessComplete.emit(request);\n>>> \t\treturn -EINVAL;\n>>> \t}\n>>>\n>>> @@ -76,9 +83,14 @@ 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\trequest->status_ = Camera3RequestDescriptor::ProcessStatus::Error;\n>>> +\t\tprocessComplete.emit(request);\n>>> \t\treturn -EINVAL;\n>>> \t}\n>>>\n>>> +\trequest->status_ = Camera3RequestDescriptor::ProcessStatus::Success;\n>>> +\tprocessComplete.emit(request);\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 0F7C3BF01C\n\tfor ;\n\tWed, 22 Sep 2021 08:44:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9561F6918E;\n\tWed, 22 Sep 2021 10:44:02 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7878F687DD\n\tfor ;\n\tWed, 22 Sep 2021 10:44:01 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.207])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1B92DF1;\n\tWed, 22 Sep 2021 10:43:59 +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=\"tx2F9dOx\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632300241;\n\tbh=JT69k93Ub38BfIAXV5p6cYr05hexlSGBZG8jKydT71M=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=tx2F9dOxBReqKdTGaTpNw4xkrgZbC5xHb8DMYDF5piVoh0QXXcMm+dFni8PPWiPMC\n\tUHxwdjFkU9xPXP0Mxwu/Wm4GwPwHQsgI8ztroz08RdpnHOlDPccGTysUSYjU25mgbU\n\tzNYp8MfxqpEIfs9Ply/COdLoE04TxbcGOBe6E2vA=","To":"Laurent Pinchart ,\n\tJacopo Mondi ","References":"<20210920173752.1346190-1-umang.jain@ideasonboard.com>\n\t<20210920173752.1346190-8-umang.jain@ideasonboard.com>\n\t<20210921112928.ru35uv6eittpmbxm@uno.localdomain>\n\t","From":"Umang Jain ","Message-ID":"","Date":"Wed, 22 Sep 2021 14:13:56 +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":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v3 07/10] android: post_processor:\n\tNotify post processing completion status","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":19782,"web_url":"https://patchwork.libcamera.org/comment/19782/","msgid":"","date":"2021-09-22T08:50:04","subject":"Re: [libcamera-devel] [PATCH v3 07/10] android: post_processor:\n\tNotify post processing completion status","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nOn Wed, Sep 22, 2021 at 02:13:56PM +0530, Umang Jain wrote:\n> On 9/22/21 4:56 AM, Laurent Pinchart wrote:\n> > On Tue, Sep 21, 2021 at 01:29:28PM +0200, Jacopo Mondi wrote:\n> >> On Mon, Sep 20, 2021 at 11:07:49PM +0530, Umang Jain wrote:\n> >>> We should be able to know if post-processing has been completed\n> >>> successfully or encountered some errors. This commit introduces a\n> >>> Signal<> that will notify that the post-processing has been completed.\n> >>> If the post processing was successful, status on the request descriptor\n> >>> will be set to Camera3RequestDescriptor::ProcessStatus::Success.\n> >>> The signal will be required when the post-processor is meant to\n> >>> run asynchronously (in subsequent commits) and capture results need\n> >>> to be sent back to the framework from the signal's slot instead.\n> >>>\n> >>> Signed-off-by: Umang Jain \n> >>> ---\n> >>> src/android/camera_device.cpp | 18 ++++++++++++++++++\n> >>> src/android/camera_device.h | 11 +++++++++++\n> >>> src/android/camera_stream.cpp | 15 ++++++++++++++-\n> >>> src/android/camera_stream.h | 2 ++\n> >>> src/android/jpeg/post_processor_jpeg.cpp | 10 +++++++++-\n> >>> src/android/post_processor.h | 4 ++++\n> >>> src/android/yuv/post_processor_yuv.cpp | 16 ++++++++++++++--\n> >>> 7 files changed, 72 insertions(+), 4 deletions(-)\n> >>>\n> >>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> >>> index fa462368..e2de4012 100644\n> >>> --- a/src/android/camera_device.cpp\n> >>> +++ b/src/android/camera_device.cpp\n> >>> @@ -246,6 +246,12 @@ Camera3RequestDescriptor::Camera3RequestDescriptor(\n> >>> \t */\n> >>> \trequest_ = std::make_unique(camera,\n> >>> \t\t\t\t\t\t reinterpret_cast(this));\n> >>> +\n> >>> +\t/*\n> >>> +\t * Denotes the post-processing status required by any stream. It is set\n> >>> +\t * to ProcessStatus::Success if processing is successful.\n> >>> +\t */\n> >>> +\tstatus_ = ProcessStatus::None;\n> >> Looking at how the status is used in CameraDevice, I would rather make\n> >> it a paramter of CameraDevice::streamProcessingComplete() and in the\n> >> following patch of sendCaptureResults().\n> >>\n> >> This way you could avoid there\n> >> \tif (d->status_ == Camera3RequestDescriptor::ProcessStatus::Processing ||\n> >> \t d->status_ == Camera3RequestDescriptor::ProcessStatus::None)\n> >> \t\treturn;\n> > I'm in two minds about this. For the PostProcessor::processComplete()\n> > signal, I agree, we could pass a status back as a signal parameter. It\n> > would have the advantage of not having to set the request status in\n> > quite a few locations in the post-processors. In that case, the status\n> > should be a\n> >\n> > \tenum class Status {\n> > \t\tSuccess,\n> > \t\tError,\n> > \t};\n> >\n> > in the PostProcessor class, as we don't need more than that.\n> >\n> > However, once we reach CameraStream::handleProcessComplete(), I think it\n> > would make sense to store the status in Camera3RequestDescriptor, to\n> > keep all request-related data together. This will be needed when\n> > iterating over all descriptors in the queue to find out which ones are\n> > complete. We would need a different Camera3RequestDescriptor::Status\n> > there, and I think it should indicate the status of the request, not the\n> > status of the post-processing. We would thus have\n> >\n> > \tenum class Status {\n> > \t\tPending,\n> > \t\tSuccess,\n> > \t\tError,\n> > \t};\n> >\n> > in Camera3RequestDescriptor.\n> \n> Ok so I need to split status variables out into two separate enums?\n> \n> - One would target PostProcessor Status and sit in CameraStream\n\nThe PostProcessor status enum should be in the PostProcessor class I\nthink. It will only be used in the post-processing completion signal, it\nwon't be stored in Camera3RequestDesriptor.\n\nOn a side note, as this enum will only have two values, I think it would\nmake sense to define a generic enum class Status { Success, Error } (or\nsimilar) in libcamera that could be reused everywhere when we need a\nboolean result.\n\n> - Other would target the descriptor's status and sit in \n> Camera3RequestDesriptor\n\nCorrect.\n\n> I think we agreed upon a centralized location of status variables hence, \n> it made sense to keep them in the descriptor itself\n\nYes, you'll need it to implement completion, by looping over the queue\nuntil you find a descriptor that has a Pending status.\n\n> I find the 2 split approach decent as well, so it should be fine.\n> \n> >>> }\n> >>>\n> >>> /*\n> >>> @@ -1150,6 +1156,8 @@ void CameraDevice::requestComplete(Request *request)\n> >>> \t\t\tcontinue;\n> >>> \t\t}\n> >>>\n> >>> +\t\tdescriptor->status_ = Camera3RequestDescriptor::ProcessStatus::Processing;\n> >>> +\n> >>> \t\tint ret = cameraStream->process(src, *buffer.buffer, descriptor);\n> >>>\n> >>> \t\t/*\n> >>> @@ -1172,6 +1180,16 @@ void CameraDevice::requestComplete(Request *request)\n> >>> \tdescriptors_.pop_front();\n> >>> }\n> >>>\n> >>> +\n> >>> +void CameraDevice::streamProcessingComplete([[maybe_unused]] CameraStream *cameraStream,\n> >>> +\t\t\t\t\t [[maybe_unused]] Camera3RequestDescriptor *request)\n> >>> +{\n> >>> +\t/*\n> >>> +\t * \\todo Stream processing is completed hence, check for errors and\n> >>> +\t * if any, mark the corresponding buffer with CAMERA3_BUFFER_STATUS_ERROR.\n> >>> +\t */\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 b2871e52..60c134dc 100644\n> >>> --- a/src/android/camera_device.h\n> >>> +++ b/src/android/camera_device.h\n> >>> @@ -36,6 +36,13 @@\n> >>> struct CameraConfigData;\n> >>>\n> >>> struct Camera3RequestDescriptor {\n> >>> +\tenum class ProcessStatus {\n> >>> +\t\tNone,\n> >>> +\t\tProcessing,\n> >>> +\t\tSuccess,\n> >>> +\t\tError\n> >>> +\t};\n> >>> +\n> >>> \tCamera3RequestDescriptor() = default;\n> >>> \t~Camera3RequestDescriptor() = default;\n> >>> \tCamera3RequestDescriptor(libcamera::Camera *camera,\n> >>> @@ -48,6 +55,8 @@ struct Camera3RequestDescriptor {\n> >>> \tCameraMetadata settings_;\n> >>> \tstd::unique_ptr request_;\n> >>> \tstd::unique_ptr resultMetadata_;\n> >>> +\n> >>> +\tProcessStatus status_;\n> >>> };\n> >>>\n> >>> class CameraDevice : protected libcamera::Loggable\n> >>> @@ -79,6 +88,8 @@ 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> >>>\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 d494f5d5..70471959 100644\n> >>> --- a/src/android/camera_stream.cpp\n> >>> +++ b/src/android/camera_stream.cpp\n> >>> @@ -81,6 +81,9 @@ 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, &CameraStream::handleProcessComplete);\n> >>> \t}\n> >>>\n> >>> \tif (allocator_) {\n> >>> @@ -102,8 +105,11 @@ int CameraStream::process(const FrameBuffer *source,\n> >>> \t\t\t buffer_handle_t camera3Dest,\n> >>> \t\t\t Camera3RequestDescriptor *request)\n> >>> {\n> >>> -\tif (!postProcessor_)\n> >>> +\tif (!postProcessor_) {\n> >>> +\t\trequest->status_ = Camera3RequestDescriptor::ProcessStatus::Error;\n> >>> +\t\thandleProcessComplete(request);\n> >>> \t\treturn 0;\n> >>> +\t}\n> >>\n> >> That was like this before, but I feel like this is a development\n> >> mistaken and should be better caught with Fatal\n> >>\n> >> same for the below !encoder and the YUV post-processor\n> >\n> > Or\n> >\n> > \tASSERT(postProcessor_);\n> >\n> > ?\n> >\n> >>> \t/*\n> >>> \t * \\todo Buffer mapping and processing should be moved to a\n> >>> @@ -114,12 +120,19 @@ int CameraStream::process(const FrameBuffer *source,\n> >>> \t\t\t PROT_READ | PROT_WRITE);\n> >>> \tif (!dest.isValid()) {\n> >>> \t\tLOG(HAL, Error) << \"Failed to map android blob buffer\";\n> >>> +\t\trequest->status_ = Camera3RequestDescriptor::ProcessStatus::Error;\n> >>> +\t\thandleProcessComplete(request);\n> >>> \t\treturn -EINVAL;\n> >>> \t}\n> >>>\n> >>> \treturn postProcessor_->process(source, &dest, request);\n> >>> }\n> >>>\n> >>> +void CameraStream::handleProcessComplete(Camera3RequestDescriptor *request)\n> >>> +{\n> >>> +\tcameraDevice_->streamProcessingComplete(this, request);\n> >>> +}\n> >>\n> >> Can't CameraStream connect CameraDevice::streamProcessingComplete with\n> >> the post-processor's processComplete signal ?\n> >\n> > We can do so with a lambda function, now supported by the signal class:\n> >\n> > \t\tpostProcessor_->processComplete.connect(\n> > \t\t\t[&](Camera3RequestDescriptor *request) {\n> > \t\t\t\tcameraDevice_->streamProcessingComplete(this, request);\n> > \t\t\t}\n> > \t\t);\n> >\n> >>> +\n> >>> FrameBuffer *CameraStream::getBuffer()\n> >>> {\n> >>> \tif (!allocator_)\n> >>> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n> >>> index 68789700..f8ad6245 100644\n> >>> --- a/src/android/camera_stream.h\n> >>> +++ b/src/android/camera_stream.h\n> >>> @@ -127,6 +127,8 @@ public:\n> >>> \tvoid putBuffer(libcamera::FrameBuffer *buffer);\n> >>>\n> >>> private:\n> >>> +\tvoid handleProcessComplete(Camera3RequestDescriptor *request);\n> >>> +\n> >>> \tCameraDevice *const cameraDevice_;\n> >>> \tconst libcamera::CameraConfiguration *config_;\n> >>> \tconst Type type_;\n> >>> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n> >>> index 31f68330..87252acd 100644\n> >>> --- a/src/android/jpeg/post_processor_jpeg.cpp\n> >>> +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> >>> @@ -101,8 +101,11 @@ int PostProcessorJpeg::process(const FrameBuffer *source,\n> >>> \t\t\t CameraBuffer *destination,\n> >>> \t\t\t Camera3RequestDescriptor *request)\n> >>> {\n> >>> -\tif (!encoder_)\n> >>> +\tif (!encoder_) {\n> >>> +\t\trequest->status_ = Camera3RequestDescriptor::ProcessStatus::Error;\n> >>> +\t\tprocessComplete.emit(request);\n> >>> \t\treturn 0;\n> >>> +\t}\n> >>>\n> >>> \tASSERT(destination->numPlanes() == 1);\n> >>>\n> >>> @@ -197,6 +200,8 @@ 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\trequest->status_ = Camera3RequestDescriptor::ProcessStatus::Error;\n> >>> +\t\tprocessComplete.emit(request);\n> >>> \t\treturn jpeg_size;\n> >>> \t}\n> >>>\n> >>> @@ -211,5 +216,8 @@ int PostProcessorJpeg::process(const FrameBuffer *source,\n> >>> \t/* Update the JPEG result Metadata. */\n> >>> \tresultMetadata->addEntry(ANDROID_JPEG_SIZE, jpeg_size);\n> >>>\n> >>> +\trequest->status_ = Camera3RequestDescriptor::ProcessStatus::Success;\n> >>> +\tprocessComplete.emit(request);\n> >>> +\n> >>> \treturn 0;\n> >>> }\n> >>> diff --git a/src/android/post_processor.h b/src/android/post_processor.h\n> >>> index bdd6afe7..f639b237 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> >>> @@ -26,6 +28,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 5e18caee..b144649a 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,19 @@ 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\trequest->status_ = Camera3RequestDescriptor::ProcessStatus::Error;\n> >>> +\t\tprocessComplete.emit(request);\n> >>\n> >> If my first suggestion will be considered, the post processor will\n> >> send the status in the signal, and you can reduce the number of status\n> >> to Success or Error maybe ?\n> >>\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\trequest->status_ = Camera3RequestDescriptor::ProcessStatus::Error;\n> >>> +\t\tprocessComplete.emit(request);\n> >>> \t\treturn -EINVAL;\n> >>> \t}\n> >>>\n> >>> @@ -76,9 +83,14 @@ 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\trequest->status_ = Camera3RequestDescriptor::ProcessStatus::Error;\n> >>> +\t\tprocessComplete.emit(request);\n> >>> \t\treturn -EINVAL;\n> >>> \t}\n> >>>\n> >>> +\trequest->status_ = Camera3RequestDescriptor::ProcessStatus::Success;\n> >>> +\tprocessComplete.emit(request);\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 76203BDC71\n\tfor ;\n\tWed, 22 Sep 2021 08:50:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B86E66918C;\n\tWed, 22 Sep 2021 10:50: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 2B5FF687DD\n\tfor ;\n\tWed, 22 Sep 2021 10:50:08 +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 31C08F1;\n\tWed, 22 Sep 2021 10:50:06 +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=\"rO3b+aeW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632300607;\n\tbh=qHyzzRRnob4bGTRIZPDCVNjlyRPGNH24PDDLx6uL2Ok=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=rO3b+aeWQBQViKm6THbMOSU1rF3fvBKTdqwsUy2r4W1FqUMkbfpsdTzHw+8HFs6/K\n\tmcPeICCsVN5m+kdYMXtImqLHdIeAIztPhHR7Xl0bQoRSTqL839LFFyT4rTgwf+R0Ob\n\trEufEKojmCh/ldO6jEqGVjOKxvGSvxzHaQAhG0rM=","Date":"Wed, 22 Sep 2021 11:50:04 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20210920173752.1346190-1-umang.jain@ideasonboard.com>\n\t<20210920173752.1346190-8-umang.jain@ideasonboard.com>\n\t<20210921112928.ru35uv6eittpmbxm@uno.localdomain>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 07/10] android: post_processor:\n\tNotify post processing completion status","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\" "}}]