[libcamera-devel] [PATCH v3 2/2] android: libyuv: Introduce PostProcessorLibyuv
Jacopo Mondi
jacopo at jmondi.org
Mon Feb 1 12:53:45 CET 2021
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
> +};
> +
> +#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