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

[Laurent Pinchart]

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.

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

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