[RFC PATCH v1 11/12] apps: lc-compliance: Support multiple streams in helpers
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Jan 10 10:41:35 CET 2025
On Fri, Jan 10, 2025 at 09:27:24AM +0000, Barnabás Pőcze wrote:
> Hi
>
>
> 2025. január 10., péntek 2:32 keltezéssel, Laurent Pinchart <laurent.pinchart at ideasonboard.com> írta:
>
> > Hi Barnabás,
> >
> > Thank you for the patch.
> >
> > On Fri, Dec 20, 2024 at 03:08:51PM +0000, Barnabás Pőcze wrote:
> > > Prepare to add a test suite for capture operations with multiple
> > > streams.
> > >
> > > Modify the Capture helper class to support multiple roles and streams
> > > in the configure() and capture() operations.
> > >
> > > Multi-stream support will be added in next patches.
> > >
> > > Co-developed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> > > Signed-off-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> > > Signed-off-by: Barnabás Pőcze <pobrn at protonmail.com>
> > > ---
> > > src/apps/lc-compliance/helpers/capture.cpp | 83 ++++++++++++++-----
> > > src/apps/lc-compliance/helpers/capture.h | 2 +-
> > > src/apps/lc-compliance/tests/capture_test.cpp | 6 +-
> > > 3 files changed, 66 insertions(+), 25 deletions(-)
> > >
> > > diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp
> > > index 7a05be9a3..38edb6f28 100644
> > > --- a/src/apps/lc-compliance/helpers/capture.cpp
> > > +++ b/src/apps/lc-compliance/helpers/capture.cpp
> > > @@ -24,15 +24,29 @@ Capture::~Capture()
> > > stop();
> > > }
> > >
> > > -void Capture::configure(StreamRole role)
> > > +void Capture::configure(libcamera::Span<const libcamera::StreamRole> roles)
> > > {
> > > - config_ = camera_->generateConfiguration({ role });
> > > + assert(!roles.empty());
> > > +
> > > + config_ = camera_->generateConfiguration(roles);
> > >
> > > if (!config_) {
> > > std::cout << "Role not supported by camera" << std::endl;
> > > GTEST_SKIP();
> > > }
> > >
> > > + /*
> > > + * Set the buffers count to the largest value across all streams.
> > > + * \todo: Should all streams from a Camera have the same buffer count ?
> > > + */
> > > + auto largest =
> > > + std::max_element(config_->begin(), config_->end(),
> > > + [](const StreamConfiguration &l, const StreamConfiguration &r)
> > > + { return l.bufferCount < r.bufferCount; });
> >
> > Would this (untested) be clearer/simpler ?
> >
> > unsigned int maxBufferCount =
> > std::reduce(config_->begin(), config_->end(), 0,
> > [](unsigned int a, const StreamConfiguration &b) {
> > return std::max(a, b->bufferCount);
> > });
>
> To me both look about the same in terms of readability.
>
>
> >
> > > +
> > > + for (auto &cfg : *config_)
> > > + cfg.bufferCount = largest->bufferCount;
> > > +
> > > if (config_->validate() != CameraConfiguration::Valid) {
> > > config_.reset();
> > > FAIL() << "Configuration not valid";
> > > @@ -46,12 +60,20 @@ void Capture::configure(StreamRole role)
> > >
> > > void Capture::start()
> > > {
> > > - Stream *stream = config_->at(0).stream();
> > > - int count = allocator_.allocate(stream);
> > > + assert(config_);
> > > + assert(!config_->empty());
> > > + assert(!allocator_.allocated());
> > > +
> > > + for (const auto &cfg : *config_) {
> > > + Stream *stream = cfg.stream();
> > > + int count = allocator_.allocate(stream);
> > > +
> > > + ASSERT_GE(count, 0) << "Failed to allocate buffers";
> > > + EXPECT_EQ(count, cfg.bufferCount) << "Allocated less buffers than expected";
> > > + ASSERT_EQ(count, allocator_.buffers(stream).size()) << "Unexpected number of buffers in allocator";
> > > + }
> > >
> > > - ASSERT_GE(count, 0) << "Failed to allocate buffers";
> > > - EXPECT_EQ(count, config_->at(0).bufferCount) << "Allocated less buffers than expected";
> > > - ASSERT_EQ(count, allocator_.buffers(stream).size()) << "Unexpected number of buffers in allocator";
> > > + ASSERT_TRUE(allocator_.allocated());
> > >
> > > camera_->requestCompleted.connect(this, &Capture::requestComplete);
> > >
> > > @@ -71,9 +93,14 @@ void Capture::stop()
> > >
> > > camera_->requestCompleted.disconnect(this);
> > >
> > > - Stream *stream = config_->at(0).stream();
> > > requests_.clear();
> > > - allocator_.free(stream);
> > > +
> > > + for (const auto &cfg : *config_) {
> > > + int res = allocator_.free(cfg.stream());
> >
> > s/res/ret/ for consistency.
> >
> > > + ASSERT_EQ(res, 0) << "Failed to free buffers associated with stream";
> > > + }
> > > +
> > > + ASSERT_FALSE(allocator_.allocated());
> > > }
> > >
> > > void Capture::prepareRequests(unsigned int plannedRequests)
> > > @@ -81,22 +108,36 @@ void Capture::prepareRequests(unsigned int plannedRequests)
> > > assert(config_);
> > > assert(requests_.empty());
> > >
> > > - Stream *stream = config_->at(0).stream();
> > > - const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_.buffers(stream);
> > > + std::size_t maxBuffers = 0;
> > >
> > > - /* No point in testing less requests then the camera depth. */
> > > - if (plannedRequests < buffers.size()) {
> > > - std::cout << "Camera needs " << buffers.size()
> > > - << " requests, can't test only "
> > > - << plannedRequests << std::endl;
> > > - GTEST_SKIP();
> > > + for (const auto &cfg : *config_) {
> > > + const auto &buffers = allocator_.buffers(cfg.stream());
> > > + ASSERT_FALSE(buffers.empty()) << "Zero buffers allocated for stream";
> > > +
> > > + /* No point in testing less requests then the camera depth. */
> > > + if (plannedRequests < buffers.size()) {
> > > + std::cout << "Camera needs " << buffers.size()
> > > + << " requests, can't test only "
> > > + << plannedRequests << std::endl;
> > > + GTEST_SKIP();
> > > + }
> >
> > I think this could be moved after the loop, testing
> >
> > if (plannedRequests < maxBuffers)
> >
> > > +
> > > + maxBuffers = std::max(maxBuffers, buffers.size());
> > > }
> > >
> > > - for (const std::unique_ptr<FrameBuffer> &buffer : buffers) {
> > > - std::unique_ptr<Request> request = camera_->createRequest();
> > > - ASSERT_TRUE(request) << "Can't create request";
> > > + for (std::size_t i = 0; i < maxBuffers; i++) {
> > > + std::unique_ptr<Request> request = camera_->createRequest(i);
> > > +
> > > + for (const auto &cfg : *config_) {
> > > + Stream *stream = cfg.stream();
> > > + const auto &buffers = allocator_.buffers(stream);
> > > + assert(!buffers.empty());
> > >
> > > - ASSERT_EQ(request->addBuffer(stream, buffer.get()), 0) << "Can't set buffer for request";
> > > + if (i < buffers.size()) {
> >
> > if (i >= buffers.size())
> > break;
> >
> > ASSERT_EQ(request->addBuffer(stream, buffers[i].get()), 0)
> > << "Can't add buffer to request";
>
> I think that changes the behaviour, but I believe `continue` would
> work.
Oops, indeed, I meant continue.
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >
> > > + ASSERT_EQ(request->addBuffer(stream, buffers[i].get()), 0)
> > > + << "Can't add buffer to request";
> > > + }
> > > + }
> > >
> > > requests_.push_back(std::move(request));
> > > }
> > > diff --git a/src/apps/lc-compliance/helpers/capture.h b/src/apps/lc-compliance/helpers/capture.h
> > > index 67c29021b..b3a390941 100644
> > > --- a/src/apps/lc-compliance/helpers/capture.h
> > > +++ b/src/apps/lc-compliance/helpers/capture.h
> > > @@ -17,7 +17,7 @@
> > > class Capture
> > > {
> > > public:
> > > - void configure(libcamera::StreamRole role);
> > > + void configure(libcamera::Span<const libcamera::StreamRole> roles);
> > >
> > > protected:
> > > Capture(std::shared_ptr<libcamera::Camera> camera);
> > > diff --git a/src/apps/lc-compliance/tests/capture_test.cpp b/src/apps/lc-compliance/tests/capture_test.cpp
> > > index 97465a612..c382fcf47 100644
> > > --- a/src/apps/lc-compliance/tests/capture_test.cpp
> > > +++ b/src/apps/lc-compliance/tests/capture_test.cpp
> > > @@ -89,7 +89,7 @@ TEST_P(SingleStream, Capture)
> > >
> > > CaptureBalanced capture(camera_);
> > >
> > > - capture.configure(role);
> > > + capture.configure(std::array{ role });
> > >
> > > capture.capture(numRequests);
> > > }
> > > @@ -108,7 +108,7 @@ TEST_P(SingleStream, CaptureStartStop)
> > >
> > > CaptureBalanced capture(camera_);
> > >
> > > - capture.configure(role);
> > > + capture.configure(std::array{ role });
> > >
> > > for (unsigned int starts = 0; starts < numRepeats; starts++)
> > > capture.capture(numRequests);
> > > @@ -127,7 +127,7 @@ TEST_P(SingleStream, UnbalancedStop)
> > >
> > > CaptureUnbalanced capture(camera_);
> > >
> > > - capture.configure(role);
> > > + capture.configure(std::array{ role });
> > >
> > > capture.capture(numRequests);
> > > }
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list