| Message ID | 20210122092335.254626-2-hiroh@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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'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? On 22/01/2021 09:23, Hirokazu Honda wrote: > This adds PostProcessorLibyuv. It supports NV12 buffer scaling. > > Signed-off-by: Hirokazu Honda <hiroh@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. > + 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. > + 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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel >
Hi Kieran, Thanks for reviewing. On Fri, Jan 22, 2021 at 10:05 PM Kieran Bingham <kieran.bingham@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@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@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > -- > Regards > -- > Kieran
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@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@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@lists.libcamera.org >>> https://lists.libcamera.org/listinfo/libcamera-devel >>> >> >> -- >> Regards >> -- >> Kieran
On Mon, Jan 25, 2021 at 7:25 PM Kieran Bingham <kieran.bingham@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@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@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@lists.libcamera.org > >>> https://lists.libcamera.org/listinfo/libcamera-devel > >>> > >> > >> -- > >> Regards > >> -- > >> Kieran > > -- > Regards > -- > Kieran
Hi Hiro, On 27/01/2021 04:18, Hirokazu Honda wrote: > On Mon, Jan 25, 2021 at 7:25 PM Kieran Bingham <snip> >>>> 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. Ok, thanks for the update. Ordinarily, I think it would be better for code to go in that is tested and known to be working. But this object will not regress other components, and will not cause breakage as long as it compiles. So if it helps you progress, and base your developments on top of it, with all review actions completed as normal, I think we can let this in before there is a user. I'm looking forwards to seeing how this will be used :-)
On Wed, Jan 27, 2021 at 6:52 PM Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Hi Hiro, > > On 27/01/2021 04:18, Hirokazu Honda wrote: > > On Mon, Jan 25, 2021 at 7:25 PM Kieran Bingham > <snip> > >>>> 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. > > Ok, thanks for the update. > > Ordinarily, I think it would be better for code to go in that is tested > and known to be working. But this object will not regress other > components, and will not cause breakage as long as it compiles. > > So if it helps you progress, and base your developments on top of it, > with all review actions completed as normal, I think we can let this in > before there is a user. > > I'm looking forwards to seeing how this will be used :-) Thanks Kieran for your patience and understanding! -Hiro > -- > Regards > -- > Kieran
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"; + 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); +} + +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); + + 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..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([
This adds PostProcessorLibyuv. It supports NV12 buffer scaling. Signed-off-by: Hirokazu Honda <hiroh@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 -- 2.30.0.280.ga3ce27912f-goog