[v9,4/8] libcamera: pipeline: Add test pattern for VirtualPipelineHandler

20972 diff mbox series

From: Konami Shu <konamiz@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

- Rename test_pattern to frame_generator
- reflect comment
	- Fix const variable name
	- Use #pragma once
	- Make configure() private

Signed-off-by: Konami Shu <konamiz@google.com>
Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
Co-developed-by: Yunke Cao <yunkec@chromium.org>
Co-developed-by: Tomasz Figa <tfiga@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

Hi Harvey, Konami

On Tue, Aug 20, 2024 at 04:23:35PM GMT, Harvey Yang wrote:
> From: Konami Shu <konamiz@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@google.com>
> Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
> Co-developed-by: Yunke Cao <yunkec@chromium.org>
> Co-developed-by: Tomasz Figa <tfiga@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
>

Thanks Jacopo,

On Tue, Aug 27, 2024 at 6:35 PM Jacopo Mondi <jacopo.mondi@ideasonboard.com>
wrote:

> Hi Harvey, Konami
>
> On Tue, Aug 20, 2024 at 04:23:35PM GMT, Harvey Yang wrote:
> > From: Konami Shu <konamiz@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.
>
> Adopted, thanks!


> >
> > - 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
>
Removed.


>
> ---
>
> So that it doesn't appear in the git history ?
>
> >
> > Signed-off-by: Konami Shu <konamiz@google.com>
> > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
> > Co-developed-by: Yunke Cao <yunkec@chromium.org>
> > Co-developed-by: Tomasz Figa <tfiga@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
>
> Right, I think Konami put it here because there's no .cpp file for this
virtual class. Removed.


> > +     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 ?
>
Removed.


>
> > +     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
>
Moved to `src/meson.build`. Please check.


>
> > +    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 ?
>
Well, in the 7th patch, yes. Also, as we try to use the same
dependency with Android adapter, how about keeping jpeg
here?


>
> > +    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
>
Done


>
> > +#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)
>
>
> Adopted.


> > +{
> > +     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


Updated in the 3rd patch to remove the raw stream and ensure formats::NV12
for each stream.


> ?
>
> 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.
>

I think it's because the Android adapter requires mostly NV12 [1]

[1]:
https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/upstream/src/android/camera_capabilities.cpp;l=68


>
>
> > +             template_.get(), /*src_stride_argb=*/size.width *
> kARGBSize,
>
> Why a comment in the middle of the code ?
>
Removed.


>
> > +             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
>
Done


>
> > +}
> > +
> > +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
>
Updated.


>
> > +                     int index = (w / colorBarWidth) %
> std::size(kColorBar);
>
> unsigned int
>
Done


>
> > +
> > +                     *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
>
Done


>
> > +
> > +                     *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 ?
>
Hmm, it should be public. Updated.


>
> > +
> > +protected:
> > +     /* Shift the buffer by 1 pixel left each frame */
> > +     void shiftLeft(const Size &size);
>
> Not implemented afaict
>
Removed


