[{"id":31402,"web_url":"https://patchwork.libcamera.org/comment/31402/","msgid":"<2shqndkz74bxvnqv7qqfzi366ck2n3qwbmwzzftx2vxuetcrqw@snateic5g5p7>","date":"2024-09-26T13:15:10","subject":"Re: [PATCH v12 6/7] libcamera: virtual: Read config and register\n\tcameras based on the config","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Harvey\n\nOn Tue, Sep 10, 2024 at 04:40:19AM GMT, Harvey Yang wrote:\n> This patch introduces the configuration file for Virtual Pipeline\n> Handler. The config file is written in yaml, and the format is\n> documented in `README.md`.\n>\n> The config file will define the camera with IDs, supported formats and\n> image sources, etc. In the default config file, only Test Patterns are\n> used. Developers can use real images loading if desired.\n>\n> Signed-off-by: Konami Shu \n> Co-developed-by: Harvey Yang \n> Co-developed-by: Yunke Cao \n> Co-developed-by: Tomasz Figa \n> ---\n> src/libcamera/pipeline/virtual/README.md | 48 ++++\n> .../pipeline/virtual/data/virtual.yaml | 36 +++\n> src/libcamera/pipeline/virtual/meson.build | 1 +\n> src/libcamera/pipeline/virtual/parser.cpp | 241 ++++++++++++++++++\n> src/libcamera/pipeline/virtual/parser.h | 39 +++\n> src/libcamera/pipeline/virtual/virtual.cpp | 120 +++++----\n> src/libcamera/pipeline/virtual/virtual.h | 21 +-\n> 7 files changed, 456 insertions(+), 50 deletions(-)\n> create mode 100644 src/libcamera/pipeline/virtual/README.md\n> create mode 100644 src/libcamera/pipeline/virtual/data/virtual.yaml\n> create mode 100644 src/libcamera/pipeline/virtual/parser.cpp\n> create mode 100644 src/libcamera/pipeline/virtual/parser.h\n>\n> diff --git a/src/libcamera/pipeline/virtual/README.md b/src/libcamera/pipeline/virtual/README.md\n> new file mode 100644\n> index 00000000..84d7657d\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/virtual/README.md\n> @@ -0,0 +1,48 @@\n> +# Virtual Pipeline Handler\n> +\n> +Virtual pipeline handler emulates fake external camera(s) on ChromeOS for testing.\n> +\n> +## Parse config file and register cameras\n> +\n> +- The config file is located at `src/libcamera/pipeline/virtual/data/virtual.yaml`\n> +\n> +### Config File Format\n> +The config file contains the information about cameras' properties to register.\n> +The config file should be a yaml file with dictionary of the cameraIds\n> +associated with their properties as top level. The default value will be applied when any property is empty.\n> +\n> +Each camera block is a dictionary, containing the following keys:\n> +- `supported_formats` (list of `VirtualCameraData::Resolution`, optional) : List of supported resolution and frame rates of the emulated camera\n> + - `width` (`unsigned int`, default=1920): Width of the window resolution. This needs to be even.\n> + - `height` (`unsigned int`, default=1080): Height of the window resolution.\n> + - `frame_rates` (list of `int`, default=`[30,60]` ): Range of the frame rate (per second). If the list contains one value, it's the lower bound and the upper bound. If the list contains two values, the first is the lower bound and the second is the upper bound. No other number of values is allowed.\n> +- `test_pattern` (`string`): Which test pattern to use as frames. The options are \"bars\", \"lines\". Cannot be set with `frames`.\n> +- `frames` (dictionary):\n> + - `path` (`string`): Path to an image, or path to a directory of a series of images. Cannot be set with `test_pattern`.\n> + - The test patterns are \"bars\" which means color bars, and \"lines\" which means diagonal lines.\n> + - The path to an image has \".jpg\" extension.\n> + - The path to a directory ends with \"/\". The name of the images in the directory are \"{n}.jpg\" with {n} is the sequence of images starting with 0.\n> +- `location` (`string`, default=\"front\"): The location of the camera. Support \"front\" and \"back\". This is displayed in qcam camera selection window but this does not change the output.\n> +- `model` (`string`, default=\"Unknown\"): The model name of the camera. This is displayed in qcam camera selection window but this does not change the output.\n> +\n> +Check `data/virtual.yaml` as the sample config file.\n> +\n> +### Implementation\n> +\n> +`Parser` class provides methods to parse the config file to register cameras\n> +in Virtual Pipeline Handler. `parseConfigFile()` is exposed to use in\n> +Virtual Pipeline Handler.\n> +\n> +This is the procedure of the Parser class:\n> +1. `parseConfigFile()` parses the config file to `YamlObject` using `YamlParser::parse()`.\n> + - Parse the top level of config file which are the camera ids and look into each camera properties.\n> +2. For each camera, `parseCameraConfigData()` returns a camera with the configuration.\n> + - The methods in the next step fill the data with the pointer to the Camera object.\n> + - If the config file contains invalid configuration, this method returns nullptr. The camera will be skipped.\n> +3. Parse each property and register the data.\n> + - `parseSupportedFormats()`: Parses `supported_formats` in the config, which contains resolutions and frame rates.\n> + - `parseFrame()`: Parses `test_pattern` or `frames` in the config.\n> + - `parseLocation()`: Parses `location` in the config.\n> + - `parseModel()`: Parses `model` in the config.\n> +4. Back to `parseConfigFile()` and append the camera configuration.\n> +5. Returns a list of camera configurations.\n> diff --git a/src/libcamera/pipeline/virtual/data/virtual.yaml b/src/libcamera/pipeline/virtual/data/virtual.yaml\n> new file mode 100644\n> index 00000000..6b73ddf2\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/virtual/data/virtual.yaml\n> @@ -0,0 +1,36 @@\n> +# SPDX-License-Identifier: CC0-1.0\n> +%YAML 1.1\n> +---\n> +\"Virtual0\":\n> + supported_formats:\n> + - width: 1920\n> + height: 1080\n> + frame_rates:\n> + - 30\n> + - 60\n> + - width: 1680\n> + height: 1050\n> + frame_rates:\n> + - 70\n> + - 80\n> + test_pattern: \"lines\"\n> + location: \"front\"\n> + model: \"Virtual Video Device\"\n> +\"Virtual1\":\n> + supported_formats:\n> + - width: 800\n> + height: 600\n> + frame_rates:\n> + - 60\n> + test_pattern: \"bars\"\n> + location: \"back\"\n> + model: \"Virtual Video Device1\"\n> +\"Virtual2\":\n> + supported_formats:\n> + - width: 400\n> + height: 300\n> + test_pattern: \"lines\"\n> + location: \"front\"\n> + model: \"Virtual Video Device2\"\n> +\"Virtual3\":\n> + test_pattern: \"bars\"\n> diff --git a/src/libcamera/pipeline/virtual/meson.build b/src/libcamera/pipeline/virtual/meson.build\n> index 73d10cc3..395919b3 100644\n> --- a/src/libcamera/pipeline/virtual/meson.build\n> +++ b/src/libcamera/pipeline/virtual/meson.build\n> @@ -2,6 +2,7 @@\n>\n> libcamera_internal_sources += files([\n> 'image_frame_generator.cpp',\n> + 'parser.cpp',\n> 'test_pattern_generator.cpp',\n> 'virtual.cpp',\n> ])\n> diff --git a/src/libcamera/pipeline/virtual/parser.cpp b/src/libcamera/pipeline/virtual/parser.cpp\n> new file mode 100644\n> index 00000000..7e6c7fdf\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/virtual/parser.cpp\n> @@ -0,0 +1,241 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Google Inc.\n> + *\n> + * parser.cpp - Virtual cameras helper to parse config file\n> + */\n> +\n> +#include \"parser.h\"\n> +\n> +#include \n\nIncluded from the header file\n\n> +#include \n> +\n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \"libcamera/internal/pipeline_handler.h\"\n> +#include \"libcamera/internal/yaml_parser.h\"\n> +\n> +#include \"virtual.h\"\n> +\n> +namespace libcamera {\n> +\n> +LOG_DECLARE_CATEGORY(Virtual)\n> +\n> +std::vector>\n> +Parser::parseConfigFile(File &file, PipelineHandler *pipe)\n> +{\n> +\tstd::vector> configurations;\n> +\n> +\tstd::unique_ptr cameras = YamlParser::parse(file);\n> +\tif (!cameras) {\n> +\t\tLOG(Virtual, Error) << \"Failed to pass config file.\";\n> +\t\treturn configurations;\n> +\t}\n> +\n> +\tif (!cameras->isDictionary()) {\n> +\t\tLOG(Virtual, Error) << \"Config file is not a dictionary at the top level.\";\n> +\t\treturn configurations;\n> +\t}\n> +\n> +\t/* Look into the configuration of each camera */\n> +\tfor (const auto &[cameraId, cameraConfigData] : cameras->asDict()) {\n> +\t\tstd::unique_ptr data =\n> +\t\t\tparseCameraConfigData(cameraConfigData, pipe);\n> +\t\t/* Parse configData to data*/\n> +\t\tif (!data) {\n> +\t\t\t/* Skip the camera if it has invalid config */\n> +\t\t\tLOG(Virtual, Error) << \"Failed to parse config of the camera: \"\n> +\t\t\t\t\t << cameraId;\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> +\t\tdata->config_.id = cameraId;\n> +\t\tControlInfoMap::Map controls;\n> +\t\t/* todo: Check which resolution's frame rate to be reported */\n> +\t\tcontrols[&controls::FrameDurationLimits] =\n> +\t\t\tControlInfo(int64_t(1000000 / data->config_.resolutions[0].frameRates[1]),\n> +\t\t\t\t int64_t(1000000 / data->config_.resolutions[0].frameRates[0]));\n> +\t\tdata->controlInfo_ = ControlInfoMap(std::move(controls), controls::controls);\n> +\t\tconfigurations.push_back(std::move(data));\n> +\t}\n> +\n> +\treturn configurations;\n> +}\n> +\n> +std::unique_ptr\n> +Parser::parseCameraConfigData(const YamlObject &cameraConfigData,\n> +\t\t\t PipelineHandler *pipe)\n> +{\n> +\tstd::vector resolutions;\n> +\tif (parseSupportedFormats(cameraConfigData, &resolutions))\n> +\t\treturn nullptr;\n> +\n> +\tstd::unique_ptr data =\n> +\t\tstd::make_unique(pipe, resolutions);\n> +\n> +\tif (parseFrame(cameraConfigData, data.get()))\n> +\t\treturn nullptr;\n> +\n> +\tif (parseLocation(cameraConfigData, data.get()))\n> +\t\treturn nullptr;\n> +\n> +\tif (parseModel(cameraConfigData, data.get()))\n> +\t\treturn nullptr;\n> +\n> +\treturn data;\n> +}\n> +\n> +int Parser::parseSupportedFormats(const YamlObject &cameraConfigData,\n> +\t\t\t\t std::vector *resolutions)\n> +{\n> +\tif (cameraConfigData.contains(\"supported_formats\")) {\n> +\t\tconst YamlObject &supportedResolutions = cameraConfigData[\"supported_formats\"];\n> +\n> +\t\tfor (const YamlObject &supportedResolution : supportedResolutions.asList()) {\n> +\t\t\tunsigned int width = supportedResolution[\"width\"].get(1920);\n> +\t\t\tunsigned int height = supportedResolution[\"height\"].get(1080);\n\nCan this happen if you have 1920 and 1080 as defaults ?\n\n> +\t\t\tif (width == 0 || height == 0) {\n> +\t\t\t\tLOG(Virtual, Error) << \"Invalid width or/and height\";\n> +\t\t\t\treturn -EINVAL;\n> +\t\t\t}\n> +\t\t\tif (width % 2 != 0) {\n> +\t\t\t\tLOG(Virtual, Error) << \"Invalid width: width needs to be even\";\n> +\t\t\t\treturn -EINVAL;\n> +\t\t\t}\n> +\n> +\t\t\tstd::vector frameRates;\n> +\t\t\tif (supportedResolution.contains(\"frame_rates\")) {\n> +\t\t\t\tauto frameRatesList =\n> +\t\t\t\t\tsupportedResolution[\"frame_rates\"].getList();\n> +\t\t\t\tif (!frameRatesList || (frameRatesList->size() != 1 &&\n\nHaven't you aleady checked that \"frameRatesList\" is in\nsupportedResolutions ? (iow you could drop the !frameRatesList check\nimo)\n\n> +\t\t\t\t\t\t\tframeRatesList->size() != 2)) {\n> +\t\t\t\t\tLOG(Virtual, Error) << \"Invalid frame_rates: either one or two values\";\n> +\t\t\t\t\treturn -EINVAL;\n> +\t\t\t\t}\n> +\n> +\t\t\t\tif (frameRatesList->size() == 2 &&\n> +\t\t\t\t frameRatesList.value()[0] > frameRatesList.value()[1]) {\n> +\t\t\t\t\tLOG(Virtual, Error) << \"frame_rates's first value(lower bound)\"\n> +\t\t\t\t\t\t\t << \" is higher than the second value(upper bound)\";\n> +\t\t\t\t\treturn -EINVAL;\n> +\t\t\t\t}\n> +\t\t\t\tframeRates.push_back(frameRatesList.value()[0]);\n> +\t\t\t\tif (frameRatesList->size() == 2)\n> +\t\t\t\t\tframeRates.push_back(frameRatesList.value()[1]);\n> +\t\t\t\telse\n> +\t\t\t\t\tframeRates.push_back(frameRatesList.value()[0]);\n\nYou can skip the else branch if I got this right\n\n> +\t\t\t} else {\n> +\t\t\t\tframeRates.push_back(30);\n> +\t\t\t\tframeRates.push_back(60);\n> +\t\t\t}\n> +\n> +\t\t\tresolutions->emplace_back(\n> +\t\t\t\tVirtualCameraData::Resolution{ Size{ width, height },\n> +\t\t\t\t\t\t\t frameRates });\n> +\t\t}\n> +\t} else {\n> +\t\tresolutions->emplace_back(\n> +\t\t\tVirtualCameraData::Resolution{ Size{ 1920, 1080 },\n> +\t\t\t\t\t\t { 30, 60 } });\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +int Parser::parseFrame(const YamlObject &cameraConfigData, VirtualCameraData *data)\n\nparseFrame is not a great name, maybe just parseFrameGenerator would\nbe better\n\n> +{\n> +\tconst std::string testPatternKey = \"test_pattern\";\n> +\tconst std::string framesKey = \"frames\";\n> +\tif (cameraConfigData.contains(testPatternKey)) {\n> +\t\tif (cameraConfigData.contains(framesKey)) {\n> +\t\t\tLOG(Virtual, Error) << \"A camera should use either \"\n> +\t\t\t\t\t << testPatternKey << \" or \" << framesKey;\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\n> +\t\tauto testPattern = cameraConfigData[testPatternKey].get(\"\");\n> +\n> +\t\tif (testPattern == \"bars\") {\n> +\t\t\tdata->config_.frame = TestPattern::ColorBars;\n> +\t\t} else if (testPattern == \"lines\") {\n> +\t\t\tdata->config_.frame = TestPattern::DiagonalLines;\n> +\t\t} else {\n> +\t\t\tLOG(Virtual, Debug) << \"Test pattern: \" << testPattern\n> +\t\t\t\t\t << \"is not supported\";\n\n\" is not supported\"\n\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tconst YamlObject &frames = cameraConfigData[framesKey];\n> +\n> +\t/* When there is no frames provided in the config file, use color bar test pattern */\n> +\tif (frames.size() == 0) {\n\nAccording to https://patchwork.libcamera.org/patch/21294/ you should\nprobably use if frames.isEmpty() or simply with if (!frames)\n\n> +\t\tdata->config_.frame = TestPattern::ColorBars;\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tif (!frames.isDictionary()) {\n> +\t\tLOG(Virtual, Error) << \"'frames' is not a dictionary.\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tstd::string path = frames[\"path\"].get(\"\");\n\nI would be stricter, drop the default \"\" and check for\n if (!path)\n\nand fail in this case\n\n> +\n> +\tif (auto ext = std::filesystem::path(path).extension();\n> +\t ext == \".jpg\" || ext == \".jpeg\") {\n> +\t\tdata->config_.frame = ImageFrames{ path, std::nullopt };\n> +\t} else if (std::filesystem::is_directory(std::filesystem::symlink_status(path))) {\n> +\t\tusing std::filesystem::directory_iterator;\n> +\t\tunsigned int numOfFiles = std::distance(directory_iterator(path), directory_iterator{});\n> +\t\tif (numOfFiles == 0) {\n> +\t\t\tLOG(Virtual, Error) << \"Empty directory\";\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\t\tdata->config_.frame = ImageFrames{ path, numOfFiles };\n> +\t} else {\n> +\t\tLOG(Virtual, Error) << \"Frame: \" << path << \" is not supported\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +int Parser::parseLocation(const YamlObject &cameraConfigData, VirtualCameraData *data)\n> +{\n> +\tstd::string location = cameraConfigData[\"location\"].get(\"\");\n\ncheck for\n if (!location) and exit early\n\nWith this minors fixed I'll ack the next version\n\n> +\n> +\t/* Default value is properties::CameraLocationFront */\n> +\tif (location == \"front\" || location == \"\") {\n> +\t\tdata->properties_.set(properties::Location,\n> +\t\t\t\t properties::CameraLocationFront);\n> +\t} else if (location == \"back\") {\n> +\t\tdata->properties_.set(properties::Location,\n> +\t\t\t\t properties::CameraLocationBack);\n> +\t} else {\n> +\t\tLOG(Virtual, Error) << \"location: \" << location\n> +\t\t\t\t << \" is not supported\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +int Parser::parseModel(const YamlObject &cameraConfigData, VirtualCameraData *data)\n> +{\n> +\tstd::string model = cameraConfigData[\"model\"].get(\"\");\n> +\n> +\t/* Default value is \"Unknown\" */\n> +\tif (model == \"\")\n> +\t\tdata->properties_.set(properties::Model, \"Unknown\");\n> +\telse\n> +\t\tdata->properties_.set(properties::Model, model);\n> +\n> +\treturn 0;\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/virtual/parser.h b/src/libcamera/pipeline/virtual/parser.h\n> new file mode 100644\n> index 00000000..9058f3c7\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/virtual/parser.h\n> @@ -0,0 +1,39 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Google Inc.\n> + *\n> + * parser.h - Virtual cameras helper to parse config file\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \"libcamera/internal/pipeline_handler.h\"\n> +#include \"libcamera/internal/yaml_parser.h\"\n> +\n> +#include \"virtual.h\"\n> +\n> +namespace libcamera {\n> +\n> +class Parser\n> +{\n> +public:\n> +\tstd::vector>\n> +\tparseConfigFile(File &file, PipelineHandler *pipe);\n> +\n> +private:\n> +\tstd::unique_ptr\n> +\tparseCameraConfigData(const YamlObject &cameraConfigData, PipelineHandler *pipe);\n> +\n> +\tint parseSupportedFormats(const YamlObject &cameraConfigData,\n> +\t\t\t\t std::vector *resolutions);\n> +\tint parseFrame(const YamlObject &cameraConfigData, VirtualCameraData *data);\n> +\tint parseLocation(const YamlObject &cameraConfigData, VirtualCameraData *data);\n> +\tint parseModel(const YamlObject &cameraConfigData, VirtualCameraData *data);\n> +};\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp\n> index 4e9a6973..b4744ca2 100644\n> --- a/src/libcamera/pipeline/virtual/virtual.cpp\n> +++ b/src/libcamera/pipeline/virtual/virtual.cpp\n> @@ -26,6 +26,9 @@\n> #include \"libcamera/internal/dma_buf_allocator.h\"\n> #include \"libcamera/internal/formats.h\"\n> #include \"libcamera/internal/pipeline_handler.h\"\n> +#include \"libcamera/internal/yaml_parser.h\"\n> +\n> +#include \"parser.h\"\n>\n> namespace libcamera {\n>\n> @@ -44,6 +47,13 @@ uint64_t currentTimestamp()\n>\n> } /* namespace */\n>\n> +template\n> +struct overloaded : Ts... {\n> +\tusing Ts::operator()...;\n> +};\n> +template\n> +overloaded(Ts...) -> overloaded;\n> +\n> class VirtualCameraConfiguration : public CameraConfiguration\n> {\n> public:\n> @@ -84,7 +94,7 @@ private:\n> \t\treturn static_cast(camera->_d());\n> \t}\n>\n> -\tvoid initFrameGenerator(Camera *camera);\n> +\tbool initFrameGenerator(Camera *camera);\n>\n> \tDmaBufAllocator dmaBufAllocator_;\n> };\n> @@ -94,15 +104,19 @@ bool PipelineHandlerVirtual::created_ = false;\n>\n> VirtualCameraData::VirtualCameraData(PipelineHandler *pipe,\n> \t\t\t\t std::vector supportedResolutions)\n> -\t: Camera::Private(pipe), supportedResolutions_(std::move(supportedResolutions))\n> +\t: Camera::Private(pipe)\n> {\n> -\tfor (const auto &resolution : supportedResolutions_) {\n> -\t\tif (minResolutionSize_.isNull() || minResolutionSize_ > resolution.size)\n> -\t\t\tminResolutionSize_ = resolution.size;\n> +\tconfig_.resolutions = std::move(supportedResolutions);\n> +\tfor (const auto &resolution : config_.resolutions) {\n> +\t\tif (config_.minResolutionSize.isNull() || config_.minResolutionSize > resolution.size)\n> +\t\t\tconfig_.minResolutionSize = resolution.size;\n>\n> -\t\tmaxResolutionSize_ = std::max(maxResolutionSize_, resolution.size);\n> +\t\tconfig_.maxResolutionSize = std::max(config_.maxResolutionSize, resolution.size);\n> \t}\n>\n> +\tproperties_.set(properties::PixelArrayActiveAreas,\n> +\t\t\t{ Rectangle(config_.maxResolutionSize) });\n> +\n> \t/* \\todo Support multiple streams and pass multi_stream_test */\n> \tstreamConfigs_.resize(kMaxStream);\n> }\n> @@ -129,7 +143,7 @@ CameraConfiguration::Status VirtualCameraConfiguration::validate()\n>\n> \tfor (StreamConfiguration &cfg : config_) {\n> \t\tbool found = false;\n> -\t\tfor (const auto &resolution : data_->supportedResolutions_) {\n> +\t\tfor (const auto &resolution : data_->config_.resolutions) {\n> \t\t\tif (resolution.size.width == cfg.size.width &&\n> \t\t\t resolution.size.height == cfg.size.height) {\n> \t\t\t\tfound = true;\n> @@ -144,7 +158,7 @@ CameraConfiguration::Status VirtualCameraConfiguration::validate()\n> \t\t\t * Defining the default logic in PipelineHandler to\n> \t\t\t * find the closest resolution would be nice.\n> \t\t\t */\n> -\t\t\tcfg.size = data_->maxResolutionSize_;\n> +\t\t\tcfg.size = data_->config_.maxResolutionSize;\n> \t\t\tstatus = Adjusted;\n> \t\t}\n>\n> @@ -201,11 +215,11 @@ PipelineHandlerVirtual::generateConfiguration(Camera *camera,\n>\n> \t\tstd::map> streamFormats;\n> \t\tPixelFormat pixelFormat = formats::NV12;\n> -\t\tstreamFormats[pixelFormat] = { { data->minResolutionSize_, data->maxResolutionSize_ } };\n> +\t\tstreamFormats[pixelFormat] = { { data->config_.minResolutionSize, data->config_.maxResolutionSize } };\n> \t\tStreamFormats formats(streamFormats);\n> \t\tStreamConfiguration cfg(formats);\n> \t\tcfg.pixelFormat = pixelFormat;\n> -\t\tcfg.size = data->maxResolutionSize_;\n> +\t\tcfg.size = data->config_.maxResolutionSize;\n> \t\tcfg.bufferCount = VirtualCameraConfiguration::kBufferCount;\n>\n> \t\tconfig->addConfiguration(cfg);\n> @@ -222,6 +236,7 @@ int PipelineHandlerVirtual::configure(Camera *camera,\n> \tVirtualCameraData *data = cameraData(camera);\n> \tfor (auto [i, c] : utils::enumerate(*config)) {\n> \t\tc.setStream(&data->streamConfigs_[i].stream);\n> +\t\t/* Start reading the images/generating test patterns */\n> \t\tdata->streamConfigs_[i].frameGenerator->configure(c.size);\n> \t}\n>\n> @@ -290,50 +305,65 @@ bool PipelineHandlerVirtual::match([[maybe_unused]] DeviceEnumerator *enumerator\n>\n> \tcreated_ = true;\n>\n> -\t/* \\todo Add virtual cameras according to a config file. */\n> -\n> -\tstd::vector supportedResolutions;\n> -\tsupportedResolutions.resize(2);\n> -\tsupportedResolutions[0] = { .size = Size(1920, 1080), .frameRates = { 30 } };\n> -\tsupportedResolutions[1] = { .size = Size(1280, 720), .frameRates = { 30 } };\n> -\n> -\tstd::unique_ptr data =\n> -\t\tstd::make_unique(this, supportedResolutions);\n> -\n> -\tdata->properties_.set(properties::Location, properties::CameraLocationFront);\n> -\tdata->properties_.set(properties::Model, \"Virtual Video Device\");\n> -\tdata->properties_.set(properties::PixelArrayActiveAreas, { Rectangle(Size(1920, 1080)) });\n> -\n> -\t/* \\todo Set FrameDurationLimits based on config. */\n> -\tControlInfoMap::Map controls;\n> -\tint64_t min_frame_duration = 33333, max_frame_duration = 33333;\n> -\tcontrols[&controls::FrameDurationLimits] = ControlInfo(min_frame_duration, max_frame_duration);\n> -\tdata->controlInfo_ = ControlInfoMap(std::move(controls), controls::controls);\n> -\n> -\t/* Create and register the camera. */\n> -\tstd::set streams;\n> -\tfor (auto &streamConfig : data->streamConfigs_)\n> -\t\tstreams.insert(&streamConfig.stream);\n> +\tFile file(configurationFile(\"virtual\", \"virtual.yaml\"));\n> +\tbool isOpen = file.open(File::OpenModeFlag::ReadOnly);\n> +\tif (!isOpen) {\n> +\t\tLOG(Virtual, Error) << \"Failed to open config file: \" << file.fileName();\n> +\t\treturn false;\n> +\t}\n>\n> -\tconst std::string id = \"Virtual0\";\n> -\tstd::shared_ptr camera = Camera::create(std::move(data), id, streams);\n> +\tParser parser;\n> +\tauto configData = parser.parseConfigFile(file, this);\n> +\tif (configData.size() == 0) {\n> +\t\tLOG(Virtual, Error) << \"Failed to parse any cameras from the config file: \"\n> +\t\t\t\t << file.fileName();\n> +\t\treturn false;\n> +\t}\n>\n> -\tinitFrameGenerator(camera.get());\n> +\t/* Configure and register cameras with configData */\n> +\tfor (auto &data : configData) {\n> +\t\tstd::set streams;\n> +\t\tfor (auto &streamConfig : data->streamConfigs_)\n> +\t\t\tstreams.insert(&streamConfig.stream);\n> +\t\tstd::string id = data->config_.id;\n> +\t\tstd::shared_ptr camera = Camera::create(std::move(data), id, streams);\n> +\n> +\t\tif (!initFrameGenerator(camera.get())) {\n> +\t\t\tLOG(Virtual, Error) << \"Failed to initialize frame \"\n> +\t\t\t\t\t << \"generator for camera: \" << id;\n> +\t\t\tcontinue;\n> +\t\t}\n>\n> -\tregisterCamera(std::move(camera));\n> +\t\tregisterCamera(std::move(camera));\n> +\t}\n>\n> \treturn true;\n> }\n>\n> -void PipelineHandlerVirtual::initFrameGenerator(Camera *camera)\n> +bool PipelineHandlerVirtual::initFrameGenerator(Camera *camera)\n> {\n> \tauto data = cameraData(camera);\n> -\tfor (auto &streamConfig : data->streamConfigs_) {\n> -\t\tif (data->testPattern_ == TestPattern::DiagonalLines)\n> -\t\t\tstreamConfig.frameGenerator = std::make_unique();\n> -\t\telse\n> -\t\t\tstreamConfig.frameGenerator = std::make_unique();\n> -\t}\n> +\tauto &frame = data->config_.frame;\n> +\tstd::visit(overloaded{\n> +\t\t\t [&](TestPattern &testPattern) {\n> +\t\t\t\t for (auto &streamConfig : data->streamConfigs_) {\n> +\t\t\t\t\t if (testPattern == TestPattern::DiagonalLines)\n> +\t\t\t\t\t\t streamConfig.frameGenerator = std::make_unique();\n> +\t\t\t\t\t else\n> +\t\t\t\t\t\t streamConfig.frameGenerator = std::make_unique();\n> +\t\t\t\t }\n> +\t\t\t },\n> +\t\t\t [&](ImageFrames &imageFrames) {\n> +\t\t\t\t for (auto &streamConfig : data->streamConfigs_)\n> +\t\t\t\t\t streamConfig.frameGenerator = ImageFrameGenerator::create(imageFrames);\n> +\t\t\t } },\n> +\t\t frame);\n> +\n> +\tfor (auto &streamConfig : data->streamConfigs_)\n> +\t\tif (!streamConfig.frameGenerator)\n> +\t\t\treturn false;\n> +\n> +\treturn true;\n> }\n>\n> REGISTER_PIPELINE_HANDLER(PipelineHandlerVirtual, \"virtual\")\n> diff --git a/src/libcamera/pipeline/virtual/virtual.h b/src/libcamera/pipeline/virtual/virtual.h\n> index acdd437e..c99a0996 100644\n> --- a/src/libcamera/pipeline/virtual/virtual.h\n> +++ b/src/libcamera/pipeline/virtual/virtual.h\n> @@ -7,6 +7,8 @@\n>\n> #pragma once\n>\n> +#include \n> +#include \n> #include \n>\n> #include \n> @@ -14,10 +16,14 @@\n> #include \"libcamera/internal/camera.h\"\n> #include \"libcamera/internal/pipeline_handler.h\"\n>\n> +#include \"frame_generator.h\"\n> +#include \"image_frame_generator.h\"\n> #include \"test_pattern_generator.h\"\n>\n> namespace libcamera {\n>\n> +using VirtualFrame = std::variant;\n> +\n> class VirtualCameraData : public Camera::Private\n> {\n> public:\n> @@ -31,17 +37,22 @@ public:\n> \t\tStream stream;\n> \t\tstd::unique_ptr frameGenerator;\n> \t};\n> +\t/* The config file is parsed to the Configuration struct */\n> +\tstruct Configuration {\n> +\t\tstd::string id;\n> +\t\tstd::vector resolutions;\n> +\t\tVirtualFrame frame;\n> +\n> +\t\tSize maxResolutionSize;\n> +\t\tSize minResolutionSize;\n> +\t};\n>\n> \tVirtualCameraData(PipelineHandler *pipe,\n> \t\t\t std::vector supportedResolutions);\n>\n> \t~VirtualCameraData() = default;\n>\n> -\tTestPattern testPattern_ = TestPattern::ColorBars;\n> -\n> -\tconst std::vector supportedResolutions_;\n> -\tSize maxResolutionSize_;\n> -\tSize minResolutionSize_;\n> +\tConfiguration config_;\n>\n> \tstd::vector streamConfigs_;\n> };\n> --\n> 2.46.0.598.g6f2099f65c-goog\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id A2611C3257\n\tfor ;\n\tThu, 26 Sep 2024 13:15:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B29B36350F;\n\tThu, 26 Sep 2024 15:15:15 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 02DF0634F9\n\tfor ;\n\tThu, 26 Sep 2024 15:15:13 +0200 (CEST)","from ideasonboard.com (mob-5-90-51-229.net.vodafone.it\n\t[5.90.51.229])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7A0A7169;\n\tThu, 26 Sep 2024 15:13:45 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"dDbGEVOs\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1727356425;\n\tbh=3NXHEkoCZ0mSdC4KpgYWLQXueTKABupUyMXgaX5zpfM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=dDbGEVOsBEnpqjEOdXtqBv2O4jd9qC54jQLeuo2YGjqPRatC+Br5Uj4JkKJcPG21M\n\to4SSuUwqqOTxGQviN9Dx1GGHLML4rccAmS+qT/JMifXIEsmm8D4IpGYurwRtD1JjHU\n\t7IwYoPR1SSGCcvzwNxSNf5dImsqVXR0eRlxio0Jc=","Date":"Thu, 26 Sep 2024 15:15:10 +0200","From":"Jacopo Mondi ","To":"Harvey Yang ","Cc":"libcamera-devel@lists.libcamera.org, \n\tHarvey Yang , Konami Shu ,\n\tYunke Cao , Tomasz Figa ","Subject":"Re: [PATCH v12 6/7] libcamera: virtual: Read config and register\n\tcameras based on the config","Message-ID":"<2shqndkz74bxvnqv7qqfzi366ck2n3qwbmwzzftx2vxuetcrqw@snateic5g5p7>","References":"<20240910044834.2477701-1-chenghaoyang@google.com>\n\t<20240910044834.2477701-7-chenghaoyang@google.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240910044834.2477701-7-chenghaoyang@google.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31417,"web_url":"https://patchwork.libcamera.org/comment/31417/","msgid":"","date":"2024-09-26T16:32:31","subject":"Re: [PATCH v12 6/7] libcamera: virtual: Read config and register\n\tcameras based on the config","submitter":{"id":148,"url":"https://patchwork.libcamera.org/api/people/148/","name":"Cheng-Hao Yang","email":"chenghaoyang@google.com"},"content":"Hi Jacopo,\n\nOn Thu, Sep 26, 2024 at 9:15 PM Jacopo Mondi\n wrote:\n>\n> Hi Harvey\n>\n> On Tue, Sep 10, 2024 at 04:40:19AM GMT, Harvey Yang wrote:\n> > This patch introduces the configuration file for Virtual Pipeline\n> > Handler. The config file is written in yaml, and the format is\n> > documented in `README.md`.\n> >\n> > The config file will define the camera with IDs, supported formats and\n> > image sources, etc. In the default config file, only Test Patterns are\n> > used. Developers can use real images loading if desired.\n> >\n> > Signed-off-by: Konami Shu \n> > Co-developed-by: Harvey Yang \n> > Co-developed-by: Yunke Cao \n> > Co-developed-by: Tomasz Figa \n> > ---\n> > src/libcamera/pipeline/virtual/README.md | 48 ++++\n> > .../pipeline/virtual/data/virtual.yaml | 36 +++\n> > src/libcamera/pipeline/virtual/meson.build | 1 +\n> > src/libcamera/pipeline/virtual/parser.cpp | 241 ++++++++++++++++++\n> > src/libcamera/pipeline/virtual/parser.h | 39 +++\n> > src/libcamera/pipeline/virtual/virtual.cpp | 120 +++++----\n> > src/libcamera/pipeline/virtual/virtual.h | 21 +-\n> > 7 files changed, 456 insertions(+), 50 deletions(-)\n> > create mode 100644 src/libcamera/pipeline/virtual/README.md\n> > create mode 100644 src/libcamera/pipeline/virtual/data/virtual.yaml\n> > create mode 100644 src/libcamera/pipeline/virtual/parser.cpp\n> > create mode 100644 src/libcamera/pipeline/virtual/parser.h\n> >\n> > diff --git a/src/libcamera/pipeline/virtual/README.md b/src/libcamera/pipeline/virtual/README.md\n> > new file mode 100644\n> > index 00000000..84d7657d\n> > --- /dev/null\n> > +++ b/src/libcamera/pipeline/virtual/README.md\n> > @@ -0,0 +1,48 @@\n> > +# Virtual Pipeline Handler\n> > +\n> > +Virtual pipeline handler emulates fake external camera(s) on ChromeOS for testing.\n> > +\n> > +## Parse config file and register cameras\n> > +\n> > +- The config file is located at `src/libcamera/pipeline/virtual/data/virtual.yaml`\n> > +\n> > +### Config File Format\n> > +The config file contains the information about cameras' properties to register.\n> > +The config file should be a yaml file with dictionary of the cameraIds\n> > +associated with their properties as top level. The default value will be applied when any property is empty.\n> > +\n> > +Each camera block is a dictionary, containing the following keys:\n> > +- `supported_formats` (list of `VirtualCameraData::Resolution`, optional) : List of supported resolution and frame rates of the emulated camera\n> > + - `width` (`unsigned int`, default=1920): Width of the window resolution. This needs to be even.\n> > + - `height` (`unsigned int`, default=1080): Height of the window resolution.\n> > + - `frame_rates` (list of `int`, default=`[30,60]` ): Range of the frame rate (per second). If the list contains one value, it's the lower bound and the upper bound. If the list contains two values, the first is the lower bound and the second is the upper bound. No other number of values is allowed.\n> > +- `test_pattern` (`string`): Which test pattern to use as frames. The options are \"bars\", \"lines\". Cannot be set with `frames`.\n> > +- `frames` (dictionary):\n> > + - `path` (`string`): Path to an image, or path to a directory of a series of images. Cannot be set with `test_pattern`.\n> > + - The test patterns are \"bars\" which means color bars, and \"lines\" which means diagonal lines.\n> > + - The path to an image has \".jpg\" extension.\n> > + - The path to a directory ends with \"/\". The name of the images in the directory are \"{n}.jpg\" with {n} is the sequence of images starting with 0.\n> > +- `location` (`string`, default=\"front\"): The location of the camera. Support \"front\" and \"back\". This is displayed in qcam camera selection window but this does not change the output.\n> > +- `model` (`string`, default=\"Unknown\"): The model name of the camera. This is displayed in qcam camera selection window but this does not change the output.\n> > +\n> > +Check `data/virtual.yaml` as the sample config file.\n> > +\n> > +### Implementation\n> > +\n> > +`Parser` class provides methods to parse the config file to register cameras\n> > +in Virtual Pipeline Handler. `parseConfigFile()` is exposed to use in\n> > +Virtual Pipeline Handler.\n> > +\n> > +This is the procedure of the Parser class:\n> > +1. `parseConfigFile()` parses the config file to `YamlObject` using `YamlParser::parse()`.\n> > + - Parse the top level of config file which are the camera ids and look into each camera properties.\n> > +2. For each camera, `parseCameraConfigData()` returns a camera with the configuration.\n> > + - The methods in the next step fill the data with the pointer to the Camera object.\n> > + - If the config file contains invalid configuration, this method returns nullptr. The camera will be skipped.\n> > +3. Parse each property and register the data.\n> > + - `parseSupportedFormats()`: Parses `supported_formats` in the config, which contains resolutions and frame rates.\n> > + - `parseFrame()`: Parses `test_pattern` or `frames` in the config.\n> > + - `parseLocation()`: Parses `location` in the config.\n> > + - `parseModel()`: Parses `model` in the config.\n> > +4. Back to `parseConfigFile()` and append the camera configuration.\n> > +5. Returns a list of camera configurations.\n> > diff --git a/src/libcamera/pipeline/virtual/data/virtual.yaml b/src/libcamera/pipeline/virtual/data/virtual.yaml\n> > new file mode 100644\n> > index 00000000..6b73ddf2\n> > --- /dev/null\n> > +++ b/src/libcamera/pipeline/virtual/data/virtual.yaml\n> > @@ -0,0 +1,36 @@\n> > +# SPDX-License-Identifier: CC0-1.0\n> > +%YAML 1.1\n> > +---\n> > +\"Virtual0\":\n> > + supported_formats:\n> > + - width: 1920\n> > + height: 1080\n> > + frame_rates:\n> > + - 30\n> > + - 60\n> > + - width: 1680\n> > + height: 1050\n> > + frame_rates:\n> > + - 70\n> > + - 80\n> > + test_pattern: \"lines\"\n> > + location: \"front\"\n> > + model: \"Virtual Video Device\"\n> > +\"Virtual1\":\n> > + supported_formats:\n> > + - width: 800\n> > + height: 600\n> > + frame_rates:\n> > + - 60\n> > + test_pattern: \"bars\"\n> > + location: \"back\"\n> > + model: \"Virtual Video Device1\"\n> > +\"Virtual2\":\n> > + supported_formats:\n> > + - width: 400\n> > + height: 300\n> > + test_pattern: \"lines\"\n> > + location: \"front\"\n> > + model: \"Virtual Video Device2\"\n> > +\"Virtual3\":\n> > + test_pattern: \"bars\"\n> > diff --git a/src/libcamera/pipeline/virtual/meson.build b/src/libcamera/pipeline/virtual/meson.build\n> > index 73d10cc3..395919b3 100644\n> > --- a/src/libcamera/pipeline/virtual/meson.build\n> > +++ b/src/libcamera/pipeline/virtual/meson.build\n> > @@ -2,6 +2,7 @@\n> >\n> > libcamera_internal_sources += files([\n> > 'image_frame_generator.cpp',\n> > + 'parser.cpp',\n> > 'test_pattern_generator.cpp',\n> > 'virtual.cpp',\n> > ])\n> > diff --git a/src/libcamera/pipeline/virtual/parser.cpp b/src/libcamera/pipeline/virtual/parser.cpp\n> > new file mode 100644\n> > index 00000000..7e6c7fdf\n> > --- /dev/null\n> > +++ b/src/libcamera/pipeline/virtual/parser.cpp\n> > @@ -0,0 +1,241 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2024, Google Inc.\n> > + *\n> > + * parser.cpp - Virtual cameras helper to parse config file\n> > + */\n> > +\n> > +#include \"parser.h\"\n> > +\n> > +#include \n>\n> Included from the header file\n\nRemoved.\n\n\n>\n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \"libcamera/internal/pipeline_handler.h\"\n> > +#include \"libcamera/internal/yaml_parser.h\"\n> > +\n> > +#include \"virtual.h\"\n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DECLARE_CATEGORY(Virtual)\n> > +\n> > +std::vector>\n> > +Parser::parseConfigFile(File &file, PipelineHandler *pipe)\n> > +{\n> > + std::vector> configurations;\n> > +\n> > + std::unique_ptr cameras = YamlParser::parse(file);\n> > + if (!cameras) {\n> > + LOG(Virtual, Error) << \"Failed to pass config file.\";\n> > + return configurations;\n> > + }\n> > +\n> > + if (!cameras->isDictionary()) {\n> > + LOG(Virtual, Error) << \"Config file is not a dictionary at the top level.\";\n> > + return configurations;\n> > + }\n> > +\n> > + /* Look into the configuration of each camera */\n> > + for (const auto &[cameraId, cameraConfigData] : cameras->asDict()) {\n> > + std::unique_ptr data =\n> > + parseCameraConfigData(cameraConfigData, pipe);\n> > + /* Parse configData to data*/\n> > + if (!data) {\n> > + /* Skip the camera if it has invalid config */\n> > + LOG(Virtual, Error) << \"Failed to parse config of the camera: \"\n> > + << cameraId;\n> > + continue;\n> > + }\n> > +\n> > + data->config_.id = cameraId;\n> > + ControlInfoMap::Map controls;\n> > + /* todo: Check which resolution's frame rate to be reported */\n> > + controls[&controls::FrameDurationLimits] =\n> > + ControlInfo(int64_t(1000000 / data->config_.resolutions[0].frameRates[1]),\n> > + int64_t(1000000 / data->config_.resolutions[0].frameRates[0]));\n> > + data->controlInfo_ = ControlInfoMap(std::move(controls), controls::controls);\n> > + configurations.push_back(std::move(data));\n> > + }\n> > +\n> > + return configurations;\n> > +}\n> > +\n> > +std::unique_ptr\n> > +Parser::parseCameraConfigData(const YamlObject &cameraConfigData,\n> > + PipelineHandler *pipe)\n> > +{\n> > + std::vector resolutions;\n> > + if (parseSupportedFormats(cameraConfigData, &resolutions))\n> > + return nullptr;\n> > +\n> > + std::unique_ptr data =\n> > + std::make_unique(pipe, resolutions);\n> > +\n> > + if (parseFrame(cameraConfigData, data.get()))\n> > + return nullptr;\n> > +\n> > + if (parseLocation(cameraConfigData, data.get()))\n> > + return nullptr;\n> > +\n> > + if (parseModel(cameraConfigData, data.get()))\n> > + return nullptr;\n> > +\n> > + return data;\n> > +}\n> > +\n> > +int Parser::parseSupportedFormats(const YamlObject &cameraConfigData,\n> > + std::vector *resolutions)\n> > +{\n> > + if (cameraConfigData.contains(\"supported_formats\")) {\n> > + const YamlObject &supportedResolutions = cameraConfigData[\"supported_formats\"];\n> > +\n> > + for (const YamlObject &supportedResolution : supportedResolutions.asList()) {\n> > + unsigned int width = supportedResolution[\"width\"].get(1920);\n> > + unsigned int height = supportedResolution[\"height\"].get(1080);\n>\n> Can this happen if you have 1920 and 1080 as defaults ?\n\nI assume you mean the `if` clause below.\nYes, if the config file specifies them to 0.\n\n>\n> > + if (width == 0 || height == 0) {\n> > + LOG(Virtual, Error) << \"Invalid width or/and height\";\n> > + return -EINVAL;\n> > + }\n> > + if (width % 2 != 0) {\n> > + LOG(Virtual, Error) << \"Invalid width: width needs to be even\";\n> > + return -EINVAL;\n> > + }\n> > +\n> > + std::vector frameRates;\n> > + if (supportedResolution.contains(\"frame_rates\")) {\n> > + auto frameRatesList =\n> > + supportedResolution[\"frame_rates\"].getList();\n> > + if (!frameRatesList || (frameRatesList->size() != 1 &&\n>\n> Haven't you aleady checked that \"frameRatesList\" is in\n> supportedResolutions ? (iow you could drop the !frameRatesList check\n> imo)\n\nChecking `frameRatesList` is in supportedResolutions doesn't mean\nit's a list. Config file may still set it to be other types that fails to call\ngetList here.\n\n>\n> > + frameRatesList->size() != 2)) {\n> > + LOG(Virtual, Error) << \"Invalid frame_rates: either one or two values\";\n> > + return -EINVAL;\n> > + }\n> > +\n> > + if (frameRatesList->size() == 2 &&\n> > + frameRatesList.value()[0] > frameRatesList.value()[1]) {\n> > + LOG(Virtual, Error) << \"frame_rates's first value(lower bound)\"\n> > + << \" is higher than the second value(upper bound)\";\n> > + return -EINVAL;\n> > + }\n> > + frameRates.push_back(frameRatesList.value()[0]);\n> > + if (frameRatesList->size() == 2)\n> > + frameRates.push_back(frameRatesList.value()[1]);\n> > + else\n> > + frameRates.push_back(frameRatesList.value()[0]);\n>\n> You can skip the else branch if I got this right\n\nDo you mean we should allow having only one value in\n`frameRatesList`? I prefer not. It means the users of\nthe list need to take care of this special case.\n\n\n>\n> > + } else {\n> > + frameRates.push_back(30);\n> > + frameRates.push_back(60);\n> > + }\n> > +\n> > + resolutions->emplace_back(\n> > + VirtualCameraData::Resolution{ Size{ width, height },\n> > + frameRates });\n> > + }\n> > + } else {\n> > + resolutions->emplace_back(\n> > + VirtualCameraData::Resolution{ Size{ 1920, 1080 },\n> > + { 30, 60 } });\n> > + }\n> > +\n> > + return 0;\n> > +}\n> > +\n> > +int Parser::parseFrame(const YamlObject &cameraConfigData, VirtualCameraData *data)\n>\n> parseFrame is not a great name, maybe just parseFrameGenerator would\n> be better\n\nDone\n\n>\n> > +{\n> > + const std::string testPatternKey = \"test_pattern\";\n> > + const std::string framesKey = \"frames\";\n> > + if (cameraConfigData.contains(testPatternKey)) {\n> > + if (cameraConfigData.contains(framesKey)) {\n> > + LOG(Virtual, Error) << \"A camera should use either \"\n> > + << testPatternKey << \" or \" << framesKey;\n> > + return -EINVAL;\n> > + }\n> > +\n> > + auto testPattern = cameraConfigData[testPatternKey].get(\"\");\n> > +\n> > + if (testPattern == \"bars\") {\n> > + data->config_.frame = TestPattern::ColorBars;\n> > + } else if (testPattern == \"lines\") {\n> > + data->config_.frame = TestPattern::DiagonalLines;\n> > + } else {\n> > + LOG(Virtual, Debug) << \"Test pattern: \" << testPattern\n> > + << \"is not supported\";\n>\n> \" is not supported\"\n\nDone\n\n>\n> > + return -EINVAL;\n> > + }\n> > +\n> > + return 0;\n> > + }\n> > +\n> > + const YamlObject &frames = cameraConfigData[framesKey];\n> > +\n> > + /* When there is no frames provided in the config file, use color bar test pattern */\n> > + if (frames.size() == 0) {\n>\n> According to https://patchwork.libcamera.org/patch/21294/ you should\n> probably use if frames.isEmpty() or simply with if (!frames)\n\nThanks! Done.\n\n>\n> > + data->config_.frame = TestPattern::ColorBars;\n> > + return 0;\n> > + }\n> > +\n> > + if (!frames.isDictionary()) {\n> > + LOG(Virtual, Error) << \"'frames' is not a dictionary.\";\n> > + return -EINVAL;\n> > + }\n> > +\n> > + std::string path = frames[\"path\"].get(\"\");\n>\n> I would be stricter, drop the default \"\" and check for\n> if (!path)\n>\n> and fail in this case\n\nUpdated with new logs.\n\n>\n> > +\n> > + if (auto ext = std::filesystem::path(path).extension();\n> > + ext == \".jpg\" || ext == \".jpeg\") {\n> > + data->config_.frame = ImageFrames{ path, std::nullopt };\n> > + } else if (std::filesystem::is_directory(std::filesystem::symlink_status(path))) {\n> > + using std::filesystem::directory_iterator;\n> > + unsigned int numOfFiles = std::distance(directory_iterator(path), directory_iterator{});\n> > + if (numOfFiles == 0) {\n> > + LOG(Virtual, Error) << \"Empty directory\";\n> > + return -EINVAL;\n> > + }\n> > + data->config_.frame = ImageFrames{ path, numOfFiles };\n> > + } else {\n> > + LOG(Virtual, Error) << \"Frame: \" << path << \" is not supported\";\n> > + return -EINVAL;\n> > + }\n> > +\n> > + return 0;\n> > +}\n> > +\n> > +int Parser::parseLocation(const YamlObject &cameraConfigData, VirtualCameraData *data)\n> > +{\n> > + std::string location = cameraConfigData[\"location\"].get(\"\");\n>\n> check for\n> if (!location) and exit early\n\nDo you think it's necessary? I specified in the README.md that\nit has the default value \"front\". As we also do that for resolutions,\nI suppose location can be a similar case?\n\n>\n> With this minors fixed I'll ack the next version\n>\n> > +\n> > + /* Default value is properties::CameraLocationFront */\n> > + if (location == \"front\" || location == \"\") {\n> > + data->properties_.set(properties::Location,\n> > + properties::CameraLocationFront);\n> > + } else if (location == \"back\") {\n> > + data->properties_.set(properties::Location,\n> > + properties::CameraLocationBack);\n> > + } else {\n> > + LOG(Virtual, Error) << \"location: \" << location\n> > + << \" is not supported\";\n> > + return -EINVAL;\n> > + }\n> > +\n> > + return 0;\n> > +}\n> > +\n> > +int Parser::parseModel(const YamlObject &cameraConfigData, VirtualCameraData *data)\n> > +{\n> > + std::string model = cameraConfigData[\"model\"].get(\"\");\n> > +\n> > + /* Default value is \"Unknown\" */\n> > + if (model == \"\")\n> > + data->properties_.set(properties::Model, \"Unknown\");\n> > + else\n> > + data->properties_.set(properties::Model, model);\n> > +\n> > + return 0;\n> > +}\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/pipeline/virtual/parser.h b/src/libcamera/pipeline/virtual/parser.h\n> > new file mode 100644\n> > index 00000000..9058f3c7\n> > --- /dev/null\n> > +++ b/src/libcamera/pipeline/virtual/parser.h\n> > @@ -0,0 +1,39 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2024, Google Inc.\n> > + *\n> > + * parser.h - Virtual cameras helper to parse config file\n> > + */\n> > +\n> > +#pragma once\n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \"libcamera/internal/pipeline_handler.h\"\n> > +#include \"libcamera/internal/yaml_parser.h\"\n> > +\n> > +#include \"virtual.h\"\n> > +\n> > +namespace libcamera {\n> > +\n> > +class Parser\n> > +{\n> > +public:\n> > + std::vector>\n> > + parseConfigFile(File &file, PipelineHandler *pipe);\n> > +\n> > +private:\n> > + std::unique_ptr\n> > + parseCameraConfigData(const YamlObject &cameraConfigData, PipelineHandler *pipe);\n> > +\n> > + int parseSupportedFormats(const YamlObject &cameraConfigData,\n> > + std::vector *resolutions);\n> > + int parseFrame(const YamlObject &cameraConfigData, VirtualCameraData *data);\n> > + int parseLocation(const YamlObject &cameraConfigData, VirtualCameraData *data);\n> > + int parseModel(const YamlObject &cameraConfigData, VirtualCameraData *data);\n> > +};\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp\n> > index 4e9a6973..b4744ca2 100644\n> > --- a/src/libcamera/pipeline/virtual/virtual.cpp\n> > +++ b/src/libcamera/pipeline/virtual/virtual.cpp\n> > @@ -26,6 +26,9 @@\n> > #include \"libcamera/internal/dma_buf_allocator.h\"\n> > #include \"libcamera/internal/formats.h\"\n> > #include \"libcamera/internal/pipeline_handler.h\"\n> > +#include \"libcamera/internal/yaml_parser.h\"\n> > +\n> > +#include \"parser.h\"\n> >\n> > namespace libcamera {\n> >\n> > @@ -44,6 +47,13 @@ uint64_t currentTimestamp()\n> >\n> > } /* namespace */\n> >\n> > +template\n> > +struct overloaded : Ts... {\n> > + using Ts::operator()...;\n> > +};\n> > +template\n> > +overloaded(Ts...) -> overloaded;\n> > +\n> > class VirtualCameraConfiguration : public CameraConfiguration\n> > {\n> > public:\n> > @@ -84,7 +94,7 @@ private:\n> > return static_cast(camera->_d());\n> > }\n> >\n> > - void initFrameGenerator(Camera *camera);\n> > + bool initFrameGenerator(Camera *camera);\n> >\n> > DmaBufAllocator dmaBufAllocator_;\n> > };\n> > @@ -94,15 +104,19 @@ bool PipelineHandlerVirtual::created_ = false;\n> >\n> > VirtualCameraData::VirtualCameraData(PipelineHandler *pipe,\n> > std::vector supportedResolutions)\n> > - : Camera::Private(pipe), supportedResolutions_(std::move(supportedResolutions))\n> > + : Camera::Private(pipe)\n> > {\n> > - for (const auto &resolution : supportedResolutions_) {\n> > - if (minResolutionSize_.isNull() || minResolutionSize_ > resolution.size)\n> > - minResolutionSize_ = resolution.size;\n> > + config_.resolutions = std::move(supportedResolutions);\n> > + for (const auto &resolution : config_.resolutions) {\n> > + if (config_.minResolutionSize.isNull() || config_.minResolutionSize > resolution.size)\n> > + config_.minResolutionSize = resolution.size;\n> >\n> > - maxResolutionSize_ = std::max(maxResolutionSize_, resolution.size);\n> > + config_.maxResolutionSize = std::max(config_.maxResolutionSize, resolution.size);\n> > }\n> >\n> > + properties_.set(properties::PixelArrayActiveAreas,\n> > + { Rectangle(config_.maxResolutionSize) });\n> > +\n> > /* \\todo Support multiple streams and pass multi_stream_test */\n> > streamConfigs_.resize(kMaxStream);\n> > }\n> > @@ -129,7 +143,7 @@ CameraConfiguration::Status VirtualCameraConfiguration::validate()\n> >\n> > for (StreamConfiguration &cfg : config_) {\n> > bool found = false;\n> > - for (const auto &resolution : data_->supportedResolutions_) {\n> > + for (const auto &resolution : data_->config_.resolutions) {\n> > if (resolution.size.width == cfg.size.width &&\n> > resolution.size.height == cfg.size.height) {\n> > found = true;\n> > @@ -144,7 +158,7 @@ CameraConfiguration::Status VirtualCameraConfiguration::validate()\n> > * Defining the default logic in PipelineHandler to\n> > * find the closest resolution would be nice.\n> > */\n> > - cfg.size = data_->maxResolutionSize_;\n> > + cfg.size = data_->config_.maxResolutionSize;\n> > status = Adjusted;\n> > }\n> >\n> > @@ -201,11 +215,11 @@ PipelineHandlerVirtual::generateConfiguration(Camera *camera,\n> >\n> > std::map> streamFormats;\n> > PixelFormat pixelFormat = formats::NV12;\n> > - streamFormats[pixelFormat] = { { data->minResolutionSize_, data->maxResolutionSize_ } };\n> > + streamFormats[pixelFormat] = { { data->config_.minResolutionSize, data->config_.maxResolutionSize } };\n> > StreamFormats formats(streamFormats);\n> > StreamConfiguration cfg(formats);\n> > cfg.pixelFormat = pixelFormat;\n> > - cfg.size = data->maxResolutionSize_;\n> > + cfg.size = data->config_.maxResolutionSize;\n> > cfg.bufferCount = VirtualCameraConfiguration::kBufferCount;\n> >\n> > config->addConfiguration(cfg);\n> > @@ -222,6 +236,7 @@ int PipelineHandlerVirtual::configure(Camera *camera,\n> > VirtualCameraData *data = cameraData(camera);\n> > for (auto [i, c] : utils::enumerate(*config)) {\n> > c.setStream(&data->streamConfigs_[i].stream);\n> > + /* Start reading the images/generating test patterns */\n> > data->streamConfigs_[i].frameGenerator->configure(c.size);\n> > }\n> >\n> > @@ -290,50 +305,65 @@ bool PipelineHandlerVirtual::match([[maybe_unused]] DeviceEnumerator *enumerator\n> >\n> > created_ = true;\n> >\n> > - /* \\todo Add virtual cameras according to a config file. */\n> > -\n> > - std::vector supportedResolutions;\n> > - supportedResolutions.resize(2);\n> > - supportedResolutions[0] = { .size = Size(1920, 1080), .frameRates = { 30 } };\n> > - supportedResolutions[1] = { .size = Size(1280, 720), .frameRates = { 30 } };\n> > -\n> > - std::unique_ptr data =\n> > - std::make_unique(this, supportedResolutions);\n> > -\n> > - data->properties_.set(properties::Location, properties::CameraLocationFront);\n> > - data->properties_.set(properties::Model, \"Virtual Video Device\");\n> > - data->properties_.set(properties::PixelArrayActiveAreas, { Rectangle(Size(1920, 1080)) });\n> > -\n> > - /* \\todo Set FrameDurationLimits based on config. */\n> > - ControlInfoMap::Map controls;\n> > - int64_t min_frame_duration = 33333, max_frame_duration = 33333;\n> > - controls[&controls::FrameDurationLimits] = ControlInfo(min_frame_duration, max_frame_duration);\n> > - data->controlInfo_ = ControlInfoMap(std::move(controls), controls::controls);\n> > -\n> > - /* Create and register the camera. */\n> > - std::set streams;\n> > - for (auto &streamConfig : data->streamConfigs_)\n> > - streams.insert(&streamConfig.stream);\n> > + File file(configurationFile(\"virtual\", \"virtual.yaml\"));\n> > + bool isOpen = file.open(File::OpenModeFlag::ReadOnly);\n> > + if (!isOpen) {\n> > + LOG(Virtual, Error) << \"Failed to open config file: \" << file.fileName();\n> > + return false;\n> > + }\n> >\n> > - const std::string id = \"Virtual0\";\n> > - std::shared_ptr camera = Camera::create(std::move(data), id, streams);\n> > + Parser parser;\n> > + auto configData = parser.parseConfigFile(file, this);\n> > + if (configData.size() == 0) {\n> > + LOG(Virtual, Error) << \"Failed to parse any cameras from the config file: \"\n> > + << file.fileName();\n> > + return false;\n> > + }\n> >\n> > - initFrameGenerator(camera.get());\n> > + /* Configure and register cameras with configData */\n> > + for (auto &data : configData) {\n> > + std::set streams;\n> > + for (auto &streamConfig : data->streamConfigs_)\n> > + streams.insert(&streamConfig.stream);\n> > + std::string id = data->config_.id;\n> > + std::shared_ptr camera = Camera::create(std::move(data), id, streams);\n> > +\n> > + if (!initFrameGenerator(camera.get())) {\n> > + LOG(Virtual, Error) << \"Failed to initialize frame \"\n> > + << \"generator for camera: \" << id;\n> > + continue;\n> > + }\n> >\n> > - registerCamera(std::move(camera));\n> > + registerCamera(std::move(camera));\n> > + }\n> >\n> > return true;\n> > }\n> >\n> > -void PipelineHandlerVirtual::initFrameGenerator(Camera *camera)\n> > +bool PipelineHandlerVirtual::initFrameGenerator(Camera *camera)\n> > {\n> > auto data = cameraData(camera);\n> > - for (auto &streamConfig : data->streamConfigs_) {\n> > - if (data->testPattern_ == TestPattern::DiagonalLines)\n> > - streamConfig.frameGenerator = std::make_unique();\n> > - else\n> > - streamConfig.frameGenerator = std::make_unique();\n> > - }\n> > + auto &frame = data->config_.frame;\n> > + std::visit(overloaded{\n> > + [&](TestPattern &testPattern) {\n> > + for (auto &streamConfig : data->streamConfigs_) {\n> > + if (testPattern == TestPattern::DiagonalLines)\n> > + streamConfig.frameGenerator = std::make_unique();\n> > + else\n> > + streamConfig.frameGenerator = std::make_unique();\n> > + }\n> > + },\n> > + [&](ImageFrames &imageFrames) {\n> > + for (auto &streamConfig : data->streamConfigs_)\n> > + streamConfig.frameGenerator = ImageFrameGenerator::create(imageFrames);\n> > + } },\n> > + frame);\n> > +\n> > + for (auto &streamConfig : data->streamConfigs_)\n> > + if (!streamConfig.frameGenerator)\n> > + return false;\n> > +\n> > + return true;\n> > }\n> >\n> > REGISTER_PIPELINE_HANDLER(PipelineHandlerVirtual, \"virtual\")\n> > diff --git a/src/libcamera/pipeline/virtual/virtual.h b/src/libcamera/pipeline/virtual/virtual.h\n> > index acdd437e..c99a0996 100644\n> > --- a/src/libcamera/pipeline/virtual/virtual.h\n> > +++ b/src/libcamera/pipeline/virtual/virtual.h\n> > @@ -7,6 +7,8 @@\n> >\n> > #pragma once\n> >\n> > +#include \n> > +#include \n> > #include \n> >\n> > #include \n> > @@ -14,10 +16,14 @@\n> > #include \"libcamera/internal/camera.h\"\n> > #include \"libcamera/internal/pipeline_handler.h\"\n> >\n> > +#include \"frame_generator.h\"\n> > +#include \"image_frame_generator.h\"\n> > #include \"test_pattern_generator.h\"\n> >\n> > namespace libcamera {\n> >\n> > +using VirtualFrame = std::variant;\n> > +\n> > class VirtualCameraData : public Camera::Private\n> > {\n> > public:\n> > @@ -31,17 +37,22 @@ public:\n> > Stream stream;\n> > std::unique_ptr frameGenerator;\n> > };\n> > + /* The config file is parsed to the Configuration struct */\n> > + struct Configuration {\n> > + std::string id;\n> > + std::vector resolutions;\n> > + VirtualFrame frame;\n> > +\n> > + Size maxResolutionSize;\n> > + Size minResolutionSize;\n> > + };\n> >\n> > VirtualCameraData(PipelineHandler *pipe,\n> > std::vector supportedResolutions);\n> >\n> > ~VirtualCameraData() = default;\n> >\n> > - TestPattern testPattern_ = TestPattern::ColorBars;\n> > -\n> > - const std::vector supportedResolutions_;\n> > - Size maxResolutionSize_;\n> > - Size minResolutionSize_;\n> > + Configuration config_;\n> >\n> > std::vector streamConfigs_;\n> > };\n> > --\n> > 2.46.0.598.g6f2099f65c-goog\n> >\n\n\n\n--\nBR,\nHarvey Yang","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 0A73FC0F1B\n\tfor ;\n\tThu, 26 Sep 2024 16:33:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B22F563520;\n\tThu, 26 Sep 2024 18:33:11 +0200 (CEST)","from mail-lf1-x136.google.com (mail-lf1-x136.google.com\n\t[IPv6:2a00:1450:4864:20::136])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 255F7634F9\n\tfor ;\n\tThu, 26 Sep 2024 18:33:09 +0200 (CEST)","by mail-lf1-x136.google.com with SMTP id\n\t2adb3069b0e04-536552fcc07so18789e87.1\n\tfor ;\n\tThu, 26 Sep 2024 09:33:09 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=google.com header.i=@google.com\n\theader.b=\"U07zNRy+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=google.com; s=20230601; t=1727368388; x=1727973188;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=GEFhQfNfyd48vpDsMsl0BdbRsqrSPWtg6fY4AZaqP8I=;\n\tb=U07zNRy++NpqoAobEqQ//1q6WkMV1uZKp2TyhDCNwoKjbqBL72zK6/iUK32cj7LO/J\n\t5YUKZaQC5kBcm/Kwht1YS+YCm9Wngvg4pgDoNZ8Z9jaUT+O25dd+4MMPZjgzYJFwQilu\n\tSpwr+csDa75dXdFxSSPXt3TXv2HXmQd5otSeR8WcsvBKSp54roVvpYTk+p/tFDPIKX97\n\t+DfDOcxRrKlk01Td83PwREH/p2716PuPZ4SKablZlPFHl2uafck5M4ZOIBAKRAGZW3Pm\n\t0sedOBRUVbQd3qcsitTapW+wJgKWibP5ZvId9DCk/VXvcBuIJsP5Jg77WpJpfMxDotkF\n\twpvg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1727368388; x=1727973188;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=GEFhQfNfyd48vpDsMsl0BdbRsqrSPWtg6fY4AZaqP8I=;\n\tb=vStMQ47+2oT5hf709P5YoqRySJRwL0UIw4KxrJO8or9jacermkXAABQ4zDo2dwIwa/\n\tj4LJozOVi7Vd1F2RBwR+t+AY1eQhvoqj5UuIZE++21vaVzh1crNflVrzFf/otwQvL/Xp\n\tyXGXb2BOaPSRR7CZiQFsWveg+tD6BLQ7+QoT0BMFFtJJDsIeDY0VBaXL7MG5mPewiISJ\n\tx8jck3+Qb2GwHbpz9zgDwRupGsQ2Ds5E6ul8g6gx3AFd3e76/ELVtzEfc61wJLez004D\n\tneJwqj6cX/K20B7eFnvtn1L5zNzwP5OwruV7kkk6bUQzL3vTuR4U2vCzOScXY7hLneMj\n\t04vA==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCVguR1d862wWWce+3ct7NGXA7OHKiR7YY7m0tvUYTtE6k9gNhpxRMplfbraWmUSc2NjL56lTNysoCTWeyT86io=@lists.libcamera.org","X-Gm-Message-State":"AOJu0YwqhQh8FzJ5WYad0NgJI7Dn1L/xgZf4IeGHjhyveNuRvOuHM6bs\n\tXdnA0q3C2Hio0nnKLFGLwcIn1V71IaNPfIcyd6AiHrd8YZGDCq7BtNBLAaW1UKVQWPUNzU9I7I2\n\ttzfRY3XTHJSKzCAnhEu/a0sT7+aGw419U5Wtm","X-Google-Smtp-Source":"AGHT+IHJllg0HS8rZyAf+A9dF/v0KF9LLIiqiC8VvIN+ftGIjFWbmavuS9Y31nxeT2s161rqCst01M5phc4K64G0pDw=","X-Received":"by 2002:a05:6512:ea6:b0:533:49ab:780e with SMTP id\n\t2adb3069b0e04-53897279d62mr308185e87.2.1727368387927; Thu, 26 Sep 2024\n\t09:33:07 -0700 (PDT)","MIME-Version":"1.0","References":"<20240910044834.2477701-1-chenghaoyang@google.com>\n\t<20240910044834.2477701-7-chenghaoyang@google.com>\n\t<2shqndkz74bxvnqv7qqfzi366ck2n3qwbmwzzftx2vxuetcrqw@snateic5g5p7>","In-Reply-To":"<2shqndkz74bxvnqv7qqfzi366ck2n3qwbmwzzftx2vxuetcrqw@snateic5g5p7>","From":"Cheng-Hao Yang ","Date":"Fri, 27 Sep 2024 00:32:31 +0800","Message-ID":"","Subject":"Re: [PATCH v12 6/7] libcamera: virtual: Read config and register\n\tcameras based on the config","To":"Jacopo Mondi ","Cc":"Harvey Yang ,\n\tlibcamera-devel@lists.libcamera.org, \n\tKonami Shu , Yunke Cao , \n\tTomasz Figa ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31419,"web_url":"https://patchwork.libcamera.org/comment/31419/","msgid":"","date":"2024-09-26T17:07:06","subject":"Re: [PATCH v12 6/7] libcamera: virtual: Read config and register\n\tcameras based on the config","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Harvey\n\nOn Fri, Sep 27, 2024 at 12:32:31AM GMT, Cheng-Hao Yang wrote:\n> Hi Jacopo,\n>\n> On Thu, Sep 26, 2024 at 9:15 PM Jacopo Mondi\n> wrote:\n> >\n> > Hi Harvey\n> >\n> > On Tue, Sep 10, 2024 at 04:40:19AM GMT, Harvey Yang wrote:\n> > > This patch introduces the configuration file for Virtual Pipeline\n> > > Handler. The config file is written in yaml, and the format is\n> > > documented in `README.md`.\n> > >\n> > > The config file will define the camera with IDs, supported formats and\n> > > image sources, etc. In the default config file, only Test Patterns are\n> > > used. Developers can use real images loading if desired.\n> > >\n> > > Signed-off-by: Konami Shu \n> > > Co-developed-by: Harvey Yang \n> > > Co-developed-by: Yunke Cao \n> > > Co-developed-by: Tomasz Figa \n> > > ---\n> > > src/libcamera/pipeline/virtual/README.md | 48 ++++\n> > > .../pipeline/virtual/data/virtual.yaml | 36 +++\n> > > src/libcamera/pipeline/virtual/meson.build | 1 +\n> > > src/libcamera/pipeline/virtual/parser.cpp | 241 ++++++++++++++++++\n> > > src/libcamera/pipeline/virtual/parser.h | 39 +++\n> > > src/libcamera/pipeline/virtual/virtual.cpp | 120 +++++----\n> > > src/libcamera/pipeline/virtual/virtual.h | 21 +-\n> > > 7 files changed, 456 insertions(+), 50 deletions(-)\n> > > create mode 100644 src/libcamera/pipeline/virtual/README.md\n> > > create mode 100644 src/libcamera/pipeline/virtual/data/virtual.yaml\n> > > create mode 100644 src/libcamera/pipeline/virtual/parser.cpp\n> > > create mode 100644 src/libcamera/pipeline/virtual/parser.h\n> > >\n> > > diff --git a/src/libcamera/pipeline/virtual/README.md b/src/libcamera/pipeline/virtual/README.md\n> > > new file mode 100644\n> > > index 00000000..84d7657d\n> > > --- /dev/null\n> > > +++ b/src/libcamera/pipeline/virtual/README.md\n> > > @@ -0,0 +1,48 @@\n> > > +# Virtual Pipeline Handler\n> > > +\n> > > +Virtual pipeline handler emulates fake external camera(s) on ChromeOS for testing.\n> > > +\n> > > +## Parse config file and register cameras\n> > > +\n> > > +- The config file is located at `src/libcamera/pipeline/virtual/data/virtual.yaml`\n> > > +\n> > > +### Config File Format\n> > > +The config file contains the information about cameras' properties to register.\n> > > +The config file should be a yaml file with dictionary of the cameraIds\n> > > +associated with their properties as top level. The default value will be applied when any property is empty.\n> > > +\n> > > +Each camera block is a dictionary, containing the following keys:\n> > > +- `supported_formats` (list of `VirtualCameraData::Resolution`, optional) : List of supported resolution and frame rates of the emulated camera\n> > > + - `width` (`unsigned int`, default=1920): Width of the window resolution. This needs to be even.\n> > > + - `height` (`unsigned int`, default=1080): Height of the window resolution.\n> > > + - `frame_rates` (list of `int`, default=`[30,60]` ): Range of the frame rate (per second). If the list contains one value, it's the lower bound and the upper bound. If the list contains two values, the first is the lower bound and the second is the upper bound. No other number of values is allowed.\n> > > +- `test_pattern` (`string`): Which test pattern to use as frames. The options are \"bars\", \"lines\". Cannot be set with `frames`.\n> > > +- `frames` (dictionary):\n> > > + - `path` (`string`): Path to an image, or path to a directory of a series of images. Cannot be set with `test_pattern`.\n> > > + - The test patterns are \"bars\" which means color bars, and \"lines\" which means diagonal lines.\n> > > + - The path to an image has \".jpg\" extension.\n> > > + - The path to a directory ends with \"/\". The name of the images in the directory are \"{n}.jpg\" with {n} is the sequence of images starting with 0.\n> > > +- `location` (`string`, default=\"front\"): The location of the camera. Support \"front\" and \"back\". This is displayed in qcam camera selection window but this does not change the output.\n> > > +- `model` (`string`, default=\"Unknown\"): The model name of the camera. This is displayed in qcam camera selection window but this does not change the output.\n> > > +\n> > > +Check `data/virtual.yaml` as the sample config file.\n> > > +\n> > > +### Implementation\n> > > +\n> > > +`Parser` class provides methods to parse the config file to register cameras\n> > > +in Virtual Pipeline Handler. `parseConfigFile()` is exposed to use in\n> > > +Virtual Pipeline Handler.\n> > > +\n> > > +This is the procedure of the Parser class:\n> > > +1. `parseConfigFile()` parses the config file to `YamlObject` using `YamlParser::parse()`.\n> > > + - Parse the top level of config file which are the camera ids and look into each camera properties.\n> > > +2. For each camera, `parseCameraConfigData()` returns a camera with the configuration.\n> > > + - The methods in the next step fill the data with the pointer to the Camera object.\n> > > + - If the config file contains invalid configuration, this method returns nullptr. The camera will be skipped.\n> > > +3. Parse each property and register the data.\n> > > + - `parseSupportedFormats()`: Parses `supported_formats` in the config, which contains resolutions and frame rates.\n> > > + - `parseFrame()`: Parses `test_pattern` or `frames` in the config.\n> > > + - `parseLocation()`: Parses `location` in the config.\n> > > + - `parseModel()`: Parses `model` in the config.\n> > > +4. Back to `parseConfigFile()` and append the camera configuration.\n> > > +5. Returns a list of camera configurations.\n> > > diff --git a/src/libcamera/pipeline/virtual/data/virtual.yaml b/src/libcamera/pipeline/virtual/data/virtual.yaml\n> > > new file mode 100644\n> > > index 00000000..6b73ddf2\n> > > --- /dev/null\n> > > +++ b/src/libcamera/pipeline/virtual/data/virtual.yaml\n> > > @@ -0,0 +1,36 @@\n> > > +# SPDX-License-Identifier: CC0-1.0\n> > > +%YAML 1.1\n> > > +---\n> > > +\"Virtual0\":\n> > > + supported_formats:\n> > > + - width: 1920\n> > > + height: 1080\n> > > + frame_rates:\n> > > + - 30\n> > > + - 60\n> > > + - width: 1680\n> > > + height: 1050\n> > > + frame_rates:\n> > > + - 70\n> > > + - 80\n> > > + test_pattern: \"lines\"\n> > > + location: \"front\"\n> > > + model: \"Virtual Video Device\"\n> > > +\"Virtual1\":\n> > > + supported_formats:\n> > > + - width: 800\n> > > + height: 600\n> > > + frame_rates:\n> > > + - 60\n> > > + test_pattern: \"bars\"\n> > > + location: \"back\"\n> > > + model: \"Virtual Video Device1\"\n> > > +\"Virtual2\":\n> > > + supported_formats:\n> > > + - width: 400\n> > > + height: 300\n> > > + test_pattern: \"lines\"\n> > > + location: \"front\"\n> > > + model: \"Virtual Video Device2\"\n> > > +\"Virtual3\":\n> > > + test_pattern: \"bars\"\n> > > diff --git a/src/libcamera/pipeline/virtual/meson.build b/src/libcamera/pipeline/virtual/meson.build\n> > > index 73d10cc3..395919b3 100644\n> > > --- a/src/libcamera/pipeline/virtual/meson.build\n> > > +++ b/src/libcamera/pipeline/virtual/meson.build\n> > > @@ -2,6 +2,7 @@\n> > >\n> > > libcamera_internal_sources += files([\n> > > 'image_frame_generator.cpp',\n> > > + 'parser.cpp',\n> > > 'test_pattern_generator.cpp',\n> > > 'virtual.cpp',\n> > > ])\n> > > diff --git a/src/libcamera/pipeline/virtual/parser.cpp b/src/libcamera/pipeline/virtual/parser.cpp\n> > > new file mode 100644\n> > > index 00000000..7e6c7fdf\n> > > --- /dev/null\n> > > +++ b/src/libcamera/pipeline/virtual/parser.cpp\n> > > @@ -0,0 +1,241 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2024, Google Inc.\n> > > + *\n> > > + * parser.cpp - Virtual cameras helper to parse config file\n> > > + */\n> > > +\n> > > +#include \"parser.h\"\n> > > +\n> > > +#include \n> >\n> > Included from the header file\n>\n> Removed.\n>\n>\n> >\n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \"libcamera/internal/pipeline_handler.h\"\n> > > +#include \"libcamera/internal/yaml_parser.h\"\n> > > +\n> > > +#include \"virtual.h\"\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +LOG_DECLARE_CATEGORY(Virtual)\n> > > +\n> > > +std::vector>\n> > > +Parser::parseConfigFile(File &file, PipelineHandler *pipe)\n> > > +{\n> > > + std::vector> configurations;\n> > > +\n> > > + std::unique_ptr cameras = YamlParser::parse(file);\n> > > + if (!cameras) {\n> > > + LOG(Virtual, Error) << \"Failed to pass config file.\";\n> > > + return configurations;\n> > > + }\n> > > +\n> > > + if (!cameras->isDictionary()) {\n> > > + LOG(Virtual, Error) << \"Config file is not a dictionary at the top level.\";\n> > > + return configurations;\n> > > + }\n> > > +\n> > > + /* Look into the configuration of each camera */\n> > > + for (const auto &[cameraId, cameraConfigData] : cameras->asDict()) {\n> > > + std::unique_ptr data =\n> > > + parseCameraConfigData(cameraConfigData, pipe);\n> > > + /* Parse configData to data*/\n> > > + if (!data) {\n> > > + /* Skip the camera if it has invalid config */\n> > > + LOG(Virtual, Error) << \"Failed to parse config of the camera: \"\n> > > + << cameraId;\n> > > + continue;\n> > > + }\n> > > +\n> > > + data->config_.id = cameraId;\n> > > + ControlInfoMap::Map controls;\n> > > + /* todo: Check which resolution's frame rate to be reported */\n> > > + controls[&controls::FrameDurationLimits] =\n> > > + ControlInfo(int64_t(1000000 / data->config_.resolutions[0].frameRates[1]),\n> > > + int64_t(1000000 / data->config_.resolutions[0].frameRates[0]));\n> > > + data->controlInfo_ = ControlInfoMap(std::move(controls), controls::controls);\n> > > + configurations.push_back(std::move(data));\n> > > + }\n> > > +\n> > > + return configurations;\n> > > +}\n> > > +\n> > > +std::unique_ptr\n> > > +Parser::parseCameraConfigData(const YamlObject &cameraConfigData,\n> > > + PipelineHandler *pipe)\n> > > +{\n> > > + std::vector resolutions;\n> > > + if (parseSupportedFormats(cameraConfigData, &resolutions))\n> > > + return nullptr;\n> > > +\n> > > + std::unique_ptr data =\n> > > + std::make_unique(pipe, resolutions);\n> > > +\n> > > + if (parseFrame(cameraConfigData, data.get()))\n> > > + return nullptr;\n> > > +\n> > > + if (parseLocation(cameraConfigData, data.get()))\n> > > + return nullptr;\n> > > +\n> > > + if (parseModel(cameraConfigData, data.get()))\n> > > + return nullptr;\n> > > +\n> > > + return data;\n> > > +}\n> > > +\n> > > +int Parser::parseSupportedFormats(const YamlObject &cameraConfigData,\n> > > + std::vector *resolutions)\n> > > +{\n> > > + if (cameraConfigData.contains(\"supported_formats\")) {\n> > > + const YamlObject &supportedResolutions = cameraConfigData[\"supported_formats\"];\n> > > +\n> > > + for (const YamlObject &supportedResolution : supportedResolutions.asList()) {\n> > > + unsigned int width = supportedResolution[\"width\"].get(1920);\n> > > + unsigned int height = supportedResolution[\"height\"].get(1080);\n> >\n> > Can this happen if you have 1920 and 1080 as defaults ?\n>\n> I assume you mean the `if` clause below.\n> Yes, if the config file specifies them to 0.\n>\n\nack\n\n> >\n> > > + if (width == 0 || height == 0) {\n> > > + LOG(Virtual, Error) << \"Invalid width or/and height\";\n> > > + return -EINVAL;\n> > > + }\n> > > + if (width % 2 != 0) {\n> > > + LOG(Virtual, Error) << \"Invalid width: width needs to be even\";\n> > > + return -EINVAL;\n> > > + }\n> > > +\n> > > + std::vector frameRates;\n> > > + if (supportedResolution.contains(\"frame_rates\")) {\n> > > + auto frameRatesList =\n> > > + supportedResolution[\"frame_rates\"].getList();\n> > > + if (!frameRatesList || (frameRatesList->size() != 1 &&\n> >\n> > Haven't you aleady checked that \"frameRatesList\" is in\n> > supportedResolutions ? (iow you could drop the !frameRatesList check\n> > imo)\n>\n> Checking `frameRatesList` is in supportedResolutions doesn't mean\n> it's a list. Config file may still set it to be other types that fails to call\n> getList here.\n>\n\nso, so the bool() operator will return false! ack\n\n> >\n> > > + frameRatesList->size() != 2)) {\n> > > + LOG(Virtual, Error) << \"Invalid frame_rates: either one or two values\";\n> > > + return -EINVAL;\n> > > + }\n> > > +\n> > > + if (frameRatesList->size() == 2 &&\n> > > + frameRatesList.value()[0] > frameRatesList.value()[1]) {\n> > > + LOG(Virtual, Error) << \"frame_rates's first value(lower bound)\"\n> > > + << \" is higher than the second value(upper bound)\";\n> > > + return -EINVAL;\n> > > + }\n> > > + frameRates.push_back(frameRatesList.value()[0]);\n> > > + if (frameRatesList->size() == 2)\n> > > + frameRates.push_back(frameRatesList.value()[1]);\n> > > + else\n> > > + frameRates.push_back(frameRatesList.value()[0]);\n> >\n> > You can skip the else branch if I got this right\n>\n> Do you mean we should allow having only one value in\n> `frameRatesList`? I prefer not. It means the users of\n> the list need to take care of this special case.\n>\n\nIgnore me, I mis-read the code\n\n>\n> >\n> > > + } else {\n> > > + frameRates.push_back(30);\n> > > + frameRates.push_back(60);\n> > > + }\n> > > +\n> > > + resolutions->emplace_back(\n> > > + VirtualCameraData::Resolution{ Size{ width, height },\n> > > + frameRates });\n> > > + }\n> > > + } else {\n> > > + resolutions->emplace_back(\n> > > + VirtualCameraData::Resolution{ Size{ 1920, 1080 },\n> > > + { 30, 60 } });\n> > > + }\n> > > +\n> > > + return 0;\n> > > +}\n> > > +\n> > > +int Parser::parseFrame(const YamlObject &cameraConfigData, VirtualCameraData *data)\n> >\n> > parseFrame is not a great name, maybe just parseFrameGenerator would\n> > be better\n>\n> Done\n>\n> >\n> > > +{\n> > > + const std::string testPatternKey = \"test_pattern\";\n> > > + const std::string framesKey = \"frames\";\n> > > + if (cameraConfigData.contains(testPatternKey)) {\n> > > + if (cameraConfigData.contains(framesKey)) {\n> > > + LOG(Virtual, Error) << \"A camera should use either \"\n> > > + << testPatternKey << \" or \" << framesKey;\n> > > + return -EINVAL;\n> > > + }\n> > > +\n> > > + auto testPattern = cameraConfigData[testPatternKey].get(\"\");\n> > > +\n> > > + if (testPattern == \"bars\") {\n> > > + data->config_.frame = TestPattern::ColorBars;\n> > > + } else if (testPattern == \"lines\") {\n> > > + data->config_.frame = TestPattern::DiagonalLines;\n> > > + } else {\n> > > + LOG(Virtual, Debug) << \"Test pattern: \" << testPattern\n> > > + << \"is not supported\";\n> >\n> > \" is not supported\"\n>\n> Done\n>\n> >\n> > > + return -EINVAL;\n> > > + }\n> > > +\n> > > + return 0;\n> > > + }\n> > > +\n> > > + const YamlObject &frames = cameraConfigData[framesKey];\n> > > +\n> > > + /* When there is no frames provided in the config file, use color bar test pattern */\n> > > + if (frames.size() == 0) {\n> >\n> > According to https://patchwork.libcamera.org/patch/21294/ you should\n> > probably use if frames.isEmpty() or simply with if (!frames)\n>\n> Thanks! Done.\n>\n> >\n> > > + data->config_.frame = TestPattern::ColorBars;\n> > > + return 0;\n> > > + }\n> > > +\n> > > + if (!frames.isDictionary()) {\n> > > + LOG(Virtual, Error) << \"'frames' is not a dictionary.\";\n> > > + return -EINVAL;\n> > > + }\n> > > +\n> > > + std::string path = frames[\"path\"].get(\"\");\n> >\n> > I would be stricter, drop the default \"\" and check for\n> > if (!path)\n> >\n> > and fail in this case\n>\n> Updated with new logs.\n>\n> >\n> > > +\n> > > + if (auto ext = std::filesystem::path(path).extension();\n> > > + ext == \".jpg\" || ext == \".jpeg\") {\n> > > + data->config_.frame = ImageFrames{ path, std::nullopt };\n> > > + } else if (std::filesystem::is_directory(std::filesystem::symlink_status(path))) {\n> > > + using std::filesystem::directory_iterator;\n> > > + unsigned int numOfFiles = std::distance(directory_iterator(path), directory_iterator{});\n> > > + if (numOfFiles == 0) {\n> > > + LOG(Virtual, Error) << \"Empty directory\";\n> > > + return -EINVAL;\n> > > + }\n> > > + data->config_.frame = ImageFrames{ path, numOfFiles };\n> > > + } else {\n> > > + LOG(Virtual, Error) << \"Frame: \" << path << \" is not supported\";\n> > > + return -EINVAL;\n> > > + }\n> > > +\n> > > + return 0;\n> > > +}\n> > > +\n> > > +int Parser::parseLocation(const YamlObject &cameraConfigData, VirtualCameraData *data)\n> > > +{\n> > > + std::string location = cameraConfigData[\"location\"].get(\"\");\n> >\n> > check for\n> > if (!location) and exit early\n>\n> Do you think it's necessary? I specified in the README.md that\n> it has the default value \"front\". As we also do that for resolutions,\n> I suppose location can be a similar case?\n\nOk, I presume with the above mentioned patch from Stefan the default\n\"\" value is now respected even if \"location\" is not there.\n\nThanks\n j\n\n>\n> >\n> > With this minors fixed I'll ack the next version\n> >\n> > > +\n> > > + /* Default value is properties::CameraLocationFront */\n> > > + if (location == \"front\" || location == \"\") {\n> > > + data->properties_.set(properties::Location,\n> > > + properties::CameraLocationFront);\n> > > + } else if (location == \"back\") {\n> > > + data->properties_.set(properties::Location,\n> > > + properties::CameraLocationBack);\n> > > + } else {\n> > > + LOG(Virtual, Error) << \"location: \" << location\n> > > + << \" is not supported\";\n> > > + return -EINVAL;\n> > > + }\n> > > +\n> > > + return 0;\n> > > +}\n> > > +\n> > > +int Parser::parseModel(const YamlObject &cameraConfigData, VirtualCameraData *data)\n> > > +{\n> > > + std::string model = cameraConfigData[\"model\"].get(\"\");\n> > > +\n> > > + /* Default value is \"Unknown\" */\n> > > + if (model == \"\")\n> > > + data->properties_.set(properties::Model, \"Unknown\");\n> > > + else\n> > > + data->properties_.set(properties::Model, model);\n> > > +\n> > > + return 0;\n> > > +}\n> > > +\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/libcamera/pipeline/virtual/parser.h b/src/libcamera/pipeline/virtual/parser.h\n> > > new file mode 100644\n> > > index 00000000..9058f3c7\n> > > --- /dev/null\n> > > +++ b/src/libcamera/pipeline/virtual/parser.h\n> > > @@ -0,0 +1,39 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2024, Google Inc.\n> > > + *\n> > > + * parser.h - Virtual cameras helper to parse config file\n> > > + */\n> > > +\n> > > +#pragma once\n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +#include \"libcamera/internal/pipeline_handler.h\"\n> > > +#include \"libcamera/internal/yaml_parser.h\"\n> > > +\n> > > +#include \"virtual.h\"\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +class Parser\n> > > +{\n> > > +public:\n> > > + std::vector>\n> > > + parseConfigFile(File &file, PipelineHandler *pipe);\n> > > +\n> > > +private:\n> > > + std::unique_ptr\n> > > + parseCameraConfigData(const YamlObject &cameraConfigData, PipelineHandler *pipe);\n> > > +\n> > > + int parseSupportedFormats(const YamlObject &cameraConfigData,\n> > > + std::vector *resolutions);\n> > > + int parseFrame(const YamlObject &cameraConfigData, VirtualCameraData *data);\n> > > + int parseLocation(const YamlObject &cameraConfigData, VirtualCameraData *data);\n> > > + int parseModel(const YamlObject &cameraConfigData, VirtualCameraData *data);\n> > > +};\n> > > +\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp\n> > > index 4e9a6973..b4744ca2 100644\n> > > --- a/src/libcamera/pipeline/virtual/virtual.cpp\n> > > +++ b/src/libcamera/pipeline/virtual/virtual.cpp\n> > > @@ -26,6 +26,9 @@\n> > > #include \"libcamera/internal/dma_buf_allocator.h\"\n> > > #include \"libcamera/internal/formats.h\"\n> > > #include \"libcamera/internal/pipeline_handler.h\"\n> > > +#include \"libcamera/internal/yaml_parser.h\"\n> > > +\n> > > +#include \"parser.h\"\n> > >\n> > > namespace libcamera {\n> > >\n> > > @@ -44,6 +47,13 @@ uint64_t currentTimestamp()\n> > >\n> > > } /* namespace */\n> > >\n> > > +template\n> > > +struct overloaded : Ts... {\n> > > + using Ts::operator()...;\n> > > +};\n> > > +template\n> > > +overloaded(Ts...) -> overloaded;\n> > > +\n> > > class VirtualCameraConfiguration : public CameraConfiguration\n> > > {\n> > > public:\n> > > @@ -84,7 +94,7 @@ private:\n> > > return static_cast(camera->_d());\n> > > }\n> > >\n> > > - void initFrameGenerator(Camera *camera);\n> > > + bool initFrameGenerator(Camera *camera);\n> > >\n> > > DmaBufAllocator dmaBufAllocator_;\n> > > };\n> > > @@ -94,15 +104,19 @@ bool PipelineHandlerVirtual::created_ = false;\n> > >\n> > > VirtualCameraData::VirtualCameraData(PipelineHandler *pipe,\n> > > std::vector supportedResolutions)\n> > > - : Camera::Private(pipe), supportedResolutions_(std::move(supportedResolutions))\n> > > + : Camera::Private(pipe)\n> > > {\n> > > - for (const auto &resolution : supportedResolutions_) {\n> > > - if (minResolutionSize_.isNull() || minResolutionSize_ > resolution.size)\n> > > - minResolutionSize_ = resolution.size;\n> > > + config_.resolutions = std::move(supportedResolutions);\n> > > + for (const auto &resolution : config_.resolutions) {\n> > > + if (config_.minResolutionSize.isNull() || config_.minResolutionSize > resolution.size)\n> > > + config_.minResolutionSize = resolution.size;\n> > >\n> > > - maxResolutionSize_ = std::max(maxResolutionSize_, resolution.size);\n> > > + config_.maxResolutionSize = std::max(config_.maxResolutionSize, resolution.size);\n> > > }\n> > >\n> > > + properties_.set(properties::PixelArrayActiveAreas,\n> > > + { Rectangle(config_.maxResolutionSize) });\n> > > +\n> > > /* \\todo Support multiple streams and pass multi_stream_test */\n> > > streamConfigs_.resize(kMaxStream);\n> > > }\n> > > @@ -129,7 +143,7 @@ CameraConfiguration::Status VirtualCameraConfiguration::validate()\n> > >\n> > > for (StreamConfiguration &cfg : config_) {\n> > > bool found = false;\n> > > - for (const auto &resolution : data_->supportedResolutions_) {\n> > > + for (const auto &resolution : data_->config_.resolutions) {\n> > > if (resolution.size.width == cfg.size.width &&\n> > > resolution.size.height == cfg.size.height) {\n> > > found = true;\n> > > @@ -144,7 +158,7 @@ CameraConfiguration::Status VirtualCameraConfiguration::validate()\n> > > * Defining the default logic in PipelineHandler to\n> > > * find the closest resolution would be nice.\n> > > */\n> > > - cfg.size = data_->maxResolutionSize_;\n> > > + cfg.size = data_->config_.maxResolutionSize;\n> > > status = Adjusted;\n> > > }\n> > >\n> > > @@ -201,11 +215,11 @@ PipelineHandlerVirtual::generateConfiguration(Camera *camera,\n> > >\n> > > std::map> streamFormats;\n> > > PixelFormat pixelFormat = formats::NV12;\n> > > - streamFormats[pixelFormat] = { { data->minResolutionSize_, data->maxResolutionSize_ } };\n> > > + streamFormats[pixelFormat] = { { data->config_.minResolutionSize, data->config_.maxResolutionSize } };\n> > > StreamFormats formats(streamFormats);\n> > > StreamConfiguration cfg(formats);\n> > > cfg.pixelFormat = pixelFormat;\n> > > - cfg.size = data->maxResolutionSize_;\n> > > + cfg.size = data->config_.maxResolutionSize;\n> > > cfg.bufferCount = VirtualCameraConfiguration::kBufferCount;\n> > >\n> > > config->addConfiguration(cfg);\n> > > @@ -222,6 +236,7 @@ int PipelineHandlerVirtual::configure(Camera *camera,\n> > > VirtualCameraData *data = cameraData(camera);\n> > > for (auto [i, c] : utils::enumerate(*config)) {\n> > > c.setStream(&data->streamConfigs_[i].stream);\n> > > + /* Start reading the images/generating test patterns */\n> > > data->streamConfigs_[i].frameGenerator->configure(c.size);\n> > > }\n> > >\n> > > @@ -290,50 +305,65 @@ bool PipelineHandlerVirtual::match([[maybe_unused]] DeviceEnumerator *enumerator\n> > >\n> > > created_ = true;\n> > >\n> > > - /* \\todo Add virtual cameras according to a config file. */\n> > > -\n> > > - std::vector supportedResolutions;\n> > > - supportedResolutions.resize(2);\n> > > - supportedResolutions[0] = { .size = Size(1920, 1080), .frameRates = { 30 } };\n> > > - supportedResolutions[1] = { .size = Size(1280, 720), .frameRates = { 30 } };\n> > > -\n> > > - std::unique_ptr data =\n> > > - std::make_unique(this, supportedResolutions);\n> > > -\n> > > - data->properties_.set(properties::Location, properties::CameraLocationFront);\n> > > - data->properties_.set(properties::Model, \"Virtual Video Device\");\n> > > - data->properties_.set(properties::PixelArrayActiveAreas, { Rectangle(Size(1920, 1080)) });\n> > > -\n> > > - /* \\todo Set FrameDurationLimits based on config. */\n> > > - ControlInfoMap::Map controls;\n> > > - int64_t min_frame_duration = 33333, max_frame_duration = 33333;\n> > > - controls[&controls::FrameDurationLimits] = ControlInfo(min_frame_duration, max_frame_duration);\n> > > - data->controlInfo_ = ControlInfoMap(std::move(controls), controls::controls);\n> > > -\n> > > - /* Create and register the camera. */\n> > > - std::set streams;\n> > > - for (auto &streamConfig : data->streamConfigs_)\n> > > - streams.insert(&streamConfig.stream);\n> > > + File file(configurationFile(\"virtual\", \"virtual.yaml\"));\n> > > + bool isOpen = file.open(File::OpenModeFlag::ReadOnly);\n> > > + if (!isOpen) {\n> > > + LOG(Virtual, Error) << \"Failed to open config file: \" << file.fileName();\n> > > + return false;\n> > > + }\n> > >\n> > > - const std::string id = \"Virtual0\";\n> > > - std::shared_ptr camera = Camera::create(std::move(data), id, streams);\n> > > + Parser parser;\n> > > + auto configData = parser.parseConfigFile(file, this);\n> > > + if (configData.size() == 0) {\n> > > + LOG(Virtual, Error) << \"Failed to parse any cameras from the config file: \"\n> > > + << file.fileName();\n> > > + return false;\n> > > + }\n> > >\n> > > - initFrameGenerator(camera.get());\n> > > + /* Configure and register cameras with configData */\n> > > + for (auto &data : configData) {\n> > > + std::set streams;\n> > > + for (auto &streamConfig : data->streamConfigs_)\n> > > + streams.insert(&streamConfig.stream);\n> > > + std::string id = data->config_.id;\n> > > + std::shared_ptr camera = Camera::create(std::move(data), id, streams);\n> > > +\n> > > + if (!initFrameGenerator(camera.get())) {\n> > > + LOG(Virtual, Error) << \"Failed to initialize frame \"\n> > > + << \"generator for camera: \" << id;\n> > > + continue;\n> > > + }\n> > >\n> > > - registerCamera(std::move(camera));\n> > > + registerCamera(std::move(camera));\n> > > + }\n> > >\n> > > return true;\n> > > }\n> > >\n> > > -void PipelineHandlerVirtual::initFrameGenerator(Camera *camera)\n> > > +bool PipelineHandlerVirtual::initFrameGenerator(Camera *camera)\n> > > {\n> > > auto data = cameraData(camera);\n> > > - for (auto &streamConfig : data->streamConfigs_) {\n> > > - if (data->testPattern_ == TestPattern::DiagonalLines)\n> > > - streamConfig.frameGenerator = std::make_unique();\n> > > - else\n> > > - streamConfig.frameGenerator = std::make_unique();\n> > > - }\n> > > + auto &frame = data->config_.frame;\n> > > + std::visit(overloaded{\n> > > + [&](TestPattern &testPattern) {\n> > > + for (auto &streamConfig : data->streamConfigs_) {\n> > > + if (testPattern == TestPattern::DiagonalLines)\n> > > + streamConfig.frameGenerator = std::make_unique();\n> > > + else\n> > > + streamConfig.frameGenerator = std::make_unique();\n> > > + }\n> > > + },\n> > > + [&](ImageFrames &imageFrames) {\n> > > + for (auto &streamConfig : data->streamConfigs_)\n> > > + streamConfig.frameGenerator = ImageFrameGenerator::create(imageFrames);\n> > > + } },\n> > > + frame);\n> > > +\n> > > + for (auto &streamConfig : data->streamConfigs_)\n> > > + if (!streamConfig.frameGenerator)\n> > > + return false;\n> > > +\n> > > + return true;\n> > > }\n> > >\n> > > REGISTER_PIPELINE_HANDLER(PipelineHandlerVirtual, \"virtual\")\n> > > diff --git a/src/libcamera/pipeline/virtual/virtual.h b/src/libcamera/pipeline/virtual/virtual.h\n> > > index acdd437e..c99a0996 100644\n> > > --- a/src/libcamera/pipeline/virtual/virtual.h\n> > > +++ b/src/libcamera/pipeline/virtual/virtual.h\n> > > @@ -7,6 +7,8 @@\n> > >\n> > > #pragma once\n> > >\n> > > +#include \n> > > +#include \n> > > #include \n> > >\n> > > #include \n> > > @@ -14,10 +16,14 @@\n> > > #include \"libcamera/internal/camera.h\"\n> > > #include \"libcamera/internal/pipeline_handler.h\"\n> > >\n> > > +#include \"frame_generator.h\"\n> > > +#include \"image_frame_generator.h\"\n> > > #include \"test_pattern_generator.h\"\n> > >\n> > > namespace libcamera {\n> > >\n> > > +using VirtualFrame = std::variant;\n> > > +\n> > > class VirtualCameraData : public Camera::Private\n> > > {\n> > > public:\n> > > @@ -31,17 +37,22 @@ public:\n> > > Stream stream;\n> > > std::unique_ptr frameGenerator;\n> > > };\n> > > + /* The config file is parsed to the Configuration struct */\n> > > + struct Configuration {\n> > > + std::string id;\n> > > + std::vector resolutions;\n> > > + VirtualFrame frame;\n> > > +\n> > > + Size maxResolutionSize;\n> > > + Size minResolutionSize;\n> > > + };\n> > >\n> > > VirtualCameraData(PipelineHandler *pipe,\n> > > std::vector supportedResolutions);\n> > >\n> > > ~VirtualCameraData() = default;\n> > >\n> > > - TestPattern testPattern_ = TestPattern::ColorBars;\n> > > -\n> > > - const std::vector supportedResolutions_;\n> > > - Size maxResolutionSize_;\n> > > - Size minResolutionSize_;\n> > > + Configuration config_;\n> > >\n> > > std::vector streamConfigs_;\n> > > };\n> > > --\n> > > 2.46.0.598.g6f2099f65c-goog\n> > >\n>\n>\n>\n> --\n> BR,\n> Harvey Yang","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 937F5C3257\n\tfor ;\n\tThu, 26 Sep 2024 17:07:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 89BC56350F;\n\tThu, 26 Sep 2024 19:07:32 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id ED4E2634F9\n\tfor ;\n\tThu, 26 Sep 2024 19:07:30 +0200 (CEST)","from ideasonboard.com (mob-5-90-51-229.net.vodafone.it\n\t[5.90.51.229])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 77A88163;\n\tThu, 26 Sep 2024 19:05:52 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"LNakyK4/\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1727370362;\n\tbh=t2aaEioPg2JulMkIqzWxUliBWQTW1jm7iNn4nhA+dmI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LNakyK4/L0VYJGS46Tm/tzKCuKC6KRqMN4aNfOxcFz/JBbtafZI5jTeROz+mJ/JJi\n\t+6j0m522X9wqeVqSz/V5q0cOkb2be4rJTE6CYgXdEQq6l/gJWKdRhL5ByLisj1SJri\n\twiTTO20Mq62rpbN84OQ/ZibSlVepeoya/vYGSaWk=","Date":"Thu, 26 Sep 2024 19:07:06 +0200","From":"Jacopo Mondi ","To":"Cheng-Hao Yang ","Cc":"Jacopo Mondi , \n\tHarvey Yang ,\n\tlibcamera-devel@lists.libcamera.org, \n\tKonami Shu , Yunke Cao , \n\tTomasz Figa ","Subject":"Re: [PATCH v12 6/7] libcamera: virtual: Read config and register\n\tcameras based on the config","Message-ID":"","References":"<20240910044834.2477701-1-chenghaoyang@google.com>\n\t<20240910044834.2477701-7-chenghaoyang@google.com>\n\t<2shqndkz74bxvnqv7qqfzi366ck2n3qwbmwzzftx2vxuetcrqw@snateic5g5p7>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]