[{"id":31122,"web_url":"https://patchwork.libcamera.org/comment/31122/","msgid":"<3yzin5r3r3nmawjocyrakt3aq6xsys2fnsjwobddfid7yxwjxk@gc6abloh6zas>","date":"2024-09-09T10:16:59","subject":"Re: [PATCH v11 6/7] libcamera: virtual: Add ImageFrameGenerator","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi\n\nOn Sat, Sep 07, 2024 at 02:28:31PM GMT, Harvey Yang wrote:\n> From: Konami Shu \n>\n> Besides TestPatternGenerator, this patch adds ImageFrameGenerator that\n> loads real images (jpg / jpeg for now) as the source and generates\n> scaled frames.\n>\n> Signed-off-by: Konami Shu \n> Co-developed-by: Harvey Yang \n> Co-developed-by: Yunke Cao \n> Co-developed-by: Tomasz Figa \n> ---\n> src/libcamera/pipeline/virtual/README.md | 9 +-\n> .../virtual/image_frame_generator.cpp | 178 ++++++++++++++++++\n> .../pipeline/virtual/image_frame_generator.h | 54 ++++++\n> src/libcamera/pipeline/virtual/meson.build | 4 +\n> src/libcamera/pipeline/virtual/parser.cpp | 76 +++++++-\n> src/libcamera/pipeline/virtual/parser.h | 2 +\n> src/libcamera/pipeline/virtual/utils.h | 17 ++\n> src/libcamera/pipeline/virtual/virtual.cpp | 60 ++++--\n> src/libcamera/pipeline/virtual/virtual.h | 24 ++-\n> 9 files changed, 389 insertions(+), 35 deletions(-)\n> create mode 100644 src/libcamera/pipeline/virtual/image_frame_generator.cpp\n> create mode 100644 src/libcamera/pipeline/virtual/image_frame_generator.h\n> create mode 100644 src/libcamera/pipeline/virtual/utils.h\n>\n> diff --git a/src/libcamera/pipeline/virtual/README.md b/src/libcamera/pipeline/virtual/README.md\n> index ef80bb48..18c8341b 100644\n> --- a/src/libcamera/pipeline/virtual/README.md\n> +++ b/src/libcamera/pipeline/virtual/README.md\n> @@ -16,7 +16,13 @@ Each camera block is a dictionary, containing the following keys:\n> - `width` (`unsigned int`, default=1920): Width of the window resolution. This needs to be even.\n> - `height` (`unsigned int`, default=1080): Height of the window resolution.\n> - `frame_rates` (list of `int`, default=`[30,60]` ): Range of the frame rate (per second). If the list contains one value, it's the lower bound and the upper bound. If the list contains two values, the first is the lower bound and the second is the upper bound. No other number of values is allowed.\n> -- `test_pattern` (`string`): Which test pattern to use as frames. The options are \"bars\", \"lines\".\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> + - `scale_mode`(`string`, default=\"fill\"): Scale mode when the frames are images. The only scale mode supported now is \"fill\". This does not affect the scale mode for now.\n\nWouldn't it be more logical to add the frame generator support before\nthe config file ? Otherwise you are adding pieces here to what has\nbeen added just one patch ago\n\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> @@ -37,6 +43,7 @@ This is the procedure of the Parser class:\n> 3. Parse each property and register the data.\n> - `parseSupportedFormats()`: Parses `supported_formats` in the config, which contains resolutions and frame rates.\n> - `parseTestPattern()`: Parses `test_pattern` in the config.\n> + - `parseFrame()`: Parses `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> diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.cpp b/src/libcamera/pipeline/virtual/image_frame_generator.cpp\n> new file mode 100644\n> index 00000000..db3efe15\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp\n> @@ -0,0 +1,178 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Google Inc.\n> + *\n> + * image_frame_generator.cpp - Derived class of FrameGenerator for\n> + * generating frames from images\n> + */\n> +\n> +#include \"image_frame_generator.h\"\n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \"libcamera/internal/mapped_framebuffer.h\"\n> +\n> +#include \"libyuv/convert.h\"\n> +#include \"libyuv/scale.h\"\n> +\n> +namespace libcamera {\n> +\n> +LOG_DECLARE_CATEGORY(Virtual)\n> +\n> +/*\n> + * Factory function to create an ImageFrameGenerator object.\n> + * Read the images and convert them to buffers in NV12 format.\n> + * Store the pointers to the buffers to a list (imageFrameDatas)\n> + */\n> +std::unique_ptr\n> +ImageFrameGenerator::create(ImageFrames &imageFrames)\n> +{\n> +\tstd::unique_ptr imageFrameGenerator =\n> +\t\tstd::make_unique();\n> +\timageFrameGenerator->imageFrames_ = &imageFrames;\n> +\n> +\t/*\n> + * For each file in the directory, load the image,\n> + * convert it to NV12, and store the pointer.\n\nweird indent\n\n> +\t */\n> +\tfor (unsigned int i = 0; i < imageFrames.number.value_or(1); i++) {\n> +\t\tstd::filesystem::path path;\n> +\t\tif (!imageFrames.number)\n> +\t\t\t/* If the path is to an image */\n> +\t\t\tpath = imageFrames.path;\n> +\t\telse\n> +\t\t\t/* If the path is to a directory */\n> +\t\t\tpath = imageFrames.path / (std::to_string(i) + \".jpg\");\n> +\n> +\t\tFile file(path);\n> +\t\tif (!file.open(File::OpenModeFlag::ReadOnly)) {\n> +\t\t\tLOG(Virtual, Error) << \"Failed to open image file \" << file.fileName()\n> +\t\t\t\t\t << \": \" << strerror(file.error());\n> +\t\t\treturn nullptr;\n> +\t\t}\n> +\n> +\t\t/* Read the image file to data */\n> +\t\tauto fileSize = file.size();\n> +\t\tauto buffer = std::make_unique(file.size());\n> +\t\tif (file.read({ buffer.get(), static_cast(fileSize) }) != fileSize) {\n> +\t\t\tLOG(Virtual, Error) << \"Failed to read file \" << file.fileName()\n> +\t\t\t\t\t << \": \" << strerror(file.error());\n> +\t\t\treturn nullptr;\n> +\t\t}\n> +\n> +\t\t/* Get the width and height of the image */\n> +\t\tint width, height;\n> +\t\tif (libyuv::MJPGSize(buffer.get(), fileSize, &width, &height)) {\n> +\t\t\tLOG(Virtual, Error) << \"Failed to get the size of the image file: \"\n> +\t\t\t\t\t << file.fileName();\n> +\t\t\treturn nullptr;\n> +\t\t}\n> +\n> +\t\t/* Convert to NV12 and write the data to tmpY and tmpUV */\n> +\t\tunsigned int halfWidth = (width + 1) / 2;\n> +\t\tunsigned int halfHeight = (height + 1) / 2;\n\nDo you need this for rounding ?\n\n> +\t\tstd::unique_ptr dstY =\n> +\t\t\tstd::make_unique(width * height);\n> +\t\tstd::unique_ptr dstUV =\n> +\t\t\tstd::make_unique(halfWidth * halfHeight * 2);\n\notherwise using width * height / 2 would do\n\n> +\t\tint ret = libyuv::MJPGToNV12(buffer.get(), fileSize,\n> +\t\t\t\t\t dstY.get(), width, dstUV.get(),\n> +\t\t\t\t\t width, width, height, width, height);\n> +\t\tif (ret != 0)\n> +\t\t\tLOG(Virtual, Error) << \"MJPGToNV12() failed with \" << ret;\n> +\n> +\t\timageFrameGenerator->imageFrameDatas_.emplace_back(\n> +\t\t\tImageFrameData{ std::move(dstY), std::move(dstUV),\n> +\t\t\t\t\tSize(width, height) });\n> +\t}\n> +\n> +\treturn imageFrameGenerator;\n> +}\n> +\n> +/* Scale the buffers for image frames. */\n> +void ImageFrameGenerator::configure(const Size &size)\n> +{\n> +\t/* Reset the source images to prevent multiple configuration calls */\n> +\tscaledFrameDatas_.clear();\n> +\tframeCount_ = 0;\n> +\tparameter_ = 0;\n> +\n> +\tfor (unsigned int i = 0; i < imageFrames_->number.value_or(1); i++) {\n> +\t\t/* Scale the imageFrameDatas_ to scaledY and scaledUV */\n> +\t\tunsigned int halfSizeWidth = (size.width + 1) / 2;\n> +\t\tunsigned int halfSizeHeight = (size.height + 1) / 2;\n> +\t\tstd::unique_ptr scaledY =\n> +\t\t\tstd::make_unique(size.width * size.height);\n> +\t\tstd::unique_ptr scaledUV =\n> +\t\t\tstd::make_unique(halfSizeWidth * halfSizeHeight * 2);\n> +\t\tauto &src = imageFrameDatas_[i];\n> +\n> +\t\t/*\n> +\t\t * \\todo Some platforms might enforce stride due to GPU, like\n> +\t\t * ChromeOS ciri (64). The weight needs to be a multiple of\n> +\t\t * the stride to work properly for now.\n> +\t\t */\n> +\t\tlibyuv::NV12Scale(src.Y.get(), src.size.width,\n> +\t\t\t\t src.UV.get(), src.size.width,\n> +\t\t\t\t src.size.width, src.size.height,\n> +\t\t\t\t scaledY.get(), size.width, scaledUV.get(), size.width,\n> +\t\t\t\t size.width, size.height, libyuv::FilterMode::kFilterBilinear);\n> +\n> +\t\tscaledFrameDatas_.emplace_back(\n> +\t\t\tImageFrameData{ std::move(scaledY), std::move(scaledUV), size });\n> +\t}\n> +}\n> +\n> +void ImageFrameGenerator::generateFrame(const Size &size, const FrameBuffer *buffer)\n> +{\n> +\t/* Don't do anything when the list of buffers is empty*/\n> +\tASSERT(!scaledFrameDatas_.empty());\n> +\n> +\tMappedFrameBuffer mappedFrameBuffer(buffer, MappedFrameBuffer::MapFlag::Write);\n> +\n> +\tauto planes = mappedFrameBuffer.planes();\n> +\n> +\t/* Make sure the frameCount does not over the number of images */\n> +\tframeCount_ %= imageFrames_->number.value_or(1);\n> +\n> +\t/* Write the scaledY and scaledUV to the mapped frame buffer */\n> +\tlibyuv::NV12Copy(scaledFrameDatas_[frameCount_].Y.get(), size.width,\n> +\t\t\t scaledFrameDatas_[frameCount_].UV.get(), size.width, planes[0].begin(),\n> +\t\t\t size.width, planes[1].begin(), size.width,\n> +\t\t\t size.width, size.height);\n> +\n> +\t/* proceed an image every 4 frames */\n> +\t/* \\todo read the parameter_ from the configuration file? */\n> +\tparameter_++;\n> +\tif (parameter_ % 4 == 0)\n\nMake this a constexpr if not configurable\n\n> +\t\tframeCount_++;\n> +}\n> +\n> +/**\n\nWe don't doxygen the pipelines, but doesn't hurt to have comments.\nMaybe remove the /**\n\n\n> + * \\var ImageFrameGenerator::imageFrameDatas_\n> + * \\brief List of pointers to the not scaled image buffers\n> + */\n> +\n> +/**\n> + * \\var ImageFrameGenerator::scaledFrameDatas_\n> + * \\brief List of pointers to the scaled image buffers\n> + */\n> +\n> +/**\n> + * \\var ImageFrameGenerator::imageFrames_\n> + * \\brief Pointer to the imageFrames_ in VirtualCameraData\n> + */\n> +\n> +/**\n> + * \\var ImageFrameGenerator::parameter_\n> + * \\brief Speed parameter. Change to the next image every parameter_ frames\n> + */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.h b/src/libcamera/pipeline/virtual/image_frame_generator.h\n> new file mode 100644\n> index 00000000..4ad8aad2\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/virtual/image_frame_generator.h\n> @@ -0,0 +1,54 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Google Inc.\n> + *\n> + * image_frame_generator.h - Derived class of FrameGenerator for\n> + * generating frames from images\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \"frame_generator.h\"\n> +\n> +namespace libcamera {\n> +\n> +enum class ScaleMode : char {\n> +\tFill = 0,\n> +};\n\nI know you want more scale modes, but as long as you only support one\nthere's no need for a type\n\n> +\n> +/* Frame configuration provided by the config file */\n> +struct ImageFrames {\n> +\tstd::filesystem::path path;\n> +\tScaleMode scaleMode;\n\nAs this can only be \"Fill\", nothing else is valid atm\n\n> +\tstd::optional number;\n> +};\n> +\n> +class ImageFrameGenerator : public FrameGenerator\n> +{\n> +public:\n> +\tstatic std::unique_ptr create(ImageFrames &imageFrames);\n> +\n> +private:\n> +\tstruct ImageFrameData {\n> +\t\tstd::unique_ptr Y;\n> +\t\tstd::unique_ptr UV;\n> +\t\tSize size;\n> +\t};\n> +\n> +\tvoid configure(const Size &size) override;\n> +\tvoid generateFrame(const Size &size, const FrameBuffer *buffer) override;\n> +\n> +\tstd::vector imageFrameDatas_;\n> +\tstd::vector scaledFrameDatas_;\n> +\tImageFrames *imageFrames_;\n> +\tunsigned int frameCount_;\n> +\tunsigned int parameter_;\n> +};\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/virtual/meson.build b/src/libcamera/pipeline/virtual/meson.build\n> index d72ac5be..395919b3 100644\n> --- a/src/libcamera/pipeline/virtual/meson.build\n> +++ b/src/libcamera/pipeline/virtual/meson.build\n> @@ -1,9 +1,13 @@\n> # SPDX-License-Identifier: CC0-1.0\n>\n> libcamera_internal_sources += files([\n> + 'image_frame_generator.cpp',\n> 'parser.cpp',\n> 'test_pattern_generator.cpp',\n> 'virtual.cpp',\n> ])\n>\n> +libjpeg = dependency('libjpeg', required : false)\n> +\n> libcamera_deps += [libyuv_dep]\n> +libcamera_deps += [libjpeg]\n> diff --git a/src/libcamera/pipeline/virtual/parser.cpp b/src/libcamera/pipeline/virtual/parser.cpp\n> index d861a52a..5076e71c 100644\n> --- a/src/libcamera/pipeline/virtual/parser.cpp\n> +++ b/src/libcamera/pipeline/virtual/parser.cpp\n> @@ -52,12 +52,12 @@ Parser::parseConfigFile(File &file, PipelineHandler *pipe)\n> \t\t\tcontinue;\n> \t\t}\n>\n> -\t\tdata->id_ = cameraId;\n> +\t\tdata->config_.id = cameraId;\n> \t\tControlInfoMap::Map controls;\n> \t\t/* todo: Check which resolution's frame rate to be reported */\n> \t\tcontrols[&controls::FrameDurationLimits] =\n> -\t\t\tControlInfo(int64_t(1000000 / data->supportedResolutions_[0].frameRates[1]),\n> -\t\t\t\t int64_t(1000000 / data->supportedResolutions_[0].frameRates[0]));\n> +\t\t\tControlInfo(int64_t(1000000 / data->config_.resolutions[0].frameRates[1]),\n> +\t\t\t\t int64_t(1000000 / data->config_.resolutions[0].frameRates[0]));\n> \t\tdata->controlInfo_ = ControlInfoMap(std::move(controls), controls::controls);\n> \t\tconfigurations.push_back(std::move(data));\n> \t}\n> @@ -75,7 +75,8 @@ Parser::parseCameraConfigData(const YamlObject &cameraConfigData,\n> \tstd::unique_ptr data =\n> \t\tstd::make_unique(pipe, resolutions);\n>\n> -\tif (parseTestPattern(cameraConfigData, data.get()))\n> +\tif (parseTestPattern(cameraConfigData, data.get()) &&\n> +\t parseFrame(cameraConfigData, data.get()))\n\nThis is fragile and only works because if parseTestPattern() returns 0\nthen parseFrame() is never called\n\n> \t\treturn nullptr;\n>\n> \tif (parseLocation(cameraConfigData, data.get()))\n> @@ -148,16 +149,75 @@ int Parser::parseTestPattern(const YamlObject &cameraConfigData, VirtualCameraDa\n> {\n> \tstd::string testPattern = cameraConfigData[\"test_pattern\"].get(\"\");\n>\n> -\t/* Default value is \"bars\" */\n> \tif (testPattern == \"bars\") {\n> -\t\tdata->testPattern_ = TestPattern::ColorBars;\n> +\t\tdata->config_.frame = TestPattern::ColorBars;\n> \t} else if (testPattern == \"lines\") {\n> -\t\tdata->testPattern_ = TestPattern::DiagonalLines;\n> +\t\tdata->config_.frame = TestPattern::DiagonalLines;\n> \t} else {\n> -\t\tLOG(Virtual, Error) << \"Test pattern: \" << testPattern\n> +\t\tLOG(Virtual, Debug) << \"Test pattern: \" << testPattern\n> \t\t\t\t << \"is not supported\";\n> \t\treturn -EINVAL;\n> \t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +int Parser::parseFrame(const YamlObject &cameraConfigData, VirtualCameraData *data)\n\nYou can unify these two functions.\n\nGet \"testPattern\", if valid make sure it's only either \"bars\" or\n\"lines\" and fail if it is specified but has an unsupported value.\n\nThen check for \"frames\". If both \"frames\" and \"testPattern\" are\nspecified, it's an error. Then parse \"frames\" like you do here.\n\n> +{\n> +\tconst YamlObject &frames = cameraConfigData[\"frames\"];\n> +\n> +\t/* When there is no frames provided in the config file, use color bar test pattern */\n> +\tif (frames.size() == 0) {\n> +\t\tdata->config_.frame = TestPattern::ColorBars;\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tif (!frames.isDictionary()) {\n> +\t\tLOG(Virtual, Error) << \"'frames' is not a dictionary.\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tstd::string path = frames[\"path\"].get(\"\");\n> +\n> +\tScaleMode scaleMode;\n> +\tif (auto ext = std::filesystem::path(path).extension();\n> +\t ext == \".jpg\" || ext == \".jpeg\") {\n> +\t\tif (parseScaleMode(frames, &scaleMode))\n> +\t\t\treturn -EINVAL;\n> +\t\tdata->config_.frame = ImageFrames{ path, scaleMode, std::nullopt };\n> +\t} else if (std::filesystem::is_directory(std::filesystem::symlink_status(path))) {\n> +\t\tif (parseScaleMode(frames, &scaleMode))\n> +\t\t\treturn -EINVAL;\n\nCould you parse scale mode before checking the file extensions ?\n\n> +\n> +\t\tusing std::filesystem::directory_iterator;\n> +\t\tunsigned int numOfFiles = std::distance(directory_iterator(path), directory_iterator{});\n> +\t\tif (numOfFiles == 0) {\n> +\t\t\tLOG(Virtual, Error) << \"Empty directory\";\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\t\tdata->config_.frame = ImageFrames{ path, scaleMode, numOfFiles };\n> +\t} else {\n> +\t\tLOG(Virtual, Error) << \"Frame: \" << path << \" is not supported\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +int Parser::parseScaleMode(\n> +\tconst YamlObject &framesConfigData, ScaleMode *scaleMode)\n\nweird indent\n\n> +{\n> +\tstd::string mode = framesConfigData[\"scale_mode\"].get(\"\");\n> +\n> +\t/* Default value is fill */\n> +\tif (mode == \"fill\" || mode == \"\") {\n> +\t\t*scaleMode = ScaleMode::Fill;\n\nYou don't need a type as suggested above, you can either assume \"Fill\"\nor fail during parsing.\n\n> +\t} else {\n> +\t\tLOG(Virtual, Error) << \"scaleMode: \" << mode\n> +\t\t\t\t << \" is not supported\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> \treturn 0;\n> }\n>\n> diff --git a/src/libcamera/pipeline/virtual/parser.h b/src/libcamera/pipeline/virtual/parser.h\n> index 09c3c56b..f65616e3 100644\n> --- a/src/libcamera/pipeline/virtual/parser.h\n> +++ b/src/libcamera/pipeline/virtual/parser.h\n> @@ -35,8 +35,10 @@ private:\n> \tint parseSupportedFormats(const YamlObject &cameraConfigData,\n> \t\t\t\t std::vector *resolutions);\n> \tint parseTestPattern(const YamlObject &cameraConfigData, VirtualCameraData *data);\n> +\tint parseFrame(const YamlObject &cameraConfigData, VirtualCameraData *data);\n> \tint parseLocation(const YamlObject &cameraConfigData, VirtualCameraData *data);\n> \tint parseModel(const YamlObject &cameraConfigData, VirtualCameraData *data);\n> +\tint parseScaleMode(const YamlObject &framesConfigData, ScaleMode *scaleMode);\n> };\n>\n> } /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/virtual/utils.h b/src/libcamera/pipeline/virtual/utils.h\n> new file mode 100644\n> index 00000000..43a14d4b\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/virtual/utils.h\n> @@ -0,0 +1,17 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Google Inc.\n> + *\n> + * utils.h - Utility types for Virtual Pipeline Handler\n> + */\n> +\n> +namespace libcamera {\n> +\n> +template\n> +struct overloaded : Ts... {\n> +\tusing Ts::operator()...;\n> +};\n> +template\n> +overloaded(Ts...) -> overloaded;\n> +\n\nDoes using a standard construct like std::visit a custom definition\nlike this one ? Also, this header is included form a single cpp file,\nmove its content there if you can.\n\n\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp\n> index 55bc30df..98aed412 100644\n> --- a/src/libcamera/pipeline/virtual/virtual.cpp\n> +++ b/src/libcamera/pipeline/virtual/virtual.cpp\n> @@ -27,6 +27,7 @@\n> #include \"libcamera/internal/yaml_parser.h\"\n>\n> #include \"parser.h\"\n> +#include \"utils.h\"\n>\n> namespace libcamera {\n>\n> @@ -49,17 +50,18 @@ uint64_t currentTimestamp()\n>\n> VirtualCameraData::VirtualCameraData(PipelineHandler *pipe,\n> \t\t\t\t std::vector supportedResolutions)\n> -\t: Camera::Private(pipe), supportedResolutions_(std::move(supportedResolutions))\n> +\t: Camera::Private(pipe)\n> {\n> -\tfor (const auto &resolution : supportedResolutions_) {\n> -\t\tif (minResolutionSize_.isNull() || minResolutionSize_ > resolution.size)\n> -\t\t\tminResolutionSize_ = resolution.size;\n> +\tconfig_.resolutions = std::move(supportedResolutions);\n> +\tfor (const auto &resolution : config_.resolutions) {\n> +\t\tif (config_.minResolutionSize.isNull() || config_.minResolutionSize > resolution.size)\n> +\t\t\tconfig_.minResolutionSize = resolution.size;\n>\n> -\t\tmaxResolutionSize_ = std::max(maxResolutionSize_, resolution.size);\n> +\t\tconfig_.maxResolutionSize = std::max(config_.maxResolutionSize, resolution.size);\n> \t}\n>\n> \tproperties_.set(properties::PixelArrayActiveAreas,\n> -\t\t\t{ Rectangle(maxResolutionSize_) });\n> +\t\t\t{ Rectangle(config_.maxResolutionSize) });\n>\n> \t/* \\todo Support multiple streams and pass multi_stream_test */\n> \tstreamConfigs_.resize(kMaxStream);\n> @@ -87,7 +89,7 @@ CameraConfiguration::Status VirtualCameraConfiguration::validate()\n>\n> \tfor (StreamConfiguration &cfg : config_) {\n> \t\tbool found = false;\n> -\t\tfor (const auto &resolution : data_->supportedResolutions_) {\n> +\t\tfor (const auto &resolution : data_->config_.resolutions) {\n> \t\t\tif (resolution.size.width == cfg.size.width &&\n> \t\t\t resolution.size.height == cfg.size.height) {\n> \t\t\t\tfound = true;\n> @@ -102,7 +104,7 @@ CameraConfiguration::Status VirtualCameraConfiguration::validate()\n> \t\t\t * Defining the default logic in PipelineHandler to\n> \t\t\t * find the closest resolution would be nice.\n> \t\t\t */\n> -\t\t\tcfg.size = data_->maxResolutionSize_;\n> +\t\t\tcfg.size = data_->config_.maxResolutionSize;\n> \t\t\tstatus = Adjusted;\n> \t\t}\n>\n> @@ -145,11 +147,11 @@ PipelineHandlerVirtual::generateConfiguration(Camera *camera,\n> \tfor (const StreamRole role : roles) {\n> \t\tstd::map> streamFormats;\n> \t\tPixelFormat pixelFormat = formats::NV12;\n> -\t\tstreamFormats[pixelFormat] = { { data->minResolutionSize_, data->maxResolutionSize_ } };\n> +\t\tstreamFormats[pixelFormat] = { { data->config_.minResolutionSize, data->config_.maxResolutionSize } };\n> \t\tStreamFormats formats(streamFormats);\n> \t\tStreamConfiguration cfg(formats);\n> \t\tcfg.pixelFormat = pixelFormat;\n> -\t\tcfg.size = data->maxResolutionSize_;\n> +\t\tcfg.size = data->config_.maxResolutionSize;\n> \t\tcfg.bufferCount = VirtualCameraConfiguration::kBufferCount;\n>\n> \t\tswitch (role) {\n> @@ -181,6 +183,7 @@ int PipelineHandlerVirtual::configure(Camera *camera,\n> \tVirtualCameraData *data = cameraData(camera);\n> \tfor (size_t i = 0; i < config->size(); ++i) {\n> \t\tconfig->at(i).setStream(&data->streamConfigs_[i].stream);\n> +\t\t/* Start reading the images/generating test patterns */\n> \t\tdata->streamConfigs_[i].frameGenerator->configure(\n> \t\t\tdata->streamConfigs_[i].stream.configuration().size);\n> \t}\n> @@ -274,10 +277,14 @@ bool PipelineHandlerVirtual::match([[maybe_unused]] DeviceEnumerator *enumerator\n> \t\tstd::set streams;\n> \t\tfor (auto &streamConfig : data->streamConfigs_)\n> \t\t\tstreams.insert(&streamConfig.stream);\n> -\t\tstd::string id = data->id_;\n> +\t\tstd::string id = data->config_.id;\n> \t\tstd::shared_ptr camera = Camera::create(std::move(data), id, streams);\n>\n> -\t\tinitFrameGenerator(camera.get());\n> +\t\tif (!initFrameGenerator(camera.get())) {\n> +\t\t\tLOG(Virtual, Error) << \"Failed to initialize frame \"\n> +\t\t\t\t\t << \"generator for camera: \" << id;\n> +\t\t\tcontinue;\n> +\t\t}\n>\n> \t\tregisterCamera(std::move(camera));\n> \t}\n> @@ -285,15 +292,30 @@ bool PipelineHandlerVirtual::match([[maybe_unused]] DeviceEnumerator *enumerator\n> \treturn true;\n> }\n>\n> -void PipelineHandlerVirtual::initFrameGenerator(Camera *camera)\n> +bool PipelineHandlerVirtual::initFrameGenerator(Camera *camera)\n> {\n> \tauto data = cameraData(camera);\n> -\tfor (auto &streamConfig : data->streamConfigs_) {\n> -\t\tif (data->testPattern_ == TestPattern::DiagonalLines)\n> -\t\t\tstreamConfig.frameGenerator = std::make_unique();\n> -\t\telse\n> -\t\t\tstreamConfig.frameGenerator = std::make_unique();\n> -\t}\n> +\tauto &frame = data->config_.frame;\n> +\tstd::visit(overloaded{\n> +\t\t\t [&](TestPattern &testPattern) {\n> +\t\t\t\t for (auto &streamConfig : data->streamConfigs_) {\n> +\t\t\t\t\t if (testPattern == TestPattern::DiagonalLines)\n> +\t\t\t\t\t\t streamConfig.frameGenerator = std::make_unique();\n> +\t\t\t\t\t else\n> +\t\t\t\t\t\t streamConfig.frameGenerator = std::make_unique();\n> +\t\t\t\t }\n> +\t\t\t },\n> +\t\t\t [&](ImageFrames &imageFrames) {\n> +\t\t\t\t for (auto &streamConfig : data->streamConfigs_)\n> +\t\t\t\t\t streamConfig.frameGenerator = ImageFrameGenerator::create(imageFrames);\n> +\t\t\t } },\n> +\t\t frame);\n> +\n> +\tfor (auto &streamConfig : data->streamConfigs_)\n> +\t\tif (!streamConfig.frameGenerator)\n> +\t\t\treturn false;\n> +\n> +\treturn true;\n> }\n>\n> REGISTER_PIPELINE_HANDLER(PipelineHandlerVirtual, \"virtual\")\n> diff --git a/src/libcamera/pipeline/virtual/virtual.h b/src/libcamera/pipeline/virtual/virtual.h\n> index 8830e00f..efa97e88 100644\n> --- a/src/libcamera/pipeline/virtual/virtual.h\n> +++ b/src/libcamera/pipeline/virtual/virtual.h\n> @@ -8,6 +8,8 @@\n> #pragma once\n>\n> #include \n> +#include \n> +#include \n> #include \n>\n> #include \n> @@ -16,10 +18,14 @@\n> #include \"libcamera/internal/dma_buf_allocator.h\"\n> #include \"libcamera/internal/pipeline_handler.h\"\n>\n> +#include \"frame_generator.h\"\n> +#include \"image_frame_generator.h\"\n> #include \"test_pattern_generator.h\"\n>\n> namespace libcamera {\n>\n> +using VirtualFrame = std::variant;\n> +\n> class VirtualCameraData : public Camera::Private\n> {\n> public:\n> @@ -33,18 +39,22 @@ public:\n> \t\tStream stream;\n> \t\tstd::unique_ptr frameGenerator;\n> \t};\n> +\t/* The config file is parsed to the Configuration struct */\n> +\tstruct Configuration {\n> +\t\tstd::string id;\n> +\t\tstd::vector resolutions;\n> +\t\tVirtualFrame frame;\n> +\n> +\t\tSize maxResolutionSize;\n> +\t\tSize minResolutionSize;\n> +\t};\n>\n> \tVirtualCameraData(PipelineHandler *pipe,\n> \t\t\t std::vector supportedResolutions);\n>\n> \t~VirtualCameraData() = default;\n>\n> -\tstd::string id_;\n> -\tTestPattern testPattern_ = TestPattern::ColorBars;\n> -\n> -\tconst std::vector supportedResolutions_;\n> -\tSize maxResolutionSize_;\n> -\tSize minResolutionSize_;\n> +\tConfiguration config_;\n>\n> \tstd::vector streamConfigs_;\n> };\n> @@ -89,7 +99,7 @@ private:\n> \t\treturn static_cast(camera->_d());\n> \t}\n>\n> -\tvoid initFrameGenerator(Camera *camera);\n> +\tbool initFrameGenerator(Camera *camera);\n>\n> \tDmaBufAllocator dmaBufAllocator_;\n> };\n> --\n> 2.46.0.469.g59c65b2a67-goog\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 9F88EC324C\n\tfor ;\n\tMon, 9 Sep 2024 10:17:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 55546634EE;\n\tMon, 9 Sep 2024 12:17:07 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 878EB634EB\n\tfor ;\n\tMon, 9 Sep 2024 12:17:04 +0200 (CEST)","from ideasonboard.com (unknown [213.208.157.109])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 85E152C6;\n\tMon, 9 Sep 2024 12:15:47 +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=\"R7II05m4\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1725876948;\n\tbh=LgFNB6iAf+NGUcRYwsiyY3zNtuFS2VHH5gSO/gVEuU8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=R7II05m444HJW9kJ7hvNycpEUZAePhF+kmEdh3BfcDkZAIEOt6LcJG5buea8RKM6o\n\tuK4rLlKTs8GpOtypePLuuWVuCr/3yUGjhsfVkwBwBUX4a6wSTfjLt6FORRU3MRs5aD\n\tmQiJPAloYevejZIeewelronGDbE/eKovILzF87UA=","Date":"Mon, 9 Sep 2024 12:16:59 +0200","From":"Jacopo Mondi ","To":"Harvey Yang ","Cc":"libcamera-devel@lists.libcamera.org, Konami Shu , \n\tYunke Cao , Tomasz Figa ","Subject":"Re: [PATCH v11 6/7] libcamera: virtual: Add ImageFrameGenerator","Message-ID":"<3yzin5r3r3nmawjocyrakt3aq6xsys2fnsjwobddfid7yxwjxk@gc6abloh6zas>","References":"<20240907143110.2210711-1-chenghaoyang@google.com>\n\t<20240907143110.2210711-7-chenghaoyang@google.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240907143110.2210711-7-chenghaoyang@google.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31132,"web_url":"https://patchwork.libcamera.org/comment/31132/","msgid":"","date":"2024-09-09T15:22:47","subject":"Re: [PATCH v11 6/7] libcamera: virtual: Add ImageFrameGenerator","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi Jacopo,\n\nOn Mon, Sep 9, 2024 at 6:17 PM Jacopo Mondi \nwrote:\n\n> Hi\n>\n> On Sat, Sep 07, 2024 at 02:28:31PM GMT, Harvey Yang wrote:\n> > From: Konami Shu \n> >\n> > Besides TestPatternGenerator, this patch adds ImageFrameGenerator that\n> > loads real images (jpg / jpeg for now) as the source and generates\n> > scaled frames.\n> >\n> > Signed-off-by: Konami Shu \n> > Co-developed-by: Harvey Yang \n> > Co-developed-by: Yunke Cao \n> > Co-developed-by: Tomasz Figa \n> > ---\n> > src/libcamera/pipeline/virtual/README.md | 9 +-\n> > .../virtual/image_frame_generator.cpp | 178 ++++++++++++++++++\n> > .../pipeline/virtual/image_frame_generator.h | 54 ++++++\n> > src/libcamera/pipeline/virtual/meson.build | 4 +\n> > src/libcamera/pipeline/virtual/parser.cpp | 76 +++++++-\n> > src/libcamera/pipeline/virtual/parser.h | 2 +\n> > src/libcamera/pipeline/virtual/utils.h | 17 ++\n> > src/libcamera/pipeline/virtual/virtual.cpp | 60 ++++--\n> > src/libcamera/pipeline/virtual/virtual.h | 24 ++-\n> > 9 files changed, 389 insertions(+), 35 deletions(-)\n> > create mode 100644\n> src/libcamera/pipeline/virtual/image_frame_generator.cpp\n> > create mode 100644\n> src/libcamera/pipeline/virtual/image_frame_generator.h\n> > create mode 100644 src/libcamera/pipeline/virtual/utils.h\n> >\n> > diff --git a/src/libcamera/pipeline/virtual/README.md\n> b/src/libcamera/pipeline/virtual/README.md\n> > index ef80bb48..18c8341b 100644\n> > --- a/src/libcamera/pipeline/virtual/README.md\n> > +++ b/src/libcamera/pipeline/virtual/README.md\n> > @@ -16,7 +16,13 @@ Each camera block is a dictionary, containing the\n> following keys:\n> > - `width` (`unsigned int`, default=1920): Width of the window\n> resolution. This needs to be even.\n> > - `height` (`unsigned int`, default=1080): Height of the window\n> resolution.\n> > - `frame_rates` (list of `int`, default=`[30,60]` ): Range of the\n> frame rate (per second). If the list contains one value, it's the lower\n> bound and the upper bound. If the list contains two values, the first is\n> the lower bound and the second is the upper bound. No other number of\n> values is allowed.\n> > -- `test_pattern` (`string`): Which test pattern to use as frames. The\n> options are \"bars\", \"lines\".\n> > +- `test_pattern` (`string`): Which test pattern to use as frames. The\n> 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\n> series of images. Cannot be set with `test_pattern`.\n> > + - The test patterns are \"bars\" which means color bars, and \"lines\"\n> 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\n> the directory are \"{n}.jpg\" with {n} is the sequence of images starting\n> with 0.\n> > + - `scale_mode`(`string`, default=\"fill\"): Scale mode when the frames\n> are images. The only scale mode supported now is \"fill\". This does not\n> affect the scale mode for now.\n>\n> Wouldn't it be more logical to add the frame generator support before\n> the config file ? Otherwise you are adding pieces here to what has\n> been added just one patch ago\n>\n\nYeah makes sense. Will be updated in v12.\n\n\n>\n> > - `location` (`string`, default=\"front\"): The location of the camera.\n> Support \"front\" and \"back\". This is displayed in qcam camera selection\n> window but this does not change the output.\n> > - `model` (`string`, default=\"Unknown\"): The model name of the camera.\n> This is displayed in qcam camera selection window but this does not change\n> the output.\n> >\n> > @@ -37,6 +43,7 @@ This is the procedure of the Parser class:\n> > 3. Parse each property and register the data.\n> > - `parseSupportedFormats()`: Parses `supported_formats` in the\n> config, which contains resolutions and frame rates.\n> > - `parseTestPattern()`: Parses `test_pattern` in the config.\n> > + - `parseFrame()`: Parses `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> > diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.cpp\n> b/src/libcamera/pipeline/virtual/image_frame_generator.cpp\n> > new file mode 100644\n> > index 00000000..db3efe15\n> > --- /dev/null\n> > +++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp\n> > @@ -0,0 +1,178 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2024, Google Inc.\n> > + *\n> > + * image_frame_generator.cpp - Derived class of FrameGenerator for\n> > + * generating frames from images\n> > + */\n> > +\n> > +#include \"image_frame_generator.h\"\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \"libcamera/internal/mapped_framebuffer.h\"\n> > +\n> > +#include \"libyuv/convert.h\"\n> > +#include \"libyuv/scale.h\"\n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DECLARE_CATEGORY(Virtual)\n> > +\n> > +/*\n> > + * Factory function to create an ImageFrameGenerator object.\n> > + * Read the images and convert them to buffers in NV12 format.\n> > + * Store the pointers to the buffers to a list (imageFrameDatas)\n> > + */\n> > +std::unique_ptr\n> > +ImageFrameGenerator::create(ImageFrames &imageFrames)\n> > +{\n> > + std::unique_ptr imageFrameGenerator =\n> > + std::make_unique();\n> > + imageFrameGenerator->imageFrames_ = &imageFrames;\n> > +\n> > + /*\n> > + * For each file in the directory, load the image,\n> > + * convert it to NV12, and store the pointer.\n>\n> weird indent\n>\n\nSorry, fixed.\n\n\n>\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) +\n> \".jpg\");\n> > +\n> > + File file(path);\n> > + if (!file.open(File::OpenModeFlag::ReadOnly)) {\n> > + LOG(Virtual, Error) << \"Failed to open image file\n> \" << file.fileName()\n> > + << \": \" <<\n> strerror(file.error());\n> > + return nullptr;\n> > + }\n> > +\n> > + /* Read the image file to data */\n> > + auto fileSize = file.size();\n> > + auto buffer = std::make_unique(file.size());\n> > + if (file.read({ buffer.get(),\n> static_cast(fileSize) }) != fileSize) {\n> > + LOG(Virtual, Error) << \"Failed to read file \" <<\n> file.fileName()\n> > + << \": \" <<\n> strerror(file.error());\n> > + return nullptr;\n> > + }\n> > +\n> > + /* Get the width and height of the image */\n> > + int width, height;\n> > + if (libyuv::MJPGSize(buffer.get(), fileSize, &width,\n> &height)) {\n> > + LOG(Virtual, Error) << \"Failed to get the size of\n> the image file: \"\n> > + << file.fileName();\n> > + return nullptr;\n> > + }\n> > +\n> > + /* Convert to NV12 and write the data to tmpY and tmpUV */\n> > + unsigned int halfWidth = (width + 1) / 2;\n> > + unsigned int halfHeight = (height + 1) / 2;\n>\n> Do you need this for rounding ?\n\n\n> > + std::unique_ptr dstY =\n> > + std::make_unique(width * height);\n> > + std::unique_ptr dstUV =\n> > + std::make_unique(halfWidth * halfHeight\n> * 2);\n>\n> otherwise using width * height / 2 would do\n>\n\nRight, it works. Thanks!\n\n\n>\n> > + int ret = libyuv::MJPGToNV12(buffer.get(), fileSize,\n> > + dstY.get(), width,\n> dstUV.get(),\n> > + width, width, height, width,\n> height);\n> > + if (ret != 0)\n> > + LOG(Virtual, Error) << \"MJPGToNV12() failed with \"\n> << ret;\n> > +\n> > + imageFrameGenerator->imageFrameDatas_.emplace_back(\n> > + ImageFrameData{ std::move(dstY), std::move(dstUV),\n> > + Size(width, height) });\n> > + }\n> > +\n> > + return imageFrameGenerator;\n> > +}\n> > +\n> > +/* Scale the buffers for image frames. */\n> > +void ImageFrameGenerator::configure(const Size &size)\n> > +{\n> > + /* Reset the source images to prevent multiple configuration calls\n> */\n> > + scaledFrameDatas_.clear();\n> > + frameCount_ = 0;\n> > + parameter_ = 0;\n> > +\n> > + for (unsigned int i = 0; i < imageFrames_->number.value_or(1);\n> 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> > + std::unique_ptr scaledY =\n> > + std::make_unique(size.width *\n> size.height);\n> > + std::unique_ptr scaledUV =\n> > + std::make_unique(halfSizeWidth *\n> halfSizeHeight * 2);\n> > + auto &src = imageFrameDatas_[i];\n> > +\n> > + /*\n> > + * \\todo Some platforms might enforce stride due to GPU,\n> like\n> > + * ChromeOS ciri (64). The weight needs to be a multiple of\n> > + * the stride to work properly for now.\n> > + */\n> > + libyuv::NV12Scale(src.Y.get(), src.size.width,\n> > + src.UV.get(), src.size.width,\n> > + src.size.width, src.size.height,\n> > + scaledY.get(), size.width,\n> scaledUV.get(), size.width,\n> > + size.width, size.height,\n> libyuv::FilterMode::kFilterBilinear);\n> > +\n> > + scaledFrameDatas_.emplace_back(\n> > + ImageFrameData{ std::move(scaledY),\n> std::move(scaledUV), size });\n> > + }\n> > +}\n> > +\n> > +void ImageFrameGenerator::generateFrame(const Size &size, const\n> FrameBuffer *buffer)\n> > +{\n> > + /* Don't do anything when the list of buffers is empty*/\n> > + ASSERT(!scaledFrameDatas_.empty());\n> > +\n> > + MappedFrameBuffer mappedFrameBuffer(buffer,\n> MappedFrameBuffer::MapFlag::Write);\n> > +\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> > +\n> > + /* Write the scaledY and scaledUV to the mapped frame buffer */\n> > + libyuv::NV12Copy(scaledFrameDatas_[frameCount_].Y.get(),\n> size.width,\n> > + scaledFrameDatas_[frameCount_].UV.get(),\n> size.width, planes[0].begin(),\n> > + size.width, planes[1].begin(), size.width,\n> > + size.width, size.height);\n> > +\n> > + /* proceed an image every 4 frames */\n> > + /* \\todo read the parameter_ from the configuration file? */\n> > + parameter_++;\n> > + if (parameter_ % 4 == 0)\n>\n> Make this a constexpr if not configurable\n>\n>\nDone.\n\n\n> > + frameCount_++;\n> > +}\n> > +\n> > +/**\n>\n> We don't doxygen the pipelines, but doesn't hurt to have comments.\n> Maybe remove the /**\n>\n>\nMake them start with `/*` you mean?\n\n\n>\n> > + * \\var ImageFrameGenerator::imageFrameDatas_\n> > + * \\brief List of pointers to the not scaled image buffers\n> > + */\n> > +\n> > +/**\n> > + * \\var ImageFrameGenerator::scaledFrameDatas_\n> > + * \\brief List of pointers to the scaled image buffers\n> > + */\n> > +\n> > +/**\n> > + * \\var ImageFrameGenerator::imageFrames_\n> > + * \\brief Pointer to the imageFrames_ in VirtualCameraData\n> > + */\n> > +\n> > +/**\n> > + * \\var ImageFrameGenerator::parameter_\n> > + * \\brief Speed parameter. Change to the next image every parameter_\n> frames\n> > + */\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.h\n> b/src/libcamera/pipeline/virtual/image_frame_generator.h\n> > new file mode 100644\n> > index 00000000..4ad8aad2\n> > --- /dev/null\n> > +++ b/src/libcamera/pipeline/virtual/image_frame_generator.h\n> > @@ -0,0 +1,54 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2024, Google Inc.\n> > + *\n> > + * image_frame_generator.h - Derived class of FrameGenerator for\n> > + * generating frames from images\n> > + */\n> > +\n> > +#pragma once\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \"frame_generator.h\"\n> > +\n> > +namespace libcamera {\n> > +\n> > +enum class ScaleMode : char {\n> > + Fill = 0,\n> > +};\n>\n> I know you want more scale modes, but as long as you only support one\n> there's no need for a type\n>\n\nHmm, do you think it's better to just remove the argument in the config\nfile?\n\n\n>\n> > +\n> > +/* Frame configuration provided by the config file */\n> > +struct ImageFrames {\n> > + std::filesystem::path path;\n> > + ScaleMode scaleMode;\n>\n> As this can only be \"Fill\", nothing else is valid atm\n>\n> > + std::optional number;\n> > +};\n> > +\n> > +class ImageFrameGenerator : public FrameGenerator\n> > +{\n> > +public:\n> > + static std::unique_ptr create(ImageFrames\n> &imageFrames);\n> > +\n> > +private:\n> > + struct ImageFrameData {\n> > + std::unique_ptr Y;\n> > + std::unique_ptr UV;\n> > + Size size;\n> > + };\n> > +\n> > + void configure(const Size &size) override;\n> > + void generateFrame(const Size &size, const FrameBuffer *buffer)\n> override;\n> > +\n> > + std::vector imageFrameDatas_;\n> > + std::vector scaledFrameDatas_;\n> > + ImageFrames *imageFrames_;\n> > + unsigned int frameCount_;\n> > + unsigned int parameter_;\n> > +};\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/pipeline/virtual/meson.build\n> b/src/libcamera/pipeline/virtual/meson.build\n> > index d72ac5be..395919b3 100644\n> > --- a/src/libcamera/pipeline/virtual/meson.build\n> > +++ b/src/libcamera/pipeline/virtual/meson.build\n> > @@ -1,9 +1,13 @@\n> > # SPDX-License-Identifier: CC0-1.0\n> >\n> > libcamera_internal_sources += files([\n> > + 'image_frame_generator.cpp',\n> > 'parser.cpp',\n> > 'test_pattern_generator.cpp',\n> > 'virtual.cpp',\n> > ])\n> >\n> > +libjpeg = dependency('libjpeg', required : false)\n> > +\n> > libcamera_deps += [libyuv_dep]\n> > +libcamera_deps += [libjpeg]\n> > diff --git a/src/libcamera/pipeline/virtual/parser.cpp\n> b/src/libcamera/pipeline/virtual/parser.cpp\n> > index d861a52a..5076e71c 100644\n> > --- a/src/libcamera/pipeline/virtual/parser.cpp\n> > +++ b/src/libcamera/pipeline/virtual/parser.cpp\n> > @@ -52,12 +52,12 @@ Parser::parseConfigFile(File &file, PipelineHandler\n> *pipe)\n> > continue;\n> > }\n> >\n> > - data->id_ = cameraId;\n> > + data->config_.id = cameraId;\n> > ControlInfoMap::Map controls;\n> > /* todo: Check which resolution's frame rate to be\n> reported */\n> > controls[&controls::FrameDurationLimits] =\n> > - ControlInfo(int64_t(1000000 /\n> data->supportedResolutions_[0].frameRates[1]),\n> > - int64_t(1000000 /\n> data->supportedResolutions_[0].frameRates[0]));\n> > + ControlInfo(int64_t(1000000 /\n> data->config_.resolutions[0].frameRates[1]),\n> > + int64_t(1000000 /\n> data->config_.resolutions[0].frameRates[0]));\n> > data->controlInfo_ = ControlInfoMap(std::move(controls),\n> controls::controls);\n> > configurations.push_back(std::move(data));\n> > }\n> > @@ -75,7 +75,8 @@ Parser::parseCameraConfigData(const YamlObject\n> &cameraConfigData,\n> > std::unique_ptr data =\n> > std::make_unique(pipe, resolutions);\n> >\n> > - if (parseTestPattern(cameraConfigData, data.get()))\n> > + if (parseTestPattern(cameraConfigData, data.get()) &&\n> > + parseFrame(cameraConfigData, data.get()))\n>\n> This is fragile and only works because if parseTestPattern() returns 0\n> then parseFrame() is never called\n\n\n> > return nullptr;\n> >\n> > if (parseLocation(cameraConfigData, data.get()))\n> > @@ -148,16 +149,75 @@ int Parser::parseTestPattern(const YamlObject\n> &cameraConfigData, VirtualCameraDa\n> > {\n> > std::string testPattern =\n> cameraConfigData[\"test_pattern\"].get(\"\");\n> >\n> > - /* Default value is \"bars\" */\n> > if (testPattern == \"bars\") {\n> > - data->testPattern_ = TestPattern::ColorBars;\n> > + data->config_.frame = TestPattern::ColorBars;\n> > } else if (testPattern == \"lines\") {\n> > - data->testPattern_ = TestPattern::DiagonalLines;\n> > + data->config_.frame = TestPattern::DiagonalLines;\n> > } else {\n> > - LOG(Virtual, Error) << \"Test pattern: \" << testPattern\n> > + LOG(Virtual, Debug) << \"Test pattern: \" << testPattern\n> > << \"is not supported\";\n> > return -EINVAL;\n> > }\n> > +\n> > + return 0;\n> > +}\n> > +\n> > +int Parser::parseFrame(const YamlObject &cameraConfigData,\n> VirtualCameraData *data)\n>\n> You can unify these two functions.\n>\n> Get \"testPattern\", if valid make sure it's only either \"bars\" or\n> \"lines\" and fail if it is specified but has an unsupported value.\n>\n> Then check for \"frames\". If both \"frames\" and \"testPattern\" are\n> specified, it's an error. Then parse \"frames\" like you do here.\n>\n\nMakes sense. Thanks! Updated.\n\n\n>\n> > +{\n> > + const YamlObject &frames = cameraConfigData[\"frames\"];\n> > +\n> > + /* When there is no frames provided in the config file, use color\n> bar test pattern */\n> > + if (frames.size() == 0) {\n> > + data->config_.frame = TestPattern::ColorBars;\n> > + return 0;\n> > + }\n> > +\n> > + if (!frames.isDictionary()) {\n> > + LOG(Virtual, Error) << \"'frames' is not a dictionary.\";\n> > + return -EINVAL;\n> > + }\n> > +\n> > + std::string path = frames[\"path\"].get(\"\");\n> > +\n> > + ScaleMode scaleMode;\n> > + if (auto ext = std::filesystem::path(path).extension();\n> > + ext == \".jpg\" || ext == \".jpeg\") {\n> > + if (parseScaleMode(frames, &scaleMode))\n> > + return -EINVAL;\n> > + data->config_.frame = ImageFrames{ path, scaleMode,\n> std::nullopt };\n> > + } else if\n> (std::filesystem::is_directory(std::filesystem::symlink_status(path))) {\n> > + if (parseScaleMode(frames, &scaleMode))\n> > + return -EINVAL;\n>\n> Could you parse scale mode before checking the file extensions ?\n>\n>\nDone.\n\n\n> > +\n> > + using std::filesystem::directory_iterator;\n> > + unsigned int numOfFiles =\n> std::distance(directory_iterator(path), directory_iterator{});\n> > + if (numOfFiles == 0) {\n> > + LOG(Virtual, Error) << \"Empty directory\";\n> > + return -EINVAL;\n> > + }\n> > + data->config_.frame = ImageFrames{ path, scaleMode,\n> numOfFiles };\n> > + } else {\n> > + LOG(Virtual, Error) << \"Frame: \" << path << \" is not\n> supported\";\n> > + return -EINVAL;\n> > + }\n> > +\n> > + return 0;\n> > +}\n> > +\n> > +int Parser::parseScaleMode(\n> > + const YamlObject &framesConfigData, ScaleMode *scaleMode)\n>\n> weird indent\n>\n\nUpdated.\n\n\n>\n> > +{\n> > + std::string mode =\n> framesConfigData[\"scale_mode\"].get(\"\");\n> > +\n> > + /* Default value is fill */\n> > + if (mode == \"fill\" || mode == \"\") {\n> > + *scaleMode = ScaleMode::Fill;\n>\n> You don't need a type as suggested above, you can either assume \"Fill\"\n> or fail during parsing.\n>\n> > + } else {\n> > + LOG(Virtual, Error) << \"scaleMode: \" << mode\n> > + << \" is not supported\";\n> > + return -EINVAL;\n> > + }\n> > +\n> > return 0;\n> > }\n> >\n> > diff --git a/src/libcamera/pipeline/virtual/parser.h\n> b/src/libcamera/pipeline/virtual/parser.h\n> > index 09c3c56b..f65616e3 100644\n> > --- a/src/libcamera/pipeline/virtual/parser.h\n> > +++ b/src/libcamera/pipeline/virtual/parser.h\n> > @@ -35,8 +35,10 @@ private:\n> > int parseSupportedFormats(const YamlObject &cameraConfigData,\n> >\n> std::vector *resolutions);\n> > int parseTestPattern(const YamlObject &cameraConfigData,\n> VirtualCameraData *data);\n> > + int parseFrame(const YamlObject &cameraConfigData,\n> VirtualCameraData *data);\n> > int parseLocation(const YamlObject &cameraConfigData,\n> VirtualCameraData *data);\n> > int parseModel(const YamlObject &cameraConfigData,\n> VirtualCameraData *data);\n> > + int parseScaleMode(const YamlObject &framesConfigData, ScaleMode\n> *scaleMode);\n> > };\n> >\n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/pipeline/virtual/utils.h\n> b/src/libcamera/pipeline/virtual/utils.h\n> > new file mode 100644\n> > index 00000000..43a14d4b\n> > --- /dev/null\n> > +++ b/src/libcamera/pipeline/virtual/utils.h\n> > @@ -0,0 +1,17 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2024, Google Inc.\n> > + *\n> > + * utils.h - Utility types for Virtual Pipeline Handler\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +template\n> > +struct overloaded : Ts... {\n> > + using Ts::operator()...;\n> > +};\n> > +template\n> > +overloaded(Ts...) -> overloaded;\n> > +\n>\n> Does using a standard construct like std::visit a custom definition\n>\n\nSorry, I don't get what you mean.\n\n\n> like this one ? Also, this header is included form a single cpp file,\n> move its content there if you can.\n>\n>\nHmm, I was suggested otherwise:\n\nhttps://patchwork.libcamera.org/patch/20974/\n```\n\nApart from the indentation looking a bit off, it seems ok.\n\nI think the `overloaded` type could go into `utils.h` or similar.\n\n\nRegards,\nBarnabás Pőcze\n\n```\n\nI'm new to std::visit, so...\n\n\n>\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/pipeline/virtual/virtual.cpp\n> b/src/libcamera/pipeline/virtual/virtual.cpp\n> > index 55bc30df..98aed412 100644\n> > --- a/src/libcamera/pipeline/virtual/virtual.cpp\n> > +++ b/src/libcamera/pipeline/virtual/virtual.cpp\n> > @@ -27,6 +27,7 @@\n> > #include \"libcamera/internal/yaml_parser.h\"\n> >\n> > #include \"parser.h\"\n> > +#include \"utils.h\"\n> >\n> > namespace libcamera {\n> >\n> > @@ -49,17 +50,18 @@ uint64_t currentTimestamp()\n> >\n> > VirtualCameraData::VirtualCameraData(PipelineHandler *pipe,\n> > std::vector\n> supportedResolutions)\n> > - : Camera::Private(pipe),\n> supportedResolutions_(std::move(supportedResolutions))\n> > + : Camera::Private(pipe)\n> > {\n> > - for (const auto &resolution : supportedResolutions_) {\n> > - if (minResolutionSize_.isNull() || minResolutionSize_ >\n> resolution.size)\n> > - minResolutionSize_ = resolution.size;\n> > + config_.resolutions = std::move(supportedResolutions);\n> > + for (const auto &resolution : config_.resolutions) {\n> > + if (config_.minResolutionSize.isNull() ||\n> config_.minResolutionSize > resolution.size)\n> > + config_.minResolutionSize = resolution.size;\n> >\n> > - maxResolutionSize_ = std::max(maxResolutionSize_,\n> resolution.size);\n> > + config_.maxResolutionSize =\n> std::max(config_.maxResolutionSize, resolution.size);\n> > }\n> >\n> > properties_.set(properties::PixelArrayActiveAreas,\n> > - { Rectangle(maxResolutionSize_) });\n> > + { Rectangle(config_.maxResolutionSize) });\n> >\n> > /* \\todo Support multiple streams and pass multi_stream_test */\n> > streamConfigs_.resize(kMaxStream);\n> > @@ -87,7 +89,7 @@ CameraConfiguration::Status\n> VirtualCameraConfiguration::validate()\n> >\n> > for (StreamConfiguration &cfg : config_) {\n> > bool found = false;\n> > - for (const auto &resolution :\n> 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> > @@ -102,7 +104,7 @@ CameraConfiguration::Status\n> 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> > @@ -145,11 +147,11 @@\n> PipelineHandlerVirtual::generateConfiguration(Camera *camera,\n> > for (const StreamRole role : roles) {\n> > std::map>\n> streamFormats;\n> > PixelFormat pixelFormat = formats::NV12;\n> > - streamFormats[pixelFormat] = { { data->minResolutionSize_,\n> data->maxResolutionSize_ } };\n> > + streamFormats[pixelFormat] = { {\n> data->config_.minResolutionSize, data->config_.maxResolutionSize } };\n> > StreamFormats formats(streamFormats);\n> > StreamConfiguration cfg(formats);\n> > cfg.pixelFormat = pixelFormat;\n> > - cfg.size = data->maxResolutionSize_;\n> > + cfg.size = data->config_.maxResolutionSize;\n> > cfg.bufferCount = VirtualCameraConfiguration::kBufferCount;\n> >\n> > switch (role) {\n> > @@ -181,6 +183,7 @@ int PipelineHandlerVirtual::configure(Camera *camera,\n> > VirtualCameraData *data = cameraData(camera);\n> > for (size_t i = 0; i < config->size(); ++i) {\n> > config->at(i).setStream(&data->streamConfigs_[i].stream);\n> > + /* Start reading the images/generating test patterns */\n> > data->streamConfigs_[i].frameGenerator->configure(\n> >\n> data->streamConfigs_[i].stream.configuration().size);\n> > }\n> > @@ -274,10 +277,14 @@ bool\n> PipelineHandlerVirtual::match([[maybe_unused]] DeviceEnumerator *enumerator\n> > std::set streams;\n> > for (auto &streamConfig : data->streamConfigs_)\n> > streams.insert(&streamConfig.stream);\n> > - std::string id = data->id_;\n> > + std::string id = data->config_.id;\n> > std::shared_ptr camera =\n> Camera::create(std::move(data), id, streams);\n> >\n> > - initFrameGenerator(camera.get());\n> > + if (!initFrameGenerator(camera.get())) {\n> > + LOG(Virtual, Error) << \"Failed to initialize frame\n> \"\n> > + << \"generator for camera: \" <<\n> id;\n> > + continue;\n> > + }\n> >\n> > registerCamera(std::move(camera));\n> > }\n> > @@ -285,15 +292,30 @@ bool\n> PipelineHandlerVirtual::match([[maybe_unused]] DeviceEnumerator *enumerator\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 =\n> std::make_unique();\n> > - else\n> > - streamConfig.frameGenerator =\n> std::make_unique();\n> > - }\n> > + auto &frame = data->config_.frame;\n> > + std::visit(overloaded{\n> > + [&](TestPattern &testPattern) {\n> > + for (auto &streamConfig :\n> data->streamConfigs_) {\n> > + if (testPattern ==\n> TestPattern::DiagonalLines)\n> > +\n> streamConfig.frameGenerator = std::make_unique();\n> > + else\n> > +\n> streamConfig.frameGenerator = std::make_unique();\n> > + }\n> > + },\n> > + [&](ImageFrames &imageFrames) {\n> > + for (auto &streamConfig :\n> data->streamConfigs_)\n> > + streamConfig.frameGenerator =\n> 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\n> b/src/libcamera/pipeline/virtual/virtual.h\n> > index 8830e00f..efa97e88 100644\n> > --- a/src/libcamera/pipeline/virtual/virtual.h\n> > +++ b/src/libcamera/pipeline/virtual/virtual.h\n> > @@ -8,6 +8,8 @@\n> > #pragma once\n> >\n> > #include \n> > +#include \n> > +#include \n> > #include \n> >\n> > #include \n> > @@ -16,10 +18,14 @@\n> > #include \"libcamera/internal/dma_buf_allocator.h\"\n> > #include \"libcamera/internal/pipeline_handler.h\"\n> >\n> > +#include \"frame_generator.h\"\n> > +#include \"image_frame_generator.h\"\n> > #include \"test_pattern_generator.h\"\n> >\n> > namespace libcamera {\n> >\n> > +using VirtualFrame = std::variant;\n> > +\n> > class VirtualCameraData : public Camera::Private\n> > {\n> > public:\n> > @@ -33,18 +39,22 @@ public:\n> > Stream stream;\n> > std::unique_ptr frameGenerator;\n> > };\n> > + /* The config file is parsed to the Configuration struct */\n> > + struct Configuration {\n> > + std::string id;\n> > + std::vector resolutions;\n> > + VirtualFrame frame;\n> > +\n> > + Size maxResolutionSize;\n> > + Size minResolutionSize;\n> > + };\n> >\n> > VirtualCameraData(PipelineHandler *pipe,\n> > std::vector supportedResolutions);\n> >\n> > ~VirtualCameraData() = default;\n> >\n> > - std::string id_;\n> > - TestPattern testPattern_ = TestPattern::ColorBars;\n> > -\n> > - const std::vector supportedResolutions_;\n> > - Size maxResolutionSize_;\n> > - Size minResolutionSize_;\n> > + Configuration config_;\n> >\n> > std::vector streamConfigs_;\n> > };\n> > @@ -89,7 +99,7 @@ private:\n> > return static_cast(camera->_d());\n> > }\n> >\n> > - void initFrameGenerator(Camera *camera);\n> > + bool initFrameGenerator(Camera *camera);\n> >\n> > DmaBufAllocator dmaBufAllocator_;\n> > };\n> > --\n> > 2.46.0.469.g59c65b2a67-goog\n> >\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id BF73BC324C\n\tfor ;\n\tMon, 9 Sep 2024 15:23:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BA9F8634F7;\n\tMon, 9 Sep 2024 17:23:04 +0200 (CEST)","from mail-lj1-x235.google.com (mail-lj1-x235.google.com\n\t[IPv6:2a00:1450:4864:20::235])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C86A7634EB\n\tfor ;\n\tMon, 9 Sep 2024 17:23:01 +0200 (CEST)","by mail-lj1-x235.google.com with SMTP id\n\t38308e7fff4ca-2f762de00fbso17849331fa.2\n\tfor ;\n\tMon, 09 Sep 2024 08:23:01 -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=\"ZKEwY5Vq\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1725895381; x=1726500181;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=PeKefRtj8hemNOTicPwlQWhh6d9BhscUq6+0TqU8MSo=;\n\tb=ZKEwY5VqCr5QDFWtZ7JCm3lU/65+NXZJPkBzGrT/1wToq2lclVdsZzh1e1Ae0e+Ck8\n\tR6/eKz4aIM7s8ddoWRefMps5uBtzQW4vO3v39oiwR9k2TFgV0vczA+wTaEOL7Q90bLH+\n\t4Ghzgsh/HAB6/nZfmS6dc34aS+cqEq5Ds8TgI=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1725895381; x=1726500181;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=PeKefRtj8hemNOTicPwlQWhh6d9BhscUq6+0TqU8MSo=;\n\tb=PbsO14SSFdFVP3Q09oJ8iNJd62ohBH5/Ezp537Jr7q8h+2luZtduNROG/bibQA20wo\n\tdOz5rdh1IRZXBsRz6RcksPGqGnpoFPLvt0NuSlI9bw/nPLatLwOCCAAH2bNSdq5Fp+Ss\n\t0wwqOdja4YYe7FgPPePVr0XzgWOte4ydAXDGdA50MMsRowHgymy87WzQ1zUqVOOZp3Yc\n\tMQKnlQ8Sv7q73IGbbNq9nQtQ3dORBk14H7MwT5Ubf1B+T1Sz6WvqMaXUe1pV5RL+Ga6D\n\tkn4C5lLof3Kwx5zqXNQltcPaTG//7gFFIWQ9liHzRTEWK7cBLIeT5y8ryDDnJ2eKnhE5\n\tEzlg==","X-Gm-Message-State":"AOJu0YyvOBxNLeJcE8Ldz76SYtWkIK+sS96G4LG8SEjhllqscrU8rELX\n\tIIrLyZcrVDr1UEr1d1/Y10SBnN6nGB+SbdM2iaF6etIGXYRu351SozxLCR9Hdfa7dzno1fYWvcC\n\tYZtdhr5LalpZ0Tvesp9pzeOAo5uwZUbwygZe9N56a5kEwBLpWXQ==","X-Google-Smtp-Source":"AGHT+IEEMMQxZq11K7r284vGMGlz1UKMVMNBZpsnqPbfFtOyiwQihIsIfq5zWRCIOxU0EGBkqQ918VEFSsFeqcvllxY=","X-Received":"by 2002:a2e:4612:0:b0:2f6:61f3:c618 with SMTP id\n\t38308e7fff4ca-2f751f85ae9mr68407981fa.43.1725895380242;\n\tMon, 09 Sep 2024 08:23:00 -0700 (PDT)","MIME-Version":"1.0","References":"<20240907143110.2210711-1-chenghaoyang@google.com>\n\t<20240907143110.2210711-7-chenghaoyang@google.com>\n\t<3yzin5r3r3nmawjocyrakt3aq6xsys2fnsjwobddfid7yxwjxk@gc6abloh6zas>","In-Reply-To":"<3yzin5r3r3nmawjocyrakt3aq6xsys2fnsjwobddfid7yxwjxk@gc6abloh6zas>","From":"Cheng-Hao Yang ","Date":"Mon, 9 Sep 2024 23:22:47 +0800","Message-ID":"","Subject":"Re: [PATCH v11 6/7] libcamera: virtual: Add ImageFrameGenerator","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org, Konami Shu , \n\tYunke Cao , Tomasz Figa ","Content-Type":"multipart/alternative; boundary=\"0000000000000f2fe50621b15590\"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31139,"web_url":"https://patchwork.libcamera.org/comment/31139/","msgid":"","date":"2024-09-09T20:26:48","subject":"Re: [PATCH v11 6/7] libcamera: virtual: Add ImageFrameGenerator","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Harvey\n\nOn Mon, Sep 09, 2024 at 11:22:47PM GMT, Cheng-Hao Yang wrote:\n> Hi Jacopo,\n>\n> On Mon, Sep 9, 2024 at 6:17 PM Jacopo Mondi \n> wrote:\n>\n> > Hi\n> >\n> > On Sat, Sep 07, 2024 at 02:28:31PM GMT, Harvey Yang wrote:\n> > > From: Konami Shu \n> > >\n> > > Besides TestPatternGenerator, this patch adds ImageFrameGenerator that\n> > > loads real images (jpg / jpeg for now) as the source and generates\n> > > scaled frames.\n> > >\n> > > Signed-off-by: Konami Shu \n> > > Co-developed-by: Harvey Yang \n> > > Co-developed-by: Yunke Cao \n> > > Co-developed-by: Tomasz Figa \n> > > ---\n> > > src/libcamera/pipeline/virtual/README.md | 9 +-\n> > > .../virtual/image_frame_generator.cpp | 178 ++++++++++++++++++\n> > > .../pipeline/virtual/image_frame_generator.h | 54 ++++++\n> > > src/libcamera/pipeline/virtual/meson.build | 4 +\n> > > src/libcamera/pipeline/virtual/parser.cpp | 76 +++++++-\n> > > src/libcamera/pipeline/virtual/parser.h | 2 +\n> > > src/libcamera/pipeline/virtual/utils.h | 17 ++\n> > > src/libcamera/pipeline/virtual/virtual.cpp | 60 ++++--\n> > > src/libcamera/pipeline/virtual/virtual.h | 24 ++-\n> > > 9 files changed, 389 insertions(+), 35 deletions(-)\n> > > create mode 100644\n> > src/libcamera/pipeline/virtual/image_frame_generator.cpp\n> > > create mode 100644\n> > src/libcamera/pipeline/virtual/image_frame_generator.h\n> > > create mode 100644 src/libcamera/pipeline/virtual/utils.h\n> > >\n> > > diff --git a/src/libcamera/pipeline/virtual/README.md\n> > b/src/libcamera/pipeline/virtual/README.md\n> > > index ef80bb48..18c8341b 100644\n> > > --- a/src/libcamera/pipeline/virtual/README.md\n> > > +++ b/src/libcamera/pipeline/virtual/README.md\n> > > @@ -16,7 +16,13 @@ Each camera block is a dictionary, containing the\n> > following keys:\n> > > - `width` (`unsigned int`, default=1920): Width of the window\n> > resolution. This needs to be even.\n> > > - `height` (`unsigned int`, default=1080): Height of the window\n> > resolution.\n> > > - `frame_rates` (list of `int`, default=`[30,60]` ): Range of the\n> > frame rate (per second). If the list contains one value, it's the lower\n> > bound and the upper bound. If the list contains two values, the first is\n> > the lower bound and the second is the upper bound. No other number of\n> > values is allowed.\n> > > -- `test_pattern` (`string`): Which test pattern to use as frames. The\n> > options are \"bars\", \"lines\".\n> > > +- `test_pattern` (`string`): Which test pattern to use as frames. The\n> > 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\n> > series of images. Cannot be set with `test_pattern`.\n> > > + - The test patterns are \"bars\" which means color bars, and \"lines\"\n> > 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\n> > the directory are \"{n}.jpg\" with {n} is the sequence of images starting\n> > with 0.\n> > > + - `scale_mode`(`string`, default=\"fill\"): Scale mode when the frames\n> > are images. The only scale mode supported now is \"fill\". This does not\n> > affect the scale mode for now.\n> >\n> > Wouldn't it be more logical to add the frame generator support before\n> > the config file ? Otherwise you are adding pieces here to what has\n> > been added just one patch ago\n> >\n>\n> Yeah makes sense. Will be updated in v12.\n>\n>\n> >\n> > > - `location` (`string`, default=\"front\"): The location of the camera.\n> > Support \"front\" and \"back\". This is displayed in qcam camera selection\n> > window but this does not change the output.\n> > > - `model` (`string`, default=\"Unknown\"): The model name of the camera.\n> > This is displayed in qcam camera selection window but this does not change\n> > the output.\n> > >\n> > > @@ -37,6 +43,7 @@ This is the procedure of the Parser class:\n> > > 3. Parse each property and register the data.\n> > > - `parseSupportedFormats()`: Parses `supported_formats` in the\n> > config, which contains resolutions and frame rates.\n> > > - `parseTestPattern()`: Parses `test_pattern` in the config.\n> > > + - `parseFrame()`: Parses `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> > > diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.cpp\n> > b/src/libcamera/pipeline/virtual/image_frame_generator.cpp\n> > > new file mode 100644\n> > > index 00000000..db3efe15\n> > > --- /dev/null\n> > > +++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp\n> > > @@ -0,0 +1,178 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2024, Google Inc.\n> > > + *\n> > > + * image_frame_generator.cpp - Derived class of FrameGenerator for\n> > > + * generating frames from images\n> > > + */\n> > > +\n> > > +#include \"image_frame_generator.h\"\n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +#include \"libcamera/internal/mapped_framebuffer.h\"\n> > > +\n> > > +#include \"libyuv/convert.h\"\n> > > +#include \"libyuv/scale.h\"\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +LOG_DECLARE_CATEGORY(Virtual)\n> > > +\n> > > +/*\n> > > + * Factory function to create an ImageFrameGenerator object.\n> > > + * Read the images and convert them to buffers in NV12 format.\n> > > + * Store the pointers to the buffers to a list (imageFrameDatas)\n> > > + */\n> > > +std::unique_ptr\n> > > +ImageFrameGenerator::create(ImageFrames &imageFrames)\n> > > +{\n> > > + std::unique_ptr imageFrameGenerator =\n> > > + std::make_unique();\n> > > + imageFrameGenerator->imageFrames_ = &imageFrames;\n> > > +\n> > > + /*\n> > > + * For each file in the directory, load the image,\n> > > + * convert it to NV12, and store the pointer.\n> >\n> > weird indent\n> >\n>\n> Sorry, fixed.\n>\n>\n> >\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) +\n> > \".jpg\");\n> > > +\n> > > + File file(path);\n> > > + if (!file.open(File::OpenModeFlag::ReadOnly)) {\n> > > + LOG(Virtual, Error) << \"Failed to open image file\n> > \" << file.fileName()\n> > > + << \": \" <<\n> > strerror(file.error());\n> > > + return nullptr;\n> > > + }\n> > > +\n> > > + /* Read the image file to data */\n> > > + auto fileSize = file.size();\n> > > + auto buffer = std::make_unique(file.size());\n> > > + if (file.read({ buffer.get(),\n> > static_cast(fileSize) }) != fileSize) {\n> > > + LOG(Virtual, Error) << \"Failed to read file \" <<\n> > file.fileName()\n> > > + << \": \" <<\n> > strerror(file.error());\n> > > + return nullptr;\n> > > + }\n> > > +\n> > > + /* Get the width and height of the image */\n> > > + int width, height;\n> > > + if (libyuv::MJPGSize(buffer.get(), fileSize, &width,\n> > &height)) {\n> > > + LOG(Virtual, Error) << \"Failed to get the size of\n> > the image file: \"\n> > > + << file.fileName();\n> > > + return nullptr;\n> > > + }\n> > > +\n> > > + /* Convert to NV12 and write the data to tmpY and tmpUV */\n> > > + unsigned int halfWidth = (width + 1) / 2;\n> > > + unsigned int halfHeight = (height + 1) / 2;\n> >\n> > Do you need this for rounding ?\n>\n>\n> > > + std::unique_ptr dstY =\n> > > + std::make_unique(width * height);\n> > > + std::unique_ptr dstUV =\n> > > + std::make_unique(halfWidth * halfHeight\n> > * 2);\n> >\n> > otherwise using width * height / 2 would do\n> >\n>\n> Right, it works. Thanks!\n>\n\nYou probable need to take care about rounding, if you accept sizes not\n2-pixels aligned in your pipeline.\n\n>\n> >\n> > > + int ret = libyuv::MJPGToNV12(buffer.get(), fileSize,\n> > > + dstY.get(), width,\n> > dstUV.get(),\n> > > + width, width, height, width,\n> > height);\n> > > + if (ret != 0)\n> > > + LOG(Virtual, Error) << \"MJPGToNV12() failed with \"\n> > << ret;\n> > > +\n> > > + imageFrameGenerator->imageFrameDatas_.emplace_back(\n> > > + ImageFrameData{ std::move(dstY), std::move(dstUV),\n> > > + Size(width, height) });\n> > > + }\n> > > +\n> > > + return imageFrameGenerator;\n> > > +}\n> > > +\n> > > +/* Scale the buffers for image frames. */\n> > > +void ImageFrameGenerator::configure(const Size &size)\n> > > +{\n> > > + /* Reset the source images to prevent multiple configuration calls\n> > */\n> > > + scaledFrameDatas_.clear();\n> > > + frameCount_ = 0;\n> > > + parameter_ = 0;\n> > > +\n> > > + for (unsigned int i = 0; i < imageFrames_->number.value_or(1);\n> > 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> > > + std::unique_ptr scaledY =\n> > > + std::make_unique(size.width *\n> > size.height);\n> > > + std::unique_ptr scaledUV =\n> > > + std::make_unique(halfSizeWidth *\n> > halfSizeHeight * 2);\n> > > + auto &src = imageFrameDatas_[i];\n> > > +\n> > > + /*\n> > > + * \\todo Some platforms might enforce stride due to GPU,\n> > like\n> > > + * ChromeOS ciri (64). The weight needs to be a multiple of\n> > > + * the stride to work properly for now.\n> > > + */\n> > > + libyuv::NV12Scale(src.Y.get(), src.size.width,\n> > > + src.UV.get(), src.size.width,\n> > > + src.size.width, src.size.height,\n> > > + scaledY.get(), size.width,\n> > scaledUV.get(), size.width,\n> > > + size.width, size.height,\n> > libyuv::FilterMode::kFilterBilinear);\n> > > +\n> > > + scaledFrameDatas_.emplace_back(\n> > > + ImageFrameData{ std::move(scaledY),\n> > std::move(scaledUV), size });\n> > > + }\n> > > +}\n> > > +\n> > > +void ImageFrameGenerator::generateFrame(const Size &size, const\n> > FrameBuffer *buffer)\n> > > +{\n> > > + /* Don't do anything when the list of buffers is empty*/\n> > > + ASSERT(!scaledFrameDatas_.empty());\n> > > +\n> > > + MappedFrameBuffer mappedFrameBuffer(buffer,\n> > MappedFrameBuffer::MapFlag::Write);\n> > > +\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> > > +\n> > > + /* Write the scaledY and scaledUV to the mapped frame buffer */\n> > > + libyuv::NV12Copy(scaledFrameDatas_[frameCount_].Y.get(),\n> > size.width,\n> > > + scaledFrameDatas_[frameCount_].UV.get(),\n> > size.width, planes[0].begin(),\n> > > + size.width, planes[1].begin(), size.width,\n> > > + size.width, size.height);\n> > > +\n> > > + /* proceed an image every 4 frames */\n> > > + /* \\todo read the parameter_ from the configuration file? */\n> > > + parameter_++;\n> > > + if (parameter_ % 4 == 0)\n> >\n> > Make this a constexpr if not configurable\n> >\n> >\n> Done.\n>\n>\n> > > + frameCount_++;\n> > > +}\n> > > +\n> > > +/**\n> >\n> > We don't doxygen the pipelines, but doesn't hurt to have comments.\n> > Maybe remove the /**\n> >\n> >\n> Make them start with `/*` you mean?\n>\n\nYep!\n\n>\n> >\n> > > + * \\var ImageFrameGenerator::imageFrameDatas_\n> > > + * \\brief List of pointers to the not scaled image buffers\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var ImageFrameGenerator::scaledFrameDatas_\n> > > + * \\brief List of pointers to the scaled image buffers\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var ImageFrameGenerator::imageFrames_\n> > > + * \\brief Pointer to the imageFrames_ in VirtualCameraData\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var ImageFrameGenerator::parameter_\n> > > + * \\brief Speed parameter. Change to the next image every parameter_\n> > frames\n> > > + */\n> > > +\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.h\n> > b/src/libcamera/pipeline/virtual/image_frame_generator.h\n> > > new file mode 100644\n> > > index 00000000..4ad8aad2\n> > > --- /dev/null\n> > > +++ b/src/libcamera/pipeline/virtual/image_frame_generator.h\n> > > @@ -0,0 +1,54 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2024, Google Inc.\n> > > + *\n> > > + * image_frame_generator.h - Derived class of FrameGenerator for\n> > > + * generating frames from images\n> > > + */\n> > > +\n> > > +#pragma once\n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \"frame_generator.h\"\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +enum class ScaleMode : char {\n> > > + Fill = 0,\n> > > +};\n> >\n> > I know you want more scale modes, but as long as you only support one\n> > there's no need for a type\n> >\n>\n> Hmm, do you think it's better to just remove the argument in the config\n> file?\n>\n\nWhat do you think ? if you can't configure in this version I would\nleave it out and simplify the implementation for this first version\n>\n> >\n> > > +\n> > > +/* Frame configuration provided by the config file */\n> > > +struct ImageFrames {\n> > > + std::filesystem::path path;\n> > > + ScaleMode scaleMode;\n> >\n> > As this can only be \"Fill\", nothing else is valid atm\n> >\n> > > + std::optional number;\n> > > +};\n> > > +\n> > > +class ImageFrameGenerator : public FrameGenerator\n> > > +{\n> > > +public:\n> > > + static std::unique_ptr create(ImageFrames\n> > &imageFrames);\n> > > +\n> > > +private:\n> > > + struct ImageFrameData {\n> > > + std::unique_ptr Y;\n> > > + std::unique_ptr UV;\n> > > + Size size;\n> > > + };\n> > > +\n> > > + void configure(const Size &size) override;\n> > > + void generateFrame(const Size &size, const FrameBuffer *buffer)\n> > override;\n> > > +\n> > > + std::vector imageFrameDatas_;\n> > > + std::vector scaledFrameDatas_;\n> > > + ImageFrames *imageFrames_;\n> > > + unsigned int frameCount_;\n> > > + unsigned int parameter_;\n> > > +};\n> > > +\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/libcamera/pipeline/virtual/meson.build\n> > b/src/libcamera/pipeline/virtual/meson.build\n> > > index d72ac5be..395919b3 100644\n> > > --- a/src/libcamera/pipeline/virtual/meson.build\n> > > +++ b/src/libcamera/pipeline/virtual/meson.build\n> > > @@ -1,9 +1,13 @@\n> > > # SPDX-License-Identifier: CC0-1.0\n> > >\n> > > libcamera_internal_sources += files([\n> > > + 'image_frame_generator.cpp',\n> > > 'parser.cpp',\n> > > 'test_pattern_generator.cpp',\n> > > 'virtual.cpp',\n> > > ])\n> > >\n> > > +libjpeg = dependency('libjpeg', required : false)\n> > > +\n> > > libcamera_deps += [libyuv_dep]\n> > > +libcamera_deps += [libjpeg]\n> > > diff --git a/src/libcamera/pipeline/virtual/parser.cpp\n> > b/src/libcamera/pipeline/virtual/parser.cpp\n> > > index d861a52a..5076e71c 100644\n> > > --- a/src/libcamera/pipeline/virtual/parser.cpp\n> > > +++ b/src/libcamera/pipeline/virtual/parser.cpp\n> > > @@ -52,12 +52,12 @@ Parser::parseConfigFile(File &file, PipelineHandler\n> > *pipe)\n> > > continue;\n> > > }\n> > >\n> > > - data->id_ = cameraId;\n> > > + data->config_.id = cameraId;\n> > > ControlInfoMap::Map controls;\n> > > /* todo: Check which resolution's frame rate to be\n> > reported */\n> > > controls[&controls::FrameDurationLimits] =\n> > > - ControlInfo(int64_t(1000000 /\n> > data->supportedResolutions_[0].frameRates[1]),\n> > > - int64_t(1000000 /\n> > data->supportedResolutions_[0].frameRates[0]));\n> > > + ControlInfo(int64_t(1000000 /\n> > data->config_.resolutions[0].frameRates[1]),\n> > > + int64_t(1000000 /\n> > data->config_.resolutions[0].frameRates[0]));\n> > > data->controlInfo_ = ControlInfoMap(std::move(controls),\n> > controls::controls);\n> > > configurations.push_back(std::move(data));\n> > > }\n> > > @@ -75,7 +75,8 @@ Parser::parseCameraConfigData(const YamlObject\n> > &cameraConfigData,\n> > > std::unique_ptr data =\n> > > std::make_unique(pipe, resolutions);\n> > >\n> > > - if (parseTestPattern(cameraConfigData, data.get()))\n> > > + if (parseTestPattern(cameraConfigData, data.get()) &&\n> > > + parseFrame(cameraConfigData, data.get()))\n> >\n> > This is fragile and only works because if parseTestPattern() returns 0\n> > then parseFrame() is never called\n>\n>\n> > > return nullptr;\n> > >\n> > > if (parseLocation(cameraConfigData, data.get()))\n> > > @@ -148,16 +149,75 @@ int Parser::parseTestPattern(const YamlObject\n> > &cameraConfigData, VirtualCameraDa\n> > > {\n> > > std::string testPattern =\n> > cameraConfigData[\"test_pattern\"].get(\"\");\n> > >\n> > > - /* Default value is \"bars\" */\n> > > if (testPattern == \"bars\") {\n> > > - data->testPattern_ = TestPattern::ColorBars;\n> > > + data->config_.frame = TestPattern::ColorBars;\n> > > } else if (testPattern == \"lines\") {\n> > > - data->testPattern_ = TestPattern::DiagonalLines;\n> > > + data->config_.frame = TestPattern::DiagonalLines;\n> > > } else {\n> > > - LOG(Virtual, Error) << \"Test pattern: \" << testPattern\n> > > + LOG(Virtual, Debug) << \"Test pattern: \" << testPattern\n> > > << \"is not supported\";\n> > > return -EINVAL;\n> > > }\n> > > +\n> > > + return 0;\n> > > +}\n> > > +\n> > > +int Parser::parseFrame(const YamlObject &cameraConfigData,\n> > VirtualCameraData *data)\n> >\n> > You can unify these two functions.\n> >\n> > Get \"testPattern\", if valid make sure it's only either \"bars\" or\n> > \"lines\" and fail if it is specified but has an unsupported value.\n> >\n> > Then check for \"frames\". If both \"frames\" and \"testPattern\" are\n> > specified, it's an error. Then parse \"frames\" like you do here.\n> >\n>\n> Makes sense. Thanks! Updated.\n>\n>\n> >\n> > > +{\n> > > + const YamlObject &frames = cameraConfigData[\"frames\"];\n> > > +\n> > > + /* When there is no frames provided in the config file, use color\n> > bar test pattern */\n> > > + if (frames.size() == 0) {\n> > > + data->config_.frame = TestPattern::ColorBars;\n> > > + return 0;\n> > > + }\n> > > +\n> > > + if (!frames.isDictionary()) {\n> > > + LOG(Virtual, Error) << \"'frames' is not a dictionary.\";\n> > > + return -EINVAL;\n> > > + }\n> > > +\n> > > + std::string path = frames[\"path\"].get(\"\");\n> > > +\n> > > + ScaleMode scaleMode;\n> > > + if (auto ext = std::filesystem::path(path).extension();\n> > > + ext == \".jpg\" || ext == \".jpeg\") {\n> > > + if (parseScaleMode(frames, &scaleMode))\n> > > + return -EINVAL;\n> > > + data->config_.frame = ImageFrames{ path, scaleMode,\n> > std::nullopt };\n> > > + } else if\n> > (std::filesystem::is_directory(std::filesystem::symlink_status(path))) {\n> > > + if (parseScaleMode(frames, &scaleMode))\n> > > + return -EINVAL;\n> >\n> > Could you parse scale mode before checking the file extensions ?\n> >\n> >\n> Done.\n>\n>\n> > > +\n> > > + using std::filesystem::directory_iterator;\n> > > + unsigned int numOfFiles =\n> > std::distance(directory_iterator(path), directory_iterator{});\n> > > + if (numOfFiles == 0) {\n> > > + LOG(Virtual, Error) << \"Empty directory\";\n> > > + return -EINVAL;\n> > > + }\n> > > + data->config_.frame = ImageFrames{ path, scaleMode,\n> > numOfFiles };\n> > > + } else {\n> > > + LOG(Virtual, Error) << \"Frame: \" << path << \" is not\n> > supported\";\n> > > + return -EINVAL;\n> > > + }\n> > > +\n> > > + return 0;\n> > > +}\n> > > +\n> > > +int Parser::parseScaleMode(\n> > > + const YamlObject &framesConfigData, ScaleMode *scaleMode)\n> >\n> > weird indent\n> >\n>\n> Updated.\n>\n>\n> >\n> > > +{\n> > > + std::string mode =\n> > framesConfigData[\"scale_mode\"].get(\"\");\n> > > +\n> > > + /* Default value is fill */\n> > > + if (mode == \"fill\" || mode == \"\") {\n> > > + *scaleMode = ScaleMode::Fill;\n> >\n> > You don't need a type as suggested above, you can either assume \"Fill\"\n> > or fail during parsing.\n> >\n> > > + } else {\n> > > + LOG(Virtual, Error) << \"scaleMode: \" << mode\n> > > + << \" is not supported\";\n> > > + return -EINVAL;\n> > > + }\n> > > +\n> > > return 0;\n> > > }\n> > >\n> > > diff --git a/src/libcamera/pipeline/virtual/parser.h\n> > b/src/libcamera/pipeline/virtual/parser.h\n> > > index 09c3c56b..f65616e3 100644\n> > > --- a/src/libcamera/pipeline/virtual/parser.h\n> > > +++ b/src/libcamera/pipeline/virtual/parser.h\n> > > @@ -35,8 +35,10 @@ private:\n> > > int parseSupportedFormats(const YamlObject &cameraConfigData,\n> > >\n> > std::vector *resolutions);\n> > > int parseTestPattern(const YamlObject &cameraConfigData,\n> > VirtualCameraData *data);\n> > > + int parseFrame(const YamlObject &cameraConfigData,\n> > VirtualCameraData *data);\n> > > int parseLocation(const YamlObject &cameraConfigData,\n> > VirtualCameraData *data);\n> > > int parseModel(const YamlObject &cameraConfigData,\n> > VirtualCameraData *data);\n> > > + int parseScaleMode(const YamlObject &framesConfigData, ScaleMode\n> > *scaleMode);\n> > > };\n> > >\n> > > } /* namespace libcamera */\n> > > diff --git a/src/libcamera/pipeline/virtual/utils.h\n> > b/src/libcamera/pipeline/virtual/utils.h\n> > > new file mode 100644\n> > > index 00000000..43a14d4b\n> > > --- /dev/null\n> > > +++ b/src/libcamera/pipeline/virtual/utils.h\n> > > @@ -0,0 +1,17 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2024, Google Inc.\n> > > + *\n> > > + * utils.h - Utility types for Virtual Pipeline Handler\n> > > + */\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +template\n> > > +struct overloaded : Ts... {\n> > > + using Ts::operator()...;\n> > > +};\n> > > +template\n> > > +overloaded(Ts...) -> overloaded;\n> > > +\n> >\n> > Does using a standard construct like std::visit a custom definition\n> >\n>\n> Sorry, I don't get what you mean.\n\nI was wondering why you need this to use a construct from stdlib, then\nI read the example at\nhttps://en.cppreference.com/w/cpp/utility/variant/visit\n\n>\n>\n> > like this one ? Also, this header is included form a single cpp file,\n> > move its content there if you can.\n> >\n> >\n> Hmm, I was suggested otherwise:\n>\n\nI see, but what you think ? What is the point of an header file\nwithout any other file including it but one, with just this small\nhelpers above ?\n\n\n> https://patchwork.libcamera.org/patch/20974/\n> ```\n>\n> Apart from the indentation looking a bit off, it seems ok.\n>\n> I think the `overloaded` type could go into `utils.h` or similar.\n>\n>\n> Regards,\n> Barnabás Pőcze\n>\n> ```\n>\n> I'm new to std::visit, so...\n>\n\nNot very much related to std::visit but the usage of an header when\nimho it shouldn't be necessary\n\n>\n> >\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/libcamera/pipeline/virtual/virtual.cpp\n> > b/src/libcamera/pipeline/virtual/virtual.cpp\n> > > index 55bc30df..98aed412 100644\n> > > --- a/src/libcamera/pipeline/virtual/virtual.cpp\n> > > +++ b/src/libcamera/pipeline/virtual/virtual.cpp\n> > > @@ -27,6 +27,7 @@\n> > > #include \"libcamera/internal/yaml_parser.h\"\n> > >\n> > > #include \"parser.h\"\n> > > +#include \"utils.h\"\n> > >\n> > > namespace libcamera {\n> > >\n> > > @@ -49,17 +50,18 @@ uint64_t currentTimestamp()\n> > >\n> > > VirtualCameraData::VirtualCameraData(PipelineHandler *pipe,\n> > > std::vector\n> > supportedResolutions)\n> > > - : Camera::Private(pipe),\n> > supportedResolutions_(std::move(supportedResolutions))\n> > > + : Camera::Private(pipe)\n> > > {\n> > > - for (const auto &resolution : supportedResolutions_) {\n> > > - if (minResolutionSize_.isNull() || minResolutionSize_ >\n> > resolution.size)\n> > > - minResolutionSize_ = resolution.size;\n> > > + config_.resolutions = std::move(supportedResolutions);\n> > > + for (const auto &resolution : config_.resolutions) {\n> > > + if (config_.minResolutionSize.isNull() ||\n> > config_.minResolutionSize > resolution.size)\n> > > + config_.minResolutionSize = resolution.size;\n> > >\n> > > - maxResolutionSize_ = std::max(maxResolutionSize_,\n> > resolution.size);\n> > > + config_.maxResolutionSize =\n> > std::max(config_.maxResolutionSize, resolution.size);\n> > > }\n> > >\n> > > properties_.set(properties::PixelArrayActiveAreas,\n> > > - { Rectangle(maxResolutionSize_) });\n> > > + { Rectangle(config_.maxResolutionSize) });\n> > >\n> > > /* \\todo Support multiple streams and pass multi_stream_test */\n> > > streamConfigs_.resize(kMaxStream);\n> > > @@ -87,7 +89,7 @@ CameraConfiguration::Status\n> > VirtualCameraConfiguration::validate()\n> > >\n> > > for (StreamConfiguration &cfg : config_) {\n> > > bool found = false;\n> > > - for (const auto &resolution :\n> > 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> > > @@ -102,7 +104,7 @@ CameraConfiguration::Status\n> > 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> > > @@ -145,11 +147,11 @@\n> > PipelineHandlerVirtual::generateConfiguration(Camera *camera,\n> > > for (const StreamRole role : roles) {\n> > > std::map>\n> > streamFormats;\n> > > PixelFormat pixelFormat = formats::NV12;\n> > > - streamFormats[pixelFormat] = { { data->minResolutionSize_,\n> > data->maxResolutionSize_ } };\n> > > + streamFormats[pixelFormat] = { {\n> > data->config_.minResolutionSize, data->config_.maxResolutionSize } };\n> > > StreamFormats formats(streamFormats);\n> > > StreamConfiguration cfg(formats);\n> > > cfg.pixelFormat = pixelFormat;\n> > > - cfg.size = data->maxResolutionSize_;\n> > > + cfg.size = data->config_.maxResolutionSize;\n> > > cfg.bufferCount = VirtualCameraConfiguration::kBufferCount;\n> > >\n> > > switch (role) {\n> > > @@ -181,6 +183,7 @@ int PipelineHandlerVirtual::configure(Camera *camera,\n> > > VirtualCameraData *data = cameraData(camera);\n> > > for (size_t i = 0; i < config->size(); ++i) {\n> > > config->at(i).setStream(&data->streamConfigs_[i].stream);\n> > > + /* Start reading the images/generating test patterns */\n> > > data->streamConfigs_[i].frameGenerator->configure(\n> > >\n> > data->streamConfigs_[i].stream.configuration().size);\n> > > }\n> > > @@ -274,10 +277,14 @@ bool\n> > PipelineHandlerVirtual::match([[maybe_unused]] DeviceEnumerator *enumerator\n> > > std::set streams;\n> > > for (auto &streamConfig : data->streamConfigs_)\n> > > streams.insert(&streamConfig.stream);\n> > > - std::string id = data->id_;\n> > > + std::string id = data->config_.id;\n> > > std::shared_ptr camera =\n> > Camera::create(std::move(data), id, streams);\n> > >\n> > > - initFrameGenerator(camera.get());\n> > > + if (!initFrameGenerator(camera.get())) {\n> > > + LOG(Virtual, Error) << \"Failed to initialize frame\n> > \"\n> > > + << \"generator for camera: \" <<\n> > id;\n> > > + continue;\n> > > + }\n> > >\n> > > registerCamera(std::move(camera));\n> > > }\n> > > @@ -285,15 +292,30 @@ bool\n> > PipelineHandlerVirtual::match([[maybe_unused]] DeviceEnumerator *enumerator\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 =\n> > std::make_unique();\n> > > - else\n> > > - streamConfig.frameGenerator =\n> > std::make_unique();\n> > > - }\n> > > + auto &frame = data->config_.frame;\n> > > + std::visit(overloaded{\n> > > + [&](TestPattern &testPattern) {\n> > > + for (auto &streamConfig :\n> > data->streamConfigs_) {\n> > > + if (testPattern ==\n> > TestPattern::DiagonalLines)\n> > > +\n> > streamConfig.frameGenerator = std::make_unique();\n> > > + else\n> > > +\n> > streamConfig.frameGenerator = std::make_unique();\n> > > + }\n> > > + },\n> > > + [&](ImageFrames &imageFrames) {\n> > > + for (auto &streamConfig :\n> > data->streamConfigs_)\n> > > + streamConfig.frameGenerator =\n> > 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\n> > b/src/libcamera/pipeline/virtual/virtual.h\n> > > index 8830e00f..efa97e88 100644\n> > > --- a/src/libcamera/pipeline/virtual/virtual.h\n> > > +++ b/src/libcamera/pipeline/virtual/virtual.h\n> > > @@ -8,6 +8,8 @@\n> > > #pragma once\n> > >\n> > > #include \n> > > +#include \n> > > +#include \n> > > #include \n> > >\n> > > #include \n> > > @@ -16,10 +18,14 @@\n> > > #include \"libcamera/internal/dma_buf_allocator.h\"\n> > > #include \"libcamera/internal/pipeline_handler.h\"\n> > >\n> > > +#include \"frame_generator.h\"\n> > > +#include \"image_frame_generator.h\"\n> > > #include \"test_pattern_generator.h\"\n> > >\n> > > namespace libcamera {\n> > >\n> > > +using VirtualFrame = std::variant;\n> > > +\n> > > class VirtualCameraData : public Camera::Private\n> > > {\n> > > public:\n> > > @@ -33,18 +39,22 @@ public:\n> > > Stream stream;\n> > > std::unique_ptr frameGenerator;\n> > > };\n> > > + /* The config file is parsed to the Configuration struct */\n> > > + struct Configuration {\n> > > + std::string id;\n> > > + std::vector resolutions;\n> > > + VirtualFrame frame;\n> > > +\n> > > + Size maxResolutionSize;\n> > > + Size minResolutionSize;\n> > > + };\n> > >\n> > > VirtualCameraData(PipelineHandler *pipe,\n> > > std::vector supportedResolutions);\n> > >\n> > > ~VirtualCameraData() = default;\n> > >\n> > > - std::string id_;\n> > > - TestPattern testPattern_ = TestPattern::ColorBars;\n> > > -\n> > > - const std::vector supportedResolutions_;\n> > > - Size maxResolutionSize_;\n> > > - Size minResolutionSize_;\n> > > + Configuration config_;\n> > >\n> > > std::vector streamConfigs_;\n> > > };\n> > > @@ -89,7 +99,7 @@ private:\n> > > return static_cast(camera->_d());\n> > > }\n> > >\n> > > - void initFrameGenerator(Camera *camera);\n> > > + bool initFrameGenerator(Camera *camera);\n> > >\n> > > DmaBufAllocator dmaBufAllocator_;\n> > > };\n> > > --\n> > > 2.46.0.469.g59c65b2a67-goog\n> > >\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 4F6EDC324C\n\tfor ;\n\tMon, 9 Sep 2024 20:26:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3B6A2634F7;\n\tMon, 9 Sep 2024 22:26:54 +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 25303634EB\n\tfor ;\n\tMon, 9 Sep 2024 22:26:52 +0200 (CEST)","from ideasonboard.com (213-229-8-243.static.upcbusiness.at\n\t[213.229.8.243])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 73407827;\n\tMon, 9 Sep 2024 22:25:35 +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=\"csXVddWI\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1725913535;\n\tbh=E02SpQ6YI8IGckp5XJOO9QwqlvStuAFw4N54ysKvrnQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=csXVddWISCvwBrKfQOQr+s20tX5zp+xRuOyvHkHPzvLJAUBPB66cqtNzL84nGONZQ\n\tS3ha0f2OMoTQ/+i8KVcK1iFFisqVr9yO8TXoMptjCWJmb06A8Y6tx2UqYMHEm6olFU\n\tlSF/AFOmiF8grQkcDDwpdMRNFZCSaIH/WfPEnxUo=","Date":"Mon, 9 Sep 2024 22:26:48 +0200","From":"Jacopo Mondi ","To":"Cheng-Hao Yang ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org, Konami Shu , \n\tYunke Cao , Tomasz Figa ","Subject":"Re: [PATCH v11 6/7] libcamera: virtual: Add ImageFrameGenerator","Message-ID":"","References":"<20240907143110.2210711-1-chenghaoyang@google.com>\n\t<20240907143110.2210711-7-chenghaoyang@google.com>\n\t<3yzin5r3r3nmawjocyrakt3aq6xsys2fnsjwobddfid7yxwjxk@gc6abloh6zas>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31152,"web_url":"https://patchwork.libcamera.org/comment/31152/","msgid":"","date":"2024-09-10T04:50:54","subject":"Re: [PATCH v11 6/7] libcamera: virtual: Add ImageFrameGenerator","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi Jacopo,\n\nOn Tue, Sep 10, 2024 at 4:26 AM Jacopo Mondi \nwrote:\n\n> Hi Harvey\n>\n> On Mon, Sep 09, 2024 at 11:22:47PM GMT, Cheng-Hao Yang wrote:\n> > Hi Jacopo,\n> >\n> > On Mon, Sep 9, 2024 at 6:17 PM Jacopo Mondi <\n> jacopo.mondi@ideasonboard.com>\n> > wrote:\n> >\n> > > Hi\n> > >\n> > > On Sat, Sep 07, 2024 at 02:28:31PM GMT, Harvey Yang wrote:\n> > > > From: Konami Shu \n> > > >\n> > > > Besides TestPatternGenerator, this patch adds ImageFrameGenerator\n> that\n> > > > loads real images (jpg / jpeg for now) as the source and generates\n> > > > scaled frames.\n> > > >\n> > > > Signed-off-by: Konami Shu \n> > > > Co-developed-by: Harvey Yang \n> > > > Co-developed-by: Yunke Cao \n> > > > Co-developed-by: Tomasz Figa \n> > > > ---\n> > > > src/libcamera/pipeline/virtual/README.md | 9 +-\n> > > > .../virtual/image_frame_generator.cpp | 178\n> ++++++++++++++++++\n> > > > .../pipeline/virtual/image_frame_generator.h | 54 ++++++\n> > > > src/libcamera/pipeline/virtual/meson.build | 4 +\n> > > > src/libcamera/pipeline/virtual/parser.cpp | 76 +++++++-\n> > > > src/libcamera/pipeline/virtual/parser.h | 2 +\n> > > > src/libcamera/pipeline/virtual/utils.h | 17 ++\n> > > > src/libcamera/pipeline/virtual/virtual.cpp | 60 ++++--\n> > > > src/libcamera/pipeline/virtual/virtual.h | 24 ++-\n> > > > 9 files changed, 389 insertions(+), 35 deletions(-)\n> > > > create mode 100644\n> > > src/libcamera/pipeline/virtual/image_frame_generator.cpp\n> > > > create mode 100644\n> > > src/libcamera/pipeline/virtual/image_frame_generator.h\n> > > > create mode 100644 src/libcamera/pipeline/virtual/utils.h\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/virtual/README.md\n> > > b/src/libcamera/pipeline/virtual/README.md\n> > > > index ef80bb48..18c8341b 100644\n> > > > --- a/src/libcamera/pipeline/virtual/README.md\n> > > > +++ b/src/libcamera/pipeline/virtual/README.md\n> > > > @@ -16,7 +16,13 @@ Each camera block is a dictionary, containing the\n> > > following keys:\n> > > > - `width` (`unsigned int`, default=1920): Width of the window\n> > > resolution. This needs to be even.\n> > > > - `height` (`unsigned int`, default=1080): Height of the window\n> > > resolution.\n> > > > - `frame_rates` (list of `int`, default=`[30,60]` ): Range of\n> the\n> > > frame rate (per second). If the list contains one value, it's the lower\n> > > bound and the upper bound. If the list contains two values, the first\n> is\n> > > the lower bound and the second is the upper bound. No other number of\n> > > values is allowed.\n> > > > -- `test_pattern` (`string`): Which test pattern to use as frames.\n> The\n> > > options are \"bars\", \"lines\".\n> > > > +- `test_pattern` (`string`): Which test pattern to use as frames.\n> The\n> > > 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\n> > > series of images. Cannot be set with `test_pattern`.\n> > > > + - The test patterns are \"bars\" which means color bars, and\n> \"lines\"\n> > > 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\n> in\n> > > the directory are \"{n}.jpg\" with {n} is the sequence of images starting\n> > > with 0.\n> > > > + - `scale_mode`(`string`, default=\"fill\"): Scale mode when the\n> frames\n> > > are images. The only scale mode supported now is \"fill\". This does not\n> > > affect the scale mode for now.\n> > >\n> > > Wouldn't it be more logical to add the frame generator support before\n> > > the config file ? Otherwise you are adding pieces here to what has\n> > > been added just one patch ago\n> > >\n> >\n> > Yeah makes sense. Will be updated in v12.\n> >\n> >\n> > >\n> > > > - `location` (`string`, default=\"front\"): The location of the\n> camera.\n> > > Support \"front\" and \"back\". This is displayed in qcam camera selection\n> > > window but this does not change the output.\n> > > > - `model` (`string`, default=\"Unknown\"): The model name of the\n> camera.\n> > > This is displayed in qcam camera selection window but this does not\n> change\n> > > the output.\n> > > >\n> > > > @@ -37,6 +43,7 @@ This is the procedure of the Parser class:\n> > > > 3. Parse each property and register the data.\n> > > > - `parseSupportedFormats()`: Parses `supported_formats` in the\n> > > config, which contains resolutions and frame rates.\n> > > > - `parseTestPattern()`: Parses `test_pattern` in the config.\n> > > > + - `parseFrame()`: Parses `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> > > > diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.cpp\n> > > b/src/libcamera/pipeline/virtual/image_frame_generator.cpp\n> > > > new file mode 100644\n> > > > index 00000000..db3efe15\n> > > > --- /dev/null\n> > > > +++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp\n> > > > @@ -0,0 +1,178 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2024, Google Inc.\n> > > > + *\n> > > > + * image_frame_generator.cpp - Derived class of FrameGenerator for\n> > > > + * generating frames from images\n> > > > + */\n> > > > +\n> > > > +#include \"image_frame_generator.h\"\n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +\n> > > > +#include \"libcamera/internal/mapped_framebuffer.h\"\n> > > > +\n> > > > +#include \"libyuv/convert.h\"\n> > > > +#include \"libyuv/scale.h\"\n> > > > +\n> > > > +namespace libcamera {\n> > > > +\n> > > > +LOG_DECLARE_CATEGORY(Virtual)\n> > > > +\n> > > > +/*\n> > > > + * Factory function to create an ImageFrameGenerator object.\n> > > > + * Read the images and convert them to buffers in NV12 format.\n> > > > + * Store the pointers to the buffers to a list (imageFrameDatas)\n> > > > + */\n> > > > +std::unique_ptr\n> > > > +ImageFrameGenerator::create(ImageFrames &imageFrames)\n> > > > +{\n> > > > + std::unique_ptr imageFrameGenerator =\n> > > > + std::make_unique();\n> > > > + imageFrameGenerator->imageFrames_ = &imageFrames;\n> > > > +\n> > > > + /*\n> > > > + * For each file in the directory, load the image,\n> > > > + * convert it to NV12, and store the pointer.\n> > >\n> > > weird indent\n> > >\n> >\n> > Sorry, fixed.\n> >\n> >\n> > >\n> > > > + */\n> > > > + for (unsigned int i = 0; i < imageFrames.number.value_or(1);\n> 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) +\n> > > \".jpg\");\n> > > > +\n> > > > + File file(path);\n> > > > + if (!file.open(File::OpenModeFlag::ReadOnly)) {\n> > > > + LOG(Virtual, Error) << \"Failed to open image\n> file\n> > > \" << file.fileName()\n> > > > + << \": \" <<\n> > > strerror(file.error());\n> > > > + return nullptr;\n> > > > + }\n> > > > +\n> > > > + /* Read the image file to data */\n> > > > + auto fileSize = file.size();\n> > > > + auto buffer = std::make_unique(file.size());\n> > > > + if (file.read({ buffer.get(),\n> > > static_cast(fileSize) }) != fileSize) {\n> > > > + LOG(Virtual, Error) << \"Failed to read file \"\n> <<\n> > > file.fileName()\n> > > > + << \": \" <<\n> > > strerror(file.error());\n> > > > + return nullptr;\n> > > > + }\n> > > > +\n> > > > + /* Get the width and height of the image */\n> > > > + int width, height;\n> > > > + if (libyuv::MJPGSize(buffer.get(), fileSize, &width,\n> > > &height)) {\n> > > > + LOG(Virtual, Error) << \"Failed to get the size\n> of\n> > > the image file: \"\n> > > > + << file.fileName();\n> > > > + return nullptr;\n> > > > + }\n> > > > +\n> > > > + /* Convert to NV12 and write the data to tmpY and\n> tmpUV */\n> > > > + unsigned int halfWidth = (width + 1) / 2;\n> > > > + unsigned int halfHeight = (height + 1) / 2;\n> > >\n> > > Do you need this for rounding ?\n> >\n> >\n> > > > + std::unique_ptr dstY =\n> > > > + std::make_unique(width * height);\n> > > > + std::unique_ptr dstUV =\n> > > > + std::make_unique(halfWidth *\n> halfHeight\n> > > * 2);\n> > >\n> > > otherwise using width * height / 2 would do\n> > >\n> >\n> > Right, it works. Thanks!\n> >\n>\n> You probable need to take care about rounding, if you accept sizes not\n> 2-pixels aligned in your pipeline.\n>\n\nThe parser only accepts an even number for width, and I've tried an odd\nnumber for height, which still works.\n\n\n>\n> >\n> > >\n> > > > + int ret = libyuv::MJPGToNV12(buffer.get(), fileSize,\n> > > > + dstY.get(), width,\n> > > dstUV.get(),\n> > > > + width, width, height,\n> width,\n> > > height);\n> > > > + if (ret != 0)\n> > > > + LOG(Virtual, Error) << \"MJPGToNV12() failed\n> with \"\n> > > << ret;\n> > > > +\n> > > > + imageFrameGenerator->imageFrameDatas_.emplace_back(\n> > > > + ImageFrameData{ std::move(dstY),\n> std::move(dstUV),\n> > > > + Size(width, height) });\n> > > > + }\n> > > > +\n> > > > + return imageFrameGenerator;\n> > > > +}\n> > > > +\n> > > > +/* Scale the buffers for image frames. */\n> > > > +void ImageFrameGenerator::configure(const Size &size)\n> > > > +{\n> > > > + /* Reset the source images to prevent multiple configuration\n> calls\n> > > */\n> > > > + scaledFrameDatas_.clear();\n> > > > + frameCount_ = 0;\n> > > > + parameter_ = 0;\n> > > > +\n> > > > + for (unsigned int i = 0; i < imageFrames_->number.value_or(1);\n> > > i++) {\n> > > > + /* Scale the imageFrameDatas_ to scaledY and scaledUV\n> */\n> > > > + unsigned int halfSizeWidth = (size.width + 1) / 2;\n> > > > + unsigned int halfSizeHeight = (size.height + 1) / 2;\n> > > > + std::unique_ptr scaledY =\n> > > > + std::make_unique(size.width *\n> > > size.height);\n> > > > + std::unique_ptr scaledUV =\n> > > > + std::make_unique(halfSizeWidth *\n> > > halfSizeHeight * 2);\n> > > > + auto &src = imageFrameDatas_[i];\n> > > > +\n> > > > + /*\n> > > > + * \\todo Some platforms might enforce stride due to\n> GPU,\n> > > like\n> > > > + * ChromeOS ciri (64). The weight needs to be a\n> multiple of\n> > > > + * the stride to work properly for now.\n> > > > + */\n> > > > + libyuv::NV12Scale(src.Y.get(), src.size.width,\n> > > > + src.UV.get(), src.size.width,\n> > > > + src.size.width, src.size.height,\n> > > > + scaledY.get(), size.width,\n> > > scaledUV.get(), size.width,\n> > > > + size.width, size.height,\n> > > libyuv::FilterMode::kFilterBilinear);\n> > > > +\n> > > > + scaledFrameDatas_.emplace_back(\n> > > > + ImageFrameData{ std::move(scaledY),\n> > > std::move(scaledUV), size });\n> > > > + }\n> > > > +}\n> > > > +\n> > > > +void ImageFrameGenerator::generateFrame(const Size &size, const\n> > > FrameBuffer *buffer)\n> > > > +{\n> > > > + /* Don't do anything when the list of buffers is empty*/\n> > > > + ASSERT(!scaledFrameDatas_.empty());\n> > > > +\n> > > > + MappedFrameBuffer mappedFrameBuffer(buffer,\n> > > MappedFrameBuffer::MapFlag::Write);\n> > > > +\n> > > > + auto planes = mappedFrameBuffer.planes();\n> > > > +\n> > > > + /* Make sure the frameCount does not over the number of images\n> */\n> > > > + frameCount_ %= imageFrames_->number.value_or(1);\n> > > > +\n> > > > + /* Write the scaledY and scaledUV to the mapped frame buffer */\n> > > > + libyuv::NV12Copy(scaledFrameDatas_[frameCount_].Y.get(),\n> > > size.width,\n> > > > + scaledFrameDatas_[frameCount_].UV.get(),\n> > > size.width, planes[0].begin(),\n> > > > + size.width, planes[1].begin(), size.width,\n> > > > + size.width, size.height);\n> > > > +\n> > > > + /* proceed an image every 4 frames */\n> > > > + /* \\todo read the parameter_ from the configuration file? */\n> > > > + parameter_++;\n> > > > + if (parameter_ % 4 == 0)\n> > >\n> > > Make this a constexpr if not configurable\n> > >\n> > >\n> > Done.\n> >\n> >\n> > > > + frameCount_++;\n> > > > +}\n> > > > +\n> > > > +/**\n> > >\n> > > We don't doxygen the pipelines, but doesn't hurt to have comments.\n> > > Maybe remove the /**\n> > >\n> > >\n> > Make them start with `/*` you mean?\n> >\n>\n> Yep!\n>\n> >\n> > >\n> > > > + * \\var ImageFrameGenerator::imageFrameDatas_\n> > > > + * \\brief List of pointers to the not scaled image buffers\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\var ImageFrameGenerator::scaledFrameDatas_\n> > > > + * \\brief List of pointers to the scaled image buffers\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\var ImageFrameGenerator::imageFrames_\n> > > > + * \\brief Pointer to the imageFrames_ in VirtualCameraData\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\var ImageFrameGenerator::parameter_\n> > > > + * \\brief Speed parameter. Change to the next image every parameter_\n> > > frames\n> > > > + */\n> > > > +\n> > > > +} /* namespace libcamera */\n> > > > diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.h\n> > > b/src/libcamera/pipeline/virtual/image_frame_generator.h\n> > > > new file mode 100644\n> > > > index 00000000..4ad8aad2\n> > > > --- /dev/null\n> > > > +++ b/src/libcamera/pipeline/virtual/image_frame_generator.h\n> > > > @@ -0,0 +1,54 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2024, Google Inc.\n> > > > + *\n> > > > + * image_frame_generator.h - Derived class of FrameGenerator for\n> > > > + * generating frames from images\n> > > > + */\n> > > > +\n> > > > +#pragma once\n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > +#include \"frame_generator.h\"\n> > > > +\n> > > > +namespace libcamera {\n> > > > +\n> > > > +enum class ScaleMode : char {\n> > > > + Fill = 0,\n> > > > +};\n> > >\n> > > I know you want more scale modes, but as long as you only support one\n> > > there's no need for a type\n> > >\n> >\n> > Hmm, do you think it's better to just remove the argument in the config\n> > file?\n> >\n>\n> What do you think ? if you can't configure in this version I would\n> leave it out and simplify the implementation for this first version\n>\n\nYeah, will remove it completely in v12.\n\n\n> >\n> > >\n> > > > +\n> > > > +/* Frame configuration provided by the config file */\n> > > > +struct ImageFrames {\n> > > > + std::filesystem::path path;\n> > > > + ScaleMode scaleMode;\n> > >\n> > > As this can only be \"Fill\", nothing else is valid atm\n> > >\n> > > > + std::optional number;\n> > > > +};\n> > > > +\n> > > > +class ImageFrameGenerator : public FrameGenerator\n> > > > +{\n> > > > +public:\n> > > > + static std::unique_ptr create(ImageFrames\n> > > &imageFrames);\n> > > > +\n> > > > +private:\n> > > > + struct ImageFrameData {\n> > > > + std::unique_ptr Y;\n> > > > + std::unique_ptr UV;\n> > > > + Size size;\n> > > > + };\n> > > > +\n> > > > + void configure(const Size &size) override;\n> > > > + void generateFrame(const Size &size, const FrameBuffer *buffer)\n> > > override;\n> > > > +\n> > > > + std::vector imageFrameDatas_;\n> > > > + std::vector scaledFrameDatas_;\n> > > > + ImageFrames *imageFrames_;\n> > > > + unsigned int frameCount_;\n> > > > + unsigned int parameter_;\n> > > > +};\n> > > > +\n> > > > +} /* namespace libcamera */\n> > > > diff --git a/src/libcamera/pipeline/virtual/meson.build\n> > > b/src/libcamera/pipeline/virtual/meson.build\n> > > > index d72ac5be..395919b3 100644\n> > > > --- a/src/libcamera/pipeline/virtual/meson.build\n> > > > +++ b/src/libcamera/pipeline/virtual/meson.build\n> > > > @@ -1,9 +1,13 @@\n> > > > # SPDX-License-Identifier: CC0-1.0\n> > > >\n> > > > libcamera_internal_sources += files([\n> > > > + 'image_frame_generator.cpp',\n> > > > 'parser.cpp',\n> > > > 'test_pattern_generator.cpp',\n> > > > 'virtual.cpp',\n> > > > ])\n> > > >\n> > > > +libjpeg = dependency('libjpeg', required : false)\n> > > > +\n> > > > libcamera_deps += [libyuv_dep]\n> > > > +libcamera_deps += [libjpeg]\n> > > > diff --git a/src/libcamera/pipeline/virtual/parser.cpp\n> > > b/src/libcamera/pipeline/virtual/parser.cpp\n> > > > index d861a52a..5076e71c 100644\n> > > > --- a/src/libcamera/pipeline/virtual/parser.cpp\n> > > > +++ b/src/libcamera/pipeline/virtual/parser.cpp\n> > > > @@ -52,12 +52,12 @@ Parser::parseConfigFile(File &file,\n> PipelineHandler\n> > > *pipe)\n> > > > continue;\n> > > > }\n> > > >\n> > > > - data->id_ = cameraId;\n> > > > + data->config_.id = cameraId;\n> > > > ControlInfoMap::Map controls;\n> > > > /* todo: Check which resolution's frame rate to be\n> > > reported */\n> > > > controls[&controls::FrameDurationLimits] =\n> > > > - ControlInfo(int64_t(1000000 /\n> > > data->supportedResolutions_[0].frameRates[1]),\n> > > > - int64_t(1000000 /\n> > > data->supportedResolutions_[0].frameRates[0]));\n> > > > + ControlInfo(int64_t(1000000 /\n> > > data->config_.resolutions[0].frameRates[1]),\n> > > > + int64_t(1000000 /\n> > > data->config_.resolutions[0].frameRates[0]));\n> > > > data->controlInfo_ =\n> ControlInfoMap(std::move(controls),\n> > > controls::controls);\n> > > > configurations.push_back(std::move(data));\n> > > > }\n> > > > @@ -75,7 +75,8 @@ Parser::parseCameraConfigData(const YamlObject\n> > > &cameraConfigData,\n> > > > std::unique_ptr data =\n> > > > std::make_unique(pipe, resolutions);\n> > > >\n> > > > - if (parseTestPattern(cameraConfigData, data.get()))\n> > > > + if (parseTestPattern(cameraConfigData, data.get()) &&\n> > > > + parseFrame(cameraConfigData, data.get()))\n> > >\n> > > This is fragile and only works because if parseTestPattern() returns 0\n> > > then parseFrame() is never called\n> >\n> >\n> > > > return nullptr;\n> > > >\n> > > > if (parseLocation(cameraConfigData, data.get()))\n> > > > @@ -148,16 +149,75 @@ int Parser::parseTestPattern(const YamlObject\n> > > &cameraConfigData, VirtualCameraDa\n> > > > {\n> > > > std::string testPattern =\n> > > cameraConfigData[\"test_pattern\"].get(\"\");\n> > > >\n> > > > - /* Default value is \"bars\" */\n> > > > if (testPattern == \"bars\") {\n> > > > - data->testPattern_ = TestPattern::ColorBars;\n> > > > + data->config_.frame = TestPattern::ColorBars;\n> > > > } else if (testPattern == \"lines\") {\n> > > > - data->testPattern_ = TestPattern::DiagonalLines;\n> > > > + data->config_.frame = TestPattern::DiagonalLines;\n> > > > } else {\n> > > > - LOG(Virtual, Error) << \"Test pattern: \" << testPattern\n> > > > + LOG(Virtual, Debug) << \"Test pattern: \" << testPattern\n> > > > << \"is not supported\";\n> > > > return -EINVAL;\n> > > > }\n> > > > +\n> > > > + return 0;\n> > > > +}\n> > > > +\n> > > > +int Parser::parseFrame(const YamlObject &cameraConfigData,\n> > > VirtualCameraData *data)\n> > >\n> > > You can unify these two functions.\n> > >\n> > > Get \"testPattern\", if valid make sure it's only either \"bars\" or\n> > > \"lines\" and fail if it is specified but has an unsupported value.\n> > >\n> > > Then check for \"frames\". If both \"frames\" and \"testPattern\" are\n> > > specified, it's an error. Then parse \"frames\" like you do here.\n> > >\n> >\n> > Makes sense. Thanks! Updated.\n> >\n> >\n> > >\n> > > > +{\n> > > > + const YamlObject &frames = cameraConfigData[\"frames\"];\n> > > > +\n> > > > + /* When there is no frames provided in the config file, use\n> color\n> > > bar test pattern */\n> > > > + if (frames.size() == 0) {\n> > > > + data->config_.frame = TestPattern::ColorBars;\n> > > > + return 0;\n> > > > + }\n> > > > +\n> > > > + if (!frames.isDictionary()) {\n> > > > + LOG(Virtual, Error) << \"'frames' is not a dictionary.\";\n> > > > + return -EINVAL;\n> > > > + }\n> > > > +\n> > > > + std::string path = frames[\"path\"].get(\"\");\n> > > > +\n> > > > + ScaleMode scaleMode;\n> > > > + if (auto ext = std::filesystem::path(path).extension();\n> > > > + ext == \".jpg\" || ext == \".jpeg\") {\n> > > > + if (parseScaleMode(frames, &scaleMode))\n> > > > + return -EINVAL;\n> > > > + data->config_.frame = ImageFrames{ path, scaleMode,\n> > > std::nullopt };\n> > > > + } else if\n> > > (std::filesystem::is_directory(std::filesystem::symlink_status(path)))\n> {\n> > > > + if (parseScaleMode(frames, &scaleMode))\n> > > > + return -EINVAL;\n> > >\n> > > Could you parse scale mode before checking the file extensions ?\n> > >\n> > >\n> > Done.\n> >\n> >\n> > > > +\n> > > > + using std::filesystem::directory_iterator;\n> > > > + unsigned int numOfFiles =\n> > > std::distance(directory_iterator(path), directory_iterator{});\n> > > > + if (numOfFiles == 0) {\n> > > > + LOG(Virtual, Error) << \"Empty directory\";\n> > > > + return -EINVAL;\n> > > > + }\n> > > > + data->config_.frame = ImageFrames{ path, scaleMode,\n> > > numOfFiles };\n> > > > + } else {\n> > > > + LOG(Virtual, Error) << \"Frame: \" << path << \" is not\n> > > supported\";\n> > > > + return -EINVAL;\n> > > > + }\n> > > > +\n> > > > + return 0;\n> > > > +}\n> > > > +\n> > > > +int Parser::parseScaleMode(\n> > > > + const YamlObject &framesConfigData, ScaleMode *scaleMode)\n> > >\n> > > weird indent\n> > >\n> >\n> > Updated.\n> >\n> >\n> > >\n> > > > +{\n> > > > + std::string mode =\n> > > framesConfigData[\"scale_mode\"].get(\"\");\n> > > > +\n> > > > + /* Default value is fill */\n> > > > + if (mode == \"fill\" || mode == \"\") {\n> > > > + *scaleMode = ScaleMode::Fill;\n> > >\n> > > You don't need a type as suggested above, you can either assume \"Fill\"\n> > > or fail during parsing.\n> > >\n> > > > + } else {\n> > > > + LOG(Virtual, Error) << \"scaleMode: \" << mode\n> > > > + << \" is not supported\";\n> > > > + return -EINVAL;\n> > > > + }\n> > > > +\n> > > > return 0;\n> > > > }\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/virtual/parser.h\n> > > b/src/libcamera/pipeline/virtual/parser.h\n> > > > index 09c3c56b..f65616e3 100644\n> > > > --- a/src/libcamera/pipeline/virtual/parser.h\n> > > > +++ b/src/libcamera/pipeline/virtual/parser.h\n> > > > @@ -35,8 +35,10 @@ private:\n> > > > int parseSupportedFormats(const YamlObject &cameraConfigData,\n> > > >\n> > > std::vector *resolutions);\n> > > > int parseTestPattern(const YamlObject &cameraConfigData,\n> > > VirtualCameraData *data);\n> > > > + int parseFrame(const YamlObject &cameraConfigData,\n> > > VirtualCameraData *data);\n> > > > int parseLocation(const YamlObject &cameraConfigData,\n> > > VirtualCameraData *data);\n> > > > int parseModel(const YamlObject &cameraConfigData,\n> > > VirtualCameraData *data);\n> > > > + int parseScaleMode(const YamlObject &framesConfigData,\n> ScaleMode\n> > > *scaleMode);\n> > > > };\n> > > >\n> > > > } /* namespace libcamera */\n> > > > diff --git a/src/libcamera/pipeline/virtual/utils.h\n> > > b/src/libcamera/pipeline/virtual/utils.h\n> > > > new file mode 100644\n> > > > index 00000000..43a14d4b\n> > > > --- /dev/null\n> > > > +++ b/src/libcamera/pipeline/virtual/utils.h\n> > > > @@ -0,0 +1,17 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2024, Google Inc.\n> > > > + *\n> > > > + * utils.h - Utility types for Virtual Pipeline Handler\n> > > > + */\n> > > > +\n> > > > +namespace libcamera {\n> > > > +\n> > > > +template\n> > > > +struct overloaded : Ts... {\n> > > > + using Ts::operator()...;\n> > > > +};\n> > > > +template\n> > > > +overloaded(Ts...) -> overloaded;\n> > > > +\n> > >\n> > > Does using a standard construct like std::visit a custom definition\n> > >\n> >\n> > Sorry, I don't get what you mean.\n>\n> I was wondering why you need this to use a construct from stdlib, then\n> I read the example at\n> https://en.cppreference.com/w/cpp/utility/variant/visit\n\n\nYeah, this is exactly where I copied the code :p\n\n\n>\n>\n> >\n> >\n> > > like this one ? Also, this header is included form a single cpp file,\n> > > move its content there if you can.\n> > >\n> > >\n> > Hmm, I was suggested otherwise:\n> >\n>\n> I see, but what you think ? What is the point of an header file\n> without any other file including it but one, with just this small\n> helpers above ?\n>\n\nYeah I do think that putting it in the source makes more sense.\nWill update in v12.\n\n\n>\n>\n> > https://patchwork.libcamera.org/patch/20974/\n> > ```\n> >\n> > Apart from the indentation looking a bit off, it seems ok.\n> >\n> > I think the `overloaded` type could go into `utils.h` or similar.\n> >\n> >\n> > Regards,\n> > Barnabás Pőcze\n> >\n> > ```\n> >\n> > I'm new to std::visit, so...\n> >\n>\n> Not very much related to std::visit but the usage of an header when\n> imho it shouldn't be necessary\n>\n> >\n> > >\n> > > > +} /* namespace libcamera */\n> > > > diff --git a/src/libcamera/pipeline/virtual/virtual.cpp\n> > > b/src/libcamera/pipeline/virtual/virtual.cpp\n> > > > index 55bc30df..98aed412 100644\n> > > > --- a/src/libcamera/pipeline/virtual/virtual.cpp\n> > > > +++ b/src/libcamera/pipeline/virtual/virtual.cpp\n> > > > @@ -27,6 +27,7 @@\n> > > > #include \"libcamera/internal/yaml_parser.h\"\n> > > >\n> > > > #include \"parser.h\"\n> > > > +#include \"utils.h\"\n> > > >\n> > > > namespace libcamera {\n> > > >\n> > > > @@ -49,17 +50,18 @@ uint64_t currentTimestamp()\n> > > >\n> > > > VirtualCameraData::VirtualCameraData(PipelineHandler *pipe,\n> > > > std::vector\n> > > supportedResolutions)\n> > > > - : Camera::Private(pipe),\n> > > supportedResolutions_(std::move(supportedResolutions))\n> > > > + : Camera::Private(pipe)\n> > > > {\n> > > > - for (const auto &resolution : supportedResolutions_) {\n> > > > - if (minResolutionSize_.isNull() || minResolutionSize_ >\n> > > resolution.size)\n> > > > - minResolutionSize_ = resolution.size;\n> > > > + config_.resolutions = std::move(supportedResolutions);\n> > > > + for (const auto &resolution : config_.resolutions) {\n> > > > + if (config_.minResolutionSize.isNull() ||\n> > > config_.minResolutionSize > resolution.size)\n> > > > + config_.minResolutionSize = resolution.size;\n> > > >\n> > > > - maxResolutionSize_ = std::max(maxResolutionSize_,\n> > > resolution.size);\n> > > > + config_.maxResolutionSize =\n> > > std::max(config_.maxResolutionSize, resolution.size);\n> > > > }\n> > > >\n> > > > properties_.set(properties::PixelArrayActiveAreas,\n> > > > - { Rectangle(maxResolutionSize_) });\n> > > > + { Rectangle(config_.maxResolutionSize) });\n> > > >\n> > > > /* \\todo Support multiple streams and pass multi_stream_test */\n> > > > streamConfigs_.resize(kMaxStream);\n> > > > @@ -87,7 +89,7 @@ CameraConfiguration::Status\n> > > VirtualCameraConfiguration::validate()\n> > > >\n> > > > for (StreamConfiguration &cfg : config_) {\n> > > > bool found = false;\n> > > > - for (const auto &resolution :\n> > > data_->supportedResolutions_) {\n> > > > + for (const auto &resolution :\n> data_->config_.resolutions) {\n> > > > if (resolution.size.width == cfg.size.width &&\n> > > > resolution.size.height == cfg.size.height)\n> {\n> > > > found = true;\n> > > > @@ -102,7 +104,7 @@ CameraConfiguration::Status\n> > > VirtualCameraConfiguration::validate()\n> > > > * Defining the default logic in\n> 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> > > > @@ -145,11 +147,11 @@\n> > > PipelineHandlerVirtual::generateConfiguration(Camera *camera,\n> > > > for (const StreamRole role : roles) {\n> > > > std::map>\n> > > streamFormats;\n> > > > PixelFormat pixelFormat = formats::NV12;\n> > > > - streamFormats[pixelFormat] = { {\n> data->minResolutionSize_,\n> > > data->maxResolutionSize_ } };\n> > > > + streamFormats[pixelFormat] = { {\n> > > data->config_.minResolutionSize, data->config_.maxResolutionSize } };\n> > > > StreamFormats formats(streamFormats);\n> > > > StreamConfiguration cfg(formats);\n> > > > cfg.pixelFormat = pixelFormat;\n> > > > - cfg.size = data->maxResolutionSize_;\n> > > > + cfg.size = data->config_.maxResolutionSize;\n> > > > cfg.bufferCount =\n> VirtualCameraConfiguration::kBufferCount;\n> > > >\n> > > > switch (role) {\n> > > > @@ -181,6 +183,7 @@ int PipelineHandlerVirtual::configure(Camera\n> *camera,\n> > > > VirtualCameraData *data = cameraData(camera);\n> > > > for (size_t i = 0; i < config->size(); ++i) {\n> > > >\n> config->at(i).setStream(&data->streamConfigs_[i].stream);\n> > > > + /* Start reading the images/generating test patterns */\n> > > > data->streamConfigs_[i].frameGenerator->configure(\n> > > >\n> > > data->streamConfigs_[i].stream.configuration().size);\n> > > > }\n> > > > @@ -274,10 +277,14 @@ bool\n> > > PipelineHandlerVirtual::match([[maybe_unused]] DeviceEnumerator\n> *enumerator\n> > > > std::set streams;\n> > > > for (auto &streamConfig : data->streamConfigs_)\n> > > > streams.insert(&streamConfig.stream);\n> > > > - std::string id = data->id_;\n> > > > + std::string id = data->config_.id;\n> > > > std::shared_ptr camera =\n> > > Camera::create(std::move(data), id, streams);\n> > > >\n> > > > - initFrameGenerator(camera.get());\n> > > > + if (!initFrameGenerator(camera.get())) {\n> > > > + LOG(Virtual, Error) << \"Failed to initialize\n> frame\n> > > \"\n> > > > + << \"generator for camera:\n> \" <<\n> > > id;\n> > > > + continue;\n> > > > + }\n> > > >\n> > > > registerCamera(std::move(camera));\n> > > > }\n> > > > @@ -285,15 +292,30 @@ bool\n> > > PipelineHandlerVirtual::match([[maybe_unused]] DeviceEnumerator\n> *enumerator\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 =\n> > > std::make_unique();\n> > > > - else\n> > > > - streamConfig.frameGenerator =\n> > > std::make_unique();\n> > > > - }\n> > > > + auto &frame = data->config_.frame;\n> > > > + std::visit(overloaded{\n> > > > + [&](TestPattern &testPattern) {\n> > > > + for (auto &streamConfig :\n> > > data->streamConfigs_) {\n> > > > + if (testPattern ==\n> > > TestPattern::DiagonalLines)\n> > > > +\n> > > streamConfig.frameGenerator =\n> std::make_unique();\n> > > > + else\n> > > > +\n> > > streamConfig.frameGenerator = std::make_unique();\n> > > > + }\n> > > > + },\n> > > > + [&](ImageFrames &imageFrames) {\n> > > > + for (auto &streamConfig :\n> > > data->streamConfigs_)\n> > > > + streamConfig.frameGenerator\n> =\n> > > 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\n> > > b/src/libcamera/pipeline/virtual/virtual.h\n> > > > index 8830e00f..efa97e88 100644\n> > > > --- a/src/libcamera/pipeline/virtual/virtual.h\n> > > > +++ b/src/libcamera/pipeline/virtual/virtual.h\n> > > > @@ -8,6 +8,8 @@\n> > > > #pragma once\n> > > >\n> > > > #include \n> > > > +#include \n> > > > +#include \n> > > > #include \n> > > >\n> > > > #include \n> > > > @@ -16,10 +18,14 @@\n> > > > #include \"libcamera/internal/dma_buf_allocator.h\"\n> > > > #include \"libcamera/internal/pipeline_handler.h\"\n> > > >\n> > > > +#include \"frame_generator.h\"\n> > > > +#include \"image_frame_generator.h\"\n> > > > #include \"test_pattern_generator.h\"\n> > > >\n> > > > namespace libcamera {\n> > > >\n> > > > +using VirtualFrame = std::variant;\n> > > > +\n> > > > class VirtualCameraData : public Camera::Private\n> > > > {\n> > > > public:\n> > > > @@ -33,18 +39,22 @@ public:\n> > > > Stream stream;\n> > > > std::unique_ptr frameGenerator;\n> > > > };\n> > > > + /* The config file is parsed to the Configuration struct */\n> > > > + struct Configuration {\n> > > > + std::string id;\n> > > > + std::vector resolutions;\n> > > > + VirtualFrame frame;\n> > > > +\n> > > > + Size maxResolutionSize;\n> > > > + Size minResolutionSize;\n> > > > + };\n> > > >\n> > > > VirtualCameraData(PipelineHandler *pipe,\n> > > > std::vector\n> supportedResolutions);\n> > > >\n> > > > ~VirtualCameraData() = default;\n> > > >\n> > > > - std::string id_;\n> > > > - TestPattern testPattern_ = TestPattern::ColorBars;\n> > > > -\n> > > > - const std::vector supportedResolutions_;\n> > > > - Size maxResolutionSize_;\n> > > > - Size minResolutionSize_;\n> > > > + Configuration config_;\n> > > >\n> > > > std::vector streamConfigs_;\n> > > > };\n> > > > @@ -89,7 +99,7 @@ private:\n> > > > return static_cast(camera->_d());\n> > > > }\n> > > >\n> > > > - void initFrameGenerator(Camera *camera);\n> > > > + bool initFrameGenerator(Camera *camera);\n> > > >\n> > > > DmaBufAllocator dmaBufAllocator_;\n> > > > };\n> > > > --\n> > > > 2.46.0.469.g59c65b2a67-goog\n> > > >\n> > >\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 0BA9EC324C\n\tfor ;\n\tTue, 10 Sep 2024 04:51:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9E50C634FE;\n\tTue, 10 Sep 2024 06:51:09 +0200 (CEST)","from mail-lj1-x22b.google.com (mail-lj1-x22b.google.com\n\t[IPv6:2a00:1450:4864:20::22b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 051BF618F2\n\tfor ;\n\tTue, 10 Sep 2024 06:51:08 +0200 (CEST)","by mail-lj1-x22b.google.com with SMTP id\n\t38308e7fff4ca-2f760f7e25bso22543951fa.2\n\tfor ;\n\tMon, 09 Sep 2024 21:51:07 -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=\"bh8ZE1LM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1725943867; x=1726548667;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=aY66XzW6rYMSHxwTauE9a76kA2M93HunyikG9rZSzLw=;\n\tb=bh8ZE1LMv1wbcn1dZdJ3H7OT+dnffDNYhK1FtteJuwnn5wB+IIdxdp7R6TiWF0FAnM\n\tFAA4MzVEZUTWOrD7ZNVmRIwvjFfgqMIzH/8EG+4KSIECdl7KyzG21IUjqWVPIjrcuejJ\n\tJpnJlPP8GcBMNJSLOnrWIGb8uvpLMG4BRqcyM=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1725943867; x=1726548667;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=aY66XzW6rYMSHxwTauE9a76kA2M93HunyikG9rZSzLw=;\n\tb=GFg5UTEuoDWQxJ8lNfQ3Nl7dB4pgMi/iTKPvLSARQdWt8YKaW+rz9//IQvIHrWisin\n\tUPLKk2MwpxG3Cay8/+6LAYtmbHOGvQTz3XD1gClsr7+DlAlRew9r6yxf4NjO0W9Pr8bi\n\tZ74IFac1nbQaprgLhr5mvJ5kyPFR04Ql/QY7rAyJUYQTnoGR/PltFUOrpQtX+qWQvQQt\n\tISgQCPtHBuh4aRcOeA2GHuRrJlxeXs+MF8qNEmfHqvi8qiybRImLf8/omZVpfW3DJXWQ\n\t8jJaHO31uf45F8HE//AZv9jCW0TBHIw+GrVdZDgSZhsDBXn1sUldsvNM0F6eng00ords\n\trbYA==","X-Gm-Message-State":"AOJu0YxIor7ifgC6QtqWHEviWhCA/5h/DsJ2WNx+AkIIN3cJuYQdWze+\n\tgkGwNKIR7Xlk5dqG92mPfoe//NQIdIB+CUAonaiHqZsV6/fkO6ScLBSwdiZokoTUZHel5OaLex4\n\tiz8EHpCmLPoRrhQrYH/woxnTFyqx3+leLRqkT","X-Google-Smtp-Source":"AGHT+IH6kH6v1bzNY/urHXVNOYDG15DY+qXbg3EpDf9bwknebz3i6xtlob08Wyv/JYUjTY3UYzdWlJ2PDwck3PIUlCM=","X-Received":"by 2002:a05:651c:198e:b0:2ec:63f:fe91 with SMTP id\n\t38308e7fff4ca-2f752495f59mr62365091fa.38.1725943866813;\n\tMon, 09 Sep 2024 21:51:06 -0700 (PDT)","MIME-Version":"1.0","References":"<20240907143110.2210711-1-chenghaoyang@google.com>\n\t<20240907143110.2210711-7-chenghaoyang@google.com>\n\t<3yzin5r3r3nmawjocyrakt3aq6xsys2fnsjwobddfid7yxwjxk@gc6abloh6zas>\n\t\n\t","In-Reply-To":"","From":"Cheng-Hao Yang ","Date":"Tue, 10 Sep 2024 12:50:54 +0800","Message-ID":"","Subject":"Re: [PATCH v11 6/7] libcamera: virtual: Add ImageFrameGenerator","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org, Konami Shu , \n\tYunke Cao , Tomasz Figa ","Content-Type":"multipart/alternative; boundary=\"00000000000015862c0621bc9f16\"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]