[libcamera-devel] [PATCH 2/2] android: libyuv: Introduce PostProcessorLibyuv
Hirokazu Honda
hiroh at chromium.org
Wed Jan 27 05:18:41 CET 2021
On Mon, Jan 25, 2021 at 7:25 PM Kieran Bingham
<kieran.bingham at ideasonboard.com> wrote:
>
> Hi Hiro,
>
> On 25/01/2021 05:21, Hirokazu Honda wrote:
> > 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.
>
> Thank you, I find it helpful.
>
>
> >> 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.
>
> Have you tested the code at all?
> Or is it purely compile tested?
>
I haven't tested this code at all.
I only confirmed the compilation was successful.
>
> > 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.
>
> Ideally only code that is 'used' would be integrated, but at the least
> I'd like to know that it has been tested in some form.
>
> But we don't easily have a way to add unit tests to the android layer
> currently ... :-(
>
>
> > If it is not good, I will hold on this post processor patch until the
> > mapping task is done.
>
> Posting for review is certainly beneficial already.
>
> I was also hoping there was something I could try and test the code with.
>
>
> >> 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?
>
> I think it would be helpful to the reader of the error to diagnose what
> happened if either the error checking was split for size checking, and
> format checking.
>
>
> >>
> >>> + return -EINVAL;
> >>> + }
> >>> + if (inCfg.pixelFormat == formats::NV12) {
> >>> + LOG(LIBYUV, Error) << "Only NV12 is supported";
> >>> + return -EINVAL;
> >>> + }
>
> In fact, You could put the pixelformat check first, and combine it with
> the output format checking?
>
> maybe even printing the configration if it fails?:
>
> if (inCfg.pixelFormat != outCfg.pixelFormat != formats::NV12)
> {
> LOG(LIBYUV, Error) << "Only NV12 is supported"
> << " - In: " << inCfg.toString()
> << " - Out: " << inCfg.toString();
> return -EINVAL;
> }
>
> Or do you later expect libyuv to support pixel format conversion too?
>
>
> >>> + 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
>
> --
> Regards
> --
> Kieran
More information about the libcamera-devel
mailing list