[libcamera-devel] [RFC PATCH 2/2] android: jpeg: Add a basic NV12 image thumbnailer
Umang Jain
email at uajain.com
Thu Oct 22 14:14:09 CEST 2020
Hi Hiro,
On 10/22/20 5:12 PM, Hirokazu Honda wrote:
> Hi Umang,
> Thanks for the work. I couldn't have time to review these today and
> will review tomorrow.
Thanks, but I think as this is still in a bit of flux, you can delay the
reviewing when I submit actual patches for merging :) I got what I
wanted from this RFC version, so, please use your discretion :)
> In my understanding, the thumbnail class is basically to scale frames
> to the given destination.
> Can we generalize it as PostProcessor interface so that we will be
> able to make use of it for down-scaling regardless of whether jpeg
> encoding is required.
> It will be necessary when Android HAL adaptation layer has to output
> multiple different NV12 streams while the native camera can produce a
> single NV12 stream simultaneously.
This sounds good. However, I would prefer if it can be done on top
maybe? That way, I would have a bit more context and can evaluate what
(new) use cases we need to satisfy when we reach there. Since, I am
already fatigued a bit with this series (it dragged for a while now), I
am keen on getting this merged to support our Exif work. Anyways, I am
up for the discussion if you want to start a thread on the list.
>
> Best Regards,
> -Hiro
>
> On Wed, Oct 21, 2020 at 8:09 PM Kieran Bingham
> <kieran.bingham at ideasonboard.com> wrote:
>> Hi Umang,
>>
>> On 21/10/2020 12:03, Umang Jain wrote:
>>> Hi Kieran,
>>>
>>> On 10/21/20 4:25 PM, Kieran Bingham wrote:
>>>> Hi Umang,
>>>>
>>>>
>>>> On 21/10/2020 11:51, Umang Jain wrote:
>>>>> Hi Kieran,
>>>>>
>>>>> Thanks for the comments.
>>>>>
>>>>> On 10/21/20 2:49 PM, Kieran Bingham wrote:
>>>>>> Hi Umang,
>>>>>>
>>>>>> On 21/10/2020 09:08, 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/post_processor_jpeg.cpp | 39 +++++++++
>>>>>>> src/android/jpeg/post_processor_jpeg.h | 3 +
>>>>>>> src/android/jpeg/thumbnailer.cpp | 100
>>>>>>> +++++++++++++++++++++++
>>>>>>> src/android/jpeg/thumbnailer.h | 40 +++++++++
>>>>>>> src/android/meson.build | 1 +
>>>>>>> 5 files changed, 183 insertions(+)
>>>>>>> create mode 100644 src/android/jpeg/thumbnailer.cpp
>>>>>>> create mode 100644 src/android/jpeg/thumbnailer.h
>>>>>>>
>>>>>>> diff --git a/src/android/jpeg/post_processor_jpeg.cpp
>>>>>>> b/src/android/jpeg/post_processor_jpeg.cpp
>>>>>>> index 9d452b7..f5f1f78 100644
>>>>>>> --- a/src/android/jpeg/post_processor_jpeg.cpp
>>>>>>> +++ b/src/android/jpeg/post_processor_jpeg.cpp
>>>>>>> @@ -11,6 +11,7 @@
>>>>>>> #include "../camera_metadata.h"
>>>>>>> #include "encoder_libjpeg.h"
>>>>>>> #include "exif.h"
>>>>>>> +#include "thumbnailer.h"
>>>>>>> #include <libcamera/formats.h>
>>>>>>> @@ -44,6 +45,32 @@ int PostProcessorJpeg::configure(const
>>>>>>> StreamConfiguration &inCfg,
>>>>>>> return encoder_->configure(inCfg);
>>>>>>> }
>>>>>>> +void PostProcessorJpeg::generateThumbnail(const
>>>>>>> libcamera::Span<uint8_t> &source,
>>>>>>> + std::vector<unsigned char> &thumbnail)
>>>>>>> +{
>>>>>>> + libcamera::Span<uint8_t> destination;
>>>>>>> + Thumbnailer thumbnailer;
>>>>>>> +
>>>>>>> + thumbnailer.configure(streamSize_, formats::NV12);
>>>>>>> + libcamera::Size targetSize = thumbnailer.computeThumbnailSize();
>>>>>>> + thumbnailer.scaleBuffer(source, thumbnail);
>>>>>>> +
>>>>>>> + if (thumbnail.data()) {
>>>>>>> + StreamConfiguration thumbnailCfg;
>>>>>>> +
>>>>>>> + std::unique_ptr<EncoderLibJpeg> encoder =
>>>>>>> + std::make_unique<EncoderLibJpeg>();
>>>>>>> +
>>>>>>> + thumbnailCfg.pixelFormat = formats::NV12;
>>>>>>> + thumbnailCfg.size = targetSize;
>>>>>>> + encoder->configure(thumbnailCfg);
>>>>>> thumbnail.capacity() might be quite low here.
>>>>>> We need to make sure the vector is big enough at this point, you might
>>>>>> need to do something like:
>>>>>>
>>>>>> thumbnail.resize(targetSize.width * targetSize.height * 4);
>>>>> I am not sure I follow. This is compressing the thumbnail part right? So
>>>>> thumbnail is the "source" for the encoder here (Please refer to
>>>>> ->encode() below) and why would we resize the source(i.e. 'thumbnail')?
>>>> Ugh, sorry - tired eyes indeed. Yes, please ignore those comments.
>>>>
>>>>
>>>>
>>>>>> Really we should obtain that size from the encoder. I thought we had a
>>>>>> helper to do that already, but it seems we don't.
>>>>>> We could/should add Encoder::maxOutput(); which would ask the encoder
>>>>>> "For your given configuration, what is the maximum number of bytes you
>>>>>> might output".
>>>>>>
>>>>>> Then of course we'd do:
>>>>>> thumbnail.resize(encoder.maxOutput());
>>>>>>
>>>>>>
>>>>>>> + int jpeg_size = encoder->encode({ thumbnail.data(),
>>>>>>> thumbnail.capacity() },
>>>>>>> + destination, { });
>>>>> As said above, thumbnail is the source here. The compressed thumbnail
>>>>> output is carried in destination. And, destination is a local span<>
>>>>> here. Does it makes sense?
>>>> Yes, I had mixed up source and destination I'm sorry.
>>>>
>>>> So who/where is allocating the destination?
>>> Ah, that's a good question and I think I just found a bug. I currently
>>> pass on a locally allocated Span<> 'destination' to the encoder. But
>>> there is no explicit allocation here. I think I need to point / create
>>> the Span with the output size / bytes and then pass to the encode().
>> :-) Ok - then please apply all of my earlier comments to that part
>> instead. ;-)
>>
>> We certainly need somewhere to store the compressed image, to be able to
>> pass it into the exif :-D
>>
>> --
>> Kieran
>>
>>
>>>>>>> + LOG(JPEG, Info) << "Thumbnail compress returned "
>>>>>>> + << jpeg_size << " bytes";
>>>>>> And I presume we could then do an:
>>>>>> thumbnail.resize(jpeg_size);
>>>>>>
>>>>>> here to update the thumbnail with the correct size. I'd be weary of the
>>>>>> resize operations doing lots of re-allocations though, so perhaps we
>>>>>> want to minimize that. But lets get to something that works first
>>>>>> before
>>>>>> worrying about optimising.
>>>>>>
>>>>>>
>>>>>>> + }
>>>>>>> +}
>>>>>>> +
>>>>>>> int PostProcessorJpeg::process(const libcamera::FrameBuffer
>>>>>>> *source,
>>>>>>> const libcamera::Span<uint8_t> &destination,
>>>>>>> CameraMetadata *metadata)
>>>>>>> @@ -73,6 +100,18 @@ int PostProcessorJpeg::process(const
>>>>>>> libcamera::FrameBuffer *source,
>>>>>>> return jpeg_size;
>>>>>>> }
>>>>>>> + std::vector<unsigned char> thumbnail;
>>>>>> You need to resize this somewhere.
>>>>>> Edit: Now seen a better place above ;-)
>>>>> The thumbnail vector just gets resized in Thumbnailer::scaleBuffer(). :)
>>>>> We just pass in to the thumbnailer, while keeping its ownership in
>>>>> PostProcessorJpeg.
>>>>>>> + generateThumbnail(destination, thumbnail);
>>>>>>> + /*
>>>>>>> + * \todo: Write the compressed thumbnail to a file for
>>>>>>> inspection.
>>>>>>> + * (I) Check if we can still write the thumbnail to EXIF here.
>>>>>>> + * If not, we might need to move the thumbnailer logic to
>>>>>>> encoder.
>>>>>>> + * And if we do that, first we need to make sure we get can
>>>>>>> + * compressed data written to destination first before
>>>>>>> calling
>>>>>>> + * jpeg_finish_compress operation somehow. Thumbnailing will
>>>>>>> + * only occur if we have compressed data available first.
>>>>>>> + */
>>>>>>> +
>>>>>>> /*
>>>>>>> * Fill in the JPEG blob header.
>>>>>>> *
>>>>>>> diff --git a/src/android/jpeg/post_processor_jpeg.h
>>>>>>> b/src/android/jpeg/post_processor_jpeg.h
>>>>>>> index 62c8650..05601ee 100644
>>>>>>> --- a/src/android/jpeg/post_processor_jpeg.h
>>>>>>> +++ b/src/android/jpeg/post_processor_jpeg.h
>>>>>>> @@ -28,6 +28,9 @@ public:
>>>>>>> CameraMetadata *metadata) override;
>>>>>>> private:
>>>>>>> + void generateThumbnail(const libcamera::Span<uint8_t> &source,
>>>>>>> + std::vector<unsigned char> &thumbnail);
>>>>>>> +
>>>>>>> CameraDevice *cameraDevice_;
>>>>>>> std::unique_ptr<Encoder> encoder_;
>>>>>>> libcamera::Size streamSize_;
>>>>>>> diff --git a/src/android/jpeg/thumbnailer.cpp
>>>>>>> b/src/android/jpeg/thumbnailer.cpp
>>>>>>> new file mode 100644
>>>>>>> index 0000000..3163576
>>>>>>> --- /dev/null
>>>>>>> +++ b/src/android/jpeg/thumbnailer.cpp
>>>>>>> @@ -0,0 +1,100 @@
>>>>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>>>>>> +/*
>>>>>>> + * Copyright (C) 2020, Google Inc.
>>>>>>> + *
>>>>>>> + * thumbnailer.cpp - Basic image thumbnailer from NV12
>>>>>>> + */
>>>>>>> +
>>>>>>> +#include "thumbnailer.h"
>>>>>>> +
>>>>>>> +#include <libcamera/formats.h>
>>>>>>> +
>>>>>>> +#include "libcamera/internal/file.h"
>>>>>>> +#include "libcamera/internal/log.h"
>>>>>>> +
>>>>>>> +using namespace libcamera;
>>>>>>> +
>>>>>>> +LOG_DEFINE_CATEGORY(Thumbnailer)
>>>>>>> +
>>>>>>> +Thumbnailer::Thumbnailer()
>>>>>>> + : validConfiguration_(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.";
>>>>>>> + return;
>>>>>>> + }
>>>>>>> +
>>>>>>> + validConfiguration_ = 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
>>>>>>> + * 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 libcamera::Span<uint8_t> &source,
>>>>>>> + std::vector<unsigned char> &destination)
>>>>>>> +{
>>>>>>> + if (!validConfiguration_) {
>>>>>>> + LOG(Thumbnailer, Error) << "config is unconfigured or
>>>>>>> invalid.";
>>>>>>> + return;
>>>>>>> + }
>>>>>>> +
>>>>>>> + targetSize_ = computeThumbnailSize();
>>>>>>> +
>>>>>>> + 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 *>(source.data());
>>>>>>> + unsigned char *src_c = src + sh * sw;
>>>>>>> + unsigned char *src_cb, *src_cr;
>>>>>>> +
>>>>>>> + size_t dstSize = (th * tw) + ((th/2) * tw);
>>>>>>> + destination.reserve(dstSize);
>>>>>>> + 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;
>>>>>>> +
>>>>>>> + 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 * tw + x] = src[sw * sourceY +
>>>>>>> sourceX];
>>>>>>> + dst[(y+1) * tw + x] = src[sw * (sourceY+1) +
>>>>>>> sourceX];
>>>>>>> + dst[y * tw + (x+1)] = src[sw * sourceY +
>>>>>>> (sourceX+1)];
>>>>>>> + dst[(y+1) * tw + (x+1)] = src[sw * (sourceY+1) +
>>>>>>> (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];
>>>>>>> + }
>>>>>>> + }
>>>>>>> +}
>>>>>>> diff --git a/src/android/jpeg/thumbnailer.h
>>>>>>> b/src/android/jpeg/thumbnailer.h
>>>>>>> new file mode 100644
>>>>>>> index 0000000..bab9855
>>>>>>> --- /dev/null
>>>>>>> +++ b/src/android/jpeg/thumbnailer.h
>>>>>>> @@ -0,0 +1,40 @@
>>>>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>>>>>> +/*
>>>>>>> + * Copyright (C) 2020, Google Inc.
>>>>>>> + *
>>>>>>> + * thumbnailer.h - Basic image thumbnailer from NV12
>>>>>>> + */
>>>>>>> +#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);
>>>>>>> +
>>>>>>> + /*
>>>>>>> + * \todo: Discuss if we can return targetSize_ via configure() or
>>>>>>> + * scaleBuffer(). We need targetSize_ to re-encode the scaled
>>>>>>> buffer
>>>>>>> + * via encoder in PostProcssorJpeg::writeThumbnail().
>>>>>>> + */
>>>>>>> + libcamera::Size computeThumbnailSize();
>>>>>>> + void scaleBuffer(const libcamera::Span<uint8_t> &source,
>>>>>>> + std::vector<unsigned char> &dest);
>>>>>>> +
>>>>>>> +private:
>>>>>>> + libcamera::PixelFormat pixelFormat_;
>>>>>>> + libcamera::Size sourceSize_;
>>>>>>> + libcamera::Size targetSize_;
>>>>>>> +
>>>>>>> + bool validConfiguration_;
>>>>>>> +};
>>>>>>> +
>>>>>>> +#endif /* __ANDROID_JPEG_THUMBNAILER_H__ */
>>>>>>> diff --git a/src/android/meson.build b/src/android/meson.build
>>>>>>> index 5a01bea..3905e2f 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
>> --
>> Kieran
>> _______________________________________________
>> 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