[libcamera-devel] [RFC PATCH 2/2] android: jpeg: Add a basic NV12 image thumbnailer
Kieran Bingham
kieran.bingham at ideasonboard.com
Wed Oct 21 11:06:03 CEST 2020
Hi Umang,
On 21/10/2020 09:08, 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 at uajain.com>
> ---
> src/android/jpeg/post_processor_jpeg.cpp | 39 +++++++++
> src/android/jpeg/post_processor_jpeg.h | 3 +
> src/android/jpeg/thumbnailer.cpp | 100 +++++++++++++++++++++++
> src/android/jpeg/thumbnailer.h | 40 +++++++++
> src/android/meson.build | 1 +
> 5 files changed, 183 insertions(+)
> create mode 100644 src/android/jpeg/thumbnailer.cpp
> create mode 100644 src/android/jpeg/thumbnailer.h
>
> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
> index 9d452b7..f5f1f78 100644
> --- a/src/android/jpeg/post_processor_jpeg.cpp
> +++ b/src/android/jpeg/post_processor_jpeg.cpp
> @@ -11,6 +11,7 @@
> #include "../camera_metadata.h"
> #include "encoder_libjpeg.h"
> #include "exif.h"
> +#include "thumbnailer.h"
>
> #include <libcamera/formats.h>
>
> @@ -44,6 +45,32 @@ int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,
> return encoder_->configure(inCfg);
> }
>
> +void PostProcessorJpeg::generateThumbnail(const libcamera::Span<uint8_t> &source,
> + std::vector<unsigned char> &thumbnail)
> +{
> + libcamera::Span<uint8_t> destination;
> + Thumbnailer thumbnailer;
> +
> + thumbnailer.configure(streamSize_, formats::NV12);
It would be nicer to see this format set from the input configuration,
so that if we ever get given a differnet format the thumbnailer will
know (and complain accordingly, until it supports that format).
> + libcamera::Size targetSize = thumbnailer.computeThumbnailSize();
> + thumbnailer.scaleBuffer(source, thumbnail);
> +
> + if (thumbnail.data()) {
> + StreamConfiguration thumbnailCfg;
> +
> + std::unique_ptr<EncoderLibJpeg> encoder =
> + std::make_unique<EncoderLibJpeg>();
No need to make this a unique pointer. It can just be constructed as a
local object:
EncoderLibJpeg encoder;
The only reason the PostProcessorJpeg::encoder is a unique_ptr is
because it might be assigned to different types of encoder:
Or perhaps rather than creating a new one each time, you could put it as
a private member in the PostProcessorJpeg, and configure it once during
PostProcessorJpeg::configure();
class PostProcessorJpeg : public PostProcessor
{
private
EncoderLibJpeg thumbnailEncoder_;
}
int PostProcessorJpeg::configure(incfg, outcfg)
{
....
thumbnailEncoder_.configure(...);
....
}
int PostProcessorJpeg::generateThumbnail(....)
{
...
thumbnailEncoder_.encode(...);
...
}
> +
> + thumbnailCfg.pixelFormat = formats::NV12;
> + thumbnailCfg.size = targetSize;
> + encoder->configure(thumbnailCfg);
> + int jpeg_size = encoder->encode({ thumbnail.data(), thumbnail.capacity() },
> + destination, { });
> + LOG(JPEG, Info) << "Thumbnail compress returned "
> + << jpeg_size << " bytes";
> + }
> +}
> +
> int PostProcessorJpeg::process(const libcamera::FrameBuffer *source,
> const libcamera::Span<uint8_t> &destination,
> CameraMetadata *metadata)
> @@ -73,6 +100,18 @@ int PostProcessorJpeg::process(const libcamera::FrameBuffer *source,
> return jpeg_size;
> }
>
> + std::vector<unsigned char> thumbnail;
> + generateThumbnail(destination, thumbnail);
> + /*
> + * \todo: Write the compressed thumbnail to a file for inspection.
> + * (I) Check if we can still write the thumbnail to EXIF here.
> + * If not, we might need to move the thumbnailer logic to encoder.
> + * And if we do that, first we need to make sure we get can
> + * compressed data written to destination first before calling
> + * jpeg_finish_compress operation somehow. Thumbnailing will
> + * only occur if we have compressed data available first.
I don't see how thumbnailing will help in the encoder. The exif is
constructed in PostProcessorJpeg::process().
Perhaps it's about getting the thumbnail into the encoder/exif.
For that I'd add a helper to our Exif class to be able to
exif.setThumbnail(span<uint8_t> &thumbnail);
Then the exif.generate() should ideally make sure the exif is fully
created correctly. (including the thumbnail) before it goes into the
encapsulating jpeg.
> + */
> +
> /*
> * Fill in the JPEG blob header.
> *
> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h
> index 62c8650..05601ee 100644
> --- a/src/android/jpeg/post_processor_jpeg.h
> +++ b/src/android/jpeg/post_processor_jpeg.h
> @@ -28,6 +28,9 @@ public:
> CameraMetadata *metadata) override;
>
> private:
> + void generateThumbnail(const libcamera::Span<uint8_t> &source,
> + std::vector<unsigned char> &thumbnail);
> +
> CameraDevice *cameraDevice_;
> std::unique_ptr<Encoder> encoder_;
> libcamera::Size streamSize_;
> diff --git a/src/android/jpeg/thumbnailer.cpp b/src/android/jpeg/thumbnailer.cpp
> new file mode 100644
> index 0000000..3163576
> --- /dev/null
> +++ b/src/android/jpeg/thumbnailer.cpp
> @@ -0,0 +1,100 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * thumbnailer.cpp - Basic image thumbnailer from NV12
I'd call it a 'Simple image thumbnailer'
No need to specify the format in the description. It might need
extending later anyway.
> + */
> +
> +#include "thumbnailer.h"
> +
> +#include <libcamera/formats.h>
> +
> +#include "libcamera/internal/file.h"
> +#include "libcamera/internal/log.h"
> +
> +using namespace libcamera;
> +
> +LOG_DEFINE_CATEGORY(Thumbnailer)
> +
> +Thumbnailer::Thumbnailer()
> + : validConfiguration_(false)
this could simply be valid_ if you want it to be shorter, but this is fine.
> +{
> +}
> +
> +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.";
> + return;
> + }
> +
> + validConfiguration_ = 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
> + * 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);
> +}
I would make this a private function, and call it from configure().
Or inline it in configure (It can be kept separate if that helps/makes
sense).
Then have a public function:
Thumbnailer::size() { return outputSize_; } const
> +
> +void
> +Thumbnailer::scaleBuffer(const libcamera::Span<uint8_t> &source,
> + std::vector<unsigned char> &destination)
> +{
> + if (!validConfiguration_) {
> + LOG(Thumbnailer, Error) << "config is unconfigured or invalid.";
Perhaps:
"Thumbnailer has an invalid configuration.";
> + return;
> + }
> +
> + targetSize_ = computeThumbnailSize();
This should be in configure();
> +
> + 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 *>(source.data());
> + unsigned char *src_c = src + sh * sw;
> + unsigned char *src_cb, *src_cr;
> +
> + size_t dstSize = (th * tw) + ((th/2) * tw);
> + destination.reserve(dstSize);
> + 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;
> +
> + 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 * tw + x] = src[sw * sourceY + sourceX];
> + dst[(y+1) * tw + x] = src[sw * (sourceY+1) + sourceX];
> + dst[y * tw + (x+1)] = src[sw * sourceY + (sourceX+1)];
> + dst[(y+1) * tw + (x+1)] = src[sw * (sourceY+1) + (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];
> + }
> + }
> +}
> diff --git a/src/android/jpeg/thumbnailer.h b/src/android/jpeg/thumbnailer.h
> new file mode 100644
> index 0000000..bab9855
> --- /dev/null
> +++ b/src/android/jpeg/thumbnailer.h
> @@ -0,0 +1,40 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * thumbnailer.h - Basic image thumbnailer from NV12
> + */
> +#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);
Shouldn't configure return an int to return if the configuration is
unsupported ?
(which in this current case would only be if someone supplies an
incorrect pixelformat I expect).
Alternatively, we could simplify the thumbnailer down to just a single
process function which passes in the source size,
> +
> + /*
> + * \todo: Discuss if we can return targetSize_ via configure() or
> + * scaleBuffer(). We need targetSize_ to re-encode the scaled buffer
> + * via encoder in PostProcssorJpeg::writeThumbnail().
s/PostProcssorJpeg/PostProcessorJpeg/
> + */
> + libcamera::Size computeThumbnailSize();
> + void scaleBuffer(const libcamera::Span<uint8_t> &source,
> + std::vector<unsigned char> &dest);
> +
> +private:
> + libcamera::PixelFormat pixelFormat_;
Until this supports something that isn't NV12, you don't actually need
to store the pixelFormat_;
> + libcamera::Size sourceSize_;
> + libcamera::Size targetSize_;
> +
> + bool validConfiguration_;
> +};
> +
> +#endif /* __ANDROID_JPEG_THUMBNAILER_H__ */
> diff --git a/src/android/meson.build b/src/android/meson.build
> index 5a01bea..3905e2f 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
--
Kieran
More information about the libcamera-devel
mailing list