[v11,3/7] libcamera: virtual: Add VirtualPipelineHandler

21195 diff mbox series

From: Harvey Yang <chenghaoyang@chromium.org>

Add VirtualPipelineHandler for more unit tests and verfiy libcamera
infrastructure works on devices without using hardware cameras.

Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
---
 meson.build                                |   1 +
 meson_options.txt                          |   3 +-
 src/libcamera/pipeline/virtual/meson.build |   5 +
 src/libcamera/pipeline/virtual/virtual.cpp | 267 +++++++++++++++++++++
 src/libcamera/pipeline/virtual/virtual.h   |  89 +++++++
 5 files changed, 364 insertions(+), 1 deletion(-)
 create mode 100644 src/libcamera/pipeline/virtual/meson.build
 create mode 100644 src/libcamera/pipeline/virtual/virtual.cpp
 create mode 100644 src/libcamera/pipeline/virtual/virtual.h

Hi Harvey

On Sat, Sep 07, 2024 at 02:28:28PM GMT, Harvey Yang wrote:
> From: Harvey Yang <chenghaoyang@chromium.org>
>
> Add VirtualPipelineHandler for more unit tests and verfiy libcamera
> infrastructure works on devices without using hardware cameras.
>
> Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
> ---
>  meson.build                                |   1 +
>  meson_options.txt                          |   3 +-
>  src/libcamera/pipeline/virtual/meson.build |   5 +
>  src/libcamera/pipeline/virtual/virtual.cpp | 267 +++++++++++++++++++++
>  src/libcamera/pipeline/virtual/virtual.h   |  89 +++++++
>  5 files changed, 364 insertions(+), 1 deletion(-)
>  create mode 100644 src/libcamera/pipeline/virtual/meson.build
>  create mode 100644 src/libcamera/pipeline/virtual/virtual.cpp
>  create mode 100644 src/libcamera/pipeline/virtual/virtual.h
>
> diff --git a/meson.build b/meson.build
> index 432ae133..ff9a70cf 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -214,6 +214,7 @@ pipelines_support = {
>      'simple':       ['any'],
>      'uvcvideo':     ['any'],
>      'vimc':         ['test'],
> +    'virtual':      ['test'],
>  }
>
>  if pipelines.contains('all')
> diff --git a/meson_options.txt b/meson_options.txt
> index 7aa41249..c91cd241 100644
> --- a/meson_options.txt
> +++ b/meson_options.txt
> @@ -53,7 +53,8 @@ option('pipelines',
>              'rpi/vc4',
>              'simple',
>              'uvcvideo',
> -            'vimc'
> +            'vimc',
> +            'virtual'
>          ],
>          description : 'Select which pipeline handlers to build. If this is set to "auto", all the pipelines applicable to the target architecture will be built. If this is set to "all", all the pipelines will be built. If both are selected then "all" will take precedence.')
>
> diff --git a/src/libcamera/pipeline/virtual/meson.build b/src/libcamera/pipeline/virtual/meson.build
> new file mode 100644
> index 00000000..ada1b335
> --- /dev/null
> +++ b/src/libcamera/pipeline/virtual/meson.build
> @@ -0,0 +1,5 @@
> +# SPDX-License-Identifier: CC0-1.0
> +
> +libcamera_internal_sources += files([
> +    'virtual.cpp',
> +])
> diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp
> new file mode 100644
> index 00000000..f85ec3dd
> --- /dev/null
> +++ b/src/libcamera/pipeline/virtual/virtual.cpp
> @@ -0,0 +1,267 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2024, Google Inc.
> + *
> + * virtual.cpp - Pipeline handler for virtual cameras
> + */
> +
> +#include "virtual.h"
> +
> +#include <algorithm>
> +#include <map>
> +#include <memory>
> +#include <set>

#include <string>

