[libcamera-devel] [PATCH 2/2] android: libyuv: Introduce PostProcessorLibyuv
Hirokazu Honda
hiroh at chromium.org
Mon Jan 25 06:21:40 CET 2021
Hi Kieran,
Thanks for reviewing.
On Fri, Jan 22, 2021 at 10:05 PM Kieran Bingham
<kieran.bingham at ideasonboard.com> wrote:
>
> 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 see. I will do in the next patch.
>
> 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?
>
I don't yet have a patch to use this post processor.
I think I have to complete a mapping task, which I have been working
in parallel, to use this post processor for NV12 scaling.
I wonder if it is allowed to check in these patches whereas it is not used now.
If it is not good, I will hold on this post processor patch until the
mapping task is done.
>
>
> 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.
>
Sorry I may not get your point. Should I change the message to something?
>
> > + 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.
>
I am not sure how much I should trust the client.
I would like to keep the check because this class can write out of bounds.
Best Regards,
-Hiro
>
>
> > + 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
More information about the libcamera-devel
mailing list