[PATCH v9 4/8] libcamera: pipeline: Add test pattern for VirtualPipelineHandler
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Tue Aug 27 18:35:36 CEST 2024
Hi Harvey, Konami
On Tue, Aug 20, 2024 at 04:23:35PM GMT, Harvey Yang wrote:
> From: Konami Shu <konamiz at google.com>
>
> - There are two test patterns: color bars and diagonal lines
> - Add class for generating test patterns
> - Add libyuv to build dependencies
> - Make VirtualPipelineHandler show the test pattern
> - Format the code
Could you make a commit message out of this please ? Like:
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.
>
> - Rename test_pattern to frame_generator
> - reflect comment
> - Fix const variable name
> - Use #pragma once
> - Make configure() private
This list looks like a changelog more than a commit message. Could you
drop it or place it below
---
So that it doesn't appear in the git history ?
>
> 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>
> ---
> .../pipeline/virtual/frame_generator.h | 33 ++++++
> src/libcamera/pipeline/virtual/meson.build | 22 ++++
> .../virtual/test_pattern_generator.cpp | 112 ++++++++++++++++++
> .../pipeline/virtual/test_pattern_generator.h | 58 +++++++++
> src/libcamera/pipeline/virtual/virtual.cpp | 28 ++++-
> src/libcamera/pipeline/virtual/virtual.h | 8 ++
> 6 files changed, 258 insertions(+), 3 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/libcamera/pipeline/virtual/frame_generator.h b/src/libcamera/pipeline/virtual/frame_generator.h
> new file mode 100644
> index 000000000..9699af7a4
> --- /dev/null
> +++ b/src/libcamera/pipeline/virtual/frame_generator.h
> @@ -0,0 +1,33 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2023, 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;
> +
> + /* Create buffers for using them in `generateFrame` */
Usually we don't comment headers, but I don't mind
> + virtual void configure(const Size &size) = 0;
> +
> + /** Fill the output frame buffer.
> + * Use the frame at the frameCount of image frames
> + */
As long as the right commenting style is used: this is not Doxygen (no
/**) and I don't see references to frameCount anywhere ?
> + 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 ba7ff754e..e1e65e68d 100644
> --- a/src/libcamera/pipeline/virtual/meson.build
> +++ b/src/libcamera/pipeline/virtual/meson.build
> @@ -2,4 +2,26 @@
>
> libcamera_sources += files([
> 'virtual.cpp',
> + 'test_pattern_generator.cpp',
> ])
> +
> +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()
The Android HAL has exactly the same snippet. I wonder if it should be
moved to the main libcamera build file and only add libyuv as a
dependency in the HAL and here. It might require a bit of
experimentation
> + 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')
Do you need jpeg support ?
> + libyuv = cmake.subproject('libyuv', options : libyuv_vars)
> + libyuv_dep = libyuv.dependency('yuv')
> +endif
> +
> +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 000000000..8dfe626e5
> --- /dev/null
> +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp
> @@ -0,0 +1,112 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2023, 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"
> +
Empty line please
> +#include "libyuv/convert_from_argb.h"
> +namespace libcamera {
> +
> +LOG_DECLARE_CATEGORY(Virtual)
> +
> +static const unsigned int kARGBSize = 4;
> +
> +void TestPatternGenerator::generateFrame(
> + const Size &size,
> + const FrameBuffer *buffer)
Weird indent
void TestPatternGenerator::generateFrame(const Size &size,
const FrameBuffer *buffer)
> +{
> + MappedFrameBuffer mappedFrameBuffer(buffer,
> + MappedFrameBuffer::MapFlag::Write);
> +
> + auto planes = mappedFrameBuffer.planes();
> +
> + /* Convert the template_ to the frame buffer */
> + int ret = libyuv::ARGBToNV12(
As this produces NV12, the pipeline handler should only accept NV12
and adjust all other pixelformats. If I'm not mistaken this is not
enforced in validate() and you return SRGGB10 for Role::Raw (which you
shouldn't support afaict). This can cause issues as the buffers
allocated might be of a different size than the ones you expect ?
I agree that generating patterns in ARGB is easier than NV12, but I
wonder why not use RGB888 as the pipeline output format directly so
that you don't have to depend on libyuv at all.
> + template_.get(), /*src_stride_argb=*/size.width * kARGBSize,
Why a comment in the middle of the code ?
> + planes[0].begin(), size.width,
> + planes[1].begin(), size.width,
> + size.width, size.height);
> + if (ret != 0) {
> + LOG(Virtual, Error) << "ARGBToNV12() failed with " << ret;
> + }
No {} for single line statements
> +}
> +
> +std::unique_ptr<ColorBarsGenerator> ColorBarsGenerator::create()
> +{
> + return std::make_unique<ColorBarsGenerator>();
> +}
> +
> +void ColorBarsGenerator::configure(const Size &size)
> +{
> + constexpr uint8_t kColorBar[8][3] = {
> + // R, G, B
> + { 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
libcamera doesn't use C++ style comments
> + int index = (w / colorBarWidth) % std::size(kColorBar);
unsigned int
> +
> + *buf++ = kColorBar[index][2]; // B
> + *buf++ = kColorBar[index][1]; // G
> + *buf++ = kColorBar[index][0]; // R
> + *buf++ = 0x00; // A
> + }
> + }
> +}
> +
> +std::unique_ptr<DiagonalLinesGenerator> DiagonalLinesGenerator::create()
> +{
> + return std::make_unique<DiagonalLinesGenerator>();
> +}
> +
> +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;
same comments
> +
> + *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 000000000..ed8d4e43b
> --- /dev/null
> +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.h
> @@ -0,0 +1,58 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2023, 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
> +{
> +private:
> + void generateFrame(const Size &size,
> + const FrameBuffer *buffer) override;
Maybe I don't get this, but this overrides a public function, and it's
called from the pipeline, why private ?
> +
> +protected:
> + /* Shift the buffer by 1 pixel left each frame */
> + void shiftLeft(const Size &size);
Not implemented afaict
> + /* Buffer of test pattern template */
> + std::unique_ptr<uint8_t[]> template_;
> +};
> +
> +class ColorBarsGenerator : public TestPatternGenerator
> +{
> +public:
> + static std::unique_ptr<ColorBarsGenerator> create();
I won't question the class hierarchy design, but this could be done
with a simple public constructur and stored as unique_ptr in the
pipeline handler maybe.
> +
> +private:
> + /* Generate a template buffer of the color bar test pattern. */
> + void configure(const Size &size) override;
Same questions as the one on generateFrame, it's surely something I
don't well understand then
> +};
> +
> +class DiagonalLinesGenerator : public TestPatternGenerator
> +{
> +public:
> + static std::unique_ptr<DiagonalLinesGenerator> create();
> +
> +private:
> + /* 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 74eb8c7ad..357fdd035 100644
> --- a/src/libcamera/pipeline/virtual/virtual.cpp
> +++ b/src/libcamera/pipeline/virtual/virtual.cpp
> @@ -192,10 +192,14 @@ int PipelineHandlerVirtual::exportFrameBuffers(
> return dmaBufAllocator_.exportBuffers(config.bufferCount, planeSizes, buffers);
> }
>
> -int PipelineHandlerVirtual::start([[maybe_unused]] Camera *camera,
> +int PipelineHandlerVirtual::start(Camera *camera,
> [[maybe_unused]] const ControlList *controls)
> {
> /* \todo Start reading the virtual video if any. */
> + VirtualCameraData *data = cameraData(camera);
> +
> + data->frameGenerator_->configure(data->stream_.configuration().size);
Shouldn't this be done at Pipeline::configure() ?
> +
> return 0;
> }
>
> @@ -207,9 +211,14 @@ 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. */
> - for (auto it : request->buffers())
> - completeBuffer(request, it.second);
> + for (auto const &[stream, buffer] : request->buffers()) {
Unless something changes in the next patches, you only have one stream
> + /* map buffer and fill test patterns */
> + data->frameGenerator_->generateFrame(stream->configuration().size, buffer);
> + completeBuffer(request, buffer);
> + }
>
> request->metadata().set(controls::SensorTimestamp, currentTimestamp());
> completeRequest(request);
> @@ -241,11 +250,24 @@ bool PipelineHandlerVirtual::match([[maybe_unused]] DeviceEnumerator *enumerator
> std::set<Stream *> streams{ &data->stream_ };
> 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 false; // Prevent infinite loops for now
> }
>
> +void PipelineHandlerVirtual::initFrameGenerator(Camera *camera)
> +{
> + auto data = cameraData(camera);
> + if (data->testPattern_ == TestPattern::DiagonalLines) {
No {} for single lines statements
Who initializes data->testPattern_ ?
> + data->frameGenerator_ = DiagonalLinesGenerator::create();
> + } else {
> + data->frameGenerator_ = ColorBarsGenerator::create();
> + }
> +}
> +
> REGISTER_PIPELINE_HANDLER(PipelineHandlerVirtual, "virtual")
>
> } /* namespace libcamera */
> diff --git a/src/libcamera/pipeline/virtual/virtual.h b/src/libcamera/pipeline/virtual/virtual.h
> index 6fc6b34d8..fecd9fa6f 100644
> --- a/src/libcamera/pipeline/virtual/virtual.h
> +++ b/src/libcamera/pipeline/virtual/virtual.h
> @@ -13,6 +13,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,9 +31,13 @@ public:
>
> ~VirtualCameraData() = default;
>
> + TestPattern testPattern_;
> +
> std::vector<Resolution> supportedResolutions_;
>
> Stream stream_;
> +
> + std::unique_ptr<FrameGenerator> frameGenerator_;
> };
>
> class VirtualCameraConfiguration : public CameraConfiguration
> @@ -72,6 +78,8 @@ private:
> return static_cast<VirtualCameraData *>(camera->_d());
> }
>
> + void initFrameGenerator(Camera *camera);
> +
> DmaBufAllocator dmaBufAllocator_;
> };
>
> --
> 2.46.0.184.g6999bdac58-goog
>
More information about the libcamera-devel
mailing list