[PATCH v9 7/8] libcamera: pipeline: Load images
Cheng-Hao Yang
chenghaoyang at chromium.org
Sat Sep 7 16:34:42 CEST 2024
Hi Jacopo,
On Sat, Aug 31, 2024 at 9:00 PM Jacopo Mondi <jacopo.mondi at 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 at 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 at chromium.org> írta:
> > > >
> > > > > From: Konami Shu <konamiz at 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 at google.com>
> > > > > Co-developed-by: Harvey Yang <chenghaoyang at chromium.org>
> > > > > Co-developed-by: Yunke Cao <yunkec at chromium.org>
> > > > > Co-developed-by: Tomasz Figa <tfiga at 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20240907/ceb96278/attachment.htm>
More information about the libcamera-devel
mailing list