[libcamera-devel,3/3] android: jpeg: exif: Embed a JPEG-encoded thumbnail

10255 diff mbox series

Add a basic image thumbnailer for NV12 frames being captured.
It shall generate a thumbnail image to be embedded as a part of
EXIF metadata of the frame. The output of the thumbnail will still
be NV12.

Signed-off-by: Umang Jain <email@uajain.com>
---
 src/android/jpeg/exif.cpp                |  16 +++-
 src/android/jpeg/exif.h                  |   1 +
 src/android/jpeg/post_processor_jpeg.cpp |  35 +++++++-
 src/android/jpeg/post_processor_jpeg.h   |   8 +-
 src/android/jpeg/thumbnailer.cpp         | 109 +++++++++++++++++++++++
 src/android/jpeg/thumbnailer.h           |  37 ++++++++
 src/android/meson.build                  |   1 +
 7 files changed, 204 insertions(+), 3 deletions(-)
 create mode 100644 src/android/jpeg/thumbnailer.cpp
 create mode 100644 src/android/jpeg/thumbnailer.h

Hi Umang,

Thank you for the patch.

On Mon, Oct 26, 2020 at 07:31:34PM +0530, Umang Jain wrote:
> Add a basic image thumbnailer for NV12 frames being captured.
> It shall generate a thumbnail image to be embedded as a part of
> EXIF metadata of the frame. The output of the thumbnail will still
> be NV12.
> 
> Signed-off-by: Umang Jain <email@uajain.com>
> ---
>  src/android/jpeg/exif.cpp                |  16 +++-
>  src/android/jpeg/exif.h                  |   1 +
>  src/android/jpeg/post_processor_jpeg.cpp |  35 +++++++-
>  src/android/jpeg/post_processor_jpeg.h   |   8 +-
>  src/android/jpeg/thumbnailer.cpp         | 109 +++++++++++++++++++++++
>  src/android/jpeg/thumbnailer.h           |  37 ++++++++
>  src/android/meson.build                  |   1 +
>  7 files changed, 204 insertions(+), 3 deletions(-)
>  create mode 100644 src/android/jpeg/thumbnailer.cpp
>  create mode 100644 src/android/jpeg/thumbnailer.h
> 
> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp
> index d21534a..24197bd 100644
> --- a/src/android/jpeg/exif.cpp
> +++ b/src/android/jpeg/exif.cpp
> @@ -75,8 +75,16 @@ Exif::~Exif()
>  	if (exifData_)
>  		free(exifData_);
>  
> -	if (data_)
> +	if (data_) {
> +		/*
> +		 * Reset thumbnail data to avoid getting double-freed by
> +		 * libexif. It is owned by the caller (i.e. PostProcessorJpeg).
> +		 */
> +		data_->data = nullptr;
> +		data_->size = 0;
> +
>  		exif_data_unref(data_);
> +	}
>  
>  	if (mem_)
>  		exif_mem_unref(mem_);
> @@ -268,6 +276,12 @@ void Exif::setOrientation(int orientation)
>  	setShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, value);
>  }
>  
> +void Exif::setThumbnail(std::vector<unsigned char> &thumbnail)
> +{
> +	data_->data = thumbnail.data();
> +	data_->size = thumbnail.size();
> +}
> +
>  [[nodiscard]] int Exif::generate()
>  {
>  	if (exifData_) {
> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h
> index 12c27b6..bd54a31 100644
> --- a/src/android/jpeg/exif.h
> +++ b/src/android/jpeg/exif.h
> @@ -26,6 +26,7 @@ public:
>  
>  	void setOrientation(int orientation);
>  	void setSize(const libcamera::Size &size);
> +	void setThumbnail(std::vector<unsigned char> &thumbnail);

You can pass a Span<const unsigned char> as the setThumbnail() function
shouldn't care what storage container is used.

It's a bit of a dangerous API, as it will store the pointer internally,
without ensuring that the caller keeps the thumbnail valid after the
call returns. It's fine, but maybe a comment above the
Exif::setThumbnail() functions to state that the thumbnail must remain
valid until the Exif object is destroyed would be a good thing.

>  	void setTimestamp(time_t timestamp);
>  
>  	libcamera::Span<const uint8_t> data() const { return { exifData_, size_ }; }

I would have moved this to a separate patch.

> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
> index c56f1b2..416e831 100644
> --- a/src/android/jpeg/post_processor_jpeg.cpp
> +++ b/src/android/jpeg/post_processor_jpeg.cpp
> @@ -9,7 +9,6 @@
>  
>  #include "../camera_device.h"
>  #include "../camera_metadata.h"
> -#include "encoder_libjpeg.h"
>  #include "exif.h"
>  
>  #include <libcamera/formats.h>
> @@ -39,11 +38,42 @@ int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,
>  	}
>  
>  	streamSize_ = outCfg.size;
> +
> +	thumbnailer_.configure(inCfg.size, inCfg.pixelFormat);
> +	StreamConfiguration thCfg = inCfg;
> +	thCfg.size = thumbnailer_.size();
> +	if (thumbnailEncoder_.configure(thCfg) != 0) {
> +		LOG(JPEG, Error) << "Failed to configure thumbnail encoder";
> +		return -EINVAL;
> +	}
> +
>  	encoder_ = std::make_unique<EncoderLibJpeg>();
>  
>  	return encoder_->configure(inCfg);
>  }
>  
> +void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,
> +					  std::vector<unsigned char> &thumbnail)
> +{
> +	/* Stores the raw scaled-down thumbnail bytes. */
> +	std::vector<unsigned char> rawThumbnail;
> +
> +	thumbnailer_.scaleBuffer(source, rawThumbnail);
> +
> +	if (rawThumbnail.data()) {

This should check for ! .empty() (see additional comments below).

> +		thumbnail.reserve(rawThumbnail.capacity());

You should call thumbnail.resize(), and use size() instead of capacity()
below, as reserve() allocates memory but doesn't change the size of the
vector, so it's semantically dangerous to write to the reserved storage
space if it hasn't been marked as in use with .resize().

> +
> +		int jpeg_size = thumbnailEncoder_.encode(
> +				{ rawThumbnail.data(), rawThumbnail.capacity() },
> +				{ thumbnail.data(), thumbnail.capacity() },

Just pass rawThumbnail and thumbnail, there's an implicit constructor
for Span that will turn them into a span using .data() and .size().

> +				{ });
> +		thumbnail.resize(jpeg_size);
> +
> +		LOG(JPEG, Info) << "Thumbnail compress returned "
> +				<< jpeg_size << " bytes";

When log messages don't fit on one line, we usually wrap them as

		LOG(JPEG, Info)
			<< "Thumbnail compress returned "
			<< jpeg_size << " bytes";

And maybe debug instead of info ?

> +	}
> +}
> +
>  int PostProcessorJpeg::process(const FrameBuffer &source,
>  			       Span<uint8_t> destination,
>  			       CameraMetadata *metadata)
> @@ -64,6 +94,9 @@ int PostProcessorJpeg::process(const FrameBuffer &source,
>  	 * second, it is good enough.
>  	 */
>  	exif.setTimestamp(std::time(nullptr));
> +	std::vector<unsigned char> thumbnail;
> +	generateThumbnail(source, thumbnail);
> +	exif.setThumbnail(thumbnail);
>  	if (exif.generate() != 0)
>  		LOG(JPEG, Error) << "Failed to generate valid EXIF data";
>  
> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h
> index 3706cec..3894231 100644
> --- a/src/android/jpeg/post_processor_jpeg.h
> +++ b/src/android/jpeg/post_processor_jpeg.h
> @@ -8,12 +8,13 @@
>  #define __ANDROID_POST_PROCESSOR_JPEG_H__
>  
>  #include "../post_processor.h"
> +#include "encoder_libjpeg.h"
> +#include "thumbnailer.h"
>  
>  #include <libcamera/geometry.h>
>  
>  #include "libcamera/internal/buffer.h"
>  
> -class Encoder;
>  class CameraDevice;
>  
>  class PostProcessorJpeg : public PostProcessor
> @@ -28,9 +29,14 @@ public:
>  		    CameraMetadata *metadata) override;
>  
>  private:
> +	void generateThumbnail(const libcamera::FrameBuffer &source,
> +			       std::vector<unsigned char> &thumbnail);
> +
>  	CameraDevice *const cameraDevice_;
>  	std::unique_ptr<Encoder> encoder_;
>  	libcamera::Size streamSize_;
> +	EncoderLibJpeg thumbnailEncoder_;

Could you store this in a std::unique_ptr<Encoder> instead ? The reason
is that we shouldn't hardcode usage of EncoderLibJpeg, in order to
support different encoders later. You won't need to include
encoder_libjpeg.h then, and can keep the Encoder forwared declaration.

> +	Thumbnailer thumbnailer_;

This is good, as I don't expect different thumbnailer types.

>  };
>  
>  #endif /* __ANDROID_POST_PROCESSOR_JPEG_H__ */
> diff --git a/src/android/jpeg/thumbnailer.cpp b/src/android/jpeg/thumbnailer.cpp
> new file mode 100644
> index 0000000..f880ffb
> --- /dev/null
> +++ b/src/android/jpeg/thumbnailer.cpp
> @@ -0,0 +1,109 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */

Wrong license.

> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * thumbnailer.cpp - Simple image thumbnailer
> + */
> +
> +#include "thumbnailer.h"
> +
> +#include <libcamera/formats.h>
> +
> +#include "libcamera/internal/file.h"

Why do you need this header ?

> +#include "libcamera/internal/log.h"
> +
> +using namespace libcamera;
> +
> +LOG_DEFINE_CATEGORY(Thumbnailer)
> +
> +Thumbnailer::Thumbnailer() : valid_(false)

	: valid_(false)

> +{
> +}
> +
> +void Thumbnailer::configure(const Size &sourceSize, PixelFormat pixelFormat)
> +{
> +	sourceSize_ = sourceSize;
> +	pixelFormat_ = pixelFormat;
> +
> +	if (pixelFormat_ != formats::NV12) {
> +		LOG (Thumbnailer, Error) << "Failed to configure: Pixel Format "
> +				    << pixelFormat_.toString() << " unsupported.";

		LOG (Thumbnailer, Error)
			<< "Failed to configure: Pixel Format "
			<< pixelFormat_.toString() << " unsupported.";

> +		return;
> +	}
> +
> +	targetSize_ = computeThumbnailSize();
> +
> +	valid_ = true;
> +}
> +
> +/*
> + * The Exif specification recommends the width of the thumbnail to be a
> + * mutiple of 16 (section 4.8.1). Hence, compute the corresponding height

s/mutiple/multiple/

> + * keeping the aspect ratio same as of the source.
> + */
> +Size Thumbnailer::computeThumbnailSize()
> +{
> +	unsigned int targetHeight;
> +	unsigned int targetWidth = 160;
> +
> +	targetHeight = targetWidth * sourceSize_.height / sourceSize_.width;
> +
> +	if (targetHeight & 1)
> +		targetHeight++;
> +
> +	return Size(targetWidth, targetHeight);
> +}
> +
> +void
> +Thumbnailer::scaleBuffer(const FrameBuffer &source,
> +			 std::vector<unsigned char> &destination)
> +{
> +	MappedFrameBuffer frame(&source, PROT_READ);
> +	if (!frame.isValid()) {
> +		LOG(Thumbnailer, Error) << "Failed to map FrameBuffer : "
> +				 << strerror(frame.error());

		LOG(Thumbnailer, Error)
			<< "Failed to map FrameBuffer : "
			<< strerror(frame.error());

> +		return;
> +	}
> +
> +	if (!valid_) {
> +		LOG(Thumbnailer, Error) << "config is unconfigured or invalid.";
> +		return;
> +	}
> +
> +	const unsigned int sw = sourceSize_.width;
> +	const unsigned int sh = sourceSize_.height;
> +	const unsigned int tw = targetSize_.width;
> +	const unsigned int th = targetSize_.height;
> +
> +	/* Image scaling block implementing nearest-neighbour algorithm. */
> +	unsigned char *src = static_cast<unsigned char *>(frame.maps()[0].data());
> +	unsigned char *src_c = src + sh * sw;
> +	unsigned char *src_cb, *src_cr;
> +	unsigned char *dst_y, *src_y;
> +
> +	size_t dstSize = (th * tw) + ((th / 2) * tw);
> +	destination.reserve(dstSize);

This should be

	destination.resize(dstSize);

as reserve() allocates memory but doesn't change the size of the vector.

> +	unsigned char *dst = destination.data();
> +	unsigned char *dst_c = dst + th * tw;
> +
> +	for (unsigned int y = 0; y < th; y += 2) {
> +		unsigned int sourceY = (sh * y + th / 2) / th;
> +
> +		dst_y = dst + y * tw;
> +		src_y = src + sw * sourceY;
> +		src_cb = src_c + (sourceY / 2) * sw + 0;
> +		src_cr = src_c + (sourceY / 2) * sw + 1;
> +
> +		for (unsigned int x = 0; x < tw; x += 2) {
> +			unsigned int sourceX = (sw * x + tw / 2) / tw;
> +
> +			dst_y[x] = src_y[sourceX];
> +			dst_y[tw + x] = src_y[sw + sourceX];
> +			dst_y[x + 1] = src_y[sourceX + 1];
> +			dst_y[tw + x + 1] = src_y[sw + sourceX + 1];
> +
> +			dst_c[(y / 2) * tw + x + 0] = src_cb[(sourceX / 2) * 2];
> +			dst_c[(y / 2) * tw + x + 1] = src_cr[(sourceX / 2) * 2];
> +		}
> +	}
> + }

Extra space.

> diff --git a/src/android/jpeg/thumbnailer.h b/src/android/jpeg/thumbnailer.h
> new file mode 100644
> index 0000000..b769619
> --- /dev/null
> +++ b/src/android/jpeg/thumbnailer.h
> @@ -0,0 +1,37 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */

Wrong license.

> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * thumbnailer.h - Simple image thumbnailer
> + */
> +#ifndef __ANDROID_JPEG_THUMBNAILER_H__
> +#define __ANDROID_JPEG_THUMBNAILER_H__
> +
> +#include <libcamera/geometry.h>
> +
> +#include "libcamera/internal/buffer.h"
> +#include "libcamera/internal/formats.h"
> +
> +class Thumbnailer
> +{
> +public:
> +	Thumbnailer();
> +
> +	void configure(const libcamera::Size &sourceSize,
> +		       libcamera::PixelFormat pixelFormat);
> +	void scaleBuffer(const libcamera::FrameBuffer &source,
> +			 std::vector<unsigned char> &dest);

How about naming this createThumbnail() or generateThumbnail() ? And the
function should be const.

> +	libcamera::Size size() const { return targetSize_; }
> +
> +private:
> +	libcamera::Size computeThumbnailSize();

This can be

	libcamera::Size computeThumbnailSize() const;

> +
> +	libcamera::PixelFormat pixelFormat_;
> +	libcamera::Size sourceSize_;
> +	libcamera::Size targetSize_;
> +
> +	bool valid_;
> +

No need for a blank line.

> +};
> +
> +#endif /* __ANDROID_JPEG_THUMBNAILER_H__ */
> diff --git a/src/android/meson.build b/src/android/meson.build
> index f72376a..3d4d3be 100644
> --- a/src/android/meson.build
> +++ b/src/android/meson.build
> @@ -25,6 +25,7 @@ android_hal_sources = files([
>      'jpeg/encoder_libjpeg.cpp',
>      'jpeg/exif.cpp',
>      'jpeg/post_processor_jpeg.cpp',
> +    'jpeg/thumbnailer.cpp',
>  ])
>  
>  android_camera_metadata_sources = files([

Hi Laurent, Umang,

On 26/10/2020 21:05, Laurent Pinchart wrote:
> Hi Umang,
> 
> Thank you for the patch.
> 
> On Mon, Oct 26, 2020 at 07:31:34PM +0530, Umang Jain wrote:
>> Add a basic image thumbnailer for NV12 frames being captured.
>> It shall generate a thumbnail image to be embedded as a part of
>> EXIF metadata of the frame. The output of the thumbnail will still
>> be NV12.
>>
>> Signed-off-by: Umang Jain <email@uajain.com>
>> ---
>>  src/android/jpeg/exif.cpp                |  16 +++-
>>  src/android/jpeg/exif.h                  |   1 +
>>  src/android/jpeg/post_processor_jpeg.cpp |  35 +++++++-
>>  src/android/jpeg/post_processor_jpeg.h   |   8 +-
>>  src/android/jpeg/thumbnailer.cpp         | 109 +++++++++++++++++++++++
>>  src/android/jpeg/thumbnailer.h           |  37 ++++++++
>>  src/android/meson.build                  |   1 +
>>  7 files changed, 204 insertions(+), 3 deletions(-)
>>  create mode 100644 src/android/jpeg/thumbnailer.cpp
>>  create mode 100644 src/android/jpeg/thumbnailer.h
>>
>> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp
>> index d21534a..24197bd 100644
>> --- a/src/android/jpeg/exif.cpp
>> +++ b/src/android/jpeg/exif.cpp
>> @@ -75,8 +75,16 @@ Exif::~Exif()
>>  	if (exifData_)
>>  		free(exifData_);
>>  
>> -	if (data_)
>> +	if (data_) {
>> +		/*
>> +		 * Reset thumbnail data to avoid getting double-freed by
>> +		 * libexif. It is owned by the caller (i.e. PostProcessorJpeg).
>> +		 */
>> +		data_->data = nullptr;
>> +		data_->size = 0;
>> +
>>  		exif_data_unref(data_);
>> +	}
>>  
>>  	if (mem_)
>>  		exif_mem_unref(mem_);
>> @@ -268,6 +276,12 @@ void Exif::setOrientation(int orientation)
>>  	setShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, value);
>>  }
>>  
>> +void Exif::setThumbnail(std::vector<unsigned char> &thumbnail)
>> +{
>> +	data_->data = thumbnail.data();
>> +	data_->size = thumbnail.size();
>> +}
>> +
>>  [[nodiscard]] int Exif::generate()
>>  {
>>  	if (exifData_) {
>> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h
>> index 12c27b6..bd54a31 100644
>> --- a/src/android/jpeg/exif.h
>> +++ b/src/android/jpeg/exif.h
>> @@ -26,6 +26,7 @@ public:
>>  
>>  	void setOrientation(int orientation);
>>  	void setSize(const libcamera::Size &size);
>> +	void setThumbnail(std::vector<unsigned char> &thumbnail);
> 
> You can pass a Span<const unsigned char> as the setThumbnail() function
> shouldn't care what storage container is used.
> 
> It's a bit of a dangerous API, as it will store the pointer internally,
> without ensuring that the caller keeps the thumbnail valid after the
> call returns. It's fine, but maybe a comment above the
> Exif::setThumbnail() functions to state that the thumbnail must remain
> valid until the Exif object is destroyed would be a good thing.

I think the comment will help indeed. The only alternative would be to
pass it into generate(), but and refactor generate() to return the span
... but that's more effort that we need right now. So just a comment
will do ;-)



>>  	void setTimestamp(time_t timestamp);
>>  
>>  	libcamera::Span<const uint8_t> data() const { return { exifData_, size_ }; }
> 
> I would have moved this to a separate patch.


Yes, I'd keep the exif extension for setting the thumbnail, and the
usage separate.

> 
>> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
>> index c56f1b2..416e831 100644
>> --- a/src/android/jpeg/post_processor_jpeg.cpp
>> +++ b/src/android/jpeg/post_processor_jpeg.cpp
>> @@ -9,7 +9,6 @@
>>  
>>  #include "../camera_device.h"
>>  #include "../camera_metadata.h"
>> -#include "encoder_libjpeg.h"
>>  #include "exif.h"
>>  
>>  #include <libcamera/formats.h>
>> @@ -39,11 +38,42 @@ int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,
>>  	}
>>  
>>  	streamSize_ = outCfg.size;
>> +
>> +	thumbnailer_.configure(inCfg.size, inCfg.pixelFormat);
>> +	StreamConfiguration thCfg = inCfg;
>> +	thCfg.size = thumbnailer_.size();
>> +	if (thumbnailEncoder_.configure(thCfg) != 0) {
>> +		LOG(JPEG, Error) << "Failed to configure thumbnail encoder";
>> +		return -EINVAL;
>> +	}
>> +
>>  	encoder_ = std::make_unique<EncoderLibJpeg>();
>>  
>>  	return encoder_->configure(inCfg);
>>  }
>>  
>> +void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,
>> +					  std::vector<unsigned char> &thumbnail)
>> +{
>> +	/* Stores the raw scaled-down thumbnail bytes. */
>> +	std::vector<unsigned char> rawThumbnail;
>> +
>> +	thumbnailer_.scaleBuffer(source, rawThumbnail);
>> +
>> +	if (rawThumbnail.data()) {
> 
> This should check for ! .empty() (see additional comments below).
> 
>> +		thumbnail.reserve(rawThumbnail.capacity());
> 
> You should call thumbnail.resize(), and use size() instead of capacity()
> below, as reserve() allocates memory but doesn't change the size of the
> vector, so it's semantically dangerous to write to the reserved storage
> space if it hasn't been marked as in use with .resize().
> 
>> +
>> +		int jpeg_size = thumbnailEncoder_.encode(
>> +				{ rawThumbnail.data(), rawThumbnail.capacity() },
>> +				{ thumbnail.data(), thumbnail.capacity() },
> 
> Just pass rawThumbnail and thumbnail, there's an implicit constructor
> for Span that will turn them into a span using .data() and .size().
> 
>> +				{ });
>> +		thumbnail.resize(jpeg_size);
>> +
>> +		LOG(JPEG, Info) << "Thumbnail compress returned "
>> +				<< jpeg_size << " bytes";
> 
> When log messages don't fit on one line, we usually wrap them as
> 
> 		LOG(JPEG, Info)
> 			<< "Thumbnail compress returned "
> 			<< jpeg_size << " bytes";
> 
> And maybe debug instead of info ?
> 
>> +	}
>> +}
>> +
>>  int PostProcessorJpeg::process(const FrameBuffer &source,
>>  			       Span<uint8_t> destination,
>>  			       CameraMetadata *metadata)
>> @@ -64,6 +94,9 @@ int PostProcessorJpeg::process(const FrameBuffer &source,
>>  	 * second, it is good enough.
>>  	 */
>>  	exif.setTimestamp(std::time(nullptr));
>> +	std::vector<unsigned char> thumbnail;
>> +	generateThumbnail(source, thumbnail);
>> +	exif.setThumbnail(thumbnail);
>>  	if (exif.generate() != 0)
>>  		LOG(JPEG, Error) << "Failed to generate valid EXIF data";
>>  
>> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h
>> index 3706cec..3894231 100644
>> --- a/src/android/jpeg/post_processor_jpeg.h
>> +++ b/src/android/jpeg/post_processor_jpeg.h
>> @@ -8,12 +8,13 @@
>>  #define __ANDROID_POST_PROCESSOR_JPEG_H__
>>  
>>  #include "../post_processor.h"
>> +#include "encoder_libjpeg.h"
>> +#include "thumbnailer.h"
>>  
>>  #include <libcamera/geometry.h>
>>  
>>  #include "libcamera/internal/buffer.h"
>>  
>> -class Encoder;
>>  class CameraDevice;
>>  
>>  class PostProcessorJpeg : public PostProcessor
>> @@ -28,9 +29,14 @@ public:
>>  		    CameraMetadata *metadata) override;
>>  
>>  private:
>> +	void generateThumbnail(const libcamera::FrameBuffer &source,
>> +			       std::vector<unsigned char> &thumbnail);
>> +
>>  	CameraDevice *const cameraDevice_;
>>  	std::unique_ptr<Encoder> encoder_;
>>  	libcamera::Size streamSize_;
>> +	EncoderLibJpeg thumbnailEncoder_;
> 
> Could you store this in a std::unique_ptr<Encoder> instead ? The reason
> is that we shouldn't hardcode usage of EncoderLibJpeg, in order to
> support different encoders later. You won't need to include
> encoder_libjpeg.h then, and can keep the Encoder forwared declaration.


I think this was already a unique_ptr in a previous iteration, and I
suggested moving it to directly store an instance of the libjpeg encoder.

In this instance of encoding a thumbnail, when encoding a 160x160 image,
I would be weary about the overhead of setting up a hardware encode, and
I'd expect the preparation phases of that to be potentially more
expensive than just a software encode. Particularly as this has just
done a software rescale, so it would have to cache-flush, when it could
just use the host-cpu with a hot-cache. (ok, so perhaps later it might
use a different scaler too ... but then it's likely a different equation
anyway)

I have not measured that of course, as we don't yet have a hw-jpeg
encode post-processor. It will be an interesting test in the future.

But essentially, I think we're just as well leaving this as a single
instance of libjpeg for thumbnails. I think I recall the CrOS HAL using
libjpeg as well, but I haven't gone back to double-check.

--
Kieran



