[{"id":32910,"web_url":"https://patchwork.libcamera.org/comment/32910/","msgid":"<173505857852.4056627.3756300565202742865@ping.linuxembedded.co.uk>","date":"2024-12-24T16:42:58","subject":"Re: [RFC PATCH v1 12/12] apps: lc-compliance: Add multi-stream tests","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Barnabás Pőcze (2024-12-20 15:08:58)\n> Rename the `SingleStream` test to `SimpleCapture`, and\n> extend it to support using multiple roles. And instantiate\n> another test suite from the `SimpleCapture` test that\n> tests multiple streams in one capture session.\n> \n> Co-developed-by: Jacopo Mondi \n> Signed-off-by: Jacopo Mondi \n> Signed-off-by: Barnabás Pőcze \n> ---\n> src/apps/lc-compliance/tests/capture_test.cpp | 85 +++++++++++--------\n> 1 file changed, 48 insertions(+), 37 deletions(-)\n> \n> diff --git a/src/apps/lc-compliance/tests/capture_test.cpp b/src/apps/lc-compliance/tests/capture_test.cpp\n> index c382fcf47..8ea422f0d 100644\n> --- a/src/apps/lc-compliance/tests/capture_test.cpp\n> +++ b/src/apps/lc-compliance/tests/capture_test.cpp\n> @@ -8,7 +8,7 @@\n> \n> #include \"capture.h\"\n> \n> -#include \n> +#include \n> \n> #include \n> \n> @@ -18,19 +18,10 @@ namespace {\n> \n> using namespace libcamera;\n> \n> -const int NUMREQUESTS[] = { 1, 2, 3, 5, 8, 13, 21, 34, 55, 89 };\n> -\n> -const StreamRole ROLES[] = {\n> - StreamRole::Raw,\n> - StreamRole::StillCapture,\n> - StreamRole::VideoRecording,\n> - StreamRole::Viewfinder\n> -};\n> -\n> -class SingleStream : public testing::TestWithParam>\n> +class SimpleCapture : public testing::TestWithParam, int>>\n> {\n> public:\n> - static std::string nameParameters(const testing::TestParamInfo &info);\n> + static std::string nameParameters(const testing::TestParamInfo &info);\n> \n> protected:\n> void SetUp() override;\n> @@ -43,7 +34,7 @@ protected:\n> * We use gtest's SetUp() and TearDown() instead of constructor and destructor\n> * in order to be able to assert on them.\n> */\n> -void SingleStream::SetUp()\n> +void SimpleCapture::SetUp()\n> {\n> Environment *env = Environment::get();\n> \n> @@ -52,7 +43,7 @@ void SingleStream::SetUp()\n> ASSERT_EQ(camera_->acquire(), 0);\n> }\n> \n> -void SingleStream::TearDown()\n> +void SimpleCapture::TearDown()\n> {\n> if (!camera_)\n> return;\n> @@ -61,19 +52,17 @@ void SingleStream::TearDown()\n> camera_.reset();\n> }\n> \n> -std::string SingleStream::nameParameters(const testing::TestParamInfo &info)\n> +std::string SimpleCapture::nameParameters(const testing::TestParamInfo &info)\n> {\n> - std::map rolesMap = {\n> - { StreamRole::Raw, \"Raw\" },\n> - { StreamRole::StillCapture, \"StillCapture\" },\n> - { StreamRole::VideoRecording, \"VideoRecording\" },\n> - { StreamRole::Viewfinder, \"Viewfinder\" }\n> - };\n> + const auto &[roles, numRequests] = info.param;\n> + std::ostringstream ss;\n> \n> - std::string roleName = rolesMap[std::get<0>(info.param)];\n> - std::string numRequestsName = std::to_string(std::get<1>(info.param));\n> + for (StreamRole r : roles)\n> + ss << r << '_';\n> \n> - return roleName + \"_\" + numRequestsName;\n> + ss << '_' << numRequests;\n> +\n> + return std::move(ss).str();\n> }\n> \n> /*\n> @@ -83,13 +72,13 @@ std::string SingleStream::nameParameters(const testing::TestParamInfo * failure is a camera that completes less requests than the number of requests\n> * queued.\n> */\n> -TEST_P(SingleStream, Capture)\n> +TEST_P(SimpleCapture, Capture)\n> {\n> - auto [role, numRequests] = GetParam();\n> + const auto &[roles, numRequests] = GetParam();\n> \n> CaptureBalanced capture(camera_);\n> \n> - capture.configure(std::array{ role });\n> + capture.configure(roles);\n> \n> capture.capture(numRequests);\n> }\n> @@ -101,14 +90,14 @@ TEST_P(SingleStream, Capture)\n> * a camera that does not clean up correctly in its error path but is only\n> * tested by single-capture applications.\n> */\n> -TEST_P(SingleStream, CaptureStartStop)\n> +TEST_P(SimpleCapture, CaptureStartStop)\n> {\n> - auto [role, numRequests] = GetParam();\n> + const auto &[roles, numRequests] = GetParam();\n> unsigned int numRepeats = 3;\n> \n> CaptureBalanced capture(camera_);\n> \n> - capture.configure(std::array{ role });\n> + capture.configure(roles);\n> \n> for (unsigned int starts = 0; starts < numRepeats; starts++)\n> capture.capture(numRequests);\n> @@ -121,21 +110,43 @@ TEST_P(SingleStream, CaptureStartStop)\n> * is a camera that does not handle cancelation of buffers coming back from the\n> * video device while stopping.\n> */\n> -TEST_P(SingleStream, UnbalancedStop)\n> +TEST_P(SimpleCapture, UnbalancedStop)\n> {\n> - auto [role, numRequests] = GetParam();\n> + const auto &[roles, numRequests] = GetParam();\n> \n> CaptureUnbalanced capture(camera_);\n> \n> - capture.configure(std::array{ role });\n> + capture.configure(roles);\n> \n> capture.capture(numRequests);\n> }\n> \n> -INSTANTIATE_TEST_SUITE_P(CaptureTests,\n> - SingleStream,\n> - testing::Combine(testing::ValuesIn(ROLES),\n> +const int NUMREQUESTS[] = { 1, 2, 3, 5, 8, 13, 21, 34, 55, 89 };\n\nNot from your series, but I'm not sure I understand the full relevance\nof why we use this sequence of 10 tests for each type. Do we really need\nto run each test 10 times for each of these combinations? Running\nlc-compliance seems to take a long time waiting for each of these\ncombinations and I can't see what value that really adds yet.\n\nI could understand 1, 2, 3, 5 as variants that might or might not stream\nbased on minimum buffer requirements, but from there - I can't see what\ndifference we get from 8, 13, 21, 34, 55, 89. I'd be tempted to chop\nthis down to just something like { 1, 2, 3, 5, 8, 30, 90 }; or somehow\nmore aligned to what we actually want to test.\n\nEspecially when we get to the multi-stream role combinations, where we\nmight want to do different combinations such as a sequences that do not\nhave a buffer in one of the streams.\n\nBut aside from that which is more of a higher level discussion, I like\nthis patch simplifying things and handling / using testing::Combine to\ncreate / manage the test suites.\n\n\n\nActions from the any discussions on the above could easily be on top.\n\nReviewed-by: Kieran Bingham \n> +\n> +const std::vector SINGLEROLES[] = {\n> + { StreamRole::Raw, },\n> + { StreamRole::StillCapture, },\n> + { StreamRole::VideoRecording, },\n> + { StreamRole::Viewfinder, },\n> +};\n> +\n> +const std::vector MULTIROLES[] = {\n> + { StreamRole::Raw, StreamRole::StillCapture },\n> + { StreamRole::Raw, StreamRole::VideoRecording },\n> + { StreamRole::StillCapture, StreamRole::VideoRecording },\n> + { StreamRole::VideoRecording, StreamRole::VideoRecording },\n> +};\n> +\n> +INSTANTIATE_TEST_SUITE_P(SingleStream,\n> + SimpleCapture,\n> + testing::Combine(testing::ValuesIn(SINGLEROLES),\n> + testing::ValuesIn(NUMREQUESTS)),\n> + SimpleCapture::nameParameters);\n> +\n> +INSTANTIATE_TEST_SUITE_P(MultiStream,\n> + SimpleCapture,\n> + testing::Combine(testing::ValuesIn(MULTIROLES),\n> testing::ValuesIn(NUMREQUESTS)),\n> - SingleStream::nameParameters);\n> + SimpleCapture::nameParameters);\n> \n> } /* namespace */\n> -- \n> 2.47.1\n> \n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 8A1C6C3226\n\tfor ;\n\tTue, 24 Dec 2024 16:43:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7BF1F684C1;\n\tTue, 24 Dec 2024 17:43:03 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 91D1E68492\n\tfor ;\n\tTue, 24 Dec 2024 17:43:01 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 19B524AD;\n\tTue, 24 Dec 2024 17:42:19 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"mRPlGf/j\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1735058539;\n\tbh=yiALm2Y8BWV5G4ICKPxMriABQ6lhkZ46kOP0N36QTic=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=mRPlGf/jQySV6qyoAFTEFcUd8WapD7KDgYm6xT0e4HeK1ZdF/Jj3c+D7Q9yWzYj2W\n\t2gsS4vwlzmCDybaXrwdB2pgOaZEtd4OCXvLaXay1IEjVNHPM3WB173/Lig/n9NXbE5\n\tAILMJujZmHtr/5NUBfAGztzay0oqdIpN787DtGt0=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20241220150759.709756-13-pobrn@protonmail.com>","References":"<20241220150759.709756-1-pobrn@protonmail.com>\n\t<20241220150759.709756-13-pobrn@protonmail.com>","Subject":"Re: [RFC PATCH v1 12/12] apps: lc-compliance: Add multi-stream tests","From":"Kieran Bingham ","Cc":"Jacopo Mondi ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 24 Dec 2024 16:42:58 +0000","Message-ID":"<173505857852.4056627.3756300565202742865@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32936,"web_url":"https://patchwork.libcamera.org/comment/32936/","msgid":"","date":"2025-01-07T08:43:13","subject":"Re: [RFC PATCH v1 12/12] apps: lc-compliance: Add multi-stream tests","submitter":{"id":133,"url":"https://patchwork.libcamera.org/api/people/133/","name":"Pőcze Barnabás","email":"pobrn@protonmail.com"},"content":"Hi\n\n\n2024. december 24., kedd 17:42 keltezéssel, Kieran Bingham írta:\n\n> Quoting Barnabás Pőcze (2024-12-20 15:08:58)\n> > Rename the `SingleStream` test to `SimpleCapture`, and\n> > extend it to support using multiple roles. And instantiate\n> > another test suite from the `SimpleCapture` test that\n> > tests multiple streams in one capture session.\n> >\n> > Co-developed-by: Jacopo Mondi \n> > Signed-off-by: Jacopo Mondi \n> > Signed-off-by: Barnabás Pőcze \n> > ---\n> > src/apps/lc-compliance/tests/capture_test.cpp | 85 +++++++++++--------\n> > 1 file changed, 48 insertions(+), 37 deletions(-)\n> >\n> > diff --git a/src/apps/lc-compliance/tests/capture_test.cpp b/src/apps/lc-compliance/tests/capture_test.cpp\n> > index c382fcf47..8ea422f0d 100644\n> > --- a/src/apps/lc-compliance/tests/capture_test.cpp\n> > +++ b/src/apps/lc-compliance/tests/capture_test.cpp\n> > @@ -8,7 +8,7 @@\n> >\n> > #include \"capture.h\"\n> >\n> > -#include \n> > +#include \n> >\n> > #include \n> >\n> > @@ -18,19 +18,10 @@ namespace {\n> >\n> > using namespace libcamera;\n> >\n> > -const int NUMREQUESTS[] = { 1, 2, 3, 5, 8, 13, 21, 34, 55, 89 };\n> > -\n> > -const StreamRole ROLES[] = {\n> > - StreamRole::Raw,\n> > - StreamRole::StillCapture,\n> > - StreamRole::VideoRecording,\n> > - StreamRole::Viewfinder\n> > -};\n> > -\n> > -class SingleStream : public testing::TestWithParam>\n> > +class SimpleCapture : public testing::TestWithParam, int>>\n> > {\n> > public:\n> > - static std::string nameParameters(const testing::TestParamInfo &info);\n> > + static std::string nameParameters(const testing::TestParamInfo &info);\n> >\n> > protected:\n> > void SetUp() override;\n> > @@ -43,7 +34,7 @@ protected:\n> > * We use gtest's SetUp() and TearDown() instead of constructor and destructor\n> > * in order to be able to assert on them.\n> > */\n> > -void SingleStream::SetUp()\n> > +void SimpleCapture::SetUp()\n> > {\n> > Environment *env = Environment::get();\n> >\n> > @@ -52,7 +43,7 @@ void SingleStream::SetUp()\n> > ASSERT_EQ(camera_->acquire(), 0);\n> > }\n> >\n> > -void SingleStream::TearDown()\n> > +void SimpleCapture::TearDown()\n> > {\n> > if (!camera_)\n> > return;\n> > @@ -61,19 +52,17 @@ void SingleStream::TearDown()\n> > camera_.reset();\n> > }\n> >\n> > -std::string SingleStream::nameParameters(const testing::TestParamInfo &info)\n> > +std::string SimpleCapture::nameParameters(const testing::TestParamInfo &info)\n> > {\n> > - std::map rolesMap = {\n> > - { StreamRole::Raw, \"Raw\" },\n> > - { StreamRole::StillCapture, \"StillCapture\" },\n> > - { StreamRole::VideoRecording, \"VideoRecording\" },\n> > - { StreamRole::Viewfinder, \"Viewfinder\" }\n> > - };\n> > + const auto &[roles, numRequests] = info.param;\n> > + std::ostringstream ss;\n> >\n> > - std::string roleName = rolesMap[std::get<0>(info.param)];\n> > - std::string numRequestsName = std::to_string(std::get<1>(info.param));\n> > + for (StreamRole r : roles)\n> > + ss << r << '_';\n> >\n> > - return roleName + \"_\" + numRequestsName;\n> > + ss << '_' << numRequests;\n> > +\n> > + return std::move(ss).str();\n> > }\n> >\n> > /*\n> > @@ -83,13 +72,13 @@ std::string SingleStream::nameParameters(const testing::TestParamInfo > * failure is a camera that completes less requests than the number of requests\n> > * queued.\n> > */\n> > -TEST_P(SingleStream, Capture)\n> > +TEST_P(SimpleCapture, Capture)\n> > {\n> > - auto [role, numRequests] = GetParam();\n> > + const auto &[roles, numRequests] = GetParam();\n> >\n> > CaptureBalanced capture(camera_);\n> >\n> > - capture.configure(std::array{ role });\n> > + capture.configure(roles);\n> >\n> > capture.capture(numRequests);\n> > }\n> > @@ -101,14 +90,14 @@ TEST_P(SingleStream, Capture)\n> > * a camera that does not clean up correctly in its error path but is only\n> > * tested by single-capture applications.\n> > */\n> > -TEST_P(SingleStream, CaptureStartStop)\n> > +TEST_P(SimpleCapture, CaptureStartStop)\n> > {\n> > - auto [role, numRequests] = GetParam();\n> > + const auto &[roles, numRequests] = GetParam();\n> > unsigned int numRepeats = 3;\n> >\n> > CaptureBalanced capture(camera_);\n> >\n> > - capture.configure(std::array{ role });\n> > + capture.configure(roles);\n> >\n> > for (unsigned int starts = 0; starts < numRepeats; starts++)\n> > capture.capture(numRequests);\n> > @@ -121,21 +110,43 @@ TEST_P(SingleStream, CaptureStartStop)\n> > * is a camera that does not handle cancelation of buffers coming back from the\n> > * video device while stopping.\n> > */\n> > -TEST_P(SingleStream, UnbalancedStop)\n> > +TEST_P(SimpleCapture, UnbalancedStop)\n> > {\n> > - auto [role, numRequests] = GetParam();\n> > + const auto &[roles, numRequests] = GetParam();\n> >\n> > CaptureUnbalanced capture(camera_);\n> >\n> > - capture.configure(std::array{ role });\n> > + capture.configure(roles);\n> >\n> > capture.capture(numRequests);\n> > }\n> >\n> > -INSTANTIATE_TEST_SUITE_P(CaptureTests,\n> > - SingleStream,\n> > - testing::Combine(testing::ValuesIn(ROLES),\n> > +const int NUMREQUESTS[] = { 1, 2, 3, 5, 8, 13, 21, 34, 55, 89 };\n> \n> Not from your series, but I'm not sure I understand the full relevance\n> of why we use this sequence of 10 tests for each type. Do we really need\n> to run each test 10 times for each of these combinations? Running\n> lc-compliance seems to take a long time waiting for each of these\n> combinations and I can't see what value that really adds yet.\n\nYou're right. It takes a bit of time to run, this can be a problem. To be honest\nI just took these numbers from the single stream case. I believe the numbers\nfollow the Fibonacci sequence, so I am not sure if there is any more intent behind\nthem. I couldn't find anything in the git history in any case.\n\n\nRegards,\nBarnabás Pőcze\n\n\n> \n> I could understand 1, 2, 3, 5 as variants that might or might not stream\n> based on minimum buffer requirements, but from there - I can't see what\n> difference we get from 8, 13, 21, 34, 55, 89. I'd be tempted to chop\n> this down to just something like { 1, 2, 3, 5, 8, 30, 90 }; or somehow\n> more aligned to what we actually want to test.\n> \n> Especially when we get to the multi-stream role combinations, where we\n> might want to do different combinations such as a sequences that do not\n> have a buffer in one of the streams.\n> \n> But aside from that which is more of a higher level discussion, I like\n> this patch simplifying things and handling / using testing::Combine to\n> create / manage the test suites.\n> \n> \n> \n> Actions from the any discussions on the above could easily be on top.\n> \n> Reviewed-by: Kieran Bingham \n> > +\n> > +const std::vector SINGLEROLES[] = {\n> > + { StreamRole::Raw, },\n> > + { StreamRole::StillCapture, },\n> > + { StreamRole::VideoRecording, },\n> > + { StreamRole::Viewfinder, },\n> > +};\n> > +\n> > +const std::vector MULTIROLES[] = {\n> > + { StreamRole::Raw, StreamRole::StillCapture },\n> > + { StreamRole::Raw, StreamRole::VideoRecording },\n> > + { StreamRole::StillCapture, StreamRole::VideoRecording },\n> > + { StreamRole::VideoRecording, StreamRole::VideoRecording },\n> > +};\n> > +\n> > +INSTANTIATE_TEST_SUITE_P(SingleStream,\n> > + SimpleCapture,\n> > + testing::Combine(testing::ValuesIn(SINGLEROLES),\n> > + testing::ValuesIn(NUMREQUESTS)),\n> > + SimpleCapture::nameParameters);\n> > +\n> > +INSTANTIATE_TEST_SUITE_P(MultiStream,\n> > + SimpleCapture,\n> > + testing::Combine(testing::ValuesIn(MULTIROLES),\n> > testing::ValuesIn(NUMREQUESTS)),\n> > - SingleStream::nameParameters);\n> > + SimpleCapture::nameParameters);\n> >\n> > } /* namespace */\n> > --\n> > 2.47.1\n> >\n> >\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 0A067BD80A\n\tfor ;\n\tTue, 7 Jan 2025 08:43:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7B2C5684E2;\n\tTue, 7 Jan 2025 09:43:26 +0100 (CET)","from mail-10631.protonmail.ch (mail-10631.protonmail.ch\n\t[79.135.106.31])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 785A9684D0\n\tfor ;\n\tTue, 7 Jan 2025 09:43:18 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=protonmail.com header.i=@protonmail.com\n\theader.b=\"VVcclpj6\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1736239397; x=1736498597;\n\tbh=qHRF7Kn3EraH364VNsNQJXTvl9u7N1sB2qwAppkqDAw=;\n\th=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References:\n\tFeedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID:\n\tMessage-ID:BIMI-Selector:List-Unsubscribe:List-Unsubscribe-Post;\n\tb=VVcclpj6dA3ejnSleKW48wwM83FKaoWNefQ2E4cx7bwBS9hFCOz5Kj40hA2h1++G6\n\tZ941H1THGdBBSLAE9mvpbsoVV1xCyIZfRQbdjnMsLZFH1dItHLl6pmMhTjEswzCbHw\n\teyEOSLT+TKvLEtP4YHBoP+1LknuDGRbzcsGdD4EDxzD7VtwCgY63ewd9Ol6hq++gCc\n\tqymhdI0Zwqmm1hfNJb93G5V09YnEruxk3OkQLx4v08QLykQNta7688m/dORs3BDbaS\n\t14iB+VbkYg/3mSaIVhOPGpfTaNKpjKFDVBuIJalk7V1nElEmS0PesQOxD4h9KGQOTK\n\tYlHejAW9es/4Q==","Date":"Tue, 07 Jan 2025 08:43:13 +0000","To":"Kieran Bingham ","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org,\n\tJacopo Mondi ","Subject":"Re: [RFC PATCH v1 12/12] apps: lc-compliance: Add multi-stream tests","Message-ID":"","In-Reply-To":"<173505857852.4056627.3756300565202742865@ping.linuxembedded.co.uk>","References":"<20241220150759.709756-1-pobrn@protonmail.com>\n\t<20241220150759.709756-13-pobrn@protonmail.com>\n\t<173505857852.4056627.3756300565202742865@ping.linuxembedded.co.uk>","Feedback-ID":"20568564:user:proton","X-Pm-Message-ID":"9e79d64043945a320f8a36a34e32398843f0b1f6","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":33006,"web_url":"https://patchwork.libcamera.org/comment/33006/","msgid":"<20250110013956.GN6159@pendragon.ideasonboard.com>","date":"2025-01-10T01:39:56","subject":"Re: [RFC PATCH v1 12/12] apps: lc-compliance: Add multi-stream tests","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Barnabás,\n\nThank you for the patch.\n\nOn Tue, Jan 07, 2025 at 08:43:13AM +0000, Barnabás Pőcze wrote:\n> 2024. december 24., kedd 17:42 keltezéssel, Kieran Bingham írta:\n> > Quoting Barnabás Pőcze (2024-12-20 15:08:58)\n> > > Rename the `SingleStream` test to `SimpleCapture`, and\n> > > extend it to support using multiple roles. And instantiate\n> > > another test suite from the `SimpleCapture` test that\n> > > tests multiple streams in one capture session.\n\nSounds like this could have been split in two patches.\n\n> > > Co-developed-by: Jacopo Mondi \n> > > Signed-off-by: Jacopo Mondi \n> > > Signed-off-by: Barnabás Pőcze \n> > > ---\n> > > src/apps/lc-compliance/tests/capture_test.cpp | 85 +++++++++++--------\n> > > 1 file changed, 48 insertions(+), 37 deletions(-)\n> > >\n> > > diff --git a/src/apps/lc-compliance/tests/capture_test.cpp b/src/apps/lc-compliance/tests/capture_test.cpp\n> > > index c382fcf47..8ea422f0d 100644\n> > > --- a/src/apps/lc-compliance/tests/capture_test.cpp\n> > > +++ b/src/apps/lc-compliance/tests/capture_test.cpp\n> > > @@ -8,7 +8,7 @@\n> > >\n> > > #include \"capture.h\"\n> > >\n> > > -#include \n> > > +#include \n> > >\n> > > #include \n> > >\n> > > @@ -18,19 +18,10 @@ namespace {\n> > >\n> > > using namespace libcamera;\n> > >\n> > > -const int NUMREQUESTS[] = { 1, 2, 3, 5, 8, 13, 21, 34, 55, 89 };\n> > > -\n> > > -const StreamRole ROLES[] = {\n> > > - StreamRole::Raw,\n> > > - StreamRole::StillCapture,\n> > > - StreamRole::VideoRecording,\n> > > - StreamRole::Viewfinder\n> > > -};\n> > > -\n> > > -class SingleStream : public testing::TestWithParam>\n> > > +class SimpleCapture : public testing::TestWithParam, int>>\n> > > {\n> > > public:\n> > > - static std::string nameParameters(const testing::TestParamInfo &info);\n> > > + static std::string nameParameters(const testing::TestParamInfo &info);\n> > >\n> > > protected:\n> > > void SetUp() override;\n> > > @@ -43,7 +34,7 @@ protected:\n> > > * We use gtest's SetUp() and TearDown() instead of constructor and destructor\n> > > * in order to be able to assert on them.\n> > > */\n> > > -void SingleStream::SetUp()\n> > > +void SimpleCapture::SetUp()\n> > > {\n> > > Environment *env = Environment::get();\n> > >\n> > > @@ -52,7 +43,7 @@ void SingleStream::SetUp()\n> > > ASSERT_EQ(camera_->acquire(), 0);\n> > > }\n> > >\n> > > -void SingleStream::TearDown()\n> > > +void SimpleCapture::TearDown()\n> > > {\n> > > if (!camera_)\n> > > return;\n> > > @@ -61,19 +52,17 @@ void SingleStream::TearDown()\n> > > camera_.reset();\n> > > }\n> > >\n> > > -std::string SingleStream::nameParameters(const testing::TestParamInfo &info)\n> > > +std::string SimpleCapture::nameParameters(const testing::TestParamInfo &info)\n> > > {\n> > > - std::map rolesMap = {\n> > > - { StreamRole::Raw, \"Raw\" },\n> > > - { StreamRole::StillCapture, \"StillCapture\" },\n> > > - { StreamRole::VideoRecording, \"VideoRecording\" },\n> > > - { StreamRole::Viewfinder, \"Viewfinder\" }\n> > > - };\n> > > + const auto &[roles, numRequests] = info.param;\n> > > + std::ostringstream ss;\n> > >\n> > > - std::string roleName = rolesMap[std::get<0>(info.param)];\n> > > - std::string numRequestsName = std::to_string(std::get<1>(info.param));\n> > > + for (StreamRole r : roles)\n> > > + ss << r << '_';\n> > >\n> > > - return roleName + \"_\" + numRequestsName;\n> > > + ss << '_' << numRequests;\n> > > +\n> > > + return std::move(ss).str();\n> > > }\n> > >\n> > > /*\n> > > @@ -83,13 +72,13 @@ std::string SingleStream::nameParameters(const testing::TestParamInfo > > * failure is a camera that completes less requests than the number of requests\n> > > * queued.\n> > > */\n> > > -TEST_P(SingleStream, Capture)\n> > > +TEST_P(SimpleCapture, Capture)\n> > > {\n> > > - auto [role, numRequests] = GetParam();\n> > > + const auto &[roles, numRequests] = GetParam();\n> > >\n> > > CaptureBalanced capture(camera_);\n> > >\n> > > - capture.configure(std::array{ role });\n> > > + capture.configure(roles);\n> > >\n> > > capture.capture(numRequests);\n> > > }\n> > > @@ -101,14 +90,14 @@ TEST_P(SingleStream, Capture)\n> > > * a camera that does not clean up correctly in its error path but is only\n> > > * tested by single-capture applications.\n> > > */\n> > > -TEST_P(SingleStream, CaptureStartStop)\n> > > +TEST_P(SimpleCapture, CaptureStartStop)\n> > > {\n> > > - auto [role, numRequests] = GetParam();\n> > > + const auto &[roles, numRequests] = GetParam();\n> > > unsigned int numRepeats = 3;\n> > >\n> > > CaptureBalanced capture(camera_);\n> > >\n> > > - capture.configure(std::array{ role });\n> > > + capture.configure(roles);\n> > >\n> > > for (unsigned int starts = 0; starts < numRepeats; starts++)\n> > > capture.capture(numRequests);\n> > > @@ -121,21 +110,43 @@ TEST_P(SingleStream, CaptureStartStop)\n> > > * is a camera that does not handle cancelation of buffers coming back from the\n> > > * video device while stopping.\n> > > */\n> > > -TEST_P(SingleStream, UnbalancedStop)\n> > > +TEST_P(SimpleCapture, UnbalancedStop)\n> > > {\n> > > - auto [role, numRequests] = GetParam();\n> > > + const auto &[roles, numRequests] = GetParam();\n> > >\n> > > CaptureUnbalanced capture(camera_);\n> > >\n> > > - capture.configure(std::array{ role });\n> > > + capture.configure(roles);\n> > >\n> > > capture.capture(numRequests);\n> > > }\n> > >\n> > > -INSTANTIATE_TEST_SUITE_P(CaptureTests,\n> > > - SingleStream,\n> > > - testing::Combine(testing::ValuesIn(ROLES),\n> > > +const int NUMREQUESTS[] = { 1, 2, 3, 5, 8, 13, 21, 34, 55, 89 };\n\nWhile at it, s/NUMREQUESTS/kNumRequests/ to match our coding style ?\nSame below.\n\n> > \n> > Not from your series, but I'm not sure I understand the full relevance\n> > of why we use this sequence of 10 tests for each type. Do we really need\n> > to run each test 10 times for each of these combinations? Running\n> > lc-compliance seems to take a long time waiting for each of these\n> > combinations and I can't see what value that really adds yet.\n> \n> You're right. It takes a bit of time to run, this can be a problem. To be honest\n> I just took these numbers from the single stream case. I believe the numbers\n> follow the Fibonacci sequence, so I am not sure if there is any more intent behind\n> them. I couldn't find anything in the git history in any case.\n> \n> > I could understand 1, 2, 3, 5 as variants that might or might not stream\n> > based on minimum buffer requirements, but from there - I can't see what\n> > difference we get from 8, 13, 21, 34, 55, 89. I'd be tempted to chop\n> > this down to just something like { 1, 2, 3, 5, 8, 30, 90 }; or somehow\n> > more aligned to what we actually want to test.\n> > \n> > Especially when we get to the multi-stream role combinations, where we\n> > might want to do different combinations such as a sequences that do not\n> > have a buffer in one of the streams.\n> > \n> > But aside from that which is more of a higher level discussion, I like\n> > this patch simplifying things and handling / using testing::Combine to\n> > create / manage the test suites.\n> > \n> > Actions from the any discussions on the above could easily be on top.\n> > \n> > Reviewed-by: Kieran Bingham \n\nAgreed.\n\nReviewed-by: Laurent Pinchart \n\n> > > +\n> > > +const std::vector SINGLEROLES[] = {\n> > > + { StreamRole::Raw, },\n> > > + { StreamRole::StillCapture, },\n> > > + { StreamRole::VideoRecording, },\n> > > + { StreamRole::Viewfinder, },\n> > > +};\n> > > +\n> > > +const std::vector MULTIROLES[] = {\n> > > + { StreamRole::Raw, StreamRole::StillCapture },\n> > > + { StreamRole::Raw, StreamRole::VideoRecording },\n> > > + { StreamRole::StillCapture, StreamRole::VideoRecording },\n> > > + { StreamRole::VideoRecording, StreamRole::VideoRecording },\n> > > +};\n> > > +\n> > > +INSTANTIATE_TEST_SUITE_P(SingleStream,\n> > > + SimpleCapture,\n> > > + testing::Combine(testing::ValuesIn(SINGLEROLES),\n> > > + testing::ValuesIn(NUMREQUESTS)),\n> > > + SimpleCapture::nameParameters);\n> > > +\n> > > +INSTANTIATE_TEST_SUITE_P(MultiStream,\n> > > + SimpleCapture,\n> > > + testing::Combine(testing::ValuesIn(MULTIROLES),\n> > > testing::ValuesIn(NUMREQUESTS)),\n> > > - SingleStream::nameParameters);\n> > > + SimpleCapture::nameParameters);\n> > >\n> > > } /* namespace */","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 0048FC32EF\n\tfor ;\n\tFri, 10 Jan 2025 01:40:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DAC71684EA;\n\tFri, 10 Jan 2025 02:40:01 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D3680608A9\n\tfor ;\n\tFri, 10 Jan 2025 02:39:59 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 988768FA;\n\tFri, 10 Jan 2025 02:39:05 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"gvb7npus\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1736473145;\n\tbh=SuHoM7kSy+PZkwUZKpgu1s1y1v5h9IYVcE6Y20kJ0XM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=gvb7npusEvQGfCQTRQCdGWWbDp4UrpBGyxGuBcXMmFDljv9oi04EwnmtAL8TBDjfo\n\teSbPsUK9gqE5LFONVFOZWBz+TWXZF/Z4YLDkxPZxlxArfJOo77qRgAFlHQvzSe5y6C\n\tjywlyHhZ+1mABMY9tKKkO4eAvYbXJ+0cHuj0VAVU=","Date":"Fri, 10 Jan 2025 03:39:56 +0200","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org,\n\tJacopo Mondi ","Subject":"Re: [RFC PATCH v1 12/12] apps: lc-compliance: Add multi-stream tests","Message-ID":"<20250110013956.GN6159@pendragon.ideasonboard.com>","References":"<20241220150759.709756-1-pobrn@protonmail.com>\n\t<20241220150759.709756-13-pobrn@protonmail.com>\n\t<173505857852.4056627.3756300565202742865@ping.linuxembedded.co.uk>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]