<div dir="ltr"><div dir="ltr">Hi Jacopo,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Sep 10, 2024 at 4:26 AM Jacopo Mondi <<a href="mailto:jacopo.mondi@ideasonboard.com" target="_blank">jacopo.mondi@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Harvey<br>
<br>
On Mon, Sep 09, 2024 at 11:22:47PM GMT, Cheng-Hao Yang wrote:<br>
> Hi Jacopo,<br>
><br>
> On Mon, Sep 9, 2024 at 6:17 PM Jacopo Mondi <<a href="mailto:jacopo.mondi@ideasonboard.com" target="_blank">jacopo.mondi@ideasonboard.com</a>><br>
> wrote:<br>
><br>
> > Hi<br>
> ><br>
> > On Sat, Sep 07, 2024 at 02:28:31PM GMT, Harvey Yang wrote:<br>
> > > From: Konami Shu <<a href="mailto:konamiz@google.com" target="_blank">konamiz@google.com</a>><br>
> > ><br>
> > > Besides TestPatternGenerator, this patch adds ImageFrameGenerator that<br>
> > > loads real images (jpg / jpeg for now) as the source and generates<br>
> > > scaled frames.<br>
> > ><br>
> > > Signed-off-by: Konami Shu <<a href="mailto:konamiz@google.com" target="_blank">konamiz@google.com</a>><br>
> > > Co-developed-by: Harvey Yang <<a href="mailto:chenghaoyang@chromium.org" target="_blank">chenghaoyang@chromium.org</a>><br>
> > > Co-developed-by: Yunke Cao <<a href="mailto:yunkec@chromium.org" target="_blank">yunkec@chromium.org</a>><br>
> > > Co-developed-by: Tomasz Figa <<a href="mailto:tfiga@chromium.org" target="_blank">tfiga@chromium.org</a>><br>
> > > ---<br>
> > > src/libcamera/pipeline/virtual/README.md | 9 +-<br>
> > > .../virtual/image_frame_generator.cpp | 178 ++++++++++++++++++<br>
> > > .../pipeline/virtual/image_frame_generator.h | 54 ++++++<br>
> > > src/libcamera/pipeline/virtual/meson.build | 4 +<br>
> > > src/libcamera/pipeline/virtual/parser.cpp | 76 +++++++-<br>
> > > src/libcamera/pipeline/virtual/parser.h | 2 +<br>
> > > src/libcamera/pipeline/virtual/utils.h | 17 ++<br>
> > > src/libcamera/pipeline/virtual/virtual.cpp | 60 ++++--<br>
> > > src/libcamera/pipeline/virtual/virtual.h | 24 ++-<br>
> > > 9 files changed, 389 insertions(+), 35 deletions(-)<br>
> > > create mode 100644<br>
> > src/libcamera/pipeline/virtual/image_frame_generator.cpp<br>
> > > create mode 100644<br>
> > src/libcamera/pipeline/virtual/image_frame_generator.h<br>
> > > create mode 100644 src/libcamera/pipeline/virtual/utils.h<br>
> > ><br>
> > > diff --git a/src/libcamera/pipeline/virtual/README.md<br>
> > b/src/libcamera/pipeline/virtual/README.md<br>
> > > index ef80bb48..18c8341b 100644<br>
> > > --- a/src/libcamera/pipeline/virtual/README.md<br>
> > > +++ b/src/libcamera/pipeline/virtual/README.md<br>
> > > @@ -16,7 +16,13 @@ Each camera block is a dictionary, containing the<br>
> > following keys:<br>
> > > - `width` (`unsigned int`, default=1920): Width of the window<br>
> > resolution. This needs to be even.<br>
> > > - `height` (`unsigned int`, default=1080): Height of the window<br>
> > resolution.<br>
> > > - `frame_rates` (list of `int`, default=`[30,60]` ): Range of the<br>
> > frame rate (per second). If the list contains one value, it's the lower<br>
> > bound and the upper bound. If the list contains two values, the first is<br>
> > the lower bound and the second is the upper bound. No other number of<br>
> > values is allowed.<br>
> > > -- `test_pattern` (`string`): Which test pattern to use as frames. The<br>
> > options are "bars", "lines".<br>
> > > +- `test_pattern` (`string`): Which test pattern to use as frames. The<br>
> > options are "bars", "lines". Cannot be set with `frames`.<br>
> > > +- `frames` (dictionary):<br>
> > > + - `path` (`string`): Path to an image, or path to a directory of a<br>
> > series of images. Cannot be set with `test_pattern`.<br>
> > > + - The test patterns are "bars" which means color bars, and "lines"<br>
> > which means diagonal lines.<br>
> > > + - The path to an image has ".jpg" extension.<br>
> > > + - The path to a directory ends with "/". The name of the images in<br>
> > the directory are "{n}.jpg" with {n} is the sequence of images starting<br>
> > with 0.<br>
> > > + - `scale_mode`(`string`, default="fill"): Scale mode when the frames<br>
> > are images. The only scale mode supported now is "fill". This does not<br>
> > affect the scale mode for now.<br>
> ><br>
> > Wouldn't it be more logical to add the frame generator support before<br>
> > the config file ? Otherwise you are adding pieces here to what has<br>
> > been added just one patch ago<br>
> ><br>
><br>
> Yeah makes sense. Will be updated in v12.<br>
><br>
><br>
> ><br>
> > > - `location` (`string`, default="front"): The location of the camera.<br>
> > Support "front" and "back". This is displayed in qcam camera selection<br>
> > window but this does not change the output.<br>
> > > - `model` (`string`, default="Unknown"): The model name of the camera.<br>
> > This is displayed in qcam camera selection window but this does not change<br>
> > the output.<br>
> > ><br>
> > > @@ -37,6 +43,7 @@ This is the procedure of the Parser class:<br>
> > > 3. Parse each property and register the data.<br>
> > > - `parseSupportedFormats()`: Parses `supported_formats` in the<br>
> > config, which contains resolutions and frame rates.<br>
> > > - `parseTestPattern()`: Parses `test_pattern` in the config.<br>
> > > + - `parseFrame()`: Parses `frames` in the config.<br>
> > > - `parseLocation()`: Parses `location` in the config.<br>
> > > - `parseModel()`: Parses `model` in the config.<br>
> > > 4. Back to `parseConfigFile()` and append the camera configuration.<br>
> > > diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.cpp<br>
> > b/src/libcamera/pipeline/virtual/image_frame_generator.cpp<br>
> > > new file mode 100644<br>
> > > index 00000000..db3efe15<br>
> > > --- /dev/null<br>
> > > +++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp<br>
> > > @@ -0,0 +1,178 @@<br>
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */<br>
> > > +/*<br>
> > > + * Copyright (C) 2024, Google Inc.<br>
> > > + *<br>
> > > + * image_frame_generator.cpp - Derived class of FrameGenerator for<br>
> > > + * generating frames from images<br>
> > > + */<br>
> > > +<br>
> > > +#include "image_frame_generator.h"<br>
> > > +<br>
> > > +#include <filesystem><br>
> > > +#include <memory><br>
> > > +#include <string><br>
> > > +<br>
> > > +#include <libcamera/base/file.h><br>
> > > +#include <libcamera/base/log.h><br>
> > > +<br>
> > > +#include <libcamera/framebuffer.h><br>
> > > +<br>
> > > +#include "libcamera/internal/mapped_framebuffer.h"<br>
> > > +<br>
> > > +#include "libyuv/convert.h"<br>
> > > +#include "libyuv/scale.h"<br>
> > > +<br>
> > > +namespace libcamera {<br>
> > > +<br>
> > > +LOG_DECLARE_CATEGORY(Virtual)<br>
> > > +<br>
> > > +/*<br>
> > > + * Factory function to create an ImageFrameGenerator object.<br>
> > > + * Read the images and convert them to buffers in NV12 format.<br>
> > > + * Store the pointers to the buffers to a list (imageFrameDatas)<br>
> > > + */<br>
> > > +std::unique_ptr<ImageFrameGenerator><br>
> > > +ImageFrameGenerator::create(ImageFrames &imageFrames)<br>
> > > +{<br>
> > > + std::unique_ptr<ImageFrameGenerator> imageFrameGenerator =<br>
> > > + std::make_unique<ImageFrameGenerator>();<br>
> > > + imageFrameGenerator->imageFrames_ = &imageFrames;<br>
> > > +<br>
> > > + /*<br>
> > > + * For each file in the directory, load the image,<br>
> > > + * convert it to NV12, and store the pointer.<br>
> ><br>
> > weird indent<br>
> ><br>
><br>
> Sorry, fixed.<br>
><br>
><br>
> ><br>
> > > + */<br>
> > > + for (unsigned int i = 0; i < imageFrames.number.value_or(1); i++) {<br>
> > > + std::filesystem::path path;<br>
> > > + if (!imageFrames.number)<br>
> > > + /* If the path is to an image */<br>
> > > + path = imageFrames.path;<br>
> > > + else<br>
> > > + /* If the path is to a directory */<br>
> > > + path = imageFrames.path / (std::to_string(i) +<br>
> > ".jpg");<br>
> > > +<br>
> > > + File file(path);<br>
> > > + if (!file.open(File::OpenModeFlag::ReadOnly)) {<br>
> > > + LOG(Virtual, Error) << "Failed to open image file<br>
> > " << file.fileName()<br>
> > > + << ": " <<<br>
> > strerror(file.error());<br>
> > > + return nullptr;<br>
> > > + }<br>
> > > +<br>
> > > + /* Read the image file to data */<br>
> > > + auto fileSize = file.size();<br>
> > > + auto buffer = std::make_unique<uint8_t[]>(file.size());<br>
> > > + if (file.read({ buffer.get(),<br>
> > static_cast<size_t>(fileSize) }) != fileSize) {<br>
> > > + LOG(Virtual, Error) << "Failed to read file " <<<br>
> > file.fileName()<br>
> > > + << ": " <<<br>
> > strerror(file.error());<br>
> > > + return nullptr;<br>
> > > + }<br>
> > > +<br>
> > > + /* Get the width and height of the image */<br>
> > > + int width, height;<br>
> > > + if (libyuv::MJPGSize(buffer.get(), fileSize, &width,<br>
> > &height)) {<br>
> > > + LOG(Virtual, Error) << "Failed to get the size of<br>
> > the image file: "<br>
> > > + << file.fileName();<br>
> > > + return nullptr;<br>
> > > + }<br>
> > > +<br>
> > > + /* Convert to NV12 and write the data to tmpY and tmpUV */<br>
> > > + unsigned int halfWidth = (width + 1) / 2;<br>
> > > + unsigned int halfHeight = (height + 1) / 2;<br>
> ><br>
> > Do you need this for rounding ?<br>
><br>
><br>
> > > + std::unique_ptr<uint8_t[]> dstY =<br>
> > > + std::make_unique<uint8_t[]>(width * height);<br>
> > > + std::unique_ptr<uint8_t[]> dstUV =<br>
> > > + std::make_unique<uint8_t[]>(halfWidth * halfHeight<br>
> > * 2);<br>
> ><br>
> > otherwise using width * height / 2 would do<br>
> ><br>
><br>
> Right, it works. Thanks!<br>
><br>
<br>
You probable need to take care about rounding, if you accept sizes not<br>
2-pixels aligned in your pipeline.<br></blockquote><div><br></div><div>The parser only accepts an even number for width, and I've tried an odd</div><div>number for height, which still works.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
><br>
> ><br>
> > > + int ret = libyuv::MJPGToNV12(buffer.get(), fileSize,<br>
> > > + dstY.get(), width,<br>
> > dstUV.get(),<br>
> > > + width, width, height, width,<br>
> > height);<br>
> > > + if (ret != 0)<br>
> > > + LOG(Virtual, Error) << "MJPGToNV12() failed with "<br>
> > << ret;<br>
> > > +<br>
> > > + imageFrameGenerator->imageFrameDatas_.emplace_back(<br>
> > > + ImageFrameData{ std::move(dstY), std::move(dstUV),<br>
> > > + Size(width, height) });<br>
> > > + }<br>
> > > +<br>
> > > + return imageFrameGenerator;<br>
> > > +}<br>
> > > +<br>
> > > +/* Scale the buffers for image frames. */<br>
> > > +void ImageFrameGenerator::configure(const Size &size)<br>
> > > +{<br>
> > > + /* Reset the source images to prevent multiple configuration calls<br>
> > */<br>
> > > + scaledFrameDatas_.clear();<br>
> > > + frameCount_ = 0;<br>
> > > + parameter_ = 0;<br>
> > > +<br>
> > > + for (unsigned int i = 0; i < imageFrames_->number.value_or(1);<br>
> > i++) {<br>
> > > + /* Scale the imageFrameDatas_ to scaledY and scaledUV */<br>
> > > + unsigned int halfSizeWidth = (size.width + 1) / 2;<br>
> > > + unsigned int halfSizeHeight = (size.height + 1) / 2;<br>
> > > + std::unique_ptr<uint8_t[]> scaledY =<br>
> > > + std::make_unique<uint8_t[]>(size.width *<br>
> > size.height);<br>
> > > + std::unique_ptr<uint8_t[]> scaledUV =<br>
> > > + std::make_unique<uint8_t[]>(halfSizeWidth *<br>
> > halfSizeHeight * 2);<br>
> > > + auto &src = imageFrameDatas_[i];<br>
> > > +<br>
> > > + /*<br>
> > > + * \todo Some platforms might enforce stride due to GPU,<br>
> > like<br>
> > > + * ChromeOS ciri (64). The weight needs to be a multiple of<br>
> > > + * the stride to work properly for now.<br>
> > > + */<br>
> > > + libyuv::NV12Scale(src.Y.get(), src.size.width,<br>
> > > + src.UV.get(), src.size.width,<br>
> > > + src.size.width, src.size.height,<br>
> > > + scaledY.get(), size.width,<br>
> > scaledUV.get(), size.width,<br>
> > > + size.width, size.height,<br>
> > libyuv::FilterMode::kFilterBilinear);<br>
> > > +<br>
> > > + scaledFrameDatas_.emplace_back(<br>
> > > + ImageFrameData{ std::move(scaledY),<br>
> > std::move(scaledUV), size });<br>
> > > + }<br>
> > > +}<br>
> > > +<br>
> > > +void ImageFrameGenerator::generateFrame(const Size &size, const<br>
> > FrameBuffer *buffer)<br>
> > > +{<br>
> > > + /* Don't do anything when the list of buffers is empty*/<br>
> > > + ASSERT(!scaledFrameDatas_.empty());<br>
> > > +<br>
> > > + MappedFrameBuffer mappedFrameBuffer(buffer,<br>
> > MappedFrameBuffer::MapFlag::Write);<br>
> > > +<br>
> > > + auto planes = mappedFrameBuffer.planes();<br>
> > > +<br>
> > > + /* Make sure the frameCount does not over the number of images */<br>
> > > + frameCount_ %= imageFrames_->number.value_or(1);<br>
> > > +<br>
> > > + /* Write the scaledY and scaledUV to the mapped frame buffer */<br>
> > > + libyuv::NV12Copy(scaledFrameDatas_[frameCount_].Y.get(),<br>
> > size.width,<br>
> > > + scaledFrameDatas_[frameCount_].UV.get(),<br>
> > size.width, planes[0].begin(),<br>
> > > + size.width, planes[1].begin(), size.width,<br>
> > > + size.width, size.height);<br>
> > > +<br>
> > > + /* proceed an image every 4 frames */<br>
> > > + /* \todo read the parameter_ from the configuration file? */<br>
> > > + parameter_++;<br>
> > > + if (parameter_ % 4 == 0)<br>
> ><br>
> > Make this a constexpr if not configurable<br>
> ><br>
> ><br>
> Done.<br>
><br>
><br>
> > > + frameCount_++;<br>
> > > +}<br>
> > > +<br>
> > > +/**<br>
> ><br>
> > We don't doxygen the pipelines, but doesn't hurt to have comments.<br>
> > Maybe remove the /**<br>
> ><br>
> ><br>
> Make them start with `/*` you mean?<br>
><br>
<br>
Yep!<br>
<br>
><br>
> ><br>
> > > + * \var ImageFrameGenerator::imageFrameDatas_<br>
> > > + * \brief List of pointers to the not scaled image buffers<br>
> > > + */<br>
> > > +<br>
> > > +/**<br>
> > > + * \var ImageFrameGenerator::scaledFrameDatas_<br>
> > > + * \brief List of pointers to the scaled image buffers<br>
> > > + */<br>
> > > +<br>
> > > +/**<br>
> > > + * \var ImageFrameGenerator::imageFrames_<br>
> > > + * \brief Pointer to the imageFrames_ in VirtualCameraData<br>
> > > + */<br>
> > > +<br>
> > > +/**<br>
> > > + * \var ImageFrameGenerator::parameter_<br>
> > > + * \brief Speed parameter. Change to the next image every parameter_<br>
> > frames<br>
> > > + */<br>
> > > +<br>
> > > +} /* namespace libcamera */<br>
> > > diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.h<br>
> > b/src/libcamera/pipeline/virtual/image_frame_generator.h<br>
> > > new file mode 100644<br>
> > > index 00000000..4ad8aad2<br>
> > > --- /dev/null<br>
> > > +++ b/src/libcamera/pipeline/virtual/image_frame_generator.h<br>
> > > @@ -0,0 +1,54 @@<br>
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */<br>
> > > +/*<br>
> > > + * Copyright (C) 2024, Google Inc.<br>
> > > + *<br>
> > > + * image_frame_generator.h - Derived class of FrameGenerator for<br>
> > > + * generating frames from images<br>
> > > + */<br>
> > > +<br>
> > > +#pragma once<br>
> > > +<br>
> > > +#include <filesystem><br>
> > > +#include <memory><br>
> > > +#include <optional><br>
> > > +#include <stdint.h><br>
> > > +#include <sys/types.h><br>
> > > +<br>
> > > +#include "frame_generator.h"<br>
> > > +<br>
> > > +namespace libcamera {<br>
> > > +<br>
> > > +enum class ScaleMode : char {<br>
> > > + Fill = 0,<br>
> > > +};<br>
> ><br>
> > I know you want more scale modes, but as long as you only support one<br>
> > there's no need for a type<br>
> ><br>
><br>
> Hmm, do you think it's better to just remove the argument in the config<br>
> file?<br>
><br>
<br>
What do you think ? if you can't configure in this version I would<br>
leave it out and simplify the implementation for this first version<br></blockquote><div><br></div><div>Yeah, will remove it completely in v12.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
><br>
> ><br>
> > > +<br>
> > > +/* Frame configuration provided by the config file */<br>
> > > +struct ImageFrames {<br>
> > > + std::filesystem::path path;<br>
> > > + ScaleMode scaleMode;<br>
> ><br>
> > As this can only be "Fill", nothing else is valid atm<br>
> ><br>
> > > + std::optional<unsigned int> number;<br>
> > > +};<br>
> > > +<br>
> > > +class ImageFrameGenerator : public FrameGenerator<br>
> > > +{<br>
> > > +public:<br>
> > > + static std::unique_ptr<ImageFrameGenerator> create(ImageFrames<br>
> > &imageFrames);<br>
> > > +<br>
> > > +private:<br>
> > > + struct ImageFrameData {<br>
> > > + std::unique_ptr<uint8_t[]> Y;<br>
> > > + std::unique_ptr<uint8_t[]> UV;<br>
> > > + Size size;<br>
> > > + };<br>
> > > +<br>
> > > + void configure(const Size &size) override;<br>
> > > + void generateFrame(const Size &size, const FrameBuffer *buffer)<br>
> > override;<br>
> > > +<br>
> > > + std::vector<ImageFrameData> imageFrameDatas_;<br>
> > > + std::vector<ImageFrameData> scaledFrameDatas_;<br>
> > > + ImageFrames *imageFrames_;<br>
> > > + unsigned int frameCount_;<br>
> > > + unsigned int parameter_;<br>
> > > +};<br>
> > > +<br>
> > > +} /* namespace libcamera */<br>
> > > diff --git a/src/libcamera/pipeline/virtual/meson.build<br>
> > b/src/libcamera/pipeline/virtual/meson.build<br>
> > > index d72ac5be..395919b3 100644<br>
> > > --- a/src/libcamera/pipeline/virtual/meson.build<br>
> > > +++ b/src/libcamera/pipeline/virtual/meson.build<br>
> > > @@ -1,9 +1,13 @@<br>
> > > # SPDX-License-Identifier: CC0-1.0<br>
> > ><br>
> > > libcamera_internal_sources += files([<br>
> > > + 'image_frame_generator.cpp',<br>
> > > 'parser.cpp',<br>
> > > 'test_pattern_generator.cpp',<br>
> > > 'virtual.cpp',<br>
> > > ])<br>
> > ><br>
> > > +libjpeg = dependency('libjpeg', required : false)<br>
> > > +<br>
> > > libcamera_deps += [libyuv_dep]<br>
> > > +libcamera_deps += [libjpeg]<br>
> > > diff --git a/src/libcamera/pipeline/virtual/parser.cpp<br>
> > b/src/libcamera/pipeline/virtual/parser.cpp<br>
> > > index d861a52a..5076e71c 100644<br>
> > > --- a/src/libcamera/pipeline/virtual/parser.cpp<br>
> > > +++ b/src/libcamera/pipeline/virtual/parser.cpp<br>
> > > @@ -52,12 +52,12 @@ Parser::parseConfigFile(File &file, PipelineHandler<br>
> > *pipe)<br>
> > > continue;<br>
> > > }<br>
> > ><br>
> > > - data->id_ = cameraId;<br>
> > > + data->config_.id = cameraId;<br>
> > > ControlInfoMap::Map controls;<br>
> > > /* todo: Check which resolution's frame rate to be<br>
> > reported */<br>
> > > controls[&controls::FrameDurationLimits] =<br>
> > > - ControlInfo(int64_t(1000000 /<br>
> > data->supportedResolutions_[0].frameRates[1]),<br>
> > > - int64_t(1000000 /<br>
> > data->supportedResolutions_[0].frameRates[0]));<br>
> > > + ControlInfo(int64_t(1000000 /<br>
> > data->config_.resolutions[0].frameRates[1]),<br>
> > > + int64_t(1000000 /<br>
> > data->config_.resolutions[0].frameRates[0]));<br>
> > > data->controlInfo_ = ControlInfoMap(std::move(controls),<br>
> > controls::controls);<br>
> > > configurations.push_back(std::move(data));<br>
> > > }<br>
> > > @@ -75,7 +75,8 @@ Parser::parseCameraConfigData(const YamlObject<br>
> > &cameraConfigData,<br>
> > > std::unique_ptr<VirtualCameraData> data =<br>
> > > std::make_unique<VirtualCameraData>(pipe, resolutions);<br>
> > ><br>
> > > - if (parseTestPattern(cameraConfigData, data.get()))<br>
> > > + if (parseTestPattern(cameraConfigData, data.get()) &&<br>
> > > + parseFrame(cameraConfigData, data.get()))<br>
> ><br>
> > This is fragile and only works because if parseTestPattern() returns 0<br>
> > then parseFrame() is never called<br>
><br>
><br>
> > > return nullptr;<br>
> > ><br>
> > > if (parseLocation(cameraConfigData, data.get()))<br>
> > > @@ -148,16 +149,75 @@ int Parser::parseTestPattern(const YamlObject<br>
> > &cameraConfigData, VirtualCameraDa<br>
> > > {<br>
> > > std::string testPattern =<br>
> > cameraConfigData["test_pattern"].get<std::string>("");<br>
> > ><br>
> > > - /* Default value is "bars" */<br>
> > > if (testPattern == "bars") {<br>
> > > - data->testPattern_ = TestPattern::ColorBars;<br>
> > > + data->config_.frame = TestPattern::ColorBars;<br>
> > > } else if (testPattern == "lines") {<br>
> > > - data->testPattern_ = TestPattern::DiagonalLines;<br>
> > > + data->config_.frame = TestPattern::DiagonalLines;<br>
> > > } else {<br>
> > > - LOG(Virtual, Error) << "Test pattern: " << testPattern<br>
> > > + LOG(Virtual, Debug) << "Test pattern: " << testPattern<br>
> > > << "is not supported";<br>
> > > return -EINVAL;<br>
> > > }<br>
> > > +<br>
> > > + return 0;<br>
> > > +}<br>
> > > +<br>
> > > +int Parser::parseFrame(const YamlObject &cameraConfigData,<br>
> > VirtualCameraData *data)<br>
> ><br>
> > You can unify these two functions.<br>
> ><br>
> > Get "testPattern", if valid make sure it's only either "bars" or<br>
> > "lines" and fail if it is specified but has an unsupported value.<br>
> ><br>
> > Then check for "frames". If both "frames" and "testPattern" are<br>
> > specified, it's an error. Then parse "frames" like you do here.<br>
> ><br>
><br>
> Makes sense. Thanks! Updated.<br>
><br>
><br>
> ><br>
> > > +{<br>
> > > + const YamlObject &frames = cameraConfigData["frames"];<br>
> > > +<br>
> > > + /* When there is no frames provided in the config file, use color<br>
> > bar test pattern */<br>
> > > + if (frames.size() == 0) {<br>
> > > + data->config_.frame = TestPattern::ColorBars;<br>
> > > + return 0;<br>
> > > + }<br>
> > > +<br>
> > > + if (!frames.isDictionary()) {<br>
> > > + LOG(Virtual, Error) << "'frames' is not a dictionary.";<br>
> > > + return -EINVAL;<br>
> > > + }<br>
> > > +<br>
> > > + std::string path = frames["path"].get<std::string>("");<br>
> > > +<br>
> > > + ScaleMode scaleMode;<br>
> > > + if (auto ext = std::filesystem::path(path).extension();<br>
> > > + ext == ".jpg" || ext == ".jpeg") {<br>
> > > + if (parseScaleMode(frames, &scaleMode))<br>
> > > + return -EINVAL;<br>
> > > + data->config_.frame = ImageFrames{ path, scaleMode,<br>
> > std::nullopt };<br>
> > > + } else if<br>
> > (std::filesystem::is_directory(std::filesystem::symlink_status(path))) {<br>
> > > + if (parseScaleMode(frames, &scaleMode))<br>
> > > + return -EINVAL;<br>
> ><br>
> > Could you parse scale mode before checking the file extensions ?<br>
> ><br>
> ><br>
> Done.<br>
><br>
><br>
> > > +<br>
> > > + using std::filesystem::directory_iterator;<br>
> > > + unsigned int numOfFiles =<br>
> > std::distance(directory_iterator(path), directory_iterator{});<br>
> > > + if (numOfFiles == 0) {<br>
> > > + LOG(Virtual, Error) << "Empty directory";<br>
> > > + return -EINVAL;<br>
> > > + }<br>
> > > + data->config_.frame = ImageFrames{ path, scaleMode,<br>
> > numOfFiles };<br>
> > > + } else {<br>
> > > + LOG(Virtual, Error) << "Frame: " << path << " is not<br>
> > supported";<br>
> > > + return -EINVAL;<br>
> > > + }<br>
> > > +<br>
> > > + return 0;<br>
> > > +}<br>
> > > +<br>
> > > +int Parser::parseScaleMode(<br>
> > > + const YamlObject &framesConfigData, ScaleMode *scaleMode)<br>
> ><br>
> > weird indent<br>
> ><br>
><br>
> Updated.<br>
><br>
><br>
> ><br>
> > > +{<br>
> > > + std::string mode =<br>
> > framesConfigData["scale_mode"].get<std::string>("");<br>
> > > +<br>
> > > + /* Default value is fill */<br>
> > > + if (mode == "fill" || mode == "") {<br>
> > > + *scaleMode = ScaleMode::Fill;<br>
> ><br>
> > You don't need a type as suggested above, you can either assume "Fill"<br>
> > or fail during parsing.<br>
> ><br>
> > > + } else {<br>
> > > + LOG(Virtual, Error) << "scaleMode: " << mode<br>
> > > + << " is not supported";<br>
> > > + return -EINVAL;<br>
> > > + }<br>
> > > +<br>
> > > return 0;<br>
> > > }<br>
> > ><br>
> > > diff --git a/src/libcamera/pipeline/virtual/parser.h<br>
> > b/src/libcamera/pipeline/virtual/parser.h<br>
> > > index 09c3c56b..f65616e3 100644<br>
> > > --- a/src/libcamera/pipeline/virtual/parser.h<br>
> > > +++ b/src/libcamera/pipeline/virtual/parser.h<br>
> > > @@ -35,8 +35,10 @@ private:<br>
> > > int parseSupportedFormats(const YamlObject &cameraConfigData,<br>
> > ><br>
> > std::vector<VirtualCameraData::Resolution> *resolutions);<br>
> > > int parseTestPattern(const YamlObject &cameraConfigData,<br>
> > VirtualCameraData *data);<br>
> > > + int parseFrame(const YamlObject &cameraConfigData,<br>
> > VirtualCameraData *data);<br>
> > > int parseLocation(const YamlObject &cameraConfigData,<br>
> > VirtualCameraData *data);<br>
> > > int parseModel(const YamlObject &cameraConfigData,<br>
> > VirtualCameraData *data);<br>
> > > + int parseScaleMode(const YamlObject &framesConfigData, ScaleMode<br>
> > *scaleMode);<br>
> > > };<br>
> > ><br>
> > > } /* namespace libcamera */<br>
> > > diff --git a/src/libcamera/pipeline/virtual/utils.h<br>
> > b/src/libcamera/pipeline/virtual/utils.h<br>
> > > new file mode 100644<br>
> > > index 00000000..43a14d4b<br>
> > > --- /dev/null<br>
> > > +++ b/src/libcamera/pipeline/virtual/utils.h<br>
> > > @@ -0,0 +1,17 @@<br>
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */<br>
> > > +/*<br>
> > > + * Copyright (C) 2024, Google Inc.<br>
> > > + *<br>
> > > + * utils.h - Utility types for Virtual Pipeline Handler<br>
> > > + */<br>
> > > +<br>
> > > +namespace libcamera {<br>
> > > +<br>
> > > +template<class... Ts><br>
> > > +struct overloaded : Ts... {<br>
> > > + using Ts::operator()...;<br>
> > > +};<br>
> > > +template<class... Ts><br>
> > > +overloaded(Ts...) -> overloaded<Ts...>;<br>
> > > +<br>
> ><br>
> > Does using a standard construct like std::visit a custom definition<br>
> ><br>
><br>
> Sorry, I don't get what you mean.<br>
<br>
I was wondering why you need this to use a construct from stdlib, then<br>
I read the example at<br>
<a href="https://en.cppreference.com/w/cpp/utility/variant/visit" rel="noreferrer" target="_blank">https://en.cppreference.com/w/cpp/utility/variant/visit</a></blockquote><div><br></div><div>Yeah, this is exactly where I copied the code :p</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
<br>
><br>
><br>
> > like this one ? Also, this header is included form a single cpp file,<br>
> > move its content there if you can.<br>
> ><br>
> ><br>
> Hmm, I was suggested otherwise:<br>
><br>
<br>
I see, but what you think ? What is the point of an header file<br>
without any other file including it but one, with just this small<br>
helpers above ?<br></blockquote><div><br></div><div>Yeah I do think that putting it in the source makes more sense.</div><div>Will update in v12.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
<br>
> <a href="https://patchwork.libcamera.org/patch/20974/" rel="noreferrer" target="_blank">https://patchwork.libcamera.org/patch/20974/</a><br>
> ```<br>
><br>
> Apart from the indentation looking a bit off, it seems ok.<br>
><br>
> I think the `overloaded` type could go into `utils.h` or similar.<br>
><br>
><br>
> Regards,<br>
> Barnabás Pőcze<br>
><br>
> ```<br>
><br>
> I'm new to std::visit, so...<br>
><br>
<br>
Not very much related to std::visit but the usage of an header when<br>
imho it shouldn't be necessary<br>
<br>
><br>
> ><br>
> > > +} /* namespace libcamera */<br>
> > > diff --git a/src/libcamera/pipeline/virtual/virtual.cpp<br>
> > b/src/libcamera/pipeline/virtual/virtual.cpp<br>
> > > index 55bc30df..98aed412 100644<br>
> > > --- a/src/libcamera/pipeline/virtual/virtual.cpp<br>
> > > +++ b/src/libcamera/pipeline/virtual/virtual.cpp<br>
> > > @@ -27,6 +27,7 @@<br>
> > > #include "libcamera/internal/yaml_parser.h"<br>
> > ><br>
> > > #include "parser.h"<br>
> > > +#include "utils.h"<br>
> > ><br>
> > > namespace libcamera {<br>
> > ><br>
> > > @@ -49,17 +50,18 @@ uint64_t currentTimestamp()<br>
> > ><br>
> > > VirtualCameraData::VirtualCameraData(PipelineHandler *pipe,<br>
> > > std::vector<Resolution><br>
> > supportedResolutions)<br>
> > > - : Camera::Private(pipe),<br>
> > supportedResolutions_(std::move(supportedResolutions))<br>
> > > + : Camera::Private(pipe)<br>
> > > {<br>
> > > - for (const auto &resolution : supportedResolutions_) {<br>
> > > - if (minResolutionSize_.isNull() || minResolutionSize_ ><br>
> > resolution.size)<br>
> > > - minResolutionSize_ = resolution.size;<br>
> > > + config_.resolutions = std::move(supportedResolutions);<br>
> > > + for (const auto &resolution : config_.resolutions) {<br>
> > > + if (config_.minResolutionSize.isNull() ||<br>
> > config_.minResolutionSize > resolution.size)<br>
> > > + config_.minResolutionSize = resolution.size;<br>
> > ><br>
> > > - maxResolutionSize_ = std::max(maxResolutionSize_,<br>
> > resolution.size);<br>
> > > + config_.maxResolutionSize =<br>
> > std::max(config_.maxResolutionSize, resolution.size);<br>
> > > }<br>
> > ><br>
> > > properties_.set(properties::PixelArrayActiveAreas,<br>
> > > - { Rectangle(maxResolutionSize_) });<br>
> > > + { Rectangle(config_.maxResolutionSize) });<br>
> > ><br>
> > > /* \todo Support multiple streams and pass multi_stream_test */<br>
> > > streamConfigs_.resize(kMaxStream);<br>
> > > @@ -87,7 +89,7 @@ CameraConfiguration::Status<br>
> > VirtualCameraConfiguration::validate()<br>
> > ><br>
> > > for (StreamConfiguration &cfg : config_) {<br>
> > > bool found = false;<br>
> > > - for (const auto &resolution :<br>
> > data_->supportedResolutions_) {<br>
> > > + for (const auto &resolution : data_->config_.resolutions) {<br>
> > > if (resolution.size.width == cfg.size.width &&<br>
> > > resolution.size.height == cfg.size.height) {<br>
> > > found = true;<br>
> > > @@ -102,7 +104,7 @@ CameraConfiguration::Status<br>
> > VirtualCameraConfiguration::validate()<br>
> > > * Defining the default logic in PipelineHandler to<br>
> > > * find the closest resolution would be nice.<br>
> > > */<br>
> > > - cfg.size = data_->maxResolutionSize_;<br>
> > > + cfg.size = data_->config_.maxResolutionSize;<br>
> > > status = Adjusted;<br>
> > > }<br>
> > ><br>
> > > @@ -145,11 +147,11 @@<br>
> > PipelineHandlerVirtual::generateConfiguration(Camera *camera,<br>
> > > for (const StreamRole role : roles) {<br>
> > > std::map<PixelFormat, std::vector<SizeRange>><br>
> > streamFormats;<br>
> > > PixelFormat pixelFormat = formats::NV12;<br>
> > > - streamFormats[pixelFormat] = { { data->minResolutionSize_,<br>
> > data->maxResolutionSize_ } };<br>
> > > + streamFormats[pixelFormat] = { {<br>
> > data->config_.minResolutionSize, data->config_.maxResolutionSize } };<br>
> > > StreamFormats formats(streamFormats);<br>
> > > StreamConfiguration cfg(formats);<br>
> > > cfg.pixelFormat = pixelFormat;<br>
> > > - cfg.size = data->maxResolutionSize_;<br>
> > > + cfg.size = data->config_.maxResolutionSize;<br>
> > > cfg.bufferCount = VirtualCameraConfiguration::kBufferCount;<br>
> > ><br>
> > > switch (role) {<br>
> > > @@ -181,6 +183,7 @@ int PipelineHandlerVirtual::configure(Camera *camera,<br>
> > > VirtualCameraData *data = cameraData(camera);<br>
> > > for (size_t i = 0; i < config->size(); ++i) {<br>
> > > config->at(i).setStream(&data->streamConfigs_[i].stream);<br>
> > > + /* Start reading the images/generating test patterns */<br>
> > > data->streamConfigs_[i].frameGenerator->configure(<br>
> > ><br>
> > data->streamConfigs_[i].stream.configuration().size);<br>
> > > }<br>
> > > @@ -274,10 +277,14 @@ bool<br>
> > PipelineHandlerVirtual::match([[maybe_unused]] DeviceEnumerator *enumerator<br>
> > > std::set<Stream *> streams;<br>
> > > for (auto &streamConfig : data->streamConfigs_)<br>
> > > streams.insert(&streamConfig.stream);<br>
> > > - std::string id = data->id_;<br>
> > > + std::string id = data->config_.id;<br>
> > > std::shared_ptr<Camera> camera =<br>
> > Camera::create(std::move(data), id, streams);<br>
> > ><br>
> > > - initFrameGenerator(camera.get());<br>
> > > + if (!initFrameGenerator(camera.get())) {<br>
> > > + LOG(Virtual, Error) << "Failed to initialize frame<br>
> > "<br>
> > > + << "generator for camera: " <<<br>
> > id;<br>
> > > + continue;<br>
> > > + }<br>
> > ><br>
> > > registerCamera(std::move(camera));<br>
> > > }<br>
> > > @@ -285,15 +292,30 @@ bool<br>
> > PipelineHandlerVirtual::match([[maybe_unused]] DeviceEnumerator *enumerator<br>
> > > return true;<br>
> > > }<br>
> > ><br>
> > > -void PipelineHandlerVirtual::initFrameGenerator(Camera *camera)<br>
> > > +bool PipelineHandlerVirtual::initFrameGenerator(Camera *camera)<br>
> > > {<br>
> > > auto data = cameraData(camera);<br>
> > > - for (auto &streamConfig : data->streamConfigs_) {<br>
> > > - if (data->testPattern_ == TestPattern::DiagonalLines)<br>
> > > - streamConfig.frameGenerator =<br>
> > std::make_unique<DiagonalLinesGenerator>();<br>
> > > - else<br>
> > > - streamConfig.frameGenerator =<br>
> > std::make_unique<ColorBarsGenerator>();<br>
> > > - }<br>
> > > + auto &frame = data->config_.frame;<br>
> > > + std::visit(overloaded{<br>
> > > + [&](TestPattern &testPattern) {<br>
> > > + for (auto &streamConfig :<br>
> > data->streamConfigs_) {<br>
> > > + if (testPattern ==<br>
> > TestPattern::DiagonalLines)<br>
> > > +<br>
> > streamConfig.frameGenerator = std::make_unique<DiagonalLinesGenerator>();<br>
> > > + else<br>
> > > +<br>
> > streamConfig.frameGenerator = std::make_unique<ColorBarsGenerator>();<br>
> > > + }<br>
> > > + },<br>
> > > + [&](ImageFrames &imageFrames) {<br>
> > > + for (auto &streamConfig :<br>
> > data->streamConfigs_)<br>
> > > + streamConfig.frameGenerator =<br>
> > ImageFrameGenerator::create(imageFrames);<br>
> > > + } },<br>
> > > + frame);<br>
> > > +<br>
> > > + for (auto &streamConfig : data->streamConfigs_)<br>
> > > + if (!streamConfig.frameGenerator)<br>
> > > + return false;<br>
> > > +<br>
> > > + return true;<br>
> > > }<br>
> > ><br>
> > > REGISTER_PIPELINE_HANDLER(PipelineHandlerVirtual, "virtual")<br>
> > > diff --git a/src/libcamera/pipeline/virtual/virtual.h<br>
> > b/src/libcamera/pipeline/virtual/virtual.h<br>
> > > index 8830e00f..efa97e88 100644<br>
> > > --- a/src/libcamera/pipeline/virtual/virtual.h<br>
> > > +++ b/src/libcamera/pipeline/virtual/virtual.h<br>
> > > @@ -8,6 +8,8 @@<br>
> > > #pragma once<br>
> > ><br>
> > > #include <memory><br>
> > > +#include <string><br>
> > > +#include <variant><br>
> > > #include <vector><br>
> > ><br>
> > > #include <libcamera/base/file.h><br>
> > > @@ -16,10 +18,14 @@<br>
> > > #include "libcamera/internal/dma_buf_allocator.h"<br>
> > > #include "libcamera/internal/pipeline_handler.h"<br>
> > ><br>
> > > +#include "frame_generator.h"<br>
> > > +#include "image_frame_generator.h"<br>
> > > #include "test_pattern_generator.h"<br>
> > ><br>
> > > namespace libcamera {<br>
> > ><br>
> > > +using VirtualFrame = std::variant<TestPattern, ImageFrames>;<br>
> > > +<br>
> > > class VirtualCameraData : public Camera::Private<br>
> > > {<br>
> > > public:<br>
> > > @@ -33,18 +39,22 @@ public:<br>
> > > Stream stream;<br>
> > > std::unique_ptr<FrameGenerator> frameGenerator;<br>
> > > };<br>
> > > + /* The config file is parsed to the Configuration struct */<br>
> > > + struct Configuration {<br>
> > > + std::string id;<br>
> > > + std::vector<Resolution> resolutions;<br>
> > > + VirtualFrame frame;<br>
> > > +<br>
> > > + Size maxResolutionSize;<br>
> > > + Size minResolutionSize;<br>
> > > + };<br>
> > ><br>
> > > VirtualCameraData(PipelineHandler *pipe,<br>
> > > std::vector<Resolution> supportedResolutions);<br>
> > ><br>
> > > ~VirtualCameraData() = default;<br>
> > ><br>
> > > - std::string id_;<br>
> > > - TestPattern testPattern_ = TestPattern::ColorBars;<br>
> > > -<br>
> > > - const std::vector<Resolution> supportedResolutions_;<br>
> > > - Size maxResolutionSize_;<br>
> > > - Size minResolutionSize_;<br>
> > > + Configuration config_;<br>
> > ><br>
> > > std::vector<StreamConfig> streamConfigs_;<br>
> > > };<br>
> > > @@ -89,7 +99,7 @@ private:<br>
> > > return static_cast<VirtualCameraData *>(camera->_d());<br>
> > > }<br>
> > ><br>
> > > - void initFrameGenerator(Camera *camera);<br>
> > > + bool initFrameGenerator(Camera *camera);<br>
> > ><br>
> > > DmaBufAllocator dmaBufAllocator_;<br>
> > > };<br>
> > > --<br>
> > > 2.46.0.469.g59c65b2a67-goog<br>
> > ><br>
> ><br>
</blockquote></div></div>