[RFC PATCH v1 12/12] apps: lc-compliance: Add multi-stream tests

[Kieran Bingham]

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.
> 
> 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 };

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.

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>
> +
> +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 */
> -- 
> 2.47.1
> 
>