> +#include <utility>
> +#include <vector>
> +
> +#include <libcamera/base/log.h>
> +
> +#include <libcamera/control_ids.h>
> +#include <libcamera/controls.h>
> +#include <libcamera/formats.h>
> +#include <libcamera/property_ids.h>
> +
> +#include "libcamera/internal/camera.h"
> +#include "libcamera/internal/formats.h"
> +#include "libcamera/internal/pipeline_handler.h"
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(Virtual)
> +
> +namespace {
> +
> +uint64_t currentTimestamp()

As suggested on v9, this could probably be replaced with a few lines
of C++ using std::chrono. Have you tested it and found it not to be
working ? Not a big deal though

> +{
> +	struct timespec ts;
> +	if (clock_gettime(CLOCK_MONOTONIC, &ts) < 0) {
> +		LOG(Virtual, Error) << "Get clock time fails";
> +		return 0;
> +	}
> +
> +	return ts.tv_sec * 1'000'000'000LL + ts.tv_nsec;
> +}
> +
> +} /* namespace */
> +
> +VirtualCameraData::VirtualCameraData(PipelineHandler *pipe,
> +				     std::vector<Resolution> supportedResolutions)
> +	: Camera::Private(pipe), supportedResolutions_(std::move(supportedResolutions))
> +{
> +	for (const auto &resolution : supportedResolutions_) {
> +		if (minResolutionSize_.isNull() || minResolutionSize_ > resolution.size)
> +			minResolutionSize_ = resolution.size;
> +
> +		maxResolutionSize_ = std::max(maxResolutionSize_, resolution.size);
> +	}
> +
> +	/* \todo Support multiple streams and pass multi_stream_test */
> +	streamConfigs_.resize(kMaxStream);
> +}
> +
> +VirtualCameraConfiguration::VirtualCameraConfiguration(VirtualCameraData *data)
> +	: CameraConfiguration(), data_(data)
> +{
> +}
> +
> +CameraConfiguration::Status VirtualCameraConfiguration::validate()
> +{
> +	Status status = Valid;
> +
> +	if (config_.empty()) {
> +		LOG(Virtual, Error) << "Empty config";
> +		return Invalid;
> +	}
> +
> +	/* Only one stream is supported */
> +	if (config_.size() > VirtualCameraData::kMaxStream) {
> +		config_.resize(VirtualCameraData::kMaxStream);
> +		status = Adjusted;
> +	}
> +
> +	for (StreamConfiguration &cfg : config_) {

Knowing that you're going to add support for multiple streams I think
it's fine having a loop here already

> +		bool found = false;
> +		for (const auto &resolution : data_->supportedResolutions_) {
> +			if (resolution.size.width == cfg.size.width &&
> +			    resolution.size.height == cfg.size.height) {
> +				found = true;
> +				break;
> +			}
> +		}
> +
> +		if (!found) {
> +			/*
> +			 * \todo It's a pipeline's decision to choose a
> +			 * resolution when the exact one is not supported.
> +			 * Defining the default logic in PipelineHandler to
> +			 * find the closest resolution would be nice.
> +			 */
> +			cfg.size = data_->maxResolutionSize_;
> +			status = Adjusted;
> +		}
> +
> +		const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);
> +		cfg.stride = info.stride(cfg.size.width, 0, 1);
> +		cfg.frameSize = info.frameSize(cfg.size, 1);
> +
> +		cfg.bufferCount = VirtualCameraConfiguration::kBufferCount;
> +
> +		if (cfg.pixelFormat != formats::NV12) {
> +			cfg.pixelFormat = formats::NV12;
> +			LOG(Virtual, Debug)
> +				<< "Stream configuration adjusted to " << cfg.toString();
> +			status = Adjusted;
> +		}
> +	}
> +
> +	return status;
> +}
> +
> +PipelineHandlerVirtual::PipelineHandlerVirtual(CameraManager *manager)
> +	: PipelineHandler(manager),
> +	  dmaBufAllocator_(DmaBufAllocator::DmaBufAllocatorFlag::CmaHeap |
> +			   DmaBufAllocator::DmaBufAllocatorFlag::SystemHeap |
> +			   DmaBufAllocator::DmaBufAllocatorFlag::UDmaBuf)
> +{
> +}
> +
> +std::unique_ptr<CameraConfiguration>
> +PipelineHandlerVirtual::generateConfiguration(Camera *camera,
> +					      Span<const StreamRole> roles)
> +{
> +	VirtualCameraData *data = cameraData(camera);
> +	auto config =
> +		std::make_unique<VirtualCameraConfiguration>(data);
> +
> +	if (roles.empty())
> +		return config;
> +
> +	for (const StreamRole role : roles) {

I presume the loop is here to support multiple streams too

> +		std::map<PixelFormat, std::vector<SizeRange>> streamFormats;
> +		PixelFormat pixelFormat = formats::NV12;
> +		streamFormats[pixelFormat] = { { data->minResolutionSize_, data->maxResolutionSize_ } };
> +		StreamFormats formats(streamFormats);
> +		StreamConfiguration cfg(formats);
> +		cfg.pixelFormat = pixelFormat;
> +		cfg.size = data->maxResolutionSize_;
> +		cfg.bufferCount = VirtualCameraConfiguration::kBufferCount;
> +
> +		switch (role) {
> +		case StreamRole::StillCapture:
> +		case StreamRole::VideoRecording:
> +		case StreamRole::Viewfinder:
> +			break;
> +
> +		case StreamRole::Raw:
> +		default:
> +			LOG(Virtual, Error)
> +				<< "Requested stream role not supported: " << role;
> +			config.reset();
> +			return config;
> +		}

I would have placed the switch before populating cfg, but it's ok, you
clear config_ anyway in case of errors

> +
> +		config->addConfiguration(cfg);
> +	}
> +
> +	if (config->validate() == CameraConfiguration::Invalid)
> +		config.reset();

If this happens, it's a programming error so you could even ASSERT
here. No problem, the ipu3 pipeline does the same here.

> +
> +	return config;
> +}
> +
> +int PipelineHandlerVirtual::configure(Camera *camera,
> +				      CameraConfiguration *config)
> +{
> +	VirtualCameraData *data = cameraData(camera);
> +	for (size_t i = 0; i < config->size(); ++i)

Or
	for (auto [i, c] : utils::enumerate(*config))
		c.setStream(&data->streamConfigs_[i].stream);

not a big deal

> +		config->at(i).setStream(&data->streamConfigs_[i].stream);
> +
> +	return 0;
> +}
> +
> +int PipelineHandlerVirtual::exportFrameBuffers([[maybe_unused]] Camera *camera,
> +					       Stream *stream,
> +					       std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> +{
> +	if (!dmaBufAllocator_.isValid())
> +		return -ENOBUFS;
> +
> +	const StreamConfiguration &config = stream->configuration();
> +
> +	auto info = PixelFormatInfo::info(config.pixelFormat);
> +
> +	std::vector<unsigned int> planeSizes;
> +	for (size_t i = 0; i < info.planes.size(); ++i)
> +		planeSizes.push_back(info.planeSize(config.size, i));
> +
> +	return dmaBufAllocator_.exportBuffers(config.bufferCount, planeSizes, buffers);
> +}
> +
> +int PipelineHandlerVirtual::start([[maybe_unused]] Camera *camera,
> +				  [[maybe_unused]] const ControlList *controls)
> +{
> +	return 0;
> +}
> +
> +void PipelineHandlerVirtual::stopDevice([[maybe_unused]] Camera *camera)
> +{
> +}
> +
> +int PipelineHandlerVirtual::queueRequestDevice([[maybe_unused]] Camera *camera,
> +					       Request *request)
> +{
> +	/* \todo Read from the virtual video if any. */
> +	for (auto it : request->buffers())
> +		completeBuffer(request, it.second);
> +
> +	request->metadata().set(controls::SensorTimestamp, currentTimestamp());
> +	completeRequest(request);
> +
> +	return 0;
> +}
> +
> +// static

I'm not sure anymore how to get the message through: No C++ comments please

> +bool PipelineHandlerVirtual::created_ = false;

You can move this to the beginng of the file if you prefer. Otherwise
it's fine.


> +
> +bool PipelineHandlerVirtual::match([[maybe_unused]] DeviceEnumerator *enumerator)
> +{
> +	if (created_)
> +		return false;
> +
> +	created_ = true;
> +
> +	/* \todo Add virtual cameras according to a config file. */
> +
> +	std::vector<VirtualCameraData::Resolution> supportedResolutions;
> +	supportedResolutions.resize(2);
> +	supportedResolutions[0] = { .size = Size(1920, 1080), .frameRates = { 30 } };
> +	supportedResolutions[1] = { .size = Size(1280, 720), .frameRates = { 30 } };
> +
> +	std::unique_ptr<VirtualCameraData> data =
> +		std::make_unique<VirtualCameraData>(this, supportedResolutions);
> +
> +	data->properties_.set(properties::Location, properties::CameraLocationFront);
> +	data->properties_.set(properties::Model, "Virtual Video Device");
> +	data->properties_.set(properties::PixelArrayActiveAreas, { Rectangle(Size(1920, 1080)) });
> +
> +	/* \todo Set FrameDurationLimits based on config. */
> +	ControlInfoMap::Map controls;
> +	int64_t min_frame_duration = 33333, max_frame_duration = 33333;
> +	controls[&controls::FrameDurationLimits] = ControlInfo(min_frame_duration, max_frame_duration);
> +	data->controlInfo_ = ControlInfoMap(std::move(controls), controls::controls);
> +
> +	/* Create and register the camera. */
> +	std::set<Stream *> streams;
> +	for (auto &streamConfig : data->streamConfigs_)
> +		streams.insert(&streamConfig.stream);
> +
> +	const std::string id = "Virtual0";
> +	std::shared_ptr<Camera> camera = Camera::create(std::move(data), id, streams);
> +	registerCamera(std::move(camera));
> +
> +	return true;
> +}
> +
> +REGISTER_PIPELINE_HANDLER(PipelineHandlerVirtual, "virtual")
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/pipeline/virtual/virtual.h b/src/libcamera/pipeline/virtual/virtual.h
> new file mode 100644
> index 00000000..fb3dbcad
> --- /dev/null
> +++ b/src/libcamera/pipeline/virtual/virtual.h
> @@ -0,0 +1,89 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2024, Google Inc.
> + *
> + * virtual.h - Pipeline handler for virtual cameras
> + */
> +
> +#pragma once
> +
> +#include <memory>
> +#include <vector>
> +
> +#include <libcamera/base/file.h>
> +
> +#include "libcamera/internal/camera.h"
> +#include "libcamera/internal/dma_buf_allocator.h"
> +#include "libcamera/internal/pipeline_handler.h"
> +
> +namespace libcamera {
> +
> +class VirtualCameraData : public Camera::Private
> +{
> +public:
> +	const static unsigned int kMaxStream = 1;
> +
> +	struct Resolution {
> +		Size size;
> +		std::vector<int> frameRates;
> +	};
> +	struct StreamConfig {
> +		Stream stream;
> +	};
> +
> +	VirtualCameraData(PipelineHandler *pipe,
> +			  std::vector<Resolution> supportedResolutions);
> +
> +	~VirtualCameraData() = default;
> +
> +	const std::vector<Resolution> supportedResolutions_;
> +	Size maxResolutionSize_;
> +	Size minResolutionSize_;
> +
> +	std::vector<StreamConfig> streamConfigs_;
> +};
> +
> +class VirtualCameraConfiguration : public CameraConfiguration

It seems to me only VirtualCameraData is used by the other files. The
other classes declarations, as not used outside of virtual.cpp, could
be moved there.

Only nits here and there, with them addressed:
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
   j

> +{
> +public:
> +	static constexpr unsigned int kBufferCount = 4;
> +
> +	VirtualCameraConfiguration(VirtualCameraData *data);
> +
> +	Status validate() override;
> +
> +private:
> +	const VirtualCameraData *data_;
> +};
> +
> +class PipelineHandlerVirtual : public PipelineHandler
> +{
> +public:
> +	PipelineHandlerVirtual(CameraManager *manager);
> +
> +	std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,
> +								   Span<const StreamRole> roles) override;
> +	int configure(Camera *camera, CameraConfiguration *config) override;
> +
> +	int exportFrameBuffers(Camera *camera, Stream *stream,
> +			       std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;
> +
> +	int start(Camera *camera, const ControlList *controls) override;
> +	void stopDevice(Camera *camera) override;
> +
> +	int queueRequestDevice(Camera *camera, Request *request) override;
> +
> +	bool match(DeviceEnumerator *enumerator) override;
> +
> +private:
> +	static bool created_;
> +
> +	VirtualCameraData *cameraData(Camera *camera)
> +	{
> +		return static_cast<VirtualCameraData *>(camera->_d());
> +	}
> +
> +	DmaBufAllocator dmaBufAllocator_;
> +};
> +
> +} /* namespace libcamera */
> --
> 2.46.0.469.g59c65b2a67-goog
>

Hi Jacopo,

On Mon, Sep 9, 2024 at 4:47 PM Jacopo Mondi <jacopo.mondi@ideasonboard.com>
wrote:

> Hi Harvey
>
> On Sat, Sep 07, 2024 at 02:28:28PM GMT, Harvey Yang wrote:
> > From: Harvey Yang <chenghaoyang@chromium.org>
> >
> > Add VirtualPipelineHandler for more unit tests and verfiy libcamera
> > infrastructure works on devices without using hardware cameras.
> >
> > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
> > ---
> >  meson.build                                |   1 +
> >  meson_options.txt                          |   3 +-
> >  src/libcamera/pipeline/virtual/meson.build |   5 +
> >  src/libcamera/pipeline/virtual/virtual.cpp | 267 +++++++++++++++++++++
> >  src/libcamera/pipeline/virtual/virtual.h   |  89 +++++++
> >  5 files changed, 364 insertions(+), 1 deletion(-)
> >  create mode 100644 src/libcamera/pipeline/virtual/meson.build
> >  create mode 100644 src/libcamera/pipeline/virtual/virtual.cpp
> >  create mode 100644 src/libcamera/pipeline/virtual/virtual.h
> >
> > diff --git a/meson.build b/meson.build
> > index 432ae133..ff9a70cf 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -214,6 +214,7 @@ pipelines_support = {
> >      'simple':       ['any'],
> >      'uvcvideo':     ['any'],
> >      'vimc':         ['test'],
> > +    'virtual':      ['test'],
> >  }
> >
> >  if pipelines.contains('all')
> > diff --git a/meson_options.txt b/meson_options.txt
> > index 7aa41249..c91cd241 100644
> > --- a/meson_options.txt
> > +++ b/meson_options.txt
> > @@ -53,7 +53,8 @@ option('pipelines',
> >              'rpi/vc4',
> >              'simple',
> >              'uvcvideo',
> > -            'vimc'
> > +            'vimc',
> > +            'virtual'
> >          ],
> >          description : 'Select which pipeline handlers to build. If this
> is set to "auto", all the pipelines applicable to the target architecture
> will be built. If this is set to "all", all the pipelines will be built. If
> both are selected then "all" will take precedence.')
> >
> > diff --git a/src/libcamera/pipeline/virtual/meson.build
> b/src/libcamera/pipeline/virtual/meson.build
> > new file mode 100644
> > index 00000000..ada1b335
> > --- /dev/null
> > +++ b/src/libcamera/pipeline/virtual/meson.build
> > @@ -0,0 +1,5 @@
> > +# SPDX-License-Identifier: CC0-1.0
> > +
> > +libcamera_internal_sources += files([
> > +    'virtual.cpp',
> > +])
> > diff --git a/src/libcamera/pipeline/virtual/virtual.cpp
> b/src/libcamera/pipeline/virtual/virtual.cpp
> > new file mode 100644
> > index 00000000..f85ec3dd
> > --- /dev/null
> > +++ b/src/libcamera/pipeline/virtual/virtual.cpp
> > @@ -0,0 +1,267 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2024, Google Inc.
> > + *
> > + * virtual.cpp - Pipeline handler for virtual cameras
> > + */
> > +
> > +#include "virtual.h"
> > +
> > +#include <algorithm>
> > +#include <map>
> > +#include <memory>
> > +#include <set>
>
> #include <string>
>
> > +#include <utility>
> > +#include <vector>
> > +
> > +#include <libcamera/base/log.h>
> > +
> > +#include <libcamera/control_ids.h>
> > +#include <libcamera/controls.h>
> > +#include <libcamera/formats.h>
> > +#include <libcamera/property_ids.h>
> > +
> > +#include "libcamera/internal/camera.h"
> > +#include "libcamera/internal/formats.h"
> > +#include "libcamera/internal/pipeline_handler.h"
> > +
> > +namespace libcamera {
> > +
> > +LOG_DEFINE_CATEGORY(Virtual)
> > +
> > +namespace {
> > +
> > +uint64_t currentTimestamp()
>
> As suggested on v9, this could probably be replaced with a few lines
> of C++ using std::chrono. Have you tested it and found it not to be
> working ? Not a big deal though
>

Ah sorry... I struggled a lot with the user interface of emails and
patchwork, so from time to time I lose track of comments.

Yes, comparing the timestamps, those two approaches produce similar
timestamps. Updated in v12.

I think we can keep the function `currentTimestamp` though, just replacing
the implementation. Let me know if you think I should get rid of it as well.


>
> > +{
> > +     struct timespec ts;
> > +     if (clock_gettime(CLOCK_MONOTONIC, &ts) < 0) {
> > +             LOG(Virtual, Error) << "Get clock time fails";
> > +             return 0;
> > +     }
> > +
> > +     return ts.tv_sec * 1'000'000'000LL + ts.tv_nsec;
> > +}
> > +
> > +} /* namespace */
> > +
> > +VirtualCameraData::VirtualCameraData(PipelineHandler *pipe,
> > +                                  std::vector<Resolution>
> supportedResolutions)
> > +     : Camera::Private(pipe),
> supportedResolutions_(std::move(supportedResolutions))
> > +{
> > +     for (const auto &resolution : supportedResolutions_) {
> > +             if (minResolutionSize_.isNull() || minResolutionSize_ >
> resolution.size)
> > +                     minResolutionSize_ = resolution.size;
> > +
> > +             maxResolutionSize_ = std::max(maxResolutionSize_,
> resolution.size);
> > +     }
> > +
> > +     /* \todo Support multiple streams and pass multi_stream_test */
> > +     streamConfigs_.resize(kMaxStream);
> > +}
> > +
> >
> +VirtualCameraConfiguration::VirtualCameraConfiguration(VirtualCameraData
> *data)
> > +     : CameraConfiguration(), data_(data)
> > +{
> > +}
> > +
> > +CameraConfiguration::Status VirtualCameraConfiguration::validate()
> > +{
> > +     Status status = Valid;
> > +
> > +     if (config_.empty()) {
> > +             LOG(Virtual, Error) << "Empty config";
> > +             return Invalid;
> > +     }
> > +
> > +     /* Only one stream is supported */
> > +     if (config_.size() > VirtualCameraData::kMaxStream) {
> > +             config_.resize(VirtualCameraData::kMaxStream);
> > +             status = Adjusted;
> > +     }
> > +
> > +     for (StreamConfiguration &cfg : config_) {
>
> Knowing that you're going to add support for multiple streams I think
> it's fine having a loop here already
>
>
Yes, thanks :)


> > +             bool found = false;
> > +             for (const auto &resolution :
> data_->supportedResolutions_) {
> > +                     if (resolution.size.width == cfg.size.width &&
> > +                         resolution.size.height == cfg.size.height) {
> > +                             found = true;
> > +                             break;
> > +                     }
> > +             }
> > +
> > +             if (!found) {
> > +                     /*
> > +                      * \todo It's a pipeline's decision to choose a
> > +                      * resolution when the exact one is not supported.
> > +                      * Defining the default logic in PipelineHandler to
> > +                      * find the closest resolution would be nice.
> > +                      */
> > +                     cfg.size = data_->maxResolutionSize_;
> > +                     status = Adjusted;
> > +             }
> > +
> > +             const PixelFormatInfo &info =
> PixelFormatInfo::info(cfg.pixelFormat);
> > +             cfg.stride = info.stride(cfg.size.width, 0, 1);
> > +             cfg.frameSize = info.frameSize(cfg.size, 1);
> > +
> > +             cfg.bufferCount = VirtualCameraConfiguration::kBufferCount;
> > +
> > +             if (cfg.pixelFormat != formats::NV12) {
> > +                     cfg.pixelFormat = formats::NV12;
> > +                     LOG(Virtual, Debug)
> > +                             << "Stream configuration adjusted to " <<
> cfg.toString();
> > +                     status = Adjusted;
> > +             }
> > +     }
> > +
> > +     return status;
> > +}
> > +
> > +PipelineHandlerVirtual::PipelineHandlerVirtual(CameraManager *manager)
> > +     : PipelineHandler(manager),
> > +       dmaBufAllocator_(DmaBufAllocator::DmaBufAllocatorFlag::CmaHeap |
> > +
> DmaBufAllocator::DmaBufAllocatorFlag::SystemHeap |
> > +                        DmaBufAllocator::DmaBufAllocatorFlag::UDmaBuf)
> > +{
> > +}
> > +
> > +std::unique_ptr<CameraConfiguration>
> > +PipelineHandlerVirtual::generateConfiguration(Camera *camera,
> > +                                           Span<const StreamRole> roles)
> > +{
> > +     VirtualCameraData *data = cameraData(camera);
> > +     auto config =
> > +             std::make_unique<VirtualCameraConfiguration>(data);
> > +
> > +     if (roles.empty())
> > +             return config;
> > +
> > +     for (const StreamRole role : roles) {
>
> I presume the loop is here to support multiple streams too
>
>
Yes, exactly.


> > +             std::map<PixelFormat, std::vector<SizeRange>>
> streamFormats;
> > +             PixelFormat pixelFormat = formats::NV12;
> > +             streamFormats[pixelFormat] = { { data->minResolutionSize_,
> data->maxResolutionSize_ } };
> > +             StreamFormats formats(streamFormats);
> > +             StreamConfiguration cfg(formats);
> > +             cfg.pixelFormat = pixelFormat;
> > +             cfg.size = data->maxResolutionSize_;
> > +             cfg.bufferCount = VirtualCameraConfiguration::kBufferCount;
> > +
> > +             switch (role) {
> > +             case StreamRole::StillCapture:
> > +             case StreamRole::VideoRecording:
> > +             case StreamRole::Viewfinder:
> > +                     break;
> > +
> > +             case StreamRole::Raw:
> > +             default:
> > +                     LOG(Virtual, Error)
> > +                             << "Requested stream role not supported: "
> << role;
> > +                     config.reset();
> > +                     return config;
> > +             }
>
> I would have placed the switch before populating cfg, but it's ok, you
> clear config_ anyway in case of errors
>
>
Right, the switch and the check on `role` can be done earlier.
Updated in v12.


> > +
> > +             config->addConfiguration(cfg);
> > +     }
> > +
> > +     if (config->validate() == CameraConfiguration::Invalid)
> > +             config.reset();
>
> If this happens, it's a programming error so you could even ASSERT
> here. No problem, the ipu3 pipeline does the same here.
>
>
Ah right, updated with ASSERT in v12.


> > +
> > +     return config;
> > +}
> > +
> > +int PipelineHandlerVirtual::configure(Camera *camera,
> > +                                   CameraConfiguration *config)
> > +{
> > +     VirtualCameraData *data = cameraData(camera);
> > +     for (size_t i = 0; i < config->size(); ++i)
>
> Or
>         for (auto [i, c] : utils::enumerate(*config))
>                 c.setStream(&data->streamConfigs_[i].stream);
>
> not a big deal
>

Thanks! Updated in v12.


>
> > +             config->at(i).setStream(&data->streamConfigs_[i].stream);
> > +
> > +     return 0;
> > +}
> > +
> > +int PipelineHandlerVirtual::exportFrameBuffers([[maybe_unused]] Camera
> *camera,
> > +                                            Stream *stream,
> > +
> std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> > +{
> > +     if (!dmaBufAllocator_.isValid())
> > +             return -ENOBUFS;
> > +
> > +     const StreamConfiguration &config = stream->configuration();
> > +
> > +     auto info = PixelFormatInfo::info(config.pixelFormat);
> > +
> > +     std::vector<unsigned int> planeSizes;
> > +     for (size_t i = 0; i < info.planes.size(); ++i)
> > +             planeSizes.push_back(info.planeSize(config.size, i));
> > +
> > +     return dmaBufAllocator_.exportBuffers(config.bufferCount,
> planeSizes, buffers);
> > +}
> > +
> > +int PipelineHandlerVirtual::start([[maybe_unused]] Camera *camera,
> > +                               [[maybe_unused]] const ControlList
> *controls)
> > +{
> > +     return 0;
> > +}
> > +
> > +void PipelineHandlerVirtual::stopDevice([[maybe_unused]] Camera *camera)
> > +{
> > +}
> > +
> > +int PipelineHandlerVirtual::queueRequestDevice([[maybe_unused]] Camera
> *camera,
> > +                                            Request *request)
> > +{
> > +     /* \todo Read from the virtual video if any. */
> > +     for (auto it : request->buffers())
> > +             completeBuffer(request, it.second);
> > +
> > +     request->metadata().set(controls::SensorTimestamp,
> currentTimestamp());
> > +     completeRequest(request);
> > +
> > +     return 0;
> > +}
> > +
> > +// static
>
> I'm not sure anymore how to get the message through: No C++ comments please
>

Sorry, updated.


>
> > +bool PipelineHandlerVirtual::created_ = false;
>
> You can move this to the beginng of the file if you prefer. Otherwise
> it's fine.
>

Done.


>
>
> > +
> > +bool PipelineHandlerVirtual::match([[maybe_unused]] DeviceEnumerator
> *enumerator)
> > +{
> > +     if (created_)
> > +             return false;
> > +
> > +     created_ = true;
> > +
> > +     /* \todo Add virtual cameras according to a config file. */
> > +
> > +     std::vector<VirtualCameraData::Resolution> supportedResolutions;
> > +     supportedResolutions.resize(2);
> > +     supportedResolutions[0] = { .size = Size(1920, 1080), .frameRates
> = { 30 } };
> > +     supportedResolutions[1] = { .size = Size(1280, 720), .frameRates =
> { 30 } };
> > +
> > +     std::unique_ptr<VirtualCameraData> data =
> > +             std::make_unique<VirtualCameraData>(this,
> supportedResolutions);
> > +
> > +     data->properties_.set(properties::Location,
> properties::CameraLocationFront);
> > +     data->properties_.set(properties::Model, "Virtual Video Device");
> > +     data->properties_.set(properties::PixelArrayActiveAreas, {
> Rectangle(Size(1920, 1080)) });
> > +
> > +     /* \todo Set FrameDurationLimits based on config. */
> > +     ControlInfoMap::Map controls;
> > +     int64_t min_frame_duration = 33333, max_frame_duration = 33333;
> > +     controls[&controls::FrameDurationLimits] =
> ControlInfo(min_frame_duration, max_frame_duration);
> > +     data->controlInfo_ = ControlInfoMap(std::move(controls),
> controls::controls);
> > +
> > +     /* Create and register the camera. */
> > +     std::set<Stream *> streams;
> > +     for (auto &streamConfig : data->streamConfigs_)
> > +             streams.insert(&streamConfig.stream);
> > +
> > +     const std::string id = "Virtual0";
> > +     std::shared_ptr<Camera> camera = Camera::create(std::move(data),
> id, streams);
> > +     registerCamera(std::move(camera));
> > +
> > +     return true;
> > +}
> > +
> > +REGISTER_PIPELINE_HANDLER(PipelineHandlerVirtual, "virtual")
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/libcamera/pipeline/virtual/virtual.h
> b/src/libcamera/pipeline/virtual/virtual.h
> > new file mode 100644
> > index 00000000..fb3dbcad
> > --- /dev/null
> > +++ b/src/libcamera/pipeline/virtual/virtual.h
> > @@ -0,0 +1,89 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2024, Google Inc.
> > + *
> > + * virtual.h - Pipeline handler for virtual cameras
> > + */
> > +
> > +#pragma once
> > +
> > +#include <memory>
> > +#include <vector>
> > +
> > +#include <libcamera/base/file.h>
> > +
> > +#include "libcamera/internal/camera.h"
> > +#include "libcamera/internal/dma_buf_allocator.h"
> > +#include "libcamera/internal/pipeline_handler.h"
> > +
> > +namespace libcamera {
> > +
> > +class VirtualCameraData : public Camera::Private
> > +{
> > +public:
> > +     const static unsigned int kMaxStream = 1;
> > +
> > +     struct Resolution {
> > +             Size size;
> > +             std::vector<int> frameRates;
> > +     };
> > +     struct StreamConfig {
> > +             Stream stream;
> > +     };
> > +
> > +     VirtualCameraData(PipelineHandler *pipe,
> > +                       std::vector<Resolution> supportedResolutions);
> > +
> > +     ~VirtualCameraData() = default;
> > +
> > +     const std::vector<Resolution> supportedResolutions_;
> > +     Size maxResolutionSize_;
> > +     Size minResolutionSize_;
> > +
> > +     std::vector<StreamConfig> streamConfigs_;
> > +};
> > +
> > +class VirtualCameraConfiguration : public CameraConfiguration
>
> It seems to me only VirtualCameraData is used by the other files. The
> other classes declarations, as not used outside of virtual.cpp, could
> be moved there.
>
> Only nits here and there, with them addressed:
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>
> Thanks
>    j
>
> > +{
> > +public:
> > +     static constexpr unsigned int kBufferCount = 4;
> > +
> > +     VirtualCameraConfiguration(VirtualCameraData *data);
> > +
> > +     Status validate() override;
> > +
> > +private:
> > +     const VirtualCameraData *data_;
> > +};
> > +
> > +class PipelineHandlerVirtual : public PipelineHandler
> > +{
> > +public:
> > +     PipelineHandlerVirtual(CameraManager *manager);
> > +
> > +     std::unique_ptr<CameraConfiguration> generateConfiguration(Camera
> *camera,
> > +
> Span<const StreamRole> roles) override;
> > +     int configure(Camera *camera, CameraConfiguration *config)
> override;
> > +
> > +     int exportFrameBuffers(Camera *camera, Stream *stream,
> > +                            std::vector<std::unique_ptr<FrameBuffer>>
> *buffers) override;
> > +
> > +     int start(Camera *camera, const ControlList *controls) override;
> > +     void stopDevice(Camera *camera) override;
> > +
> > +     int queueRequestDevice(Camera *camera, Request *request) override;
> > +
> > +     bool match(DeviceEnumerator *enumerator) override;
> > +
> > +private:
> > +     static bool created_;
> > +
> > +     VirtualCameraData *cameraData(Camera *camera)
> > +     {
> > +             return static_cast<VirtualCameraData *>(camera->_d());
> > +     }
> > +
> > +     DmaBufAllocator dmaBufAllocator_;
> > +};
> > +
> > +} /* namespace libcamera */
> > --
> > 2.46.0.469.g59c65b2a67-goog
> >
>