[libcamera-devel] [PATCH v2 4/9] android: post_processor: Notify post processing completion status

[Laurent Pinchart]

Hi Umang,

Thank you for the patch.

On Fri, Sep 10, 2021 at 12:36:33PM +0530, Umang Jain wrote:
> We should be able to know if post-processing has been completed
> successfully or encountered some errors. This commit introduces a
> Signal<> which when connected, will notify that the post-processing

The signal notifies completion regardless of whether it's connected or
not, the same way that a tree falling in an empty forest still makes
sound :-)

> has been completed. The status of PostProcessor::process() will be
> passed as a PostProcessor::Status argument. The signal will be required
> when the post-processor is meant to run asynchronously (in subsequent
> commits).
> 
> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> ---
>  src/android/jpeg/post_processor_jpeg.cpp | 7 ++++++-
>  src/android/post_processor.h             | 8 ++++++++
>  src/android/yuv/post_processor_yuv.cpp   | 4 ++++
>  3 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
> index 741eeb21..1df3dfb1 100644
> --- a/src/android/jpeg/post_processor_jpeg.cpp
> +++ b/src/android/jpeg/post_processor_jpeg.cpp
> @@ -103,8 +103,10 @@ int PostProcessorJpeg::process(const FrameBuffer *source,
>  			       CameraMetadata *resultMetadata,
>  			       const Camera3RequestDescriptor *context)
>  {
> -	if (!encoder_)
> +	if (!encoder_) {
> +		processComplete.emit(PostProcessor::Status::Failed, context);

As PostProcessorJpeg inherits from PostProcessor, I think you can write

		processComplete.emit(Status::Failed, context);

here. Same below.

>  		return 0;
> +	}
>  
>  	ASSERT(destination->numPlanes() == 1);
>  
> @@ -197,6 +199,7 @@ int PostProcessorJpeg::process(const FrameBuffer *source,
>  					 exif.data(), quality);
>  	if (jpeg_size < 0) {
>  		LOG(JPEG, Error) << "Failed to encode stream image";
> +		processComplete.emit(PostProcessor::Status::Failed, context);
>  		return jpeg_size;
>  	}
>  
> @@ -211,5 +214,7 @@ int PostProcessorJpeg::process(const FrameBuffer *source,
>  	/* Update the JPEG result Metadata. */
>  	resultMetadata->addEntry(ANDROID_JPEG_SIZE, jpeg_size);
>  
> +	processComplete.emit(PostProcessor::Status::Succeeded, context);
> +
>  	return 0;
>  }
> diff --git a/src/android/post_processor.h b/src/android/post_processor.h
> index 1213e7e6..25c9411f 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 <libcamera/base/signal.h>
> +
>  #include <libcamera/framebuffer.h>
>  #include <libcamera/stream.h>
>  
> @@ -28,6 +30,12 @@ public:
>  			    const CameraMetadata &requestMetadata,
>  			    CameraMetadata *resultMetadata,
>  			    const Camera3RequestDescriptor *context) = 0;
> +
> +	enum Status {
> +		Succeeded,
> +		Failed,
> +	};

This should move right after "public", we define types before fucntions.

We have

	enum Status {
		FrameSuccess,
		FrameError,
		FrameCancelled,
	};

in FrameMetadata, I'd thus name the enumerators Success and Error.

I'd also make this an enum class to make usage of the Status:: prefix
mandatory, given that you already use it everywhere.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +	libcamera::Signal<Status, const Camera3RequestDescriptor *> 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 13a78abe..d2df110a 100644
> --- a/src/android/yuv/post_processor_yuv.cpp
> +++ b/src/android/yuv/post_processor_yuv.cpp
> @@ -61,6 +61,7 @@ int PostProcessorYuv::process(const FrameBuffer *source,
>  	const MappedFrameBuffer sourceMapped(source, MappedFrameBuffer::MapFlag::Read);
>  	if (!sourceMapped.isValid()) {
>  		LOG(YUV, Error) << "Failed to mmap camera frame buffer";
> +		processComplete.emit(PostProcessor::Status::Failed, context);
>  		return -EINVAL;
>  	}
>  
> @@ -78,9 +79,12 @@ int PostProcessorYuv::process(const FrameBuffer *source,
>  				    libyuv::FilterMode::kFilterBilinear);
>  	if (ret) {
>  		LOG(YUV, Error) << "Failed NV12 scaling: " << ret;
> +		processComplete.emit(PostProcessor::Status::Failed, context);
>  		return -EINVAL;
>  	}
>  
> +	processComplete.emit(PostProcessor::Status::Succeeded, context);
> +
>  	return 0;
>  }
>  

-- 
Regards,

Laurent Pinchart