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

21417 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 | 317 +++++++++++++++++++++
 src/libcamera/pipeline/virtual/virtual.h   |  45 +++
 5 files changed, 370 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

Quoting Harvey Yang (2024-09-30 07:29:44)
> 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 | 317 +++++++++++++++++++++
>  src/libcamera/pipeline/virtual/virtual.h   |  45 +++
>  5 files changed, 370 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 63e45465..5e533b0c 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..d1584f59
> --- /dev/null
> +++ b/src/libcamera/pipeline/virtual/virtual.cpp
> @@ -0,0 +1,317 @@
> +/* 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 <array>
> +#include <chrono>
> +#include <errno.h>
> +#include <map>
> +#include <memory>
> +#include <ostream>
> +#include <set>
> +#include <stdint.h>
> +#include <string>
> +#include <time.h>
> +#include <utility>
> +#include <vector>
> +
> +#include <libcamera/base/flags.h>
> +#include <libcamera/base/log.h>
> +
> +#include <libcamera/control_ids.h>
> +#include <libcamera/controls.h>
> +#include <libcamera/formats.h>
> +#include <libcamera/pixel_format.h>
> +#include <libcamera/property_ids.h>
> +#include <libcamera/request.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();
> +}
> +
> +} /* 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_;

Could this be 
	static bool created_ = false;

saving the need for the outer initialisation below?

> +
> +       VirtualCameraData *cameraData(Camera *camera)
> +       {
> +               return static_cast<VirtualCameraData *>(camera->_d());
> +       }
> +
> +       DmaBufAllocator dmaBufAllocator_;
> +};
> +
> +/* static */
> +bool PipelineHandlerVirtual::created_ = false;

This stands out as a curious way to do this initialisation, but I
wouldn't block on this here ...

I'm curious why we 'need' a static created_ and how that will be used
though...


> +
> +VirtualCameraData::VirtualCameraData(PipelineHandler *pipe,
> +                                    const std::vector<Resolution> &supportedResolutions)
> +       : Camera::Private(pipe), supportedResolutions_(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;
> +               }
> +
> +               if (cfg.pixelFormat != formats::NV12) {
> +                       cfg.pixelFormat = formats::NV12;
> +                       LOG(Virtual, Debug)
> +                               << "Stream configuration adjusted to " << cfg.toString();

Wouldn't it be worth reporting this if any adjustment is made?

I'd probably have put this at the end of this scope as 
			if (status == Adjusted)
                               LOG(Virtual, Info)
                                   << "Stream configuration adjusted to " << cfg.toString();


And I'd think 'info' would be suitable here over debug - as this is a
test / virtual pipeline handler so the additional log would be
beneficial IMO.



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

I think if you return false here, you don't need any of the created_; 

	/*
	 * Ensure we only ever register and create a single virtual
	 * pipeline handler.
	 */
	return false;

This is core libcamera not your patch, - I still dislike this return
usage of match(). It's either not clear or not fitting well to the needs
of most pipeline handlers.

It's should be clearer at core that returning true here simply means
'please try to create more pipeline handlers'




Anyway, comments above could be applied on top if you wish - or handled
how you prefer.

The created_ just seems like an awkward workaround, but it's a
workaround for trying to return the right thing at the end of match. But
I think the 'right thing' to return there is poorly defined maybe...

Anyway, with any of the above handled or not - or handled on top - none
of that blocks this patch for me.


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>


> +}
> +
> +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..4df70a13
> --- /dev/null
> +++ b/src/libcamera/pipeline/virtual/virtual.h
> @@ -0,0 +1,45 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2024, Google Inc.
> + *
> + * virtual.h - Pipeline handler for virtual cameras
> + */
> +
> +#pragma once
> +
> +#include <vector>
> +
> +#include <libcamera/geometry.h>
> +#include <libcamera/stream.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,
> +                         const std::vector<Resolution> &supportedResolutions);
> +
> +       ~VirtualCameraData() = default;
> +
> +       const std::vector<Resolution> supportedResolutions_;
> +       Size maxResolutionSize_;
> +       Size minResolutionSize_;
> +
> +       std::vector<StreamConfig> streamConfigs_;
> +};
> +
> +} /* namespace libcamera */
> -- 
> 2.46.1.824.gd892dcdcdd-goog
>

