[{"id":31558,"web_url":"https://patchwork.libcamera.org/comment/31558/","msgid":"<172795153430.1619946.2402640830315311386@ping.linuxembedded.co.uk>","date":"2024-10-03T10:32:14","subject":"Re: [PATCH v14 5/7] libcamera: virtual: Add ImageFrameGenerator","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Harvey Yang (2024-09-30 07:29:46)\n> Besides TestPatternGenerator, this patch adds ImageFrameGenerator that\n> loads real images (jpg / jpeg for now) as the source and generates\n> scaled frames.\n> \n> Signed-off-by: Konami Shu \n> Co-developed-by: Harvey Yang \n> Co-developed-by: Yunke Cao \n> Co-developed-by: Tomasz Figa \n> Reviewed-by: Jacopo Mondi \n> ---\n> .../virtual/image_frame_generator.cpp | 175 ++++++++++++++++++\n> .../pipeline/virtual/image_frame_generator.h | 51 +++++\n> src/libcamera/pipeline/virtual/meson.build | 4 +\n> 3 files changed, 230 insertions(+)\n> create mode 100644 src/libcamera/pipeline/virtual/image_frame_generator.cpp\n> create mode 100644 src/libcamera/pipeline/virtual/image_frame_generator.h\n> \n> diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.cpp b/src/libcamera/pipeline/virtual/image_frame_generator.cpp\n> new file mode 100644\n> index 00000000..a1915b24\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp\n> @@ -0,0 +1,175 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Google Inc.\n> + *\n> + * image_frame_generator.cpp - Derived class of FrameGenerator for\n> + * generating frames from images\n> + */\n> +\n> +#include \"image_frame_generator.h\"\n> +\n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \"libcamera/internal/mapped_framebuffer.h\"\n> +\n> +#include \"libyuv/convert.h\"\n> +#include \"libyuv/scale.h\"\n> +\n> +namespace libcamera {\n> +\n> +LOG_DECLARE_CATEGORY(Virtual)\n> +\n> +/*\n> + * Factory function to create an ImageFrameGenerator object.\n> + * Read the images and convert them to buffers in NV12 format.\n> + * Store the pointers to the buffers to a list (imageFrameDatas)\n> + */\n> +std::unique_ptr\n> +ImageFrameGenerator::create(ImageFrames &imageFrames)\n> +{\n> + std::unique_ptr imageFrameGenerator =\n> + std::make_unique();\n> + imageFrameGenerator->imageFrames_ = &imageFrames;\n> +\n> + /*\n> + * For each file in the directory, load the image,\n> + * convert it to NV12, and store the pointer.\n\nWhat happens if the directory has 200 images? Will this load all images\ninto memory?\n\nShould we impose any limits? Or would you see issues there?\n\n\n> + */\n> + for (unsigned int i = 0; i < imageFrames.number.value_or(1); i++) {\n> + std::filesystem::path path;\n> + if (!imageFrames.number)\n> + /* If the path is to an image */\n> + path = imageFrames.path;\n> + else\n> + /* If the path is to a directory */\n> + path = imageFrames.path / (std::to_string(i) + \".jpg\");\n> +\n> + File file(path);\n> + if (!file.open(File::OpenModeFlag::ReadOnly)) {\n> + LOG(Virtual, Error) << \"Failed to open image file \" << file.fileName()\n> + << \": \" << strerror(file.error());\n> + return nullptr;\n> + }\n> +\n> + /* Read the image file to data */\n> + auto fileSize = file.size();\n> + auto buffer = std::make_unique(file.size());\n\nnot fileSize?\n\n> + if (file.read({ buffer.get(), static_cast(fileSize) }) != fileSize) {\n> + LOG(Virtual, Error) << \"Failed to read file \" << file.fileName()\n> + << \": \" << strerror(file.error());\n> + return nullptr;\n> + }\n> +\n> + /* Get the width and height of the image */\n> + int width, height;\n\nunsigned int?\n\n> + if (libyuv::MJPGSize(buffer.get(), fileSize, &width, &height)) {\n> + LOG(Virtual, Error) << \"Failed to get the size of the image file: \"\n> + << file.fileName();\n> + return nullptr;\n> + }\n> +\n> + std::unique_ptr dstY =\n> + std::make_unique(width * height);\n> + std::unique_ptr dstUV =\n> + std::make_unique(width * height / 2);\n> + int ret = libyuv::MJPGToNV12(buffer.get(), fileSize,\n> + dstY.get(), width, dstUV.get(),\n> + width, width, height, width, height);\n> + if (ret != 0)\n> + LOG(Virtual, Error) << \"MJPGToNV12() failed with \" << ret;\n> +\n> + imageFrameGenerator->imageFrameDatas_.emplace_back(\n> + ImageFrameData{ std::move(dstY), std::move(dstUV),\n> + Size(width, height) });\n> + }\n> +\n> + return imageFrameGenerator;\n> +}\n> +\n> +/* Scale the buffers for image frames. */\n> +void ImageFrameGenerator::configure(const Size &size)\n> +{\n> + /* Reset the source images to prevent multiple configuration calls */\n> + scaledFrameDatas_.clear();\n> + frameCount_ = 0;\n> + parameter_ = 0;\n> +\n> + for (unsigned int i = 0; i < imageFrames_->number.value_or(1); i++) {\n> + /* Scale the imageFrameDatas_ to scaledY and scaledUV */\n> + unsigned int halfSizeWidth = (size.width + 1) / 2;\n> + unsigned int halfSizeHeight = (size.height + 1) / 2;\n> + std::unique_ptr scaledY =\n> + std::make_unique(size.width * size.height);\n> + std::unique_ptr scaledUV =\n> + std::make_unique(halfSizeWidth * halfSizeHeight * 2);\n> + auto &src = imageFrameDatas_[i];\n> +\n> + /*\n> + * \\todo Some platforms might enforce stride due to GPU, like\n> + * ChromeOS ciri (64). The weight needs to be a multiple of\n\nweight ?\n\nAnd I think this isn't anything to do with Ciri. We don't know (or care)\nwhat a Ciri is inside libcamera, but we do care about externally\nallocated buffers or buffers with defined stride requirements.\n\nThe stride should always be correctly considered for any output buffer\nmanaged by a pipeline handler.\n\nI wonder if we can make some specific unit tests in lc-compliance to\nvalidate that too.\n\n\n> + * the stride to work properly for now.\n> + */\n> + libyuv::NV12Scale(src.Y.get(), src.size.width,\n> + src.UV.get(), src.size.width,\n> + src.size.width, src.size.height,\n> + scaledY.get(), size.width, scaledUV.get(), size.width,\n> + size.width, size.height, libyuv::FilterMode::kFilterBilinear);\n> +\n> + scaledFrameDatas_.emplace_back(\n> + ImageFrameData{ std::move(scaledY), std::move(scaledUV), size });\n> + }\n> +}\n> +\n> +int ImageFrameGenerator::generateFrame(const Size &size, const FrameBuffer *buffer)\n> +{\n> + /* Don't do anything when the list of buffers is empty*/\n\nmissing space between empty*/\n\nBut ... well an ASSERT is definitely a way to not do anything at all\never again ;-)\n\n> + ASSERT(!scaledFrameDatas_.empty());\n> +\n> + MappedFrameBuffer mappedFrameBuffer(buffer, MappedFrameBuffer::MapFlag::Write);\n> +\n> + auto planes = mappedFrameBuffer.planes();\n> +\n> + /* Make sure the frameCount does not over the number of images */\n\n\t/* Loop only around the number of images available */\n\n> + frameCount_ %= imageFrames_->number.value_or(1);\n\nThen, I don't think this is a frameCount. It's a frameIndex_.\n\nA FrameCount would be something you would set as the sequence numbers in\nthe FrameBuffers.\n\nDo you set those?\n\n> +\n> + /* Write the scaledY and scaledUV to the mapped frame buffer */\n> + libyuv::NV12Copy(scaledFrameDatas_[frameCount_].Y.get(), size.width,\n> + scaledFrameDatas_[frameCount_].UV.get(), size.width, planes[0].begin(),\n> + size.width, planes[1].begin(), size.width,\n> + size.width, size.height);\n> +\n> + /* Proceed an image every 4 frames */\n\n\t/* Proceed to the next image every 4 frames */\n\n> + /* \\todo Consider setting the proceed interval in the config file */\n\ns/proceed interval/frameInterval/\n\nBut now I wonder if it should be frameRepeat ... this isn't an interval\nis it ?\n\n> + parameter_++;\n> + if (parameter_ % frameInterval == 0)\n> + frameCount_++;\n> +\n> + return 0;\n> +}\n> +\n> +/*\n> + * \\var ImageFrameGenerator::imageFrameDatas_\n> + * \\brief List of pointers to the not scaled image buffers\n> + */\n> +\n> +/*\n> + * \\var ImageFrameGenerator::scaledFrameDatas_\n> + * \\brief List of pointers to the scaled image buffers\n> + */\n> +\n> +/*\n> + * \\var ImageFrameGenerator::imageFrames_\n> + * \\brief Pointer to the imageFrames_ in VirtualCameraData\n> + */\n> +\n> +/*\n> + * \\var ImageFrameGenerator::parameter_\n> + * \\brief Speed parameter. Change to the next image every parameter_ frames\n> + */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.h b/src/libcamera/pipeline/virtual/image_frame_generator.h\n> new file mode 100644\n> index 00000000..cd243816\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/virtual/image_frame_generator.h\n> @@ -0,0 +1,51 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Google Inc.\n> + *\n> + * image_frame_generator.h - Derived class of FrameGenerator for\n> + * generating frames from images\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \"frame_generator.h\"\n> +\n> +namespace libcamera {\n> +\n> +/* Frame configuration provided by the config file */\n> +struct ImageFrames {\n> + std::filesystem::path path;\n> + std::optional number;\n> +};\n> +\n> +class ImageFrameGenerator : public FrameGenerator\n> +{\n> +public:\n> + static std::unique_ptr create(ImageFrames &imageFrames);\n> +\n> +private:\n\nA brief comment here would help to say what a frameInterval is.\n\nBut I don't know if it's named correctly. It's a frameRepeat from my\nunderstanding... I.e. it causes a loaded frame to be repeated for N\nframes.\n\n> + static constexpr unsigned int frameInterval = 4;\n> +\n> + struct ImageFrameData {\n> + std::unique_ptr Y;\n> + std::unique_ptr UV;\n> + Size size;\n> + };\n> +\n> + void configure(const Size &size) override;\n> + int generateFrame(const Size &size, const FrameBuffer *buffer) override;\n> +\n> + std::vector imageFrameDatas_;\n> + std::vector scaledFrameDatas_;\n> + ImageFrames *imageFrames_;\n> + unsigned int frameCount_;\n> + unsigned int parameter_;\n> +};\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/virtual/meson.build b/src/libcamera/pipeline/virtual/meson.build\n> index 0c537777..bb38c193 100644\n> --- a/src/libcamera/pipeline/virtual/meson.build\n> +++ b/src/libcamera/pipeline/virtual/meson.build\n> @@ -1,8 +1,12 @@\n> # SPDX-License-Identifier: CC0-1.0\n> \n> libcamera_internal_sources += files([\n> + 'image_frame_generator.cpp',\n> 'test_pattern_generator.cpp',\n> 'virtual.cpp',\n> ])\n> \n> +libjpeg = dependency('libjpeg', required : true)\n> +\n> libcamera_deps += [libyuv_dep]\n> +libcamera_deps += [libjpeg]\n> -- \n> 2.46.1.824.gd892dcdcdd-goog\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 8B2B0BD80A\n\tfor ;\n\tThu, 3 Oct 2024 10:32:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5B36D63526;\n\tThu, 3 Oct 2024 12:32:19 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C63A662C92\n\tfor ;\n\tThu, 3 Oct 2024 12:32:17 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9552D4C7;\n\tThu, 3 Oct 2024 12:30:44 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"SQE36GcE\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1727951444;\n\tbh=MbMjpIc+tAEFcWJl2wzXuptwX9U5Wvk0ezJB5JNgBtA=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=SQE36GcEFHz7oToJSHI0bcnkGfLH7gBzpKt+AgRcbclYTEsr0fHUox1Cg8rMB3ABL\n\tuHfIehX3vOftx4aco5aJztxLXkcRv3BwNphAXFm6Zy/Viar4E554oDi+Vl3ApemKAC\n\t/WHEbHjnv1x6lanuSrpqE8g0mqElPiM5vP4JYuXc=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240930063342.3014837-6-chenghaoyang@google.com>","References":"<20240930063342.3014837-1-chenghaoyang@google.com>\n\t<20240930063342.3014837-6-chenghaoyang@google.com>","Subject":"Re: [PATCH v14 5/7] libcamera: virtual: Add ImageFrameGenerator","From":"Kieran Bingham ","Cc":"Harvey Yang , Konami Shu , \n\tHarvey Yang , Yunke Cao ,\n\tTomasz Figa ,\n\tJacopo Mondi ","To":"Harvey Yang ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 03 Oct 2024 11:32:14 +0100","Message-ID":"<172795153430.1619946.2402640830315311386@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31590,"web_url":"https://patchwork.libcamera.org/comment/31590/","msgid":"","date":"2024-10-04T10:00:34","subject":"Re: [PATCH v14 5/7] libcamera: virtual: Add ImageFrameGenerator","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi Kieran,\n\nOn Thu, Oct 3, 2024 at 6:32 PM Kieran Bingham\n wrote:\n>\n> Quoting Harvey Yang (2024-09-30 07:29:46)\n> > Besides TestPatternGenerator, this patch adds ImageFrameGenerator that\n> > loads real images (jpg / jpeg for now) as the source and generates\n> > scaled frames.\n> >\n> > Signed-off-by: Konami Shu \n> > Co-developed-by: Harvey Yang \n> > Co-developed-by: Yunke Cao \n> > Co-developed-by: Tomasz Figa \n> > Reviewed-by: Jacopo Mondi \n> > ---\n> > .../virtual/image_frame_generator.cpp | 175 ++++++++++++++++++\n> > .../pipeline/virtual/image_frame_generator.h | 51 +++++\n> > src/libcamera/pipeline/virtual/meson.build | 4 +\n> > 3 files changed, 230 insertions(+)\n> > create mode 100644 src/libcamera/pipeline/virtual/image_frame_generator.cpp\n> > create mode 100644 src/libcamera/pipeline/virtual/image_frame_generator.h\n> >\n> > diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.cpp b/src/libcamera/pipeline/virtual/image_frame_generator.cpp\n> > new file mode 100644\n> > index 00000000..a1915b24\n> > --- /dev/null\n> > +++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp\n> > @@ -0,0 +1,175 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2024, Google Inc.\n> > + *\n> > + * image_frame_generator.cpp - Derived class of FrameGenerator for\n> > + * generating frames from images\n> > + */\n> > +\n> > +#include \"image_frame_generator.h\"\n> > +\n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \"libcamera/internal/mapped_framebuffer.h\"\n> > +\n> > +#include \"libyuv/convert.h\"\n> > +#include \"libyuv/scale.h\"\n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DECLARE_CATEGORY(Virtual)\n> > +\n> > +/*\n> > + * Factory function to create an ImageFrameGenerator object.\n> > + * Read the images and convert them to buffers in NV12 format.\n> > + * Store the pointers to the buffers to a list (imageFrameDatas)\n> > + */\n> > +std::unique_ptr\n> > +ImageFrameGenerator::create(ImageFrames &imageFrames)\n> > +{\n> > + std::unique_ptr imageFrameGenerator =\n> > + std::make_unique();\n> > + imageFrameGenerator->imageFrames_ = &imageFrames;\n> > +\n> > + /*\n> > + * For each file in the directory, load the image,\n> > + * convert it to NV12, and store the pointer.\n>\n> What happens if the directory has 200 images? Will this load all images\n> into memory?\n\nCurrently, yes.\n\n>\n> Should we impose any limits? Or would you see issues there?\n\nWe can either have a limit of file size or number of files, while I think\nwe can keep it as is for now, when we have more tests depending on\nvirtual pipeline handler. Over-designing might take much more time\nthan needed here IMO.\n\nAnother idea: We can also pre-load a certain amount of files, and\ndrop some images to save memory usage.\n\n>\n>\n> > + */\n> > + for (unsigned int i = 0; i < imageFrames.number.value_or(1); i++) {\n> > + std::filesystem::path path;\n> > + if (!imageFrames.number)\n> > + /* If the path is to an image */\n> > + path = imageFrames.path;\n> > + else\n> > + /* If the path is to a directory */\n> > + path = imageFrames.path / (std::to_string(i) + \".jpg\");\n> > +\n> > + File file(path);\n> > + if (!file.open(File::OpenModeFlag::ReadOnly)) {\n> > + LOG(Virtual, Error) << \"Failed to open image file \" << file.fileName()\n> > + << \": \" << strerror(file.error());\n> > + return nullptr;\n> > + }\n> > +\n> > + /* Read the image file to data */\n> > + auto fileSize = file.size();\n> > + auto buffer = std::make_unique(file.size());\n>\n> not fileSize?\n\nAh right. Updated.\n\n>\n> > + if (file.read({ buffer.get(), static_cast(fileSize) }) != fileSize) {\n> > + LOG(Virtual, Error) << \"Failed to read file \" << file.fileName()\n> > + << \": \" << strerror(file.error());\n> > + return nullptr;\n> > + }\n> > +\n> > + /* Get the width and height of the image */\n> > + int width, height;\n>\n> unsigned int?\n\nNo, as `libyuv::MJPGToNV12` takes `int` instead of `unsigned int`.\n\n>\n> > + if (libyuv::MJPGSize(buffer.get(), fileSize, &width, &height)) {\n> > + LOG(Virtual, Error) << \"Failed to get the size of the image file: \"\n> > + << file.fileName();\n> > + return nullptr;\n> > + }\n> > +\n> > + std::unique_ptr dstY =\n> > + std::make_unique(width * height);\n> > + std::unique_ptr dstUV =\n> > + std::make_unique(width * height / 2);\n> > + int ret = libyuv::MJPGToNV12(buffer.get(), fileSize,\n> > + dstY.get(), width, dstUV.get(),\n> > + width, width, height, width, height);\n> > + if (ret != 0)\n> > + LOG(Virtual, Error) << \"MJPGToNV12() failed with \" << ret;\n> > +\n> > + imageFrameGenerator->imageFrameDatas_.emplace_back(\n> > + ImageFrameData{ std::move(dstY), std::move(dstUV),\n> > + Size(width, height) });\n> > + }\n> > +\n> > + return imageFrameGenerator;\n> > +}\n> > +\n> > +/* Scale the buffers for image frames. */\n> > +void ImageFrameGenerator::configure(const Size &size)\n> > +{\n> > + /* Reset the source images to prevent multiple configuration calls */\n> > + scaledFrameDatas_.clear();\n> > + frameCount_ = 0;\n> > + parameter_ = 0;\n> > +\n> > + for (unsigned int i = 0; i < imageFrames_->number.value_or(1); i++) {\n> > + /* Scale the imageFrameDatas_ to scaledY and scaledUV */\n> > + unsigned int halfSizeWidth = (size.width + 1) / 2;\n> > + unsigned int halfSizeHeight = (size.height + 1) / 2;\n> > + std::unique_ptr scaledY =\n> > + std::make_unique(size.width * size.height);\n> > + std::unique_ptr scaledUV =\n> > + std::make_unique(halfSizeWidth * halfSizeHeight * 2);\n> > + auto &src = imageFrameDatas_[i];\n> > +\n> > + /*\n> > + * \\todo Some platforms might enforce stride due to GPU, like\n> > + * ChromeOS ciri (64). The weight needs to be a multiple of\n>\n> weight ?\n\nShould be width. Thanks!\n\n>\n> And I think this isn't anything to do with Ciri. We don't know (or care)\n> what a Ciri is inside libcamera, but we do care about externally\n> allocated buffers or buffers with defined stride requirements.\n>\n> The stride should always be correctly considered for any output buffer\n> managed by a pipeline handler.\n\nTrue. Let me remove Ciri as an example for now.\n\n>\n> I wonder if we can make some specific unit tests in lc-compliance to\n> validate that too.\n>\n>\n> > + * the stride to work properly for now.\n> > + */\n> > + libyuv::NV12Scale(src.Y.get(), src.size.width,\n> > + src.UV.get(), src.size.width,\n> > + src.size.width, src.size.height,\n> > + scaledY.get(), size.width, scaledUV.get(), size.width,\n> > + size.width, size.height, libyuv::FilterMode::kFilterBilinear);\n> > +\n> > + scaledFrameDatas_.emplace_back(\n> > + ImageFrameData{ std::move(scaledY), std::move(scaledUV), size });\n> > + }\n> > +}\n> > +\n> > +int ImageFrameGenerator::generateFrame(const Size &size, const FrameBuffer *buffer)\n> > +{\n> > + /* Don't do anything when the list of buffers is empty*/\n>\n> missing space between empty*/\n>\n> But ... well an ASSERT is definitely a way to not do anything at all\n> ever again ;-)\n\nYeah the comment doesn't make much sense...\nLet me remove it.\n\n>\n> > + ASSERT(!scaledFrameDatas_.empty());\n> > +\n> > + MappedFrameBuffer mappedFrameBuffer(buffer, MappedFrameBuffer::MapFlag::Write);\n> > +\n> > + auto planes = mappedFrameBuffer.planes();\n> > +\n> > + /* Make sure the frameCount does not over the number of images */\n>\n> /* Loop only around the number of images available */\n\nDone\n\n>\n> > + frameCount_ %= imageFrames_->number.value_or(1);\n>\n> Then, I don't think this is a frameCount. It's a frameIndex_.\n\nRight, updated.\n\n>\n> A FrameCount would be something you would set as the sequence numbers in\n> the FrameBuffers.\n>\n> Do you set those?\n\nNo. In which member variable of FrameBuffer?\n\n>\n> > +\n> > + /* Write the scaledY and scaledUV to the mapped frame buffer */\n> > + libyuv::NV12Copy(scaledFrameDatas_[frameCount_].Y.get(), size.width,\n> > + scaledFrameDatas_[frameCount_].UV.get(), size.width, planes[0].begin(),\n> > + size.width, planes[1].begin(), size.width,\n> > + size.width, size.height);\n> > +\n> > + /* Proceed an image every 4 frames */\n>\n> /* Proceed to the next image every 4 frames */\n\nDone\n\n>\n> > + /* \\todo Consider setting the proceed interval in the config file */\n>\n> s/proceed interval/frameInterval/\n>\n> But now I wonder if it should be frameRepeat ... this isn't an interval\n> is it ?\n>\n> > + parameter_++;\n> > + if (parameter_ % frameInterval == 0)\n> > + frameCount_++;\n> > +\n> > + return 0;\n> > +}\n> > +\n> > +/*\n> > + * \\var ImageFrameGenerator::imageFrameDatas_\n> > + * \\brief List of pointers to the not scaled image buffers\n> > + */\n> > +\n> > +/*\n> > + * \\var ImageFrameGenerator::scaledFrameDatas_\n> > + * \\brief List of pointers to the scaled image buffers\n> > + */\n> > +\n> > +/*\n> > + * \\var ImageFrameGenerator::imageFrames_\n> > + * \\brief Pointer to the imageFrames_ in VirtualCameraData\n> > + */\n> > +\n> > +/*\n> > + * \\var ImageFrameGenerator::parameter_\n> > + * \\brief Speed parameter. Change to the next image every parameter_ frames\n> > + */\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.h b/src/libcamera/pipeline/virtual/image_frame_generator.h\n> > new file mode 100644\n> > index 00000000..cd243816\n> > --- /dev/null\n> > +++ b/src/libcamera/pipeline/virtual/image_frame_generator.h\n> > @@ -0,0 +1,51 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2024, Google Inc.\n> > + *\n> > + * image_frame_generator.h - Derived class of FrameGenerator for\n> > + * generating frames from images\n> > + */\n> > +\n> > +#pragma once\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \"frame_generator.h\"\n> > +\n> > +namespace libcamera {\n> > +\n> > +/* Frame configuration provided by the config file */\n> > +struct ImageFrames {\n> > + std::filesystem::path path;\n> > + std::optional number;\n> > +};\n> > +\n> > +class ImageFrameGenerator : public FrameGenerator\n> > +{\n> > +public:\n> > + static std::unique_ptr create(ImageFrames &imageFrames);\n> > +\n> > +private:\n>\n> A brief comment here would help to say what a frameInterval is.\n>\n> But I don't know if it's named correctly. It's a frameRepeat from my\n> understanding... I.e. it causes a loaded frame to be repeated for N\n> frames.\n\nYeah right, frameRepeat makes more sense. Added a brief comment.\n\n\n>\n> > + static constexpr unsigned int frameInterval = 4;\n> > +\n> > + struct ImageFrameData {\n> > + std::unique_ptr Y;\n> > + std::unique_ptr UV;\n> > + Size size;\n> > + };\n> > +\n> > + void configure(const Size &size) override;\n> > + int generateFrame(const Size &size, const FrameBuffer *buffer) override;\n> > +\n> > + std::vector imageFrameDatas_;\n> > + std::vector scaledFrameDatas_;\n> > + ImageFrames *imageFrames_;\n> > + unsigned int frameCount_;\n> > + unsigned int parameter_;\n> > +};\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/pipeline/virtual/meson.build b/src/libcamera/pipeline/virtual/meson.build\n> > index 0c537777..bb38c193 100644\n> > --- a/src/libcamera/pipeline/virtual/meson.build\n> > +++ b/src/libcamera/pipeline/virtual/meson.build\n> > @@ -1,8 +1,12 @@\n> > # SPDX-License-Identifier: CC0-1.0\n> >\n> > libcamera_internal_sources += files([\n> > + 'image_frame_generator.cpp',\n> > 'test_pattern_generator.cpp',\n> > 'virtual.cpp',\n> > ])\n> >\n> > +libjpeg = dependency('libjpeg', required : true)\n> > +\n> > libcamera_deps += [libyuv_dep]\n> > +libcamera_deps += [libjpeg]\n> > --\n> > 2.46.1.824.gd892dcdcdd-goog\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 5DBBCBD80A\n\tfor ;\n\tFri, 4 Oct 2024 10:00:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F1B2E6353A;\n\tFri, 4 Oct 2024 12:00:47 +0200 (CEST)","from mail-lj1-x235.google.com (mail-lj1-x235.google.com\n\t[IPv6:2a00:1450:4864:20::235])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B7E0A6352B\n\tfor ;\n\tFri, 4 Oct 2024 12:00:46 +0200 (CEST)","by mail-lj1-x235.google.com with SMTP id\n\t38308e7fff4ca-2facf481587so18557971fa.1\n\tfor ;\n\tFri, 04 Oct 2024 03:00:46 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"ZtcQivem\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1728036046; x=1728640846;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=xAmf+/PbcjRbIP+bvGECFIImMbtAVy+YI1t1UTctzTY=;\n\tb=ZtcQivema6dfKWAwXzwwiCxf5Uobnrw3feD04Cw98p3AiXJMkBYOpw/XF8E9iedgAN\n\t3zG/DlSUCU/16JK0UUa7g5uMS5VuTV3EKd63HSPVlx++RpVQtHzyrwh+uOGGgfop/mam\n\t2cM9erzM5dFR7GT2a2eLpMCXr2wAPrMF0s+PQ=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1728036046; x=1728640846;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=xAmf+/PbcjRbIP+bvGECFIImMbtAVy+YI1t1UTctzTY=;\n\tb=ICIZPDj0MJNWSg7kN9VTqUZx2FK2Ps5dHZmxDgDr4p+tF8G/6qTlmwxIBqiBGPc+Ni\n\tfkEOHLKxh+pw4xyYdsws5x6NWDsuo+5CV8iM/urTQPPMB2IusYnQvCxVFsQwndd4uxZg\n\tkB7/lZK7czcU/o98lUDDZt4LnR9MYFtyAtmCuQksK+2+Tbj1IujpiuJCdh1nRkaGtKCS\n\t9hgXe/P6EDbQ7qJPagO0Cs3UkOvXApeCfFGCcoqURiQ5QMoyIQ9ED7Cci4UQ5JXGeUBn\n\twW2nKvt3MMh/RDKogH5HEzbpoZpN0f+K2yOoH1w1/bPwqeGbjfZF8gS+1EeA0pjDJzTz\n\t2b+Q==","X-Gm-Message-State":"AOJu0Ywx3fCInh5wNQjDzRVnbZEbGFCGp7XI3OMLbZxK75v1Uns1LnIh\n\teDQGFvL4rp9BuiejOM3fkujE7aCFaWBbrkEEvx6qOHHD4DGnRqPTUi1FTF2RzyszZdHIWifqawB\n\t1ef4ygBeulqs3Xb2ijrID+7mDvCOFNHe5ZGqV","X-Google-Smtp-Source":"AGHT+IGnq7CWA7adHaqya/E/x5s/IBOufJvtY3pzRP52b10o9oEUPePxjz9nYv+8JAcI+23lCiE2/su8I1hWx57oy2g=","X-Received":"by 2002:a2e:bea3:0:b0:2fa:e52f:4470 with SMTP id\n\t38308e7fff4ca-2faf3c0c3d7mr10462281fa.9.1728036045761;\n\tFri, 04 Oct 2024 03:00:45 -0700 (PDT)","MIME-Version":"1.0","References":"<20240930063342.3014837-1-chenghaoyang@google.com>\n\t<20240930063342.3014837-6-chenghaoyang@google.com>\n\t<172795153430.1619946.2402640830315311386@ping.linuxembedded.co.uk>","In-Reply-To":"<172795153430.1619946.2402640830315311386@ping.linuxembedded.co.uk>","From":"Cheng-Hao Yang ","Date":"Fri, 4 Oct 2024 18:00:34 +0800","Message-ID":"","Subject":"Re: [PATCH v14 5/7] libcamera: virtual: Add ImageFrameGenerator","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org,\n\tHarvey Yang , \n\tKonami Shu , Yunke Cao , \n\tTomasz Figa ,\n\tJacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]