[PATCH v9 3/8] libcamera: pipeline: Add VirtualPipelineHandler
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Sat Aug 31 14:44:40 CEST 2024
Hi
On Thu, Aug 29, 2024 at 09:57:58PM GMT, Cheng-Hao Yang wrote:
> Thanks for the review, Jacopo!
>
> On Tue, Aug 27, 2024 at 5:43 PM Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> wrote:
>
> > Hi Harvey
> >
> > On Tue, Aug 20, 2024 at 04:23:34PM GMT, Harvey Yang wrote:
> > > 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>
> > > ---
> > > meson.build | 1 +
> > > meson_options.txt | 3 +-
> > > src/libcamera/pipeline/virtual/meson.build | 5 +
> > > src/libcamera/pipeline/virtual/virtual.cpp | 251 +++++++++++++++++++++
> > > src/libcamera/pipeline/virtual/virtual.h | 78 +++++++
> >
> > Do you expect other components to include this header in future ? If
> > not, you can move its content to the .cpp file I guess
> >
> >
> Actually `virtual/parser.h` needs to include it to return VirtualCameraData
> when parsing the yaml config file [1]. Does that count :p?
I guess it does :)
>
> [1]: https://patchwork.libcamera.org/patch/20971/
>
> > 5 files changed, 337 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 f946eba94..3cad3249a 100644
> > > --- a/meson.build
> > > +++ b/meson.build
> > > @@ -222,6 +222,7 @@ pipelines_support = {
> > > 'simple': arch_arm,
> > > 'uvcvideo': ['any'],
> > > 'vimc': ['test'],
> > > + 'virtual': ['test'],
> > > }
> > >
> > > if pipelines.contains('all')
> > > diff --git a/meson_options.txt b/meson_options.txt
> > > index 7aa412491..c91cd241a 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 000000000..ba7ff754e
> > > --- /dev/null
> > > +++ b/src/libcamera/pipeline/virtual/meson.build
> > > @@ -0,0 +1,5 @@
> > > +# SPDX-License-Identifier: CC0-1.0
> > > +
> > > +libcamera_sources += files([
> > > + 'virtual.cpp',
> > > +])
> > > diff --git a/src/libcamera/pipeline/virtual/virtual.cpp
> > b/src/libcamera/pipeline/virtual/virtual.cpp
> > > new file mode 100644
> > > index 000000000..74eb8c7ad
> > > --- /dev/null
> > > +++ b/src/libcamera/pipeline/virtual/virtual.cpp
> > > @@ -0,0 +1,251 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2023, Google Inc.
> > > + *
> > > + * virtual.cpp - Pipeline handler for virtual cameras
> > > + */
> > > +
> >
> > You should include the header for the standard library construct you
> > use. I see vectors, maps, unique_ptrs etc
> >
> Done, please check again.
>
>
> >
> > > +#include "virtual.h"
> > > +
> > > +#include <libcamera/base/log.h>
> > > +
> > > +#include <libcamera/camera.h>
> > > +#include <libcamera/control_ids.h>
> > > +#include <libcamera/controls.h>
> > > +#include <libcamera/formats.h>
> > > +#include <libcamera/property_ids.h>
> > > +
> > > +#include "libcamera/internal/camera.h"
> >
> > The internal header includes the public one by definition
> >
> Ack. Removed the public one.
>
>
> >
> > > +#include "libcamera/internal/formats.h"
> >
> > This doesn't as <libcamera/formats.h> is generated. I wonder if it
> > should.
> >
> Keeping `#include <libcamera/formats.h>`.
>
>
> >
> > > +#include "libcamera/internal/pipeline_handler.h"
> > > +
> > > +namespace libcamera {
> > > +
> > > +LOG_DEFINE_CATEGORY(Virtual)
> > > +
> > > +namespace {
> > > +
> > > +uint64_t currentTimestamp()
> > > +{
> > > + struct timespec ts;
> > > + if (clock_gettime(CLOCK_MONOTONIC, &ts) < 0) {
> > > + LOG(Virtual, Error) << "Get clock time fails";
> > > + return 0;
> > > + }
> > > +
> > > + return ts.tv_sec * 1'000'000'000LL + ts.tv_nsec;
> > > +}
> >
> > Could https://en.cppreference.com/w/cpp/chrono/steady_clock/now save
> > you a custom function ?
> >
> > In example:
> >
> > const auto now = std::chrono::steady_clock::now();
> > auto nsecs =
> > std::chrono::duration_cast<std::chrono::nanoseconds>(now.time_since_epoch());
> > std::cout << nsecs.count();
> >
> > should give you the time in nanoseconds since system boot (if I got it
> > right)
> >
> >
> > > +
> > > +} // namespace
> >
> > nit: /* namespace */
> > here and in other places
> >
> > Done
>
>
> > > +
> > >
> > +VirtualCameraConfiguration::VirtualCameraConfiguration(VirtualCameraData
> > *data)
> > > + : CameraConfiguration(), data_(data)
> > > +{
> > > +}
> > > +
> > > +CameraConfiguration::Status VirtualCameraConfiguration::validate()
> > > +{
> > > + Status status = Valid;
> > > +
> > > + if (config_.empty()) {
> > > + LOG(Virtual, Error) << "Empty config";
> > > + return Invalid;
> > > + }
> > > +
> > > + /* Currently only one stream is supported */
> > > + if (config_.size() > 1) {
> > > + config_.resize(1);
> > > + status = Adjusted;
> > > + }
> > > +
> > > + Size maxSize;
> > > + for (const auto &resolution : data_->supportedResolutions_)
> > > + maxSize = std::max(maxSize, resolution.size);
> > > +
> > > + for (StreamConfiguration &cfg : config_) {
> >
> > you only have config, or in the next patches will this be augmented ?
> >
> > Do you mean that I should check `sensorConfig` or `orientation` as well?
>
No I meant I was assuming the virtual pipline works with a single
stream by design. But seeing your replies to the next patches makes me
think my assumption was wrong.
> > + 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) {
> > > + cfg.size = maxSize;
> > > + status = Adjusted;
> > > + }
> >
> > so it's either the exact resolution or the biggest available one ?
> >
> > As you have a single config it's easy to get the closest one to the
> > requested size, if it doesn't match exactly one of the supported
> > resolutions.
> >
>
> Hmm, I think it's a bit hard to define the "closest". Do we compare
> the area size directly? Do we prefer a size that has both larger or
> the same height and width?
>
Good question, it's generally a pipeline decision (which is not
optimal, as we should aim to a unified behaviour among pipelines).
As this is for testing, I think it's fine to keep what you have, but
could you add a comment to highlight this implementation decision ?
>
> >
> > > +
> > > + const PixelFormatInfo &info =
> > PixelFormatInfo::info(cfg.pixelFormat);
> > > + cfg.stride = info.stride(cfg.size.width, 0, 1);
> > > + cfg.frameSize = info.frameSize(cfg.size, 1);
> > > +
> > > + cfg.setStream(const_cast<Stream *>(&data_->stream_));
> > > +
> > > + cfg.bufferCount = VirtualCameraConfiguration::kBufferCount;
> > > + }
> > > +
> > > + return status;
> > > +}
> > > +
> > > +PipelineHandlerVirtual::PipelineHandlerVirtual(CameraManager *manager)
> > > + : PipelineHandler(manager)
> > > +{
> > > +}
> > > +
> > > +std::unique_ptr<CameraConfiguration>
> > > +PipelineHandlerVirtual::generateConfiguration(Camera *camera,
> > > + Span<const StreamRole> roles)
> > > +{
> > > + VirtualCameraData *data = cameraData(camera);
> > > + auto config =
> > > + std::make_unique<VirtualCameraConfiguration>(data);
> > > +
> > > + if (roles.empty())
> > > + return config;
> > > +
> > > + Size minSize, sensorResolution;
> > > + for (const auto &resolution : data->supportedResolutions_) {
> > > + if (minSize.isNull() || minSize > resolution.size)
> > > + minSize = resolution.size;
> > > +
> > > + sensorResolution = std::max(sensorResolution,
> > resolution.size);
> >
> > As you do this min/max search in a few places, why not doing it when
> > you construct data->supportedResolutions_ once ?
> >
> Added in VirtualCameraData.
>
>
> >
> > > + }
> > > +
> > > + for (const StreamRole role : roles) {
> >
> > If the pipeline handler works with a single stream, you should only
> > consider the first role maybe
> >
> Actually I think there's no reason to only support one Stream in
> Virtual Pipeline Handler. The raw stream doesn't seem to be
> properly supported in the following patches though. I think we
> should drop the support of raw.
>
I assumed (maybe from a previous discussion) you were going to have a
single stream. I'm sorry, I was wrong.
Please drop RAW, yes.
>
> >
> > > + std::map<PixelFormat, std::vector<SizeRange>>
> > streamFormats;
> > > + unsigned int bufferCount;
> > > + PixelFormat pixelFormat;
> > > +
> > > + switch (role) {
> > > + case StreamRole::StillCapture:
> > > + pixelFormat = formats::NV12;
> > > + bufferCount =
> > VirtualCameraConfiguration::kBufferCount;
> > > + streamFormats[pixelFormat] = { { minSize,
> > sensorResolution } };
> >
> > bufferCount and streamFormats can be assigned outsize of the cases,
> > they're the same for all roles
> >
> Done
>
>
> >
> > > +
> > > + break;
> > > +
> > > + case StreamRole::Raw: {
> > > + /* \todo check */
> > > + pixelFormat = formats::SBGGR10;
> > > + bufferCount =
> > VirtualCameraConfiguration::kBufferCount;
> > > + streamFormats[pixelFormat] = { { minSize,
> > sensorResolution } };
> > > +
> > > + break;
> > > + }
> > > +
> > > + case StreamRole::Viewfinder:
> > > + case StreamRole::VideoRecording: {
> > > + pixelFormat = formats::NV12;
> > > + bufferCount =
> > VirtualCameraConfiguration::kBufferCount;
> > > + streamFormats[pixelFormat] = { { minSize,
> > sensorResolution } };
> > > +
> > > + break;
> > > + }
> > > +
> > > + default:
> > > + LOG(Virtual, Error)
> > > + << "Requested stream role not supported: "
> > << role;
> > > + config.reset();
> > > + return config;
> > > + }
> > > +
> > > + StreamFormats formats(streamFormats);
> > > + StreamConfiguration cfg(formats);
> > > + cfg.size = sensorResolution;
> > > + cfg.pixelFormat = pixelFormat;
> > > + cfg.bufferCount = bufferCount;
> > > + config->addConfiguration(cfg);
> > > + }
> > > +
> > > + if (config->validate() == CameraConfiguration::Invalid)
> > > + config.reset();
> > > +
> > > + return config;
> > > +}
> > > +
> > > +int PipelineHandlerVirtual::configure(
> > > + [[maybe_unused]] Camera *camera,
> > > + [[maybe_unused]] CameraConfiguration *config)
> >
> > int PipelineHandlerVirtual::configure([[maybe_unused]] Camera *camera,
> > [[maybe_unused]] CameraConfiguration
> > *config)
> >
> > Done
>
>
> > > +{
> > > + // Nothing to be done.
> > > + return 0;
> > > +}
> > > +
> > > +int PipelineHandlerVirtual::exportFrameBuffers(
> > > + [[maybe_unused]] Camera *camera,
> > > + Stream *stream,
> > > + std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> >
> > int PipelineHandlerVirtual::exportFrameBuffers([[maybe_unused]] Camera
> > *camera,
> > Stream *stream,
> >
> > std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> >
> > if you prefer
> >
> > Done
>
>
> > > +{
> > > + if (!dmaBufAllocator_.isValid())
> > > + return -ENOBUFS;
> > > +
> > > + const StreamConfiguration &config = stream->configuration();
> > > +
> > > + auto info = PixelFormatInfo::info(config.pixelFormat);
> > > +
> > > + std::vector<std::size_t> 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);
> >
> > ah that's probably why you return count from
> > DmaBufferAllocator::exportBuffers()
> >
> Exactly :)
>
>
> >
> > > +}
> > > +
> > > +int PipelineHandlerVirtual::start([[maybe_unused]] Camera *camera,
> > > + [[maybe_unused]] const ControlList
> > *controls)
> > > +{
> > > + /* \todo Start reading the virtual video if any. */
> > > + return 0;
> > > +}
> > > +
> > > +void PipelineHandlerVirtual::stopDevice([[maybe_unused]] Camera *camera)
> > > +{
> > > + /* \todo Reset the virtual video if any. */
> > > +}
> > > +
> > > +int PipelineHandlerVirtual::queueRequestDevice([[maybe_unused]] Camera
> > *camera,
> > > + Request *request)
> > > +{
> > > + /* \todo Read from the virtual video if any. */
> > > + for (auto it : request->buffers())
> > > + completeBuffer(request, it.second);
> > > +
> > > + request->metadata().set(controls::SensorTimestamp,
> > currentTimestamp());
> > > + completeRequest(request);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +bool PipelineHandlerVirtual::match([[maybe_unused]] DeviceEnumerator
> > *enumerator)
> > > +{
> > > + /* \todo Add virtual cameras according to a config file. */
> > > +
> > > + std::unique_ptr<VirtualCameraData> data =
> > std::make_unique<VirtualCameraData>(this);
> > > +
> > > + data->supportedResolutions_.resize(2);
> > > + data->supportedResolutions_[0] = { .size = Size(1920, 1080),
> > .frame_rates = { 30 } };
> > > + data->supportedResolutions_[1] = { .size = Size(1280, 720),
> > .frame_rates = { 30, 60 } };
> > > +
> > > + 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 = 30, max_frame_duration = 60;
> >
> > doesn't match the above frame rates and it should be expressed in
> > microseconds. I would suggest for this patch to set both framerates to
> > 30 and initialize FrameDurationLimits with {33333, 333333}
> >
> > It's not a big deal however, it will be replaced later in the series
> >
> >
> Done, thanks!
>
>
> >
> > > + controls[&controls::FrameDurationLimits] =
> > ControlInfo(min_frame_duration, max_frame_duration);
> > > + data->controlInfo_ = ControlInfoMap(std::move(controls),
> > controls::controls);
> > > +
> > > + /* Create and register the camera. */
> > > + std::set<Stream *> streams{ &data->stream_ };
> > > + const std::string id = "Virtual0";
> > > + std::shared_ptr<Camera> camera = Camera::create(std::move(data),
> > id, streams);
> > > + registerCamera(std::move(camera));
> > > +
> > > + return false; // Prevent infinite loops for now
> >
> > I presume this will also be changed in next patches...
> >
> Updated in this patch, with a static boolean though.
>
>
> >
> > > +}
> > > +
> > > +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 000000000..6fc6b34d8
> > > --- /dev/null
> > > +++ b/src/libcamera/pipeline/virtual/virtual.h
> > > @@ -0,0 +1,78 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2023, Google Inc.
> > > + *
> > > + * virtual.h - Pipeline handler for virtual cameras
> > > + */
> > > +
> > > +#pragma once
> > > +
> > > +#include <libcamera/base/file.h>
> > > +
> > > +#include "libcamera/internal/camera.h"
> > > +#include "libcamera/internal/dma_buf_allocator.h"
> > > +#include "libcamera/internal/pipeline_handler.h"
> > > +
> > > +namespace libcamera {
> > > +
> > > +class VirtualCameraData : public Camera::Private
> > > +{
> > > +public:
> > > + struct Resolution {
> > > + Size size;
> > > + std::vector<int> frame_rates;
> > > + };
> > > + VirtualCameraData(PipelineHandler *pipe)
> > > + : Camera::Private(pipe)
> > > + {
> > > + }
> > > +
> > > + ~VirtualCameraData() = default;
> > > +
> > > + std::vector<Resolution> supportedResolutions_;
> > > +
> > > + Stream stream_;
> > > +};
> > > +
> > > +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:
> > > + VirtualCameraData *cameraData(Camera *camera)
> > > + {
> > > + return static_cast<VirtualCameraData *>(camera->_d());
> > > + }
> > > +
> > > + DmaBufAllocator dmaBufAllocator_;
> > > +};
> > > +
> > > +} // namespace libcamera
> > > --
> > > 2.46.0.184.g6999bdac58-goog
> > >
> >
More information about the libcamera-devel
mailing list