Quoting Kieran Bingham (2024-10-03 09:38:26)
> Quoting Harvey Yang (2024-09-30 07:29:44)
> > 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>
> > ---
... snip ...

> > +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;
> 
> I think if you return false here, you don't need any of the created_; 
> 
>         /*
>          * Ensure we only ever register and create a single virtual
>          * pipeline handler.
>          */
>         return false;
> 
> This is core libcamera not your patch, - I still dislike this return
> usage of match(). It's either not clear or not fitting well to the needs
> of most pipeline handlers.
> 
> It's should be clearer at core that returning true here simply means
> 'please try to create more pipeline handlers'

Hrm - Jacopo has highlighted that if the 'successful' creation returns
false - then there would be no output that the pipeline handler had
matched successfully ...

So given that - what you have is correct...

I'd like it to be different but that requires core pipeline handler
iteration rework ;-) so lets not dwell on that right now.


> Anyway, comments above could be applied on top if you wish - or handled
> how you prefer.
> 
> The created_ just seems like an awkward workaround, but it's a
> workaround for trying to return the right thing at the end of match. But
> I think the 'right thing' to return there is poorly defined maybe...
> 
> Anyway, with any of the above handled or not - or handled on top - none
> of that blocks this patch for me.
> 
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Hi Kieran,

On Thu, Oct 3, 2024 at 4:38 PM Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> Quoting Harvey Yang (2024-09-30 07:29:44)
> > 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 | 317 +++++++++++++++++++++
> >  src/libcamera/pipeline/virtual/virtual.h   |  45 +++
> >  5 files changed, 370 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 63e45465..5e533b0c 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..d1584f59
> > --- /dev/null
> > +++ b/src/libcamera/pipeline/virtual/virtual.cpp
> > @@ -0,0 +1,317 @@
> > +/* 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 <array>
> > +#include <chrono>
> > +#include <errno.h>
> > +#include <map>
> > +#include <memory>
> > +#include <ostream>
> > +#include <set>
> > +#include <stdint.h>
> > +#include <string>
> > +#include <time.h>
> > +#include <utility>
> > +#include <vector>
> > +
> > +#include <libcamera/base/flags.h>
> > +#include <libcamera/base/log.h>
> > +
> > +#include <libcamera/control_ids.h>
> > +#include <libcamera/controls.h>
> > +#include <libcamera/formats.h>
> > +#include <libcamera/pixel_format.h>
> > +#include <libcamera/property_ids.h>
> > +#include <libcamera/request.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();
> > +}
> > +
> > +} /* 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_;
>
> Could this be
>         static bool created_ = false;
>
> saving the need for the outer initialisation below?

Unfortunately, there will be a compile error:
`ISO C++ forbids in-class initialization of non-const static member`
, so let's keep it this way.

>
> > +
> > +       VirtualCameraData *cameraData(Camera *camera)
> > +       {
> > +               return static_cast<VirtualCameraData *>(camera->_d());
> > +       }
> > +
> > +       DmaBufAllocator dmaBufAllocator_;
> > +};
> > +
> > +/* static */
> > +bool PipelineHandlerVirtual::created_ = false;
>
> This stands out as a curious way to do this initialisation, but I
> wouldn't block on this here ...
>
> I'm curious why we 'need' a static created_ and how that will be used
> though...
>
>
> > +
> > +VirtualCameraData::VirtualCameraData(PipelineHandler *pipe,
> > +                                    const std::vector<Resolution> &supportedResolutions)
> > +       : Camera::Private(pipe), supportedResolutions_(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;
> > +               }
> > +
> > +               if (cfg.pixelFormat != formats::NV12) {
> > +                       cfg.pixelFormat = formats::NV12;
> > +                       LOG(Virtual, Debug)
> > +                               << "Stream configuration adjusted to " << cfg.toString();
>
> Wouldn't it be worth reporting this if any adjustment is made?
>
> I'd probably have put this at the end of this scope as
>                         if (status == Adjusted)
>                                LOG(Virtual, Info)
>                                    << "Stream configuration adjusted to " << cfg.toString();

As we share the same `status` among all `StreamConfiguration`s,
I think it might be better to add an info log for each place we might
adjust a StreamConfiguration?

Let me know if you think we should have a local variable of status
for each StreamConfiguration.


>
>
> And I'd think 'info' would be suitable here over debug - as this is a
> test / virtual pipeline handler so the additional log would be
> beneficial IMO.
>
>
>
> > +                       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;
> > +       }
> > +
> > +       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) {
> > +               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_ } };
> > +               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)
> > +{
> > +       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;
>
> I think if you return false here, you don't need any of the created_;
>
>         /*
>          * Ensure we only ever register and create a single virtual
>          * pipeline handler.
>          */
>         return false;
>
> This is core libcamera not your patch, - I still dislike this return
> usage of match(). It's either not clear or not fitting well to the needs
> of most pipeline handlers.
>
> It's should be clearer at core that returning true here simply means
> 'please try to create more pipeline handlers'
>
>
>
>
> Anyway, comments above could be applied on top if you wish - or handled
> how you prefer.
>
> The created_ just seems like an awkward workaround, but it's a
> workaround for trying to return the right thing at the end of match. But
> I think the 'right thing' to return there is poorly defined maybe...
>
> Anyway, with any of the above handled or not - or handled on top - none
> of that blocks this patch for me.
>
>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
>
> > +}
> > +
> > +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..4df70a13
> > --- /dev/null
> > +++ b/src/libcamera/pipeline/virtual/virtual.h
> > @@ -0,0 +1,45 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2024, Google Inc.
> > + *
> > + * virtual.h - Pipeline handler for virtual cameras
> > + */
> > +
> > +#pragma once
> > +
> > +#include <vector>
> > +
> > +#include <libcamera/geometry.h>
> > +#include <libcamera/stream.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,
> > +                         const std::vector<Resolution> &supportedResolutions);
> > +
> > +       ~VirtualCameraData() = default;
> > +
> > +       const std::vector<Resolution> supportedResolutions_;
> > +       Size maxResolutionSize_;
> > +       Size minResolutionSize_;
> > +
> > +       std::vector<StreamConfig> streamConfigs_;
> > +};
> > +
> > +} /* namespace libcamera */
> > --
> > 2.46.1.824.gd892dcdcdd-goog
> >

