[libcamera-devel] [PATCH v2 1/4] android: Modify PostProcessor interface

[Umang Jain]

Hi Hiro,

On 10/20/20 1:12 PM, Hirokazu Honda wrote:
> In PostProcessor::process(), the |source| argument doesn't have
> to be a pointer. This replace its type, const pointer, with const
> reference as the latter is preferred to the former.
> libcamera::Span is cheap to construct/copy/move. We should deal
> with the type as pass-by-value parameter. Therefore this also
> drops the const reference in the |destination| argument.
>
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>

This looks good to me.
Reviewed-by: Umang Jain <email at uajain.com>
> ---
>   src/android/camera_stream.cpp            | 2 +-
>   src/android/jpeg/post_processor_jpeg.cpp | 6 +++---
>   src/android/jpeg/post_processor_jpeg.h   | 4 ++--
>   src/android/post_processor.h             | 4 ++--
>   4 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> index 1b8afa8..28a6e09 100644
> --- a/src/android/camera_stream.cpp
> +++ b/src/android/camera_stream.cpp
> @@ -102,7 +102,7 @@ int CameraStream::process(const libcamera::FrameBuffer &source,
>   	if (!postProcessor_)
>   		return 0;
>   
> -	return postProcessor_->process(&source, dest->maps()[0], metadata);
> +	return postProcessor_->process(source, dest->maps()[0], metadata);
>   }
>   
>   FrameBuffer *CameraStream::getBuffer()
> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
> index 753c28e..55b9088 100644
> --- a/src/android/jpeg/post_processor_jpeg.cpp
> +++ b/src/android/jpeg/post_processor_jpeg.cpp
> @@ -44,8 +44,8 @@ int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,
>   	return encoder_->configure(inCfg);
>   }
>   
> -int PostProcessorJpeg::process(const libcamera::FrameBuffer *source,
> -			       const libcamera::Span<uint8_t> &destination,
> +int PostProcessorJpeg::process(const libcamera::FrameBuffer &source,
> +			       libcamera::Span<uint8_t> destination,
>   			       CameraMetadata *metadata)
>   {
>   	if (!encoder_)
> @@ -67,7 +67,7 @@ int PostProcessorJpeg::process(const libcamera::FrameBuffer *source,
>   	if (exif.generate() != 0)
>   		LOG(JPEG, Error) << "Failed to generate valid EXIF data";
>   
> -	int jpeg_size = encoder_->encode(source, destination, exif.data());
> +	int jpeg_size = encoder_->encode(&source, destination, exif.data());
I see that this change is required to keep the patch compile-able 
because the argument's type has been changed. I would expect this would 
be reworked in next patches while correctly the encoder interface.
>   	if (jpeg_size < 0) {
>   		LOG(JPEG, Error) << "Failed to encode stream image";
>   		return jpeg_size;
> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h
> index 62c8650..ae636ff 100644
> --- a/src/android/jpeg/post_processor_jpeg.h
> +++ b/src/android/jpeg/post_processor_jpeg.h
> @@ -23,8 +23,8 @@ public:
>   
>   	int configure(const libcamera::StreamConfiguration &incfg,
>   		      const libcamera::StreamConfiguration &outcfg) override;
> -	int process(const libcamera::FrameBuffer *source,
> -		    const libcamera::Span<uint8_t> &destination,
> +	int process(const libcamera::FrameBuffer &source,
> +		    libcamera::Span<uint8_t> destination,
>   		    CameraMetadata *metadata) override;
>   
>   private:
> diff --git a/src/android/post_processor.h b/src/android/post_processor.h
> index a891c43..5f87a5d 100644
> --- a/src/android/post_processor.h
> +++ b/src/android/post_processor.h
> @@ -20,8 +20,8 @@ public:
>   
>   	virtual int configure(const libcamera::StreamConfiguration &inCfg,
>   			      const libcamera::StreamConfiguration &outCfg) = 0;
> -	virtual int process(const libcamera::FrameBuffer *source,
> -			    const libcamera::Span<uint8_t> &destination,
> +	virtual int process(const libcamera::FrameBuffer &source,
> +			    libcamera::Span<uint8_t> destination,
>   			    CameraMetadata *metadata) = 0;
>   };
>