> 
>> +	Thumbnailer thumbnailer_;
> 
> This is good, as I don't expect different thumbnailer types.
> 
>>  };
>>  
>>  #endif /* __ANDROID_POST_PROCESSOR_JPEG_H__ */
>> diff --git a/src/android/jpeg/thumbnailer.cpp b/src/android/jpeg/thumbnailer.cpp
>> new file mode 100644
>> index 0000000..f880ffb
>> --- /dev/null
>> +++ b/src/android/jpeg/thumbnailer.cpp
>> @@ -0,0 +1,109 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> 
> Wrong license.
> 
>> +/*
>> + * Copyright (C) 2020, Google Inc.
>> + *
>> + * thumbnailer.cpp - Simple image thumbnailer
>> + */
>> +
>> +#include "thumbnailer.h"
>> +
>> +#include <libcamera/formats.h>
>> +
>> +#include "libcamera/internal/file.h"
> 
> Why do you need this header ?
> 
>> +#include "libcamera/internal/log.h"
>> +
>> +using namespace libcamera;
>> +
>> +LOG_DEFINE_CATEGORY(Thumbnailer)
>> +
>> +Thumbnailer::Thumbnailer() : valid_(false)
> 
> 	: valid_(false)
> 
>> +{
>> +}
>> +
>> +void Thumbnailer::configure(const Size &sourceSize, PixelFormat pixelFormat)
>> +{
>> +	sourceSize_ = sourceSize;
>> +	pixelFormat_ = pixelFormat;
>> +
>> +	if (pixelFormat_ != formats::NV12) {
>> +		LOG (Thumbnailer, Error) << "Failed to configure: Pixel Format "
>> +				    << pixelFormat_.toString() << " unsupported.";
> 
> 		LOG (Thumbnailer, Error)
> 			<< "Failed to configure: Pixel Format "
> 			<< pixelFormat_.toString() << " unsupported.";
> 
>> +		return;
>> +	}
>> +
>> +	targetSize_ = computeThumbnailSize();
>> +
>> +	valid_ = true;
>> +}
>> +
>> +/*
>> + * The Exif specification recommends the width of the thumbnail to be a
>> + * mutiple of 16 (section 4.8.1). Hence, compute the corresponding height
> 
> s/mutiple/multiple/
> 
>> + * keeping the aspect ratio same as of the source.
>> + */
>> +Size Thumbnailer::computeThumbnailSize()
>> +{
>> +	unsigned int targetHeight;
>> +	unsigned int targetWidth = 160;
>> +
>> +	targetHeight = targetWidth * sourceSize_.height / sourceSize_.width;
>> +
>> +	if (targetHeight & 1)
>> +		targetHeight++;
>> +
>> +	return Size(targetWidth, targetHeight);
>> +}
>> +
>> +void
>> +Thumbnailer::scaleBuffer(const FrameBuffer &source,
>> +			 std::vector<unsigned char> &destination)
>> +{
>> +	MappedFrameBuffer frame(&source, PROT_READ);
>> +	if (!frame.isValid()) {
>> +		LOG(Thumbnailer, Error) << "Failed to map FrameBuffer : "
>> +				 << strerror(frame.error());
> 
> 		LOG(Thumbnailer, Error)
> 			<< "Failed to map FrameBuffer : "
> 			<< strerror(frame.error());
> 
>> +		return;
>> +	}
>> +
>> +	if (!valid_) {
>> +		LOG(Thumbnailer, Error) << "config is unconfigured or invalid.";
>> +		return;
>> +	}
>> +
>> +	const unsigned int sw = sourceSize_.width;
>> +	const unsigned int sh = sourceSize_.height;
>> +	const unsigned int tw = targetSize_.width;
>> +	const unsigned int th = targetSize_.height;
>> +
>> +	/* Image scaling block implementing nearest-neighbour algorithm. */
>> +	unsigned char *src = static_cast<unsigned char *>(frame.maps()[0].data());
>> +	unsigned char *src_c = src + sh * sw;
>> +	unsigned char *src_cb, *src_cr;
>> +	unsigned char *dst_y, *src_y;
>> +
>> +	size_t dstSize = (th * tw) + ((th / 2) * tw);
>> +	destination.reserve(dstSize);
> 
> This should be
> 
> 	destination.resize(dstSize);
> 
> as reserve() allocates memory but doesn't change the size of the vector.
> 
>> +	unsigned char *dst = destination.data();
>> +	unsigned char *dst_c = dst + th * tw;
>> +
>> +	for (unsigned int y = 0; y < th; y += 2) {
>> +		unsigned int sourceY = (sh * y + th / 2) / th;
>> +
>> +		dst_y = dst + y * tw;
>> +		src_y = src + sw * sourceY;
>> +		src_cb = src_c + (sourceY / 2) * sw + 0;
>> +		src_cr = src_c + (sourceY / 2) * sw + 1;
>> +
>> +		for (unsigned int x = 0; x < tw; x += 2) {
>> +			unsigned int sourceX = (sw * x + tw / 2) / tw;
>> +
>> +			dst_y[x] = src_y[sourceX];
>> +			dst_y[tw + x] = src_y[sw + sourceX];
>> +			dst_y[x + 1] = src_y[sourceX + 1];
>> +			dst_y[tw + x + 1] = src_y[sw + sourceX + 1];
>> +
>> +			dst_c[(y / 2) * tw + x + 0] = src_cb[(sourceX / 2) * 2];
>> +			dst_c[(y / 2) * tw + x + 1] = src_cr[(sourceX / 2) * 2];
>> +		}
>> +	}
>> + }
> 
> Extra space.
> 
>> diff --git a/src/android/jpeg/thumbnailer.h b/src/android/jpeg/thumbnailer.h
>> new file mode 100644
>> index 0000000..b769619
>> --- /dev/null
>> +++ b/src/android/jpeg/thumbnailer.h
>> @@ -0,0 +1,37 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> 
> Wrong license.
> 
>> +/*
>> + * Copyright (C) 2020, Google Inc.
>> + *
>> + * thumbnailer.h - Simple image thumbnailer
>> + */
>> +#ifndef __ANDROID_JPEG_THUMBNAILER_H__
>> +#define __ANDROID_JPEG_THUMBNAILER_H__
>> +
>> +#include <libcamera/geometry.h>
>> +
>> +#include "libcamera/internal/buffer.h"
>> +#include "libcamera/internal/formats.h"
>> +
>> +class Thumbnailer
>> +{
>> +public:
>> +	Thumbnailer();
>> +
>> +	void configure(const libcamera::Size &sourceSize,
>> +		       libcamera::PixelFormat pixelFormat);
>> +	void scaleBuffer(const libcamera::FrameBuffer &source,
>> +			 std::vector<unsigned char> &dest);
> 
> How about naming this createThumbnail() or generateThumbnail() ? And the
> function should be const.
> 
>> +	libcamera::Size size() const { return targetSize_; }
>> +
>> +private:
>> +	libcamera::Size computeThumbnailSize();
> 
> This can be
> 
> 	libcamera::Size computeThumbnailSize() const;
> 
>> +
>> +	libcamera::PixelFormat pixelFormat_;
>> +	libcamera::Size sourceSize_;
>> +	libcamera::Size targetSize_;
>> +
>> +	bool valid_;
>> +
> 
> No need for a blank line.
> 
>> +};
>> +
>> +#endif /* __ANDROID_JPEG_THUMBNAILER_H__ */
>> diff --git a/src/android/meson.build b/src/android/meson.build
>> index f72376a..3d4d3be 100644
>> --- a/src/android/meson.build
>> +++ b/src/android/meson.build
>> @@ -25,6 +25,7 @@ android_hal_sources = files([
>>      'jpeg/encoder_libjpeg.cpp',
>>      'jpeg/exif.cpp',
>>      'jpeg/post_processor_jpeg.cpp',
>> +    'jpeg/thumbnailer.cpp',
>>  ])
>>  
>>  android_camera_metadata_sources = files([
>

Hi Kieran,

On Mon, Oct 26, 2020 at 10:02:43PM +0000, Kieran Bingham wrote:
> On 26/10/2020 21:05, Laurent Pinchart wrote:
> > On Mon, Oct 26, 2020 at 07:31:34PM +0530, Umang Jain wrote:
> >> Add a basic image thumbnailer for NV12 frames being captured.
> >> It shall generate a thumbnail image to be embedded as a part of
> >> EXIF metadata of the frame. The output of the thumbnail will still
> >> be NV12.
> >>
> >> Signed-off-by: Umang Jain <email@uajain.com>
> >> ---
> >>  src/android/jpeg/exif.cpp                |  16 +++-
> >>  src/android/jpeg/exif.h                  |   1 +
> >>  src/android/jpeg/post_processor_jpeg.cpp |  35 +++++++-
> >>  src/android/jpeg/post_processor_jpeg.h   |   8 +-
> >>  src/android/jpeg/thumbnailer.cpp         | 109 +++++++++++++++++++++++
> >>  src/android/jpeg/thumbnailer.h           |  37 ++++++++
> >>  src/android/meson.build                  |   1 +
> >>  7 files changed, 204 insertions(+), 3 deletions(-)
> >>  create mode 100644 src/android/jpeg/thumbnailer.cpp
> >>  create mode 100644 src/android/jpeg/thumbnailer.h
> >>
> >> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp
> >> index d21534a..24197bd 100644
> >> --- a/src/android/jpeg/exif.cpp
> >> +++ b/src/android/jpeg/exif.cpp
> >> @@ -75,8 +75,16 @@ Exif::~Exif()
> >>  	if (exifData_)
> >>  		free(exifData_);
> >>  
> >> -	if (data_)
> >> +	if (data_) {
> >> +		/*
> >> +		 * Reset thumbnail data to avoid getting double-freed by
> >> +		 * libexif. It is owned by the caller (i.e. PostProcessorJpeg).
> >> +		 */
> >> +		data_->data = nullptr;
> >> +		data_->size = 0;
> >> +
> >>  		exif_data_unref(data_);
> >> +	}
> >>  
> >>  	if (mem_)
> >>  		exif_mem_unref(mem_);
> >> @@ -268,6 +276,12 @@ void Exif::setOrientation(int orientation)
> >>  	setShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, value);
> >>  }
> >>  
> >> +void Exif::setThumbnail(std::vector<unsigned char> &thumbnail)
> >> +{
> >> +	data_->data = thumbnail.data();
> >> +	data_->size = thumbnail.size();
> >> +}
> >> +
> >>  [[nodiscard]] int Exif::generate()
> >>  {
> >>  	if (exifData_) {
> >> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h
> >> index 12c27b6..bd54a31 100644
> >> --- a/src/android/jpeg/exif.h
> >> +++ b/src/android/jpeg/exif.h
> >> @@ -26,6 +26,7 @@ public:
> >>  
> >>  	void setOrientation(int orientation);
> >>  	void setSize(const libcamera::Size &size);
> >> +	void setThumbnail(std::vector<unsigned char> &thumbnail);
> > 
> > You can pass a Span<const unsigned char> as the setThumbnail() function
> > shouldn't care what storage container is used.
> > 
> > It's a bit of a dangerous API, as it will store the pointer internally,
> > without ensuring that the caller keeps the thumbnail valid after the
> > call returns. It's fine, but maybe a comment above the
> > Exif::setThumbnail() functions to state that the thumbnail must remain
> > valid until the Exif object is destroyed would be a good thing.
> 
> I think the comment will help indeed. The only alternative would be to
> pass it into generate(), but and refactor generate() to return the span
> ... but that's more effort that we need right now. So just a comment
> will do ;-)
> 
> 
> 
> >>  	void setTimestamp(time_t timestamp);
> >>  
> >>  	libcamera::Span<const uint8_t> data() const { return { exifData_, size_ }; }
> > 
> > I would have moved this to a separate patch.
> 
> 
> Yes, I'd keep the exif extension for setting the thumbnail, and the
> usage separate.
> 
> > 
> >> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
> >> index c56f1b2..416e831 100644
> >> --- a/src/android/jpeg/post_processor_jpeg.cpp
> >> +++ b/src/android/jpeg/post_processor_jpeg.cpp
> >> @@ -9,7 +9,6 @@
> >>  
> >>  #include "../camera_device.h"
> >>  #include "../camera_metadata.h"
> >> -#include "encoder_libjpeg.h"
> >>  #include "exif.h"
> >>  
> >>  #include <libcamera/formats.h>
> >> @@ -39,11 +38,42 @@ int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,
> >>  	}
> >>  
> >>  	streamSize_ = outCfg.size;
> >> +
> >> +	thumbnailer_.configure(inCfg.size, inCfg.pixelFormat);
> >> +	StreamConfiguration thCfg = inCfg;
> >> +	thCfg.size = thumbnailer_.size();
> >> +	if (thumbnailEncoder_.configure(thCfg) != 0) {
> >> +		LOG(JPEG, Error) << "Failed to configure thumbnail encoder";
> >> +		return -EINVAL;
> >> +	}
> >> +
> >>  	encoder_ = std::make_unique<EncoderLibJpeg>();
> >>  
> >>  	return encoder_->configure(inCfg);
> >>  }
> >>  
> >> +void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,
> >> +					  std::vector<unsigned char> &thumbnail)
> >> +{
> >> +	/* Stores the raw scaled-down thumbnail bytes. */
> >> +	std::vector<unsigned char> rawThumbnail;
> >> +
> >> +	thumbnailer_.scaleBuffer(source, rawThumbnail);
> >> +
> >> +	if (rawThumbnail.data()) {
> > 
> > This should check for ! .empty() (see additional comments below).
> > 
> >> +		thumbnail.reserve(rawThumbnail.capacity());
> > 
> > You should call thumbnail.resize(), and use size() instead of capacity()
> > below, as reserve() allocates memory but doesn't change the size of the
> > vector, so it's semantically dangerous to write to the reserved storage
> > space if it hasn't been marked as in use with .resize().
> > 
> >> +
> >> +		int jpeg_size = thumbnailEncoder_.encode(
> >> +				{ rawThumbnail.data(), rawThumbnail.capacity() },
> >> +				{ thumbnail.data(), thumbnail.capacity() },
> > 
> > Just pass rawThumbnail and thumbnail, there's an implicit constructor
> > for Span that will turn them into a span using .data() and .size().
> > 
> >> +				{ });
> >> +		thumbnail.resize(jpeg_size);
> >> +
> >> +		LOG(JPEG, Info) << "Thumbnail compress returned "
> >> +				<< jpeg_size << " bytes";
> > 
> > When log messages don't fit on one line, we usually wrap them as
> > 
> > 		LOG(JPEG, Info)
> > 			<< "Thumbnail compress returned "
> > 			<< jpeg_size << " bytes";
> > 
> > And maybe debug instead of info ?
> > 
> >> +	}
> >> +}
> >> +
> >>  int PostProcessorJpeg::process(const FrameBuffer &source,
> >>  			       Span<uint8_t> destination,
> >>  			       CameraMetadata *metadata)
> >> @@ -64,6 +94,9 @@ int PostProcessorJpeg::process(const FrameBuffer &source,
> >>  	 * second, it is good enough.
> >>  	 */
> >>  	exif.setTimestamp(std::time(nullptr));
> >> +	std::vector<unsigned char> thumbnail;
> >> +	generateThumbnail(source, thumbnail);
> >> +	exif.setThumbnail(thumbnail);
> >>  	if (exif.generate() != 0)
> >>  		LOG(JPEG, Error) << "Failed to generate valid EXIF data";
> >>  
> >> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h
> >> index 3706cec..3894231 100644
> >> --- a/src/android/jpeg/post_processor_jpeg.h
> >> +++ b/src/android/jpeg/post_processor_jpeg.h
> >> @@ -8,12 +8,13 @@
> >>  #define __ANDROID_POST_PROCESSOR_JPEG_H__
> >>  
> >>  #include "../post_processor.h"
> >> +#include "encoder_libjpeg.h"
> >> +#include "thumbnailer.h"
> >>  
> >>  #include <libcamera/geometry.h>
> >>  
> >>  #include "libcamera/internal/buffer.h"
> >>  
> >> -class Encoder;
> >>  class CameraDevice;
> >>  
> >>  class PostProcessorJpeg : public PostProcessor
> >> @@ -28,9 +29,14 @@ public:
> >>  		    CameraMetadata *metadata) override;
> >>  
> >>  private:
> >> +	void generateThumbnail(const libcamera::FrameBuffer &source,
> >> +			       std::vector<unsigned char> &thumbnail);
> >> +
> >>  	CameraDevice *const cameraDevice_;
> >>  	std::unique_ptr<Encoder> encoder_;
> >>  	libcamera::Size streamSize_;
> >> +	EncoderLibJpeg thumbnailEncoder_;
> > 
> > Could you store this in a std::unique_ptr<Encoder> instead ? The reason
> > is that we shouldn't hardcode usage of EncoderLibJpeg, in order to
> > support different encoders later. You won't need to include
> > encoder_libjpeg.h then, and can keep the Encoder forwared declaration.
> 
> I think this was already a unique_ptr in a previous iteration, and I
> suggested moving it to directly store an instance of the libjpeg encoder.
> 
> In this instance of encoding a thumbnail, when encoding a 160x160 image,
> I would be weary about the overhead of setting up a hardware encode, and
> I'd expect the preparation phases of that to be potentially more
> expensive than just a software encode. Particularly as this has just
> done a software rescale, so it would have to cache-flush, when it could
> just use the host-cpu with a hot-cache. (ok, so perhaps later it might
> use a different scaler too ... but then it's likely a different equation
> anyway)
> 
> I have not measured that of course, as we don't yet have a hw-jpeg
> encode post-processor. It will be an interesting test in the future.
> 
> But essentially, I think we're just as well leaving this as a single
> instance of libjpeg for thumbnails. I think I recall the CrOS HAL using
> libjpeg as well, but I haven't gone back to double-check.

Fair enough, those are good points. I'm fine if the code is kept as-is
(I wouldn't mind a unique_ptr of course :-)). Umang, up to you.

> >> +	Thumbnailer thumbnailer_;
> > 
> > This is good, as I don't expect different thumbnailer types.
> > 
> >>  };
> >>  
> >>  #endif /* __ANDROID_POST_PROCESSOR_JPEG_H__ */
> >> diff --git a/src/android/jpeg/thumbnailer.cpp b/src/android/jpeg/thumbnailer.cpp
> >> new file mode 100644
> >> index 0000000..f880ffb
> >> --- /dev/null
> >> +++ b/src/android/jpeg/thumbnailer.cpp
> >> @@ -0,0 +1,109 @@
> >> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > 
> > Wrong license.
> > 
> >> +/*
> >> + * Copyright (C) 2020, Google Inc.
> >> + *
> >> + * thumbnailer.cpp - Simple image thumbnailer
> >> + */
> >> +
> >> +#include "thumbnailer.h"
> >> +
> >> +#include <libcamera/formats.h>
> >> +
> >> +#include "libcamera/internal/file.h"
> > 
> > Why do you need this header ?
> > 
> >> +#include "libcamera/internal/log.h"
> >> +
> >> +using namespace libcamera;
> >> +
> >> +LOG_DEFINE_CATEGORY(Thumbnailer)
> >> +
> >> +Thumbnailer::Thumbnailer() : valid_(false)
> > 
> > 	: valid_(false)
> > 
> >> +{
> >> +}
> >> +
> >> +void Thumbnailer::configure(const Size &sourceSize, PixelFormat pixelFormat)
> >> +{
> >> +	sourceSize_ = sourceSize;
> >> +	pixelFormat_ = pixelFormat;
> >> +
> >> +	if (pixelFormat_ != formats::NV12) {
> >> +		LOG (Thumbnailer, Error) << "Failed to configure: Pixel Format "
> >> +				    << pixelFormat_.toString() << " unsupported.";
> > 
> > 		LOG (Thumbnailer, Error)
> > 			<< "Failed to configure: Pixel Format "
> > 			<< pixelFormat_.toString() << " unsupported.";
> > 
> >> +		return;
> >> +	}
> >> +
> >> +	targetSize_ = computeThumbnailSize();
> >> +
> >> +	valid_ = true;
> >> +}
> >> +
> >> +/*
> >> + * The Exif specification recommends the width of the thumbnail to be a
> >> + * mutiple of 16 (section 4.8.1). Hence, compute the corresponding height
> > 
> > s/mutiple/multiple/
> > 
> >> + * keeping the aspect ratio same as of the source.
> >> + */
> >> +Size Thumbnailer::computeThumbnailSize()
> >> +{
> >> +	unsigned int targetHeight;
> >> +	unsigned int targetWidth = 160;
> >> +
> >> +	targetHeight = targetWidth * sourceSize_.height / sourceSize_.width;
> >> +
> >> +	if (targetHeight & 1)
> >> +		targetHeight++;
> >> +
> >> +	return Size(targetWidth, targetHeight);
> >> +}
> >> +
> >> +void
> >> +Thumbnailer::scaleBuffer(const FrameBuffer &source,
> >> +			 std::vector<unsigned char> &destination)
> >> +{
> >> +	MappedFrameBuffer frame(&source, PROT_READ);
> >> +	if (!frame.isValid()) {
> >> +		LOG(Thumbnailer, Error) << "Failed to map FrameBuffer : "
> >> +				 << strerror(frame.error());
> > 
> > 		LOG(Thumbnailer, Error)
> > 			<< "Failed to map FrameBuffer : "
> > 			<< strerror(frame.error());
> > 
> >> +		return;
> >> +	}
> >> +
> >> +	if (!valid_) {
> >> +		LOG(Thumbnailer, Error) << "config is unconfigured or invalid.";
> >> +		return;
> >> +	}
> >> +
> >> +	const unsigned int sw = sourceSize_.width;
> >> +	const unsigned int sh = sourceSize_.height;
> >> +	const unsigned int tw = targetSize_.width;
> >> +	const unsigned int th = targetSize_.height;
> >> +
> >> +	/* Image scaling block implementing nearest-neighbour algorithm. */
> >> +	unsigned char *src = static_cast<unsigned char *>(frame.maps()[0].data());
> >> +	unsigned char *src_c = src + sh * sw;
> >> +	unsigned char *src_cb, *src_cr;
> >> +	unsigned char *dst_y, *src_y;
> >> +
> >> +	size_t dstSize = (th * tw) + ((th / 2) * tw);
> >> +	destination.reserve(dstSize);
> > 
> > This should be
> > 
> > 	destination.resize(dstSize);
> > 
> > as reserve() allocates memory but doesn't change the size of the vector.
> > 
> >> +	unsigned char *dst = destination.data();
> >> +	unsigned char *dst_c = dst + th * tw;
> >> +
> >> +	for (unsigned int y = 0; y < th; y += 2) {
> >> +		unsigned int sourceY = (sh * y + th / 2) / th;
> >> +
> >> +		dst_y = dst + y * tw;
> >> +		src_y = src + sw * sourceY;
> >> +		src_cb = src_c + (sourceY / 2) * sw + 0;
> >> +		src_cr = src_c + (sourceY / 2) * sw + 1;
> >> +
> >> +		for (unsigned int x = 0; x < tw; x += 2) {
> >> +			unsigned int sourceX = (sw * x + tw / 2) / tw;
> >> +
> >> +			dst_y[x] = src_y[sourceX];
> >> +			dst_y[tw + x] = src_y[sw + sourceX];
> >> +			dst_y[x + 1] = src_y[sourceX + 1];
> >> +			dst_y[tw + x + 1] = src_y[sw + sourceX + 1];
> >> +
> >> +			dst_c[(y / 2) * tw + x + 0] = src_cb[(sourceX / 2) * 2];
> >> +			dst_c[(y / 2) * tw + x + 1] = src_cr[(sourceX / 2) * 2];
> >> +		}
> >> +	}
> >> + }
> > 
> > Extra space.
> > 
> >> diff --git a/src/android/jpeg/thumbnailer.h b/src/android/jpeg/thumbnailer.h
> >> new file mode 100644
> >> index 0000000..b769619
> >> --- /dev/null
> >> +++ b/src/android/jpeg/thumbnailer.h
> >> @@ -0,0 +1,37 @@
> >> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > 
> > Wrong license.
> > 
> >> +/*
> >> + * Copyright (C) 2020, Google Inc.
> >> + *
> >> + * thumbnailer.h - Simple image thumbnailer
> >> + */
> >> +#ifndef __ANDROID_JPEG_THUMBNAILER_H__
> >> +#define __ANDROID_JPEG_THUMBNAILER_H__
> >> +
> >> +#include <libcamera/geometry.h>
> >> +
> >> +#include "libcamera/internal/buffer.h"
> >> +#include "libcamera/internal/formats.h"
> >> +
> >> +class Thumbnailer
> >> +{
> >> +public:
> >> +	Thumbnailer();
> >> +
> >> +	void configure(const libcamera::Size &sourceSize,
> >> +		       libcamera::PixelFormat pixelFormat);
> >> +	void scaleBuffer(const libcamera::FrameBuffer &source,
> >> +			 std::vector<unsigned char> &dest);
> > 
> > How about naming this createThumbnail() or generateThumbnail() ? And the
> > function should be const.
> > 
> >> +	libcamera::Size size() const { return targetSize_; }
> >> +
> >> +private:
> >> +	libcamera::Size computeThumbnailSize();
> > 
> > This can be
> > 
> > 	libcamera::Size computeThumbnailSize() const;
> > 
> >> +
> >> +	libcamera::PixelFormat pixelFormat_;
> >> +	libcamera::Size sourceSize_;
> >> +	libcamera::Size targetSize_;
> >> +
> >> +	bool valid_;
> >> +
> > 
> > No need for a blank line.
> > 
> >> +};
> >> +
> >> +#endif /* __ANDROID_JPEG_THUMBNAILER_H__ */
> >> diff --git a/src/android/meson.build b/src/android/meson.build
> >> index f72376a..3d4d3be 100644
> >> --- a/src/android/meson.build
> >> +++ b/src/android/meson.build
> >> @@ -25,6 +25,7 @@ android_hal_sources = files([
> >>      'jpeg/encoder_libjpeg.cpp',
> >>      'jpeg/exif.cpp',
> >>      'jpeg/post_processor_jpeg.cpp',
> >> +    'jpeg/thumbnailer.cpp',
> >>  ])
> >>  
> >>  android_camera_metadata_sources = files([

Hi Umang,

On Tue, Oct 27, 2020 at 7:09 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Kieran,
>
> On Mon, Oct 26, 2020 at 10:02:43PM +0000, Kieran Bingham wrote:
> > On 26/10/2020 21:05, Laurent Pinchart wrote:
> > > On Mon, Oct 26, 2020 at 07:31:34PM +0530, Umang Jain wrote:
> > >> Add a basic image thumbnailer for NV12 frames being captured.
> > >> It shall generate a thumbnail image to be embedded as a part of
> > >> EXIF metadata of the frame. The output of the thumbnail will still
> > >> be NV12.
> > >>
> > >> Signed-off-by: Umang Jain <email@uajain.com>
> > >> ---
> > >>  src/android/jpeg/exif.cpp                |  16 +++-
> > >>  src/android/jpeg/exif.h                  |   1 +
> > >>  src/android/jpeg/post_processor_jpeg.cpp |  35 +++++++-
> > >>  src/android/jpeg/post_processor_jpeg.h   |   8 +-
> > >>  src/android/jpeg/thumbnailer.cpp         | 109 +++++++++++++++++++++++
> > >>  src/android/jpeg/thumbnailer.h           |  37 ++++++++
> > >>  src/android/meson.build                  |   1 +
> > >>  7 files changed, 204 insertions(+), 3 deletions(-)
> > >>  create mode 100644 src/android/jpeg/thumbnailer.cpp
> > >>  create mode 100644 src/android/jpeg/thumbnailer.h
> > >>
> > >> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp
> > >> index d21534a..24197bd 100644
> > >> --- a/src/android/jpeg/exif.cpp
> > >> +++ b/src/android/jpeg/exif.cpp
> > >> @@ -75,8 +75,16 @@ Exif::~Exif()
> > >>    if (exifData_)
> > >>            free(exifData_);
> > >>
> > >> -  if (data_)
> > >> +  if (data_) {
> > >> +          /*
> > >> +           * Reset thumbnail data to avoid getting double-freed by
> > >> +           * libexif. It is owned by the caller (i.e. PostProcessorJpeg).
> > >> +           */
> > >> +          data_->data = nullptr;
> > >> +          data_->size = 0;
> > >> +
> > >>            exif_data_unref(data_);
> > >> +  }
> > >>
> > >>    if (mem_)
> > >>            exif_mem_unref(mem_);
> > >> @@ -268,6 +276,12 @@ void Exif::setOrientation(int orientation)
> > >>    setShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, value);
> > >>  }
> > >>
> > >> +void Exif::setThumbnail(std::vector<unsigned char> &thumbnail)
> > >> +{
> > >> +  data_->data = thumbnail.data();
> > >> +  data_->size = thumbnail.size();
> > >> +}
> > >> +
> > >>  [[nodiscard]] int Exif::generate()
> > >>  {
> > >>    if (exifData_) {
> > >> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h
> > >> index 12c27b6..bd54a31 100644
> > >> --- a/src/android/jpeg/exif.h
> > >> +++ b/src/android/jpeg/exif.h
> > >> @@ -26,6 +26,7 @@ public:
> > >>
> > >>    void setOrientation(int orientation);
> > >>    void setSize(const libcamera::Size &size);
> > >> +  void setThumbnail(std::vector<unsigned char> &thumbnail);
> > >
> > > You can pass a Span<const unsigned char> as the setThumbnail() function
> > > shouldn't care what storage container is used.
> > >
> > > It's a bit of a dangerous API, as it will store the pointer internally,
> > > without ensuring that the caller keeps the thumbnail valid after the
> > > call returns. It's fine, but maybe a comment above the
> > > Exif::setThumbnail() functions to state that the thumbnail must remain
> > > valid until the Exif object is destroyed would be a good thing.
> >
> > I think the comment will help indeed. The only alternative would be to
> > pass it into generate(), but and refactor generate() to return the span
> > ... but that's more effort that we need right now. So just a comment
> > will do ;-)
> >
> >
> >
> > >>    void setTimestamp(time_t timestamp);
> > >>
> > >>    libcamera::Span<const uint8_t> data() const { return { exifData_, size_ }; }
> > >
> > > I would have moved this to a separate patch.
> >
> >
> > Yes, I'd keep the exif extension for setting the thumbnail, and the
> > usage separate.
> >
> > >
> > >> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
> > >> index c56f1b2..416e831 100644
> > >> --- a/src/android/jpeg/post_processor_jpeg.cpp
> > >> +++ b/src/android/jpeg/post_processor_jpeg.cpp
> > >> @@ -9,7 +9,6 @@
> > >>
> > >>  #include "../camera_device.h"
> > >>  #include "../camera_metadata.h"
> > >> -#include "encoder_libjpeg.h"
> > >>  #include "exif.h"
> > >>
> > >>  #include <libcamera/formats.h>
> > >> @@ -39,11 +38,42 @@ int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,
> > >>    }
> > >>
> > >>    streamSize_ = outCfg.size;
> > >> +
> > >> +  thumbnailer_.configure(inCfg.size, inCfg.pixelFormat);
> > >> +  StreamConfiguration thCfg = inCfg;
> > >> +  thCfg.size = thumbnailer_.size();
> > >> +  if (thumbnailEncoder_.configure(thCfg) != 0) {
> > >> +          LOG(JPEG, Error) << "Failed to configure thumbnail encoder";
> > >> +          return -EINVAL;
> > >> +  }
> > >> +
> > >>    encoder_ = std::make_unique<EncoderLibJpeg>();
> > >>
> > >>    return encoder_->configure(inCfg);
> > >>  }
> > >>
> > >> +void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,
> > >> +                                    std::vector<unsigned char> &thumbnail)

std::vector<unsigned char>* thumbnail
We would rather use a pointer than reference if a function changes it.
Same for other places.

