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

[Kieran Bingham]

Quoting Jacopo Mondi (2024-02-26 17:08:58)
> 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 ?

But what steps should we use on the gain? How do we validate a 1-1
mapping when we don't know the gain code?

The advantage of comparing a gain code to a gain and back again is that
it verifies the exactness of the conversion (which is what the patch for
3/3 corrects).


> 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..

Yes, and right now our helpers are ... wrong! Which this test
highlights.

Maybe should I concentrate on trying to merge the fix without the test?
It seems testing this seems to be controversial :-(

--
Kieran


> >
> > --
> > 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
> > > >