[{"id":30929,"web_url":"https://patchwork.libcamera.org/comment/30929/","msgid":"","date":"2024-08-27T16:35:36","subject":"Re: [PATCH v9 4/8] libcamera: pipeline: Add test pattern for\n\tVirtualPipelineHandler","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Harvey, Konami\n\nOn Tue, Aug 20, 2024 at 04:23:35PM GMT, Harvey Yang wrote:\n> From: Konami Shu \n>\n> - There are two test patterns: color bars and diagonal lines\n> - Add class for generating test patterns\n> - Add libyuv to build dependencies\n> - Make VirtualPipelineHandler show the test pattern\n> - Format the code\n\nCould you make a commit message out of this please ? Like:\n\nAdd a test pattern generator class hierarchy for the Virtual\npipeline handler.\n\nImplement two types of test patterns: color bars and diagonal lines\ngenerator and use them in the Virtual pipeline handler.\n\nAdd a dependency for libyuv to the build system to generate images\nin NV12 format from the test pattern.\n\n>\n> - Rename test_pattern to frame_generator\n> - reflect comment\n> \t- Fix const variable name\n> \t- Use #pragma once\n> \t- Make configure() private\n\nThis list looks like a changelog more than a commit message. Could you\ndrop it or place it below\n\n---\n\nSo that it doesn't appear in the git history ?\n\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> ---\n> .../pipeline/virtual/frame_generator.h | 33 ++++++\n> src/libcamera/pipeline/virtual/meson.build | 22 ++++\n> .../virtual/test_pattern_generator.cpp | 112 ++++++++++++++++++\n> .../pipeline/virtual/test_pattern_generator.h | 58 +++++++++\n> src/libcamera/pipeline/virtual/virtual.cpp | 28 ++++-\n> src/libcamera/pipeline/virtual/virtual.h | 8 ++\n> 6 files changed, 258 insertions(+), 3 deletions(-)\n> create mode 100644 src/libcamera/pipeline/virtual/frame_generator.h\n> create mode 100644 src/libcamera/pipeline/virtual/test_pattern_generator.cpp\n> create mode 100644 src/libcamera/pipeline/virtual/test_pattern_generator.h\n>\n> diff --git a/src/libcamera/pipeline/virtual/frame_generator.h b/src/libcamera/pipeline/virtual/frame_generator.h\n> new file mode 100644\n> index 000000000..9699af7a4\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/virtual/frame_generator.h\n> @@ -0,0 +1,33 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2023, Google Inc.\n> + *\n> + * frame_generator.h - Virtual cameras helper to generate frames\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +#include \n> +\n> +namespace libcamera {\n> +\n> +class FrameGenerator\n> +{\n> +public:\n> +\tvirtual ~FrameGenerator() = default;\n> +\n> +\t/* Create buffers for using them in `generateFrame` */\n\nUsually we don't comment headers, but I don't mind\n\n> +\tvirtual void configure(const Size &size) = 0;\n> +\n> +\t/** Fill the output frame buffer.\n> +\t * Use the frame at the frameCount of image frames\n> +\t */\n\nAs long as the right commenting style is used: this is not Doxygen (no\n/**) and I don't see references to frameCount anywhere ?\n\n> +\tvirtual void generateFrame(const Size &size,\n> +\t\t\t\t const FrameBuffer *buffer) = 0;\n> +\n> +protected:\n> +\tFrameGenerator() {}\n> +};\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/virtual/meson.build b/src/libcamera/pipeline/virtual/meson.build\n> index ba7ff754e..e1e65e68d 100644\n> --- a/src/libcamera/pipeline/virtual/meson.build\n> +++ b/src/libcamera/pipeline/virtual/meson.build\n> @@ -2,4 +2,26 @@\n>\n> libcamera_sources += files([\n> 'virtual.cpp',\n> + 'test_pattern_generator.cpp',\n> ])\n> +\n> +libyuv_dep = dependency('libyuv', required : false)\n> +\n> +# Fallback to a subproject if libyuv isn't found, as it's typically not\n> +# provided by distributions.\n> +if not libyuv_dep.found()\n\nThe Android HAL has exactly the same snippet. I wonder if it should be\nmoved to the main libcamera build file and only add libyuv as a\ndependency in the HAL and here. It might require a bit of\nexperimentation\n\n> + cmake = import('cmake')\n> +\n> + libyuv_vars = cmake.subproject_options()\n> + libyuv_vars.add_cmake_defines({'CMAKE_POSITION_INDEPENDENT_CODE': 'ON'})\n> + libyuv_vars.set_override_option('cpp_std', 'c++17')\n> + libyuv_vars.append_compile_args('cpp',\n> + '-Wno-sign-compare',\n> + '-Wno-unused-variable',\n> + '-Wno-unused-parameter')\n> + libyuv_vars.append_link_args('-ljpeg')\n\nDo you need jpeg support ?\n\n> + libyuv = cmake.subproject('libyuv', options : libyuv_vars)\n> + libyuv_dep = libyuv.dependency('yuv')\n> +endif\n> +\n> +libcamera_deps += [libyuv_dep]\n> diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp\n> new file mode 100644\n> index 000000000..8dfe626e5\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp\n> @@ -0,0 +1,112 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2023, Google Inc.\n> + *\n> + * test_pattern_generator.cpp - Derived class of FrameGenerator for\n> + * generating test patterns\n> + */\n> +\n> +#include \"test_pattern_generator.h\"\n> +\n> +#include \n> +\n> +#include \"libcamera/internal/mapped_framebuffer.h\"\n> +\n\nEmpty line please\n\n> +#include \"libyuv/convert_from_argb.h\"\n> +namespace libcamera {\n> +\n> +LOG_DECLARE_CATEGORY(Virtual)\n> +\n> +static const unsigned int kARGBSize = 4;\n> +\n> +void TestPatternGenerator::generateFrame(\n> +\tconst Size &size,\n> +\tconst FrameBuffer *buffer)\n\nWeird indent\n\nvoid TestPatternGenerator::generateFrame(const Size &size,\n\t\t\t\t\t const FrameBuffer *buffer)\n\n\n> +{\n> +\tMappedFrameBuffer mappedFrameBuffer(buffer,\n> +\t\t\t\t\t MappedFrameBuffer::MapFlag::Write);\n> +\n> +\tauto planes = mappedFrameBuffer.planes();\n> +\n> +\t/* Convert the template_ to the frame buffer */\n> +\tint ret = libyuv::ARGBToNV12(\n\nAs this produces NV12, the pipeline handler should only accept NV12\nand adjust all other pixelformats. If I'm not mistaken this is not\nenforced in validate() and you return SRGGB10 for Role::Raw (which you\nshouldn't support afaict). This can cause issues as the buffers\nallocated might be of a different size than the ones you expect ?\n\nI agree that generating patterns in ARGB is easier than NV12, but I\nwonder why not use RGB888 as the pipeline output format directly so\nthat you don't have to depend on libyuv at all.\n\n\n> +\t\ttemplate_.get(), /*src_stride_argb=*/size.width * kARGBSize,\n\nWhy a comment in the middle of the code ?\n\n> +\t\tplanes[0].begin(), size.width,\n> +\t\tplanes[1].begin(), size.width,\n> +\t\tsize.width, size.height);\n> +\tif (ret != 0) {\n> +\t\tLOG(Virtual, Error) << \"ARGBToNV12() failed with \" << ret;\n> +\t}\n\nNo {} for single line statements\n\n> +}\n> +\n> +std::unique_ptr ColorBarsGenerator::create()\n> +{\n> +\treturn std::make_unique();\n> +}\n> +\n> +void ColorBarsGenerator::configure(const Size &size)\n> +{\n> +\tconstexpr uint8_t kColorBar[8][3] = {\n> +\t\t// R, G, B\n> +\t\t{ 0xff, 0xff, 0xff }, // White\n> +\t\t{ 0xff, 0xff, 0x00 }, // Yellow\n> +\t\t{ 0x00, 0xff, 0xff }, // Cyan\n> +\t\t{ 0x00, 0xff, 0x00 }, // Green\n> +\t\t{ 0xff, 0x00, 0xff }, // Magenta\n> +\t\t{ 0xff, 0x00, 0x00 }, // Red\n> +\t\t{ 0x00, 0x00, 0xff }, // Blue\n> +\t\t{ 0x00, 0x00, 0x00 }, // Black\n> +\t};\n> +\n> +\ttemplate_ = std::make_unique(\n> +\t\tsize.width * size.height * kARGBSize);\n> +\n> +\tunsigned int colorBarWidth = size.width / std::size(kColorBar);\n> +\n> +\tuint8_t *buf = template_.get();\n> +\tfor (size_t h = 0; h < size.height; h++) {\n> +\t\tfor (size_t w = 0; w < size.width; w++) {\n> +\t\t\t// repeat when the width is exceed\n\nlibcamera doesn't use C++ style comments\n\n> +\t\t\tint index = (w / colorBarWidth) % std::size(kColorBar);\n\nunsigned int\n\n> +\n> +\t\t\t*buf++ = kColorBar[index][2]; // B\n> +\t\t\t*buf++ = kColorBar[index][1]; // G\n> +\t\t\t*buf++ = kColorBar[index][0]; // R\n> +\t\t\t*buf++ = 0x00; // A\n> +\t\t}\n> +\t}\n> +}\n> +\n> +std::unique_ptr DiagonalLinesGenerator::create()\n> +{\n> +\treturn std::make_unique();\n> +}\n> +\n> +void DiagonalLinesGenerator::configure(const Size &size)\n> +{\n> +\tconstexpr uint8_t kColorBar[8][3] = {\n> +\t\t// R, G, B\n> +\t\t{ 0xff, 0xff, 0xff }, // White\n> +\t\t{ 0x00, 0x00, 0x00 }, // Black\n> +\t};\n> +\n> +\ttemplate_ = std::make_unique(\n> +\t\tsize.width * size.height * kARGBSize);\n> +\n> +\tunsigned int lineWidth = size.width / 10;\n> +\n> +\tuint8_t *buf = template_.get();\n> +\tfor (size_t h = 0; h < size.height; h++) {\n> +\t\tfor (size_t w = 0; w < size.width; w++) {\n> +\t\t\t// repeat when the width is exceed\n> +\t\t\tint index = ((w + h) / lineWidth) % 2;\n\nsame comments\n\n> +\n> +\t\t\t*buf++ = kColorBar[index][2]; // B\n> +\t\t\t*buf++ = kColorBar[index][1]; // G\n> +\t\t\t*buf++ = kColorBar[index][0]; // R\n> +\t\t\t*buf++ = 0x00; // A\n> +\t\t}\n> +\t}\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.h b/src/libcamera/pipeline/virtual/test_pattern_generator.h\n> new file mode 100644\n> index 000000000..ed8d4e43b\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.h\n> @@ -0,0 +1,58 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2023, Google Inc.\n> + *\n> + * test_pattern_generator.h - Derived class of FrameGenerator for\n> + * generating test patterns\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \"frame_generator.h\"\n> +\n> +namespace libcamera {\n> +\n> +enum class TestPattern : char {\n> +\tColorBars = 0,\n> +\tDiagonalLines = 1,\n> +};\n> +\n> +class TestPatternGenerator : public FrameGenerator\n> +{\n> +private:\n> +\tvoid generateFrame(const Size &size,\n> +\t\t\t const FrameBuffer *buffer) override;\n\n\nMaybe I don't get this, but this overrides a public function, and it's\ncalled from the pipeline, why private ?\n\n> +\n> +protected:\n> +\t/* Shift the buffer by 1 pixel left each frame */\n> +\tvoid shiftLeft(const Size &size);\n\nNot implemented afaict\n\n> +\t/* Buffer of test pattern template */\n> +\tstd::unique_ptr template_;\n> +};\n> +\n> +class ColorBarsGenerator : public TestPatternGenerator\n> +{\n> +public:\n> +\tstatic std::unique_ptr create();\n\nI won't question the class hierarchy design, but this could be done\nwith a simple public constructur and stored as unique_ptr in the\npipeline handler maybe.\n\n> +\n> +private:\n> +\t/* Generate a template buffer of the color bar test pattern. */\n> +\tvoid configure(const Size &size) override;\n\nSame questions as the one on generateFrame, it's surely something I\ndon't well understand then\n\n> +};\n> +\n> +class DiagonalLinesGenerator : public TestPatternGenerator\n> +{\n> +public:\n> +\tstatic std::unique_ptr create();\n> +\n> +private:\n> +\t/* Generate a template buffer of the diagonal lines test pattern. */\n> +\tvoid configure(const Size &size) override;\n> +};\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp\n> index 74eb8c7ad..357fdd035 100644\n> --- a/src/libcamera/pipeline/virtual/virtual.cpp\n> +++ b/src/libcamera/pipeline/virtual/virtual.cpp\n> @@ -192,10 +192,14 @@ int PipelineHandlerVirtual::exportFrameBuffers(\n> \treturn dmaBufAllocator_.exportBuffers(config.bufferCount, planeSizes, buffers);\n> }\n>\n> -int PipelineHandlerVirtual::start([[maybe_unused]] Camera *camera,\n> +int PipelineHandlerVirtual::start(Camera *camera,\n> \t\t\t\t [[maybe_unused]] const ControlList *controls)\n> {\n> \t/* \\todo Start reading the virtual video if any. */\n> +\tVirtualCameraData *data = cameraData(camera);\n> +\n> +\tdata->frameGenerator_->configure(data->stream_.configuration().size);\n\nShouldn't this be done at Pipeline::configure() ?\n\n> +\n> \treturn 0;\n> }\n>\n> @@ -207,9 +211,14 @@ void PipelineHandlerVirtual::stopDevice([[maybe_unused]] Camera *camera)\n> int PipelineHandlerVirtual::queueRequestDevice([[maybe_unused]] Camera *camera,\n> \t\t\t\t\t Request *request)\n> {\n> +\tVirtualCameraData *data = cameraData(camera);\n> +\n> \t/* \\todo Read from the virtual video if any. */\n> -\tfor (auto it : request->buffers())\n> -\t\tcompleteBuffer(request, it.second);\n> +\tfor (auto const &[stream, buffer] : request->buffers()) {\n\nUnless something changes in the next patches, you only have one stream\n\n> +\t\t/* map buffer and fill test patterns */\n> +\t\tdata->frameGenerator_->generateFrame(stream->configuration().size, buffer);\n> +\t\tcompleteBuffer(request, buffer);\n> +\t}\n>\n> \trequest->metadata().set(controls::SensorTimestamp, currentTimestamp());\n> \tcompleteRequest(request);\n> @@ -241,11 +250,24 @@ bool PipelineHandlerVirtual::match([[maybe_unused]] DeviceEnumerator *enumerator\n> \tstd::set streams{ &data->stream_ };\n> \tconst std::string id = \"Virtual0\";\n> \tstd::shared_ptr camera = Camera::create(std::move(data), id, streams);\n> +\n> +\tinitFrameGenerator(camera.get());\n> +\n> \tregisterCamera(std::move(camera));\n>\n> \treturn false; // Prevent infinite loops for now\n> }\n>\n> +void PipelineHandlerVirtual::initFrameGenerator(Camera *camera)\n> +{\n> +\tauto data = cameraData(camera);\n> +\tif (data->testPattern_ == TestPattern::DiagonalLines) {\n\nNo {} for single lines statements\n\nWho initializes data->testPattern_ ?\n\n> +\t\tdata->frameGenerator_ = DiagonalLinesGenerator::create();\n> +\t} else {\n> +\t\tdata->frameGenerator_ = ColorBarsGenerator::create();\n> +\t}\n> +}\n> +\n> REGISTER_PIPELINE_HANDLER(PipelineHandlerVirtual, \"virtual\")\n>\n> } /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/virtual/virtual.h b/src/libcamera/pipeline/virtual/virtual.h\n> index 6fc6b34d8..fecd9fa6f 100644\n> --- a/src/libcamera/pipeline/virtual/virtual.h\n> +++ b/src/libcamera/pipeline/virtual/virtual.h\n> @@ -13,6 +13,8 @@\n> #include \"libcamera/internal/dma_buf_allocator.h\"\n> #include \"libcamera/internal/pipeline_handler.h\"\n>\n> +#include \"test_pattern_generator.h\"\n> +\n> namespace libcamera {\n>\n> class VirtualCameraData : public Camera::Private\n> @@ -29,9 +31,13 @@ public:\n>\n> \t~VirtualCameraData() = default;\n>\n> +\tTestPattern testPattern_;\n> +\n> \tstd::vector supportedResolutions_;\n>\n> \tStream stream_;\n> +\n> +\tstd::unique_ptr frameGenerator_;\n> };\n>\n> class VirtualCameraConfiguration : public CameraConfiguration\n> @@ -72,6 +78,8 @@ private:\n> \t\treturn static_cast(camera->_d());\n> \t}\n>\n> +\tvoid initFrameGenerator(Camera *camera);\n> +\n> \tDmaBufAllocator dmaBufAllocator_;\n> };\n>\n> --\n> 2.46.0.184.g6999bdac58-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 D2871C323E\n\tfor ;\n\tTue, 27 Aug 2024 16:35:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D3DB46345D;\n\tTue, 27 Aug 2024 18:35:41 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BD1E761901\n\tfor ;\n\tTue, 27 Aug 2024 18:35:39 +0200 (CEST)","from ideasonboard.com (mob-5-90-141-165.net.vodafone.it\n\t[5.90.141.165])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5DF906A6;\n\tTue, 27 Aug 2024 18:34:32 +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=\"sI53GuIG\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1724776472;\n\tbh=z+pxTBuz88lIxVm941WXsxZjFZYvJAQUB5yAlRsmDHE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=sI53GuIGpVIvHG0hxKISjML9WIJcMNWTl9qEQ7DHiemIzcRdLMMcwOPm4JmLYJwDR\n\tD4UN6n5qlY/WlK044G4B4aMc2MNHPDi7u+/ddg5/g5t4l7LZeoAyRVQgkigcGK3S6y\n\tkBEJrU9+RR21DQmBYqIfVOgfafDuZhxGzYvgCx3Y=","Date":"Tue, 27 Aug 2024 18:35:36 +0200","From":"Jacopo Mondi ","To":"Harvey Yang ","Cc":"libcamera-devel@lists.libcamera.org, Konami Shu , \n\tYunke Cao , Tomasz Figa ","Subject":"Re: [PATCH v9 4/8] libcamera: pipeline: Add test pattern for\n\tVirtualPipelineHandler","Message-ID":"","References":"<20240820172202.526547-1-chenghaoyang@google.com>\n\t<20240820172202.526547-5-chenghaoyang@google.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240820172202.526547-5-chenghaoyang@google.com>","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":30957,"web_url":"https://patchwork.libcamera.org/comment/30957/","msgid":"","date":"2024-08-29T19:57:38","subject":"Re: [PATCH v9 4/8] libcamera: pipeline: Add test pattern for\n\tVirtualPipelineHandler","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Thanks Jacopo,\n\nOn Tue, Aug 27, 2024 at 6:35 PM Jacopo Mondi \nwrote:\n\n> Hi Harvey, Konami\n>\n> On Tue, Aug 20, 2024 at 04:23:35PM GMT, Harvey Yang wrote:\n> > From: Konami Shu \n> >\n> > - There are two test patterns: color bars and diagonal lines\n> > - Add class for generating test patterns\n> > - Add libyuv to build dependencies\n> > - Make VirtualPipelineHandler show the test pattern\n> > - Format the code\n>\n> Could you make a commit message out of this please ? Like:\n>\n> Add a test pattern generator class hierarchy for the Virtual\n> pipeline handler.\n>\n> Implement two types of test patterns: color bars and diagonal lines\n> generator and use them in the Virtual pipeline handler.\n>\n> Add a dependency for libyuv to the build system to generate images\n> in NV12 format from the test pattern.\n>\n> Adopted, thanks!\n\n\n> >\n> > - Rename test_pattern to frame_generator\n> > - reflect comment\n> > - Fix const variable name\n> > - Use #pragma once\n> > - Make configure() private\n>\n> This list looks like a changelog more than a commit message. Could you\n> drop it or place it below\n>\nRemoved.\n\n\n>\n> ---\n>\n> So that it doesn't appear in the git history ?\n>\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> > ---\n> > .../pipeline/virtual/frame_generator.h | 33 ++++++\n> > src/libcamera/pipeline/virtual/meson.build | 22 ++++\n> > .../virtual/test_pattern_generator.cpp | 112 ++++++++++++++++++\n> > .../pipeline/virtual/test_pattern_generator.h | 58 +++++++++\n> > src/libcamera/pipeline/virtual/virtual.cpp | 28 ++++-\n> > src/libcamera/pipeline/virtual/virtual.h | 8 ++\n> > 6 files changed, 258 insertions(+), 3 deletions(-)\n> > create mode 100644 src/libcamera/pipeline/virtual/frame_generator.h\n> > create mode 100644\n> src/libcamera/pipeline/virtual/test_pattern_generator.cpp\n> > create mode 100644\n> src/libcamera/pipeline/virtual/test_pattern_generator.h\n> >\n> > diff --git a/src/libcamera/pipeline/virtual/frame_generator.h\n> b/src/libcamera/pipeline/virtual/frame_generator.h\n> > new file mode 100644\n> > index 000000000..9699af7a4\n> > --- /dev/null\n> > +++ b/src/libcamera/pipeline/virtual/frame_generator.h\n> > @@ -0,0 +1,33 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2023, Google Inc.\n> > + *\n> > + * frame_generator.h - Virtual cameras helper to generate frames\n> > + */\n> > +\n> > +#pragma once\n> > +\n> > +#include \n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +class FrameGenerator\n> > +{\n> > +public:\n> > + virtual ~FrameGenerator() = default;\n> > +\n> > + /* Create buffers for using them in `generateFrame` */\n>\n> Usually we don't comment headers, but I don't mind\n>\n> Right, I think Konami put it here because there's no .cpp file for this\nvirtual class. Removed.\n\n\n> > + virtual void configure(const Size &size) = 0;\n> > +\n> > + /** Fill the output frame buffer.\n> > + * Use the frame at the frameCount of image frames\n> > + */\n>\n> As long as the right commenting style is used: this is not Doxygen (no\n> /**) and I don't see references to frameCount anywhere ?\n>\nRemoved.\n\n\n>\n> > + virtual void generateFrame(const Size &size,\n> > + const FrameBuffer *buffer) = 0;\n> > +\n> > +protected:\n> > + FrameGenerator() {}\n> > +};\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/pipeline/virtual/meson.build\n> b/src/libcamera/pipeline/virtual/meson.build\n> > index ba7ff754e..e1e65e68d 100644\n> > --- a/src/libcamera/pipeline/virtual/meson.build\n> > +++ b/src/libcamera/pipeline/virtual/meson.build\n> > @@ -2,4 +2,26 @@\n> >\n> > libcamera_sources += files([\n> > 'virtual.cpp',\n> > + 'test_pattern_generator.cpp',\n> > ])\n> > +\n> > +libyuv_dep = dependency('libyuv', required : false)\n> > +\n> > +# Fallback to a subproject if libyuv isn't found, as it's typically not\n> > +# provided by distributions.\n> > +if not libyuv_dep.found()\n>\n> The Android HAL has exactly the same snippet. I wonder if it should be\n> moved to the main libcamera build file and only add libyuv as a\n> dependency in the HAL and here. It might require a bit of\n> experimentation\n>\nMoved to `src/meson.build`. Please check.\n\n\n>\n> > + cmake = import('cmake')\n> > +\n> > + libyuv_vars = cmake.subproject_options()\n> > + libyuv_vars.add_cmake_defines({'CMAKE_POSITION_INDEPENDENT_CODE':\n> 'ON'})\n> > + libyuv_vars.set_override_option('cpp_std', 'c++17')\n> > + libyuv_vars.append_compile_args('cpp',\n> > + '-Wno-sign-compare',\n> > + '-Wno-unused-variable',\n> > + '-Wno-unused-parameter')\n> > + libyuv_vars.append_link_args('-ljpeg')\n>\n> Do you need jpeg support ?\n>\nWell, in the 7th patch, yes. Also, as we try to use the same\ndependency with Android adapter, how about keeping jpeg\nhere?\n\n\n>\n> > + libyuv = cmake.subproject('libyuv', options : libyuv_vars)\n> > + libyuv_dep = libyuv.dependency('yuv')\n> > +endif\n> > +\n> > +libcamera_deps += [libyuv_dep]\n> > diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp\n> b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp\n> > new file mode 100644\n> > index 000000000..8dfe626e5\n> > --- /dev/null\n> > +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp\n> > @@ -0,0 +1,112 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2023, Google Inc.\n> > + *\n> > + * test_pattern_generator.cpp - Derived class of FrameGenerator for\n> > + * generating test patterns\n> > + */\n> > +\n> > +#include \"test_pattern_generator.h\"\n> > +\n> > +#include \n> > +\n> > +#include \"libcamera/internal/mapped_framebuffer.h\"\n> > +\n>\n> Empty line please\n>\nDone\n\n\n>\n> > +#include \"libyuv/convert_from_argb.h\"\n> > +namespace libcamera {\n> > +\n> > +LOG_DECLARE_CATEGORY(Virtual)\n> > +\n> > +static const unsigned int kARGBSize = 4;\n> > +\n> > +void TestPatternGenerator::generateFrame(\n> > + const Size &size,\n> > + const FrameBuffer *buffer)\n>\n> Weird indent\n>\n> void TestPatternGenerator::generateFrame(const Size &size,\n> const FrameBuffer *buffer)\n>\n>\n> Adopted.\n\n\n> > +{\n> > + MappedFrameBuffer mappedFrameBuffer(buffer,\n> > +\n> MappedFrameBuffer::MapFlag::Write);\n> > +\n> > + auto planes = mappedFrameBuffer.planes();\n> > +\n> > + /* Convert the template_ to the frame buffer */\n> > + int ret = libyuv::ARGBToNV12(\n>\n> As this produces NV12, the pipeline handler should only accept NV12\n> and adjust all other pixelformats. If I'm not mistaken this is not\n> enforced in validate() and you return SRGGB10 for Role::Raw (which you\n> shouldn't support afaict). This can cause issues as the buffers\n> allocated might be of a different size than the ones you expect\n\n\nUpdated in the 3rd patch to remove the raw stream and ensure formats::NV12\nfor each stream.\n\n\n> ?\n>\n> I agree that generating patterns in ARGB is easier than NV12, but I\n> wonder why not use RGB888 as the pipeline output format directly so\n> that you don't have to depend on libyuv at all.\n>\n\nI think it's because the Android adapter requires mostly NV12 [1]\n\n[1]:\nhttps://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/upstream/src/android/camera_capabilities.cpp;l=68\n\n\n>\n>\n> > + template_.get(), /*src_stride_argb=*/size.width *\n> kARGBSize,\n>\n> Why a comment in the middle of the code ?\n>\nRemoved.\n\n\n>\n> > + planes[0].begin(), size.width,\n> > + planes[1].begin(), size.width,\n> > + size.width, size.height);\n> > + if (ret != 0) {\n> > + LOG(Virtual, Error) << \"ARGBToNV12() failed with \" << ret;\n> > + }\n>\n> No {} for single line statements\n>\nDone\n\n\n>\n> > +}\n> > +\n> > +std::unique_ptr ColorBarsGenerator::create()\n> > +{\n> > + return std::make_unique();\n> > +}\n> > +\n> > +void ColorBarsGenerator::configure(const Size &size)\n> > +{\n> > + constexpr uint8_t kColorBar[8][3] = {\n> > + // R, G, B\n> > + { 0xff, 0xff, 0xff }, // White\n> > + { 0xff, 0xff, 0x00 }, // Yellow\n> > + { 0x00, 0xff, 0xff }, // Cyan\n> > + { 0x00, 0xff, 0x00 }, // Green\n> > + { 0xff, 0x00, 0xff }, // Magenta\n> > + { 0xff, 0x00, 0x00 }, // Red\n> > + { 0x00, 0x00, 0xff }, // Blue\n> > + { 0x00, 0x00, 0x00 }, // Black\n> > + };\n> > +\n> > + template_ = std::make_unique(\n> > + size.width * size.height * kARGBSize);\n> > +\n> > + unsigned int colorBarWidth = size.width / std::size(kColorBar);\n> > +\n> > + uint8_t *buf = template_.get();\n> > + for (size_t h = 0; h < size.height; h++) {\n> > + for (size_t w = 0; w < size.width; w++) {\n> > + // repeat when the width is exceed\n>\n> libcamera doesn't use C++ style comments\n>\nUpdated.\n\n\n>\n> > + int index = (w / colorBarWidth) %\n> std::size(kColorBar);\n>\n> unsigned int\n>\nDone\n\n\n>\n> > +\n> > + *buf++ = kColorBar[index][2]; // B\n> > + *buf++ = kColorBar[index][1]; // G\n> > + *buf++ = kColorBar[index][0]; // R\n> > + *buf++ = 0x00; // A\n> > + }\n> > + }\n> > +}\n> > +\n> > +std::unique_ptr DiagonalLinesGenerator::create()\n> > +{\n> > + return std::make_unique();\n> > +}\n> > +\n> > +void DiagonalLinesGenerator::configure(const Size &size)\n> > +{\n> > + constexpr uint8_t kColorBar[8][3] = {\n> > + // R, G, B\n> > + { 0xff, 0xff, 0xff }, // White\n> > + { 0x00, 0x00, 0x00 }, // Black\n> > + };\n> > +\n> > + template_ = std::make_unique(\n> > + size.width * size.height * kARGBSize);\n> > +\n> > + unsigned int lineWidth = size.width / 10;\n> > +\n> > + uint8_t *buf = template_.get();\n> > + for (size_t h = 0; h < size.height; h++) {\n> > + for (size_t w = 0; w < size.width; w++) {\n> > + // repeat when the width is exceed\n> > + int index = ((w + h) / lineWidth) % 2;\n>\n> same comments\n>\nDone\n\n\n>\n> > +\n> > + *buf++ = kColorBar[index][2]; // B\n> > + *buf++ = kColorBar[index][1]; // G\n> > + *buf++ = kColorBar[index][0]; // R\n> > + *buf++ = 0x00; // A\n> > + }\n> > + }\n> > +}\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.h\n> b/src/libcamera/pipeline/virtual/test_pattern_generator.h\n> > new file mode 100644\n> > index 000000000..ed8d4e43b\n> > --- /dev/null\n> > +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.h\n> > @@ -0,0 +1,58 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2023, Google Inc.\n> > + *\n> > + * test_pattern_generator.h - Derived class of FrameGenerator for\n> > + * generating test patterns\n> > + */\n> > +\n> > +#pragma once\n> > +\n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \"frame_generator.h\"\n> > +\n> > +namespace libcamera {\n> > +\n> > +enum class TestPattern : char {\n> > + ColorBars = 0,\n> > + DiagonalLines = 1,\n> > +};\n> > +\n> > +class TestPatternGenerator : public FrameGenerator\n> > +{\n> > +private:\n> > + void generateFrame(const Size &size,\n> > + const FrameBuffer *buffer) override;\n>\n>\n> Maybe I don't get this, but this overrides a public function, and it's\n> called from the pipeline, why private ?\n>\nHmm, it should be public. Updated.\n\n\n>\n> > +\n> > +protected:\n> > + /* Shift the buffer by 1 pixel left each frame */\n> > + void shiftLeft(const Size &size);\n>\n> Not implemented afaict\n>\nRemoved\n\n\n> > + /* Buffer of test pattern template */\n> > + std::unique_ptr template_;\n> > +};\n> > +\n> > +class ColorBarsGenerator : public TestPatternGenerator\n> > +{\n> > +public:\n> > + static std::unique_ptr create();\n>\n> I won't question the class hierarchy design, but this could be done\n> with a simple public constructur and stored as unique_ptr in the\n> pipeline handler maybe.\n>\nThe hierarchy design is because the 6th patch introduces the shift,\nwhich can be used on all test pattern generators.\nAnd yes, a static create function is not needed. Updated.\n\n\n>\n> > +\n> > +private:\n> > + /* Generate a template buffer of the color bar test pattern. */\n> > + void configure(const Size &size) override;\n>\n> Same questions as the one on generateFrame, it's surely something I\n> don't well understand then\n>\n> > +};\n> > +\n> > +class DiagonalLinesGenerator : public TestPatternGenerator\n> > +{\n> > +public:\n> > + static std::unique_ptr create();\n> > +\n> > +private:\n> > + /* Generate a template buffer of the diagonal lines test pattern.\n> */\n> > + void configure(const Size &size) override;\n> > +};\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/pipeline/virtual/virtual.cpp\n> b/src/libcamera/pipeline/virtual/virtual.cpp\n> > index 74eb8c7ad..357fdd035 100644\n> > --- a/src/libcamera/pipeline/virtual/virtual.cpp\n> > +++ b/src/libcamera/pipeline/virtual/virtual.cpp\n> > @@ -192,10 +192,14 @@ int PipelineHandlerVirtual::exportFrameBuffers(\n> > return dmaBufAllocator_.exportBuffers(config.bufferCount,\n> planeSizes, buffers);\n> > }\n> >\n> > -int PipelineHandlerVirtual::start([[maybe_unused]] Camera *camera,\n> > +int PipelineHandlerVirtual::start(Camera *camera,\n> > [[maybe_unused]] const ControlList\n> *controls)\n> > {\n> > /* \\todo Start reading the virtual video if any. */\n> > + VirtualCameraData *data = cameraData(camera);\n> > +\n> > +\n> data->frameGenerator_->configure(data->stream_.configuration().size);\n>\n> Shouldn't this be done at Pipeline::configure() ?\n\nRight, migrated. Only that the generators will prepare the source\nimages, which will be duplicated when applications like Android\nadapters calls configure() multiple times.\n\n\n>\n> > +\n> > return 0;\n> > }\n> >\n> > @@ -207,9 +211,14 @@ void\n> PipelineHandlerVirtual::stopDevice([[maybe_unused]] Camera *camera)\n> > int PipelineHandlerVirtual::queueRequestDevice([[maybe_unused]] Camera\n> *camera,\n> > Request *request)\n> > {\n> > + VirtualCameraData *data = cameraData(camera);\n> > +\n> > /* \\todo Read from the virtual video if any. */\n> > - for (auto it : request->buffers())\n> > - completeBuffer(request, it.second);\n> > + for (auto const &[stream, buffer] : request->buffers()) {\n>\n> Unless something changes in the next patches, you only have one stream\n>\nLet's consider multiple-stream support: I tried to enable multiple streams\nin\nthe next version, while I failed to pass the unit test: multi_stream_test:\n```\n\n[295:43:43.910237144] [1441841] INFO IPAManager ipa_manager.cpp:137 libcamera\nis not installed. Adding\n'/usr/local/google/home/chenghaoyang/Workspace/libca\n\nmera/build/src/ipa' to the IPA search path\n\n[295:43:43.914030564] [1441841] INFO Camera camera_manager.cpp:325 libcamera\nv0.3.1+79-8ca4f033-dirty (2024-08-29T19:18:51UTC)\n\n[295:43:43.915493754] [1441844] ERROR DmaBufAllocator dma_buf_allocator.cpp:120\nCould not open any dma-buf provider\n\n[295:43:43.919118835] [1441841] INFO IPAManager ipa_manager.cpp:137 libcamera\nis not installed. Adding\n'/usr/local/google/home/chenghaoyang/Workspace/libca\n\nmera/build/src/ipa' to the IPA search path\n\n[295:43:43.922245825] [1441841] INFO Camera camera_manager.cpp:325 libcamera\nv0.3.1+79-8ca4f033-dirty (2024-08-29T19:18:51UTC)\n\n[295:43:43.922820175] [1441846] ERROR DmaBufAllocator dma_buf_allocator.cpp:120\nCould not open any dma-buf provider\n\nUnable to set the pipeline to the playing state.\n```\ngitlab pipeline link:\nhttps://gitlab.freedesktop.org/chenghaoyang/libcamera/-/pipelines/1260412\n\nLooking into `gsteramer_multi_stream_test.cpp`,\nI still couldn't figure out what's wrong...\nDo you know what's tested in this unit test? I know that\nDmaBufAllocator isn't valid, while I added logs and didn't\nfind `PipelineHandlerVirtual::exportFrameBuffers` being\ncalled.\n\nI also need your opinion: I think there's nothing, except for the unit test\n:p,\nto stop Virtual Pipeline Handler from supporting multiple streams, while\ndo you think there should be a limitation?\nI was setting the maximum to 3 for StillCapture, VideoRecording, and\nViewFinder, while you know there can be multiple streams as the same\nStreamRole...\n\n\n> > + /* map buffer and fill test patterns */\n> > +\n> data->frameGenerator_->generateFrame(stream->configuration().size, buffer);\n> > + completeBuffer(request, buffer);\n> > + }\n> >\n> > request->metadata().set(controls::SensorTimestamp,\n> currentTimestamp());\n> > completeRequest(request);\n> > @@ -241,11 +250,24 @@ bool\n> PipelineHandlerVirtual::match([[maybe_unused]] DeviceEnumerator *enumerator\n> > std::set streams{ &data->stream_ };\n> > const std::string id = \"Virtual0\";\n> > std::shared_ptr camera = Camera::create(std::move(data),\n> id, streams);\n> > +\n> > + initFrameGenerator(camera.get());\n> > +\n> > registerCamera(std::move(camera));\n> >\n> > return false; // Prevent infinite loops for now\n> > }\n> >\n> > +void PipelineHandlerVirtual::initFrameGenerator(Camera *camera)\n> > +{\n> > + auto data = cameraData(camera);\n> > + if (data->testPattern_ == TestPattern::DiagonalLines) {\n>\n> No {} for single lines statements\n>\nDone\n\n\n>\n> Who initializes data->testPattern_ ?\n>\nHmm, it'll be done in the next patch, when we read from the\nconfiguration file...\nIn this patch though, it's using the default value when we create\nCameraData in the match() function.\n\n\n> > + data->frameGenerator_ = DiagonalLinesGenerator::create();\n> > + } else {\n> > + data->frameGenerator_ = ColorBarsGenerator::create();\n> > + }\n> > +}\n> > +\n> > REGISTER_PIPELINE_HANDLER(PipelineHandlerVirtual, \"virtual\")\n> >\n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/pipeline/virtual/virtual.h\n> b/src/libcamera/pipeline/virtual/virtual.h\n> > index 6fc6b34d8..fecd9fa6f 100644\n> > --- a/src/libcamera/pipeline/virtual/virtual.h\n> > +++ b/src/libcamera/pipeline/virtual/virtual.h\n> > @@ -13,6 +13,8 @@\n> > #include \"libcamera/internal/dma_buf_allocator.h\"\n> > #include \"libcamera/internal/pipeline_handler.h\"\n> >\n> > +#include \"test_pattern_generator.h\"\n> > +\n> > namespace libcamera {\n> >\n> > class VirtualCameraData : public Camera::Private\n> > @@ -29,9 +31,13 @@ public:\n> >\n> > ~VirtualCameraData() = default;\n> >\n> > + TestPattern testPattern_;\n> > +\n> > std::vector supportedResolutions_;\n> >\n> > Stream stream_;\n> > +\n> > + std::unique_ptr frameGenerator_;\n> > };\n> >\n> > class VirtualCameraConfiguration : public CameraConfiguration\n> > @@ -72,6 +78,8 @@ private:\n> > return static_cast(camera->_d());\n> > }\n> >\n> > + void initFrameGenerator(Camera *camera);\n> > +\n> > DmaBufAllocator dmaBufAllocator_;\n> > };\n> >\n> > --\n> > 2.46.0.184.g6999bdac58-goog\n> >\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 0DBD8C324C\n\tfor ;\n\tThu, 29 Aug 2024 19:57:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B5AD0634C8;\n\tThu, 29 Aug 2024 21:57:51 +0200 (CEST)","from mail-lj1-x22b.google.com (mail-lj1-x22b.google.com\n\t[IPv6:2a00:1450:4864:20::22b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4520B63458\n\tfor ;\n\tThu, 29 Aug 2024 21:57:50 +0200 (CEST)","by mail-lj1-x22b.google.com with SMTP id\n\t38308e7fff4ca-2f3edb2d908so11184921fa.2\n\tfor ;\n\tThu, 29 Aug 2024 12:57:50 -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=\"F4+pjPqJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1724961469; x=1725566269;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=EbWJ6X8CYKKgZSjXrxo3Ht6Ivd1Xk2JFdX8wX8gKWgQ=;\n\tb=F4+pjPqJLEzRjQl+cUzyqaNVVWbkEa7xXP8HOmSv8wiOOpez1wWh5OkvE4vd/jG+oJ\n\tadamCEZ2D6lkHP2v77hpK6Pj+Del/ESxoO0clWw/IMsOuPEFrnAFX/cnI+Q8cBBXTDxK\n\tz933FljBGV3KYUYj5SxCLR1nyQO53jEu4vBSo=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1724961469; x=1725566269;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=EbWJ6X8CYKKgZSjXrxo3Ht6Ivd1Xk2JFdX8wX8gKWgQ=;\n\tb=aM8zouGh5qJIYu/zVBHgvrjHe3OmIQwohgNq5Ezw3CCCvBgxvToF0b4TFOny/IbYVx\n\tN2UGacPrsxl51JUqcmOUHjjOGEsVbBKyBUZYnYSh29VN55pX8PgiatSUsXMo1ohPKlxL\n\tvd5VuBhan6nSvmtTfT9Y2Kb7Y9uLOPQinqKFdk9jalNiotSa2kx3iytw4InqK4zuUphb\n\tZFw8MI6B5UQvafxEV8uKQeC49RujBgeAtG+BhkDdIDKI3fJ5acuBHiRlAbxTn5BlxwLo\n\tiABoADPJrTD3AIoT0JCJ9zDjVmi2RE0RADzFPrS3Ql+sc6o8g3EEB6V7Pcc033hVwR4P\n\t9rEA==","X-Gm-Message-State":"AOJu0YxJTygATKtJhRG+APPrzI1t6yS+w627aj/eI+Bo3Nd7iUOA+GhE\n\tiwJZEIthb3o/AE54eGpWswf6UwT15ntiuj5t1pFUJVDz78czbsPweuVAafMrSLfBIFWMBWLDoGa\n\ttKD+KeaDdmYbXwE3IuqUG8X100KINXQYIzePq","X-Google-Smtp-Source":"AGHT+IHbF67QiB4PWlGL4+D3Zllq0lFjTLIuMRS3Qk87RAqF1vTx8AUfDIzu1BZntmpLoUog+WAVXN0KpYmiydYCvFY=","X-Received":"by 2002:a2e:4e11:0:b0:2f3:eabc:d267 with SMTP id\n\t38308e7fff4ca-2f61092398bmr24982201fa.30.1724961469103;\n\tThu, 29 Aug 2024 12:57:49 -0700 (PDT)","MIME-Version":"1.0","References":"<20240820172202.526547-1-chenghaoyang@google.com>\n\t<20240820172202.526547-5-chenghaoyang@google.com>\n\t","In-Reply-To":"","From":"Cheng-Hao Yang ","Date":"Thu, 29 Aug 2024 21:57:38 +0200","Message-ID":"","Subject":"Re: [PATCH v9 4/8] libcamera: pipeline: Add test pattern for\n\tVirtualPipelineHandler","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org, Konami Shu , \n\tYunke Cao , Tomasz Figa ","Content-Type":"multipart/alternative; boundary=\"0000000000009e1a510620d7e3d2\"","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":31110,"web_url":"https://patchwork.libcamera.org/comment/31110/","msgid":"","date":"2024-09-07T14:34:11","subject":"Re: [PATCH v9 4/8] libcamera: pipeline: Add test pattern for\n\tVirtualPipelineHandler","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi Jacopo,\n\nOn Sat, Aug 31, 2024 at 9:50 PM Jacopo Mondi \nwrote:\n\n> Hi Harvey\n>\n> On Thu, Aug 29, 2024 at 09:57:38PM GMT, Cheng-Hao Yang wrote:\n> > Thanks Jacopo,\n> >\n> > On Tue, Aug 27, 2024 at 6:35 PM Jacopo Mondi <\n> jacopo.mondi@ideasonboard.com>\n> > wrote:\n> >\n> > > Hi Harvey, Konami\n> > >\n> > > On Tue, Aug 20, 2024 at 04:23:35PM GMT, Harvey Yang wrote:\n> > > > From: Konami Shu \n> > > >\n> > > > - There are two test patterns: color bars and diagonal lines\n> > > > - Add class for generating test patterns\n> > > > - Add libyuv to build dependencies\n> > > > - Make VirtualPipelineHandler show the test pattern\n> > > > - Format the code\n> > >\n> > > Could you make a commit message out of this please ? Like:\n> > >\n> > > Add a test pattern generator class hierarchy for the Virtual\n> > > pipeline handler.\n> > >\n> > > Implement two types of test patterns: color bars and diagonal lines\n> > > generator and use them in the Virtual pipeline handler.\n> > >\n> > > Add a dependency for libyuv to the build system to generate images\n> > > in NV12 format from the test pattern.\n> > >\n> > > Adopted, thanks!\n> >\n> >\n> > > >\n> > > > - Rename test_pattern to frame_generator\n> > > > - reflect comment\n> > > > - Fix const variable name\n> > > > - Use #pragma once\n> > > > - Make configure() private\n> > >\n> > > This list looks like a changelog more than a commit message. Could you\n> > > drop it or place it below\n> > >\n> > Removed.\n> >\n> >\n> > >\n> > > ---\n> > >\n> > > So that it doesn't appear in the git history ?\n> > >\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> > > > ---\n> > > > .../pipeline/virtual/frame_generator.h | 33 ++++++\n> > > > src/libcamera/pipeline/virtual/meson.build | 22 ++++\n> > > > .../virtual/test_pattern_generator.cpp | 112\n> ++++++++++++++++++\n> > > > .../pipeline/virtual/test_pattern_generator.h | 58 +++++++++\n> > > > src/libcamera/pipeline/virtual/virtual.cpp | 28 ++++-\n> > > > src/libcamera/pipeline/virtual/virtual.h | 8 ++\n> > > > 6 files changed, 258 insertions(+), 3 deletions(-)\n> > > > create mode 100644 src/libcamera/pipeline/virtual/frame_generator.h\n> > > > create mode 100644\n> > > src/libcamera/pipeline/virtual/test_pattern_generator.cpp\n> > > > create mode 100644\n> > > src/libcamera/pipeline/virtual/test_pattern_generator.h\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/virtual/frame_generator.h\n> > > b/src/libcamera/pipeline/virtual/frame_generator.h\n> > > > new file mode 100644\n> > > > index 000000000..9699af7a4\n> > > > --- /dev/null\n> > > > +++ b/src/libcamera/pipeline/virtual/frame_generator.h\n> > > > @@ -0,0 +1,33 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2023, Google Inc.\n> > > > + *\n> > > > + * frame_generator.h - Virtual cameras helper to generate frames\n> > > > + */\n> > > > +\n> > > > +#pragma once\n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > +namespace libcamera {\n> > > > +\n> > > > +class FrameGenerator\n> > > > +{\n> > > > +public:\n> > > > + virtual ~FrameGenerator() = default;\n> > > > +\n> > > > + /* Create buffers for using them in `generateFrame` */\n> > >\n> > > Usually we don't comment headers, but I don't mind\n> > >\n> > > Right, I think Konami put it here because there's no .cpp file for this\n> > virtual class. Removed.\n> >\n> >\n> > > > + virtual void configure(const Size &size) = 0;\n> > > > +\n> > > > + /** Fill the output frame buffer.\n> > > > + * Use the frame at the frameCount of image frames\n> > > > + */\n> > >\n> > > As long as the right commenting style is used: this is not Doxygen (no\n> > > /**) and I don't see references to frameCount anywhere ?\n> > >\n> > Removed.\n> >\n> >\n> > >\n> > > > + virtual void generateFrame(const Size &size,\n> > > > + const FrameBuffer *buffer) = 0;\n> > > > +\n> > > > +protected:\n> > > > + FrameGenerator() {}\n> > > > +};\n> > > > +\n> > > > +} /* namespace libcamera */\n> > > > diff --git a/src/libcamera/pipeline/virtual/meson.build\n> > > b/src/libcamera/pipeline/virtual/meson.build\n> > > > index ba7ff754e..e1e65e68d 100644\n> > > > --- a/src/libcamera/pipeline/virtual/meson.build\n> > > > +++ b/src/libcamera/pipeline/virtual/meson.build\n> > > > @@ -2,4 +2,26 @@\n> > > >\n> > > > libcamera_sources += files([\n> > > > 'virtual.cpp',\n> > > > + 'test_pattern_generator.cpp',\n> > > > ])\n> > > > +\n> > > > +libyuv_dep = dependency('libyuv', required : false)\n> > > > +\n> > > > +# Fallback to a subproject if libyuv isn't found, as it's typically\n> not\n> > > > +# provided by distributions.\n> > > > +if not libyuv_dep.found()\n> > >\n> > > The Android HAL has exactly the same snippet. I wonder if it should be\n> > > moved to the main libcamera build file and only add libyuv as a\n> > > dependency in the HAL and here. It might require a bit of\n> > > experimentation\n> > >\n> > Moved to `src/meson.build`. Please check.\n> >\n> >\n> > >\n> > > > + cmake = import('cmake')\n> > > > +\n> > > > + libyuv_vars = cmake.subproject_options()\n> > > > +\n> libyuv_vars.add_cmake_defines({'CMAKE_POSITION_INDEPENDENT_CODE':\n> > > 'ON'})\n> > > > + libyuv_vars.set_override_option('cpp_std', 'c++17')\n> > > > + libyuv_vars.append_compile_args('cpp',\n> > > > + '-Wno-sign-compare',\n> > > > + '-Wno-unused-variable',\n> > > > + '-Wno-unused-parameter')\n> > > > + libyuv_vars.append_link_args('-ljpeg')\n> > >\n> > > Do you need jpeg support ?\n> > >\n> > Well, in the 7th patch, yes. Also, as we try to use the same\n> > dependency with Android adapter, how about keeping jpeg\n> > here?\n> >\n> >\n> > >\n> > > > + libyuv = cmake.subproject('libyuv', options : libyuv_vars)\n> > > > + libyuv_dep = libyuv.dependency('yuv')\n> > > > +endif\n> > > > +\n> > > > +libcamera_deps += [libyuv_dep]\n> > > > diff --git\n> a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp\n> > > b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp\n> > > > new file mode 100644\n> > > > index 000000000..8dfe626e5\n> > > > --- /dev/null\n> > > > +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp\n> > > > @@ -0,0 +1,112 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2023, Google Inc.\n> > > > + *\n> > > > + * test_pattern_generator.cpp - Derived class of FrameGenerator for\n> > > > + * generating test patterns\n> > > > + */\n> > > > +\n> > > > +#include \"test_pattern_generator.h\"\n> > > > +\n> > > > +#include \n> > > > +\n> > > > +#include \"libcamera/internal/mapped_framebuffer.h\"\n> > > > +\n> > >\n> > > Empty line please\n> > >\n> > Done\n> >\n> >\n> > >\n> > > > +#include \"libyuv/convert_from_argb.h\"\n> > > > +namespace libcamera {\n> > > > +\n> > > > +LOG_DECLARE_CATEGORY(Virtual)\n> > > > +\n> > > > +static const unsigned int kARGBSize = 4;\n> > > > +\n> > > > +void TestPatternGenerator::generateFrame(\n> > > > + const Size &size,\n> > > > + const FrameBuffer *buffer)\n> > >\n> > > Weird indent\n> > >\n> > > void TestPatternGenerator::generateFrame(const Size &size,\n> > > const FrameBuffer *buffer)\n> > >\n> > >\n> > > Adopted.\n> >\n> >\n> > > > +{\n> > > > + MappedFrameBuffer mappedFrameBuffer(buffer,\n> > > > +\n> > > MappedFrameBuffer::MapFlag::Write);\n> > > > +\n> > > > + auto planes = mappedFrameBuffer.planes();\n> > > > +\n> > > > + /* Convert the template_ to the frame buffer */\n> > > > + int ret = libyuv::ARGBToNV12(\n> > >\n> > > As this produces NV12, the pipeline handler should only accept NV12\n> > > and adjust all other pixelformats. If I'm not mistaken this is not\n> > > enforced in validate() and you return SRGGB10 for Role::Raw (which you\n> > > shouldn't support afaict). This can cause issues as the buffers\n> > > allocated might be of a different size than the ones you expect\n> >\n> >\n> > Updated in the 3rd patch to remove the raw stream and ensure\n> formats::NV12\n> > for each stream.\n> >\n> >\n> > > ?\n> > >\n> > > I agree that generating patterns in ARGB is easier than NV12, but I\n> > > wonder why not use RGB888 as the pipeline output format directly so\n> > > that you don't have to depend on libyuv at all.\n> > >\n> >\n> > I think it's because the Android adapter requires mostly NV12 [1]\n> >\n> > [1]:\n> >\n> https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/upstream/src/android/camera_capabilities.cpp;l=68\n> >\n> >\n> > >\n> > >\n> > > > + template_.get(), /*src_stride_argb=*/size.width *\n> > > kARGBSize,\n> > >\n> > > Why a comment in the middle of the code ?\n> > >\n> > Removed.\n> >\n> >\n> > >\n> > > > + planes[0].begin(), size.width,\n> > > > + planes[1].begin(), size.width,\n> > > > + size.width, size.height);\n> > > > + if (ret != 0) {\n> > > > + LOG(Virtual, Error) << \"ARGBToNV12() failed with \" <<\n> ret;\n> > > > + }\n> > >\n> > > No {} for single line statements\n> > >\n> > Done\n> >\n> >\n> > >\n> > > > +}\n> > > > +\n> > > > +std::unique_ptr ColorBarsGenerator::create()\n> > > > +{\n> > > > + return std::make_unique();\n> > > > +}\n> > > > +\n> > > > +void ColorBarsGenerator::configure(const Size &size)\n> > > > +{\n> > > > + constexpr uint8_t kColorBar[8][3] = {\n> > > > + // R, G, B\n> > > > + { 0xff, 0xff, 0xff }, // White\n> > > > + { 0xff, 0xff, 0x00 }, // Yellow\n> > > > + { 0x00, 0xff, 0xff }, // Cyan\n> > > > + { 0x00, 0xff, 0x00 }, // Green\n> > > > + { 0xff, 0x00, 0xff }, // Magenta\n> > > > + { 0xff, 0x00, 0x00 }, // Red\n> > > > + { 0x00, 0x00, 0xff }, // Blue\n> > > > + { 0x00, 0x00, 0x00 }, // Black\n> > > > + };\n> > > > +\n> > > > + template_ = std::make_unique(\n> > > > + size.width * size.height * kARGBSize);\n> > > > +\n> > > > + unsigned int colorBarWidth = size.width / std::size(kColorBar);\n> > > > +\n> > > > + uint8_t *buf = template_.get();\n> > > > + for (size_t h = 0; h < size.height; h++) {\n> > > > + for (size_t w = 0; w < size.width; w++) {\n> > > > + // repeat when the width is exceed\n> > >\n> > > libcamera doesn't use C++ style comments\n> > >\n> > Updated.\n> >\n> >\n> > >\n> > > > + int index = (w / colorBarWidth) %\n> > > std::size(kColorBar);\n> > >\n> > > unsigned int\n> > >\n> > Done\n> >\n> >\n> > >\n> > > > +\n> > > > + *buf++ = kColorBar[index][2]; // B\n> > > > + *buf++ = kColorBar[index][1]; // G\n> > > > + *buf++ = kColorBar[index][0]; // R\n> > > > + *buf++ = 0x00; // A\n> > > > + }\n> > > > + }\n> > > > +}\n> > > > +\n> > > > +std::unique_ptr\n> DiagonalLinesGenerator::create()\n> > > > +{\n> > > > + return std::make_unique();\n> > > > +}\n> > > > +\n> > > > +void DiagonalLinesGenerator::configure(const Size &size)\n> > > > +{\n> > > > + constexpr uint8_t kColorBar[8][3] = {\n> > > > + // R, G, B\n> > > > + { 0xff, 0xff, 0xff }, // White\n> > > > + { 0x00, 0x00, 0x00 }, // Black\n> > > > + };\n> > > > +\n> > > > + template_ = std::make_unique(\n> > > > + size.width * size.height * kARGBSize);\n> > > > +\n> > > > + unsigned int lineWidth = size.width / 10;\n> > > > +\n> > > > + uint8_t *buf = template_.get();\n> > > > + for (size_t h = 0; h < size.height; h++) {\n> > > > + for (size_t w = 0; w < size.width; w++) {\n> > > > + // repeat when the width is exceed\n> > > > + int index = ((w + h) / lineWidth) % 2;\n> > >\n> > > same comments\n> > >\n> > Done\n> >\n> >\n> > >\n> > > > +\n> > > > + *buf++ = kColorBar[index][2]; // B\n> > > > + *buf++ = kColorBar[index][1]; // G\n> > > > + *buf++ = kColorBar[index][0]; // R\n> > > > + *buf++ = 0x00; // A\n> > > > + }\n> > > > + }\n> > > > +}\n> > > > +\n> > > > +} /* namespace libcamera */\n> > > > diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.h\n> > > b/src/libcamera/pipeline/virtual/test_pattern_generator.h\n> > > > new file mode 100644\n> > > > index 000000000..ed8d4e43b\n> > > > --- /dev/null\n> > > > +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.h\n> > > > @@ -0,0 +1,58 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2023, Google Inc.\n> > > > + *\n> > > > + * test_pattern_generator.h - Derived class of FrameGenerator for\n> > > > + * generating test patterns\n> > > > + */\n> > > > +\n> > > > +#pragma once\n> > > > +\n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > +#include \"frame_generator.h\"\n> > > > +\n> > > > +namespace libcamera {\n> > > > +\n> > > > +enum class TestPattern : char {\n> > > > + ColorBars = 0,\n> > > > + DiagonalLines = 1,\n> > > > +};\n> > > > +\n> > > > +class TestPatternGenerator : public FrameGenerator\n> > > > +{\n> > > > +private:\n> > > > + void generateFrame(const Size &size,\n> > > > + const FrameBuffer *buffer) override;\n> > >\n> > >\n> > > Maybe I don't get this, but this overrides a public function, and it's\n> > > called from the pipeline, why private ?\n> > >\n> > Hmm, it should be public. Updated.\n> >\n> >\n> > >\n> > > > +\n> > > > +protected:\n> > > > + /* Shift the buffer by 1 pixel left each frame */\n> > > > + void shiftLeft(const Size &size);\n> > >\n> > > Not implemented afaict\n> > >\n> > Removed\n> >\n> >\n> > > > + /* Buffer of test pattern template */\n> > > > + std::unique_ptr template_;\n> > > > +};\n> > > > +\n> > > > +class ColorBarsGenerator : public TestPatternGenerator\n> > > > +{\n> > > > +public:\n> > > > + static std::unique_ptr create();\n> > >\n> > > I won't question the class hierarchy design, but this could be done\n> > > with a simple public constructur and stored as unique_ptr in the\n> > > pipeline handler maybe.\n> > >\n> > The hierarchy design is because the 6th patch introduces the shift,\n> > which can be used on all test pattern generators.\n> > And yes, a static create function is not needed. Updated.\n> >\n> >\n> > >\n> > > > +\n> > > > +private:\n> > > > + /* Generate a template buffer of the color bar test pattern. */\n> > > > + void configure(const Size &size) override;\n> > >\n> > > Same questions as the one on generateFrame, it's surely something I\n> > > don't well understand then\n> > >\n> > > > +};\n> > > > +\n> > > > +class DiagonalLinesGenerator : public TestPatternGenerator\n> > > > +{\n> > > > +public:\n> > > > + static std::unique_ptr create();\n> > > > +\n> > > > +private:\n> > > > + /* Generate a template buffer of the diagonal lines test\n> pattern.\n> > > */\n> > > > + void configure(const Size &size) override;\n> > > > +};\n> > > > +\n> > > > +} /* namespace libcamera */\n> > > > diff --git a/src/libcamera/pipeline/virtual/virtual.cpp\n> > > b/src/libcamera/pipeline/virtual/virtual.cpp\n> > > > index 74eb8c7ad..357fdd035 100644\n> > > > --- a/src/libcamera/pipeline/virtual/virtual.cpp\n> > > > +++ b/src/libcamera/pipeline/virtual/virtual.cpp\n> > > > @@ -192,10 +192,14 @@ int PipelineHandlerVirtual::exportFrameBuffers(\n> > > > return dmaBufAllocator_.exportBuffers(config.bufferCount,\n> > > planeSizes, buffers);\n> > > > }\n> > > >\n> > > > -int PipelineHandlerVirtual::start([[maybe_unused]] Camera *camera,\n> > > > +int PipelineHandlerVirtual::start(Camera *camera,\n> > > > [[maybe_unused]] const ControlList\n> > > *controls)\n> > > > {\n> > > > /* \\todo Start reading the virtual video if any. */\n> > > > + VirtualCameraData *data = cameraData(camera);\n> > > > +\n> > > > +\n> > > data->frameGenerator_->configure(data->stream_.configuration().size);\n> > >\n> > > Shouldn't this be done at Pipeline::configure() ?\n> >\n> > Right, migrated. Only that the generators will prepare the source\n> > images, which will be duplicated when applications like Android\n> > adapters calls configure() multiple times.\n> >\n>\n> I see. It might be reasonable then, could you plese record the reason\n> with a comment ?\n>\n> >\n> > >\n> > > > +\n> > > > return 0;\n> > > > }\n> > > >\n> > > > @@ -207,9 +211,14 @@ void\n> > > PipelineHandlerVirtual::stopDevice([[maybe_unused]] Camera *camera)\n> > > > int PipelineHandlerVirtual::queueRequestDevice([[maybe_unused]]\n> Camera\n> > > *camera,\n> > > > Request *request)\n> > > > {\n> > > > + VirtualCameraData *data = cameraData(camera);\n> > > > +\n> > > > /* \\todo Read from the virtual video if any. */\n> > > > - for (auto it : request->buffers())\n> > > > - completeBuffer(request, it.second);\n> > > > + for (auto const &[stream, buffer] : request->buffers()) {\n> > >\n> > > Unless something changes in the next patches, you only have one stream\n> > >\n> > Let's consider multiple-stream support: I tried to enable multiple\n> streams\n> > in\n> > the next version, while I failed to pass the unit test:\n> multi_stream_test:\n>\n> Ah ok, that's answer my question about multi-stream support in this\n> version\n>\n> > ```\n> >\n> > [295:43:43.910237144] [1441841] INFO IPAManager ipa_manager.cpp:137\n> libcamera\n> > is not installed. Adding\n> > '/usr/local/google/home/chenghaoyang/Workspace/libca\n> >\n> > mera/build/src/ipa' to the IPA search path\n> >\n> > [295:43:43.914030564] [1441841] INFO Camera camera_manager.cpp:325\n> libcamera\n> > v0.3.1+79-8ca4f033-dirty (2024-08-29T19:18:51UTC)\n> >\n> > [295:43:43.915493754] [1441844] ERROR DmaBufAllocator\n> dma_buf_allocator.cpp:120\n> > Could not open any dma-buf provider\n> >\n> > [295:43:43.919118835] [1441841] INFO IPAManager ipa_manager.cpp:137\n> libcamera\n> > is not installed. Adding\n> > '/usr/local/google/home/chenghaoyang/Workspace/libca\n> >\n> > mera/build/src/ipa' to the IPA search path\n> >\n> > [295:43:43.922245825] [1441841] INFO Camera camera_manager.cpp:325\n> libcamera\n> > v0.3.1+79-8ca4f033-dirty (2024-08-29T19:18:51UTC)\n> >\n> > [295:43:43.922820175] [1441846] ERROR DmaBufAllocator\n> dma_buf_allocator.cpp:120\n> > Could not open any dma-buf provider\n> >\n> > Unable to set the pipeline to the playing state.\n> > ```\n> > gitlab pipeline link:\n> >\n> https://gitlab.freedesktop.org/chenghaoyang/libcamera/-/pipelines/1260412\n> >\n> > Looking into `gsteramer_multi_stream_test.cpp`,\n> > I still couldn't figure out what's wrong...\n> > Do you know what's tested in this unit test? I know that\n> > DmaBufAllocator isn't valid, while I added logs and didn't\n> > find `PipelineHandlerVirtual::exportFrameBuffers` being\n> > called.\n> >\n>\n> The above error message comes from the construction of the class\n> member:\n>\n> src/libcamera/pipeline/virtual/virtual.h: DmaBufAllocator\n> dmaBufAllocator_;\n>\n> While gstreamer fails for multi stream but not for single, I'm not\n> sure..\n>\n>\nYeah, and as I mentioned above,\n`PipelineHandlerVirtual::exportFrameBuffers` is never called.\nDmaBufAllocator doesn't seem to be the root cause...\n\n\n>\n> > I also need your opinion: I think there's nothing, except for the unit\n> test\n> > :p,\n> > to stop Virtual Pipeline Handler from supporting multiple streams, while\n> > do you think there should be a limitation?\n>\n> No I just thought this is what you were aiming to\n>\n\nI mean, do we want to limit the max number of streams, say 3, or we can\nsupport any number of streams?\n\n\n>\n> > I was setting the maximum to 3 for StillCapture, VideoRecording, and\n> > ViewFinder, while you know there can be multiple streams as the same\n> > StreamRole...\n> >\n> >\n> > > > + /* map buffer and fill test patterns */\n> > > > +\n> > > data->frameGenerator_->generateFrame(stream->configuration().size,\n> buffer);\n> > > > + completeBuffer(request, buffer);\n> > > > + }\n> > > >\n> > > > request->metadata().set(controls::SensorTimestamp,\n> > > currentTimestamp());\n> > > > completeRequest(request);\n> > > > @@ -241,11 +250,24 @@ bool\n> > > PipelineHandlerVirtual::match([[maybe_unused]] DeviceEnumerator\n> *enumerator\n> > > > std::set streams{ &data->stream_ };\n> > > > const std::string id = \"Virtual0\";\n> > > > std::shared_ptr camera =\n> Camera::create(std::move(data),\n> > > id, streams);\n> > > > +\n> > > > + initFrameGenerator(camera.get());\n> > > > +\n> > > > registerCamera(std::move(camera));\n> > > >\n> > > > return false; // Prevent infinite loops for now\n> > > > }\n> > > >\n> > > > +void PipelineHandlerVirtual::initFrameGenerator(Camera *camera)\n> > > > +{\n> > > > + auto data = cameraData(camera);\n> > > > + if (data->testPattern_ == TestPattern::DiagonalLines) {\n> > >\n> > > No {} for single lines statements\n> > >\n> > Done\n> >\n> >\n> > >\n> > > Who initializes data->testPattern_ ?\n> > >\n> > Hmm, it'll be done in the next patch, when we read from the\n> > configuration file...\n> > In this patch though, it's using the default value when we create\n> > CameraData in the match() function.\n>\n> afaik enum variables are not initialized to any default value\n>\n> in example\n>\n> -------------------------------------------------------------------------------\n> #include \n>\n> enum class Status : int {\n> FrameSuccess,\n> FrameError,\n> FrameCancelled,\n> };\n>\n> int main(int argc, char *argv[])\n> {\n> Status stat;\n>\n> std::cout << static_cast(stat) << std::endl;\n>\n> return 0;\n> }\n>\n> -------------------------------------------------------------------------------\n>\n> prints a random (?) number (32766)\n>\n>\nI think putting an enum in a class as a member variable\nsets a default value, but sure, we can give it one in the\nheader file.\n\nBTW, I compiled and ran your code with g++ and clang++.\nBoth constantly showed 0 instead of random numbers...\n\n\n> >\n> >\n> > > > + data->frameGenerator_ =\n> DiagonalLinesGenerator::create();\n> > > > + } else {\n> > > > + data->frameGenerator_ = ColorBarsGenerator::create();\n> > > > + }\n> > > > +}\n> > > > +\n> > > > REGISTER_PIPELINE_HANDLER(PipelineHandlerVirtual, \"virtual\")\n> > > >\n> > > > } /* namespace libcamera */\n> > > > diff --git a/src/libcamera/pipeline/virtual/virtual.h\n> > > b/src/libcamera/pipeline/virtual/virtual.h\n> > > > index 6fc6b34d8..fecd9fa6f 100644\n> > > > --- a/src/libcamera/pipeline/virtual/virtual.h\n> > > > +++ b/src/libcamera/pipeline/virtual/virtual.h\n> > > > @@ -13,6 +13,8 @@\n> > > > #include \"libcamera/internal/dma_buf_allocator.h\"\n> > > > #include \"libcamera/internal/pipeline_handler.h\"\n> > > >\n> > > > +#include \"test_pattern_generator.h\"\n> > > > +\n> > > > namespace libcamera {\n> > > >\n> > > > class VirtualCameraData : public Camera::Private\n> > > > @@ -29,9 +31,13 @@ public:\n> > > >\n> > > > ~VirtualCameraData() = default;\n> > > >\n> > > > + TestPattern testPattern_;\n> > > > +\n> > > > std::vector supportedResolutions_;\n> > > >\n> > > > Stream stream_;\n> > > > +\n> > > > + std::unique_ptr frameGenerator_;\n> > > > };\n> > > >\n> > > > class VirtualCameraConfiguration : public CameraConfiguration\n> > > > @@ -72,6 +78,8 @@ private:\n> > > > return static_cast(camera->_d());\n> > > > }\n> > > >\n> > > > + void initFrameGenerator(Camera *camera);\n> > > > +\n> > > > DmaBufAllocator dmaBufAllocator_;\n> > > > };\n> > > >\n> > > > --\n> > > > 2.46.0.184.g6999bdac58-goog\n> > > >\n> > >\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 193ACC324C\n\tfor ;\n\tSat, 7 Sep 2024 14:34:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B1D58634F8;\n\tSat, 7 Sep 2024 16:34:24 +0200 (CEST)","from mail-lj1-x236.google.com (mail-lj1-x236.google.com\n\t[IPv6:2a00:1450:4864:20::236])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1661C633CC\n\tfor ;\n\tSat, 7 Sep 2024 16:34:23 +0200 (CEST)","by mail-lj1-x236.google.com with SMTP id\n\t38308e7fff4ca-2f3e9fb6ee9so37329951fa.3\n\tfor ;\n\tSat, 07 Sep 2024 07:34:23 -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=\"Ex4eGIaz\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1725719662; x=1726324462;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=+JQKoqICYBTIunQA5ebMlMi3VVY9I01RTrjs2uZ2TrE=;\n\tb=Ex4eGIaz3xdkZXiAY9buuGVbZio+giwOjTAnE0U1u7y2pq3lRruajjsIlfYrTBo9Ga\n\t0w5SaffzQY/Q7cn/a09OtDWAiY5WYCJoiv1ApKm8DJZmhCyE7fa9jvB2s32dcFnTRUpm\n\tleliM1zGZ+HgDzb7++ocJEc62TIwqh1zXTpCQ=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1725719662; x=1726324462;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=+JQKoqICYBTIunQA5ebMlMi3VVY9I01RTrjs2uZ2TrE=;\n\tb=k6r73MZ86mpMs1hfrgE/5NI41ps3d3jfLCGCL2AriZxKtYcO7zrfCfu1fyaGaYBxfB\n\tB/SKzGBx4Z1f/xKeyp+nldJMDy2IJZiSKc8w06D4psMHnE7VkPg408wlnQs9Xzl36Hz3\n\twTrYdoZM1LvfcDa1vIJ/pz5VKxB6QfqXeuQw0KAE1IYSzncTv4zwh3xWQlLu9MtV8IPU\n\tqqhwIJG+aG9X/6Hr+vlAzSztfpHYeYE717dlVeiB4+2HybHwbU5XzQPTXntHAlYjpgmw\n\tL+oyKsqXu/XmaNSGufj79Tw1Bon/aA5neITr4cUBGJUPJugtD7InoWywm/jHB7CM+ChZ\n\tphcQ==","X-Gm-Message-State":"AOJu0YzA4Xn8kjTxOHmPQyAzpY4vqCmdnlYzvXYLe0NtLc38wDzOUs/m\n\tfwNQeIRMHH3ajK17sxXr9rhMies0QQ2Ppgo0s2iO6TjuTwyUt0uYPCEJGiyLCQqQ8zidKPk8rMa\n\tk2CPxyzRFe06ouwd81ahtT8a2L+U+nz4ZMcz1","X-Google-Smtp-Source":"AGHT+IEZr2tL6G/vKr4fnzpWB0cKmqMwsfOWTwX6cIl97hnIf0jpU+plgVESfozlaxfCnjfFe6U6zFBpCSP1J7+1s98=","X-Received":"by 2002:a2e:7006:0:b0:2f3:b71a:1e91 with SMTP id\n\t38308e7fff4ca-2f751ef717bmr33050481fa.17.1725719662087;\n\tSat, 07 Sep 2024 07:34:22 -0700 (PDT)","MIME-Version":"1.0","References":"<20240820172202.526547-1-chenghaoyang@google.com>\n\t<20240820172202.526547-5-chenghaoyang@google.com>\n\t\n\t\n\t","In-Reply-To":"","From":"Cheng-Hao Yang ","Date":"Sat, 7 Sep 2024 22:34:11 +0800","Message-ID":"","Subject":"Re: [PATCH v9 4/8] libcamera: pipeline: Add test pattern for\n\tVirtualPipelineHandler","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org, Konami Shu , \n\tYunke Cao , Tomasz Figa ","Content-Type":"multipart/alternative; boundary=\"00000000000070ef7f0621886b4c\"","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\" "}}]