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

21209 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>
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 meson.build                                |   1 +
 meson_options.txt                          |   3 +-
 src/libcamera/pipeline/virtual/meson.build |   5 +
 src/libcamera/pipeline/virtual/virtual.cpp | 309 +++++++++++++++++++++
 src/libcamera/pipeline/virtual/virtual.h   |  44 +++
 5 files changed, 361 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 Tue, Sep 10, 2024 at 04:40:16AM 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>
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>  meson.build                                |   1 +
>  meson_options.txt                          |   3 +-
>  src/libcamera/pipeline/virtual/meson.build |   5 +
>  src/libcamera/pipeline/virtual/virtual.cpp | 309 +++++++++++++++++++++
>  src/libcamera/pipeline/virtual/virtual.h   |  44 +++
>  5 files changed, 361 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..56e05528
> --- /dev/null
> +++ b/src/libcamera/pipeline/virtual/virtual.cpp
> @@ -0,0 +1,309 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2024, Google Inc.
> + *
> + * virtual.cpp - Pipeline handler for virtual cameras
> + */
> +

I get this output from iwyu

#include <errno.h>                                // for ENOBUFS
#include <stdint.h>                               // for int64_t, uint64_t
#include <time.h>                                 // for timespec, clock_get...
#include <array>                                  // for array
#include <ostream>                                // for basic_ostream, oper...
#include <string>                                 // for char_traits, operat...
#include "libcamera/base/flags.h"                 // for operator|, Flags
#include "libcamera/pixel_format.h"               // for PixelFormat
#include "libcamera/request.h"                    // for Request

> +#include "virtual.h"
> +
> +#include <algorithm>
> +#include <chrono>
> +#include <map>
> +#include <memory>
> +#include <set>
> +#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/dma_buf_allocator.h"
> +#include "libcamera/internal/formats.h"
> +#include "libcamera/internal/pipeline_handler.h"
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(Virtual)
> +
> +namespace {
> +
> +uint64_t currentTimestamp()
> +{
> +	const auto now = std::chrono::steady_clock::now();
> +	auto nsecs = std::chrono::duration_cast<std::chrono::nanoseconds>(
> +		now.time_since_epoch());
> +
> +	return nsecs.count();
> +}

I won't push on this, but as commented on previous versions this could
be replaced by a single line of C++. Not a big deal, you can keep what
you have here

> +
> +} /* namespace */
> +
> +class VirtualCameraConfiguration : public CameraConfiguration
> +{
> +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_;
> +};
> +
> +/* static */
> +bool PipelineHandlerVirtual::created_ = false;
> +
> +VirtualCameraData::VirtualCameraData(PipelineHandler *pipe,
> +				     std::vector<Resolution> supportedResolutions)

supportedResolution should be passed by reference

> +	: 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_) {
> +		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);

You should calculate stride and frameSize after having adjusted the
format to NV12

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

Fits on one line

> +
> +	if (roles.empty())
> +		return config;
> +
> +	for (const StreamRole role : roles) {
> +		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;
> +		}
> +
> +		std::map<PixelFormat, std::vector<SizeRange>> streamFormats;
> +		PixelFormat pixelFormat = formats::NV12;
> +		streamFormats[pixelFormat] = { { data->minResolutionSize_, data->maxResolutionSize_ } };

Can easily be shortened to

		streamFormats[pixelFormat] = { { data->minResolutionSize_,
						 data->maxResolutionSize_ } };

> +		StreamFormats formats(streamFormats);
> +		StreamConfiguration cfg(formats);
> +		cfg.pixelFormat = pixelFormat;
> +		cfg.size = data->maxResolutionSize_;
> +		cfg.bufferCount = VirtualCameraConfiguration::kBufferCount;
> +
> +		config->addConfiguration(cfg);
> +	}
> +
> +	ASSERT(config->validate() != CameraConfiguration::Invalid);
> +
> +	return config;
> +}
> +
> +int PipelineHandlerVirtual::configure(Camera *camera,
> +				      CameraConfiguration *config)
> +{
> +	VirtualCameraData *data = cameraData(camera);
> +	for (auto [i, c] : utils::enumerate(*config))
> +		c.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. */

I thought this had to be dropped

> +	for (auto it : request->buffers())
> +		completeBuffer(request, it.second);
> +
> +	request->metadata().set(controls::SensorTimestamp, currentTimestamp());
> +	completeRequest(request);
> +
> +	return 0;
> +}
> +
> +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..77823c41
> --- /dev/null
> +++ b/src/libcamera/pipeline/virtual/virtual.h
> @@ -0,0 +1,44 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2024, Google Inc.
> + *
> + * virtual.h - Pipeline handler for virtual cameras
> + */
> +

I get this output from iwyu

../src/libcamera/pipeline/virtual/virtual.h should add these lines:
#include "libcamera/base/span.h"                   // for Span
#include "libcamera/camera.h"                      // for Camera, CameraConf...
#include "libcamera/geometry.h"                    // for Size
#include "libcamera/stream.h"                      // for Stream, StreamRole...

../src/libcamera/pipeline/virtual/virtual.h should remove these lines:
- #include <libcamera/base/file.h>  // lines 13-13


> +#pragma once
> +
> +#include <vector>
> +
> +#include <libcamera/base/file.h>
> +
> +#include "libcamera/internal/camera.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_;
> +};

With the above fixed you can keep my tag

Thanks
  j

> +
> +} /* namespace libcamera */
> --
> 2.46.0.598.g6f2099f65c-goog
>

Hi Jacopo,

On Thu, Sep 26, 2024 at 7:03 PM Jacopo Mondi
<jacopo.mondi@ideasonboard.com> wrote:
>
> Hi Harvey
>
> On Tue, Sep 10, 2024 at 04:40:16AM 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>
> > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > ---
> >  meson.build                                |   1 +
> >  meson_options.txt                          |   3 +-
> >  src/libcamera/pipeline/virtual/meson.build |   5 +
> >  src/libcamera/pipeline/virtual/virtual.cpp | 309 +++++++++++++++++++++
> >  src/libcamera/pipeline/virtual/virtual.h   |  44 +++
> >  5 files changed, 361 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..56e05528
> > --- /dev/null
> > +++ b/src/libcamera/pipeline/virtual/virtual.cpp
> > @@ -0,0 +1,309 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2024, Google Inc.
> > + *
> > + * virtual.cpp - Pipeline handler for virtual cameras
> > + */
> > +
>
> I get this output from iwyu
>
> #include <errno.h>                                // for ENOBUFS
> #include <stdint.h>                               // for int64_t, uint64_t
> #include <time.h>                                 // for timespec, clock_get...
> #include <array>                                  // for array
> #include <ostream>                                // for basic_ostream, oper...
> #include <string>                                 // for char_traits, operat...
> #include "libcamera/base/flags.h"                 // for operator|, Flags
> #include "libcamera/pixel_format.h"               // for PixelFormat
> #include "libcamera/request.h"                    // for Request

Added.

>
> > +#include "virtual.h"
> > +
> > +#include <algorithm>
> > +#include <chrono>
> > +#include <map>
> > +#include <memory>
> > +#include <set>
> > +#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/dma_buf_allocator.h"
> > +#include "libcamera/internal/formats.h"
> > +#include "libcamera/internal/pipeline_handler.h"
> > +
> > +namespace libcamera {
> > +
> > +LOG_DEFINE_CATEGORY(Virtual)
> > +
> > +namespace {
> > +
> > +uint64_t currentTimestamp()
> > +{
> > +     const auto now = std::chrono::steady_clock::now();
> > +     auto nsecs = std::chrono::duration_cast<std::chrono::nanoseconds>(
> > +             now.time_since_epoch());
> > +
> > +     return nsecs.count();
> > +}
>
> I won't push on this, but as commented on previous versions this could
> be replaced by a single line of C++. Not a big deal, you can keep what
> you have here

Hmm, I still think defining it as a function is cleaner. Let's keep it as is :)

>
> > +
> > +} /* namespace */
> > +
> > +class VirtualCameraConfiguration : public CameraConfiguration
> > +{
> > +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_;
> > +};
> > +
> > +/* static */
> > +bool PipelineHandlerVirtual::created_ = false;
> > +
> > +VirtualCameraData::VirtualCameraData(PipelineHandler *pipe,
> > +                                  std::vector<Resolution> supportedResolutions)
>
> supportedResolution should be passed by reference

Used const reference, and removed `std::move` below.

>
> > +     : 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_) {
> > +             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);
>
> You should calculate stride and frameSize after having adjusted the
> format to NV12

Right, moved this block after checking pixelFormat.

>
> > +
> > +             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);
>
> Fits on one line

Done

>
> > +
> > +     if (roles.empty())
> > +             return config;
> > +
> > +     for (const StreamRole role : roles) {
> > +             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;
> > +             }
> > +
> > +             std::map<PixelFormat, std::vector<SizeRange>> streamFormats;
> > +             PixelFormat pixelFormat = formats::NV12;
> > +             streamFormats[pixelFormat] = { { data->minResolutionSize_, data->maxResolutionSize_ } };
>
> Can easily be shortened to
>
>                 streamFormats[pixelFormat] = { { data->minResolutionSize_,
>                                                  data->maxResolutionSize_ } };

Done

>
> > +             StreamFormats formats(streamFormats);
> > +             StreamConfiguration cfg(formats);
> > +             cfg.pixelFormat = pixelFormat;
> > +             cfg.size = data->maxResolutionSize_;
> > +             cfg.bufferCount = VirtualCameraConfiguration::kBufferCount;
> > +
> > +             config->addConfiguration(cfg);
> > +     }
> > +
> > +     ASSERT(config->validate() != CameraConfiguration::Invalid);
> > +
> > +     return config;
> > +}
> > +
> > +int PipelineHandlerVirtual::configure(Camera *camera,
> > +                                   CameraConfiguration *config)
> > +{
> > +     VirtualCameraData *data = cameraData(camera);
> > +     for (auto [i, c] : utils::enumerate(*config))
> > +             c.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. */
>
> I thought this had to be dropped

Done

>
> > +     for (auto it : request->buffers())
> > +             completeBuffer(request, it.second);
> > +
> > +     request->metadata().set(controls::SensorTimestamp, currentTimestamp());
> > +     completeRequest(request);
> > +
> > +     return 0;
> > +}
> > +
> > +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..77823c41
> > --- /dev/null
> > +++ b/src/libcamera/pipeline/virtual/virtual.h
> > @@ -0,0 +1,44 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2024, Google Inc.
> > + *
> > + * virtual.h - Pipeline handler for virtual cameras
> > + */
> > +
>
> I get this output from iwyu
>
> ../src/libcamera/pipeline/virtual/virtual.h should add these lines:
> #include "libcamera/base/span.h"                   // for Span

Didn't use this in this header. Only in the source file.

> #include "libcamera/camera.h"                      // for Camera, CameraConf...

Only used Camera::Private, and I already included
`libcamera/internal/camera.h`

> #include "libcamera/geometry.h"                    // for Size
> #include "libcamera/stream.h"                      // for Stream, StreamRole...
>
> ../src/libcamera/pipeline/virtual/virtual.h should remove these lines:
> - #include <libcamera/base/file.h>  // lines 13-13

Done


>
>
> > +#pragma once
> > +
> > +#include <vector>
> > +
> > +#include <libcamera/base/file.h>
> > +
> > +#include "libcamera/internal/camera.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_;
> > +};
>
> With the above fixed you can keep my tag
>
> Thanks
>   j
>
> > +
> > +} /* namespace libcamera */
> > --
> > 2.46.0.598.g6f2099f65c-goog
> >



--
BR,
Harvey Yang