[libcamera-devel] [PATCH v2 2/4] android: Modify Encoder interface

[Umang Jain]

Hi Hiro,

Thanks for your work.

On 10/20/20 1:12 PM, Hirokazu Honda wrote:
> In Encoder::encode(), the |source| argument doesn't have to be a
> pointer. This replaces its type, const pointer, with const
> reference as the latter is preferred to the former.
> libcamera::Span is chea to construct/copy/move. We should deal
s/chea/cheap
> 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>
LGTM again.
Reviewed-by: Umang Jain <email at uajain.com>
> ---
>   src/android/jpeg/encoder.h               | 4 ++--
>   src/android/jpeg/encoder_libjpeg.cpp     | 6 +++---
>   src/android/jpeg/encoder_libjpeg.h       | 4 ++--
>   src/android/jpeg/post_processor_jpeg.cpp | 2 +-
>   4 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/src/android/jpeg/encoder.h b/src/android/jpeg/encoder.h
> index 4483153..4fe71cf 100644
> --- a/src/android/jpeg/encoder.h
> +++ b/src/android/jpeg/encoder.h
> @@ -17,8 +17,8 @@ public:
>   	virtual ~Encoder() {};
>   
>   	virtual int configure(const libcamera::StreamConfiguration &cfg) = 0;
> -	virtual int encode(const libcamera::FrameBuffer *source,
> -			   const libcamera::Span<uint8_t> &destination,
> +	virtual int encode(const libcamera::FrameBuffer &source,
> +			   libcamera::Span<uint8_t> destination,
>   			   const libcamera::Span<const uint8_t> &exifData) = 0;
>   };
>   
> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp
> index 8995ba5..4236c9d 100644
> --- a/src/android/jpeg/encoder_libjpeg.cpp
> +++ b/src/android/jpeg/encoder_libjpeg.cpp
> @@ -179,11 +179,11 @@ void EncoderLibJpeg::compressNV(const libcamera::MappedBuffer *frame)
>   	}
>   }
>   
> -int EncoderLibJpeg::encode(const FrameBuffer *source,
> -			   const libcamera::Span<uint8_t> &dest,
> +int EncoderLibJpeg::encode(const FrameBuffer &source,
> +			   libcamera::Span<uint8_t> dest,
>   			   const libcamera::Span<const uint8_t> &exifData)
>   {
> -	MappedFrameBuffer frame(source, PROT_READ);
> +	MappedFrameBuffer frame(&source, PROT_READ);
>   	if (!frame.isValid()) {
>   		LOG(JPEG, Error) << "Failed to map FrameBuffer : "
>   				 << strerror(frame.error());
> diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h
> index 934caef..391a53c 100644
> --- a/src/android/jpeg/encoder_libjpeg.h
> +++ b/src/android/jpeg/encoder_libjpeg.h
> @@ -21,8 +21,8 @@ public:
>   	~EncoderLibJpeg();
>   
>   	int configure(const libcamera::StreamConfiguration &cfg) override;
> -	int encode(const libcamera::FrameBuffer *source,
> -		   const libcamera::Span<uint8_t> &destination,
> +	int encode(const libcamera::FrameBuffer &source,
> +		   libcamera::Span<uint8_t> destination,
>   		   const libcamera::Span<const uint8_t> &exifData) override;
>   
>   private:
> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
> index 55b9088..6f33631 100644
> --- a/src/android/jpeg/post_processor_jpeg.cpp
> +++ b/src/android/jpeg/post_processor_jpeg.cpp
> @@ -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());
Perfect. I was expecting this from v1.
>   	if (jpeg_size < 0) {
>   		LOG(JPEG, Error) << "Failed to encode stream image";
>   		return jpeg_size;