[libcamera-devel] [PATCH 3/3] android: jpeg: exif: Embed a JPEG-encoded thumbnail
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Oct 26 22:05:33 CET 2020
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 at 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([
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list