[libcamera-devel,1/1] android: jpeg: Add a basic NV12 image scaler

9683 diff mbox series

Add a basic image scaler for NV12 frames being captured. The primary
use of this scaler will be to generate a thumbnail image to be embedded
as a part of EXIF metadata of the frame.

Signed-off-by: Umang Jain <email@uajain.com>
---
 src/android/jpeg/encoder_libjpeg.cpp |  10 ++
 src/android/jpeg/scaler.cpp          | 137 +++++++++++++++++++++++++++
 src/android/jpeg/scaler.h            |  32 +++++++
 src/android/meson.build              |   1 +
 4 files changed, 180 insertions(+)
 create mode 100644 src/android/jpeg/scaler.cpp
 create mode 100644 src/android/jpeg/scaler.h

Hi Umang,

On 22/09/2020 09:54, Umang Jain wrote:
> Add a basic image scaler for NV12 frames being captured. The primary
> use of this scaler will be to generate a thumbnail image to be embedded
> as a part of EXIF metadata of the frame.
> 
> Signed-off-by: Umang Jain <email@uajain.com>
> ---
>  src/android/jpeg/encoder_libjpeg.cpp |  10 ++
>  src/android/jpeg/scaler.cpp          | 137 +++++++++++++++++++++++++++
>  src/android/jpeg/scaler.h            |  32 +++++++
>  src/android/meson.build              |   1 +
>  4 files changed, 180 insertions(+)
>  create mode 100644 src/android/jpeg/scaler.cpp
>  create mode 100644 src/android/jpeg/scaler.h
> 
> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp
> index 510613c..9ecf9b1 100644
> --- a/src/android/jpeg/encoder_libjpeg.cpp
> +++ b/src/android/jpeg/encoder_libjpeg.cpp
> @@ -6,6 +6,7 @@
>   */
>  
>  #include "encoder_libjpeg.h"
> +#include "scaler.h"
>  
>  #include <fcntl.h>
>  #include <iomanip>
> @@ -214,6 +215,15 @@ int EncoderLibJpeg::encode(const FrameBuffer *source,
>  	LOG(JPEG, Debug) << "JPEG Encode Starting:" << compress_.image_width
>  			 << "x" << compress_.image_height;
>  
> +	Scaler scaler;
> +	libcamera::Span<uint8_t> thumbnail;
> +	scaler.configure(Size (compress_.image_width, compress_.image_height),
> +			 /* \todo: Check for exact thumbnail size required by EXIF spec. */
> +		         Size (compress_.image_width / 3, compress_.image_height / 3),


Indeed, I think this should set explicit sizes, not simply reduce to a
third.

http://www.fifi.org/doc/jhead/exif-e.html#ExifThumbs

States, that usually the thumbnails are 160x120 for Exif2.1 or later,
and are usually encoded as JPG we can use another JPEG compressor
instance most likely, but if the RGB format works, that's fine too for now.

I'd hard code the size to 160 x 120 all the same.

> +			 pixelFormatInfo_);
> +	scaler.scaleBuffer(source, thumbnail);
> +	/* \todo: Write thumbnail as part of exifData. */
> +
>  	if (nv_)
>  		compressNV(&frame);
>  	else
> diff --git a/src/android/jpeg/scaler.cpp b/src/android/jpeg/scaler.cpp
> new file mode 100644
> index 0000000..ff36ece
> --- /dev/null
> +++ b/src/android/jpeg/scaler.cpp
> @@ -0,0 +1,137 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * scaler.cpp - Basic image scaler from NV12 to RGB24 format
> + */
> +
> +#include "scaler.h"
> +
> +#include <math.h>
> +#include <string.h>
> +
> +#include "libcamera/internal/log.h"
> +#include "libcamera/internal/file.h"
> +
> +#define RGBSHIFT                8
> +#ifndef MAX
> +#define MAX(a,b)                ((a)>(b)?(a):(b))
> +#endif
> +#ifndef MIN
> +#define MIN(a,b)                ((a)<(b)?(a):(b))
> +#endif

#include <algorithm>
will give you std::min, std::max, and std::clamp()

> +#ifndef CLAMP
> +#define CLAMP(a,low,high)       MAX((low),MIN((high),(a)))
> +#endif


> +#ifndef CLIP
> +#define CLIP(x)                 CLAMP(x,0,255)
> +#endif

I think this one is distinct though, Depends on how it's utilised to see
whether it's worth just using std::clamp directly.


> +
> +using namespace libcamera;
> +
> +LOG_DEFINE_CATEGORY(SCALER)
> +
> +Scaler::Scaler()
> +	: sourceSize_(0, 0), targetSize_(0,0)
> +{
> +}
> +
> +Scaler::~Scaler()
> +{
> +}
> +
> +void Scaler::configure(Size sourceSize, Size targetSize, const PixelFormatInfo *pixelFormatInfo)
> +{
> +	sourceSize_ = sourceSize;
> +	targetSize_ = targetSize;
> +	pixelFormatInfo_ = pixelFormatInfo;
> +}
> +
> +static std::string datetime()
> +{
> +	time_t rawtime;
> +	struct tm *timeinfo;
> +	char buffer[80];
> +	static unsigned int milliseconds = 0;
> +
> +	time(&rawtime);
> +	timeinfo = localtime(&rawtime);
> +
> +	strftime(buffer, 80, "%d-%m-%Y.%H-%M-%S.", timeinfo);
> +
> +	/* milliseconds is just a fast hack to ensure unique filenames */
> +	return std::string(buffer) + std::to_string(milliseconds++);
> +}

I think this function is here for debug, but shouldn't be in the scaler.

> +
> +/* Handpicked from src/qcam/format_converter.cpp */
> +static void yuv_to_rgb(int y, int u, int v, int *r, int *g, int *b)
> +{
> +	int c = y - 16;
> +	int d = u - 128;
> +	int e = v - 128;
> +	*r = CLIP(( 298 * c           + 409 * e + 128) >> RGBSHIFT);
> +	*g = CLIP(( 298 * c - 100 * d - 208 * e + 128) >> RGBSHIFT);
> +	*b = CLIP(( 298 * c + 516 * d           + 128) >> RGBSHIFT);
> +}

CLIP does look better than std::clamp there.

As RGBSHIFT is only used here, maybe put it as a constexpr in this function?


But I wonder if we should really be making the scaler do pixel-format
conversions.

In the case of the Thumbnail generations it's a nice optimisztion, but
it doesn't feel right, and leads to the object being an anything to
anything scaler/convertor.

As that is possible in some hardware (scale and pixel conversion
combined) I'm almost tempted to say it's ok ... but I'm just not sure.

The alternative would be to take the incoming image, rescale it to the
thumbnail, and then run that through another JPEG compressor anyway,
which already supports NV12.


> +
> +int Scaler::scaleBuffer(const FrameBuffer *source, Span<uint8_t> &dest)
> +{
> +	MappedFrameBuffer frame(source, PROT_READ);
> +	if (!frame.isValid()) {
> +		LOG(SCALER, Error) << "Failed to map FrameBuffer : "
> +				   << strerror(frame.error());
> +		return frame.error();
> +	}
> +
> +	if (strcmp(pixelFormatInfo_->name, "NV12") != 0) {

Can you compare against formats::NV12 directly?

 	if (pixelFormatInfo_->format != formats::NV12) {

> +		LOG (SCALER, Info) << "Source Buffer not in NV12 format, returning...";
> +		return -1;

This should be in configure() though, not on every call to scaleBuffer().

> +	}
> +
> +	/* Image scaling block implementing nearest-neighbour algorithm. */
> +	{

I'd remove the scoping here and indent left - or if you want to keep it
distinct, put it into a function called

  scaleNearestNeighbour(MappedFrameBuffer source*, Span<uint8_t> &dest)

That would leave scale() only doing the mapping ... but make the code
paths clear if another scaler implementation was added later...?


> +		unsigned int cb_pos = 0;
> +		unsigned int cr_pos = 1;
> +		unsigned char *src = static_cast<unsigned char *>(frame.maps()[0].data());
> +		unsigned char *src_c = src + sourceSize_.height * sourceSize_.width;
> +		unsigned int stride = sourceSize_.width;
> +		unsigned char *src_y, *src_cb, *src_cr;
> +
> +		unsigned int bpp = 3;
> +		size_t dstSize = targetSize_.height * targetSize_.width * bpp;
> +		unsigned char *dst = static_cast<unsigned char *>(malloc(dstSize));

Hrm ... so the caller would have to know to free the span it receives.
That's not nice as you normally expect a Span to by just a pointer into
some space, that you don't have control over.

I think the caller should probably allocate and provide the destination
memory.


> +		unsigned char *destination = dst;
> +		int r, g, b;
> +
> +		for (unsigned int y = 0; y < targetSize_.height; y++) {
> +			int32_t sourceY = lround(sourceSize_.height * y / targetSize_.height);
> +
> +			for (unsigned int x = 0; x < targetSize_.width; x++) {
> +				int32_t sourceX = lround(sourceSize_.width * x / targetSize_.width);
> +
> +				src_y = src + stride * sourceY + sourceX;
> +				src_cb = src_c + (sourceY / 2) * stride + (sourceX / 2) * 2 + cb_pos;
> +				src_cr = src_c + (sourceY / 2) * stride + (sourceX / 2) * 2 + cr_pos;
> +
> +				yuv_to_rgb(*src_y, *src_cb, *src_cr, &r, &g, &b);
> +
> +				destination[x * bpp + 0] = r;
> +				destination[x * bpp + 1] = g;
> +				destination[x * bpp + 2] = b;
> +			}
> +
> +			destination = destination + bpp * targetSize_.width;
> +		}
> +
> +		/* Helper code: Write the output pixels to a file so we can inspect */
> +		File file("/tmp/" + datetime() + ".raw");
> +		int32_t ret = file.open(File::WriteOnly);
> +		ret = file.write({ dst, dstSize });
> +		LOG(SCALER, Info) << "Wrote " << ret << " bytes: " << targetSize_.width << "x" << targetSize_.height;
> +

We should keep debug to a separate patch.

It can be provided in the series with a DNI prefix if it's helpful
during review/development - but try to keep the patch targeted for
integration clean.

> +		/* Write scaled pixels to dest */
> +		dest = { dst, dstSize };

If the caller provides the memory, we shouldn't need to do this, as we
should be dealing with fixed size buffers, so the dest should be already
correctly sized.



> +	}
> +
> +	return 0;
> +}
> diff --git a/src/android/jpeg/scaler.h b/src/android/jpeg/scaler.h
> new file mode 100644
> index 0000000..c68a279
> --- /dev/null
> +++ b/src/android/jpeg/scaler.h
> @@ -0,0 +1,32 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * Basic image scaler from NV12 to RGB24 format

That's a scaler and pixelformat convertor ;-)

> + */
> +#ifndef __ANDROID_JPEG_SCALER_H__
> +#define __ANDROID_JPEG_SCALER_H__
> +
> +#include <libcamera/geometry.h>
> +
> +#include "libcamera/internal/buffer.h"
> +#include "libcamera/internal/formats.h"
> +
> +class Scaler
> +{
> +public:
> +	Scaler();
> +	~Scaler();
> +
> +	void configure(libcamera::Size sourceSize, libcamera::Size targetSize,
> +		       const libcamera::PixelFormatInfo *pixelFormatInfo);
> +	int scaleBuffer(const libcamera::FrameBuffer *source, libcamera::Span<uint8_t> &dest);
> +
> +private:
> +
> +	libcamera::Size sourceSize_;
> +	libcamera::Size targetSize_;
> +	const libcamera::PixelFormatInfo *pixelFormatInfo_;
> +};
> +
> +#endif /* __ANDROID_JPEG_SCALER_H__ */
> diff --git a/src/android/meson.build b/src/android/meson.build
> index 0293c20..aefb0da 100644
> --- a/src/android/meson.build
> +++ b/src/android/meson.build
> @@ -22,6 +22,7 @@ android_hal_sources = files([
>      'camera_ops.cpp',
>      'jpeg/encoder_libjpeg.cpp',
>      'jpeg/exif.cpp',
> +    'jpeg/scaler.cpp',
>  ])
>  
>  android_camera_metadata_sources = files([
>

Hi Kieran

Thanks for the review

On 9/25/20 8:33 PM, Kieran Bingham wrote:
> Hi Umang,
>
> On 22/09/2020 09:54, Umang Jain wrote:
>> Add a basic image scaler for NV12 frames being captured. The primary
>> use of this scaler will be to generate a thumbnail image to be embedded
>> as a part of EXIF metadata of the frame.
>>
>> Signed-off-by: Umang Jain <email@uajain.com>
>> ---
>>   src/android/jpeg/encoder_libjpeg.cpp |  10 ++
>>   src/android/jpeg/scaler.cpp          | 137 +++++++++++++++++++++++++++
>>   src/android/jpeg/scaler.h            |  32 +++++++
>>   src/android/meson.build              |   1 +
>>   4 files changed, 180 insertions(+)
>>   create mode 100644 src/android/jpeg/scaler.cpp
>>   create mode 100644 src/android/jpeg/scaler.h
>>
>> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp
>> index 510613c..9ecf9b1 100644
>> --- a/src/android/jpeg/encoder_libjpeg.cpp
>> +++ b/src/android/jpeg/encoder_libjpeg.cpp
>> @@ -6,6 +6,7 @@
>>    */
>>   
>>   #include "encoder_libjpeg.h"
>> +#include "scaler.h"
>>   
>>   #include <fcntl.h>
>>   #include <iomanip>
>> @@ -214,6 +215,15 @@ int EncoderLibJpeg::encode(const FrameBuffer *source,
>>   	LOG(JPEG, Debug) << "JPEG Encode Starting:" << compress_.image_width
>>   			 << "x" << compress_.image_height;
>>   
>> +	Scaler scaler;
>> +	libcamera::Span<uint8_t> thumbnail;
>> +	scaler.configure(Size (compress_.image_width, compress_.image_height),
>> +			 /* \todo: Check for exact thumbnail size required by EXIF spec. */
>> +		         Size (compress_.image_width / 3, compress_.image_height / 3),
>
> Indeed, I think this should set explicit sizes, not simply reduce to a
> third.
>
> http://www.fifi.org/doc/jhead/exif-e.html#ExifThumbs
>
> States, that usually the thumbnails are 160x120 for Exif2.1 or later,
> and are usually encoded as JPG we can use another JPEG compressor
> instance most likely, but if the RGB format works, that's fine too for now.
The EXIF standard states that:
```
No limit is placed on the size of thumbnail images. It is optional to 
record thumbnails but it is recommended that they be recorded if 
possible, unless hardware or other restrictions preclude this.Thumbnail 
data does not necessarily have to adopt the same data structure as that 
used for primary images. If, however, the primary images are recorded as 
uncompressed RGB data or as uncompressed YCbCr data, thumbnail images 
shall not be recorded as JPEG compressed data.

When thumbnails are recorded in uncompressed format, they are to be 
recorded in the 1st IFD in conformance with Baseline TIFF Rev. 6.0 RGB 
Full Color Images or TIFF Rev. 6.0 Extensions YCbCr Images.
```
(Page 24 - Exif Version 2.31 spec)

So it seems we can write uncompressed data for thumbnail in RGB888, 
right? It also states YCbCr  too (for uncompressed format) , so maybe we 
want that? That would take out the 'converter' part of this patch.
I am not really sure which format to use, so I will move forward as per 
the advice given in the reviews...
>
> I'd hard code the size to 160 x 120 all the same.
Yeah, that would come along in a proper patch.
>
>> +			 pixelFormatInfo_);
>> +	scaler.scaleBuffer(source, thumbnail);
>> +	/* \todo: Write thumbnail as part of exifData. */
>> +
>>   	if (nv_)
>>   		compressNV(&frame);
>>   	else
>> diff --git a/src/android/jpeg/scaler.cpp b/src/android/jpeg/scaler.cpp
>> new file mode 100644
>> index 0000000..ff36ece
>> --- /dev/null
>> +++ b/src/android/jpeg/scaler.cpp
>> @@ -0,0 +1,137 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Copyright (C) 2020, Google Inc.
>> + *
>> + * scaler.cpp - Basic image scaler from NV12 to RGB24 format
>> + */
>> +
>> +#include "scaler.h"
>> +
>> +#include <math.h>
>> +#include <string.h>
>> +
>> +#include "libcamera/internal/log.h"
>> +#include "libcamera/internal/file.h"
>> +
>> +#define RGBSHIFT                8
>> +#ifndef MAX
>> +#define MAX(a,b)                ((a)>(b)?(a):(b))
>> +#endif
>> +#ifndef MIN
>> +#define MIN(a,b)                ((a)<(b)?(a):(b))
>> +#endif
> #include <algorithm>
> will give you std::min, std::max, and std::clamp()
>
>> +#ifndef CLAMP
>> +#define CLAMP(a,low,high)       MAX((low),MIN((high),(a)))
>> +#endif
>
>> +#ifndef CLIP
>> +#define CLIP(x)                 CLAMP(x,0,255)
>> +#endif
> I think this one is distinct though, Depends on how it's utilised to see
> whether it's worth just using std::clamp directly.
>
>
>> +
>> +using namespace libcamera;
>> +
>> +LOG_DEFINE_CATEGORY(SCALER)
>> +
>> +Scaler::Scaler()
>> +	: sourceSize_(0, 0), targetSize_(0,0)
>> +{
>> +}
>> +
>> +Scaler::~Scaler()
>> +{
>> +}
>> +
>> +void Scaler::configure(Size sourceSize, Size targetSize, const PixelFormatInfo *pixelFormatInfo)
>> +{
>> +	sourceSize_ = sourceSize;
>> +	targetSize_ = targetSize;
>> +	pixelFormatInfo_ = pixelFormatInfo;
>> +}
>> +
>> +static std::string datetime()
>> +{
>> +	time_t rawtime;
>> +	struct tm *timeinfo;
>> +	char buffer[80];
>> +	static unsigned int milliseconds = 0;
>> +
>> +	time(&rawtime);
>> +	timeinfo = localtime(&rawtime);
>> +
>> +	strftime(buffer, 80, "%d-%m-%Y.%H-%M-%S.", timeinfo);
>> +
>> +	/* milliseconds is just a fast hack to ensure unique filenames */
>> +	return std::string(buffer) + std::to_string(milliseconds++);
>> +}
> I think this function is here for debug, but shouldn't be in the scaler.
>
>> +
>> +/* Handpicked from src/qcam/format_converter.cpp */
>> +static void yuv_to_rgb(int y, int u, int v, int *r, int *g, int *b)
>> +{
>> +	int c = y - 16;
>> +	int d = u - 128;
>> +	int e = v - 128;
>> +	*r = CLIP(( 298 * c           + 409 * e + 128) >> RGBSHIFT);
>> +	*g = CLIP(( 298 * c - 100 * d - 208 * e + 128) >> RGBSHIFT);
>> +	*b = CLIP(( 298 * c + 516 * d           + 128) >> RGBSHIFT);
>> +}
> CLIP does look better than std::clamp there.
>
> As RGBSHIFT is only used here, maybe put it as a constexpr in this function?
>
>
> But I wonder if we should really be making the scaler do pixel-format
> conversions.
>
> In the case of the Thumbnail generations it's a nice optimisztion, but
> it doesn't feel right, and leads to the object being an anything to
> anything scaler/convertor.
>
> As that is possible in some hardware (scale and pixel conversion
> combined) I'm almost tempted to say it's ok ... but I'm just not sure.
If it seems OK and *if* we do end up keeping this scaler + converter 
combo, I would rename the class as Thumbnailer, instead of Scaler.
>
> The alternative would be to take the incoming image, rescale it to the
> thumbnail, and then run that through another JPEG compressor anyway,
> which already supports NV12.
That sounds an overkill maybe? Not sure... The EXIF spec supports 
uncompressed YCbCr as thumbnail data, so ... we can just scale down our 
NV12.

One question here to tinker is, what's in-general use-case of the 
thumbnail data? Image previewing? in something like file-manager(s) / 
apps. I would love to learn if  they decode this data and then display 
it (like 'raw2rgbgnm' equivalent) maybe? Or they do something completely 
different
>
>
>> +
>> +int Scaler::scaleBuffer(const FrameBuffer *source, Span<uint8_t> &dest)
>> +{
>> +	MappedFrameBuffer frame(source, PROT_READ);
>> +	if (!frame.isValid()) {
>> +		LOG(SCALER, Error) << "Failed to map FrameBuffer : "
>> +				   << strerror(frame.error());
>> +		return frame.error();
>> +	}
>> +
>> +	if (strcmp(pixelFormatInfo_->name, "NV12") != 0) {
> Can you compare against formats::NV12 directly?
>
>   	if (pixelFormatInfo_->format != formats::NV12) {
>
>> +		LOG (SCALER, Info) << "Source Buffer not in NV12 format, returning...";
>> +		return -1;
> This should be in configure() though, not on every call to scaleBuffer().
ah yeah, Make sense.
>
>> +	}
>> +
>> +	/* Image scaling block implementing nearest-neighbour algorithm. */
>> +	{
> I'd remove the scoping here and indent left - or if you want to keep it
> distinct, put it into a function called
>
>    scaleNearestNeighbour(MappedFrameBuffer source*, Span<uint8_t> &dest)
>
> That would leave scale() only doing the mapping ... but make the code
> paths clear if another scaler implementation was added later...?
>
>
>> +		unsigned int cb_pos = 0;
>> +		unsigned int cr_pos = 1;
>> +		unsigned char *src = static_cast<unsigned char *>(frame.maps()[0].data());
>> +		unsigned char *src_c = src + sourceSize_.height * sourceSize_.width;
>> +		unsigned int stride = sourceSize_.width;
>> +		unsigned char *src_y, *src_cb, *src_cr;
>> +
>> +		unsigned int bpp = 3;
>> +		size_t dstSize = targetSize_.height * targetSize_.width * bpp;
>> +		unsigned char *dst = static_cast<unsigned char *>(malloc(dstSize));
> Hrm ... so the caller would have to know to free the span it receives.
> That's not nice as you normally expect a Span to by just a pointer into
> some space, that you don't have control over.
>
> I think the caller should probably allocate and provide the destination
> memory.
ah indeed, this now seems a leak.
>
>
>> +		unsigned char *destination = dst;
>> +		int r, g, b;
>> +
>> +		for (unsigned int y = 0; y < targetSize_.height; y++) {
>> +			int32_t sourceY = lround(sourceSize_.height * y / targetSize_.height);
>> +
>> +			for (unsigned int x = 0; x < targetSize_.width; x++) {
>> +				int32_t sourceX = lround(sourceSize_.width * x / targetSize_.width);
>> +
>> +				src_y = src + stride * sourceY + sourceX;
>> +				src_cb = src_c + (sourceY / 2) * stride + (sourceX / 2) * 2 + cb_pos;
>> +				src_cr = src_c + (sourceY / 2) * stride + (sourceX / 2) * 2 + cr_pos;
>> +
>> +				yuv_to_rgb(*src_y, *src_cb, *src_cr, &r, &g, &b);
>> +
>> +				destination[x * bpp + 0] = r;
>> +				destination[x * bpp + 1] = g;
>> +				destination[x * bpp + 2] = b;
>> +			}
>> +
>> +			destination = destination + bpp * targetSize_.width;
>> +		}
>> +
>> +		/* Helper code: Write the output pixels to a file so we can inspect */
>> +		File file("/tmp/" + datetime() + ".raw");
>> +		int32_t ret = file.open(File::WriteOnly);
>> +		ret = file.write({ dst, dstSize });
>> +		LOG(SCALER, Info) << "Wrote " << ret << " bytes: " << targetSize_.width << "x" << targetSize_.height;
>> +
> We should keep debug to a separate patch.
>
> It can be provided in the series with a DNI prefix if it's helpful
> during review/development - but try to keep the patch targeted for
> integration clean.
>
>> +		/* Write scaled pixels to dest */
>> +		dest = { dst, dstSize };
> If the caller provides the memory, we shouldn't need to do this, as we
> should be dealing with fixed size buffers, so the dest should be already
> correctly sized.
>
>
>
>> +	}
>> +
>> +	return 0;
>> +}
>> diff --git a/src/android/jpeg/scaler.h b/src/android/jpeg/scaler.h
>> new file mode 100644
>> index 0000000..c68a279
>> --- /dev/null
>> +++ b/src/android/jpeg/scaler.h
>> @@ -0,0 +1,32 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Copyright (C) 2020, Google Inc.
>> + *
>> + * Basic image scaler from NV12 to RGB24 format
> That's a scaler and pixelformat convertor ;-)
Yes, I can imagine now. My preference is to rename this Thumbnailer
OR
remove the convertor part and simply scaling down NV12 stream. The EXIF 
spec (if I inferred correctly) supports uncompressed YCbCr for thumbnail 
data.
(Page 24 - Exif Version 2.31 spec)
>
>> + */
>> +#ifndef __ANDROID_JPEG_SCALER_H__
>> +#define __ANDROID_JPEG_SCALER_H__
>> +
>> +#include <libcamera/geometry.h>
>> +
>> +#include "libcamera/internal/buffer.h"
>> +#include "libcamera/internal/formats.h"
>> +
>> +class Scaler
>> +{
>> +public:
>> +	Scaler();
>> +	~Scaler();
>> +
>> +	void configure(libcamera::Size sourceSize, libcamera::Size targetSize,
>> +		       const libcamera::PixelFormatInfo *pixelFormatInfo);
>> +	int scaleBuffer(const libcamera::FrameBuffer *source, libcamera::Span<uint8_t> &dest);
>> +
>> +private:
>> +
>> +	libcamera::Size sourceSize_;
>> +	libcamera::Size targetSize_;
>> +	const libcamera::PixelFormatInfo *pixelFormatInfo_;
>> +};
>> +
>> +#endif /* __ANDROID_JPEG_SCALER_H__ */
>> diff --git a/src/android/meson.build b/src/android/meson.build
>> index 0293c20..aefb0da 100644
>> --- a/src/android/meson.build
>> +++ b/src/android/meson.build
>> @@ -22,6 +22,7 @@ android_hal_sources = files([
>>       'camera_ops.cpp',
>>       'jpeg/encoder_libjpeg.cpp',
>>       'jpeg/exif.cpp',
>> +    'jpeg/scaler.cpp',
>>   ])
>>   
>>   android_camera_metadata_sources = files([
>>

Hi Umang,

Thank you for the patch.

On Sat, Sep 26, 2020 at 02:06:59AM +0530, Umang Jain wrote:
> On 9/25/20 8:33 PM, Kieran Bingham wrote:
> > On 22/09/2020 09:54, Umang Jain wrote:
> >> Add a basic image scaler for NV12 frames being captured. The primary
> >> use of this scaler will be to generate a thumbnail image to be embedded
> >> as a part of EXIF metadata of the frame.
> >>
> >> Signed-off-by: Umang Jain <email@uajain.com>
> >> ---
> >>   src/android/jpeg/encoder_libjpeg.cpp |  10 ++
> >>   src/android/jpeg/scaler.cpp          | 137 +++++++++++++++++++++++++++
> >>   src/android/jpeg/scaler.h            |  32 +++++++
> >>   src/android/meson.build              |   1 +
> >>   4 files changed, 180 insertions(+)
> >>   create mode 100644 src/android/jpeg/scaler.cpp
> >>   create mode 100644 src/android/jpeg/scaler.h
> >>
> >> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp
> >> index 510613c..9ecf9b1 100644
> >> --- a/src/android/jpeg/encoder_libjpeg.cpp
> >> +++ b/src/android/jpeg/encoder_libjpeg.cpp
> >> @@ -6,6 +6,7 @@
> >>    */
> >>   
> >>   #include "encoder_libjpeg.h"
> >> +#include "scaler.h"
> >>   
> >>   #include <fcntl.h>
> >>   #include <iomanip>
> >> @@ -214,6 +215,15 @@ int EncoderLibJpeg::encode(const FrameBuffer *source,
> >>   	LOG(JPEG, Debug) << "JPEG Encode Starting:" << compress_.image_width
> >>   			 << "x" << compress_.image_height;
> >>   
> >> +	Scaler scaler;
> >> +	libcamera::Span<uint8_t> thumbnail;
> >> +	scaler.configure(Size (compress_.image_width, compress_.image_height),
> >> +			 /* \todo: Check for exact thumbnail size required by EXIF spec. */
> >> +		         Size (compress_.image_width / 3, compress_.image_height / 3),
> >
> > Indeed, I think this should set explicit sizes, not simply reduce to a
> > third.
> >
> > http://www.fifi.org/doc/jhead/exif-e.html#ExifThumbs
> >
> > States, that usually the thumbnails are 160x120 for Exif2.1 or later,
> > and are usually encoded as JPG we can use another JPEG compressor
> > instance most likely, but if the RGB format works, that's fine too for now.
>
> The EXIF standard states that:
> ```
> No limit is placed on the size of thumbnail images. It is optional to 
> record thumbnails but it is recommended that they be recorded if 
> possible, unless hardware or other restrictions preclude this.Thumbnail 
> data does not necessarily have to adopt the same data structure as that 
> used for primary images. If, however, the primary images are recorded as 
> uncompressed RGB data or as uncompressed YCbCr data, thumbnail images 
> shall not be recorded as JPEG compressed data.
> 
> When thumbnails are recorded in uncompressed format, they are to be 
> recorded in the 1st IFD in conformance with Baseline TIFF Rev. 6.0 RGB 
> Full Color Images or TIFF Rev. 6.0 Extensions YCbCr Images.
> ```
> (Page 24 - Exif Version 2.31 spec)
> 
> So it seems we can write uncompressed data for thumbnail in RGB888, 
> right? It also states YCbCr  too (for uncompressed format) , so maybe we 
> want that? That would take out the 'converter' part of this patch.
> I am not really sure which format to use, so I will move forward as per 
> the advice given in the reviews...

Both should be acceptable. If I had to guess I'd say that RGB would be
better supported, but that's just a guess. However, converting to RGB
means we have to deal with colorspace issues. And if we later encode
the thumbnail to JPEG, we'd need YUV anyway. I'm thus tempted to drop
the RGB conversion from the scaler.

> > I'd hard code the size to 160 x 120 all the same.
>
> Yeah, that would come along in a proper patch.

Scaling down by 3 and storing the result in uncompressed form would
consume 4MB in RGB. That's a lot for a thumbnail :-) 160x120 is
57.6 kiB, which is better, but maybe still a good candidate for JPEG
compression (don't forget to set the Compression tag in IFD1).

Regarding the size, please make sure to preserve the original aspect
ratio (this is required by section 4.4.2 of Exif v2.32). This means that
160x120 should be either the minimum or maximum size (e.g. a 16:9 image
would be scaled down to either 160x90 or 213x120 - the latter isn't nice
with rounding errors, so it could also be rounded to 208x117 or
224x126). I don't have a strong preference on whether 160x120 should be
the minimum or maximum size.

> >> +			 pixelFormatInfo_);
> >> +	scaler.scaleBuffer(source, thumbnail);
> >> +	/* \todo: Write thumbnail as part of exifData. */
> >> +
> >>   	if (nv_)
> >>   		compressNV(&frame);
> >>   	else
> >> diff --git a/src/android/jpeg/scaler.cpp b/src/android/jpeg/scaler.cpp
> >> new file mode 100644
> >> index 0000000..ff36ece
> >> --- /dev/null
> >> +++ b/src/android/jpeg/scaler.cpp
> >> @@ -0,0 +1,137 @@
> >> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> >> +/*
> >> + * Copyright (C) 2020, Google Inc.
> >> + *
> >> + * scaler.cpp - Basic image scaler from NV12 to RGB24 format
> >> + */
> >> +
> >> +#include "scaler.h"
> >> +
> >> +#include <math.h>
> >> +#include <string.h>
> >> +
> >> +#include "libcamera/internal/log.h"
> >> +#include "libcamera/internal/file.h"

Doesn't checkstyle.py tell you to keep these two headers alphabetically
sorted ?

> >> +
> >> +#define RGBSHIFT                8
> >> +#ifndef MAX
> >> +#define MAX(a,b)                ((a)>(b)?(a):(b))
> >> +#endif
> >> +#ifndef MIN
> >> +#define MIN(a,b)                ((a)<(b)?(a):(b))
> >> +#endif
> >
> > #include <algorithm>
> > will give you std::min, std::max, and std::clamp()
> >
> >> +#ifndef CLAMP
> >> +#define CLAMP(a,low,high)       MAX((low),MIN((high),(a)))
> >> +#endif
> >
> >> +#ifndef CLIP
> >> +#define CLIP(x)                 CLAMP(x,0,255)
> >> +#endif
> >
> > I think this one is distinct though, Depends on how it's utilised to see
> > whether it's worth just using std::clamp directly.
> >
> >> +
> >> +using namespace libcamera;
> >> +
> >> +LOG_DEFINE_CATEGORY(SCALER)

s/SCALER/Scaler/

> >> +
> >> +Scaler::Scaler()
> >> +	: sourceSize_(0, 0), targetSize_(0,0)

No need for this, the default constructor for Size does the same. You
can thus drop the explicit constructor for the Scaler class.

> >> +{
> >> +}
> >> +
> >> +Scaler::~Scaler()

Same here, you can drop the explicit destructor.

> >> +{
> >> +}
> >> +
> >> +void Scaler::configure(Size sourceSize, Size targetSize, const PixelFormatInfo *pixelFormatInfo)

You could pass sizes as const references.

If we decide to turn this class into a thumbnail generator, I would also
compute the target size internally.

> >> +{
> >> +	sourceSize_ = sourceSize;
> >> +	targetSize_ = targetSize;
> >> +	pixelFormatInfo_ = pixelFormatInfo;
> >> +}
> >> +
> >> +static std::string datetime()
> >> +{
> >> +	time_t rawtime;
> >> +	struct tm *timeinfo;
> >> +	char buffer[80];
> >> +	static unsigned int milliseconds = 0;
> >> +
> >> +	time(&rawtime);
> >> +	timeinfo = localtime(&rawtime);
> >> +
> >> +	strftime(buffer, 80, "%d-%m-%Y.%H-%M-%S.", timeinfo);
> >> +
> >> +	/* milliseconds is just a fast hack to ensure unique filenames */
> >> +	return std::string(buffer) + std::to_string(milliseconds++);
> >> +}
> >
> > I think this function is here for debug, but shouldn't be in the scaler.
> >
> >> +
> >> +/* Handpicked from src/qcam/format_converter.cpp */
> >> +static void yuv_to_rgb(int y, int u, int v, int *r, int *g, int *b)
> >> +{
> >> +	int c = y - 16;
> >> +	int d = u - 128;
> >> +	int e = v - 128;
> >> +	*r = CLIP(( 298 * c           + 409 * e + 128) >> RGBSHIFT);
> >> +	*g = CLIP(( 298 * c - 100 * d - 208 * e + 128) >> RGBSHIFT);
> >> +	*b = CLIP(( 298 * c + 516 * d           + 128) >> RGBSHIFT);
> >> +}
> >
> > CLIP does look better than std::clamp there.
> >
> > As RGBSHIFT is only used here, maybe put it as a constexpr in this function?
> >
> > But I wonder if we should really be making the scaler do pixel-format
> > conversions.
> >
> > In the case of the Thumbnail generations it's a nice optimisztion, but
> > it doesn't feel right, and leads to the object being an anything to
> > anything scaler/convertor.
> >
> > As that is possible in some hardware (scale and pixel conversion
> > combined) I'm almost tempted to say it's ok ... but I'm just not sure.
>
> If it seems OK and *if* we do end up keeping this scaler + converter 
> combo, I would rename the class as Thumbnailer, instead of Scaler.
>
> > The alternative would be to take the incoming image, rescale it to the
> > thumbnail, and then run that through another JPEG compressor anyway,
> > which already supports NV12.
>
> That sounds an overkill maybe? Not sure... The EXIF spec supports 
> uncompressed YCbCr as thumbnail data, so ... we can just scale down our 
> NV12.
> 
> One question here to tinker is, what's in-general use-case of the 
> thumbnail data? Image previewing? in something like file-manager(s) / 
> apps. I would love to learn if  they decode this data and then display 
> it (like 'raw2rgbgnm' equivalent) maybe? Or they do something completely 
> different

The main use case is preview in gallery or file manager applications,
yes.

> >> +
> >> +int Scaler::scaleBuffer(const FrameBuffer *source, Span<uint8_t> &dest)
> >> +{
> >> +	MappedFrameBuffer frame(source, PROT_READ);
> >> +	if (!frame.isValid()) {
> >> +		LOG(SCALER, Error) << "Failed to map FrameBuffer : "
> >> +				   << strerror(frame.error());
> >> +		return frame.error();
> >> +	}
> >> +
> >> +	if (strcmp(pixelFormatInfo_->name, "NV12") != 0) {
> >
> > Can you compare against formats::NV12 directly?
> >
> >   	if (pixelFormatInfo_->format != formats::NV12) {
> >
> >> +		LOG (SCALER, Info) << "Source Buffer not in NV12 format, returning...";
> >> +		return -1;
> >
> > This should be in configure() though, not on every call to scaleBuffer().
>
> ah yeah, Make sense.
>
> >> +	}
> >> +
> >> +	/* Image scaling block implementing nearest-neighbour algorithm. */
> >> +	{
> >
> > I'd remove the scoping here and indent left - or if you want to keep it
> > distinct, put it into a function called
> >
> >    scaleNearestNeighbour(MappedFrameBuffer source*, Span<uint8_t> &dest)
> >
> > That would leave scale() only doing the mapping ... but make the code
> > paths clear if another scaler implementation was added later...?
> >
> >> +		unsigned int cb_pos = 0;
> >> +		unsigned int cr_pos = 1;
> >> +		unsigned char *src = static_cast<unsigned char *>(frame.maps()[0].data());

Think should be const.

> >> +		unsigned char *src_c = src + sourceSize_.height * sourceSize_.width;

This too.

> >> +		unsigned int stride = sourceSize_.width;
> >> +		unsigned char *src_y, *src_cb, *src_cr;

And this too.

> >> +
> >> +		unsigned int bpp = 3;

constexpr

> >> +		size_t dstSize = targetSize_.height * targetSize_.width * bpp;
> >> +		unsigned char *dst = static_cast<unsigned char *>(malloc(dstSize));
> >
> > Hrm ... so the caller would have to know to free the span it receives.
> > That's not nice as you normally expect a Span to by just a pointer into
> > some space, that you don't have control over.
> >
> > I think the caller should probably allocate and provide the destination
> > memory.

That, or we should use appropriate containers. The caller could pass a
pointer to a std::vector<> that would be resized appropriately by this
function, or the function could return a std::vector<>. I think it makes
sense to calculate the destination size in this class, so I wouldn't
allocate memory in the caller.

> ah indeed, this now seems a leak.
>
> >> +		unsigned char *destination = dst;
> >> +		int r, g, b;
> >> +
> >> +		for (unsigned int y = 0; y < targetSize_.height; y++) {
> >> +			int32_t sourceY = lround(sourceSize_.height * y / targetSize_.height);

sourceY can never be negative, you can make it an unsigned int.

Size.height and h are all integers, so the result of the division will
be an integer, and lround() won't do much. What you want here is

		unsigned int sourceY = (sourceSize_.height * y + targetSize_.height / 2)
				     / targetSize_.height;

> >> +
> >> +			for (unsigned int x = 0; x < targetSize_.width; x++) {
> >> +				int32_t sourceX = lround(sourceSize_.width * x / targetSize_.width);

Same here.

> >> +
> >> +				src_y = src + stride * sourceY + sourceX;
> >> +				src_cb = src_c + (sourceY / 2) * stride + (sourceX / 2) * 2 + cb_pos;
> >> +				src_cr = src_c + (sourceY / 2) * stride + (sourceX / 2) * 2 + cr_pos;

Instead of doing this computation for each pixel, you could optimize by
calculating the line address before the X loop (src + stride * sourceY,
src_c + sourceY / 2 * stride + cb_pos, and similarly for src_cr) and
just indexing here

				yuv_to_rgb(src_y[sourceX], src_cb[sourceX / 2 * 2],
					   src_cr[sourceX / 2 * 2], &r, &g, &b);

And additional optimization would just be to set src_y = src before the
two loops, and adding src_y += stride after the X loop (same for src_cb
and src_cr).

> >> +
> >> +				yuv_to_rgb(*src_y, *src_cb, *src_cr, &r, &g, &b);
> >> +
> >> +				destination[x * bpp + 0] = r;
> >> +				destination[x * bpp + 1] = g;
> >> +				destination[x * bpp + 2] = b;
> >> +			}
> >> +
> >> +			destination = destination + bpp * targetSize_.width;
> >> +		}
> >> +
> >> +		/* Helper code: Write the output pixels to a file so we can inspect */
> >> +		File file("/tmp/" + datetime() + ".raw");
> >> +		int32_t ret = file.open(File::WriteOnly);
> >> +		ret = file.write({ dst, dstSize });
> >> +		LOG(SCALER, Info) << "Wrote " << ret << " bytes: " << targetSize_.width << "x" << targetSize_.height;
> >> +
> >
> > We should keep debug to a separate patch.
> >
> > It can be provided in the series with a DNI prefix if it's helpful
> > during review/development - but try to keep the patch targeted for
> > integration clean.
> >
> >> +		/* Write scaled pixels to dest */
> >> +		dest = { dst, dstSize };
> >
> > If the caller provides the memory, we shouldn't need to do this, as we
> > should be dealing with fixed size buffers, so the dest should be already
> > correctly sized.
> >
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> diff --git a/src/android/jpeg/scaler.h b/src/android/jpeg/scaler.h
> >> new file mode 100644
> >> index 0000000..c68a279
> >> --- /dev/null
> >> +++ b/src/android/jpeg/scaler.h
> >> @@ -0,0 +1,32 @@
> >> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> >> +/*
> >> + * Copyright (C) 2020, Google Inc.
> >> + *
> >> + * Basic image scaler from NV12 to RGB24 format
> >
> > That's a scaler and pixelformat convertor ;-)
>
> Yes, I can imagine now. My preference is to rename this Thumbnailer
> OR
> remove the convertor part and simply scaling down NV12 stream. The EXIF 
> spec (if I inferred correctly) supports uncompressed YCbCr for thumbnail 
> data.
> (Page 24 - Exif Version 2.31 spec)

I agree with you. A Thumbnailer could compress to JPEG as well, while a
Scaler would output NV12. There's a use case for splitting the scaler to
a separate component in order to reuse it, but that can be done later on
top if/when needed. I think I'd rather go for Thumbnailer for now (also
because a nearest-neighbour scaler, while probably fine for thumbnails,
will not be good enough to scale streams for other use cases).

> >> + */
> >> +#ifndef __ANDROID_JPEG_SCALER_H__
> >> +#define __ANDROID_JPEG_SCALER_H__
> >> +
> >> +#include <libcamera/geometry.h>
> >> +
> >> +#include "libcamera/internal/buffer.h"
> >> +#include "libcamera/internal/formats.h"
> >> +
> >> +class Scaler
> >> +{
> >> +public:
> >> +	Scaler();
> >> +	~Scaler();
> >> +
> >> +	void configure(libcamera::Size sourceSize, libcamera::Size targetSize,
> >> +		       const libcamera::PixelFormatInfo *pixelFormatInfo);
> >> +	int scaleBuffer(const libcamera::FrameBuffer *source, libcamera::Span<uint8_t> &dest);
> >> +
> >> +private:
> >> +
> >> +	libcamera::Size sourceSize_;
> >> +	libcamera::Size targetSize_;
> >> +	const libcamera::PixelFormatInfo *pixelFormatInfo_;
> >> +};
> >> +
> >> +#endif /* __ANDROID_JPEG_SCALER_H__ */
> >> diff --git a/src/android/meson.build b/src/android/meson.build
> >> index 0293c20..aefb0da 100644
> >> --- a/src/android/meson.build
> >> +++ b/src/android/meson.build
> >> @@ -22,6 +22,7 @@ android_hal_sources = files([
> >>       'camera_ops.cpp',
> >>       'jpeg/encoder_libjpeg.cpp',
> >>       'jpeg/exif.cpp',
> >> +    'jpeg/scaler.cpp',
> >>   ])
> >>   
> >>   android_camera_metadata_sources = files([