[libcamera-devel] [PATCH v2 2/2] android: libyuv: Introduce PostProcessorLibyuv
Hirokazu Honda
hiroh at chromium.org
Thu Jan 28 23:38:22 CET 2021
On Fri, Jan 29, 2021 at 12:51 AM Laurent Pinchart
<laurent.pinchart at 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 at 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
More information about the libcamera-devel
mailing list