[PATCH v9 5/8] libcamera: pipeline: Read config and register cameras based on the config
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Tue Aug 27 19:01:36 CEST 2024
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
>
> 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
> + model: "Virtual Video Device3"
> +"Virtual4":
Is this a valid entry ?
> 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
> + *
> + * 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
> + 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
Do you want to have the properties mandatory ? Make the default 0 and
reject it in this case.
> + 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 ?
> + 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
> +
> + 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
> +#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...
> + 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.
> }
> 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