[RFC,2/3] test: ipa: libipa: Add CameraSensorHelper Gain Model tests
diff mbox series

Message ID 20240223155954.4139705-3-kieran.bingham@ideasonboard.com
State Superseded
Headers show
Series
  • libipa: Fix CameraSensorHelper gain helpers
Related show

Commit Message

Kieran Bingham Feb. 23, 2024, 3:59 p.m. UTC 
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@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

Comments

Jacopo Mondi Feb. 26, 2024, 2:50 p.m. UTC | #1 
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@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 ?

> +			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
>
Kieran Bingham Feb. 26, 2024, 4:54 p.m. UTC | #2 
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@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 ?

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 ?

--
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
> >
Jacopo Mondi Feb. 26, 2024, 5:08 p.m. UTC | #3 
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@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
> > >
Laurent Pinchart Feb. 27, 2024, 3:05 p.m. UTC | #4 
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@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']},
> >  ]
Kieran Bingham Feb. 27, 2024, 5:12 p.m. UTC | #5 
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@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
Kieran Bingham May 24, 2024, 12:53 p.m. UTC | #6 
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@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
> > > >
Kieran Bingham May 24, 2024, 12:56 p.m. UTC | #7 
Quoting Kieran Bingham (2024-02-27 17:12:32)
> 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@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.

I'll give it a quick go now.

It means we won't catch issues in future camera sensor helpers - but
that already requires extending the current camera sensor helpers with
repetetive data anyway so maybe this is sufficient for now.

--
Kieran

Patch
diff mbox series 

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++) {
+			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']},
 ]