[libcamera-devel] [PATCH 3/3] android: jpeg: exif: Embed a JPEG-encoded thumbnail
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Oct 26 23:08:18 CET 2020
Hi Kieran,
On Mon, Oct 26, 2020 at 10:02:43PM +0000, Kieran Bingham wrote:
> On 26/10/2020 21:05, Laurent Pinchart wrote:
> > 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.
>
> I think the comment will help indeed. The only alternative would be to
> pass it into generate(), but and refactor generate() to return the span
> ... but that's more effort that we need right now. So just a comment
> will do ;-)
>
>
>
> >> void setTimestamp(time_t timestamp);
> >>
> >> libcamera::Span<const uint8_t> data() const { return { exifData_, size_ }; }
> >
> > I would have moved this to a separate patch.
>
>
> Yes, I'd keep the exif extension for setting the thumbnail, and the
> usage separate.
>
> >
> >> 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.
>
> I think this was already a unique_ptr in a previous iteration, and I
> suggested moving it to directly store an instance of the libjpeg encoder.
>
> In this instance of encoding a thumbnail, when encoding a 160x160 image,
> I would be weary about the overhead of setting up a hardware encode, and
> I'd expect the preparation phases of that to be potentially more
> expensive than just a software encode. Particularly as this has just
> done a software rescale, so it would have to cache-flush, when it could
> just use the host-cpu with a hot-cache. (ok, so perhaps later it might
> use a different scaler too ... but then it's likely a different equation
> anyway)
>
> I have not measured that of course, as we don't yet have a hw-jpeg
> encode post-processor. It will be an interesting test in the future.
>
> But essentially, I think we're just as well leaving this as a single
> instance of libjpeg for thumbnails. I think I recall the CrOS HAL using
> libjpeg as well, but I haven't gone back to double-check.
Fair enough, those are good points. I'm fine if the code is kept as-is
(I wouldn't mind a unique_ptr of course :-)). Umang, up to you.
> >> + 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