[libcamera-devel] [RFC PATCH v2 3/3] lc-compliance: Refactor using Googletest
Nícolas F. R. A. Prado
nfraprado at collabora.com
Tue May 11 21:06:04 CEST 2021
Hello again,
Em 2021-05-11 14:17, Nícolas F. R. A. Prado escreveu:
> 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...
So, I just read about the Singleton pattern on Wikipedia and checked out the
CameraManager class (should've done it before replying :) ).
>From what I can tell, there's a big difference between the CameraManager class
and a Singleton. The CameraManager just makes sure that there's a single
instance of it by saving a reference to it in an internal static variable. A
Singleton apparently, in addition to doing that, makes its instatiation through
a public static function. This means that not only there's a single instance,
it's also available from the global scope.
So I wanted to ask you to clarify which one you meant. If you meant that we
should have a class like CameraManager, which just makes sure it's the single
instance, my previous comments below are still valid.
On the other hand, if you meant that we should a have singleton, which is
available globally, then it is basically the same thing as having a global
variable, only a bit more organized, and my comments below are no longer valid.
We could indeed go back to static test registration, since the camera instance
could be accessed from any test as something like
TestEnvironment::getInstance().camera()
making the alternative 1 have the benefit of making the test registration and
definition code simpler.
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.
>
> 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 ;-)
More information about the libcamera-devel
mailing list