[libcamera-devel] [RFC PATCH 2/2] android: jpeg: Add a basic NV12 image thumbnailer
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Oct 23 05:59:06 CEST 2020
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.
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.
> [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,
Laurent Pinchart
More information about the libcamera-devel
mailing list