[libcamera-devel] [PATCH 2/2] android: libyuv: Introduce PostProcessorLibyuv
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Jan 25 11:25:53 CET 2021
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 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