<div dir="ltr"><div dir="ltr">Hi Jacopo,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sat, Aug 31, 2024 at 9:50 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 Harvey<br>
<br>
On Thu, Aug 29, 2024 at 09:57:38PM GMT, Cheng-Hao Yang wrote:<br>
> Thanks Jacopo,<br>
><br>
> On Tue, Aug 27, 2024 at 6:35 PM Jacopo Mondi <<a href="mailto:jacopo.mondi@ideasonboard.com" target="_blank">jacopo.mondi@ideasonboard.com</a>><br>
> wrote:<br>
><br>
> > Hi Harvey, Konami<br>
> ><br>
> > On Tue, Aug 20, 2024 at 04:23:35PM GMT, Harvey Yang wrote:<br>
> > > From: Konami Shu <<a href="mailto:konamiz@google.com" target="_blank">konamiz@google.com</a>><br>
> > ><br>
> > > - There are two test patterns: color bars and diagonal lines<br>
> > > - Add class for generating test patterns<br>
> > > - Add libyuv to build dependencies<br>
> > > - Make VirtualPipelineHandler show the test pattern<br>
> > > - Format the code<br>
> ><br>
> > Could you make a commit message out of this please ? Like:<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>
> > Adopted, thanks!<br>
><br>
><br>
> > ><br>
> > > - Rename test_pattern to frame_generator<br>
> > > - reflect comment<br>
> > > - Fix const variable name<br>
> > > - Use #pragma once<br>
> > > - Make configure() private<br>
> ><br>
> > This list looks like a changelog more than a commit message. Could you<br>
> > drop it or place it below<br>
> ><br>
> Removed.<br>
><br>
><br>
> ><br>
> > ---<br>
> ><br>
> > So that it doesn't appear in the git history ?<br>
> ><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>
> > > .../pipeline/virtual/frame_generator.h | 33 ++++++<br>
> > > src/libcamera/pipeline/virtual/meson.build | 22 ++++<br>
> > > .../virtual/test_pattern_generator.cpp | 112 ++++++++++++++++++<br>
> > > .../pipeline/virtual/test_pattern_generator.h | 58 +++++++++<br>
> > > src/libcamera/pipeline/virtual/virtual.cpp | 28 ++++-<br>
> > > src/libcamera/pipeline/virtual/virtual.h | 8 ++<br>
> > > 6 files changed, 258 insertions(+), 3 deletions(-)<br>
> > > create mode 100644 src/libcamera/pipeline/virtual/frame_generator.h<br>
> > > create mode 100644<br>
> > src/libcamera/pipeline/virtual/test_pattern_generator.cpp<br>
> > > create mode 100644<br>
> > src/libcamera/pipeline/virtual/test_pattern_generator.h<br>
> > ><br>
> > > diff --git a/src/libcamera/pipeline/virtual/frame_generator.h<br>
> > b/src/libcamera/pipeline/virtual/frame_generator.h<br>
> > > new file mode 100644<br>
> > > index 000000000..9699af7a4<br>
> > > --- /dev/null<br>
> > > +++ b/src/libcamera/pipeline/virtual/frame_generator.h<br>
> > > @@ -0,0 +1,33 @@<br>
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */<br>
> > > +/*<br>
> > > + * Copyright (C) 2023, 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>
> > > + /* Create buffers for using them in `generateFrame` */<br>
> ><br>
> > Usually we don't comment headers, but I don't mind<br>
> ><br>
> > Right, I think Konami put it here because there's no .cpp file for this<br>
> virtual class. Removed.<br>
><br>
><br>
> > > + virtual void configure(const Size &size) = 0;<br>
> > > +<br>
> > > + /** Fill the output frame buffer.<br>
> > > + * Use the frame at the frameCount of image frames<br>
> > > + */<br>
> ><br>
> > As long as the right commenting style is used: this is not Doxygen (no<br>
> > /**) and I don't see references to frameCount anywhere ?<br>
> ><br>
> Removed.<br>
><br>
><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<br>
> > b/src/libcamera/pipeline/virtual/meson.build<br>
> > > index ba7ff754e..e1e65e68d 100644<br>
> > > --- a/src/libcamera/pipeline/virtual/meson.build<br>
> > > +++ b/src/libcamera/pipeline/virtual/meson.build<br>
> > > @@ -2,4 +2,26 @@<br>
> > ><br>
> > > libcamera_sources += files([<br>
> > > 'virtual.cpp',<br>
> > > + 'test_pattern_generator.cpp',<br>
> > > ])<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>
> ><br>
> > The Android HAL has exactly the same snippet. I wonder if it should be<br>
> > moved to the main libcamera build file and only add libyuv as a<br>
> > dependency in the HAL and here. It might require a bit of<br>
> > experimentation<br>
> ><br>
> Moved to `src/meson.build`. Please check.<br>
><br>
><br>
> ><br>
> > > + cmake = import('cmake')<br>
> > > +<br>
> > > + libyuv_vars = cmake.subproject_options()<br>
> > > + libyuv_vars.add_cmake_defines({'CMAKE_POSITION_INDEPENDENT_CODE':<br>
> > '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>
> ><br>
> > Do you need jpeg support ?<br>
> ><br>
> Well, in the 7th patch, yes. Also, as we try to use the same<br>
> dependency with Android adapter, how about keeping jpeg<br>
> here?<br>
><br>
><br>
> ><br>
> > > + libyuv = cmake.subproject('libyuv', options : libyuv_vars)<br>
> > > + libyuv_dep = libyuv.dependency('yuv')<br>
> > > +endif<br>
> > > +<br>
> > > +libcamera_deps += [libyuv_dep]<br>
> > > diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp<br>
> > b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp<br>
> > > new file mode 100644<br>
> > > index 000000000..8dfe626e5<br>
> > > --- /dev/null<br>
> > > +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp<br>
> > > @@ -0,0 +1,112 @@<br>
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */<br>
> > > +/*<br>
> > > + * Copyright (C) 2023, 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>
> ><br>
> > Empty line please<br>
> ><br>
> Done<br>
><br>
><br>
> ><br>
> > > +#include "libyuv/convert_from_argb.h"<br>
> > > +namespace libcamera {<br>
> > > +<br>
> > > +LOG_DECLARE_CATEGORY(Virtual)<br>
> > > +<br>
> > > +static const unsigned int kARGBSize = 4;<br>
> > > +<br>
> > > +void TestPatternGenerator::generateFrame(<br>
> > > + const Size &size,<br>
> > > + const FrameBuffer *buffer)<br>
> ><br>
> > Weird indent<br>
> ><br>
> > void TestPatternGenerator::generateFrame(const Size &size,<br>
> > const FrameBuffer *buffer)<br>
> ><br>
> ><br>
> > Adopted.<br>
><br>
><br>
> > > +{<br>
> > > + MappedFrameBuffer mappedFrameBuffer(buffer,<br>
> > > +<br>
> > MappedFrameBuffer::MapFlag::Write);<br>
> > > +<br>
> > > + auto planes = mappedFrameBuffer.planes();<br>
> > > +<br>
> > > + /* Convert the template_ to the frame buffer */<br>
> > > + int ret = libyuv::ARGBToNV12(<br>
> ><br>
> > As this produces NV12, the pipeline handler should only accept NV12<br>
> > and adjust all other pixelformats. If I'm not mistaken this is not<br>
> > enforced in validate() and you return SRGGB10 for Role::Raw (which you<br>
> > shouldn't support afaict). This can cause issues as the buffers<br>
> > allocated might be of a different size than the ones you expect<br>
><br>
><br>
> Updated in the 3rd patch to remove the raw stream and ensure formats::NV12<br>
> for each stream.<br>
><br>
><br>
> > ?<br>
> ><br>
> > I agree that generating patterns in ARGB is easier than NV12, but I<br>
> > wonder why not use RGB888 as the pipeline output format directly so<br>
> > that you don't have to depend on libyuv at all.<br>
> ><br>
><br>
> I think it's because the Android adapter requires mostly NV12 [1]<br>
><br>
> [1]:<br>
> <a href="https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/upstream/src/android/camera_capabilities.cpp;l=68" rel="noreferrer" target="_blank">https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/upstream/src/android/camera_capabilities.cpp;l=68</a><br>
><br>
><br>
> ><br>
> ><br>
> > > + template_.get(), /*src_stride_argb=*/size.width *<br>
> > kARGBSize,<br>
> ><br>
> > Why a comment in the middle of the code ?<br>
> ><br>
> Removed.<br>
><br>
><br>
> ><br>
> > > + planes[0].begin(), size.width,<br>
> > > + planes[1].begin(), size.width,<br>
> > > + size.width, size.height);<br>
> > > + if (ret != 0) {<br>
> > > + LOG(Virtual, Error) << "ARGBToNV12() failed with " << ret;<br>
> > > + }<br>
> ><br>
> > No {} for single line statements<br>
> ><br>
> Done<br>
><br>
><br>
> ><br>
> > > +}<br>
> > > +<br>
> > > +std::unique_ptr<ColorBarsGenerator> ColorBarsGenerator::create()<br>
> > > +{<br>
> > > + return std::make_unique<ColorBarsGenerator>();<br>
> > > +}<br>
> > > +<br>
> > > +void ColorBarsGenerator::configure(const Size &size)<br>
> > > +{<br>
> > > + constexpr uint8_t kColorBar[8][3] = {<br>
> > > + // R, G, B<br>
> > > + { 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>
> ><br>
> > libcamera doesn't use C++ style comments<br>
> ><br>
> Updated.<br>
><br>
><br>
> ><br>
> > > + int index = (w / colorBarWidth) %<br>
> > std::size(kColorBar);<br>
> ><br>
> > unsigned int<br>
> ><br>
> Done<br>
><br>
><br>
> ><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>
> > > +std::unique_ptr<DiagonalLinesGenerator> DiagonalLinesGenerator::create()<br>
> > > +{<br>
> > > + return std::make_unique<DiagonalLinesGenerator>();<br>
> > > +}<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>
> > same comments<br>
> ><br>
> Done<br>
><br>
><br>
> ><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<br>
> > b/src/libcamera/pipeline/virtual/test_pattern_generator.h<br>
> > > new file mode 100644<br>
> > > index 000000000..ed8d4e43b<br>
> > > --- /dev/null<br>
> > > +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.h<br>
> > > @@ -0,0 +1,58 @@<br>
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */<br>
> > > +/*<br>
> > > + * Copyright (C) 2023, 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>
> > > +private:<br>
> > > + void generateFrame(const Size &size,<br>
> > > + const FrameBuffer *buffer) override;<br>
> ><br>
> ><br>
> > Maybe I don't get this, but this overrides a public function, and it's<br>
> > called from the pipeline, why private ?<br>
> ><br>
> Hmm, it should be public. Updated.<br>
><br>
><br>
> ><br>
> > > +<br>
> > > +protected:<br>
> > > + /* Shift the buffer by 1 pixel left each frame */<br>
> > > + void shiftLeft(const Size &size);<br>
> ><br>
> > Not implemented afaict<br>
> ><br>
> Removed<br>
><br>
><br>
> > > + /* Buffer of test pattern template */<br>
> > > + std::unique_ptr<uint8_t[]> template_;<br>
> > > +};<br>
> > > +<br>
> > > +class ColorBarsGenerator : public TestPatternGenerator<br>
> > > +{<br>
> > > +public:<br>
> > > + static std::unique_ptr<ColorBarsGenerator> create();<br>
> ><br>
> > I won't question the class hierarchy design, but this could be done<br>
> > with a simple public constructur and stored as unique_ptr in the<br>
> > pipeline handler maybe.<br>
> ><br>
> The hierarchy design is because the 6th patch introduces the shift,<br>
> which can be used on all test pattern generators.<br>
> And yes, a static create function is not needed. Updated.<br>
><br>
><br>
> ><br>
> > > +<br>
> > > +private:<br>
> > > + /* Generate a template buffer of the color bar test pattern. */<br>
> > > + void configure(const Size &size) override;<br>
> ><br>
> > Same questions as the one on generateFrame, it's surely something I<br>
> > don't well understand then<br>
> ><br>
> > > +};<br>
> > > +<br>
> > > +class DiagonalLinesGenerator : public TestPatternGenerator<br>
> > > +{<br>
> > > +public:<br>
> > > + static std::unique_ptr<DiagonalLinesGenerator> create();<br>
> > > +<br>
> > > +private:<br>
> > > + /* Generate a template buffer of the diagonal lines test pattern.<br>
> > */<br>
> > > + void configure(const Size &size) override;<br>
> > > +};<br>
> > > +<br>
> > > +} /* namespace libcamera */<br>
> > > diff --git a/src/libcamera/pipeline/virtual/virtual.cpp<br>
> > b/src/libcamera/pipeline/virtual/virtual.cpp<br>
> > > index 74eb8c7ad..357fdd035 100644<br>
> > > --- a/src/libcamera/pipeline/virtual/virtual.cpp<br>
> > > +++ b/src/libcamera/pipeline/virtual/virtual.cpp<br>
> > > @@ -192,10 +192,14 @@ int PipelineHandlerVirtual::exportFrameBuffers(<br>
> > > return dmaBufAllocator_.exportBuffers(config.bufferCount,<br>
> > planeSizes, buffers);<br>
> > > }<br>
> > ><br>
> > > -int PipelineHandlerVirtual::start([[maybe_unused]] Camera *camera,<br>
> > > +int PipelineHandlerVirtual::start(Camera *camera,<br>
> > > [[maybe_unused]] const ControlList<br>
> > *controls)<br>
> > > {<br>
> > > /* \todo Start reading the virtual video if any. */<br>
> > > + VirtualCameraData *data = cameraData(camera);<br>
> > > +<br>
> > > +<br>
> > data->frameGenerator_->configure(data->stream_.configuration().size);<br>
> ><br>
> > Shouldn't this be done at Pipeline::configure() ?<br>
><br>
> Right, migrated. Only that the generators will prepare the source<br>
> images, which will be duplicated when applications like Android<br>
> adapters calls configure() multiple times.<br>
><br>
<br>
I see. It might be reasonable then, could you plese record the reason<br>
with a comment ?<br>
<br>
><br>
> ><br>
> > > +<br>
> > > return 0;<br>
> > > }<br>
> > ><br>
> > > @@ -207,9 +211,14 @@ void<br>
> > PipelineHandlerVirtual::stopDevice([[maybe_unused]] Camera *camera)<br>
> > > int PipelineHandlerVirtual::queueRequestDevice([[maybe_unused]] Camera<br>
> > *camera,<br>
> > > Request *request)<br>
> > > {<br>
> > > + VirtualCameraData *data = cameraData(camera);<br>
> > > +<br>
> > > /* \todo Read from the virtual video if any. */<br>
> > > - for (auto it : request->buffers())<br>
> > > - completeBuffer(request, it.second);<br>
> > > + for (auto const &[stream, buffer] : request->buffers()) {<br>
> ><br>
> > Unless something changes in the next patches, you only have one stream<br>
> ><br>
> Let's consider multiple-stream support: I tried to enable multiple streams<br>
> in<br>
> the next version, while I failed to pass the unit test: multi_stream_test:<br>
<br>
Ah ok, that's answer my question about multi-stream support in this<br>
version<br>
<br>
> ```<br>
><br>
> [295:43:43.910237144] [1441841] INFO IPAManager ipa_manager.cpp:137 libcamera<br>
> is not installed. Adding<br>
> '/usr/local/google/home/chenghaoyang/Workspace/libca<br>
><br>
> mera/build/src/ipa' to the IPA search path<br>
><br>
> [295:43:43.914030564] [1441841] INFO Camera camera_manager.cpp:325 libcamera<br>
> v0.3.1+79-8ca4f033-dirty (2024-08-29T19:18:51UTC)<br>
><br>
> [295:43:43.915493754] [1441844] ERROR DmaBufAllocator dma_buf_allocator.cpp:120<br>
> Could not open any dma-buf provider<br>
><br>
> [295:43:43.919118835] [1441841] INFO IPAManager ipa_manager.cpp:137 libcamera<br>
> is not installed. Adding<br>
> '/usr/local/google/home/chenghaoyang/Workspace/libca<br>
><br>
> mera/build/src/ipa' to the IPA search path<br>
><br>
> [295:43:43.922245825] [1441841] INFO Camera camera_manager.cpp:325 libcamera<br>
> v0.3.1+79-8ca4f033-dirty (2024-08-29T19:18:51UTC)<br>
><br>
> [295:43:43.922820175] [1441846] ERROR DmaBufAllocator dma_buf_allocator.cpp:120<br>
> Could not open any dma-buf provider<br>
><br>
> Unable to set the pipeline to the playing state.<br>
> ```<br>
> gitlab pipeline link:<br>
> <a href="https://gitlab.freedesktop.org/chenghaoyang/libcamera/-/pipelines/1260412" rel="noreferrer" target="_blank">https://gitlab.freedesktop.org/chenghaoyang/libcamera/-/pipelines/1260412</a><br>
><br>
> Looking into `gsteramer_multi_stream_test.cpp`,<br>
> I still couldn't figure out what's wrong...<br>
> Do you know what's tested in this unit test? I know that<br>
> DmaBufAllocator isn't valid, while I added logs and didn't<br>
> find `PipelineHandlerVirtual::exportFrameBuffers` being<br>
> called.<br>
><br>
<br>
The above error message comes from the construction of the class<br>
member:<br>
<br>
src/libcamera/pipeline/virtual/virtual.h: DmaBufAllocator dmaBufAllocator_;<br>
<br>
While gstreamer fails for multi stream but not for single, I'm not<br>
sure..<br>
<br></blockquote><div><br></div><div>Yeah, and as I mentioned above,</div><div>`PipelineHandlerVirtual::exportFrameBuffers` is never called.</div><div>DmaBufAllocator doesn't seem to be the root cause...</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>
> I also need your opinion: I think there's nothing, except for the unit test<br>
> :p,<br>
> to stop Virtual Pipeline Handler from supporting multiple streams, while<br>
> do you think there should be a limitation?<br>
<br>
No I just thought this is what you were aiming to<br></blockquote><div><br></div><div>I mean, do we want to limit the max number of streams, say 3, or we can</div><div>support any number of streams?</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>
> I was setting the maximum to 3 for StillCapture, VideoRecording, and<br>
> ViewFinder, while you know there can be multiple streams as the same<br>
> StreamRole...<br>
><br>
><br>
> > > + /* map buffer and fill test patterns */<br>
> > > +<br>
> > data->frameGenerator_->generateFrame(stream->configuration().size, buffer);<br>
> > > + completeBuffer(request, buffer);<br>
> > > + }<br>
> > ><br>
> > > request->metadata().set(controls::SensorTimestamp,<br>
> > currentTimestamp());<br>
> > > completeRequest(request);<br>
> > > @@ -241,11 +250,24 @@ bool<br>
> > PipelineHandlerVirtual::match([[maybe_unused]] DeviceEnumerator *enumerator<br>
> > > std::set<Stream *> streams{ &data->stream_ };<br>
> > > const std::string id = "Virtual0";<br>
> > > std::shared_ptr<Camera> camera = Camera::create(std::move(data),<br>
> > id, streams);<br>
> > > +<br>
> > > + initFrameGenerator(camera.get());<br>
> > > +<br>
> > > registerCamera(std::move(camera));<br>
> > ><br>
> > > return false; // Prevent infinite loops for now<br>
> > > }<br>
> > ><br>
> > > +void PipelineHandlerVirtual::initFrameGenerator(Camera *camera)<br>
> > > +{<br>
> > > + auto data = cameraData(camera);<br>
> > > + if (data->testPattern_ == TestPattern::DiagonalLines) {<br>
> ><br>
> > No {} for single lines statements<br>
> ><br>
> Done<br>
><br>
><br>
> ><br>
> > Who initializes data->testPattern_ ?<br>
> ><br>
> Hmm, it'll be done in the next patch, when we read from the<br>
> configuration file...<br>
> In this patch though, it's using the default value when we create<br>
> CameraData in the match() function.<br>
<br>
afaik enum variables are not initialized to any default value<br>
<br>
in example<br>
-------------------------------------------------------------------------------<br>
#include <iostream><br>
<br>
enum class Status : int {<br>
FrameSuccess,<br>
FrameError,<br>
FrameCancelled,<br>
};<br>
<br>
int main(int argc, char *argv[])<br>
{<br>
Status stat;<br>
<br>
std::cout << static_cast<int>(stat) << std::endl;<br>
<br>
return 0;<br>
}<br>
-------------------------------------------------------------------------------<br>
<br>
prints a random (?) number (32766)<br>
<br></blockquote><div><br></div><div>I think putting an enum in a class as a member variable</div><div>sets a default value, but sure, we can give it one in the</div><div>header file.</div><div><br></div><div>BTW, I compiled and ran your code with g++ and clang++.</div><div>Both constantly showed 0 instead of random numbers...</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>
> > > + data->frameGenerator_ = DiagonalLinesGenerator::create();<br>
> > > + } else {<br>
> > > + data->frameGenerator_ = ColorBarsGenerator::create();<br>
> > > + }<br>
> > > +}<br>
> > > +<br>
> > > REGISTER_PIPELINE_HANDLER(PipelineHandlerVirtual, "virtual")<br>
> > ><br>
> > > } /* namespace libcamera */<br>
> > > diff --git a/src/libcamera/pipeline/virtual/virtual.h<br>
> > b/src/libcamera/pipeline/virtual/virtual.h<br>
> > > index 6fc6b34d8..fecd9fa6f 100644<br>
> > > --- a/src/libcamera/pipeline/virtual/virtual.h<br>
> > > +++ b/src/libcamera/pipeline/virtual/virtual.h<br>
> > > @@ -13,6 +13,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,9 +31,13 @@ public:<br>
> > ><br>
> > > ~VirtualCameraData() = default;<br>
> > ><br>
> > > + TestPattern testPattern_;<br>
> > > +<br>
> > > std::vector<Resolution> supportedResolutions_;<br>
> > ><br>
> > > Stream stream_;<br>
> > > +<br>
> > > + std::unique_ptr<FrameGenerator> frameGenerator_;<br>
> > > };<br>
> > ><br>
> > > class VirtualCameraConfiguration : public CameraConfiguration<br>
> > > @@ -72,6 +78,8 @@ private:<br>
> > > return static_cast<VirtualCameraData *>(camera->_d());<br>
> > > }<br>
> > ><br>
> > > + void initFrameGenerator(Camera *camera);<br>
> > > +<br>
> > > DmaBufAllocator dmaBufAllocator_;<br>
> > > };<br>
> > ><br>
> > > --<br>
> > > 2.46.0.184.g6999bdac58-goog<br>
> > ><br>
> ><br>
</blockquote></div></div>