| Message ID | 20250314174220.1015597-2-barnabas.pocze@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Barnabás On Fri, Mar 14, 2025 at 06:42:18PM +0100, 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. The buffer count s/capture()/start()/ ? > of each stream is asserted to be the same. > > Multi-stream support will be added in next patches. > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > --- > src/apps/lc-compliance/helpers/capture.cpp | 64 ++++++++++++++----- > src/apps/lc-compliance/helpers/capture.h | 2 +- > src/apps/lc-compliance/tests/capture_test.cpp | 6 +- > 3 files changed, 51 insertions(+), 21 deletions(-) > > diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp > index f2c6d58ce..2a3fa3b68 100644 > --- a/src/apps/lc-compliance/helpers/capture.cpp > +++ b/src/apps/lc-compliance/helpers/capture.cpp > @@ -23,12 +23,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_) > - GTEST_SKIP() << "Role not supported by camera"; > + GTEST_SKIP() << "Roles 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 ? > + */ > + auto largest = > + std::max_element(config_->begin(), config_->end(), > + [](const StreamConfiguration &l, const StreamConfiguration &r) > + { return l.bufferCount < r.bufferCount; }); > + > + assert(largest != config_->end()); The only way this could happen if config is empty, something you check for a few lines above > + > + for (auto &cfg : *config_) > + cfg.bufferCount = largest->bufferCount; > > if (config_->validate() != CameraConfiguration::Valid) { > config_.reset(); > @@ -103,29 +120,37 @@ void Capture::start() > assert(!allocator_.allocated()); > assert(requests_.empty()); > > - Stream *stream = config_->at(0).stream(); > - int count = allocator_.allocate(stream); > - > - ASSERT_GE(count, 0) << "Failed to allocate buffers"; > - EXPECT_EQ(count, config_->at(0).bufferCount) << "Allocated less buffers than expected"; > - > - const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_.buffers(stream); > + const auto bufferCount = config_->at(0).bufferCount; This hunk changes the logic to use bufferCount in place of buffers.size() However, the code made sure that EXPECT_EQ(count, config_->at(0).bufferCount) so indeed the two are equivalent > > /* No point in testing less requests then the camera depth. */ > - if (queueLimit_ && *queueLimit_ < buffers.size()) { > - GTEST_SKIP() << "Camera needs " << buffers.size() > + if (queueLimit_ && *queueLimit_ < bufferCount) { > + GTEST_SKIP() << "Camera needs " << bufferCount > << " requests, can't test only " << *queueLimit_; > } > > - for (const std::unique_ptr<FrameBuffer> &buffer : buffers) { > + for (std::size_t i = 0; i < bufferCount; i++) { > std::unique_ptr<Request> request = camera_->createRequest(); > ASSERT_TRUE(request) << "Can't create request"; > + requests_.push_back(std::move(request)); > + } > > - ASSERT_EQ(request->addBuffer(stream, buffer.get()), 0) << "Can't set buffer for request"; > + for (const auto &cfg : *config_) { > + Stream *stream = cfg.stream(); > > - requests_.push_back(std::move(request)); > + int count = allocator_.allocate(stream); > + ASSERT_GE(count, 0) << "Failed to allocate buffers"; > + > + const auto &buffers = allocator_.buffers(stream); > + ASSERT_EQ(buffers.size(), bufferCount) << "Mismatching buffer count"; > + > + for (std::size_t i = 0; i < bufferCount; i++) { > + ASSERT_EQ(requests_[i]->addBuffer(stream, buffers[i].get()), 0) > + << "Failed to add buffer to request"; > + } > } > + ASSERT_TRUE(allocator_.allocated()); This has been validated by the above assretions for count > 0 > + > camera_->requestCompleted.connect(this, &Capture::requestComplete); > > ASSERT_EQ(camera_->start(), 0) << "Failed to start camera"; > @@ -140,7 +165,12 @@ void Capture::stop() > > camera_->requestCompleted.disconnect(this); > > - Stream *stream = config_->at(0).stream(); > requests_.clear(); > - allocator_.free(stream); > + > + 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 0e7b848fb..ea01c11c1 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..06f15bdb4 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({ { role } }); > > capture.run(numRequests, numRequests); > } > @@ -108,7 +108,7 @@ TEST_P(SingleStream, CaptureStartStop) > > Capture capture(camera_); > > - capture.configure(role); > + capture.configure({ { 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({ { role } }); > > capture.run(numRequests); Patch looks good! Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > } > -- > 2.48.1 >
diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp index f2c6d58ce..2a3fa3b68 100644 --- a/src/apps/lc-compliance/helpers/capture.cpp +++ b/src/apps/lc-compliance/helpers/capture.cpp @@ -23,12 +23,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_) - GTEST_SKIP() << "Role not supported by camera"; + GTEST_SKIP() << "Roles 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 ? + */ + auto largest = + std::max_element(config_->begin(), config_->end(), + [](const StreamConfiguration &l, const StreamConfiguration &r) + { return l.bufferCount < r.bufferCount; }); + + assert(largest != config_->end()); + + for (auto &cfg : *config_) + cfg.bufferCount = largest->bufferCount; if (config_->validate() != CameraConfiguration::Valid) { config_.reset(); @@ -103,29 +120,37 @@ void Capture::start() assert(!allocator_.allocated()); assert(requests_.empty()); - Stream *stream = config_->at(0).stream(); - int count = allocator_.allocate(stream); - - ASSERT_GE(count, 0) << "Failed to allocate buffers"; - EXPECT_EQ(count, config_->at(0).bufferCount) << "Allocated less buffers than expected"; - - const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_.buffers(stream); + const auto bufferCount = config_->at(0).bufferCount; /* No point in testing less requests then the camera depth. */ - if (queueLimit_ && *queueLimit_ < buffers.size()) { - GTEST_SKIP() << "Camera needs " << buffers.size() + if (queueLimit_ && *queueLimit_ < bufferCount) { + GTEST_SKIP() << "Camera needs " << bufferCount << " requests, can't test only " << *queueLimit_; } - for (const std::unique_ptr<FrameBuffer> &buffer : buffers) { + for (std::size_t i = 0; i < bufferCount; i++) { std::unique_ptr<Request> request = camera_->createRequest(); ASSERT_TRUE(request) << "Can't create request"; + requests_.push_back(std::move(request)); + } - ASSERT_EQ(request->addBuffer(stream, buffer.get()), 0) << "Can't set buffer for request"; + for (const auto &cfg : *config_) { + Stream *stream = cfg.stream(); - requests_.push_back(std::move(request)); + int count = allocator_.allocate(stream); + ASSERT_GE(count, 0) << "Failed to allocate buffers"; + + const auto &buffers = allocator_.buffers(stream); + ASSERT_EQ(buffers.size(), bufferCount) << "Mismatching buffer count"; + + for (std::size_t i = 0; i < bufferCount; i++) { + ASSERT_EQ(requests_[i]->addBuffer(stream, buffers[i].get()), 0) + << "Failed to add buffer to request"; + } } + ASSERT_TRUE(allocator_.allocated()); + camera_->requestCompleted.connect(this, &Capture::requestComplete); ASSERT_EQ(camera_->start(), 0) << "Failed to start camera"; @@ -140,7 +165,12 @@ void Capture::stop() camera_->requestCompleted.disconnect(this); - Stream *stream = config_->at(0).stream(); requests_.clear(); - allocator_.free(stream); + + 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 0e7b848fb..ea01c11c1 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..06f15bdb4 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({ { role } }); capture.run(numRequests, numRequests); } @@ -108,7 +108,7 @@ TEST_P(SingleStream, CaptureStartStop) Capture capture(camera_); - capture.configure(role); + capture.configure({ { 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({ { role } }); capture.run(numRequests); }
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. The buffer count of each stream is asserted to be the same. Multi-stream support will be added in next patches. Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> --- src/apps/lc-compliance/helpers/capture.cpp | 64 ++++++++++++++----- src/apps/lc-compliance/helpers/capture.h | 2 +- src/apps/lc-compliance/tests/capture_test.cpp | 6 +- 3 files changed, 51 insertions(+), 21 deletions(-)