Quoting Cheng-Hao Yang (2024-10-03 10:16:15)
> Hi Kieran,
> 
> On Thu, Oct 3, 2024 at 4:38 PM Kieran Bingham
> <kieran.bingham@ideasonboard.com> wrote:
> >
> > Quoting Harvey Yang (2024-09-30 07:29:44)
> > > 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 | 317 +++++++++++++++++++++
> > >  src/libcamera/pipeline/virtual/virtual.h   |  45 +++
> > >  5 files changed, 370 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 63e45465..5e533b0c 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..d1584f59
> > > --- /dev/null
> > > +++ b/src/libcamera/pipeline/virtual/virtual.cpp
> > > @@ -0,0 +1,317 @@
> > > +/* 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 <array>
> > > +#include <chrono>
> > > +#include <errno.h>
> > > +#include <map>
> > > +#include <memory>
> > > +#include <ostream>
> > > +#include <set>
> > > +#include <stdint.h>
> > > +#include <string>
> > > +#include <time.h>
> > > +#include <utility>
> > > +#include <vector>
> > > +
> > > +#include <libcamera/base/flags.h>
> > > +#include <libcamera/base/log.h>
> > > +
> > > +#include <libcamera/control_ids.h>
> > > +#include <libcamera/controls.h>
> > > +#include <libcamera/formats.h>
> > > +#include <libcamera/pixel_format.h>
> > > +#include <libcamera/property_ids.h>
> > > +#include <libcamera/request.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();
> > > +}
> > > +
> > > +} /* 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_;
> >
> > Could this be
> >         static bool created_ = false;
> >
> > saving the need for the outer initialisation below?
> 
> Unfortunately, there will be a compile error:
> `ISO C++ forbids in-class initialization of non-const static member`
> , so let's keep it this way.
> 

Ack.

> >
> > > +
> > > +       VirtualCameraData *cameraData(Camera *camera)
> > > +       {
> > > +               return static_cast<VirtualCameraData *>(camera->_d());
> > > +       }
> > > +
> > > +       DmaBufAllocator dmaBufAllocator_;
> > > +};
> > > +
> > > +/* static */
> > > +bool PipelineHandlerVirtual::created_ = false;
> >
> > This stands out as a curious way to do this initialisation, but I
> > wouldn't block on this here ...
> >
> > I'm curious why we 'need' a static created_ and how that will be used
> > though...
> >
> >
> > > +
> > > +VirtualCameraData::VirtualCameraData(PipelineHandler *pipe,
> > > +                                    const std::vector<Resolution> &supportedResolutions)
> > > +       : Camera::Private(pipe), supportedResolutions_(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;
> > > +               }
> > > +
> > > +               if (cfg.pixelFormat != formats::NV12) {
> > > +                       cfg.pixelFormat = formats::NV12;
> > > +                       LOG(Virtual, Debug)
> > > +                               << "Stream configuration adjusted to " << cfg.toString();
> >
> > Wouldn't it be worth reporting this if any adjustment is made?
> >
> > I'd probably have put this at the end of this scope as
> >                         if (status == Adjusted)
> >                                LOG(Virtual, Info)
> >                                    << "Stream configuration adjusted to " << cfg.toString();
> 
> As we share the same `status` among all `StreamConfiguration`s,
> I think it might be better to add an info log for each place we might
> adjust a StreamConfiguration?
> 
> Let me know if you think we should have a local variable of status
> for each StreamConfiguration.
> 

The output of  "Stream configuration adjusted to " << cfg.toString();
should say the full new output configuration right ? I.e. already
including the size if the resolution wasn't found and defaulted to
data_->maxResolutionSize_;

But I think that only needs to be printed once.

> > And I'd think 'info' would be suitable here over debug - as this is a
> > test / virtual pipeline handler so the additional log would be
> > beneficial IMO.
> >
> >
> >
> > > +                       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;
> > > +       }
> > > +
> > > +       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) {
> > > +               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_ } };
> > > +               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)
> > > +{
> > > +       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;
> >
> > I think if you return false here, you don't need any of the created_;
> >
> >         /*
> >          * Ensure we only ever register and create a single virtual
> >          * pipeline handler.
> >          */
> >         return false;
> >
> > This is core libcamera not your patch, - I still dislike this return
> > usage of match(). It's either not clear or not fitting well to the needs
> > of most pipeline handlers.
> >
> > It's should be clearer at core that returning true here simply means
> > 'please try to create more pipeline handlers'
> >
> >
> >
> >
> > Anyway, comments above could be applied on top if you wish - or handled
> > how you prefer.
> >
> > The created_ just seems like an awkward workaround, but it's a
> > workaround for trying to return the right thing at the end of match. But
> > I think the 'right thing' to return there is poorly defined maybe...
> >
> > Anyway, with any of the above handled or not - or handled on top - none
> > of that blocks this patch for me.
> >
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >
> >
> > > +}
> > > +
> > > +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..4df70a13
> > > --- /dev/null
> > > +++ b/src/libcamera/pipeline/virtual/virtual.h
> > > @@ -0,0 +1,45 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2024, Google Inc.
> > > + *
> > > + * virtual.h - Pipeline handler for virtual cameras
> > > + */
> > > +
> > > +#pragma once
> > > +
> > > +#include <vector>
> > > +
> > > +#include <libcamera/geometry.h>
> > > +#include <libcamera/stream.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,
> > > +                         const std::vector<Resolution> &supportedResolutions);
> > > +
> > > +       ~VirtualCameraData() = default;
> > > +
> > > +       const std::vector<Resolution> supportedResolutions_;
> > > +       Size maxResolutionSize_;
> > > +       Size minResolutionSize_;
> > > +
> > > +       std::vector<StreamConfig> streamConfigs_;
> > > +};
> > > +
> > > +} /* namespace libcamera */
> > > --
> > > 2.46.1.824.gd892dcdcdd-goog
> > >

