| Message ID | 20231230162912.827669-7-jacopo.mondi@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, thanks for the patch. I did not have a setup with support for multistream. I compiled it and ran the tests which then got skipped (as extpected) On Sat, Dec 30, 2023 at 05:29:11PM +0100, Jacopo Mondi 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. > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > --- > src/apps/lc-compliance/helpers/capture.cpp | 85 ++++++++++++++----- > src/apps/lc-compliance/helpers/capture.h | 2 +- > src/apps/lc-compliance/tests/capture_test.cpp | 26 +++--- > 3 files changed, 76 insertions(+), 37 deletions(-) > > diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp > index 5aab973f0392..bb95af3d758c 100644 > --- a/src/apps/lc-compliance/helpers/capture.cpp > +++ b/src/apps/lc-compliance/helpers/capture.cpp > @@ -7,6 +7,8 @@ > > #include "capture.h" > > +#include <algorithm> > + > #include <gtest/gtest.h> > > using namespace libcamera; > @@ -22,15 +24,27 @@ Capture::~Capture() > stop(); > } > > -void Capture::configure(StreamRole role) > +void Capture::configure(Span<const StreamRole> roles) > { > - config_ = camera_->generateConfiguration({ role }); > + config_ = camera_->generateConfiguration(roles); > > if (!config_) { > - std::cout << "Role not supported by camera" << std::endl; > + std::cout << "Roles 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 ? > + * */ Nit: Not sure about the rules here, but every comment I saw just ended with */ on the last line. > + auto largest = > + std::max_element(config_->begin(), config_->end(), > + [](StreamConfiguration &l, StreamConfiguration &r) > + { return l.bufferCount < r.bufferCount; }); > + > + for (auto &stream : *config_) > + stream.bufferCount = largest->bufferCount; > + > if (config_->validate() != CameraConfiguration::Valid) { > config_.reset(); > FAIL() << "Configuration not valid"; > @@ -44,11 +58,17 @@ void Capture::configure(StreamRole role) > > void Capture::start() > { > - Stream *stream = config_->at(0).stream(); > - int count = allocator_->allocate(stream); > + unsigned int i = 0; > + for (auto const &config : *config_) { > + Stream *stream = config.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"; > + ASSERT_GE(count, 0) << "Failed to allocate buffers for stream " << i; > + EXPECT_EQ(count, config.bufferCount) > + << "Allocated less buffers than expected for stream " << i; > + > + ++i; > + } > > camera_->requestCompleted.connect(this, &Capture::requestComplete); > > @@ -64,9 +84,12 @@ void Capture::stop() > > camera_->requestCompleted.disconnect(this); > > - Stream *stream = config_->at(0).stream(); > requests_.clear(); > - allocator_->free(stream); > + > + for (auto const &config : *config_) { > + Stream *stream = config.stream(); > + allocator_->free(stream); > + } > } > > /* CaptureBalanced */ > @@ -80,14 +103,12 @@ void CaptureBalanced::capture(unsigned int numRequests) > { > start(); > > - Stream *stream = config_->at(0).stream(); > - const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_->buffers(stream); > - > /* No point in testing less requests then the camera depth. */ > - if (buffers.size() > numRequests) { > - std::cout << "Camera needs " + std::to_string(buffers.size()) > - + " requests, can't test only " > - + std::to_string(numRequests) << std::endl; > + const unsigned int bufferCount = config_->at(0).bufferCount; > + if (bufferCount > numRequests) { > + std::cout << "Camera needs " << bufferCount > + << " requests, can't test only " << numRequests > + << std::endl; I don't know the details here. Can different streams have different bufferCount? I guess that is unlikely. Maybe add a comment? > GTEST_SKIP(); > } > > @@ -96,11 +117,21 @@ void CaptureBalanced::capture(unsigned int numRequests) > captureLimit_ = numRequests; > > /* Queue the recommended number of requests. */ > - for (const std::unique_ptr<FrameBuffer> &buffer : buffers) { > + for (unsigned int bufferIdx = 0; bufferIdx < bufferCount; ++bufferIdx) { > std::unique_ptr<Request> request = camera_->createRequest(); > ASSERT_TRUE(request) << "Can't create request"; > > - ASSERT_EQ(request->addBuffer(stream, buffer.get()), 0) << "Can't set buffer for request"; > + /* Add buffers for each stream. */ > + unsigned int i = 0; > + for (const auto &config : *config_) { > + Stream *stream = config.stream(); > + const auto &buffers = allocator_->buffers(stream); > + > + ASSERT_EQ(request->addBuffer(stream, buffers[bufferIdx].get()), 0) > + << "Can't add buffers for stream " << i; > + > + ++i; > + } > > ASSERT_EQ(queueRequest(request.get()), 0) << "Failed to queue request"; > > @@ -152,18 +183,26 @@ void CaptureUnbalanced::capture(unsigned int numRequests) > { > start(); > > - Stream *stream = config_->at(0).stream(); > - const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_->buffers(stream); > - > captureCount_ = 0; > captureLimit_ = numRequests; > > /* Queue the recommended number of requests. */ > - for (const std::unique_ptr<FrameBuffer> &buffer : buffers) { > + const unsigned int bufferCount = config_->at(0).bufferCount; > + for (unsigned int bufferIdx = 0; bufferIdx < bufferCount; ++bufferIdx) { > std::unique_ptr<Request> request = camera_->createRequest(); > ASSERT_TRUE(request) << "Can't create request"; > > - ASSERT_EQ(request->addBuffer(stream, buffer.get()), 0) << "Can't set buffer for request"; > + /* Add buffers for each stream. */ > + unsigned int i = 0; > + for (const auto &config : *config_) { > + Stream *stream = config.stream(); > + const auto &buffers = allocator_->buffers(stream); > + > + ASSERT_EQ(request->addBuffer(stream, buffers[bufferIdx].get()), 0) > + << "Can't add buffers for stream " << i; > + > + ++i; > + } Same question as above. If the bufferCount could differ between the streams, this might be problematic. Cheers, Stefan > > ASSERT_EQ(camera_->queueRequest(request.get()), 0) << "Failed to queue request"; > > diff --git a/src/apps/lc-compliance/helpers/capture.h b/src/apps/lc-compliance/helpers/capture.h > index 0574ab1c7ac7..3e2b2889244d 100644 > --- a/src/apps/lc-compliance/helpers/capture.h > +++ b/src/apps/lc-compliance/helpers/capture.h > @@ -16,7 +16,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 284d36307619..3d3cc97791d9 100644 > --- a/src/apps/lc-compliance/tests/capture_test.cpp > +++ b/src/apps/lc-compliance/tests/capture_test.cpp > @@ -17,14 +17,14 @@ > using namespace libcamera; > > const std::vector<int> NUMREQUESTS = { 1, 2, 3, 5, 8, 13, 21, 34, 55, 89 }; > -const std::vector<StreamRole> ROLES = { > - StreamRole::Raw, > - StreamRole::StillCapture, > - StreamRole::VideoRecording, > - StreamRole::Viewfinder > +const std::vector<std::vector<StreamRole>> ROLES = { > + { StreamRole::Raw }, > + { StreamRole::StillCapture }, > + { StreamRole::VideoRecording }, > + { StreamRole::Viewfinder }, > }; > > -class SingleStream : public testing::TestWithParam<std::tuple<StreamRole, int>> > +class SingleStream : public testing::TestWithParam<std::tuple<std::vector<StreamRole>, int>> > { > public: > static std::string nameParameters(const testing::TestParamInfo<SingleStream::ParamType> &info); > @@ -67,7 +67,7 @@ std::string SingleStream::nameParameters(const testing::TestParamInfo<SingleStre > { StreamRole::Viewfinder, "Viewfinder" } > }; > > - std::string roleName = rolesMap[std::get<0>(info.param)]; > + std::string roleName = rolesMap[std::get<0>(info.param)[0]]; > std::string numRequestsName = std::to_string(std::get<1>(info.param)); > > return roleName + "_" + numRequestsName; > @@ -82,11 +82,11 @@ std::string SingleStream::nameParameters(const testing::TestParamInfo<SingleStre > */ > TEST_P(SingleStream, Capture) > { > - auto [role, numRequests] = GetParam(); > + const auto [role, numRequests] = GetParam(); > > CaptureBalanced capture(camera_); > > - capture.configure(role); > + capture.configure(Span<const StreamRole>{ role }); > > capture.capture(numRequests); > } > @@ -100,12 +100,12 @@ TEST_P(SingleStream, Capture) > */ > TEST_P(SingleStream, CaptureStartStop) > { > - auto [role, numRequests] = GetParam(); > + const auto [role, numRequests] = GetParam(); > unsigned int numRepeats = 3; > > CaptureBalanced capture(camera_); > > - capture.configure(role); > + capture.configure(Span<const StreamRole>{ role }); > > for (unsigned int starts = 0; starts < numRepeats; starts++) > capture.capture(numRequests); > @@ -120,11 +120,11 @@ TEST_P(SingleStream, CaptureStartStop) > */ > TEST_P(SingleStream, UnbalancedStop) > { > - auto [role, numRequests] = GetParam(); > + const auto [role, numRequests] = GetParam(); > > CaptureUnbalanced capture(camera_); > > - capture.configure(role); > + capture.configure(Span<const StreamRole>{ role }); > > capture.capture(numRequests); > }
Hi Stefan On Fri, Mar 15, 2024 at 02:00:35PM +0100, Stefan Klug wrote: > Hi Jacopo, > > thanks for the patch. I did not have a setup with support for > multistream. I compiled it and ran the tests which then got skipped (as > extpected) > > On Sat, Dec 30, 2023 at 05:29:11PM +0100, Jacopo Mondi 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. > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > --- > > src/apps/lc-compliance/helpers/capture.cpp | 85 ++++++++++++++----- > > src/apps/lc-compliance/helpers/capture.h | 2 +- > > src/apps/lc-compliance/tests/capture_test.cpp | 26 +++--- > > 3 files changed, 76 insertions(+), 37 deletions(-) > > > > diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp > > index 5aab973f0392..bb95af3d758c 100644 > > --- a/src/apps/lc-compliance/helpers/capture.cpp > > +++ b/src/apps/lc-compliance/helpers/capture.cpp > > @@ -7,6 +7,8 @@ > > > > #include "capture.h" > > > > +#include <algorithm> > > + > > #include <gtest/gtest.h> > > > > using namespace libcamera; > > @@ -22,15 +24,27 @@ Capture::~Capture() > > stop(); > > } > > > > -void Capture::configure(StreamRole role) > > +void Capture::configure(Span<const StreamRole> roles) > > { > > - config_ = camera_->generateConfiguration({ role }); > > + config_ = camera_->generateConfiguration(roles); > > > > if (!config_) { > > - std::cout << "Role not supported by camera" << std::endl; > > + std::cout << "Roles 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 ? > > + * */ > > Nit: Not sure about the rules here, but every comment I saw just ended with */ on the last line. > This is clearly wrong, I'll fix > > + auto largest = > > + std::max_element(config_->begin(), config_->end(), > > + [](StreamConfiguration &l, StreamConfiguration &r) > > + { return l.bufferCount < r.bufferCount; }); > > + > > + for (auto &stream : *config_) > > + stream.bufferCount = largest->bufferCount; > > + > > if (config_->validate() != CameraConfiguration::Valid) { > > config_.reset(); > > FAIL() << "Configuration not valid"; > > @@ -44,11 +58,17 @@ void Capture::configure(StreamRole role) > > > > void Capture::start() > > { > > - Stream *stream = config_->at(0).stream(); > > - int count = allocator_->allocate(stream); > > + unsigned int i = 0; > > + for (auto const &config : *config_) { > > + Stream *stream = config.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"; > > + ASSERT_GE(count, 0) << "Failed to allocate buffers for stream " << i; > > + EXPECT_EQ(count, config.bufferCount) > > + << "Allocated less buffers than expected for stream " << i; > > + > > + ++i; > > + } > > > > camera_->requestCompleted.connect(this, &Capture::requestComplete); > > > > @@ -64,9 +84,12 @@ void Capture::stop() > > > > camera_->requestCompleted.disconnect(this); > > > > - Stream *stream = config_->at(0).stream(); > > requests_.clear(); > > - allocator_->free(stream); > > + > > + for (auto const &config : *config_) { > > + Stream *stream = config.stream(); > > + allocator_->free(stream); > > + } > > } > > > > /* CaptureBalanced */ > > @@ -80,14 +103,12 @@ void CaptureBalanced::capture(unsigned int numRequests) > > { > > start(); > > > > - Stream *stream = config_->at(0).stream(); > > - const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_->buffers(stream); > > - > > /* No point in testing less requests then the camera depth. */ > > - if (buffers.size() > numRequests) { > > - std::cout << "Camera needs " + std::to_string(buffers.size()) > > - + " requests, can't test only " > > - + std::to_string(numRequests) << std::endl; > > + const unsigned int bufferCount = config_->at(0).bufferCount; > > + if (bufferCount > numRequests) { > > + std::cout << "Camera needs " << bufferCount > > + << " requests, can't test only " << numRequests > > + << std::endl; > > I don't know the details here. Can different streams have different > bufferCount? I guess that is unlikely. Maybe add a comment? > Theoretically they could, as bufferCount is part of StreamConfiguration. In practice the bufferCount field is ill-defined and gets populated with the number of min buffers required by the video device node, which doesn't differ between capture devices (from which each stream is generated from) in any platform I know of. I can add a \todo to keep track of this > > GTEST_SKIP(); > > } > > > > @@ -96,11 +117,21 @@ void CaptureBalanced::capture(unsigned int numRequests) > > captureLimit_ = numRequests; > > > > /* Queue the recommended number of requests. */ > > - for (const std::unique_ptr<FrameBuffer> &buffer : buffers) { > > + for (unsigned int bufferIdx = 0; bufferIdx < bufferCount; ++bufferIdx) { > > std::unique_ptr<Request> request = camera_->createRequest(); > > ASSERT_TRUE(request) << "Can't create request"; > > > > - ASSERT_EQ(request->addBuffer(stream, buffer.get()), 0) << "Can't set buffer for request"; > > + /* Add buffers for each stream. */ > > + unsigned int i = 0; > > + for (const auto &config : *config_) { > > + Stream *stream = config.stream(); > > + const auto &buffers = allocator_->buffers(stream); > > + > > + ASSERT_EQ(request->addBuffer(stream, buffers[bufferIdx].get()), 0) > > + << "Can't add buffers for stream " << i; > > + > > + ++i; > > + } > > > > ASSERT_EQ(queueRequest(request.get()), 0) << "Failed to queue request"; > > > > @@ -152,18 +183,26 @@ void CaptureUnbalanced::capture(unsigned int numRequests) > > { > > start(); > > > > - Stream *stream = config_->at(0).stream(); > > - const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_->buffers(stream); > > - > > captureCount_ = 0; > > captureLimit_ = numRequests; > > > > /* Queue the recommended number of requests. */ > > - for (const std::unique_ptr<FrameBuffer> &buffer : buffers) { > > + const unsigned int bufferCount = config_->at(0).bufferCount; > > + for (unsigned int bufferIdx = 0; bufferIdx < bufferCount; ++bufferIdx) { > > std::unique_ptr<Request> request = camera_->createRequest(); > > ASSERT_TRUE(request) << "Can't create request"; > > > > - ASSERT_EQ(request->addBuffer(stream, buffer.get()), 0) << "Can't set buffer for request"; > > + /* Add buffers for each stream. */ > > + unsigned int i = 0; > > + for (const auto &config : *config_) { > > + Stream *stream = config.stream(); > > + const auto &buffers = allocator_->buffers(stream); > > + > > + ASSERT_EQ(request->addBuffer(stream, buffers[bufferIdx].get()), 0) > > + << "Can't add buffers for stream " << i; > > + > > + ++i; > > + } > > Same question as above. If the bufferCount could differ between the > streams, this might be problematic. > > Cheers, > Stefan > > > > > ASSERT_EQ(camera_->queueRequest(request.get()), 0) << "Failed to queue request"; > > > > diff --git a/src/apps/lc-compliance/helpers/capture.h b/src/apps/lc-compliance/helpers/capture.h > > index 0574ab1c7ac7..3e2b2889244d 100644 > > --- a/src/apps/lc-compliance/helpers/capture.h > > +++ b/src/apps/lc-compliance/helpers/capture.h > > @@ -16,7 +16,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 284d36307619..3d3cc97791d9 100644 > > --- a/src/apps/lc-compliance/tests/capture_test.cpp > > +++ b/src/apps/lc-compliance/tests/capture_test.cpp > > @@ -17,14 +17,14 @@ > > using namespace libcamera; > > > > const std::vector<int> NUMREQUESTS = { 1, 2, 3, 5, 8, 13, 21, 34, 55, 89 }; > > -const std::vector<StreamRole> ROLES = { > > - StreamRole::Raw, > > - StreamRole::StillCapture, > > - StreamRole::VideoRecording, > > - StreamRole::Viewfinder > > +const std::vector<std::vector<StreamRole>> ROLES = { > > + { StreamRole::Raw }, > > + { StreamRole::StillCapture }, > > + { StreamRole::VideoRecording }, > > + { StreamRole::Viewfinder }, > > }; > > > > -class SingleStream : public testing::TestWithParam<std::tuple<StreamRole, int>> > > +class SingleStream : public testing::TestWithParam<std::tuple<std::vector<StreamRole>, int>> > > { > > public: > > static std::string nameParameters(const testing::TestParamInfo<SingleStream::ParamType> &info); > > @@ -67,7 +67,7 @@ std::string SingleStream::nameParameters(const testing::TestParamInfo<SingleStre > > { StreamRole::Viewfinder, "Viewfinder" } > > }; > > > > - std::string roleName = rolesMap[std::get<0>(info.param)]; > > + std::string roleName = rolesMap[std::get<0>(info.param)[0]]; > > std::string numRequestsName = std::to_string(std::get<1>(info.param)); > > > > return roleName + "_" + numRequestsName; > > @@ -82,11 +82,11 @@ std::string SingleStream::nameParameters(const testing::TestParamInfo<SingleStre > > */ > > TEST_P(SingleStream, Capture) > > { > > - auto [role, numRequests] = GetParam(); > > + const auto [role, numRequests] = GetParam(); > > > > CaptureBalanced capture(camera_); > > > > - capture.configure(role); > > + capture.configure(Span<const StreamRole>{ role }); > > > > capture.capture(numRequests); > > } > > @@ -100,12 +100,12 @@ TEST_P(SingleStream, Capture) > > */ > > TEST_P(SingleStream, CaptureStartStop) > > { > > - auto [role, numRequests] = GetParam(); > > + const auto [role, numRequests] = GetParam(); > > unsigned int numRepeats = 3; > > > > CaptureBalanced capture(camera_); > > > > - capture.configure(role); > > + capture.configure(Span<const StreamRole>{ role }); > > > > for (unsigned int starts = 0; starts < numRepeats; starts++) > > capture.capture(numRequests); > > @@ -120,11 +120,11 @@ TEST_P(SingleStream, CaptureStartStop) > > */ > > TEST_P(SingleStream, UnbalancedStop) > > { > > - auto [role, numRequests] = GetParam(); > > + const auto [role, numRequests] = GetParam(); > > > > CaptureUnbalanced capture(camera_); > > > > - capture.configure(role); > > + capture.configure(Span<const StreamRole>{ role }); > > > > capture.capture(numRequests); > > }
Hello again On Tue, Mar 19, 2024 at 04:59:30PM +0100, Jacopo Mondi wrote: > Hi Stefan > > On Fri, Mar 15, 2024 at 02:00:35PM +0100, Stefan Klug wrote: > > Hi Jacopo, > > > > thanks for the patch. I did not have a setup with support for > > multistream. I compiled it and ran the tests which then got skipped (as > > extpected) > > > > On Sat, Dec 30, 2023 at 05:29:11PM +0100, Jacopo Mondi 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. > > > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > --- > > > src/apps/lc-compliance/helpers/capture.cpp | 85 ++++++++++++++----- > > > src/apps/lc-compliance/helpers/capture.h | 2 +- > > > src/apps/lc-compliance/tests/capture_test.cpp | 26 +++--- > > > 3 files changed, 76 insertions(+), 37 deletions(-) > > > > > > diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp > > > index 5aab973f0392..bb95af3d758c 100644 > > > --- a/src/apps/lc-compliance/helpers/capture.cpp > > > +++ b/src/apps/lc-compliance/helpers/capture.cpp > > > @@ -7,6 +7,8 @@ > > > > > > #include "capture.h" > > > > > > +#include <algorithm> > > > + > > > #include <gtest/gtest.h> > > > > > > using namespace libcamera; > > > @@ -22,15 +24,27 @@ Capture::~Capture() > > > stop(); > > > } > > > > > > -void Capture::configure(StreamRole role) > > > +void Capture::configure(Span<const StreamRole> roles) > > > { > > > - config_ = camera_->generateConfiguration({ role }); > > > + config_ = camera_->generateConfiguration(roles); > > > > > > if (!config_) { > > > - std::cout << "Role not supported by camera" << std::endl; > > > + std::cout << "Roles 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 ? > > > + * */ > > > > Nit: Not sure about the rules here, but every comment I saw just ended with */ on the last line. > > > > This is clearly wrong, I'll fix > > > > + auto largest = > > > + std::max_element(config_->begin(), config_->end(), > > > + [](StreamConfiguration &l, StreamConfiguration &r) > > > + { return l.bufferCount < r.bufferCount; }); > > > + > > > + for (auto &stream : *config_) > > > + stream.bufferCount = largest->bufferCount; > > > + > > > if (config_->validate() != CameraConfiguration::Valid) { > > > config_.reset(); > > > FAIL() << "Configuration not valid"; > > > @@ -44,11 +58,17 @@ void Capture::configure(StreamRole role) > > > > > > void Capture::start() > > > { > > > - Stream *stream = config_->at(0).stream(); > > > - int count = allocator_->allocate(stream); > > > + unsigned int i = 0; > > > + for (auto const &config : *config_) { > > > + Stream *stream = config.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"; > > > + ASSERT_GE(count, 0) << "Failed to allocate buffers for stream " << i; > > > + EXPECT_EQ(count, config.bufferCount) > > > + << "Allocated less buffers than expected for stream " << i; > > > + > > > + ++i; > > > + } > > > > > > camera_->requestCompleted.connect(this, &Capture::requestComplete); > > > > > > @@ -64,9 +84,12 @@ void Capture::stop() > > > > > > camera_->requestCompleted.disconnect(this); > > > > > > - Stream *stream = config_->at(0).stream(); > > > requests_.clear(); > > > - allocator_->free(stream); > > > + > > > + for (auto const &config : *config_) { > > > + Stream *stream = config.stream(); > > > + allocator_->free(stream); > > > + } > > > } > > > > > > /* CaptureBalanced */ > > > @@ -80,14 +103,12 @@ void CaptureBalanced::capture(unsigned int numRequests) > > > { > > > start(); > > > > > > - Stream *stream = config_->at(0).stream(); > > > - const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_->buffers(stream); > > > - > > > /* No point in testing less requests then the camera depth. */ > > > - if (buffers.size() > numRequests) { > > > - std::cout << "Camera needs " + std::to_string(buffers.size()) > > > - + " requests, can't test only " > > > - + std::to_string(numRequests) << std::endl; > > > + const unsigned int bufferCount = config_->at(0).bufferCount; > > > + if (bufferCount > numRequests) { > > > + std::cout << "Camera needs " << bufferCount > > > + << " requests, can't test only " << numRequests > > > + << std::endl; > > > > I don't know the details here. Can different streams have different > > bufferCount? I guess that is unlikely. Maybe add a comment? > > > > Theoretically they could, as bufferCount is part of > StreamConfiguration. In practice the bufferCount field is ill-defined > and gets populated with the number of min buffers required by the > video device node, which doesn't differ between capture devices (from > which each stream is generated from) in any platform I know of. > > I can add a \todo to keep track of this > Ah ups, I already had one \todo: Should all streams from a Camera have the same buffer count ? > > > GTEST_SKIP(); > > > } > > > > > > @@ -96,11 +117,21 @@ void CaptureBalanced::capture(unsigned int numRequests) > > > captureLimit_ = numRequests; > > > > > > /* Queue the recommended number of requests. */ > > > - for (const std::unique_ptr<FrameBuffer> &buffer : buffers) { > > > + for (unsigned int bufferIdx = 0; bufferIdx < bufferCount; ++bufferIdx) { > > > std::unique_ptr<Request> request = camera_->createRequest(); > > > ASSERT_TRUE(request) << "Can't create request"; > > > > > > - ASSERT_EQ(request->addBuffer(stream, buffer.get()), 0) << "Can't set buffer for request"; > > > + /* Add buffers for each stream. */ > > > + unsigned int i = 0; > > > + for (const auto &config : *config_) { > > > + Stream *stream = config.stream(); > > > + const auto &buffers = allocator_->buffers(stream); > > > + > > > + ASSERT_EQ(request->addBuffer(stream, buffers[bufferIdx].get()), 0) > > > + << "Can't add buffers for stream " << i; > > > + > > > + ++i; > > > + } > > > > > > ASSERT_EQ(queueRequest(request.get()), 0) << "Failed to queue request"; > > > > > > @@ -152,18 +183,26 @@ void CaptureUnbalanced::capture(unsigned int numRequests) > > > { > > > start(); > > > > > > - Stream *stream = config_->at(0).stream(); > > > - const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_->buffers(stream); > > > - > > > captureCount_ = 0; > > > captureLimit_ = numRequests; > > > > > > /* Queue the recommended number of requests. */ > > > - for (const std::unique_ptr<FrameBuffer> &buffer : buffers) { > > > + const unsigned int bufferCount = config_->at(0).bufferCount; > > > + for (unsigned int bufferIdx = 0; bufferIdx < bufferCount; ++bufferIdx) { > > > std::unique_ptr<Request> request = camera_->createRequest(); > > > ASSERT_TRUE(request) << "Can't create request"; > > > > > > - ASSERT_EQ(request->addBuffer(stream, buffer.get()), 0) << "Can't set buffer for request"; > > > + /* Add buffers for each stream. */ > > > + unsigned int i = 0; > > > + for (const auto &config : *config_) { > > > + Stream *stream = config.stream(); > > > + const auto &buffers = allocator_->buffers(stream); > > > + > > > + ASSERT_EQ(request->addBuffer(stream, buffers[bufferIdx].get()), 0) > > > + << "Can't add buffers for stream " << i; > > > + > > > + ++i; > > > + } > > > > Same question as above. If the bufferCount could differ between the > > streams, this might be problematic. > > > > Cheers, > > Stefan > > > > > > > > ASSERT_EQ(camera_->queueRequest(request.get()), 0) << "Failed to queue request"; > > > > > > diff --git a/src/apps/lc-compliance/helpers/capture.h b/src/apps/lc-compliance/helpers/capture.h > > > index 0574ab1c7ac7..3e2b2889244d 100644 > > > --- a/src/apps/lc-compliance/helpers/capture.h > > > +++ b/src/apps/lc-compliance/helpers/capture.h > > > @@ -16,7 +16,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 284d36307619..3d3cc97791d9 100644 > > > --- a/src/apps/lc-compliance/tests/capture_test.cpp > > > +++ b/src/apps/lc-compliance/tests/capture_test.cpp > > > @@ -17,14 +17,14 @@ > > > using namespace libcamera; > > > > > > const std::vector<int> NUMREQUESTS = { 1, 2, 3, 5, 8, 13, 21, 34, 55, 89 }; > > > -const std::vector<StreamRole> ROLES = { > > > - StreamRole::Raw, > > > - StreamRole::StillCapture, > > > - StreamRole::VideoRecording, > > > - StreamRole::Viewfinder > > > +const std::vector<std::vector<StreamRole>> ROLES = { > > > + { StreamRole::Raw }, > > > + { StreamRole::StillCapture }, > > > + { StreamRole::VideoRecording }, > > > + { StreamRole::Viewfinder }, > > > }; > > > > > > -class SingleStream : public testing::TestWithParam<std::tuple<StreamRole, int>> > > > +class SingleStream : public testing::TestWithParam<std::tuple<std::vector<StreamRole>, int>> > > > { > > > public: > > > static std::string nameParameters(const testing::TestParamInfo<SingleStream::ParamType> &info); > > > @@ -67,7 +67,7 @@ std::string SingleStream::nameParameters(const testing::TestParamInfo<SingleStre > > > { StreamRole::Viewfinder, "Viewfinder" } > > > }; > > > > > > - std::string roleName = rolesMap[std::get<0>(info.param)]; > > > + std::string roleName = rolesMap[std::get<0>(info.param)[0]]; > > > std::string numRequestsName = std::to_string(std::get<1>(info.param)); > > > > > > return roleName + "_" + numRequestsName; > > > @@ -82,11 +82,11 @@ std::string SingleStream::nameParameters(const testing::TestParamInfo<SingleStre > > > */ > > > TEST_P(SingleStream, Capture) > > > { > > > - auto [role, numRequests] = GetParam(); > > > + const auto [role, numRequests] = GetParam(); > > > > > > CaptureBalanced capture(camera_); > > > > > > - capture.configure(role); > > > + capture.configure(Span<const StreamRole>{ role }); > > > > > > capture.capture(numRequests); > > > } > > > @@ -100,12 +100,12 @@ TEST_P(SingleStream, Capture) > > > */ > > > TEST_P(SingleStream, CaptureStartStop) > > > { > > > - auto [role, numRequests] = GetParam(); > > > + const auto [role, numRequests] = GetParam(); > > > unsigned int numRepeats = 3; > > > > > > CaptureBalanced capture(camera_); > > > > > > - capture.configure(role); > > > + capture.configure(Span<const StreamRole>{ role }); > > > > > > for (unsigned int starts = 0; starts < numRepeats; starts++) > > > capture.capture(numRequests); > > > @@ -120,11 +120,11 @@ TEST_P(SingleStream, CaptureStartStop) > > > */ > > > TEST_P(SingleStream, UnbalancedStop) > > > { > > > - auto [role, numRequests] = GetParam(); > > > + const auto [role, numRequests] = GetParam(); > > > > > > CaptureUnbalanced capture(camera_); > > > > > > - capture.configure(role); > > > + capture.configure(Span<const StreamRole>{ role }); > > > > > > capture.capture(numRequests); > > > }
Hi Jacopo, On Tue, Mar 19, 2024 at 05:03:01PM +0100, Jacopo Mondi wrote: > Hello again > > On Tue, Mar 19, 2024 at 04:59:30PM +0100, Jacopo Mondi wrote: > > Hi Stefan > > > > On Fri, Mar 15, 2024 at 02:00:35PM +0100, Stefan Klug wrote: > > > Hi Jacopo, > > > > > > thanks for the patch. I did not have a setup with support for > > > multistream. I compiled it and ran the tests which then got skipped (as > > > extpected) > > > > > > On Sat, Dec 30, 2023 at 05:29:11PM +0100, Jacopo Mondi 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. > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > --- > > > > src/apps/lc-compliance/helpers/capture.cpp | 85 ++++++++++++++----- > > > > src/apps/lc-compliance/helpers/capture.h | 2 +- > > > > src/apps/lc-compliance/tests/capture_test.cpp | 26 +++--- > > > > 3 files changed, 76 insertions(+), 37 deletions(-) > > > > > > > > diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp > > > > index 5aab973f0392..bb95af3d758c 100644 > > > > --- a/src/apps/lc-compliance/helpers/capture.cpp > > > > +++ b/src/apps/lc-compliance/helpers/capture.cpp > > > > @@ -7,6 +7,8 @@ > > > > > > > > #include "capture.h" > > > > > > > > +#include <algorithm> > > > > + > > > > #include <gtest/gtest.h> > > > > > > > > using namespace libcamera; > > > > @@ -22,15 +24,27 @@ Capture::~Capture() > > > > stop(); > > > > } > > > > > > > > -void Capture::configure(StreamRole role) > > > > +void Capture::configure(Span<const StreamRole> roles) > > > > { > > > > - config_ = camera_->generateConfiguration({ role }); > > > > + config_ = camera_->generateConfiguration(roles); > > > > > > > > if (!config_) { > > > > - std::cout << "Role not supported by camera" << std::endl; > > > > + std::cout << "Roles 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 ? > > > > + * */ > > > > > > Nit: Not sure about the rules here, but every comment I saw just ended with */ on the last line. > > > > > > > This is clearly wrong, I'll fix > > > > > > + auto largest = > > > > + std::max_element(config_->begin(), config_->end(), > > > > + [](StreamConfiguration &l, StreamConfiguration &r) > > > > + { return l.bufferCount < r.bufferCount; }); > > > > + > > > > + for (auto &stream : *config_) > > > > + stream.bufferCount = largest->bufferCount; > > > > + > > > > if (config_->validate() != CameraConfiguration::Valid) { > > > > config_.reset(); > > > > FAIL() << "Configuration not valid"; > > > > @@ -44,11 +58,17 @@ void Capture::configure(StreamRole role) > > > > > > > > void Capture::start() > > > > { > > > > - Stream *stream = config_->at(0).stream(); > > > > - int count = allocator_->allocate(stream); > > > > + unsigned int i = 0; > > > > + for (auto const &config : *config_) { > > > > + Stream *stream = config.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"; > > > > + ASSERT_GE(count, 0) << "Failed to allocate buffers for stream " << i; > > > > + EXPECT_EQ(count, config.bufferCount) > > > > + << "Allocated less buffers than expected for stream " << i; > > > > + > > > > + ++i; > > > > + } > > > > > > > > camera_->requestCompleted.connect(this, &Capture::requestComplete); > > > > > > > > @@ -64,9 +84,12 @@ void Capture::stop() > > > > > > > > camera_->requestCompleted.disconnect(this); > > > > > > > > - Stream *stream = config_->at(0).stream(); > > > > requests_.clear(); > > > > - allocator_->free(stream); > > > > + > > > > + for (auto const &config : *config_) { > > > > + Stream *stream = config.stream(); > > > > + allocator_->free(stream); > > > > + } > > > > } > > > > > > > > /* CaptureBalanced */ > > > > @@ -80,14 +103,12 @@ void CaptureBalanced::capture(unsigned int numRequests) > > > > { > > > > start(); > > > > > > > > - Stream *stream = config_->at(0).stream(); > > > > - const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_->buffers(stream); > > > > - > > > > /* No point in testing less requests then the camera depth. */ > > > > - if (buffers.size() > numRequests) { > > > > - std::cout << "Camera needs " + std::to_string(buffers.size()) > > > > - + " requests, can't test only " > > > > - + std::to_string(numRequests) << std::endl; > > > > + const unsigned int bufferCount = config_->at(0).bufferCount; > > > > + if (bufferCount > numRequests) { > > > > + std::cout << "Camera needs " << bufferCount > > > > + << " requests, can't test only " << numRequests > > > > + << std::endl; > > > > > > I don't know the details here. Can different streams have different > > > bufferCount? I guess that is unlikely. Maybe add a comment? > > > > > > > Theoretically they could, as bufferCount is part of > > StreamConfiguration. In practice the bufferCount field is ill-defined > > and gets populated with the number of min buffers required by the > > video device node, which doesn't differ between capture devices (from > > which each stream is generated from) in any platform I know of. > > > > I can add a \todo to keep track of this > > > > Ah ups, I already had one > > \todo: Should all streams from a Camera have the same buffer count ? Oh I missed that. so Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> Cheers, Stefan > > > > > GTEST_SKIP(); > > > > } > > > > > > > > @@ -96,11 +117,21 @@ void CaptureBalanced::capture(unsigned int numRequests) > > > > captureLimit_ = numRequests; > > > > > > > > /* Queue the recommended number of requests. */ > > > > - for (const std::unique_ptr<FrameBuffer> &buffer : buffers) { > > > > + for (unsigned int bufferIdx = 0; bufferIdx < bufferCount; ++bufferIdx) { > > > > std::unique_ptr<Request> request = camera_->createRequest(); > > > > ASSERT_TRUE(request) << "Can't create request"; > > > > > > > > - ASSERT_EQ(request->addBuffer(stream, buffer.get()), 0) << "Can't set buffer for request"; > > > > + /* Add buffers for each stream. */ > > > > + unsigned int i = 0; > > > > + for (const auto &config : *config_) { > > > > + Stream *stream = config.stream(); > > > > + const auto &buffers = allocator_->buffers(stream); > > > > + > > > > + ASSERT_EQ(request->addBuffer(stream, buffers[bufferIdx].get()), 0) > > > > + << "Can't add buffers for stream " << i; > > > > + > > > > + ++i; > > > > + } > > > > > > > > ASSERT_EQ(queueRequest(request.get()), 0) << "Failed to queue request"; > > > > > > > > @@ -152,18 +183,26 @@ void CaptureUnbalanced::capture(unsigned int numRequests) > > > > { > > > > start(); > > > > > > > > - Stream *stream = config_->at(0).stream(); > > > > - const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_->buffers(stream); > > > > - > > > > captureCount_ = 0; > > > > captureLimit_ = numRequests; > > > > > > > > /* Queue the recommended number of requests. */ > > > > - for (const std::unique_ptr<FrameBuffer> &buffer : buffers) { > > > > + const unsigned int bufferCount = config_->at(0).bufferCount; > > > > + for (unsigned int bufferIdx = 0; bufferIdx < bufferCount; ++bufferIdx) { > > > > std::unique_ptr<Request> request = camera_->createRequest(); > > > > ASSERT_TRUE(request) << "Can't create request"; > > > > > > > > - ASSERT_EQ(request->addBuffer(stream, buffer.get()), 0) << "Can't set buffer for request"; > > > > + /* Add buffers for each stream. */ > > > > + unsigned int i = 0; > > > > + for (const auto &config : *config_) { > > > > + Stream *stream = config.stream(); > > > > + const auto &buffers = allocator_->buffers(stream); > > > > + > > > > + ASSERT_EQ(request->addBuffer(stream, buffers[bufferIdx].get()), 0) > > > > + << "Can't add buffers for stream " << i; > > > > + > > > > + ++i; > > > > + } > > > > > > Same question as above. If the bufferCount could differ between the > > > streams, this might be problematic. > > > > > > Cheers, > > > Stefan > > > > > > > > > > > ASSERT_EQ(camera_->queueRequest(request.get()), 0) << "Failed to queue request"; > > > > > > > > diff --git a/src/apps/lc-compliance/helpers/capture.h b/src/apps/lc-compliance/helpers/capture.h > > > > index 0574ab1c7ac7..3e2b2889244d 100644 > > > > --- a/src/apps/lc-compliance/helpers/capture.h > > > > +++ b/src/apps/lc-compliance/helpers/capture.h > > > > @@ -16,7 +16,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 284d36307619..3d3cc97791d9 100644 > > > > --- a/src/apps/lc-compliance/tests/capture_test.cpp > > > > +++ b/src/apps/lc-compliance/tests/capture_test.cpp > > > > @@ -17,14 +17,14 @@ > > > > using namespace libcamera; > > > > > > > > const std::vector<int> NUMREQUESTS = { 1, 2, 3, 5, 8, 13, 21, 34, 55, 89 }; > > > > -const std::vector<StreamRole> ROLES = { > > > > - StreamRole::Raw, > > > > - StreamRole::StillCapture, > > > > - StreamRole::VideoRecording, > > > > - StreamRole::Viewfinder > > > > +const std::vector<std::vector<StreamRole>> ROLES = { > > > > + { StreamRole::Raw }, > > > > + { StreamRole::StillCapture }, > > > > + { StreamRole::VideoRecording }, > > > > + { StreamRole::Viewfinder }, > > > > }; > > > > > > > > -class SingleStream : public testing::TestWithParam<std::tuple<StreamRole, int>> > > > > +class SingleStream : public testing::TestWithParam<std::tuple<std::vector<StreamRole>, int>> > > > > { > > > > public: > > > > static std::string nameParameters(const testing::TestParamInfo<SingleStream::ParamType> &info); > > > > @@ -67,7 +67,7 @@ std::string SingleStream::nameParameters(const testing::TestParamInfo<SingleStre > > > > { StreamRole::Viewfinder, "Viewfinder" } > > > > }; > > > > > > > > - std::string roleName = rolesMap[std::get<0>(info.param)]; > > > > + std::string roleName = rolesMap[std::get<0>(info.param)[0]]; > > > > std::string numRequestsName = std::to_string(std::get<1>(info.param)); > > > > > > > > return roleName + "_" + numRequestsName; > > > > @@ -82,11 +82,11 @@ std::string SingleStream::nameParameters(const testing::TestParamInfo<SingleStre > > > > */ > > > > TEST_P(SingleStream, Capture) > > > > { > > > > - auto [role, numRequests] = GetParam(); > > > > + const auto [role, numRequests] = GetParam(); > > > > > > > > CaptureBalanced capture(camera_); > > > > > > > > - capture.configure(role); > > > > + capture.configure(Span<const StreamRole>{ role }); > > > > > > > > capture.capture(numRequests); > > > > } > > > > @@ -100,12 +100,12 @@ TEST_P(SingleStream, Capture) > > > > */ > > > > TEST_P(SingleStream, CaptureStartStop) > > > > { > > > > - auto [role, numRequests] = GetParam(); > > > > + const auto [role, numRequests] = GetParam(); > > > > unsigned int numRepeats = 3; > > > > > > > > CaptureBalanced capture(camera_); > > > > > > > > - capture.configure(role); > > > > + capture.configure(Span<const StreamRole>{ role }); > > > > > > > > for (unsigned int starts = 0; starts < numRepeats; starts++) > > > > capture.capture(numRequests); > > > > @@ -120,11 +120,11 @@ TEST_P(SingleStream, CaptureStartStop) > > > > */ > > > > TEST_P(SingleStream, UnbalancedStop) > > > > { > > > > - auto [role, numRequests] = GetParam(); > > > > + const auto [role, numRequests] = GetParam(); > > > > > > > > CaptureUnbalanced capture(camera_); > > > > > > > > - capture.configure(role); > > > > + capture.configure(Span<const StreamRole>{ role }); > > > > > > > > capture.capture(numRequests); > > > > }
diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp index 5aab973f0392..bb95af3d758c 100644 --- a/src/apps/lc-compliance/helpers/capture.cpp +++ b/src/apps/lc-compliance/helpers/capture.cpp @@ -7,6 +7,8 @@ #include "capture.h" +#include <algorithm> + #include <gtest/gtest.h> using namespace libcamera; @@ -22,15 +24,27 @@ Capture::~Capture() stop(); } -void Capture::configure(StreamRole role) +void Capture::configure(Span<const StreamRole> roles) { - config_ = camera_->generateConfiguration({ role }); + config_ = camera_->generateConfiguration(roles); if (!config_) { - std::cout << "Role not supported by camera" << std::endl; + std::cout << "Roles 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(), + [](StreamConfiguration &l, StreamConfiguration &r) + { return l.bufferCount < r.bufferCount; }); + + for (auto &stream : *config_) + stream.bufferCount = largest->bufferCount; + if (config_->validate() != CameraConfiguration::Valid) { config_.reset(); FAIL() << "Configuration not valid"; @@ -44,11 +58,17 @@ void Capture::configure(StreamRole role) void Capture::start() { - Stream *stream = config_->at(0).stream(); - int count = allocator_->allocate(stream); + unsigned int i = 0; + for (auto const &config : *config_) { + Stream *stream = config.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"; + ASSERT_GE(count, 0) << "Failed to allocate buffers for stream " << i; + EXPECT_EQ(count, config.bufferCount) + << "Allocated less buffers than expected for stream " << i; + + ++i; + } camera_->requestCompleted.connect(this, &Capture::requestComplete); @@ -64,9 +84,12 @@ void Capture::stop() camera_->requestCompleted.disconnect(this); - Stream *stream = config_->at(0).stream(); requests_.clear(); - allocator_->free(stream); + + for (auto const &config : *config_) { + Stream *stream = config.stream(); + allocator_->free(stream); + } } /* CaptureBalanced */ @@ -80,14 +103,12 @@ void CaptureBalanced::capture(unsigned int numRequests) { start(); - Stream *stream = config_->at(0).stream(); - const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_->buffers(stream); - /* No point in testing less requests then the camera depth. */ - if (buffers.size() > numRequests) { - std::cout << "Camera needs " + std::to_string(buffers.size()) - + " requests, can't test only " - + std::to_string(numRequests) << std::endl; + const unsigned int bufferCount = config_->at(0).bufferCount; + if (bufferCount > numRequests) { + std::cout << "Camera needs " << bufferCount + << " requests, can't test only " << numRequests + << std::endl; GTEST_SKIP(); } @@ -96,11 +117,21 @@ void CaptureBalanced::capture(unsigned int numRequests) captureLimit_ = numRequests; /* Queue the recommended number of requests. */ - for (const std::unique_ptr<FrameBuffer> &buffer : buffers) { + for (unsigned int bufferIdx = 0; bufferIdx < bufferCount; ++bufferIdx) { std::unique_ptr<Request> request = camera_->createRequest(); ASSERT_TRUE(request) << "Can't create request"; - ASSERT_EQ(request->addBuffer(stream, buffer.get()), 0) << "Can't set buffer for request"; + /* Add buffers for each stream. */ + unsigned int i = 0; + for (const auto &config : *config_) { + Stream *stream = config.stream(); + const auto &buffers = allocator_->buffers(stream); + + ASSERT_EQ(request->addBuffer(stream, buffers[bufferIdx].get()), 0) + << "Can't add buffers for stream " << i; + + ++i; + } ASSERT_EQ(queueRequest(request.get()), 0) << "Failed to queue request"; @@ -152,18 +183,26 @@ void CaptureUnbalanced::capture(unsigned int numRequests) { start(); - Stream *stream = config_->at(0).stream(); - const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_->buffers(stream); - captureCount_ = 0; captureLimit_ = numRequests; /* Queue the recommended number of requests. */ - for (const std::unique_ptr<FrameBuffer> &buffer : buffers) { + const unsigned int bufferCount = config_->at(0).bufferCount; + for (unsigned int bufferIdx = 0; bufferIdx < bufferCount; ++bufferIdx) { std::unique_ptr<Request> request = camera_->createRequest(); ASSERT_TRUE(request) << "Can't create request"; - ASSERT_EQ(request->addBuffer(stream, buffer.get()), 0) << "Can't set buffer for request"; + /* Add buffers for each stream. */ + unsigned int i = 0; + for (const auto &config : *config_) { + Stream *stream = config.stream(); + const auto &buffers = allocator_->buffers(stream); + + ASSERT_EQ(request->addBuffer(stream, buffers[bufferIdx].get()), 0) + << "Can't add buffers for stream " << i; + + ++i; + } ASSERT_EQ(camera_->queueRequest(request.get()), 0) << "Failed to queue request"; diff --git a/src/apps/lc-compliance/helpers/capture.h b/src/apps/lc-compliance/helpers/capture.h index 0574ab1c7ac7..3e2b2889244d 100644 --- a/src/apps/lc-compliance/helpers/capture.h +++ b/src/apps/lc-compliance/helpers/capture.h @@ -16,7 +16,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 284d36307619..3d3cc97791d9 100644 --- a/src/apps/lc-compliance/tests/capture_test.cpp +++ b/src/apps/lc-compliance/tests/capture_test.cpp @@ -17,14 +17,14 @@ using namespace libcamera; const std::vector<int> NUMREQUESTS = { 1, 2, 3, 5, 8, 13, 21, 34, 55, 89 }; -const std::vector<StreamRole> ROLES = { - StreamRole::Raw, - StreamRole::StillCapture, - StreamRole::VideoRecording, - StreamRole::Viewfinder +const std::vector<std::vector<StreamRole>> ROLES = { + { StreamRole::Raw }, + { StreamRole::StillCapture }, + { StreamRole::VideoRecording }, + { StreamRole::Viewfinder }, }; -class SingleStream : public testing::TestWithParam<std::tuple<StreamRole, int>> +class SingleStream : public testing::TestWithParam<std::tuple<std::vector<StreamRole>, int>> { public: static std::string nameParameters(const testing::TestParamInfo<SingleStream::ParamType> &info); @@ -67,7 +67,7 @@ std::string SingleStream::nameParameters(const testing::TestParamInfo<SingleStre { StreamRole::Viewfinder, "Viewfinder" } }; - std::string roleName = rolesMap[std::get<0>(info.param)]; + std::string roleName = rolesMap[std::get<0>(info.param)[0]]; std::string numRequestsName = std::to_string(std::get<1>(info.param)); return roleName + "_" + numRequestsName; @@ -82,11 +82,11 @@ std::string SingleStream::nameParameters(const testing::TestParamInfo<SingleStre */ TEST_P(SingleStream, Capture) { - auto [role, numRequests] = GetParam(); + const auto [role, numRequests] = GetParam(); CaptureBalanced capture(camera_); - capture.configure(role); + capture.configure(Span<const StreamRole>{ role }); capture.capture(numRequests); } @@ -100,12 +100,12 @@ TEST_P(SingleStream, Capture) */ TEST_P(SingleStream, CaptureStartStop) { - auto [role, numRequests] = GetParam(); + const auto [role, numRequests] = GetParam(); unsigned int numRepeats = 3; CaptureBalanced capture(camera_); - capture.configure(role); + capture.configure(Span<const StreamRole>{ role }); for (unsigned int starts = 0; starts < numRepeats; starts++) capture.capture(numRequests); @@ -120,11 +120,11 @@ TEST_P(SingleStream, CaptureStartStop) */ TEST_P(SingleStream, UnbalancedStop) { - auto [role, numRequests] = GetParam(); + const auto [role, numRequests] = GetParam(); CaptureUnbalanced capture(camera_); - capture.configure(role); + capture.configure(Span<const StreamRole>{ role }); capture.capture(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. Multi-stream support will be added in next patches. Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> --- src/apps/lc-compliance/helpers/capture.cpp | 85 ++++++++++++++----- src/apps/lc-compliance/helpers/capture.h | 2 +- src/apps/lc-compliance/tests/capture_test.cpp | 26 +++--- 3 files changed, 76 insertions(+), 37 deletions(-)