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

[Kieran Bingham]

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 at 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([
> 

-- 
Regards
--
Kieran