> > +     /* 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.
>
The hierarchy design is because the 6th patch introduces the shift,
which can be used on all test pattern generators.
And yes, a static create function is not needed. Updated.


>
> > +
> > +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() ?

Right, migrated. Only that the generators will prepare the source
images, which will be duplicated when applications like Android
adapters calls configure() multiple times.


>
> > +
> >       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
>
Let's consider multiple-stream support: I tried to enable multiple streams
in
the next version, while I failed to pass the unit test: multi_stream_test:
```

[295:43:43.910237144] [1441841]  INFO IPAManager ipa_manager.cpp:137 libcamera
is not installed. Adding
'/usr/local/google/home/chenghaoyang/Workspace/libca

mera/build/src/ipa' to the IPA search path

[295:43:43.914030564] [1441841]  INFO Camera camera_manager.cpp:325 libcamera
v0.3.1+79-8ca4f033-dirty (2024-08-29T19:18:51UTC)

[295:43:43.915493754] [1441844] ERROR DmaBufAllocator dma_buf_allocator.cpp:120
Could not open any dma-buf provider

[295:43:43.919118835] [1441841]  INFO IPAManager ipa_manager.cpp:137 libcamera
is not installed. Adding
'/usr/local/google/home/chenghaoyang/Workspace/libca

mera/build/src/ipa' to the IPA search path

[295:43:43.922245825] [1441841]  INFO Camera camera_manager.cpp:325 libcamera
v0.3.1+79-8ca4f033-dirty (2024-08-29T19:18:51UTC)

[295:43:43.922820175] [1441846] ERROR DmaBufAllocator dma_buf_allocator.cpp:120
Could not open any dma-buf provider

Unable to set the pipeline to the playing state.
```
gitlab pipeline link:
https://gitlab.freedesktop.org/chenghaoyang/libcamera/-/pipelines/1260412

Looking into `gsteramer_multi_stream_test.cpp`,
I still couldn't figure out what's wrong...
Do you know what's tested in this unit test? I know that
DmaBufAllocator isn't valid, while I added logs and didn't
find `PipelineHandlerVirtual::exportFrameBuffers` being
called.

I also need your opinion: I think there's nothing, except for the unit test
:p,
to stop Virtual Pipeline Handler from supporting multiple streams, while
do you think there should be a limitation?
I was setting the maximum to 3 for StillCapture, VideoRecording, and
ViewFinder, while you know there can be multiple streams as the same
StreamRole...


> > +             /* 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
>
Done


>
> Who initializes data->testPattern_ ?
>
Hmm, it'll be done in the next patch, when we read from the
configuration file...
In this patch though, it's using the default value when we create
CameraData in the match() function.


> > +             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
> >
>

Hi Jacopo,

On Sat, Aug 31, 2024 at 9:50 PM Jacopo Mondi <jacopo.mondi@ideasonboard.com>
wrote:

> Hi Harvey
>
> On Thu, Aug 29, 2024 at 09:57:38PM GMT, Cheng-Hao Yang wrote:
> > Thanks Jacopo,
> >
> > On Tue, Aug 27, 2024 at 6:35 PM Jacopo Mondi <
> jacopo.mondi@ideasonboard.com>
> > wrote:
> >
> > > Hi Harvey, Konami
> > >
> > > On Tue, Aug 20, 2024 at 04:23:35PM GMT, Harvey Yang wrote:
> > > > From: Konami Shu <konamiz@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.
> > >
> > > Adopted, thanks!
> >
> >
> > > >
> > > > - 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
> > >
> > Removed.
> >
> >
> > >
> > > ---
> > >
> > > So that it doesn't appear in the git history ?
> > >
> > > >
> > > > Signed-off-by: Konami Shu <konamiz@google.com>
> > > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
> > > > Co-developed-by: Yunke Cao <yunkec@chromium.org>
> > > > Co-developed-by: Tomasz Figa <tfiga@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
> > >
> > > Right, I think Konami put it here because there's no .cpp file for this
> > virtual class. Removed.
> >
> >
> > > > +     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 ?
> > >
> > Removed.
> >
> >
> > >
> > > > +     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
> > >
> > Moved to `src/meson.build`. Please check.
> >
> >
> > >
> > > > +    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 ?
> > >
> > Well, in the 7th patch, yes. Also, as we try to use the same
> > dependency with Android adapter, how about keeping jpeg
> > here?
> >
> >
> > >
> > > > +    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
> > >
> > Done
> >
> >
> > >
> > > > +#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)
> > >
> > >
> > > Adopted.
> >
> >
> > > > +{
> > > > +     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
> >
> >
> > Updated in the 3rd patch to remove the raw stream and ensure
> formats::NV12
> > for each stream.
> >
> >
> > > ?
> > >
> > > 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.
> > >
> >
> > I think it's because the Android adapter requires mostly NV12 [1]
> >
> > [1]:
> >
> https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/upstream/src/android/camera_capabilities.cpp;l=68
> >
> >
> > >
> > >
> > > > +             template_.get(), /*src_stride_argb=*/size.width *
> > > kARGBSize,
> > >
> > > Why a comment in the middle of the code ?
> > >
> > Removed.
> >
> >
> > >
> > > > +             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
> > >
> > Done
> >
> >
> > >
> > > > +}
> > > > +
> > > > +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
> > >
> > Updated.
> >
> >
> > >
> > > > +                     int index = (w / colorBarWidth) %
> > > std::size(kColorBar);
> > >
> > > unsigned int
> > >
> > Done
> >
> >
> > >
> > > > +
> > > > +                     *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
> > >
> > Done
> >
> >
> > >
> > > > +
> > > > +                     *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 ?
> > >
> > Hmm, it should be public. Updated.
> >
> >
> > >
> > > > +
> > > > +protected:
> > > > +     /* Shift the buffer by 1 pixel left each frame */
> > > > +     void shiftLeft(const Size &size);
> > >
> > > Not implemented afaict
> > >
> > Removed
> >
> >
> > > > +     /* 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.
> > >
> > The hierarchy design is because the 6th patch introduces the shift,
> > which can be used on all test pattern generators.
> > And yes, a static create function is not needed. Updated.
> >
> >
> > >
> > > > +
> > > > +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() ?
> >
> > Right, migrated. Only that the generators will prepare the source
> > images, which will be duplicated when applications like Android
> > adapters calls configure() multiple times.
> >
>
> I see. It might be reasonable then, could you plese record the reason
> with a comment ?
>
> >
> > >
> > > > +
> > > >       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
> > >
> > Let's consider multiple-stream support: I tried to enable multiple
> streams
> > in
> > the next version, while I failed to pass the unit test:
> multi_stream_test:
>
> Ah ok, that's answer my question about multi-stream support in this
> version
>
> > ```
> >
> > [295:43:43.910237144] [1441841]  INFO IPAManager ipa_manager.cpp:137
> libcamera
> > is not installed. Adding
> > '/usr/local/google/home/chenghaoyang/Workspace/libca
> >
> > mera/build/src/ipa' to the IPA search path
> >
> > [295:43:43.914030564] [1441841]  INFO Camera camera_manager.cpp:325
> libcamera
> > v0.3.1+79-8ca4f033-dirty (2024-08-29T19:18:51UTC)
> >
> > [295:43:43.915493754] [1441844] ERROR DmaBufAllocator
> dma_buf_allocator.cpp:120
> > Could not open any dma-buf provider
> >
> > [295:43:43.919118835] [1441841]  INFO IPAManager ipa_manager.cpp:137
> libcamera
> > is not installed. Adding
> > '/usr/local/google/home/chenghaoyang/Workspace/libca
> >
> > mera/build/src/ipa' to the IPA search path
> >
> > [295:43:43.922245825] [1441841]  INFO Camera camera_manager.cpp:325
> libcamera
> > v0.3.1+79-8ca4f033-dirty (2024-08-29T19:18:51UTC)
> >
> > [295:43:43.922820175] [1441846] ERROR DmaBufAllocator
> dma_buf_allocator.cpp:120
> > Could not open any dma-buf provider
> >
> > Unable to set the pipeline to the playing state.
> > ```
> > gitlab pipeline link:
> >
> https://gitlab.freedesktop.org/chenghaoyang/libcamera/-/pipelines/1260412
> >
> > Looking into `gsteramer_multi_stream_test.cpp`,
> > I still couldn't figure out what's wrong...
> > Do you know what's tested in this unit test? I know that
> > DmaBufAllocator isn't valid, while I added logs and didn't
> > find `PipelineHandlerVirtual::exportFrameBuffers` being
> > called.
> >
>
> The above error message comes from the construction of the class
> member:
>
> src/libcamera/pipeline/virtual/virtual.h:       DmaBufAllocator
> dmaBufAllocator_;
>
> While gstreamer fails for multi stream but not for single, I'm not
> sure..
>
>
Yeah, and as I mentioned above,
`PipelineHandlerVirtual::exportFrameBuffers` is never called.
DmaBufAllocator doesn't seem to be the root cause...


>
> > I also need your opinion: I think there's nothing, except for the unit
> test
> > :p,
> > to stop Virtual Pipeline Handler from supporting multiple streams, while
> > do you think there should be a limitation?
>
> No I just thought this is what you were aiming to
>

I mean, do we want to limit the max number of streams, say 3, or we can
support any number of streams?


>
> > I was setting the maximum to 3 for StillCapture, VideoRecording, and
> > ViewFinder, while you know there can be multiple streams as the same
> > StreamRole...
> >
> >
> > > > +             /* 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
> > >
> > Done
> >
> >
> > >
> > > Who initializes data->testPattern_ ?
> > >
> > Hmm, it'll be done in the next patch, when we read from the
> > configuration file...
> > In this patch though, it's using the default value when we create
> > CameraData in the match() function.
>
> afaik enum variables are not initialized to any default value
>
> in example
>
> -------------------------------------------------------------------------------
> #include <iostream>
>
> enum class Status : int {
>         FrameSuccess,
>         FrameError,
>         FrameCancelled,
> };
>
> int main(int argc, char *argv[])
> {
>         Status stat;
>
>         std::cout << static_cast<int>(stat) << std::endl;
>
>         return 0;
> }
>
> -------------------------------------------------------------------------------
>
> prints a random (?) number (32766)
>
>
I think putting an enum in a class as a member variable
sets a default value, but sure, we can give it one in the
header file.

BTW, I compiled and ran your code with g++ and clang++.
Both constantly showed 0 instead of random numbers...


> >
> >
> > > > +             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
> > > >
> > >
>