[PATCH v9 3/8] libcamera: pipeline: Add VirtualPipelineHandler
Cheng-Hao Yang
chenghaoyang at chromium.org
Sat Sep 7 16:34:01 CEST 2024
Hi Jacopo,
On Sat, Aug 31, 2024 at 8:44 PM Jacopo Mondi <jacopo.mondi at ideasonboard.com>
wrote:
> 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 ?
>
>
Sure, will be updated in v11. Please take another look.
> >
> > >
> > > > +
> > > > + 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.
>
Nah, I prepared for multiple streams, but haven't enabled it yet :p.
>
> Please drop RAW, yes.
>
Done in v10.
>
> >
> > >
> > > > + 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
> > > >
> > >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20240907/b57f4f11/attachment.htm>
More information about the libcamera-devel
mailing list