[PATCH v14 3/7] libcamera: virtual: Add VirtualPipelineHandler
Kieran Bingham
kieran.bingham at ideasonboard.com
Thu Oct 3 12:08:15 CEST 2024
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 at 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 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.
>
> 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
More information about the libcamera-devel
mailing list