[libcamera-devel,v2,2/2] android: libyuv: Introduce PostProcessorLibyuv
diff mbox series

Message ID 20210127044425.2193480-3-hiroh@chromium.org
State Superseded
Headers show
Series
  • Introduce Post processor using libyuv
Related show

Commit Message

Hirokazu Honda Jan. 27, 2021, 4:44 a.m. UTC 
This adds PostProcessorLibyuv. It supports NV12 buffer scaling.

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

---
 src/android/libyuv/post_processor_libyuv.cpp | 126 +++++++++++++++++++
 src/android/libyuv/post_processor_libyuv.h   |  38 ++++++
 src/android/meson.build                      |   1 +
 3 files changed, 165 insertions(+)
 create mode 100644 src/android/libyuv/post_processor_libyuv.cpp
 create mode 100644 src/android/libyuv/post_processor_libyuv.h

--
2.30.0.280.ga3ce27912f-goog

Comments

Laurent Pinchart Jan. 27, 2021, 11:38 a.m. UTC | #1 
Hi Hiro,

Thank you for the patch.

On Wed, Jan 27, 2021 at 04:44:25AM +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 | 126 +++++++++++++++++++
>  src/android/libyuv/post_processor_libyuv.h   |  38 ++++++
>  src/android/meson.build                      |   1 +
>  3 files changed, 165 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..5f40fd25
> --- /dev/null
> +++ b/src/android/libyuv/post_processor_libyuv.cpp
> @@ -0,0 +1,126 @@
> +/* 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)
> +
> +namespace {

Missing blank line.

> +void getNV12LengthAndStride(Size size, unsigned int length[2],
> +			    unsigned int stride[2])
> +{
> +	const auto nv12Info = PixelFormatInfo::info(PixelFormat(formats::NV12));

formats::NV12 is already a PixelFormat, so you can write

	const auto nv12Info = PixelFormatInfo::info(formats::NV12);

I'd write out the type explicitly though:

	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);
> +	}
> +}

Missing blank line here too.

> +} // namespace

We could also make this a private static member function. For classes
exposed through the public API it could polute the public headers
unnecessarily, but for internal classes, it's less of an issue.

> +
> +PostProcessorLibyuv::PostProcessorLibyuv() = default;

Is this needed ? The compiler should generate a default constructor. You
could drop this line and the declaration of the constructor in the
class definition.

> +
> +int PostProcessorLibyuv::configure(const StreamConfiguration &inCfg,
> +				   const StreamConfiguration &outCfg)
> +{
> +	if (inCfg.pixelFormat != outCfg.pixelFormat) {
> +		LOG(LIBYUV, Error)
> +			<< "Pixel format conversion is not supported"
> +			<< "-In: " << inCfg.toString()
> +			<< "-Out: " << outCfg.toString();

This will print something like

	"Pixel format conversion is not supported-In: NV12-Out: NV24"

How about the following ?

		LOG(LIBYUV, Error)
			<< "Pixel format conversion is not supported"
			<< " (from " << inCfg.toString()
			<< " to " << outCfg.toString() << ")";

> +		return -EINVAL;
> +	}

Blank line.

> +	if (inCfg.size < outCfg.size) {
> +		LOG(LIBYUV, Error) << "Up-scaling is not supported"
> +				   << ", -In: " << inCfg.toString()
> +				   << ", -Out: " << outCfg.toString();

Same here for the message.

> +		return -EINVAL;
> +	}

Here too.

> +	if (inCfg.pixelFormat == formats::NV12) {
> +		LOG(LIBYUV, Error) << "Only NV12 is supported"
> +				   << ", -In: " << inCfg.toString()
> +				   << ", -Out: " << outCfg.toString();

		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,
> +				 const CameraMetadata & /*requestMetadata*/,
> +				 CameraMetadata * /*metadata*/)
> +{
> +	if (!IsValidNV12Buffers(source, *destination)) {
> +		LOG(LIBYUV, Error) << "Invalid source and destination";

I think you could drop this message as IsValidNV12Buffers() already logs
errors.

> +		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(

s/IsValidNV12Buffers/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;
> +}
> diff --git a/src/android/libyuv/post_processor_libyuv.h b/src/android/libyuv/post_processor_libyuv.h
> new file mode 100644
> index 00000000..40c635a1
> --- /dev/null
> +++ b/src/android/libyuv/post_processor_libyuv.h
> @@ -0,0 +1,38 @@
> +/* 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 "../post_processor.h"
> +
> +class CameraDevice;
> +
> +class PostProcessorLibyuv : public PostProcessor
> +{
> +public:
> +	PostProcessorLibyuv();
> +
> +	int configure(const libcamera::StreamConfiguration &incfg,
> +		      const libcamera::StreamConfiguration &outcfg) override;
> +	int process(const libcamera::FrameBuffer &source,
> +		    libcamera::MappedBuffer *destination,
> +		    const CameraMetadata & /*requestMetadata*/,

You can use [[maybe_unused]].

> +		    CameraMetadata *metadata) override;
> +
> +private:
> +	bool IsValidNV12Buffers(const libcamera::FrameBuffer &source,
> +				const libcamera::MappedBuffer &destination) const;
> +
> +	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 3d4d3be4..ff067d12 100644
> --- a/src/android/meson.build
> +++ b/src/android/meson.build
> @@ -26,6 +26,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([
Hirokazu Honda Jan. 28, 2021, 12:52 a.m. UTC | #2 
Hi Laurent,

Thanks for reviewing.

On Wed, Jan 27, 2021 at 8:39 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Hiro,
>
> Thank you for the patch.
>
> On Wed, Jan 27, 2021 at 04:44:25AM +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 | 126 +++++++++++++++++++
> >  src/android/libyuv/post_processor_libyuv.h   |  38 ++++++
> >  src/android/meson.build                      |   1 +
> >  3 files changed, 165 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..5f40fd25
> > --- /dev/null
> > +++ b/src/android/libyuv/post_processor_libyuv.cpp
> > @@ -0,0 +1,126 @@
> > +/* 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)
> > +
> > +namespace {
>
> Missing blank line.
>
> > +void getNV12LengthAndStride(Size size, unsigned int length[2],
> > +                         unsigned int stride[2])
> > +{
> > +     const auto nv12Info = PixelFormatInfo::info(PixelFormat(formats::NV12));
>
> formats::NV12 is already a PixelFormat, so you can write
>
>         const auto nv12Info = PixelFormatInfo::info(formats::NV12);
>
> I'd write out the type explicitly though:
>
>         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);
> > +     }
> > +}
>
> Missing blank line here too.
>
> > +} // namespace
>
> We could also make this a private static member function. For classes
> exposed through the public API it could polute the public headers
> unnecessarily, but for internal classes, it's less of an issue.
>
> > +
> > +PostProcessorLibyuv::PostProcessorLibyuv() = default;
>
> Is this needed ? The compiler should generate a default constructor. You
> could drop this line and the declaration of the constructor in the
> class definition.
>

Done.
The answer is up to our code policy. I am used to obey this chromium
coding style rule.
 In this case, using default in the header file is okay as the class
is sufficiently simple.
https://www.chromium.org/developers/coding-style/chromium-style-checker-errors#TOC-Constructor-Destructor-Errors

Best Regards,
-Hiro


> > +
> > +int PostProcessorLibyuv::configure(const StreamConfiguration &inCfg,
> > +                                const StreamConfiguration &outCfg)
> > +{
> > +     if (inCfg.pixelFormat != outCfg.pixelFormat) {
> > +             LOG(LIBYUV, Error)
> > +                     << "Pixel format conversion is not supported"
> > +                     << "-In: " << inCfg.toString()
> > +                     << "-Out: " << outCfg.toString();
>
> This will print something like
>
>         "Pixel format conversion is not supported-In: NV12-Out: NV24"
>
> How about the following ?
>
>                 LOG(LIBYUV, Error)
>                         << "Pixel format conversion is not supported"
>                         << " (from " << inCfg.toString()
>                         << " to " << outCfg.toString() << ")";
>
> > +             return -EINVAL;
> > +     }
>
> Blank line.
>
> > +     if (inCfg.size < outCfg.size) {
> > +             LOG(LIBYUV, Error) << "Up-scaling is not supported"
> > +                                << ", -In: " << inCfg.toString()
> > +                                << ", -Out: " << outCfg.toString();
>
> Same here for the message.
>
> > +             return -EINVAL;
> > +     }
>
> Here too.
>
> > +     if (inCfg.pixelFormat == formats::NV12) {
> > +             LOG(LIBYUV, Error) << "Only NV12 is supported"
> > +                                << ", -In: " << inCfg.toString()
> > +                                << ", -Out: " << outCfg.toString();
>
>                 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,
> > +                              const CameraMetadata & /*requestMetadata*/,
> > +                              CameraMetadata * /*metadata*/)
> > +{
> > +     if (!IsValidNV12Buffers(source, *destination)) {
> > +             LOG(LIBYUV, Error) << "Invalid source and destination";
>
> I think you could drop this message as IsValidNV12Buffers() already logs
> errors.
>
> > +             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(
>
> s/IsValidNV12Buffers/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;
> > +}
> > diff --git a/src/android/libyuv/post_processor_libyuv.h b/src/android/libyuv/post_processor_libyuv.h
> > new file mode 100644
> > index 00000000..40c635a1
> > --- /dev/null
> > +++ b/src/android/libyuv/post_processor_libyuv.h
> > @@ -0,0 +1,38 @@
> > +/* 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 "../post_processor.h"
> > +
> > +class CameraDevice;
> > +
> > +class PostProcessorLibyuv : public PostProcessor
> > +{
> > +public:
> > +     PostProcessorLibyuv();
> > +
> > +     int configure(const libcamera::StreamConfiguration &incfg,
> > +                   const libcamera::StreamConfiguration &outcfg) override;
> > +     int process(const libcamera::FrameBuffer &source,
> > +                 libcamera::MappedBuffer *destination,
> > +                 const CameraMetadata & /*requestMetadata*/,
>
> You can use [[maybe_unused]].
>
> > +                 CameraMetadata *metadata) override;
> > +
> > +private:
> > +     bool IsValidNV12Buffers(const libcamera::FrameBuffer &source,
> > +                             const libcamera::MappedBuffer &destination) const;
> > +
> > +     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 3d4d3be4..ff067d12 100644
> > --- a/src/android/meson.build
> > +++ b/src/android/meson.build
> > @@ -26,6 +26,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([
>
> --
> Regards,
>
> Laurent Pinchart
Laurent Pinchart Jan. 28, 2021, 3:50 p.m. UTC | #3 
Hi Hiro,

On Thu, Jan 28, 2021 at 09:52:46AM +0900, Hirokazu Honda wrote:
> On Wed, Jan 27, 2021 at 8:39 PM Laurent Pinchart wrote:
> > On Wed, Jan 27, 2021 at 04:44:25AM +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 | 126 +++++++++++++++++++
> > >  src/android/libyuv/post_processor_libyuv.h   |  38 ++++++
> > >  src/android/meson.build                      |   1 +
> > >  3 files changed, 165 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..5f40fd25
> > > --- /dev/null
> > > +++ b/src/android/libyuv/post_processor_libyuv.cpp
> > > @@ -0,0 +1,126 @@
> > > +/* 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)
> > > +
> > > +namespace {
> >
> > Missing blank line.
> >
> > > +void getNV12LengthAndStride(Size size, unsigned int length[2],
> > > +                         unsigned int stride[2])
> > > +{
> > > +     const auto nv12Info = PixelFormatInfo::info(PixelFormat(formats::NV12));
> >
> > formats::NV12 is already a PixelFormat, so you can write
> >
> >         const auto nv12Info = PixelFormatInfo::info(formats::NV12);
> >
> > I'd write out the type explicitly though:
> >
> >         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);
> > > +     }
> > > +}
> >
> > Missing blank line here too.
> >
> > > +} // namespace
> >
> > We could also make this a private static member function. For classes
> > exposed through the public API it could polute the public headers
> > unnecessarily, but for internal classes, it's less of an issue.
> >
> > > +
> > > +PostProcessorLibyuv::PostProcessorLibyuv() = default;
> >
> > Is this needed ? The compiler should generate a default constructor. You
> > could drop this line and the declaration of the constructor in the
> > class definition.
> 
> Done.
> The answer is up to our code policy. I am used to obey this chromium
> coding style rule.
>  In this case, using default in the header file is okay as the class
> is sufficiently simple.
> https://www.chromium.org/developers/coding-style/chromium-style-checker-errors#TOC-Constructor-Destructor-Errors

Thanks for sharing the information, it's useful. Constructors (and
destructors) can definitely be large, even if a class looks simple (it's
one of the beauties of C++ :-)), and the fact that functions defined
inline in the class definition are inline in C++, and thus duplicated in
different compilation units, even without explicit usage of the inline
keyword, can easily be overlooked and lead to code
size increase.

I was however under the impression that defaulted constructors were not
duplicated, but after reading the coding style guide, it occurred to me
that it could have been an incorrect assumption. I've written this very
simple test:

---------- a.h --------
#pragma once

void a(const char *name);

---------- b.h --------
#pragma once

void b(unsigned int value);

---------- foo.h --------
#pragma once

#include <string>

class Foo
{
public:
	const std::string &name() const;
	void setName(const std::string &name);

private:
	std::string name_;
};

---------- a.cpp --------
#include <iostream>

#include "foo.h"

void a(const char *name)
{
	Foo f;

	f.setName(name);
	std::cout << f.name() << std::endl;
}

---------- b.cpp --------
#include <iostream>

#include "foo.h"

void b(unsigned int value)
{
	Foo f;

	f.setName(std::to_string(value));
	std::cout << f.name() << std::endl;
}

---------- foo.cpp --------
#include "foo.h"

const std::string &Foo::name() const
{
	return name_;
}

void Foo::setName(const std::string &name)
{
	name_ = name;
}

---------- main.cpp --------
#include "a.h"
#include "b.h"

int main()
{
	a("bar");
	b(42);

	return 0;
}

---------- Makefile --------

all: main

main: a.o b.o foo.o main.o
	g++ -W -Wall -o $@ $^
%.o: %.cpp
	g++ -W -Wall -c -o $@ $<


Looking at the output of objdump -d -C main, I see a single occurrence
of Foo::Foo(), with two calls to the function, one in a() and one in
b(). It thus looks like the compiler doesn't inline the implicitly
defined default constructor. Changing the Foo class definition to

class Foo
{
public:
	Foo() = default;

	const std::string &name() const;
	void setName(const std::string &name);

private:
	std::string name_;
};

doesn't change the situation, and quite interestingly, neither does

class Foo
{
public:
	Foo()
	{
	}

	const std::string &name() const;
	void setName(const std::string &name);

private:
	std::string name_;
};

I wonder what I'm missing, and if my test is incorrect :-)

> > > +
> > > +int PostProcessorLibyuv::configure(const StreamConfiguration &inCfg,
> > > +                                const StreamConfiguration &outCfg)
> > > +{
> > > +     if (inCfg.pixelFormat != outCfg.pixelFormat) {
> > > +             LOG(LIBYUV, Error)
> > > +                     << "Pixel format conversion is not supported"
> > > +                     << "-In: " << inCfg.toString()
> > > +                     << "-Out: " << outCfg.toString();
> >
> > This will print something like
> >
> >         "Pixel format conversion is not supported-In: NV12-Out: NV24"
> >
> > How about the following ?
> >
> >                 LOG(LIBYUV, Error)
> >                         << "Pixel format conversion is not supported"
> >                         << " (from " << inCfg.toString()
> >                         << " to " << outCfg.toString() << ")";
> >
> > > +             return -EINVAL;
> > > +     }
> >
> > Blank line.
> >
> > > +     if (inCfg.size < outCfg.size) {
> > > +             LOG(LIBYUV, Error) << "Up-scaling is not supported"
> > > +                                << ", -In: " << inCfg.toString()
> > > +                                << ", -Out: " << outCfg.toString();
> >
> > Same here for the message.
> >
> > > +             return -EINVAL;
> > > +     }
> >
> > Here too.
> >
> > > +     if (inCfg.pixelFormat == formats::NV12) {
> > > +             LOG(LIBYUV, Error) << "Only NV12 is supported"
> > > +                                << ", -In: " << inCfg.toString()
> > > +                                << ", -Out: " << outCfg.toString();
> >
> >                 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,
> > > +                              const CameraMetadata & /*requestMetadata*/,
> > > +                              CameraMetadata * /*metadata*/)
> > > +{
> > > +     if (!IsValidNV12Buffers(source, *destination)) {
> > > +             LOG(LIBYUV, Error) << "Invalid source and destination";
> >
> > I think you could drop this message as IsValidNV12Buffers() already logs
> > errors.
> >
> > > +             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(
> >
> > s/IsValidNV12Buffers/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;
> > > +}
> > > diff --git a/src/android/libyuv/post_processor_libyuv.h b/src/android/libyuv/post_processor_libyuv.h
> > > new file mode 100644
> > > index 00000000..40c635a1
> > > --- /dev/null
> > > +++ b/src/android/libyuv/post_processor_libyuv.h
> > > @@ -0,0 +1,38 @@
> > > +/* 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 "../post_processor.h"
> > > +
> > > +class CameraDevice;
> > > +
> > > +class PostProcessorLibyuv : public PostProcessor
> > > +{
> > > +public:
> > > +     PostProcessorLibyuv();
> > > +
> > > +     int configure(const libcamera::StreamConfiguration &incfg,
> > > +                   const libcamera::StreamConfiguration &outcfg) override;
> > > +     int process(const libcamera::FrameBuffer &source,
> > > +                 libcamera::MappedBuffer *destination,
> > > +                 const CameraMetadata & /*requestMetadata*/,
> >
> > You can use [[maybe_unused]].
> >
> > > +                 CameraMetadata *metadata) override;
> > > +
> > > +private:
> > > +     bool IsValidNV12Buffers(const libcamera::FrameBuffer &source,
> > > +                             const libcamera::MappedBuffer &destination) const;
> > > +
> > > +     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 3d4d3be4..ff067d12 100644
> > > --- a/src/android/meson.build
> > > +++ b/src/android/meson.build
> > > @@ -26,6 +26,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([
Hirokazu Honda Jan. 28, 2021, 10:38 p.m. UTC | #4 
On Fri, Jan 29, 2021 at 12:51 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Hiro,
>
> On Thu, Jan 28, 2021 at 09:52:46AM +0900, Hirokazu Honda wrote:
> > On Wed, Jan 27, 2021 at 8:39 PM Laurent Pinchart wrote:
> > > On Wed, Jan 27, 2021 at 04:44:25AM +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 | 126 +++++++++++++++++++
> > > >  src/android/libyuv/post_processor_libyuv.h   |  38 ++++++
> > > >  src/android/meson.build                      |   1 +
> > > >  3 files changed, 165 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..5f40fd25
> > > > --- /dev/null
> > > > +++ b/src/android/libyuv/post_processor_libyuv.cpp
> > > > @@ -0,0 +1,126 @@
> > > > +/* 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)
> > > > +
> > > > +namespace {
> > >
> > > Missing blank line.
> > >
> > > > +void getNV12LengthAndStride(Size size, unsigned int length[2],
> > > > +                         unsigned int stride[2])
> > > > +{
> > > > +     const auto nv12Info = PixelFormatInfo::info(PixelFormat(formats::NV12));
> > >
> > > formats::NV12 is already a PixelFormat, so you can write
> > >
> > >         const auto nv12Info = PixelFormatInfo::info(formats::NV12);
> > >
> > > I'd write out the type explicitly though:
> > >
> > >         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);
> > > > +     }
> > > > +}
> > >
> > > Missing blank line here too.
> > >
> > > > +} // namespace
> > >
> > > We could also make this a private static member function. For classes
> > > exposed through the public API it could polute the public headers
> > > unnecessarily, but for internal classes, it's less of an issue.
> > >
> > > > +
> > > > +PostProcessorLibyuv::PostProcessorLibyuv() = default;
> > >
> > > Is this needed ? The compiler should generate a default constructor. You
> > > could drop this line and the declaration of the constructor in the
> > > class definition.
> >
> > Done.
> > The answer is up to our code policy. I am used to obey this chromium
> > coding style rule.
> >  In this case, using default in the header file is okay as the class
> > is sufficiently simple.
> > https://www.chromium.org/developers/coding-style/chromium-style-checker-errors#TOC-Constructor-Destructor-Errors
>
> Thanks for sharing the information, it's useful. Constructors (and
> destructors) can definitely be large, even if a class looks simple (it's
> one of the beauties of C++ :-)), and the fact that functions defined
> inline in the class definition are inline in C++, and thus duplicated in
> different compilation units, even without explicit usage of the inline
> keyword, can easily be overlooked and lead to code
> size increase.
>
> I was however under the impression that defaulted constructors were not
> duplicated, but after reading the coding style guide, it occurred to me
> that it could have been an incorrect assumption. I've written this very
> simple test:
>
> ---------- a.h --------
> #pragma once
>
> void a(const char *name);
>
> ---------- b.h --------
> #pragma once
>
> void b(unsigned int value);
>
> ---------- foo.h --------
> #pragma once
>
> #include <string>
>
> class Foo
> {
> public:
>         const std::string &name() const;
>         void setName(const std::string &name);
>
> private:
>         std::string name_;
> };
>
> ---------- a.cpp --------
> #include <iostream>
>
> #include "foo.h"
>
> void a(const char *name)
> {
>         Foo f;
>
>         f.setName(name);
>         std::cout << f.name() << std::endl;
> }
>
> ---------- b.cpp --------
> #include <iostream>
>
> #include "foo.h"
>
> void b(unsigned int value)
> {
>         Foo f;
>
>         f.setName(std::to_string(value));
>         std::cout << f.name() << std::endl;
> }
>
> ---------- foo.cpp --------
> #include "foo.h"
>
> const std::string &Foo::name() const
> {
>         return name_;
> }
>
> void Foo::setName(const std::string &name)
> {
>         name_ = name;
> }
>
> ---------- main.cpp --------
> #include "a.h"
> #include "b.h"
>
> int main()
> {
>         a("bar");
>         b(42);
>
>         return 0;
> }
>
> ---------- Makefile --------
>
> all: main
>
> main: a.o b.o foo.o main.o
>         g++ -W -Wall -o $@ $^
> %.o: %.cpp
>         g++ -W -Wall -c -o $@ $<
>
>
> Looking at the output of objdump -d -C main, I see a single occurrence
> of Foo::Foo(), with two calls to the function, one in a() and one in
> b(). It thus looks like the compiler doesn't inline the implicitly
> defined default constructor. Changing the Foo class definition to
>
> class Foo
> {
> public:
>         Foo() = default;
>
>         const std::string &name() const;
>         void setName(const std::string &name);
>
> private:
>         std::string name_;
> };
>
> doesn't change the situation, and quite interestingly, neither does
>
> class Foo
> {
> public:
>         Foo()
>         {
>         }
>
>         const std::string &name() const;
>         void setName(const std::string &name);
>
> private:
>         std::string name_;
> };
>
> I wonder what I'm missing, and if my test is incorrect :-)
>

The "inline" keyword is basically a compiler hint.
A function with "inline" is not necessarily inlined.
According to https://stackoverflow.com/a/21322, putting a function
within a class definition has the same effect as using "inline"
keyword.

Perhaps, does the result change with -O options?
I am happy to continue discussing here. Since this may not change the
code so much, let me upload the next patch series.
Thanks.

Best Regards,
-Hiro
> > > > +
> > > > +int PostProcessorLibyuv::configure(const StreamConfiguration &inCfg,
> > > > +                                const StreamConfiguration &outCfg)
> > > > +{
> > > > +     if (inCfg.pixelFormat != outCfg.pixelFormat) {
> > > > +             LOG(LIBYUV, Error)
> > > > +                     << "Pixel format conversion is not supported"
> > > > +                     << "-In: " << inCfg.toString()
> > > > +                     << "-Out: " << outCfg.toString();
> > >
> > > This will print something like
> > >
> > >         "Pixel format conversion is not supported-In: NV12-Out: NV24"
> > >
> > > How about the following ?
> > >
> > >                 LOG(LIBYUV, Error)
> > >                         << "Pixel format conversion is not supported"
> > >                         << " (from " << inCfg.toString()
> > >                         << " to " << outCfg.toString() << ")";
> > >
> > > > +             return -EINVAL;
> > > > +     }
> > >
> > > Blank line.
> > >
> > > > +     if (inCfg.size < outCfg.size) {
> > > > +             LOG(LIBYUV, Error) << "Up-scaling is not supported"
> > > > +                                << ", -In: " << inCfg.toString()
> > > > +                                << ", -Out: " << outCfg.toString();
> > >
> > > Same here for the message.
> > >
> > > > +             return -EINVAL;
> > > > +     }
> > >
> > > Here too.
> > >
> > > > +     if (inCfg.pixelFormat == formats::NV12) {
> > > > +             LOG(LIBYUV, Error) << "Only NV12 is supported"
> > > > +                                << ", -In: " << inCfg.toString()
> > > > +                                << ", -Out: " << outCfg.toString();
> > >
> > >                 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,
> > > > +                              const CameraMetadata & /*requestMetadata*/,
> > > > +                              CameraMetadata * /*metadata*/)
> > > > +{
> > > > +     if (!IsValidNV12Buffers(source, *destination)) {
> > > > +             LOG(LIBYUV, Error) << "Invalid source and destination";
> > >
> > > I think you could drop this message as IsValidNV12Buffers() already logs
> > > errors.
> > >
> > > > +             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(
> > >
> > > s/IsValidNV12Buffers/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;
> > > > +}
> > > > diff --git a/src/android/libyuv/post_processor_libyuv.h b/src/android/libyuv/post_processor_libyuv.h
> > > > new file mode 100644
> > > > index 00000000..40c635a1
> > > > --- /dev/null
> > > > +++ b/src/android/libyuv/post_processor_libyuv.h
> > > > @@ -0,0 +1,38 @@
> > > > +/* 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 "../post_processor.h"
> > > > +
> > > > +class CameraDevice;
> > > > +
> > > > +class PostProcessorLibyuv : public PostProcessor
> > > > +{
> > > > +public:
> > > > +     PostProcessorLibyuv();
> > > > +
> > > > +     int configure(const libcamera::StreamConfiguration &incfg,
> > > > +                   const libcamera::StreamConfiguration &outcfg) override;
> > > > +     int process(const libcamera::FrameBuffer &source,
> > > > +                 libcamera::MappedBuffer *destination,
> > > > +                 const CameraMetadata & /*requestMetadata*/,
> > >
> > > You can use [[maybe_unused]].
> > >
> > > > +                 CameraMetadata *metadata) override;
> > > > +
> > > > +private:
> > > > +     bool IsValidNV12Buffers(const libcamera::FrameBuffer &source,
> > > > +                             const libcamera::MappedBuffer &destination) const;
> > > > +
> > > > +     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 3d4d3be4..ff067d12 100644
> > > > --- a/src/android/meson.build
> > > > +++ b/src/android/meson.build
> > > > @@ -26,6 +26,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([
>
> --
> Regards,
>
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/src/android/libyuv/post_processor_libyuv.cpp b/src/android/libyuv/post_processor_libyuv.cpp
new file mode 100644
index 00000000..5f40fd25
--- /dev/null
+++ b/src/android/libyuv/post_processor_libyuv.cpp
@@ -0,0 +1,126 @@ 
+/* 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)
+
+namespace {
+void getNV12LengthAndStride(Size size, unsigned int length[2],
+			    unsigned int stride[2])
+{
+	const auto nv12Info = PixelFormatInfo::info(PixelFormat(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);
+	}
+}
+} // namespace
+
+PostProcessorLibyuv::PostProcessorLibyuv() = default;
+
+int PostProcessorLibyuv::configure(const StreamConfiguration &inCfg,
+				   const StreamConfiguration &outCfg)
+{
+	if (inCfg.pixelFormat != outCfg.pixelFormat) {
+		LOG(LIBYUV, Error)
+			<< "Pixel format conversion is not supported"
+			<< "-In: " << inCfg.toString()
+			<< "-Out: " << outCfg.toString();
+		return -EINVAL;
+	}
+	if (inCfg.size < outCfg.size) {
+		LOG(LIBYUV, Error) << "Up-scaling is not supported"
+				   << ", -In: " << inCfg.toString()
+				   << ", -Out: " << outCfg.toString();
+		return -EINVAL;
+	}
+	if (inCfg.pixelFormat == formats::NV12) {
+		LOG(LIBYUV, Error) << "Only NV12 is supported"
+				   << ", -In: " << inCfg.toString()
+				   << ", -Out: " << outCfg.toString();
+		return -EINVAL;
+	}
+
+	getNV12LengthAndStride(inCfg.size, sourceLength_, sourceStride_);
+	getNV12LengthAndStride(outCfg.size, destinationLength_,
+			       destinationStride_);
+	return 0;
+}
+
+int PostProcessorLibyuv::process(const FrameBuffer &source,
+				 libcamera::MappedBuffer *destination,
+				 const CameraMetadata & /*requestMetadata*/,
+				 CameraMetadata * /*metadata*/)
+{
+	if (!IsValidNV12Buffers(source, *destination)) {
+		LOG(LIBYUV, Error) << "Invalid source and 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;
+}
diff --git a/src/android/libyuv/post_processor_libyuv.h b/src/android/libyuv/post_processor_libyuv.h
new file mode 100644
index 00000000..40c635a1
--- /dev/null
+++ b/src/android/libyuv/post_processor_libyuv.h
@@ -0,0 +1,38 @@ 
+/* 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 "../post_processor.h"
+
+class CameraDevice;
+
+class PostProcessorLibyuv : public PostProcessor
+{
+public:
+	PostProcessorLibyuv();
+
+	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;
+
+	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 3d4d3be4..ff067d12 100644
--- a/src/android/meson.build
+++ b/src/android/meson.build
@@ -26,6 +26,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([