[PATCH v11 4/7] libcamera: pipeline: Add test pattern for VirtualPipelineHandler
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Mon Sep 9 11:06:33 CEST 2024
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
> ---
> 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
> 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 ?
> +
> +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);
> + 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.
> + /* 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 ?
> +
> +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 ?
> + { 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
> +
> + /* Generate a template buffer of the color bar test pattern. */
> + void configure(const Size &size) override;
> +};
> +
> +class DiagonalLinesGenerator : public TestPatternGenerator
> +{
> +public:
> + DiagonalLinesGenerator();
ditto
> +
> + /* 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
> + }
>
> 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" ?
> - 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
> + 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
> + }
>
> 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
>
More information about the libcamera-devel
mailing list