[libcamera-devel] [RFC PATCH 2/2] android: jpeg: Add a basic NV12 image thumbnailer

[Hirokazu Honda]

Hi Laurent,

On Thu, Oct 22, 2020 at 11:00 PM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Hiro-san,
>
> 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?

[1] https://chromium.googlesource.com/libyuv/libyuv/+/refs/heads/master/include/libyuv/scale.h#148

Best Regards,
-Hiro

> > 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
> > >
> > > --
> > > 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,
>
> Laurent Pinchart