[RFC PATCH v1 12/12] apps: lc-compliance: Add multi-stream tests
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Jan 10 02:39:56 CET 2025
Hi Barnabás,
Thank you for the patch.
On Tue, Jan 07, 2025 at 08:43:13AM +0000, Barnabás Pőcze wrote:
> 2024. december 24., kedd 17:42 keltezéssel, Kieran Bingham írta:
> > Quoting Barnabás Pőcze (2024-12-20 15:08:58)
> > > Rename the `SingleStream` test to `SimpleCapture`, and
> > > extend it to support using multiple roles. And instantiate
> > > another test suite from the `SimpleCapture` test that
> > > tests multiple streams in one capture session.
Sounds like this could have been split in two 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/tests/capture_test.cpp | 85 +++++++++++--------
> > > 1 file changed, 48 insertions(+), 37 deletions(-)
> > >
> > > diff --git a/src/apps/lc-compliance/tests/capture_test.cpp b/src/apps/lc-compliance/tests/capture_test.cpp
> > > index c382fcf47..8ea422f0d 100644
> > > --- a/src/apps/lc-compliance/tests/capture_test.cpp
> > > +++ b/src/apps/lc-compliance/tests/capture_test.cpp
> > > @@ -8,7 +8,7 @@
> > >
> > > #include "capture.h"
> > >
> > > -#include <iostream>
> > > +#include <sstream>
> > >
> > > #include <gtest/gtest.h>
> > >
> > > @@ -18,19 +18,10 @@ namespace {
> > >
> > > using namespace libcamera;
> > >
> > > -const int NUMREQUESTS[] = { 1, 2, 3, 5, 8, 13, 21, 34, 55, 89 };
> > > -
> > > -const StreamRole ROLES[] = {
> > > - StreamRole::Raw,
> > > - StreamRole::StillCapture,
> > > - StreamRole::VideoRecording,
> > > - StreamRole::Viewfinder
> > > -};
> > > -
> > > -class SingleStream : public testing::TestWithParam<std::tuple<StreamRole, int>>
> > > +class SimpleCapture : public testing::TestWithParam<std::tuple<std::vector<StreamRole>, int>>
> > > {
> > > public:
> > > - static std::string nameParameters(const testing::TestParamInfo<SingleStream::ParamType> &info);
> > > + static std::string nameParameters(const testing::TestParamInfo<SimpleCapture::ParamType> &info);
> > >
> > > protected:
> > > void SetUp() override;
> > > @@ -43,7 +34,7 @@ protected:
> > > * We use gtest's SetUp() and TearDown() instead of constructor and destructor
> > > * in order to be able to assert on them.
> > > */
> > > -void SingleStream::SetUp()
> > > +void SimpleCapture::SetUp()
> > > {
> > > Environment *env = Environment::get();
> > >
> > > @@ -52,7 +43,7 @@ void SingleStream::SetUp()
> > > ASSERT_EQ(camera_->acquire(), 0);
> > > }
> > >
> > > -void SingleStream::TearDown()
> > > +void SimpleCapture::TearDown()
> > > {
> > > if (!camera_)
> > > return;
> > > @@ -61,19 +52,17 @@ void SingleStream::TearDown()
> > > camera_.reset();
> > > }
> > >
> > > -std::string SingleStream::nameParameters(const testing::TestParamInfo<SingleStream::ParamType> &info)
> > > +std::string SimpleCapture::nameParameters(const testing::TestParamInfo<SimpleCapture::ParamType> &info)
> > > {
> > > - std::map<StreamRole, std::string> rolesMap = {
> > > - { StreamRole::Raw, "Raw" },
> > > - { StreamRole::StillCapture, "StillCapture" },
> > > - { StreamRole::VideoRecording, "VideoRecording" },
> > > - { StreamRole::Viewfinder, "Viewfinder" }
> > > - };
> > > + const auto &[roles, numRequests] = info.param;
> > > + std::ostringstream ss;
> > >
> > > - std::string roleName = rolesMap[std::get<0>(info.param)];
> > > - std::string numRequestsName = std::to_string(std::get<1>(info.param));
> > > + for (StreamRole r : roles)
> > > + ss << r << '_';
> > >
> > > - return roleName + "_" + numRequestsName;
> > > + ss << '_' << numRequests;
> > > +
> > > + return std::move(ss).str();
> > > }
> > >
> > > /*
> > > @@ -83,13 +72,13 @@ std::string SingleStream::nameParameters(const testing::TestParamInfo<SingleStre
> > > * failure is a camera that completes less requests than the number of requests
> > > * queued.
> > > */
> > > -TEST_P(SingleStream, Capture)
> > > +TEST_P(SimpleCapture, Capture)
> > > {
> > > - auto [role, numRequests] = GetParam();
> > > + const auto &[roles, numRequests] = GetParam();
> > >
> > > CaptureBalanced capture(camera_);
> > >
> > > - capture.configure(std::array{ role });
> > > + capture.configure(roles);
> > >
> > > capture.capture(numRequests);
> > > }
> > > @@ -101,14 +90,14 @@ TEST_P(SingleStream, Capture)
> > > * a camera that does not clean up correctly in its error path but is only
> > > * tested by single-capture applications.
> > > */
> > > -TEST_P(SingleStream, CaptureStartStop)
> > > +TEST_P(SimpleCapture, CaptureStartStop)
> > > {
> > > - auto [role, numRequests] = GetParam();
> > > + const auto &[roles, numRequests] = GetParam();
> > > unsigned int numRepeats = 3;
> > >
> > > CaptureBalanced capture(camera_);
> > >
> > > - capture.configure(std::array{ role });
> > > + capture.configure(roles);
> > >
> > > for (unsigned int starts = 0; starts < numRepeats; starts++)
> > > capture.capture(numRequests);
> > > @@ -121,21 +110,43 @@ TEST_P(SingleStream, CaptureStartStop)
> > > * is a camera that does not handle cancelation of buffers coming back from the
> > > * video device while stopping.
> > > */
> > > -TEST_P(SingleStream, UnbalancedStop)
> > > +TEST_P(SimpleCapture, UnbalancedStop)
> > > {
> > > - auto [role, numRequests] = GetParam();
> > > + const auto &[roles, numRequests] = GetParam();
> > >
> > > CaptureUnbalanced capture(camera_);
> > >
> > > - capture.configure(std::array{ role });
> > > + capture.configure(roles);
> > >
> > > capture.capture(numRequests);
> > > }
> > >
> > > -INSTANTIATE_TEST_SUITE_P(CaptureTests,
> > > - SingleStream,
> > > - testing::Combine(testing::ValuesIn(ROLES),
> > > +const int NUMREQUESTS[] = { 1, 2, 3, 5, 8, 13, 21, 34, 55, 89 };
While at it, s/NUMREQUESTS/kNumRequests/ to match our coding style ?
Same below.
> >
> > Not from your series, but I'm not sure I understand the full relevance
> > of why we use this sequence of 10 tests for each type. Do we really need
> > to run each test 10 times for each of these combinations? Running
> > lc-compliance seems to take a long time waiting for each of these
> > combinations and I can't see what value that really adds yet.
>
> You're right. It takes a bit of time to run, this can be a problem. To be honest
> I just took these numbers from the single stream case. I believe the numbers
> follow the Fibonacci sequence, so I am not sure if there is any more intent behind
> them. I couldn't find anything in the git history in any case.
>
> > I could understand 1, 2, 3, 5 as variants that might or might not stream
> > based on minimum buffer requirements, but from there - I can't see what
> > difference we get from 8, 13, 21, 34, 55, 89. I'd be tempted to chop
> > this down to just something like { 1, 2, 3, 5, 8, 30, 90 }; or somehow
> > more aligned to what we actually want to test.
> >
> > Especially when we get to the multi-stream role combinations, where we
> > might want to do different combinations such as a sequences that do not
> > have a buffer in one of the streams.
> >
> > But aside from that which is more of a higher level discussion, I like
> > this patch simplifying things and handling / using testing::Combine to
> > create / manage the test suites.
> >
> > Actions from the any discussions on the above could easily be on top.
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
Agreed.
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > +
> > > +const std::vector<StreamRole> SINGLEROLES[] = {
> > > + { StreamRole::Raw, },
> > > + { StreamRole::StillCapture, },
> > > + { StreamRole::VideoRecording, },
> > > + { StreamRole::Viewfinder, },
> > > +};
> > > +
> > > +const std::vector<StreamRole> MULTIROLES[] = {
> > > + { StreamRole::Raw, StreamRole::StillCapture },
> > > + { StreamRole::Raw, StreamRole::VideoRecording },
> > > + { StreamRole::StillCapture, StreamRole::VideoRecording },
> > > + { StreamRole::VideoRecording, StreamRole::VideoRecording },
> > > +};
> > > +
> > > +INSTANTIATE_TEST_SUITE_P(SingleStream,
> > > + SimpleCapture,
> > > + testing::Combine(testing::ValuesIn(SINGLEROLES),
> > > + testing::ValuesIn(NUMREQUESTS)),
> > > + SimpleCapture::nameParameters);
> > > +
> > > +INSTANTIATE_TEST_SUITE_P(MultiStream,
> > > + SimpleCapture,
> > > + testing::Combine(testing::ValuesIn(MULTIROLES),
> > > testing::ValuesIn(NUMREQUESTS)),
> > > - SingleStream::nameParameters);
> > > + SimpleCapture::nameParameters);
> > >
> > > } /* namespace */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list