[PATCH v11 3/7] libcamera: virtual: Add VirtualPipelineHandler
Cheng-Hao Yang
chenghaoyang at chromium.org
Tue Sep 10 06:49:43 CEST 2024
Hi Jacopo,
On Mon, Sep 9, 2024 at 4:47 PM Jacopo Mondi <jacopo.mondi at ideasonboard.com>
wrote:
> Hi Harvey
>
> On Sat, Sep 07, 2024 at 02:28:28PM 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 | 267 +++++++++++++++++++++
> > src/libcamera/pipeline/virtual/virtual.h | 89 +++++++
> > 5 files changed, 364 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 432ae133..ff9a70cf 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..f85ec3dd
> > --- /dev/null
> > +++ b/src/libcamera/pipeline/virtual/virtual.cpp
> > @@ -0,0 +1,267 @@
> > +/* 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 <map>
> > +#include <memory>
> > +#include <set>
>
> #include <string>
>
> > +#include <utility>
> > +#include <vector>
> > +
> > +#include <libcamera/base/log.h>
> > +
> > +#include <libcamera/control_ids.h>
> > +#include <libcamera/controls.h>
> > +#include <libcamera/formats.h>
> > +#include <libcamera/property_ids.h>
> > +
> > +#include "libcamera/internal/camera.h"
> > +#include "libcamera/internal/formats.h"
> > +#include "libcamera/internal/pipeline_handler.h"
> > +
> > +namespace libcamera {
> > +
> > +LOG_DEFINE_CATEGORY(Virtual)
> > +
> > +namespace {
> > +
> > +uint64_t currentTimestamp()
>
> As suggested on v9, this could probably be replaced with a few lines
> of C++ using std::chrono. Have you tested it and found it not to be
> working ? Not a big deal though
>
Ah sorry... I struggled a lot with the user interface of emails and
patchwork, so from time to time I lose track of comments.
Yes, comparing the timestamps, those two approaches produce similar
timestamps. Updated in v12.
I think we can keep the function `currentTimestamp` though, just replacing
the implementation. Let me know if you think I should get rid of it as well.
>
> > +{
> > + 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;
> > +}
> > +
> > +} /* namespace */
> > +
> > +VirtualCameraData::VirtualCameraData(PipelineHandler *pipe,
> > + std::vector<Resolution>
> supportedResolutions)
> > + : Camera::Private(pipe),
> supportedResolutions_(std::move(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_) {
>
> Knowing that you're going to add support for multiple streams I think
> it's fine having a loop here already
>
>
Yes, thanks :)
> > + 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;
> > + }
> > +
> > + 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;
> > +
> > + if (cfg.pixelFormat != formats::NV12) {
> > + cfg.pixelFormat = formats::NV12;
> > + LOG(Virtual, Debug)
> > + << "Stream configuration adjusted to " <<
> cfg.toString();
> > + status = Adjusted;
> > + }
> > + }
> > +
> > + 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) {
>
> I presume the loop is here to support multiple streams too
>
>
Yes, exactly.
> > + 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;
> > +
> > + 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;
> > + }
>
> I would have placed the switch before populating cfg, but it's ok, you
> clear config_ anyway in case of errors
>
>
Right, the switch and the check on `role` can be done earlier.
Updated in v12.
> > +
> > + config->addConfiguration(cfg);
> > + }
> > +
> > + if (config->validate() == CameraConfiguration::Invalid)
> > + config.reset();
>
> If this happens, it's a programming error so you could even ASSERT
> here. No problem, the ipu3 pipeline does the same here.
>
>
Ah right, updated with ASSERT in v12.
> > +
> > + return config;
> > +}
> > +
> > +int PipelineHandlerVirtual::configure(Camera *camera,
> > + CameraConfiguration *config)
> > +{
> > + VirtualCameraData *data = cameraData(camera);
> > + for (size_t i = 0; i < config->size(); ++i)
>
> Or
> for (auto [i, c] : utils::enumerate(*config))
> c.setStream(&data->streamConfigs_[i].stream);
>
> not a big deal
>
Thanks! Updated in v12.
>
> > + config->at(i).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)
> > +{
> > + /* \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;
> > +}
> > +
> > +// static
>
> I'm not sure anymore how to get the message through: No C++ comments please
>
Sorry, updated.
>
> > +bool PipelineHandlerVirtual::created_ = false;
>
> You can move this to the beginng of the file if you prefer. Otherwise
> it's fine.
>
Done.
>
>
> > +
> > +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;
> > +}
> > +
> > +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..fb3dbcad
> > --- /dev/null
> > +++ b/src/libcamera/pipeline/virtual/virtual.h
> > @@ -0,0 +1,89 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2024, Google Inc.
> > + *
> > + * virtual.h - Pipeline handler for virtual cameras
> > + */
> > +
> > +#pragma once
> > +
> > +#include <memory>
> > +#include <vector>
> > +
> > +#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:
> > + const static unsigned int kMaxStream = 1;
> > +
> > + struct Resolution {
> > + Size size;
> > + std::vector<int> frameRates;
> > + };
> > + struct StreamConfig {
> > + Stream stream;
> > + };
> > +
> > + VirtualCameraData(PipelineHandler *pipe,
> > + std::vector<Resolution> supportedResolutions);
> > +
> > + ~VirtualCameraData() = default;
> > +
> > + const std::vector<Resolution> supportedResolutions_;
> > + Size maxResolutionSize_;
> > + Size minResolutionSize_;
> > +
> > + std::vector<StreamConfig> streamConfigs_;
> > +};
> > +
> > +class VirtualCameraConfiguration : public CameraConfiguration
>
> It seems to me only VirtualCameraData is used by the other files. The
> other classes declarations, as not used outside of virtual.cpp, could
> be moved there.
>
> Only nits here and there, with them addressed:
> Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
>
> Thanks
> j
>
> > +{
> > +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_;
> > +
> > + VirtualCameraData *cameraData(Camera *camera)
> > + {
> > + return static_cast<VirtualCameraData *>(camera->_d());
> > + }
> > +
> > + DmaBufAllocator dmaBufAllocator_;
> > +};
> > +
> > +} /* namespace libcamera */
> > --
> > 2.46.0.469.g59c65b2a67-goog
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20240910/a12561e1/attachment.htm>
More information about the libcamera-devel
mailing list