[PATCH v9 5/8] libcamera: pipeline: Read config and register cameras based on the config
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Sat Aug 31 14:54:28 CEST 2024
Hi Harvey
On Thu, Aug 29, 2024 at 09:58:01PM GMT, Cheng-Hao Yang wrote:
> Thanks Jacopo,
>
> On Tue, Aug 27, 2024 at 7:01 PM Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> wrote:
>
> > Hello
> >
> > On Tue, Aug 20, 2024 at 04:23:36PM GMT, Harvey Yang wrote:
> > > From: Konami Shu <konamiz at google.com>
> > >
> > > - Use Yaml::Parser to parse the config
> > > - Add config file at "virtual/data/virtual.yaml"
> > > - README.md contains the documentation for the format of config file and
> > > the implementation of Parser class.
> > > - Add Parser class to parse config file in Virtual
> > > pipeline handler
> > > - Add header file of virtual.cpp to use VirtualCameraData class in
> > > Parser class
> > > - Parse test patterns
> > > - Raise Error when the width of a resolution is an odd number
> >
> > Please make a commit message out of this list
> >
> Updated. Please check.
>
>
> >
> > >
> > > Signed-off-by: Konami Shu <konamiz at google.com>
> > > Co-developed-by: Harvey Yang <chenghaoyang at chromium.org>
> > > Co-developed-by: Yunke Cao <yunkec at chromium.org>
> > > Co-developed-by: Tomasz Figa <tfiga at chromium.org>
> > > ---
> > > src/libcamera/pipeline/virtual/README.md | 68 ++++++
> > > .../pipeline/virtual/data/virtual.yaml | 49 +++++
> > > src/libcamera/pipeline/virtual/meson.build | 1 +
> > > src/libcamera/pipeline/virtual/parser.cpp | 198 ++++++++++++++++++
> > > src/libcamera/pipeline/virtual/parser.h | 45 ++++
> > > src/libcamera/pipeline/virtual/virtual.cpp | 47 +++--
> > > src/libcamera/pipeline/virtual/virtual.h | 6 +-
> > > 7 files changed, 389 insertions(+), 25 deletions(-)
> > > create mode 100644 src/libcamera/pipeline/virtual/README.md
> > > create mode 100644 src/libcamera/pipeline/virtual/data/virtual.yaml
> > > create mode 100644 src/libcamera/pipeline/virtual/parser.cpp
> > > create mode 100644 src/libcamera/pipeline/virtual/parser.h
> > >
> > > diff --git a/src/libcamera/pipeline/virtual/README.md
> > b/src/libcamera/pipeline/virtual/README.md
> > > new file mode 100644
> > > index 000000000..27d6283df
> > > --- /dev/null
> > > +++ b/src/libcamera/pipeline/virtual/README.md
> >
> > [snip]
> >
> > > diff --git a/src/libcamera/pipeline/virtual/data/virtual.yaml
> > b/src/libcamera/pipeline/virtual/data/virtual.yaml
> > > new file mode 100644
> > > index 000000000..4eb239e24
> > > --- /dev/null
> > > +++ b/src/libcamera/pipeline/virtual/data/virtual.yaml
> > > @@ -0,0 +1,49 @@
> > > +# SPDX-License-Identifier: CC0-1.0
> > > +%YAML 1.1
> > > +---
> > > +"Virtual0":
> > > + supported_formats:
> > > + - width: 1920
> > > + height: 1080
> > > + frame_rates:
> > > + - 30
> > > + - 60
> > > + - width: 1680
> > > + height: 1050
> > > + frame_rates:
> > > + - 70
> > > + - 80
> > > + test_pattern: "lines"
> > > + location: "front"
> > > + model: "Virtual Video Device"
> > > +"Virtual1":
> > > + supported_formats:
> > > + - width: 800
> > > + height: 600
> > > + frame_rates:
> > > + - 30
> > > + - 60
> > > + test_pattern:
> > > + location: "back"
> > > + model: "Virtual Video Device1"
> > > +"Virtual2":
> > > + supported_formats:
> > > + - width: 100
> > > + height: 100
> > > + test_pattern: "lines"
> > > + location: "front"
> > > + model: "Virtual Video Device2"
> > > +"Virtual3":
> > > + supported_formats:
> > > + - width: 100
> > > + height: 100
> > > + - width: 800
> > > + height: 600
> > > + - width: 1920
> > > + height: 1080
> > > + frame_rates:
> > > + - 20
> > > + - 30
> > > + location: "a"
> >
> > I get an error here
> >
> > ERROR Virtual parser.cpp:221 location: a is not supported
> > ERROR Virtual parser.cpp:51 Failed to parse config of the camera: Virtual3
> >
> Right, I guess it was to check the parser catches the error and
> aborts parsing the camera, while still supporting other ones.
>
> Removed the camera in the config file.
>
>
> >
> > > + model: "Virtual Video Device3"
> > > +"Virtual4":
> >
> > Is this a valid entry ?
> >
> Actually, yes. Every field has a valid default value.
>
>
> >
> > > diff --git a/src/libcamera/pipeline/virtual/meson.build
> > b/src/libcamera/pipeline/virtual/meson.build
> > > index e1e65e68d..2e82e64cb 100644
> > > --- a/src/libcamera/pipeline/virtual/meson.build
> > > +++ b/src/libcamera/pipeline/virtual/meson.build
> > > @@ -3,6 +3,7 @@
> > > libcamera_sources += files([
> > > 'virtual.cpp',
> > > 'test_pattern_generator.cpp',
> > > + 'parser.cpp',
> > > ])
> > >
> > > libyuv_dep = dependency('libyuv', required : false)
> > > diff --git a/src/libcamera/pipeline/virtual/parser.cpp
> > b/src/libcamera/pipeline/virtual/parser.cpp
> > > new file mode 100644
> > > index 000000000..032c0cd9d
> > > --- /dev/null
> > > +++ b/src/libcamera/pipeline/virtual/parser.cpp
> > > @@ -0,0 +1,198 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2023, Google Inc.
> >
> > 2024
> >
> Done
>
>
> >
> > > + *
> > > + * parser.cpp - Virtual cameras helper to parse config file
> > > + */
> > > +
> > > +#include "parser.h"
> > > +
> > > +#include <memory>
> > > +#include <utility>
> > > +
> > > +#include <libcamera/base/log.h>
> > > +
> > > +#include <libcamera/control_ids.h>
> > > +#include <libcamera/property_ids.h>
> > > +
> > > +#include "libcamera/internal/pipeline_handler.h"
> > > +#include "libcamera/internal/yaml_parser.h"
> > > +
> > > +#include "virtual.h"
> > > +
> > > +namespace libcamera {
> > > +
> > > +LOG_DECLARE_CATEGORY(Virtual)
> > > +
> > > +std::vector<std::unique_ptr<VirtualCameraData>> Parser::parseConfigFile(
> > > + File &file, PipelineHandler *pipe)
> > > +{
> > > + std::vector<std::unique_ptr<VirtualCameraData>> configurations;
> > > +
> > > + std::unique_ptr<YamlObject> cameras = YamlParser::parse(file);
> > > + if (!cameras) {
> > > + LOG(Virtual, Error) << "Failed to pass config file.";
> > > + return configurations;
> > > + }
> > > +
> > > + if (!cameras->isDictionary()) {
> > > + LOG(Virtual, Error) << "Config file is not a dictionary at
> > the top level.";
> > > + return configurations;
> > > + }
> > > +
> > > + /* Look into the configuration of each camera */
> > > + for (const auto &[cameraId, cameraConfigData] : cameras->asDict())
> > {
> > > + std::unique_ptr<VirtualCameraData> data =
> > > + parseCameraConfigData(cameraConfigData, pipe);
> > > + /* Parse configData to data*/
> > > + if (!data) {
> > > + /* Skip the camera if it has invalid config */
> > > + LOG(Virtual, Error) << "Failed to parse config of
> > the camera: "
> > > + << cameraId;
> > > + continue;
> > > + }
> > > +
> > > + data->id_ = cameraId;
> > > + ControlInfoMap::Map controls;
> > > + /* todo: Check which resolution's frame rate to be
> > reported */
> > > + controls[&controls::FrameDurationLimits] =
> > > + ControlInfo(int64_t(1000 /
> > data->supportedResolutions_[0].frameRates[1]),
> > > + int64_t(1000 /
> > data->supportedResolutions_[0].frameRates[0]));
> >
> > FrameDurationLimits is expressed in useconds
> >
> Do you mean microseconds? Updated to be 1,000,000 / framerate.
>
>
> >
> > > + data->controlInfo_ = ControlInfoMap(std::move(controls),
> > controls::controls);
> > > + configurations.push_back(std::move(data));
> > > + }
> > > + return configurations;
> > > +}
> > > +
> > > +std::unique_ptr<VirtualCameraData> Parser::parseCameraConfigData(
> > > + const YamlObject &cameraConfigData, PipelineHandler *pipe)
> > > +{
> > > + std::unique_ptr<VirtualCameraData> data =
> > std::make_unique<VirtualCameraData>(pipe);
> > > +
> > > + if (parseSupportedFormats(cameraConfigData, data.get()))
> > > + return nullptr;
> > > +
> > > + if (parseTestPattern(cameraConfigData, data.get()))
> > > + return nullptr;
> > > +
> > > + if (parseLocation(cameraConfigData, data.get()))
> > > + return nullptr;
> > > +
> > > + if (parseModel(cameraConfigData, data.get()))
> > > + return nullptr;
> > > +
> > > + return data;
> > > +}
> > > +
> > > +int Parser::parseSupportedFormats(
> > > + const YamlObject &cameraConfigData, VirtualCameraData *data)
> > > +{
> > > + Size activeResolution{ 0, 0 };
> > > + if (cameraConfigData.contains("supported_formats")) {
> > > + const YamlObject &supportedResolutions =
> > cameraConfigData["supported_formats"];
> > > +
> > > + for (const YamlObject &supportedResolution :
> > supportedResolutions.asList()) {
> > > + unsigned int width =
> > supportedResolution["width"].get<unsigned int>(1920);
> > > + unsigned int height =
> > supportedResolution["height"].get<unsigned int>(1080);
> > > + if (width <= 0 || height <= 0) {
> >
> > How can they be < 0 ? It's unsigned and the default is 1920 or 1080 if
> > the property is not present
> >
> Right, updated to be `== 0`.
>
>
> >
> > Do you want to have the properties mandatory ? Make the default 0 and
> > reject it in this case.
> >
> Hmm, that's a good question. Currently we have all the default values valid.
> Do you think we should do the reverse?
>
>
> >
> > > + LOG(Virtual, Error) << "Invalid width
> > or/and height";
> > > + return -EINVAL;
> > > + }
> > > + if (width % 2 != 0) {
> > > + LOG(Virtual, Error) << "Invalid width:
> > width needs to be even";
> > > + return -EINVAL;
> > > + }
> > > +
> > > + std::vector<int> frameRates;
> > > + if (supportedResolution.contains("frame_rates")) {
> > > + auto frameRatesList =
> > > +
> > supportedResolution["frame_rates"].getList<int>().value();
> > > + if (frameRatesList.size() != 2) {
> > > + LOG(Virtual, Error) <<
> > "frame_rates needs to be the two edge values of a range";
> >
> > Can't a config support a single frame rate ?
> >
> In the current design, the upperBound equals to the lowerBound,
> while in the yaml file, the developer still needs to set two values.
> Do you think we should support only one value set in the config
> file?
I would say why not, but as this pipeline is just for testing, up to
you...
>
>
> > > + return -EINVAL;
> > > + }
> > > + if (frameRatesList[0] > frameRatesList[1])
> > {
> > > + LOG(Virtual, Error) <<
> > "frame_rates's first value(lower bound) is higher than the second
> > value(upper bound)";
> > > + return -EINVAL;
> > > + }
> > > + frameRates.push_back(frameRatesList[0]);
> > > + frameRates.push_back(frameRatesList[1]);
> > > + } else {
> > > + frameRates.push_back(30);
> > > + frameRates.push_back(60);
> > > + }
> > > +
> > > + data->supportedResolutions_.emplace_back(
> > > + VirtualCameraData::Resolution{ Size{
> > width, height },
> > > + frameRates
> > });
> > > +
> > > + activeResolution = std::max(activeResolution,
> > Size{ width, height });
> > > + }
> > > + } else {
> > > + data->supportedResolutions_.emplace_back(
> > > + VirtualCameraData::Resolution{ Size{ 1920, 1080 },
> > > + { 30, 60 } });
> > > + activeResolution = Size(1920, 1080);
> > > + }
> > > +
> > > + data->properties_.set(properties::PixelArrayActiveAreas,
> > > + { Rectangle(activeResolution) });
> >
> > Do you need this property ? Otherwise I would skip it as a camera max
> > resolution is often different from the sensor's pixel array size
> >
> Yes, actually the Android adapter requires this property to be set [1].
>
> [1]:
> https://git.libcamera.org/libcamera/libcamera.git/tree/src/android/camera_capabilities.cpp#n1085
>
>
> >
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +int Parser::parseTestPattern(
> > > + const YamlObject &cameraConfigData, VirtualCameraData *data)
> > > +{
> > > + std::string testPattern =
> > cameraConfigData["test_pattern"].get<std::string>().value();
> > > +
> > > + /* Default value is "bars" */
> > > + if (testPattern == "bars" || testPattern == "") {
> > > + data->testPattern_ = TestPattern::ColorBars;
> > > + } else if (testPattern == "lines") {
> > > + data->testPattern_ = TestPattern::DiagonalLines;
> > > + } else {
> > > + LOG(Virtual, Error) << "Test pattern: " << testPattern
> > > + << "is not supported";
> > > + return -EINVAL;
> > > + }
> > > + return 0;
> > > +}
> > > +
> > > +int Parser::parseLocation(
> > > + const YamlObject &cameraConfigData, VirtualCameraData *data)
> > > +{
> > > + std::string location =
> > cameraConfigData["location"].get<std::string>().value();
> > > +
> > > + /* Default value is properties::CameraLocationFront */
> > > + if (location == "front" || location == "") {
> > > + data->properties_.set(properties::Location,
> > > + properties::CameraLocationFront);
> > > + } else if (location == "back") {
> > > + data->properties_.set(properties::Location,
> > > + properties::CameraLocationBack);
> > > + } else {
> > > + LOG(Virtual, Error) << "location: " << location
> > > + << " is not supported";
> > > + return -EINVAL;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +int Parser::parseModel(
> > > + const YamlObject &cameraConfigData, VirtualCameraData *data)
> > > +{
> > > + std::string model =
> > > + cameraConfigData["model"].get<std::string>().value();
> > > +
> > > + /* Default value is "Unknown" */
> > > + if (model == "")
> > > + data->properties_.set(properties::Model, "Unknown");
> > > + else
> > > + data->properties_.set(properties::Model, model);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +} /* namespace libcamera */
> > > diff --git a/src/libcamera/pipeline/virtual/parser.h
> > b/src/libcamera/pipeline/virtual/parser.h
> > > new file mode 100644
> > > index 000000000..a377d8aa1
> > > --- /dev/null
> > > +++ b/src/libcamera/pipeline/virtual/parser.h
> > > @@ -0,0 +1,45 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2023, Google Inc.
> > > + *
> > > + * parser.h - Virtual cameras helper to parse config file
> > > + */
> > > +
> > > +#pragma once
> > > +
> > > +#include <memory>
> > > +#include <vector>
> > > +
> > > +#include <libcamera/base/file.h>
> > > +
> > > +#include "libcamera/internal/pipeline_handler.h"
> > > +#include "libcamera/internal/yaml_parser.h"
> > > +
> > > +#include "virtual.h"
> > > +
> > > +namespace libcamera {
> > > +
> > > +class Parser
> > > +{
> > > +public:
> > > + Parser() {}
> > > + ~Parser() = default;
> > > +
> > > + std::vector<std::unique_ptr<VirtualCameraData>>
> > > + parseConfigFile(File &file, PipelineHandler *pipe);
> > > +
> > > +private:
> > > + std::unique_ptr<VirtualCameraData> parseCameraConfigData(
> > > + const YamlObject &cameraConfigData, PipelineHandler *pipe);
> > > +
> > > + int parseSupportedFormats(
> > > + const YamlObject &cameraConfigData, VirtualCameraData
> > *data);
> > > + int parseTestPattern(
> > > + const YamlObject &cameraConfigData, VirtualCameraData
> > *data);
> > > + int parseLocation(
> > > + const YamlObject &cameraConfigData, VirtualCameraData
> > *data);
> > > + int parseModel(
> > > + const YamlObject &cameraConfigData, VirtualCameraData
> > *data);
> > > +};
> > > +
> > > +} // namespace libcamera
> > > diff --git a/src/libcamera/pipeline/virtual/virtual.cpp
> > b/src/libcamera/pipeline/virtual/virtual.cpp
> > > index 357fdd035..0fe471f00 100644
> > > --- a/src/libcamera/pipeline/virtual/virtual.cpp
> > > +++ b/src/libcamera/pipeline/virtual/virtual.cpp
> > > @@ -18,6 +18,10 @@
> > > #include "libcamera/internal/camera.h"
> > > #include "libcamera/internal/formats.h"
> > > #include "libcamera/internal/pipeline_handler.h"
> > > +#include "libcamera/internal/yaml_parser.h"
> > > +
> > > +#include "frame_generator.h"
> >
> > This should probably have been added in the previous patch
> >
> Removed.
>
>
> >
> > > +#include "parser.h"
> > >
> > > namespace libcamera {
> > >
> > > @@ -228,32 +232,31 @@ int
> > PipelineHandlerVirtual::queueRequestDevice([[maybe_unused]] Camera *camera,
> > >
> > > 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)) });
> > > + File file(configurationFile("virtual", "virtual.yaml"));
> >
> > I might have missed what 'configurationFile' is...
> >
> Sorry, I don't get what you mean. Do you mean that I should add
> a comment to explain how the path works [2]?
> [2]:
> https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline_handler.cpp#n558
I meant that I get a single hit for
$ git grep configurationFile src/libcamera/pipeline/virtual/
src/libcamera/pipeline/virtual/virtual.cpp: File file(configurationFile("virtual", "virtual.yaml"));
so I don't get what the "configuratioFile" symbol means here. It
compiles, so I am missing something for sure.
>
>
> >
> > > + bool isOpen = file.open(File::OpenModeFlag::ReadOnly);
> > > + if (!isOpen) {
> > > + LOG(Virtual, Error) << "Failed to open config file: " <<
> > file.fileName();
> > > + return false;
> > > + }
> > >
> > > - /* \todo Set FrameDurationLimits based on config. */
> > > - ControlInfoMap::Map controls;
> > > - int64_t min_frame_duration = 30, max_frame_duration = 60;
> > > - controls[&controls::FrameDurationLimits] =
> > ControlInfo(min_frame_duration, max_frame_duration);
> > > - data->controlInfo_ = ControlInfoMap(std::move(controls),
> > controls::controls);
> > > + Parser parser;
> > > + auto configData = parser.parseConfigFile(file, this);
> > > + if (configData.size() == 0) {
> > > + LOG(Virtual, Error) << "Failed to parse any cameras from
> > the config file: "
> > > + << file.fileName();
> > > + return false;
> > > + }
> > >
> > > - /* 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);
> > > + /* Configure and register cameras with configData */
> > > + for (auto &data : configData) {
> > > + std::set<Stream *> streams{ &data->stream_ };
> > > + std::string id = data->id_;
> > > + std::shared_ptr<Camera> camera =
> > Camera::create(std::move(data), id, streams);
> > >
> > > - initFrameGenerator(camera.get());
> > > + initFrameGenerator(camera.get());
> > >
> > > - registerCamera(std::move(camera));
> > > + registerCamera(std::move(camera));
> > > + }
> > >
> > > return false; // Prevent infinite loops for now
> >
> > Ok, this doesn't change. It's not big deal, you're missing just the
> > printout below
> >
> > void CameraManager::Private::pipelineFactoryMatch(const
> > PipelineHandlerFactoryBase *factory)
> > {
> > CameraManager *const o = LIBCAMERA_O_PTR();
> >
> > /* Provide as many matching pipelines as possible. */
> > while (1) {
> > std::shared_ptr<PipelineHandler> pipe = factory->create(o);
> > if (!pipe->match(enumerator_.get()))
> > break;
> >
> > LOG(Camera, Debug)
> > << "Pipeline handler \"" << factory->name()
> > << "\" matched";
> > }
> > }
> >
> > However it still feels like working around the intended design a bit.
> > I don't have much more to suggest if not keeping a static variable
> > around to store if the pipeline handler has been matches already or
> > not, and once it has been matched fail at the next call.
> >
> > Yeah, I did exactly that in the 3rd patch. Thanks for checking!
>
>
> > > }
> > > diff --git a/src/libcamera/pipeline/virtual/virtual.h
> > b/src/libcamera/pipeline/virtual/virtual.h
> > > index fecd9fa6f..c1ac4eb90 100644
> > > --- a/src/libcamera/pipeline/virtual/virtual.h
> > > +++ b/src/libcamera/pipeline/virtual/virtual.h
> > > @@ -22,7 +22,7 @@ class VirtualCameraData : public Camera::Private
> > > public:
> > > struct Resolution {
> > > Size size;
> > > - std::vector<int> frame_rates;
> > > + std::vector<int> frameRates;
> > > };
> > > VirtualCameraData(PipelineHandler *pipe)
> > > : Camera::Private(pipe)
> > > @@ -31,9 +31,9 @@ public:
> > >
> > > ~VirtualCameraData() = default;
> > >
> > > - TestPattern testPattern_;
> > > -
> > > + std::string id_;
> > > std::vector<Resolution> supportedResolutions_;
> > > + TestPattern testPattern_;
> > >
> > > Stream stream_;
> > >
> > > --
> > > 2.46.0.184.g6999bdac58-goog
> > >
> >
More information about the libcamera-devel
mailing list