[libcamera-devel,v7,4/6] Move generateThumbnail from PostProcessorJpeg to Encoder

17926 diff mbox series

From: Harvey Yang <chenghaoyang@chromium.org>

In the following patch, generateThumbnail will have a different
implementation in the jea encoder. Therefore, this patch moves the
generateThumbnail function from PostProcessorJpeg to Encoder.

Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
---
 src/android/jpeg/encoder.h               |   5 +
 src/android/jpeg/encoder_libjpeg.cpp     | 122 ++++++++++++++++++-----
 src/android/jpeg/encoder_libjpeg.h       |  28 +++++-
 src/android/jpeg/post_processor_jpeg.cpp |  54 +---------
 src/android/jpeg/post_processor_jpeg.h   |  11 +-
 5 files changed, 130 insertions(+), 90 deletions(-)

Hi Harvey,

Thank you for the patch.

On Thu, Dec 01, 2022 at 09:27:31AM +0000, Harvey Yang via libcamera-devel wrote:
> From: Harvey Yang <chenghaoyang@chromium.org>
> 
> In the following patch, generateThumbnail will have a different
> implementation in the jea encoder. Therefore, this patch moves the
> generateThumbnail function from PostProcessorJpeg to Encoder.
> 
> Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
> ---
>  src/android/jpeg/encoder.h               |   5 +
>  src/android/jpeg/encoder_libjpeg.cpp     | 122 ++++++++++++++++++-----
>  src/android/jpeg/encoder_libjpeg.h       |  28 +++++-
>  src/android/jpeg/post_processor_jpeg.cpp |  54 +---------
>  src/android/jpeg/post_processor_jpeg.h   |  11 +-
>  5 files changed, 130 insertions(+), 90 deletions(-)
> 
> diff --git a/src/android/jpeg/encoder.h b/src/android/jpeg/encoder.h
> index b974d367..7dc1ee27 100644
> --- a/src/android/jpeg/encoder.h
> +++ b/src/android/jpeg/encoder.h
> @@ -22,4 +22,9 @@ public:
>  			   libcamera::Span<uint8_t> destination,
>  			   libcamera::Span<const uint8_t> exifData,
>  			   unsigned int quality) = 0;
> +	virtual int generateThumbnail(
> +		const libcamera::FrameBuffer &source,
> +		const libcamera::Size &targetSize,
> +		unsigned int quality,
> +		std::vector<unsigned char> *thumbnail) = 0;

	virtual int generateThumbnail(const libcamera::FrameBuffer &source,
				      const libcamera::Size &targetSize,
				      unsigned int quality,
				      std::vector<unsigned char> *thumbnail) = 0;

to be consistent with the coding style. Same below where applicable.

