[libcamera-devel] [PATCH 3/3] android: jpeg: exif: Embed a JPEG-encoded thumbnail
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Oct 28 02:11:13 CET 2020
Hi Umang,
On Wed, Oct 28, 2020 at 02:10:46AM +0530, Umang Jain wrote:
> On 10/27/20 3:38 AM, Laurent Pinchart wrote:
> > 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.
>
> Do you specfically suggest using unique_ptr, to make use of forward
> declaration, in this instance of EncoderLibJpeg(see a diff below)? I am
> curious to ask, because in order to make it work with incomplete
> types(i.e. forward declared), I need to insert destructor declaration in
> post_processor_jpeg.h and define a (blank)destructor in
> post_processor_jpeg.cpp
I was thinking of storing a unique pointer to Encoder, not
EncoderLibJpeg, even if we always instantiate a EncoderLibJpeg, but that
doesn't matter much.
> I learnt this and can see it in action. Ref:
> https://stackoverflow.com/a/42158611
>
> -----
> diff --git a/src/android/jpeg/post_processor_jpeg.h
> b/src/android/jpeg/post_processor_jpeg.h
> index 3706cec..8b36a21 100644
> --- a/src/android/jpeg/post_processor_jpeg.h
> +++ b/src/android/jpeg/post_processor_jpeg.h
> @@ -8,12 +8,14 @@
> #define __ANDROID_POST_PROCESSOR_JPEG_H__
>
> #include "../post_processor.h"
> +#include "thumbnailer.h"
>
> #include <libcamera/geometry.h>
>
> #include "libcamera/internal/buffer.h"
>
> class Encoder;
> +class EncoderLibJpeg;
> class CameraDevice;
>
> class PostProcessorJpeg : public PostProcessor
> @@ -28,9 +30,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_;
> + std::unique_ptr<EncoderLibJpeg> thumbnailEncoder_;
> + Thumbnailer thumbnailer_;
> };
>
>
> >>>> + 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