[PATCH v14 5/7] libcamera: virtual: Add ImageFrameGenerator
Cheng-Hao Yang
chenghaoyang at chromium.org
Fri Oct 4 12:00:34 CEST 2024
Hi Kieran,
On Thu, Oct 3, 2024 at 6:32 PM Kieran Bingham
<kieran.bingham at ideasonboard.com> wrote:
>
> Quoting Harvey Yang (2024-09-30 07:29:46)
> > Besides TestPatternGenerator, this patch adds ImageFrameGenerator that
> > loads real images (jpg / jpeg for now) as the source and generates
> > scaled frames.
> >
> > 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>
> > Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> > ---
> > .../virtual/image_frame_generator.cpp | 175 ++++++++++++++++++
> > .../pipeline/virtual/image_frame_generator.h | 51 +++++
> > src/libcamera/pipeline/virtual/meson.build | 4 +
> > 3 files changed, 230 insertions(+)
> > 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/image_frame_generator.cpp b/src/libcamera/pipeline/virtual/image_frame_generator.cpp
> > new file mode 100644
> > index 00000000..a1915b24
> > --- /dev/null
> > +++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp
> > @@ -0,0 +1,175 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2024, Google Inc.
> > + *
> > + * image_frame_generator.cpp - Derived class of FrameGenerator for
> > + * generating frames from images
> > + */
> > +
> > +#include "image_frame_generator.h"
> > +
> > +#include <string>
> > +
> > +#include <libcamera/base/file.h>
> > +#include <libcamera/base/log.h>
> > +
> > +#include <libcamera/framebuffer.h>
> > +
> > +#include "libcamera/internal/mapped_framebuffer.h"
> > +
> > +#include "libyuv/convert.h"
> > +#include "libyuv/scale.h"
> > +
> > +namespace libcamera {
> > +
> > +LOG_DECLARE_CATEGORY(Virtual)
> > +
> > +/*
> > + * Factory function to create an ImageFrameGenerator object.
> > + * Read the images and convert them to buffers in NV12 format.
> > + * Store the pointers to the buffers to a list (imageFrameDatas)
> > + */
> > +std::unique_ptr<ImageFrameGenerator>
> > +ImageFrameGenerator::create(ImageFrames &imageFrames)
> > +{
> > + std::unique_ptr<ImageFrameGenerator> imageFrameGenerator =
> > + std::make_unique<ImageFrameGenerator>();
> > + imageFrameGenerator->imageFrames_ = &imageFrames;
> > +
> > + /*
> > + * For each file in the directory, load the image,
> > + * convert it to NV12, and store the pointer.
>
> What happens if the directory has 200 images? Will this load all images
> into memory?
Currently, yes.
>
> Should we impose any limits? Or would you see issues there?
We can either have a limit of file size or number of files, while I think
we can keep it as is for now, when we have more tests depending on
virtual pipeline handler. Over-designing might take much more time
than needed here IMO.
Another idea: We can also pre-load a certain amount of files, and
drop some images to save memory usage.
>
>
> > + */
> > + for (unsigned int i = 0; i < imageFrames.number.value_or(1); i++) {
> > + std::filesystem::path path;
> > + if (!imageFrames.number)
> > + /* If the path is to an image */
> > + path = imageFrames.path;
> > + else
> > + /* If the path is to a directory */
> > + path = imageFrames.path / (std::to_string(i) + ".jpg");
> > +
> > + File file(path);
> > + if (!file.open(File::OpenModeFlag::ReadOnly)) {
> > + LOG(Virtual, Error) << "Failed to open image file " << file.fileName()
> > + << ": " << strerror(file.error());
> > + return nullptr;
> > + }
> > +
> > + /* Read the image file to data */
> > + auto fileSize = file.size();
> > + auto buffer = std::make_unique<uint8_t[]>(file.size());
>
> not fileSize?
Ah right. Updated.
>
> > + if (file.read({ buffer.get(), static_cast<size_t>(fileSize) }) != fileSize) {
> > + LOG(Virtual, Error) << "Failed to read file " << file.fileName()
> > + << ": " << strerror(file.error());
> > + return nullptr;
> > + }
> > +
> > + /* Get the width and height of the image */
> > + int width, height;
>
> unsigned int?
No, as `libyuv::MJPGToNV12` takes `int` instead of `unsigned int`.
>
> > + if (libyuv::MJPGSize(buffer.get(), fileSize, &width, &height)) {
> > + LOG(Virtual, Error) << "Failed to get the size of the image file: "
> > + << file.fileName();
> > + return nullptr;
> > + }
> > +
> > + std::unique_ptr<uint8_t[]> dstY =
> > + std::make_unique<uint8_t[]>(width * height);
> > + std::unique_ptr<uint8_t[]> dstUV =
> > + std::make_unique<uint8_t[]>(width * height / 2);
> > + int ret = libyuv::MJPGToNV12(buffer.get(), fileSize,
> > + dstY.get(), width, dstUV.get(),
> > + width, width, height, width, height);
> > + if (ret != 0)
> > + LOG(Virtual, Error) << "MJPGToNV12() failed with " << ret;
> > +
> > + imageFrameGenerator->imageFrameDatas_.emplace_back(
> > + ImageFrameData{ std::move(dstY), std::move(dstUV),
> > + Size(width, height) });
> > + }
> > +
> > + return imageFrameGenerator;
> > +}
> > +
> > +/* Scale the buffers for image frames. */
> > +void ImageFrameGenerator::configure(const Size &size)
> > +{
> > + /* Reset the source images to prevent multiple configuration calls */
> > + scaledFrameDatas_.clear();
> > + frameCount_ = 0;
> > + parameter_ = 0;
> > +
> > + for (unsigned int i = 0; i < imageFrames_->number.value_or(1); i++) {
> > + /* Scale the imageFrameDatas_ to scaledY and scaledUV */
> > + unsigned int halfSizeWidth = (size.width + 1) / 2;
> > + unsigned int halfSizeHeight = (size.height + 1) / 2;
> > + std::unique_ptr<uint8_t[]> scaledY =
> > + std::make_unique<uint8_t[]>(size.width * size.height);
> > + std::unique_ptr<uint8_t[]> scaledUV =
> > + std::make_unique<uint8_t[]>(halfSizeWidth * halfSizeHeight * 2);
> > + auto &src = imageFrameDatas_[i];
> > +
> > + /*
> > + * \todo Some platforms might enforce stride due to GPU, like
> > + * ChromeOS ciri (64). The weight needs to be a multiple of
>
> weight ?
Should be width. Thanks!
>
> And I think this isn't anything to do with Ciri. We don't know (or care)
> what a Ciri is inside libcamera, but we do care about externally
> allocated buffers or buffers with defined stride requirements.
>
> The stride should always be correctly considered for any output buffer
> managed by a pipeline handler.
True. Let me remove Ciri as an example for now.
>
> I wonder if we can make some specific unit tests in lc-compliance to
> validate that too.
>
>
> > + * the stride to work properly for now.
> > + */
> > + libyuv::NV12Scale(src.Y.get(), src.size.width,
> > + src.UV.get(), src.size.width,
> > + src.size.width, src.size.height,
> > + scaledY.get(), size.width, scaledUV.get(), size.width,
> > + size.width, size.height, libyuv::FilterMode::kFilterBilinear);
> > +
> > + scaledFrameDatas_.emplace_back(
> > + ImageFrameData{ std::move(scaledY), std::move(scaledUV), size });
> > + }
> > +}
> > +
> > +int ImageFrameGenerator::generateFrame(const Size &size, const FrameBuffer *buffer)
> > +{
> > + /* Don't do anything when the list of buffers is empty*/
>
> missing space between empty*/
>
> But ... well an ASSERT is definitely a way to not do anything at all
> ever again ;-)
Yeah the comment doesn't make much sense...
Let me remove it.
>
> > + ASSERT(!scaledFrameDatas_.empty());
> > +
> > + MappedFrameBuffer mappedFrameBuffer(buffer, MappedFrameBuffer::MapFlag::Write);
> > +
> > + auto planes = mappedFrameBuffer.planes();
> > +
> > + /* Make sure the frameCount does not over the number of images */
>
> /* Loop only around the number of images available */
Done
>
> > + frameCount_ %= imageFrames_->number.value_or(1);
>
> Then, I don't think this is a frameCount. It's a frameIndex_.
Right, updated.
>
> A FrameCount would be something you would set as the sequence numbers in
> the FrameBuffers.
>
> Do you set those?
No. In which member variable of FrameBuffer?
>
> > +
> > + /* 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 */
>
> /* Proceed to the next image every 4 frames */
Done
>
> > + /* \todo Consider setting the proceed interval in the config file */
>
> s/proceed interval/frameInterval/
>
> But now I wonder if it should be frameRepeat ... this isn't an interval
> is it ?
>
> > + parameter_++;
> > + if (parameter_ % frameInterval == 0)
> > + frameCount_++;
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > + * \var ImageFrameGenerator::imageFrameDatas_
> > + * \brief List of pointers to the not scaled image buffers
> > + */
> > +
> > +/*
> > + * \var ImageFrameGenerator::scaledFrameDatas_
> > + * \brief List of pointers to the scaled image buffers
> > + */
> > +
> > +/*
> > + * \var ImageFrameGenerator::imageFrames_
> > + * \brief Pointer to the imageFrames_ in VirtualCameraData
> > + */
> > +
> > +/*
> > + * \var ImageFrameGenerator::parameter_
> > + * \brief Speed parameter. Change to the next image every parameter_ frames
> > + */
> > +
> > +} /* 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 00000000..cd243816
> > --- /dev/null
> > +++ b/src/libcamera/pipeline/virtual/image_frame_generator.h
> > @@ -0,0 +1,51 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2024, Google Inc.
> > + *
> > + * image_frame_generator.h - Derived class of FrameGenerator for
> > + * generating frames from images
> > + */
> > +
> > +#pragma once
> > +
> > +#include <filesystem>
> > +#include <memory>
> > +#include <optional>
> > +#include <stdint.h>
> > +#include <sys/types.h>
> > +
> > +#include "frame_generator.h"
> > +
> > +namespace libcamera {
> > +
> > +/* Frame configuration provided by the config file */
> > +struct ImageFrames {
> > + std::filesystem::path path;
> > + std::optional<unsigned int> number;
> > +};
> > +
> > +class ImageFrameGenerator : public FrameGenerator
> > +{
> > +public:
> > + static std::unique_ptr<ImageFrameGenerator> create(ImageFrames &imageFrames);
> > +
> > +private:
>
> A brief comment here would help to say what a frameInterval is.
>
> But I don't know if it's named correctly. It's a frameRepeat from my
> understanding... I.e. it causes a loaded frame to be repeated for N
> frames.
Yeah right, frameRepeat makes more sense. Added a brief comment.
>
> > + static constexpr unsigned int frameInterval = 4;
> > +
> > + struct ImageFrameData {
> > + std::unique_ptr<uint8_t[]> Y;
> > + std::unique_ptr<uint8_t[]> UV;
> > + Size size;
> > + };
> > +
> > + void configure(const Size &size) override;
> > + int generateFrame(const Size &size, const FrameBuffer *buffer) override;
> > +
> > + std::vector<ImageFrameData> imageFrameDatas_;
> > + std::vector<ImageFrameData> scaledFrameDatas_;
> > + ImageFrames *imageFrames_;
> > + unsigned int frameCount_;
> > + unsigned int parameter_;
> > +};
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/libcamera/pipeline/virtual/meson.build b/src/libcamera/pipeline/virtual/meson.build
> > index 0c537777..bb38c193 100644
> > --- a/src/libcamera/pipeline/virtual/meson.build
> > +++ b/src/libcamera/pipeline/virtual/meson.build
> > @@ -1,8 +1,12 @@
> > # SPDX-License-Identifier: CC0-1.0
> >
> > libcamera_internal_sources += files([
> > + 'image_frame_generator.cpp',
> > 'test_pattern_generator.cpp',
> > 'virtual.cpp',
> > ])
> >
> > +libjpeg = dependency('libjpeg', required : true)
> > +
> > libcamera_deps += [libyuv_dep]
> > +libcamera_deps += [libjpeg]
> > --
> > 2.46.1.824.gd892dcdcdd-goog
> >
More information about the libcamera-devel
mailing list