Hi Kieran,

On Thu, Oct 3, 2024 at 5:50 PM Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> Quoting Cheng-Hao Yang (2024-10-03 10:16:15)
> > Hi Kieran,
> >
> > On Thu, Oct 3, 2024 at 4:38 PM Kieran Bingham
> > <kieran.bingham@ideasonboard.com> wrote:
> > >
> > > Quoting Harvey Yang (2024-09-30 07:29:44)
> > > > 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 | 317 +++++++++++++++++++++
> > > >  src/libcamera/pipeline/virtual/virtual.h   |  45 +++
> > > >  5 files changed, 370 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 63e45465..5e533b0c 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..d1584f59
> > > > --- /dev/null
> > > > +++ b/src/libcamera/pipeline/virtual/virtual.cpp
> > > > @@ -0,0 +1,317 @@
> > > > +/* 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 <array>
> > > > +#include <chrono>
> > > > +#include <errno.h>
> > > > +#include <map>
> > > > +#include <memory>
> > > > +#include <ostream>
> > > > +#include <set>
> > > > +#include <stdint.h>
> > > > +#include <string>
> > > > +#include <time.h>
> > > > +#include <utility>
> > > > +#include <vector>
> > > > +
> > > > +#include <libcamera/base/flags.h>
> > > > +#include <libcamera/base/log.h>
> > > > +
> > > > +#include <libcamera/control_ids.h>
> > > > +#include <libcamera/controls.h>
> > > > +#include <libcamera/formats.h>
> > > > +#include <libcamera/pixel_format.h>
> > > > +#include <libcamera/property_ids.h>
> > > > +#include <libcamera/request.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();
> > > > +}
> > > > +
> > > > +} /* 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_;
> > >
> > > Could this be
> > >         static bool created_ = false;
> > >
> > > saving the need for the outer initialisation below?
> >
> > Unfortunately, there will be a compile error:
> > `ISO C++ forbids in-class initialization of non-const static member`
> > , so let's keep it this way.
> >
>
> Ack.
>
> > >
> > > > +
> > > > +       VirtualCameraData *cameraData(Camera *camera)
> > > > +       {
> > > > +               return static_cast<VirtualCameraData *>(camera->_d());
> > > > +       }
> > > > +
> > > > +       DmaBufAllocator dmaBufAllocator_;
> > > > +};
> > > > +
> > > > +/* static */
> > > > +bool PipelineHandlerVirtual::created_ = false;
> > >
> > > This stands out as a curious way to do this initialisation, but I
> > > wouldn't block on this here ...
> > >
> > > I'm curious why we 'need' a static created_ and how that will be used
> > > though...
> > >
> > >
> > > > +
> > > > +VirtualCameraData::VirtualCameraData(PipelineHandler *pipe,
> > > > +                                    const std::vector<Resolution> &supportedResolutions)
> > > > +       : Camera::Private(pipe), supportedResolutions_(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;
> > > > +               }
> > > > +
> > > > +               if (cfg.pixelFormat != formats::NV12) {
> > > > +                       cfg.pixelFormat = formats::NV12;
> > > > +                       LOG(Virtual, Debug)
> > > > +                               << "Stream configuration adjusted to " << cfg.toString();
> > >
> > > Wouldn't it be worth reporting this if any adjustment is made?
> > >
> > > I'd probably have put this at the end of this scope as
> > >                         if (status == Adjusted)
> > >                                LOG(Virtual, Info)
> > >                                    << "Stream configuration adjusted to " << cfg.toString();
> >
> > As we share the same `status` among all `StreamConfiguration`s,
> > I think it might be better to add an info log for each place we might
> > adjust a StreamConfiguration?
> >
> > Let me know if you think we should have a local variable of status
> > for each StreamConfiguration.
> >
>
> The output of  "Stream configuration adjusted to " << cfg.toString();
> should say the full new output configuration right ? I.e. already
> including the size if the resolution wasn't found and defaulted to
> data_->maxResolutionSize_;
>
> But I think that only needs to be printed once.

Yeah that's right, `cfg.toString()` contains the full output.
So, do you mean that you prefer to keep an extra variable in the
for loop to record if the current StreamConfiguration is changed?
It's only needed for multi-stream support though, but I want to
make sure the log still makes sense when we enable multiple
streams in the future.

BR,
Harvey

>
> > > And I'd think 'info' would be suitable here over debug - as this is a
> > > test / virtual pipeline handler so the additional log would be
> > > beneficial IMO.
> > >
> > >
> > >
> > > > +                       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;
> > > > +       }
> > > > +
> > > > +       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) {
> > > > +               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_ } };
> > > > +               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)
> > > > +{
> > > > +       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;
> > >
> > > I think if you return false here, you don't need any of the created_;
> > >
> > >         /*
> > >          * Ensure we only ever register and create a single virtual
> > >          * pipeline handler.
> > >          */
> > >         return false;
> > >
> > > This is core libcamera not your patch, - I still dislike this return
> > > usage of match(). It's either not clear or not fitting well to the needs
> > > of most pipeline handlers.
> > >
> > > It's should be clearer at core that returning true here simply means
> > > 'please try to create more pipeline handlers'
> > >
> > >
> > >
> > >
> > > Anyway, comments above could be applied on top if you wish - or handled
> > > how you prefer.
> > >
> > > The created_ just seems like an awkward workaround, but it's a
> > > workaround for trying to return the right thing at the end of match. But
> > > I think the 'right thing' to return there is poorly defined maybe...
> > >
> > > Anyway, with any of the above handled or not - or handled on top - none
> > > of that blocks this patch for me.
> > >
> > >
> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > >
> > >
> > > > +}
> > > > +
> > > > +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..4df70a13
> > > > --- /dev/null
> > > > +++ b/src/libcamera/pipeline/virtual/virtual.h
> > > > @@ -0,0 +1,45 @@
> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > +/*
> > > > + * Copyright (C) 2024, Google Inc.
> > > > + *
> > > > + * virtual.h - Pipeline handler for virtual cameras
> > > > + */
> > > > +
> > > > +#pragma once
> > > > +
> > > > +#include <vector>
> > > > +
> > > > +#include <libcamera/geometry.h>
> > > > +#include <libcamera/stream.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,
> > > > +                         const std::vector<Resolution> &supportedResolutions);
> > > > +
> > > > +       ~VirtualCameraData() = default;
> > > > +
> > > > +       const std::vector<Resolution> supportedResolutions_;
> > > > +       Size maxResolutionSize_;
> > > > +       Size minResolutionSize_;
> > > > +
> > > > +       std::vector<StreamConfig> streamConfigs_;
> > > > +};
> > > > +
> > > > +} /* namespace libcamera */
> > > > --
> > > > 2.46.1.824.gd892dcdcdd-goog
> > > >

