[libcamera-devel] [PATCH 1/1] android: jpeg: Add a basic NV12 image scaler
Umang Jain
email at uajain.com
Fri Sep 25 22:36:59 CEST 2020
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 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.
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([
>>
More information about the libcamera-devel
mailing list