>  };
> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp
> index fd62bd9c..cc37fde3 100644
> --- a/src/android/jpeg/encoder_libjpeg.cpp
> +++ b/src/android/jpeg/encoder_libjpeg.cpp
> @@ -71,29 +71,43 @@ const struct JPEGPixelFormatInfo &findPixelInfo(const PixelFormat &format)
>  EncoderLibJpeg::EncoderLibJpeg()
>  {
>  	/* \todo Expand error handling coverage with a custom handler. */
> -	compress_.err = jpeg_std_error(&jerr_);
> +	image_data_.compress.err = jpeg_std_error(&image_data_.jerr);
> +	thumbnail_data_.compress.err = jpeg_std_error(&thumbnail_data_.jerr);
>  
> -	jpeg_create_compress(&compress_);
> +	jpeg_create_compress(&image_data_.compress);
> +	jpeg_create_compress(&thumbnail_data_.compress);
>  }
>  
>  EncoderLibJpeg::~EncoderLibJpeg()
>  {
> -	jpeg_destroy_compress(&compress_);
> +	jpeg_destroy_compress(&image_data_.compress);
> +	jpeg_destroy_compress(&thumbnail_data_.compress);
>  }
>  
>  int EncoderLibJpeg::configure(const StreamConfiguration &cfg)
>  {
> -	const struct JPEGPixelFormatInfo info = findPixelInfo(cfg.pixelFormat);
> +	thumbnailer_.configure(cfg.size, cfg.pixelFormat);
> +
> +	return configureEncoder(&image_data_.compress, cfg.pixelFormat,
> +				cfg.size);
> +}
> +
> +int EncoderLibJpeg::configureEncoder(struct jpeg_compress_struct *compress,
> +				     libcamera::PixelFormat pixelFormat,
> +				     libcamera::Size size)
> +{
> +	const struct JPEGPixelFormatInfo info = findPixelInfo(pixelFormat);
> +
>  	if (info.colorSpace == JCS_UNKNOWN)
>  		return -ENOTSUP;
>  
> -	compress_.image_width = cfg.size.width;
> -	compress_.image_height = cfg.size.height;
> -	compress_.in_color_space = info.colorSpace;
> +	compress->image_width = size.width;
> +	compress->image_height = size.height;
> +	compress->in_color_space = info.colorSpace;
>  
> -	compress_.input_components = info.colorSpace == JCS_GRAYSCALE ? 1 : 3;
> +	compress->input_components = info.colorSpace == JCS_GRAYSCALE ? 1 : 3;
>  
> -	jpeg_set_defaults(&compress_);
> +	jpeg_set_defaults(compress);
>  
>  	pixelFormatInfo_ = &info.pixelFormatInfo;
>  
> @@ -107,13 +121,13 @@ void EncoderLibJpeg::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. */
> -	unsigned int stride = pixelFormatInfo_->stride(compress_.image_width, 0);
> +	unsigned int stride = pixelFormatInfo_->stride(compress_->image_width, 0);
>  
>  	JSAMPROW row_pointer[1];
>  
> -	while (compress_.next_scanline < compress_.image_height) {
> -		row_pointer[0] = &src[compress_.next_scanline * stride];
> -		jpeg_write_scanlines(&compress_, row_pointer, 1);
> +	while (compress_->next_scanline < compress_->image_height) {
> +		row_pointer[0] = &src[compress_->next_scanline * stride];
> +		jpeg_write_scanlines(compress_, row_pointer, 1);
>  	}
>  }
>  
> @@ -123,7 +137,7 @@ void EncoderLibJpeg::compressRGB(const std::vector<Span<uint8_t>> &planes)
>   */
>  void EncoderLibJpeg::compressNV(const std::vector<Span<uint8_t>> &planes)
>  {
> -	uint8_t tmprowbuf[compress_.image_width * 3];
> +	uint8_t tmprowbuf[compress_->image_width * 3];
>  
>  	/*
>  	 * \todo Use the raw api, and only unpack the cb/cr samples to new line
> @@ -133,10 +147,10 @@ void EncoderLibJpeg::compressNV(const std::vector<Span<uint8_t>> &planes)
>  	 * Possible hints at:
>  	 * https://sourceforge.net/p/libjpeg/mailman/message/30815123/
>  	 */
> -	unsigned int y_stride = pixelFormatInfo_->stride(compress_.image_width, 0);
> -	unsigned int c_stride = pixelFormatInfo_->stride(compress_.image_width, 1);
> +	unsigned int y_stride = pixelFormatInfo_->stride(compress_->image_width, 0);
> +	unsigned int c_stride = pixelFormatInfo_->stride(compress_->image_width, 1);
>  
> -	unsigned int horzSubSample = 2 * compress_.image_width / c_stride;
> +	unsigned int horzSubSample = 2 * compress_->image_width / c_stride;
>  	unsigned int vertSubSample = pixelFormatInfo_->planes[1].verticalSubSampling;
>  
>  	unsigned int c_inc = horzSubSample == 1 ? 2 : 0;
> @@ -149,14 +163,14 @@ void EncoderLibJpeg::compressNV(const std::vector<Span<uint8_t>> &planes)
>  	JSAMPROW row_pointer[1];
>  	row_pointer[0] = &tmprowbuf[0];
>  
> -	for (unsigned int y = 0; y < compress_.image_height; y++) {
> +	for (unsigned int y = 0; y < compress_->image_height; y++) {
>  		unsigned char *dst = &tmprowbuf[0];
>  
>  		const unsigned char *src_y = src + y * y_stride;
>  		const unsigned char *src_cb = src_c + (y / vertSubSample) * c_stride + cb_pos;
>  		const unsigned char *src_cr = src_c + (y / vertSubSample) * c_stride + cr_pos;
>  
> -		for (unsigned int x = 0; x < compress_.image_width; x += 2) {
> +		for (unsigned int x = 0; x < compress_->image_width; x += 2) {
>  			dst[0] = *src_y;
>  			dst[1] = *src_cb;
>  			dst[2] = *src_cr;
> @@ -174,13 +188,67 @@ void EncoderLibJpeg::compressNV(const std::vector<Span<uint8_t>> &planes)
>  			dst += 3;
>  		}
>  
> -		jpeg_write_scanlines(&compress_, row_pointer, 1);
> +		jpeg_write_scanlines(compress_, row_pointer, 1);
>  	}
>  }
>  
> +int EncoderLibJpeg::generateThumbnail(const libcamera::FrameBuffer &source,
> +				      const libcamera::Size &targetSize,
> +				      unsigned int quality,
> +				      std::vector<unsigned char> *thumbnail)
> +{
> +	/* Stores the raw scaled-down thumbnail bytes. */
> +	std::vector<unsigned char> rawThumbnail;
> +
> +	thumbnailer_.createThumbnail(source, targetSize, &rawThumbnail);
> +
> +	int ret = configureEncoder(&thumbnail_data_.compress,
> +				   thumbnailer_.pixelFormat(), targetSize);
> +	compress_ = &thumbnail_data_.compress;

This is all becoming a bit of spaghetti code with the compress_ pointer.
It feels like you're bundling two classes into one. It would be cleaner,
I think, to create an internal JpegEncoder class (similar to your
JpegData, I'd name it EncoderLibJpeg::Encoder) that handles one encoder
(moving all the private data members of EncoderLibJpeg to that class).
You could then instantiate it twice as private data members of
EncoderLibJpeg. What do you think ? The refactoring of EncoderLibJpeg
should go in first, before moving the thumbnailer to it.

> +
> +	if (!rawThumbnail.empty() && !ret) {
> +		/*
> +		 * \todo Avoid value-initialization of all elements of the
> +		 * vector.
> +		 */
> +		thumbnail->resize(rawThumbnail.size());
> +
> +		/*
> +		 * Split planes manually as the encoder expects a vector of
> +		 * planes.
> +		 *
> +		 * \todo Pass a vector of planes directly to
> +		 * Thumbnailer::createThumbnailer above and remove the manual
> +		 * planes split from here.
> +		 */
> +		std::vector<Span<uint8_t>> thumbnailPlanes;
> +		const PixelFormatInfo &formatNV12 =
> +			PixelFormatInfo::info(formats::NV12);
> +		size_t yPlaneSize = formatNV12.planeSize(targetSize, 0);
> +		size_t uvPlaneSize = formatNV12.planeSize(targetSize, 1);
> +		thumbnailPlanes.push_back({ rawThumbnail.data(), yPlaneSize });
> +		thumbnailPlanes.push_back({ rawThumbnail.data() + yPlaneSize,
> +					    uvPlaneSize });
> +
> +		int jpeg_size = encode(thumbnailPlanes, *thumbnail, {},
> +				       quality);

camelCase (I know it was like this already, but let's fix it).

> +		thumbnail->resize(jpeg_size);
> +
> +		LOG(JPEG, Debug)
> +			<< "Thumbnail compress returned "
> +			<< jpeg_size << " bytes";
> +
> +		return jpeg_size;
> +	}
> +
> +	return -1;

Also while at it, retur

> +}
> +
>  int EncoderLibJpeg::encode(const FrameBuffer &source, Span<uint8_t> dest,
>  			   Span<const uint8_t> exifData, unsigned int quality)
>  {
> +	compress_ = &image_data_.compress;
> +
>  	MappedFrameBuffer frame(&source, MappedFrameBuffer::MapFlag::Read);
>  	if (!frame.isValid()) {
>  		LOG(JPEG, Error) << "Failed to map FrameBuffer : "
> @@ -198,7 +266,7 @@ int EncoderLibJpeg::encode(const std::vector<Span<uint8_t>> &src,
>  	unsigned char *destination = dest.data();
>  	unsigned long size = dest.size();
>  
> -	jpeg_set_quality(&compress_, quality, TRUE);
> +	jpeg_set_quality(compress_, quality, TRUE);
>  
>  	/*
>  	 * The jpeg_mem_dest will reallocate if the required size is not
> @@ -208,18 +276,18 @@ int EncoderLibJpeg::encode(const std::vector<Span<uint8_t>> &src,
>  	 * \todo Implement our own custom memory destination to prevent
>  	 * reallocation and prefer failure with correct reporting.
>  	 */
> -	jpeg_mem_dest(&compress_, &destination, &size);
> +	jpeg_mem_dest(compress_, &destination, &size);
>  
> -	jpeg_start_compress(&compress_, TRUE);
> +	jpeg_start_compress(compress_, TRUE);
>  
>  	if (exifData.size())
>  		/* Store Exif data in the JPEG_APP1 data block. */
> -		jpeg_write_marker(&compress_, JPEG_APP0 + 1,
> +		jpeg_write_marker(compress_, JPEG_APP0 + 1,
>  				  static_cast<const JOCTET *>(exifData.data()),
>  				  exifData.size());
>  
> -	LOG(JPEG, Debug) << "JPEG Encode Starting:" << compress_.image_width
> -			 << "x" << compress_.image_height;
> +	LOG(JPEG, Debug) << "JPEG Encode Starting:" << compress_->image_width
> +			 << "x" << compress_->image_height;
>  
>  	ASSERT(src.size() == pixelFormatInfo_->numPlanes());
>  
> @@ -228,7 +296,7 @@ int EncoderLibJpeg::encode(const std::vector<Span<uint8_t>> &src,
>  	else
>  		compressRGB(src);
>  
> -	jpeg_finish_compress(&compress_);
> +	jpeg_finish_compress(compress_);
>  
>  	return size;
>  }
> diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h
> index 1b3ac067..1ec2ba13 100644
> --- a/src/android/jpeg/encoder_libjpeg.h
> +++ b/src/android/jpeg/encoder_libjpeg.h
> @@ -15,6 +15,8 @@
>  
>  #include <jpeglib.h>
>  
> +#include "thumbnailer.h"
> +
>  class EncoderLibJpeg : public Encoder
>  {
>  public:
> @@ -26,20 +28,40 @@ public:
>  		   libcamera::Span<uint8_t> destination,
>  		   libcamera::Span<const uint8_t> exifData,
>  		   unsigned int quality) override;
> +	int generateThumbnail(
> +		const libcamera::FrameBuffer &source,
> +		const libcamera::Size &targetSize,
> +		unsigned int quality,
> +		std::vector<unsigned char> *thumbnail) override;

This patch would be easier to review if it didn't bundle a change of
return type for this function. Furthermore, it should be explained in
the commit message. Is the return type change needed here, or later in
the series ? If later in the series, I'd split it to a separate patch.
In general, please try to have only one change per patch to simplify
review.

> +
> +private:
> +	struct JpegData {
> +		struct jpeg_compress_struct compress;
> +		struct jpeg_error_mgr jerr;
> +	};
> +
> +	int configureEncoder(struct jpeg_compress_struct *compress,
> +			     libcamera::PixelFormat pixelFormat,
> +			     libcamera::Size size);
> +
>  	int encode(const std::vector<libcamera::Span<uint8_t>> &planes,
>  		   libcamera::Span<uint8_t> destination,
>  		   libcamera::Span<const uint8_t> exifData,
>  		   unsigned int quality);
>  
> -private:
>  	void compressRGB(const std::vector<libcamera::Span<uint8_t>> &planes);
>  	void compressNV(const std::vector<libcamera::Span<uint8_t>> &planes);
>  
> -	struct jpeg_compress_struct compress_;
> -	struct jpeg_error_mgr jerr_;
> +	JpegData image_data_;
> +	JpegData thumbnail_data_;

camelCase for both.

> +
> +	// |&image_data_.compress| or |&thumbnail_data_.compress|.

C-style comments (/* ... */).

> +	struct jpeg_compress_struct *compress_;
>  
>  	const libcamera::PixelFormatInfo *pixelFormatInfo_;
>  
>  	bool nv_;
>  	bool nvSwap_;
> +
> +	Thumbnailer thumbnailer_;
>  };
> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
> index 0cf56716..60eebb11 100644
> --- a/src/android/jpeg/post_processor_jpeg.cpp
> +++ b/src/android/jpeg/post_processor_jpeg.cpp
> @@ -44,60 +44,11 @@ int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,
>  
>  	streamSize_ = outCfg.size;
>  
> -	thumbnailer_.configure(inCfg.size, inCfg.pixelFormat);
> -
>  	encoder_ = std::make_unique<EncoderLibJpeg>();
>  
>  	return encoder_->configure(inCfg);
>  }
>  
> -void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,
> -					  const Size &targetSize,
> -					  unsigned int quality,
> -					  std::vector<unsigned char> *thumbnail)
> -{
> -	/* Stores the raw scaled-down thumbnail bytes. */
> -	std::vector<unsigned char> rawThumbnail;
> -
> -	thumbnailer_.createThumbnail(source, targetSize, &rawThumbnail);
> -
> -	StreamConfiguration thCfg;
> -	thCfg.size = targetSize;
> -	thCfg.pixelFormat = thumbnailer_.pixelFormat();
> -	int ret = thumbnailEncoder_.configure(thCfg);
> -
> -	if (!rawThumbnail.empty() && !ret) {
> -		/*
> -		 * \todo Avoid value-initialization of all elements of the
> -		 * vector.
> -		 */
> -		thumbnail->resize(rawThumbnail.size());
> -
> -		/*
> -		 * Split planes manually as the encoder expects a vector of
> -		 * planes.
> -		 *
> -		 * \todo Pass a vector of planes directly to
> -		 * Thumbnailer::createThumbnailer above and remove the manual
> -		 * planes split from here.
> -		 */
> -		std::vector<Span<uint8_t>> thumbnailPlanes;
> -		const PixelFormatInfo &formatNV12 = PixelFormatInfo::info(formats::NV12);
> -		size_t yPlaneSize = formatNV12.planeSize(targetSize, 0);
> -		size_t uvPlaneSize = formatNV12.planeSize(targetSize, 1);
> -		thumbnailPlanes.push_back({ rawThumbnail.data(), yPlaneSize });
> -		thumbnailPlanes.push_back({ rawThumbnail.data() + yPlaneSize, uvPlaneSize });
> -
> -		int jpeg_size = thumbnailEncoder_.encode(thumbnailPlanes,
> -							 *thumbnail, {}, quality);
> -		thumbnail->resize(jpeg_size);
> -
> -		LOG(JPEG, Debug)
> -			<< "Thumbnail compress returned "
> -			<< jpeg_size << " bytes";
> -	}
> -}
> -
>  void PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBuffer)
>  {
>  	ASSERT(encoder_);
> @@ -164,8 +115,9 @@ void PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBu
>  
>  		if (thumbnailSize != Size(0, 0)) {
>  			std::vector<unsigned char> thumbnail;
> -			generateThumbnail(source, thumbnailSize, quality, &thumbnail);
> -			if (!thumbnail.empty())
> +			ret = encoder_->generateThumbnail(source, thumbnailSize,
> +							  quality, &thumbnail);
> +			if (ret > 0 && !thumbnail.empty())

Do you need to check both conditions ? Also, the generateThumbnail()
function returns -1 in case of error or the JPEG data size otherwise,
while you only check ret > 0 here. I'd return a bool from the function
and check that only, moving the thumbnail.empty() check (if needed)
inside generateThumbnail() to simplify the API.

>  				exif.setThumbnail(std::move(thumbnail), Exif::Compression::JPEG);
>  		}
>  
> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h
> index 98309b01..55b23d7d 100644
> --- a/src/android/jpeg/post_processor_jpeg.h
> +++ b/src/android/jpeg/post_processor_jpeg.h
> @@ -8,11 +8,11 @@
>  #pragma once
>  
>  #include "../post_processor.h"
> -#include "encoder_libjpeg.h"
> -#include "thumbnailer.h"
>  
>  #include <libcamera/geometry.h>
>  
> +#include "encoder.h"
> +

You can add a

class Encoder;

declaration instead (just below CameraDevice).

>  class CameraDevice;
>  
>  class PostProcessorJpeg : public PostProcessor
> @@ -25,14 +25,7 @@ public:
>  	void process(Camera3RequestDescriptor::StreamBuffer *streamBuffer) override;
>  
>  private:
> -	void generateThumbnail(const libcamera::FrameBuffer &source,
> -			       const libcamera::Size &targetSize,
> -			       unsigned int quality,
> -			       std::vector<unsigned char> *thumbnail);
> -
>  	CameraDevice *const cameraDevice_;
>  	std::unique_ptr<Encoder> encoder_;
>  	libcamera::Size streamSize_;
> -	EncoderLibJpeg thumbnailEncoder_;
> -	Thumbnailer thumbnailer_;
>  };

Thank you Laurent for the review!

On Wed, Dec 7, 2022 at 12:24 PM Laurent Pinchart <
laurent.pinchart@ideasonboard.com> wrote:

> Hi Harvey,
>
> Thank you for the patch.
>
> On Thu, Dec 01, 2022 at 09:27:31AM +0000, Harvey Yang via libcamera-devel
> wrote:
> > From: Harvey Yang <chenghaoyang@chromium.org>
> >
> > In the following patch, generateThumbnail will have a different
> > implementation in the jea encoder. Therefore, this patch moves the
> > generateThumbnail function from PostProcessorJpeg to Encoder.
> >
> > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
> > ---
> >  src/android/jpeg/encoder.h               |   5 +
> >  src/android/jpeg/encoder_libjpeg.cpp     | 122 ++++++++++++++++++-----
> >  src/android/jpeg/encoder_libjpeg.h       |  28 +++++-
> >  src/android/jpeg/post_processor_jpeg.cpp |  54 +---------
> >  src/android/jpeg/post_processor_jpeg.h   |  11 +-
> >  5 files changed, 130 insertions(+), 90 deletions(-)
> >
> > diff --git a/src/android/jpeg/encoder.h b/src/android/jpeg/encoder.h
> > index b974d367..7dc1ee27 100644
> > --- a/src/android/jpeg/encoder.h
> > +++ b/src/android/jpeg/encoder.h
> > @@ -22,4 +22,9 @@ public:
> >                          libcamera::Span<uint8_t> destination,
> >                          libcamera::Span<const uint8_t> exifData,
> >                          unsigned int quality) = 0;
> > +     virtual int generateThumbnail(
> > +             const libcamera::FrameBuffer &source,
> > +             const libcamera::Size &targetSize,
> > +             unsigned int quality,
> > +             std::vector<unsigned char> *thumbnail) = 0;
>
>         virtual int generateThumbnail(const libcamera::FrameBuffer &source,
>                                       const libcamera::Size &targetSize,
>                                       unsigned int quality,
>                                       std::vector<unsigned char>
> *thumbnail) = 0;
>
> to be consistent with the coding style. Same below where applicable.
>
>
Done.


> >  };
> > diff --git a/src/android/jpeg/encoder_libjpeg.cpp
> b/src/android/jpeg/encoder_libjpeg.cpp
> > index fd62bd9c..cc37fde3 100644
> > --- a/src/android/jpeg/encoder_libjpeg.cpp
> > +++ b/src/android/jpeg/encoder_libjpeg.cpp
> > @@ -71,29 +71,43 @@ const struct JPEGPixelFormatInfo
> &findPixelInfo(const PixelFormat &format)
> >  EncoderLibJpeg::EncoderLibJpeg()
> >  {
> >       /* \todo Expand error handling coverage with a custom handler. */
> > -     compress_.err = jpeg_std_error(&jerr_);
> > +     image_data_.compress.err = jpeg_std_error(&image_data_.jerr);
> > +     thumbnail_data_.compress.err =
> jpeg_std_error(&thumbnail_data_.jerr);
> >
> > -     jpeg_create_compress(&compress_);
> > +     jpeg_create_compress(&image_data_.compress);
> > +     jpeg_create_compress(&thumbnail_data_.compress);
> >  }
> >
> >  EncoderLibJpeg::~EncoderLibJpeg()
> >  {
> > -     jpeg_destroy_compress(&compress_);
> > +     jpeg_destroy_compress(&image_data_.compress);
> > +     jpeg_destroy_compress(&thumbnail_data_.compress);
> >  }
> >
> >  int EncoderLibJpeg::configure(const StreamConfiguration &cfg)
> >  {
> > -     const struct JPEGPixelFormatInfo info =
> findPixelInfo(cfg.pixelFormat);
> > +     thumbnailer_.configure(cfg.size, cfg.pixelFormat);
> > +
> > +     return configureEncoder(&image_data_.compress, cfg.pixelFormat,
> > +                             cfg.size);
> > +}
> > +
> > +int EncoderLibJpeg::configureEncoder(struct jpeg_compress_struct
> *compress,
> > +                                  libcamera::PixelFormat pixelFormat,
> > +                                  libcamera::Size size)
> > +{
> > +     const struct JPEGPixelFormatInfo info = findPixelInfo(pixelFormat);
> > +
> >       if (info.colorSpace == JCS_UNKNOWN)
> >               return -ENOTSUP;
> >
> > -     compress_.image_width = cfg.size.width;
> > -     compress_.image_height = cfg.size.height;
> > -     compress_.in_color_space = info.colorSpace;
> > +     compress->image_width = size.width;
> > +     compress->image_height = size.height;
> > +     compress->in_color_space = info.colorSpace;
> >
> > -     compress_.input_components = info.colorSpace == JCS_GRAYSCALE ? 1
> : 3;
> > +     compress->input_components = info.colorSpace == JCS_GRAYSCALE ? 1
> : 3;
> >
> > -     jpeg_set_defaults(&compress_);
> > +     jpeg_set_defaults(compress);
> >
> >       pixelFormatInfo_ = &info.pixelFormatInfo;
> >
> > @@ -107,13 +121,13 @@ void EncoderLibJpeg::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.
> */
> > -     unsigned int stride =
> pixelFormatInfo_->stride(compress_.image_width, 0);
> > +     unsigned int stride =
> pixelFormatInfo_->stride(compress_->image_width, 0);
> >
> >       JSAMPROW row_pointer[1];
> >
> > -     while (compress_.next_scanline < compress_.image_height) {
> > -             row_pointer[0] = &src[compress_.next_scanline * stride];
> > -             jpeg_write_scanlines(&compress_, row_pointer, 1);
> > +     while (compress_->next_scanline < compress_->image_height) {
> > +             row_pointer[0] = &src[compress_->next_scanline * stride];
> > +             jpeg_write_scanlines(compress_, row_pointer, 1);
> >       }
> >  }
> >
> > @@ -123,7 +137,7 @@ void EncoderLibJpeg::compressRGB(const
> std::vector<Span<uint8_t>> &planes)
> >   */
> >  void EncoderLibJpeg::compressNV(const std::vector<Span<uint8_t>>
> &planes)
> >  {
> > -     uint8_t tmprowbuf[compress_.image_width * 3];
> > +     uint8_t tmprowbuf[compress_->image_width * 3];
> >
> >       /*
> >        * \todo Use the raw api, and only unpack the cb/cr samples to new
> line
> > @@ -133,10 +147,10 @@ void EncoderLibJpeg::compressNV(const
> std::vector<Span<uint8_t>> &planes)
> >        * Possible hints at:
> >        * https://sourceforge.net/p/libjpeg/mailman/message/30815123/
> >        */
> > -     unsigned int y_stride =
> pixelFormatInfo_->stride(compress_.image_width, 0);
> > -     unsigned int c_stride =
> pixelFormatInfo_->stride(compress_.image_width, 1);
> > +     unsigned int y_stride =
> pixelFormatInfo_->stride(compress_->image_width, 0);
> > +     unsigned int c_stride =
> pixelFormatInfo_->stride(compress_->image_width, 1);
> >
> > -     unsigned int horzSubSample = 2 * compress_.image_width / c_stride;
> > +     unsigned int horzSubSample = 2 * compress_->image_width / c_stride;
> >       unsigned int vertSubSample =
> pixelFormatInfo_->planes[1].verticalSubSampling;
> >
> >       unsigned int c_inc = horzSubSample == 1 ? 2 : 0;
> > @@ -149,14 +163,14 @@ void EncoderLibJpeg::compressNV(const
> std::vector<Span<uint8_t>> &planes)
> >       JSAMPROW row_pointer[1];
> >       row_pointer[0] = &tmprowbuf[0];
> >
> > -     for (unsigned int y = 0; y < compress_.image_height; y++) {
> > +     for (unsigned int y = 0; y < compress_->image_height; y++) {
> >               unsigned char *dst = &tmprowbuf[0];
> >
> >               const unsigned char *src_y = src + y * y_stride;
> >               const unsigned char *src_cb = src_c + (y / vertSubSample)
> * c_stride + cb_pos;
> >               const unsigned char *src_cr = src_c + (y / vertSubSample)
> * c_stride + cr_pos;
> >
> > -             for (unsigned int x = 0; x < compress_.image_width; x +=
> 2) {
> > +             for (unsigned int x = 0; x < compress_->image_width; x +=
> 2) {
> >                       dst[0] = *src_y;
> >                       dst[1] = *src_cb;
> >                       dst[2] = *src_cr;
> > @@ -174,13 +188,67 @@ void EncoderLibJpeg::compressNV(const
> std::vector<Span<uint8_t>> &planes)
> >                       dst += 3;
> >               }
> >
> > -             jpeg_write_scanlines(&compress_, row_pointer, 1);
> > +             jpeg_write_scanlines(compress_, row_pointer, 1);
> >       }
> >  }
> >
> > +int EncoderLibJpeg::generateThumbnail(const libcamera::FrameBuffer
> &source,
> > +                                   const libcamera::Size &targetSize,
> > +                                   unsigned int quality,
> > +                                   std::vector<unsigned char>
> *thumbnail)
> > +{
> > +     /* Stores the raw scaled-down thumbnail bytes. */
> > +     std::vector<unsigned char> rawThumbnail;
> > +
> > +     thumbnailer_.createThumbnail(source, targetSize, &rawThumbnail);
> > +
> > +     int ret = configureEncoder(&thumbnail_data_.compress,
> > +                                thumbnailer_.pixelFormat(), targetSize);
> > +     compress_ = &thumbnail_data_.compress;
>
> This is all becoming a bit of spaghetti code with the compress_ pointer.
> It feels like you're bundling two classes into one. It would be cleaner,
> I think, to create an internal JpegEncoder class (similar to your
> JpegData, I'd name it EncoderLibJpeg::Encoder) that handles one encoder
> (moving all the private data members of EncoderLibJpeg to that class).
> You could then instantiate it twice as private data members of
> EncoderLibJpeg. What do you think ? The refactoring of EncoderLibJpeg
> should go in first, before moving the thumbnailer to it.
>
>
Yes, you're absolutely right. This version actually has conflicts in
configuring
the capture and thumbnail with shared member variables.

Updated with your suggestion that EncoderLibJpeg is a wrapper that consists
of two real encoders for captures and thumbnails respectively.

I also separate this patch into two: the first adds the internal encoder,
and the
second moves the thumbnailer and add the |thumbnailEncoder_| in
EncoderLibJpeg.

Please check if I missed anything.


> > +
> > +     if (!rawThumbnail.empty() && !ret) {
> > +             /*
> > +              * \todo Avoid value-initialization of all elements of the
> > +              * vector.
> > +              */
> > +             thumbnail->resize(rawThumbnail.size());
> > +
> > +             /*
> > +              * Split planes manually as the encoder expects a vector of
> > +              * planes.
> > +              *
> > +              * \todo Pass a vector of planes directly to
> > +              * Thumbnailer::createThumbnailer above and remove the
> manual
> > +              * planes split from here.
> > +              */
> > +             std::vector<Span<uint8_t>> thumbnailPlanes;
> > +             const PixelFormatInfo &formatNV12 =
> > +                     PixelFormatInfo::info(formats::NV12);
> > +             size_t yPlaneSize = formatNV12.planeSize(targetSize, 0);
> > +             size_t uvPlaneSize = formatNV12.planeSize(targetSize, 1);
> > +             thumbnailPlanes.push_back({ rawThumbnail.data(),
> yPlaneSize });
> > +             thumbnailPlanes.push_back({ rawThumbnail.data() +
> yPlaneSize,
> > +                                         uvPlaneSize });
> > +
> > +             int jpeg_size = encode(thumbnailPlanes, *thumbnail, {},
> > +                                    quality);
>
> camelCase (I know it was like this already, but let's fix it).
>
>
Done.


> > +             thumbnail->resize(jpeg_size);
> > +
> > +             LOG(JPEG, Debug)
> > +                     << "Thumbnail compress returned "
> > +                     << jpeg_size << " bytes";
> > +
> > +             return jpeg_size;
> > +     }
> > +
> > +     return -1;
>
> Also while at it, retur
>
>
`return -EINVAL;`?


> > +}
> > +
> >  int EncoderLibJpeg::encode(const FrameBuffer &source, Span<uint8_t>
> dest,
> >                          Span<const uint8_t> exifData, unsigned int
> quality)
> >  {
> > +     compress_ = &image_data_.compress;
> > +
> >       MappedFrameBuffer frame(&source, MappedFrameBuffer::MapFlag::Read);
> >       if (!frame.isValid()) {
> >               LOG(JPEG, Error) << "Failed to map FrameBuffer : "
> > @@ -198,7 +266,7 @@ int EncoderLibJpeg::encode(const
> std::vector<Span<uint8_t>> &src,
> >       unsigned char *destination = dest.data();
> >       unsigned long size = dest.size();
> >
> > -     jpeg_set_quality(&compress_, quality, TRUE);
> > +     jpeg_set_quality(compress_, quality, TRUE);
> >
> >       /*
> >        * The jpeg_mem_dest will reallocate if the required size is not
> > @@ -208,18 +276,18 @@ int EncoderLibJpeg::encode(const
> std::vector<Span<uint8_t>> &src,
> >        * \todo Implement our own custom memory destination to prevent
> >        * reallocation and prefer failure with correct reporting.
> >        */
> > -     jpeg_mem_dest(&compress_, &destination, &size);
> > +     jpeg_mem_dest(compress_, &destination, &size);
> >
> > -     jpeg_start_compress(&compress_, TRUE);
> > +     jpeg_start_compress(compress_, TRUE);
> >
> >       if (exifData.size())
> >               /* Store Exif data in the JPEG_APP1 data block. */
> > -             jpeg_write_marker(&compress_, JPEG_APP0 + 1,
> > +             jpeg_write_marker(compress_, JPEG_APP0 + 1,
> >                                 static_cast<const JOCTET
> *>(exifData.data()),
> >                                 exifData.size());
> >
> > -     LOG(JPEG, Debug) << "JPEG Encode Starting:" <<
> compress_.image_width
> > -                      << "x" << compress_.image_height;
> > +     LOG(JPEG, Debug) << "JPEG Encode Starting:" <<
> compress_->image_width
> > +                      << "x" << compress_->image_height;
> >
> >       ASSERT(src.size() == pixelFormatInfo_->numPlanes());
> >
> > @@ -228,7 +296,7 @@ int EncoderLibJpeg::encode(const
> std::vector<Span<uint8_t>> &src,
> >       else
> >               compressRGB(src);
> >
> > -     jpeg_finish_compress(&compress_);
> > +     jpeg_finish_compress(compress_);
> >
> >       return size;
> >  }
> > diff --git a/src/android/jpeg/encoder_libjpeg.h
> b/src/android/jpeg/encoder_libjpeg.h
> > index 1b3ac067..1ec2ba13 100644
> > --- a/src/android/jpeg/encoder_libjpeg.h
> > +++ b/src/android/jpeg/encoder_libjpeg.h
> > @@ -15,6 +15,8 @@
> >
> >  #include <jpeglib.h>
> >
> > +#include "thumbnailer.h"
> > +
> >  class EncoderLibJpeg : public Encoder
> >  {
> >  public:
> > @@ -26,20 +28,40 @@ public:
> >                  libcamera::Span<uint8_t> destination,
> >                  libcamera::Span<const uint8_t> exifData,
> >                  unsigned int quality) override;
> > +     int generateThumbnail(
> > +             const libcamera::FrameBuffer &source,
> > +             const libcamera::Size &targetSize,
> > +             unsigned int quality,
> > +             std::vector<unsigned char> *thumbnail) override;
>
> This patch would be easier to review if it didn't bundle a change of
> return type for this function. Furthermore, it should be explained in
> the commit message. Is the return type change needed here, or later in
> the series ? If later in the series, I'd split it to a separate patch.
> In general, please try to have only one change per patch to simplify
> review.
>
>
Right, the return value is actually not needed. Removed.


> > +
> > +private:
> > +     struct JpegData {
> > +             struct jpeg_compress_struct compress;
> > +             struct jpeg_error_mgr jerr;
> > +     };
> > +
> > +     int configureEncoder(struct jpeg_compress_struct *compress,
> > +                          libcamera::PixelFormat pixelFormat,
> > +                          libcamera::Size size);
> > +
> >       int encode(const std::vector<libcamera::Span<uint8_t>> &planes,
> >                  libcamera::Span<uint8_t> destination,
> >                  libcamera::Span<const uint8_t> exifData,
> >                  unsigned int quality);
> >
> > -private:
> >       void compressRGB(const std::vector<libcamera::Span<uint8_t>>
> &planes);
> >       void compressNV(const std::vector<libcamera::Span<uint8_t>>
> &planes);
> >
> > -     struct jpeg_compress_struct compress_;
> > -     struct jpeg_error_mgr jerr_;
> > +     JpegData image_data_;
> > +     JpegData thumbnail_data_;
>
> camelCase for both.
>
>
Removed.


> > +
> > +     // |&image_data_.compress| or |&thumbnail_data_.compress|.
>
> C-style comments (/* ... */).
>
> > +     struct jpeg_compress_struct *compress_;
> >
> >       const libcamera::PixelFormatInfo *pixelFormatInfo_;
> >
> >       bool nv_;
> >       bool nvSwap_;
> > +
> > +     Thumbnailer thumbnailer_;
> >  };
> > diff --git a/src/android/jpeg/post_processor_jpeg.cpp
> b/src/android/jpeg/post_processor_jpeg.cpp
> > index 0cf56716..60eebb11 100644
> > --- a/src/android/jpeg/post_processor_jpeg.cpp
> > +++ b/src/android/jpeg/post_processor_jpeg.cpp
> > @@ -44,60 +44,11 @@ int PostProcessorJpeg::configure(const
> StreamConfiguration &inCfg,
> >
> >       streamSize_ = outCfg.size;
> >
> > -     thumbnailer_.configure(inCfg.size, inCfg.pixelFormat);
> > -
> >       encoder_ = std::make_unique<EncoderLibJpeg>();
> >
> >       return encoder_->configure(inCfg);
> >  }
> >
> > -void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,
> > -                                       const Size &targetSize,
> > -                                       unsigned int quality,
> > -                                       std::vector<unsigned char>
> *thumbnail)
> > -{
> > -     /* Stores the raw scaled-down thumbnail bytes. */
> > -     std::vector<unsigned char> rawThumbnail;
> > -
> > -     thumbnailer_.createThumbnail(source, targetSize, &rawThumbnail);
> > -
> > -     StreamConfiguration thCfg;
> > -     thCfg.size = targetSize;
> > -     thCfg.pixelFormat = thumbnailer_.pixelFormat();
> > -     int ret = thumbnailEncoder_.configure(thCfg);
> > -
> > -     if (!rawThumbnail.empty() && !ret) {
> > -             /*
> > -              * \todo Avoid value-initialization of all elements of the
> > -              * vector.
> > -              */
> > -             thumbnail->resize(rawThumbnail.size());
> > -
> > -             /*
> > -              * Split planes manually as the encoder expects a vector of
> > -              * planes.
> > -              *
> > -              * \todo Pass a vector of planes directly to
> > -              * Thumbnailer::createThumbnailer above and remove the
> manual
> > -              * planes split from here.
> > -              */
> > -             std::vector<Span<uint8_t>> thumbnailPlanes;
> > -             const PixelFormatInfo &formatNV12 =
> PixelFormatInfo::info(formats::NV12);
> > -             size_t yPlaneSize = formatNV12.planeSize(targetSize, 0);
> > -             size_t uvPlaneSize = formatNV12.planeSize(targetSize, 1);
> > -             thumbnailPlanes.push_back({ rawThumbnail.data(),
> yPlaneSize });
> > -             thumbnailPlanes.push_back({ rawThumbnail.data() +
> yPlaneSize, uvPlaneSize });
> > -
> > -             int jpeg_size = thumbnailEncoder_.encode(thumbnailPlanes,
> > -                                                      *thumbnail, {},
> quality);
> > -             thumbnail->resize(jpeg_size);
> > -
> > -             LOG(JPEG, Debug)
> > -                     << "Thumbnail compress returned "
> > -                     << jpeg_size << " bytes";
> > -     }
> > -}
> > -
> >  void PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer
> *streamBuffer)
> >  {
> >       ASSERT(encoder_);
> > @@ -164,8 +115,9 @@ void
> PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBu
> >
> >               if (thumbnailSize != Size(0, 0)) {
> >                       std::vector<unsigned char> thumbnail;
> > -                     generateThumbnail(source, thumbnailSize, quality,
> &thumbnail);
> > -                     if (!thumbnail.empty())
> > +                     ret = encoder_->generateThumbnail(source,
> thumbnailSize,
> > +                                                       quality,
> &thumbnail);
> > +                     if (ret > 0 && !thumbnail.empty())
>
> Do you need to check both conditions ? Also, the generateThumbnail()
> function returns -1 in case of error or the JPEG data size otherwise,
> while you only check ret > 0 here. I'd return a bool from the function
> and check that only, moving the thumbnail.empty() check (if needed)
> inside generateThumbnail() to simplify the API.
>
>
Removed the return value.


> >                               exif.setThumbnail(std::move(thumbnail),
> Exif::Compression::JPEG);
> >               }
> >
> > diff --git a/src/android/jpeg/post_processor_jpeg.h
> b/src/android/jpeg/post_processor_jpeg.h
> > index 98309b01..55b23d7d 100644
> > --- a/src/android/jpeg/post_processor_jpeg.h
> > +++ b/src/android/jpeg/post_processor_jpeg.h
> > @@ -8,11 +8,11 @@
> >  #pragma once
> >
> >  #include "../post_processor.h"
> > -#include "encoder_libjpeg.h"
> > -#include "thumbnailer.h"
> >
> >  #include <libcamera/geometry.h>
> >
> > +#include "encoder.h"
> > +
>
> You can add a
>
> class Encoder;
>
> declaration instead (just below CameraDevice).
>
> >  class CameraDevice;
> >
> >  class PostProcessorJpeg : public PostProcessor
> > @@ -25,14 +25,7 @@ public:
> >       void process(Camera3RequestDescriptor::StreamBuffer *streamBuffer)
> override;
> >
> >  private:
> > -     void generateThumbnail(const libcamera::FrameBuffer &source,
> > -                            const libcamera::Size &targetSize,
> > -                            unsigned int quality,
> > -                            std::vector<unsigned char> *thumbnail);
> > -
> >       CameraDevice *const cameraDevice_;
> >       std::unique_ptr<Encoder> encoder_;
> >       libcamera::Size streamSize_;
> > -     EncoderLibJpeg thumbnailEncoder_;
> > -     Thumbnailer thumbnailer_;
> >  };
>
> --
> Regards,
>
> Laurent Pinchart
>