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