> > >> +{
> > >> +  /* Stores the raw scaled-down thumbnail bytes. */
> > >> +  std::vector<unsigned char> rawThumbnail;
> > >> +
> > >> +  thumbnailer_.scaleBuffer(source, rawThumbnail);
> > >> +
> > >> +  if (rawThumbnail.data()) {
> > >
> > > This should check for ! .empty() (see additional comments below).
> > >
> > >> +          thumbnail.reserve(rawThumbnail.capacity());
> > >
> > > You should call thumbnail.resize(), and use size() instead of capacity()
> > > below, as reserve() allocates memory but doesn't change the size of the
> > > vector, so it's semantically dangerous to write to the reserved storage
> > > space if it hasn't been marked as in use with .resize().
> > >
> > >> +
> > >> +          int jpeg_size = thumbnailEncoder_.encode(
> > >> +                          { rawThumbnail.data(), rawThumbnail.capacity() },
> > >> +                          { thumbnail.data(), thumbnail.capacity() },
> > >
> > > Just pass rawThumbnail and thumbnail, there's an implicit constructor
> > > for Span that will turn them into a span using .data() and .size().
> > >
> > >> +                          { });
> > >> +          thumbnail.resize(jpeg_size);
> > >> +
> > >> +          LOG(JPEG, Info) << "Thumbnail compress returned "
> > >> +                          << jpeg_size << " bytes";
> > >
> > > When log messages don't fit on one line, we usually wrap them as
> > >
> > >             LOG(JPEG, Info)
> > >                     << "Thumbnail compress returned "
> > >                     << jpeg_size << " bytes";
> > >
> > > And maybe debug instead of info ?
> > >
> > >> +  }
> > >> +}

Shall this function handle a failure of jpeg encode?

