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 at 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);
> > > }