[libcamera-devel] [RFC PATCH 2/2] lc-compliance: Refactor using Googletest

[Niklas Söderlund]

Hello Nícolas,

Thanks for your efforts!

On 2021-04-30 10:19:03 -0300, Nícolas F. R. A. Prado wrote:
> Hi Niklas,
> 
> thanks for the feedback.
> 
> Em 2021-04-29 08:48, Niklas Söderlund escreveu:
> 
> > Hello Nícolas,
> >
> > Thanks for your work.
> >
> > On 2021-04-22 18:06:09 -0300, Nícolas F. R. A. Prado wrote:
> > > Refactor lc-compliance using Googletest as the test framework.
> >
> > I really like the gtest rework from a first reading of this patch.
> > Unfortunately it no longer applies to master so I have not been able to
> > take it for a test drive.
> >
> > I like that we get most if not all features we discussed such as skip
> > and test suite/case selection form the CLI more or less out-for-the box
> > for free.
> >
> > Some random comments below.
> >
> > > 
> > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado at collabora.com>
> > > ---
> > >  src/lc-compliance/main.cpp           |  60 +++++------
> > >  src/lc-compliance/meson.build        |   3 +
> > >  src/lc-compliance/simple_capture.cpp |  73 +++++---------
> > >  src/lc-compliance/simple_capture.h   |   8 +-
> > >  src/lc-compliance/single_stream.cpp  | 142 ++++++++++++++-------------
> > >  5 files changed, 132 insertions(+), 154 deletions(-)
> > > 
> > > diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp
> > > index 54cee54aa978..04cd0732ee3d 100644
> > > --- a/src/lc-compliance/main.cpp
> > > +++ b/src/lc-compliance/main.cpp
> > > @@ -14,8 +14,12 @@
> > >  #include "../cam/options.h"
> > >  #include "tests.h"
> > >  
> > > +#include "gtest/gtest.h"
> >
> > As gtest is pulled in as an dependably and not added to the libcamera
> > tee should this not be <gtest/gtest.h> ?
> 
> It should indeed. Fixed for v2.
> 
> >
> > > +
> > >  using namespace libcamera;
> > >  
> > > +std::shared_ptr<Camera> cam;
> > > +
> >
> > Would it be possible to factor the code so that the Camera instance is
> > not a global variable? I'm thinking that this will not scale when tests
> > requiring more then one camera are needed.
> 
> So... This is something that annoyed me as well. The short answer is I think it
> could, but the test registration code wouldn't be as clean.

IMHO almost any mess that removes a global variable is worth it :-)

> 
> The not so short answer: The only way I think this could be done is by adding a
> parameter to every test case with the camera instance. But I'm currently
> registering the test suites statically:
> 
> INSTANTIATE_TEST_SUITE_P(Raw,
>                          FixedRequestsTest,
>                          testing::Combine(testing::Values(Raw),
> 					testing::ValuesIn(NUMREQUESTS)));
> 
> So in order to pass the camera instance, they would need to be registered
> dynamically, as the example in [1] shows, after the camera to use is parsed from
> the command line.
> 
> [1] https://google.github.io/googletest/advanced.html#registering-tests-programmatically
> 
> But indeed, this seems to be needed to support working with multiple cameras.
> The other solutions I was thinking about: making the global variable a vector of
> cameras and/or running gtest multiple times and each time with the camera
> variable set differently, wouldn't give a single report at the end with the
> results from all cameras which would make it harder to parse the results.
> 
> I'm not sure which tests would need multiple cameras (if you think of examples
> let me know), but at least being able to run each of the tests for each of the
> cameras on a system in a single lc-compliance run would be important I think.

It's a tricky question and its implementation may be even tricker. The 
first 'problem' I have in mind for this type of test are pipelines where 
some part of the capture pipeline are shared between two discrete 
libcamera cameras.

If we look closer at the RkISP1 pipeline handler it has a activeCamera_ 
variable that designates witch camera is currently using the shared ISP.  
It would be cool if lc-compliance had a test case that somehow could 
figure out (either thru the API or informed using cli arguments) which 
cameras share resources and test that they error our correctly when 
tried to be used at the same time.

> 
> So I'll take a stab at it and see how things go.
> 
> Regarding the other issues, I'll have them fixed for v2.

Nice! Remember the on-top definition. As long as your work only uses a 
'previous bad idea' and not introduces a new one, it's fine to fix 
things on-top :-)

> 
> Thanks,
> Nícolas

-- 
Regards,
Niklas Söderlund