[libcamera-devel,v3,2/2] android: libyuv: Introduce PostProcessorLibyuv

11047 diff mbox series

This adds PostProcessorLibyuv. It supports NV12 buffer scaling.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>

---
 src/android/libyuv/post_processor_libyuv.cpp | 123 +++++++++++++++++++
 src/android/libyuv/post_processor_libyuv.h   |  44 +++++++
 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

--
2.30.0.365.g02bc693789-goog

Laurent, can you review this patch and merge if there is no issue?

Thanks so much in advance,

Best Regards,
-Hiro

On Fri, Jan 29, 2021 at 7:42 AM Hirokazu Honda <hiroh@chromium.org> wrote:
>
> This adds PostProcessorLibyuv. It supports NV12 buffer scaling.
>
> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
>
> ---
>  src/android/libyuv/post_processor_libyuv.cpp | 123 +++++++++++++++++++
>  src/android/libyuv/post_processor_libyuv.h   |  44 +++++++
>  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)
> +
> +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)";
> +               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);
> +}
> +
> +bool PostProcessorLibyuv::isValidNV12Buffers(
> +       const FrameBuffer &source,
> +       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";
> +               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";
> +               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
> +void PostProcessorLibyuv::getNV12LengthAndStride(Size size,
> +                                                unsigned int length[2],
> +                                                unsigned int stride[2])
> +{
> +       const PixelFormatInfo &nv12Info = PixelFormatInfo::info(formats::NV12);
> +       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) *
> +                           ((size.height + vertSubSample - 1) / vertSubSample);
> +               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;
> +
> +       int configure(const libcamera::StreamConfiguration &incfg,
> +                     const libcamera::StreamConfiguration &outcfg) override;
> +       int process(const libcamera::FrameBuffer &source,
> +                   libcamera::MappedBuffer *destination,
> +                   const CameraMetadata & /*requestMetadata*/,
> +                   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]);
> +
> +       libcamera::Size sourceSize_;
> +       libcamera::Size destinationSize_;
> +       unsigned int sourceLength_[2] = {};
> +       unsigned int destinationLength_[2] = {};
> +       unsigned int sourceStride_[2] = {};
> +       unsigned int destinationStride_[2] = {};
> +};
> +
> +#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

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@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

> +};
> +
> +#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@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Jacopo,

Thanks for reviewing!

On Mon, Feb 1, 2021 at 8:53 PM Jacopo Mondi <jacopo@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@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@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel