[libcamera-devel,v5,2/2] libcamera: pipeline: Add VirtualPipelineHandler

18533 diff mbox series

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

Updating `/etc/camera/libcamera/camera_hal.yaml` on a chromebook DUT is
required to find the virtual camera with id `Virtual0`.

[todo Read frames from the virtual video if any]

Test the configurations can be generated and reported with cam -I:
"""
build/src/apps/cam/cam -c 1 -I
[45:19:28.901456135] [2611530]  INFO IPAManager ipa_manager.cpp:143
libcamera is not installed. Adding
'/usr/local/google/home/chenghaoyang/cros2/src/third_party/libcamera/build/src
/ipa' to the IPA search path
[45:19:28.904364465] [2611530]  INFO Camera camera_manager.cpp:293
libcamera v0.0.1+56-4f4554fa-dirty (2022-12-07T06:55:04+00:00)
Using camera Virtual0 as cam0
0: 1920x1080-NV12
 * Pixelformat: NV12 (1280x720)-(1920x1080)/(+1,+1)
  - 1280x720
  - 1280x800
  - 1360x768
  - 1366x768
  - 1440x900
  - 1280x1024
  - 1536x864
  - 1280x1080
  - 1600x900
  - 1400x1050
  - 1680x1050
  - 1920x1080
"""

"""
build/src/apps/cam/cam -l
[550:47:04.505850647] [1969734]  INFO IPAManager ipa_manager.cpp:143
libcamera is not installed. Adding ... to the IPA search path
[550:47:04.509307667] [1969734]  INFO Camera camera_manager.cpp:293
libcamera v0.0.1+54-55fecb4d-dirty (2022-12-01T05:47:11+00:00)
Available cameras:
1: (Virtual0)
"""

Using qcam should receive blank (all green) frames:
"""
build/src/apps/qcam/qcam -c Virtual0
"""

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 | 302 +++++++++++++++++++++
 4 files changed, 310 insertions(+), 1 deletion(-)
 create mode 100644 src/libcamera/pipeline/virtual/meson.build
 create mode 100644 src/libcamera/pipeline/virtual/virtual.cpp

Hi Harvey

   mostly cosmetic comments, my concern is on 1/2 and I wuold like to
hear from others, maybe it has been discussed in the past, but it
worries me a bit.

On Fri, Apr 07, 2023 at 06:20:50PM +0800, Harvey Yang via libcamera-devel wrote:
> Add VirtualPipelineHandler for more unit tests and verfiy libcamera
> infrastructure works on devices without using hardware cameras.
>
> Updating `/etc/camera/libcamera/camera_hal.yaml` on a chromebook DUT is
> required to find the virtual camera with id `Virtual0`.

Why ? For the Android HAL ? If that's the case this is not related to
this patch and you can drop this line

>
> [todo Read frames from the virtual video if any]

I think you can drop this as well

>
> Test the configurations can be generated and reported with cam -I:
> """
> build/src/apps/cam/cam -c 1 -I
> [45:19:28.901456135] [2611530]  INFO IPAManager ipa_manager.cpp:143
> libcamera is not installed. Adding
> '/usr/local/google/home/chenghaoyang/cros2/src/third_party/libcamera/build/src
> /ipa' to the IPA search path
> [45:19:28.904364465] [2611530]  INFO Camera camera_manager.cpp:293
> libcamera v0.0.1+56-4f4554fa-dirty (2022-12-07T06:55:04+00:00)
> Using camera Virtual0 as cam0
> 0: 1920x1080-NV12
>  * Pixelformat: NV12 (1280x720)-(1920x1080)/(+1,+1)
>   - 1280x720
>   - 1280x800
>   - 1360x768
>   - 1366x768
>   - 1440x900
>   - 1280x1024
>   - 1536x864
>   - 1280x1080
>   - 1600x900
>   - 1400x1050
>   - 1680x1050
>   - 1920x1080
> """
>
> """
> build/src/apps/cam/cam -l
> [550:47:04.505850647] [1969734]  INFO IPAManager ipa_manager.cpp:143
> libcamera is not installed. Adding ... to the IPA search path
> [550:47:04.509307667] [1969734]  INFO Camera camera_manager.cpp:293
> libcamera v0.0.1+54-55fecb4d-dirty (2022-12-01T05:47:11+00:00)
> Available cameras:
> 1: (Virtual0)
> """
>
> Using qcam should receive blank (all green) frames:
> """
> build/src/apps/qcam/qcam -c Virtual0
> """

All the examples are better placed in the cover letter imho, there's
no need to have them in the commit history ?

>
> 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 | 302 +++++++++++++++++++++
>  4 files changed, 310 insertions(+), 1 deletion(-)
>  create mode 100644 src/libcamera/pipeline/virtual/meson.build
>  create mode 100644 src/libcamera/pipeline/virtual/virtual.cpp
>
> diff --git a/meson.build b/meson.build
> index 0f89b45a..c2c4ba5f 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -177,6 +177,7 @@ pipelines_support = {
>      'simple':       arch_arm,
>      'uvcvideo':     ['any'],
>      'vimc':         ['test'],
> +    'virtual':      ['test'],
>  }
>
>  if pipelines.contains('all')
> diff --git a/meson_options.txt b/meson_options.txt
> index 78a78b72..a0a75e7f 100644
> --- a/meson_options.txt
> +++ b/meson_options.txt
> @@ -47,7 +47,8 @@ option('pipelines',
>              'rkisp1',
>              '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..ba7ff754
> --- /dev/null
> +++ b/src/libcamera/pipeline/virtual/meson.build
> @@ -0,0 +1,5 @@
> +# SPDX-License-Identifier: CC0-1.0
> +
> +libcamera_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..78616fab
> --- /dev/null
> +++ b/src/libcamera/pipeline/virtual/virtual.cpp
> @@ -0,0 +1,302 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2022, Google Inc.
> + *
> + * fake.cpp - Pipeline handler for fake cameras
> + */

This is not called virtual, please update this and the copyright year
maybe

> +
> +#include <libcamera/base/log.h>
> +
> +#include <libcamera/camera.h>
> +#include <libcamera/control_ids.h>
> +#include <libcamera/controls.h>
> +#include <libcamera/formats.h>
> +#include <libcamera/heap_allocator.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)

I would use "Virtual"

> +
> +static const ControlInfoMap::Map VirtualControls = {
> +	{ &controls::draft::PipelineDepth, ControlInfo(2, 3) },

Is this just for testing ?

> +};
> +
> +uint64_t CurrentTimestamp()

function names start with lowecase letter
if you want a static helper please place it in an anonymous namespace

> +{
> +	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;
> +}
> +
> +class VirtualCameraData : public Camera::Private
> +{
> +public:
> +	struct Resolution {
> +		Size size;
> +		std::vector<int> frame_rates;
> +		std::vector<std::string> formats;
> +	};
> +	VirtualCameraData(PipelineHandler *pipe)
> +		: Camera::Private(pipe)
> +	{
> +	}
> +
> +	~VirtualCameraData() = default;
> +
> +	std::vector<Resolution> supportedResolutions_;

Do you plan to initialize this based on some configuration file ?
Currently it is dynamically initialized at match() time with static
data, so it could very well be a static const class member.

> +
> +	Stream stream_;
> +};
> +
> +class VirtualCameraConfiguration : public CameraConfiguration
> +{
> +public:
> +	static constexpr unsigned int kBufferCount = 4; // 4~6

In the whole file, please do not use C++ comment style, we use C99
ones. Also, this comment seems a development leftover so please drop
it

> +
> +	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,
> +								   const StreamRoles &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:
> +	VirtualCameraData *cameraData(Camera *camera)
> +	{
> +		return static_cast<VirtualCameraData *>(camera->_d());
> +	}
> +
> +	HeapAllocator heapAllocator_;
> +};
> +
> +VirtualCameraConfiguration::VirtualCameraConfiguration(VirtualCameraData *data)
> +	: CameraConfiguration(), data_(data)
> +{
> +}
> +
> +CameraConfiguration::Status VirtualCameraConfiguration::validate()
> +{
> +	Status status = Valid;
> +
> +	if (config_.empty()) {
> +		LOG(VIRTUAL, Error) << "Empty config";
> +		return Invalid;
> +	}
> +
> +	// TODO: check if we should limit |config_.size()|

Please use \todo as the rest of the codebase does

And yes, you only have one stream, so you should resize(1) and Adjust
Otherwise, you should make VirtualCameraData::stream_ an array of
multiple entries if you want to support multiple streams

> +
> +	Size maxSize;
> +	for (const auto &resolution : data_->supportedResolutions_)
> +		maxSize = std::max(maxSize, resolution.size);

if supportedResolutions_ is constructed at match() time and never
changed at run-time you could very well find the max and min element
once at match() time

> +
> +	for (StreamConfiguration &cfg : config_) {

And you can spare this cycle if you only support one stream

> +		bool found = false;
> +		for (const auto &resolution : data_->supportedResolutions_) {
> +			if (resolution.size.width >= cfg.size.width && resolution.size.height >= cfg.size.height) {

Please break this rather long line

Also, is this correct ? You "found = true" when you hit a resolution
 >= than the requested one. Shouldn't you "Adjust" if the resolution is
not exactly equal to the one you're looking for ?

> +				found = true;
> +				break;
> +			}
> +		}
> +
> +		if (!found) {
> +			cfg.size = maxSize;
> +			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.setStream(const_cast<Stream *>(&data_->stream_));
> +
> +		cfg.bufferCount = VirtualCameraConfiguration::kBufferCount;
> +	}
> +
> +	return status;
> +}
> +
> +PipelineHandlerVirtual::PipelineHandlerVirtual(CameraManager *manager)
> +	: PipelineHandler(manager)
> +{
> +}
> +
> +std::unique_ptr<CameraConfiguration> PipelineHandlerVirtual::generateConfiguration(Camera *camera,
> +										   const StreamRoles &roles)
> +{
> +	VirtualCameraData *data = cameraData(camera);
> +	auto config =
> +		std::make_unique<VirtualCameraConfiguration>(data);
> +
> +	if (roles.empty())
> +		return config;

Like in validate() if you only support one stream you should only
consider a single role

> +
> +	Size minSize, sensorResolution;
> +	for (const auto &resolution : data->supportedResolutions_) {
> +		if (minSize.isNull() || minSize > resolution.size)
> +			minSize = resolution.size;
> +
> +		sensorResolution = std::max(sensorResolution, resolution.size);
> +	}
> +
> +	for (const StreamRole role : roles) {
> +		std::map<PixelFormat, std::vector<SizeRange>> streamFormats;
> +		unsigned int bufferCount;
> +		PixelFormat pixelFormat;
> +
> +		switch (role) {
> +		case StreamRole::StillCapture:
> +			pixelFormat = formats::NV12;
> +			bufferCount = VirtualCameraConfiguration::kBufferCount;
> +			streamFormats[pixelFormat] = { { minSize, sensorResolution } };
> +
> +			break;
> +
> +		case StreamRole::Raw: {
> +			// TODO: check
> +			pixelFormat = formats::SBGGR10;
> +			bufferCount = VirtualCameraConfiguration::kBufferCount;
> +			streamFormats[pixelFormat] = { { minSize, sensorResolution } };
> +
> +			break;
> +		}
> +
> +		case StreamRole::Viewfinder:
> +		case StreamRole::VideoRecording: {
> +			pixelFormat = formats::NV12;
> +			bufferCount = VirtualCameraConfiguration::kBufferCount;
> +			streamFormats[pixelFormat] = { { minSize, sensorResolution } };
> +
> +			break;
> +		}
> +
> +		default:
> +			LOG(VIRTUAL, Error)
> +				<< "Requested stream role not supported: " << role;
> +			config.reset();
> +			return config;
> +		}
> +
> +		StreamFormats formats(streamFormats);
> +		StreamConfiguration cfg(formats);
> +		cfg.size = sensorResolution;
> +		cfg.pixelFormat = pixelFormat;
> +		cfg.bufferCount = bufferCount;
> +		config->addConfiguration(cfg);
> +	}
> +
> +	if (config->validate() == CameraConfiguration::Invalid) {
> +		config.reset();
> +		return config;

you can remove this line as you would return config anyway a line
below

> +	}
> +
> +	return config;
> +}
> +
> +int PipelineHandlerVirtual::configure(Camera *camera, CameraConfiguration *config)
> +{
> +	(void)camera;
> +	(void)config;

please use [[maybe_unused]] for not used parameters

> +	// Nothing to be done.
> +	return 0;
> +}
> +
> +int PipelineHandlerVirtual::exportFrameBuffers(Camera *camera, Stream *stream,
> +					       std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> +{
> +	if (!heapAllocator_.isValid())
> +		return -ENOBUFS;
> +
> +	return heapAllocator_.exportFrameBuffers(camera, stream, buffers);
> +}
> +
> +int PipelineHandlerVirtual::start(Camera *camera, const ControlList *controls)
> +{
> +	(void)camera;
> +	(void)controls;
> +	// TODO: Start reading the virtual video if any.
> +	return 0;
> +}
> +
> +void PipelineHandlerVirtual::stopDevice(Camera *camera)
> +{
> +	(void)camera;
> +	// TODO: Reset the virtual video if any.
> +}
> +
> +int PipelineHandlerVirtual::queueRequestDevice(Camera *camera, Request *request)
> +{
> +	(void)camera;
> +
> +	// 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;
> +}
> +
> +bool PipelineHandlerVirtual::match(DeviceEnumerator *enumerator)
> +{
> +	(void)enumerator;
> +
> +	// TODO: Add virtual cameras according to a config file.
> +
> +	std::unique_ptr<VirtualCameraData> data = std::make_unique<VirtualCameraData>(this);
> +
> +	data->supportedResolutions_.resize(2);
> +	data->supportedResolutions_[0] = { .size = Size(1920, 1080), .frame_rates = { 30 }, .formats = { "YCbCr_420_888" } };
> +	data->supportedResolutions_[1] = { .size = Size(1280, 720), .frame_rates = { 30, 60 }, .formats = { "YCbCr_420_888" } };

Not sure what you will use "formats" for, but those are the Android
format names. Could you use the libcamera ones ?

Thanks
   j

> +
> +	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 = VirtualControls;
> +	int64_t min_frame_duration = 30, max_frame_duration = 60;
> +	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{ &data->stream_ };
> +	const std::string id = "Virtual0";
> +	std::shared_ptr<Camera> camera = Camera::create(std::move(data), id, streams);
> +	registerCamera(std::move(camera));
> +
> +	return false; // Prevent infinite loops for now
> +}
> +
> +REGISTER_PIPELINE_HANDLER(PipelineHandlerVirtual)
> +
> +} /* namespace libcamera */
> --
> 2.40.0
>

Thanks Jacopo,

The fixes are applied to v6.

On Mon, May 15, 2023 at 3:20 PM Jacopo Mondi <jacopo.mondi@ideasonboard.com>
wrote:

> Hi Harvey
>
>    mostly cosmetic comments, my concern is on 1/2 and I wuold like to
> hear from others, maybe it has been discussed in the past, but it
> worries me a bit.
>
> On Fri, Apr 07, 2023 at 06:20:50PM +0800, Harvey Yang via libcamera-devel
> wrote:
> > Add VirtualPipelineHandler for more unit tests and verfiy libcamera
> > infrastructure works on devices without using hardware cameras.
> >
> > Updating `/etc/camera/libcamera/camera_hal.yaml` on a chromebook DUT is
> > required to find the virtual camera with id `Virtual0`.
>
> Why ? For the Android HAL ? If that's the case this is not related to
> this patch and you can drop this line
>
>
Yes, it's only for chromeos that uses android adapter. Removed.


> >
> > [todo Read frames from the virtual video if any]
>
> I think you can drop this as well
>
>
Removed.


> >
> > Test the configurations can be generated and reported with cam -I:
> > """
> > build/src/apps/cam/cam -c 1 -I
> > [45:19:28.901456135] [2611530]  INFO IPAManager ipa_manager.cpp:143
> > libcamera is not installed. Adding
> >
> '/usr/local/google/home/chenghaoyang/cros2/src/third_party/libcamera/build/src
> > /ipa' to the IPA search path
> > [45:19:28.904364465] [2611530]  INFO Camera camera_manager.cpp:293
> > libcamera v0.0.1+56-4f4554fa-dirty (2022-12-07T06:55:04+00:00)
> > Using camera Virtual0 as cam0
> > 0: 1920x1080-NV12
> >  * Pixelformat: NV12 (1280x720)-(1920x1080)/(+1,+1)
> >   - 1280x720
> >   - 1280x800
> >   - 1360x768
> >   - 1366x768
> >   - 1440x900
> >   - 1280x1024
> >   - 1536x864
> >   - 1280x1080
> >   - 1600x900
> >   - 1400x1050
> >   - 1680x1050
> >   - 1920x1080
> > """
> >
> > """
> > build/src/apps/cam/cam -l
> > [550:47:04.505850647] [1969734]  INFO IPAManager ipa_manager.cpp:143
> > libcamera is not installed. Adding ... to the IPA search path
> > [550:47:04.509307667] [1969734]  INFO Camera camera_manager.cpp:293
> > libcamera v0.0.1+54-55fecb4d-dirty (2022-12-01T05:47:11+00:00)
> > Available cameras:
> > 1: (Virtual0)
> > """
> >
> > Using qcam should receive blank (all green) frames:
> > """
> > build/src/apps/qcam/qcam -c Virtual0
> > """
>
> All the examples are better placed in the cover letter imho, there's
> no need to have them in the commit history ?
>
>
Removed.


> >
> > 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 | 302 +++++++++++++++++++++
> >  4 files changed, 310 insertions(+), 1 deletion(-)
> >  create mode 100644 src/libcamera/pipeline/virtual/meson.build
> >  create mode 100644 src/libcamera/pipeline/virtual/virtual.cpp
> >
> > diff --git a/meson.build b/meson.build
> > index 0f89b45a..c2c4ba5f 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -177,6 +177,7 @@ pipelines_support = {
> >      'simple':       arch_arm,
> >      'uvcvideo':     ['any'],
> >      'vimc':         ['test'],
> > +    'virtual':      ['test'],
> >  }
> >
> >  if pipelines.contains('all')
> > diff --git a/meson_options.txt b/meson_options.txt
> > index 78a78b72..a0a75e7f 100644
> > --- a/meson_options.txt
> > +++ b/meson_options.txt
> > @@ -47,7 +47,8 @@ option('pipelines',
> >              'rkisp1',
> >              '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..ba7ff754
> > --- /dev/null
> > +++ b/src/libcamera/pipeline/virtual/meson.build
> > @@ -0,0 +1,5 @@
> > +# SPDX-License-Identifier: CC0-1.0
> > +
> > +libcamera_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..78616fab
> > --- /dev/null
> > +++ b/src/libcamera/pipeline/virtual/virtual.cpp
> > @@ -0,0 +1,302 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2022, Google Inc.
> > + *
> > + * fake.cpp - Pipeline handler for fake cameras
> > + */
>
> This is not called virtual, please update this and the copyright year
> maybe
>
>
Done


> > +
> > +#include <libcamera/base/log.h>
> > +
> > +#include <libcamera/camera.h>
> > +#include <libcamera/control_ids.h>
> > +#include <libcamera/controls.h>
> > +#include <libcamera/formats.h>
> > +#include <libcamera/heap_allocator.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)
>
> I would use "Virtual"
>
>
Done


> > +
> > +static const ControlInfoMap::Map VirtualControls = {
> > +     { &controls::draft::PipelineDepth, ControlInfo(2, 3) },
>
> Is this just for testing ?
>
>
Removed.


> > +};
> > +
> > +uint64_t CurrentTimestamp()
>
> function names start with lowecase letter
> if you want a static helper please place it in an anonymous namespace
>
>
Done


> > +{
> > +     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;
> > +}
> > +
> > +class VirtualCameraData : public Camera::Private
> > +{
> > +public:
> > +     struct Resolution {
> > +             Size size;
> > +             std::vector<int> frame_rates;
> > +             std::vector<std::string> formats;
> > +     };
> > +     VirtualCameraData(PipelineHandler *pipe)
> > +             : Camera::Private(pipe)
> > +     {
> > +     }
> > +
> > +     ~VirtualCameraData() = default;
> > +
> > +     std::vector<Resolution> supportedResolutions_;
>
> Do you plan to initialize this based on some configuration file ?
> Currently it is dynamically initialized at match() time with static
> data, so it could very well be a static const class member.
>
>
Yes, we'll initialize this with a configuration file in the following
patches.
Therefore, let's keep it as a member variable for now.


> > +
> > +     Stream stream_;
> > +};
> > +
> > +class VirtualCameraConfiguration : public CameraConfiguration
> > +{
> > +public:
> > +     static constexpr unsigned int kBufferCount = 4; // 4~6
>
> In the whole file, please do not use C++ comment style, we use C99
> ones. Also, this comment seems a development leftover so please drop
> it
>
>
Dropped


> > +
> > +     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,
> > +                                                                const
> StreamRoles &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:
> > +     VirtualCameraData *cameraData(Camera *camera)
> > +     {
> > +             return static_cast<VirtualCameraData *>(camera->_d());
> > +     }
> > +
> > +     HeapAllocator heapAllocator_;
> > +};
> > +
> >
> +VirtualCameraConfiguration::VirtualCameraConfiguration(VirtualCameraData
> *data)
> > +     : CameraConfiguration(), data_(data)
> > +{
> > +}
> > +
> > +CameraConfiguration::Status VirtualCameraConfiguration::validate()
> > +{
> > +     Status status = Valid;
> > +
> > +     if (config_.empty()) {
> > +             LOG(VIRTUAL, Error) << "Empty config";
> > +             return Invalid;
> > +     }
> > +
> > +     // TODO: check if we should limit |config_.size()|
>
> Please use \todo as the rest of the codebase does
>
>
Done


> And yes, you only have one stream, so you should resize(1) and Adjust
> Otherwise, you should make VirtualCameraData::stream_ an array of
> multiple entries if you want to support multiple streams
>
>
Updated and added a comment.


> > +
> > +     Size maxSize;
> > +     for (const auto &resolution : data_->supportedResolutions_)
> > +             maxSize = std::max(maxSize, resolution.size);
>
> if supportedResolutions_ is constructed at match() time and never
> changed at run-time you could very well find the max and min element
> once at match() time
>
>
Hmm, I'd prefer to keep the same source of truth. It doesn't affect the
performance that much as well. WDYT?


> > +
> > +     for (StreamConfiguration &cfg : config_) {
>
> And you can spare this cycle if you only support one stream
>
>
I'd prefer to keep this, in case we support more than one stream in the
future.


> > +             bool found = false;
> > +             for (const auto &resolution :
> data_->supportedResolutions_) {
> > +                     if (resolution.size.width >= cfg.size.width &&
> resolution.size.height >= cfg.size.height) {
>
> Please break this rather long line
>
>
Done


> Also, is this correct ? You "found = true" when you hit a resolution
>  >= than the requested one. Shouldn't you "Adjust" if the resolution is
> not exactly equal to the one you're looking for ?
>

You're right. Updated.


> > +                             found = true;
> > +                             break;
> > +                     }
> > +             }
> > +
> > +             if (!found) {
> > +                     cfg.size = maxSize;
> > +                     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.setStream(const_cast<Stream *>(&data_->stream_));
> > +
> > +             cfg.bufferCount = VirtualCameraConfiguration::kBufferCount;
> > +     }
> > +
> > +     return status;
> > +}
> > +
> > +PipelineHandlerVirtual::PipelineHandlerVirtual(CameraManager *manager)
> > +     : PipelineHandler(manager)
> > +{
> > +}
> > +
> > +std::unique_ptr<CameraConfiguration>
> PipelineHandlerVirtual::generateConfiguration(Camera *camera,
> > +
>         const StreamRoles &roles)
> > +{
> > +     VirtualCameraData *data = cameraData(camera);
> > +     auto config =
> > +             std::make_unique<VirtualCameraConfiguration>(data);
> > +
> > +     if (roles.empty())
> > +             return config;
>
> Like in validate() if you only support one stream you should only
> consider a single role
>
>
I think it's a different thing here: We currently only support only one
stream, but the StreamRole of it can be one of {StillCapture, Raw,
ViewFinder, VideoRecorder}. Should the caller get the default
configuration of each StreamRole one by one, or it can get all four
by once?


> > +
> > +     Size minSize, sensorResolution;
> > +     for (const auto &resolution : data->supportedResolutions_) {
> > +             if (minSize.isNull() || minSize > resolution.size)
> > +                     minSize = resolution.size;
> > +
> > +             sensorResolution = std::max(sensorResolution,
> resolution.size);
> > +     }
> > +
> > +     for (const StreamRole role : roles) {
> > +             std::map<PixelFormat, std::vector<SizeRange>>
> streamFormats;
> > +             unsigned int bufferCount;
> > +             PixelFormat pixelFormat;
> > +
> > +             switch (role) {
> > +             case StreamRole::StillCapture:
> > +                     pixelFormat = formats::NV12;
> > +                     bufferCount =
> VirtualCameraConfiguration::kBufferCount;
> > +                     streamFormats[pixelFormat] = { { minSize,
> sensorResolution } };
> > +
> > +                     break;
> > +
> > +             case StreamRole::Raw: {
> > +                     // TODO: check
> > +                     pixelFormat = formats::SBGGR10;
> > +                     bufferCount =
> VirtualCameraConfiguration::kBufferCount;
> > +                     streamFormats[pixelFormat] = { { minSize,
> sensorResolution } };
> > +
> > +                     break;
> > +             }
> > +
> > +             case StreamRole::Viewfinder:
> > +             case StreamRole::VideoRecording: {
> > +                     pixelFormat = formats::NV12;
> > +                     bufferCount =
> VirtualCameraConfiguration::kBufferCount;
> > +                     streamFormats[pixelFormat] = { { minSize,
> sensorResolution } };
> > +
> > +                     break;
> > +             }
> > +
> > +             default:
> > +                     LOG(VIRTUAL, Error)
> > +                             << "Requested stream role not supported: "
> << role;
> > +                     config.reset();
> > +                     return config;
> > +             }
> > +
> > +             StreamFormats formats(streamFormats);
> > +             StreamConfiguration cfg(formats);
> > +             cfg.size = sensorResolution;
> > +             cfg.pixelFormat = pixelFormat;
> > +             cfg.bufferCount = bufferCount;
> > +             config->addConfiguration(cfg);
> > +     }
> > +
> > +     if (config->validate() == CameraConfiguration::Invalid) {
> > +             config.reset();
> > +             return config;
>
> you can remove this line as you would return config anyway a line
> below
>
>
Done.


> > +     }
> > +
> > +     return config;
> > +}
> > +
> > +int PipelineHandlerVirtual::configure(Camera *camera,
> CameraConfiguration *config)
> > +{
> > +     (void)camera;
> > +     (void)config;
>
> please use [[maybe_unused]] for not used parameters
>
>
Done


> > +     // Nothing to be done.
> > +     return 0;
> > +}
> > +
> > +int PipelineHandlerVirtual::exportFrameBuffers(Camera *camera, Stream
> *stream,
> > +
> std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> > +{
> > +     if (!heapAllocator_.isValid())
> > +             return -ENOBUFS;
> > +
> > +     return heapAllocator_.exportFrameBuffers(camera, stream, buffers);
> > +}
> > +
> > +int PipelineHandlerVirtual::start(Camera *camera, const ControlList
> *controls)
> > +{
> > +     (void)camera;
> > +     (void)controls;
> > +     // TODO: Start reading the virtual video if any.
> > +     return 0;
> > +}
> > +
> > +void PipelineHandlerVirtual::stopDevice(Camera *camera)
> > +{
> > +     (void)camera;
> > +     // TODO: Reset the virtual video if any.
> > +}
> > +
> > +int PipelineHandlerVirtual::queueRequestDevice(Camera *camera, Request
> *request)
> > +{
> > +     (void)camera;
> > +
> > +     // 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;
> > +}
> > +
> > +bool PipelineHandlerVirtual::match(DeviceEnumerator *enumerator)
> > +{
> > +     (void)enumerator;
> > +
> > +     // TODO: Add virtual cameras according to a config file.
> > +
> > +     std::unique_ptr<VirtualCameraData> data =
> std::make_unique<VirtualCameraData>(this);
> > +
> > +     data->supportedResolutions_.resize(2);
> > +     data->supportedResolutions_[0] = { .size = Size(1920, 1080),
> .frame_rates = { 30 }, .formats = { "YCbCr_420_888" } };
> > +     data->supportedResolutions_[1] = { .size = Size(1280, 720),
> .frame_rates = { 30, 60 }, .formats = { "YCbCr_420_888" } };
>
> Not sure what you will use "formats" for, but those are the Android
> format names. Could you use the libcamera ones ?
>
>
Removed `formats` in `VirtualCameraData::Resolution`.


> Thanks
>    j
>
> > +
> > +     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 = VirtualControls;
> > +     int64_t min_frame_duration = 30, max_frame_duration = 60;
> > +     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{ &data->stream_ };
> > +     const std::string id = "Virtual0";
> > +     std::shared_ptr<Camera> camera = Camera::create(std::move(data),
> id, streams);
> > +     registerCamera(std::move(camera));
> > +
> > +     return false; // Prevent infinite loops for now
> > +}
> > +
> > +REGISTER_PIPELINE_HANDLER(PipelineHandlerVirtual)
> > +
> > +} /* namespace libcamera */
> > --
> > 2.40.0
> >
>