[libcamera-devel] [RFC PATCH 2/2] android: jpeg: Add a basic NV12 image thumbnailer
Hirokazu Honda
hiroh at chromium.org
Thu Oct 22 15:39:00 CEST 2020
On Thu, Oct 22, 2020 at 9:14 PM Umang Jain <email at uajain.com> wrote:
>
> 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.
I am ok with your idea.
Another question, you scale with your own code.
How about using libyuv [1]?
The code is optimized for various platforms and the libyuv is widely
used on a lot of products.
Faster and less buggy.
[1] https://chromium.googlesource.com/libyuv/libyuv/+/refs/heads/master/include/libyuv/scale.h#159
Best Regards,
-Hiro
> >
> > 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