[PATCH v11 4/7] libcamera: pipeline: Add test pattern for VirtualPipelineHandler
Cheng-Hao Yang
chenghaoyang at chromium.org
Tue Sep 10 06:50:08 CEST 2024
Hi Jacopo,
On Mon, Sep 9, 2024 at 5:06 PM Jacopo Mondi <jacopo.mondi at ideasonboard.com>
wrote:
> Hi
>
> On Sat, Sep 07, 2024 at 02:28:29PM GMT, Harvey Yang wrote:
> > From: Konami Shu <konamiz at google.com>
> >
> > Add a test pattern generator class hierarchy for the Virtual
> > pipeline handler.
> >
> > Implement two types of test patterns: color bars and diagonal lines
> > generator and use them in the Virtual pipeline handler.
> >
> > Add a dependency for libyuv to the build system to generate images
> > in NV12 format from the test pattern.
> >
> > Signed-off-by: Konami Shu <konamiz at google.com>
> > Co-developed-by: Harvey Yang <chenghaoyang at chromium.org>
> > Co-developed-by: Yunke Cao <yunkec at chromium.org>
> > Co-developed-by: Tomasz Figa <tfiga at chromium.org>
> >
> > libcamera: pipeline: Shift test pattern by 1 pixel left every frame
> >
> > - This write the buffer every frame
> > - Shifting makes the frame rate dropped from about 160 to 40
> >
> > Patchset1->2
> > - Use constant instead of using a magic number
> >
> > Patchset2->3
> > - Make shiftLeft() private
> >
> > Signed-off-by: Konami Shu <konamiz at google.com>
> > Co-developed-by: Harvey Yang <chenghaoyang at chromium.org>
> > Co-developed-by: Yunke Cao <yunkec at chromium.org>
> > Co-developed-by: Tomasz Figa <tfiga at chromium.org>
>
> This is not a well-formed commit message. When you squash two patches,
> make a single commit message out of them
>
Right, sorry. Updated.
>
> > ---
> > src/android/meson.build | 19 ---
> > .../pipeline/virtual/frame_generator.h | 29 ++++
> > src/libcamera/pipeline/virtual/meson.build | 3 +
> > .../virtual/test_pattern_generator.cpp | 140 ++++++++++++++++++
> > .../pipeline/virtual/test_pattern_generator.h | 57 +++++++
> > src/libcamera/pipeline/virtual/virtual.cpp | 38 ++++-
> > src/libcamera/pipeline/virtual/virtual.h | 7 +
> > src/meson.build | 19 +++
> > 8 files changed, 290 insertions(+), 22 deletions(-)
> > create mode 100644 src/libcamera/pipeline/virtual/frame_generator.h
> > create mode 100644
> src/libcamera/pipeline/virtual/test_pattern_generator.cpp
> > create mode 100644
> src/libcamera/pipeline/virtual/test_pattern_generator.h
> >
> > diff --git a/src/android/meson.build b/src/android/meson.build
> > index 68646120..6341ee8b 100644
> > --- a/src/android/meson.build
> > +++ b/src/android/meson.build
> > @@ -15,25 +15,6 @@ foreach dep : android_deps
> > endif
> > endforeach
> >
> > -libyuv_dep = dependency('libyuv', required : false)
> > -
> > -# Fallback to a subproject if libyuv isn't found, as it's typically not
> > -# provided by distributions.
> > -if not libyuv_dep.found()
> > - cmake = import('cmake')
> > -
> > - libyuv_vars = cmake.subproject_options()
> > - libyuv_vars.add_cmake_defines({'CMAKE_POSITION_INDEPENDENT_CODE':
> 'ON'})
> > - libyuv_vars.set_override_option('cpp_std', 'c++17')
> > - libyuv_vars.append_compile_args('cpp',
> > - '-Wno-sign-compare',
> > - '-Wno-unused-variable',
> > - '-Wno-unused-parameter')
> > - libyuv_vars.append_link_args('-ljpeg')
> > - libyuv = cmake.subproject('libyuv', options : libyuv_vars)
> > - libyuv_dep = libyuv.dependency('yuv')
> > -endif
> > -
>
> I thought moving this to the virtual/ directory would have break
> building Android without building the Virtual pipline. Turns out it
> doesn't, as iirc, all symbols are global in meson, so
>
Actually I moved it to `src/meson.build`, instead of to the `virtual/`
directory, so it's expected that `src/android/meson.build` finds
libyuv_dep again.
>
> > android_deps += [libyuv_dep]
>
> this is enough to pull-in libyuv as a dependency for android even if
> the definition is somewhere else!
>
> >
> > android_hal_sources = files([
> > diff --git a/src/libcamera/pipeline/virtual/frame_generator.h
> b/src/libcamera/pipeline/virtual/frame_generator.h
> > new file mode 100644
> > index 00000000..d8727b8f
> > --- /dev/null
> > +++ b/src/libcamera/pipeline/virtual/frame_generator.h
> > @@ -0,0 +1,29 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2024, Google Inc.
> > + *
> > + * frame_generator.h - Virtual cameras helper to generate frames
> > + */
> > +
> > +#pragma once
> > +
> > +#include <libcamera/framebuffer.h>
> > +#include <libcamera/geometry.h>
> > +
> > +namespace libcamera {
> > +
> > +class FrameGenerator
> > +{
> > +public:
> > + virtual ~FrameGenerator() = default;
> > +
> > + virtual void configure(const Size &size) = 0;
> > +
> > + virtual void generateFrame(const Size &size,
> > + const FrameBuffer *buffer) = 0;
> > +
> > +protected:
> > + FrameGenerator() {}
> > +};
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/libcamera/pipeline/virtual/meson.build
> b/src/libcamera/pipeline/virtual/meson.build
> > index ada1b335..0c537777 100644
> > --- a/src/libcamera/pipeline/virtual/meson.build
> > +++ b/src/libcamera/pipeline/virtual/meson.build
> > @@ -1,5 +1,8 @@
> > # SPDX-License-Identifier: CC0-1.0
> >
> > libcamera_internal_sources += files([
> > + 'test_pattern_generator.cpp',
> > 'virtual.cpp',
> > ])
> > +
> > +libcamera_deps += [libyuv_dep]
> > diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp
> b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp
> > new file mode 100644
> > index 00000000..7094818e
> > --- /dev/null
> > +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp
> > @@ -0,0 +1,140 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2024, Google Inc.
> > + *
> > + * test_pattern_generator.cpp - Derived class of FrameGenerator for
> > + * generating test patterns
> > + */
> > +
> > +#include "test_pattern_generator.h"
> > +
> > +#include <libcamera/base/log.h>
> > +
> > +#include "libcamera/internal/mapped_framebuffer.h"
> > +
> > +#include "libyuv/convert_from_argb.h"
>
> Why use the "" include directive variant ?
>
Updated to <> instead.
>
> > +
> > +namespace libcamera {
> > +
> > +LOG_DECLARE_CATEGORY(Virtual)
> > +
> > +static const unsigned int kARGBSize = 4;
> > +
> > +void TestPatternGenerator::generateFrame(const Size &size,
> > + const FrameBuffer *buffer)
> > +{
> > + MappedFrameBuffer mappedFrameBuffer(buffer,
> > +
> MappedFrameBuffer::MapFlag::Write);
> > +
> > + auto planes = mappedFrameBuffer.planes();
> > +
> > + shiftLeft(size);
> > +
> > + /* Convert the template_ to the frame buffer */
> > + int ret = libyuv::ARGBToNV12(
> > + template_.get(), size.width * kARGBSize,
> > + planes[0].begin(), size.width,
> > + planes[1].begin(), size.width,
> > + size.width, size.height);
>
> You can easily use a more conventional indendation style
>
> int ret = libyuv::ARGBToNV12(template_.get(), size.width *
> kARGBSize,
> planes[0].begin(), size.width,
> planes[1].begin(), size.width,
> size.width, size.height);
>
>
Done.
> > + if (ret != 0)
> > + LOG(Virtual, Error) << "ARGBToNV12() failed with " << ret;
> > +}
> > +
> > +void TestPatternGenerator::shiftLeft(const Size &size)
> > +{
>
> This function basically copies the whole pattern at every frame.
> As this is a testing pipeline I think it's fine, but it's certainly
> consuming.
>
Yeah... We could probably come up with an approach to swap
only one column, while libyuv::ARGBToNV12 seems to require
continuous memory for the source, let's leave it as is for now.
>
> > + /* Store the first column temporarily */
> > + auto firstColumn = std::make_unique<uint8_t[]>(size.height *
> kARGBSize);
> > + for (size_t h = 0; h < size.height; h++) {
> > + unsigned int index = h * size.width * kARGBSize;
> > + unsigned int index1 = h * kARGBSize;
> > + firstColumn[index1] = template_[index];
> > + firstColumn[index1 + 1] = template_[index + 1];
> > + firstColumn[index1 + 2] = template_[index + 2];
> > + firstColumn[index1 + 3] = 0x00;
> > + }
> > +
> > + /* Overwrite template_ */
> > + uint8_t *buf = template_.get();
> > + for (size_t h = 0; h < size.height; h++) {
> > + for (size_t w = 0; w < size.width - 1; w++) {
> > + /* Overwrite with the pixel on the right */
> > + unsigned int index = (h * size.width + w + 1) *
> kARGBSize;
> > + *buf++ = template_[index]; // B
> > + *buf++ = template_[index + 1]; // G
> > + *buf++ = template_[index + 2]; // R
> > + *buf++ = 0x00; // A
> > + }
> > + /* Overwrite the new last column with the original first
> column */
> > + unsigned int index1 = h * kARGBSize;
> > + *buf++ = firstColumn[index1]; // B
> > + *buf++ = firstColumn[index1 + 1]; // G
> > + *buf++ = firstColumn[index1 + 2]; // R
> > + *buf++ = 0x00; // A
> > + }
> > +}
> > +
> > +ColorBarsGenerator::ColorBarsGenerator() = default;
>
> Do you need this and the below one ?
>
>
Removed the declarations and definitions.
> > +
> > +void ColorBarsGenerator::configure(const Size &size)
> > +{
> > + constexpr uint8_t kColorBar[8][3] = {
> > + // R, G, B
>
> Could you please make sure, once and for all, that in your next
> patches there won't be // comments ?
>
>
Yeah sorry. Updated.
> > + { 0xff, 0xff, 0xff }, // White
> > + { 0xff, 0xff, 0x00 }, // Yellow
> > + { 0x00, 0xff, 0xff }, // Cyan
> > + { 0x00, 0xff, 0x00 }, // Green
> > + { 0xff, 0x00, 0xff }, // Magenta
> > + { 0xff, 0x00, 0x00 }, // Red
> > + { 0x00, 0x00, 0xff }, // Blue
> > + { 0x00, 0x00, 0x00 }, // Black
> > + };
> > +
> > + template_ = std::make_unique<uint8_t[]>(
> > + size.width * size.height * kARGBSize);
> > +
> > + unsigned int colorBarWidth = size.width / std::size(kColorBar);
> > +
> > + uint8_t *buf = template_.get();
> > + for (size_t h = 0; h < size.height; h++) {
> > + for (size_t w = 0; w < size.width; w++) {
> > + /* Repeat when the width is exceed */
> > + unsigned int index = (w / colorBarWidth) %
> std::size(kColorBar);
> > +
> > + *buf++ = kColorBar[index][2]; /* B */
> > + *buf++ = kColorBar[index][1]; /* G */
> > + *buf++ = kColorBar[index][0]; /* R */
> > + *buf++ = 0x00; /* A */
> > + }
> > + }
> > +}
> > +
> > +DiagonalLinesGenerator::DiagonalLinesGenerator() = default;
> > +
> > +void DiagonalLinesGenerator::configure(const Size &size)
> > +{
> > + constexpr uint8_t kColorBar[8][3] = {
> > + /* R, G, B */
> > + { 0xff, 0xff, 0xff }, /* White */
> > + { 0x00, 0x00, 0x00 }, /* Black */
> > + };
> > +
> > + template_ = std::make_unique<uint8_t[]>(
> > + size.width * size.height * kARGBSize);
> > +
> > + unsigned int lineWidth = size.width / 10;
> > +
> > + uint8_t *buf = template_.get();
> > + for (size_t h = 0; h < size.height; h++) {
> > + for (size_t w = 0; w < size.width; w++) {
> > + /* Repeat when the width is exceed */
> > + int index = ((w + h) / lineWidth) % 2;
> > +
> > + *buf++ = kColorBar[index][2]; /* B */
> > + *buf++ = kColorBar[index][1]; /* G */
> > + *buf++ = kColorBar[index][0]; /* R */
> > + *buf++ = 0x00; /* A */
> > + }
> > + }
> > +}
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.h
> b/src/libcamera/pipeline/virtual/test_pattern_generator.h
> > new file mode 100644
> > index 00000000..11bcb5f0
> > --- /dev/null
> > +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.h
> > @@ -0,0 +1,57 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2024, Google Inc.
> > + *
> > + * test_pattern_generator.h - Derived class of FrameGenerator for
> > + * generating test patterns
> > + */
> > +
> > +#pragma once
> > +
> > +#include <memory>
> > +
> > +#include <libcamera/framebuffer.h>
> > +#include <libcamera/geometry.h>
> > +
> > +#include "frame_generator.h"
> > +
> > +namespace libcamera {
> > +
> > +enum class TestPattern : char {
> > + ColorBars = 0,
> > + DiagonalLines = 1,
> > +};
> > +
> > +class TestPatternGenerator : public FrameGenerator
> > +{
> > +public:
> > + void generateFrame(const Size &size, const FrameBuffer *buffer)
> override;
> > +
> > +protected:
> > + /* Buffer of test pattern template */
> > + std::unique_ptr<uint8_t[]> template_;
> > +
> > +private:
> > + /* Shift the buffer by 1 pixel left each frame */
> > + void shiftLeft(const Size &size);
> > +};
> > +
> > +class ColorBarsGenerator : public TestPatternGenerator
> > +{
> > +public:
> > + ColorBarsGenerator();
>
> You can drop this and rely on the default constructor generated by the
> compiler
>
>
Done.
> > +
> > + /* Generate a template buffer of the color bar test pattern. */
> > + void configure(const Size &size) override;
> > +};
> > +
> > +class DiagonalLinesGenerator : public TestPatternGenerator
> > +{
> > +public:
> > + DiagonalLinesGenerator();
>
> ditto
>
>
Done.
> > +
> > + /* Generate a template buffer of the diagonal lines test pattern.
> */
> > + void configure(const Size &size) override;
> > +};
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/libcamera/pipeline/virtual/virtual.cpp
> b/src/libcamera/pipeline/virtual/virtual.cpp
> > index f85ec3dd..6e64aeee 100644
> > --- a/src/libcamera/pipeline/virtual/virtual.cpp
> > +++ b/src/libcamera/pipeline/virtual/virtual.cpp
> > @@ -173,8 +173,11 @@ int PipelineHandlerVirtual::configure(Camera
> *camera,
> > CameraConfiguration *config)
> > {
> > VirtualCameraData *data = cameraData(camera);
> > - for (size_t i = 0; i < config->size(); ++i)
> > + for (size_t i = 0; i < config->size(); ++i) {
> > config->at(i).setStream(&data->streamConfigs_[i].stream);
> > + data->streamConfigs_[i].frameGenerator->configure(
> > +
> data->streamConfigs_[i].stream.configuration().size);
>
> If you use enumerate() as previously suggested you will be able to
> access the StreamConfiguration more easily
>
Done, and this fixes a bug that
`data->streamConfigs_[i].stream.configuration()` hasn't been set yet.
Thanks!
>
> > + }
> >
> > return 0;
> > }
> > @@ -210,9 +213,24 @@ void
> PipelineHandlerVirtual::stopDevice([[maybe_unused]] Camera *camera)
> > int PipelineHandlerVirtual::queueRequestDevice([[maybe_unused]] Camera
> *camera,
> > Request *request)
> > {
> > + VirtualCameraData *data = cameraData(camera);
> > +
> > /* \todo Read from the virtual video if any. */
>
> What's this "virtual video" ?
>
>
It means the potential video support. In the next patches we support
image loading, while video loading is nice to have.
Maybe the todo comment is redundant?
> > - for (auto it : request->buffers())
> > - completeBuffer(request, it.second);
> > + for (auto const &[stream, buffer] : request->buffers()) {
> > + bool found = false;
> > + /* map buffer and fill test patterns */
> > + for (auto &streamConfig : data->streamConfigs_) {
>
> Now sure how VirtualCameraData::StreamConfig will grow in the next
> patches, but an std::map indexed by libcamera::Stream * migth be
> easier here
>
>
Hmm, as we assign the streams to StreamConfigurations based on the
order of the streams (VirtualCameraData::streamConfigs_), would it be
a bit weird to depend on the order of a map? Just curious :p
> > + if (stream == &streamConfig.stream) {
> > + found = true;
> > + streamConfig.frameGenerator->generateFrame(
> > + stream->configuration().size,
> buffer);
> > + completeBuffer(request, buffer);
> > + break;
> > + }
> > + }
> > + if (!found)
> > + LOG(Virtual, Fatal) << "Stream not defined";
>
> This can' happen, unless there's a big issue that should be spotted
> during the development phase. You can ASSERT() I presume
>
>
Done.
>
> > + }
> >
> > request->metadata().set(controls::SensorTimestamp,
> currentTimestamp());
> > completeRequest(request);
> > @@ -257,11 +275,25 @@ bool
> PipelineHandlerVirtual::match([[maybe_unused]] DeviceEnumerator *enumerator
> >
> > const std::string id = "Virtual0";
> > std::shared_ptr<Camera> camera = Camera::create(std::move(data),
> id, streams);
> > +
> > + initFrameGenerator(camera.get());
> > +
> > registerCamera(std::move(camera));
> >
> > return true;
> > }
> >
> > +void PipelineHandlerVirtual::initFrameGenerator(Camera *camera)
> > +{
> > + auto data = cameraData(camera);
> > + for (auto &streamConfig : data->streamConfigs_) {
> > + if (data->testPattern_ == TestPattern::DiagonalLines)
> > + streamConfig.frameGenerator =
> std::make_unique<DiagonalLinesGenerator>();
> > + else
> > + streamConfig.frameGenerator =
> std::make_unique<ColorBarsGenerator>();
> > + }
> > +}
> > +
> > REGISTER_PIPELINE_HANDLER(PipelineHandlerVirtual, "virtual")
> >
> > } /* namespace libcamera */
> > diff --git a/src/libcamera/pipeline/virtual/virtual.h
> b/src/libcamera/pipeline/virtual/virtual.h
> > index fb3dbcad..09d73051 100644
> > --- a/src/libcamera/pipeline/virtual/virtual.h
> > +++ b/src/libcamera/pipeline/virtual/virtual.h
> > @@ -16,6 +16,8 @@
> > #include "libcamera/internal/dma_buf_allocator.h"
> > #include "libcamera/internal/pipeline_handler.h"
> >
> > +#include "test_pattern_generator.h"
> > +
> > namespace libcamera {
> >
> > class VirtualCameraData : public Camera::Private
> > @@ -29,6 +31,7 @@ public:
> > };
> > struct StreamConfig {
> > Stream stream;
> > + std::unique_ptr<FrameGenerator> frameGenerator;
> > };
> >
> > VirtualCameraData(PipelineHandler *pipe,
> > @@ -36,6 +39,8 @@ public:
> >
> > ~VirtualCameraData() = default;
> >
> > + TestPattern testPattern_ = TestPattern::ColorBars;
> > +
> > const std::vector<Resolution> supportedResolutions_;
> > Size maxResolutionSize_;
> > Size minResolutionSize_;
> > @@ -83,6 +88,8 @@ private:
> > return static_cast<VirtualCameraData *>(camera->_d());
> > }
> >
> > + void initFrameGenerator(Camera *camera);
> > +
> > DmaBufAllocator dmaBufAllocator_;
> > };
> >
> > diff --git a/src/meson.build b/src/meson.build
> > index 165a77bb..91bea775 100644
> > --- a/src/meson.build
> > +++ b/src/meson.build
> > @@ -27,6 +27,25 @@ else
> > ipa_sign_module = false
> > endif
> >
> > +libyuv_dep = dependency('libyuv', required : false)
> > +
> > +# Fallback to a subproject if libyuv isn't found, as it's typically not
> > +# provided by distributions.
> > +if not libyuv_dep.found()
> > + cmake = import('cmake')
> > +
> > + libyuv_vars = cmake.subproject_options()
> > + libyuv_vars.add_cmake_defines({'CMAKE_POSITION_INDEPENDENT_CODE':
> 'ON'})
> > + libyuv_vars.set_override_option('cpp_std', 'c++17')
> > + libyuv_vars.append_compile_args('cpp',
> > + '-Wno-sign-compare',
> > + '-Wno-unused-variable',
> > + '-Wno-unused-parameter')
> > + libyuv_vars.append_link_args('-ljpeg')
> > + libyuv = cmake.subproject('libyuv', options : libyuv_vars)
> > + libyuv_dep = libyuv.dependency('yuv')
> > +endif
> > +
>
> I think it's better to have this in src/meson.build, yes
>
> Mostly minors
> Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
>
> Thanks
> j
>
>
> > # libcamera must be built first as a dependency to the other components.
> > subdir('libcamera')
> >
> > --
> > 2.46.0.469.g59c65b2a67-goog
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20240910/a439fbd8/attachment.htm>
More information about the libcamera-devel
mailing list