> > >> +
> > >>  int PostProcessorJpeg::process(const FrameBuffer &source,
> > >>                           Span<uint8_t> destination,
> > >>                           CameraMetadata *metadata)
> > >> @@ -64,6 +94,9 @@ int PostProcessorJpeg::process(const FrameBuffer &source,
> > >>     * second, it is good enough.
> > >>     */
> > >>    exif.setTimestamp(std::time(nullptr));
> > >> +  std::vector<unsigned char> thumbnail;
> > >> +  generateThumbnail(source, thumbnail);
> > >> +  exif.setThumbnail(thumbnail);
> > >>    if (exif.generate() != 0)
> > >>            LOG(JPEG, Error) << "Failed to generate valid EXIF data";
> > >>
> > >> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h
> > >> index 3706cec..3894231 100644
> > >> --- a/src/android/jpeg/post_processor_jpeg.h
> > >> +++ b/src/android/jpeg/post_processor_jpeg.h
> > >> @@ -8,12 +8,13 @@
> > >>  #define __ANDROID_POST_PROCESSOR_JPEG_H__
> > >>
> > >>  #include "../post_processor.h"
> > >> +#include "encoder_libjpeg.h"
> > >> +#include "thumbnailer.h"
> > >>
> > >>  #include <libcamera/geometry.h>
> > >>
> > >>  #include "libcamera/internal/buffer.h"
> > >>
> > >> -class Encoder;
> > >>  class CameraDevice;
> > >>
> > >>  class PostProcessorJpeg : public PostProcessor
> > >> @@ -28,9 +29,14 @@ public:
> > >>                CameraMetadata *metadata) override;
> > >>
> > >>  private:
> > >> +  void generateThumbnail(const libcamera::FrameBuffer &source,
> > >> +                         std::vector<unsigned char> &thumbnail);
> > >> +
> > >>    CameraDevice *const cameraDevice_;
> > >>    std::unique_ptr<Encoder> encoder_;
> > >>    libcamera::Size streamSize_;
> > >> +  EncoderLibJpeg thumbnailEncoder_;
> > >
> > > Could you store this in a std::unique_ptr<Encoder> instead ? The reason
> > > is that we shouldn't hardcode usage of EncoderLibJpeg, in order to
> > > support different encoders later. You won't need to include
> > > encoder_libjpeg.h then, and can keep the Encoder forwared declaration.
> >
> > I think this was already a unique_ptr in a previous iteration, and I
> > suggested moving it to directly store an instance of the libjpeg encoder.
> >
> > In this instance of encoding a thumbnail, when encoding a 160x160 image,
> > I would be weary about the overhead of setting up a hardware encode, and
> > I'd expect the preparation phases of that to be potentially more
> > expensive than just a software encode. Particularly as this has just
> > done a software rescale, so it would have to cache-flush, when it could
> > just use the host-cpu with a hot-cache. (ok, so perhaps later it might
> > use a different scaler too ... but then it's likely a different equation
> > anyway)
> >
> > I have not measured that of course, as we don't yet have a hw-jpeg
> > encode post-processor. It will be an interesting test in the future.
> >
> > But essentially, I think we're just as well leaving this as a single
> > instance of libjpeg for thumbnails. I think I recall the CrOS HAL using
> > libjpeg as well, but I haven't gone back to double-check.
>
> Fair enough, those are good points. I'm fine if the code is kept as-is
> (I wouldn't mind a unique_ptr of course :-)). Umang, up to you.
>

Although either is fine to me, I wonder why the encoder for the
thumbnail is within the thumbnailer.
Since the thumbnailer is currently being used for thumbnail, it seems
to be reasonable to move the encoder to it.
What do you think?


> > >> +  Thumbnailer thumbnailer_;
> > >
> > > This is good, as I don't expect different thumbnailer types.
> > >
> > >>  };
> > >>
> > >>  #endif /* __ANDROID_POST_PROCESSOR_JPEG_H__ */
> > >> diff --git a/src/android/jpeg/thumbnailer.cpp b/src/android/jpeg/thumbnailer.cpp
> > >> new file mode 100644
> > >> index 0000000..f880ffb
> > >> --- /dev/null
> > >> +++ b/src/android/jpeg/thumbnailer.cpp
> > >> @@ -0,0 +1,109 @@
> > >> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > >
> > > Wrong license.
> > >
> > >> +/*
> > >> + * Copyright (C) 2020, Google Inc.
> > >> + *
> > >> + * thumbnailer.cpp - Simple image thumbnailer
> > >> + */
> > >> +
> > >> +#include "thumbnailer.h"
> > >> +
> > >> +#include <libcamera/formats.h>
> > >> +
> > >> +#include "libcamera/internal/file.h"
> > >
> > > Why do you need this header ?
> > >
> > >> +#include "libcamera/internal/log.h"
> > >> +
> > >> +using namespace libcamera;
> > >> +
> > >> +LOG_DEFINE_CATEGORY(Thumbnailer)
> > >> +
> > >> +Thumbnailer::Thumbnailer() : valid_(false)
> > >
> > >     : valid_(false)
> > >
> > >> +{
> > >> +}
> > >> +
> > >> +void Thumbnailer::configure(const Size &sourceSize, PixelFormat pixelFormat)
> > >> +{
> > >> +  sourceSize_ = sourceSize;
> > >> +  pixelFormat_ = pixelFormat;
> > >> +
> > >> +  if (pixelFormat_ != formats::NV12) {
> > >> +          LOG (Thumbnailer, Error) << "Failed to configure: Pixel Format "
> > >> +                              << pixelFormat_.toString() << " unsupported.";
> > >
> > >             LOG (Thumbnailer, Error)
> > >                     << "Failed to configure: Pixel Format "
> > >                     << pixelFormat_.toString() << " unsupported.";
> > >
> > >> +          return;
> > >> +  }
> > >> +
> > >> +  targetSize_ = computeThumbnailSize();
> > >> +
> > >> +  valid_ = true;
> > >> +}
> > >> +
> > >> +/*
> > >> + * The Exif specification recommends the width of the thumbnail to be a
> > >> + * mutiple of 16 (section 4.8.1). Hence, compute the corresponding height
> > >
> > > s/mutiple/multiple/
> > >
> > >> + * keeping the aspect ratio same as of the source.
> > >> + */
> > >> +Size Thumbnailer::computeThumbnailSize()
> > >> +{
> > >> +  unsigned int targetHeight;
> > >> +  unsigned int targetWidth = 160;

nit: constexpr unsigned int kTargetWidth = 160;

> > >> +
> > >> +  targetHeight = targetWidth * sourceSize_.height / sourceSize_.width;
> > >> +
> > >> +  if (targetHeight & 1)
> > >> +          targetHeight++;
> > >> +
> > >> +  return Size(targetWidth, targetHeight);
> > >> +}
> > >> +
> > >> +void
> > >> +Thumbnailer::scaleBuffer(const FrameBuffer &source,
> > >> +                   std::vector<unsigned char> &destination)
> > >> +{
> > >> +  MappedFrameBuffer frame(&source, PROT_READ);
> > >> +  if (!frame.isValid()) {
> > >> +          LOG(Thumbnailer, Error) << "Failed to map FrameBuffer : "
> > >> +                           << strerror(frame.error());
> > >
> > >             LOG(Thumbnailer, Error)
> > >                     << "Failed to map FrameBuffer : "
> > >                     << strerror(frame.error());
> > >
> > >> +          return;
> > >> +  }
> > >> +
> > >> +  if (!valid_) {
> > >> +          LOG(Thumbnailer, Error) << "config is unconfigured or invalid.";
> > >> +          return;
> > >> +  }
> > >> +
> > >> +  const unsigned int sw = sourceSize_.width;
> > >> +  const unsigned int sh = sourceSize_.height;
> > >> +  const unsigned int tw = targetSize_.width;
> > >> +  const unsigned int th = targetSize_.height;
> > >> +
> > >> +  /* Image scaling block implementing nearest-neighbour algorithm. */
> > >> +  unsigned char *src = static_cast<unsigned char *>(frame.maps()[0].data());
> > >> +  unsigned char *src_c = src + sh * sw;
> > >> +  unsigned char *src_cb, *src_cr;
> > >> +  unsigned char *dst_y, *src_y;
> > >> +
> > >> +  size_t dstSize = (th * tw) + ((th / 2) * tw);
> > >> +  destination.reserve(dstSize);
> > >
> > > This should be
> > >
> > >     destination.resize(dstSize);
> > >
> > > as reserve() allocates memory but doesn't change the size of the vector.
> > >
> > >> +  unsigned char *dst = destination.data();
> > >> +  unsigned char *dst_c = dst + th * tw;
> > >> +
> > >> +  for (unsigned int y = 0; y < th; y += 2) {
> > >> +          unsigned int sourceY = (sh * y + th / 2) / th;
> > >> +
> > >> +          dst_y = dst + y * tw;
> > >> +          src_y = src + sw * sourceY;
> > >> +          src_cb = src_c + (sourceY / 2) * sw + 0;
> > >> +          src_cr = src_c + (sourceY / 2) * sw + 1;
> > >> +
> > >> +          for (unsigned int x = 0; x < tw; x += 2) {
> > >> +                  unsigned int sourceX = (sw * x + tw / 2) / tw;
> > >> +
> > >> +                  dst_y[x] = src_y[sourceX];
> > >> +                  dst_y[tw + x] = src_y[sw + sourceX];
> > >> +                  dst_y[x + 1] = src_y[sourceX + 1];
> > >> +                  dst_y[tw + x + 1] = src_y[sw + sourceX + 1];
> > >> +
> > >> +                  dst_c[(y / 2) * tw + x + 0] = src_cb[(sourceX / 2) * 2];
> > >> +                  dst_c[(y / 2) * tw + x + 1] = src_cr[(sourceX / 2) * 2];
> > >> +          }
> > >> +  }
> > >> + }
> > >
> > > Extra space.
> > >
> > >> diff --git a/src/android/jpeg/thumbnailer.h b/src/android/jpeg/thumbnailer.h
> > >> new file mode 100644
> > >> index 0000000..b769619
> > >> --- /dev/null
> > >> +++ b/src/android/jpeg/thumbnailer.h
> > >> @@ -0,0 +1,37 @@
> > >> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > >
> > > Wrong license.
> > >
> > >> +/*
> > >> + * Copyright (C) 2020, Google Inc.
> > >> + *
> > >> + * thumbnailer.h - Simple image thumbnailer
> > >> + */
> > >> +#ifndef __ANDROID_JPEG_THUMBNAILER_H__
> > >> +#define __ANDROID_JPEG_THUMBNAILER_H__
> > >> +
> > >> +#include <libcamera/geometry.h>
> > >> +
> > >> +#include "libcamera/internal/buffer.h"
> > >> +#include "libcamera/internal/formats.h"
> > >> +
> > >> +class Thumbnailer
> > >> +{
> > >> +public:
> > >> +  Thumbnailer();
> > >> +
> > >> +  void configure(const libcamera::Size &sourceSize,
> > >> +                 libcamera::PixelFormat pixelFormat);
> > >> +  void scaleBuffer(const libcamera::FrameBuffer &source,
> > >> +                   std::vector<unsigned char> &dest);
> > >
> > > How about naming this createThumbnail() or generateThumbnail() ? And the
> > > function should be const.
> > >
> > >> +  libcamera::Size size() const { return targetSize_; }

nit: to reduce unnecessary copy, let's return const reference.
const libcamera::Size& computeThumbanilSize() const;

Best Regards,
-Hiro
> > >> +
> > >> +private:
> > >> +  libcamera::Size computeThumbnailSize();
> > >
> > > This can be
> > >
> > >     libcamera::Size computeThumbnailSize() const;
> > >
> > >> +
> > >> +  libcamera::PixelFormat pixelFormat_;
> > >> +  libcamera::Size sourceSize_;
> > >> +  libcamera::Size targetSize_;
> > >> +
> > >> +  bool valid_;
> > >> +
> > >
> > > No need for a blank line.
> > >
> > >> +};
> > >> +
> > >> +#endif /* __ANDROID_JPEG_THUMBNAILER_H__ */
> > >> diff --git a/src/android/meson.build b/src/android/meson.build
> > >> index f72376a..3d4d3be 100644
> > >> --- a/src/android/meson.build
> > >> +++ b/src/android/meson.build
> > >> @@ -25,6 +25,7 @@ android_hal_sources = files([
> > >>      'jpeg/encoder_libjpeg.cpp',
> > >>      'jpeg/exif.cpp',
> > >>      'jpeg/post_processor_jpeg.cpp',
> > >> +    'jpeg/thumbnailer.cpp',
> > >>  ])
> > >>
> > >>  android_camera_metadata_sources = files([
>
> --
> Regards,
>
> Laurent Pinchart
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Umang,

Just few additional comments.

On Tue, Oct 27, 2020 at 2:21 PM Hirokazu Honda <hiroh@chromium.org> wrote:
>
> Hi Umang,
>
> On Tue, Oct 27, 2020 at 7:09 AM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> >
> > Hi Kieran,
> >
> > On Mon, Oct 26, 2020 at 10:02:43PM +0000, Kieran Bingham wrote:
> > > On 26/10/2020 21:05, Laurent Pinchart wrote:
> > > > On Mon, Oct 26, 2020 at 07:31:34PM +0530, Umang Jain wrote:
> > > >> Add a basic image thumbnailer for NV12 frames being captured.
> > > >> It shall generate a thumbnail image to be embedded as a part of
> > > >> EXIF metadata of the frame. The output of the thumbnail will still
> > > >> be NV12.
> > > >>
> > > >> Signed-off-by: Umang Jain <email@uajain.com>
> > > >> ---
> > > >>  src/android/jpeg/exif.cpp                |  16 +++-
> > > >>  src/android/jpeg/exif.h                  |   1 +
> > > >>  src/android/jpeg/post_processor_jpeg.cpp |  35 +++++++-
> > > >>  src/android/jpeg/post_processor_jpeg.h   |   8 +-
> > > >>  src/android/jpeg/thumbnailer.cpp         | 109 +++++++++++++++++++++++
> > > >>  src/android/jpeg/thumbnailer.h           |  37 ++++++++
> > > >>  src/android/meson.build                  |   1 +
> > > >>  7 files changed, 204 insertions(+), 3 deletions(-)
> > > >>  create mode 100644 src/android/jpeg/thumbnailer.cpp
> > > >>  create mode 100644 src/android/jpeg/thumbnailer.h
> > > >>
> > > >> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp
> > > >> index d21534a..24197bd 100644
> > > >> --- a/src/android/jpeg/exif.cpp
> > > >> +++ b/src/android/jpeg/exif.cpp
> > > >> @@ -75,8 +75,16 @@ Exif::~Exif()
> > > >>    if (exifData_)
> > > >>            free(exifData_);
> > > >>
> > > >> -  if (data_)
> > > >> +  if (data_) {
> > > >> +          /*
> > > >> +           * Reset thumbnail data to avoid getting double-freed by
> > > >> +           * libexif. It is owned by the caller (i.e. PostProcessorJpeg).
> > > >> +           */
> > > >> +          data_->data = nullptr;
> > > >> +          data_->size = 0;
> > > >> +
> > > >>            exif_data_unref(data_);
> > > >> +  }
> > > >>
> > > >>    if (mem_)
> > > >>            exif_mem_unref(mem_);
> > > >> @@ -268,6 +276,12 @@ void Exif::setOrientation(int orientation)
> > > >>    setShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, value);
> > > >>  }
> > > >>
> > > >> +void Exif::setThumbnail(std::vector<unsigned char> &thumbnail)
> > > >> +{
> > > >> +  data_->data = thumbnail.data();
> > > >> +  data_->size = thumbnail.size();
> > > >> +}
> > > >> +
> > > >>  [[nodiscard]] int Exif::generate()
> > > >>  {
> > > >>    if (exifData_) {
> > > >> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h
> > > >> index 12c27b6..bd54a31 100644
> > > >> --- a/src/android/jpeg/exif.h
> > > >> +++ b/src/android/jpeg/exif.h
> > > >> @@ -26,6 +26,7 @@ public:
> > > >>
> > > >>    void setOrientation(int orientation);
> > > >>    void setSize(const libcamera::Size &size);
> > > >> +  void setThumbnail(std::vector<unsigned char> &thumbnail);
> > > >
> > > > You can pass a Span<const unsigned char> as the setThumbnail() function
> > > > shouldn't care what storage container is used.
> > > >
> > > > It's a bit of a dangerous API, as it will store the pointer internally,
> > > > without ensuring that the caller keeps the thumbnail valid after the
> > > > call returns. It's fine, but maybe a comment above the
> > > > Exif::setThumbnail() functions to state that the thumbnail must remain
> > > > valid until the Exif object is destroyed would be a good thing.
> > >
> > > I think the comment will help indeed. The only alternative would be to
> > > pass it into generate(), but and refactor generate() to return the span
> > > ... but that's more effort that we need right now. So just a comment
> > > will do ;-)
> > >
> > >
> > >
> > > >>    void setTimestamp(time_t timestamp);
> > > >>
> > > >>    libcamera::Span<const uint8_t> data() const { return { exifData_, size_ }; }
> > > >
> > > > I would have moved this to a separate patch.
> > >
> > >
> > > Yes, I'd keep the exif extension for setting the thumbnail, and the
> > > usage separate.
> > >
> > > >
> > > >> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
> > > >> index c56f1b2..416e831 100644
> > > >> --- a/src/android/jpeg/post_processor_jpeg.cpp
> > > >> +++ b/src/android/jpeg/post_processor_jpeg.cpp
> > > >> @@ -9,7 +9,6 @@
> > > >>
> > > >>  #include "../camera_device.h"
> > > >>  #include "../camera_metadata.h"
> > > >> -#include "encoder_libjpeg.h"
> > > >>  #include "exif.h"
> > > >>
> > > >>  #include <libcamera/formats.h>
> > > >> @@ -39,11 +38,42 @@ int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,
> > > >>    }
> > > >>
> > > >>    streamSize_ = outCfg.size;
> > > >> +
> > > >> +  thumbnailer_.configure(inCfg.size, inCfg.pixelFormat);
> > > >> +  StreamConfiguration thCfg = inCfg;
> > > >> +  thCfg.size = thumbnailer_.size();
> > > >> +  if (thumbnailEncoder_.configure(thCfg) != 0) {
> > > >> +          LOG(JPEG, Error) << "Failed to configure thumbnail encoder";
> > > >> +          return -EINVAL;
> > > >> +  }
> > > >> +
> > > >>    encoder_ = std::make_unique<EncoderLibJpeg>();
> > > >>
> > > >>    return encoder_->configure(inCfg);
> > > >>  }
> > > >>
> > > >> +void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,
> > > >> +                                    std::vector<unsigned char> &thumbnail)
>
> std::vector<unsigned char>* thumbnail
> We would rather use a pointer than reference if a function changes it.
> Same for other places.
>
> > > >> +{
> > > >> +  /* Stores the raw scaled-down thumbnail bytes. */
> > > >> +  std::vector<unsigned char> rawThumbnail;
> > > >> +
> > > >> +  thumbnailer_.scaleBuffer(source, rawThumbnail);
> > > >> +
> > > >> +  if (rawThumbnail.data()) {
> > > >
> > > > This should check for ! .empty() (see additional comments below).
> > > >
> > > >> +          thumbnail.reserve(rawThumbnail.capacity());
> > > >
> > > > You should call thumbnail.resize(), and use size() instead of capacity()
> > > > below, as reserve() allocates memory but doesn't change the size of the
> > > > vector, so it's semantically dangerous to write to the reserved storage
> > > > space if it hasn't been marked as in use with .resize().
> > > >
> > > >> +
> > > >> +          int jpeg_size = thumbnailEncoder_.encode(
> > > >> +                          { rawThumbnail.data(), rawThumbnail.capacity() },
> > > >> +                          { thumbnail.data(), thumbnail.capacity() },
> > > >
> > > > Just pass rawThumbnail and thumbnail, there's an implicit constructor
> > > > for Span that will turn them into a span using .data() and .size().
> > > >
> > > >> +                          { });
> > > >> +          thumbnail.resize(jpeg_size);
> > > >> +
> > > >> +          LOG(JPEG, Info) << "Thumbnail compress returned "
> > > >> +                          << jpeg_size << " bytes";
> > > >
> > > > When log messages don't fit on one line, we usually wrap them as
> > > >
> > > >             LOG(JPEG, Info)
> > > >                     << "Thumbnail compress returned "
> > > >                     << jpeg_size << " bytes";
> > > >
> > > > And maybe debug instead of info ?
> > > >
> > > >> +  }
> > > >> +}
>
> Shall this function handle a failure of jpeg encode?
>
> > > >> +
> > > >>  int PostProcessorJpeg::process(const FrameBuffer &source,
> > > >>                           Span<uint8_t> destination,
> > > >>                           CameraMetadata *metadata)
> > > >> @@ -64,6 +94,9 @@ int PostProcessorJpeg::process(const FrameBuffer &source,
> > > >>     * second, it is good enough.
> > > >>     */
> > > >>    exif.setTimestamp(std::time(nullptr));
> > > >> +  std::vector<unsigned char> thumbnail;
> > > >> +  generateThumbnail(source, thumbnail);
> > > >> +  exif.setThumbnail(thumbnail);
> > > >>    if (exif.generate() != 0)
> > > >>            LOG(JPEG, Error) << "Failed to generate valid EXIF data";
> > > >>
> > > >> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h
> > > >> index 3706cec..3894231 100644
> > > >> --- a/src/android/jpeg/post_processor_jpeg.h
> > > >> +++ b/src/android/jpeg/post_processor_jpeg.h
> > > >> @@ -8,12 +8,13 @@
> > > >>  #define __ANDROID_POST_PROCESSOR_JPEG_H__
> > > >>
> > > >>  #include "../post_processor.h"
> > > >> +#include "encoder_libjpeg.h"
> > > >> +#include "thumbnailer.h"
> > > >>
> > > >>  #include <libcamera/geometry.h>
> > > >>
> > > >>  #include "libcamera/internal/buffer.h"
> > > >>
> > > >> -class Encoder;
> > > >>  class CameraDevice;
> > > >>
> > > >>  class PostProcessorJpeg : public PostProcessor
> > > >> @@ -28,9 +29,14 @@ public:
> > > >>                CameraMetadata *metadata) override;
> > > >>
> > > >>  private:
> > > >> +  void generateThumbnail(const libcamera::FrameBuffer &source,
> > > >> +                         std::vector<unsigned char> &thumbnail);
> > > >> +
> > > >>    CameraDevice *const cameraDevice_;
> > > >>    std::unique_ptr<Encoder> encoder_;
> > > >>    libcamera::Size streamSize_;
> > > >> +  EncoderLibJpeg thumbnailEncoder_;
> > > >
> > > > Could you store this in a std::unique_ptr<Encoder> instead ? The reason
> > > > is that we shouldn't hardcode usage of EncoderLibJpeg, in order to
> > > > support different encoders later. You won't need to include
> > > > encoder_libjpeg.h then, and can keep the Encoder forwared declaration.
> > >
> > > I think this was already a unique_ptr in a previous iteration, and I
> > > suggested moving it to directly store an instance of the libjpeg encoder.
> > >
> > > In this instance of encoding a thumbnail, when encoding a 160x160 image,
> > > I would be weary about the overhead of setting up a hardware encode, and
> > > I'd expect the preparation phases of that to be potentially more
> > > expensive than just a software encode. Particularly as this has just
> > > done a software rescale, so it would have to cache-flush, when it could
> > > just use the host-cpu with a hot-cache. (ok, so perhaps later it might
> > > use a different scaler too ... but then it's likely a different equation
> > > anyway)
> > >
> > > I have not measured that of course, as we don't yet have a hw-jpeg
> > > encode post-processor. It will be an interesting test in the future.
> > >
> > > But essentially, I think we're just as well leaving this as a single
> > > instance of libjpeg for thumbnails. I think I recall the CrOS HAL using
> > > libjpeg as well, but I haven't gone back to double-check.
> >
> > Fair enough, those are good points. I'm fine if the code is kept as-is
> > (I wouldn't mind a unique_ptr of course :-)). Umang, up to you.
> >
>
> Although either is fine to me, I wonder why the encoder for the
> thumbnail is within the thumbnailer.
> Since the thumbnailer is currently being used for thumbnail, it seems
> to be reasonable to move the encoder to it.
> What do you think?
>
>
> > > >> +  Thumbnailer thumbnailer_;
> > > >
> > > > This is good, as I don't expect different thumbnailer types.
> > > >
> > > >>  };
> > > >>
> > > >>  #endif /* __ANDROID_POST_PROCESSOR_JPEG_H__ */
> > > >> diff --git a/src/android/jpeg/thumbnailer.cpp b/src/android/jpeg/thumbnailer.cpp
> > > >> new file mode 100644
> > > >> index 0000000..f880ffb
> > > >> --- /dev/null
> > > >> +++ b/src/android/jpeg/thumbnailer.cpp
> > > >> @@ -0,0 +1,109 @@
> > > >> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > >
> > > > Wrong license.
> > > >
> > > >> +/*
> > > >> + * Copyright (C) 2020, Google Inc.
> > > >> + *
> > > >> + * thumbnailer.cpp - Simple image thumbnailer
> > > >> + */
> > > >> +
> > > >> +#include "thumbnailer.h"
> > > >> +
> > > >> +#include <libcamera/formats.h>
> > > >> +
> > > >> +#include "libcamera/internal/file.h"
> > > >
> > > > Why do you need this header ?
> > > >
> > > >> +#include "libcamera/internal/log.h"
> > > >> +
> > > >> +using namespace libcamera;
> > > >> +
> > > >> +LOG_DEFINE_CATEGORY(Thumbnailer)
> > > >> +
> > > >> +Thumbnailer::Thumbnailer() : valid_(false)
> > > >
> > > >     : valid_(false)
> > > >
> > > >> +{
> > > >> +}
> > > >> +
> > > >> +void Thumbnailer::configure(const Size &sourceSize, PixelFormat pixelFormat)
> > > >> +{
> > > >> +  sourceSize_ = sourceSize;
> > > >> +  pixelFormat_ = pixelFormat;
> > > >> +
> > > >> +  if (pixelFormat_ != formats::NV12) {
> > > >> +          LOG (Thumbnailer, Error) << "Failed to configure: Pixel Format "
> > > >> +                              << pixelFormat_.toString() << " unsupported.";
> > > >
> > > >             LOG (Thumbnailer, Error)
> > > >                     << "Failed to configure: Pixel Format "
> > > >                     << pixelFormat_.toString() << " unsupported.";
> > > >
> > > >> +          return;
> > > >> +  }
> > > >> +
> > > >> +  targetSize_ = computeThumbnailSize();
> > > >> +
> > > >> +  valid_ = true;
> > > >> +}
> > > >> +
> > > >> +/*

Shall we check sourceSize is an even dimension and smaller than targetSize_?

> > > >> + * The Exif specification recommends the width of the thumbnail to be a
> > > >> + * mutiple of 16 (section 4.8.1). Hence, compute the corresponding height
> > > >
> > > > s/mutiple/multiple/
> > > >
> > > >> + * keeping the aspect ratio same as of the source.
> > > >> + */
> > > >> +Size Thumbnailer::computeThumbnailSize()
> > > >> +{
> > > >> +  unsigned int targetHeight;
> > > >> +  unsigned int targetWidth = 160;
>
> nit: constexpr unsigned int kTargetWidth = 160;
>
> > > >> +
> > > >> +  targetHeight = targetWidth * sourceSize_.height / sourceSize_.width;
> > > >> +
> > > >> +  if (targetHeight & 1)
> > > >> +          targetHeight++;
> > > >> +
> > > >> +  return Size(targetWidth, targetHeight);
> > > >> +}
> > > >> +
> > > >> +void
> > > >> +Thumbnailer::scaleBuffer(const FrameBuffer &source,
> > > >> +                   std::vector<unsigned char> &destination)
> > > >> +{
> > > >> +  MappedFrameBuffer frame(&source, PROT_READ);
> > > >> +  if (!frame.isValid()) {
> > > >> +          LOG(Thumbnailer, Error) << "Failed to map FrameBuffer : "
> > > >> +                           << strerror(frame.error());
> > > >
> > > >             LOG(Thumbnailer, Error)
> > > >                     << "Failed to map FrameBuffer : "
> > > >                     << strerror(frame.error());
> > > >
> > > >> +          return;
> > > >> +  }
> > > >> +
> > > >> +  if (!valid_) {
> > > >> +          LOG(Thumbnailer, Error) << "config is unconfigured or invalid.";
> > > >> +          return;
> > > >> +  }
> > > >> +
> > > >> +  const unsigned int sw = sourceSize_.width;
> > > >> +  const unsigned int sh = sourceSize_.height;
> > > >> +  const unsigned int tw = targetSize_.width;
> > > >> +  const unsigned int th = targetSize_.height;
> > > >> +

assert(tw % 2 == 0 && th % 2 == 0);

> > > >> +  /* Image scaling block implementing nearest-neighbour algorithm. */
> > > >> +  unsigned char *src = static_cast<unsigned char *>(frame.maps()[0].data());

Should we check just in case, frame.size() is sourceSize_?

> > > >> +  unsigned char *src_c = src + sh * sw;
> > > >> +  unsigned char *src_cb, *src_cr;
> > > >> +  unsigned char *dst_y, *src_y;
> > > >> +

nit: Follow variable naming rule? srcY, srcCb, and so on.

> > > >> +  size_t dstSize = (th * tw) + ((th / 2) * tw);
> > > >> +  destination.reserve(dstSize);
> > > >
> > > > This should be
> > > >
> > > >     destination.resize(dstSize);
> > > >
> > > > as reserve() allocates memory but doesn't change the size of the vector.
> > > >
> > > >> +  unsigned char *dst = destination.data();
> > > >> +  unsigned char *dst_c = dst + th * tw;
> > > >> +
> > > >> +  for (unsigned int y = 0; y < th; y += 2) {
> > > >> +          unsigned int sourceY = (sh * y + th / 2) / th;
> > > >> +

I don't understand well why "+ th / 2" is required.
Could you explain?

Best Regards,
-Hiro
> > > >> +          dst_y = dst + y * tw;
> > > >> +          src_y = src + sw * sourceY;
> > > >> +          src_cb = src_c + (sourceY / 2) * sw + 0;
> > > >> +          src_cr = src_c + (sourceY / 2) * sw + 1;
> > > >> +
> > > >> +          for (unsigned int x = 0; x < tw; x += 2) {
> > > >> +                  unsigned int sourceX = (sw * x + tw / 2) / tw;
> > > >> +
> > > >> +                  dst_y[x] = src_y[sourceX];
> > > >> +                  dst_y[tw + x] = src_y[sw + sourceX];
> > > >> +                  dst_y[x + 1] = src_y[sourceX + 1];
> > > >> +                  dst_y[tw + x + 1] = src_y[sw + sourceX + 1];
> > > >> +
> > > >> +                  dst_c[(y / 2) * tw + x + 0] = src_cb[(sourceX / 2) * 2];
> > > >> +                  dst_c[(y / 2) * tw + x + 1] = src_cr[(sourceX / 2) * 2];
> > > >> +          }
> > > >> +  }
> > > >> + }
> > > >
> > > > Extra space.
> > > >
> > > >> diff --git a/src/android/jpeg/thumbnailer.h b/src/android/jpeg/thumbnailer.h
> > > >> new file mode 100644
> > > >> index 0000000..b769619
> > > >> --- /dev/null
> > > >> +++ b/src/android/jpeg/thumbnailer.h
> > > >> @@ -0,0 +1,37 @@
> > > >> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > >
> > > > Wrong license.
> > > >
> > > >> +/*
> > > >> + * Copyright (C) 2020, Google Inc.
> > > >> + *
> > > >> + * thumbnailer.h - Simple image thumbnailer
> > > >> + */
> > > >> +#ifndef __ANDROID_JPEG_THUMBNAILER_H__
> > > >> +#define __ANDROID_JPEG_THUMBNAILER_H__
> > > >> +
> > > >> +#include <libcamera/geometry.h>
> > > >> +
> > > >> +#include "libcamera/internal/buffer.h"
> > > >> +#include "libcamera/internal/formats.h"
> > > >> +
> > > >> +class Thumbnailer
> > > >> +{
> > > >> +public:
> > > >> +  Thumbnailer();
> > > >> +
> > > >> +  void configure(const libcamera::Size &sourceSize,
> > > >> +                 libcamera::PixelFormat pixelFormat);
> > > >> +  void scaleBuffer(const libcamera::FrameBuffer &source,
> > > >> +                   std::vector<unsigned char> &dest);
> > > >
> > > > How about naming this createThumbnail() or generateThumbnail() ? And the
> > > > function should be const.
> > > >
> > > >> +  libcamera::Size size() const { return targetSize_; }
>
> nit: to reduce unnecessary copy, let's return const reference.
> const libcamera::Size& computeThumbanilSize() const;
>
> Best Regards,
> -Hiro
> > > >> +
> > > >> +private:
> > > >> +  libcamera::Size computeThumbnailSize();
> > > >
> > > > This can be
> > > >
> > > >     libcamera::Size computeThumbnailSize() const;
> > > >
> > > >> +
> > > >> +  libcamera::PixelFormat pixelFormat_;
> > > >> +  libcamera::Size sourceSize_;
> > > >> +  libcamera::Size targetSize_;
> > > >> +
> > > >> +  bool valid_;
> > > >> +
> > > >
> > > > No need for a blank line.
> > > >
> > > >> +};
> > > >> +
> > > >> +#endif /* __ANDROID_JPEG_THUMBNAILER_H__ */
> > > >> diff --git a/src/android/meson.build b/src/android/meson.build
> > > >> index f72376a..3d4d3be 100644
> > > >> --- a/src/android/meson.build
> > > >> +++ b/src/android/meson.build
> > > >> @@ -25,6 +25,7 @@ android_hal_sources = files([
> > > >>      'jpeg/encoder_libjpeg.cpp',
> > > >>      'jpeg/exif.cpp',
> > > >>      'jpeg/post_processor_jpeg.cpp',
> > > >> +    'jpeg/thumbnailer.cpp',
> > > >>  ])
> > > >>
> > > >>  android_camera_metadata_sources = files([
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel

On Tue, Oct 27, 2020 at 3:53 PM Hirokazu Honda <hiroh@chromium.org> wrote:
>
> Hi Umang,
>
> Just few additional comments.
>
> On Tue, Oct 27, 2020 at 2:21 PM Hirokazu Honda <hiroh@chromium.org> wrote:
> >
> > Hi Umang,
> >
> > On Tue, Oct 27, 2020 at 7:09 AM Laurent Pinchart
> > <laurent.pinchart@ideasonboard.com> wrote:
> > >
> > > Hi Kieran,
> > >
> > > On Mon, Oct 26, 2020 at 10:02:43PM +0000, Kieran Bingham wrote:
> > > > On 26/10/2020 21:05, Laurent Pinchart wrote:
> > > > > On Mon, Oct 26, 2020 at 07:31:34PM +0530, Umang Jain wrote:
> > > > >> Add a basic image thumbnailer for NV12 frames being captured.
> > > > >> It shall generate a thumbnail image to be embedded as a part of
> > > > >> EXIF metadata of the frame. The output of the thumbnail will still
> > > > >> be NV12.
> > > > >>
> > > > >> Signed-off-by: Umang Jain <email@uajain.com>
> > > > >> ---
> > > > >>  src/android/jpeg/exif.cpp                |  16 +++-
> > > > >>  src/android/jpeg/exif.h                  |   1 +
> > > > >>  src/android/jpeg/post_processor_jpeg.cpp |  35 +++++++-
> > > > >>  src/android/jpeg/post_processor_jpeg.h   |   8 +-
> > > > >>  src/android/jpeg/thumbnailer.cpp         | 109 +++++++++++++++++++++++
> > > > >>  src/android/jpeg/thumbnailer.h           |  37 ++++++++
> > > > >>  src/android/meson.build                  |   1 +
> > > > >>  7 files changed, 204 insertions(+), 3 deletions(-)
> > > > >>  create mode 100644 src/android/jpeg/thumbnailer.cpp
> > > > >>  create mode 100644 src/android/jpeg/thumbnailer.h
> > > > >>
> > > > >> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp
> > > > >> index d21534a..24197bd 100644
> > > > >> --- a/src/android/jpeg/exif.cpp
> > > > >> +++ b/src/android/jpeg/exif.cpp
> > > > >> @@ -75,8 +75,16 @@ Exif::~Exif()
> > > > >>    if (exifData_)
> > > > >>            free(exifData_);
> > > > >>
> > > > >> -  if (data_)
> > > > >> +  if (data_) {
> > > > >> +          /*
> > > > >> +           * Reset thumbnail data to avoid getting double-freed by
> > > > >> +           * libexif. It is owned by the caller (i.e. PostProcessorJpeg).
> > > > >> +           */
> > > > >> +          data_->data = nullptr;
> > > > >> +          data_->size = 0;
> > > > >> +
> > > > >>            exif_data_unref(data_);
> > > > >> +  }
> > > > >>
> > > > >>    if (mem_)
> > > > >>            exif_mem_unref(mem_);
> > > > >> @@ -268,6 +276,12 @@ void Exif::setOrientation(int orientation)
> > > > >>    setShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, value);
> > > > >>  }
> > > > >>
> > > > >> +void Exif::setThumbnail(std::vector<unsigned char> &thumbnail)
> > > > >> +{
> > > > >> +  data_->data = thumbnail.data();
> > > > >> +  data_->size = thumbnail.size();
> > > > >> +}
> > > > >> +
> > > > >>  [[nodiscard]] int Exif::generate()
> > > > >>  {
> > > > >>    if (exifData_) {
> > > > >> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h
> > > > >> index 12c27b6..bd54a31 100644
> > > > >> --- a/src/android/jpeg/exif.h
> > > > >> +++ b/src/android/jpeg/exif.h
> > > > >> @@ -26,6 +26,7 @@ public:
> > > > >>
> > > > >>    void setOrientation(int orientation);
> > > > >>    void setSize(const libcamera::Size &size);
> > > > >> +  void setThumbnail(std::vector<unsigned char> &thumbnail);
> > > > >
> > > > > You can pass a Span<const unsigned char> as the setThumbnail() function
> > > > > shouldn't care what storage container is used.
> > > > >
> > > > > It's a bit of a dangerous API, as it will store the pointer internally,
> > > > > without ensuring that the caller keeps the thumbnail valid after the
> > > > > call returns. It's fine, but maybe a comment above the
> > > > > Exif::setThumbnail() functions to state that the thumbnail must remain
> > > > > valid until the Exif object is destroyed would be a good thing.
> > > >
> > > > I think the comment will help indeed. The only alternative would be to
> > > > pass it into generate(), but and refactor generate() to return the span
> > > > ... but that's more effort that we need right now. So just a comment
> > > > will do ;-)
> > > >
> > > >
> > > >
> > > > >>    void setTimestamp(time_t timestamp);
> > > > >>
> > > > >>    libcamera::Span<const uint8_t> data() const { return { exifData_, size_ }; }
> > > > >
> > > > > I would have moved this to a separate patch.
> > > >
> > > >
> > > > Yes, I'd keep the exif extension for setting the thumbnail, and the
> > > > usage separate.
> > > >
> > > > >
> > > > >> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
> > > > >> index c56f1b2..416e831 100644
> > > > >> --- a/src/android/jpeg/post_processor_jpeg.cpp
> > > > >> +++ b/src/android/jpeg/post_processor_jpeg.cpp
> > > > >> @@ -9,7 +9,6 @@
> > > > >>
> > > > >>  #include "../camera_device.h"
> > > > >>  #include "../camera_metadata.h"
> > > > >> -#include "encoder_libjpeg.h"
> > > > >>  #include "exif.h"
> > > > >>
> > > > >>  #include <libcamera/formats.h>
> > > > >> @@ -39,11 +38,42 @@ int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,
> > > > >>    }
> > > > >>
> > > > >>    streamSize_ = outCfg.size;
> > > > >> +
> > > > >> +  thumbnailer_.configure(inCfg.size, inCfg.pixelFormat);
> > > > >> +  StreamConfiguration thCfg = inCfg;
> > > > >> +  thCfg.size = thumbnailer_.size();
> > > > >> +  if (thumbnailEncoder_.configure(thCfg) != 0) {
> > > > >> +          LOG(JPEG, Error) << "Failed to configure thumbnail encoder";
> > > > >> +          return -EINVAL;
> > > > >> +  }
> > > > >> +
> > > > >>    encoder_ = std::make_unique<EncoderLibJpeg>();
> > > > >>
> > > > >>    return encoder_->configure(inCfg);
> > > > >>  }
> > > > >>
> > > > >> +void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,
> > > > >> +                                    std::vector<unsigned char> &thumbnail)
> >
> > std::vector<unsigned char>* thumbnail
> > We would rather use a pointer than reference if a function changes it.
> > Same for other places.
> >
> > > > >> +{
> > > > >> +  /* Stores the raw scaled-down thumbnail bytes. */
> > > > >> +  std::vector<unsigned char> rawThumbnail;
> > > > >> +
> > > > >> +  thumbnailer_.scaleBuffer(source, rawThumbnail);
> > > > >> +
> > > > >> +  if (rawThumbnail.data()) {
> > > > >
> > > > > This should check for ! .empty() (see additional comments below).
> > > > >
> > > > >> +          thumbnail.reserve(rawThumbnail.capacity());
> > > > >
> > > > > You should call thumbnail.resize(), and use size() instead of capacity()
> > > > > below, as reserve() allocates memory but doesn't change the size of the
> > > > > vector, so it's semantically dangerous to write to the reserved storage
> > > > > space if it hasn't been marked as in use with .resize().
> > > > >
> > > > >> +
> > > > >> +          int jpeg_size = thumbnailEncoder_.encode(
> > > > >> +                          { rawThumbnail.data(), rawThumbnail.capacity() },
> > > > >> +                          { thumbnail.data(), thumbnail.capacity() },
> > > > >
> > > > > Just pass rawThumbnail and thumbnail, there's an implicit constructor
> > > > > for Span that will turn them into a span using .data() and .size().
> > > > >
> > > > >> +                          { });
> > > > >> +          thumbnail.resize(jpeg_size);
> > > > >> +
> > > > >> +          LOG(JPEG, Info) << "Thumbnail compress returned "
> > > > >> +                          << jpeg_size << " bytes";
> > > > >
> > > > > When log messages don't fit on one line, we usually wrap them as
> > > > >
> > > > >             LOG(JPEG, Info)
> > > > >                     << "Thumbnail compress returned "
> > > > >                     << jpeg_size << " bytes";
> > > > >
> > > > > And maybe debug instead of info ?
> > > > >
> > > > >> +  }
> > > > >> +}
> >
> > Shall this function handle a failure of jpeg encode?
> >
> > > > >> +
> > > > >>  int PostProcessorJpeg::process(const FrameBuffer &source,
> > > > >>                           Span<uint8_t> destination,
> > > > >>                           CameraMetadata *metadata)
> > > > >> @@ -64,6 +94,9 @@ int PostProcessorJpeg::process(const FrameBuffer &source,
> > > > >>     * second, it is good enough.
> > > > >>     */
> > > > >>    exif.setTimestamp(std::time(nullptr));
> > > > >> +  std::vector<unsigned char> thumbnail;
> > > > >> +  generateThumbnail(source, thumbnail);
> > > > >> +  exif.setThumbnail(thumbnail);
> > > > >>    if (exif.generate() != 0)
> > > > >>            LOG(JPEG, Error) << "Failed to generate valid EXIF data";
> > > > >>
> > > > >> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h
> > > > >> index 3706cec..3894231 100644
> > > > >> --- a/src/android/jpeg/post_processor_jpeg.h
> > > > >> +++ b/src/android/jpeg/post_processor_jpeg.h
> > > > >> @@ -8,12 +8,13 @@
> > > > >>  #define __ANDROID_POST_PROCESSOR_JPEG_H__
> > > > >>
> > > > >>  #include "../post_processor.h"
> > > > >> +#include "encoder_libjpeg.h"
> > > > >> +#include "thumbnailer.h"
> > > > >>
> > > > >>  #include <libcamera/geometry.h>
> > > > >>
> > > > >>  #include "libcamera/internal/buffer.h"
> > > > >>
> > > > >> -class Encoder;
> > > > >>  class CameraDevice;
> > > > >>
> > > > >>  class PostProcessorJpeg : public PostProcessor
> > > > >> @@ -28,9 +29,14 @@ public:
> > > > >>                CameraMetadata *metadata) override;
> > > > >>
> > > > >>  private:
> > > > >> +  void generateThumbnail(const libcamera::FrameBuffer &source,
> > > > >> +                         std::vector<unsigned char> &thumbnail);
> > > > >> +
> > > > >>    CameraDevice *const cameraDevice_;
> > > > >>    std::unique_ptr<Encoder> encoder_;
> > > > >>    libcamera::Size streamSize_;
> > > > >> +  EncoderLibJpeg thumbnailEncoder_;
> > > > >
> > > > > Could you store this in a std::unique_ptr<Encoder> instead ? The reason
> > > > > is that we shouldn't hardcode usage of EncoderLibJpeg, in order to
> > > > > support different encoders later. You won't need to include
> > > > > encoder_libjpeg.h then, and can keep the Encoder forwared declaration.
> > > >
> > > > I think this was already a unique_ptr in a previous iteration, and I
> > > > suggested moving it to directly store an instance of the libjpeg encoder.
> > > >
> > > > In this instance of encoding a thumbnail, when encoding a 160x160 image,
> > > > I would be weary about the overhead of setting up a hardware encode, and
> > > > I'd expect the preparation phases of that to be potentially more
> > > > expensive than just a software encode. Particularly as this has just
> > > > done a software rescale, so it would have to cache-flush, when it could
> > > > just use the host-cpu with a hot-cache. (ok, so perhaps later it might
> > > > use a different scaler too ... but then it's likely a different equation
> > > > anyway)
> > > >
> > > > I have not measured that of course, as we don't yet have a hw-jpeg
> > > > encode post-processor. It will be an interesting test in the future.
> > > >
> > > > But essentially, I think we're just as well leaving this as a single
> > > > instance of libjpeg for thumbnails. I think I recall the CrOS HAL using
> > > > libjpeg as well, but I haven't gone back to double-check.
> > >
> > > Fair enough, those are good points. I'm fine if the code is kept as-is
> > > (I wouldn't mind a unique_ptr of course :-)). Umang, up to you.
> > >
> >
> > Although either is fine to me, I wonder why the encoder for the
> > thumbnail is within the thumbnailer.
> > Since the thumbnailer is currently being used for thumbnail, it seems
> > to be reasonable to move the encoder to it.
> > What do you think?
> >
> >
> > > > >> +  Thumbnailer thumbnailer_;
> > > > >
> > > > > This is good, as I don't expect different thumbnailer types.
> > > > >
> > > > >>  };
> > > > >>
> > > > >>  #endif /* __ANDROID_POST_PROCESSOR_JPEG_H__ */
> > > > >> diff --git a/src/android/jpeg/thumbnailer.cpp b/src/android/jpeg/thumbnailer.cpp
> > > > >> new file mode 100644
> > > > >> index 0000000..f880ffb
> > > > >> --- /dev/null
> > > > >> +++ b/src/android/jpeg/thumbnailer.cpp
> > > > >> @@ -0,0 +1,109 @@
> > > > >> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > > >
> > > > > Wrong license.
> > > > >
> > > > >> +/*
> > > > >> + * Copyright (C) 2020, Google Inc.
> > > > >> + *
> > > > >> + * thumbnailer.cpp - Simple image thumbnailer
> > > > >> + */
> > > > >> +
> > > > >> +#include "thumbnailer.h"
> > > > >> +
> > > > >> +#include <libcamera/formats.h>
> > > > >> +
> > > > >> +#include "libcamera/internal/file.h"
> > > > >
> > > > > Why do you need this header ?
> > > > >
> > > > >> +#include "libcamera/internal/log.h"
> > > > >> +
> > > > >> +using namespace libcamera;
> > > > >> +
> > > > >> +LOG_DEFINE_CATEGORY(Thumbnailer)
> > > > >> +
> > > > >> +Thumbnailer::Thumbnailer() : valid_(false)
> > > > >
> > > > >     : valid_(false)
> > > > >
> > > > >> +{
> > > > >> +}
> > > > >> +
> > > > >> +void Thumbnailer::configure(const Size &sourceSize, PixelFormat pixelFormat)
> > > > >> +{
> > > > >> +  sourceSize_ = sourceSize;
> > > > >> +  pixelFormat_ = pixelFormat;
> > > > >> +
> > > > >> +  if (pixelFormat_ != formats::NV12) {
> > > > >> +          LOG (Thumbnailer, Error) << "Failed to configure: Pixel Format "
> > > > >> +                              << pixelFormat_.toString() << " unsupported.";
> > > > >
> > > > >             LOG (Thumbnailer, Error)
> > > > >                     << "Failed to configure: Pixel Format "
> > > > >                     << pixelFormat_.toString() << " unsupported.";
> > > > >
> > > > >> +          return;
> > > > >> +  }
> > > > >> +
> > > > >> +  targetSize_ = computeThumbnailSize();
> > > > >> +
> > > > >> +  valid_ = true;
> > > > >> +}
> > > > >> +
> > > > >> +/*
>
> Shall we check sourceSize is an even dimension and smaller than targetSize_?
>
> > > > >> + * The Exif specification recommends the width of the thumbnail to be a
> > > > >> + * mutiple of 16 (section 4.8.1). Hence, compute the corresponding height
> > > > >
> > > > > s/mutiple/multiple/
> > > > >
> > > > >> + * keeping the aspect ratio same as of the source.
> > > > >> + */
> > > > >> +Size Thumbnailer::computeThumbnailSize()
> > > > >> +{
> > > > >> +  unsigned int targetHeight;
> > > > >> +  unsigned int targetWidth = 160;
> >
> > nit: constexpr unsigned int kTargetWidth = 160;
> >
> > > > >> +
> > > > >> +  targetHeight = targetWidth * sourceSize_.height / sourceSize_.width;
> > > > >> +
> > > > >> +  if (targetHeight & 1)
> > > > >> +          targetHeight++;
> > > > >> +
> > > > >> +  return Size(targetWidth, targetHeight);
> > > > >> +}
> > > > >> +
> > > > >> +void
> > > > >> +Thumbnailer::scaleBuffer(const FrameBuffer &source,
> > > > >> +                   std::vector<unsigned char> &destination)
> > > > >> +{
> > > > >> +  MappedFrameBuffer frame(&source, PROT_READ);
> > > > >> +  if (!frame.isValid()) {
> > > > >> +          LOG(Thumbnailer, Error) << "Failed to map FrameBuffer : "
> > > > >> +                           << strerror(frame.error());
> > > > >
> > > > >             LOG(Thumbnailer, Error)
> > > > >                     << "Failed to map FrameBuffer : "
> > > > >                     << strerror(frame.error());
> > > > >
> > > > >> +          return;
> > > > >> +  }
> > > > >> +
> > > > >> +  if (!valid_) {
> > > > >> +          LOG(Thumbnailer, Error) << "config is unconfigured or invalid.";
> > > > >> +          return;
> > > > >> +  }
> > > > >> +
> > > > >> +  const unsigned int sw = sourceSize_.width;
> > > > >> +  const unsigned int sh = sourceSize_.height;
> > > > >> +  const unsigned int tw = targetSize_.width;
> > > > >> +  const unsigned int th = targetSize_.height;
> > > > >> +
>
> assert(tw % 2 == 0 && th % 2 == 0);
>
> > > > >> +  /* Image scaling block implementing nearest-neighbour algorithm. */
> > > > >> +  unsigned char *src = static_cast<unsigned char *>(frame.maps()[0].data());
>
> Should we check just in case, frame.size() is sourceSize_?
>
> > > > >> +  unsigned char *src_c = src + sh * sw;
> > > > >> +  unsigned char *src_cb, *src_cr;
> > > > >> +  unsigned char *dst_y, *src_y;
> > > > >> +
>
> nit: Follow variable naming rule? srcY, srcCb, and so on.
>
> > > > >> +  size_t dstSize = (th * tw) + ((th / 2) * tw);
> > > > >> +  destination.reserve(dstSize);
> > > > >
> > > > > This should be
> > > > >
> > > > >     destination.resize(dstSize);
> > > > >
> > > > > as reserve() allocates memory but doesn't change the size of the vector.
> > > > >
> > > > >> +  unsigned char *dst = destination.data();
> > > > >> +  unsigned char *dst_c = dst + th * tw;
> > > > >> +
> > > > >> +  for (unsigned int y = 0; y < th; y += 2) {
> > > > >> +          unsigned int sourceY = (sh * y + th / 2) / th;
> > > > >> +
>
> I don't understand well why "+ th / 2" is required.
> Could you explain?
>

Please ignore. I found that this addition is for round to the nearest integer.

> Best Regards,
> -Hiro
> > > > >> +          dst_y = dst + y * tw;
> > > > >> +          src_y = src + sw * sourceY;
> > > > >> +          src_cb = src_c + (sourceY / 2) * sw + 0;
> > > > >> +          src_cr = src_c + (sourceY / 2) * sw + 1;
> > > > >> +
> > > > >> +          for (unsigned int x = 0; x < tw; x += 2) {
> > > > >> +                  unsigned int sourceX = (sw * x + tw / 2) / tw;
> > > > >> +
> > > > >> +                  dst_y[x] = src_y[sourceX];
> > > > >> +                  dst_y[tw + x] = src_y[sw + sourceX];
> > > > >> +                  dst_y[x + 1] = src_y[sourceX + 1];
> > > > >> +                  dst_y[tw + x + 1] = src_y[sw + sourceX + 1];
> > > > >> +
> > > > >> +                  dst_c[(y / 2) * tw + x + 0] = src_cb[(sourceX / 2) * 2];
> > > > >> +                  dst_c[(y / 2) * tw + x + 1] = src_cr[(sourceX / 2) * 2];
> > > > >> +          }
> > > > >> +  }
> > > > >> + }
> > > > >
> > > > > Extra space.
> > > > >
> > > > >> diff --git a/src/android/jpeg/thumbnailer.h b/src/android/jpeg/thumbnailer.h
> > > > >> new file mode 100644
> > > > >> index 0000000..b769619
> > > > >> --- /dev/null
> > > > >> +++ b/src/android/jpeg/thumbnailer.h
> > > > >> @@ -0,0 +1,37 @@
> > > > >> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > > >
> > > > > Wrong license.
> > > > >
> > > > >> +/*
> > > > >> + * Copyright (C) 2020, Google Inc.
> > > > >> + *
> > > > >> + * thumbnailer.h - Simple image thumbnailer
> > > > >> + */
> > > > >> +#ifndef __ANDROID_JPEG_THUMBNAILER_H__
> > > > >> +#define __ANDROID_JPEG_THUMBNAILER_H__
> > > > >> +
> > > > >> +#include <libcamera/geometry.h>
> > > > >> +
> > > > >> +#include "libcamera/internal/buffer.h"
> > > > >> +#include "libcamera/internal/formats.h"
> > > > >> +
> > > > >> +class Thumbnailer
> > > > >> +{
> > > > >> +public:
> > > > >> +  Thumbnailer();
> > > > >> +
> > > > >> +  void configure(const libcamera::Size &sourceSize,
> > > > >> +                 libcamera::PixelFormat pixelFormat);
> > > > >> +  void scaleBuffer(const libcamera::FrameBuffer &source,
> > > > >> +                   std::vector<unsigned char> &dest);
> > > > >
> > > > > How about naming this createThumbnail() or generateThumbnail() ? And the
> > > > > function should be const.
> > > > >
> > > > >> +  libcamera::Size size() const { return targetSize_; }
> >
> > nit: to reduce unnecessary copy, let's return const reference.
> > const libcamera::Size& computeThumbanilSize() const;
> >
> > Best Regards,
> > -Hiro
> > > > >> +
> > > > >> +private:
> > > > >> +  libcamera::Size computeThumbnailSize();
> > > > >
> > > > > This can be
> > > > >
> > > > >     libcamera::Size computeThumbnailSize() const;
> > > > >
> > > > >> +
> > > > >> +  libcamera::PixelFormat pixelFormat_;
> > > > >> +  libcamera::Size sourceSize_;
> > > > >> +  libcamera::Size targetSize_;
> > > > >> +
> > > > >> +  bool valid_;
> > > > >> +
> > > > >
> > > > > No need for a blank line.
> > > > >
> > > > >> +};
> > > > >> +
> > > > >> +#endif /* __ANDROID_JPEG_THUMBNAILER_H__ */
> > > > >> diff --git a/src/android/meson.build b/src/android/meson.build
> > > > >> index f72376a..3d4d3be 100644
> > > > >> --- a/src/android/meson.build
> > > > >> +++ b/src/android/meson.build
> > > > >> @@ -25,6 +25,7 @@ android_hal_sources = files([
> > > > >>      'jpeg/encoder_libjpeg.cpp',
> > > > >>      'jpeg/exif.cpp',
> > > > >>      'jpeg/post_processor_jpeg.cpp',
> > > > >> +    'jpeg/thumbnailer.cpp',
> > > > >>  ])
> > > > >>
> > > > >>  android_camera_metadata_sources = files([
> > >
> > > --
> > > Regards,
> > >
> > > Laurent Pinchart
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel@lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel

Hi Hiro,

On 10/27/20 10:51 AM, Hirokazu Honda wrote:
> Hi Umang,
>
> On Tue, Oct 27, 2020 at 7:09 AM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
>> Hi Kieran,
>>
>> On Mon, Oct 26, 2020 at 10:02:43PM +0000, Kieran Bingham wrote:
>>> On 26/10/2020 21:05, Laurent Pinchart wrote:
>>>> On Mon, Oct 26, 2020 at 07:31:34PM +0530, Umang Jain wrote:
>>>>> Add a basic image thumbnailer for NV12 frames being captured.
>>>>> It shall generate a thumbnail image to be embedded as a part of
>>>>> EXIF metadata of the frame. The output of the thumbnail will still
>>>>> be NV12.
>>>>>
>>>>> Signed-off-by: Umang Jain <email@uajain.com>
>>>>> ---
>>>>>   src/android/jpeg/exif.cpp                |  16 +++-
>>>>>   src/android/jpeg/exif.h                  |   1 +
>>>>>   src/android/jpeg/post_processor_jpeg.cpp |  35 +++++++-
>>>>>   src/android/jpeg/post_processor_jpeg.h   |   8 +-
>>>>>   src/android/jpeg/thumbnailer.cpp         | 109 +++++++++++++++++++++++
>>>>>   src/android/jpeg/thumbnailer.h           |  37 ++++++++
>>>>>   src/android/meson.build                  |   1 +
>>>>>   7 files changed, 204 insertions(+), 3 deletions(-)
>>>>>   create mode 100644 src/android/jpeg/thumbnailer.cpp
>>>>>   create mode 100644 src/android/jpeg/thumbnailer.h
>>>>>
>>>>> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp
>>>>> index d21534a..24197bd 100644
>>>>> --- a/src/android/jpeg/exif.cpp
>>>>> +++ b/src/android/jpeg/exif.cpp
>>>>> @@ -75,8 +75,16 @@ Exif::~Exif()
>>>>>     if (exifData_)
>>>>>             free(exifData_);
>>>>>
>>>>> -  if (data_)
>>>>> +  if (data_) {
>>>>> +          /*
>>>>> +           * Reset thumbnail data to avoid getting double-freed by
>>>>> +           * libexif. It is owned by the caller (i.e. PostProcessorJpeg).
>>>>> +           */
>>>>> +          data_->data = nullptr;
>>>>> +          data_->size = 0;
>>>>> +
>>>>>             exif_data_unref(data_);
>>>>> +  }
>>>>>
>>>>>     if (mem_)
>>>>>             exif_mem_unref(mem_);
>>>>> @@ -268,6 +276,12 @@ void Exif::setOrientation(int orientation)
>>>>>     setShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, value);
>>>>>   }
>>>>>
>>>>> +void Exif::setThumbnail(std::vector<unsigned char> &thumbnail)
>>>>> +{
>>>>> +  data_->data = thumbnail.data();
>>>>> +  data_->size = thumbnail.size();
>>>>> +}
>>>>> +
>>>>>   [[nodiscard]] int Exif::generate()
>>>>>   {
>>>>>     if (exifData_) {
>>>>> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h
>>>>> index 12c27b6..bd54a31 100644
>>>>> --- a/src/android/jpeg/exif.h
>>>>> +++ b/src/android/jpeg/exif.h
>>>>> @@ -26,6 +26,7 @@ public:
>>>>>
>>>>>     void setOrientation(int orientation);
>>>>>     void setSize(const libcamera::Size &size);
>>>>> +  void setThumbnail(std::vector<unsigned char> &thumbnail);
>>>> You can pass a Span<const unsigned char> as the setThumbnail() function
>>>> shouldn't care what storage container is used.
>>>>
>>>> It's a bit of a dangerous API, as it will store the pointer internally,
>>>> without ensuring that the caller keeps the thumbnail valid after the
>>>> call returns. It's fine, but maybe a comment above the
>>>> Exif::setThumbnail() functions to state that the thumbnail must remain
>>>> valid until the Exif object is destroyed would be a good thing.
>>> I think the comment will help indeed. The only alternative would be to
>>> pass it into generate(), but and refactor generate() to return the span
>>> ... but that's more effort that we need right now. So just a comment
>>> will do ;-)
>>>
>>>
>>>
>>>>>     void setTimestamp(time_t timestamp);
>>>>>
>>>>>     libcamera::Span<const uint8_t> data() const { return { exifData_, size_ }; }
>>>> I would have moved this to a separate patch.
>>> Yes, I'd keep the exif extension for setting the thumbnail, and the
>>> usage separate.
>>>
>>>>> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
>>>>> index c56f1b2..416e831 100644
>>>>> --- a/src/android/jpeg/post_processor_jpeg.cpp
>>>>> +++ b/src/android/jpeg/post_processor_jpeg.cpp
>>>>> @@ -9,7 +9,6 @@
>>>>>
>>>>>   #include "../camera_device.h"
>>>>>   #include "../camera_metadata.h"
>>>>> -#include "encoder_libjpeg.h"
>>>>>   #include "exif.h"
>>>>>
>>>>>   #include <libcamera/formats.h>
>>>>> @@ -39,11 +38,42 @@ int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,
>>>>>     }
>>>>>
>>>>>     streamSize_ = outCfg.size;
>>>>> +
>>>>> +  thumbnailer_.configure(inCfg.size, inCfg.pixelFormat);
>>>>> +  StreamConfiguration thCfg = inCfg;
>>>>> +  thCfg.size = thumbnailer_.size();
>>>>> +  if (thumbnailEncoder_.configure(thCfg) != 0) {
>>>>> +          LOG(JPEG, Error) << "Failed to configure thumbnail encoder";
>>>>> +          return -EINVAL;
>>>>> +  }
>>>>> +
>>>>>     encoder_ = std::make_unique<EncoderLibJpeg>();
>>>>>
>>>>>     return encoder_->configure(inCfg);
>>>>>   }
>>>>>
>>>>> +void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,
>>>>> +                                    std::vector<unsigned char> &thumbnail)
> std::vector<unsigned char>* thumbnail
> We would rather use a pointer than reference if a function changes it.
> Same for other places.
Ack.
>>>>> +{
>>>>> +  /* Stores the raw scaled-down thumbnail bytes. */
>>>>> +  std::vector<unsigned char> rawThumbnail;
>>>>> +
>>>>> +  thumbnailer_.scaleBuffer(source, rawThumbnail);
>>>>> +
>>>>> +  if (rawThumbnail.data()) {
>>>> This should check for ! .empty() (see additional comments below).
>>>>
>>>>> +          thumbnail.reserve(rawThumbnail.capacity());
>>>> You should call thumbnail.resize(), and use size() instead of capacity()
>>>> below, as reserve() allocates memory but doesn't change the size of the
>>>> vector, so it's semantically dangerous to write to the reserved storage
>>>> space if it hasn't been marked as in use with .resize().
>>>>
>>>>> +
>>>>> +          int jpeg_size = thumbnailEncoder_.encode(
>>>>> +                          { rawThumbnail.data(), rawThumbnail.capacity() },
>>>>> +                          { thumbnail.data(), thumbnail.capacity() },
>>>> Just pass rawThumbnail and thumbnail, there's an implicit constructor
>>>> for Span that will turn them into a span using .data() and .size().
>>>>
>>>>> +                          { });
>>>>> +          thumbnail.resize(jpeg_size);
>>>>> +
>>>>> +          LOG(JPEG, Info) << "Thumbnail compress returned "
>>>>> +                          << jpeg_size << " bytes";
>>>> When log messages don't fit on one line, we usually wrap them as
>>>>
>>>>              LOG(JPEG, Info)
>>>>                      << "Thumbnail compress returned "
>>>>                      << jpeg_size << " bytes";
>>>>
>>>> And maybe debug instead of info ?
>>>>
>>>>> +  }
>>>>> +}
> Shall this function handle a failure of jpeg encode?
Ideally it should but I am wondering in what style. I would prefer that 
checking for the (thumbnail data != 0) before calling 
Exif::setThumbnail() below. Since, when the encoding fails, jpeg_size 
will get 0, and the thumbnail vector(placeholder for our thumbnail data) 
will get resized to '0'.
>>>>> +
>>>>>   int PostProcessorJpeg::process(const FrameBuffer &source,
>>>>>                            Span<uint8_t> destination,
>>>>>                            CameraMetadata *metadata)
>>>>> @@ -64,6 +94,9 @@ int PostProcessorJpeg::process(const FrameBuffer &source,
>>>>>      * second, it is good enough.
>>>>>      */
>>>>>     exif.setTimestamp(std::time(nullptr));
>>>>> +  std::vector<unsigned char> thumbnail;
>>>>> +  generateThumbnail(source, thumbnail);
>>>>> +  exif.setThumbnail(thumbnail);
>>>>>     if (exif.generate() != 0)
>>>>>             LOG(JPEG, Error) << "Failed to generate valid EXIF data";
>>>>>
>>>>> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h
>>>>> index 3706cec..3894231 100644
>>>>> --- a/src/android/jpeg/post_processor_jpeg.h
>>>>> +++ b/src/android/jpeg/post_processor_jpeg.h
>>>>> @@ -8,12 +8,13 @@
>>>>>   #define __ANDROID_POST_PROCESSOR_JPEG_H__
>>>>>
>>>>>   #include "../post_processor.h"
>>>>> +#include "encoder_libjpeg.h"
>>>>> +#include "thumbnailer.h"
>>>>>
>>>>>   #include <libcamera/geometry.h>
>>>>>
>>>>>   #include "libcamera/internal/buffer.h"
>>>>>
>>>>> -class Encoder;
>>>>>   class CameraDevice;
>>>>>
>>>>>   class PostProcessorJpeg : public PostProcessor
>>>>> @@ -28,9 +29,14 @@ public:
>>>>>                 CameraMetadata *metadata) override;
>>>>>
>>>>>   private:
>>>>> +  void generateThumbnail(const libcamera::FrameBuffer &source,
>>>>> +                         std::vector<unsigned char> &thumbnail);
>>>>> +
>>>>>     CameraDevice *const cameraDevice_;
>>>>>     std::unique_ptr<Encoder> encoder_;
>>>>>     libcamera::Size streamSize_;
>>>>> +  EncoderLibJpeg thumbnailEncoder_;
>>>> Could you store this in a std::unique_ptr<Encoder> instead ? The reason
>>>> is that we shouldn't hardcode usage of EncoderLibJpeg, in order to
>>>> support different encoders later. You won't need to include
>>>> encoder_libjpeg.h then, and can keep the Encoder forwared declaration.
>>> I think this was already a unique_ptr in a previous iteration, and I
>>> suggested moving it to directly store an instance of the libjpeg encoder.
>>>
>>> In this instance of encoding a thumbnail, when encoding a 160x160 image,
>>> I would be weary about the overhead of setting up a hardware encode, and
>>> I'd expect the preparation phases of that to be potentially more
>>> expensive than just a software encode. Particularly as this has just
>>> done a software rescale, so it would have to cache-flush, when it could
>>> just use the host-cpu with a hot-cache. (ok, so perhaps later it might
>>> use a different scaler too ... but then it's likely a different equation
>>> anyway)
>>>
>>> I have not measured that of course, as we don't yet have a hw-jpeg
>>> encode post-processor. It will be an interesting test in the future.
>>>
>>> But essentially, I think we're just as well leaving this as a single
>>> instance of libjpeg for thumbnails. I think I recall the CrOS HAL using
>>> libjpeg as well, but I haven't gone back to double-check.
>> Fair enough, those are good points. I'm fine if the code is kept as-is
>> (I wouldn't mind a unique_ptr of course :-)). Umang, up to you.
>>
> Although either is fine to me, I wonder why the encoder for the
> thumbnail is within the thumbnailer.
It is not within the encoder. It is inside the PostProcessorJpeg
> Since the thumbnailer is currently being used for thumbnail, it seems
> to be reasonable to move the encoder to it.
> What do you think?
If you mean moving the thumbnail-encoder to `class Thumbnailer` - hmm, 
that could also work I suppose. However,  I might not be very 
enthusiastic about it, as PostProcessorJpeg captures the entire flow at 
one place : e.g. exif generation, creating a thumbnail, encoding it and 
finally encoding the entire frame. I like this.

If others support moving the thumbnail-encoder to Thumbnailer, that's 
also fine! (Also, could be done on top, as an independent refactor)
>
>>>>> +  Thumbnailer thumbnailer_;
>>>> This is good, as I don't expect different thumbnailer types.
>>>>
>>>>>   };
>>>>>
>>>>>   #endif /* __ANDROID_POST_PROCESSOR_JPEG_H__ */
>>>>> diff --git a/src/android/jpeg/thumbnailer.cpp b/src/android/jpeg/thumbnailer.cpp
>>>>> new file mode 100644
>>>>> index 0000000..f880ffb
>>>>> --- /dev/null
>>>>> +++ b/src/android/jpeg/thumbnailer.cpp
>>>>> @@ -0,0 +1,109 @@
>>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>>> Wrong license.
>>>>
>>>>> +/*
>>>>> + * Copyright (C) 2020, Google Inc.
>>>>> + *
>>>>> + * thumbnailer.cpp - Simple image thumbnailer
>>>>> + */
>>>>> +
>>>>> +#include "thumbnailer.h"
>>>>> +
>>>>> +#include <libcamera/formats.h>
>>>>> +
>>>>> +#include "libcamera/internal/file.h"
>>>> Why do you need this header ?
>>>>
>>>>> +#include "libcamera/internal/log.h"
>>>>> +
>>>>> +using namespace libcamera;
>>>>> +
>>>>> +LOG_DEFINE_CATEGORY(Thumbnailer)
>>>>> +
>>>>> +Thumbnailer::Thumbnailer() : valid_(false)
>>>>      : valid_(false)
>>>>
>>>>> +{
>>>>> +}
>>>>> +
>>>>> +void Thumbnailer::configure(const Size &sourceSize, PixelFormat pixelFormat)
>>>>> +{
>>>>> +  sourceSize_ = sourceSize;
>>>>> +  pixelFormat_ = pixelFormat;
>>>>> +
>>>>> +  if (pixelFormat_ != formats::NV12) {
>>>>> +          LOG (Thumbnailer, Error) << "Failed to configure: Pixel Format "
>>>>> +                              << pixelFormat_.toString() << " unsupported.";
>>>>              LOG (Thumbnailer, Error)
>>>>                      << "Failed to configure: Pixel Format "
>>>>                      << pixelFormat_.toString() << " unsupported.";
>>>>
>>>>> +          return;
>>>>> +  }
>>>>> +
>>>>> +  targetSize_ = computeThumbnailSize();
>>>>> +
>>>>> +  valid_ = true;
>>>>> +}
>>>>> +
>>>>> +/*
>>>>> + * The Exif specification recommends the width of the thumbnail to be a
>>>>> + * mutiple of 16 (section 4.8.1). Hence, compute the corresponding height
>>>> s/mutiple/multiple/
>>>>
>>>>> + * keeping the aspect ratio same as of the source.
>>>>> + */
>>>>> +Size Thumbnailer::computeThumbnailSize()
>>>>> +{
>>>>> +  unsigned int targetHeight;
>>>>> +  unsigned int targetWidth = 160;
> nit: constexpr unsigned int kTargetWidth = 160;
>
>>>>> +
>>>>> +  targetHeight = targetWidth * sourceSize_.height / sourceSize_.width;
>>>>> +
>>>>> +  if (targetHeight & 1)
>>>>> +          targetHeight++;
>>>>> +
>>>>> +  return Size(targetWidth, targetHeight);
>>>>> +}
>>>>> +
>>>>> +void
>>>>> +Thumbnailer::scaleBuffer(const FrameBuffer &source,
>>>>> +                   std::vector<unsigned char> &destination)
>>>>> +{
>>>>> +  MappedFrameBuffer frame(&source, PROT_READ);
>>>>> +  if (!frame.isValid()) {
>>>>> +          LOG(Thumbnailer, Error) << "Failed to map FrameBuffer : "
>>>>> +                           << strerror(frame.error());
>>>>              LOG(Thumbnailer, Error)
>>>>                      << "Failed to map FrameBuffer : "
>>>>                      << strerror(frame.error());
>>>>
>>>>> +          return;
>>>>> +  }
>>>>> +
>>>>> +  if (!valid_) {
>>>>> +          LOG(Thumbnailer, Error) << "config is unconfigured or invalid.";
>>>>> +          return;
>>>>> +  }
>>>>> +
>>>>> +  const unsigned int sw = sourceSize_.width;
>>>>> +  const unsigned int sh = sourceSize_.height;
>>>>> +  const unsigned int tw = targetSize_.width;
>>>>> +  const unsigned int th = targetSize_.height;
>>>>> +
>>>>> +  /* Image scaling block implementing nearest-neighbour algorithm. */
>>>>> +  unsigned char *src = static_cast<unsigned char *>(frame.maps()[0].data());
>>>>> +  unsigned char *src_c = src + sh * sw;
>>>>> +  unsigned char *src_cb, *src_cr;
>>>>> +  unsigned char *dst_y, *src_y;
>>>>> +
>>>>> +  size_t dstSize = (th * tw) + ((th / 2) * tw);
>>>>> +  destination.reserve(dstSize);
>>>> This should be
>>>>
>>>>      destination.resize(dstSize);
>>>>
>>>> as reserve() allocates memory but doesn't change the size of the vector.
>>>>
>>>>> +  unsigned char *dst = destination.data();
>>>>> +  unsigned char *dst_c = dst + th * tw;
>>>>> +
>>>>> +  for (unsigned int y = 0; y < th; y += 2) {
>>>>> +          unsigned int sourceY = (sh * y + th / 2) / th;
>>>>> +
>>>>> +          dst_y = dst + y * tw;
>>>>> +          src_y = src + sw * sourceY;
>>>>> +          src_cb = src_c + (sourceY / 2) * sw + 0;
>>>>> +          src_cr = src_c + (sourceY / 2) * sw + 1;
>>>>> +
>>>>> +          for (unsigned int x = 0; x < tw; x += 2) {
>>>>> +                  unsigned int sourceX = (sw * x + tw / 2) / tw;
>>>>> +
>>>>> +                  dst_y[x] = src_y[sourceX];
>>>>> +                  dst_y[tw + x] = src_y[sw + sourceX];
>>>>> +                  dst_y[x + 1] = src_y[sourceX + 1];
>>>>> +                  dst_y[tw + x + 1] = src_y[sw + sourceX + 1];
>>>>> +
>>>>> +                  dst_c[(y / 2) * tw + x + 0] = src_cb[(sourceX / 2) * 2];
>>>>> +                  dst_c[(y / 2) * tw + x + 1] = src_cr[(sourceX / 2) * 2];
>>>>> +          }
>>>>> +  }
>>>>> + }
>>>> Extra space.
>>>>
>>>>> diff --git a/src/android/jpeg/thumbnailer.h b/src/android/jpeg/thumbnailer.h
>>>>> new file mode 100644
>>>>> index 0000000..b769619
>>>>> --- /dev/null
>>>>> +++ b/src/android/jpeg/thumbnailer.h
>>>>> @@ -0,0 +1,37 @@
>>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>>> Wrong license.
>>>>
>>>>> +/*
>>>>> + * Copyright (C) 2020, Google Inc.
>>>>> + *
>>>>> + * thumbnailer.h - Simple image thumbnailer
>>>>> + */
>>>>> +#ifndef __ANDROID_JPEG_THUMBNAILER_H__
>>>>> +#define __ANDROID_JPEG_THUMBNAILER_H__
>>>>> +
>>>>> +#include <libcamera/geometry.h>
>>>>> +
>>>>> +#include "libcamera/internal/buffer.h"
>>>>> +#include "libcamera/internal/formats.h"
>>>>> +
>>>>> +class Thumbnailer
>>>>> +{
>>>>> +public:
>>>>> +  Thumbnailer();
>>>>> +
>>>>> +  void configure(const libcamera::Size &sourceSize,
>>>>> +                 libcamera::PixelFormat pixelFormat);
>>>>> +  void scaleBuffer(const libcamera::FrameBuffer &source,
>>>>> +                   std::vector<unsigned char> &dest);
>>>> How about naming this createThumbnail() or generateThumbnail() ? And the
>>>> function should be const.
>>>>
>>>>> +  libcamera::Size size() const { return targetSize_; }
> nit: to reduce unnecessary copy, let's return const reference.
> const libcamera::Size& computeThumbanilSize() const;
>
> Best Regards,
> -Hiro
>>>>> +
>>>>> +private:
>>>>> +  libcamera::Size computeThumbnailSize();
>>>> This can be
>>>>
>>>>      libcamera::Size computeThumbnailSize() const;
>>>>
>>>>> +
>>>>> +  libcamera::PixelFormat pixelFormat_;
>>>>> +  libcamera::Size sourceSize_;
>>>>> +  libcamera::Size targetSize_;
>>>>> +
>>>>> +  bool valid_;
>>>>> +
>>>> No need for a blank line.
>>>>
>>>>> +};
>>>>> +
>>>>> +#endif /* __ANDROID_JPEG_THUMBNAILER_H__ */
>>>>> diff --git a/src/android/meson.build b/src/android/meson.build
>>>>> index f72376a..3d4d3be 100644
>>>>> --- a/src/android/meson.build
>>>>> +++ b/src/android/meson.build
>>>>> @@ -25,6 +25,7 @@ android_hal_sources = files([
>>>>>       'jpeg/encoder_libjpeg.cpp',
>>>>>       'jpeg/exif.cpp',
>>>>>       'jpeg/post_processor_jpeg.cpp',
>>>>> +    'jpeg/thumbnailer.cpp',
>>>>>   ])
>>>>>
>>>>>   android_camera_metadata_sources = files([
>> --
>> Regards,
>>
>> Laurent Pinchart
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel@lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

On Tue, Oct 27, 2020 at 4:40 PM Umang Jain <email@uajain.com> wrote:
>
> Hi Hiro,
>
> On 10/27/20 10:51 AM, Hirokazu Honda wrote:
> > Hi Umang,
> >
> > On Tue, Oct 27, 2020 at 7:09 AM Laurent Pinchart
> > <laurent.pinchart@ideasonboard.com> wrote:
> >> Hi Kieran,
> >>
> >> On Mon, Oct 26, 2020 at 10:02:43PM +0000, Kieran Bingham wrote:
> >>> On 26/10/2020 21:05, Laurent Pinchart wrote:
> >>>> On Mon, Oct 26, 2020 at 07:31:34PM +0530, Umang Jain wrote:
> >>>>> Add a basic image thumbnailer for NV12 frames being captured.
> >>>>> It shall generate a thumbnail image to be embedded as a part of
> >>>>> EXIF metadata of the frame. The output of the thumbnail will still
> >>>>> be NV12.
> >>>>>
> >>>>> Signed-off-by: Umang Jain <email@uajain.com>
> >>>>> ---
> >>>>>   src/android/jpeg/exif.cpp                |  16 +++-
> >>>>>   src/android/jpeg/exif.h                  |   1 +
> >>>>>   src/android/jpeg/post_processor_jpeg.cpp |  35 +++++++-
> >>>>>   src/android/jpeg/post_processor_jpeg.h   |   8 +-
> >>>>>   src/android/jpeg/thumbnailer.cpp         | 109 +++++++++++++++++++++++
> >>>>>   src/android/jpeg/thumbnailer.h           |  37 ++++++++
> >>>>>   src/android/meson.build                  |   1 +
> >>>>>   7 files changed, 204 insertions(+), 3 deletions(-)
> >>>>>   create mode 100644 src/android/jpeg/thumbnailer.cpp
> >>>>>   create mode 100644 src/android/jpeg/thumbnailer.h
> >>>>>
> >>>>> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp
> >>>>> index d21534a..24197bd 100644
> >>>>> --- a/src/android/jpeg/exif.cpp
> >>>>> +++ b/src/android/jpeg/exif.cpp
> >>>>> @@ -75,8 +75,16 @@ Exif::~Exif()
> >>>>>     if (exifData_)
> >>>>>             free(exifData_);
> >>>>>
> >>>>> -  if (data_)
> >>>>> +  if (data_) {
> >>>>> +          /*
> >>>>> +           * Reset thumbnail data to avoid getting double-freed by
> >>>>> +           * libexif. It is owned by the caller (i.e. PostProcessorJpeg).
> >>>>> +           */
> >>>>> +          data_->data = nullptr;
> >>>>> +          data_->size = 0;
> >>>>> +
> >>>>>             exif_data_unref(data_);
> >>>>> +  }
> >>>>>
> >>>>>     if (mem_)
> >>>>>             exif_mem_unref(mem_);
> >>>>> @@ -268,6 +276,12 @@ void Exif::setOrientation(int orientation)
> >>>>>     setShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, value);
> >>>>>   }
> >>>>>
> >>>>> +void Exif::setThumbnail(std::vector<unsigned char> &thumbnail)
> >>>>> +{
> >>>>> +  data_->data = thumbnail.data();
> >>>>> +  data_->size = thumbnail.size();
> >>>>> +}
> >>>>> +
> >>>>>   [[nodiscard]] int Exif::generate()
> >>>>>   {
> >>>>>     if (exifData_) {
> >>>>> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h
> >>>>> index 12c27b6..bd54a31 100644
> >>>>> --- a/src/android/jpeg/exif.h
> >>>>> +++ b/src/android/jpeg/exif.h
> >>>>> @@ -26,6 +26,7 @@ public:
> >>>>>
> >>>>>     void setOrientation(int orientation);
> >>>>>     void setSize(const libcamera::Size &size);
> >>>>> +  void setThumbnail(std::vector<unsigned char> &thumbnail);
> >>>> You can pass a Span<const unsigned char> as the setThumbnail() function
> >>>> shouldn't care what storage container is used.
> >>>>
> >>>> It's a bit of a dangerous API, as it will store the pointer internally,
> >>>> without ensuring that the caller keeps the thumbnail valid after the
> >>>> call returns. It's fine, but maybe a comment above the
> >>>> Exif::setThumbnail() functions to state that the thumbnail must remain
> >>>> valid until the Exif object is destroyed would be a good thing.
> >>> I think the comment will help indeed. The only alternative would be to
> >>> pass it into generate(), but and refactor generate() to return the span
> >>> ... but that's more effort that we need right now. So just a comment
> >>> will do ;-)
> >>>
> >>>
> >>>
> >>>>>     void setTimestamp(time_t timestamp);
> >>>>>
> >>>>>     libcamera::Span<const uint8_t> data() const { return { exifData_, size_ }; }
> >>>> I would have moved this to a separate patch.
> >>> Yes, I'd keep the exif extension for setting the thumbnail, and the
> >>> usage separate.
> >>>
> >>>>> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
> >>>>> index c56f1b2..416e831 100644
> >>>>> --- a/src/android/jpeg/post_processor_jpeg.cpp
> >>>>> +++ b/src/android/jpeg/post_processor_jpeg.cpp
> >>>>> @@ -9,7 +9,6 @@
> >>>>>
> >>>>>   #include "../camera_device.h"
> >>>>>   #include "../camera_metadata.h"
> >>>>> -#include "encoder_libjpeg.h"
> >>>>>   #include "exif.h"
> >>>>>
> >>>>>   #include <libcamera/formats.h>
> >>>>> @@ -39,11 +38,42 @@ int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,
> >>>>>     }
> >>>>>
> >>>>>     streamSize_ = outCfg.size;
> >>>>> +
> >>>>> +  thumbnailer_.configure(inCfg.size, inCfg.pixelFormat);
> >>>>> +  StreamConfiguration thCfg = inCfg;
> >>>>> +  thCfg.size = thumbnailer_.size();
> >>>>> +  if (thumbnailEncoder_.configure(thCfg) != 0) {
> >>>>> +          LOG(JPEG, Error) << "Failed to configure thumbnail encoder";
> >>>>> +          return -EINVAL;
> >>>>> +  }
> >>>>> +
> >>>>>     encoder_ = std::make_unique<EncoderLibJpeg>();
> >>>>>
> >>>>>     return encoder_->configure(inCfg);
> >>>>>   }
> >>>>>
> >>>>> +void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,
> >>>>> +                                    std::vector<unsigned char> &thumbnail)
> > std::vector<unsigned char>* thumbnail
> > We would rather use a pointer than reference if a function changes it.
> > Same for other places.
> Ack.
> >>>>> +{
> >>>>> +  /* Stores the raw scaled-down thumbnail bytes. */
> >>>>> +  std::vector<unsigned char> rawThumbnail;
> >>>>> +
> >>>>> +  thumbnailer_.scaleBuffer(source, rawThumbnail);
> >>>>> +
> >>>>> +  if (rawThumbnail.data()) {
> >>>> This should check for ! .empty() (see additional comments below).
> >>>>
> >>>>> +          thumbnail.reserve(rawThumbnail.capacity());
> >>>> You should call thumbnail.resize(), and use size() instead of capacity()
> >>>> below, as reserve() allocates memory but doesn't change the size of the
> >>>> vector, so it's semantically dangerous to write to the reserved storage
> >>>> space if it hasn't been marked as in use with .resize().
> >>>>
> >>>>> +
> >>>>> +          int jpeg_size = thumbnailEncoder_.encode(
> >>>>> +                          { rawThumbnail.data(), rawThumbnail.capacity() },
> >>>>> +                          { thumbnail.data(), thumbnail.capacity() },
> >>>> Just pass rawThumbnail and thumbnail, there's an implicit constructor
> >>>> for Span that will turn them into a span using .data() and .size().
> >>>>
> >>>>> +                          { });
> >>>>> +          thumbnail.resize(jpeg_size);
> >>>>> +
> >>>>> +          LOG(JPEG, Info) << "Thumbnail compress returned "
> >>>>> +                          << jpeg_size << " bytes";
> >>>> When log messages don't fit on one line, we usually wrap them as
> >>>>
> >>>>              LOG(JPEG, Info)
> >>>>                      << "Thumbnail compress returned "
> >>>>                      << jpeg_size << " bytes";
> >>>>
> >>>> And maybe debug instead of info ?
> >>>>
> >>>>> +  }
> >>>>> +}
> > Shall this function handle a failure of jpeg encode?
> Ideally it should but I am wondering in what style. I would prefer that
> checking for the (thumbnail data != 0) before calling
> Exif::setThumbnail() below. Since, when the encoding fails, jpeg_size
> will get 0, and the thumbnail vector(placeholder for our thumbnail data)
> will get resized to '0'.

