[libcamera-devel] [RFC PATCH v2 3/3] lc-compliance: Refactor using Googletest
Nícolas F. R. A. Prado
nfraprado at collabora.com
Wed May 12 15:43:35 CEST 2021
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.
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)
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.
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.
More information about the libcamera-devel
mailing list