[libcamera-devel] [PATCH v8 4/7] Add an internal Encoder class in EncoderLibJpeg

[Laurent Pinchart]

Hi Harvey,

Thank you for the patch.

The subject requires a prefix. "android: jpeg: " would be appropriate.
Same comment for all patches in this series.

On Wed, Dec 14, 2022 at 09:33:27AM +0000, Harvey Yang via libcamera-devel wrote:
> To move the thumbnail encoder into EncoderLibJpeg in the following
> patch, this patch adds a wrapper/internal Encoder class that allows
> EncoderLibJpeg to have more than one encoder to tackle both captures
> and thumbnails.

I'd write "to tackle encoding of both captured images and their
thumbnails".

> Signed-off-by: Harvey Yang <chenghaoyang at chromium.org>
> ---
>  src/android/jpeg/encoder_libjpeg.cpp | 32 +++++++++++++++++++++-------
>  src/android/jpeg/encoder_libjpeg.h   | 30 ++++++++++++++++++++------
>  2 files changed, 47 insertions(+), 15 deletions(-)
> 
> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp
> index fd62bd9c..d849547f 100644
> --- a/src/android/jpeg/encoder_libjpeg.cpp
> +++ b/src/android/jpeg/encoder_libjpeg.cpp
> @@ -68,7 +68,16 @@ const struct JPEGPixelFormatInfo &findPixelInfo(const PixelFormat &format)
>  
>  } /* namespace */
>  
> -EncoderLibJpeg::EncoderLibJpeg()
> +EncoderLibJpeg::EncoderLibJpeg() = default;
> +
> +EncoderLibJpeg::~EncoderLibJpeg() = default;
> +
> +int EncoderLibJpeg::configure(const StreamConfiguration &cfg)
> +{
> +	return encoder_.configure(cfg);
> +}
> +
> +EncoderLibJpeg::Encoder::Encoder()
>  {
>  	/* \todo Expand error handling coverage with a custom handler. */
>  	compress_.err = jpeg_std_error(&jerr_);
> @@ -76,12 +85,12 @@ EncoderLibJpeg::EncoderLibJpeg()
>  	jpeg_create_compress(&compress_);
>  }
>  
> -EncoderLibJpeg::~EncoderLibJpeg()
> +EncoderLibJpeg::Encoder::~Encoder()
>  {
>  	jpeg_destroy_compress(&compress_);
>  }
>  
> -int EncoderLibJpeg::configure(const StreamConfiguration &cfg)
> +int EncoderLibJpeg::Encoder::configure(const StreamConfiguration &cfg)
>  {
>  	const struct JPEGPixelFormatInfo info = findPixelInfo(cfg.pixelFormat);
>  	if (info.colorSpace == JCS_UNKNOWN)
> @@ -103,7 +112,7 @@ int EncoderLibJpeg::configure(const StreamConfiguration &cfg)
>  	return 0;
>  }
>  
> -void EncoderLibJpeg::compressRGB(const std::vector<Span<uint8_t>> &planes)
> +void EncoderLibJpeg::Encoder::compressRGB(const std::vector<Span<uint8_t>> &planes)
>  {
>  	unsigned char *src = const_cast<unsigned char *>(planes[0].data());
>  	/* \todo Stride information should come from buffer configuration. */
> @@ -121,7 +130,7 @@ void EncoderLibJpeg::compressRGB(const std::vector<Span<uint8_t>> &planes)
>   * Compress the incoming buffer from a supported NV format.
>   * This naively unpacks the semi-planar NV12 to a YUV888 format for libjpeg.
>   */
> -void EncoderLibJpeg::compressNV(const std::vector<Span<uint8_t>> &planes)
> +void EncoderLibJpeg::Encoder::compressNV(const std::vector<Span<uint8_t>> &planes)
>  {
>  	uint8_t tmprowbuf[compress_.image_width * 3];
>  
> @@ -188,12 +197,19 @@ int EncoderLibJpeg::encode(const FrameBuffer &source, Span<uint8_t> dest,
>  		return frame.error();
>  	}
>  
> -	return encode(frame.planes(), dest, exifData, quality);
> +	return encoder_.encode(frame.planes(), dest, exifData, quality);
>  }
>  
>  int EncoderLibJpeg::encode(const std::vector<Span<uint8_t>> &src,

Let's not mix-n-match the EncoderLibJpeg and EncoderLibJpeg::Encoder
functions

> -			   Span<uint8_t> dest, Span<const uint8_t> exifData,
> -			   unsigned int quality)
> +				    Span<uint8_t> dest, Span<const uint8_t> exifData,
> +				    unsigned int quality)


Wrong indentation. This looks like an unneeded change.

> +{
> +	return encoder_.encode(src, std::move(dest), std::move(exifData), quality);

I don't think you need std::move here.

> +}
> +
> +int EncoderLibJpeg::Encoder::encode(const std::vector<Span<uint8_t>> &src,
> +				    Span<uint8_t> dest, Span<const uint8_t> exifData,
> +				    unsigned int quality)
>  {
>  	unsigned char *destination = dest.data();
>  	unsigned long size = dest.size();
> diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h
> index 1b3ac067..97182b96 100644
> --- a/src/android/jpeg/encoder_libjpeg.h
> +++ b/src/android/jpeg/encoder_libjpeg.h
> @@ -32,14 +32,30 @@ public:
>  		   unsigned int quality);
>  
>  private:
> -	void compressRGB(const std::vector<libcamera::Span<uint8_t>> &planes);
> -	void compressNV(const std::vector<libcamera::Span<uint8_t>> &planes);
> +	class Encoder
> +	{
> +	public:
> +		Encoder();
> +		~Encoder();
>  
> -	struct jpeg_compress_struct compress_;
> -	struct jpeg_error_mgr jerr_;
> +		int encode(const std::vector<libcamera::Span<uint8_t>> &planes,
> +			   libcamera::Span<uint8_t> destination,
> +			   libcamera::Span<const uint8_t> exifData,
> +			   unsigned int quality);
> +		int configure(const libcamera::StreamConfiguration &cfg);

Please declare the functions in the same order as in the EncoderLibJpeg
class, with configure before encode, and define functions in the same
order in the .cpp file.

I can handle all these changes when applying the patch if nothing else
in the series requires sending a new version. Otherwise, after fixing
these, you can add my 

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

>  
> -	const libcamera::PixelFormatInfo *pixelFormatInfo_;
> +	private:
> +		void compressRGB(const std::vector<libcamera::Span<uint8_t>> &planes);
> +		void compressNV(const std::vector<libcamera::Span<uint8_t>> &planes);
>  
> -	bool nv_;
> -	bool nvSwap_;
> +		struct jpeg_compress_struct compress_;
> +		struct jpeg_error_mgr jerr_;
> +
> +		const libcamera::PixelFormatInfo *pixelFormatInfo_;
> +
> +		bool nv_;
> +		bool nvSwap_;
> +	};
> +
> +	Encoder encoder_;
>  };

-- 
Regards,

Laurent Pinchart