[{"id":31562,"web_url":"https://patchwork.libcamera.org/comment/31562/","msgid":"<172795626198.1619946.1174039723197152006@ping.linuxembedded.co.uk>","date":"2024-10-03T11:51:01","subject":"Re: [PATCH v14 6/7] libcamera: virtual: Read config and register\n\tcameras based on the config","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Harvey Yang (2024-09-30 07:29:47)\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 <konamiz@google.com>\n> Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>\n\nCI complains that you don't have an explicit SoB tag here, but you're\nlisted as the Author.\n\nI think that's on another patch too... something to look for.\n\n\n> Co-developed-by: Yunke Cao <yunkec@chromium.org>\n> Co-developed-by: Tomasz Figa <tfiga@chromium.org>\n> ---\n>  src/libcamera/pipeline/virtual/README.md      |  48 ++++\n>  .../pipeline/virtual/data/virtual.yaml        |  36 +++\n>  .../virtual/image_frame_generator.cpp         |  16 +-\n>  .../pipeline/virtual/image_frame_generator.h  |   5 +-\n>  src/libcamera/pipeline/virtual/meson.build    |   1 +\n>  src/libcamera/pipeline/virtual/parser.cpp     | 260 ++++++++++++++++++\n>  src/libcamera/pipeline/virtual/parser.h       |  39 +++\n>  src/libcamera/pipeline/virtual/virtual.cpp    | 122 ++++----\n>  src/libcamera/pipeline/virtual/virtual.h      |  21 +-\n>  9 files changed, 483 insertions(+), 65 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..b51eb5ac\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\nWhere does it get installed ?\n\nSurely this is what users will modify ?\n\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\nMaybe some level of wrapping would be nice throughout ?\n\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> +    - `parseFrameGenerator()`: 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/image_frame_generator.cpp b/src/libcamera/pipeline/virtual/image_frame_generator.cpp\n> index a1915b24..26ee1a54 100644\n> --- a/src/libcamera/pipeline/virtual/image_frame_generator.cpp\n> +++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp\n> @@ -40,15 +40,7 @@ ImageFrameGenerator::create(ImageFrames &imageFrames)\n>          * For each file in the directory, load the image,\n>          * convert it to NV12, and store the pointer.\n>          */\n> -       for (unsigned int i = 0; i < imageFrames.number.value_or(1); i++) {\n> -               std::filesystem::path path;\n> -               if (!imageFrames.number)\n> -                       /* If the path is to an image */\n> -                       path = imageFrames.path;\n> -               else\n> -                       /* If the path is to a directory */\n> -                       path = imageFrames.path / (std::to_string(i) + \".jpg\");\n> -\n> +       for (std::filesystem::path path : imageFrames.files) {\n>                 File file(path);\n>                 if (!file.open(File::OpenModeFlag::ReadOnly)) {\n>                         LOG(Virtual, Error) << \"Failed to open image file \" << file.fileName()\n> @@ -88,6 +80,8 @@ ImageFrameGenerator::create(ImageFrames &imageFrames)\n>                                         Size(width, height) });\n>         }\n>  \n> +       ASSERT(!imageFrameGenerator->imageFrameDatas_.empty());\n> +\n>         return imageFrameGenerator;\n>  }\n>  \n> @@ -99,7 +93,7 @@ void ImageFrameGenerator::configure(const Size &size)\n>         frameCount_ = 0;\n>         parameter_ = 0;\n>  \n> -       for (unsigned int i = 0; i < imageFrames_->number.value_or(1); i++) {\n> +       for (unsigned int i = 0; i < imageFrameDatas_.size(); i++) {\n>                 /* Scale the imageFrameDatas_ to scaledY and scaledUV */\n>                 unsigned int halfSizeWidth = (size.width + 1) / 2;\n>                 unsigned int halfSizeHeight = (size.height + 1) / 2;\n> @@ -135,7 +129,7 @@ int ImageFrameGenerator::generateFrame(const Size &size, const FrameBuffer *buff\n>         auto planes = mappedFrameBuffer.planes();\n>  \n>         /* Make sure the frameCount does not over the number of images */\n> -       frameCount_ %= imageFrames_->number.value_or(1);\n> +       frameCount_ %= imageFrameDatas_.size();\n>  \n>         /* Write the scaledY and scaledUV to the mapped frame buffer */\n>         libyuv::NV12Copy(scaledFrameDatas_[frameCount_].Y.get(), size.width,\n> diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.h b/src/libcamera/pipeline/virtual/image_frame_generator.h\n> index cd243816..6e83ab20 100644\n> --- a/src/libcamera/pipeline/virtual/image_frame_generator.h\n> +++ b/src/libcamera/pipeline/virtual/image_frame_generator.h\n> @@ -10,9 +10,9 @@\n>  \n>  #include <filesystem>\n>  #include <memory>\n> -#include <optional>\n>  #include <stdint.h>\n>  #include <sys/types.h>\n> +#include <vector>\n>  \n>  #include \"frame_generator.h\"\n>  \n> @@ -20,8 +20,7 @@ namespace libcamera {\n>  \n>  /* Frame configuration provided by the config file */\n>  struct ImageFrames {\n> -       std::filesystem::path path;\n> -       std::optional<unsigned int> number;\n> +       std::vector<std::filesystem::path> files;\n>  };\n>  \n>  class ImageFrameGenerator : public FrameGenerator\n> diff --git a/src/libcamera/pipeline/virtual/meson.build b/src/libcamera/pipeline/virtual/meson.build\n> index bb38c193..f3cb80b4 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\nconfig_parser.cpp ?\n\n\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..cc782d12\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/virtual/parser.cpp\n> @@ -0,0 +1,260 @@\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 <string.h>\n> +#include <utility>\n> +\n> +#include <libcamera/base/log.h>\n> +\n> +#include <libcamera/control_ids.h>\n> +#include <libcamera/property_ids.h>\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<std::unique_ptr<VirtualCameraData>>\n> +Parser::parseConfigFile(File &file, PipelineHandler *pipe)\n> +{\n> +       std::vector<std::unique_ptr<VirtualCameraData>> configurations;\n> +\n> +       std::unique_ptr<YamlObject> 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<VirtualCameraData> data =\n> +                       parseCameraConfigData(cameraConfigData, pipe);\n> +               /* Parse configData to data*/\n\nWatch those end of comments.\n\n\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\nShould it just be the min/max ?\n\n> +               controls[&controls::FrameDurationLimits] =\n> +                       ControlInfo(int64_t(1000000 / data->config_.resolutions[0].frameRates[1]),\n\nc++ casts.\n\nPerhaps just calculate the frame duration min/max in their own int64_t\nvariable which probably then avoids a cast anyway.\n\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<VirtualCameraData>\n> +Parser::parseCameraConfigData(const YamlObject &cameraConfigData,\n> +                             PipelineHandler *pipe)\n> +{\n> +       std::vector<VirtualCameraData::Resolution> resolutions;\n> +       if (parseSupportedFormats(cameraConfigData, &resolutions))\n> +               return nullptr;\n> +\n> +       std::unique_ptr<VirtualCameraData> data =\n> +               std::make_unique<VirtualCameraData>(pipe, resolutions);\n> +\n> +       if (parseFrameGenerator(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<VirtualCameraData::Resolution> *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<unsigned int>(1920);\n> +                       unsigned int height = supportedResolution[\"height\"].get<unsigned int>(1080);\n> +                       if (width == 0 || height == 0) {\n> +                               LOG(Virtual, Error) << \"Invalid width or/and height\";\n\nCan this happen? You've added defaults of 1920, 1080.\nWhat happens if only one default gets there?\n\n\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<int> frameRates;\n> +                       if (supportedResolution.contains(\"frame_rates\")) {\n> +                               auto frameRatesList =\n> +                                       supportedResolution[\"frame_rates\"].getList<int>();\n> +                               if (!frameRatesList || (frameRatesList->size() != 1 &&\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\nNot worth anything specifically now - but I wonder would this work/be\nclean:\n\n\t\t\t\t/*\n\t\t\t\t * Push the min and max framerates. A\n\t\t\t\t * single rate is duplicated.\n\t\t\t\t */\n\t\t\t\tframeRates.push_back(frameRatesList.front());\n\t\t\t\tframeRates.push_back(frameRatesList.back());\n\nAnd in fact *all* of that code above could possibly  be simplified with\n(not complied/tested) code:\n\n#include <algorithm>\n\n\n\t\t\tstd::vector<int> frameRates;\n\t\t\tauto frameRatesList = supportedResolution[\"frame_rates\"].getList<int>({30, 60});\n\t\t\tunsigned int minRate = *std::min_element(frameRatesList.begin(), frameRatesList.end());\n\t\t\tunsigned int maxRate = *std::max_element(frameRatesList.begin(), frameRatesList.end());\n\n\t\t\tframeRates.push_back(minRate);\n\t\t\tframeRates.push_back(maxRate);\n\n\t\t\tresolutions->emplace_back(VirtualCameraData::Resolution{ Size{ width, height },\n\t\t\t\t\t\t\t\t\t\t frameRates });\n\nI can't believe we don't have a class to make a range of a minimum and\nmaximum in libcamera yet ;-)\n\n\n\n> +                       } else {\n> +                               frameRates.push_back(30);\n> +                               frameRates.push_back(60);\n> +                       }\n\nAnd those would get picked from the getList<int>({30, 60}); above then.\n\n\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::parseFrameGenerator(const YamlObject &cameraConfigData, VirtualCameraData *data)\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<std::string>(\"\");\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> +                       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) {\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> +       auto path = frames[\"path\"].get<std::string>();\n> +\n> +       if (!path) {\n> +               LOG(Virtual, Error) << \"Test pattern or path should be specified.\";\n> +               return -EINVAL;\n> +       }\n> +\n> +       std::vector<std::filesystem::path> files;\n> +\n> +       switch (std::filesystem::symlink_status(*path).type()) {\n> +       case std::filesystem::file_type::regular:\n> +               files.push_back(*path);\n> +               break;\n> +\n> +       case std::filesystem::file_type::directory:\n> +               for (const auto &dentry : std::filesystem::directory_iterator{ *path }) {\n> +                       if (dentry.is_regular_file())\n> +                               files.push_back(dentry.path());\n> +               }\n> +\n> +               std::sort(files.begin(), files.end(), [](const auto &a, const auto &b) {\n> +                       return ::strverscmp(a.c_str(), b.c_str()) < 0;\n> +               });\n> +\n> +               if (files.empty()) {\n> +                       LOG(Virtual, Error) << \"Directory has no files: \" << *path;\n> +                       return -EINVAL;\n> +               }\n> +               break;\n> +\n> +       default:\n> +               LOG(Virtual, Error) << \"Frame: \" << *path << \" is not supported\";\n> +               return -EINVAL;\n> +       }\n> +\n> +       data->config_.frame = ImageFrames{ std::move(files) };\n> +\n> +       return 0;\n> +}\n> +\n> +int Parser::parseLocation(const YamlObject &cameraConfigData, VirtualCameraData *data)\n> +{\n> +       std::string location = cameraConfigData[\"location\"].get<std::string>(\"\");\n\nWhy not put 'front' in the .get<std::string>(\"front\"); ?\n\n> +\n> +       /* Default value is properties::CameraLocationFront */\n> +       if (location == \"front\" || location == \"\") {\n\nand simplify here ?\n\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<std::string>(\"\");\n\nAgain - put the 'Unknown' into the default, and remove the if below.\n\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..6717be31\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 <memory>\n> +#include <vector>\n> +\n> +#include <libcamera/base/file.h>\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<std::unique_ptr<VirtualCameraData>>\n> +       parseConfigFile(File &file, PipelineHandler *pipe);\n> +\n> +private:\n> +       std::unique_ptr<VirtualCameraData>\n> +       parseCameraConfigData(const YamlObject &cameraConfigData, PipelineHandler *pipe);\n> +\n> +       int parseSupportedFormats(const YamlObject &cameraConfigData,\n> +                                 std::vector<VirtualCameraData::Resolution> *resolutions);\n> +       int parseFrameGenerator(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 2b258492..3c9752c2 100644\n> --- a/src/libcamera/pipeline/virtual/virtual.cpp\n> +++ b/src/libcamera/pipeline/virtual/virtual.cpp\n> @@ -36,6 +36,9 @@\n>  #include \"libcamera/internal/formats.h\"\n>  #include \"libcamera/internal/framebuffer.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> @@ -54,6 +57,13 @@ uint64_t currentTimestamp()\n>  \n>  } /* namespace */\n>  \n> +template<class... Ts>\n> +struct overloaded : Ts... {\n> +       using Ts::operator()...;\n> +};\n> +template<class... Ts>\n> +overloaded(Ts...) -> overloaded<Ts...>;\n> +\n>  class VirtualCameraConfiguration : public CameraConfiguration\n>  {\n>  public:\n> @@ -94,7 +104,7 @@ private:\n>                 return static_cast<VirtualCameraData *>(camera->_d());\n>         }\n>  \n> -       void initFrameGenerator(Camera *camera);\n> +       bool initFrameGenerator(Camera *camera);\n>  \n>         DmaBufAllocator dmaBufAllocator_;\n>  };\n> @@ -104,15 +114,19 @@ bool PipelineHandlerVirtual::created_ = false;\n>  \n>  VirtualCameraData::VirtualCameraData(PipelineHandler *pipe,\n>                                      const std::vector<Resolution> &supportedResolutions)\n> -       : Camera::Private(pipe), supportedResolutions_(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 = 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> @@ -139,7 +153,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> @@ -154,7 +168,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> @@ -210,12 +224,12 @@ PipelineHandlerVirtual::generateConfiguration(Camera *camera,\n>  \n>                 std::map<PixelFormat, std::vector<SizeRange>> streamFormats;\n>                 PixelFormat pixelFormat = formats::NV12;\n> -               streamFormats[pixelFormat] = { { data->minResolutionSize_,\n> -                                                data->maxResolutionSize_ } };\n> +               streamFormats[pixelFormat] = { { data->config_.minResolutionSize,\n> +                                                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> @@ -232,6 +246,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> @@ -301,50 +316,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<VirtualCameraData::Resolution> 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<VirtualCameraData> data =\n> -               std::make_unique<VirtualCameraData>(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<Stream *> 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 = 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\nI'm surprised you don't just create the original Virtual0 with bars if there's no\nconfig file ...\n\n> +               return false;\n> +       }\n>  \n> -       initFrameGenerator(camera.get());\n> +       /* Configure and register cameras with configData */\n> +       for (auto &data : configData) {\n> +               std::set<Stream *> 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 = 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<DiagonalLinesGenerator>();\n> -               else\n> -                       streamConfig.frameGenerator = std::make_unique<ColorBarsGenerator>();\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<DiagonalLinesGenerator>();\n> +                                          else\n> +                                                  streamConfig.frameGenerator = std::make_unique<ColorBarsGenerator>();\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 9b79ac09..5ac0942b 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 <string>\n> +#include <variant>\n>  #include <vector>\n>  \n>  #include <libcamera/geometry.h>\n> @@ -15,10 +17,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<TestPattern, ImageFrames>;\n> +\n>  class VirtualCameraData : public Camera::Private\n>  {\n>  public:\n> @@ -32,17 +38,22 @@ public:\n>                 Stream stream;\n>                 std::unique_ptr<FrameGenerator> frameGenerator;\n>         };\n> +       /* The config file is parsed to the Configuration struct */\n> +       struct Configuration {\n> +               std::string id;\n> +               std::vector<Resolution> resolutions;\n> +               VirtualFrame frame;\n> +\n> +               Size maxResolutionSize;\n> +               Size minResolutionSize;\n> +       };\n>  \n>         VirtualCameraData(PipelineHandler *pipe,\n>                           const std::vector<Resolution> &supportedResolutions);\n>  \n>         ~VirtualCameraData() = default;\n>  \n> -       TestPattern testPattern_ = TestPattern::ColorBars;\n> -\n> -       const std::vector<Resolution> supportedResolutions_;\n> -       Size maxResolutionSize_;\n> -       Size minResolutionSize_;\n> +       Configuration config_;\n>  \n>         std::vector<StreamConfig> streamConfigs_;\n>  };\n> -- \n> 2.46.1.824.gd892dcdcdd-goog\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 C3CF7BD80A\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu,  3 Oct 2024 11:51:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7A8576351F;\n\tThu,  3 Oct 2024 13:51:07 +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 BC94862C92\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu,  3 Oct 2024 13:51:05 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D90A1593;\n\tThu,  3 Oct 2024 13:49:31 +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=\"NxDKZNAs\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1727956172;\n\tbh=F5BNeTqXLG3VN4h+qVVxh1oWgLvF2kFagCMq2Saj8FM=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=NxDKZNAsQqVPLDQU1jm0lLEoMVgOUIX/CXVdV2b6GiCisJv2QHKKuRvNz7raoTfGh\n\tG7cISU+co3ZQlp8CeomOY7c3cZmEvOzUaHaqf/lWDG3irBuw8eh0TL6mb4Apr4Tl4B\n\tBDn0J5pSJXCo+Dh8KjpycsqMJ1GB0dfbk5rVpBUU=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240930063342.3014837-7-chenghaoyang@google.com>","References":"<20240930063342.3014837-1-chenghaoyang@google.com>\n\t<20240930063342.3014837-7-chenghaoyang@google.com>","Subject":"Re: [PATCH v14 6/7] libcamera: virtual: Read config and register\n\tcameras based on the config","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"Harvey Yang <chenghaoyang@google.com>, Konami Shu <konamiz@google.com>, \n\tHarvey Yang <chenghaoyang@chromium.org>, Yunke Cao <yunkec@chromium.org>,\n\tTomasz Figa <tfiga@chromium.org>","To":"Harvey Yang <chenghaoyang@chromium.org>,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 03 Oct 2024 12:51:01 +0100","Message-ID":"<172795626198.1619946.1174039723197152006@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":31591,"web_url":"https://patchwork.libcamera.org/comment/31591/","msgid":"<CAEB1ahvgD2GB0u71ZNDxC+naWNPZat1LvpjXvJUSC2j0VXu-9A@mail.gmail.com>","date":"2024-10-04T10:00:36","subject":"Re: [PATCH v14 6/7] libcamera: virtual: Read config and register\n\tcameras based on the config","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi Kieran,\n\nOn Thu, Oct 3, 2024 at 7:51 PM Kieran Bingham\n<kieran.bingham@ideasonboard.com> wrote:\n>\n> Quoting Harvey Yang (2024-09-30 07:29:47)\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 <konamiz@google.com>\n> > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>\n>\n> CI complains that you don't have an explicit SoB tag here, but you're\n> listed as the Author.\n>\n> I think that's on another patch too... something to look for.\n\nMaybe it presumes the SoB should also be the committer...\n\n>\n>\n> > Co-developed-by: Yunke Cao <yunkec@chromium.org>\n> > Co-developed-by: Tomasz Figa <tfiga@chromium.org>\n> > ---\n> >  src/libcamera/pipeline/virtual/README.md      |  48 ++++\n> >  .../pipeline/virtual/data/virtual.yaml        |  36 +++\n> >  .../virtual/image_frame_generator.cpp         |  16 +-\n> >  .../pipeline/virtual/image_frame_generator.h  |   5 +-\n> >  src/libcamera/pipeline/virtual/meson.build    |   1 +\n> >  src/libcamera/pipeline/virtual/parser.cpp     | 260 ++++++++++++++++++\n> >  src/libcamera/pipeline/virtual/parser.h       |  39 +++\n> >  src/libcamera/pipeline/virtual/virtual.cpp    | 122 ++++----\n> >  src/libcamera/pipeline/virtual/virtual.h      |  21 +-\n> >  9 files changed, 483 insertions(+), 65 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..b51eb5ac\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> Where does it get installed ?\n>\n> Surely this is what users will modify ?\n\nYes, at least when running qcam/cam,\n`src/libcamera/pipeline/virtual/data/virtual.yaml` is available directly.\nThere are multiple paths available with `PipelineHandler::configurationFile`,\nwhich you might be more familiar with than I am :)\n\n>\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>\n> Maybe some level of wrapping would be nice throughout ?\n\nYou mean limiting the length of lines, right?\nUpdated.\n\n>\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> > +    - `parseFrameGenerator()`: 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/image_frame_generator.cpp b/src/libcamera/pipeline/virtual/image_frame_generator.cpp\n> > index a1915b24..26ee1a54 100644\n> > --- a/src/libcamera/pipeline/virtual/image_frame_generator.cpp\n> > +++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp\n> > @@ -40,15 +40,7 @@ ImageFrameGenerator::create(ImageFrames &imageFrames)\n> >          * For each file in the directory, load the image,\n> >          * convert it to NV12, and store the pointer.\n> >          */\n> > -       for (unsigned int i = 0; i < imageFrames.number.value_or(1); i++) {\n> > -               std::filesystem::path path;\n> > -               if (!imageFrames.number)\n> > -                       /* If the path is to an image */\n> > -                       path = imageFrames.path;\n> > -               else\n> > -                       /* If the path is to a directory */\n> > -                       path = imageFrames.path / (std::to_string(i) + \".jpg\");\n> > -\n> > +       for (std::filesystem::path path : imageFrames.files) {\n> >                 File file(path);\n> >                 if (!file.open(File::OpenModeFlag::ReadOnly)) {\n> >                         LOG(Virtual, Error) << \"Failed to open image file \" << file.fileName()\n> > @@ -88,6 +80,8 @@ ImageFrameGenerator::create(ImageFrames &imageFrames)\n> >                                         Size(width, height) });\n> >         }\n> >\n> > +       ASSERT(!imageFrameGenerator->imageFrameDatas_.empty());\n> > +\n> >         return imageFrameGenerator;\n> >  }\n> >\n> > @@ -99,7 +93,7 @@ void ImageFrameGenerator::configure(const Size &size)\n> >         frameCount_ = 0;\n> >         parameter_ = 0;\n> >\n> > -       for (unsigned int i = 0; i < imageFrames_->number.value_or(1); i++) {\n> > +       for (unsigned int i = 0; i < imageFrameDatas_.size(); i++) {\n> >                 /* Scale the imageFrameDatas_ to scaledY and scaledUV */\n> >                 unsigned int halfSizeWidth = (size.width + 1) / 2;\n> >                 unsigned int halfSizeHeight = (size.height + 1) / 2;\n> > @@ -135,7 +129,7 @@ int ImageFrameGenerator::generateFrame(const Size &size, const FrameBuffer *buff\n> >         auto planes = mappedFrameBuffer.planes();\n> >\n> >         /* Make sure the frameCount does not over the number of images */\n> > -       frameCount_ %= imageFrames_->number.value_or(1);\n> > +       frameCount_ %= imageFrameDatas_.size();\n> >\n> >         /* Write the scaledY and scaledUV to the mapped frame buffer */\n> >         libyuv::NV12Copy(scaledFrameDatas_[frameCount_].Y.get(), size.width,\n> > diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.h b/src/libcamera/pipeline/virtual/image_frame_generator.h\n> > index cd243816..6e83ab20 100644\n> > --- a/src/libcamera/pipeline/virtual/image_frame_generator.h\n> > +++ b/src/libcamera/pipeline/virtual/image_frame_generator.h\n> > @@ -10,9 +10,9 @@\n> >\n> >  #include <filesystem>\n> >  #include <memory>\n> > -#include <optional>\n> >  #include <stdint.h>\n> >  #include <sys/types.h>\n> > +#include <vector>\n> >\n> >  #include \"frame_generator.h\"\n> >\n> > @@ -20,8 +20,7 @@ namespace libcamera {\n> >\n> >  /* Frame configuration provided by the config file */\n> >  struct ImageFrames {\n> > -       std::filesystem::path path;\n> > -       std::optional<unsigned int> number;\n> > +       std::vector<std::filesystem::path> files;\n> >  };\n> >\n> >  class ImageFrameGenerator : public FrameGenerator\n> > diff --git a/src/libcamera/pipeline/virtual/meson.build b/src/libcamera/pipeline/virtual/meson.build\n> > index bb38c193..f3cb80b4 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>\n> config_parser.cpp ?\n\nSure, renamed the class as well.\n\n>\n>\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..cc782d12\n> > --- /dev/null\n> > +++ b/src/libcamera/pipeline/virtual/parser.cpp\n> > @@ -0,0 +1,260 @@\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 <string.h>\n> > +#include <utility>\n> > +\n> > +#include <libcamera/base/log.h>\n> > +\n> > +#include <libcamera/control_ids.h>\n> > +#include <libcamera/property_ids.h>\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<std::unique_ptr<VirtualCameraData>>\n> > +Parser::parseConfigFile(File &file, PipelineHandler *pipe)\n> > +{\n> > +       std::vector<std::unique_ptr<VirtualCameraData>> configurations;\n> > +\n> > +       std::unique_ptr<YamlObject> 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<VirtualCameraData> data =\n> > +                       parseCameraConfigData(cameraConfigData, pipe);\n> > +               /* Parse configData to data*/\n>\n> Watch those end of comments.\n\nDone\n\n>\n>\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>\n> Should it just be the min/max ?\n\nI don't know. The description in `control_ids_core.yaml` doesn't\nmention different resolutions. Is it how it should be?\n\n>\n> > +               controls[&controls::FrameDurationLimits] =\n> > +                       ControlInfo(int64_t(1000000 / data->config_.resolutions[0].frameRates[1]),\n>\n> c++ casts.\n>\n> Perhaps just calculate the frame duration min/max in their own int64_t\n> variable which probably then avoids a cast anyway.\n\nRight, made `frameRates`'s type as `std::vector<int64_t>`.\n\n>\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<VirtualCameraData>\n> > +Parser::parseCameraConfigData(const YamlObject &cameraConfigData,\n> > +                             PipelineHandler *pipe)\n> > +{\n> > +       std::vector<VirtualCameraData::Resolution> resolutions;\n> > +       if (parseSupportedFormats(cameraConfigData, &resolutions))\n> > +               return nullptr;\n> > +\n> > +       std::unique_ptr<VirtualCameraData> data =\n> > +               std::make_unique<VirtualCameraData>(pipe, resolutions);\n> > +\n> > +       if (parseFrameGenerator(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<VirtualCameraData::Resolution> *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<unsigned int>(1920);\n> > +                       unsigned int height = supportedResolution[\"height\"].get<unsigned int>(1080);\n> > +                       if (width == 0 || height == 0) {\n> > +                               LOG(Virtual, Error) << \"Invalid width or/and height\";\n>\n> Can this happen? You've added defaults of 1920, 1080.\n\nYes, if the user sets the value to be 0 directly.\n\n> What happens if only one default gets there?\n\nHmm, do we need to assume that to be unexpected?\n(I don't think our parser needs to be that careful :P)\n\n>\n>\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<int> frameRates;\n> > +                       if (supportedResolution.contains(\"frame_rates\")) {\n> > +                               auto frameRatesList =\n> > +                                       supportedResolution[\"frame_rates\"].getList<int>();\n> > +                               if (!frameRatesList || (frameRatesList->size() != 1 &&\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> Not worth anything specifically now - but I wonder would this work/be\n> clean:\n>\n>                                 /*\n>                                  * Push the min and max framerates. A\n>                                  * single rate is duplicated.\n>                                  */\n>                                 frameRates.push_back(frameRatesList.front());\n>                                 frameRates.push_back(frameRatesList.back());\n>\n\nYeah it's cleaner. Thanks!\n\n> And in fact *all* of that code above could possibly  be simplified with\n> (not complied/tested) code:\n>\n> #include <algorithm>\n>\n>\n>                         std::vector<int> frameRates;\n>                         auto frameRatesList = supportedResolution[\"frame_rates\"].getList<int>({30, 60});\n\nActually `getList` doesn't support a default value.\n\n>                         unsigned int minRate = *std::min_element(frameRatesList.begin(), frameRatesList.end());\n>                         unsigned int maxRate = *std::max_element(frameRatesList.begin(), frameRatesList.end());\n\nHmm, I'd prefer to keep it as is, that allows only two formats.\n\n>\n>                         frameRates.push_back(minRate);\n>                         frameRates.push_back(maxRate);\n>\n>                         resolutions->emplace_back(VirtualCameraData::Resolution{ Size{ width, height },\n>                                                                                  frameRates });\n>\n> I can't believe we don't have a class to make a range of a minimum and\n> maximum in libcamera yet ;-)\n>\n>\n>\n> > +                       } else {\n> > +                               frameRates.push_back(30);\n> > +                               frameRates.push_back(60);\n> > +                       }\n>\n> And those would get picked from the getList<int>({30, 60}); above then.\n>\n>\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::parseFrameGenerator(const YamlObject &cameraConfigData, VirtualCameraData *data)\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<std::string>(\"\");\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> > +                       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) {\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> > +       auto path = frames[\"path\"].get<std::string>();\n> > +\n> > +       if (!path) {\n> > +               LOG(Virtual, Error) << \"Test pattern or path should be specified.\";\n> > +               return -EINVAL;\n> > +       }\n> > +\n> > +       std::vector<std::filesystem::path> files;\n> > +\n> > +       switch (std::filesystem::symlink_status(*path).type()) {\n> > +       case std::filesystem::file_type::regular:\n> > +               files.push_back(*path);\n> > +               break;\n> > +\n> > +       case std::filesystem::file_type::directory:\n> > +               for (const auto &dentry : std::filesystem::directory_iterator{ *path }) {\n> > +                       if (dentry.is_regular_file())\n> > +                               files.push_back(dentry.path());\n> > +               }\n> > +\n> > +               std::sort(files.begin(), files.end(), [](const auto &a, const auto &b) {\n> > +                       return ::strverscmp(a.c_str(), b.c_str()) < 0;\n> > +               });\n> > +\n> > +               if (files.empty()) {\n> > +                       LOG(Virtual, Error) << \"Directory has no files: \" << *path;\n> > +                       return -EINVAL;\n> > +               }\n> > +               break;\n> > +\n> > +       default:\n> > +               LOG(Virtual, Error) << \"Frame: \" << *path << \" is not supported\";\n> > +               return -EINVAL;\n> > +       }\n> > +\n> > +       data->config_.frame = ImageFrames{ std::move(files) };\n> > +\n> > +       return 0;\n> > +}\n> > +\n> > +int Parser::parseLocation(const YamlObject &cameraConfigData, VirtualCameraData *data)\n> > +{\n> > +       std::string location = cameraConfigData[\"location\"].get<std::string>(\"\");\n>\n> Why not put 'front' in the .get<std::string>(\"front\"); ?\n\nYeah makes sense.\n\n>\n> > +\n> > +       /* Default value is properties::CameraLocationFront */\n> > +       if (location == \"front\" || location == \"\") {\n>\n> and simplify here ?\n\nDone\n\n>\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<std::string>(\"\");\n>\n> Again - put the 'Unknown' into the default, and remove the if below.\n\nDone\n\n>\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..6717be31\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 <memory>\n> > +#include <vector>\n> > +\n> > +#include <libcamera/base/file.h>\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<std::unique_ptr<VirtualCameraData>>\n> > +       parseConfigFile(File &file, PipelineHandler *pipe);\n> > +\n> > +private:\n> > +       std::unique_ptr<VirtualCameraData>\n> > +       parseCameraConfigData(const YamlObject &cameraConfigData, PipelineHandler *pipe);\n> > +\n> > +       int parseSupportedFormats(const YamlObject &cameraConfigData,\n> > +                                 std::vector<VirtualCameraData::Resolution> *resolutions);\n> > +       int parseFrameGenerator(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 2b258492..3c9752c2 100644\n> > --- a/src/libcamera/pipeline/virtual/virtual.cpp\n> > +++ b/src/libcamera/pipeline/virtual/virtual.cpp\n> > @@ -36,6 +36,9 @@\n> >  #include \"libcamera/internal/formats.h\"\n> >  #include \"libcamera/internal/framebuffer.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> > @@ -54,6 +57,13 @@ uint64_t currentTimestamp()\n> >\n> >  } /* namespace */\n> >\n> > +template<class... Ts>\n> > +struct overloaded : Ts... {\n> > +       using Ts::operator()...;\n> > +};\n> > +template<class... Ts>\n> > +overloaded(Ts...) -> overloaded<Ts...>;\n> > +\n> >  class VirtualCameraConfiguration : public CameraConfiguration\n> >  {\n> >  public:\n> > @@ -94,7 +104,7 @@ private:\n> >                 return static_cast<VirtualCameraData *>(camera->_d());\n> >         }\n> >\n> > -       void initFrameGenerator(Camera *camera);\n> > +       bool initFrameGenerator(Camera *camera);\n> >\n> >         DmaBufAllocator dmaBufAllocator_;\n> >  };\n> > @@ -104,15 +114,19 @@ bool PipelineHandlerVirtual::created_ = false;\n> >\n> >  VirtualCameraData::VirtualCameraData(PipelineHandler *pipe,\n> >                                      const std::vector<Resolution> &supportedResolutions)\n> > -       : Camera::Private(pipe), supportedResolutions_(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 = 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> > @@ -139,7 +153,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> > @@ -154,7 +168,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> > @@ -210,12 +224,12 @@ PipelineHandlerVirtual::generateConfiguration(Camera *camera,\n> >\n> >                 std::map<PixelFormat, std::vector<SizeRange>> streamFormats;\n> >                 PixelFormat pixelFormat = formats::NV12;\n> > -               streamFormats[pixelFormat] = { { data->minResolutionSize_,\n> > -                                                data->maxResolutionSize_ } };\n> > +               streamFormats[pixelFormat] = { { data->config_.minResolutionSize,\n> > +                                                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> > @@ -232,6 +246,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> > @@ -301,50 +316,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<VirtualCameraData::Resolution> 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<VirtualCameraData> data =\n> > -               std::make_unique<VirtualCameraData>(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<Stream *> 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 = 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>\n> I'm surprised you don't just create the original Virtual0 with bars if there's no\n> config file ...\n\nHmm... We already have a default config file. Do we really need to handle the\ncase that the developer decides not to provide any?\n\nBR,\nHarvey\n\n\n>\n> > +               return false;\n> > +       }\n> >\n> > -       initFrameGenerator(camera.get());\n> > +       /* Configure and register cameras with configData */\n> > +       for (auto &data : configData) {\n> > +               std::set<Stream *> 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 = 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<DiagonalLinesGenerator>();\n> > -               else\n> > -                       streamConfig.frameGenerator = std::make_unique<ColorBarsGenerator>();\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<DiagonalLinesGenerator>();\n> > +                                          else\n> > +                                                  streamConfig.frameGenerator = std::make_unique<ColorBarsGenerator>();\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 9b79ac09..5ac0942b 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 <string>\n> > +#include <variant>\n> >  #include <vector>\n> >\n> >  #include <libcamera/geometry.h>\n> > @@ -15,10 +17,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<TestPattern, ImageFrames>;\n> > +\n> >  class VirtualCameraData : public Camera::Private\n> >  {\n> >  public:\n> > @@ -32,17 +38,22 @@ public:\n> >                 Stream stream;\n> >                 std::unique_ptr<FrameGenerator> frameGenerator;\n> >         };\n> > +       /* The config file is parsed to the Configuration struct */\n> > +       struct Configuration {\n> > +               std::string id;\n> > +               std::vector<Resolution> resolutions;\n> > +               VirtualFrame frame;\n> > +\n> > +               Size maxResolutionSize;\n> > +               Size minResolutionSize;\n> > +       };\n> >\n> >         VirtualCameraData(PipelineHandler *pipe,\n> >                           const std::vector<Resolution> &supportedResolutions);\n> >\n> >         ~VirtualCameraData() = default;\n> >\n> > -       TestPattern testPattern_ = TestPattern::ColorBars;\n> > -\n> > -       const std::vector<Resolution> supportedResolutions_;\n> > -       Size maxResolutionSize_;\n> > -       Size minResolutionSize_;\n> > +       Configuration config_;\n> >\n> >         std::vector<StreamConfig> streamConfigs_;\n> >  };\n> > --\n> > 2.46.1.824.gd892dcdcdd-goog\n> >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 46791BD80A\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri,  4 Oct 2024 10:00:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DA7F06353A;\n\tFri,  4 Oct 2024 12:00:51 +0200 (CEST)","from mail-lj1-x231.google.com (mail-lj1-x231.google.com\n\t[IPv6:2a00:1450:4864:20::231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7755563530\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri,  4 Oct 2024 12:00:50 +0200 (CEST)","by mail-lj1-x231.google.com with SMTP id\n\t38308e7fff4ca-2fad8337aa4so24119361fa.0\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 04 Oct 2024 03:00:50 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"lJkrAzNk\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1728036050; x=1728640850;\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=FWFRdc8KR1Z8k/p9erg3V9oykLA+uxxUMqDWj77nBo4=;\n\tb=lJkrAzNk/vMcICyrSGJHIlJf7DCKMk8Azu1pWBS6FjuPKHgKNEhn+uc7ipXs0AeSB3\n\tMU8UU5zaL374oAC1qaSo7oP5u1U75HL1yWphjmxgLS4Qx/IMz8p8+BX/QH9xT8sziqxl\n\tzfKKfUfPIl6Us7YlrlAkSmxnTpCnpR3IReVZk=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1728036050; x=1728640850;\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=FWFRdc8KR1Z8k/p9erg3V9oykLA+uxxUMqDWj77nBo4=;\n\tb=Y6xySaYu03zK1KSZi1865jhIICceYxK9QTdLv3LyRVLuVixHtVREJae0jtlZLP7+W+\n\tcFWemzzEl4e/9996AjpPnQjoxITmXUtTiShQUP3Qr3SUftbDCXLs7mQVp0A068TD2i1i\n\tfmDoxA4hVyFZ7UmlMPEMQs5nNFkdYnxK+j7w8wd04AY0iQbDfd/r2YG7Qj1Q66H63HHd\n\t8XJdhDpf4S4G5dQnmT/S4khdRf7NUzjeTPVyZQNCS8bhhP+dCAuXf4A3lHaRwZ9EF7YV\n\tEARxfus2LK31FfxHf7vGzdHwCf+E+FxYmI2D7Ivnh4uqL6nUV5XtwTeETMawa33Sn7TO\n\tVIBw==","X-Gm-Message-State":"AOJu0Yyb5r0I6JhUfPEaEuYan/504WThHDHc4UzVc5VAmJx3E2WEt3M5\n\tulx9WbM2sIdWLCB7LupCtUsDtXuENtpkXmOiyLDo4ZD/g9PuMs8YidPfxFJPd0LOvRJmhRdmlzS\n\txih1YShPHXjeigO2+KWgcFM8PN6ewXq0junTK","X-Google-Smtp-Source":"AGHT+IHVbk8ZnGqFVs44ijjVUgTogOTDHUxCbbWKu42243Ss7RLhtcP9bjM4y7TC162vjbSy8ffm/VVgluO/3zAJshg=","X-Received":"by 2002:a2e:be9b:0:b0:2f6:62c3:4e0e with SMTP id\n\t38308e7fff4ca-2faf3bfee3bmr10970231fa.6.1728036048031;\n\tFri, 04 Oct 2024 03:00:48 -0700 (PDT)","MIME-Version":"1.0","References":"<20240930063342.3014837-1-chenghaoyang@google.com>\n\t<20240930063342.3014837-7-chenghaoyang@google.com>\n\t<172795626198.1619946.1174039723197152006@ping.linuxembedded.co.uk>","In-Reply-To":"<172795626198.1619946.1174039723197152006@ping.linuxembedded.co.uk>","From":"Cheng-Hao Yang <chenghaoyang@chromium.org>","Date":"Fri, 4 Oct 2024 18:00:36 +0800","Message-ID":"<CAEB1ahvgD2GB0u71ZNDxC+naWNPZat1LvpjXvJUSC2j0VXu-9A@mail.gmail.com>","Subject":"Re: [PATCH v14 6/7] libcamera: virtual: Read config and register\n\tcameras based on the config","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org,\n\tHarvey Yang <chenghaoyang@google.com>, \n\tKonami Shu <konamiz@google.com>, Yunke Cao <yunkec@chromium.org>, \n\tTomasz Figa <tfiga@chromium.org>","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":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":31600,"web_url":"https://patchwork.libcamera.org/comment/31600/","msgid":"<20241007185437.GI14766@pendragon.ideasonboard.com>","date":"2024-10-07T18:54:37","subject":"Re: [PATCH v14 6/7] libcamera: virtual: Read config and register\n\tcameras based on the config","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, Oct 04, 2024 at 06:00:36PM +0800, Cheng-Hao Yang wrote:\n> Hi Kieran,\n> \n> On Thu, Oct 3, 2024 at 7:51 PM Kieran Bingham\n> <kieran.bingham@ideasonboard.com> wrote:\n> >\n> > Quoting Harvey Yang (2024-09-30 07:29:47)\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 <konamiz@google.com>\n> > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>\n> >\n> > CI complains that you don't have an explicit SoB tag here, but you're\n> > listed as the Author.\n> >\n> > I think that's on another patch too... something to look for.\n> \n> Maybe it presumes the SoB should also be the committer...\n\nBoth the author and the committer of a commit need to have a SoB line.\nAnybody listed as a co-author with Co-developed-by also needs a SoB\nnice. Quoting the kernel documentation,\n\n - Co-developed-by: states that the patch was co-created by several developers;\n   it is a used to give attribution to co-authors (in addition to the author\n   attributed by the From: tag) when multiple people work on a single patch.\n   Every Co-developed-by: must be immediately followed by a Signed-off-by: of\n   the associated co-author.\n\n> > > Co-developed-by: Yunke Cao <yunkec@chromium.org>\n> > > Co-developed-by: Tomasz Figa <tfiga@chromium.org>\n> > > ---\n> > >  src/libcamera/pipeline/virtual/README.md      |  48 ++++\n> > >  .../pipeline/virtual/data/virtual.yaml        |  36 +++\n> > >  .../virtual/image_frame_generator.cpp         |  16 +-\n> > >  .../pipeline/virtual/image_frame_generator.h  |   5 +-\n> > >  src/libcamera/pipeline/virtual/meson.build    |   1 +\n> > >  src/libcamera/pipeline/virtual/parser.cpp     | 260 ++++++++++++++++++\n> > >  src/libcamera/pipeline/virtual/parser.h       |  39 +++\n> > >  src/libcamera/pipeline/virtual/virtual.cpp    | 122 ++++----\n> > >  src/libcamera/pipeline/virtual/virtual.h      |  21 +-\n> > >  9 files changed, 483 insertions(+), 65 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[snip]","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 65BB6C3263\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon,  7 Oct 2024 18:54:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6F4F76353A;\n\tMon,  7 Oct 2024 20:54:45 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 749026351F\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon,  7 Oct 2024 20:54:43 +0200 (CEST)","from pendragon.ideasonboard.com (unknown [132.205.230.14])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BF0F62EC;\n\tMon,  7 Oct 2024 20:53:06 +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=\"N6zoXUMG\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1728327187;\n\tbh=/G60r0GIkHmP+Nj63SxmROhjHdDlOVDm3/NzG4c0woM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=N6zoXUMGeq+dImzw20uNwpas3wDwOwqh57clEENcLG6P4P3yhDB4gqqyBZmtHxZhH\n\tF0FFgjwscJvVJfEfJKyCxbQsQTIJ1Qga4fYAExsrY4D9L7Lvg/clqwIMCOykJ0ubDn\n\taZNlpxv/IssJfOKkEYmMRtb5ovI3cVRv6dfX59YQ=","Date":"Mon, 7 Oct 2024 21:54:37 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Cheng-Hao Yang <chenghaoyang@chromium.org>","Cc":"Kieran Bingham <kieran.bingham@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org,\n\tHarvey Yang <chenghaoyang@google.com>, \n\tKonami Shu <konamiz@google.com>, Yunke Cao <yunkec@chromium.org>,\n\tTomasz Figa <tfiga@chromium.org>","Subject":"Re: [PATCH v14 6/7] libcamera: virtual: Read config and register\n\tcameras based on the config","Message-ID":"<20241007185437.GI14766@pendragon.ideasonboard.com>","References":"<20240930063342.3014837-1-chenghaoyang@google.com>\n\t<20240930063342.3014837-7-chenghaoyang@google.com>\n\t<172795626198.1619946.1174039723197152006@ping.linuxembedded.co.uk>\n\t<CAEB1ahvgD2GB0u71ZNDxC+naWNPZat1LvpjXvJUSC2j0VXu-9A@mail.gmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<CAEB1ahvgD2GB0u71ZNDxC+naWNPZat1LvpjXvJUSC2j0VXu-9A@mail.gmail.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":31611,"web_url":"https://patchwork.libcamera.org/comment/31611/","msgid":"<CAEB1ahsfW+e-5FoPM84s8CM8JNfcphRJ9djeSzLRwkGY4_YFRQ@mail.gmail.com>","date":"2024-10-08T08:30:31","subject":"Re: [PATCH v14 6/7] libcamera: virtual: Read config and register\n\tcameras based on the config","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi Laurent,\n\nOn Tue, Oct 8, 2024 at 2:54 AM Laurent Pinchart\n<laurent.pinchart@ideasonboard.com> wrote:\n>\n> On Fri, Oct 04, 2024 at 06:00:36PM +0800, Cheng-Hao Yang wrote:\n> > Hi Kieran,\n> >\n> > On Thu, Oct 3, 2024 at 7:51 PM Kieran Bingham\n> > <kieran.bingham@ideasonboard.com> wrote:\n> > >\n> > > Quoting Harvey Yang (2024-09-30 07:29:47)\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 <konamiz@google.com>\n> > > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>\n> > >\n> > > CI complains that you don't have an explicit SoB tag here, but you're\n> > > listed as the Author.\n> > >\n> > > I think that's on another patch too... something to look for.\n> >\n> > Maybe it presumes the SoB should also be the committer...\n>\n> Both the author and the committer of a commit need to have a SoB line.\n> Anybody listed as a co-author with Co-developed-by also needs a SoB\n> nice. Quoting the kernel documentation,\n>\n>  - Co-developed-by: states that the patch was co-created by several developers;\n>    it is a used to give attribution to co-authors (in addition to the author\n>    attributed by the From: tag) when multiple people work on a single patch.\n>    Every Co-developed-by: must be immediately followed by a Signed-off-by: of\n>    the associated co-author.\n\nGot it. Thanks!\n\nLinter passes: https://gitlab.freedesktop.org/chenghaoyang/libcamera/-/pipelines/1285528\n\nI'll upload another version with other comments fixed and\nreview tags to prevent the spam.\n\nBR,\nHarvey\n\n>\n> > > > Co-developed-by: Yunke Cao <yunkec@chromium.org>\n> > > > Co-developed-by: Tomasz Figa <tfiga@chromium.org>\n> > > > ---\n> > > >  src/libcamera/pipeline/virtual/README.md      |  48 ++++\n> > > >  .../pipeline/virtual/data/virtual.yaml        |  36 +++\n> > > >  .../virtual/image_frame_generator.cpp         |  16 +-\n> > > >  .../pipeline/virtual/image_frame_generator.h  |   5 +-\n> > > >  src/libcamera/pipeline/virtual/meson.build    |   1 +\n> > > >  src/libcamera/pipeline/virtual/parser.cpp     | 260 ++++++++++++++++++\n> > > >  src/libcamera/pipeline/virtual/parser.h       |  39 +++\n> > > >  src/libcamera/pipeline/virtual/virtual.cpp    | 122 ++++----\n> > > >  src/libcamera/pipeline/virtual/virtual.h      |  21 +-\n> > > >  9 files changed, 483 insertions(+), 65 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> [snip]\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 67845BE080\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue,  8 Oct 2024 08:30:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 45CE76353A;\n\tTue,  8 Oct 2024 10:30:45 +0200 (CEST)","from mail-lj1-x22a.google.com (mail-lj1-x22a.google.com\n\t[IPv6:2a00:1450:4864:20::22a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5FA6B62C8E\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue,  8 Oct 2024 10:30:43 +0200 (CEST)","by mail-lj1-x22a.google.com with SMTP id\n\t38308e7fff4ca-2faccada15bso50027681fa.3\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 08 Oct 2024 01:30:43 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"Xf5zl3s2\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1728376242; x=1728981042;\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=DZyDA9j/VT12ba58328Qtk/JIUiZxuqfdXqqmZlhCTI=;\n\tb=Xf5zl3s26dVoWi5NwIXE53CgiTC51AMPlullW7fjaUo/WrpfodFPkeMPrkq3fBZ3KR\n\tEaPckl/uQ/QxqtSgmcoPfoaAnho5ChNnkcsIHMYKPZaycoHwGHUHhG6qAHw5/tdUXJtj\n\tZMiene11DM5FckvSkRwpUvXuOwo51eNmwtAjg=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1728376242; x=1728981042;\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=DZyDA9j/VT12ba58328Qtk/JIUiZxuqfdXqqmZlhCTI=;\n\tb=EnLqXal2f/fyoFxJ+TWusDjarOopytru9ZB3V061G5HKnHFThQAwhn2jVyd/KlbOip\n\tfuGSz/pXNPONtlkPlYoWmNRU2uxQipMQJOfU4yjliWOqejtd3xY9+2X45Yxm9dlopbRt\n\tcul2VJrlcKdooUqcXRXdTdxQPJCFBKCrvafPzvaWBL7A91wBvYE2cc3UkOJS2dmiSkr4\n\tOxf2VkjGPyTVSvCRpmO2EdjCA7Sq++8yC00iW1J7ULsX2d7oyWYJAHHDUAoV4Mnosk0B\n\t0cCsJrHAl/RuBaU46HEaC6LbgbI1Xhpg299WcWjLziMrH+RJMU1WPLAO4Sw8SdfPvaXY\n\tK1CA==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCXLeinzuUI+UW/e6WWJPkvXMzh+htirFWKX9kHNud4W/2+++sVBPdNUN/cv10hqI29k7PCFQVfevCHui7XDfLk=@lists.libcamera.org","X-Gm-Message-State":"AOJu0YzYHt0bzi6iUBd7K09roBDIMzNxb1P1agafKBSyp9wEiiclnioG\n\tA5K8pLtbmJLEOc2iKS5tAckRVkvZdU21TWMgOj2s/n8tXVXTK7k8IQ7oPPmQU0BRu9SbouAjAM3\n\tNMuWUa53FH/83PS77Jpn/Y/js20qDOrbWY7BK","X-Google-Smtp-Source":"AGHT+IEDEnAHqAC+b2P90MTiirNjj3MpSn9cRUyhSDyLTw2Q33XE7vTHMujEup3EwUuEw/mYMuIP///Hh4SYIJk93Ig=","X-Received":"by 2002:a2e:b8c7:0:b0:2fa:c0fc:e3d8 with SMTP id\n\t38308e7fff4ca-2faf3d8a8f4mr60932611fa.38.1728376242428;\n\tTue, 08 Oct 2024 01:30:42 -0700 (PDT)","MIME-Version":"1.0","References":"<20240930063342.3014837-1-chenghaoyang@google.com>\n\t<20240930063342.3014837-7-chenghaoyang@google.com>\n\t<172795626198.1619946.1174039723197152006@ping.linuxembedded.co.uk>\n\t<CAEB1ahvgD2GB0u71ZNDxC+naWNPZat1LvpjXvJUSC2j0VXu-9A@mail.gmail.com>\n\t<20241007185437.GI14766@pendragon.ideasonboard.com>","In-Reply-To":"<20241007185437.GI14766@pendragon.ideasonboard.com>","From":"Cheng-Hao Yang <chenghaoyang@chromium.org>","Date":"Tue, 8 Oct 2024 16:30:31 +0800","Message-ID":"<CAEB1ahsfW+e-5FoPM84s8CM8JNfcphRJ9djeSzLRwkGY4_YFRQ@mail.gmail.com>","Subject":"Re: [PATCH v14 6/7] libcamera: virtual: Read config and register\n\tcameras based on the config","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"Kieran Bingham <kieran.bingham@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org, \n\tHarvey Yang <chenghaoyang@google.com>, Konami Shu <konamiz@google.com>,\n\tYunke Cao <yunkec@chromium.org>, Tomasz Figa <tfiga@chromium.org>","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":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]