[libcamera-devel] [RFC PATCH v2 3/3] lc-compliance: Refactor using Googletest
Niklas Söderlund
niklas.soderlund at ragnatech.se
Thu May 13 10:59:14 CEST 2021
Hello Nícolas,
On 2021-05-12 10:43:35 -0300, Nícolas F. R. A. Prado wrote:
> Hi Niklas,
>
> Em 2021-05-12 05:49, Niklas Söderlund escreveu:
>
> > Hi Nícolas,
> >
> > On 2021-05-11 14:17:56 -0300, Nícolas F. R. A. Prado wrote:
> > > Hi Niklas,
> > >
> > > Em 2021-05-10 18:04, Niklas Söderlund escreveu:
> > >
> > > > Hi Nícolas,
> > > >
> > > > Thanks for your work on this. I really think gtest is a step in the
> > > > right direction!
> > > >
> > > > On 2021-05-07 12:52:38 -0300, Nícolas F. R. A. Prado wrote:
> > > > > Hi Niklas,
> > > > >
> > > > > Em 2021-05-07 06:11, Niklas Söderlund escreveu:
> > > > >
> > > > > > Hi Nícolas,
> > > > > >
> > > > > > On 2021-05-06 17:20:29 -0300, Nícolas F. R. A. Prado wrote:
> > > > > > > Hi Niklas,
> > > > > > >
> > > > > > > thanks for the feedback.
> > > > > > >
> > > > > > > Em 2021-05-06 05:48, Niklas Söderlund escreveu:
> > > > > > >
> > > > > > > > Hi Nícolas,
> > > > > > > >
> > > > > > > > Thanks for your work!
> > > > > > > >
> > > > > > > > I really like how gtest helps make the actual tests to be easier to
> > > > > > > > read.
> > > > > > > >
> > > > > > > > On 2021-05-03 16:33:07 -0300, Nícolas F. R. A. Prado wrote:
> > > > > > > > > Refactor lc-compliance using Googletest as the test framework.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado at collabora.com>
> > > > > > > > > ---
> > > > > > > > > Changes in v2:
> > > > > > > > > - Changed from static to dynamic test registration
> > > > > > > > > - Removed -c flag
> > > > > > > > >
> > > > > > > > > There's still an issue with the refactoring that the shared_ptr of the tests
> > > > > > > > > apparently aren't being deleted on time, which causes:
> > > > > > > > >
> > > > > > > > > [20:26:08.744507935] [103243] ERROR DeviceEnumerator device_enumerator.cpp:165 Removing media device /dev/media0 while still in use
> > > > > > > > > [20:26:08.744548175] [103243] ERROR DeviceEnumerator device_enumerator.cpp:165 Removing media device /dev/media1 while still in use
> > > > > > > > > Segmentation fault (core dumped)
> > > > > > > > >
> > > > > > > > > I tried explicitly resetting the shared_ptr on the destructor of the tests but
> > > > > > > > > that didn't work. In fact, when just listing the tests, the constructor for the
> > > > > > > > > tests isn't even called. So I think the issue has to do with the passing of the
> > > > > > > > > camera shared pointer to the REGISTER_TESTS() macro through that functor (?):
> > > > > > > > >
> > > > > > > > > [=]() -> testsuite* { return new testcase(c, r, rq); }); \
> > > > > > > > >
> > > > > > > > > Not sure how to solve this. Any tip would be welcome.
> > > > > > > >
> > > > > > > > I will leave this for now as I have a different worry about
> > > > > > > > REGISTER_TESTS() design below.
> > > > > > > >
> > > > > > > > >
> > > > > > > > > src/lc-compliance/main.cpp | 99 +++++++++---------
> > > > > > > > > src/lc-compliance/meson.build | 3 +
> > > > > > > > > src/lc-compliance/simple_capture.cpp | 74 +++++--------
> > > > > > > > > src/lc-compliance/simple_capture.h | 8 +-
> > > > > > > > > src/lc-compliance/single_stream.cpp | 151 ++++++++++++++-------------
> > > > > > > > > src/lc-compliance/tests.h | 15 ++-
> > > > > > > > > 6 files changed, 175 insertions(+), 175 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp
> > > > > > > > > index 54cee54aa978..add0d7729aec 100644
> > > > > > > > > --- a/src/lc-compliance/main.cpp
> > > > > > > > > +++ b/src/lc-compliance/main.cpp
> > > > > > > > > @@ -9,6 +9,8 @@
> > > > > > > > > #include <iostream>
> > > > > > > > > #include <string.h>
> > > > > > > > >
> > > > > > > > > +#include <gtest/gtest.h>
> > > > > > > > > +
> > > > > > > > > #include <libcamera/libcamera.h>
> > > > > > > > >
> > > > > > > > > #include "../cam/options.h"
> > > > > > > > > @@ -17,7 +19,6 @@
> > > > > > > > > using namespace libcamera;
> > > > > > > > >
> > > > > > > > > enum {
> > > > > > > > > - OptCamera = 'c',
> > > > > > > > > OptHelp = 'h',
> > > > > > > > > };
> > > > > > > > >
> > > > > > > > > @@ -28,14 +29,15 @@ public:
> > > > > > > > > ~Harness();
> > > > > > > > >
> > > > > > > > > int exec();
> > > > > > > > > + int init();
> > > > > > > > > + void registerTests();
> > > > > > > > >
> > > > > > > > > private:
> > > > > > > > > - int init();
> > > > > > > > > void listCameras();
> > > > > > > > >
> > > > > > > > > OptionsParser::Options options_;
> > > > > > > > > std::unique_ptr<CameraManager> cm_;
> > > > > > > > > - std::shared_ptr<Camera> camera_;
> > > > > > > > > + std::vector<std::shared_ptr<Camera>> cameras_;
> > > > > > > > > };
> > > > > > > > >
> > > > > > > > > Harness::Harness(const OptionsParser::Options &options)
> > > > > > > > > @@ -46,33 +48,14 @@ Harness::Harness(const OptionsParser::Options &options)
> > > > > > > > >
> > > > > > > > > Harness::~Harness()
> > > > > > > > > {
> > > > > > > > > - if (camera_) {
> > > > > > > > > - camera_->release();
> > > > > > > > > - camera_.reset();
> > > > > > > > > + for (auto &c : cameras_) {
> > > > > > > > > + c->release();
> > > > > > > > > + c.reset();
> > > > > > > > > }
> > > > > > > > >
> > > > > > > > > cm_->stop();
> > > > > > > > > }
> > > > > > > > >
> > > > > > > > > -int Harness::exec()
> > > > > > > > > -{
> > > > > > > > > - int ret = init();
> > > > > > > > > - if (ret)
> > > > > > > > > - return ret;
> > > > > > > > > -
> > > > > > > > > - std::vector<Results> results;
> > > > > > > > > -
> > > > > > > > > - results.push_back(testSingleStream(camera_));
> > > > > > > > > -
> > > > > > > > > - for (const Results &result : results) {
> > > > > > > > > - ret = result.summary();
> > > > > > > > > - if (ret)
> > > > > > > > > - return ret;
> > > > > > > > > - }
> > > > > > > > > -
> > > > > > > > > - return 0;
> > > > > > > > > -}
> > > > > > > > > -
> > > > > > > > > int Harness::init()
> > > > > > > > > {
> > > > > > > > > int ret = cm_->start();
> > > > > > > > > @@ -82,42 +65,26 @@ int Harness::init()
> > > > > > > > > return ret;
> > > > > > > > > }
> > > > > > > > >
> > > > > > > > > - if (!options_.isSet(OptCamera)) {
> > > > > > > > > - std::cout << "No camera specified, available cameras:" << std::endl;
> > > > > > > > > - listCameras();
> > > > > > > > > - return -ENODEV;
> > > > > > > > > - }
> > > > > > > > > -
> > > > > > > > > - const std::string &cameraId = options_[OptCamera];
> > > > > > > > > - camera_ = cm_->get(cameraId);
> > > > > > > > > - if (!camera_) {
> > > > > > > > > - std::cout << "Camera " << cameraId << " not found, available cameras:" << std::endl;
> > > > > > > > > - listCameras();
> > > > > > > > > - return -ENODEV;
> > > > > > > > > + for (auto cam : cm_->cameras()) {
> > > > > > > > > + if (cam->acquire()) {
> > > > > > > > > + std::cout << "Failed to acquire camera" << std::endl;
> > > > > > > > > + return -EINVAL;
> > > > > > > > > + }
> > > > > > > > > + cameras_.push_back(cam);
> > > > > > > >
> > > > > > > > I don't like this, I think we need to retain the ability to select which
> > > > > > > > camera to test. And further down the line maybe allow more then one
> > > > > > > > camera to be selected if we want to test concurrent use of two specific
> > > > > > > > ones. But for the work in this series I think you can ignore multiple
> > > > > > > > cameras.
> > > > > > >
> > > > > > > Actually, that doesn't mean that multiple cameras will be tested, that's
> > > > > > > determined by the test filter, so doing --gtest_filter='.*idCamera1.*' would
> > > > > > > only test that camera. But I think I see what you mean, I shouldn't be acquiring
> > > > > > > all cameras, only the ones that I will actually test (and in this code there's a
> > > > > > > big issue where if a single camera is already busy, even if it won't be tested,
> > > > > > > lc-compliance as a whole fails).
> > > > > >
> > > > > > Ahh I understand.
> > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > If I understand the coverletter correctly the correct thing here the cli
> > > > > > > > arguments should be feed to gtest directly to be able to filter on what
> > > > > > > > tests are run. Is there support in getst to also pass arguments to
> > > > > > > > tests? Maybe we could remove the need to parse arguments ourself
> > > > > > > > completely?
> > > > > > >
> > > > > > > There isn't any CLI argument in gtest to pass arguments to tests, but I think we
> > > > > > > could solve both problems (this and the one above) like this:
> > > > > > >
> > > > > > > - In the main(), query the id of each camera from libcamera.
> > > > > > > - Register each of the tests dynamically in gtest passing as an argument the
> > > > > > > camera id, rather than a camera pointer (which is the current behavior).
> > > > > > > - To select which tests to run, and on which camera(s), the user just uses
> > > > > > > --gtest_filter='testToRun.*idCamera' (or '.*idCamera.*' to run all tests for
> > > > > > > the camera).
> > > > > > > - In the startup of each test the camera is acquired for that test, and when
> > > > > > > it's done it is released.
> > > > > > >
> > > > > > > Implications:
> > > > > > > - We no longer need to parse arguments and can drop that code, as both camera
> > > > > > > and test are specified directly with gtest's filter.
> > > > > > > - We only acquire the cameras that we'll actually test and only while they're
> > > > > > > being tested.
> > > > > > > - As a downside, a single camera will probably be acquired multiple times during
> > > > > > > a single run as lc-compliance moves from one test to the next, but I don't
> > > > > > > think this would be a big issue. (Maybe we could do this at the test suite
> > > > > > > level instead to greatly decrease the number of times of acquire(), but I'm
> > > > > > > not sure if it's possible, since we instantiate, by passing the camera id, at
> > > > > > > the test level, I'll have to test that).
> > > > > > >
> > > > > > > What do you think?
> > > > > >
> > > > > > I understand what you are trying to do but I see two issues.
> > > > > >
> > > > > > 1. It depends on the implicit knowledge of users how to build the filter
> > > > > > string and is rather unintuitive compared to having two distinct
> > > > > > arguments, one to select camera and one to express the test filter.
> > > > > >
> > > > > > This could possibly be worked around by keeping the cam options code
> > > > > > and building the gtest filter string internally by injecitng the
> > > > > > selected camera name in the correct location, but the we have the
> > > > > > next issue,
> > > > > >
> > > > > > 2. I don't think it will be practically possible to express a test
> > > > > > filter that selects a specific combination of two cameras.
> > > > > >
> > > > > > I do think we need to be able to parameterize test cases. If gtest does
> > > > > > not allow to do so using function arguments there are other ways of
> > > > > > doing so.
> > > > >
> > > > > I think I didn't express myself well. I meant to say that gtest's CLI doesn't
> > > > > have a flag to allow passing parameters to tests, meaning that it would not be
> > > > > possible to parametrize the tests from the command line and also remove our
> > > > > own parser and rely on gtest's.
> > > >
> > > > Check, I think we need to keep our own parser for now as we need to be
> > > > able to parametrize the tests from the CLI.
> > > >
> > > > > But gtest does allow test parametrization at runtime and I'm already relying on
> > > > > it since v2. Let me go a bit more over the way I'm doing this.
> > > > >
> > > > > - I have a test defined, eg BalancedSingleCycle, which takes some parameters:
> > > > >
> > > > > std::shared_ptr<Camera> camera, StreamRole role, unsigned int numRequests
> > > > >
> > > > > - Since we may want to run the same test for all cameras, roles and numRequests,
> > > > > I register it, at runtime, using my REGISTER_TESTS() macro, which calls
> > > > > gtest's testing::RegisterTest() with all possible combinations of parameters.
> > > > >
> > > > > - Based on the --gtest_filter CLI argument from gtest, it decides which tests
> > > > > are actually run
> > > > >
> > > > > So the key point is that even though I parametrize the tests, I do it for all
> > > > > combinations in order to later rely on gtest_filter to select which tests to
> > > > > run.
> > > >
> > > > I see, that explains how the parameters get filled. I noticed it but did
> > > > not dig further, see below for an expansion of my thoughts now that I
> > > > understand this detail :-)
> > > >
> > > > >
> > > > > Now, I do agree with both of your points. And if we want to make the interface
> > > > > more intuitive to use (which I also find important), there's no way around
> > > > > having our own parser.
> > > > >
> > > > > And if we're using our own camera flag, then it makes no sense to register tests
> > > > > for all cameras, we can register tests only with the specified camera.
> > > >
> > > > This feels wrong. It feels like tests should be register without a
> > > > camera component. The camera (and possible other arguments) should come
> > > > from either arguments [1] or a singleton parameter class [2].
> > > >
> > > > 1. This do not seem to be possible with gtests as test parameters needs
> > > > to be registerd.
> > > >
> > > > 2. This would be similar to the global variable used in earlier versions
> > > > but instead modeled as a class singleton like CameraManager.
> > > >
> > > > Maybe there are other options tho.
> > > >
> > > > >
> > > > > I think here it would be helpful to think if the interface suits our problem.
> > > > > To me we're looking at having:
> > > > >
> > > > > lc-compliance --camera 'cameraId' --filter 'BalancedSingleCycle'
> > > >
> > > > This matches my view as well.
> > > >
> > > > >
> > > > > to only test camera 'cameraId' with a single test 'BalancedSingleCycle'. Here,
> > > > > the camera flag is used to only register tests with that camera, and the filter
> > > > > is passed as --gtest_filter, possibly a bit adapted, to gtest.
> > > > >
> > > > > And if we imagine we have a test ConcurrentCameras, which receives two cameras
> > > > > as parameters, and we call
> > > > >
> > > > > lc-compliance --camera 'cameraId1,cameraId2'
> > > >
> > > > This also looks good. But for now lets focus on one camera, I think it
> > > > will give us enough headache :-)
> > > >
> > > > >
> > > > > it would test both each of the cameras separately in all current tests which
> > > > > accept a single camera, like BalancedSingleCycle, as well as test both at the
> > > > > same time in the ConcurrentCameras test (and we could make only the former or
> > > > > the latter occur by using the filter parameter).
> > > >
> > > > I have not thought enough about this to know if that is a good idea or
> > > > not. At the top of my head I still tend to think that lc-compliance
> > > > tests a single 'primary' camera and any additional camera supplied are
> > > > only used for tests where two (or more) cameras are needed to test some
> > > > specific feature of the primary camera.
> > > >
> > > > That is only the 'primary' camera is testes, the other camera(s) are
> > > > just there to help/provoke the testing. But maybe I'm to narrow minded.
> > >
> > > Okay, so it now comes down to how lc-compliance will get used. Either:
> > >
> > > 1. lc-compliance will be called to test a single camera, with optionally other
> > > secondary cameras
> > >
> > > 2. lc-compliance will be called to test multiple cameras at once (not
> > > necessarily simultaneously, but in a single run), with the default case being
> > > "Test all the cameras in this system".
> > >
> > > Given that the final objective of my internship is to have lc-compliance running
> > > on real boards by KernelCI, to catch any kernel regressions that affect camera
> > > usage, I'm trying to think how it would work there. (I wonder if this discussion
> > > should be happening together with KernelCI to decide on this...)
> > >
> > > So I'm imagining a RockPi4 board with a camera sensor plugged in, running
> > > lc-compliance to catch any kernel regressions.
> > >
> > > The limitation I see in 1 is that if a system has multiple cameras and want to
> > > ensure that all keep working, it will need to either hardcode all camera ids and
> > > manually call lc-compliance once for each one, or have a separate program that
> > > lists all cameras from libcamera (using 'cam -l' for example) and calls
> > > lc-compliance with each one. That's to say, running on all cameras is not as
> > > easy. And then there are multiple reports, one for each camera, instead of a
> > > single one for all cameras, which might be good or bad, not sure.
> > >
> > > That said, since the RockPi4 (and also the Raspberry Pi 4) has a single CSI-2
> > > connector, I believe it only supports a single camera anyway, so this shouldn't
> > > be a problem for this board, but I'm not sure about others. Phones for example
> > > seem to have at least two independent cameras...
> >
> > I now understand your point of view better, thanks for brining up the
> > KernelCI angle. I agree from a KernelCI perspective the goal is likely
> > to test all cameras of the system to get the widest test base. But from
> > a (current) libcamera perspective the lc-complinance tool primary usage
> > is to be able to test a particular camera (or pipeline) to prove changes
> > do not break things, kind of like v4l2-complinace is for V4L2.
> >
> > That being said I think it's trivial for us to make lc-complinance
> > satisfy both use-cases. If the arg parsing is structured something like
> > this,
> >
> > if (cliargs.continas("--camera")) {
> > run_tests_for_camera(cliargs.value_for("--camera");
> > } else if (cliargs.continas("--run-all-cameras")) {
> > for (camera : camer_manager.list_all_cameras()) {
> > run_tests_for_camera(camera)
> > }
> > }
> >
> > Or am I missing something?
>
> Unfortunately I don't think that would work. All tests would run fine, but the
> issue is in the results reporting at the end. Instead of having a single report,
> with the right number of tests failed, skipped or succeeded and details for each
> one, there would be N consecutive reports, where N is the number of cameras,
> each one reporting about its own camera. From a user perspective that would be
> very confusing, since you'd need to scroll through each of the reports to check
> the result for each camera.
Good point, for some reason I thought we could aggregate each test run
and create a lc-compliance report at the end. But as you point out maybe
it's best to keep a one camera test view.
>
> Another issue is that for KernelCI we need machine-parsable results, which we
> can do with --gtest_output=json to write the result in the JSON format into a
> file, but then I bet each camera's result would overwrite the previous one
> (haven't tried it yet, though).
>
> (Although thinking a bit more about it now, we could take advantage of the fact
> that --gtest_output=(json|xml)[:DIRECTORY_PATH/|:FILE_PATH] allows to set the
> file name to make a sure each camera result is written to a different file, but
> this still doesn't solve the first case with the text output to the terminal for
> the user)
For KernelCI this could be the way forward. A worst case maybe the
output files from multiple lc-compliance runs can be aggregated into a
single report file and then feed back to KernelCI?
>
> So I don't see any way to have lc-compliance work with multiple cameras and have
> a single complete results report without relying on dynamic registration of
> tests for each camera.
>
> In any case, given your answer in the second email that we could indeed use the
> singleton pattern and have the camera globally available, meaning we could go
> back to static registration of tests, I think it's worth it to have
> lc-compliance focused on a single camera (with optional secondary ones) and push
> the burden of how to test all cameras to KernelCI (which I'll probably be the
> one to solve anyway :P) in exchange for a lot easier test registration (and
> getting rid of that REGISTER_TESTS macro I wrote that I hated from the start :)
> ).
>
> So assuming you're okay with that, I'll get working on it for v3.
I think this sounds like a good way forward! Thanks again for working on
this, I think switching to gtest really is a good step in the right
direction.
>
> Thanks,
> Nícolas
>
> >
> > >
> > > Regarding the benefit of 1, I'm not sure. If we were to use a global variable
> > > like in v1, it would make the code a lot simpler by making the test registration
> > > static. But that doesn't seem to be an option :). I'm not familiar with using a
> > > Singleton, so I'll take a look at CameraManager. But I assume it wouldn't be
> > > accessible statically, so the dynamic test registration complexities would have
> > > to stay.
> > >
> > > Also, just to make sure we're on the same page, by using a Singleton, and again
> > > assuming it isn't stored in a global variable to be accessible from the tests
> > > statically, even though we would eliminate the camera as a parameter for the
> > > tests, the reference to the singleton itself would need to be added as a
> > > parameter for the tests.
> >
> > I will follow up on this in your second mail.
> >
> > >
> > > Thanks,
> > > Nícolas
> > >
> > > >
> > > > >
> > > > > I know we don't need to implement support for multiple cameras right now, I just
> > > > > want to make sure it's possible and thinking about the interface and the
> > > > > scenarios help me picture how they'd work.
> > > > >
> > > > > Thoughts?
> > > > >
> > > > > Thanks,
> > > > > Nícolas
> > > > >
> > > > > >
> > > > > > I'm sure there are many solutions to this problem and maybe someone more
> > > > > > familiar with gtest knows of a neat way to piggyback on its
> > > > > > infrastrcuture? Since I know very little about the gtest framework the
> > > > > > following is just an idea and I'm happy to yield.
> > > > > >
> > > > > > How about using a singleton class that carries arguments constructed
> > > > > > from cli arguments (camera id) before the gtest are executed. Each test
> > > > > > case could then consult this singleton if it needs to access parameters.
> > > > > > I think you could take inspiration from the libcamera CameraManager
> > > > > > class.
> > > > > >
> > > > > > This could (at lest for now) imply keeping the cam option parser. If so
> > > > > > I think it would be nice if somewhen before we integrate this work that
> > > > > > the gtest filter string could be supplied as an argument using that
> > > > > > parser. Of course if we can do it the other way around and add arguments
> > > > > > to the gtest parser that would be even cooler as it would be less code
> > > > > > for us to maintain ;-)
> >
> > --
> > Regards,
> > Niklas Söderlund
> >
> > --
> > To unsubscribe, send mail to kernel-unsubscribe at lists.collabora.co.uk.
>
>
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list