| Message ID | 20240820172202.526547-8-chenghaoyang@google.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi 2024. augusztus 20., kedd 18:23 keltezéssel, Harvey Yang <chenghaoyang@chromium.org> írta: > From: Konami Shu <konamiz@google.com> > > - Refactor ImageFrameGenerator and TestPatternGenerator > - Extend the config file to have section to choose to use images or test > pattern > - Extend Parser to parse the configuration > - Add ImageFrameGenerator which uses images to provide frames > > Patchset1->2 > - Extend the parser to accept files with ".jpeg" > - Heap allocate some buffers in ImageFrameGenerator::generateFrame so > the buffers won't cause stack over flow > > Patchset5->6 > - Move static factory function from the interface class to the derived > classes > > Patchset6->8 > - create FrameGenerator before starting the camera > > Signed-off-by: Konami Shu <konamiz@google.com> > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org> > Co-developed-by: Yunke Cao <yunkec@chromium.org> > Co-developed-by: Tomasz Figa <tfiga@chromium.org> > --- > src/libcamera/pipeline/virtual/README.md | 18 +- > .../pipeline/virtual/common_functions.cpp | 27 +++ > .../pipeline/virtual/common_functions.h | 18 ++ > .../pipeline/virtual/frame_generator.h | 2 +- > .../virtual/image_frame_generator.cpp | 154 ++++++++++++++++++ > .../pipeline/virtual/image_frame_generator.h | 65 ++++++++ > src/libcamera/pipeline/virtual/meson.build | 6 +- > src/libcamera/pipeline/virtual/parser.cpp | 77 +++++++-- > src/libcamera/pipeline/virtual/parser.h | 5 +- > .../virtual/test_pattern_generator.cpp | 4 +- > .../pipeline/virtual/test_pattern_generator.h | 2 +- > src/libcamera/pipeline/virtual/virtual.cpp | 37 +++-- > src/libcamera/pipeline/virtual/virtual.h | 20 ++- > 13 files changed, 390 insertions(+), 45 deletions(-) > create mode 100644 src/libcamera/pipeline/virtual/common_functions.cpp > create mode 100644 src/libcamera/pipeline/virtual/common_functions.h > create mode 100644 src/libcamera/pipeline/virtual/image_frame_generator.cpp > create mode 100644 src/libcamera/pipeline/virtual/image_frame_generator.h > > diff --git a/src/libcamera/pipeline/virtual/README.md b/src/libcamera/pipeline/virtual/README.md > index 27d6283df..5e21ce74a 100644 > --- a/src/libcamera/pipeline/virtual/README.md > +++ b/src/libcamera/pipeline/virtual/README.md > @@ -15,8 +15,13 @@ Each camera block is a dictionary, containing the following keys: > - `supported_formats` (list of `VirtualCameraData::Resolution`, optional) : List of supported resolution and frame rates of the emulated camera > - `width` (`unsigned int`, default=1920): Width of the window resolution. This needs to be even. > - `height` (`unsigned int`, default=1080): Height of the window resolution. > - - `frame_rates` (list of `int`, default=`[30,60]` ): Range of the frame rate. The list has to be two values of the lower bound and the upper bound of the frame rate. > -- `test_pattern` (`string`, default="bars"): Which test pattern to use as frames. The options are "bars", "lines". > + - `frame_rates` (list of `int`, default=`[30,60]` ): Range of the frame rate. The list has to be two values of the lower bound and the upper bound of the frame rate. This does not affect the frame rate for now. > +- `frames` (dictionary): > + - `path` (`string`, default="bars"): Name of a test pattern, path to an image, or path to a directory of a series of images. > + - The test patterns are "bars" which means color bars, and "lines" which means diagonal lines. > + - The path to an image has ".jpg" extension. > + - 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. > + - `scale_mode`(`string`, default="fill"): Scale mode when the frames are images. The scale modes are "fill", "contain", and "cover". This does not matter when frames is a test pattern. This does not affect the scale mode for now. > - `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. > - `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. > > @@ -35,13 +40,16 @@ A sample config file: > frame_rates: > - 70 > - 80 > - test_pattern: "bars" > + frames: > + path: "lines" > location: "front" > model: "Virtual Video Device" > "Virtual1": > supported_formats: > - width: 800 > - test_pattern: "lines" > + frames: > + path: "path/to/directory_of_images/" > + scale_mode: "contain" > location: "back" > model: "Virtual Video Device1" > "Virtual2": > @@ -61,7 +69,7 @@ This is the procedure of the Parser class: > - If the config file contains invalid configuration, this method returns nullptr. The camera will be skipped. > 3. Parse each property and register the data. > - `parseSupportedFormats()`: Parses `supported_formats` in the config, which contains resolutions and frame rates. > - - `parseTestPattern()`: Parses `test_pattern` in the config. > + - `parseFrame()`: Parses `frames` in the config. > - `parseLocation()`: Parses `location` in the config. > - `parseModel()`: Parses `model` in the config. > 4. Back to `parseConfigFile()` and append the camera configuration. > diff --git a/src/libcamera/pipeline/virtual/common_functions.cpp b/src/libcamera/pipeline/virtual/common_functions.cpp > new file mode 100644 > index 000000000..207827ee0 > --- /dev/null > +++ b/src/libcamera/pipeline/virtual/common_functions.cpp > @@ -0,0 +1,27 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2023, Google Inc. > + * > + * common_functions.cpp - Helper that do not depend on any class > + */ > + > +#include "common_functions.h" > + > +namespace libcamera { > + > +std::string getExtension(const std::string &path) This looks very fragile. Why not use `std::filesystem::path::extension()`? > +{ > + size_t i = path.find("."); > + if (i != std::string::npos) { > + return path.substr(i); > + } > + return ""; > +} > + > +std::size_t numberOfFilesInDirectory(std::filesystem::path path) > +{ > + using std::filesystem::directory_iterator; > + return std::distance(directory_iterator(path), directory_iterator{}); > +} > + > +} // namespace libcamera > diff --git a/src/libcamera/pipeline/virtual/common_functions.h b/src/libcamera/pipeline/virtual/common_functions.h > new file mode 100644 > index 000000000..4203f9505 > --- /dev/null > +++ b/src/libcamera/pipeline/virtual/common_functions.h > @@ -0,0 +1,18 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2023, Google Inc. > + * > + * common_functions.h - Helper that do not depend on any class > + */ > + > +#pragma once > + > +#include <filesystem> > + > +namespace libcamera { > + > +std::string getExtension(const std::string &path); > + > +std::size_t numberOfFilesInDirectory(std::filesystem::path path); > + > +} // namespace libcamera > diff --git a/src/libcamera/pipeline/virtual/frame_generator.h b/src/libcamera/pipeline/virtual/frame_generator.h > index 9699af7a4..f69576b36 100644 > --- a/src/libcamera/pipeline/virtual/frame_generator.h > +++ b/src/libcamera/pipeline/virtual/frame_generator.h > @@ -23,7 +23,7 @@ public: > /** Fill the output frame buffer. > * Use the frame at the frameCount of image frames > */ > - virtual void generateFrame(const Size &size, > + virtual void generateFrame(unsigned int &frameCount, const Size &size, > const FrameBuffer *buffer) = 0; > > protected: > diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.cpp b/src/libcamera/pipeline/virtual/image_frame_generator.cpp > new file mode 100644 > index 000000000..d374877ff > --- /dev/null > +++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp > @@ -0,0 +1,154 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2023, Google Inc. > + * > + * image_frame_generator.cpp - Derived class of FrameGenerator for > + * generating frames from images > + */ > + > +#include "image_frame_generator.h" > + > +#include <memory> > +#include <optional> > +#include <string> > + > +#include <libcamera/base/file.h> > +#include <libcamera/base/log.h> > + > +#include <libcamera/framebuffer.h> > + > +#include "libcamera/internal/mapped_framebuffer.h" > + > +#include "libyuv/convert.h" > +#include "libyuv/scale.h" > +namespace libcamera { > + > +LOG_DECLARE_CATEGORY(Virtual) > + > +std::unique_ptr<ImageFrameGenerator> ImageFrameGenerator::create( > + ImageFrames &imageFrames) > +{ > + std::unique_ptr<ImageFrameGenerator> imageFrameGenerator = > + std::make_unique<ImageFrameGenerator>(); > + imageFrameGenerator->imageFrames_ = &imageFrames; > + > + /** For each file in the directory > + * load the image, convert it to NV12, and store the pointer > + */ > + for (unsigned int i = 0; i < imageFrames.number.value_or(1); i++) { > + std::string path; > + if (!imageFrames.number.has_value()) { > + /* If the path is to an image */ > + path = imageFrames.path; > + } else { > + /* If the path is to a directory */ > + path = constructPath(imageFrames.path, i); > + } > + > + File file(path); > + bool isOpen = file.open(File::OpenModeFlag::ReadOnly); > + if (!isOpen) { I think `if (!file.open(...))` is fine. > + LOG(Virtual, Error) << "Failed to open image file: " << file.fileName(); > + return nullptr; > + } > + > + /* Read the image file to data */ > + uint8_t buffer[file.size()]; It's probably best to avoid VLAs. > + Span<unsigned char> data{ buffer, (unsigned long)file.size() }; > + long dataSize = file.read(data); > + > + /* Get the width and height of the image */ > + int width, height; > + if (libyuv::MJPGSize(data.data(), dataSize, &width, &height)) { > + LOG(Virtual, Error) << "Failed to get the size of the image file: " > + << file.fileName(); > + return nullptr; > + } > + > + /* Convert to NV12 and write the data to tmpY and tmpUV */ > + int halfWidth = (width + 1) / 2; > + int halfHeight = (height + 1) / 2; > + std::unique_ptr<uint8_t[]> dstY = > + std::make_unique<uint8_t[]>(width * height); > + std::unique_ptr<uint8_t[]> dstUV = > + std::make_unique<uint8_t[]>(halfWidth * halfHeight * 2); > + int ret = libyuv::MJPGToNV12(data.data(), dataSize, > + dstY.get(), width, dstUV.get(), > + width, width, height, width, height); > + if (ret != 0) { > + LOG(Virtual, Error) << "MJPGToNV12() failed with " << ret; > + } > + > + imageFrameGenerator->imageFrameDatas_.emplace_back( > + ImageFrameData{ std::move(dstY), std::move(dstUV), > + Size(width, height) }); > + } > + return imageFrameGenerator; > +} > + > +std::string ImageFrameGenerator::constructPath(std::string &name, unsigned int &i) Does this need to be in `ImageFrameGenerator`? Is it not enough to have it in an anonymous namespace in this file? > +{ > + return name + std::to_string(i) + ".jpg"; > +} > + > +void ImageFrameGenerator::configure(const Size &size) > +{ > + for (unsigned int i = 0; i < imageFrames_->number.value_or(1); i++) { > + /* Scale the imageFrameDatas_ to scaledY and scaledUV */ > + int halfSizeWidth = (size.width + 1) / 2; > + int halfSizeHeight = (size.height + 1) / 2; > + std::unique_ptr<uint8_t[]> scaledY = > + std::make_unique<uint8_t[]>(size.width * size.height); > + std::unique_ptr<uint8_t[]> scaledUV = > + std::make_unique<uint8_t[]>(halfSizeWidth * halfSizeHeight * 2); > + auto &src = imageFrameDatas_[i]; > + > + /* > + * \todo Implement "contain" & "cover", based on > + * |imageFrames_[i].scaleMode|. > + */ > + > + /* > + * \todo Some platforms might enforce stride due to GPU, like > + * ChromeOS ciri (64). The weight needs to be a multiple of > + * the stride to work properly for now. > + */ > + libyuv::NV12Scale(src.Y.get(), src.size.width, > + src.UV.get(), src.size.width, > + src.size.width, src.size.height, > + scaledY.get(), size.width, scaledUV.get(), size.width, > + size.width, size.height, libyuv::FilterMode::kFilterBilinear); > + > + /* Store the pointers to member variable */ > + scaledFrameDatas_.emplace_back( > + ImageFrameData{ std::move(scaledY), std::move(scaledUV), size }); > + } > +} > + > +void ImageFrameGenerator::generateFrame(unsigned int &frameCount, const Size &size, const FrameBuffer *buffer) > +{ > + /* Don't do anything when the list of buffers is empty*/ > + if (scaledFrameDatas_.size() == 0) > + return; > + > + MappedFrameBuffer mappedFrameBuffer(buffer, MappedFrameBuffer::MapFlag::Write); > + > + auto planes = mappedFrameBuffer.planes(); > + > + /* Make sure the frameCount does not over the number of images */ > + frameCount %= imageFrames_->number.value_or(1); > + > + /* Write the scaledY and scaledUV to the mapped frame buffer */ > + libyuv::NV12Copy(scaledFrameDatas_[frameCount].Y.get(), size.width, > + scaledFrameDatas_[frameCount].UV.get(), size.width, planes[0].begin(), > + size.width, planes[1].begin(), size.width, > + size.width, size.height); > + > + /* proceed an image every 4 frames */ > + /* \todo read the parameter_ from the configuration file? */ > + parameter_++; > + if (parameter_ % 4 == 0) > + frameCount++; > +} > + > +} // namespace libcamera > diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.h b/src/libcamera/pipeline/virtual/image_frame_generator.h > new file mode 100644 > index 000000000..74468e075 > --- /dev/null > +++ b/src/libcamera/pipeline/virtual/image_frame_generator.h > @@ -0,0 +1,65 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2023, Google Inc. > + * > + * image_frame_generator.h - Derived class of FrameGenerator for > + * generating frames from images > + */ > + > +#pragma once > + > +#include <memory> > +#include <optional> > +#include <stdint.h> > +#include <sys/types.h> > + > +#include "frame_generator.h" > + > +namespace libcamera { > + > +enum class ScaleMode : char { > + Fill = 0, > + Contain = 1, > + Cover = 2, > +}; > + > +/* Frame configuration provided by the config file */ > +struct ImageFrames { > + std::string path; I think `std::filesystem::path` would be preferable. > + ScaleMode scaleMode; > + std::optional<unsigned int> number; > +}; > + > +class ImageFrameGenerator : public FrameGenerator > +{ > +public: > + /** Factory function to create an ImageFrameGenerator object. > + * Read the images and convert them to buffers in NV12 format. > + * Store the pointers to the buffers to a list (imageFrameDatas) > + */ > + static std::unique_ptr<ImageFrameGenerator> create(ImageFrames &imageFrames); > + > +private: > + struct ImageFrameData { > + std::unique_ptr<uint8_t[]> Y; > + std::unique_ptr<uint8_t[]> UV; > + Size size; > + }; > + > + /* Scale the buffers for image frames. */ > + void configure(const Size &size) override; > + void generateFrame(unsigned int &frameCount, const Size &size, const FrameBuffer *buffer) override; > + > + static std::string constructPath(std::string &name, unsigned int &i); > + > + /* List of pointers to the not scaled image buffers */ > + std::vector<ImageFrameData> imageFrameDatas_; > + /* List of pointers to the scaled image buffers */ > + std::vector<ImageFrameData> scaledFrameDatas_; > + /* Pointer to the imageFrames_ in VirtualCameraData */ > + ImageFrames *imageFrames_; > + /* Speed parameter. Change to the next image every parameter_ frames. */ > + int parameter_; > +}; > + > +} /* namespace libcamera */ > diff --git a/src/libcamera/pipeline/virtual/meson.build b/src/libcamera/pipeline/virtual/meson.build > index 2e82e64cb..d56aedec4 100644 > --- a/src/libcamera/pipeline/virtual/meson.build > +++ b/src/libcamera/pipeline/virtual/meson.build > @@ -2,11 +2,14 @@ > > libcamera_sources += files([ > 'virtual.cpp', > - 'test_pattern_generator.cpp', > 'parser.cpp', > + 'test_pattern_generator.cpp', > + 'image_frame_generator.cpp', > + 'common_functions.cpp', > ]) > > libyuv_dep = dependency('libyuv', required : false) > +libjpeg = dependency('libjpeg', required : false) > > # Fallback to a subproject if libyuv isn't found, as it's typically not > # provided by distributions. > @@ -26,3 +29,4 @@ if not libyuv_dep.found() > endif > > libcamera_deps += [libyuv_dep] > +libcamera_deps += [libjpeg] > diff --git a/src/libcamera/pipeline/virtual/parser.cpp b/src/libcamera/pipeline/virtual/parser.cpp > index 032c0cd9d..46d1ab181 100644 > --- a/src/libcamera/pipeline/virtual/parser.cpp > +++ b/src/libcamera/pipeline/virtual/parser.cpp > @@ -18,6 +18,7 @@ > #include "libcamera/internal/pipeline_handler.h" > #include "libcamera/internal/yaml_parser.h" > > +#include "common_functions.h" > #include "virtual.h" > > namespace libcamera { > @@ -52,12 +53,12 @@ std::vector<std::unique_ptr<VirtualCameraData>> Parser::parseConfigFile( > continue; > } > > - data->id_ = cameraId; > + data->config_.id = cameraId; > ControlInfoMap::Map controls; > /* todo: Check which resolution's frame rate to be reported */ > controls[&controls::FrameDurationLimits] = > - ControlInfo(int64_t(1000 / data->supportedResolutions_[0].frameRates[1]), > - int64_t(1000 / data->supportedResolutions_[0].frameRates[0])); > + ControlInfo(int64_t(1000 / data->config_.resolutions[0].frameRates[1]), > + int64_t(1000 / data->config_.resolutions[0].frameRates[0])); > data->controlInfo_ = ControlInfoMap(std::move(controls), controls::controls); > configurations.push_back(std::move(data)); > } > @@ -72,7 +73,7 @@ std::unique_ptr<VirtualCameraData> Parser::parseCameraConfigData( > if (parseSupportedFormats(cameraConfigData, data.get())) > return nullptr; > > - if (parseTestPattern(cameraConfigData, data.get())) > + if (parseFrame(cameraConfigData, data.get())) > return nullptr; > > if (parseLocation(cameraConfigData, data.get())) > @@ -122,14 +123,14 @@ int Parser::parseSupportedFormats( > frameRates.push_back(60); > } > > - data->supportedResolutions_.emplace_back( > + data->config_.resolutions.emplace_back( > VirtualCameraData::Resolution{ Size{ width, height }, > frameRates }); > > activeResolution = std::max(activeResolution, Size{ width, height }); > } > } else { > - data->supportedResolutions_.emplace_back( > + data->config_.resolutions.emplace_back( > VirtualCameraData::Resolution{ Size{ 1920, 1080 }, > { 30, 60 } }); > activeResolution = Size(1920, 1080); > @@ -141,21 +142,65 @@ int Parser::parseSupportedFormats( > return 0; > } > > -int Parser::parseTestPattern( > +int Parser::parseFrame( > const YamlObject &cameraConfigData, VirtualCameraData *data) > { > - std::string testPattern = cameraConfigData["test_pattern"].get<std::string>().value(); > + const YamlObject &frames = cameraConfigData["frames"]; > + /* When there is no frames provided in the config file, use color bar test pattern */ > + if (frames.size() == 0) { > + data->config_.frame = TestPattern::ColorBars; > + return 0; > + } > + > + if (!frames.isDictionary()) { > + LOG(Virtual, Error) << "'frames' is not a dictionary."; > + return -EINVAL; > + } > + > + std::string path = frames["path"].get<std::string>().value(); What if `path` is not specified? Is it checked somewhere else? > + > + if (auto ext = getExtension(path); ext == ".jpg" || ext == ".jpeg") { > + ScaleMode scaleMode; > + if (parseScaleMode(frames, &scaleMode)) > + return -EINVAL; > + data->config_.frame = ImageFrames{ path, scaleMode, std::nullopt }; > + } else if (path.back() == '/') { This looks very fragile. Why not use `std::filesystem::symlink_status()` to check if it's a file or a directory. > + ScaleMode scaleMode; > + if (parseScaleMode(frames, &scaleMode)) > + return -EINVAL; > + data->config_.frame = ImageFrames{ path, scaleMode, > + numberOfFilesInDirectory(path) }; Admittedly I did not check in detail, but is this necessary? Is it necessary to count the number of files in advance? > + } else if (path == "bars" || path == "") { > + /* Default value is "bars" */ > + data->config_.frame = TestPattern::ColorBars; > + } else if (path == "lines") { > + data->config_.frame = TestPattern::DiagonalLines; > + } else { > + LOG(Virtual, Error) << "Frame: " << path > + << " is not supported"; > + return -EINVAL; > + } > + return 0; > +} > > - /* Default value is "bars" */ > - if (testPattern == "bars" || testPattern == "") { > - data->testPattern_ = TestPattern::ColorBars; > - } else if (testPattern == "lines") { > - data->testPattern_ = TestPattern::DiagonalLines; > +int Parser::parseScaleMode( > + const YamlObject &framesConfigData, ScaleMode *scaleMode) > +{ > + std::string mode = framesConfigData["scale_mode"].get<std::string>().value(); Same here, regarding empty optionals. > + > + /* Default value is fill */ > + if (mode == "fill" || mode == "") { > + *scaleMode = ScaleMode::Fill; > + } else if (mode == "contain") { > + *scaleMode = ScaleMode::Contain; > + } else if (mode == "cover") { > + *scaleMode = ScaleMode::Cover; > } else { > - LOG(Virtual, Error) << "Test pattern: " << testPattern > - << "is not supported"; > + LOG(Virtual, Error) << "scaleMode: " << mode > + << " is not supported"; > return -EINVAL; > } > + > return 0; > } > > @@ -195,4 +240,4 @@ int Parser::parseModel( > return 0; > } > > -} /* namespace libcamera */ > +} // namespace libcamera > diff --git a/src/libcamera/pipeline/virtual/parser.h b/src/libcamera/pipeline/virtual/parser.h > index a377d8aa1..38ea460d5 100644 > --- a/src/libcamera/pipeline/virtual/parser.h > +++ b/src/libcamera/pipeline/virtual/parser.h > @@ -34,12 +34,15 @@ private: > > int parseSupportedFormats( > const YamlObject &cameraConfigData, VirtualCameraData *data); > - int parseTestPattern( > + int parseFrame( > const YamlObject &cameraConfigData, VirtualCameraData *data); > int parseLocation( > const YamlObject &cameraConfigData, VirtualCameraData *data); > int parseModel( > const YamlObject &cameraConfigData, VirtualCameraData *data); > + > + int parseScaleMode( > + const YamlObject &framesConfigData, ScaleMode *scaleMode); > }; > > } // namespace libcamera > diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > index 6df9b31e9..844dffd49 100644 > --- a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > @@ -20,7 +20,7 @@ LOG_DECLARE_CATEGORY(Virtual) > static const unsigned int kARGBSize = 4; > > void TestPatternGenerator::generateFrame( > - const Size &size, > + [[maybe_unused]] unsigned int &frameCount, const Size &size, > const FrameBuffer *buffer) > { > MappedFrameBuffer mappedFrameBuffer(buffer, > @@ -29,7 +29,7 @@ void TestPatternGenerator::generateFrame( > auto planes = mappedFrameBuffer.planes(); > > /* TODO: select whether to do shifting or not */ > - shiftLeft(size); > + // shiftLeft(size); > > /* Convert the template_ to the frame buffer */ > int ret = libyuv::ARGBToNV12( > diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.h b/src/libcamera/pipeline/virtual/test_pattern_generator.h > index ed8d4e43b..cebdd0141 100644 > --- a/src/libcamera/pipeline/virtual/test_pattern_generator.h > +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.h > @@ -25,7 +25,7 @@ enum class TestPattern : char { > class TestPatternGenerator : public FrameGenerator > { > private: > - void generateFrame(const Size &size, > + void generateFrame(unsigned int &frameCount, const Size &size, > const FrameBuffer *buffer) override; > > protected: > diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp > index 0fe471f00..2412af703 100644 > --- a/src/libcamera/pipeline/virtual/virtual.cpp > +++ b/src/libcamera/pipeline/virtual/virtual.cpp > @@ -20,9 +20,13 @@ > #include "libcamera/internal/pipeline_handler.h" > #include "libcamera/internal/yaml_parser.h" > > -#include "frame_generator.h" > #include "parser.h" > > +#define ifTestPattern(v) std::holds_alternative<TestPattern>(v) > +#define getTestPattern(v) std::get<TestPattern>(v) > +#define ifImageFrames(v) std::holds_alternative<ImageFrames>(v) > +#define getImageFrames(v) std::get<ImageFrames>(v) > + > namespace libcamera { > > LOG_DEFINE_CATEGORY(Virtual) > @@ -63,12 +67,12 @@ CameraConfiguration::Status VirtualCameraConfiguration::validate() > } > > Size maxSize; > - for (const auto &resolution : data_->supportedResolutions_) > + for (const auto &resolution : data_->config_.resolutions) > maxSize = std::max(maxSize, resolution.size); > > for (StreamConfiguration &cfg : config_) { > bool found = false; > - for (const auto &resolution : data_->supportedResolutions_) { > + for (const auto &resolution : data_->config_.resolutions) { > if (resolution.size.width == cfg.size.width && > resolution.size.height == cfg.size.height) { > found = true; > @@ -110,7 +114,7 @@ PipelineHandlerVirtual::generateConfiguration(Camera *camera, > return config; > > Size minSize, sensorResolution; > - for (const auto &resolution : data->supportedResolutions_) { > + for (const auto &resolution : data->config_.resolutions) { > if (minSize.isNull() || minSize > resolution.size) > minSize = resolution.size; > > @@ -199,7 +203,7 @@ int PipelineHandlerVirtual::exportFrameBuffers( > int PipelineHandlerVirtual::start(Camera *camera, > [[maybe_unused]] const ControlList *controls) > { > - /* \todo Start reading the virtual video if any. */ > + /* Start reading the images/generating test patterns */ > VirtualCameraData *data = cameraData(camera); > > data->frameGenerator_->configure(data->stream_.configuration().size); > @@ -219,8 +223,8 @@ int PipelineHandlerVirtual::queueRequestDevice([[maybe_unused]] Camera *camera, > > /* \todo Read from the virtual video if any. */ > for (auto const &[stream, buffer] : request->buffers()) { > - /* map buffer and fill test patterns */ > - data->frameGenerator_->generateFrame(stream->configuration().size, buffer); > + /* Map buffer. Fill test patterns or images */ > + data->frameGenerator_->generateFrame(data->frameCount_, stream->configuration().size, buffer); > completeBuffer(request, buffer); > } > > @@ -250,9 +254,10 @@ bool PipelineHandlerVirtual::match([[maybe_unused]] DeviceEnumerator *enumerator > /* Configure and register cameras with configData */ > for (auto &data : configData) { > std::set<Stream *> streams{ &data->stream_ }; > - std::string id = data->id_; > + std::string id = data->config_.id; > std::shared_ptr<Camera> camera = Camera::create(std::move(data), id, streams); > > + /* Initialize FrameGenerator*/ > initFrameGenerator(camera.get()); > > registerCamera(std::move(camera)); > @@ -264,13 +269,19 @@ bool PipelineHandlerVirtual::match([[maybe_unused]] DeviceEnumerator *enumerator > void PipelineHandlerVirtual::initFrameGenerator(Camera *camera) > { > auto data = cameraData(camera); > - if (data->testPattern_ == TestPattern::DiagonalLines) { > - data->frameGenerator_ = DiagonalLinesGenerator::create(); > - } else { > - data->frameGenerator_ = ColorBarsGenerator::create(); > + auto &frame = data->config_.frame; > + if (ifTestPattern(frame)) { > + TestPattern &testPattern = getTestPattern(frame); > + if (testPattern == TestPattern::DiagonalLines) { > + data->frameGenerator_ = DiagonalLinesGenerator::create(); > + } else { > + data->frameGenerator_ = ColorBarsGenerator::create(); > + } > + } else if (ifImageFrames(frame)) { > + data->frameGenerator_ = ImageFrameGenerator::create(getImageFrames(frame)); > } I think I would strongly prefer `std::visit()` here. But even `std::get_if()` is better in my opinion. > } > > REGISTER_PIPELINE_HANDLER(PipelineHandlerVirtual, "virtual") > > -} /* namespace libcamera */ > +} // namespace libcamera > diff --git a/src/libcamera/pipeline/virtual/virtual.h b/src/libcamera/pipeline/virtual/virtual.h > index c1ac4eb90..f41a4a906 100644 > --- a/src/libcamera/pipeline/virtual/virtual.h > +++ b/src/libcamera/pipeline/virtual/virtual.h > @@ -7,16 +7,22 @@ > > #pragma once > > +#include <variant> > + > #include <libcamera/base/file.h> > > #include "libcamera/internal/camera.h" > #include "libcamera/internal/dma_buf_allocator.h" > #include "libcamera/internal/pipeline_handler.h" > > +#include "frame_generator.h" > +#include "image_frame_generator.h" > #include "test_pattern_generator.h" > > namespace libcamera { > > +using VirtualFrame = std::variant<TestPattern, ImageFrames>; > + > class VirtualCameraData : public Camera::Private > { > public: > @@ -24,6 +30,13 @@ public: > Size size; > std::vector<int> frameRates; > }; > + /* The config file is parsed to the Configuration struct */ > + struct Configuration { > + std::string id; > + std::vector<Resolution> resolutions; > + VirtualFrame frame; > + }; > + > VirtualCameraData(PipelineHandler *pipe) > : Camera::Private(pipe) > { > @@ -31,12 +44,9 @@ public: > > ~VirtualCameraData() = default; > > - std::string id_; > - std::vector<Resolution> supportedResolutions_; > - TestPattern testPattern_; > - > + unsigned int frameCount_ = 0; > + Configuration config_; > Stream stream_; > - > std::unique_ptr<FrameGenerator> frameGenerator_; > }; > > -- > 2.46.0.184.g6999bdac58-goog > Regards, Barnabás Pőcze
Thank you Barnabás for the review :) Uploaded to gitlab first: https://gitlab.freedesktop.org/chenghaoyang/libcamera/-/commit/b7342672e14438a72ab5de00174440862ff33c68 Will submit another series of patches after the question below is resolved. On Wed, Aug 21, 2024 at 4:06 AM Barnabás Pőcze <pobrn@protonmail.com> wrote: > Hi > > > 2024. augusztus 20., kedd 18:23 keltezéssel, Harvey Yang < > chenghaoyang@chromium.org> írta: > > > From: Konami Shu <konamiz@google.com> > > > > - Refactor ImageFrameGenerator and TestPatternGenerator > > - Extend the config file to have section to choose to use images or test > > pattern > > - Extend Parser to parse the configuration > > - Add ImageFrameGenerator which uses images to provide frames > > > > Patchset1->2 > > - Extend the parser to accept files with ".jpeg" > > - Heap allocate some buffers in ImageFrameGenerator::generateFrame so > > the buffers won't cause stack over flow > > > > Patchset5->6 > > - Move static factory function from the interface class to the derived > > classes > > > > Patchset6->8 > > - create FrameGenerator before starting the camera > > > > Signed-off-by: Konami Shu <konamiz@google.com> > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org> > > Co-developed-by: Yunke Cao <yunkec@chromium.org> > > Co-developed-by: Tomasz Figa <tfiga@chromium.org> > > --- > > src/libcamera/pipeline/virtual/README.md | 18 +- > > .../pipeline/virtual/common_functions.cpp | 27 +++ > > .../pipeline/virtual/common_functions.h | 18 ++ > > .../pipeline/virtual/frame_generator.h | 2 +- > > .../virtual/image_frame_generator.cpp | 154 ++++++++++++++++++ > > .../pipeline/virtual/image_frame_generator.h | 65 ++++++++ > > src/libcamera/pipeline/virtual/meson.build | 6 +- > > src/libcamera/pipeline/virtual/parser.cpp | 77 +++++++-- > > src/libcamera/pipeline/virtual/parser.h | 5 +- > > .../virtual/test_pattern_generator.cpp | 4 +- > > .../pipeline/virtual/test_pattern_generator.h | 2 +- > > src/libcamera/pipeline/virtual/virtual.cpp | 37 +++-- > > src/libcamera/pipeline/virtual/virtual.h | 20 ++- > > 13 files changed, 390 insertions(+), 45 deletions(-) > > create mode 100644 src/libcamera/pipeline/virtual/common_functions.cpp > > create mode 100644 src/libcamera/pipeline/virtual/common_functions.h > > create mode 100644 > src/libcamera/pipeline/virtual/image_frame_generator.cpp > > create mode 100644 > src/libcamera/pipeline/virtual/image_frame_generator.h > > > > diff --git a/src/libcamera/pipeline/virtual/README.md > b/src/libcamera/pipeline/virtual/README.md > > index 27d6283df..5e21ce74a 100644 > > --- a/src/libcamera/pipeline/virtual/README.md > > +++ b/src/libcamera/pipeline/virtual/README.md > > @@ -15,8 +15,13 @@ Each camera block is a dictionary, containing the > following keys: > > - `supported_formats` (list of `VirtualCameraData::Resolution`, > optional) : List of supported resolution and frame rates of the emulated > camera > > - `width` (`unsigned int`, default=1920): Width of the window > resolution. This needs to be even. > > - `height` (`unsigned int`, default=1080): Height of the window > resolution. > > - - `frame_rates` (list of `int`, default=`[30,60]` ): Range of the > frame rate. The list has to be two values of the lower bound and the upper > bound of the frame rate. > > -- `test_pattern` (`string`, default="bars"): Which test pattern to use > as frames. The options are "bars", "lines". > > + - `frame_rates` (list of `int`, default=`[30,60]` ): Range of the > frame rate. The list has to be two values of the lower bound and the upper > bound of the frame rate. This does not affect the frame rate for now. > > +- `frames` (dictionary): > > + - `path` (`string`, default="bars"): Name of a test pattern, path to > an image, or path to a directory of a series of images. > > + - The test patterns are "bars" which means color bars, and "lines" > which means diagonal lines. > > + - The path to an image has ".jpg" extension. > > + - 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. > > + - `scale_mode`(`string`, default="fill"): Scale mode when the frames > are images. The scale modes are "fill", "contain", and "cover". This does > not matter when frames is a test pattern. This does not affect the scale > mode for now. > > - `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. > > - `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. > > > > @@ -35,13 +40,16 @@ A sample config file: > > frame_rates: > > - 70 > > - 80 > > - test_pattern: "bars" > > + frames: > > + path: "lines" > > location: "front" > > model: "Virtual Video Device" > > "Virtual1": > > supported_formats: > > - width: 800 > > - test_pattern: "lines" > > + frames: > > + path: "path/to/directory_of_images/" > > + scale_mode: "contain" > > location: "back" > > model: "Virtual Video Device1" > > "Virtual2": > > @@ -61,7 +69,7 @@ This is the procedure of the Parser class: > > - If the config file contains invalid configuration, this method > returns nullptr. The camera will be skipped. > > 3. Parse each property and register the data. > > - `parseSupportedFormats()`: Parses `supported_formats` in the > config, which contains resolutions and frame rates. > > - - `parseTestPattern()`: Parses `test_pattern` in the config. > > + - `parseFrame()`: Parses `frames` in the config. > > - `parseLocation()`: Parses `location` in the config. > > - `parseModel()`: Parses `model` in the config. > > 4. Back to `parseConfigFile()` and append the camera configuration. > > diff --git a/src/libcamera/pipeline/virtual/common_functions.cpp > b/src/libcamera/pipeline/virtual/common_functions.cpp > > new file mode 100644 > > index 000000000..207827ee0 > > --- /dev/null > > +++ b/src/libcamera/pipeline/virtual/common_functions.cpp > > @@ -0,0 +1,27 @@ > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > +/* > > + * Copyright (C) 2023, Google Inc. > > + * > > + * common_functions.cpp - Helper that do not depend on any class > > + */ > > + > > +#include "common_functions.h" > > + > > +namespace libcamera { > > + > > +std::string getExtension(const std::string &path) > > This looks very fragile. Why not use `std::filesystem::path::extension()`? > > Right, updated. > > > +{ > > + size_t i = path.find("."); > > + if (i != std::string::npos) { > > + return path.substr(i); > > + } > > + return ""; > > +} > > + > > +std::size_t numberOfFilesInDirectory(std::filesystem::path path) > > +{ > > + using std::filesystem::directory_iterator; > > + return std::distance(directory_iterator(path), > directory_iterator{}); > > +} > > + > > +} // namespace libcamera > > diff --git a/src/libcamera/pipeline/virtual/common_functions.h > b/src/libcamera/pipeline/virtual/common_functions.h > > new file mode 100644 > > index 000000000..4203f9505 > > --- /dev/null > > +++ b/src/libcamera/pipeline/virtual/common_functions.h > > @@ -0,0 +1,18 @@ > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > +/* > > + * Copyright (C) 2023, Google Inc. > > + * > > + * common_functions.h - Helper that do not depend on any class > > + */ > > + > > +#pragma once > > + > > +#include <filesystem> > > + > > +namespace libcamera { > > + > > +std::string getExtension(const std::string &path); > > + > > +std::size_t numberOfFilesInDirectory(std::filesystem::path path); > > + > > +} // namespace libcamera > > diff --git a/src/libcamera/pipeline/virtual/frame_generator.h > b/src/libcamera/pipeline/virtual/frame_generator.h > > index 9699af7a4..f69576b36 100644 > > --- a/src/libcamera/pipeline/virtual/frame_generator.h > > +++ b/src/libcamera/pipeline/virtual/frame_generator.h > > @@ -23,7 +23,7 @@ public: > > /** Fill the output frame buffer. > > * Use the frame at the frameCount of image frames > > */ > > - virtual void generateFrame(const Size &size, > > + virtual void generateFrame(unsigned int &frameCount, const Size > &size, > > const FrameBuffer *buffer) = 0; > > > > protected: > > diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.cpp > b/src/libcamera/pipeline/virtual/image_frame_generator.cpp > > new file mode 100644 > > index 000000000..d374877ff > > --- /dev/null > > +++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp > > @@ -0,0 +1,154 @@ > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > +/* > > + * Copyright (C) 2023, Google Inc. > > + * > > + * image_frame_generator.cpp - Derived class of FrameGenerator for > > + * generating frames from images > > + */ > > + > > +#include "image_frame_generator.h" > > + > > +#include <memory> > > +#include <optional> > > +#include <string> > > + > > +#include <libcamera/base/file.h> > > +#include <libcamera/base/log.h> > > + > > +#include <libcamera/framebuffer.h> > > + > > +#include "libcamera/internal/mapped_framebuffer.h" > > + > > +#include "libyuv/convert.h" > > +#include "libyuv/scale.h" > > +namespace libcamera { > > + > > +LOG_DECLARE_CATEGORY(Virtual) > > + > > +std::unique_ptr<ImageFrameGenerator> ImageFrameGenerator::create( > > + ImageFrames &imageFrames) > > +{ > > + std::unique_ptr<ImageFrameGenerator> imageFrameGenerator = > > + std::make_unique<ImageFrameGenerator>(); > > + imageFrameGenerator->imageFrames_ = &imageFrames; > > + > > + /** For each file in the directory > > + * load the image, convert it to NV12, and store the pointer > > + */ > > + for (unsigned int i = 0; i < imageFrames.number.value_or(1); i++) { > > + std::string path; > > + if (!imageFrames.number.has_value()) { > > + /* If the path is to an image */ > > + path = imageFrames.path; > > + } else { > > + /* If the path is to a directory */ > > + path = constructPath(imageFrames.path, i); > > + } > > + > > + File file(path); > > + bool isOpen = file.open(File::OpenModeFlag::ReadOnly); > > + if (!isOpen) { > > I think `if (!file.open(...))` is fine. > > Done. > > > + LOG(Virtual, Error) << "Failed to open image file: > " << file.fileName(); > > + return nullptr; > > + } > > + > > + /* Read the image file to data */ > > + uint8_t buffer[file.size()]; > > It's probably best to avoid VLAs. > > Right, replaced with std::vector. Please check again. > > > + Span<unsigned char> data{ buffer, (unsigned > long)file.size() }; > > + long dataSize = file.read(data); > > + > > + /* Get the width and height of the image */ > > + int width, height; > > + if (libyuv::MJPGSize(data.data(), dataSize, &width, > &height)) { > > + LOG(Virtual, Error) << "Failed to get the size of > the image file: " > > + << file.fileName(); > > + return nullptr; > > + } > > + > > + /* Convert to NV12 and write the data to tmpY and tmpUV */ > > + int halfWidth = (width + 1) / 2; > > + int halfHeight = (height + 1) / 2; > > + std::unique_ptr<uint8_t[]> dstY = > > + std::make_unique<uint8_t[]>(width * height); > > + std::unique_ptr<uint8_t[]> dstUV = > > + std::make_unique<uint8_t[]>(halfWidth * halfHeight > * 2); > > + int ret = libyuv::MJPGToNV12(data.data(), dataSize, > > + dstY.get(), width, > dstUV.get(), > > + width, width, height, width, > height); > > + if (ret != 0) { > > + LOG(Virtual, Error) << "MJPGToNV12() failed with " > << ret; > > + } > > + > > + imageFrameGenerator->imageFrameDatas_.emplace_back( > > + ImageFrameData{ std::move(dstY), std::move(dstUV), > > + Size(width, height) }); > > + } > > + return imageFrameGenerator; > > +} > > + > > +std::string ImageFrameGenerator::constructPath(std::string &name, > unsigned int &i) > > Does this need to be in `ImageFrameGenerator`? Is it not enough to have it > in an > anonymous namespace in this file? > > Right, done. > > > +{ > > + return name + std::to_string(i) + ".jpg"; > > +} > > + > > +void ImageFrameGenerator::configure(const Size &size) > > +{ > > + for (unsigned int i = 0; i < imageFrames_->number.value_or(1); > i++) { > > + /* Scale the imageFrameDatas_ to scaledY and scaledUV */ > > + int halfSizeWidth = (size.width + 1) / 2; > > + int halfSizeHeight = (size.height + 1) / 2; > > + std::unique_ptr<uint8_t[]> scaledY = > > + std::make_unique<uint8_t[]>(size.width * > size.height); > > + std::unique_ptr<uint8_t[]> scaledUV = > > + std::make_unique<uint8_t[]>(halfSizeWidth * > halfSizeHeight * 2); > > + auto &src = imageFrameDatas_[i]; > > + > > + /* > > + * \todo Implement "contain" & "cover", based on > > + * |imageFrames_[i].scaleMode|. > > + */ > > + > > + /* > > + * \todo Some platforms might enforce stride due to GPU, > like > > + * ChromeOS ciri (64). The weight needs to be a > multiple of > > + * the stride to work properly for now. > > + */ > > + libyuv::NV12Scale(src.Y.get(), src.size.width, > > + src.UV.get(), src.size.width, > > + src.size.width, src.size.height, > > + scaledY.get(), size.width, > scaledUV.get(), size.width, > > + size.width, size.height, > libyuv::FilterMode::kFilterBilinear); > > + > > + /* Store the pointers to member variable */ > > + scaledFrameDatas_.emplace_back( > > + ImageFrameData{ std::move(scaledY), > std::move(scaledUV), size }); > > + } > > +} > > + > > +void ImageFrameGenerator::generateFrame(unsigned int &frameCount, const > Size &size, const FrameBuffer *buffer) > > +{ > > + /* Don't do anything when the list of buffers is empty*/ > > + if (scaledFrameDatas_.size() == 0) > > + return; > > + > > + MappedFrameBuffer mappedFrameBuffer(buffer, > MappedFrameBuffer::MapFlag::Write); > > + > > + auto planes = mappedFrameBuffer.planes(); > > + > > + /* Make sure the frameCount does not over the number of images */ > > + frameCount %= imageFrames_->number.value_or(1); > > + > > + /* Write the scaledY and scaledUV to the mapped frame buffer */ > > + libyuv::NV12Copy(scaledFrameDatas_[frameCount].Y.get(), size.width, > > + scaledFrameDatas_[frameCount].UV.get(), > size.width, planes[0].begin(), > > + size.width, planes[1].begin(), size.width, > > + size.width, size.height); > > + > > + /* proceed an image every 4 frames */ > > + /* \todo read the parameter_ from the configuration file? */ > > + parameter_++; > > + if (parameter_ % 4 == 0) > > + frameCount++; > > +} > > + > > +} // namespace libcamera > > diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.h > b/src/libcamera/pipeline/virtual/image_frame_generator.h > > new file mode 100644 > > index 000000000..74468e075 > > --- /dev/null > > +++ b/src/libcamera/pipeline/virtual/image_frame_generator.h > > @@ -0,0 +1,65 @@ > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > +/* > > + * Copyright (C) 2023, Google Inc. > > + * > > + * image_frame_generator.h - Derived class of FrameGenerator for > > + * generating frames from images > > + */ > > + > > +#pragma once > > + > > +#include <memory> > > +#include <optional> > > +#include <stdint.h> > > +#include <sys/types.h> > > + > > +#include "frame_generator.h" > > + > > +namespace libcamera { > > + > > +enum class ScaleMode : char { > > + Fill = 0, > > + Contain = 1, > > + Cover = 2, > > +}; > > + > > +/* Frame configuration provided by the config file */ > > +struct ImageFrames { > > + std::string path; > > I think `std::filesystem::path` would be preferable. > > Done. > > > + ScaleMode scaleMode; > > + std::optional<unsigned int> number; > > +}; > > + > > +class ImageFrameGenerator : public FrameGenerator > > +{ > > +public: > > + /** Factory function to create an ImageFrameGenerator object. > > + * Read the images and convert them to buffers in NV12 format. > > + * Store the pointers to the buffers to a list (imageFrameDatas) > > + */ > > + static std::unique_ptr<ImageFrameGenerator> create(ImageFrames > &imageFrames); > > + > > +private: > > + struct ImageFrameData { > > + std::unique_ptr<uint8_t[]> Y; > > + std::unique_ptr<uint8_t[]> UV; > > + Size size; > > + }; > > + > > + /* Scale the buffers for image frames. */ > > + void configure(const Size &size) override; > > + void generateFrame(unsigned int &frameCount, const Size &size, > const FrameBuffer *buffer) override; > > + > > + static std::string constructPath(std::string &name, unsigned int > &i); > > + > > + /* List of pointers to the not scaled image buffers */ > > + std::vector<ImageFrameData> imageFrameDatas_; > > + /* List of pointers to the scaled image buffers */ > > + std::vector<ImageFrameData> scaledFrameDatas_; > > + /* Pointer to the imageFrames_ in VirtualCameraData */ > > + ImageFrames *imageFrames_; > > + /* Speed parameter. Change to the next image every parameter_ > frames. */ > > + int parameter_; > > +}; > > + > > +} /* namespace libcamera */ > > diff --git a/src/libcamera/pipeline/virtual/meson.build > b/src/libcamera/pipeline/virtual/meson.build > > index 2e82e64cb..d56aedec4 100644 > > --- a/src/libcamera/pipeline/virtual/meson.build > > +++ b/src/libcamera/pipeline/virtual/meson.build > > @@ -2,11 +2,14 @@ > > > > libcamera_sources += files([ > > 'virtual.cpp', > > - 'test_pattern_generator.cpp', > > 'parser.cpp', > > + 'test_pattern_generator.cpp', > > + 'image_frame_generator.cpp', > > + 'common_functions.cpp', > > ]) > > > > libyuv_dep = dependency('libyuv', required : false) > > +libjpeg = dependency('libjpeg', required : false) > > > > # Fallback to a subproject if libyuv isn't found, as it's typically not > > # provided by distributions. > > @@ -26,3 +29,4 @@ if not libyuv_dep.found() > > endif > > > > libcamera_deps += [libyuv_dep] > > +libcamera_deps += [libjpeg] > > diff --git a/src/libcamera/pipeline/virtual/parser.cpp > b/src/libcamera/pipeline/virtual/parser.cpp > > index 032c0cd9d..46d1ab181 100644 > > --- a/src/libcamera/pipeline/virtual/parser.cpp > > +++ b/src/libcamera/pipeline/virtual/parser.cpp > > @@ -18,6 +18,7 @@ > > #include "libcamera/internal/pipeline_handler.h" > > #include "libcamera/internal/yaml_parser.h" > > > > +#include "common_functions.h" > > #include "virtual.h" > > > > namespace libcamera { > > @@ -52,12 +53,12 @@ std::vector<std::unique_ptr<VirtualCameraData>> > Parser::parseConfigFile( > > continue; > > } > > > > - data->id_ = cameraId; > > + data->config_.id = cameraId; > > ControlInfoMap::Map controls; > > /* todo: Check which resolution's frame rate to be > reported */ > > controls[&controls::FrameDurationLimits] = > > - ControlInfo(int64_t(1000 / > data->supportedResolutions_[0].frameRates[1]), > > - int64_t(1000 / > data->supportedResolutions_[0].frameRates[0])); > > + ControlInfo(int64_t(1000 / > data->config_.resolutions[0].frameRates[1]), > > + int64_t(1000 / > data->config_.resolutions[0].frameRates[0])); > > data->controlInfo_ = ControlInfoMap(std::move(controls), > controls::controls); > > configurations.push_back(std::move(data)); > > } > > @@ -72,7 +73,7 @@ std::unique_ptr<VirtualCameraData> > Parser::parseCameraConfigData( > > if (parseSupportedFormats(cameraConfigData, data.get())) > > return nullptr; > > > > - if (parseTestPattern(cameraConfigData, data.get())) > > + if (parseFrame(cameraConfigData, data.get())) > > return nullptr; > > > > if (parseLocation(cameraConfigData, data.get())) > > @@ -122,14 +123,14 @@ int Parser::parseSupportedFormats( > > frameRates.push_back(60); > > } > > > > - data->supportedResolutions_.emplace_back( > > + data->config_.resolutions.emplace_back( > > VirtualCameraData::Resolution{ Size{ > width, height }, > > frameRates > }); > > > > activeResolution = std::max(activeResolution, > Size{ width, height }); > > } > > } else { > > - data->supportedResolutions_.emplace_back( > > + data->config_.resolutions.emplace_back( > > VirtualCameraData::Resolution{ Size{ 1920, 1080 }, > > { 30, 60 } }); > > activeResolution = Size(1920, 1080); > > @@ -141,21 +142,65 @@ int Parser::parseSupportedFormats( > > return 0; > > } > > > > -int Parser::parseTestPattern( > > +int Parser::parseFrame( > > const YamlObject &cameraConfigData, VirtualCameraData *data) > > { > > - std::string testPattern = > cameraConfigData["test_pattern"].get<std::string>().value(); > > + const YamlObject &frames = cameraConfigData["frames"]; > > + /* When there is no frames provided in the config file, use color > bar test pattern */ > > + if (frames.size() == 0) { > > + data->config_.frame = TestPattern::ColorBars; > > + return 0; > > + } > > + > > + if (!frames.isDictionary()) { > > + LOG(Virtual, Error) << "'frames' is not a dictionary."; > > + return -EINVAL; > > + } > > + > > + std::string path = frames["path"].get<std::string>().value(); > > What if `path` is not specified? Is it checked somewhere else? > > The default value of `empty.get<std::string>()` is actually an empty std::string, instead of std::nullopt [1] [2]. However, you're right that it's confusing. Updated with a default value `""` instead. [1]: https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/yaml_parser.cpp#n46 [2]: https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/yaml_parser.cpp#n313 > > > + > > + if (auto ext = getExtension(path); ext == ".jpg" || ext == > ".jpeg") { > > + ScaleMode scaleMode; > > + if (parseScaleMode(frames, &scaleMode)) > > + return -EINVAL; > > + data->config_.frame = ImageFrames{ path, scaleMode, > std::nullopt }; > > + } else if (path.back() == '/') { > > This looks very fragile. Why not use `std::filesystem::symlink_status()` > to check > if it's a file or a directory. > > Thanks! Done. > > > + ScaleMode scaleMode; > > + if (parseScaleMode(frames, &scaleMode)) > > + return -EINVAL; > > + data->config_.frame = ImageFrames{ path, scaleMode, > > + > numberOfFilesInDirectory(path) }; > > Admittedly I did not check in detail, but is this necessary? Is it > necessary > to count the number of files in advance? > > Hmm, the logic of this is to loop the images in the directory, without setting the number of images in the directory. Are you suggesting to count the number when we really use `data->config_`? That also works, while the user of ImageFrame would need to check if `ImageFrame.path` is a file or directory again. > > + } else if (path == "bars" || path == "") { > > + /* Default value is "bars" */ > > + data->config_.frame = TestPattern::ColorBars; > > + } else if (path == "lines") { > > + data->config_.frame = TestPattern::DiagonalLines; > > + } else { > > + LOG(Virtual, Error) << "Frame: " << path > > + << " is not supported"; > > + return -EINVAL; > > + } > > + return 0; > > +} > > > > - /* Default value is "bars" */ > > - if (testPattern == "bars" || testPattern == "") { > > - data->testPattern_ = TestPattern::ColorBars; > > - } else if (testPattern == "lines") { > > - data->testPattern_ = TestPattern::DiagonalLines; > > +int Parser::parseScaleMode( > > + const YamlObject &framesConfigData, ScaleMode *scaleMode) > > +{ > > + std::string mode = > framesConfigData["scale_mode"].get<std::string>().value(); > > Same here, regarding empty optionals. > > Updated, as well as other optionals. > > > + > > + /* Default value is fill */ > > + if (mode == "fill" || mode == "") { > > + *scaleMode = ScaleMode::Fill; > > + } else if (mode == "contain") { > > + *scaleMode = ScaleMode::Contain; > > + } else if (mode == "cover") { > > + *scaleMode = ScaleMode::Cover; > > } else { > > - LOG(Virtual, Error) << "Test pattern: " << testPattern > > - << "is not supported"; > > + LOG(Virtual, Error) << "scaleMode: " << mode > > + << " is not supported"; > > return -EINVAL; > > } > > + > > return 0; > > } > > > > @@ -195,4 +240,4 @@ int Parser::parseModel( > > return 0; > > } > > > > -} /* namespace libcamera */ > > +} // namespace libcamera > > diff --git a/src/libcamera/pipeline/virtual/parser.h > b/src/libcamera/pipeline/virtual/parser.h > > index a377d8aa1..38ea460d5 100644 > > --- a/src/libcamera/pipeline/virtual/parser.h > > +++ b/src/libcamera/pipeline/virtual/parser.h > > @@ -34,12 +34,15 @@ private: > > > > int parseSupportedFormats( > > const YamlObject &cameraConfigData, VirtualCameraData > *data); > > - int parseTestPattern( > > + int parseFrame( > > const YamlObject &cameraConfigData, VirtualCameraData > *data); > > int parseLocation( > > const YamlObject &cameraConfigData, VirtualCameraData > *data); > > int parseModel( > > const YamlObject &cameraConfigData, VirtualCameraData > *data); > > + > > + int parseScaleMode( > > + const YamlObject &framesConfigData, ScaleMode *scaleMode); > > }; > > > > } // namespace libcamera > > diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > > index 6df9b31e9..844dffd49 100644 > > --- a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > > +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > > @@ -20,7 +20,7 @@ LOG_DECLARE_CATEGORY(Virtual) > > static const unsigned int kARGBSize = 4; > > > > void TestPatternGenerator::generateFrame( > > - const Size &size, > > + [[maybe_unused]] unsigned int &frameCount, const Size &size, > > const FrameBuffer *buffer) > > { > > MappedFrameBuffer mappedFrameBuffer(buffer, > > @@ -29,7 +29,7 @@ void TestPatternGenerator::generateFrame( > > auto planes = mappedFrameBuffer.planes(); > > > > /* TODO: select whether to do shifting or not */ > > - shiftLeft(size); > > + // shiftLeft(size); > > > > /* Convert the template_ to the frame buffer */ > > int ret = libyuv::ARGBToNV12( > > diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.h > b/src/libcamera/pipeline/virtual/test_pattern_generator.h > > index ed8d4e43b..cebdd0141 100644 > > --- a/src/libcamera/pipeline/virtual/test_pattern_generator.h > > +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.h > > @@ -25,7 +25,7 @@ enum class TestPattern : char { > > class TestPatternGenerator : public FrameGenerator > > { > > private: > > - void generateFrame(const Size &size, > > + void generateFrame(unsigned int &frameCount, const Size &size, > > const FrameBuffer *buffer) override; > > > > protected: > > diff --git a/src/libcamera/pipeline/virtual/virtual.cpp > b/src/libcamera/pipeline/virtual/virtual.cpp > > index 0fe471f00..2412af703 100644 > > --- a/src/libcamera/pipeline/virtual/virtual.cpp > > +++ b/src/libcamera/pipeline/virtual/virtual.cpp > > @@ -20,9 +20,13 @@ > > #include "libcamera/internal/pipeline_handler.h" > > #include "libcamera/internal/yaml_parser.h" > > > > -#include "frame_generator.h" > > #include "parser.h" > > > > +#define ifTestPattern(v) std::holds_alternative<TestPattern>(v) > > +#define getTestPattern(v) std::get<TestPattern>(v) > > +#define ifImageFrames(v) std::holds_alternative<ImageFrames>(v) > > +#define getImageFrames(v) std::get<ImageFrames>(v) > > + > > namespace libcamera { > > > > LOG_DEFINE_CATEGORY(Virtual) > > @@ -63,12 +67,12 @@ CameraConfiguration::Status > VirtualCameraConfiguration::validate() > > } > > > > Size maxSize; > > - for (const auto &resolution : data_->supportedResolutions_) > > + for (const auto &resolution : data_->config_.resolutions) > > maxSize = std::max(maxSize, resolution.size); > > > > for (StreamConfiguration &cfg : config_) { > > bool found = false; > > - for (const auto &resolution : > data_->supportedResolutions_) { > > + for (const auto &resolution : data_->config_.resolutions) { > > if (resolution.size.width == cfg.size.width && > > resolution.size.height == cfg.size.height) { > > found = true; > > @@ -110,7 +114,7 @@ PipelineHandlerVirtual::generateConfiguration(Camera > *camera, > > return config; > > > > Size minSize, sensorResolution; > > - for (const auto &resolution : data->supportedResolutions_) { > > + for (const auto &resolution : data->config_.resolutions) { > > if (minSize.isNull() || minSize > resolution.size) > > minSize = resolution.size; > > > > @@ -199,7 +203,7 @@ int PipelineHandlerVirtual::exportFrameBuffers( > > int PipelineHandlerVirtual::start(Camera *camera, > > [[maybe_unused]] const ControlList > *controls) > > { > > - /* \todo Start reading the virtual video if any. */ > > + /* Start reading the images/generating test patterns */ > > VirtualCameraData *data = cameraData(camera); > > > > > data->frameGenerator_->configure(data->stream_.configuration().size); > > @@ -219,8 +223,8 @@ int > PipelineHandlerVirtual::queueRequestDevice([[maybe_unused]] Camera *camera, > > > > /* \todo Read from the virtual video if any. */ > > for (auto const &[stream, buffer] : request->buffers()) { > > - /* map buffer and fill test patterns */ > > - > data->frameGenerator_->generateFrame(stream->configuration().size, buffer); > > + /* Map buffer. Fill test patterns or images */ > > + data->frameGenerator_->generateFrame(data->frameCount_, > stream->configuration().size, buffer); > > completeBuffer(request, buffer); > > } > > > > @@ -250,9 +254,10 @@ bool PipelineHandlerVirtual::match([[maybe_unused]] > DeviceEnumerator *enumerator > > /* Configure and register cameras with configData */ > > for (auto &data : configData) { > > std::set<Stream *> streams{ &data->stream_ }; > > - std::string id = data->id_; > > + std::string id = data->config_.id; > > std::shared_ptr<Camera> camera = > Camera::create(std::move(data), id, streams); > > > > + /* Initialize FrameGenerator*/ > > initFrameGenerator(camera.get()); > > > > registerCamera(std::move(camera)); > > @@ -264,13 +269,19 @@ bool > PipelineHandlerVirtual::match([[maybe_unused]] DeviceEnumerator *enumerator > > void PipelineHandlerVirtual::initFrameGenerator(Camera *camera) > > { > > auto data = cameraData(camera); > > - if (data->testPattern_ == TestPattern::DiagonalLines) { > > - data->frameGenerator_ = DiagonalLinesGenerator::create(); > > - } else { > > - data->frameGenerator_ = ColorBarsGenerator::create(); > > + auto &frame = data->config_.frame; > > + if (ifTestPattern(frame)) { > > + TestPattern &testPattern = getTestPattern(frame); > > + if (testPattern == TestPattern::DiagonalLines) { > > + data->frameGenerator_ = > DiagonalLinesGenerator::create(); > > + } else { > > + data->frameGenerator_ = > ColorBarsGenerator::create(); > > + } > > + } else if (ifImageFrames(frame)) { > > + data->frameGenerator_ = > ImageFrameGenerator::create(getImageFrames(frame)); > > } > > I think I would strongly prefer `std::visit()` here. But even > `std::get_if()` is better in my opinion. > > Thanks for the suggestion. I'm quite a newbie to use std::variant. Please take another look. > > > } > > > > REGISTER_PIPELINE_HANDLER(PipelineHandlerVirtual, "virtual") > > > > -} /* namespace libcamera */ > > +} // namespace libcamera > > diff --git a/src/libcamera/pipeline/virtual/virtual.h > b/src/libcamera/pipeline/virtual/virtual.h > > index c1ac4eb90..f41a4a906 100644 > > --- a/src/libcamera/pipeline/virtual/virtual.h > > +++ b/src/libcamera/pipeline/virtual/virtual.h > > @@ -7,16 +7,22 @@ > > > > #pragma once > > > > +#include <variant> > > + > > #include <libcamera/base/file.h> > > > > #include "libcamera/internal/camera.h" > > #include "libcamera/internal/dma_buf_allocator.h" > > #include "libcamera/internal/pipeline_handler.h" > > > > +#include "frame_generator.h" > > +#include "image_frame_generator.h" > > #include "test_pattern_generator.h" > > > > namespace libcamera { > > > > +using VirtualFrame = std::variant<TestPattern, ImageFrames>; > > + > > class VirtualCameraData : public Camera::Private > > { > > public: > > @@ -24,6 +30,13 @@ public: > > Size size; > > std::vector<int> frameRates; > > }; > > + /* The config file is parsed to the Configuration struct */ > > + struct Configuration { > > + std::string id; > > + std::vector<Resolution> resolutions; > > + VirtualFrame frame; > > + }; > > + > > VirtualCameraData(PipelineHandler *pipe) > > : Camera::Private(pipe) > > { > > @@ -31,12 +44,9 @@ public: > > > > ~VirtualCameraData() = default; > > > > - std::string id_; > > - std::vector<Resolution> supportedResolutions_; > > - TestPattern testPattern_; > > - > > + unsigned int frameCount_ = 0; > > + Configuration config_; > > Stream stream_; > > - > > std::unique_ptr<FrameGenerator> frameGenerator_; > > }; > > > > -- > > 2.46.0.184.g6999bdac58-goog > > > > > Regards, > Barnabás Pőcze >
Hi 2024. augusztus 21., szerda 23:55 keltezéssel, Cheng-Hao Yang <chenghaoyang@chromium.org> írta: > Thank you Barnabás for the review :) > > Uploaded to gitlab first: https://gitlab.freedesktop.org/chenghaoyang/libcamera/-/commit/b7342672e14438a72ab5de00174440862ff33c68 > Will submit another series of patches after the question below is resolved. > > On Wed, Aug 21, 2024 at 4:06 AM Barnabás Pőcze <pobrn@protonmail.com> wrote: > > Hi > > > 2024. augusztus 20., kedd 18:23 keltezéssel, Harvey Yang <chenghaoyang@chromium.org> írta: > > > From: Konami Shu <konamiz@google.com> > > > > - Refactor ImageFrameGenerator and TestPatternGenerator > > - Extend the config file to have section to choose to use images or test > > pattern > > - Extend Parser to parse the configuration > > - Add ImageFrameGenerator which uses images to provide frames > > > > Patchset1->2 > > - Extend the parser to accept files with ".jpeg" > > - Heap allocate some buffers in ImageFrameGenerator::generateFrame so > > the buffers won't cause stack over flow > > > > Patchset5->6 > > - Move static factory function from the interface class to the derived > > classes > > > > Patchset6->8 > > - create FrameGenerator before starting the camera > > > > Signed-off-by: Konami Shu <konamiz@google.com> > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org> > > Co-developed-by: Yunke Cao <yunkec@chromium.org> > > Co-developed-by: Tomasz Figa <tfiga@chromium.org> > > --- > > src/libcamera/pipeline/virtual/README.md | 18 +- > > .../pipeline/virtual/common_functions.cpp | 27 +++ > > .../pipeline/virtual/common_functions.h | 18 ++ > > .../pipeline/virtual/frame_generator.h | 2 +- > > .../virtual/image_frame_generator.cpp | 154 ++++++++++++++++++ > > .../pipeline/virtual/image_frame_generator.h | 65 ++++++++ > > src/libcamera/pipeline/virtual/meson.build | 6 +- > > src/libcamera/pipeline/virtual/parser.cpp | 77 +++++++-- > > src/libcamera/pipeline/virtual/parser.h | 5 +- > > .../virtual/test_pattern_generator.cpp | 4 +- > > .../pipeline/virtual/test_pattern_generator.h | 2 +- > > src/libcamera/pipeline/virtual/virtual.cpp | 37 +++-- > > src/libcamera/pipeline/virtual/virtual.h | 20 ++- > > 13 files changed, 390 insertions(+), 45 deletions(-) > > create mode 100644 src/libcamera/pipeline/virtual/common_functions.cpp > > create mode 100644 src/libcamera/pipeline/virtual/common_functions.h > > create mode 100644 src/libcamera/pipeline/virtual/image_frame_generator.cpp > > create mode 100644 src/libcamera/pipeline/virtual/image_frame_generator.h > > > > diff --git a/src/libcamera/pipeline/virtual/README.md b/src/libcamera/pipeline/virtual/README.md > > index 27d6283df..5e21ce74a 100644 > > --- a/src/libcamera/pipeline/virtual/README.md > > +++ b/src/libcamera/pipeline/virtual/README.md > > @@ -15,8 +15,13 @@ Each camera block is a dictionary, containing the following keys: > > - `supported_formats` (list of `VirtualCameraData::Resolution`, optional) : List of supported resolution and frame rates of the emulated camera > > - `width` (`unsigned int`, default=1920): Width of the window resolution. This needs to be even. > > - `height` (`unsigned int`, default=1080): Height of the window resolution. > > - - `frame_rates` (list of `int`, default=`[30,60]` ): Range of the frame rate. The list has to be two values of the lower bound and the upper bound of the frame rate. > > -- `test_pattern` (`string`, default="bars"): Which test pattern to use as frames. The options are "bars", "lines". > > + - `frame_rates` (list of `int`, default=`[30,60]` ): Range of the frame rate. The list has to be two values of the lower bound and the upper bound of the frame rate. This does not affect the frame rate for now. > > +- `frames` (dictionary): > > + - `path` (`string`, default="bars"): Name of a test pattern, path to an image, or path to a directory of a series of images. > > + - The test patterns are "bars" which means color bars, and "lines" which means diagonal lines. > > + - The path to an image has ".jpg" extension. > > + - 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. > > + - `scale_mode`(`string`, default="fill"): Scale mode when the frames are images. The scale modes are "fill", "contain", and "cover". This does not matter when frames is a test pattern. This does not affect the scale mode for now. > > - `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. > > - `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. > > > > @@ -35,13 +40,16 @@ A sample config file: > > frame_rates: > > - 70 > > - 80 > > - test_pattern: "bars" > > + frames: > > + path: "lines" > > location: "front" > > model: "Virtual Video Device" > > "Virtual1": > > supported_formats: > > - width: 800 > > - test_pattern: "lines" > > + frames: > > + path: "path/to/directory_of_images/" > > + scale_mode: "contain" > > location: "back" > > model: "Virtual Video Device1" > > "Virtual2": > > @@ -61,7 +69,7 @@ This is the procedure of the Parser class: > > - If the config file contains invalid configuration, this method returns nullptr. The camera will be skipped. > > 3. Parse each property and register the data. > > - `parseSupportedFormats()`: Parses `supported_formats` in the config, which contains resolutions and frame rates. > > - - `parseTestPattern()`: Parses `test_pattern` in the config. > > + - `parseFrame()`: Parses `frames` in the config. > > - `parseLocation()`: Parses `location` in the config. > > - `parseModel()`: Parses `model` in the config. > > 4. Back to `parseConfigFile()` and append the camera configuration. > > diff --git a/src/libcamera/pipeline/virtual/common_functions.cpp b/src/libcamera/pipeline/virtual/common_functions.cpp > > new file mode 100644 > > index 000000000..207827ee0 > > --- /dev/null > > +++ b/src/libcamera/pipeline/virtual/common_functions.cpp > > @@ -0,0 +1,27 @@ > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > +/* > > + * Copyright (C) 2023, Google Inc. > > + * > > + * common_functions.cpp - Helper that do not depend on any class > > + */ > > + > > +#include "common_functions.h" > > + > > +namespace libcamera { > > + > > +std::string getExtension(const std::string &path) > > This looks very fragile. Why not use `std::filesystem::path::extension()`? > > Right, updated. > > > > +{ > > + size_t i = path.find("."); > > + if (i != std::string::npos) { > > + return path.substr(i); > > + } > > + return ""; > > +} > > + > > +std::size_t numberOfFilesInDirectory(std::filesystem::path path) > > +{ > > + using std::filesystem::directory_iterator; > > + return std::distance(directory_iterator(path), directory_iterator{}); > > +} > > + > > +} // namespace libcamera > > diff --git a/src/libcamera/pipeline/virtual/common_functions.h b/src/libcamera/pipeline/virtual/common_functions.h > > new file mode 100644 > > index 000000000..4203f9505 > > --- /dev/null > > +++ b/src/libcamera/pipeline/virtual/common_functions.h > > @@ -0,0 +1,18 @@ > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > +/* > > + * Copyright (C) 2023, Google Inc. > > + * > > + * common_functions.h - Helper that do not depend on any class > > + */ > > + > > +#pragma once > > + > > +#include <filesystem> > > + > > +namespace libcamera { > > + > > +std::string getExtension(const std::string &path); > > + > > +std::size_t numberOfFilesInDirectory(std::filesystem::path path); > > + > > +} // namespace libcamera > > diff --git a/src/libcamera/pipeline/virtual/frame_generator.h b/src/libcamera/pipeline/virtual/frame_generator.h > > index 9699af7a4..f69576b36 100644 > > --- a/src/libcamera/pipeline/virtual/frame_generator.h > > +++ b/src/libcamera/pipeline/virtual/frame_generator.h > > @@ -23,7 +23,7 @@ public: > > /** Fill the output frame buffer. > > * Use the frame at the frameCount of image frames > > */ > > - virtual void generateFrame(const Size &size, > > + virtual void generateFrame(unsigned int &frameCount, const Size &size, > > const FrameBuffer *buffer) = 0; > > > > protected: > > diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.cpp b/src/libcamera/pipeline/virtual/image_frame_generator.cpp > > new file mode 100644 > > index 000000000..d374877ff > > --- /dev/null > > +++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp > > @@ -0,0 +1,154 @@ > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > +/* > > + * Copyright (C) 2023, Google Inc. > > + * > > + * image_frame_generator.cpp - Derived class of FrameGenerator for > > + * generating frames from images > > + */ > > + > > +#include "image_frame_generator.h" > > + > > +#include <memory> > > +#include <optional> > > +#include <string> > > + > > +#include <libcamera/base/file.h> > > +#include <libcamera/base/log.h> > > + > > +#include <libcamera/framebuffer.h> > > + > > +#include "libcamera/internal/mapped_framebuffer.h" > > + > > +#include "libyuv/convert.h" > > +#include "libyuv/scale.h" > > +namespace libcamera { > > + > > +LOG_DECLARE_CATEGORY(Virtual) > > + > > +std::unique_ptr<ImageFrameGenerator> ImageFrameGenerator::create( > > + ImageFrames &imageFrames) > > +{ > > + std::unique_ptr<ImageFrameGenerator> imageFrameGenerator = > > + std::make_unique<ImageFrameGenerator>(); > > + imageFrameGenerator->imageFrames_ = &imageFrames; > > + > > + /** For each file in the directory > > + * load the image, convert it to NV12, and store the pointer > > + */ > > + for (unsigned int i = 0; i < imageFrames.number.value_or(1); i++) { > > + std::string path; > > + if (!imageFrames.number.has_value()) { > > + /* If the path is to an image */ > > + path = imageFrames.path; > > + } else { > > + /* If the path is to a directory */ > > + path = constructPath(imageFrames.path, i); > > + } > > + > > + File file(path); > > + bool isOpen = file.open(File::OpenModeFlag::ReadOnly); > > + if (!isOpen) { > > I think `if (!file.open(...))` is fine. > > Done. > > > > + LOG(Virtual, Error) << "Failed to open image file: " << file.fileName(); Forgot to mention, it might be useful to print `strerror(file.error())` or similar as well. > > + return nullptr; > > + } > > + > > + /* Read the image file to data */ > > + uint8_t buffer[file.size()]; > > It's probably best to avoid VLAs. > > Right, replaced with std::vector. Please check again. Looks ok, although I would just use auto buffer = std::make_unique<uint8_t[]>(file.size()); Also, every `File::size()` incurs an `fstat()`, so you probably want to call that once and ensure that it is >= 0. > > > > + Span<unsigned char> data{ buffer, (unsigned long)file.size() }; > > + long dataSize = file.read(data); > > + > > + /* Get the width and height of the image */ > > + int width, height; > > + if (libyuv::MJPGSize(data.data(), dataSize, &width, &height)) { > > + LOG(Virtual, Error) << "Failed to get the size of the image file: " > > + << file.fileName(); > > + return nullptr; > > + } > > + > > + /* Convert to NV12 and write the data to tmpY and tmpUV */ > > + int halfWidth = (width + 1) / 2; > > + int halfHeight = (height + 1) / 2; > > + std::unique_ptr<uint8_t[]> dstY = > > + std::make_unique<uint8_t[]>(width * height); > > + std::unique_ptr<uint8_t[]> dstUV = > > + std::make_unique<uint8_t[]>(halfWidth * halfHeight * 2); > > + int ret = libyuv::MJPGToNV12(data.data(), dataSize, > > + dstY.get(), width, dstUV.get(), > > + width, width, height, width, height); > > + if (ret != 0) { > > + LOG(Virtual, Error) << "MJPGToNV12() failed with " << ret; > > + } > > + > > + imageFrameGenerator->imageFrameDatas_.emplace_back( > > + ImageFrameData{ std::move(dstY), std::move(dstUV), > > + Size(width, height) }); > > + } > > + return imageFrameGenerator; > > +} > > + > > +std::string ImageFrameGenerator::constructPath(std::string &name, unsigned int &i) > > Does this need to be in `ImageFrameGenerator`? Is it not enough to have it in an > anonymous namespace in this file? > > > Right, done. I think `return name / (std::to_string(i) + ".jpg");` would work just fine. Also, why is `i` a reference? > > > > +{ > > + return name + std::to_string(i) + ".jpg"; > > +} > > + > > +void ImageFrameGenerator::configure(const Size &size) > > +{ > > + for (unsigned int i = 0; i < imageFrames_->number.value_or(1); i++) { > > + /* Scale the imageFrameDatas_ to scaledY and scaledUV */ > > + int halfSizeWidth = (size.width + 1) / 2; > > + int halfSizeHeight = (size.height + 1) / 2; > > + std::unique_ptr<uint8_t[]> scaledY = > > + std::make_unique<uint8_t[]>(size.width * size.height); > > + std::unique_ptr<uint8_t[]> scaledUV = > > + std::make_unique<uint8_t[]>(halfSizeWidth * halfSizeHeight * 2); > > + auto &src = imageFrameDatas_[i]; > > + > > + /* > > + * \todo Implement "contain" & "cover", based on > > + * |imageFrames_[i].scaleMode|. > > + */ > > + > > + /* > > + * \todo Some platforms might enforce stride due to GPU, like > > + * ChromeOS ciri (64). The weight needs to be a multiple of > > + * the stride to work properly for now. > > + */ > > + libyuv::NV12Scale(src.Y.get(), src.size.width, > > + src.UV.get(), src.size.width, > > + src.size.width, src.size.height, > > + scaledY.get(), size.width, scaledUV.get(), size.width, > > + size.width, size.height, libyuv::FilterMode::kFilterBilinear); > > + > > + /* Store the pointers to member variable */ > > + scaledFrameDatas_.emplace_back( > > + ImageFrameData{ std::move(scaledY), std::move(scaledUV), size }); > > + } > > +} > > + > > +void ImageFrameGenerator::generateFrame(unsigned int &frameCount, const Size &size, const FrameBuffer *buffer) > > +{ > > + /* Don't do anything when the list of buffers is empty*/ > > + if (scaledFrameDatas_.size() == 0) > > + return; > > + > > + MappedFrameBuffer mappedFrameBuffer(buffer, MappedFrameBuffer::MapFlag::Write); > > + > > + auto planes = mappedFrameBuffer.planes(); > > + > > + /* Make sure the frameCount does not over the number of images */ > > + frameCount %= imageFrames_->number.value_or(1); > > + > > + /* Write the scaledY and scaledUV to the mapped frame buffer */ > > + libyuv::NV12Copy(scaledFrameDatas_[frameCount].Y.get(), size.width, > > + scaledFrameDatas_[frameCount].UV.get(), size.width, planes[0].begin(), > > + size.width, planes[1].begin(), size.width, > > + size.width, size.height); > > + > > + /* proceed an image every 4 frames */ > > + /* \todo read the parameter_ from the configuration file? */ > > + parameter_++; > > + if (parameter_ % 4 == 0) > > + frameCount++; > > +} > > + > > +} // namespace libcamera > > diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.h b/src/libcamera/pipeline/virtual/image_frame_generator.h > > new file mode 100644 > > index 000000000..74468e075 > > --- /dev/null > > +++ b/src/libcamera/pipeline/virtual/image_frame_generator.h > > @@ -0,0 +1,65 @@ > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > +/* > > + * Copyright (C) 2023, Google Inc. > > + * > > + * image_frame_generator.h - Derived class of FrameGenerator for > > + * generating frames from images > > + */ > > + > > +#pragma once > > + > > +#include <memory> > > +#include <optional> > > +#include <stdint.h> > > +#include <sys/types.h> > > + > > +#include "frame_generator.h" > > + > > +namespace libcamera { > > + > > +enum class ScaleMode : char { > > + Fill = 0, > > + Contain = 1, > > + Cover = 2, > > +}; > > + > > +/* Frame configuration provided by the config file */ > > +struct ImageFrames { > > + std::string path; > > I think `std::filesystem::path` would be preferable. > > Done. > > > > + ScaleMode scaleMode; > > + std::optional<unsigned int> number; > > +}; > > + > > +class ImageFrameGenerator : public FrameGenerator > > +{ > > +public: > > + /** Factory function to create an ImageFrameGenerator object. > > + * Read the images and convert them to buffers in NV12 format. > > + * Store the pointers to the buffers to a list (imageFrameDatas) > > + */ > > + static std::unique_ptr<ImageFrameGenerator> create(ImageFrames &imageFrames); > > + > > +private: > > + struct ImageFrameData { > > + std::unique_ptr<uint8_t[]> Y; > > + std::unique_ptr<uint8_t[]> UV; > > + Size size; > > + }; > > + > > + /* Scale the buffers for image frames. */ > > + void configure(const Size &size) override; > > + void generateFrame(unsigned int &frameCount, const Size &size, const FrameBuffer *buffer) override; > > + > > + static std::string constructPath(std::string &name, unsigned int &i); > > + > > + /* List of pointers to the not scaled image buffers */ > > + std::vector<ImageFrameData> imageFrameDatas_; > > + /* List of pointers to the scaled image buffers */ > > + std::vector<ImageFrameData> scaledFrameDatas_; > > + /* Pointer to the imageFrames_ in VirtualCameraData */ > > + ImageFrames *imageFrames_; > > + /* Speed parameter. Change to the next image every parameter_ frames. */ > > + int parameter_; > > +}; > > + > > +} /* namespace libcamera */ > > diff --git a/src/libcamera/pipeline/virtual/meson.build b/src/libcamera/pipeline/virtual/meson.build > > index 2e82e64cb..d56aedec4 100644 > > --- a/src/libcamera/pipeline/virtual/meson.build > > +++ b/src/libcamera/pipeline/virtual/meson.build > > @@ -2,11 +2,14 @@ > > > > libcamera_sources += files([ > > 'virtual.cpp', > > - 'test_pattern_generator.cpp', > > 'parser.cpp', > > + 'test_pattern_generator.cpp', > > + 'image_frame_generator.cpp', > > + 'common_functions.cpp', > > ]) > > > > libyuv_dep = dependency('libyuv', required : false) > > +libjpeg = dependency('libjpeg', required : false) > > > > # Fallback to a subproject if libyuv isn't found, as it's typically not > > # provided by distributions. > > @@ -26,3 +29,4 @@ if not libyuv_dep.found() > > endif > > > > libcamera_deps += [libyuv_dep] > > +libcamera_deps += [libjpeg] > > diff --git a/src/libcamera/pipeline/virtual/parser.cpp b/src/libcamera/pipeline/virtual/parser.cpp > > index 032c0cd9d..46d1ab181 100644 > > --- a/src/libcamera/pipeline/virtual/parser.cpp > > +++ b/src/libcamera/pipeline/virtual/parser.cpp > > @@ -18,6 +18,7 @@ > > #include "libcamera/internal/pipeline_handler.h" > > #include "libcamera/internal/yaml_parser.h" > > > > +#include "common_functions.h" > > #include "virtual.h" > > > > namespace libcamera { > > @@ -52,12 +53,12 @@ std::vector<std::unique_ptr<VirtualCameraData>> Parser::parseConfigFile( > > continue; > > } > > > > - data->id_ = cameraId; > > + data->config_.id = cameraId; > > ControlInfoMap::Map controls; > > /* todo: Check which resolution's frame rate to be reported */ > > controls[&controls::FrameDurationLimits] = > > - ControlInfo(int64_t(1000 / data->supportedResolutions_[0].frameRates[1]), > > - int64_t(1000 / data->supportedResolutions_[0].frameRates[0])); > > + ControlInfo(int64_t(1000 / data->config_.resolutions[0].frameRates[1]), > > + int64_t(1000 / data->config_.resolutions[0].frameRates[0])); > > data->controlInfo_ = ControlInfoMap(std::move(controls), controls::controls); > > configurations.push_back(std::move(data)); > > } > > @@ -72,7 +73,7 @@ std::unique_ptr<VirtualCameraData> Parser::parseCameraConfigData( > > if (parseSupportedFormats(cameraConfigData, data.get())) > > return nullptr; > > > > - if (parseTestPattern(cameraConfigData, data.get())) > > + if (parseFrame(cameraConfigData, data.get())) > > return nullptr; > > > > if (parseLocation(cameraConfigData, data.get())) > > @@ -122,14 +123,14 @@ int Parser::parseSupportedFormats( > > frameRates.push_back(60); > > } > > > > - data->supportedResolutions_.emplace_back( > > + data->config_.resolutions.emplace_back( > > VirtualCameraData::Resolution{ Size{ width, height }, > > frameRates }); > > > > activeResolution = std::max(activeResolution, Size{ width, height }); > > } > > } else { > > - data->supportedResolutions_.emplace_back( > > + data->config_.resolutions.emplace_back( > > VirtualCameraData::Resolution{ Size{ 1920, 1080 }, > > { 30, 60 } }); > > activeResolution = Size(1920, 1080); > > @@ -141,21 +142,65 @@ int Parser::parseSupportedFormats( > > return 0; > > } > > > > -int Parser::parseTestPattern( > > +int Parser::parseFrame( > > const YamlObject &cameraConfigData, VirtualCameraData *data) > > { > > - std::string testPattern = cameraConfigData["test_pattern"].get<std::string>().value(); > > + const YamlObject &frames = cameraConfigData["frames"]; > > + /* When there is no frames provided in the config file, use color bar test pattern */ > > + if (frames.size() == 0) { > > + data->config_.frame = TestPattern::ColorBars; > > + return 0; > > + } > > + > > + if (!frames.isDictionary()) { > > + LOG(Virtual, Error) << "'frames' is not a dictionary."; > > + return -EINVAL; > > + } > > + > > + std::string path = frames["path"].get<std::string>().value(); > > What if `path` is not specified? Is it checked somewhere else? > > > The default value of `empty.get<std::string>()` is actually an empty > std::string, instead of std::nullopt [1] [2]. However, you're right that > it's confusing. Updated with a default value `""` instead. > > [1]: https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/yaml_parser.cpp#n46 > [2]: https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/yaml_parser.cpp#n313 Oh you're right. I think this should be fixed. It is quite counter-intuitive. > > > > + > > + if (auto ext = getExtension(path); ext == ".jpg" || ext == ".jpeg") { > > + ScaleMode scaleMode; > > + if (parseScaleMode(frames, &scaleMode)) > > + return -EINVAL; > > + data->config_.frame = ImageFrames{ path, scaleMode, std::nullopt }; > > + } else if (path.back() == '/') { > > This looks very fragile. Why not use `std::filesystem::symlink_status()` to check > if it's a file or a directory. > > Thanks! Done. > > > > + ScaleMode scaleMode; > > + if (parseScaleMode(frames, &scaleMode)) > > + return -EINVAL; > > + data->config_.frame = ImageFrames{ path, scaleMode, > > + numberOfFilesInDirectory(path) }; > > Admittedly I did not check in detail, but is this necessary? Is it necessary > to count the number of files in advance? > > Hmm, the logic of this is to loop the images in the directory, without setting > the number of images in the directory. > Are you suggesting to count the number when we really use `data->config_`? > That also works, while the user of ImageFrame would need to check if > `ImageFrame.path` is a file or directory again. Let's ignore what I said for now. > > > > + } else if (path == "bars" || path == "") { > > + /* Default value is "bars" */ > > + data->config_.frame = TestPattern::ColorBars; > > + } else if (path == "lines") { > > + data->config_.frame = TestPattern::DiagonalLines; > > + } else { > > + LOG(Virtual, Error) << "Frame: " << path > > + << " is not supported"; > > + return -EINVAL; > > + } > > + return 0; > > +} > > > > - /* Default value is "bars" */ > > - if (testPattern == "bars" || testPattern == "") { > > - data->testPattern_ = TestPattern::ColorBars; > > - } else if (testPattern == "lines") { > > - data->testPattern_ = TestPattern::DiagonalLines; > > +int Parser::parseScaleMode( > > + const YamlObject &framesConfigData, ScaleMode *scaleMode) > > +{ > > + std::string mode = framesConfigData["scale_mode"].get<std::string>().value(); > > Same here, regarding empty optionals. > > Updated, as well as other optionals. > > > > + > > + /* Default value is fill */ > > + if (mode == "fill" || mode == "") { > > + *scaleMode = ScaleMode::Fill; > > + } else if (mode == "contain") { > > + *scaleMode = ScaleMode::Contain; > > + } else if (mode == "cover") { > > + *scaleMode = ScaleMode::Cover; > > } else { > > - LOG(Virtual, Error) << "Test pattern: " << testPattern > > - << "is not supported"; > > + LOG(Virtual, Error) << "scaleMode: " << mode > > + << " is not supported"; > > return -EINVAL; > > } > > + > > return 0; > > } > > > > @@ -195,4 +240,4 @@ int Parser::parseModel( > > return 0; > > } > > > > -} /* namespace libcamera */ > > +} // namespace libcamera > > diff --git a/src/libcamera/pipeline/virtual/parser.h b/src/libcamera/pipeline/virtual/parser.h > > index a377d8aa1..38ea460d5 100644 > > --- a/src/libcamera/pipeline/virtual/parser.h > > +++ b/src/libcamera/pipeline/virtual/parser.h > > @@ -34,12 +34,15 @@ private: > > > > int parseSupportedFormats( > > const YamlObject &cameraConfigData, VirtualCameraData *data); > > - int parseTestPattern( > > + int parseFrame( > > const YamlObject &cameraConfigData, VirtualCameraData *data); > > int parseLocation( > > const YamlObject &cameraConfigData, VirtualCameraData *data); > > int parseModel( > > const YamlObject &cameraConfigData, VirtualCameraData *data); > > + > > + int parseScaleMode( > > + const YamlObject &framesConfigData, ScaleMode *scaleMode); > > }; > > > > } // namespace libcamera > > diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > > index 6df9b31e9..844dffd49 100644 > > --- a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > > +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > > @@ -20,7 +20,7 @@ LOG_DECLARE_CATEGORY(Virtual) > > static const unsigned int kARGBSize = 4; > > > > void TestPatternGenerator::generateFrame( > > - const Size &size, > > + [[maybe_unused]] unsigned int &frameCount, const Size &size, > > const FrameBuffer *buffer) > > { > > MappedFrameBuffer mappedFrameBuffer(buffer, > > @@ -29,7 +29,7 @@ void TestPatternGenerator::generateFrame( > > auto planes = mappedFrameBuffer.planes(); > > > > /* TODO: select whether to do shifting or not */ > > - shiftLeft(size); > > + // shiftLeft(size); > > > > /* Convert the template_ to the frame buffer */ > > int ret = libyuv::ARGBToNV12( > > diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.h b/src/libcamera/pipeline/virtual/test_pattern_generator.h > > index ed8d4e43b..cebdd0141 100644 > > --- a/src/libcamera/pipeline/virtual/test_pattern_generator.h > > +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.h > > @@ -25,7 +25,7 @@ enum class TestPattern : char { > > class TestPatternGenerator : public FrameGenerator > > { > > private: > > - void generateFrame(const Size &size, > > + void generateFrame(unsigned int &frameCount, const Size &size, > > const FrameBuffer *buffer) override; > > > > protected: > > diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp > > index 0fe471f00..2412af703 100644 > > --- a/src/libcamera/pipeline/virtual/virtual.cpp > > +++ b/src/libcamera/pipeline/virtual/virtual.cpp > > @@ -20,9 +20,13 @@ > > #include "libcamera/internal/pipeline_handler.h" > > #include "libcamera/internal/yaml_parser.h" > > > > -#include "frame_generator.h" > > #include "parser.h" > > > > +#define ifTestPattern(v) std::holds_alternative<TestPattern>(v) > > +#define getTestPattern(v) std::get<TestPattern>(v) > > +#define ifImageFrames(v) std::holds_alternative<ImageFrames>(v) > > +#define getImageFrames(v) std::get<ImageFrames>(v) > > + > > namespace libcamera { > > > > LOG_DEFINE_CATEGORY(Virtual) > > @@ -63,12 +67,12 @@ CameraConfiguration::Status VirtualCameraConfiguration::validate() > > } > > > > Size maxSize; > > - for (const auto &resolution : data_->supportedResolutions_) > > + for (const auto &resolution : data_->config_.resolutions) > > maxSize = std::max(maxSize, resolution.size); > > > > for (StreamConfiguration &cfg : config_) { > > bool found = false; > > - for (const auto &resolution : data_->supportedResolutions_) { > > + for (const auto &resolution : data_->config_.resolutions) { > > if (resolution.size.width == cfg.size.width && > > resolution.size.height == cfg.size.height) { > > found = true; > > @@ -110,7 +114,7 @@ PipelineHandlerVirtual::generateConfiguration(Camera *camera, > > return config; > > > > Size minSize, sensorResolution; > > - for (const auto &resolution : data->supportedResolutions_) { > > + for (const auto &resolution : data->config_.resolutions) { > > if (minSize.isNull() || minSize > resolution.size) > > minSize = resolution.size; > > > > @@ -199,7 +203,7 @@ int PipelineHandlerVirtual::exportFrameBuffers( > > int PipelineHandlerVirtual::start(Camera *camera, > > [[maybe_unused]] const ControlList *controls) > > { > > - /* \todo Start reading the virtual video if any. */ > > + /* Start reading the images/generating test patterns */ > > VirtualCameraData *data = cameraData(camera); > > > > data->frameGenerator_->configure(data->stream_.configuration().size); > > @@ -219,8 +223,8 @@ int PipelineHandlerVirtual::queueRequestDevice([[maybe_unused]] Camera *camera, > > > > /* \todo Read from the virtual video if any. */ > > for (auto const &[stream, buffer] : request->buffers()) { > > - /* map buffer and fill test patterns */ > > - data->frameGenerator_->generateFrame(stream->configuration().size, buffer); > > + /* Map buffer. Fill test patterns or images */ > > + data->frameGenerator_->generateFrame(data->frameCount_, stream->configuration().size, buffer); > > completeBuffer(request, buffer); > > } > > > > @@ -250,9 +254,10 @@ bool PipelineHandlerVirtual::match([[maybe_unused]] DeviceEnumerator *enumerator > > /* Configure and register cameras with configData */ > > for (auto &data : configData) { > > std::set<Stream *> streams{ &data->stream_ }; > > - std::string id = data->id_; > > + std::string id = data->config_.id; > > std::shared_ptr<Camera> camera = Camera::create(std::move(data), id, streams); > > > > + /* Initialize FrameGenerator*/ > > initFrameGenerator(camera.get()); > > > > registerCamera(std::move(camera)); > > @@ -264,13 +269,19 @@ bool PipelineHandlerVirtual::match([[maybe_unused]] DeviceEnumerator *enumerator > > void PipelineHandlerVirtual::initFrameGenerator(Camera *camera) > > { > > auto data = cameraData(camera); > > - if (data->testPattern_ == TestPattern::DiagonalLines) { > > - data->frameGenerator_ = DiagonalLinesGenerator::create(); > > - } else { > > - data->frameGenerator_ = ColorBarsGenerator::create(); > > + auto &frame = data->config_.frame; > > + if (ifTestPattern(frame)) { > > + TestPattern &testPattern = getTestPattern(frame); > > + if (testPattern == TestPattern::DiagonalLines) { > > + data->frameGenerator_ = DiagonalLinesGenerator::create(); > > + } else { > > + data->frameGenerator_ = ColorBarsGenerator::create(); > > + } > > + } else if (ifImageFrames(frame)) { > > + data->frameGenerator_ = ImageFrameGenerator::create(getImageFrames(frame)); > > } > > I think I would strongly prefer `std::visit()` here. But even `std::get_if()` is better in my opinion. > > > Thanks for the suggestion. I'm quite a newbie to use std::variant. > Please take another look. Apart from the indentation looking a bit off, it seems ok. I think the `overloaded` type could go into `utils.h` or similar. Regards, Barnabás Pőcze > > > > } > > > > REGISTER_PIPELINE_HANDLER(PipelineHandlerVirtual, "virtual") > > > > -} /* namespace libcamera */ > > +} // namespace libcamera > > diff --git a/src/libcamera/pipeline/virtual/virtual.h b/src/libcamera/pipeline/virtual/virtual.h > > index c1ac4eb90..f41a4a906 100644 > > --- a/src/libcamera/pipeline/virtual/virtual.h > > +++ b/src/libcamera/pipeline/virtual/virtual.h > > @@ -7,16 +7,22 @@ > > > > #pragma once > > > > +#include <variant> > > + > > #include <libcamera/base/file.h> > > > > #include "libcamera/internal/camera.h" > > #include "libcamera/internal/dma_buf_allocator.h" > > #include "libcamera/internal/pipeline_handler.h" > > > > +#include "frame_generator.h" > > +#include "image_frame_generator.h" > > #include "test_pattern_generator.h" > > > > namespace libcamera { > > > > +using VirtualFrame = std::variant<TestPattern, ImageFrames>; > > + > > class VirtualCameraData : public Camera::Private > > { > > public: > > @@ -24,6 +30,13 @@ public: > > Size size; > > std::vector<int> frameRates; > > }; > > + /* The config file is parsed to the Configuration struct */ > > + struct Configuration { > > + std::string id; > > + std::vector<Resolution> resolutions; > > + VirtualFrame frame; > > + }; > > + > > VirtualCameraData(PipelineHandler *pipe) > > : Camera::Private(pipe) > > { > > @@ -31,12 +44,9 @@ public: > > > > ~VirtualCameraData() = default; > > > > - std::string id_; > > - std::vector<Resolution> supportedResolutions_; > > - TestPattern testPattern_; > > - > > + unsigned int frameCount_ = 0; > > + Configuration config_; > > Stream stream_; > > - > > std::unique_ptr<FrameGenerator> frameGenerator_; > > }; > > > > -- > > 2.46.0.184.g6999bdac58-goog > > > > > Regards, > Barnabás Pőcze
Hi Barnabás, Please check https://gitlab.freedesktop.org/chenghaoyang/libcamera/-/pipelines/1256500 or https://patchwork.libcamera.org/patch/21008/ On Fri, Aug 23, 2024 at 6:55 PM Barnabás Pőcze <pobrn@protonmail.com> wrote: > Hi > > > 2024. augusztus 21., szerda 23:55 keltezéssel, Cheng-Hao Yang < > chenghaoyang@chromium.org> írta: > > > > Thank you Barnabás for the review :) > > > > Uploaded to gitlab first: > https://gitlab.freedesktop.org/chenghaoyang/libcamera/-/commit/b7342672e14438a72ab5de00174440862ff33c68 > > Will submit another series of patches after the question below is > resolved. > > > > On Wed, Aug 21, 2024 at 4:06 AM Barnabás Pőcze <pobrn@protonmail.com> > wrote: > > > > Hi > > > > > > 2024. augusztus 20., kedd 18:23 keltezéssel, Harvey Yang < > chenghaoyang@chromium.org> írta: > > > > > From: Konami Shu <konamiz@google.com> > > > > > > - Refactor ImageFrameGenerator and TestPatternGenerator > > > - Extend the config file to have section to choose to use images > or test > > > pattern > > > - Extend Parser to parse the configuration > > > - Add ImageFrameGenerator which uses images to provide frames > > > > > > Patchset1->2 > > > - Extend the parser to accept files with ".jpeg" > > > - Heap allocate some buffers in ImageFrameGenerator::generateFrame > so > > > the buffers won't cause stack over flow > > > > > > Patchset5->6 > > > - Move static factory function from the interface class to the > derived > > > classes > > > > > > Patchset6->8 > > > - create FrameGenerator before starting the camera > > > > > > Signed-off-by: Konami Shu <konamiz@google.com> > > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org> > > > Co-developed-by: Yunke Cao <yunkec@chromium.org> > > > Co-developed-by: Tomasz Figa <tfiga@chromium.org> > > > --- > > > src/libcamera/pipeline/virtual/README.md | 18 +- > > > .../pipeline/virtual/common_functions.cpp | 27 +++ > > > .../pipeline/virtual/common_functions.h | 18 ++ > > > .../pipeline/virtual/frame_generator.h | 2 +- > > > .../virtual/image_frame_generator.cpp | 154 ++++++++++++++++++ > > > .../pipeline/virtual/image_frame_generator.h | 65 ++++++++ > > > src/libcamera/pipeline/virtual/meson.build | 6 +- > > > src/libcamera/pipeline/virtual/parser.cpp | 77 +++++++-- > > > src/libcamera/pipeline/virtual/parser.h | 5 +- > > > .../virtual/test_pattern_generator.cpp | 4 +- > > > .../pipeline/virtual/test_pattern_generator.h | 2 +- > > > src/libcamera/pipeline/virtual/virtual.cpp | 37 +++-- > > > src/libcamera/pipeline/virtual/virtual.h | 20 ++- > > > 13 files changed, 390 insertions(+), 45 deletions(-) > > > create mode 100644 > src/libcamera/pipeline/virtual/common_functions.cpp > > > create mode 100644 > src/libcamera/pipeline/virtual/common_functions.h > > > create mode 100644 > src/libcamera/pipeline/virtual/image_frame_generator.cpp > > > create mode 100644 > src/libcamera/pipeline/virtual/image_frame_generator.h > > > > > > diff --git a/src/libcamera/pipeline/virtual/README.md > b/src/libcamera/pipeline/virtual/README.md > > > index 27d6283df..5e21ce74a 100644 > > > --- a/src/libcamera/pipeline/virtual/README.md > > > +++ b/src/libcamera/pipeline/virtual/README.md > > > @@ -15,8 +15,13 @@ Each camera block is a dictionary, containing > the following keys: > > > - `supported_formats` (list of `VirtualCameraData::Resolution`, > optional) : List of supported resolution and frame rates of the emulated > camera > > > - `width` (`unsigned int`, default=1920): Width of the window > resolution. This needs to be even. > > > - `height` (`unsigned int`, default=1080): Height of the window > resolution. > > > - - `frame_rates` (list of `int`, default=`[30,60]` ): Range of > the frame rate. The list has to be two values of the lower bound and the > upper bound of the frame rate. > > > -- `test_pattern` (`string`, default="bars"): Which test pattern > to use as frames. The options are "bars", "lines". > > > + - `frame_rates` (list of `int`, default=`[30,60]` ): Range of > the frame rate. The list has to be two values of the lower bound and the > upper bound of the frame rate. This does not affect the frame rate for now. > > > +- `frames` (dictionary): > > > + - `path` (`string`, default="bars"): Name of a test pattern, > path to an image, or path to a directory of a series of images. > > > + - The test patterns are "bars" which means color bars, and > "lines" which means diagonal lines. > > > + - The path to an image has ".jpg" extension. > > > + - 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. > > > + - `scale_mode`(`string`, default="fill"): Scale mode when the > frames are images. The scale modes are "fill", "contain", and "cover". This > does not matter when frames is a test pattern. This does not affect the > scale mode for now. > > > - `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. > > > - `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. > > > > > > @@ -35,13 +40,16 @@ A sample config file: > > > frame_rates: > > > - 70 > > > - 80 > > > - test_pattern: "bars" > > > + frames: > > > + path: "lines" > > > location: "front" > > > model: "Virtual Video Device" > > > "Virtual1": > > > supported_formats: > > > - width: 800 > > > - test_pattern: "lines" > > > + frames: > > > + path: "path/to/directory_of_images/" > > > + scale_mode: "contain" > > > location: "back" > > > model: "Virtual Video Device1" > > > "Virtual2": > > > @@ -61,7 +69,7 @@ This is the procedure of the Parser class: > > > - If the config file contains invalid configuration, this method > returns nullptr. The camera will be skipped. > > > 3. Parse each property and register the data. > > > - `parseSupportedFormats()`: Parses `supported_formats` in the > config, which contains resolutions and frame rates. > > > - - `parseTestPattern()`: Parses `test_pattern` in the config. > > > + - `parseFrame()`: Parses `frames` in the config. > > > - `parseLocation()`: Parses `location` in the config. > > > - `parseModel()`: Parses `model` in the config. > > > 4. Back to `parseConfigFile()` and append the camera configuration. > > > diff --git a/src/libcamera/pipeline/virtual/common_functions.cpp > b/src/libcamera/pipeline/virtual/common_functions.cpp > > > new file mode 100644 > > > index 000000000..207827ee0 > > > --- /dev/null > > > +++ b/src/libcamera/pipeline/virtual/common_functions.cpp > > > @@ -0,0 +1,27 @@ > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > +/* > > > + * Copyright (C) 2023, Google Inc. > > > + * > > > + * common_functions.cpp - Helper that do not depend on any class > > > + */ > > > + > > > +#include "common_functions.h" > > > + > > > +namespace libcamera { > > > + > > > +std::string getExtension(const std::string &path) > > > > This looks very fragile. Why not use > `std::filesystem::path::extension()`? > > > > Right, updated. > > > > > > > +{ > > > + size_t i = path.find("."); > > > + if (i != std::string::npos) { > > > + return path.substr(i); > > > + } > > > + return ""; > > > +} > > > + > > > +std::size_t numberOfFilesInDirectory(std::filesystem::path path) > > > +{ > > > + using std::filesystem::directory_iterator; > > > + return std::distance(directory_iterator(path), > directory_iterator{}); > > > +} > > > + > > > +} // namespace libcamera > > > diff --git a/src/libcamera/pipeline/virtual/common_functions.h > b/src/libcamera/pipeline/virtual/common_functions.h > > > new file mode 100644 > > > index 000000000..4203f9505 > > > --- /dev/null > > > +++ b/src/libcamera/pipeline/virtual/common_functions.h > > > @@ -0,0 +1,18 @@ > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > +/* > > > + * Copyright (C) 2023, Google Inc. > > > + * > > > + * common_functions.h - Helper that do not depend on any class > > > + */ > > > + > > > +#pragma once > > > + > > > +#include <filesystem> > > > + > > > +namespace libcamera { > > > + > > > +std::string getExtension(const std::string &path); > > > + > > > +std::size_t numberOfFilesInDirectory(std::filesystem::path path); > > > + > > > +} // namespace libcamera > > > diff --git a/src/libcamera/pipeline/virtual/frame_generator.h > b/src/libcamera/pipeline/virtual/frame_generator.h > > > index 9699af7a4..f69576b36 100644 > > > --- a/src/libcamera/pipeline/virtual/frame_generator.h > > > +++ b/src/libcamera/pipeline/virtual/frame_generator.h > > > @@ -23,7 +23,7 @@ public: > > > /** Fill the output frame buffer. > > > * Use the frame at the frameCount of image frames > > > */ > > > - virtual void generateFrame(const Size &size, > > > + virtual void generateFrame(unsigned int &frameCount, const Size > &size, > > > const FrameBuffer *buffer) = 0; > > > > > > protected: > > > diff --git > a/src/libcamera/pipeline/virtual/image_frame_generator.cpp > b/src/libcamera/pipeline/virtual/image_frame_generator.cpp > > > new file mode 100644 > > > index 000000000..d374877ff > > > --- /dev/null > > > +++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp > > > @@ -0,0 +1,154 @@ > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > +/* > > > + * Copyright (C) 2023, Google Inc. > > > + * > > > + * image_frame_generator.cpp - Derived class of FrameGenerator for > > > + * generating frames from images > > > + */ > > > + > > > +#include "image_frame_generator.h" > > > + > > > +#include <memory> > > > +#include <optional> > > > +#include <string> > > > + > > > +#include <libcamera/base/file.h> > > > +#include <libcamera/base/log.h> > > > + > > > +#include <libcamera/framebuffer.h> > > > + > > > +#include "libcamera/internal/mapped_framebuffer.h" > > > + > > > +#include "libyuv/convert.h" > > > +#include "libyuv/scale.h" > > > +namespace libcamera { > > > + > > > +LOG_DECLARE_CATEGORY(Virtual) > > > + > > > +std::unique_ptr<ImageFrameGenerator> ImageFrameGenerator::create( > > > + ImageFrames &imageFrames) > > > +{ > > > + std::unique_ptr<ImageFrameGenerator> imageFrameGenerator = > > > + std::make_unique<ImageFrameGenerator>(); > > > + imageFrameGenerator->imageFrames_ = &imageFrames; > > > + > > > + /** For each file in the directory > > > + * load the image, convert it to NV12, and store the pointer > > > + */ > > > + for (unsigned int i = 0; i < imageFrames.number.value_or(1); > i++) { > > > + std::string path; > > > + if (!imageFrames.number.has_value()) { > > > + /* If the path is to an image */ > > > + path = imageFrames.path; > > > + } else { > > > + /* If the path is to a directory */ > > > + path = constructPath(imageFrames.path, i); > > > + } > > > + > > > + File file(path); > > > + bool isOpen = file.open(File::OpenModeFlag::ReadOnly); > > > + if (!isOpen) { > > > > I think `if (!file.open(...))` is fine. > > > > Done. > > > > > > > + LOG(Virtual, Error) << "Failed to open image file: " << > file.fileName(); > > Forgot to mention, it might be useful to print `strerror(file.error())` or > similar as well. > > Done. > > > > + return nullptr; > > > + } > > > + > > > + /* Read the image file to data */ > > > + uint8_t buffer[file.size()]; > > > > It's probably best to avoid VLAs. > > > > Right, replaced with std::vector. Please check again. > > Looks ok, although I would just use > > auto buffer = std::make_unique<uint8_t[]>(file.size()); > > Done. > Also, every `File::size()` incurs an `fstat()`, so you probably want to > call that > once and ensure that it is >= 0. > > Updated. Please check again. > > > > > > > > + Span<unsigned char> data{ buffer, (unsigned long)file.size() }; > > > + long dataSize = file.read(data); > > > + > > > + /* Get the width and height of the image */ > > > + int width, height; > > > + if (libyuv::MJPGSize(data.data(), dataSize, &width, &height)) { > > > + LOG(Virtual, Error) << "Failed to get the size of the image > file: " > > > + << file.fileName(); > > > + return nullptr; > > > + } > > > + > > > + /* Convert to NV12 and write the data to tmpY and tmpUV */ > > > + int halfWidth = (width + 1) / 2; > > > + int halfHeight = (height + 1) / 2; > > > + std::unique_ptr<uint8_t[]> dstY = > > > + std::make_unique<uint8_t[]>(width * height); > > > + std::unique_ptr<uint8_t[]> dstUV = > > > + std::make_unique<uint8_t[]>(halfWidth * halfHeight * 2); > > > + int ret = libyuv::MJPGToNV12(data.data(), dataSize, > > > + dstY.get(), width, dstUV.get(), > > > + width, width, height, width, height); > > > + if (ret != 0) { > > > + LOG(Virtual, Error) << "MJPGToNV12() failed with " << ret; > > > + } > > > + > > > + imageFrameGenerator->imageFrameDatas_.emplace_back( > > > + ImageFrameData{ std::move(dstY), std::move(dstUV), > > > + Size(width, height) }); > > > + } > > > + return imageFrameGenerator; > > > +} > > > + > > > +std::string ImageFrameGenerator::constructPath(std::string &name, > unsigned int &i) > > > > Does this need to be in `ImageFrameGenerator`? Is it not enough to > have it in an > > anonymous namespace in this file? > > > > > > Right, done. > > I think `return name / (std::to_string(i) + ".jpg");` would work just fine. > Thanks! Done. > Also, why is `i` a reference? > Updated to a simple value. > > > > > > > > > +{ > > > + return name + std::to_string(i) + ".jpg"; > > > +} > > > + > > > +void ImageFrameGenerator::configure(const Size &size) > > > +{ > > > + for (unsigned int i = 0; i < imageFrames_->number.value_or(1); > i++) { > > > + /* Scale the imageFrameDatas_ to scaledY and scaledUV */ > > > + int halfSizeWidth = (size.width + 1) / 2; > > > + int halfSizeHeight = (size.height + 1) / 2; > > > + std::unique_ptr<uint8_t[]> scaledY = > > > + std::make_unique<uint8_t[]>(size.width * size.height); > > > + std::unique_ptr<uint8_t[]> scaledUV = > > > + std::make_unique<uint8_t[]>(halfSizeWidth * halfSizeHeight * 2); > > > + auto &src = imageFrameDatas_[i]; > > > + > > > + /* > > > + * \todo Implement "contain" & "cover", based on > > > + * |imageFrames_[i].scaleMode|. > > > + */ > > > + > > > + /* > > > + * \todo Some platforms might enforce stride due to GPU, like > > > + * ChromeOS ciri (64). The weight needs to be a multiple of > > > + * the stride to work properly for now. > > > + */ > > > + libyuv::NV12Scale(src.Y.get(), src.size.width, > > > + src.UV.get(), src.size.width, > > > + src.size.width, src.size.height, > > > + scaledY.get(), size.width, scaledUV.get(), size.width, > > > + size.width, size.height, libyuv::FilterMode::kFilterBilinear); > > > + > > > + /* Store the pointers to member variable */ > > > + scaledFrameDatas_.emplace_back( > > > + ImageFrameData{ std::move(scaledY), std::move(scaledUV), size }); > > > + } > > > +} > > > + > > > +void ImageFrameGenerator::generateFrame(unsigned int &frameCount, > const Size &size, const FrameBuffer *buffer) > > > +{ > > > + /* Don't do anything when the list of buffers is empty*/ > > > + if (scaledFrameDatas_.size() == 0) > > > + return; > > > + > > > + MappedFrameBuffer mappedFrameBuffer(buffer, > MappedFrameBuffer::MapFlag::Write); > > > + > > > + auto planes = mappedFrameBuffer.planes(); > > > + > > > + /* Make sure the frameCount does not over the number of images */ > > > + frameCount %= imageFrames_->number.value_or(1); > > > + > > > + /* Write the scaledY and scaledUV to the mapped frame buffer */ > > > + libyuv::NV12Copy(scaledFrameDatas_[frameCount].Y.get(), > size.width, > > > + scaledFrameDatas_[frameCount].UV.get(), size.width, > planes[0].begin(), > > > + size.width, planes[1].begin(), size.width, > > > + size.width, size.height); > > > + > > > + /* proceed an image every 4 frames */ > > > + /* \todo read the parameter_ from the configuration file? */ > > > + parameter_++; > > > + if (parameter_ % 4 == 0) > > > + frameCount++; > > > +} > > > + > > > +} // namespace libcamera > > > diff --git > a/src/libcamera/pipeline/virtual/image_frame_generator.h > b/src/libcamera/pipeline/virtual/image_frame_generator.h > > > new file mode 100644 > > > index 000000000..74468e075 > > > --- /dev/null > > > +++ b/src/libcamera/pipeline/virtual/image_frame_generator.h > > > @@ -0,0 +1,65 @@ > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > +/* > > > + * Copyright (C) 2023, Google Inc. > > > + * > > > + * image_frame_generator.h - Derived class of FrameGenerator for > > > + * generating frames from images > > > + */ > > > + > > > +#pragma once > > > + > > > +#include <memory> > > > +#include <optional> > > > +#include <stdint.h> > > > +#include <sys/types.h> > > > + > > > +#include "frame_generator.h" > > > + > > > +namespace libcamera { > > > + > > > +enum class ScaleMode : char { > > > + Fill = 0, > > > + Contain = 1, > > > + Cover = 2, > > > +}; > > > + > > > +/* Frame configuration provided by the config file */ > > > +struct ImageFrames { > > > + std::string path; > > > > I think `std::filesystem::path` would be preferable. > > > > Done. > > > > > > > + ScaleMode scaleMode; > > > + std::optional<unsigned int> number; > > > +}; > > > + > > > +class ImageFrameGenerator : public FrameGenerator > > > +{ > > > +public: > > > + /** Factory function to create an ImageFrameGenerator object. > > > + * Read the images and convert them to buffers in NV12 format. > > > + * Store the pointers to the buffers to a list (imageFrameDatas) > > > + */ > > > + static std::unique_ptr<ImageFrameGenerator> create(ImageFrames > &imageFrames); > > > + > > > +private: > > > + struct ImageFrameData { > > > + std::unique_ptr<uint8_t[]> Y; > > > + std::unique_ptr<uint8_t[]> UV; > > > + Size size; > > > + }; > > > + > > > + /* Scale the buffers for image frames. */ > > > + void configure(const Size &size) override; > > > + void generateFrame(unsigned int &frameCount, const Size &size, > const FrameBuffer *buffer) override; > > > + > > > + static std::string constructPath(std::string &name, unsigned int > &i); > > > + > > > + /* List of pointers to the not scaled image buffers */ > > > + std::vector<ImageFrameData> imageFrameDatas_; > > > + /* List of pointers to the scaled image buffers */ > > > + std::vector<ImageFrameData> scaledFrameDatas_; > > > + /* Pointer to the imageFrames_ in VirtualCameraData */ > > > + ImageFrames *imageFrames_; > > > + /* Speed parameter. Change to the next image every parameter_ > frames. */ > > > + int parameter_; > > > +}; > > > + > > > +} /* namespace libcamera */ > > > diff --git a/src/libcamera/pipeline/virtual/meson.build > b/src/libcamera/pipeline/virtual/meson.build > > > index 2e82e64cb..d56aedec4 100644 > > > --- a/src/libcamera/pipeline/virtual/meson.build > > > +++ b/src/libcamera/pipeline/virtual/meson.build > > > @@ -2,11 +2,14 @@ > > > > > > libcamera_sources += files([ > > > 'virtual.cpp', > > > - 'test_pattern_generator.cpp', > > > 'parser.cpp', > > > + 'test_pattern_generator.cpp', > > > + 'image_frame_generator.cpp', > > > + 'common_functions.cpp', > > > ]) > > > > > > libyuv_dep = dependency('libyuv', required : false) > > > +libjpeg = dependency('libjpeg', required : false) > > > > > > # Fallback to a subproject if libyuv isn't found, as it's > typically not > > > # provided by distributions. > > > @@ -26,3 +29,4 @@ if not libyuv_dep.found() > > > endif > > > > > > libcamera_deps += [libyuv_dep] > > > +libcamera_deps += [libjpeg] > > > diff --git a/src/libcamera/pipeline/virtual/parser.cpp > b/src/libcamera/pipeline/virtual/parser.cpp > > > index 032c0cd9d..46d1ab181 100644 > > > --- a/src/libcamera/pipeline/virtual/parser.cpp > > > +++ b/src/libcamera/pipeline/virtual/parser.cpp > > > @@ -18,6 +18,7 @@ > > > #include "libcamera/internal/pipeline_handler.h" > > > #include "libcamera/internal/yaml_parser.h" > > > > > > +#include "common_functions.h" > > > #include "virtual.h" > > > > > > namespace libcamera { > > > @@ -52,12 +53,12 @@ > std::vector<std::unique_ptr<VirtualCameraData>> Parser::parseConfigFile( > > > continue; > > > } > > > > > > - data->id_ = cameraId; > > > + data->config_.id = cameraId; > > > ControlInfoMap::Map controls; > > > /* todo: Check which resolution's frame rate to be reported */ > > > controls[&controls::FrameDurationLimits] = > > > - ControlInfo(int64_t(1000 / > data->supportedResolutions_[0].frameRates[1]), > > > - int64_t(1000 / data->supportedResolutions_[0].frameRates[0])); > > > + ControlInfo(int64_t(1000 / > data->config_.resolutions[0].frameRates[1]), > > > + int64_t(1000 / data->config_.resolutions[0].frameRates[0])); > > > data->controlInfo_ = ControlInfoMap(std::move(controls), > controls::controls); > > > configurations.push_back(std::move(data)); > > > } > > > @@ -72,7 +73,7 @@ std::unique_ptr<VirtualCameraData> > Parser::parseCameraConfigData( > > > if (parseSupportedFormats(cameraConfigData, data.get())) > > > return nullptr; > > > > > > - if (parseTestPattern(cameraConfigData, data.get())) > > > + if (parseFrame(cameraConfigData, data.get())) > > > return nullptr; > > > > > > if (parseLocation(cameraConfigData, data.get())) > > > @@ -122,14 +123,14 @@ int Parser::parseSupportedFormats( > > > frameRates.push_back(60); > > > } > > > > > > - data->supportedResolutions_.emplace_back( > > > + data->config_.resolutions.emplace_back( > > > VirtualCameraData::Resolution{ Size{ width, height }, > > > frameRates }); > > > > > > activeResolution = std::max(activeResolution, Size{ width, height > }); > > > } > > > } else { > > > - data->supportedResolutions_.emplace_back( > > > + data->config_.resolutions.emplace_back( > > > VirtualCameraData::Resolution{ Size{ 1920, 1080 }, > > > { 30, 60 } }); > > > activeResolution = Size(1920, 1080); > > > @@ -141,21 +142,65 @@ int Parser::parseSupportedFormats( > > > return 0; > > > } > > > > > > -int Parser::parseTestPattern( > > > +int Parser::parseFrame( > > > const YamlObject &cameraConfigData, VirtualCameraData *data) > > > { > > > - std::string testPattern = > cameraConfigData["test_pattern"].get<std::string>().value(); > > > + const YamlObject &frames = cameraConfigData["frames"]; > > > + /* When there is no frames provided in the config file, use > color bar test pattern */ > > > + if (frames.size() == 0) { > > > + data->config_.frame = TestPattern::ColorBars; > > > + return 0; > > > + } > > > + > > > + if (!frames.isDictionary()) { > > > + LOG(Virtual, Error) << "'frames' is not a dictionary."; > > > + return -EINVAL; > > > + } > > > + > > > + std::string path = frames["path"].get<std::string>().value(); > > > > What if `path` is not specified? Is it checked somewhere else? > > > > > > The default value of `empty.get<std::string>()` is actually an empty > > std::string, instead of std::nullopt [1] [2]. However, you're right that > > it's confusing. Updated with a default value `""` instead. > > > > [1]: > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/yaml_parser.cpp#n46 > > [2]: > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/yaml_parser.cpp#n313 > > Oh you're right. I think this should be fixed. It is quite > counter-intuitive. > > > > > > > > > + > > > + if (auto ext = getExtension(path); ext == ".jpg" || ext == > ".jpeg") { > > > + ScaleMode scaleMode; > > > + if (parseScaleMode(frames, &scaleMode)) > > > + return -EINVAL; > > > + data->config_.frame = ImageFrames{ path, scaleMode, std::nullopt > }; > > > + } else if (path.back() == '/') { > > > > This looks very fragile. Why not use > `std::filesystem::symlink_status()` to check > > if it's a file or a directory. > > > > Thanks! Done. > > > > > > > + ScaleMode scaleMode; > > > + if (parseScaleMode(frames, &scaleMode)) > > > + return -EINVAL; > > > + data->config_.frame = ImageFrames{ path, scaleMode, > > > + numberOfFilesInDirectory(path) }; > > > > Admittedly I did not check in detail, but is this necessary? Is it > necessary > > to count the number of files in advance? > > > > Hmm, the logic of this is to loop the images in the directory, without > setting > > the number of images in the directory. > > Are you suggesting to count the number when we really use > `data->config_`? > > That also works, while the user of ImageFrame would need to check if > > `ImageFrame.path` is a file or directory again. > > Let's ignore what I said for now. > > > > > > > > > + } else if (path == "bars" || path == "") { > > > + /* Default value is "bars" */ > > > + data->config_.frame = TestPattern::ColorBars; > > > + } else if (path == "lines") { > > > + data->config_.frame = TestPattern::DiagonalLines; > > > + } else { > > > + LOG(Virtual, Error) << "Frame: " << path > > > + << " is not supported"; > > > + return -EINVAL; > > > + } > > > + return 0; > > > +} > > > > > > - /* Default value is "bars" */ > > > - if (testPattern == "bars" || testPattern == "") { > > > - data->testPattern_ = TestPattern::ColorBars; > > > - } else if (testPattern == "lines") { > > > - data->testPattern_ = TestPattern::DiagonalLines; > > > +int Parser::parseScaleMode( > > > + const YamlObject &framesConfigData, ScaleMode *scaleMode) > > > +{ > > > + std::string mode = > framesConfigData["scale_mode"].get<std::string>().value(); > > > > Same here, regarding empty optionals. > > > > Updated, as well as other optionals. > > > > > > > + > > > + /* Default value is fill */ > > > + if (mode == "fill" || mode == "") { > > > + *scaleMode = ScaleMode::Fill; > > > + } else if (mode == "contain") { > > > + *scaleMode = ScaleMode::Contain; > > > + } else if (mode == "cover") { > > > + *scaleMode = ScaleMode::Cover; > > > } else { > > > - LOG(Virtual, Error) << "Test pattern: " << testPattern > > > - << "is not supported"; > > > + LOG(Virtual, Error) << "scaleMode: " << mode > > > + << " is not supported"; > > > return -EINVAL; > > > } > > > + > > > return 0; > > > } > > > > > > @@ -195,4 +240,4 @@ int Parser::parseModel( > > > return 0; > > > } > > > > > > -} /* namespace libcamera */ > > > +} // namespace libcamera > > > diff --git a/src/libcamera/pipeline/virtual/parser.h > b/src/libcamera/pipeline/virtual/parser.h > > > index a377d8aa1..38ea460d5 100644 > > > --- a/src/libcamera/pipeline/virtual/parser.h > > > +++ b/src/libcamera/pipeline/virtual/parser.h > > > @@ -34,12 +34,15 @@ private: > > > > > > int parseSupportedFormats( > > > const YamlObject &cameraConfigData, VirtualCameraData *data); > > > - int parseTestPattern( > > > + int parseFrame( > > > const YamlObject &cameraConfigData, VirtualCameraData *data); > > > int parseLocation( > > > const YamlObject &cameraConfigData, VirtualCameraData *data); > > > int parseModel( > > > const YamlObject &cameraConfigData, VirtualCameraData *data); > > > + > > > + int parseScaleMode( > > > + const YamlObject &framesConfigData, ScaleMode *scaleMode); > > > }; > > > > > > } // namespace libcamera > > > diff --git > a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > > > index 6df9b31e9..844dffd49 100644 > > > --- a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > > > +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > > > @@ -20,7 +20,7 @@ LOG_DECLARE_CATEGORY(Virtual) > > > static const unsigned int kARGBSize = 4; > > > > > > void TestPatternGenerator::generateFrame( > > > - const Size &size, > > > + [[maybe_unused]] unsigned int &frameCount, const Size &size, > > > const FrameBuffer *buffer) > > > { > > > MappedFrameBuffer mappedFrameBuffer(buffer, > > > @@ -29,7 +29,7 @@ void TestPatternGenerator::generateFrame( > > > auto planes = mappedFrameBuffer.planes(); > > > > > > /* TODO: select whether to do shifting or not */ > > > - shiftLeft(size); > > > + // shiftLeft(size); > > > > > > /* Convert the template_ to the frame buffer */ > > > int ret = libyuv::ARGBToNV12( > > > diff --git > a/src/libcamera/pipeline/virtual/test_pattern_generator.h > b/src/libcamera/pipeline/virtual/test_pattern_generator.h > > > index ed8d4e43b..cebdd0141 100644 > > > --- a/src/libcamera/pipeline/virtual/test_pattern_generator.h > > > +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.h > > > @@ -25,7 +25,7 @@ enum class TestPattern : char { > > > class TestPatternGenerator : public FrameGenerator > > > { > > > private: > > > - void generateFrame(const Size &size, > > > + void generateFrame(unsigned int &frameCount, const Size &size, > > > const FrameBuffer *buffer) override; > > > > > > protected: > > > diff --git a/src/libcamera/pipeline/virtual/virtual.cpp > b/src/libcamera/pipeline/virtual/virtual.cpp > > > index 0fe471f00..2412af703 100644 > > > --- a/src/libcamera/pipeline/virtual/virtual.cpp > > > +++ b/src/libcamera/pipeline/virtual/virtual.cpp > > > @@ -20,9 +20,13 @@ > > > #include "libcamera/internal/pipeline_handler.h" > > > #include "libcamera/internal/yaml_parser.h" > > > > > > -#include "frame_generator.h" > > > #include "parser.h" > > > > > > +#define ifTestPattern(v) std::holds_alternative<TestPattern>(v) > > > +#define getTestPattern(v) std::get<TestPattern>(v) > > > +#define ifImageFrames(v) std::holds_alternative<ImageFrames>(v) > > > +#define getImageFrames(v) std::get<ImageFrames>(v) > > > + > > > namespace libcamera { > > > > > > LOG_DEFINE_CATEGORY(Virtual) > > > @@ -63,12 +67,12 @@ CameraConfiguration::Status > VirtualCameraConfiguration::validate() > > > } > > > > > > Size maxSize; > > > - for (const auto &resolution : data_->supportedResolutions_) > > > + for (const auto &resolution : data_->config_.resolutions) > > > maxSize = std::max(maxSize, resolution.size); > > > > > > for (StreamConfiguration &cfg : config_) { > > > bool found = false; > > > - for (const auto &resolution : data_->supportedResolutions_) { > > > + for (const auto &resolution : data_->config_.resolutions) { > > > if (resolution.size.width == cfg.size.width && > > > resolution.size.height == cfg.size.height) { > > > found = true; > > > @@ -110,7 +114,7 @@ > PipelineHandlerVirtual::generateConfiguration(Camera *camera, > > > return config; > > > > > > Size minSize, sensorResolution; > > > - for (const auto &resolution : data->supportedResolutions_) { > > > + for (const auto &resolution : data->config_.resolutions) { > > > if (minSize.isNull() || minSize > resolution.size) > > > minSize = resolution.size; > > > > > > @@ -199,7 +203,7 @@ int PipelineHandlerVirtual::exportFrameBuffers( > > > int PipelineHandlerVirtual::start(Camera *camera, > > > [[maybe_unused]] const ControlList *controls) > > > { > > > - /* \todo Start reading the virtual video if any. */ > > > + /* Start reading the images/generating test patterns */ > > > VirtualCameraData *data = cameraData(camera); > > > > > > > data->frameGenerator_->configure(data->stream_.configuration().size); > > > @@ -219,8 +223,8 @@ int > PipelineHandlerVirtual::queueRequestDevice([[maybe_unused]] Camera *camera, > > > > > > /* \todo Read from the virtual video if any. */ > > > for (auto const &[stream, buffer] : request->buffers()) { > > > - /* map buffer and fill test patterns */ > > > - > data->frameGenerator_->generateFrame(stream->configuration().size, buffer); > > > + /* Map buffer. Fill test patterns or images */ > > > + data->frameGenerator_->generateFrame(data->frameCount_, > stream->configuration().size, buffer); > > > completeBuffer(request, buffer); > > > } > > > > > > @@ -250,9 +254,10 @@ bool > PipelineHandlerVirtual::match([[maybe_unused]] DeviceEnumerator *enumerator > > > /* Configure and register cameras with configData */ > > > for (auto &data : configData) { > > > std::set<Stream *> streams{ &data->stream_ }; > > > - std::string id = data->id_; > > > + std::string id = data->config_.id; > > > std::shared_ptr<Camera> camera = Camera::create(std::move(data), > id, streams); > > > > > > + /* Initialize FrameGenerator*/ > > > initFrameGenerator(camera.get()); > > > > > > registerCamera(std::move(camera)); > > > @@ -264,13 +269,19 @@ bool > PipelineHandlerVirtual::match([[maybe_unused]] DeviceEnumerator *enumerator > > > void PipelineHandlerVirtual::initFrameGenerator(Camera *camera) > > > { > > > auto data = cameraData(camera); > > > - if (data->testPattern_ == TestPattern::DiagonalLines) { > > > - data->frameGenerator_ = DiagonalLinesGenerator::create(); > > > - } else { > > > - data->frameGenerator_ = ColorBarsGenerator::create(); > > > + auto &frame = data->config_.frame; > > > + if (ifTestPattern(frame)) { > > > + TestPattern &testPattern = getTestPattern(frame); > > > + if (testPattern == TestPattern::DiagonalLines) { > > > + data->frameGenerator_ = DiagonalLinesGenerator::create(); > > > + } else { > > > + data->frameGenerator_ = ColorBarsGenerator::create(); > > > + } > > > + } else if (ifImageFrames(frame)) { > > > + data->frameGenerator_ = > ImageFrameGenerator::create(getImageFrames(frame)); > > > } > > > > I think I would strongly prefer `std::visit()` here. But even > `std::get_if()` is better in my opinion. > > > > > > Thanks for the suggestion. I'm quite a newbie to use std::variant. > > Please take another look. > > Apart from the indentation looking a bit off, it seems ok. > Hmm, it's done by my linter, and it follows the .clang-format, so... > > I think the `overloaded` type could go into `utils.h` or similar. > Moved to a new header `utils.h`. > > > Regards, > Barnabás Pőcze > > > > > > > > } > > > > > > REGISTER_PIPELINE_HANDLER(PipelineHandlerVirtual, "virtual") > > > > > > -} /* namespace libcamera */ > > > +} // namespace libcamera > > > diff --git a/src/libcamera/pipeline/virtual/virtual.h > b/src/libcamera/pipeline/virtual/virtual.h > > > index c1ac4eb90..f41a4a906 100644 > > > --- a/src/libcamera/pipeline/virtual/virtual.h > > > +++ b/src/libcamera/pipeline/virtual/virtual.h > > > @@ -7,16 +7,22 @@ > > > > > > #pragma once > > > > > > +#include <variant> > > > + > > > #include <libcamera/base/file.h> > > > > > > #include "libcamera/internal/camera.h" > > > #include "libcamera/internal/dma_buf_allocator.h" > > > #include "libcamera/internal/pipeline_handler.h" > > > > > > +#include "frame_generator.h" > > > +#include "image_frame_generator.h" > > > #include "test_pattern_generator.h" > > > > > > namespace libcamera { > > > > > > +using VirtualFrame = std::variant<TestPattern, ImageFrames>; > > > + > > > class VirtualCameraData : public Camera::Private > > > { > > > public: > > > @@ -24,6 +30,13 @@ public: > > > Size size; > > > std::vector<int> frameRates; > > > }; > > > + /* The config file is parsed to the Configuration struct */ > > > + struct Configuration { > > > + std::string id; > > > + std::vector<Resolution> resolutions; > > > + VirtualFrame frame; > > > + }; > > > + > > > VirtualCameraData(PipelineHandler *pipe) > > > : Camera::Private(pipe) > > > { > > > @@ -31,12 +44,9 @@ public: > > > > > > ~VirtualCameraData() = default; > > > > > > - std::string id_; > > > - std::vector<Resolution> supportedResolutions_; > > > - TestPattern testPattern_; > > > - > > > + unsigned int frameCount_ = 0; > > > + Configuration config_; > > > Stream stream_; > > > - > > > std::unique_ptr<FrameGenerator> frameGenerator_; > > > }; > > > > > > -- > > > 2.46.0.184.g6999bdac58-goog > > > > > > > > > Regards, > > Barnabás Pőcze > BR, Harvey
Hi On Wed, Aug 21, 2024 at 02:05:57AM GMT, Barnabás Pőcze wrote: > Hi > > > 2024. augusztus 20., kedd 18:23 keltezéssel, Harvey Yang <chenghaoyang@chromium.org> írta: > > > From: Konami Shu <konamiz@google.com> > > > > - Refactor ImageFrameGenerator and TestPatternGenerator > > - Extend the config file to have section to choose to use images or test > > pattern > > - Extend Parser to parse the configuration > > - Add ImageFrameGenerator which uses images to provide frames Please make a proper commit message out of this > > > > Patchset1->2 > > - Extend the parser to accept files with ".jpeg" > > - Heap allocate some buffers in ImageFrameGenerator::generateFrame so > > the buffers won't cause stack over flow > > > > Patchset5->6 > > - Move static factory function from the interface class to the derived > > classes > > > > Patchset6->8 > > - create FrameGenerator before starting the camera Changelogs should go below --- So that they don't appear in git history > > > > Signed-off-by: Konami Shu <konamiz@google.com> > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org> > > Co-developed-by: Yunke Cao <yunkec@chromium.org> > > Co-developed-by: Tomasz Figa <tfiga@chromium.org> > > --- > > src/libcamera/pipeline/virtual/README.md | 18 +- > > .../pipeline/virtual/common_functions.cpp | 27 +++ > > .../pipeline/virtual/common_functions.h | 18 ++ > > .../pipeline/virtual/frame_generator.h | 2 +- > > .../virtual/image_frame_generator.cpp | 154 ++++++++++++++++++ > > .../pipeline/virtual/image_frame_generator.h | 65 ++++++++ > > src/libcamera/pipeline/virtual/meson.build | 6 +- > > src/libcamera/pipeline/virtual/parser.cpp | 77 +++++++-- > > src/libcamera/pipeline/virtual/parser.h | 5 +- > > .../virtual/test_pattern_generator.cpp | 4 +- > > .../pipeline/virtual/test_pattern_generator.h | 2 +- > > src/libcamera/pipeline/virtual/virtual.cpp | 37 +++-- > > src/libcamera/pipeline/virtual/virtual.h | 20 ++- > > 13 files changed, 390 insertions(+), 45 deletions(-) > > create mode 100644 src/libcamera/pipeline/virtual/common_functions.cpp > > create mode 100644 src/libcamera/pipeline/virtual/common_functions.h > > create mode 100644 src/libcamera/pipeline/virtual/image_frame_generator.cpp > > create mode 100644 src/libcamera/pipeline/virtual/image_frame_generator.h > > > > diff --git a/src/libcamera/pipeline/virtual/README.md b/src/libcamera/pipeline/virtual/README.md > > index 27d6283df..5e21ce74a 100644 > > --- a/src/libcamera/pipeline/virtual/README.md > > +++ b/src/libcamera/pipeline/virtual/README.md > > @@ -15,8 +15,13 @@ Each camera block is a dictionary, containing the following keys: > > - `supported_formats` (list of `VirtualCameraData::Resolution`, optional) : List of supported resolution and frame rates of the emulated camera > > - `width` (`unsigned int`, default=1920): Width of the window resolution. This needs to be even. > > - `height` (`unsigned int`, default=1080): Height of the window resolution. > > - - `frame_rates` (list of `int`, default=`[30,60]` ): Range of the frame rate. The list has to be two values of the lower bound and the upper bound of the frame rate. > > -- `test_pattern` (`string`, default="bars"): Which test pattern to use as frames. The options are "bars", "lines". > > + - `frame_rates` (list of `int`, default=`[30,60]` ): Range of the frame rate. The list has to be two values of the lower bound and the upper bound of the frame rate. This does not affect the frame rate for now. > > +- `frames` (dictionary): > > + - `path` (`string`, default="bars"): Name of a test pattern, path to an image, or path to a directory of a series of images. > > + - The test patterns are "bars" which means color bars, and "lines" which means diagonal lines. > > + - The path to an image has ".jpg" extension. > > + - 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. > > + - `scale_mode`(`string`, default="fill"): Scale mode when the frames are images. The scale modes are "fill", "contain", and "cover". This does not matter when frames is a test pattern. This does not affect the scale mode for now. > > - `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. > > - `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. > > > > @@ -35,13 +40,16 @@ A sample config file: > > frame_rates: > > - 70 > > - 80 > > - test_pattern: "bars" > > + frames: > > + path: "lines" > > location: "front" > > model: "Virtual Video Device" > > "Virtual1": > > supported_formats: > > - width: 800 > > - test_pattern: "lines" > > + frames: > > + path: "path/to/directory_of_images/" > > + scale_mode: "contain" > > location: "back" > > model: "Virtual Video Device1" > > "Virtual2": > > @@ -61,7 +69,7 @@ This is the procedure of the Parser class: > > - If the config file contains invalid configuration, this method returns nullptr. The camera will be skipped. > > 3. Parse each property and register the data. > > - `parseSupportedFormats()`: Parses `supported_formats` in the config, which contains resolutions and frame rates. > > - - `parseTestPattern()`: Parses `test_pattern` in the config. > > + - `parseFrame()`: Parses `frames` in the config. > > - `parseLocation()`: Parses `location` in the config. > > - `parseModel()`: Parses `model` in the config. > > 4. Back to `parseConfigFile()` and append the camera configuration. > > diff --git a/src/libcamera/pipeline/virtual/common_functions.cpp b/src/libcamera/pipeline/virtual/common_functions.cpp > > new file mode 100644 > > index 000000000..207827ee0 > > --- /dev/null > > +++ b/src/libcamera/pipeline/virtual/common_functions.cpp > > @@ -0,0 +1,27 @@ > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > +/* > > + * Copyright (C) 2023, Google Inc. > > + * > > + * common_functions.cpp - Helper that do not depend on any class > > + */ > > + > > +#include "common_functions.h" > > + > > +namespace libcamera { > > + > > +std::string getExtension(const std::string &path) > > This looks very fragile. Why not use `std::filesystem::path::extension()`? > > > > +{ > > + size_t i = path.find("."); > > + if (i != std::string::npos) { > > + return path.substr(i); > > + } > > + return ""; > > +} > > + > > +std::size_t numberOfFilesInDirectory(std::filesystem::path path) > > +{ > > + using std::filesystem::directory_iterator; > > + return std::distance(directory_iterator(path), directory_iterator{}); > > +} And inline this function so that you can drop common_functions.cpp/h completely > > + > > +} // namespace libcamera > > diff --git a/src/libcamera/pipeline/virtual/common_functions.h b/src/libcamera/pipeline/virtual/common_functions.h > > new file mode 100644 > > index 000000000..4203f9505 > > --- /dev/null > > +++ b/src/libcamera/pipeline/virtual/common_functions.h > > @@ -0,0 +1,18 @@ > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > +/* > > + * Copyright (C) 2023, Google Inc. > > + * > > + * common_functions.h - Helper that do not depend on any class > > + */ > > + > > +#pragma once > > + > > +#include <filesystem> > > + > > +namespace libcamera { > > + > > +std::string getExtension(const std::string &path); > > + > > +std::size_t numberOfFilesInDirectory(std::filesystem::path path); > > + > > +} // namespace libcamera > > diff --git a/src/libcamera/pipeline/virtual/frame_generator.h b/src/libcamera/pipeline/virtual/frame_generator.h > > index 9699af7a4..f69576b36 100644 > > --- a/src/libcamera/pipeline/virtual/frame_generator.h > > +++ b/src/libcamera/pipeline/virtual/frame_generator.h > > @@ -23,7 +23,7 @@ public: > > /** Fill the output frame buffer. > > * Use the frame at the frameCount of image frames > > */ > > - virtual void generateFrame(const Size &size, > > + virtual void generateFrame(unsigned int &frameCount, const Size &size, Are you passing frameCount by reference because you want the function to modify the value passed in by caller ? Use a pointer then which better conveys this semantic in my opinion. > > const FrameBuffer *buffer) = 0; > > > > protected: > > diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.cpp b/src/libcamera/pipeline/virtual/image_frame_generator.cpp > > new file mode 100644 > > index 000000000..d374877ff > > --- /dev/null > > +++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp > > @@ -0,0 +1,154 @@ > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > +/* > > + * Copyright (C) 2023, Google Inc. > > + * > > + * image_frame_generator.cpp - Derived class of FrameGenerator for > > + * generating frames from images > > + */ > > + > > +#include "image_frame_generator.h" > > + > > +#include <memory> > > +#include <optional> both already included by image_frame_generator.h > > +#include <string> > > + > > +#include <libcamera/base/file.h> > > +#include <libcamera/base/log.h> > > + > > +#include <libcamera/framebuffer.h> > > + > > +#include "libcamera/internal/mapped_framebuffer.h" > > + > > +#include "libyuv/convert.h" > > +#include "libyuv/scale.h" empty line > > +namespace libcamera { > > + > > +LOG_DECLARE_CATEGORY(Virtual) > > + > > +std::unique_ptr<ImageFrameGenerator> ImageFrameGenerator::create( > > + ImageFrames &imageFrames) Or rather std::unique_ptr<ImageFrameGenerator> ImageFrameGenerator::create(ImageFrames &imageFrames) { > > +{ > > + std::unique_ptr<ImageFrameGenerator> imageFrameGenerator = > > + std::make_unique<ImageFrameGenerator>(); > > + imageFrameGenerator->imageFrames_ = &imageFrames; > > + > > + /** For each file in the directory This is not doxygen. Drop /** and use the conventional commenting style /* * This is for multi * line comments */ > > + * load the image, convert it to NV12, and store the pointer > > + */ > > + for (unsigned int i = 0; i < imageFrames.number.value_or(1); i++) { > > + std::string path; > > + if (!imageFrames.number.has_value()) { I think you can drop has_value() as it's an alias for operator bool > > + /* If the path is to an image */ > > + path = imageFrames.path; > > + } else { > > + /* If the path is to a directory */ > > + path = constructPath(imageFrames.path, i); This assumes that if you're given a directory images are named 0.jpeg 1.jpeg etc. Up to you, but it would be trivial to enumerate all .jpeg files in a directory. True, you won't know in which order to display them then... > > + } No {} for single liners, please > > + > > + File file(path); > > + bool isOpen = file.open(File::OpenModeFlag::ReadOnly); > > + if (!isOpen) { > > I think `if (!file.open(...))` is fine. > > > > + LOG(Virtual, Error) << "Failed to open image file: " << file.fileName(); > > + return nullptr; > > + } > > + > > + /* Read the image file to data */ > > + uint8_t buffer[file.size()]; > > It's probably best to avoid VLAs. > > > > + Span<unsigned char> data{ buffer, (unsigned long)file.size() }; You can use uint8_t here as well for consistency No plain C-style casts. In the version I pulled from fdo you can declare fileSize a unsigned long and replace > > + long dataSize = file.read(data); with auto buffer = std::make_unique<uint8_t[]>(fileSize); long dataSize = file.read({ buffer.get(), fileSize }); then replace all occurrences of data.data() with buffer.get(). Also 'dataSize' and 'fileSize' should have the same value, so I think you can drop dataSize (or if you want to make sure do if (file.read({ buffer.get(), fileSize }) != fileSize) error out > > + > > + /* Get the width and height of the image */ > > + int width, height; unsigned > > + if (libyuv::MJPGSize(data.data(), dataSize, &width, &height)) { > > + LOG(Virtual, Error) << "Failed to get the size of the image file: " > > + << file.fileName(); > > + return nullptr; > > + } > > + > > + /* Convert to NV12 and write the data to tmpY and tmpUV */ > > + int halfWidth = (width + 1) / 2; > > + int halfHeight = (height + 1) / 2; unsigned > > + std::unique_ptr<uint8_t[]> dstY = > > + std::make_unique<uint8_t[]>(width * height); > > + std::unique_ptr<uint8_t[]> dstUV = > > + std::make_unique<uint8_t[]>(halfWidth * halfHeight * 2); > > + int ret = libyuv::MJPGToNV12(data.data(), dataSize, > > + dstY.get(), width, dstUV.get(), > > + width, width, height, width, height); > > + if (ret != 0) { > > + LOG(Virtual, Error) << "MJPGToNV12() failed with " << ret; > > + } no {} > > + > > + imageFrameGenerator->imageFrameDatas_.emplace_back( > > + ImageFrameData{ std::move(dstY), std::move(dstUV), > > + Size(width, height) }); > > + } empty line before return > > + return imageFrameGenerator; > > +} > > + > > +std::string ImageFrameGenerator::constructPath(std::string &name, unsigned int &i) > > Does this need to be in `ImageFrameGenerator`? Is it not enough to have it in an > anonymous namespace in this file? In the version I have pulled from gitlab this is now in an anonymous namespace. The real question I have is what is the point of a one liner function called from a single place. Drop it and inline it in the caller please > > > > +{ > > + return name + std::to_string(i) + ".jpg"; > > +} > > + > > +void ImageFrameGenerator::configure(const Size &size) > > +{ > > + for (unsigned int i = 0; i < imageFrames_->number.value_or(1); i++) { > > + /* Scale the imageFrameDatas_ to scaledY and scaledUV */ > > + int halfSizeWidth = (size.width + 1) / 2; > > + int halfSizeHeight = (size.height + 1) / 2; unsigned > > + std::unique_ptr<uint8_t[]> scaledY = > > + std::make_unique<uint8_t[]>(size.width * size.height); > > + std::unique_ptr<uint8_t[]> scaledUV = > > + std::make_unique<uint8_t[]>(halfSizeWidth * halfSizeHeight * 2); > > + auto &src = imageFrameDatas_[i]; > > + > > + /* > > + * \todo Implement "contain" & "cover", based on > > + * |imageFrames_[i].scaleMode|. > > + */ drop them from the class and documentation if not implemented > > + > > + /* > > + * \todo Some platforms might enforce stride due to GPU, like > > + * ChromeOS ciri (64). The weight needs to be a multiple of > > + * the stride to work properly for now. Indented with spaces > > + */ > > + libyuv::NV12Scale(src.Y.get(), src.size.width, > > + src.UV.get(), src.size.width, > > + src.size.width, src.size.height, > > + scaledY.get(), size.width, scaledUV.get(), size.width, > > + size.width, size.height, libyuv::FilterMode::kFilterBilinear); > > + > > + /* Store the pointers to member variable */ rather useless comment > > + scaledFrameDatas_.emplace_back( > > + ImageFrameData{ std::move(scaledY), std::move(scaledUV), size }); > > + } > > +} > > + > > +void ImageFrameGenerator::generateFrame(unsigned int &frameCount, const Size &size, const FrameBuffer *buffer) > > +{ > > + /* Don't do anything when the list of buffers is empty*/ > > + if (scaledFrameDatas_.size() == 0) > > + return; can this happen ? If create() fails, you shouln't be able to call generateFrames() and configure() doesn't seem to fail anywhere. > > + > > + MappedFrameBuffer mappedFrameBuffer(buffer, MappedFrameBuffer::MapFlag::Write); > > + > > + auto planes = mappedFrameBuffer.planes(); > > + > > + /* Make sure the frameCount does not over the number of images */ > > + frameCount %= imageFrames_->number.value_or(1); am I wrong or if you have a single image the index you want is 0 not 1 ? > > + > > + /* Write the scaledY and scaledUV to the mapped frame buffer */ > > + libyuv::NV12Copy(scaledFrameDatas_[frameCount].Y.get(), size.width, > > + scaledFrameDatas_[frameCount].UV.get(), size.width, planes[0].begin(), > > + size.width, planes[1].begin(), size.width, > > + size.width, size.height); > > + > > + /* proceed an image every 4 frames */ > > + /* \todo read the parameter_ from the configuration file? */ > > + parameter_++; this is never initialized > > + if (parameter_ % 4 == 0) > > + frameCount++; > > +} > > + > > +} // namespace libcamera > > diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.h b/src/libcamera/pipeline/virtual/image_frame_generator.h > > new file mode 100644 > > index 000000000..74468e075 > > --- /dev/null > > +++ b/src/libcamera/pipeline/virtual/image_frame_generator.h > > @@ -0,0 +1,65 @@ > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > +/* > > + * Copyright (C) 2023, Google Inc. > > + * > > + * image_frame_generator.h - Derived class of FrameGenerator for > > + * generating frames from images > > + */ > > + > > +#pragma once > > + > > +#include <memory> > > +#include <optional> > > +#include <stdint.h> > > +#include <sys/types.h> > > + > > +#include "frame_generator.h" > > + > > +namespace libcamera { > > + > > +enum class ScaleMode : char { > > + Fill = 0, > > + Contain = 1, > > + Cover = 2, > > +}; > > + > > +/* Frame configuration provided by the config file */ > > +struct ImageFrames { > > + std::string path; > > I think `std::filesystem::path` would be preferable. > yes! > > > + ScaleMode scaleMode; > > + std::optional<unsigned int> number; What's the point of an optional ? The Parser::parseFrame() initializes this with either: data->config_.frame = ImageFrames{ path, scaleMode, std::nullopt }; or data->config_.frame = ImageFrames{ path, scaleMode, numberOfFilesInDirectory(path) }; and you always get it with value_or(1). Why not set it to 1 directly then ? > > +}; > > + > > +class ImageFrameGenerator : public FrameGenerator > > +{ > > +public: > > + /** Factory function to create an ImageFrameGenerator object. This is not doxygen. Use the canonical commenting style used throughout the whole code base. Move comments to function to the cpp file. > > + * Read the images and convert them to buffers in NV12 format. > > + * Store the pointers to the buffers to a list (imageFrameDatas) > > + */ > > + static std::unique_ptr<ImageFrameGenerator> create(ImageFrames &imageFrames); > > + > > +private: > > + struct ImageFrameData { > > + std::unique_ptr<uint8_t[]> Y; > > + std::unique_ptr<uint8_t[]> UV; > > + Size size; > > + }; > > + > > + /* Scale the buffers for image frames. */ > > + void configure(const Size &size) override; > > + void generateFrame(unsigned int &frameCount, const Size &size, const FrameBuffer *buffer) override; You could easily void generateFrame(unsigned int &frameCount, const Size &size, const FrameBuffer *buffer) override; From coding-style.rst * 2 "Breaking Long Lines" striving to fit code within 80 columns and accepting up to 120 columns when necessary > > + > > + static std::string constructPath(std::string &name, unsigned int &i); > > + > > + /* List of pointers to the not scaled image buffers */ > > + std::vector<ImageFrameData> imageFrameDatas_; > > + /* List of pointers to the scaled image buffers */ > > + std::vector<ImageFrameData> scaledFrameDatas_; > > + /* Pointer to the imageFrames_ in VirtualCameraData */ > > + ImageFrames *imageFrames_; > > + /* Speed parameter. Change to the next image every parameter_ frames. */ > > + int parameter_; unsigned as long as it's not configurable make it a constexpr > > +}; > > + > > +} /* namespace libcamera */ > > diff --git a/src/libcamera/pipeline/virtual/meson.build b/src/libcamera/pipeline/virtual/meson.build > > index 2e82e64cb..d56aedec4 100644 > > --- a/src/libcamera/pipeline/virtual/meson.build > > +++ b/src/libcamera/pipeline/virtual/meson.build > > @@ -2,11 +2,14 @@ > > > > libcamera_sources += files([ > > 'virtual.cpp', > > - 'test_pattern_generator.cpp', > > 'parser.cpp', > > + 'test_pattern_generator.cpp', > > + 'image_frame_generator.cpp', > > + 'common_functions.cpp', > > ]) > > > > libyuv_dep = dependency('libyuv', required : false) > > +libjpeg = dependency('libjpeg', required : false) where is this required ? > > > > # Fallback to a subproject if libyuv isn't found, as it's typically not > > # provided by distributions. > > @@ -26,3 +29,4 @@ if not libyuv_dep.found() > > endif > > > > libcamera_deps += [libyuv_dep] > > +libcamera_deps += [libjpeg] > > diff --git a/src/libcamera/pipeline/virtual/parser.cpp b/src/libcamera/pipeline/virtual/parser.cpp > > index 032c0cd9d..46d1ab181 100644 > > --- a/src/libcamera/pipeline/virtual/parser.cpp > > +++ b/src/libcamera/pipeline/virtual/parser.cpp > > @@ -18,6 +18,7 @@ > > #include "libcamera/internal/pipeline_handler.h" > > #include "libcamera/internal/yaml_parser.h" > > > > +#include "common_functions.h" drop this > > #include "virtual.h" > > > > namespace libcamera { > > @@ -52,12 +53,12 @@ std::vector<std::unique_ptr<VirtualCameraData>> Parser::parseConfigFile( > > continue; > > } > > > > - data->id_ = cameraId; > > + data->config_.id = cameraId; > > ControlInfoMap::Map controls; > > /* todo: Check which resolution's frame rate to be reported */ > > controls[&controls::FrameDurationLimits] = > > - ControlInfo(int64_t(1000 / data->supportedResolutions_[0].frameRates[1]), > > - int64_t(1000 / data->supportedResolutions_[0].frameRates[0])); > > + ControlInfo(int64_t(1000 / data->config_.resolutions[0].frameRates[1]), > > + int64_t(1000 / data->config_.resolutions[0].frameRates[0])); > > data->controlInfo_ = ControlInfoMap(std::move(controls), controls::controls); > > configurations.push_back(std::move(data)); > > } > > @@ -72,7 +73,7 @@ std::unique_ptr<VirtualCameraData> Parser::parseCameraConfigData( > > if (parseSupportedFormats(cameraConfigData, data.get())) > > return nullptr; > > > > - if (parseTestPattern(cameraConfigData, data.get())) > > + if (parseFrame(cameraConfigData, data.get())) > > return nullptr; > > > > if (parseLocation(cameraConfigData, data.get())) > > @@ -122,14 +123,14 @@ int Parser::parseSupportedFormats( > > frameRates.push_back(60); > > } > > > > - data->supportedResolutions_.emplace_back( > > + data->config_.resolutions.emplace_back( > > VirtualCameraData::Resolution{ Size{ width, height }, > > frameRates }); > > > > activeResolution = std::max(activeResolution, Size{ width, height }); > > } > > } else { > > - data->supportedResolutions_.emplace_back( > > + data->config_.resolutions.emplace_back( > > VirtualCameraData::Resolution{ Size{ 1920, 1080 }, > > { 30, 60 } }); > > activeResolution = Size(1920, 1080); > > @@ -141,21 +142,65 @@ int Parser::parseSupportedFormats( > > return 0; > > } > > > > -int Parser::parseTestPattern( > > +int Parser::parseFrame( > > const YamlObject &cameraConfigData, VirtualCameraData *data) Why ?? int Parser::parseFrame(const YamlObject &cameraConfigData, VirtualCameraData *data) is fine or int Parser::parseFrame(const YamlObject &cameraConfigData, VirtualCameraData *data) if you really want to > > { > > - std::string testPattern = cameraConfigData["test_pattern"].get<std::string>().value(); > > + const YamlObject &frames = cameraConfigData["frames"]; empty line please > > + /* When there is no frames provided in the config file, use color bar test pattern */ > > + if (frames.size() == 0) { > > + data->config_.frame = TestPattern::ColorBars; > > + return 0; > > + } > > + > > + if (!frames.isDictionary()) { > > + LOG(Virtual, Error) << "'frames' is not a dictionary."; > > + return -EINVAL; > > + } > > + > > + std::string path = frames["path"].get<std::string>().value(); > > What if `path` is not specified? Is it checked somewhere else? > I would really make the "path" and "test_pattern" cases separate, otherwise you're here parsing a string that can be either a system path (which you have to validate) or "bars"\"lines" that are used for TPG. > > > + > > + if (auto ext = getExtension(path); ext == ".jpg" || ext == ".jpeg") { > > + ScaleMode scaleMode; declaring a variable inside the if () clause is valid but rather unusual (at least to me) > > + if (parseScaleMode(frames, &scaleMode)) > > + return -EINVAL; > > + data->config_.frame = ImageFrames{ path, scaleMode, std::nullopt }; > > + } else if (path.back() == '/') { > > This looks very fragile. Why not use `std::filesystem::symlink_status()` to check > if it's a file or a directory. > > > > + ScaleMode scaleMode; > > + if (parseScaleMode(frames, &scaleMode)) > > + return -EINVAL; > > + data->config_.frame = ImageFrames{ path, scaleMode, > > + numberOfFilesInDirectory(path) }; > > Admittedly I did not check in detail, but is this necessary? Is it necessary > to count the number of files in advance? > > > > + } else if (path == "bars" || path == "") { > > + /* Default value is "bars" */ > > + data->config_.frame = TestPattern::ColorBars; > > + } else if (path == "lines") { > > + data->config_.frame = TestPattern::DiagonalLines; > > + } else { > > + LOG(Virtual, Error) << "Frame: " << path > > + << " is not supported"; > > + return -EINVAL; > > + } Could you consider reworking the above by changing the schema to separate TPG from images ? Empty line > > + return 0; > > +} > > > > - /* Default value is "bars" */ > > - if (testPattern == "bars" || testPattern == "") { > > - data->testPattern_ = TestPattern::ColorBars; > > - } else if (testPattern == "lines") { > > - data->testPattern_ = TestPattern::DiagonalLines; > > +int Parser::parseScaleMode( > > + const YamlObject &framesConfigData, ScaleMode *scaleMode) > > +{ > > + std::string mode = framesConfigData["scale_mode"].get<std::string>().value(); > > Same here, regarding empty optionals. > > > > + > > + /* Default value is fill */ > > + if (mode == "fill" || mode == "") { > > + *scaleMode = ScaleMode::Fill; > > + } else if (mode == "contain") { > > + *scaleMode = ScaleMode::Contain; > > + } else if (mode == "cover") { > > + *scaleMode = ScaleMode::Cover; You don't seem to support these in code, why allow them ? Drop them all and assume the only supported one for now > > } else { > > - LOG(Virtual, Error) << "Test pattern: " << testPattern > > - << "is not supported"; > > + LOG(Virtual, Error) << "scaleMode: " << mode > > + << " is not supported"; > > return -EINVAL; > > } > > + > > return 0; > > } > > > > @@ -195,4 +240,4 @@ int Parser::parseModel( > > return 0; > > } > > > > -} /* namespace libcamera */ > > +} // namespace libcamera Why oh why ? > > diff --git a/src/libcamera/pipeline/virtual/parser.h b/src/libcamera/pipeline/virtual/parser.h > > index a377d8aa1..38ea460d5 100644 > > --- a/src/libcamera/pipeline/virtual/parser.h > > +++ b/src/libcamera/pipeline/virtual/parser.h > > @@ -34,12 +34,15 @@ private: > > > > int parseSupportedFormats( > > const YamlObject &cameraConfigData, VirtualCameraData *data); > > - int parseTestPattern( > > + int parseFrame( > > const YamlObject &cameraConfigData, VirtualCameraData *data); > > int parseLocation( > > const YamlObject &cameraConfigData, VirtualCameraData *data); > > int parseModel( > > const YamlObject &cameraConfigData, VirtualCameraData *data); > > + > > + int parseScaleMode( > > + const YamlObject &framesConfigData, ScaleMode *scaleMode); I've gave up trying to understand these indentation rules > > }; > > > > } // namespace libcamera > > diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > > index 6df9b31e9..844dffd49 100644 > > --- a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > > +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > > @@ -20,7 +20,7 @@ LOG_DECLARE_CATEGORY(Virtual) > > static const unsigned int kARGBSize = 4; > > > > void TestPatternGenerator::generateFrame( > > - const Size &size, > > + [[maybe_unused]] unsigned int &frameCount, const Size &size, > > const FrameBuffer *buffer) > > { > > MappedFrameBuffer mappedFrameBuffer(buffer, > > @@ -29,7 +29,7 @@ void TestPatternGenerator::generateFrame( > > auto planes = mappedFrameBuffer.planes(); > > > > /* TODO: select whether to do shifting or not */ > > - shiftLeft(size); > > + // shiftLeft(size); Is this related at all ? > > > > /* Convert the template_ to the frame buffer */ > > int ret = libyuv::ARGBToNV12( > > diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.h b/src/libcamera/pipeline/virtual/test_pattern_generator.h > > index ed8d4e43b..cebdd0141 100644 > > --- a/src/libcamera/pipeline/virtual/test_pattern_generator.h > > +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.h > > @@ -25,7 +25,7 @@ enum class TestPattern : char { > > class TestPatternGenerator : public FrameGenerator > > { > > private: > > - void generateFrame(const Size &size, > > + void generateFrame(unsigned int &frameCount, const Size &size, > > const FrameBuffer *buffer) override; > > > > protected: > > diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp > > index 0fe471f00..2412af703 100644 > > --- a/src/libcamera/pipeline/virtual/virtual.cpp > > +++ b/src/libcamera/pipeline/virtual/virtual.cpp > > @@ -20,9 +20,13 @@ > > #include "libcamera/internal/pipeline_handler.h" > > #include "libcamera/internal/yaml_parser.h" > > > > -#include "frame_generator.h" > > #include "parser.h" > > > > +#define ifTestPattern(v) std::holds_alternative<TestPattern>(v) > > +#define getTestPattern(v) std::get<TestPattern>(v) > > +#define ifImageFrames(v) std::holds_alternative<ImageFrames>(v) > > +#define getImageFrames(v) std::get<ImageFrames>(v) > > + > > namespace libcamera { > > > > LOG_DEFINE_CATEGORY(Virtual) > > @@ -63,12 +67,12 @@ CameraConfiguration::Status VirtualCameraConfiguration::validate() > > } > > > > Size maxSize; > > - for (const auto &resolution : data_->supportedResolutions_) > > + for (const auto &resolution : data_->config_.resolutions) > > maxSize = std::max(maxSize, resolution.size); > > > > for (StreamConfiguration &cfg : config_) { > > bool found = false; > > - for (const auto &resolution : data_->supportedResolutions_) { > > + for (const auto &resolution : data_->config_.resolutions) { > > if (resolution.size.width == cfg.size.width && > > resolution.size.height == cfg.size.height) { > > found = true; > > @@ -110,7 +114,7 @@ PipelineHandlerVirtual::generateConfiguration(Camera *camera, > > return config; > > > > Size minSize, sensorResolution; > > - for (const auto &resolution : data->supportedResolutions_) { > > + for (const auto &resolution : data->config_.resolutions) { > > if (minSize.isNull() || minSize > resolution.size) > > minSize = resolution.size; > > > > @@ -199,7 +203,7 @@ int PipelineHandlerVirtual::exportFrameBuffers( > > int PipelineHandlerVirtual::start(Camera *camera, > > [[maybe_unused]] const ControlList *controls) > > { > > - /* \todo Start reading the virtual video if any. */ > > + /* Start reading the images/generating test patterns */ > > VirtualCameraData *data = cameraData(camera); > > > > data->frameGenerator_->configure(data->stream_.configuration().size); > > @@ -219,8 +223,8 @@ int PipelineHandlerVirtual::queueRequestDevice([[maybe_unused]] Camera *camera, > > > > /* \todo Read from the virtual video if any. */ > > for (auto const &[stream, buffer] : request->buffers()) { > > - /* map buffer and fill test patterns */ > > - data->frameGenerator_->generateFrame(stream->configuration().size, buffer); > > + /* Map buffer. Fill test patterns or images */ > > + data->frameGenerator_->generateFrame(data->frameCount_, stream->configuration().size, buffer); I don't see frameCount_ be used anywhere here, but I suspect you want it be modified by generateFrame() (answers my first question of the email). I don't know what you intend to use it for, but if, as the name suggest you want to count frames, you can do it here without the frameGenerator being involved ? It's one-request-one-frame after all, isn't it ? So you don't have to pollute TestPatternGenerator::generateFrame with a [[maybe_unused]] unsigned int &frameCount, > > completeBuffer(request, buffer); > > } > > > > @@ -250,9 +254,10 @@ bool PipelineHandlerVirtual::match([[maybe_unused]] DeviceEnumerator *enumerator > > /* Configure and register cameras with configData */ > > for (auto &data : configData) { > > std::set<Stream *> streams{ &data->stream_ }; > > - std::string id = data->id_; > > + std::string id = data->config_.id; > > std::shared_ptr<Camera> camera = Camera::create(std::move(data), id, streams); > > > > + /* Initialize FrameGenerator*/ > > initFrameGenerator(camera.get()); > > > > registerCamera(std::move(camera)); > > @@ -264,13 +269,19 @@ bool PipelineHandlerVirtual::match([[maybe_unused]] DeviceEnumerator *enumerator > > void PipelineHandlerVirtual::initFrameGenerator(Camera *camera) > > { > > auto data = cameraData(camera); > > - if (data->testPattern_ == TestPattern::DiagonalLines) { > > - data->frameGenerator_ = DiagonalLinesGenerator::create(); > > - } else { > > - data->frameGenerator_ = ColorBarsGenerator::create(); > > + auto &frame = data->config_.frame; > > + if (ifTestPattern(frame)) { > > + TestPattern &testPattern = getTestPattern(frame); > > + if (testPattern == TestPattern::DiagonalLines) { > > + data->frameGenerator_ = DiagonalLinesGenerator::create(); > > + } else { > > + data->frameGenerator_ = ColorBarsGenerator::create(); > > + } > > + } else if (ifImageFrames(frame)) { > > + data->frameGenerator_ = ImageFrameGenerator::create(getImageFrames(frame)); > > } > > I think I would strongly prefer `std::visit()` here. But even `std::get_if()` is better in my opinion. Regarding the revistied version on fdo if (testPattern == TestPattern::DiagonalLines) { data->frameGenerator_ = DiagonalLinesGenerator::create(); } else { data->frameGenerator_ = ColorBarsGenerator::create(); } again, no {} for single line statements > > > > } > > > > REGISTER_PIPELINE_HANDLER(PipelineHandlerVirtual, "virtual") > > > > -} /* namespace libcamera */ > > +} // namespace libcamera meh > > diff --git a/src/libcamera/pipeline/virtual/virtual.h b/src/libcamera/pipeline/virtual/virtual.h > > index c1ac4eb90..f41a4a906 100644 > > --- a/src/libcamera/pipeline/virtual/virtual.h > > +++ b/src/libcamera/pipeline/virtual/virtual.h > > @@ -7,16 +7,22 @@ > > > > #pragma once > > > > +#include <variant> > > + > > #include <libcamera/base/file.h> > > > > #include "libcamera/internal/camera.h" > > #include "libcamera/internal/dma_buf_allocator.h" > > #include "libcamera/internal/pipeline_handler.h" > > > > +#include "frame_generator.h" > > +#include "image_frame_generator.h" > > #include "test_pattern_generator.h" > > > > namespace libcamera { > > > > +using VirtualFrame = std::variant<TestPattern, ImageFrames>; > > + > > class VirtualCameraData : public Camera::Private > > { > > public: > > @@ -24,6 +30,13 @@ public: > > Size size; > > std::vector<int> frameRates; > > }; > > + /* The config file is parsed to the Configuration struct */ > > + struct Configuration { > > + std::string id; so you need <string> > > + std::vector<Resolution> resolutions; and <vector> > > + VirtualFrame frame; > > + }; > > + > > VirtualCameraData(PipelineHandler *pipe) > > : Camera::Private(pipe) > > { > > @@ -31,12 +44,9 @@ public: > > > > ~VirtualCameraData() = default; > > > > - std::string id_; > > - std::vector<Resolution> supportedResolutions_; well, you did already > > - TestPattern testPattern_; > > - > > + unsigned int frameCount_ = 0; > > + Configuration config_; > > Stream stream_; > > - > > std::unique_ptr<FrameGenerator> frameGenerator_; > > }; > > > > -- > > 2.46.0.184.g6999bdac58-goog > > > > > Regards, > Barnabás Pőcze
Hi Jacopo, On Wed, Aug 28, 2024 at 10:14 AM Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > Hi > > On Wed, Aug 21, 2024 at 02:05:57AM GMT, Barnabás Pőcze wrote: > > Hi > > > > > > 2024. augusztus 20., kedd 18:23 keltezéssel, Harvey Yang < > chenghaoyang@chromium.org> írta: > > > > > From: Konami Shu <konamiz@google.com> > > > > > > - Refactor ImageFrameGenerator and TestPatternGenerator > > > - Extend the config file to have section to choose to use images or > test > > > pattern > > > - Extend Parser to parse the configuration > > > - Add ImageFrameGenerator which uses images to provide frames > > Please make a proper commit message out of this > Updated. > > > > > > > Patchset1->2 > > > - Extend the parser to accept files with ".jpeg" > > > - Heap allocate some buffers in ImageFrameGenerator::generateFrame so > > > the buffers won't cause stack over flow > > > > > > Patchset5->6 > > > - Move static factory function from the interface class to the derived > > > classes > > > > > > Patchset6->8 > > > - create FrameGenerator before starting the camera > > Changelogs should go below > Let's put them in the cover letter, if it's even needed. > > --- > > So that they don't appear in git history > > > > > > > Signed-off-by: Konami Shu <konamiz@google.com> > > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org> > > > Co-developed-by: Yunke Cao <yunkec@chromium.org> > > > Co-developed-by: Tomasz Figa <tfiga@chromium.org> > > > --- > > > src/libcamera/pipeline/virtual/README.md | 18 +- > > > .../pipeline/virtual/common_functions.cpp | 27 +++ > > > .../pipeline/virtual/common_functions.h | 18 ++ > > > .../pipeline/virtual/frame_generator.h | 2 +- > > > .../virtual/image_frame_generator.cpp | 154 ++++++++++++++++++ > > > .../pipeline/virtual/image_frame_generator.h | 65 ++++++++ > > > src/libcamera/pipeline/virtual/meson.build | 6 +- > > > src/libcamera/pipeline/virtual/parser.cpp | 77 +++++++-- > > > src/libcamera/pipeline/virtual/parser.h | 5 +- > > > .../virtual/test_pattern_generator.cpp | 4 +- > > > .../pipeline/virtual/test_pattern_generator.h | 2 +- > > > src/libcamera/pipeline/virtual/virtual.cpp | 37 +++-- > > > src/libcamera/pipeline/virtual/virtual.h | 20 ++- > > > 13 files changed, 390 insertions(+), 45 deletions(-) > > > create mode 100644 src/libcamera/pipeline/virtual/common_functions.cpp > > > create mode 100644 src/libcamera/pipeline/virtual/common_functions.h > > > create mode 100644 > src/libcamera/pipeline/virtual/image_frame_generator.cpp > > > create mode 100644 > src/libcamera/pipeline/virtual/image_frame_generator.h > > > > > > diff --git a/src/libcamera/pipeline/virtual/README.md > b/src/libcamera/pipeline/virtual/README.md > > > index 27d6283df..5e21ce74a 100644 > > > --- a/src/libcamera/pipeline/virtual/README.md > > > +++ b/src/libcamera/pipeline/virtual/README.md > > > @@ -15,8 +15,13 @@ Each camera block is a dictionary, containing the > following keys: > > > - `supported_formats` (list of `VirtualCameraData::Resolution`, > optional) : List of supported resolution and frame rates of the emulated > camera > > > - `width` (`unsigned int`, default=1920): Width of the window > resolution. This needs to be even. > > > - `height` (`unsigned int`, default=1080): Height of the window > resolution. > > > - - `frame_rates` (list of `int`, default=`[30,60]` ): Range of the > frame rate. The list has to be two values of the lower bound and the upper > bound of the frame rate. > > > -- `test_pattern` (`string`, default="bars"): Which test pattern to > use as frames. The options are "bars", "lines". > > > + - `frame_rates` (list of `int`, default=`[30,60]` ): Range of the > frame rate. The list has to be two values of the lower bound and the upper > bound of the frame rate. This does not affect the frame rate for now. > > > +- `frames` (dictionary): > > > + - `path` (`string`, default="bars"): Name of a test pattern, path > to an image, or path to a directory of a series of images. > > > + - The test patterns are "bars" which means color bars, and > "lines" which means diagonal lines. > > > + - The path to an image has ".jpg" extension. > > > + - 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. > > > + - `scale_mode`(`string`, default="fill"): Scale mode when the > frames are images. The scale modes are "fill", "contain", and "cover". This > does not matter when frames is a test pattern. This does not affect the > scale mode for now. > > > - `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. > > > - `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. > > > > > > @@ -35,13 +40,16 @@ A sample config file: > > > frame_rates: > > > - 70 > > > - 80 > > > - test_pattern: "bars" > > > + frames: > > > + path: "lines" > > > location: "front" > > > model: "Virtual Video Device" > > > "Virtual1": > > > supported_formats: > > > - width: 800 > > > - test_pattern: "lines" > > > + frames: > > > + path: "path/to/directory_of_images/" > > > + scale_mode: "contain" > > > location: "back" > > > model: "Virtual Video Device1" > > > "Virtual2": > > > @@ -61,7 +69,7 @@ This is the procedure of the Parser class: > > > - If the config file contains invalid configuration, this method > returns nullptr. The camera will be skipped. > > > 3. Parse each property and register the data. > > > - `parseSupportedFormats()`: Parses `supported_formats` in the > config, which contains resolutions and frame rates. > > > - - `parseTestPattern()`: Parses `test_pattern` in the config. > > > + - `parseFrame()`: Parses `frames` in the config. > > > - `parseLocation()`: Parses `location` in the config. > > > - `parseModel()`: Parses `model` in the config. > > > 4. Back to `parseConfigFile()` and append the camera configuration. > > > diff --git a/src/libcamera/pipeline/virtual/common_functions.cpp > b/src/libcamera/pipeline/virtual/common_functions.cpp > > > new file mode 100644 > > > index 000000000..207827ee0 > > > --- /dev/null > > > +++ b/src/libcamera/pipeline/virtual/common_functions.cpp > > > @@ -0,0 +1,27 @@ > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > +/* > > > + * Copyright (C) 2023, Google Inc. > > > + * > > > + * common_functions.cpp - Helper that do not depend on any class > > > + */ > > > + > > > +#include "common_functions.h" > > > + > > > +namespace libcamera { > > > + > > > +std::string getExtension(const std::string &path) > > > > This looks very fragile. Why not use > `std::filesystem::path::extension()`? > > > > > > > +{ > > > + size_t i = path.find("."); > > > + if (i != std::string::npos) { > > > + return path.substr(i); > > > + } > > > + return ""; > > > +} > > > + > > > +std::size_t numberOfFilesInDirectory(std::filesystem::path path) > > > +{ > > > + using std::filesystem::directory_iterator; > > > + return std::distance(directory_iterator(path), > directory_iterator{}); > > > +} > > And inline this function so that you can drop common_functions.cpp/h > completely > Done > > > > + > > > +} // namespace libcamera > > > diff --git a/src/libcamera/pipeline/virtual/common_functions.h > b/src/libcamera/pipeline/virtual/common_functions.h > > > new file mode 100644 > > > index 000000000..4203f9505 > > > --- /dev/null > > > +++ b/src/libcamera/pipeline/virtual/common_functions.h > > > @@ -0,0 +1,18 @@ > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > +/* > > > + * Copyright (C) 2023, Google Inc. > > > + * > > > + * common_functions.h - Helper that do not depend on any class > > > + */ > > > + > > > +#pragma once > > > + > > > +#include <filesystem> > > > + > > > +namespace libcamera { > > > + > > > +std::string getExtension(const std::string &path); > > > + > > > +std::size_t numberOfFilesInDirectory(std::filesystem::path path); > > > + > > > +} // namespace libcamera > > > diff --git a/src/libcamera/pipeline/virtual/frame_generator.h > b/src/libcamera/pipeline/virtual/frame_generator.h > > > index 9699af7a4..f69576b36 100644 > > > --- a/src/libcamera/pipeline/virtual/frame_generator.h > > > +++ b/src/libcamera/pipeline/virtual/frame_generator.h > > > @@ -23,7 +23,7 @@ public: > > > /** Fill the output frame buffer. > > > * Use the frame at the frameCount of image frames > > > */ > > > - virtual void generateFrame(const Size &size, > > > + virtual void generateFrame(unsigned int &frameCount, const Size > &size, > > Are you passing frameCount by reference because you want the function > to modify the value passed in by caller ? Use a pointer then which > better conveys this semantic in my opinion. > Done. Thanks! > > > > const FrameBuffer *buffer) = 0; > > > > > > protected: > > > diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.cpp > b/src/libcamera/pipeline/virtual/image_frame_generator.cpp > > > new file mode 100644 > > > index 000000000..d374877ff > > > --- /dev/null > > > +++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp > > > @@ -0,0 +1,154 @@ > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > +/* > > > + * Copyright (C) 2023, Google Inc. > > > + * > > > + * image_frame_generator.cpp - Derived class of FrameGenerator for > > > + * generating frames from images > > > + */ > > > + > > > +#include "image_frame_generator.h" > > > + > > > +#include <memory> > > > +#include <optional> > > both already included by image_frame_generator.h > Removed. I added memory again as the source file uses std::make_unique on its own as well, but anyways :) > > > > +#include <string> > > > + > > > +#include <libcamera/base/file.h> > > > +#include <libcamera/base/log.h> > > > + > > > +#include <libcamera/framebuffer.h> > > > + > > > +#include "libcamera/internal/mapped_framebuffer.h" > > > + > > > +#include "libyuv/convert.h" > > > +#include "libyuv/scale.h" > > empty line > Done > > > > +namespace libcamera { > > > + > > > +LOG_DECLARE_CATEGORY(Virtual) > > > + > > > +std::unique_ptr<ImageFrameGenerator> ImageFrameGenerator::create( > > > + ImageFrames &imageFrames) > > Or rather > > std::unique_ptr<ImageFrameGenerator> > ImageFrameGenerator::create(ImageFrames &imageFrames) > { > Done > > > > +{ > > > + std::unique_ptr<ImageFrameGenerator> imageFrameGenerator = > > > + std::make_unique<ImageFrameGenerator>(); > > > + imageFrameGenerator->imageFrames_ = &imageFrames; > > > + > > > + /** For each file in the directory > > This is not doxygen. Drop /** and use the conventional commenting > style > /* > * This is for multi > * line comments > */ > > Done. Thanks! > > > + * load the image, convert it to NV12, and store the pointer > > > + */ > > > + for (unsigned int i = 0; i < imageFrames.number.value_or(1); i++) { > > > + std::string path; > > > + if (!imageFrames.number.has_value()) { > > I think you can drop has_value() as it's an alias for operator bool > Done > > > > + /* If the path is to an image */ > > > + path = imageFrames.path; > > > + } else { > > > + /* If the path is to a directory */ > > > + path = constructPath(imageFrames.path, i); > > This assumes that if you're given a directory images are named > 0.jpeg 1.jpeg etc. Up to you, but it would be trivial to enumerate all > .jpeg files in a directory. True, you won't know in which order to > display them then... > Yes, the original concern is exactly about the order... Do you think we should use alphabetic order instead? > > > > + } > > No {} for single liners, please > Hmm, even with comments that make them more than one line? > > > > + > > > + File file(path); > > > + bool isOpen = file.open(File::OpenModeFlag::ReadOnly); > > > + if (!isOpen) { > > > > I think `if (!file.open(...))` is fine. > > > > > > > + LOG(Virtual, Error) << "Failed to open image file: > " << file.fileName(); > > > + return nullptr; > > > + } > > > + > > > + /* Read the image file to data */ > > > + uint8_t buffer[file.size()]; > > > > It's probably best to avoid VLAs. > > > > > > > + Span<unsigned char> data{ buffer, (unsigned > long)file.size() }; > > You can use uint8_t here as well for consistency > > No plain C-style casts. In the version I pulled from fdo you can > declare fileSize a unsigned long and replace > > > > + long dataSize = file.read(data); > > with > > auto buffer = std::make_unique<uint8_t[]>(fileSize); > long dataSize = file.read({ buffer.get(), fileSize }); > > then replace all occurrences of data.data() with buffer.get(). > > Also 'dataSize' and 'fileSize' should have the same value, so I think > you can drop dataSize (or if you want to make sure do > > if (file.read({ buffer.get(), fileSize }) != fileSize) > error out > > Adopted mostly, except that the return type of file.read is also ssize_t. End up using one static_cast. > > > + > > > + /* Get the width and height of the image */ > > > + int width, height; > > unsigned > Hmm, libyuv::MJPGSize takes int* though... > > > > + if (libyuv::MJPGSize(data.data(), dataSize, &width, > &height)) { > > > + LOG(Virtual, Error) << "Failed to get the size of > the image file: " > > > + << file.fileName(); > > > + return nullptr; > > > + } > > > + > > > + /* Convert to NV12 and write the data to tmpY and tmpUV */ > > > + int halfWidth = (width + 1) / 2; > > > + int halfHeight = (height + 1) / 2; > > unsigned > Done > > > > + std::unique_ptr<uint8_t[]> dstY = > > > + std::make_unique<uint8_t[]>(width * height); > > > + std::unique_ptr<uint8_t[]> dstUV = > > > + std::make_unique<uint8_t[]>(halfWidth * halfHeight > * 2); > > > + int ret = libyuv::MJPGToNV12(data.data(), dataSize, > > > + dstY.get(), width, > dstUV.get(), > > > + width, width, height, width, > height); > > > + if (ret != 0) { > > > + LOG(Virtual, Error) << "MJPGToNV12() failed with " > << ret; > > > + } > > no {} > > > > + > > > + imageFrameGenerator->imageFrameDatas_.emplace_back( > > > + ImageFrameData{ std::move(dstY), std::move(dstUV), > > > + Size(width, height) }); > > > + } > > empty line before return > Done > > > > + return imageFrameGenerator; > > > +} > > > + > > > +std::string ImageFrameGenerator::constructPath(std::string &name, > unsigned int &i) > > > > Does this need to be in `ImageFrameGenerator`? Is it not enough to have > it in an > > anonymous namespace in this file? > > In the version I have pulled from gitlab this is now in an anonymous > namespace. The real question I have is what is the point of a one > liner function called from a single place. Drop it and inline it in > the caller please > Done > > > > > > > > +{ > > > + return name + std::to_string(i) + ".jpg"; > > > +} > > > + > > > +void ImageFrameGenerator::configure(const Size &size) > > > +{ > > > + for (unsigned int i = 0; i < imageFrames_->number.value_or(1); > i++) { > > > + /* Scale the imageFrameDatas_ to scaledY and scaledUV */ > > > + int halfSizeWidth = (size.width + 1) / 2; > > > + int halfSizeHeight = (size.height + 1) / 2; > > unsigned > Done > > > > + std::unique_ptr<uint8_t[]> scaledY = > > > + std::make_unique<uint8_t[]>(size.width * > size.height); > > > + std::unique_ptr<uint8_t[]> scaledUV = > > > + std::make_unique<uint8_t[]>(halfSizeWidth * > halfSizeHeight * 2); > > > + auto &src = imageFrameDatas_[i]; > > > + > > > + /* > > > + * \todo Implement "contain" & "cover", based on > > > + * |imageFrames_[i].scaleMode|. > > > + */ > > drop them from the class and documentation if not implemented > Done > > > > + > > > + /* > > > + * \todo Some platforms might enforce stride due to GPU, > like > > > + * ChromeOS ciri (64). The weight needs to be a > multiple of > > > + * the stride to work properly for now. > > Indented with spaces > Using tabs now. > > > > + */ > > > + libyuv::NV12Scale(src.Y.get(), src.size.width, > > > + src.UV.get(), src.size.width, > > > + src.size.width, src.size.height, > > > + scaledY.get(), size.width, > scaledUV.get(), size.width, > > > + size.width, size.height, > libyuv::FilterMode::kFilterBilinear); > > > + > > > + /* Store the pointers to member variable */ > > rather useless comment > Removed > > > > + scaledFrameDatas_.emplace_back( > > > + ImageFrameData{ std::move(scaledY), > std::move(scaledUV), size }); > > > + } > > > +} > > > + > > > +void ImageFrameGenerator::generateFrame(unsigned int &frameCount, > const Size &size, const FrameBuffer *buffer) > > > +{ > > > + /* Don't do anything when the list of buffers is empty*/ > > > + if (scaledFrameDatas_.size() == 0) > > > + return; > > can this happen ? If create() fails, you shouln't be able to call > generateFrames() and configure() doesn't seem to fail anywhere. > Hmm, actually I didn't check if a directory could be empty or not... Added the check in the parser, and made this into an ASSERT instead. > > > > + > > > + MappedFrameBuffer mappedFrameBuffer(buffer, > MappedFrameBuffer::MapFlag::Write); > > > + > > > + auto planes = mappedFrameBuffer.planes(); > > > + > > > + /* Make sure the frameCount does not over the number of images */ > > > + frameCount %= imageFrames_->number.value_or(1); > > am I wrong or if you have a single image the index you want is 0 not 1 ? > If the number is std::nullopt, which means that the path is a single image, we can only divide `frameCount` by 1, instead of 0. (Well, we take the remainder of the division) `frameCount` will always be 0 in this case, which is what we want as the index. > > > + > > > + /* Write the scaledY and scaledUV to the mapped frame buffer */ > > > + libyuv::NV12Copy(scaledFrameDatas_[frameCount].Y.get(), size.width, > > > + scaledFrameDatas_[frameCount].UV.get(), > size.width, planes[0].begin(), > > > + size.width, planes[1].begin(), size.width, > > > + size.width, size.height); > > > + > > > + /* proceed an image every 4 frames */ > > > + /* \todo read the parameter_ from the configuration file? */ > > > + parameter_++; > > this is never initialized > Right, reset it at the beginning of configure(). > > > > + if (parameter_ % 4 == 0) > > > + frameCount++; > > > +} > > > + > > > +} // namespace libcamera > > > diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.h > b/src/libcamera/pipeline/virtual/image_frame_generator.h > > > new file mode 100644 > > > index 000000000..74468e075 > > > --- /dev/null > > > +++ b/src/libcamera/pipeline/virtual/image_frame_generator.h > > > @@ -0,0 +1,65 @@ > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > +/* > > > + * Copyright (C) 2023, Google Inc. > > > + * > > > + * image_frame_generator.h - Derived class of FrameGenerator for > > > + * generating frames from images > > > + */ > > > + > > > +#pragma once > > > + > > > +#include <memory> > > > +#include <optional> > > > +#include <stdint.h> > > > +#include <sys/types.h> > > > + > > > +#include "frame_generator.h" > > > + > > > +namespace libcamera { > > > + > > > +enum class ScaleMode : char { > > > + Fill = 0, > > > + Contain = 1, > > > + Cover = 2, > > > +}; > > > + > > > +/* Frame configuration provided by the config file */ > > > +struct ImageFrames { > > > + std::string path; > > > > I think `std::filesystem::path` would be preferable. > > > > yes! > Done :) > > > > > > + ScaleMode scaleMode; > > > + std::optional<unsigned int> number; > > What's the point of an optional ? The Parser::parseFrame() initializes > this with either: > > data->config_.frame = ImageFrames{ path, scaleMode, std::nullopt }; > or > > data->config_.frame = ImageFrames{ path, scaleMode, > numberOfFilesInDirectory(path) }; > > and you always get it with value_or(1). Why not set it to 1 directly > then ? > > It's checked in `ImageFrameGenerator::create`, to know if it's a file or a directory [1]. [1]: https://chromium-review.git.corp.google.com/c/chromiumos/third_party/libcamera/+/4874478/11/src/libcamera/pipeline/virtual/image_frame_generator.cpp#40 > > > +}; > > > + > > > +class ImageFrameGenerator : public FrameGenerator > > > +{ > > > +public: > > > + /** Factory function to create an ImageFrameGenerator object. > > This is not doxygen. > Use the canonical commenting style used throughout the whole code > base. > Move comments to function to the cpp file. > Done, please check again. > > > > + * Read the images and convert them to buffers in NV12 format. > > > + * Store the pointers to the buffers to a list (imageFrameDatas) > > > + */ > > > + static std::unique_ptr<ImageFrameGenerator> create(ImageFrames > &imageFrames); > > > + > > > +private: > > > + struct ImageFrameData { > > > + std::unique_ptr<uint8_t[]> Y; > > > + std::unique_ptr<uint8_t[]> UV; > > > + Size size; > > > + }; > > > + > > > + /* Scale the buffers for image frames. */ > > > + void configure(const Size &size) override; > > > + void generateFrame(unsigned int &frameCount, const Size &size, > const FrameBuffer *buffer) override; > > You could easily > > void generateFrame(unsigned int &frameCount, const Size &size, > const FrameBuffer *buffer) override; > > From coding-style.rst > > * 2 "Breaking Long Lines" striving to fit code within 80 columns and > accepting up to 120 columns when necessary > > Done > > > + > > > + static std::string constructPath(std::string &name, unsigned int > &i); > > > + > > > + /* List of pointers to the not scaled image buffers */ > > > + std::vector<ImageFrameData> imageFrameDatas_; > > > + /* List of pointers to the scaled image buffers */ > > > + std::vector<ImageFrameData> scaledFrameDatas_; > > > + /* Pointer to the imageFrames_ in VirtualCameraData */ > > > + ImageFrames *imageFrames_; > > > + /* Speed parameter. Change to the next image every parameter_ > frames. */ > > > + int parameter_; > > unsigned > Done > > as long as it's not configurable make it a constexpr > It'll be updated in generateFrame(). > > > > +}; > > > + > > > +} /* namespace libcamera */ > > > diff --git a/src/libcamera/pipeline/virtual/meson.build > b/src/libcamera/pipeline/virtual/meson.build > > > index 2e82e64cb..d56aedec4 100644 > > > --- a/src/libcamera/pipeline/virtual/meson.build > > > +++ b/src/libcamera/pipeline/virtual/meson.build > > > @@ -2,11 +2,14 @@ > > > > > > libcamera_sources += files([ > > > 'virtual.cpp', > > > - 'test_pattern_generator.cpp', > > > 'parser.cpp', > > > + 'test_pattern_generator.cpp', > > > + 'image_frame_generator.cpp', > > > + 'common_functions.cpp', > > > ]) > > > > > > libyuv_dep = dependency('libyuv', required : false) > > > +libjpeg = dependency('libjpeg', required : false) > > where is this required ? > I think it's needed in libyuv. Without it, building will fail with: ``` /usr/bin/ld: subprojects/libyuv/libyuv.a(source_mjpeg_decoder.cc.o): in function `libyuv::MJpegDecoder::MJpegDecoder()': /usr/local/google/home/chenghaoyang/Workspace/libcamera/build/../subprojects/libyuv/source/mjpeg_decoder.cc:79:(.text+0xa9): undefined reference to `jpeg_std_error' /usr/bin/ld: /usr/local/google/home/chenghaoyang/Workspace/libcamera/build/../subprojects/libyuv/source/mjpeg_decoder.cc:88:(.text+0x129): undefined reference to `jpeg_resync_to_restart' /usr/bin/ld: /usr/local/google/home/chenghaoyang/Workspace/libcamera/build/../subprojects/libyuv/source/mjpeg_decoder.cc:90:(.text+0x15a): undefined reference to `jpeg_CreateDecompress' /usr/bin/ld: subprojects/libyuv/libyuv.a(source_mjpeg_decoder.cc.o): in function `libyuv::MJpegDecoder::~MJpegDecoder()': /usr/local/google/home/chenghaoyang/Workspace/libcamera/build/../subprojects/libyuv/source/mjpeg_decoder.cc:97:(.text+0x1a8): undefined reference to `jpeg_destroy_decompress' /usr/bin/ld: subprojects/libyuv/libyuv.a(source_mjpeg_decoder.cc.o): in function `libyuv::MJpegDecoder::LoadFrame(unsigned char const*, unsigned long)': /usr/local/google/home/chenghaoyang/Workspace/libcamera/build/../subprojects/libyuv/source/mjpeg_decoder.cc:122:(.text+0x2b6): undefined reference to `jpeg_read_header' /usr/bin/ld: subprojects/libyuv/libyuv.a(source_mjpeg_decoder.cc.o): in function `libyuv::MJpegDecoder::UnloadFrame()': /usr/local/google/home/chenghaoyang/Workspace/libcamera/build/../subprojects/libyuv/source/mjpeg_decoder.cc:242:(.text+0x7d2): undefined reference to `jpeg_abort_decompress' /usr/bin/ld: subprojects/libyuv/libyuv.a(source_mjpeg_decoder.cc.o): in function `libyuv::MJpegDecoder::StartDecode()': /usr/local/google/home/chenghaoyang/Workspace/libcamera/build/../subprojects/libyuv/source/mjpeg_decoder.cc:528:(.text+0x166b): undefined reference to `jpeg_start_decompress' /usr/bin/ld: subprojects/libyuv/libyuv.a(source_mjpeg_decoder.cc.o): in function `libyuv::MJpegDecoder::FinishDecode()': /usr/local/google/home/chenghaoyang/Workspace/libcamera/build/../subprojects/libyuv/source/mjpeg_decoder.cc:538:(.text+0x169e): undefined reference to `jpeg_abort_decompress' /usr/bin/ld: subprojects/libyuv/libyuv.a(source_mjpeg_decoder.cc.o): in function `libyuv::MJpegDecoder::DecodeImcuRow()': /usr/local/google/home/chenghaoyang/Workspace/libcamera/build/../subprojects/libyuv/source/mjpeg_decoder.cc:554:(.text._ZN6libyuv12MJpegDecoder13DecodeImcuRowEv[_ZN6libyuv12MJpegDecoder13DecodeImcuRowEv]+0x40): undefined reference to `jpeg_read_raw_data' collect2: error: ld returned 1 exit status ``` > > > > > > > # Fallback to a subproject if libyuv isn't found, as it's typically > not > > > # provided by distributions. > > > @@ -26,3 +29,4 @@ if not libyuv_dep.found() > > > endif > > > > > > libcamera_deps += [libyuv_dep] > > > +libcamera_deps += [libjpeg] > > > diff --git a/src/libcamera/pipeline/virtual/parser.cpp > b/src/libcamera/pipeline/virtual/parser.cpp > > > index 032c0cd9d..46d1ab181 100644 > > > --- a/src/libcamera/pipeline/virtual/parser.cpp > > > +++ b/src/libcamera/pipeline/virtual/parser.cpp > > > @@ -18,6 +18,7 @@ > > > #include "libcamera/internal/pipeline_handler.h" > > > #include "libcamera/internal/yaml_parser.h" > > > > > > +#include "common_functions.h" > > drop this > Done > > > > #include "virtual.h" > > > > > > namespace libcamera { > > > @@ -52,12 +53,12 @@ std::vector<std::unique_ptr<VirtualCameraData>> > Parser::parseConfigFile( > > > continue; > > > } > > > > > > - data->id_ = cameraId; > > > + data->config_.id = cameraId; > > > ControlInfoMap::Map controls; > > > /* todo: Check which resolution's frame rate to be > reported */ > > > controls[&controls::FrameDurationLimits] = > > > - ControlInfo(int64_t(1000 / > data->supportedResolutions_[0].frameRates[1]), > > > - int64_t(1000 / > data->supportedResolutions_[0].frameRates[0])); > > > + ControlInfo(int64_t(1000 / > data->config_.resolutions[0].frameRates[1]), > > > + int64_t(1000 / > data->config_.resolutions[0].frameRates[0])); > > > data->controlInfo_ = ControlInfoMap(std::move(controls), > controls::controls); > > > configurations.push_back(std::move(data)); > > > } > > > @@ -72,7 +73,7 @@ std::unique_ptr<VirtualCameraData> > Parser::parseCameraConfigData( > > > if (parseSupportedFormats(cameraConfigData, data.get())) > > > return nullptr; > > > > > > - if (parseTestPattern(cameraConfigData, data.get())) > > > + if (parseFrame(cameraConfigData, data.get())) > > > return nullptr; > > > > > > if (parseLocation(cameraConfigData, data.get())) > > > @@ -122,14 +123,14 @@ int Parser::parseSupportedFormats( > > > frameRates.push_back(60); > > > } > > > > > > - data->supportedResolutions_.emplace_back( > > > + data->config_.resolutions.emplace_back( > > > VirtualCameraData::Resolution{ Size{ > width, height }, > > > frameRates > }); > > > > > > activeResolution = std::max(activeResolution, > Size{ width, height }); > > > } > > > } else { > > > - data->supportedResolutions_.emplace_back( > > > + data->config_.resolutions.emplace_back( > > > VirtualCameraData::Resolution{ Size{ 1920, 1080 }, > > > { 30, 60 } }); > > > activeResolution = Size(1920, 1080); > > > @@ -141,21 +142,65 @@ int Parser::parseSupportedFormats( > > > return 0; > > > } > > > > > > -int Parser::parseTestPattern( > > > +int Parser::parseFrame( > > > const YamlObject &cameraConfigData, VirtualCameraData *data) > > Why ?? > > int Parser::parseFrame(const YamlObject &cameraConfigData, > VirtualCameraData *data) > > is fine or > > int Parser::parseFrame(const YamlObject &cameraConfigData, > VirtualCameraData *data) > > if you really want to > > Done > > > { > > > - std::string testPattern = > cameraConfigData["test_pattern"].get<std::string>().value(); > > > + const YamlObject &frames = cameraConfigData["frames"]; > > empty line please > Done > > > > + /* When there is no frames provided in the config file, use color > bar test pattern */ > > > + if (frames.size() == 0) { > > > + data->config_.frame = TestPattern::ColorBars; > > > + return 0; > > > + } > > > + > > > + if (!frames.isDictionary()) { > > > + LOG(Virtual, Error) << "'frames' is not a dictionary."; > > > + return -EINVAL; > > > + } > > > + > > > + std::string path = frames["path"].get<std::string>().value(); > > > > What if `path` is not specified? Is it checked somewhere else? > > > > I would really make the "path" and "test_pattern" cases separate, > otherwise you're here parsing a string that can be either a system > path (which you have to validate) or "bars"\"lines" that are used for > TPG. > Yeah you're right. Split it into two different arguments. > > > > > > + > > > + if (auto ext = getExtension(path); ext == ".jpg" || ext == > ".jpeg") { > > > + ScaleMode scaleMode; > > declaring a variable inside the if () clause is valid but rather > unusual (at least to me) > Moved it outside. > > > > + if (parseScaleMode(frames, &scaleMode)) > > > + return -EINVAL; > > > + data->config_.frame = ImageFrames{ path, scaleMode, > std::nullopt }; > > > + } else if (path.back() == '/') { > > > > This looks very fragile. Why not use `std::filesystem::symlink_status()` > to check > > if it's a file or a directory. > > > > > > > + ScaleMode scaleMode; > > > + if (parseScaleMode(frames, &scaleMode)) > > > + return -EINVAL; > > > + data->config_.frame = ImageFrames{ path, scaleMode, > > > + > numberOfFilesInDirectory(path) }; > > > > Admittedly I did not check in detail, but is this necessary? Is it > necessary > > to count the number of files in advance? > > > > > > > + } else if (path == "bars" || path == "") { > > > + /* Default value is "bars" */ > > > + data->config_.frame = TestPattern::ColorBars; > > > + } else if (path == "lines") { > > > + data->config_.frame = TestPattern::DiagonalLines; > > > + } else { > > > + LOG(Virtual, Error) << "Frame: " << path > > > + << " is not supported"; > > > + return -EINVAL; > > > + } > > Could you consider reworking the above by changing the schema to > separate TPG from images ? > Done > > Empty line > Done > > > > + return 0; > > > +} > > > > > > - /* Default value is "bars" */ > > > - if (testPattern == "bars" || testPattern == "") { > > > - data->testPattern_ = TestPattern::ColorBars; > > > - } else if (testPattern == "lines") { > > > - data->testPattern_ = TestPattern::DiagonalLines; > > > +int Parser::parseScaleMode( > > > + const YamlObject &framesConfigData, ScaleMode *scaleMode) > > > +{ > > > + std::string mode = > framesConfigData["scale_mode"].get<std::string>().value(); > > > > Same here, regarding empty optionals. > > > > > > > + > > > + /* Default value is fill */ > > > + if (mode == "fill" || mode == "") { > > > + *scaleMode = ScaleMode::Fill; > > > + } else if (mode == "contain") { > > > + *scaleMode = ScaleMode::Contain; > > > + } else if (mode == "cover") { > > > + *scaleMode = ScaleMode::Cover; > > You don't seem to support these in code, why allow them ? Drop them > all and assume the only supported one for now > Done > > > > } else { > > > - LOG(Virtual, Error) << "Test pattern: " << testPattern > > > - << "is not supported"; > > > + LOG(Virtual, Error) << "scaleMode: " << mode > > > + << " is not supported"; > > > return -EINVAL; > > > } > > > + > > > return 0; > > > } > > > > > > @@ -195,4 +240,4 @@ int Parser::parseModel( > > > return 0; > > > } > > > > > > -} /* namespace libcamera */ > > > +} // namespace libcamera > > Why oh why ? > Hmm, not sure why I had this change... Removed. > > > > diff --git a/src/libcamera/pipeline/virtual/parser.h > b/src/libcamera/pipeline/virtual/parser.h > > > index a377d8aa1..38ea460d5 100644 > > > --- a/src/libcamera/pipeline/virtual/parser.h > > > +++ b/src/libcamera/pipeline/virtual/parser.h > > > @@ -34,12 +34,15 @@ private: > > > > > > int parseSupportedFormats( > > > const YamlObject &cameraConfigData, VirtualCameraData > *data); > > > - int parseTestPattern( > > > + int parseFrame( > > > const YamlObject &cameraConfigData, VirtualCameraData > *data); > > > int parseLocation( > > > const YamlObject &cameraConfigData, VirtualCameraData > *data); > > > int parseModel( > > > const YamlObject &cameraConfigData, VirtualCameraData > *data); > > > + > > > + int parseScaleMode( > > > + const YamlObject &framesConfigData, ScaleMode *scaleMode); > > I've gave up trying to understand these indentation rules > Made it one line. > > > > }; > > > > > > } // namespace libcamera > > > diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > > > index 6df9b31e9..844dffd49 100644 > > > --- a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > > > +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > > > @@ -20,7 +20,7 @@ LOG_DECLARE_CATEGORY(Virtual) > > > static const unsigned int kARGBSize = 4; > > > > > > void TestPatternGenerator::generateFrame( > > > - const Size &size, > > > + [[maybe_unused]] unsigned int &frameCount, const Size &size, > > > const FrameBuffer *buffer) > > > { > > > MappedFrameBuffer mappedFrameBuffer(buffer, > > > @@ -29,7 +29,7 @@ void TestPatternGenerator::generateFrame( > > > auto planes = mappedFrameBuffer.planes(); > > > > > > /* TODO: select whether to do shifting or not */ > > > - shiftLeft(size); > > > + // shiftLeft(size); > > Is this related at all ? > Removed. > > > > > > > /* Convert the template_ to the frame buffer */ > > > int ret = libyuv::ARGBToNV12( > > > diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.h > b/src/libcamera/pipeline/virtual/test_pattern_generator.h > > > index ed8d4e43b..cebdd0141 100644 > > > --- a/src/libcamera/pipeline/virtual/test_pattern_generator.h > > > +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.h > > > @@ -25,7 +25,7 @@ enum class TestPattern : char { > > > class TestPatternGenerator : public FrameGenerator > > > { > > > private: > > > - void generateFrame(const Size &size, > > > + void generateFrame(unsigned int &frameCount, const Size &size, > > > const FrameBuffer *buffer) override; > > > > > > protected: > > > diff --git a/src/libcamera/pipeline/virtual/virtual.cpp > b/src/libcamera/pipeline/virtual/virtual.cpp > > > index 0fe471f00..2412af703 100644 > > > --- a/src/libcamera/pipeline/virtual/virtual.cpp > > > +++ b/src/libcamera/pipeline/virtual/virtual.cpp > > > @@ -20,9 +20,13 @@ > > > #include "libcamera/internal/pipeline_handler.h" > > > #include "libcamera/internal/yaml_parser.h" > > > > > > -#include "frame_generator.h" > > > #include "parser.h" > > > > > > +#define ifTestPattern(v) std::holds_alternative<TestPattern>(v) > > > +#define getTestPattern(v) std::get<TestPattern>(v) > > > +#define ifImageFrames(v) std::holds_alternative<ImageFrames>(v) > > > +#define getImageFrames(v) std::get<ImageFrames>(v) > > > + > > > namespace libcamera { > > > > > > LOG_DEFINE_CATEGORY(Virtual) > > > @@ -63,12 +67,12 @@ CameraConfiguration::Status > VirtualCameraConfiguration::validate() > > > } > > > > > > Size maxSize; > > > - for (const auto &resolution : data_->supportedResolutions_) > > > + for (const auto &resolution : data_->config_.resolutions) > > > maxSize = std::max(maxSize, resolution.size); > > > > > > for (StreamConfiguration &cfg : config_) { > > > bool found = false; > > > - for (const auto &resolution : > data_->supportedResolutions_) { > > > + for (const auto &resolution : data_->config_.resolutions) { > > > if (resolution.size.width == cfg.size.width && > > > resolution.size.height == cfg.size.height) { > > > found = true; > > > @@ -110,7 +114,7 @@ > PipelineHandlerVirtual::generateConfiguration(Camera *camera, > > > return config; > > > > > > Size minSize, sensorResolution; > > > - for (const auto &resolution : data->supportedResolutions_) { > > > + for (const auto &resolution : data->config_.resolutions) { > > > if (minSize.isNull() || minSize > resolution.size) > > > minSize = resolution.size; > > > > > > @@ -199,7 +203,7 @@ int PipelineHandlerVirtual::exportFrameBuffers( > > > int PipelineHandlerVirtual::start(Camera *camera, > > > [[maybe_unused]] const ControlList > *controls) > > > { > > > - /* \todo Start reading the virtual video if any. */ > > > + /* Start reading the images/generating test patterns */ > > > VirtualCameraData *data = cameraData(camera); > > > > > > > data->frameGenerator_->configure(data->stream_.configuration().size); > > > @@ -219,8 +223,8 @@ int > PipelineHandlerVirtual::queueRequestDevice([[maybe_unused]] Camera *camera, > > > > > > /* \todo Read from the virtual video if any. */ > > > for (auto const &[stream, buffer] : request->buffers()) { > > > - /* map buffer and fill test patterns */ > > > - > data->frameGenerator_->generateFrame(stream->configuration().size, buffer); > > > + /* Map buffer. Fill test patterns or images */ > > > + data->frameGenerator_->generateFrame(data->frameCount_, > stream->configuration().size, buffer); > > I don't see frameCount_ be used anywhere here, but I suspect you want > it be modified by generateFrame() (answers my first question of the > email). I don't know what you intend to use > it for, but if, as the name suggest you want to count frames, you can > do it here without the frameGenerator being involved ? It's > one-request-one-frame after all, isn't it ? So you don't have to > pollute TestPatternGenerator::generateFrame with a > [[maybe_unused]] unsigned int &frameCount, > Yeah right. Migrated to ImageFrameGenerator. I think the original idea is it allows the pipeline handler to control which frame to use in each request, but let's delay the feature when we actually implement it. > > > > > completeBuffer(request, buffer); > > > } > > > > > > @@ -250,9 +254,10 @@ bool > PipelineHandlerVirtual::match([[maybe_unused]] DeviceEnumerator *enumerator > > > /* Configure and register cameras with configData */ > > > for (auto &data : configData) { > > > std::set<Stream *> streams{ &data->stream_ }; > > > - std::string id = data->id_; > > > + std::string id = data->config_.id; > > > std::shared_ptr<Camera> camera = > Camera::create(std::move(data), id, streams); > > > > > > + /* Initialize FrameGenerator*/ > > > initFrameGenerator(camera.get()); > > > > > > registerCamera(std::move(camera)); > > > @@ -264,13 +269,19 @@ bool > PipelineHandlerVirtual::match([[maybe_unused]] DeviceEnumerator *enumerator > > > void PipelineHandlerVirtual::initFrameGenerator(Camera *camera) > > > { > > > auto data = cameraData(camera); > > > - if (data->testPattern_ == TestPattern::DiagonalLines) { > > > - data->frameGenerator_ = DiagonalLinesGenerator::create(); > > > - } else { > > > - data->frameGenerator_ = ColorBarsGenerator::create(); > > > + auto &frame = data->config_.frame; > > > + if (ifTestPattern(frame)) { > > > + TestPattern &testPattern = getTestPattern(frame); > > > + if (testPattern == TestPattern::DiagonalLines) { > > > + data->frameGenerator_ = > DiagonalLinesGenerator::create(); > > > + } else { > > > + data->frameGenerator_ = > ColorBarsGenerator::create(); > > > + } > > > + } else if (ifImageFrames(frame)) { > > > + data->frameGenerator_ = > ImageFrameGenerator::create(getImageFrames(frame)); > > > } > > > > I think I would strongly prefer `std::visit()` here. But even > `std::get_if()` is better in my opinion. > > Regarding the revistied version on fdo > > if (testPattern == > TestPattern::DiagonalLines) { > data->frameGenerator_ = > DiagonalLinesGenerator::create(); > } else { > data->frameGenerator_ = > ColorBarsGenerator::create(); > } > > again, no {} for single line statements > Removed {}. > > > > > > > > } > > > > > > REGISTER_PIPELINE_HANDLER(PipelineHandlerVirtual, "virtual") > > > > > > -} /* namespace libcamera */ > > > +} // namespace libcamera > > meh > Removed. > > > > diff --git a/src/libcamera/pipeline/virtual/virtual.h > b/src/libcamera/pipeline/virtual/virtual.h > > > index c1ac4eb90..f41a4a906 100644 > > > --- a/src/libcamera/pipeline/virtual/virtual.h > > > +++ b/src/libcamera/pipeline/virtual/virtual.h > > > @@ -7,16 +7,22 @@ > > > > > > #pragma once > > > > > > +#include <variant> > > > + > > > #include <libcamera/base/file.h> > > > > > > #include "libcamera/internal/camera.h" > > > #include "libcamera/internal/dma_buf_allocator.h" > > > #include "libcamera/internal/pipeline_handler.h" > > > > > > +#include "frame_generator.h" > > > +#include "image_frame_generator.h" > > > #include "test_pattern_generator.h" > > > > > > namespace libcamera { > > > > > > +using VirtualFrame = std::variant<TestPattern, ImageFrames>; > > > + > > > class VirtualCameraData : public Camera::Private > > > { > > > public: > > > @@ -24,6 +30,13 @@ public: > > > Size size; > > > std::vector<int> frameRates; > > > }; > > > + /* The config file is parsed to the Configuration struct */ > > > + struct Configuration { > > > + std::string id; > > so you need <string> > Done > > > > + std::vector<Resolution> resolutions; > > and <vector> > Done > > > > + VirtualFrame frame; > > > + }; > > > + > > > VirtualCameraData(PipelineHandler *pipe) > > > : Camera::Private(pipe) > > > { > > > @@ -31,12 +44,9 @@ public: > > > > > > ~VirtualCameraData() = default; > > > > > > - std::string id_; > > > - std::vector<Resolution> supportedResolutions_; > > well, you did already > Sorry, I did what...? > > > > - TestPattern testPattern_; > > > - > > > + unsigned int frameCount_ = 0; > > > + Configuration config_; > > > Stream stream_; > > > - > > > std::unique_ptr<FrameGenerator> frameGenerator_; > > > }; > > > > > > -- > > > 2.46.0.184.g6999bdac58-goog > > > > > > > > > Regards, > > Barnabás Pőcze > BR, Harvey
Hi Jacopo, On Sat, Aug 31, 2024 at 9:00 PM Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > Hi Harvey, > > On Thu, Aug 29, 2024 at 09:58:04PM GMT, Cheng-Hao Yang wrote: > > Hi Jacopo, > > > > On Wed, Aug 28, 2024 at 10:14 AM Jacopo Mondi < > jacopo.mondi@ideasonboard.com> > > wrote: > > > > > Hi > > > > > > On Wed, Aug 21, 2024 at 02:05:57AM GMT, Barnabás Pőcze wrote: > > > > Hi > > > > > > > > > > > > 2024. augusztus 20., kedd 18:23 keltezéssel, Harvey Yang < > > > chenghaoyang@chromium.org> írta: > > > > > > > > > From: Konami Shu <konamiz@google.com> > > > > > > > > > > - Refactor ImageFrameGenerator and TestPatternGenerator > > > > > - Extend the config file to have section to choose to use images or > > > test > > > > > pattern > > > > > - Extend Parser to parse the configuration > > > > > - Add ImageFrameGenerator which uses images to provide frames > > > > > > Please make a proper commit message out of this > > > > > Updated. > > > > > > > > > > > > > > > > > Patchset1->2 > > > > > - Extend the parser to accept files with ".jpeg" > > > > > - Heap allocate some buffers in ImageFrameGenerator::generateFrame > so > > > > > the buffers won't cause stack over flow > > > > > > > > > > Patchset5->6 > > > > > - Move static factory function from the interface class to the > derived > > > > > classes > > > > > > > > > > Patchset6->8 > > > > > - create FrameGenerator before starting the camera > > > > > > Changelogs should go below > > > > > Let's put them in the cover letter, if it's even needed. > > > > > > > > > > --- > > > > > > So that they don't appear in git history > > > > > > > > > > > > > Signed-off-by: Konami Shu <konamiz@google.com> > > > > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org> > > > > > Co-developed-by: Yunke Cao <yunkec@chromium.org> > > > > > Co-developed-by: Tomasz Figa <tfiga@chromium.org> > > > > > --- > > > > > src/libcamera/pipeline/virtual/README.md | 18 +- > > > > > .../pipeline/virtual/common_functions.cpp | 27 +++ > > > > > .../pipeline/virtual/common_functions.h | 18 ++ > > > > > .../pipeline/virtual/frame_generator.h | 2 +- > > > > > .../virtual/image_frame_generator.cpp | 154 > ++++++++++++++++++ > > > > > .../pipeline/virtual/image_frame_generator.h | 65 ++++++++ > > > > > src/libcamera/pipeline/virtual/meson.build | 6 +- > > > > > src/libcamera/pipeline/virtual/parser.cpp | 77 +++++++-- > > > > > src/libcamera/pipeline/virtual/parser.h | 5 +- > > > > > .../virtual/test_pattern_generator.cpp | 4 +- > > > > > .../pipeline/virtual/test_pattern_generator.h | 2 +- > > > > > src/libcamera/pipeline/virtual/virtual.cpp | 37 +++-- > > > > > src/libcamera/pipeline/virtual/virtual.h | 20 ++- > > > > > 13 files changed, 390 insertions(+), 45 deletions(-) > > > > > create mode 100644 > src/libcamera/pipeline/virtual/common_functions.cpp > > > > > create mode 100644 > src/libcamera/pipeline/virtual/common_functions.h > > > > > create mode 100644 > > > src/libcamera/pipeline/virtual/image_frame_generator.cpp > > > > > create mode 100644 > > > src/libcamera/pipeline/virtual/image_frame_generator.h > > > > > > > > > > diff --git a/src/libcamera/pipeline/virtual/README.md > > > b/src/libcamera/pipeline/virtual/README.md > > > > > index 27d6283df..5e21ce74a 100644 > > > > > --- a/src/libcamera/pipeline/virtual/README.md > > > > > +++ b/src/libcamera/pipeline/virtual/README.md > > > > > @@ -15,8 +15,13 @@ Each camera block is a dictionary, containing > the > > > following keys: > > > > > - `supported_formats` (list of `VirtualCameraData::Resolution`, > > > optional) : List of supported resolution and frame rates of the > emulated > > > camera > > > > > - `width` (`unsigned int`, default=1920): Width of the window > > > resolution. This needs to be even. > > > > > - `height` (`unsigned int`, default=1080): Height of the > window > > > resolution. > > > > > - - `frame_rates` (list of `int`, default=`[30,60]` ): Range of > the > > > frame rate. The list has to be two values of the lower bound and the > upper > > > bound of the frame rate. > > > > > -- `test_pattern` (`string`, default="bars"): Which test pattern to > > > use as frames. The options are "bars", "lines". > > > > > + - `frame_rates` (list of `int`, default=`[30,60]` ): Range of > the > > > frame rate. The list has to be two values of the lower bound and the > upper > > > bound of the frame rate. This does not affect the frame rate for now. > > > > > +- `frames` (dictionary): > > > > > + - `path` (`string`, default="bars"): Name of a test pattern, > path > > > to an image, or path to a directory of a series of images. > > > > > + - The test patterns are "bars" which means color bars, and > > > "lines" which means diagonal lines. > > > > > + - The path to an image has ".jpg" extension. > > > > > + - 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. > > > > > + - `scale_mode`(`string`, default="fill"): Scale mode when the > > > frames are images. The scale modes are "fill", "contain", and "cover". > This > > > does not matter when frames is a test pattern. This does not affect the > > > scale mode for now. > > > > > - `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. > > > > > - `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. > > > > > > > > > > @@ -35,13 +40,16 @@ A sample config file: > > > > > frame_rates: > > > > > - 70 > > > > > - 80 > > > > > - test_pattern: "bars" > > > > > + frames: > > > > > + path: "lines" > > > > > location: "front" > > > > > model: "Virtual Video Device" > > > > > "Virtual1": > > > > > supported_formats: > > > > > - width: 800 > > > > > - test_pattern: "lines" > > > > > + frames: > > > > > + path: "path/to/directory_of_images/" > > > > > + scale_mode: "contain" > > > > > location: "back" > > > > > model: "Virtual Video Device1" > > > > > "Virtual2": > > > > > @@ -61,7 +69,7 @@ This is the procedure of the Parser class: > > > > > - If the config file contains invalid configuration, this > method > > > returns nullptr. The camera will be skipped. > > > > > 3. Parse each property and register the data. > > > > > - `parseSupportedFormats()`: Parses `supported_formats` in the > > > config, which contains resolutions and frame rates. > > > > > - - `parseTestPattern()`: Parses `test_pattern` in the config. > > > > > + - `parseFrame()`: Parses `frames` in the config. > > > > > - `parseLocation()`: Parses `location` in the config. > > > > > - `parseModel()`: Parses `model` in the config. > > > > > 4. Back to `parseConfigFile()` and append the camera > configuration. > > > > > diff --git a/src/libcamera/pipeline/virtual/common_functions.cpp > > > b/src/libcamera/pipeline/virtual/common_functions.cpp > > > > > new file mode 100644 > > > > > index 000000000..207827ee0 > > > > > --- /dev/null > > > > > +++ b/src/libcamera/pipeline/virtual/common_functions.cpp > > > > > @@ -0,0 +1,27 @@ > > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > > > +/* > > > > > + * Copyright (C) 2023, Google Inc. > > > > > + * > > > > > + * common_functions.cpp - Helper that do not depend on any class > > > > > + */ > > > > > + > > > > > +#include "common_functions.h" > > > > > + > > > > > +namespace libcamera { > > > > > + > > > > > +std::string getExtension(const std::string &path) > > > > > > > > This looks very fragile. Why not use > > > `std::filesystem::path::extension()`? > > > > > > > > > > > > > +{ > > > > > + size_t i = path.find("."); > > > > > + if (i != std::string::npos) { > > > > > + return path.substr(i); > > > > > + } > > > > > + return ""; > > > > > +} > > > > > + > > > > > +std::size_t numberOfFilesInDirectory(std::filesystem::path path) > > > > > +{ > > > > > + using std::filesystem::directory_iterator; > > > > > + return std::distance(directory_iterator(path), > > > directory_iterator{}); > > > > > +} > > > > > > And inline this function so that you can drop common_functions.cpp/h > > > completely > > > > > Done > > > > > > > > > > > > + > > > > > +} // namespace libcamera > > > > > diff --git a/src/libcamera/pipeline/virtual/common_functions.h > > > b/src/libcamera/pipeline/virtual/common_functions.h > > > > > new file mode 100644 > > > > > index 000000000..4203f9505 > > > > > --- /dev/null > > > > > +++ b/src/libcamera/pipeline/virtual/common_functions.h > > > > > @@ -0,0 +1,18 @@ > > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > > > +/* > > > > > + * Copyright (C) 2023, Google Inc. > > > > > + * > > > > > + * common_functions.h - Helper that do not depend on any class > > > > > + */ > > > > > + > > > > > +#pragma once > > > > > + > > > > > +#include <filesystem> > > > > > + > > > > > +namespace libcamera { > > > > > + > > > > > +std::string getExtension(const std::string &path); > > > > > + > > > > > +std::size_t numberOfFilesInDirectory(std::filesystem::path path); > > > > > + > > > > > +} // namespace libcamera > > > > > diff --git a/src/libcamera/pipeline/virtual/frame_generator.h > > > b/src/libcamera/pipeline/virtual/frame_generator.h > > > > > index 9699af7a4..f69576b36 100644 > > > > > --- a/src/libcamera/pipeline/virtual/frame_generator.h > > > > > +++ b/src/libcamera/pipeline/virtual/frame_generator.h > > > > > @@ -23,7 +23,7 @@ public: > > > > > /** Fill the output frame buffer. > > > > > * Use the frame at the frameCount of image frames > > > > > */ > > > > > - virtual void generateFrame(const Size &size, > > > > > + virtual void generateFrame(unsigned int &frameCount, const Size > > > &size, > > > > > > Are you passing frameCount by reference because you want the function > > > to modify the value passed in by caller ? Use a pointer then which > > > better conveys this semantic in my opinion. > > > > > Done. Thanks! > > > > > > > > > > > > const FrameBuffer *buffer) = 0; > > > > > > > > > > protected: > > > > > diff --git > a/src/libcamera/pipeline/virtual/image_frame_generator.cpp > > > b/src/libcamera/pipeline/virtual/image_frame_generator.cpp > > > > > new file mode 100644 > > > > > index 000000000..d374877ff > > > > > --- /dev/null > > > > > +++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp > > > > > @@ -0,0 +1,154 @@ > > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > > > +/* > > > > > + * Copyright (C) 2023, Google Inc. > > > > > + * > > > > > + * image_frame_generator.cpp - Derived class of FrameGenerator for > > > > > + * generating frames from images > > > > > + */ > > > > > + > > > > > +#include "image_frame_generator.h" > > > > > + > > > > > +#include <memory> > > > > > +#include <optional> > > > > > > both already included by image_frame_generator.h > > > > > Removed. I added memory again as the source file uses > > std::make_unique on its own as well, but anyways :) > > > > > > > > > > > > +#include <string> > > > > > + > > > > > +#include <libcamera/base/file.h> > > > > > +#include <libcamera/base/log.h> > > > > > + > > > > > +#include <libcamera/framebuffer.h> > > > > > + > > > > > +#include "libcamera/internal/mapped_framebuffer.h" > > > > > + > > > > > +#include "libyuv/convert.h" > > > > > +#include "libyuv/scale.h" > > > > > > empty line > > > > > Done > > > > > > > > > > > > +namespace libcamera { > > > > > + > > > > > +LOG_DECLARE_CATEGORY(Virtual) > > > > > + > > > > > +std::unique_ptr<ImageFrameGenerator> ImageFrameGenerator::create( > > > > > + ImageFrames &imageFrames) > > > > > > Or rather > > > > > > std::unique_ptr<ImageFrameGenerator> > > > ImageFrameGenerator::create(ImageFrames &imageFrames) > > > { > > > > > Done > > > > > > > > > > > > +{ > > > > > + std::unique_ptr<ImageFrameGenerator> imageFrameGenerator = > > > > > + std::make_unique<ImageFrameGenerator>(); > > > > > + imageFrameGenerator->imageFrames_ = &imageFrames; > > > > > + > > > > > + /** For each file in the directory > > > > > > This is not doxygen. Drop /** and use the conventional commenting > > > style > > > /* > > > * This is for multi > > > * line comments > > > */ > > > > > > Done. Thanks! > > > > > > > > > + * load the image, convert it to NV12, and store the pointer > > > > > + */ > > > > > + for (unsigned int i = 0; i < imageFrames.number.value_or(1); > i++) { > > > > > + std::string path; > > > > > + if (!imageFrames.number.has_value()) { > > > > > > I think you can drop has_value() as it's an alias for operator bool > > > > > Done > > > > > > > > > > > > + /* If the path is to an image */ > > > > > + path = imageFrames.path; > > > > > + } else { > > > > > + /* If the path is to a directory */ > > > > > + path = constructPath(imageFrames.path, i); > > > > > > This assumes that if you're given a directory images are named > > > 0.jpeg 1.jpeg etc. Up to you, but it would be trivial to enumerate all > > > .jpeg files in a directory. True, you won't know in which order to > > > display them then... > > > > > Yes, the original concern is exactly about the order... Do you think we > > should use alphabetic order instead? > > No no, as long as the assumption is documented, as it is, it's fine > > > > > > > > > > > > > + } > > > > > > No {} for single liners, please > > > > > Hmm, even with comments that make them more than one line? > > generally no, it's not necessary. > Got it. Will be updated in v11. > > > > > > > > > > > > > + > > > > > + File file(path); > > > > > + bool isOpen = file.open(File::OpenModeFlag::ReadOnly); > > > > > + if (!isOpen) { > > > > > > > > I think `if (!file.open(...))` is fine. > > > > > > > > > > > > > + LOG(Virtual, Error) << "Failed to open image > file: > > > " << file.fileName(); > > > > > + return nullptr; > > > > > + } > > > > > + > > > > > + /* Read the image file to data */ > > > > > + uint8_t buffer[file.size()]; > > > > > > > > It's probably best to avoid VLAs. > > > > > > > > > > > > > + Span<unsigned char> data{ buffer, (unsigned > > > long)file.size() }; > > > > > > You can use uint8_t here as well for consistency > > > > > > No plain C-style casts. In the version I pulled from fdo you can > > > declare fileSize a unsigned long and replace > > > > > > > > + long dataSize = file.read(data); > > > > > > with > > > > > > auto buffer = std::make_unique<uint8_t[]>(fileSize); > > > long dataSize = file.read({ buffer.get(), fileSize }); > > > > > > then replace all occurrences of data.data() with buffer.get(). > > > > > > Also 'dataSize' and 'fileSize' should have the same value, so I think > > > you can drop dataSize (or if you want to make sure do > > > > > > if (file.read({ buffer.get(), fileSize }) != fileSize) > > > error out > > > > > > Adopted mostly, except that the return type of file.read is also > ssize_t. > > End up using one static_cast. > > > > > > > > > + > > > > > + /* Get the width and height of the image */ > > > > > + int width, height; > > > > > > unsigned > > > > > Hmm, libyuv::MJPGSize takes int* though... > > > > Ack > > > > > > > > > + if (libyuv::MJPGSize(data.data(), dataSize, &width, > > > &height)) { > > > > > + LOG(Virtual, Error) << "Failed to get the size > of > > > the image file: " > > > > > + << file.fileName(); > > > > > + return nullptr; > > > > > + } > > > > > + > > > > > + /* Convert to NV12 and write the data to tmpY and > tmpUV */ > > > > > + int halfWidth = (width + 1) / 2; > > > > > + int halfHeight = (height + 1) / 2; > > > > > > unsigned > > > > > Done > > > > > > > > > > > > + std::unique_ptr<uint8_t[]> dstY = > > > > > + std::make_unique<uint8_t[]>(width * height); > > > > > + std::unique_ptr<uint8_t[]> dstUV = > > > > > + std::make_unique<uint8_t[]>(halfWidth * > halfHeight > > > * 2); > > > > > + int ret = libyuv::MJPGToNV12(data.data(), dataSize, > > > > > + dstY.get(), width, > > > dstUV.get(), > > > > > + width, width, height, > width, > > > height); > > > > > + if (ret != 0) { > > > > > + LOG(Virtual, Error) << "MJPGToNV12() failed > with " > > > << ret; > > > > > + } > > > > > > no {} > > > > > > > > + > > > > > + imageFrameGenerator->imageFrameDatas_.emplace_back( > > > > > + ImageFrameData{ std::move(dstY), > std::move(dstUV), > > > > > + Size(width, height) }); > > > > > + } > > > > > > empty line before return > > > > > Done > > > > > > > > > > > > + return imageFrameGenerator; > > > > > +} > > > > > + > > > > > +std::string ImageFrameGenerator::constructPath(std::string &name, > > > unsigned int &i) > > > > > > > > Does this need to be in `ImageFrameGenerator`? Is it not enough to > have > > > it in an > > > > anonymous namespace in this file? > > > > > > In the version I have pulled from gitlab this is now in an anonymous > > > namespace. The real question I have is what is the point of a one > > > liner function called from a single place. Drop it and inline it in > > > the caller please > > > > > Done > > > > > > > > > > > > > > > > > > > > +{ > > > > > + return name + std::to_string(i) + ".jpg"; > > > > > +} > > > > > + > > > > > +void ImageFrameGenerator::configure(const Size &size) > > > > > +{ > > > > > + for (unsigned int i = 0; i < imageFrames_->number.value_or(1); > > > i++) { > > > > > + /* Scale the imageFrameDatas_ to scaledY and scaledUV > */ > > > > > + int halfSizeWidth = (size.width + 1) / 2; > > > > > + int halfSizeHeight = (size.height + 1) / 2; > > > > > > unsigned > > > > > Done > > > > > > > > > > > > + std::unique_ptr<uint8_t[]> scaledY = > > > > > + std::make_unique<uint8_t[]>(size.width * > > > size.height); > > > > > + std::unique_ptr<uint8_t[]> scaledUV = > > > > > + std::make_unique<uint8_t[]>(halfSizeWidth * > > > halfSizeHeight * 2); > > > > > + auto &src = imageFrameDatas_[i]; > > > > > + > > > > > + /* > > > > > + * \todo Implement "contain" & "cover", based on > > > > > + * |imageFrames_[i].scaleMode|. > > > > > + */ > > > > > > drop them from the class and documentation if not implemented > > > > > Done > > > > > > > > > > > > + > > > > > + /* > > > > > + * \todo Some platforms might enforce stride due to > GPU, > > > like > > > > > + * ChromeOS ciri (64). The weight needs to be a > > > multiple of > > > > > + * the stride to work properly for now. > > > > > > Indented with spaces > > > > > Using tabs now. > > > > > > > > > > > > + */ > > > > > + libyuv::NV12Scale(src.Y.get(), src.size.width, > > > > > + src.UV.get(), src.size.width, > > > > > + src.size.width, src.size.height, > > > > > + scaledY.get(), size.width, > > > scaledUV.get(), size.width, > > > > > + size.width, size.height, > > > libyuv::FilterMode::kFilterBilinear); > > > > > + > > > > > + /* Store the pointers to member variable */ > > > > > > rather useless comment > > > > > Removed > > > > > > > > > > > > + scaledFrameDatas_.emplace_back( > > > > > + ImageFrameData{ std::move(scaledY), > > > std::move(scaledUV), size }); > > > > > + } > > > > > +} > > > > > + > > > > > +void ImageFrameGenerator::generateFrame(unsigned int &frameCount, > > > const Size &size, const FrameBuffer *buffer) > > > > > +{ > > > > > + /* Don't do anything when the list of buffers is empty*/ > > > > > + if (scaledFrameDatas_.size() == 0) > > > > > + return; > > > > > > can this happen ? If create() fails, you shouln't be able to call > > > generateFrames() and configure() doesn't seem to fail anywhere. > > > > > Hmm, actually I didn't check if a directory could be empty or not... > > Added the check in the parser, and made this into an ASSERT instead. > > > > > > > > > > > > + > > > > > + MappedFrameBuffer mappedFrameBuffer(buffer, > > > MappedFrameBuffer::MapFlag::Write); > > > > > + > > > > > + auto planes = mappedFrameBuffer.planes(); > > > > > + > > > > > + /* Make sure the frameCount does not over the number of images > */ > > > > > + frameCount %= imageFrames_->number.value_or(1); > > > > > > am I wrong or if you have a single image the index you want is 0 not 1 > ? > > > > > If the number is std::nullopt, which means that the path is a single > image, > > we can only divide `frameCount` by 1, instead of 0. (Well, we take the > > remainder of the division) > > `frameCount` will always be 0 in this case, which is what we want as the > > index. > > Sure. Dividing by 0 would not be a good idea :) > > > > > > > > > > + > > > > > + /* Write the scaledY and scaledUV to the mapped frame buffer */ > > > > > + libyuv::NV12Copy(scaledFrameDatas_[frameCount].Y.get(), > size.width, > > > > > + scaledFrameDatas_[frameCount].UV.get(), > > > size.width, planes[0].begin(), > > > > > + size.width, planes[1].begin(), size.width, > > > > > + size.width, size.height); > > > > > + > > > > > + /* proceed an image every 4 frames */ > > > > > + /* \todo read the parameter_ from the configuration file? */ > > > > > + parameter_++; > > > > > > this is never initialized > > > > > Right, reset it at the beginning of configure(). > > > > > > > > > > > > + if (parameter_ % 4 == 0) > > > > > + frameCount++; > > > > > +} > > > > > + > > > > > +} // namespace libcamera > > > > > diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.h > > > b/src/libcamera/pipeline/virtual/image_frame_generator.h > > > > > new file mode 100644 > > > > > index 000000000..74468e075 > > > > > --- /dev/null > > > > > +++ b/src/libcamera/pipeline/virtual/image_frame_generator.h > > > > > @@ -0,0 +1,65 @@ > > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > > > +/* > > > > > + * Copyright (C) 2023, Google Inc. > > > > > + * > > > > > + * image_frame_generator.h - Derived class of FrameGenerator for > > > > > + * generating frames from images > > > > > + */ > > > > > + > > > > > +#pragma once > > > > > + > > > > > +#include <memory> > > > > > +#include <optional> > > > > > +#include <stdint.h> > > > > > +#include <sys/types.h> > > > > > + > > > > > +#include "frame_generator.h" > > > > > + > > > > > +namespace libcamera { > > > > > + > > > > > +enum class ScaleMode : char { > > > > > + Fill = 0, > > > > > + Contain = 1, > > > > > + Cover = 2, > > > > > +}; > > > > > + > > > > > +/* Frame configuration provided by the config file */ > > > > > +struct ImageFrames { > > > > > + std::string path; > > > > > > > > I think `std::filesystem::path` would be preferable. > > > > > > > > > > yes! > > > > > Done :) > > > > > > > > > > > > > > > > + ScaleMode scaleMode; > > > > > + std::optional<unsigned int> number; > > > > > > What's the point of an optional ? The Parser::parseFrame() initializes > > > this with either: > > > > > > data->config_.frame = ImageFrames{ path, scaleMode, std::nullopt }; > > > or > > > > > > data->config_.frame = ImageFrames{ path, scaleMode, > > > numberOfFilesInDirectory(path) }; > > > > > > and you always get it with value_or(1). Why not set it to 1 directly > > > then ? > > > > > > It's checked in `ImageFrameGenerator::create`, to know if it's a file > > or a directory [1]. > > > > [1]: > > > https://chromium-review.git.corp.google.com/c/chromiumos/third_party/libcamera/+/4874478/11/src/libcamera/pipeline/virtual/image_frame_generator.cpp#40 > > > > > > > > > +}; > > > > > + > > > > > +class ImageFrameGenerator : public FrameGenerator > > > > > +{ > > > > > +public: > > > > > + /** Factory function to create an ImageFrameGenerator object. > > > > > > This is not doxygen. > > > Use the canonical commenting style used throughout the whole code > > > base. > > > Move comments to function to the cpp file. > > > > > Done, please check again. > > > > > > > > > > > > + * Read the images and convert them to buffers in NV12 format. > > > > > + * Store the pointers to the buffers to a list > (imageFrameDatas) > > > > > + */ > > > > > + static std::unique_ptr<ImageFrameGenerator> create(ImageFrames > > > &imageFrames); > > > > > + > > > > > +private: > > > > > + struct ImageFrameData { > > > > > + std::unique_ptr<uint8_t[]> Y; > > > > > + std::unique_ptr<uint8_t[]> UV; > > > > > + Size size; > > > > > + }; > > > > > + > > > > > + /* Scale the buffers for image frames. */ > > > > > + void configure(const Size &size) override; > > > > > + void generateFrame(unsigned int &frameCount, const Size &size, > > > const FrameBuffer *buffer) override; > > > > > > You could easily > > > > > > void generateFrame(unsigned int &frameCount, const Size &size, > > > const FrameBuffer *buffer) override; > > > > > > From coding-style.rst > > > > > > * 2 "Breaking Long Lines" striving to fit code within 80 columns and > > > accepting up to 120 columns when necessary > > > > > > Done > > > > > > > > > + > > > > > + static std::string constructPath(std::string &name, unsigned > int > > > &i); > > > > > + > > > > > + /* List of pointers to the not scaled image buffers */ > > > > > + std::vector<ImageFrameData> imageFrameDatas_; > > > > > + /* List of pointers to the scaled image buffers */ > > > > > + std::vector<ImageFrameData> scaledFrameDatas_; > > > > > + /* Pointer to the imageFrames_ in VirtualCameraData */ > > > > > + ImageFrames *imageFrames_; > > > > > + /* Speed parameter. Change to the next image every parameter_ > > > frames. */ > > > > > + int parameter_; > > > > > > unsigned > > > > > Done > > > > > > > > > > as long as it's not configurable make it a constexpr > > > > > It'll be updated in generateFrame(). > > > > > > > > > > > > +}; > > > > > + > > > > > +} /* namespace libcamera */ > > > > > diff --git a/src/libcamera/pipeline/virtual/meson.build > > > b/src/libcamera/pipeline/virtual/meson.build > > > > > index 2e82e64cb..d56aedec4 100644 > > > > > --- a/src/libcamera/pipeline/virtual/meson.build > > > > > +++ b/src/libcamera/pipeline/virtual/meson.build > > > > > @@ -2,11 +2,14 @@ > > > > > > > > > > libcamera_sources += files([ > > > > > 'virtual.cpp', > > > > > - 'test_pattern_generator.cpp', > > > > > 'parser.cpp', > > > > > + 'test_pattern_generator.cpp', > > > > > + 'image_frame_generator.cpp', > > > > > + 'common_functions.cpp', > > > > > ]) > > > > > > > > > > libyuv_dep = dependency('libyuv', required : false) > > > > > +libjpeg = dependency('libjpeg', required : false) > > > > > > where is this required ? > > > > > I think it's needed in libyuv. Without it, building will fail with: > > Ah yes, libyuv is compiled with -ljpeg. > > > ``` > > > > /usr/bin/ld: subprojects/libyuv/libyuv.a(source_mjpeg_decoder.cc.o): in > > function `libyuv::MJpegDecoder::MJpegDecoder()': > > > > > /usr/local/google/home/chenghaoyang/Workspace/libcamera/build/../subprojects/libyuv/source/mjpeg_decoder.cc:79:(.text+0xa9): > > undefined reference to `jpeg_std_error' > > > > /usr/bin/ld: > > > /usr/local/google/home/chenghaoyang/Workspace/libcamera/build/../subprojects/libyuv/source/mjpeg_decoder.cc:88:(.text+0x129): > > undefined reference to `jpeg_resync_to_restart' > > > > /usr/bin/ld: > > > /usr/local/google/home/chenghaoyang/Workspace/libcamera/build/../subprojects/libyuv/source/mjpeg_decoder.cc:90:(.text+0x15a): > > undefined reference to `jpeg_CreateDecompress' > > > > /usr/bin/ld: subprojects/libyuv/libyuv.a(source_mjpeg_decoder.cc.o): in > > function `libyuv::MJpegDecoder::~MJpegDecoder()': > > > > > /usr/local/google/home/chenghaoyang/Workspace/libcamera/build/../subprojects/libyuv/source/mjpeg_decoder.cc:97:(.text+0x1a8): > > undefined reference to `jpeg_destroy_decompress' > > > > /usr/bin/ld: subprojects/libyuv/libyuv.a(source_mjpeg_decoder.cc.o): in > > function `libyuv::MJpegDecoder::LoadFrame(unsigned char const*, unsigned > > long)': > > > > > /usr/local/google/home/chenghaoyang/Workspace/libcamera/build/../subprojects/libyuv/source/mjpeg_decoder.cc:122:(.text+0x2b6): > > undefined reference to `jpeg_read_header' > > > > /usr/bin/ld: subprojects/libyuv/libyuv.a(source_mjpeg_decoder.cc.o): in > > function `libyuv::MJpegDecoder::UnloadFrame()': > > > > > /usr/local/google/home/chenghaoyang/Workspace/libcamera/build/../subprojects/libyuv/source/mjpeg_decoder.cc:242:(.text+0x7d2): > > undefined reference to `jpeg_abort_decompress' > > > > /usr/bin/ld: subprojects/libyuv/libyuv.a(source_mjpeg_decoder.cc.o): in > > function `libyuv::MJpegDecoder::StartDecode()': > > > > > /usr/local/google/home/chenghaoyang/Workspace/libcamera/build/../subprojects/libyuv/source/mjpeg_decoder.cc:528:(.text+0x166b): > > undefined reference to `jpeg_start_decompress' > > > > /usr/bin/ld: subprojects/libyuv/libyuv.a(source_mjpeg_decoder.cc.o): in > > function `libyuv::MJpegDecoder::FinishDecode()': > > > > > /usr/local/google/home/chenghaoyang/Workspace/libcamera/build/../subprojects/libyuv/source/mjpeg_decoder.cc:538:(.text+0x169e): > > undefined reference to `jpeg_abort_decompress' > > > > /usr/bin/ld: subprojects/libyuv/libyuv.a(source_mjpeg_decoder.cc.o): in > > function `libyuv::MJpegDecoder::DecodeImcuRow()': > > > > > /usr/local/google/home/chenghaoyang/Workspace/libcamera/build/../subprojects/libyuv/source/mjpeg_decoder.cc:554:(.text._ZN6libyuv12MJpegDecoder13DecodeImcuRowEv[_ZN6libyuv12MJpegDecoder13DecodeImcuRowEv]+0x40): > > undefined reference to `jpeg_read_raw_data' > > > > collect2: error: ld returned 1 exit status > > ``` > > > > > > > > > > > > > > > > > # Fallback to a subproject if libyuv isn't found, as it's > typically > > > not > > > > > # provided by distributions. > > > > > @@ -26,3 +29,4 @@ if not libyuv_dep.found() > > > > > endif > > > > > > > > > > libcamera_deps += [libyuv_dep] > > > > > +libcamera_deps += [libjpeg] > > > > > diff --git a/src/libcamera/pipeline/virtual/parser.cpp > > > b/src/libcamera/pipeline/virtual/parser.cpp > > > > > index 032c0cd9d..46d1ab181 100644 > > > > > --- a/src/libcamera/pipeline/virtual/parser.cpp > > > > > +++ b/src/libcamera/pipeline/virtual/parser.cpp > > > > > @@ -18,6 +18,7 @@ > > > > > #include "libcamera/internal/pipeline_handler.h" > > > > > #include "libcamera/internal/yaml_parser.h" > > > > > > > > > > +#include "common_functions.h" > > > > > > drop this > > > > > Done > > > > > > > > > > > > #include "virtual.h" > > > > > > > > > > namespace libcamera { > > > > > @@ -52,12 +53,12 @@ std::vector<std::unique_ptr<VirtualCameraData>> > > > Parser::parseConfigFile( > > > > > continue; > > > > > } > > > > > > > > > > - data->id_ = cameraId; > > > > > + data->config_.id = cameraId; > > > > > ControlInfoMap::Map controls; > > > > > /* todo: Check which resolution's frame rate to be > > > reported */ > > > > > controls[&controls::FrameDurationLimits] = > > > > > - ControlInfo(int64_t(1000 / > > > data->supportedResolutions_[0].frameRates[1]), > > > > > - int64_t(1000 / > > > data->supportedResolutions_[0].frameRates[0])); > > > > > + ControlInfo(int64_t(1000 / > > > data->config_.resolutions[0].frameRates[1]), > > > > > + int64_t(1000 / > > > data->config_.resolutions[0].frameRates[0])); > > > > > data->controlInfo_ = > ControlInfoMap(std::move(controls), > > > controls::controls); > > > > > configurations.push_back(std::move(data)); > > > > > } > > > > > @@ -72,7 +73,7 @@ std::unique_ptr<VirtualCameraData> > > > Parser::parseCameraConfigData( > > > > > if (parseSupportedFormats(cameraConfigData, data.get())) > > > > > return nullptr; > > > > > > > > > > - if (parseTestPattern(cameraConfigData, data.get())) > > > > > + if (parseFrame(cameraConfigData, data.get())) > > > > > return nullptr; > > > > > > > > > > if (parseLocation(cameraConfigData, data.get())) > > > > > @@ -122,14 +123,14 @@ int Parser::parseSupportedFormats( > > > > > frameRates.push_back(60); > > > > > } > > > > > > > > > > - data->supportedResolutions_.emplace_back( > > > > > + data->config_.resolutions.emplace_back( > > > > > VirtualCameraData::Resolution{ Size{ > > > width, height }, > > > > > > frameRates > > > }); > > > > > > > > > > activeResolution = std::max(activeResolution, > > > Size{ width, height }); > > > > > } > > > > > } else { > > > > > - data->supportedResolutions_.emplace_back( > > > > > + data->config_.resolutions.emplace_back( > > > > > VirtualCameraData::Resolution{ Size{ 1920, > 1080 }, > > > > > { 30, 60 } }); > > > > > activeResolution = Size(1920, 1080); > > > > > @@ -141,21 +142,65 @@ int Parser::parseSupportedFormats( > > > > > return 0; > > > > > } > > > > > > > > > > -int Parser::parseTestPattern( > > > > > +int Parser::parseFrame( > > > > > const YamlObject &cameraConfigData, VirtualCameraData *data) > > > > > > Why ?? > > > > > > int Parser::parseFrame(const YamlObject &cameraConfigData, > > > VirtualCameraData *data) > > > > > > is fine or > > > > > > int Parser::parseFrame(const YamlObject &cameraConfigData, > > > VirtualCameraData *data) > > > > > > if you really want to > > > > > > Done > > > > > > > > > { > > > > > - std::string testPattern = > > > cameraConfigData["test_pattern"].get<std::string>().value(); > > > > > + const YamlObject &frames = cameraConfigData["frames"]; > > > > > > empty line please > > > > > Done > > > > > > > > > > > > + /* When there is no frames provided in the config file, use > color > > > bar test pattern */ > > > > > + if (frames.size() == 0) { > > > > > + data->config_.frame = TestPattern::ColorBars; > > > > > + return 0; > > > > > + } > > > > > + > > > > > + if (!frames.isDictionary()) { > > > > > + LOG(Virtual, Error) << "'frames' is not a dictionary."; > > > > > + return -EINVAL; > > > > > + } > > > > > + > > > > > + std::string path = frames["path"].get<std::string>().value(); > > > > > > > > What if `path` is not specified? Is it checked somewhere else? > > > > > > > > > > I would really make the "path" and "test_pattern" cases separate, > > > otherwise you're here parsing a string that can be either a system > > > path (which you have to validate) or "bars"\"lines" that are used for > > > TPG. > > > > > Yeah you're right. Split it into two different arguments. > > > > > > > > > > > > > > > > + > > > > > + if (auto ext = getExtension(path); ext == ".jpg" || ext == > > > ".jpeg") { > > > > > + ScaleMode scaleMode; > > > > > > declaring a variable inside the if () clause is valid but rather > > > unusual (at least to me) > > > > > Moved it outside. > > > > > > > > > > > > + if (parseScaleMode(frames, &scaleMode)) > > > > > + return -EINVAL; > > > > > + data->config_.frame = ImageFrames{ path, scaleMode, > > > std::nullopt }; > > > > > + } else if (path.back() == '/') { > > > > > > > > This looks very fragile. Why not use > `std::filesystem::symlink_status()` > > > to check > > > > if it's a file or a directory. > > > > > > > > > > > > > + ScaleMode scaleMode; > > > > > + if (parseScaleMode(frames, &scaleMode)) > > > > > + return -EINVAL; > > > > > + data->config_.frame = ImageFrames{ path, scaleMode, > > > > > + > > > numberOfFilesInDirectory(path) }; > > > > > > > > Admittedly I did not check in detail, but is this necessary? Is it > > > necessary > > > > to count the number of files in advance? > > > > > > > > > > > > > + } else if (path == "bars" || path == "") { > > > > > + /* Default value is "bars" */ > > > > > + data->config_.frame = TestPattern::ColorBars; > > > > > + } else if (path == "lines") { > > > > > + data->config_.frame = TestPattern::DiagonalLines; > > > > > + } else { > > > > > + LOG(Virtual, Error) << "Frame: " << path > > > > > + << " is not supported"; > > > > > + return -EINVAL; > > > > > + } > > > > > > Could you consider reworking the above by changing the schema to > > > separate TPG from images ? > > > > > Done > > > > > > > > > > Empty line > > > > > Done > > > > > > > > > > > > + return 0; > > > > > +} > > > > > > > > > > - /* Default value is "bars" */ > > > > > - if (testPattern == "bars" || testPattern == "") { > > > > > - data->testPattern_ = TestPattern::ColorBars; > > > > > - } else if (testPattern == "lines") { > > > > > - data->testPattern_ = TestPattern::DiagonalLines; > > > > > +int Parser::parseScaleMode( > > > > > + const YamlObject &framesConfigData, ScaleMode *scaleMode) > > > > > +{ > > > > > + std::string mode = > > > framesConfigData["scale_mode"].get<std::string>().value(); > > > > > > > > Same here, regarding empty optionals. > > > > > > > > > > > > > + > > > > > + /* Default value is fill */ > > > > > + if (mode == "fill" || mode == "") { > > > > > + *scaleMode = ScaleMode::Fill; > > > > > + } else if (mode == "contain") { > > > > > + *scaleMode = ScaleMode::Contain; > > > > > + } else if (mode == "cover") { > > > > > + *scaleMode = ScaleMode::Cover; > > > > > > You don't seem to support these in code, why allow them ? Drop them > > > all and assume the only supported one for now > > > > > Done > > > > > > > > > > > > } else { > > > > > - LOG(Virtual, Error) << "Test pattern: " << testPattern > > > > > - << "is not supported"; > > > > > + LOG(Virtual, Error) << "scaleMode: " << mode > > > > > + << " is not supported"; > > > > > return -EINVAL; > > > > > } > > > > > + > > > > > return 0; > > > > > } > > > > > > > > > > @@ -195,4 +240,4 @@ int Parser::parseModel( > > > > > return 0; > > > > > } > > > > > > > > > > -} /* namespace libcamera */ > > > > > +} // namespace libcamera > > > > > > Why oh why ? > > > > > Hmm, not sure why I had this change... > > Removed. > > > > > > > > > > > > diff --git a/src/libcamera/pipeline/virtual/parser.h > > > b/src/libcamera/pipeline/virtual/parser.h > > > > > index a377d8aa1..38ea460d5 100644 > > > > > --- a/src/libcamera/pipeline/virtual/parser.h > > > > > +++ b/src/libcamera/pipeline/virtual/parser.h > > > > > @@ -34,12 +34,15 @@ private: > > > > > > > > > > int parseSupportedFormats( > > > > > const YamlObject &cameraConfigData, VirtualCameraData > > > *data); > > > > > - int parseTestPattern( > > > > > + int parseFrame( > > > > > const YamlObject &cameraConfigData, VirtualCameraData > > > *data); > > > > > int parseLocation( > > > > > const YamlObject &cameraConfigData, VirtualCameraData > > > *data); > > > > > int parseModel( > > > > > const YamlObject &cameraConfigData, VirtualCameraData > > > *data); > > > > > + > > > > > + int parseScaleMode( > > > > > + const YamlObject &framesConfigData, ScaleMode > *scaleMode); > > > > > > I've gave up trying to understand these indentation rules > > > > > Made it one line. > > > > > > > > > > > > }; > > > > > > > > > > } // namespace libcamera > > > > > diff --git > a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > > > b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > > > > > index 6df9b31e9..844dffd49 100644 > > > > > --- a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > > > > > +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > > > > > @@ -20,7 +20,7 @@ LOG_DECLARE_CATEGORY(Virtual) > > > > > static const unsigned int kARGBSize = 4; > > > > > > > > > > void TestPatternGenerator::generateFrame( > > > > > - const Size &size, > > > > > + [[maybe_unused]] unsigned int &frameCount, const Size &size, > > > > > const FrameBuffer *buffer) > > > > > { > > > > > MappedFrameBuffer mappedFrameBuffer(buffer, > > > > > @@ -29,7 +29,7 @@ void TestPatternGenerator::generateFrame( > > > > > auto planes = mappedFrameBuffer.planes(); > > > > > > > > > > /* TODO: select whether to do shifting or not */ > > > > > - shiftLeft(size); > > > > > + // shiftLeft(size); > > > > > > Is this related at all ? > > > > > Removed. > > > > > > > > > > > > > > > > > /* Convert the template_ to the frame buffer */ > > > > > int ret = libyuv::ARGBToNV12( > > > > > diff --git > a/src/libcamera/pipeline/virtual/test_pattern_generator.h > > > b/src/libcamera/pipeline/virtual/test_pattern_generator.h > > > > > index ed8d4e43b..cebdd0141 100644 > > > > > --- a/src/libcamera/pipeline/virtual/test_pattern_generator.h > > > > > +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.h > > > > > @@ -25,7 +25,7 @@ enum class TestPattern : char { > > > > > class TestPatternGenerator : public FrameGenerator > > > > > { > > > > > private: > > > > > - void generateFrame(const Size &size, > > > > > + void generateFrame(unsigned int &frameCount, const Size &size, > > > > > const FrameBuffer *buffer) override; > > > > > > > > > > protected: > > > > > diff --git a/src/libcamera/pipeline/virtual/virtual.cpp > > > b/src/libcamera/pipeline/virtual/virtual.cpp > > > > > index 0fe471f00..2412af703 100644 > > > > > --- a/src/libcamera/pipeline/virtual/virtual.cpp > > > > > +++ b/src/libcamera/pipeline/virtual/virtual.cpp > > > > > @@ -20,9 +20,13 @@ > > > > > #include "libcamera/internal/pipeline_handler.h" > > > > > #include "libcamera/internal/yaml_parser.h" > > > > > > > > > > -#include "frame_generator.h" > > > > > #include "parser.h" > > > > > > > > > > +#define ifTestPattern(v) std::holds_alternative<TestPattern>(v) > > > > > +#define getTestPattern(v) std::get<TestPattern>(v) > > > > > +#define ifImageFrames(v) std::holds_alternative<ImageFrames>(v) > > > > > +#define getImageFrames(v) std::get<ImageFrames>(v) > > > > > + > > > > > namespace libcamera { > > > > > > > > > > LOG_DEFINE_CATEGORY(Virtual) > > > > > @@ -63,12 +67,12 @@ CameraConfiguration::Status > > > VirtualCameraConfiguration::validate() > > > > > } > > > > > > > > > > Size maxSize; > > > > > - for (const auto &resolution : data_->supportedResolutions_) > > > > > + for (const auto &resolution : data_->config_.resolutions) > > > > > maxSize = std::max(maxSize, resolution.size); > > > > > > > > > > for (StreamConfiguration &cfg : config_) { > > > > > bool found = false; > > > > > - for (const auto &resolution : > > > data_->supportedResolutions_) { > > > > > + for (const auto &resolution : > data_->config_.resolutions) { > > > > > if (resolution.size.width == cfg.size.width && > > > > > resolution.size.height == cfg.size.height) > { > > > > > found = true; > > > > > @@ -110,7 +114,7 @@ > > > PipelineHandlerVirtual::generateConfiguration(Camera *camera, > > > > > return config; > > > > > > > > > > Size minSize, sensorResolution; > > > > > - for (const auto &resolution : data->supportedResolutions_) { > > > > > + for (const auto &resolution : data->config_.resolutions) { > > > > > if (minSize.isNull() || minSize > resolution.size) > > > > > minSize = resolution.size; > > > > > > > > > > @@ -199,7 +203,7 @@ int PipelineHandlerVirtual::exportFrameBuffers( > > > > > int PipelineHandlerVirtual::start(Camera *camera, > > > > > [[maybe_unused]] const ControlList > > > *controls) > > > > > { > > > > > - /* \todo Start reading the virtual video if any. */ > > > > > + /* Start reading the images/generating test patterns */ > > > > > VirtualCameraData *data = cameraData(camera); > > > > > > > > > > > > > data->frameGenerator_->configure(data->stream_.configuration().size); > > > > > @@ -219,8 +223,8 @@ int > > > PipelineHandlerVirtual::queueRequestDevice([[maybe_unused]] Camera > *camera, > > > > > > > > > > /* \todo Read from the virtual video if any. */ > > > > > for (auto const &[stream, buffer] : request->buffers()) { > > > > > - /* map buffer and fill test patterns */ > > > > > - > > > data->frameGenerator_->generateFrame(stream->configuration().size, > buffer); > > > > > + /* Map buffer. Fill test patterns or images */ > > > > > + data->frameGenerator_->generateFrame(data->frameCount_, > > > stream->configuration().size, buffer); > > > > > > I don't see frameCount_ be used anywhere here, but I suspect you want > > > it be modified by generateFrame() (answers my first question of the > > > email). I don't know what you intend to use > > > it for, but if, as the name suggest you want to count frames, you can > > > do it here without the frameGenerator being involved ? It's > > > one-request-one-frame after all, isn't it ? So you don't have to > > > pollute TestPatternGenerator::generateFrame with a > > > [[maybe_unused]] unsigned int &frameCount, > > > > > Yeah right. Migrated to ImageFrameGenerator. > > > > I think the original idea is it allows the pipeline handler to control > > which frame > > to use in each request, but let's delay the feature when we actually > > implement > > it. > > > > > > > > > > > > > > > completeBuffer(request, buffer); > > > > > } > > > > > > > > > > @@ -250,9 +254,10 @@ bool > > > PipelineHandlerVirtual::match([[maybe_unused]] DeviceEnumerator > *enumerator > > > > > /* Configure and register cameras with configData */ > > > > > for (auto &data : configData) { > > > > > std::set<Stream *> streams{ &data->stream_ }; > > > > > - std::string id = data->id_; > > > > > + std::string id = data->config_.id; > > > > > std::shared_ptr<Camera> camera = > > > Camera::create(std::move(data), id, streams); > > > > > > > > > > + /* Initialize FrameGenerator*/ > > > > > initFrameGenerator(camera.get()); > > > > > > > > > > registerCamera(std::move(camera)); > > > > > @@ -264,13 +269,19 @@ bool > > > PipelineHandlerVirtual::match([[maybe_unused]] DeviceEnumerator > *enumerator > > > > > void PipelineHandlerVirtual::initFrameGenerator(Camera *camera) > > > > > { > > > > > auto data = cameraData(camera); > > > > > - if (data->testPattern_ == TestPattern::DiagonalLines) { > > > > > - data->frameGenerator_ = > DiagonalLinesGenerator::create(); > > > > > - } else { > > > > > - data->frameGenerator_ = ColorBarsGenerator::create(); > > > > > + auto &frame = data->config_.frame; > > > > > + if (ifTestPattern(frame)) { > > > > > + TestPattern &testPattern = getTestPattern(frame); > > > > > + if (testPattern == TestPattern::DiagonalLines) { > > > > > + data->frameGenerator_ = > > > DiagonalLinesGenerator::create(); > > > > > + } else { > > > > > + data->frameGenerator_ = > > > ColorBarsGenerator::create(); > > > > > + } > > > > > + } else if (ifImageFrames(frame)) { > > > > > + data->frameGenerator_ = > > > ImageFrameGenerator::create(getImageFrames(frame)); > > > > > } > > > > > > > > I think I would strongly prefer `std::visit()` here. But even > > > `std::get_if()` is better in my opinion. > > > > > > Regarding the revistied version on fdo > > > > > > if (testPattern == > > > TestPattern::DiagonalLines) { > > > data->frameGenerator_ = > > > DiagonalLinesGenerator::create(); > > > } else { > > > data->frameGenerator_ = > > > ColorBarsGenerator::create(); > > > } > > > > > > again, no {} for single line statements > > > > > Removed {}. > > > > > > > > > > > > > > > > > > > > } > > > > > > > > > > REGISTER_PIPELINE_HANDLER(PipelineHandlerVirtual, "virtual") > > > > > > > > > > -} /* namespace libcamera */ > > > > > +} // namespace libcamera > > > > > > meh > > > > > Removed. > > > > > > > > > > > > diff --git a/src/libcamera/pipeline/virtual/virtual.h > > > b/src/libcamera/pipeline/virtual/virtual.h > > > > > index c1ac4eb90..f41a4a906 100644 > > > > > --- a/src/libcamera/pipeline/virtual/virtual.h > > > > > +++ b/src/libcamera/pipeline/virtual/virtual.h > > > > > @@ -7,16 +7,22 @@ > > > > > > > > > > #pragma once > > > > > > > > > > +#include <variant> > > > > > + > > > > > #include <libcamera/base/file.h> > > > > > > > > > > #include "libcamera/internal/camera.h" > > > > > #include "libcamera/internal/dma_buf_allocator.h" > > > > > #include "libcamera/internal/pipeline_handler.h" > > > > > > > > > > +#include "frame_generator.h" > > > > > +#include "image_frame_generator.h" > > > > > #include "test_pattern_generator.h" > > > > > > > > > > namespace libcamera { > > > > > > > > > > +using VirtualFrame = std::variant<TestPattern, ImageFrames>; > > > > > + > > > > > class VirtualCameraData : public Camera::Private > > > > > { > > > > > public: > > > > > @@ -24,6 +30,13 @@ public: > > > > > Size size; > > > > > std::vector<int> frameRates; > > > > > }; > > > > > + /* The config file is parsed to the Configuration struct */ > > > > > + struct Configuration { > > > > > + std::string id; > > > > > > so you need <string> > > > > > Done > > > > > > > > > > > > + std::vector<Resolution> resolutions; > > > > > > and <vector> > > > > > Done > > > > > > > > > > > > + VirtualFrame frame; > > > > > + }; > > > > > + > > > > > VirtualCameraData(PipelineHandler *pipe) > > > > > : Camera::Private(pipe) > > > > > { > > > > > @@ -31,12 +44,9 @@ public: > > > > > > > > > > ~VirtualCameraData() = default; > > > > > > > > > > - std::string id_; > > > > > - std::vector<Resolution> supportedResolutions_; > > > > > > well, you did already > > > > > Sorry, I did what...? > > Need the above includes in the previous patch that introduced the > supportedResolutions_ vector ;) > Oh haha got it :) > > > > > > > > > > > > > - TestPattern testPattern_; > > > > > - > > > > > + unsigned int frameCount_ = 0; > > > > > + Configuration config_; > > > > > Stream stream_; > > > > > - > > > > > std::unique_ptr<FrameGenerator> frameGenerator_; > > > > > }; > > > > > > > > > > -- > > > > > 2.46.0.184.g6999bdac58-goog > > > > > > > > > > > > > > > > > Regards, > > > > Barnabás Pőcze > > > > > > > BR, Harvey >
diff --git a/src/libcamera/pipeline/virtual/README.md b/src/libcamera/pipeline/virtual/README.md index 27d6283df..5e21ce74a 100644 --- a/src/libcamera/pipeline/virtual/README.md +++ b/src/libcamera/pipeline/virtual/README.md @@ -15,8 +15,13 @@ Each camera block is a dictionary, containing the following keys: - `supported_formats` (list of `VirtualCameraData::Resolution`, optional) : List of supported resolution and frame rates of the emulated camera - `width` (`unsigned int`, default=1920): Width of the window resolution. This needs to be even. - `height` (`unsigned int`, default=1080): Height of the window resolution. - - `frame_rates` (list of `int`, default=`[30,60]` ): Range of the frame rate. The list has to be two values of the lower bound and the upper bound of the frame rate. -- `test_pattern` (`string`, default="bars"): Which test pattern to use as frames. The options are "bars", "lines". + - `frame_rates` (list of `int`, default=`[30,60]` ): Range of the frame rate. The list has to be two values of the lower bound and the upper bound of the frame rate. This does not affect the frame rate for now. +- `frames` (dictionary): + - `path` (`string`, default="bars"): Name of a test pattern, path to an image, or path to a directory of a series of images. + - The test patterns are "bars" which means color bars, and "lines" which means diagonal lines. + - The path to an image has ".jpg" extension. + - 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. + - `scale_mode`(`string`, default="fill"): Scale mode when the frames are images. The scale modes are "fill", "contain", and "cover". This does not matter when frames is a test pattern. This does not affect the scale mode for now. - `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. - `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. @@ -35,13 +40,16 @@ A sample config file: frame_rates: - 70 - 80 - test_pattern: "bars" + frames: + path: "lines" location: "front" model: "Virtual Video Device" "Virtual1": supported_formats: - width: 800 - test_pattern: "lines" + frames: + path: "path/to/directory_of_images/" + scale_mode: "contain" location: "back" model: "Virtual Video Device1" "Virtual2": @@ -61,7 +69,7 @@ This is the procedure of the Parser class: - If the config file contains invalid configuration, this method returns nullptr. The camera will be skipped. 3. Parse each property and register the data. - `parseSupportedFormats()`: Parses `supported_formats` in the config, which contains resolutions and frame rates. - - `parseTestPattern()`: Parses `test_pattern` in the config. + - `parseFrame()`: Parses `frames` in the config. - `parseLocation()`: Parses `location` in the config. - `parseModel()`: Parses `model` in the config. 4. Back to `parseConfigFile()` and append the camera configuration. diff --git a/src/libcamera/pipeline/virtual/common_functions.cpp b/src/libcamera/pipeline/virtual/common_functions.cpp new file mode 100644 index 000000000..207827ee0 --- /dev/null +++ b/src/libcamera/pipeline/virtual/common_functions.cpp @@ -0,0 +1,27 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2023, Google Inc. + * + * common_functions.cpp - Helper that do not depend on any class + */ + +#include "common_functions.h" + +namespace libcamera { + +std::string getExtension(const std::string &path) +{ + size_t i = path.find("."); + if (i != std::string::npos) { + return path.substr(i); + } + return ""; +} + +std::size_t numberOfFilesInDirectory(std::filesystem::path path) +{ + using std::filesystem::directory_iterator; + return std::distance(directory_iterator(path), directory_iterator{}); +} + +} // namespace libcamera diff --git a/src/libcamera/pipeline/virtual/common_functions.h b/src/libcamera/pipeline/virtual/common_functions.h new file mode 100644 index 000000000..4203f9505 --- /dev/null +++ b/src/libcamera/pipeline/virtual/common_functions.h @@ -0,0 +1,18 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2023, Google Inc. + * + * common_functions.h - Helper that do not depend on any class + */ + +#pragma once + +#include <filesystem> + +namespace libcamera { + +std::string getExtension(const std::string &path); + +std::size_t numberOfFilesInDirectory(std::filesystem::path path); + +} // namespace libcamera diff --git a/src/libcamera/pipeline/virtual/frame_generator.h b/src/libcamera/pipeline/virtual/frame_generator.h index 9699af7a4..f69576b36 100644 --- a/src/libcamera/pipeline/virtual/frame_generator.h +++ b/src/libcamera/pipeline/virtual/frame_generator.h @@ -23,7 +23,7 @@ public: /** Fill the output frame buffer. * Use the frame at the frameCount of image frames */ - virtual void generateFrame(const Size &size, + virtual void generateFrame(unsigned int &frameCount, const Size &size, const FrameBuffer *buffer) = 0; protected: diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.cpp b/src/libcamera/pipeline/virtual/image_frame_generator.cpp new file mode 100644 index 000000000..d374877ff --- /dev/null +++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp @@ -0,0 +1,154 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2023, Google Inc. + * + * image_frame_generator.cpp - Derived class of FrameGenerator for + * generating frames from images + */ + +#include "image_frame_generator.h" + +#include <memory> +#include <optional> +#include <string> + +#include <libcamera/base/file.h> +#include <libcamera/base/log.h> + +#include <libcamera/framebuffer.h> + +#include "libcamera/internal/mapped_framebuffer.h" + +#include "libyuv/convert.h" +#include "libyuv/scale.h" +namespace libcamera { + +LOG_DECLARE_CATEGORY(Virtual) + +std::unique_ptr<ImageFrameGenerator> ImageFrameGenerator::create( + ImageFrames &imageFrames) +{ + std::unique_ptr<ImageFrameGenerator> imageFrameGenerator = + std::make_unique<ImageFrameGenerator>(); + imageFrameGenerator->imageFrames_ = &imageFrames; + + /** For each file in the directory + * load the image, convert it to NV12, and store the pointer + */ + for (unsigned int i = 0; i < imageFrames.number.value_or(1); i++) { + std::string path; + if (!imageFrames.number.has_value()) { + /* If the path is to an image */ + path = imageFrames.path; + } else { + /* If the path is to a directory */ + path = constructPath(imageFrames.path, i); + } + + File file(path); + bool isOpen = file.open(File::OpenModeFlag::ReadOnly); + if (!isOpen) { + LOG(Virtual, Error) << "Failed to open image file: " << file.fileName(); + return nullptr; + } + + /* Read the image file to data */ + uint8_t buffer[file.size()]; + Span<unsigned char> data{ buffer, (unsigned long)file.size() }; + long dataSize = file.read(data); + + /* Get the width and height of the image */ + int width, height; + if (libyuv::MJPGSize(data.data(), dataSize, &width, &height)) { + LOG(Virtual, Error) << "Failed to get the size of the image file: " + << file.fileName(); + return nullptr; + } + + /* Convert to NV12 and write the data to tmpY and tmpUV */ + int halfWidth = (width + 1) / 2; + int halfHeight = (height + 1) / 2; + std::unique_ptr<uint8_t[]> dstY = + std::make_unique<uint8_t[]>(width * height); + std::unique_ptr<uint8_t[]> dstUV = + std::make_unique<uint8_t[]>(halfWidth * halfHeight * 2); + int ret = libyuv::MJPGToNV12(data.data(), dataSize, + dstY.get(), width, dstUV.get(), + width, width, height, width, height); + if (ret != 0) { + LOG(Virtual, Error) << "MJPGToNV12() failed with " << ret; + } + + imageFrameGenerator->imageFrameDatas_.emplace_back( + ImageFrameData{ std::move(dstY), std::move(dstUV), + Size(width, height) }); + } + return imageFrameGenerator; +} + +std::string ImageFrameGenerator::constructPath(std::string &name, unsigned int &i) +{ + return name + std::to_string(i) + ".jpg"; +} + +void ImageFrameGenerator::configure(const Size &size) +{ + for (unsigned int i = 0; i < imageFrames_->number.value_or(1); i++) { + /* Scale the imageFrameDatas_ to scaledY and scaledUV */ + int halfSizeWidth = (size.width + 1) / 2; + int halfSizeHeight = (size.height + 1) / 2; + std::unique_ptr<uint8_t[]> scaledY = + std::make_unique<uint8_t[]>(size.width * size.height); + std::unique_ptr<uint8_t[]> scaledUV = + std::make_unique<uint8_t[]>(halfSizeWidth * halfSizeHeight * 2); + auto &src = imageFrameDatas_[i]; + + /* + * \todo Implement "contain" & "cover", based on + * |imageFrames_[i].scaleMode|. + */ + + /* + * \todo Some platforms might enforce stride due to GPU, like + * ChromeOS ciri (64). The weight needs to be a multiple of + * the stride to work properly for now. + */ + libyuv::NV12Scale(src.Y.get(), src.size.width, + src.UV.get(), src.size.width, + src.size.width, src.size.height, + scaledY.get(), size.width, scaledUV.get(), size.width, + size.width, size.height, libyuv::FilterMode::kFilterBilinear); + + /* Store the pointers to member variable */ + scaledFrameDatas_.emplace_back( + ImageFrameData{ std::move(scaledY), std::move(scaledUV), size }); + } +} + +void ImageFrameGenerator::generateFrame(unsigned int &frameCount, const Size &size, const FrameBuffer *buffer) +{ + /* Don't do anything when the list of buffers is empty*/ + if (scaledFrameDatas_.size() == 0) + return; + + MappedFrameBuffer mappedFrameBuffer(buffer, MappedFrameBuffer::MapFlag::Write); + + auto planes = mappedFrameBuffer.planes(); + + /* Make sure the frameCount does not over the number of images */ + frameCount %= imageFrames_->number.value_or(1); + + /* Write the scaledY and scaledUV to the mapped frame buffer */ + libyuv::NV12Copy(scaledFrameDatas_[frameCount].Y.get(), size.width, + scaledFrameDatas_[frameCount].UV.get(), size.width, planes[0].begin(), + size.width, planes[1].begin(), size.width, + size.width, size.height); + + /* proceed an image every 4 frames */ + /* \todo read the parameter_ from the configuration file? */ + parameter_++; + if (parameter_ % 4 == 0) + frameCount++; +} + +} // namespace libcamera diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.h b/src/libcamera/pipeline/virtual/image_frame_generator.h new file mode 100644 index 000000000..74468e075 --- /dev/null +++ b/src/libcamera/pipeline/virtual/image_frame_generator.h @@ -0,0 +1,65 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2023, Google Inc. + * + * image_frame_generator.h - Derived class of FrameGenerator for + * generating frames from images + */ + +#pragma once + +#include <memory> +#include <optional> +#include <stdint.h> +#include <sys/types.h> + +#include "frame_generator.h" + +namespace libcamera { + +enum class ScaleMode : char { + Fill = 0, + Contain = 1, + Cover = 2, +}; + +/* Frame configuration provided by the config file */ +struct ImageFrames { + std::string path; + ScaleMode scaleMode; + std::optional<unsigned int> number; +}; + +class ImageFrameGenerator : public FrameGenerator +{ +public: + /** Factory function to create an ImageFrameGenerator object. + * Read the images and convert them to buffers in NV12 format. + * Store the pointers to the buffers to a list (imageFrameDatas) + */ + static std::unique_ptr<ImageFrameGenerator> create(ImageFrames &imageFrames); + +private: + struct ImageFrameData { + std::unique_ptr<uint8_t[]> Y; + std::unique_ptr<uint8_t[]> UV; + Size size; + }; + + /* Scale the buffers for image frames. */ + void configure(const Size &size) override; + void generateFrame(unsigned int &frameCount, const Size &size, const FrameBuffer *buffer) override; + + static std::string constructPath(std::string &name, unsigned int &i); + + /* List of pointers to the not scaled image buffers */ + std::vector<ImageFrameData> imageFrameDatas_; + /* List of pointers to the scaled image buffers */ + std::vector<ImageFrameData> scaledFrameDatas_; + /* Pointer to the imageFrames_ in VirtualCameraData */ + ImageFrames *imageFrames_; + /* Speed parameter. Change to the next image every parameter_ frames. */ + int parameter_; +}; + +} /* namespace libcamera */ diff --git a/src/libcamera/pipeline/virtual/meson.build b/src/libcamera/pipeline/virtual/meson.build index 2e82e64cb..d56aedec4 100644 --- a/src/libcamera/pipeline/virtual/meson.build +++ b/src/libcamera/pipeline/virtual/meson.build @@ -2,11 +2,14 @@ libcamera_sources += files([ 'virtual.cpp', - 'test_pattern_generator.cpp', 'parser.cpp', + 'test_pattern_generator.cpp', + 'image_frame_generator.cpp', + 'common_functions.cpp', ]) libyuv_dep = dependency('libyuv', required : false) +libjpeg = dependency('libjpeg', required : false) # Fallback to a subproject if libyuv isn't found, as it's typically not # provided by distributions. @@ -26,3 +29,4 @@ if not libyuv_dep.found() endif libcamera_deps += [libyuv_dep] +libcamera_deps += [libjpeg] diff --git a/src/libcamera/pipeline/virtual/parser.cpp b/src/libcamera/pipeline/virtual/parser.cpp index 032c0cd9d..46d1ab181 100644 --- a/src/libcamera/pipeline/virtual/parser.cpp +++ b/src/libcamera/pipeline/virtual/parser.cpp @@ -18,6 +18,7 @@ #include "libcamera/internal/pipeline_handler.h" #include "libcamera/internal/yaml_parser.h" +#include "common_functions.h" #include "virtual.h" namespace libcamera { @@ -52,12 +53,12 @@ std::vector<std::unique_ptr<VirtualCameraData>> Parser::parseConfigFile( continue; } - data->id_ = cameraId; + data->config_.id = cameraId; ControlInfoMap::Map controls; /* todo: Check which resolution's frame rate to be reported */ controls[&controls::FrameDurationLimits] = - ControlInfo(int64_t(1000 / data->supportedResolutions_[0].frameRates[1]), - int64_t(1000 / data->supportedResolutions_[0].frameRates[0])); + ControlInfo(int64_t(1000 / data->config_.resolutions[0].frameRates[1]), + int64_t(1000 / data->config_.resolutions[0].frameRates[0])); data->controlInfo_ = ControlInfoMap(std::move(controls), controls::controls); configurations.push_back(std::move(data)); } @@ -72,7 +73,7 @@ std::unique_ptr<VirtualCameraData> Parser::parseCameraConfigData( if (parseSupportedFormats(cameraConfigData, data.get())) return nullptr; - if (parseTestPattern(cameraConfigData, data.get())) + if (parseFrame(cameraConfigData, data.get())) return nullptr; if (parseLocation(cameraConfigData, data.get())) @@ -122,14 +123,14 @@ int Parser::parseSupportedFormats( frameRates.push_back(60); } - data->supportedResolutions_.emplace_back( + data->config_.resolutions.emplace_back( VirtualCameraData::Resolution{ Size{ width, height }, frameRates }); activeResolution = std::max(activeResolution, Size{ width, height }); } } else { - data->supportedResolutions_.emplace_back( + data->config_.resolutions.emplace_back( VirtualCameraData::Resolution{ Size{ 1920, 1080 }, { 30, 60 } }); activeResolution = Size(1920, 1080); @@ -141,21 +142,65 @@ int Parser::parseSupportedFormats( return 0; } -int Parser::parseTestPattern( +int Parser::parseFrame( const YamlObject &cameraConfigData, VirtualCameraData *data) { - std::string testPattern = cameraConfigData["test_pattern"].get<std::string>().value(); + const YamlObject &frames = cameraConfigData["frames"]; + /* When there is no frames provided in the config file, use color bar test pattern */ + if (frames.size() == 0) { + data->config_.frame = TestPattern::ColorBars; + return 0; + } + + if (!frames.isDictionary()) { + LOG(Virtual, Error) << "'frames' is not a dictionary."; + return -EINVAL; + } + + std::string path = frames["path"].get<std::string>().value(); + + if (auto ext = getExtension(path); ext == ".jpg" || ext == ".jpeg") { + ScaleMode scaleMode; + if (parseScaleMode(frames, &scaleMode)) + return -EINVAL; + data->config_.frame = ImageFrames{ path, scaleMode, std::nullopt }; + } else if (path.back() == '/') { + ScaleMode scaleMode; + if (parseScaleMode(frames, &scaleMode)) + return -EINVAL; + data->config_.frame = ImageFrames{ path, scaleMode, + numberOfFilesInDirectory(path) }; + } else if (path == "bars" || path == "") { + /* Default value is "bars" */ + data->config_.frame = TestPattern::ColorBars; + } else if (path == "lines") { + data->config_.frame = TestPattern::DiagonalLines; + } else { + LOG(Virtual, Error) << "Frame: " << path + << " is not supported"; + return -EINVAL; + } + return 0; +} - /* Default value is "bars" */ - if (testPattern == "bars" || testPattern == "") { - data->testPattern_ = TestPattern::ColorBars; - } else if (testPattern == "lines") { - data->testPattern_ = TestPattern::DiagonalLines; +int Parser::parseScaleMode( + const YamlObject &framesConfigData, ScaleMode *scaleMode) +{ + std::string mode = framesConfigData["scale_mode"].get<std::string>().value(); + + /* Default value is fill */ + if (mode == "fill" || mode == "") { + *scaleMode = ScaleMode::Fill; + } else if (mode == "contain") { + *scaleMode = ScaleMode::Contain; + } else if (mode == "cover") { + *scaleMode = ScaleMode::Cover; } else { - LOG(Virtual, Error) << "Test pattern: " << testPattern - << "is not supported"; + LOG(Virtual, Error) << "scaleMode: " << mode + << " is not supported"; return -EINVAL; } + return 0; } @@ -195,4 +240,4 @@ int Parser::parseModel( return 0; } -} /* namespace libcamera */ +} // namespace libcamera diff --git a/src/libcamera/pipeline/virtual/parser.h b/src/libcamera/pipeline/virtual/parser.h index a377d8aa1..38ea460d5 100644 --- a/src/libcamera/pipeline/virtual/parser.h +++ b/src/libcamera/pipeline/virtual/parser.h @@ -34,12 +34,15 @@ private: int parseSupportedFormats( const YamlObject &cameraConfigData, VirtualCameraData *data); - int parseTestPattern( + int parseFrame( const YamlObject &cameraConfigData, VirtualCameraData *data); int parseLocation( const YamlObject &cameraConfigData, VirtualCameraData *data); int parseModel( const YamlObject &cameraConfigData, VirtualCameraData *data); + + int parseScaleMode( + const YamlObject &framesConfigData, ScaleMode *scaleMode); }; } // namespace libcamera diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp index 6df9b31e9..844dffd49 100644 --- a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp @@ -20,7 +20,7 @@ LOG_DECLARE_CATEGORY(Virtual) static const unsigned int kARGBSize = 4; void TestPatternGenerator::generateFrame( - const Size &size, + [[maybe_unused]] unsigned int &frameCount, const Size &size, const FrameBuffer *buffer) { MappedFrameBuffer mappedFrameBuffer(buffer, @@ -29,7 +29,7 @@ void TestPatternGenerator::generateFrame( auto planes = mappedFrameBuffer.planes(); /* TODO: select whether to do shifting or not */ - shiftLeft(size); + // shiftLeft(size); /* Convert the template_ to the frame buffer */ int ret = libyuv::ARGBToNV12( diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.h b/src/libcamera/pipeline/virtual/test_pattern_generator.h index ed8d4e43b..cebdd0141 100644 --- a/src/libcamera/pipeline/virtual/test_pattern_generator.h +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.h @@ -25,7 +25,7 @@ enum class TestPattern : char { class TestPatternGenerator : public FrameGenerator { private: - void generateFrame(const Size &size, + void generateFrame(unsigned int &frameCount, const Size &size, const FrameBuffer *buffer) override; protected: diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp index 0fe471f00..2412af703 100644 --- a/src/libcamera/pipeline/virtual/virtual.cpp +++ b/src/libcamera/pipeline/virtual/virtual.cpp @@ -20,9 +20,13 @@ #include "libcamera/internal/pipeline_handler.h" #include "libcamera/internal/yaml_parser.h" -#include "frame_generator.h" #include "parser.h" +#define ifTestPattern(v) std::holds_alternative<TestPattern>(v) +#define getTestPattern(v) std::get<TestPattern>(v) +#define ifImageFrames(v) std::holds_alternative<ImageFrames>(v) +#define getImageFrames(v) std::get<ImageFrames>(v) + namespace libcamera { LOG_DEFINE_CATEGORY(Virtual) @@ -63,12 +67,12 @@ CameraConfiguration::Status VirtualCameraConfiguration::validate() } Size maxSize; - for (const auto &resolution : data_->supportedResolutions_) + for (const auto &resolution : data_->config_.resolutions) maxSize = std::max(maxSize, resolution.size); for (StreamConfiguration &cfg : config_) { bool found = false; - for (const auto &resolution : data_->supportedResolutions_) { + for (const auto &resolution : data_->config_.resolutions) { if (resolution.size.width == cfg.size.width && resolution.size.height == cfg.size.height) { found = true; @@ -110,7 +114,7 @@ PipelineHandlerVirtual::generateConfiguration(Camera *camera, return config; Size minSize, sensorResolution; - for (const auto &resolution : data->supportedResolutions_) { + for (const auto &resolution : data->config_.resolutions) { if (minSize.isNull() || minSize > resolution.size) minSize = resolution.size; @@ -199,7 +203,7 @@ int PipelineHandlerVirtual::exportFrameBuffers( int PipelineHandlerVirtual::start(Camera *camera, [[maybe_unused]] const ControlList *controls) { - /* \todo Start reading the virtual video if any. */ + /* Start reading the images/generating test patterns */ VirtualCameraData *data = cameraData(camera); data->frameGenerator_->configure(data->stream_.configuration().size); @@ -219,8 +223,8 @@ int PipelineHandlerVirtual::queueRequestDevice([[maybe_unused]] Camera *camera, /* \todo Read from the virtual video if any. */ for (auto const &[stream, buffer] : request->buffers()) { - /* map buffer and fill test patterns */ - data->frameGenerator_->generateFrame(stream->configuration().size, buffer); + /* Map buffer. Fill test patterns or images */ + data->frameGenerator_->generateFrame(data->frameCount_, stream->configuration().size, buffer); completeBuffer(request, buffer); } @@ -250,9 +254,10 @@ bool PipelineHandlerVirtual::match([[maybe_unused]] DeviceEnumerator *enumerator /* Configure and register cameras with configData */ for (auto &data : configData) { std::set<Stream *> streams{ &data->stream_ }; - std::string id = data->id_; + std::string id = data->config_.id; std::shared_ptr<Camera> camera = Camera::create(std::move(data), id, streams); + /* Initialize FrameGenerator*/ initFrameGenerator(camera.get()); registerCamera(std::move(camera)); @@ -264,13 +269,19 @@ bool PipelineHandlerVirtual::match([[maybe_unused]] DeviceEnumerator *enumerator void PipelineHandlerVirtual::initFrameGenerator(Camera *camera) { auto data = cameraData(camera); - if (data->testPattern_ == TestPattern::DiagonalLines) { - data->frameGenerator_ = DiagonalLinesGenerator::create(); - } else { - data->frameGenerator_ = ColorBarsGenerator::create(); + auto &frame = data->config_.frame; + if (ifTestPattern(frame)) { + TestPattern &testPattern = getTestPattern(frame); + if (testPattern == TestPattern::DiagonalLines) { + data->frameGenerator_ = DiagonalLinesGenerator::create(); + } else { + data->frameGenerator_ = ColorBarsGenerator::create(); + } + } else if (ifImageFrames(frame)) { + data->frameGenerator_ = ImageFrameGenerator::create(getImageFrames(frame)); } } REGISTER_PIPELINE_HANDLER(PipelineHandlerVirtual, "virtual") -} /* namespace libcamera */ +} // namespace libcamera diff --git a/src/libcamera/pipeline/virtual/virtual.h b/src/libcamera/pipeline/virtual/virtual.h index c1ac4eb90..f41a4a906 100644 --- a/src/libcamera/pipeline/virtual/virtual.h +++ b/src/libcamera/pipeline/virtual/virtual.h @@ -7,16 +7,22 @@ #pragma once +#include <variant> + #include <libcamera/base/file.h> #include "libcamera/internal/camera.h" #include "libcamera/internal/dma_buf_allocator.h" #include "libcamera/internal/pipeline_handler.h" +#include "frame_generator.h" +#include "image_frame_generator.h" #include "test_pattern_generator.h" namespace libcamera { +using VirtualFrame = std::variant<TestPattern, ImageFrames>; + class VirtualCameraData : public Camera::Private { public: @@ -24,6 +30,13 @@ public: Size size; std::vector<int> frameRates; }; + /* The config file is parsed to the Configuration struct */ + struct Configuration { + std::string id; + std::vector<Resolution> resolutions; + VirtualFrame frame; + }; + VirtualCameraData(PipelineHandler *pipe) : Camera::Private(pipe) { @@ -31,12 +44,9 @@ public: ~VirtualCameraData() = default; - std::string id_; - std::vector<Resolution> supportedResolutions_; - TestPattern testPattern_; - + unsigned int frameCount_ = 0; + Configuration config_; Stream stream_; - std::unique_ptr<FrameGenerator> frameGenerator_; };