Quoting Cheng-Hao Yang (2024-10-03 10:58:45)
> Hi Kieran,
> 
> On Thu, Oct 3, 2024 at 5:50 PM Kieran Bingham
> <kieran.bingham@ideasonboard.com> wrote:
> >
> > Quoting Cheng-Hao Yang (2024-10-03 10:16:15)
> > > Hi Kieran,
> > >
> > > On Thu, Oct 3, 2024 at 4:38 PM Kieran Bingham
> > > <kieran.bingham@ideasonboard.com> wrote:
> > > >
> > > > Quoting Harvey Yang (2024-09-30 07:29:44)
> > > > > 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 | 317 +++++++++++++++++++++
> > > > >  src/libcamera/pipeline/virtual/virtual.h   |  45 +++
> > > > >  5 files changed, 370 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 63e45465..5e533b0c 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..d1584f59
> > > > > --- /dev/null
> > > > > +++ b/src/libcamera/pipeline/virtual/virtual.cpp
> > > > > @@ -0,0 +1,317 @@
> > > > > +/* 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 <array>
> > > > > +#include <chrono>
> > > > > +#include <errno.h>
> > > > > +#include <map>
> > > > > +#include <memory>
> > > > > +#include <ostream>
> > > > > +#include <set>
> > > > > +#include <stdint.h>
> > > > > +#include <string>
> > > > > +#include <time.h>
> > > > > +#include <utility>
> > > > > +#include <vector>
> > > > > +
> > > > > +#include <libcamera/base/flags.h>
> > > > > +#include <libcamera/base/log.h>
> > > > > +
> > > > > +#include <libcamera/control_ids.h>
> > > > > +#include <libcamera/controls.h>
> > > > > +#include <libcamera/formats.h>
> > > > > +#include <libcamera/pixel_format.h>
> > > > > +#include <libcamera/property_ids.h>
> > > > > +#include <libcamera/request.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();
> > > > > +}
> > > > > +
> > > > > +} /* 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_;
> > > >
> > > > Could this be
> > > >         static bool created_ = false;
> > > >
> > > > saving the need for the outer initialisation below?
> > >
> > > Unfortunately, there will be a compile error:
> > > `ISO C++ forbids in-class initialization of non-const static member`
> > > , so let's keep it this way.
> > >
> >
> > Ack.
> >
> > > >
> > > > > +
> > > > > +       VirtualCameraData *cameraData(Camera *camera)
> > > > > +       {
> > > > > +               return static_cast<VirtualCameraData *>(camera->_d());
> > > > > +       }
> > > > > +
> > > > > +       DmaBufAllocator dmaBufAllocator_;
> > > > > +};
> > > > > +
> > > > > +/* static */
> > > > > +bool PipelineHandlerVirtual::created_ = false;
> > > >
> > > > This stands out as a curious way to do this initialisation, but I
> > > > wouldn't block on this here ...
> > > >
> > > > I'm curious why we 'need' a static created_ and how that will be used
> > > > though...
> > > >
> > > >
> > > > > +
> > > > > +VirtualCameraData::VirtualCameraData(PipelineHandler *pipe,
> > > > > +                                    const std::vector<Resolution> &supportedResolutions)
> > > > > +       : Camera::Private(pipe), supportedResolutions_(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;
> > > > > +               }
> > > > > +
> > > > > +               if (cfg.pixelFormat != formats::NV12) {
> > > > > +                       cfg.pixelFormat = formats::NV12;
> > > > > +                       LOG(Virtual, Debug)
> > > > > +                               << "Stream configuration adjusted to " << cfg.toString();
> > > >
> > > > Wouldn't it be worth reporting this if any adjustment is made?
> > > >
> > > > I'd probably have put this at the end of this scope as
> > > >                         if (status == Adjusted)
> > > >                                LOG(Virtual, Info)
> > > >                                    << "Stream configuration adjusted to " << cfg.toString();
> > >
> > > As we share the same `status` among all `StreamConfiguration`s,
> > > I think it might be better to add an info log for each place we might
> > > adjust a StreamConfiguration?
> > >
> > > Let me know if you think we should have a local variable of status
> > > for each StreamConfiguration.
> > >
> >
> > The output of  "Stream configuration adjusted to " << cfg.toString();
> > should say the full new output configuration right ? I.e. already
> > including the size if the resolution wasn't found and defaulted to
> > data_->maxResolutionSize_;
> >
> > But I think that only needs to be printed once.
> 
> Yeah that's right, `cfg.toString()` contains the full output.
> So, do you mean that you prefer to keep an extra variable in the
> for loop to record if the current StreamConfiguration is changed?
> It's only needed for multi-stream support though, but I want to
> make sure the log still makes sense when we enable multiple
> streams in the future.

When you enable multiple streams, then yes - I think if you are going to
return Adjusted here, it would be worth reporting the entirety of

`StreamConfiguration &cfg`

in the log.

But right now - this only affects a single stream.

--
Kieran

Hi Kieran,

On Thu, Oct 3, 2024 at 6:08 PM Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> Quoting Cheng-Hao Yang (2024-10-03 10:58:45)
> > Hi Kieran,
> >
> > On Thu, Oct 3, 2024 at 5:50 PM Kieran Bingham
> > <kieran.bingham@ideasonboard.com> wrote:
> > >
> > > Quoting Cheng-Hao Yang (2024-10-03 10:16:15)
> > > > Hi Kieran,
> > > >
> > > > On Thu, Oct 3, 2024 at 4:38 PM Kieran Bingham
> > > > <kieran.bingham@ideasonboard.com> wrote:
> > > > >
> > > > > Quoting Harvey Yang (2024-09-30 07:29:44)
> > > > > > 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 | 317 +++++++++++++++++++++
> > > > > >  src/libcamera/pipeline/virtual/virtual.h   |  45 +++
> > > > > >  5 files changed, 370 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 63e45465..5e533b0c 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..d1584f59
> > > > > > --- /dev/null
> > > > > > +++ b/src/libcamera/pipeline/virtual/virtual.cpp
> > > > > > @@ -0,0 +1,317 @@
> > > > > > +/* 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 <array>
> > > > > > +#include <chrono>
> > > > > > +#include <errno.h>
> > > > > > +#include <map>
> > > > > > +#include <memory>
> > > > > > +#include <ostream>
> > > > > > +#include <set>
> > > > > > +#include <stdint.h>
> > > > > > +#include <string>
> > > > > > +#include <time.h>
> > > > > > +#include <utility>
> > > > > > +#include <vector>
> > > > > > +
> > > > > > +#include <libcamera/base/flags.h>
> > > > > > +#include <libcamera/base/log.h>
> > > > > > +
> > > > > > +#include <libcamera/control_ids.h>
> > > > > > +#include <libcamera/controls.h>
> > > > > > +#include <libcamera/formats.h>
> > > > > > +#include <libcamera/pixel_format.h>
> > > > > > +#include <libcamera/property_ids.h>
> > > > > > +#include <libcamera/request.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();
> > > > > > +}
> > > > > > +
> > > > > > +} /* 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_;
> > > > >
> > > > > Could this be
> > > > >         static bool created_ = false;
> > > > >
> > > > > saving the need for the outer initialisation below?
> > > >
> > > > Unfortunately, there will be a compile error:
> > > > `ISO C++ forbids in-class initialization of non-const static member`
> > > > , so let's keep it this way.
> > > >
> > >
> > > Ack.
> > >
> > > > >
> > > > > > +
> > > > > > +       VirtualCameraData *cameraData(Camera *camera)
> > > > > > +       {
> > > > > > +               return static_cast<VirtualCameraData *>(camera->_d());
> > > > > > +       }
> > > > > > +
> > > > > > +       DmaBufAllocator dmaBufAllocator_;
> > > > > > +};
> > > > > > +
> > > > > > +/* static */
> > > > > > +bool PipelineHandlerVirtual::created_ = false;
> > > > >
> > > > > This stands out as a curious way to do this initialisation, but I
> > > > > wouldn't block on this here ...
> > > > >
> > > > > I'm curious why we 'need' a static created_ and how that will be used
> > > > > though...
> > > > >
> > > > >
> > > > > > +
> > > > > > +VirtualCameraData::VirtualCameraData(PipelineHandler *pipe,
> > > > > > +                                    const std::vector<Resolution> &supportedResolutions)
> > > > > > +       : Camera::Private(pipe), supportedResolutions_(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;
> > > > > > +               }
> > > > > > +
> > > > > > +               if (cfg.pixelFormat != formats::NV12) {
> > > > > > +                       cfg.pixelFormat = formats::NV12;
> > > > > > +                       LOG(Virtual, Debug)
> > > > > > +                               << "Stream configuration adjusted to " << cfg.toString();
> > > > >
> > > > > Wouldn't it be worth reporting this if any adjustment is made?
> > > > >
> > > > > I'd probably have put this at the end of this scope as
> > > > >                         if (status == Adjusted)
> > > > >                                LOG(Virtual, Info)
> > > > >                                    << "Stream configuration adjusted to " << cfg.toString();
> > > >
> > > > As we share the same `status` among all `StreamConfiguration`s,
> > > > I think it might be better to add an info log for each place we might
> > > > adjust a StreamConfiguration?
> > > >
> > > > Let me know if you think we should have a local variable of status
> > > > for each StreamConfiguration.
> > > >
> > >
> > > The output of  "Stream configuration adjusted to " << cfg.toString();
> > > should say the full new output configuration right ? I.e. already
> > > including the size if the resolution wasn't found and defaulted to
> > > data_->maxResolutionSize_;
> > >
> > > But I think that only needs to be printed once.
> >
> > Yeah that's right, `cfg.toString()` contains the full output.
> > So, do you mean that you prefer to keep an extra variable in the
> > for loop to record if the current StreamConfiguration is changed?
> > It's only needed for multi-stream support though, but I want to
> > make sure the log still makes sense when we enable multiple
> > streams in the future.
>
> When you enable multiple streams, then yes - I think if you are going to
> return Adjusted here, it would be worth reporting the entirety of
>
> `StreamConfiguration &cfg`
>
> in the log.
>
> But right now - this only affects a single stream.

Enabled multi-streams in the next version. Therefore, I added a
local variable that records if the current StreamConfiguration is
adjusted or not.

BR,
Harvey


>
> --
> Kieran