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

[Kieran Bingham]

Quoting Laurent Pinchart (2024-02-27 15:05:51)
> Hi Kieran
> 
> On Mon, Feb 26, 2024 at 03:50:46PM +0100, Jacopo Mondi wrote:
> > 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.
> 
> Where does that value come from ? I don't see it in the code.

It's what the test outputs as the maximum value. All values after this
fail, and only 63 is returned for any value above this.


I expect it's this:

 
uint32_t CameraSensorHelperAr0521::gainCode(double gain) const
{
	gain = std::clamp(gain, 1.0, 15.5);
	...
}


> > > 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.
> 
> One option would be to retrieve the maximum by calling gainCode(very
> high value). Or that would be an option if the helpers actually
> implemented limits, which is currently not the case. If I recall
> correctly, we get the gain code limits dynamically from the kernel
> driver. Hardcoding limits in libcamera could be problematic.
> 
> How about creating subclasses of the camera sensor class in this test,
> with different models and coefficients ? That would also remove the
> requirement of exposing the factory names.

Do you mean only validate 3 instances? Instead of validating every
CameraSensorHelper?

I liked that this gives us the opportunity to validate every
CameraSensorHelper that gets added, especially as they can be
overridden.

I was imagining that there would be other camera specific overrides that
may warrant additional tests in the future but maybe that's too early to
identify, or the other parts that will be added will be too simple to
test, unlike these calculations.

--
Kieran

> > >  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 ?
> > 
> > > +                   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']},
> > >  ]
> 
> -- 
> Regards,
> 
> Laurent Pinchart