Ack.

> >>>>> +
> >>>>>   int PostProcessorJpeg::process(const FrameBuffer &source,
> >>>>>                            Span<uint8_t> destination,
> >>>>>                            CameraMetadata *metadata)
> >>>>> @@ -64,6 +94,9 @@ int PostProcessorJpeg::process(const FrameBuffer &source,
> >>>>>      * second, it is good enough.
> >>>>>      */
> >>>>>     exif.setTimestamp(std::time(nullptr));
> >>>>> +  std::vector<unsigned char> thumbnail;
> >>>>> +  generateThumbnail(source, thumbnail);
> >>>>> +  exif.setThumbnail(thumbnail);
> >>>>>     if (exif.generate() != 0)
> >>>>>             LOG(JPEG, Error) << "Failed to generate valid EXIF data";
> >>>>>
> >>>>> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h
> >>>>> index 3706cec..3894231 100644
> >>>>> --- a/src/android/jpeg/post_processor_jpeg.h
> >>>>> +++ b/src/android/jpeg/post_processor_jpeg.h
> >>>>> @@ -8,12 +8,13 @@
> >>>>>   #define __ANDROID_POST_PROCESSOR_JPEG_H__
> >>>>>
> >>>>>   #include "../post_processor.h"
> >>>>> +#include "encoder_libjpeg.h"
> >>>>> +#include "thumbnailer.h"
> >>>>>
> >>>>>   #include <libcamera/geometry.h>
> >>>>>
> >>>>>   #include "libcamera/internal/buffer.h"
> >>>>>
> >>>>> -class Encoder;
> >>>>>   class CameraDevice;
> >>>>>
> >>>>>   class PostProcessorJpeg : public PostProcessor
> >>>>> @@ -28,9 +29,14 @@ public:
> >>>>>                 CameraMetadata *metadata) override;
> >>>>>
> >>>>>   private:
> >>>>> +  void generateThumbnail(const libcamera::FrameBuffer &source,
> >>>>> +                         std::vector<unsigned char> &thumbnail);
> >>>>> +
> >>>>>     CameraDevice *const cameraDevice_;
> >>>>>     std::unique_ptr<Encoder> encoder_;
> >>>>>     libcamera::Size streamSize_;
> >>>>> +  EncoderLibJpeg thumbnailEncoder_;
> >>>> Could you store this in a std::unique_ptr<Encoder> instead ? The reason
> >>>> is that we shouldn't hardcode usage of EncoderLibJpeg, in order to
> >>>> support different encoders later. You won't need to include
> >>>> encoder_libjpeg.h then, and can keep the Encoder forwared declaration.
> >>> I think this was already a unique_ptr in a previous iteration, and I
> >>> suggested moving it to directly store an instance of the libjpeg encoder.
> >>>
> >>> In this instance of encoding a thumbnail, when encoding a 160x160 image,
> >>> I would be weary about the overhead of setting up a hardware encode, and
> >>> I'd expect the preparation phases of that to be potentially more
> >>> expensive than just a software encode. Particularly as this has just
> >>> done a software rescale, so it would have to cache-flush, when it could
> >>> just use the host-cpu with a hot-cache. (ok, so perhaps later it might
> >>> use a different scaler too ... but then it's likely a different equation
> >>> anyway)
> >>>
> >>> I have not measured that of course, as we don't yet have a hw-jpeg
> >>> encode post-processor. It will be an interesting test in the future.
> >>>
> >>> But essentially, I think we're just as well leaving this as a single
> >>> instance of libjpeg for thumbnails. I think I recall the CrOS HAL using
> >>> libjpeg as well, but I haven't gone back to double-check.
> >> Fair enough, those are good points. I'm fine if the code is kept as-is
> >> (I wouldn't mind a unique_ptr of course :-)). Umang, up to you.
> >>
> > Although either is fine to me, I wonder why the encoder for the
> > thumbnail is within the thumbnailer.
> It is not within the encoder. It is inside the PostProcessorJpeg

Sorry, I meant why ... is NOT within ....

> > Since the thumbnailer is currently being used for thumbnail, it seems
> > to be reasonable to move the encoder to it.
> > What do you think?
> If you mean moving the thumbnail-encoder to `class Thumbnailer` - hmm,
> that could also work I suppose. However,  I might not be very
> enthusiastic about it, as PostProcessorJpeg captures the entire flow at
> one place : e.g. exif generation, creating a thumbnail, encoding it and
> finally encoding the entire frame. I like this.
>
> If others support moving the thumbnail-encoder to Thumbnailer, that's
> also fine! (Also, could be done on top, as an independent refactor)

I would like to hear others opinions here.
I would move it to Thumbnailer, so that
1.) a reader can understand at a glance the encoder is used for
generating thumbnails, and
2.) thumbnail is always meant JPEG here and then it makes more sense
that thumbnailer outputs JPEG.

