[libcamera-devel] [PATCH 3/3] android: jpeg: exif: Embed a JPEG-encoded thumbnail
Hirokazu Honda
hiroh at chromium.org
Tue Oct 27 08:34:34 CET 2020
On Tue, Oct 27, 2020 at 3:53 PM Hirokazu Honda <hiroh at chromium.org> wrote:
>
> Hi Umang,
>
> Just few additional comments.
>
> On Tue, Oct 27, 2020 at 2:21 PM Hirokazu Honda <hiroh at chromium.org> wrote:
> >
> > 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;
> > > > >> +}
> > > > >> +
> > > > >> +/*
>
> Shall we check sourceSize is an even dimension and smaller than targetSize_?
>
> > > > >> + * 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;
> > > > >> +
>
> assert(tw % 2 == 0 && th % 2 == 0);
>
> > > > >> + /* Image scaling block implementing nearest-neighbour algorithm. */
> > > > >> + unsigned char *src = static_cast<unsigned char *>(frame.maps()[0].data());
>
> Should we check just in case, frame.size() is sourceSize_?
>
> > > > >> + unsigned char *src_c = src + sh * sw;
> > > > >> + unsigned char *src_cb, *src_cr;
> > > > >> + unsigned char *dst_y, *src_y;
> > > > >> +
>
> nit: Follow variable naming rule? srcY, srcCb, and so on.
>
> > > > >> + 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;
> > > > >> +
>
> I don't understand well why "+ th / 2" is required.
> Could you explain?
>
Please ignore. I found that this addition is for round to the nearest integer.
> Best Regards,
> -Hiro
> > > > >> + 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