[libcamera-devel] [PATCH 3/3] android: jpeg: exif: Embed a JPEG-encoded thumbnail
Hirokazu Honda
hiroh at chromium.org
Tue Oct 27 06:21:35 CET 2020
Hi Umang,
On Tue, Oct 27, 2020 at 7:09 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> 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)
std::vector<unsigned char>* thumbnail
We would rather use a pointer than reference if a function changes it.
Same for other places.
> > >> +{
> > >> + /* 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 ?
> > >
> > >> + }
> > >> +}
Shall this function handle a failure of jpeg encode?
> > >> +
> > >> 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.
>
Although either is fine to me, I wonder why the encoder for the
thumbnail is within the thumbnailer.
Since the thumbnailer is currently being used for thumbnail, it seems
to be reasonable to move the encoder to it.
What do you think?
> > >> + 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;
nit: constexpr unsigned int kTargetWidth = 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_; }
nit: to reduce unnecessary copy, let's return const reference.
const libcamera::Size& computeThumbanilSize() const;
Best Regards,
-Hiro
> > >> +
> > >> +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
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list