[RFC PATCH 2/3] test: ipa: libipa: Add CameraSensorHelper Gain Model tests

[Jacopo Mondi]

Hi Kieran

On Mon, Feb 26, 2024 at 04:54:27PM +0000, Kieran Bingham wrote:
> Quoting Jacopo Mondi (2024-02-26 14:50:46)
> > Hi Kieran
> >
> > On Fri, Feb 23, 2024 at 03:59:53PM +0000, Kieran Bingham wrote:
> > > Introduce a test that validates conversion of the gain model
> > > in each CameraSensorHelper.
> > >
> > > It should be expected that if a gainCode produces a given gain
> > > value, then converting that gain back should produce the same
> > > result.
> > >
> > > This test fails on the following CameraSensorHelpers:
> > >  - imx219 imx258 imx283 imx290 imx296 imx327 imx335 imx477
> > >
> > > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > > ---
> > >
> > > This test also fails on the ar0521 helper as that has a defined code
> > > limit of '63', which the test does not yet have a way to infer.
> > >
> > > Adding a 'maxCode' helper to the base class may be the way to go here,
> > > and would also let us define a maximum value for other helpers directly.
> > >
> > >  test/ipa/camera_sensor_helper.cpp | 69 +++++++++++++++++++++++++++++++
> > >  test/ipa/meson.build              |  1 +
> > >  2 files changed, 70 insertions(+)
> > >  create mode 100644 test/ipa/camera_sensor_helper.cpp
> > >
> > > diff --git a/test/ipa/camera_sensor_helper.cpp b/test/ipa/camera_sensor_helper.cpp
> > > new file mode 100644
> > > index 000000000000..2d37628f87c4
> > > --- /dev/null
> > > +++ b/test/ipa/camera_sensor_helper.cpp
> > > @@ -0,0 +1,69 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > +/*
> > > + * Copyright (C) 2024, Ideas on Board Oy.
> > > + */
> > > +
> > > +#include "libipa/camera_sensor_helper.h"
> > > +
> > > +#include <iostream>
> > > +#include <string.h>
> > > +
> > > +#include "test.h"
> > > +
> > > +using namespace std;
> > > +using namespace libcamera;
> > > +using namespace libcamera::ipa;
> > > +
> > > +class CameraSensorHelperTest : public Test
> > > +{
> > > +protected:
> > > +     int testGainModel(std::string model)
> > > +     {
> > > +             int ret = TestPass;
> > > +
> > > +             std::unique_ptr<CameraSensorHelper> camHelper_;
> > > +
> > > +             camHelper_ = CameraSensorHelperFactoryBase::create(model);
> > > +             if (!camHelper_) {
> > > +                     std::cout
> > > +                             << "Failed to create camera sensor helper for "
> > > +                             << model;
> > > +                     return TestFail;
> > > +             }
> > > +
> > > +             for (unsigned int i = 0; i < 240; i++) {
> >
> > How did you establish 0 and 240 as the min/max ? As you suggested
> > above, isn't it better if the helper advertise these ?

This also assume the gain codes range is continuous, but we anyway
assume that in our helpers..

>
> Because that's what value the IMX283 uses. Yes - as I said above, and
> why this is an RFC, we don't have a way to get the max value from here.
> But getting it is not so clear. Should we define the max code for every
> camera sensor helper? Should it be defaulted to something supplied from
> the driver? (How?) or are we then just duplicating and hardcoding values
> from v4l2 drivers here in the helpers... maybe that's fine ?

Just to throw more options in the mix, we also have the min/max v4l2
control values. Not accessible by this test though...

Duplicating those values in the camera sensor helpers might require we
are really really careful and keep in sync the kernel driver and the
helper, but we already have many things to keep synchronized, so
that's maybe not that bad ?

Anyway, you here use the gain codes and from there you get to the gain
value and then back to the code. Isn't it better to do the other way
around and test in a 'safe' 1x-16x range ?

Using gain code is 'risky' different sensors might express gains with
different Q formats, with non-continuous ranges etc.. True, we're only
testing the helpers without going to the device here..

>
> --
> Kieran
>
>
> >
> > > +                     float gain = camHelper_->gain(i);
> > > +                     uint32_t gainCode = camHelper_->gainCode(gain);
> > > +
> > > +                     if (i != gainCode) {
> > > +                             std::cout << model << ": Gain conversions failed: "
> > > +                                       << i << " : " << gain << " : "
> > > +                                       << gainCode << std::endl;
> > > +
> > > +                             ret = TestFail;
> > > +                     }
> > > +             };
> > > +
> > > +             return ret;
> > > +     }
> > > +
> > > +     int run() override
> > > +     {
> > > +             unsigned int failures = 0;
> > > +
> > > +             std::vector<CameraSensorHelperFactoryBase *> factories;
> > > +
> > > +             for (auto factory : CameraSensorHelperFactoryBase::factories()) {
> > > +                     const std::string &model = factory->name();
> > > +
> > > +                     cout << "Testing CameraSensorHelper for " << model << endl;
> > > +
> > > +                     if (testGainModel(factory->name()) == TestFail)
> > > +                             failures++;
> > > +             }
> > > +
> > > +             return failures ? TestFail : TestPass;
> > > +     }
> > > +};
> > > +
> > > +TEST_REGISTER(CameraSensorHelperTest)
> > > diff --git a/test/ipa/meson.build b/test/ipa/meson.build
> > > index 180b0da0a51a..c99385a7cb8b 100644
> > > --- a/test/ipa/meson.build
> > > +++ b/test/ipa/meson.build
> > > @@ -1,6 +1,7 @@
> > >  # SPDX-License-Identifier: CC0-1.0
> > >
> > >  ipa_test = [
> > > +    {'name': 'camera_sensor_helper', 'sources': ['camera_sensor_helper.cpp']},
> > >      {'name': 'ipa_module_test', 'sources': ['ipa_module_test.cpp']},
> > >      {'name': 'ipa_interface_test', 'sources': ['ipa_interface_test.cpp']},
> > >  ]
> > > --
> > > 2.34.1
> > >