Especially regarding 2, will the thumbnail possibly be other formats?

Best Regards,
-Hiro
> >
> >>>>> +  Thumbnailer thumbnailer_;
> >>>> This is good, as I don't expect different thumbnailer types.
> >>>>
> >>>>>   };
> >>>>>
> >>>>>   #endif /* __ANDROID_POST_PROCESSOR_JPEG_H__ */
> >>>>> diff --git a/src/android/jpeg/thumbnailer.cpp b/src/android/jpeg/thumbnailer.cpp
> >>>>> new file mode 100644
> >>>>> index 0000000..f880ffb
> >>>>> --- /dev/null
> >>>>> +++ b/src/android/jpeg/thumbnailer.cpp
> >>>>> @@ -0,0 +1,109 @@
> >>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> >>>> Wrong license.
> >>>>
> >>>>> +/*
> >>>>> + * Copyright (C) 2020, Google Inc.
> >>>>> + *
> >>>>> + * thumbnailer.cpp - Simple image thumbnailer
> >>>>> + */
> >>>>> +
> >>>>> +#include "thumbnailer.h"
> >>>>> +
> >>>>> +#include <libcamera/formats.h>
> >>>>> +
> >>>>> +#include "libcamera/internal/file.h"
> >>>> Why do you need this header ?
> >>>>
> >>>>> +#include "libcamera/internal/log.h"
> >>>>> +
> >>>>> +using namespace libcamera;
> >>>>> +
> >>>>> +LOG_DEFINE_CATEGORY(Thumbnailer)
> >>>>> +
> >>>>> +Thumbnailer::Thumbnailer() : valid_(false)
> >>>>      : valid_(false)
> >>>>
> >>>>> +{
> >>>>> +}
> >>>>> +
> >>>>> +void Thumbnailer::configure(const Size &sourceSize, PixelFormat pixelFormat)
> >>>>> +{
> >>>>> +  sourceSize_ = sourceSize;
> >>>>> +  pixelFormat_ = pixelFormat;
> >>>>> +
> >>>>> +  if (pixelFormat_ != formats::NV12) {
> >>>>> +          LOG (Thumbnailer, Error) << "Failed to configure: Pixel Format "
> >>>>> +                              << pixelFormat_.toString() << " unsupported.";
> >>>>              LOG (Thumbnailer, Error)
> >>>>                      << "Failed to configure: Pixel Format "
> >>>>                      << pixelFormat_.toString() << " unsupported.";
> >>>>
> >>>>> +          return;
> >>>>> +  }
> >>>>> +
> >>>>> +  targetSize_ = computeThumbnailSize();
> >>>>> +
> >>>>> +  valid_ = true;
> >>>>> +}
> >>>>> +
> >>>>> +/*
> >>>>> + * The Exif specification recommends the width of the thumbnail to be a
> >>>>> + * mutiple of 16 (section 4.8.1). Hence, compute the corresponding height
> >>>> s/mutiple/multiple/
> >>>>
> >>>>> + * keeping the aspect ratio same as of the source.
> >>>>> + */
> >>>>> +Size Thumbnailer::computeThumbnailSize()
> >>>>> +{
> >>>>> +  unsigned int targetHeight;
> >>>>> +  unsigned int targetWidth = 160;
> > nit: constexpr unsigned int kTargetWidth = 160;
> >
> >>>>> +
> >>>>> +  targetHeight = targetWidth * sourceSize_.height / sourceSize_.width;
> >>>>> +
> >>>>> +  if (targetHeight & 1)
> >>>>> +          targetHeight++;
> >>>>> +
> >>>>> +  return Size(targetWidth, targetHeight);
> >>>>> +}
> >>>>> +
> >>>>> +void
> >>>>> +Thumbnailer::scaleBuffer(const FrameBuffer &source,
> >>>>> +                   std::vector<unsigned char> &destination)
> >>>>> +{
> >>>>> +  MappedFrameBuffer frame(&source, PROT_READ);
> >>>>> +  if (!frame.isValid()) {
> >>>>> +          LOG(Thumbnailer, Error) << "Failed to map FrameBuffer : "
> >>>>> +                           << strerror(frame.error());
> >>>>              LOG(Thumbnailer, Error)
> >>>>                      << "Failed to map FrameBuffer : "
> >>>>                      << strerror(frame.error());
> >>>>
> >>>>> +          return;
> >>>>> +  }
> >>>>> +
> >>>>> +  if (!valid_) {
> >>>>> +          LOG(Thumbnailer, Error) << "config is unconfigured or invalid.";
> >>>>> +          return;
> >>>>> +  }
> >>>>> +
> >>>>> +  const unsigned int sw = sourceSize_.width;
> >>>>> +  const unsigned int sh = sourceSize_.height;
> >>>>> +  const unsigned int tw = targetSize_.width;
> >>>>> +  const unsigned int th = targetSize_.height;
> >>>>> +
> >>>>> +  /* Image scaling block implementing nearest-neighbour algorithm. */
> >>>>> +  unsigned char *src = static_cast<unsigned char *>(frame.maps()[0].data());
> >>>>> +  unsigned char *src_c = src + sh * sw;
> >>>>> +  unsigned char *src_cb, *src_cr;
> >>>>> +  unsigned char *dst_y, *src_y;
> >>>>> +
> >>>>> +  size_t dstSize = (th * tw) + ((th / 2) * tw);
> >>>>> +  destination.reserve(dstSize);
> >>>> This should be
> >>>>
> >>>>      destination.resize(dstSize);
> >>>>
> >>>> as reserve() allocates memory but doesn't change the size of the vector.
> >>>>
> >>>>> +  unsigned char *dst = destination.data();
> >>>>> +  unsigned char *dst_c = dst + th * tw;
> >>>>> +
> >>>>> +  for (unsigned int y = 0; y < th; y += 2) {
> >>>>> +          unsigned int sourceY = (sh * y + th / 2) / th;
> >>>>> +
> >>>>> +          dst_y = dst + y * tw;
> >>>>> +          src_y = src + sw * sourceY;
> >>>>> +          src_cb = src_c + (sourceY / 2) * sw + 0;
> >>>>> +          src_cr = src_c + (sourceY / 2) * sw + 1;
> >>>>> +
> >>>>> +          for (unsigned int x = 0; x < tw; x += 2) {
> >>>>> +                  unsigned int sourceX = (sw * x + tw / 2) / tw;
> >>>>> +
> >>>>> +                  dst_y[x] = src_y[sourceX];
> >>>>> +                  dst_y[tw + x] = src_y[sw + sourceX];
> >>>>> +                  dst_y[x + 1] = src_y[sourceX + 1];
> >>>>> +                  dst_y[tw + x + 1] = src_y[sw + sourceX + 1];
> >>>>> +
> >>>>> +                  dst_c[(y / 2) * tw + x + 0] = src_cb[(sourceX / 2) * 2];
> >>>>> +                  dst_c[(y / 2) * tw + x + 1] = src_cr[(sourceX / 2) * 2];
> >>>>> +          }
> >>>>> +  }
> >>>>> + }
> >>>> Extra space.
> >>>>
> >>>>> diff --git a/src/android/jpeg/thumbnailer.h b/src/android/jpeg/thumbnailer.h
> >>>>> new file mode 100644
> >>>>> index 0000000..b769619
> >>>>> --- /dev/null
> >>>>> +++ b/src/android/jpeg/thumbnailer.h
> >>>>> @@ -0,0 +1,37 @@
> >>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> >>>> Wrong license.
> >>>>
> >>>>> +/*
> >>>>> + * Copyright (C) 2020, Google Inc.
> >>>>> + *
> >>>>> + * thumbnailer.h - Simple image thumbnailer
> >>>>> + */
> >>>>> +#ifndef __ANDROID_JPEG_THUMBNAILER_H__
> >>>>> +#define __ANDROID_JPEG_THUMBNAILER_H__
> >>>>> +
> >>>>> +#include <libcamera/geometry.h>
> >>>>> +
> >>>>> +#include "libcamera/internal/buffer.h"
> >>>>> +#include "libcamera/internal/formats.h"
> >>>>> +
> >>>>> +class Thumbnailer
> >>>>> +{
> >>>>> +public:
> >>>>> +  Thumbnailer();
> >>>>> +
> >>>>> +  void configure(const libcamera::Size &sourceSize,
> >>>>> +                 libcamera::PixelFormat pixelFormat);
> >>>>> +  void scaleBuffer(const libcamera::FrameBuffer &source,
> >>>>> +                   std::vector<unsigned char> &dest);
> >>>> How about naming this createThumbnail() or generateThumbnail() ? And the
> >>>> function should be const.
> >>>>
> >>>>> +  libcamera::Size size() const { return targetSize_; }
> > nit: to reduce unnecessary copy, let's return const reference.
> > const libcamera::Size& computeThumbanilSize() const;
> >
> > Best Regards,
> > -Hiro
> >>>>> +
> >>>>> +private:
> >>>>> +  libcamera::Size computeThumbnailSize();
> >>>> This can be
> >>>>
> >>>>      libcamera::Size computeThumbnailSize() const;
> >>>>
> >>>>> +
> >>>>> +  libcamera::PixelFormat pixelFormat_;
> >>>>> +  libcamera::Size sourceSize_;
> >>>>> +  libcamera::Size targetSize_;
> >>>>> +
> >>>>> +  bool valid_;
> >>>>> +
> >>>> No need for a blank line.
> >>>>
> >>>>> +};
> >>>>> +
> >>>>> +#endif /* __ANDROID_JPEG_THUMBNAILER_H__ */
> >>>>> diff --git a/src/android/meson.build b/src/android/meson.build
> >>>>> index f72376a..3d4d3be 100644
> >>>>> --- a/src/android/meson.build
> >>>>> +++ b/src/android/meson.build
> >>>>> @@ -25,6 +25,7 @@ android_hal_sources = files([
> >>>>>       'jpeg/encoder_libjpeg.cpp',
> >>>>>       'jpeg/exif.cpp',
> >>>>>       'jpeg/post_processor_jpeg.cpp',
> >>>>> +    'jpeg/thumbnailer.cpp',
> >>>>>   ])
> >>>>>
> >>>>>   android_camera_metadata_sources = files([
> >> --
> >> Regards,
> >>
> >> Laurent Pinchart
> >> _______________________________________________
> >> libcamera-devel mailing list
> >> libcamera-devel@lists.libcamera.org
> >> https://lists.libcamera.org/listinfo/libcamera-devel
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>

Hi Laurent,

On 10/27/20 3:38 AM, Laurent Pinchart wrote:
> Hi Kieran,
>
> On Mon, Oct 26, 2020 at 10:02:43PM +0000, Kieran Bingham wrote:
>> On 26/10/2020 21:05, Laurent Pinchart wrote:
>>> On Mon, Oct 26, 2020 at 07:31:34PM +0530, Umang Jain wrote:
>>>> Add a basic image thumbnailer for NV12 frames being captured.
>>>> It shall generate a thumbnail image to be embedded as a part of
>>>> EXIF metadata of the frame. The output of the thumbnail will still
>>>> be NV12.
>>>>
>>>> Signed-off-by: Umang Jain <email@uajain.com>
>>>> ---
>>>>   src/android/jpeg/exif.cpp                |  16 +++-
>>>>   src/android/jpeg/exif.h                  |   1 +
>>>>   src/android/jpeg/post_processor_jpeg.cpp |  35 +++++++-
>>>>   src/android/jpeg/post_processor_jpeg.h   |   8 +-
>>>>   src/android/jpeg/thumbnailer.cpp         | 109 +++++++++++++++++++++++
>>>>   src/android/jpeg/thumbnailer.h           |  37 ++++++++
>>>>   src/android/meson.build                  |   1 +
>>>>   7 files changed, 204 insertions(+), 3 deletions(-)
>>>>   create mode 100644 src/android/jpeg/thumbnailer.cpp
>>>>   create mode 100644 src/android/jpeg/thumbnailer.h
>>>>
>>>> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp
>>>> index d21534a..24197bd 100644
>>>> --- a/src/android/jpeg/exif.cpp
>>>> +++ b/src/android/jpeg/exif.cpp
>>>> @@ -75,8 +75,16 @@ Exif::~Exif()
>>>>   	if (exifData_)
>>>>   		free(exifData_);
>>>>   
>>>> -	if (data_)
>>>> +	if (data_) {
>>>> +		/*
>>>> +		 * Reset thumbnail data to avoid getting double-freed by
>>>> +		 * libexif. It is owned by the caller (i.e. PostProcessorJpeg).
>>>> +		 */
>>>> +		data_->data = nullptr;
>>>> +		data_->size = 0;
>>>> +
>>>>   		exif_data_unref(data_);
>>>> +	}
>>>>   
>>>>   	if (mem_)
>>>>   		exif_mem_unref(mem_);
>>>> @@ -268,6 +276,12 @@ void Exif::setOrientation(int orientation)
>>>>   	setShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, value);
>>>>   }
>>>>   
>>>> +void Exif::setThumbnail(std::vector<unsigned char> &thumbnail)
>>>> +{
>>>> +	data_->data = thumbnail.data();
>>>> +	data_->size = thumbnail.size();
>>>> +}
>>>> +
>>>>   [[nodiscard]] int Exif::generate()
>>>>   {
>>>>   	if (exifData_) {
>>>> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h
>>>> index 12c27b6..bd54a31 100644
>>>> --- a/src/android/jpeg/exif.h
>>>> +++ b/src/android/jpeg/exif.h
>>>> @@ -26,6 +26,7 @@ public:
>>>>   
>>>>   	void setOrientation(int orientation);
>>>>   	void setSize(const libcamera::Size &size);
>>>> +	void setThumbnail(std::vector<unsigned char> &thumbnail);
>>> You can pass a Span<const unsigned char> as the setThumbnail() function
>>> shouldn't care what storage container is used.
>>>
>>> It's a bit of a dangerous API, as it will store the pointer internally,
>>> without ensuring that the caller keeps the thumbnail valid after the
>>> call returns. It's fine, but maybe a comment above the
>>> Exif::setThumbnail() functions to state that the thumbnail must remain
>>> valid until the Exif object is destroyed would be a good thing.
>> I think the comment will help indeed. The only alternative would be to
>> pass it into generate(), but and refactor generate() to return the span
>> ... but that's more effort that we need right now. So just a comment
>> will do ;-)
>>
>>
>>
>>>>   	void setTimestamp(time_t timestamp);
>>>>   
>>>>   	libcamera::Span<const uint8_t> data() const { return { exifData_, size_ }; }
>>> I would have moved this to a separate patch.
>>
>> Yes, I'd keep the exif extension for setting the thumbnail, and the
>> usage separate.
>>
>>>> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
>>>> index c56f1b2..416e831 100644
>>>> --- a/src/android/jpeg/post_processor_jpeg.cpp
>>>> +++ b/src/android/jpeg/post_processor_jpeg.cpp
>>>> @@ -9,7 +9,6 @@
>>>>   
>>>>   #include "../camera_device.h"
>>>>   #include "../camera_metadata.h"
>>>> -#include "encoder_libjpeg.h"
>>>>   #include "exif.h"
>>>>   
>>>>   #include <libcamera/formats.h>
>>>> @@ -39,11 +38,42 @@ int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,
>>>>   	}
>>>>   
>>>>   	streamSize_ = outCfg.size;
>>>> +
>>>> +	thumbnailer_.configure(inCfg.size, inCfg.pixelFormat);
>>>> +	StreamConfiguration thCfg = inCfg;
>>>> +	thCfg.size = thumbnailer_.size();
>>>> +	if (thumbnailEncoder_.configure(thCfg) != 0) {
>>>> +		LOG(JPEG, Error) << "Failed to configure thumbnail encoder";
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>>   	encoder_ = std::make_unique<EncoderLibJpeg>();
>>>>   
>>>>   	return encoder_->configure(inCfg);
>>>>   }
>>>>   
>>>> +void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,
>>>> +					  std::vector<unsigned char> &thumbnail)
>>>> +{
>>>> +	/* Stores the raw scaled-down thumbnail bytes. */
>>>> +	std::vector<unsigned char> rawThumbnail;
>>>> +
>>>> +	thumbnailer_.scaleBuffer(source, rawThumbnail);
>>>> +
>>>> +	if (rawThumbnail.data()) {
>>> This should check for ! .empty() (see additional comments below).
>>>
>>>> +		thumbnail.reserve(rawThumbnail.capacity());
>>> You should call thumbnail.resize(), and use size() instead of capacity()
>>> below, as reserve() allocates memory but doesn't change the size of the
>>> vector, so it's semantically dangerous to write to the reserved storage
>>> space if it hasn't been marked as in use with .resize().
>>>
>>>> +
>>>> +		int jpeg_size = thumbnailEncoder_.encode(
>>>> +				{ rawThumbnail.data(), rawThumbnail.capacity() },
>>>> +				{ thumbnail.data(), thumbnail.capacity() },
>>> Just pass rawThumbnail and thumbnail, there's an implicit constructor
>>> for Span that will turn them into a span using .data() and .size().
>>>
>>>> +				{ });
>>>> +		thumbnail.resize(jpeg_size);
>>>> +
>>>> +		LOG(JPEG, Info) << "Thumbnail compress returned "
>>>> +				<< jpeg_size << " bytes";
>>> When log messages don't fit on one line, we usually wrap them as
>>>
>>> 		LOG(JPEG, Info)
>>> 			<< "Thumbnail compress returned "
>>> 			<< jpeg_size << " bytes";
>>>
>>> And maybe debug instead of info ?
>>>
>>>> +	}
>>>> +}
>>>> +
>>>>   int PostProcessorJpeg::process(const FrameBuffer &source,
>>>>   			       Span<uint8_t> destination,
>>>>   			       CameraMetadata *metadata)
>>>> @@ -64,6 +94,9 @@ int PostProcessorJpeg::process(const FrameBuffer &source,
>>>>   	 * second, it is good enough.
>>>>   	 */
>>>>   	exif.setTimestamp(std::time(nullptr));
>>>> +	std::vector<unsigned char> thumbnail;
>>>> +	generateThumbnail(source, thumbnail);
>>>> +	exif.setThumbnail(thumbnail);
>>>>   	if (exif.generate() != 0)
>>>>   		LOG(JPEG, Error) << "Failed to generate valid EXIF data";
>>>>   
>>>> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h
>>>> index 3706cec..3894231 100644
>>>> --- a/src/android/jpeg/post_processor_jpeg.h
>>>> +++ b/src/android/jpeg/post_processor_jpeg.h
>>>> @@ -8,12 +8,13 @@
>>>>   #define __ANDROID_POST_PROCESSOR_JPEG_H__
>>>>   
>>>>   #include "../post_processor.h"
>>>> +#include "encoder_libjpeg.h"
>>>> +#include "thumbnailer.h"
>>>>   
>>>>   #include <libcamera/geometry.h>
>>>>   
>>>>   #include "libcamera/internal/buffer.h"
>>>>   
>>>> -class Encoder;
>>>>   class CameraDevice;
>>>>   
>>>>   class PostProcessorJpeg : public PostProcessor
>>>> @@ -28,9 +29,14 @@ public:
>>>>   		    CameraMetadata *metadata) override;
>>>>   
>>>>   private:
>>>> +	void generateThumbnail(const libcamera::FrameBuffer &source,
>>>> +			       std::vector<unsigned char> &thumbnail);
>>>> +
>>>>   	CameraDevice *const cameraDevice_;
>>>>   	std::unique_ptr<Encoder> encoder_;
>>>>   	libcamera::Size streamSize_;
>>>> +	EncoderLibJpeg thumbnailEncoder_;
>>> Could you store this in a std::unique_ptr<Encoder> instead ? The reason
>>> is that we shouldn't hardcode usage of EncoderLibJpeg, in order to
>>> support different encoders later. You won't need to include
>>> encoder_libjpeg.h then, and can keep the Encoder forwared declaration.
>> I think this was already a unique_ptr in a previous iteration, and I
>> suggested moving it to directly store an instance of the libjpeg encoder.
>>
>> In this instance of encoding a thumbnail, when encoding a 160x160 image,
>> I would be weary about the overhead of setting up a hardware encode, and
>> I'd expect the preparation phases of that to be potentially more
>> expensive than just a software encode. Particularly as this has just
>> done a software rescale, so it would have to cache-flush, when it could
>> just use the host-cpu with a hot-cache. (ok, so perhaps later it might
>> use a different scaler too ... but then it's likely a different equation
>> anyway)
>>
>> I have not measured that of course, as we don't yet have a hw-jpeg
>> encode post-processor. It will be an interesting test in the future.
>>
>> But essentially, I think we're just as well leaving this as a single
>> instance of libjpeg for thumbnails. I think I recall the CrOS HAL using
>> libjpeg as well, but I haven't gone back to double-check.
> Fair enough, those are good points. I'm fine if the code is kept as-is
> (I wouldn't mind a unique_ptr of course :-)). Umang, up to you.
Do you specfically suggest using unique_ptr, to make use of forward 
declaration, in this instance of EncoderLibJpeg(see a diff below)? I am 
curious to ask, because in order to make it work with incomplete 
types(i.e. forward declared), I need to insert destructor declaration in 
post_processor_jpeg.h and define a (blank)destructor in 
post_processor_jpeg.cpp

I learnt this and can see it in action. Ref: 
https://stackoverflow.com/a/42158611

-----
diff --git a/src/android/jpeg/post_processor_jpeg.h 
b/src/android/jpeg/post_processor_jpeg.h
index 3706cec..8b36a21 100644
--- a/src/android/jpeg/post_processor_jpeg.h
+++ b/src/android/jpeg/post_processor_jpeg.h
@@ -8,12 +8,14 @@
  #define __ANDROID_POST_PROCESSOR_JPEG_H__

  #include "../post_processor.h"
+#include "thumbnailer.h"

  #include <libcamera/geometry.h>

  #include "libcamera/internal/buffer.h"

  class Encoder;
+class EncoderLibJpeg;
  class CameraDevice;

  class PostProcessorJpeg : public PostProcessor
@@ -28,9 +30,14 @@ public:
                     CameraMetadata *metadata) override;

  private:
