<div dir="ltr"><div dir="ltr">Hi Jacopo,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Sep 9, 2024 at 5:06 PM Jacopo Mondi <<a href="mailto:jacopo.mondi@ideasonboard.com" target="_blank">jacopo.mondi@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi<br>
<br>
On Sat, Sep 07, 2024 at 02:28:29PM GMT, Harvey Yang wrote:<br>
> From: Konami Shu <<a href="mailto:konamiz@google.com" target="_blank">konamiz@google.com</a>><br>
><br>
> Add a test pattern generator class hierarchy for the Virtual<br>
> pipeline handler.<br>
><br>
> Implement two types of test patterns: color bars and diagonal lines<br>
> generator and use them in the Virtual pipeline handler.<br>
><br>
> Add a dependency for libyuv to the build system to generate images<br>
> in NV12 format from the test pattern.<br>
><br>
> Signed-off-by: Konami Shu <<a href="mailto:konamiz@google.com" target="_blank">konamiz@google.com</a>><br>
> Co-developed-by: Harvey Yang <<a href="mailto:chenghaoyang@chromium.org" target="_blank">chenghaoyang@chromium.org</a>><br>
> Co-developed-by: Yunke Cao <<a href="mailto:yunkec@chromium.org" target="_blank">yunkec@chromium.org</a>><br>
> Co-developed-by: Tomasz Figa <<a href="mailto:tfiga@chromium.org" target="_blank">tfiga@chromium.org</a>><br>
><br>
> libcamera: pipeline: Shift test pattern by 1 pixel left every frame<br>
><br>
> - This write the buffer every frame<br>
> - Shifting makes the frame rate dropped from about 160 to 40<br>
><br>
> Patchset1->2<br>
> - Use constant instead of using a magic number<br>
><br>
> Patchset2->3<br>
> - Make shiftLeft() private<br>
><br>
> Signed-off-by: Konami Shu <<a href="mailto:konamiz@google.com" target="_blank">konamiz@google.com</a>><br>
> Co-developed-by: Harvey Yang <<a href="mailto:chenghaoyang@chromium.org" target="_blank">chenghaoyang@chromium.org</a>><br>
> Co-developed-by: Yunke Cao <<a href="mailto:yunkec@chromium.org" target="_blank">yunkec@chromium.org</a>><br>
> Co-developed-by: Tomasz Figa <<a href="mailto:tfiga@chromium.org" target="_blank">tfiga@chromium.org</a>><br>
<br>
This is not a well-formed commit message. When you squash two patches,<br>
make a single commit message out of them<br></blockquote><div><br></div><div>Right, sorry. Updated.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> ---<br>
>  src/android/meson.build  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â |  19 ---<br>
>  .../pipeline/virtual/frame_generator.h  Â  Â  Â  |  29 ++++<br>
>  src/libcamera/pipeline/virtual/meson.build  Â  |  Â 3 +<br>
>  .../virtual/test_pattern_generator.cpp  Â  Â  Â  | 140 ++++++++++++++++++<br>
>  .../pipeline/virtual/test_pattern_generator.h |  57 +++++++<br>
>  src/libcamera/pipeline/virtual/virtual.cpp  Â  |  38 ++++-<br>
>  src/libcamera/pipeline/virtual/virtual.h  Â  Â  |  Â 7 +<br>
>  src/meson.build  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â |  19 +++<br>
>  8 files changed, 290 insertions(+), 22 deletions(-)<br>
>  create mode 100644 src/libcamera/pipeline/virtual/frame_generator.h<br>
>  create mode 100644 src/libcamera/pipeline/virtual/test_pattern_generator.cpp<br>
>  create mode 100644 src/libcamera/pipeline/virtual/test_pattern_generator.h<br>
><br>
> diff --git a/src/android/meson.build b/src/android/meson.build<br>
> index 68646120..6341ee8b 100644<br>
> --- a/src/android/meson.build<br>
> +++ b/src/android/meson.build<br>
> @@ -15,25 +15,6 @@ foreach dep : android_deps<br>
>  Â  Â  endif<br>
>  endforeach<br>
><br>
> -libyuv_dep = dependency('libyuv', required : false)<br>
> -<br>
> -# Fallback to a subproject if libyuv isn't found, as it's typically not<br>
> -# provided by distributions.<br>
> -if not libyuv_dep.found()<br>
> -  Â  cmake = import('cmake')<br>
> -<br>
> -  Â  libyuv_vars = cmake.subproject_options()<br>
> -  Â  libyuv_vars.add_cmake_defines({'CMAKE_POSITION_INDEPENDENT_CODE': 'ON'})<br>
> -  Â  libyuv_vars.set_override_option('cpp_std', 'c++17')<br>
> -  Â  libyuv_vars.append_compile_args('cpp',<br>
> -  Â  Â  Â  Â '-Wno-sign-compare',<br>
> -  Â  Â  Â  Â '-Wno-unused-variable',<br>
> -  Â  Â  Â  Â '-Wno-unused-parameter')<br>
> -  Â  libyuv_vars.append_link_args('-ljpeg')<br>
> -  Â  libyuv = cmake.subproject('libyuv', options : libyuv_vars)<br>
> -  Â  libyuv_dep = libyuv.dependency('yuv')<br>
> -endif<br>
> -<br>
<br>
I thought moving this to the virtual/ directory would have break<br>
building Android without building the Virtual pipline. Turns out it<br>
doesn't, as iirc, all symbols are global in meson, so<br></blockquote><div><br></div><div>Actually I moved it to `src/meson.build`, instead of to the `virtual/`</div><div>directory, so it's expected that `src/android/meson.build` finds</div><div>libyuv_dep again.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
>  android_deps += [libyuv_dep]<br>
<br>
this is enough to pull-in libyuv as a dependency for android even if<br>
the definition is somewhere else!<br>
<br>
><br>
>  android_hal_sources = files([<br>
> diff --git a/src/libcamera/pipeline/virtual/frame_generator.h b/src/libcamera/pipeline/virtual/frame_generator.h<br>
> new file mode 100644<br>
> index 00000000..d8727b8f<br>
> --- /dev/null<br>
> +++ b/src/libcamera/pipeline/virtual/frame_generator.h<br>
> @@ -0,0 +1,29 @@<br>
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */<br>
> +/*<br>
> + * Copyright (C) 2024, Google Inc.<br>
> + *<br>
> + * frame_generator.h - Virtual cameras helper to generate frames<br>
> + */<br>
> +<br>
> +#pragma once<br>
> +<br>
> +#include <libcamera/framebuffer.h><br>
> +#include <libcamera/geometry.h><br>
> +<br>
> +namespace libcamera {<br>
> +<br>
> +class FrameGenerator<br>
> +{<br>
> +public:<br>
> +  Â  Â virtual ~FrameGenerator() = default;<br>
> +<br>
> +  Â  Â virtual void configure(const Size &size) = 0;<br>
> +<br>
> +  Â  Â virtual void generateFrame(const Size &size,<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  const FrameBuffer *buffer) = 0;<br>
> +<br>
> +protected:<br>
> +  Â  Â FrameGenerator() {}<br>
> +};<br>
> +<br>
> +} /* namespace libcamera */<br>
> diff --git a/src/libcamera/pipeline/virtual/meson.build b/src/libcamera/pipeline/virtual/meson.build<br>
> index ada1b335..0c537777 100644<br>
> --- a/src/libcamera/pipeline/virtual/meson.build<br>
> +++ b/src/libcamera/pipeline/virtual/meson.build<br>
> @@ -1,5 +1,8 @@<br>
>  # SPDX-License-Identifier: CC0-1.0<br>
><br>
>  libcamera_internal_sources += files([<br>
> +  Â  'test_pattern_generator.cpp',<br>
>  Â  Â  'virtual.cpp',<br>
>  ])<br>
> +<br>
> +libcamera_deps += [libyuv_dep]<br>
> diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp<br>
> new file mode 100644<br>
> index 00000000..7094818e<br>
> --- /dev/null<br>
> +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp<br>
> @@ -0,0 +1,140 @@<br>
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */<br>
> +/*<br>
> + * Copyright (C) 2024, Google Inc.<br>
> + *<br>
> + * test_pattern_generator.cpp - Derived class of FrameGenerator for<br>
> + * generating test patterns<br>
> + */<br>
> +<br>
> +#include "test_pattern_generator.h"<br>
> +<br>
> +#include <libcamera/base/log.h><br>
> +<br>
> +#include "libcamera/internal/mapped_framebuffer.h"<br>
> +<br>
> +#include "libyuv/convert_from_argb.h"<br>
<br>
Why use the "" include directive variant ?<br></blockquote><div><br></div><div>Updated to <> instead.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> +<br>
> +namespace libcamera {<br>
> +<br>
> +LOG_DECLARE_CATEGORY(Virtual)<br>
> +<br>
> +static const unsigned int kARGBSize = 4;<br>
> +<br>
> +void TestPatternGenerator::generateFrame(const Size &size,<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  const FrameBuffer *buffer)<br>
> +{<br>
> +  Â  Â MappedFrameBuffer mappedFrameBuffer(buffer,<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â MappedFrameBuffer::MapFlag::Write);<br>
> +<br>
> +  Â  Â auto planes = mappedFrameBuffer.planes();<br>
> +<br>
> +  Â  Â shiftLeft(size);<br>
> +<br>
> +  Â  Â /* Convert the template_ to the frame buffer */<br>
> +  Â  Â int ret = libyuv::ARGBToNV12(<br>
> +  Â  Â  Â  Â  Â  Â template_.get(), size.width * kARGBSize,<br>
> +  Â  Â  Â  Â  Â  Â planes[0].begin(), size.width,<br>
> +  Â  Â  Â  Â  Â  Â planes[1].begin(), size.width,<br>
> +  Â  Â  Â  Â  Â  Â size.width, size.height);<br>
<br>
You can easily use a more conventional indendation style<br>
<br>
  Â  Â  Â  int ret = libyuv::ARGBToNV12(template_.get(), size.width * kARGBSize,<br>
  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â planes[0].begin(), size.width,<br>
  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â planes[1].begin(), size.width,<br>
  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â size.width, size.height);<br>
<br></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +  Â  Â if (ret != 0)<br>
> +  Â  Â  Â  Â  Â  Â LOG(Virtual, Error) << "ARGBToNV12() failed with " << ret;<br>
> +}<br>
> +<br>
> +void TestPatternGenerator::shiftLeft(const Size &size)<br>
> +{<br>
<br>
This function basically copies the whole pattern at every frame.<br>
As this is a testing pipeline I think it's fine, but it's certainly<br>
consuming.<br></blockquote><div><br></div><div>Yeah... We could probably come up with an approach to swap</div><div>only one column, while libyuv::ARGBToNV12 seems to require</div><div>continuous memory for the source, let's leave it as is for now.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> +  Â  Â /* Store the first column temporarily */<br>
> +  Â  Â auto firstColumn = std::make_unique<uint8_t[]>(size.height * kARGBSize);<br>
> +  Â  Â for (size_t h = 0; h < size.height; h++) {<br>
> +  Â  Â  Â  Â  Â  Â unsigned int index = h * size.width * kARGBSize;<br>
> +  Â  Â  Â  Â  Â  Â unsigned int index1 = h * kARGBSize;<br>
> +  Â  Â  Â  Â  Â  Â firstColumn[index1] = template_[index];<br>
> +  Â  Â  Â  Â  Â  Â firstColumn[index1 + 1] = template_[index + 1];<br>
> +  Â  Â  Â  Â  Â  Â firstColumn[index1 + 2] = template_[index + 2];<br>
> +  Â  Â  Â  Â  Â  Â firstColumn[index1 + 3] = 0x00;<br>
> +  Â  Â }<br>
> +<br>
> +  Â  Â /* Overwrite template_ */<br>
> +  Â  Â uint8_t *buf = template_.get();<br>
> +  Â  Â for (size_t h = 0; h < size.height; h++) {<br>
> +  Â  Â  Â  Â  Â  Â for (size_t w = 0; w < size.width - 1; w++) {<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â /* Overwrite with the pixel on the right */<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â unsigned int index = (h * size.width + w + 1) * kARGBSize;<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â *buf++ = template_[index]; // B<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â *buf++ = template_[index + 1]; // G<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â *buf++ = template_[index + 2]; // R<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â *buf++ = 0x00; // A<br>
> +  Â  Â  Â  Â  Â  Â }<br>
> +  Â  Â  Â  Â  Â  Â /* Overwrite the new last column with the original first column */<br>
> +  Â  Â  Â  Â  Â  Â unsigned int index1 = h * kARGBSize;<br>
> +  Â  Â  Â  Â  Â  Â *buf++ = firstColumn[index1]; // B<br>
> +  Â  Â  Â  Â  Â  Â *buf++ = firstColumn[index1 + 1]; // G<br>
> +  Â  Â  Â  Â  Â  Â *buf++ = firstColumn[index1 + 2]; // R<br>
> +  Â  Â  Â  Â  Â  Â *buf++ = 0x00; // A<br>
> +  Â  Â }<br>
> +}<br>
> +<br>
> +ColorBarsGenerator::ColorBarsGenerator() = default;<br>
<br>
Do you need this and the below one ?<br>
<br></blockquote><div><br></div><div>Removed the declarations and definitions.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +<br>
> +void ColorBarsGenerator::configure(const Size &size)<br>
> +{<br>
> +  Â  Â constexpr uint8_t kColorBar[8][3] = {<br>
> +  Â  Â  Â  Â  Â  Â //  R,  Â  G,  Â  B<br>
<br>
Could you please make sure, once and for all, that in your next<br>
patches there won't be // comments ?<br>
<br></blockquote><div><br></div><div>Yeah sorry. Updated.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +  Â  Â  Â  Â  Â  Â { 0xff, 0xff, 0xff }, // White<br>
> +  Â  Â  Â  Â  Â  Â { 0xff, 0xff, 0x00 }, // Yellow<br>
> +  Â  Â  Â  Â  Â  Â { 0x00, 0xff, 0xff }, // Cyan<br>
> +  Â  Â  Â  Â  Â  Â { 0x00, 0xff, 0x00 }, // Green<br>
> +  Â  Â  Â  Â  Â  Â { 0xff, 0x00, 0xff }, // Magenta<br>
> +  Â  Â  Â  Â  Â  Â { 0xff, 0x00, 0x00 }, // Red<br>
> +  Â  Â  Â  Â  Â  Â { 0x00, 0x00, 0xff }, // Blue<br>
> +  Â  Â  Â  Â  Â  Â { 0x00, 0x00, 0x00 }, // Black<br>
> +  Â  Â };<br>
> +<br>
> +  Â  Â template_ = std::make_unique<uint8_t[]>(<br>
> +  Â  Â  Â  Â  Â  Â size.width * size.height * kARGBSize);<br>
> +<br>
> +  Â  Â unsigned int colorBarWidth = size.width / std::size(kColorBar);<br>
> +<br>
> +  Â  Â uint8_t *buf = template_.get();<br>
> +  Â  Â for (size_t h = 0; h < size.height; h++) {<br>
> +  Â  Â  Â  Â  Â  Â for (size_t w = 0; w < size.width; w++) {<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â /* Repeat when the width is exceed */<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â unsigned int index = (w / colorBarWidth) % std::size(kColorBar);<br>
> +<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â *buf++ = kColorBar[index][2]; /* B */<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â *buf++ = kColorBar[index][1]; /* G */<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â *buf++ = kColorBar[index][0]; /* R */<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â *buf++ = 0x00; /* A */<br>
> +  Â  Â  Â  Â  Â  Â }<br>
> +  Â  Â }<br>
> +}<br>
> +<br>
> +DiagonalLinesGenerator::DiagonalLinesGenerator() = default;<br>
> +<br>
> +void DiagonalLinesGenerator::configure(const Size &size)<br>
> +{<br>
> +  Â  Â constexpr uint8_t kColorBar[8][3] = {<br>
> +  Â  Â  Â  Â  Â  Â /*  R,  Â  G,  Â  B */<br>
> +  Â  Â  Â  Â  Â  Â { 0xff, 0xff, 0xff }, /* White */<br>
> +  Â  Â  Â  Â  Â  Â { 0x00, 0x00, 0x00 }, /* Black */<br>
> +  Â  Â };<br>
> +<br>
> +  Â  Â template_ = std::make_unique<uint8_t[]>(<br>
> +  Â  Â  Â  Â  Â  Â size.width * size.height * kARGBSize);<br>
> +<br>
> +  Â  Â unsigned int lineWidth = size.width / 10;<br>
> +<br>
> +  Â  Â uint8_t *buf = template_.get();<br>
> +  Â  Â for (size_t h = 0; h < size.height; h++) {<br>
> +  Â  Â  Â  Â  Â  Â for (size_t w = 0; w < size.width; w++) {<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â /* Repeat when the width is exceed */<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â int index = ((w + h) / lineWidth) % 2;<br>
> +<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â *buf++ = kColorBar[index][2]; /* B */<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â *buf++ = kColorBar[index][1]; /* G */<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â *buf++ = kColorBar[index][0]; /* R */<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â *buf++ = 0x00; /* A */<br>
> +  Â  Â  Â  Â  Â  Â }<br>
> +  Â  Â }<br>
> +}<br>
> +<br>
> +} /* namespace libcamera */<br>
> diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.h b/src/libcamera/pipeline/virtual/test_pattern_generator.h<br>
> new file mode 100644<br>
> index 00000000..11bcb5f0<br>
> --- /dev/null<br>
> +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.h<br>
> @@ -0,0 +1,57 @@<br>
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */<br>
> +/*<br>
> + * Copyright (C) 2024, Google Inc.<br>
> + *<br>
> + * test_pattern_generator.h - Derived class of FrameGenerator for<br>
> + * generating test patterns<br>
> + */<br>
> +<br>
> +#pragma once<br>
> +<br>
> +#include <memory><br>
> +<br>
> +#include <libcamera/framebuffer.h><br>
> +#include <libcamera/geometry.h><br>
> +<br>
> +#include "frame_generator.h"<br>
> +<br>
> +namespace libcamera {<br>
> +<br>
> +enum class TestPattern : char {<br>
> +  Â  Â ColorBars = 0,<br>
> +  Â  Â DiagonalLines = 1,<br>
> +};<br>
> +<br>
> +class TestPatternGenerator : public FrameGenerator<br>
> +{<br>
> +public:<br>
> +  Â  Â void generateFrame(const Size &size, const FrameBuffer *buffer) override;<br>
> +<br>
> +protected:<br>
> +  Â  Â /* Buffer of test pattern template */<br>
> +  Â  Â std::unique_ptr<uint8_t[]> template_;<br>
> +<br>
> +private:<br>
> +  Â  Â /* Shift the buffer by 1 pixel left each frame */<br>
> +  Â  Â void shiftLeft(const Size &size);<br>
> +};<br>
> +<br>
> +class ColorBarsGenerator : public TestPatternGenerator<br>
> +{<br>
> +public:<br>
> +  Â  Â ColorBarsGenerator();<br>
<br>
You can drop this and rely on the default constructor generated by the<br>
compiler<br>
<br></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +<br>
> +  Â  Â /* Generate a template buffer of the color bar test pattern. */<br>
> +  Â  Â void configure(const Size &size) override;<br>
> +};<br>
> +<br>
> +class DiagonalLinesGenerator : public TestPatternGenerator<br>
> +{<br>
> +public:<br>
> +  Â  Â DiagonalLinesGenerator();<br>
<br>
ditto<br>
<br></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +<br>
> +  Â  Â /* Generate a template buffer of the diagonal lines test pattern. */<br>
> +  Â  Â void configure(const Size &size) override;<br>
> +};<br>
> +<br>
> +} /* namespace libcamera */<br>
> diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp<br>
> index f85ec3dd..6e64aeee 100644<br>
> --- a/src/libcamera/pipeline/virtual/virtual.cpp<br>
> +++ b/src/libcamera/pipeline/virtual/virtual.cpp<br>
> @@ -173,8 +173,11 @@ int PipelineHandlerVirtual::configure(Camera *camera,<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â CameraConfiguration *config)<br>
>  {<br>
>  Â  Â  Â VirtualCameraData *data = cameraData(camera);<br>
> -  Â  Â for (size_t i = 0; i < config->size(); ++i)<br>
> +  Â  Â for (size_t i = 0; i < config->size(); ++i) {<br>
>  Â  Â  Â  Â  Â  Â  Â config->at(i).setStream(&data->streamConfigs_[i].stream);<br>
> +  Â  Â  Â  Â  Â  Â data->streamConfigs_[i].frameGenerator->configure(<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â data->streamConfigs_[i].stream.configuration().size);<br>
<br>
If you use enumerate() as previously suggested you will be able to<br>
access the StreamConfiguration more easily<br></blockquote><div><br></div><div>Done, and this fixes a bug that</div><div>`data->streamConfigs_[i].stream.configuration()` hasn't been set yet.</div><div>Thanks!</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> +  Â  Â }<br>
><br>
>  Â  Â  Â return 0;<br>
>  }<br>
> @@ -210,9 +213,24 @@ void PipelineHandlerVirtual::stopDevice([[maybe_unused]] Camera *camera)<br>
>  int PipelineHandlerVirtual::queueRequestDevice([[maybe_unused]] Camera *camera,<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Request *request)<br>
>  {<br>
> +  Â  Â VirtualCameraData *data = cameraData(camera);<br>
> +<br>
>  Â  Â  Â /* \todo Read from the virtual video if any. */<br>
<br>
What's this "virtual video" ?<br>
<br></blockquote><div><br></div><div>It means the potential video support. In the next patches we support</div><div>image loading, while video loading is nice to have.</div><div><br></div><div>Maybe the todo comment is redundant?</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> -  Â  Â for (auto it : request->buffers())<br>
> -  Â  Â  Â  Â  Â  Â completeBuffer(request, it.second);<br>
> +  Â  Â for (auto const &[stream, buffer] : request->buffers()) {<br>
> +  Â  Â  Â  Â  Â  Â bool found = false;<br>
> +  Â  Â  Â  Â  Â  Â /* map buffer and fill test patterns */<br>
> +  Â  Â  Â  Â  Â  Â for (auto &streamConfig : data->streamConfigs_) {<br>
<br>
Now sure how VirtualCameraData::StreamConfig will grow in the next<br>
patches, but an std::map indexed by libcamera::Stream * migth be<br>
easier here<br>
<br></blockquote><div><br></div><div>Hmm, as we assign the streams to StreamConfigurations based on the</div><div>order of the streams (VirtualCameraData::streamConfigs_), would it be</div><div>a bit weird to depend on the order of a map? Just curious :p</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â if (stream == &streamConfig.stream) {<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â found = true;<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â streamConfig.frameGenerator->generateFrame(<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â stream->configuration().size, buffer);<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â completeBuffer(request, buffer);<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â break;<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â }<br>
> +  Â  Â  Â  Â  Â  Â }<br>
> +  Â  Â  Â  Â  Â  Â if (!found)<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â LOG(Virtual, Fatal) << "Stream not defined";<br>
<br>
This can' happen, unless there's a big issue that should be spotted<br>
during the development phase. You can ASSERT() I presume<br>
<br></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> +  Â  Â }<br>
><br>
>  Â  Â  Â request->metadata().set(controls::SensorTimestamp, currentTimestamp());<br>
>  Â  Â  Â completeRequest(request);<br>
> @@ -257,11 +275,25 @@ bool PipelineHandlerVirtual::match([[maybe_unused]] DeviceEnumerator *enumerator<br>
><br>
>  Â  Â  Â const std::string id = "Virtual0";<br>
>  Â  Â  Â std::shared_ptr<Camera> camera = Camera::create(std::move(data), id, streams);<br>
> +<br>
> +  Â  Â initFrameGenerator(camera.get());<br>
> +<br>
>  Â  Â  Â registerCamera(std::move(camera));<br>
><br>
>  Â  Â  Â return true;<br>
>  }<br>
><br>
> +void PipelineHandlerVirtual::initFrameGenerator(Camera *camera)<br>
> +{<br>
> +  Â  Â auto data = cameraData(camera);<br>
> +  Â  Â for (auto &streamConfig : data->streamConfigs_) {<br>
> +  Â  Â  Â  Â  Â  Â if (data->testPattern_ == TestPattern::DiagonalLines)<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â streamConfig.frameGenerator = std::make_unique<DiagonalLinesGenerator>();<br>
> +  Â  Â  Â  Â  Â  Â else<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â streamConfig.frameGenerator = std::make_unique<ColorBarsGenerator>();<br>
> +  Â  Â }<br>
> +}<br>
> +<br>
>  REGISTER_PIPELINE_HANDLER(PipelineHandlerVirtual, "virtual")<br>
><br>
>  } /* namespace libcamera */<br>
> diff --git a/src/libcamera/pipeline/virtual/virtual.h b/src/libcamera/pipeline/virtual/virtual.h<br>
> index fb3dbcad..09d73051 100644<br>
> --- a/src/libcamera/pipeline/virtual/virtual.h<br>
> +++ b/src/libcamera/pipeline/virtual/virtual.h<br>
> @@ -16,6 +16,8 @@<br>
>  #include "libcamera/internal/dma_buf_allocator.h"<br>
>  #include "libcamera/internal/pipeline_handler.h"<br>
><br>
> +#include "test_pattern_generator.h"<br>
> +<br>
>  namespace libcamera {<br>
><br>
>  class VirtualCameraData : public Camera::Private<br>
> @@ -29,6 +31,7 @@ public:<br>
>  Â  Â  Â };<br>
>  Â  Â  Â struct StreamConfig {<br>
>  Â  Â  Â  Â  Â  Â  Â Stream stream;<br>
> +  Â  Â  Â  Â  Â  Â std::unique_ptr<FrameGenerator> frameGenerator;<br>
>  Â  Â  Â };<br>
><br>
>  Â  Â  Â VirtualCameraData(PipelineHandler *pipe,<br>
> @@ -36,6 +39,8 @@ public:<br>
><br>
>  Â  Â  Â ~VirtualCameraData() = default;<br>
><br>
> +  Â  Â TestPattern testPattern_ = TestPattern::ColorBars;<br>
> +<br>
>  Â  Â  Â const std::vector<Resolution> supportedResolutions_;<br>
>  Â  Â  Â Size maxResolutionSize_;<br>
>  Â  Â  Â Size minResolutionSize_;<br>
> @@ -83,6 +88,8 @@ private:<br>
>  Â  Â  Â  Â  Â  Â  Â return static_cast<VirtualCameraData *>(camera->_d());<br>
>  Â  Â  Â }<br>
><br>
> +  Â  Â void initFrameGenerator(Camera *camera);<br>
> +<br>
>  Â  Â  Â DmaBufAllocator dmaBufAllocator_;<br>
>  };<br>
><br>
> diff --git a/src/meson.build b/src/meson.build<br>
> index 165a77bb..91bea775 100644<br>
> --- a/src/meson.build<br>
> +++ b/src/meson.build<br>
> @@ -27,6 +27,25 @@ else<br>
>  Â  Â  ipa_sign_module = false<br>
>  endif<br>
><br>
> +libyuv_dep = dependency('libyuv', required : false)<br>
> +<br>
> +# Fallback to a subproject if libyuv isn't found, as it's typically not<br>
> +# provided by distributions.<br>
> +if not libyuv_dep.found()<br>
> +  Â  cmake = import('cmake')<br>
> +<br>
> +  Â  libyuv_vars = cmake.subproject_options()<br>
> +  Â  libyuv_vars.add_cmake_defines({'CMAKE_POSITION_INDEPENDENT_CODE': 'ON'})<br>
> +  Â  libyuv_vars.set_override_option('cpp_std', 'c++17')<br>
> +  Â  libyuv_vars.append_compile_args('cpp',<br>
> +  Â  Â  Â  Â '-Wno-sign-compare',<br>
> +  Â  Â  Â  Â '-Wno-unused-variable',<br>
> +  Â  Â  Â  Â '-Wno-unused-parameter')<br>
> +  Â  libyuv_vars.append_link_args('-ljpeg')<br>
> +  Â  libyuv = cmake.subproject('libyuv', options : libyuv_vars)<br>
> +  Â  libyuv_dep = libyuv.dependency('yuv')<br>
> +endif<br>
> +<br>
<br>
I think it's better to have this in src/meson.build, yes<br>
<br>
Mostly minors<br>
Reviewed-by: Jacopo Mondi <<a href="mailto:jacopo.mondi@ideasonboard.com" target="_blank">jacopo.mondi@ideasonboard.com</a>><br>
<br>
Thanks<br>
  j<br>
<br>
<br>
>  # libcamera must be built first as a dependency to the other components.<br>
>  subdir('libcamera')<br>
><br>
> --<br>
> 2.46.0.469.g59c65b2a67-goog<br>
><br>
</blockquote></div></div>