[libcamera-devel] [RFC PATCH 2/2] android: jpeg: Add a basic NV12 image thumbnailer
Kieran Bingham
kieran.bingham at ideasonboard.com
Fri Oct 23 10:39:35 CEST 2020
Hi Hiro-san, Laurent,
On 23/10/2020 04:59, Laurent Pinchart wrote:
> Hi Hiro-san,
>
> On Fri, Oct 23, 2020 at 09:37:09AM +0900, Hirokazu Honda wrote:
>> On Thu, Oct 22, 2020 at 11:00 PM Laurent Pinchart wrote:
>>> On Thu, Oct 22, 2020 at 08:42:01PM +0900, Hirokazu Honda wrote:
>>>> Hi Umang,
>>>> Thanks for the work. I couldn't have time to review these today and
>>>> will review tomorrow.
>>>> 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.
>>>
>>> One point to consider is that the thumbnailer is a down-scaler using a
>>> nearest-neighbour algorithm. It's good enough for thumbnails, but will
>>> lead to awful quality when scaling streams. For that, I think we need a
>>> better scaler implementation, and we could then replace the thumbnailer
>>> class with it.
>>
>> We can select nearest-neighbor and bilinear algorithms in
>> libyuv::NV12Scale with the filtering mode argument[1].
>> We may want to specify the scale algorithm on a class construction.
>> I would use libyuv here for better performance and less buggy (+ less
>> lines to be reviewed).
>> What do you'all think?
>
> We have considered libyuv early on, and I think it's an interesting
> option moving forward. The reason we have decided to go for a minimal
> scaler implementation to create JPEG thumbnails is to avoid adding an
> external dependency that isn't packaged by distributions. This allows us
> to finalize JPEG thumbnail support without delay, and decide on the
> longer term solution separately.
>
> It would be very nice if Google could work with distributions to package
> libyuv. I'd like to avoid including a copy of libyuv in libcamera.
Lack of packaging is a big factor here. If it's not available we can't
use it ;-)
However I looked into this briefly yesterday, - we could use a meson
wrap. But I don't know how that will affect us distributing packages, as
then we also have to distribute libyuv.
Or perhaps the meson wrap would link statically.
> One last comment to provide all the design background. We have also
> considered creating a format conversion library in libcamera, to gather
> code from various projects that exist out there. There's format
> conversion in libv4l2, in raw2rgbpnm, in projects such as ffmpeg, and of
> course in libyuv. I would be great if that could be consolidated and
> packaged, but that's a big task in itself.
And that's the bigger picture. This should be consolidated. And that
consolidated library should support more than yuv ;-)
Given it would be a software format convertor, and scaler, and ideally
with hardware acceleration support where possible, I think that quickly
becomes libisp - and the feature creep will ... creep in ;-)
>
>> [1] https://chromium.googlesource.com/libyuv/libyuv/+/refs/heads/master/include/libyuv/scale.h#148
>>
>>>> On Wed, Oct 21, 2020 at 8:09 PM Kieran Bingham wrote:
>>>>> On 21/10/2020 12:03, Umang Jain wrote:
>>>>>> On 10/21/20 4:25 PM, Kieran Bingham wrote:
>>>>>>> On 21/10/2020 11:51, Umang Jain wrote:
>>>>>>>> On 10/21/20 2:49 PM, Kieran Bingham wrote:
>>>>>>>>> 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
>>>>>
>>>>>>>>>> + 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
More information about the libcamera-devel
mailing list