[PATCH v14 3/7] libcamera: virtual: Add VirtualPipelineHandler
Kieran Bingham
kieran.bingham at ideasonboard.com
Thu Oct 3 11:50:40 CEST 2024
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 at ideasonboard.com> wrote:
> >
> > Quoting Harvey Yang (2024-09-30 07:29:44)
> > > From: Harvey Yang <chenghaoyang at 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 at chromium.org>
> > > Reviewed-by: Jacopo Mondi <jacopo.mondi at 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 at 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
> > >
More information about the libcamera-devel
mailing list