Hi Umang,
One more thing.
On Sun, Oct 04, 2020 at 10:16:57PM +0300, Laurent Pinchart wrote:
> On Fri, Oct 02, 2020 at 04:04:14PM +0530, 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/encoder_libjpeg.cpp | 12 +++
> > src/android/jpeg/thumbnailer.cpp | 106 +++++++++++++++++++++++++++
> > src/android/jpeg/thumbnailer.h | 34 +++++++++
> > src/android/meson.build | 1 +
> > 4 files changed, 153 insertions(+)
> > create mode 100644 src/android/jpeg/thumbnailer.cpp
> > create mode 100644 src/android/jpeg/thumbnailer.h
> >
> > diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp
> > index 510613c..7d097c6 100644
> > --- a/src/android/jpeg/encoder_libjpeg.cpp
> > +++ b/src/android/jpeg/encoder_libjpeg.cpp
> > @@ -6,6 +6,7 @@
> > */
> >
> > #include "encoder_libjpeg.h"
> > +#include "thumbnailer.h"
> >
> > #include <fcntl.h>
> > #include <iomanip>
> > @@ -214,6 +215,17 @@ int EncoderLibJpeg::encode(const FrameBuffer *source,
> > LOG(JPEG, Debug) << "JPEG Encode Starting:" << compress_.image_width
> > << "x" << compress_.image_height;
> >
> > + Thumbnailer thScaler;
> > + libcamera::Span<uint8_t> thumbnail;
> > + thScaler.configure(Size (compress_.image_width, compress_.image_height),
> > + pixelFormatInfo_->format);
> > + thScaler.scaleBuffer(source, thumbnail);
> > + /*
> > + * \todo: Discuss if we require scaling here or one level above where
> > + * exif data is set (setThumbnail()?).
> > + * Setting thumbnailed scaled data seems a bit convulated initially.
> > + */
>
> Here's a proposal.
>
> As the JPEG encoder could also be used to create the thumbnail, I would
> create an additional layer. A new JPEGPostProcessor (we can bikeshed the
> name) class would handle the whole JFIF image creation. It would would
> internally encode the image to JPEG using EncoderLibjpeg, downscale it
> to thumbnail size, compress the thumbnail with EncoderLibjpeg, generate
> the Exif, and store the JPEG-compressed thumbnail in the Exif. That way,
> JPEG encoding, Exif generation and thumbnail generation are all handled
> inside JPEGPostProcessor and not visible to the CameraDevice or
> CameraStream, and the JPEG encoder itself is kept generic enough to be
> used for both the main image and the thumbnail.
>
> > +
> > if (nv_)
> > compressNV(&frame);
> > else
> > diff --git a/src/android/jpeg/thumbnailer.cpp b/src/android/jpeg/thumbnailer.cpp
> > new file mode 100644
> > index 0000000..d706ea6
> > --- /dev/null
> > +++ b/src/android/jpeg/thumbnailer.cpp
> > @@ -0,0 +1,106 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2020, Google Inc.
> > + *
> > + * thumbnailer.cpp - Basic image thumbnailer from NV12
> > + */
> > +
> > +#include "thumbnailer.h"
> > +#include "system/graphics.h"
>
> This header shouldn't be needed, the implementation of this class should
> not be Android-specific.
>
> > +
> > +#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_ .fourcc() == HAL_PIXEL_FORMAT_YV12) {
>
> Please don't use fourcc(), this is completely wrong. PixelFormat stores
> a DRM fourcc, and you're comparing it to a completely unrelated value.
> The PixelFormat class is meant to abstract pixel formats, you must
> compare it with formats::NV12 instead.
>
> And shouldn't the check be reversed ? This accepts all formats except
> NV12.
>
> > + LOG (Thumbnailer, Error) << "Failed to configure: Pixel Format "
> > + << pixelFormat_.toString() << " unsupported.";
> > + return;
> > + }
> > +
> > + validConfiguration_ = true;
> > +}
>
> I'd simplify the API by passing the pixel format and size to the
> scaleBuffer() functionn and remove the configure() function.
>
> > +
> > +/* Compute a thumbnail size with same aspect ratio as source. */
>
> Could you please extend this comment to explain how the size is computed
> ? Not a description of the implementation, but the rules that the
> function implements.
>
> > +void Thumbnailer::computeThumbnailSize()
> > +{
> > + unsigned int baseWidth = 160;
> > +
> > + unsigned int width = baseWidth * sourceSize_.width / sourceSize_.height;
> > + targetSize_.width = width - (width % 16);
> > + targetSize_.height = targetSize_.width * sourceSize_.height
> > + / sourceSize_.width;
> > + if (targetSize_.height & 1)
> > + targetSize_.height++;
>
> With a source aspect ratio of 4:3, this outputs 208x156 instead of the
> expected 160x120.
>
> > +}
> > +
> > +int Thumbnailer::scaleBuffer(const FrameBuffer *source, Span<uint8_t> &dest)
> > +{
> > + MappedFrameBuffer frame(source, PROT_READ);
>
> Mappings are expensive. I think the caller should create the
> MappedFrameBuffer and pass it to both the JPEG encoder and the thumbnail
> generator.
>
> > + if (!frame.isValid()) {
> > + LOG(Thumbnailer, Error) << "Failed to map FrameBuffer : "
> > + << strerror(frame.error());
> > + return frame.error();
> > + }
> > +
> > + if (!validConfiguration_) {
> > + LOG(Thumbnailer, Error) << "config is unconfigured or invalid.";
> > + return -1;
> > + }
> > +
> > + computeThumbnailSize();
>
> You can make the function return the size instead of storing it in a
> member variable.
>
> > +
> > + 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 *>(frame.maps()[0].data());
> > + unsigned char *src_c = src + sh * sw;
The stride shouldn't be hardcoded to sh, but should come from the stream
configuration. This doesn't have to be implemented yet, a \todo comment
would be fine for now (see encoder_libjpeg.cpp). I would however
recommend using PixelFormatInfo to calculate the stride for now (see
encoder_libjpeg.cpp too :-)).
> > + unsigned int cb_pos = 0;
> > + unsigned int cr_pos = 1;
> > + unsigned char *src_cb, *src_cr;
> > +
> > + size_t dstSize = (th * tw) + ((th/2) * tw);
>
> No need for parentheses.
>
> > + unsigned char *destination = static_cast<unsigned char *>(malloc(dstSize));
>
> C++ uses new.
>
> > + unsigned char *dst = destination;
> > + unsigned char *dst_c = destination + th * tw;
> > +
> > + for (unsigned int y = 0; y < th; y+=2) {
>
> s/+=/ += /
>
> There are a few other locations below that need spaces around operators.
>
> > + unsigned int sourceY = (sh*y + th/2) / th;
> > +
> > + src_cb = src_c + (sourceY/2) * sw + cb_pos;
> > + src_cr = src_c + (sourceY/2) * sw + cr_pos;
>
> I'd create a new src_y variable to handle the luma plane the same way as
> the chroma plane.
>
> > +
> > + 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 + cb_pos] = src_cb[(sourceX/2) * 2];
> > + dst_c[(y/2) * tw + x + cr_pos] = src_cr[(sourceX/2) * 2];
>
> You can just do
>
> dst_c[(y/2) * tw + x + 0] = src_c[(sourceX/2) * 2 + 0];
> dst_c[(y/2) * tw + x + 1] = src_c[(sourceX/2) * 2 + 1];
>
> and drop cb_pos, cr_pos, src_cb and src_cr, as the only thing that
> matters it to copy both pixels.
>
> src_c should be handled the same way as src_cb (computed in the outer
> loop), and I would do the same for dst_c (and add a new dst_y).
>
> > + }
> > + }
> > +
> > + /* Write scaled pixels to dest */
> > + dest = { destination, dstSize };
>
> This is a very good way to cause memory leaks :-S There's nothing in the
> function signature that tells that it allocates memory internally. Let's
> create a better interface that makes leaks impossible.
>
> > +
> > + return 0;
> > +}
> > diff --git a/src/android/jpeg/thumbnailer.h b/src/android/jpeg/thumbnailer.h
> > new file mode 100644
> > index 0000000..8ba9816
> > --- /dev/null
> > +++ b/src/android/jpeg/thumbnailer.h
> > @@ -0,0 +1,34 @@
> > +/* 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);
> > + int scaleBuffer(const libcamera::FrameBuffer *source, libcamera::Span<uint8_t> &dest);
> > +
> > +private:
> > + void computeThumbnailSize();
> > +
> > + 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 0293c20..f35ba04 100644
> > --- a/src/android/meson.build
> > +++ b/src/android/meson.build
> > @@ -22,6 +22,7 @@ android_hal_sources = files([
> > 'camera_ops.cpp',
> > 'jpeg/encoder_libjpeg.cpp',
> > 'jpeg/exif.cpp',
> > + 'jpeg/thumbnailer.cpp',
> > ])
> >
> > android_camera_metadata_sources = files([
--
Regards,
Laurent Pinchart