[PATCH v9 5/8] libcamera: pipeline: Read config and register cameras based on the config
Cheng-Hao Yang
chenghaoyang at chromium.org
Thu Aug 29 21:58:01 CEST 2024
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?
> > + 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
>
> > + 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
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20240829/1baca016/attachment.htm>
More information about the libcamera-devel
mailing list