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

[Kieran Bingham]

Hi Hiro,

When there's more than one patch, could you also add a cover letter
please? it helps grouping them, and provides a way to comment on the
whole series as well as individual patches.

It doesn't have to be extensive, just a title and a brief.


I'm happy to see this work being done, but there's no usage of this post
processor.

Do you have another patch in progress to do that which would become a
part of this series?



On 22/01/2021 09:23, 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 | 120 +++++++++++++++++++
>  src/android/libyuv/post_processor_libyuv.h   |  33 +++++
>  src/android/meson.build                      |   1 +
>  3 files changed, 154 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..7cfa0539
> --- /dev/null
> +++ b/src/android/libyuv/post_processor_libyuv.cpp
> @@ -0,0 +1,120 @@
> +/* 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 getNV12Length(Size size, unsigned int length[2])
> +{
> +	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);
> +	}
> +}
> +
> +unsigned int getNV12Stride(Size size, unsigned int i)
> +{
> +	auto nv12Info = PixelFormatInfo::info(PixelFormat(formats::NV12));
> +	return nv12Info.stride(size.width, i, 1);
> +}
> +} // namespace
> +
> +PostProcessorLibyuv::PostProcessorLibyuv() = default;
> +
> +int PostProcessorLibyuv::configure(const StreamConfiguration &inCfg,
> +				   const StreamConfiguration &outCfg)
> +{
> +	if (inCfg.pixelFormat != outCfg.pixelFormat ||
> +	    inCfg.size < outCfg.size) {
> +		LOG(LIBYUV, Error) << "Only down-scaling is supported";

This will report "Only down-scaling is supported" if someone tries to
use it to do pixel format conversion.


> +		return -EINVAL;
> +	}
> +	if (inCfg.pixelFormat == formats::NV12) {
> +		LOG(LIBYUV, Error) << "Only NV12 is supported";
> +		return -EINVAL;
> +	}
> +	inCfg_ = inCfg;
> +	outCfg_ = outCfg;
> +	return 0;
> +}
> +
> +int PostProcessorLibyuv::process(const FrameBuffer &source,
> +				 libcamera::MappedBuffer *destination,
> +				 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(),
> +				      getNV12Stride(inCfg_.size, 0),
> +				      sourceMapped.maps()[1].data(),
> +				      getNV12Stride(inCfg_.size, 1),
> +				      inCfg_.size.height, inCfg_.size.width,

> +				      destination->maps()[0].data(),
> +				      getNV12Stride(outCfg_.size, 0),
> +				      destination->maps()[1].data(),
> +				      getNV12Stride(outCfg_.size, 1),
> +				      outCfg_.size.width, outCfg_.size.height,

> +				      libyuv::FilterMode::kFilterBilinear);

Eeek, that's quite terse to read, but that's more of a restriction on
the libyuv() I expect.


> +}
> +
> +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;
> +	}
> +
> +	unsigned int sourceLength[2] = {};
> +	unsigned int destinationLength[2] = {};
> +	getNV12Length(inCfg_.size, sourceLength);
> +	getNV12Length(outCfg_.size, destinationLength);

I think these can be calculated once at configure() time rather than in
every frame.

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

I would expect that we have an element of 'we can trust the sizes of a
FrameBuffer()' ?


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

new line here.

That's a lot of checks for each buffer. Do we need to do that on every
buffer? I suspect we might need to because we don't know what buffers
we're given before they come in ...

But I wonder if we can make assumptions that they are accurate.



> +	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..860a664b
> --- /dev/null
> +++ b/src/android/libyuv/post_processor_libyuv.h
> @@ -0,0 +1,33 @@
> +/* 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,
> +		    CameraMetadata *metadata) override;
> +
> +private:
> +	bool IsValidNV12Buffers(const libcamera::FrameBuffer &source,
> +				const libcamera::MappedBuffer &destination) const;
> +
> +	libcamera::StreamConfiguration inCfg_;
> +	libcamera::StreamConfiguration outCfg_;
> +};
> +
> +#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([
> --
> 2.30.0.280.ga3ce27912f-goog
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
> 

-- 
Regards
--
Kieran