+       void generateThumbnail(const libcamera::FrameBuffer &source,
+                              std::vector<unsigned char> *thumbnail);
+
         CameraDevice *const cameraDevice_;
         std::unique_ptr<Encoder> encoder_;
         libcamera::Size streamSize_;
+       std::unique_ptr<EncoderLibJpeg> thumbnailEncoder_;
+       Thumbnailer thumbnailer_;
  };

>
>>>> +	Thumbnailer thumbnailer_;
>>> This is good, as I don't expect different thumbnailer types.
>>>
>>>>   };
>>>>   
>>>>   #endif /* __ANDROID_POST_PROCESSOR_JPEG_H__ */
>>>> diff --git a/src/android/jpeg/thumbnailer.cpp b/src/android/jpeg/thumbnailer.cpp
>>>> new file mode 100644
>>>> index 0000000..f880ffb
>>>> --- /dev/null
>>>> +++ b/src/android/jpeg/thumbnailer.cpp
>>>> @@ -0,0 +1,109 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>> Wrong license.
>>>
>>>> +/*
>>>> + * Copyright (C) 2020, Google Inc.
>>>> + *
>>>> + * thumbnailer.cpp - Simple image thumbnailer
>>>> + */
>>>> +
>>>> +#include "thumbnailer.h"
>>>> +
>>>> +#include <libcamera/formats.h>
>>>> +
>>>> +#include "libcamera/internal/file.h"
>>> Why do you need this header ?
>>>
>>>> +#include "libcamera/internal/log.h"
>>>> +
>>>> +using namespace libcamera;
>>>> +
>>>> +LOG_DEFINE_CATEGORY(Thumbnailer)
>>>> +
>>>> +Thumbnailer::Thumbnailer() : valid_(false)
>>> 	: valid_(false)
>>>
>>>> +{
>>>> +}
>>>> +
>>>> +void Thumbnailer::configure(const Size &sourceSize, PixelFormat pixelFormat)
>>>> +{
>>>> +	sourceSize_ = sourceSize;
>>>> +	pixelFormat_ = pixelFormat;
>>>> +
>>>> +	if (pixelFormat_ != formats::NV12) {
>>>> +		LOG (Thumbnailer, Error) << "Failed to configure: Pixel Format "
>>>> +				    << pixelFormat_.toString() << " unsupported.";
>>> 		LOG (Thumbnailer, Error)
>>> 			<< "Failed to configure: Pixel Format "
>>> 			<< pixelFormat_.toString() << " unsupported.";
>>>
>>>> +		return;
>>>> +	}
>>>> +
>>>> +	targetSize_ = computeThumbnailSize();
>>>> +
>>>> +	valid_ = true;
>>>> +}
>>>> +
>>>> +/*
>>>> + * The Exif specification recommends the width of the thumbnail to be a
>>>> + * mutiple of 16 (section 4.8.1). Hence, compute the corresponding height
>>> s/mutiple/multiple/
>>>
>>>> + * keeping the aspect ratio same as of the source.
>>>> + */
>>>> +Size Thumbnailer::computeThumbnailSize()
>>>> +{
>>>> +	unsigned int targetHeight;
>>>> +	unsigned int targetWidth = 160;
>>>> +
>>>> +	targetHeight = targetWidth * sourceSize_.height / sourceSize_.width;
>>>> +
>>>> +	if (targetHeight & 1)
>>>> +		targetHeight++;
>>>> +
>>>> +	return Size(targetWidth, targetHeight);
>>>> +}
>>>> +
>>>> +void
>>>> +Thumbnailer::scaleBuffer(const FrameBuffer &source,
>>>> +			 std::vector<unsigned char> &destination)
>>>> +{
>>>> +	MappedFrameBuffer frame(&source, PROT_READ);
>>>> +	if (!frame.isValid()) {
>>>> +		LOG(Thumbnailer, Error) << "Failed to map FrameBuffer : "
>>>> +				 << strerror(frame.error());
>>> 		LOG(Thumbnailer, Error)
>>> 			<< "Failed to map FrameBuffer : "
>>> 			<< strerror(frame.error());
>>>
>>>> +		return;
>>>> +	}
>>>> +
>>>> +	if (!valid_) {
>>>> +		LOG(Thumbnailer, Error) << "config is unconfigured or invalid.";
>>>> +		return;
>>>> +	}
>>>> +
>>>> +	const unsigned int sw = sourceSize_.width;
>>>> +	const unsigned int sh = sourceSize_.height;
>>>> +	const unsigned int tw = targetSize_.width;
>>>> +	const unsigned int th = targetSize_.height;
>>>> +
>>>> +	/* Image scaling block implementing nearest-neighbour algorithm. */
>>>> +	unsigned char *src = static_cast<unsigned char *>(frame.maps()[0].data());
>>>> +	unsigned char *src_c = src + sh * sw;
>>>> +	unsigned char *src_cb, *src_cr;
>>>> +	unsigned char *dst_y, *src_y;
>>>> +
>>>> +	size_t dstSize = (th * tw) + ((th / 2) * tw);
>>>> +	destination.reserve(dstSize);
>>> This should be
>>>
>>> 	destination.resize(dstSize);
>>>
>>> as reserve() allocates memory but doesn't change the size of the vector.
>>>
>>>> +	unsigned char *dst = destination.data();
>>>> +	unsigned char *dst_c = dst + th * tw;
>>>> +
>>>> +	for (unsigned int y = 0; y < th; y += 2) {
>>>> +		unsigned int sourceY = (sh * y + th / 2) / th;
>>>> +
>>>> +		dst_y = dst + y * tw;
>>>> +		src_y = src + sw * sourceY;
>>>> +		src_cb = src_c + (sourceY / 2) * sw + 0;
>>>> +		src_cr = src_c + (sourceY / 2) * sw + 1;
>>>> +
>>>> +		for (unsigned int x = 0; x < tw; x += 2) {
>>>> +			unsigned int sourceX = (sw * x + tw / 2) / tw;
>>>> +
>>>> +			dst_y[x] = src_y[sourceX];
>>>> +			dst_y[tw + x] = src_y[sw + sourceX];
>>>> +			dst_y[x + 1] = src_y[sourceX + 1];
>>>> +			dst_y[tw + x + 1] = src_y[sw + sourceX + 1];
>>>> +
>>>> +			dst_c[(y / 2) * tw + x + 0] = src_cb[(sourceX / 2) * 2];
>>>> +			dst_c[(y / 2) * tw + x + 1] = src_cr[(sourceX / 2) * 2];
>>>> +		}
>>>> +	}
>>>> + }
>>> Extra space.
>>>
>>>> diff --git a/src/android/jpeg/thumbnailer.h b/src/android/jpeg/thumbnailer.h
>>>> new file mode 100644
>>>> index 0000000..b769619
>>>> --- /dev/null
>>>> +++ b/src/android/jpeg/thumbnailer.h
>>>> @@ -0,0 +1,37 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>> Wrong license.
>>>
>>>> +/*
>>>> + * Copyright (C) 2020, Google Inc.
>>>> + *
>>>> + * thumbnailer.h - Simple image thumbnailer
>>>> + */
>>>> +#ifndef __ANDROID_JPEG_THUMBNAILER_H__
>>>> +#define __ANDROID_JPEG_THUMBNAILER_H__
>>>> +
>>>> +#include <libcamera/geometry.h>
>>>> +
>>>> +#include "libcamera/internal/buffer.h"
>>>> +#include "libcamera/internal/formats.h"
>>>> +
>>>> +class Thumbnailer
>>>> +{
>>>> +public:
>>>> +	Thumbnailer();
>>>> +
>>>> +	void configure(const libcamera::Size &sourceSize,
>>>> +		       libcamera::PixelFormat pixelFormat);
>>>> +	void scaleBuffer(const libcamera::FrameBuffer &source,
>>>> +			 std::vector<unsigned char> &dest);
>>> How about naming this createThumbnail() or generateThumbnail() ? And the
>>> function should be const.
>>>
>>>> +	libcamera::Size size() const { return targetSize_; }
>>>> +
>>>> +private:
>>>> +	libcamera::Size computeThumbnailSize();
>>> This can be
>>>
>>> 	libcamera::Size computeThumbnailSize() const;
>>>
>>>> +
>>>> +	libcamera::PixelFormat pixelFormat_;
>>>> +	libcamera::Size sourceSize_;
>>>> +	libcamera::Size targetSize_;
>>>> +
>>>> +	bool valid_;
>>>> +
>>> No need for a blank line.
>>>
>>>> +};
>>>> +
>>>> +#endif /* __ANDROID_JPEG_THUMBNAILER_H__ */
>>>> diff --git a/src/android/meson.build b/src/android/meson.build
>>>> index f72376a..3d4d3be 100644
>>>> --- a/src/android/meson.build
>>>> +++ b/src/android/meson.build
>>>> @@ -25,6 +25,7 @@ android_hal_sources = files([
>>>>       'jpeg/encoder_libjpeg.cpp',
>>>>       'jpeg/exif.cpp',
>>>>       'jpeg/post_processor_jpeg.cpp',
>>>> +    'jpeg/thumbnailer.cpp',
>>>>   ])
>>>>   
>>>>   android_camera_metadata_sources = files([

Hi Hiro-san,

On Tue, Oct 27, 2020 at 05:04:00PM +0900, Hirokazu Honda wrote:
> On Tue, Oct 27, 2020 at 4:40 PM Umang Jain wrote:
> > On 10/27/20 10:51 AM, Hirokazu Honda wrote:
> > > On Tue, Oct 27, 2020 at 7:09 AM Laurent Pinchart wrote:
> > >> On Mon, Oct 26, 2020 at 10:02:43PM +0000, Kieran Bingham wrote:
> > >>> On 26/10/2020 21:05, Laurent Pinchart wrote:
> > >>>> On Mon, Oct 26, 2020 at 07:31:34PM +0530, Umang Jain wrote:
> > >>>>> Add a basic image thumbnailer for NV12 frames being captured.
> > >>>>> It shall generate a thumbnail image to be embedded as a part of
> > >>>>> EXIF metadata of the frame. The output of the thumbnail will still
> > >>>>> be NV12.
> > >>>>>
> > >>>>> Signed-off-by: Umang Jain <email@uajain.com>
> > >>>>> ---
> > >>>>>   src/android/jpeg/exif.cpp                |  16 +++-
> > >>>>>   src/android/jpeg/exif.h                  |   1 +
> > >>>>>   src/android/jpeg/post_processor_jpeg.cpp |  35 +++++++-
> > >>>>>   src/android/jpeg/post_processor_jpeg.h   |   8 +-
> > >>>>>   src/android/jpeg/thumbnailer.cpp         | 109 +++++++++++++++++++++++
> > >>>>>   src/android/jpeg/thumbnailer.h           |  37 ++++++++
> > >>>>>   src/android/meson.build                  |   1 +
> > >>>>>   7 files changed, 204 insertions(+), 3 deletions(-)
> > >>>>>   create mode 100644 src/android/jpeg/thumbnailer.cpp
> > >>>>>   create mode 100644 src/android/jpeg/thumbnailer.h
> > >>>>>
> > >>>>> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp
> > >>>>> index d21534a..24197bd 100644
> > >>>>> --- a/src/android/jpeg/exif.cpp
> > >>>>> +++ b/src/android/jpeg/exif.cpp
> > >>>>> @@ -75,8 +75,16 @@ Exif::~Exif()
> > >>>>>     if (exifData_)
> > >>>>>             free(exifData_);
> > >>>>>
> > >>>>> -  if (data_)
> > >>>>> +  if (data_) {
> > >>>>> +          /*
> > >>>>> +           * Reset thumbnail data to avoid getting double-freed by
> > >>>>> +           * libexif. It is owned by the caller (i.e. PostProcessorJpeg).
> > >>>>> +           */
> > >>>>> +          data_->data = nullptr;
> > >>>>> +          data_->size = 0;
> > >>>>> +
> > >>>>>             exif_data_unref(data_);
> > >>>>> +  }
> > >>>>>
> > >>>>>     if (mem_)
> > >>>>>             exif_mem_unref(mem_);
> > >>>>> @@ -268,6 +276,12 @@ void Exif::setOrientation(int orientation)
> > >>>>>     setShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, value);
> > >>>>>   }
> > >>>>>
> > >>>>> +void Exif::setThumbnail(std::vector<unsigned char> &thumbnail)
> > >>>>> +{
> > >>>>> +  data_->data = thumbnail.data();
> > >>>>> +  data_->size = thumbnail.size();
> > >>>>> +}
> > >>>>> +
> > >>>>>   [[nodiscard]] int Exif::generate()
> > >>>>>   {
> > >>>>>     if (exifData_) {
> > >>>>> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h
> > >>>>> index 12c27b6..bd54a31 100644
> > >>>>> --- a/src/android/jpeg/exif.h
> > >>>>> +++ b/src/android/jpeg/exif.h
> > >>>>> @@ -26,6 +26,7 @@ public:
> > >>>>>
> > >>>>>     void setOrientation(int orientation);
> > >>>>>     void setSize(const libcamera::Size &size);
> > >>>>> +  void setThumbnail(std::vector<unsigned char> &thumbnail);
> > >>>> You can pass a Span<const unsigned char> as the setThumbnail() function
> > >>>> shouldn't care what storage container is used.
> > >>>>
> > >>>> It's a bit of a dangerous API, as it will store the pointer internally,
> > >>>> without ensuring that the caller keeps the thumbnail valid after the
> > >>>> call returns. It's fine, but maybe a comment above the
> > >>>> Exif::setThumbnail() functions to state that the thumbnail must remain
> > >>>> valid until the Exif object is destroyed would be a good thing.
> > >>> I think the comment will help indeed. The only alternative would be to
> > >>> pass it into generate(), but and refactor generate() to return the span
> > >>> ... but that's more effort that we need right now. So just a comment
> > >>> will do ;-)
> > >>>
> > >>>
> > >>>
> > >>>>>     void setTimestamp(time_t timestamp);
> > >>>>>
> > >>>>>     libcamera::Span<const uint8_t> data() const { return { exifData_, size_ }; }
> > >>>> I would have moved this to a separate patch.
> > >>> Yes, I'd keep the exif extension for setting the thumbnail, and the
> > >>> usage separate.
> > >>>
> > >>>>> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
> > >>>>> index c56f1b2..416e831 100644
> > >>>>> --- a/src/android/jpeg/post_processor_jpeg.cpp
> > >>>>> +++ b/src/android/jpeg/post_processor_jpeg.cpp
> > >>>>> @@ -9,7 +9,6 @@
> > >>>>>
> > >>>>>   #include "../camera_device.h"
> > >>>>>   #include "../camera_metadata.h"
> > >>>>> -#include "encoder_libjpeg.h"
> > >>>>>   #include "exif.h"
> > >>>>>
> > >>>>>   #include <libcamera/formats.h>
> > >>>>> @@ -39,11 +38,42 @@ int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,
> > >>>>>     }
> > >>>>>
> > >>>>>     streamSize_ = outCfg.size;
> > >>>>> +
> > >>>>> +  thumbnailer_.configure(inCfg.size, inCfg.pixelFormat);
> > >>>>> +  StreamConfiguration thCfg = inCfg;
> > >>>>> +  thCfg.size = thumbnailer_.size();
> > >>>>> +  if (thumbnailEncoder_.configure(thCfg) != 0) {
> > >>>>> +          LOG(JPEG, Error) << "Failed to configure thumbnail encoder";
> > >>>>> +          return -EINVAL;
> > >>>>> +  }
> > >>>>> +
> > >>>>>     encoder_ = std::make_unique<EncoderLibJpeg>();
> > >>>>>
> > >>>>>     return encoder_->configure(inCfg);
> > >>>>>   }
> > >>>>>
> > >>>>> +void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,
> > >>>>> +                                    std::vector<unsigned char> &thumbnail)
> > > std::vector<unsigned char>* thumbnail
> > > We would rather use a pointer than reference if a function changes it.
> > > Same for other places.
> > Ack.
> > >>>>> +{
> > >>>>> +  /* Stores the raw scaled-down thumbnail bytes. */
> > >>>>> +  std::vector<unsigned char> rawThumbnail;
> > >>>>> +
> > >>>>> +  thumbnailer_.scaleBuffer(source, rawThumbnail);
> > >>>>> +
> > >>>>> +  if (rawThumbnail.data()) {
> > >>>> This should check for ! .empty() (see additional comments below).
> > >>>>
> > >>>>> +          thumbnail.reserve(rawThumbnail.capacity());
> > >>>> You should call thumbnail.resize(), and use size() instead of capacity()
> > >>>> below, as reserve() allocates memory but doesn't change the size of the
> > >>>> vector, so it's semantically dangerous to write to the reserved storage
> > >>>> space if it hasn't been marked as in use with .resize().
> > >>>>
> > >>>>> +
> > >>>>> +          int jpeg_size = thumbnailEncoder_.encode(
> > >>>>> +                          { rawThumbnail.data(), rawThumbnail.capacity() },
> > >>>>> +                          { thumbnail.data(), thumbnail.capacity() },
> > >>>> Just pass rawThumbnail and thumbnail, there's an implicit constructor
> > >>>> for Span that will turn them into a span using .data() and .size().
> > >>>>
> > >>>>> +                          { });
> > >>>>> +          thumbnail.resize(jpeg_size);
> > >>>>> +
> > >>>>> +          LOG(JPEG, Info) << "Thumbnail compress returned "
> > >>>>> +                          << jpeg_size << " bytes";
> > >>>> When log messages don't fit on one line, we usually wrap them as
> > >>>>
> > >>>>              LOG(JPEG, Info)
> > >>>>                      << "Thumbnail compress returned "
> > >>>>                      << jpeg_size << " bytes";
> > >>>>
> > >>>> And maybe debug instead of info ?
> > >>>>
> > >>>>> +  }
> > >>>>> +}
> > > Shall this function handle a failure of jpeg encode?
> > Ideally it should but I am wondering in what style. I would prefer that
> > checking for the (thumbnail data != 0) before calling
> > Exif::setThumbnail() below. Since, when the encoding fails, jpeg_size
> > will get 0, and the thumbnail vector(placeholder for our thumbnail data)
> > will get resized to '0'.
> 
> Ack.
> 
> > >>>>> +
> > >>>>>   int PostProcessorJpeg::process(const FrameBuffer &source,
> > >>>>>                            Span<uint8_t> destination,
> > >>>>>                            CameraMetadata *metadata)
> > >>>>> @@ -64,6 +94,9 @@ int PostProcessorJpeg::process(const FrameBuffer &source,
> > >>>>>      * second, it is good enough.
> > >>>>>      */
> > >>>>>     exif.setTimestamp(std::time(nullptr));
> > >>>>> +  std::vector<unsigned char> thumbnail;
> > >>>>> +  generateThumbnail(source, thumbnail);
> > >>>>> +  exif.setThumbnail(thumbnail);
> > >>>>>     if (exif.generate() != 0)
> > >>>>>             LOG(JPEG, Error) << "Failed to generate valid EXIF data";
> > >>>>>
> > >>>>> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h
> > >>>>> index 3706cec..3894231 100644
> > >>>>> --- a/src/android/jpeg/post_processor_jpeg.h
> > >>>>> +++ b/src/android/jpeg/post_processor_jpeg.h
> > >>>>> @@ -8,12 +8,13 @@
> > >>>>>   #define __ANDROID_POST_PROCESSOR_JPEG_H__
> > >>>>>
> > >>>>>   #include "../post_processor.h"
> > >>>>> +#include "encoder_libjpeg.h"
> > >>>>> +#include "thumbnailer.h"
> > >>>>>
> > >>>>>   #include <libcamera/geometry.h>
> > >>>>>
> > >>>>>   #include "libcamera/internal/buffer.h"
> > >>>>>
> > >>>>> -class Encoder;
> > >>>>>   class CameraDevice;
> > >>>>>
> > >>>>>   class PostProcessorJpeg : public PostProcessor
> > >>>>> @@ -28,9 +29,14 @@ public:
> > >>>>>                 CameraMetadata *metadata) override;
> > >>>>>
> > >>>>>   private:
> > >>>>> +  void generateThumbnail(const libcamera::FrameBuffer &source,
> > >>>>> +                         std::vector<unsigned char> &thumbnail);
> > >>>>> +
> > >>>>>     CameraDevice *const cameraDevice_;
> > >>>>>     std::unique_ptr<Encoder> encoder_;
> > >>>>>     libcamera::Size streamSize_;
> > >>>>> +  EncoderLibJpeg thumbnailEncoder_;
> > >>>> Could you store this in a std::unique_ptr<Encoder> instead ? The reason
> > >>>> is that we shouldn't hardcode usage of EncoderLibJpeg, in order to
> > >>>> support different encoders later. You won't need to include
> > >>>> encoder_libjpeg.h then, and can keep the Encoder forwared declaration.
> > >>> I think this was already a unique_ptr in a previous iteration, and I
> > >>> suggested moving it to directly store an instance of the libjpeg encoder.
> > >>>
> > >>> In this instance of encoding a thumbnail, when encoding a 160x160 image,
> > >>> I would be weary about the overhead of setting up a hardware encode, and
> > >>> I'd expect the preparation phases of that to be potentially more
> > >>> expensive than just a software encode. Particularly as this has just
> > >>> done a software rescale, so it would have to cache-flush, when it could
> > >>> just use the host-cpu with a hot-cache. (ok, so perhaps later it might
> > >>> use a different scaler too ... but then it's likely a different equation
> > >>> anyway)
> > >>>
> > >>> I have not measured that of course, as we don't yet have a hw-jpeg
> > >>> encode post-processor. It will be an interesting test in the future.
> > >>>
> > >>> But essentially, I think we're just as well leaving this as a single
> > >>> instance of libjpeg for thumbnails. I think I recall the CrOS HAL using
> > >>> libjpeg as well, but I haven't gone back to double-check.
> > >>
> > >> Fair enough, those are good points. I'm fine if the code is kept as-is
> > >> (I wouldn't mind a unique_ptr of course :-)). Umang, up to you.
> > >
> > > Although either is fine to me, I wonder why the encoder for the
> > > thumbnail is within the thumbnailer.
> >
> > It is not within the encoder. It is inside the PostProcessorJpeg
> 
> Sorry, I meant why ... is NOT within ....
> 
> > > Since the thumbnailer is currently being used for thumbnail, it seems
> > > to be reasonable to move the encoder to it.
> > > What do you think?
> >
> > If you mean moving the thumbnail-encoder to `class Thumbnailer` - hmm,
> > that could also work I suppose. However,  I might not be very
> > enthusiastic about it, as PostProcessorJpeg captures the entire flow at
> > one place : e.g. exif generation, creating a thumbnail, encoding it and
> > finally encoding the entire frame. I like this.
> >
> > If others support moving the thumbnail-encoder to Thumbnailer, that's
> > also fine! (Also, could be done on top, as an independent refactor)
> 
> I would like to hear others opinions here.
> I would move it to Thumbnailer, so that
> 1.) a reader can understand at a glance the encoder is used for
> generating thumbnails, and
> 2.) thumbnail is always meant JPEG here and then it makes more sense
> that thumbnailer outputs JPEG.
> 
> Especially regarding 2, will the thumbnail possibly be other formats?

Thumbnails can also be stored in uncompressed form (the only combination
that is not allowed is storing compressed thumbnails in a TIFF container
for an uncompressed image). We have no plan at this point to support
this though, but if we realize that the size gain from the JPEG
compression of the thumbnail is minimal compared to the extra CPU time
to encode it, we could consider dropping the compression.

