[libcamera-devel] [PATCH v3 2/2] android: libyuv: Introduce PostProcessorLibyuv
Hirokazu Honda
hiroh at chromium.org
Tue Feb 2 08:26:54 CET 2021
Hi Jacopo,
Thanks for reviewing!
On Mon, Feb 1, 2021 at 8:53 PM Jacopo Mondi <jacopo at jmondi.org> wrote:
>
> Hi Hiro,
>
> On Thu, Jan 28, 2021 at 10:42:16PM +0000, Hirokazu Honda wrote:
> > This adds PostProcessorLibyuv. It supports NV12 buffer scaling.
> >
> > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> >
> > ---
> > src/android/libyuv/post_processor_libyuv.cpp | 123 +++++++++++++++++++
> > src/android/libyuv/post_processor_libyuv.h | 44 +++++++
>
> we have
> src/android/jpeg
> which used libjpeg, but that's not reflected in the directory name
>
> would
> src/android/yuv/
> be a better match ?
>
> same question for the class and the associated filename, would
> PostProcessorYuv be better ?
>
> > src/android/meson.build | 1 +
> > 3 files changed, 168 insertions(+)
> > create mode 100644 src/android/libyuv/post_processor_libyuv.cpp
> > create mode 100644 src/android/libyuv/post_processor_libyuv.h
> >
> > diff --git a/src/android/libyuv/post_processor_libyuv.cpp b/src/android/libyuv/post_processor_libyuv.cpp
> > new file mode 100644
> > index 00000000..81f237e0
> > --- /dev/null
> > +++ b/src/android/libyuv/post_processor_libyuv.cpp
> > @@ -0,0 +1,123 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2021, Google Inc.
> > + *
> > + * post_processor_libyuv.cpp - Post Processor using libyuv
> > + */
> > +
> > +#include "post_processor_libyuv.h"
> > +
> > +#include <libyuv/scale.h>
> > +
> > +#include <libcamera/formats.h>
> > +#include <libcamera/geometry.h>
> > +#include <libcamera/internal/formats.h>
> > +#include <libcamera/internal/log.h>
> > +#include <libcamera/pixel_format.h>
> > +
> > +using namespace libcamera;
> > +
> > +LOG_DEFINE_CATEGORY(LIBYUV)
>
> Same question as the usage of 'lib' in class and file name.
>
> > +
> > +int PostProcessorLibyuv::configure(const StreamConfiguration &inCfg,
> > + const StreamConfiguration &outCfg)
> > +{
> > + if (inCfg.pixelFormat != outCfg.pixelFormat) {
> > + LOG(LIBYUV, Error)
> > + << "Pixel format conversion is not supported"
> > + << " (from " << inCfg.toString()
> > + << " to " << outCfg.toString() << ")";
> > + return -EINVAL;
> > + }
> > +
> > + if (inCfg.size < outCfg.size) {
> > + LOG(LIBYUV, Error) << "Up-scaling is not supported"
> > + << " (from " << inCfg.toString()
> > + << " to " << outCfg.toString() << ")";
> > + return -EINVAL;
> > + }
> > +
> > + if (inCfg.pixelFormat == formats::NV12) {
> > + LOG(LIBYUV, Error) << "Unsupported format " << inCfg.pixelFormat
> > + << " (only NV12 is supported)";
>
> Should the condition check for != insted of == ?
>
> > + return -EINVAL;
> > + }
> > +
> > + getNV12LengthAndStride(inCfg.size, sourceLength_, sourceStride_);
> > + getNV12LengthAndStride(outCfg.size, destinationLength_,
> > + destinationStride_);
> > + return 0;
> > +}
> > +
> > +int PostProcessorLibyuv::process(const FrameBuffer &source,
> > + libcamera::MappedBuffer *destination,
> > + [[maybe_unused]] const CameraMetadata &requestMetadata,
> > + [[maybe_unused]] CameraMetadata *metadata)
> > +{
> > + if (!isValidNV12Buffers(source, *destination))
> > + return -EINVAL;
> > +
> > + const MappedFrameBuffer sourceMapped(&source, PROT_READ);
> > + if (!sourceMapped.isValid()) {
> > + LOG(LIBYUV, Error) << "Failed to mmap camera frame buffer";
> > + return -EINVAL;
> > + }
> > +
> > + return 0 == libyuv::NV12Scale(sourceMapped.maps()[0].data(),
> > + sourceStride_[0],
> > + sourceMapped.maps()[1].data(),
> > + sourceStride_[1],
> > + sourceSize_.width, sourceSize_.height,
> > + destination->maps()[0].data(),
> > + destinationStride_[0],
> > + destination->maps()[1].data(),
> > + destinationStride_[1],
> > + destinationSize_.width,
> > + destinationSize_.height,
> > + libyuv::FilterMode::kFilterBilinear);
>
> Nice API libyuv! :/
>
> > +}
> > +
> > +bool PostProcessorLibyuv::isValidNV12Buffers(
>
> Please drop NV12 from name.
> This could be validateBuffers()
>
> > + const FrameBuffer &source,
>
> Fit on the previous line ?
>
> > + const libcamera::MappedBuffer &destination) const
> > +{
> > + if (source.planes().size() != 2u) {
> > + LOG(LIBYUV, Error) << "The number of source planes is not 2";
> > + return false;
> > + }
> > + if (destination.maps().size() != 2u) {
> > + LOG(LIBYUV, Error)
> > + << "The number of destination planes is not 2";
>
> Here and above
> "Invalid number of planes: " << destination.maps().size();
>
> > + return false;
> > + }
> > +
> > + if (source.planes()[0].length < sourceLength_[0] ||
> > + source.planes()[1].length < sourceLength_[1]) {
> > + LOG(LIBYUV, Error)
> > + << "The source planes lengths are too small";
>
> I would also print the actual size and the expected one to make the
> error more useful for debug.
>
> > + return false;
> > + }
> > + if (destination.maps()[0].size() < destinationLength_[0] ||
> > + destination.maps()[1].size() < destinationLength_[1]) {
> > + LOG(LIBYUV, Error)
> > + << "The destination planes lengths are too small";
> > + return false;
> > + }
> > +
> > + return true;
> > +}
> > +
> > +// static
>
> C++ comment, and not of much value. Why does checkstyle not complain ?
>
> > +void PostProcessorLibyuv::getNV12LengthAndStride(Size size,
> > + unsigned int length[2],
> > + unsigned int stride[2])
>
> Please drop NV12 from the function name, it will work for other
> formats too.
>
> The lenghts and strides are class members this functions has access
> to, see the below suggestion on how to avoid passing them in.
>
> > +{
> > + const PixelFormatInfo &nv12Info = PixelFormatInfo::info(formats::NV12);
>
> Pass in the whole StreamConfig and use the pixelformat there contained
> to retrieve the info, do not assume NV12 even if it's the only
> supported one, as as soon NV16 is added this will have to be changed.
>
> All in all, I would make this
>
> void PostProcessorLibyuv::calculateLengths(const StreamConfiguration &inCfg,
> const StreamConfiguration &outCfg)
> {
> }
>
> and initialize both [source|destination][Length_|Stride_] in one go.
> After all you call the function two times one after the other. This
> way you won't have to pass paramters in.
>
> > + for (unsigned int i = 0; i < 2; i++) {
> > + const unsigned int vertSubSample =
> > + nv12Info.planes[i].verticalSubSampling;
> > + length[i] = nv12Info.stride(size.width, i, /*align=*/1) *
>
> A comment inside a function call is rather unusual in the library code
> base.
>
> > + ((size.height + vertSubSample - 1) / vertSubSample);
>
> This might be a good occasion to create a PixelFormatInfo::planeSize()
> as we already have frameSize that performs the same calculation on all
> planes. Not required for this patch though.
>
> > + stride[i] = nv12Info.stride(size.width, i, 1);
> > + }
> > +}
> > diff --git a/src/android/libyuv/post_processor_libyuv.h b/src/android/libyuv/post_processor_libyuv.h
> > new file mode 100644
> > index 00000000..a65a16b3
> > --- /dev/null
> > +++ b/src/android/libyuv/post_processor_libyuv.h
> > @@ -0,0 +1,44 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2021, Google Inc.
> > + *
> > + * post_processor_libyuv.h - Post Processor using libyuv
> > + */
> > +#ifndef __ANDROID_POST_PROCESSOR_LIBYUV_H__
> > +#define __ANDROID_POST_PROCESSOR_LIBYUV_H__
> > +
> > +#include <libcamera/geometry.h>
> > +
> > +#include "../post_processor.h"
> > +
> > +class CameraDevice;
> > +
> > +class PostProcessorLibyuv : public PostProcessor
> > +{
> > +public:
> > + PostProcessorLibyuv() = default;
>
> This looks weird, but I recall you and Laurent discussed this lenght, so I
> presume it's ok.
>
> > +
> > + int configure(const libcamera::StreamConfiguration &incfg,
> > + const libcamera::StreamConfiguration &outcfg) override;
> > + int process(const libcamera::FrameBuffer &source,
> > + libcamera::MappedBuffer *destination,
> > + const CameraMetadata & /*requestMetadata*/,
>
> why is requestMetadata commented ?
>
> > + CameraMetadata *metadata) override;
> > +
> > +private:
> > + bool isValidNV12Buffers(const libcamera::FrameBuffer &source,
> > + const libcamera::MappedBuffer &destination) const;
> > +
> > + static void getNV12LengthAndStride(libcamera::Size size,
> > + unsigned int length[2],
> > + unsigned int stride[2]);
>
> Why static if it operates on class members ?
>
> > +
> > + libcamera::Size sourceSize_;
> > + libcamera::Size destinationSize_;
> > + unsigned int sourceLength_[2] = {};
> > + unsigned int destinationLength_[2] = {};
> > + unsigned int sourceStride_[2] = {};
> > + unsigned int destinationStride_[2] = {};
>
> Class member initialization is something we don't often do in
> libcamera. Not sure why, it's a C++11 feature, so we should have
> supported it from the very beginning. Not sure, just pointing it out.
>
> Thanks
> j
>
I addressed your comments locally. I wait for more comments from
others and will upload the patches tomorrow's evening in JST.
Best Regards,
-Hiro
> > +};
> > +
> > +#endif /* __ANDROID_POST_PROCESSOR_LIBYUV_H__ */
> > diff --git a/src/android/meson.build b/src/android/meson.build
> > index e1533d7c..cbe09200 100644
> > --- a/src/android/meson.build
> > +++ b/src/android/meson.build
> > @@ -44,6 +44,7 @@ android_hal_sources = files([
> > 'jpeg/exif.cpp',
> > 'jpeg/post_processor_jpeg.cpp',
> > 'jpeg/thumbnailer.cpp',
> > + 'libyuv/post_processor_libyuv.cpp'
> > ])
> >
> > android_camera_metadata_sources = files([
> > --
> > 2.30.0.365.g02bc693789-goog
> > _______________________________________________
> > 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