[libcamera-devel] [PATCH 3/3] android: jpeg: exif: Embed a JPEG-encoded thumbnail
Umang Jain
email at uajain.com
Tue Oct 27 08:40:24 CET 2020
Hi Hiro,
On 10/27/20 10:51 AM, Hirokazu Honda 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.
Ack.
>>>>> +{
>>>>> + /* 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?
Ideally it should but I am wondering in what style. I would prefer that
checking for the (thumbnail data != 0) before calling
Exif::setThumbnail() below. Since, when the encoding fails, jpeg_size
will get 0, and the thumbnail vector(placeholder for our thumbnail data)
will get resized to '0'.
>>>>> +
>>>>> 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.
It is not within the encoder. It is inside the PostProcessorJpeg
> Since the thumbnailer is currently being used for thumbnail, it seems
> to be reasonable to move the encoder to it.
> What do you think?
If you mean moving the thumbnail-encoder to `class Thumbnailer` - hmm,
that could also work I suppose. However, I might not be very
enthusiastic about it, as PostProcessorJpeg captures the entire flow at
one place : e.g. exif generation, creating a thumbnail, encoding it and
finally encoding the entire frame. I like this.
If others support moving the thumbnail-encoder to Thumbnailer, that's
also fine! (Also, could be done on top, as an independent refactor)
>
>>>>> + 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
> _______________________________________________
> 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