> > >>>>> +  Thumbnailer thumbnailer_;
> > >>>>
> > >>>> This is good, as I don't expect different thumbnailer types.
> > >>>>
> > >>>>>   };
> > >>>>>
> > >>>>>   #endif /* __ANDROID_POST_PROCESSOR_JPEG_H__ */
> > >>>>> diff --git a/src/android/jpeg/thumbnailer.cpp b/src/android/jpeg/thumbnailer.cpp
> > >>>>> new file mode 100644
> > >>>>> index 0000000..f880ffb
> > >>>>> --- /dev/null
> > >>>>> +++ b/src/android/jpeg/thumbnailer.cpp
> > >>>>> @@ -0,0 +1,109 @@
> > >>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > >>>> Wrong license.
> > >>>>
> > >>>>> +/*
> > >>>>> + * Copyright (C) 2020, Google Inc.
> > >>>>> + *
> > >>>>> + * thumbnailer.cpp - Simple image thumbnailer
> > >>>>> + */
> > >>>>> +
> > >>>>> +#include "thumbnailer.h"
> > >>>>> +
> > >>>>> +#include <libcamera/formats.h>
> > >>>>> +
> > >>>>> +#include "libcamera/internal/file.h"
> > >>>> Why do you need this header ?
> > >>>>
> > >>>>> +#include "libcamera/internal/log.h"
> > >>>>> +
> > >>>>> +using namespace libcamera;
> > >>>>> +
> > >>>>> +LOG_DEFINE_CATEGORY(Thumbnailer)
> > >>>>> +
> > >>>>> +Thumbnailer::Thumbnailer() : valid_(false)
> > >>>>      : valid_(false)
> > >>>>
> > >>>>> +{
> > >>>>> +}
> > >>>>> +
> > >>>>> +void Thumbnailer::configure(const Size &sourceSize, PixelFormat pixelFormat)
> > >>>>> +{
> > >>>>> +  sourceSize_ = sourceSize;
> > >>>>> +  pixelFormat_ = pixelFormat;
> > >>>>> +
> > >>>>> +  if (pixelFormat_ != formats::NV12) {
> > >>>>> +          LOG (Thumbnailer, Error) << "Failed to configure: Pixel Format "
> > >>>>> +                              << pixelFormat_.toString() << " unsupported.";
> > >>>>              LOG (Thumbnailer, Error)
> > >>>>                      << "Failed to configure: Pixel Format "
> > >>>>                      << pixelFormat_.toString() << " unsupported.";
> > >>>>
> > >>>>> +          return;
> > >>>>> +  }
> > >>>>> +
> > >>>>> +  targetSize_ = computeThumbnailSize();
> > >>>>> +
> > >>>>> +  valid_ = true;
> > >>>>> +}
> > >>>>> +
> > >>>>> +/*
> > >>>>> + * The Exif specification recommends the width of the thumbnail to be a
> > >>>>> + * mutiple of 16 (section 4.8.1). Hence, compute the corresponding height
> > >>>> s/mutiple/multiple/
> > >>>>
> > >>>>> + * keeping the aspect ratio same as of the source.
> > >>>>> + */
> > >>>>> +Size Thumbnailer::computeThumbnailSize()
> > >>>>> +{
> > >>>>> +  unsigned int targetHeight;
> > >>>>> +  unsigned int targetWidth = 160;
> > > nit: constexpr unsigned int kTargetWidth = 160;
> > >
> > >>>>> +
> > >>>>> +  targetHeight = targetWidth * sourceSize_.height / sourceSize_.width;
> > >>>>> +
> > >>>>> +  if (targetHeight & 1)
> > >>>>> +          targetHeight++;
> > >>>>> +
> > >>>>> +  return Size(targetWidth, targetHeight);
> > >>>>> +}
> > >>>>> +
> > >>>>> +void
> > >>>>> +Thumbnailer::scaleBuffer(const FrameBuffer &source,
> > >>>>> +                   std::vector<unsigned char> &destination)
> > >>>>> +{
> > >>>>> +  MappedFrameBuffer frame(&source, PROT_READ);
> > >>>>> +  if (!frame.isValid()) {
> > >>>>> +          LOG(Thumbnailer, Error) << "Failed to map FrameBuffer : "
> > >>>>> +                           << strerror(frame.error());
> > >>>>              LOG(Thumbnailer, Error)
> > >>>>                      << "Failed to map FrameBuffer : "
> > >>>>                      << strerror(frame.error());
> > >>>>
> > >>>>> +          return;
> > >>>>> +  }
> > >>>>> +
> > >>>>> +  if (!valid_) {
> > >>>>> +          LOG(Thumbnailer, Error) << "config is unconfigured or invalid.";
> > >>>>> +          return;
> > >>>>> +  }
> > >>>>> +
> > >>>>> +  const unsigned int sw = sourceSize_.width;
> > >>>>> +  const unsigned int sh = sourceSize_.height;
> > >>>>> +  const unsigned int tw = targetSize_.width;
> > >>>>> +  const unsigned int th = targetSize_.height;
> > >>>>> +
> > >>>>> +  /* Image scaling block implementing nearest-neighbour algorithm. */
> > >>>>> +  unsigned char *src = static_cast<unsigned char *>(frame.maps()[0].data());
> > >>>>> +  unsigned char *src_c = src + sh * sw;
> > >>>>> +  unsigned char *src_cb, *src_cr;
> > >>>>> +  unsigned char *dst_y, *src_y;
> > >>>>> +
> > >>>>> +  size_t dstSize = (th * tw) + ((th / 2) * tw);
> > >>>>> +  destination.reserve(dstSize);
> > >>>> This should be
> > >>>>
> > >>>>      destination.resize(dstSize);
> > >>>>
> > >>>> as reserve() allocates memory but doesn't change the size of the vector.
> > >>>>
> > >>>>> +  unsigned char *dst = destination.data();
> > >>>>> +  unsigned char *dst_c = dst + th * tw;
> > >>>>> +
> > >>>>> +  for (unsigned int y = 0; y < th; y += 2) {
> > >>>>> +          unsigned int sourceY = (sh * y + th / 2) / th;
> > >>>>> +
> > >>>>> +          dst_y = dst + y * tw;
> > >>>>> +          src_y = src + sw * sourceY;
> > >>>>> +          src_cb = src_c + (sourceY / 2) * sw + 0;
> > >>>>> +          src_cr = src_c + (sourceY / 2) * sw + 1;
> > >>>>> +
> > >>>>> +          for (unsigned int x = 0; x < tw; x += 2) {
> > >>>>> +                  unsigned int sourceX = (sw * x + tw / 2) / tw;
> > >>>>> +
> > >>>>> +                  dst_y[x] = src_y[sourceX];
> > >>>>> +                  dst_y[tw + x] = src_y[sw + sourceX];
> > >>>>> +                  dst_y[x + 1] = src_y[sourceX + 1];
> > >>>>> +                  dst_y[tw + x + 1] = src_y[sw + sourceX + 1];
> > >>>>> +
> > >>>>> +                  dst_c[(y / 2) * tw + x + 0] = src_cb[(sourceX / 2) * 2];
> > >>>>> +                  dst_c[(y / 2) * tw + x + 1] = src_cr[(sourceX / 2) * 2];
> > >>>>> +          }
> > >>>>> +  }
> > >>>>> + }
> > >>>> Extra space.
> > >>>>
> > >>>>> diff --git a/src/android/jpeg/thumbnailer.h b/src/android/jpeg/thumbnailer.h
> > >>>>> new file mode 100644
> > >>>>> index 0000000..b769619
> > >>>>> --- /dev/null
> > >>>>> +++ b/src/android/jpeg/thumbnailer.h
> > >>>>> @@ -0,0 +1,37 @@
> > >>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > >>>> Wrong license.
> > >>>>
> > >>>>> +/*
> > >>>>> + * Copyright (C) 2020, Google Inc.
> > >>>>> + *
> > >>>>> + * thumbnailer.h - Simple image thumbnailer
> > >>>>> + */
> > >>>>> +#ifndef __ANDROID_JPEG_THUMBNAILER_H__
> > >>>>> +#define __ANDROID_JPEG_THUMBNAILER_H__
> > >>>>> +
> > >>>>> +#include <libcamera/geometry.h>
> > >>>>> +
> > >>>>> +#include "libcamera/internal/buffer.h"
> > >>>>> +#include "libcamera/internal/formats.h"
> > >>>>> +
> > >>>>> +class Thumbnailer
> > >>>>> +{
> > >>>>> +public:
> > >>>>> +  Thumbnailer();
> > >>>>> +
> > >>>>> +  void configure(const libcamera::Size &sourceSize,
> > >>>>> +                 libcamera::PixelFormat pixelFormat);
> > >>>>> +  void scaleBuffer(const libcamera::FrameBuffer &source,
> > >>>>> +                   std::vector<unsigned char> &dest);
> > >>>> How about naming this createThumbnail() or generateThumbnail() ? And the
> > >>>> function should be const.
> > >>>>
> > >>>>> +  libcamera::Size size() const { return targetSize_; }
> > >
> > > nit: to reduce unnecessary copy, let's return const reference.
> > > const libcamera::Size& computeThumbanilSize() const;
> > >
> > >>>>> +
> > >>>>> +private:
> > >>>>> +  libcamera::Size computeThumbnailSize();
> > >>>> This can be
> > >>>>
> > >>>>      libcamera::Size computeThumbnailSize() const;
> > >>>>
> > >>>>> +
> > >>>>> +  libcamera::PixelFormat pixelFormat_;
> > >>>>> +  libcamera::Size sourceSize_;
> > >>>>> +  libcamera::Size targetSize_;
> > >>>>> +
> > >>>>> +  bool valid_;
> > >>>>> +
> > >>>> No need for a blank line.
> > >>>>
> > >>>>> +};
> > >>>>> +
> > >>>>> +#endif /* __ANDROID_JPEG_THUMBNAILER_H__ */
> > >>>>> diff --git a/src/android/meson.build b/src/android/meson.build
> > >>>>> index f72376a..3d4d3be 100644
> > >>>>> --- a/src/android/meson.build
> > >>>>> +++ b/src/android/meson.build
> > >>>>> @@ -25,6 +25,7 @@ android_hal_sources = files([
> > >>>>>       'jpeg/encoder_libjpeg.cpp',
> > >>>>>       'jpeg/exif.cpp',
> > >>>>>       'jpeg/post_processor_jpeg.cpp',
> > >>>>> +    'jpeg/thumbnailer.cpp',
> > >>>>>   ])
> > >>>>>
> > >>>>>   android_camera_metadata_sources = files([

Hi Umang,

On Wed, Oct 28, 2020 at 02:10:46AM +0530, Umang Jain wrote:
> On 10/27/20 3:38 AM, Laurent Pinchart wrote:
> > On Mon, Oct 26, 2020 at 10:02:43PM +0000, Kieran Bingham wrote:
> >> On 26/10/2020 21:05, Laurent Pinchart wrote:
> >>> On Mon, Oct 26, 2020 at 07:31:34PM +0530, Umang Jain wrote:
> >>>> Add a basic image thumbnailer for NV12 frames being captured.
> >>>> It shall generate a thumbnail image to be embedded as a part of
> >>>> EXIF metadata of the frame. The output of the thumbnail will still
> >>>> be NV12.
> >>>>
> >>>> Signed-off-by: Umang Jain <email@uajain.com>
> >>>> ---
> >>>>   src/android/jpeg/exif.cpp                |  16 +++-
> >>>>   src/android/jpeg/exif.h                  |   1 +
> >>>>   src/android/jpeg/post_processor_jpeg.cpp |  35 +++++++-
> >>>>   src/android/jpeg/post_processor_jpeg.h   |   8 +-
> >>>>   src/android/jpeg/thumbnailer.cpp         | 109 +++++++++++++++++++++++
> >>>>   src/android/jpeg/thumbnailer.h           |  37 ++++++++
> >>>>   src/android/meson.build                  |   1 +
> >>>>   7 files changed, 204 insertions(+), 3 deletions(-)
> >>>>   create mode 100644 src/android/jpeg/thumbnailer.cpp
> >>>>   create mode 100644 src/android/jpeg/thumbnailer.h
> >>>>
> >>>> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp
> >>>> index d21534a..24197bd 100644
> >>>> --- a/src/android/jpeg/exif.cpp
> >>>> +++ b/src/android/jpeg/exif.cpp
> >>>> @@ -75,8 +75,16 @@ Exif::~Exif()
> >>>>   	if (exifData_)
> >>>>   		free(exifData_);
> >>>>   
> >>>> -	if (data_)
> >>>> +	if (data_) {
> >>>> +		/*
> >>>> +		 * Reset thumbnail data to avoid getting double-freed by
> >>>> +		 * libexif. It is owned by the caller (i.e. PostProcessorJpeg).
> >>>> +		 */
> >>>> +		data_->data = nullptr;
> >>>> +		data_->size = 0;
> >>>> +
> >>>>   		exif_data_unref(data_);
> >>>> +	}
> >>>>   
> >>>>   	if (mem_)
> >>>>   		exif_mem_unref(mem_);
> >>>> @@ -268,6 +276,12 @@ void Exif::setOrientation(int orientation)
> >>>>   	setShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, value);
> >>>>   }
> >>>>   
> >>>> +void Exif::setThumbnail(std::vector<unsigned char> &thumbnail)
> >>>> +{
> >>>> +	data_->data = thumbnail.data();
> >>>> +	data_->size = thumbnail.size();
> >>>> +}
> >>>> +
> >>>>   [[nodiscard]] int Exif::generate()
> >>>>   {
> >>>>   	if (exifData_) {
> >>>> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h
> >>>> index 12c27b6..bd54a31 100644
> >>>> --- a/src/android/jpeg/exif.h
> >>>> +++ b/src/android/jpeg/exif.h
> >>>> @@ -26,6 +26,7 @@ public:
> >>>>   
> >>>>   	void setOrientation(int orientation);
> >>>>   	void setSize(const libcamera::Size &size);
> >>>> +	void setThumbnail(std::vector<unsigned char> &thumbnail);
> >>> You can pass a Span<const unsigned char> as the setThumbnail() function
> >>> shouldn't care what storage container is used.
> >>>
> >>> It's a bit of a dangerous API, as it will store the pointer internally,
> >>> without ensuring that the caller keeps the thumbnail valid after the
> >>> call returns. It's fine, but maybe a comment above the
> >>> Exif::setThumbnail() functions to state that the thumbnail must remain
> >>> valid until the Exif object is destroyed would be a good thing.
> >> I think the comment will help indeed. The only alternative would be to
> >> pass it into generate(), but and refactor generate() to return the span
> >> ... but that's more effort that we need right now. So just a comment
> >> will do ;-)
> >>
> >>
> >>
> >>>>   	void setTimestamp(time_t timestamp);
> >>>>   
> >>>>   	libcamera::Span<const uint8_t> data() const { return { exifData_, size_ }; }
> >>> I would have moved this to a separate patch.
> >>
> >> Yes, I'd keep the exif extension for setting the thumbnail, and the
> >> usage separate.
> >>
> >>>> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
> >>>> index c56f1b2..416e831 100644
> >>>> --- a/src/android/jpeg/post_processor_jpeg.cpp
> >>>> +++ b/src/android/jpeg/post_processor_jpeg.cpp
> >>>> @@ -9,7 +9,6 @@
> >>>>   
> >>>>   #include "../camera_device.h"
> >>>>   #include "../camera_metadata.h"
> >>>> -#include "encoder_libjpeg.h"
> >>>>   #include "exif.h"
> >>>>   
> >>>>   #include <libcamera/formats.h>
> >>>> @@ -39,11 +38,42 @@ int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,
> >>>>   	}
> >>>>   
> >>>>   	streamSize_ = outCfg.size;
> >>>> +
> >>>> +	thumbnailer_.configure(inCfg.size, inCfg.pixelFormat);
> >>>> +	StreamConfiguration thCfg = inCfg;
> >>>> +	thCfg.size = thumbnailer_.size();
> >>>> +	if (thumbnailEncoder_.configure(thCfg) != 0) {
> >>>> +		LOG(JPEG, Error) << "Failed to configure thumbnail encoder";
> >>>> +		return -EINVAL;
> >>>> +	}
> >>>> +
> >>>>   	encoder_ = std::make_unique<EncoderLibJpeg>();
> >>>>   
> >>>>   	return encoder_->configure(inCfg);
> >>>>   }
> >>>>   
> >>>> +void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,
> >>>> +					  std::vector<unsigned char> &thumbnail)
> >>>> +{
> >>>> +	/* Stores the raw scaled-down thumbnail bytes. */
> >>>> +	std::vector<unsigned char> rawThumbnail;
> >>>> +
> >>>> +	thumbnailer_.scaleBuffer(source, rawThumbnail);
> >>>> +
> >>>> +	if (rawThumbnail.data()) {
> >>> This should check for ! .empty() (see additional comments below).
> >>>
> >>>> +		thumbnail.reserve(rawThumbnail.capacity());
> >>> You should call thumbnail.resize(), and use size() instead of capacity()
> >>> below, as reserve() allocates memory but doesn't change the size of the
> >>> vector, so it's semantically dangerous to write to the reserved storage
> >>> space if it hasn't been marked as in use with .resize().
> >>>
> >>>> +
> >>>> +		int jpeg_size = thumbnailEncoder_.encode(
> >>>> +				{ rawThumbnail.data(), rawThumbnail.capacity() },
> >>>> +				{ thumbnail.data(), thumbnail.capacity() },
> >>> Just pass rawThumbnail and thumbnail, there's an implicit constructor
> >>> for Span that will turn them into a span using .data() and .size().
> >>>
> >>>> +				{ });
> >>>> +		thumbnail.resize(jpeg_size);
> >>>> +
> >>>> +		LOG(JPEG, Info) << "Thumbnail compress returned "
> >>>> +				<< jpeg_size << " bytes";
> >>> When log messages don't fit on one line, we usually wrap them as
> >>>
> >>> 		LOG(JPEG, Info)
> >>> 			<< "Thumbnail compress returned "
> >>> 			<< jpeg_size << " bytes";
> >>>
> >>> And maybe debug instead of info ?
> >>>
> >>>> +	}
> >>>> +}
> >>>> +
> >>>>   int PostProcessorJpeg::process(const FrameBuffer &source,
> >>>>   			       Span<uint8_t> destination,
> >>>>   			       CameraMetadata *metadata)
> >>>> @@ -64,6 +94,9 @@ int PostProcessorJpeg::process(const FrameBuffer &source,
> >>>>   	 * second, it is good enough.
> >>>>   	 */
> >>>>   	exif.setTimestamp(std::time(nullptr));
> >>>> +	std::vector<unsigned char> thumbnail;
> >>>> +	generateThumbnail(source, thumbnail);
> >>>> +	exif.setThumbnail(thumbnail);
> >>>>   	if (exif.generate() != 0)
> >>>>   		LOG(JPEG, Error) << "Failed to generate valid EXIF data";
> >>>>   
> >>>> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h
> >>>> index 3706cec..3894231 100644
> >>>> --- a/src/android/jpeg/post_processor_jpeg.h
> >>>> +++ b/src/android/jpeg/post_processor_jpeg.h
> >>>> @@ -8,12 +8,13 @@
> >>>>   #define __ANDROID_POST_PROCESSOR_JPEG_H__
> >>>>   
> >>>>   #include "../post_processor.h"
> >>>> +#include "encoder_libjpeg.h"
> >>>> +#include "thumbnailer.h"
> >>>>   
> >>>>   #include <libcamera/geometry.h>
> >>>>   
> >>>>   #include "libcamera/internal/buffer.h"
> >>>>   
> >>>> -class Encoder;
> >>>>   class CameraDevice;
> >>>>   
> >>>>   class PostProcessorJpeg : public PostProcessor
> >>>> @@ -28,9 +29,14 @@ public:
> >>>>   		    CameraMetadata *metadata) override;
> >>>>   
> >>>>   private:
> >>>> +	void generateThumbnail(const libcamera::FrameBuffer &source,
> >>>> +			       std::vector<unsigned char> &thumbnail);
> >>>> +
> >>>>   	CameraDevice *const cameraDevice_;
> >>>>   	std::unique_ptr<Encoder> encoder_;
> >>>>   	libcamera::Size streamSize_;
> >>>> +	EncoderLibJpeg thumbnailEncoder_;
> >>> Could you store this in a std::unique_ptr<Encoder> instead ? The reason
> >>> is that we shouldn't hardcode usage of EncoderLibJpeg, in order to
> >>> support different encoders later. You won't need to include
> >>> encoder_libjpeg.h then, and can keep the Encoder forwared declaration.
> >> I think this was already a unique_ptr in a previous iteration, and I
> >> suggested moving it to directly store an instance of the libjpeg encoder.
> >>
> >> In this instance of encoding a thumbnail, when encoding a 160x160 image,
> >> I would be weary about the overhead of setting up a hardware encode, and
> >> I'd expect the preparation phases of that to be potentially more
> >> expensive than just a software encode. Particularly as this has just
> >> done a software rescale, so it would have to cache-flush, when it could
> >> just use the host-cpu with a hot-cache. (ok, so perhaps later it might
> >> use a different scaler too ... but then it's likely a different equation
> >> anyway)
> >>
> >> I have not measured that of course, as we don't yet have a hw-jpeg
> >> encode post-processor. It will be an interesting test in the future.
> >>
> >> But essentially, I think we're just as well leaving this as a single
> >> instance of libjpeg for thumbnails. I think I recall the CrOS HAL using
> >> libjpeg as well, but I haven't gone back to double-check.
> >
> > Fair enough, those are good points. I'm fine if the code is kept as-is
> > (I wouldn't mind a unique_ptr of course :-)). Umang, up to you.
>
> Do you specfically suggest using unique_ptr, to make use of forward 
> declaration, in this instance of EncoderLibJpeg(see a diff below)? I am 
> curious to ask, because in order to make it work with incomplete 
> types(i.e. forward declared), I need to insert destructor declaration in 
> post_processor_jpeg.h and define a (blank)destructor in 
> post_processor_jpeg.cpp

I was thinking of storing a unique pointer to Encoder, not
EncoderLibJpeg, even if we always instantiate a EncoderLibJpeg, but that
doesn't matter much.

> I learnt this and can see it in action. Ref: 
> https://stackoverflow.com/a/42158611
> 
> -----
> diff --git a/src/android/jpeg/post_processor_jpeg.h 
> b/src/android/jpeg/post_processor_jpeg.h
> index 3706cec..8b36a21 100644
> --- a/src/android/jpeg/post_processor_jpeg.h
> +++ b/src/android/jpeg/post_processor_jpeg.h
> @@ -8,12 +8,14 @@
>   #define __ANDROID_POST_PROCESSOR_JPEG_H__
> 
>   #include "../post_processor.h"
> +#include "thumbnailer.h"
> 
>   #include <libcamera/geometry.h>
> 
>   #include "libcamera/internal/buffer.h"
> 
>   class Encoder;
> +class EncoderLibJpeg;
>   class CameraDevice;
> 
>   class PostProcessorJpeg : public PostProcessor
> @@ -28,9 +30,14 @@ public:
>                      CameraMetadata *metadata) override;
> 
>   private:
> +       void generateThumbnail(const libcamera::FrameBuffer &source,
> +                              std::vector<unsigned char> *thumbnail);
> +
>          CameraDevice *const cameraDevice_;
>          std::unique_ptr<Encoder> encoder_;
>          libcamera::Size streamSize_;
> +       std::unique_ptr<EncoderLibJpeg> thumbnailEncoder_;
> +       Thumbnailer thumbnailer_;
>   };
> 
>
> >>>> +	Thumbnailer thumbnailer_;
> >>> This is good, as I don't expect different thumbnailer types.
> >>>
> >>>>   };
> >>>>   
> >>>>   #endif /* __ANDROID_POST_PROCESSOR_JPEG_H__ */
> >>>> diff --git a/src/android/jpeg/thumbnailer.cpp b/src/android/jpeg/thumbnailer.cpp
> >>>> new file mode 100644
> >>>> index 0000000..f880ffb
> >>>> --- /dev/null
> >>>> +++ b/src/android/jpeg/thumbnailer.cpp
> >>>> @@ -0,0 +1,109 @@
> >>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> >>> Wrong license.
> >>>
> >>>> +/*
> >>>> + * Copyright (C) 2020, Google Inc.
> >>>> + *
> >>>> + * thumbnailer.cpp - Simple image thumbnailer
> >>>> + */
> >>>> +
> >>>> +#include "thumbnailer.h"
> >>>> +
> >>>> +#include <libcamera/formats.h>
> >>>> +
> >>>> +#include "libcamera/internal/file.h"
> >>> Why do you need this header ?
> >>>
> >>>> +#include "libcamera/internal/log.h"
> >>>> +
> >>>> +using namespace libcamera;
> >>>> +
> >>>> +LOG_DEFINE_CATEGORY(Thumbnailer)
> >>>> +
> >>>> +Thumbnailer::Thumbnailer() : valid_(false)
> >>> 	: valid_(false)
> >>>
> >>>> +{
> >>>> +}
> >>>> +
> >>>> +void Thumbnailer::configure(const Size &sourceSize, PixelFormat pixelFormat)
> >>>> +{
> >>>> +	sourceSize_ = sourceSize;
> >>>> +	pixelFormat_ = pixelFormat;
> >>>> +
> >>>> +	if (pixelFormat_ != formats::NV12) {
> >>>> +		LOG (Thumbnailer, Error) << "Failed to configure: Pixel Format "
> >>>> +				    << pixelFormat_.toString() << " unsupported.";
> >>> 		LOG (Thumbnailer, Error)
> >>> 			<< "Failed to configure: Pixel Format "
> >>> 			<< pixelFormat_.toString() << " unsupported.";
> >>>
> >>>> +		return;
> >>>> +	}
> >>>> +
> >>>> +	targetSize_ = computeThumbnailSize();
> >>>> +
> >>>> +	valid_ = true;
> >>>> +}
> >>>> +
> >>>> +/*
> >>>> + * The Exif specification recommends the width of the thumbnail to be a
> >>>> + * mutiple of 16 (section 4.8.1). Hence, compute the corresponding height
> >>> s/mutiple/multiple/
> >>>
> >>>> + * keeping the aspect ratio same as of the source.
> >>>> + */
> >>>> +Size Thumbnailer::computeThumbnailSize()
> >>>> +{
> >>>> +	unsigned int targetHeight;
> >>>> +	unsigned int targetWidth = 160;
> >>>> +
> >>>> +	targetHeight = targetWidth * sourceSize_.height / sourceSize_.width;
> >>>> +
> >>>> +	if (targetHeight & 1)
> >>>> +		targetHeight++;
> >>>> +
> >>>> +	return Size(targetWidth, targetHeight);
> >>>> +}
> >>>> +
> >>>> +void
> >>>> +Thumbnailer::scaleBuffer(const FrameBuffer &source,
> >>>> +			 std::vector<unsigned char> &destination)
> >>>> +{
> >>>> +	MappedFrameBuffer frame(&source, PROT_READ);
> >>>> +	if (!frame.isValid()) {
> >>>> +		LOG(Thumbnailer, Error) << "Failed to map FrameBuffer : "
> >>>> +				 << strerror(frame.error());
> >>> 		LOG(Thumbnailer, Error)
> >>> 			<< "Failed to map FrameBuffer : "
> >>> 			<< strerror(frame.error());
> >>>
> >>>> +		return;
> >>>> +	}
> >>>> +
> >>>> +	if (!valid_) {
> >>>> +		LOG(Thumbnailer, Error) << "config is unconfigured or invalid.";
> >>>> +		return;
> >>>> +	}
> >>>> +
> >>>> +	const unsigned int sw = sourceSize_.width;
> >>>> +	const unsigned int sh = sourceSize_.height;
> >>>> +	const unsigned int tw = targetSize_.width;
> >>>> +	const unsigned int th = targetSize_.height;
> >>>> +
> >>>> +	/* Image scaling block implementing nearest-neighbour algorithm. */
> >>>> +	unsigned char *src = static_cast<unsigned char *>(frame.maps()[0].data());
> >>>> +	unsigned char *src_c = src + sh * sw;
> >>>> +	unsigned char *src_cb, *src_cr;
> >>>> +	unsigned char *dst_y, *src_y;
> >>>> +
> >>>> +	size_t dstSize = (th * tw) + ((th / 2) * tw);
> >>>> +	destination.reserve(dstSize);
> >>> This should be
> >>>
> >>> 	destination.resize(dstSize);
> >>>
> >>> as reserve() allocates memory but doesn't change the size of the vector.
> >>>
> >>>> +	unsigned char *dst = destination.data();
> >>>> +	unsigned char *dst_c = dst + th * tw;
> >>>> +
> >>>> +	for (unsigned int y = 0; y < th; y += 2) {
> >>>> +		unsigned int sourceY = (sh * y + th / 2) / th;
> >>>> +
> >>>> +		dst_y = dst + y * tw;
> >>>> +		src_y = src + sw * sourceY;
> >>>> +		src_cb = src_c + (sourceY / 2) * sw + 0;
> >>>> +		src_cr = src_c + (sourceY / 2) * sw + 1;
> >>>> +
> >>>> +		for (unsigned int x = 0; x < tw; x += 2) {
> >>>> +			unsigned int sourceX = (sw * x + tw / 2) / tw;
> >>>> +
> >>>> +			dst_y[x] = src_y[sourceX];
> >>>> +			dst_y[tw + x] = src_y[sw + sourceX];
> >>>> +			dst_y[x + 1] = src_y[sourceX + 1];
> >>>> +			dst_y[tw + x + 1] = src_y[sw + sourceX + 1];
> >>>> +
> >>>> +			dst_c[(y / 2) * tw + x + 0] = src_cb[(sourceX / 2) * 2];
> >>>> +			dst_c[(y / 2) * tw + x + 1] = src_cr[(sourceX / 2) * 2];
> >>>> +		}
> >>>> +	}
> >>>> + }
> >>> Extra space.
> >>>
> >>>> diff --git a/src/android/jpeg/thumbnailer.h b/src/android/jpeg/thumbnailer.h
> >>>> new file mode 100644
> >>>> index 0000000..b769619
> >>>> --- /dev/null
> >>>> +++ b/src/android/jpeg/thumbnailer.h
> >>>> @@ -0,0 +1,37 @@
> >>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> >>> Wrong license.
> >>>
> >>>> +/*
> >>>> + * Copyright (C) 2020, Google Inc.
> >>>> + *
> >>>> + * thumbnailer.h - Simple image thumbnailer
> >>>> + */
> >>>> +#ifndef __ANDROID_JPEG_THUMBNAILER_H__
> >>>> +#define __ANDROID_JPEG_THUMBNAILER_H__
> >>>> +
> >>>> +#include <libcamera/geometry.h>
> >>>> +
> >>>> +#include "libcamera/internal/buffer.h"
> >>>> +#include "libcamera/internal/formats.h"
> >>>> +
> >>>> +class Thumbnailer
> >>>> +{
> >>>> +public:
> >>>> +	Thumbnailer();
> >>>> +
> >>>> +	void configure(const libcamera::Size &sourceSize,
> >>>> +		       libcamera::PixelFormat pixelFormat);
> >>>> +	void scaleBuffer(const libcamera::FrameBuffer &source,
> >>>> +			 std::vector<unsigned char> &dest);
> >>> How about naming this createThumbnail() or generateThumbnail() ? And the
> >>> function should be const.
> >>>
> >>>> +	libcamera::Size size() const { return targetSize_; }
> >>>> +
> >>>> +private:
> >>>> +	libcamera::Size computeThumbnailSize();
> >>> This can be
> >>>
> >>> 	libcamera::Size computeThumbnailSize() const;
> >>>
> >>>> +
> >>>> +	libcamera::PixelFormat pixelFormat_;
> >>>> +	libcamera::Size sourceSize_;
> >>>> +	libcamera::Size targetSize_;
> >>>> +
> >>>> +	bool valid_;
> >>>> +
> >>> No need for a blank line.
> >>>
> >>>> +};
> >>>> +
> >>>> +#endif /* __ANDROID_JPEG_THUMBNAILER_H__ */
> >>>> diff --git a/src/android/meson.build b/src/android/meson.build
> >>>> index f72376a..3d4d3be 100644
> >>>> --- a/src/android/meson.build
> >>>> +++ b/src/android/meson.build
> >>>> @@ -25,6 +25,7 @@ android_hal_sources = files([
> >>>>       'jpeg/encoder_libjpeg.cpp',
> >>>>       'jpeg/exif.cpp',
> >>>>       'jpeg/post_processor_jpeg.cpp',
> >>>> +    'jpeg/thumbnailer.cpp',
> >>>>   ])
> >>>>   
> >>>>   android_camera_metadata_sources = files([
>