[RFC PATCH v3 17/21] apps: lc-compliance: Support multiple streams in helpers
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Thu Feb 6 20:24:07 CET 2025
Hi Barnabás
On Thu, Feb 06, 2025 at 06:23:57PM +0000, Barnabás Pőcze wrote:
> Hi
>
>
> 2025. február 6., csütörtök 18:11 keltezéssel, Jacopo Mondi <jacopo.mondi at ideasonboard.com> írta:
>
> > Hi Barnabás
> >
> > On Thu, Jan 30, 2025 at 11:51:28AM +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>
> > > Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> > > ---
> > > src/apps/lc-compliance/helpers/capture.cpp | 77 +++++++++++++++----
> > > src/apps/lc-compliance/helpers/capture.h | 2 +-
> > > src/apps/lc-compliance/tests/capture_test.cpp | 6 +-
> > > 3 files changed, 66 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp
> > > index 940646f6c..4a8627662 100644
> > > --- a/src/apps/lc-compliance/helpers/capture.cpp
> > > +++ b/src/apps/lc-compliance/helpers/capture.cpp
> > > @@ -24,13 +24,31 @@ 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_)
> > > GTEST_SKIP() << "Role not supported by camera";
> > >
> > > + ASSERT_EQ(config_->size(), roles.size()) << "Unexpected number of streams in configuration";
> > > +
> > > + /*
> > > + * Set the buffers count to the largest value across all streams.
> > > + * \todo: Should all streams from a Camera have the same buffer count ?
> >
> > The way we currently handle bufferCount is sub-optimal, so for the
> > time being I would leave the \todo in place
>
> Sorry, I don't quite understand what you mean by "leave the \todo in place".
>
Sorry, I was just reinforcing that bufferCount is poorly handled, and
I agree with the todo being there
>
> >
> > > + */
> > > + auto largest =
> > > + std::max_element(config_->begin(), config_->end(),
> > > + [](const StreamConfiguration &l, const StreamConfiguration &r)
> > > + { return l.bufferCount < r.bufferCount; });
> > > +
> > > + assert(largest != config_->end());
> >
> > Can this happen ?
>
> I don't think so.
>
>
> >
> > > +
> > > + for (auto &cfg : *config_)
> > > + cfg.bufferCount = largest->bufferCount;
> > > +
> >
> > I presume having all streams with the same buffer count makes it way
> > easier to handle request queuing etc. However there might be system
> > where this might not be possible ? I guess we'll revisit if they
> > appear
> >
> > > if (config_->validate() != CameraConfiguration::Valid) {
> > > config_.reset();
> > > FAIL() << "Configuration not valid";
> > > @@ -74,20 +92,36 @@ void Capture::prepareRequests()
> > > 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;
> > > +
> > > + for (const auto &cfg : *config_) {
> > > + const auto &buffers = allocator_.buffers(cfg.stream());
> > > + ASSERT_FALSE(buffers.empty()) << "Zero buffers allocated for stream";
> > > +
> > > + maxBuffers = std::max(maxBuffers, buffers.size());
> >
> > Give the above, I guess all streams have the same buffer count ?
> > If that's the case, can we record it in a comment ? Otherwise when
> > we'll re-look at this in a few months we'll wonder why we have to
> > compute maxBuffers if all streams have the same buffer count.
> >
> > (actually the choice of how many buffers to allocate is left to
> > PipelineHandler::exportBuffers(). All (?) our implementations use
> > bufferCount at the moment. If we want this to be enforced we should
> > check that all streams have allocated the same buffers, as we forced
> > bufferCount to have the same value for all streams ?) This can also be
> > recorded in a todo comment if you agree ?
>
> I have opened https://bugs.libcamera.org/show_bug.cgi?id=251 in relation with this topic.
Thanks
> The semantics of that field are not entirely clear to me. So I tried to handle
> mismatching buffer counts gracefully. Nonetheless, I would not consider the
> implementation here good by any means. Maybe it would indeed be better
> to require the same buffer count for now.
Ok, then let's compute the 'largest' bufferCount as above and set all
cfg.bufferCount to that as you're doing already.
Then just make sure in prepareRequests() that all streams have the
same number of buffers and that should be it ?
>
>
> >
> > > + }
> > >
> > > /* No point in testing less requests then the camera depth. */
> > > - if (queueLimit_ && *queueLimit_ < buffers.size()) {
> > > - GTEST_SKIP() << "Camera needs " << buffers.size()
> > > + if (queueLimit_ && *queueLimit_ < maxBuffers) {
> > > + GTEST_SKIP() << "Camera needs " << maxBuffers
> > > << " requests, can't test only " << *queueLimit_;
> > > }
> >
> > No need for {}, right ?
>
> I usually use `{}` for when there are multiple lines and checkstyle.py did not
> complain, so should I change it?
>
it's a single statement, so theoretically, yes. Practically, whatever :)
>
> >
> > >
> > > - for (const std::unique_ptr<FrameBuffer> &buffer : buffers) {
> > > - std::unique_ptr<Request> request = camera_->createRequest();
> > > + for (std::size_t i = 0; i < maxBuffers; i++) {
> > > + std::unique_ptr<Request> request = camera_->createRequest(i);
> > > ASSERT_TRUE(request) << "Can't create request";
> > >
> > > - ASSERT_EQ(request->addBuffer(stream, buffer.get()), 0) << "Can't set buffer for request";
> > > + for (const auto &cfg : *config_) {
> > > + Stream *stream = cfg.stream();
> > > + const auto &buffers = allocator_.buffers(stream);
> > > + assert(!buffers.empty());
> > > +
> > > + if (i >= buffers.size())
> > > + continue;
> >
> > As per the above, we could assert i < buffer.size() ?
If we validate that all streams have the same number of buffers you
can remove this
> >
> > > +
> > > + ASSERT_EQ(request->addBuffer(stream, buffers[i].get()), 0)
> > > + << "Can't add buffer to request";
> > > + }
> > >
> > > requests_.push_back(std::move(request));
> > > }
> > > @@ -124,11 +158,19 @@ void Capture::requestComplete(Request *request)
> > >
> > > 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";
> >
> > This last check includes the above GE(0)
>
> `EXPECT_*()` checks fail the test but they do not abort the execution.
ASSERT_EQ() then ?
>
>
> >
> > > + }
> > >
> > > - ASSERT_GE(count, 0) << "Failed to allocate buffers";
> > > - EXPECT_EQ(count, config_->at(0).bufferCount) << "Allocated less buffers than expected";
> > > + ASSERT_TRUE(allocator_.allocated());
> > >
> > > camera_->requestCompleted.connect(this, &Capture::requestComplete);
> > >
> > > @@ -144,7 +186,12 @@ void Capture::stop()
> > >
> > > camera_->requestCompleted.disconnect(this);
> > >
> > > - Stream *stream = config_->at(0).stream();
> > > requests_.clear();
> > > - allocator_.free(stream);
> > > +
> > > + for (const auto &cfg : *config_) {
> > > + int ret = allocator_.free(cfg.stream());
> > > + EXPECT_EQ(ret, 0) << "Failed to free buffers associated with stream";
> >
> > If ret doesn't have to be returned
>
> ACK
>
>
> >
> > for (const auto &cfg : *config_)
> > EXPECT_EQ(allocator_.free(cfg.stream(), 0)
> > << "Failed to free buffers associated with stream";
> >
> > > + }
> > > +
> > > + EXPECT_FALSE(allocator_.allocated());
> > > }
> > > diff --git a/src/apps/lc-compliance/helpers/capture.h b/src/apps/lc-compliance/helpers/capture.h
> > > index ede395e2a..48a8dadcb 100644
> > > --- a/src/apps/lc-compliance/helpers/capture.h
> > > +++ b/src/apps/lc-compliance/helpers/capture.h
> > > @@ -20,7 +20,7 @@ public:
> > > Capture(std::shared_ptr<libcamera::Camera> camera);
> > > ~Capture();
> > >
> > > - void configure(libcamera::StreamRole role);
> > > + void configure(libcamera::Span<const libcamera::StreamRole> roles);
> > > void run(unsigned int captureLimit, std::optional<unsigned int> queueLimit = {});
> > >
> > > private:
> > > diff --git a/src/apps/lc-compliance/tests/capture_test.cpp b/src/apps/lc-compliance/tests/capture_test.cpp
> > > index 93bed48f0..147e17019 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)
> > >
> > > Capture capture(camera_);
> > >
> > > - capture.configure(role);
> > > + capture.configure(std::array{ role });
> >
> > Is there any advantage in passing in a Span<StreamRole> compared to
> > passing a const reference to the container (it's an std::array<> in
> > this patch, an std::vector<> since the next one).
>
> Well, I did not see a good enough reason not to use one. :)
>
True that :)
Thanks
j
>
> >
> > >
> > > capture.run(numRequests, numRequests);
> > > }
> > > @@ -108,7 +108,7 @@ TEST_P(SingleStream, CaptureStartStop)
> > >
> > > Capture capture(camera_);
> > >
> > > - capture.configure(role);
> > > + capture.configure(std::array{ role });
> > >
> > > for (unsigned int starts = 0; starts < numRepeats; starts++)
> > > capture.run(numRequests, numRequests);
> > > @@ -127,7 +127,7 @@ TEST_P(SingleStream, UnbalancedStop)
> > >
> > > Capture capture(camera_);
> > >
> > > - capture.configure(role);
> > > + capture.configure(std::array{ role });
> > >
> > > capture.run(numRequests);
> > > }
> > > --
> > > 2.48.1
> > >
> > >
More information about the libcamera-devel
mailing list