[libcamera-devel] [PATCH 3/3] android: jpeg: exif: Embed a JPEG-encoded thumbnail
Umang Jain
email at uajain.com
Tue Oct 27 21:40:46 CET 2020
Hi Laurent,
On 10/27/20 3:38 AM, Laurent Pinchart 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)
>>>> +{
>>>> + /* 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 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([
More information about the libcamera-devel
mailing list