[RFC PATCH v1 11/12] apps: lc-compliance: Support multiple streams in helpers
Barnabás Pőcze
pobrn at protonmail.com
Fri Jan 10 10:27:24 CET 2025
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.
Regards,
Barnabás Pőcze
>
> 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