[PATCH v2 2/2] libipa: camera_sensor_helper: Fix rounding of gainCode
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon May 27 14:46:25 CEST 2024
Quoting David Plowman (2024-05-24 16:00:31)
> On Fri, 24 May 2024 at 15:32, Naushir Patuck <naush at raspberrypi.com> wrote:
> >
> > Hi Kieran,
> >
> > On Fri, 24 May 2024, 2:53 pm Kieran Bingham, <kieran.bingham at ideasonboard.com> wrote:
> >>
> >> The implementation of gainCode for both Exponential and Linear gain
> >> models does not generate a gainCode that matches the result of the
> >> reverse operation.
> >>
> >> This can be seen by converting sequential gainCodes to a gain
> >> and converting that back to the gainCode. The values do not
> >> translate back due to only rounding down, rather than to the closest
> >> integer.
> >>
> >> Correct the rounding error and ensure that gainCode translation
> >> produces accurate bi-directional conversions from the perspective
> >> of the gainCode.
> >
> >
> > Just to cause a bit of controversy, some IPAs might prefer to round
> > down the gain code. This would allow a stable total exposure by
> > compensating with digital gain. If we round up, our total exposure
> > will be higher than what was requested, which is harder to
> > compensate for.
> >
> > However the visual differences will be extremely marginal.
>
> Good spot, Naush. Indeed, I would quite deeply dislike anything that
> gives me more gain than I asked for! Less is ok, because you can
> compensate somewhere else. I know we aren't using any of these
> helpers, but no sense in doing anything that would make adopting them
> harder...
Well, I don't mind the controversy at all! and indeed I'm pleased you're
keeping an eye on them too.
My concern was that presently we were not producing consistent reverse
calculations when we determine /what/ the gain was from a configured
gain code when we report that back to the IPA or users.
> Maybe it's a case of fixing the test?
Well, it's not so much the test as making sure everything is behaving
correctly.
Essentially I think it's important that we make sure we are correct in
how the real numbers get quantised through the gain models.
In particular, part of this is that I want to be able to make sure
applications can know what gains they can expect to actually be applied
if they set a free-form float value - because of course the path through
to a gain code will clamp, which we don't currently tell applications if
they set manual gains, except when they get the metadata back.
It's very interesting that you want/prefer only to clamp down though -
that's important context!
I'll go through a worked example of what caused me to send this series,
and lets see if we're both on the same side. If not - I'll adapt.
The current output of the test (when it fails) is here:
- https://paste.debian.net/1318267/
In particular, it generates lines like this:
(code) : gain(code) : gainCode(gain(code))
Gain conversions failed: 2 : 1.07152 : 1
Gain conversions failed: 5 : 1.1885 : 4
Gain conversions failed: 6 : 1.23027 : 5
Gain conversions failed: 7 : 1.2735 : 6
Gain conversions failed: 8 : 1.31826 : 7
Gain conversions failed: 9 : 1.36458 : 8
Gain conversions failed: 11 : 1.46218 : 10
Gain conversions failed: 14 : 1.62181 : 13
Gain conversions failed: 18 : 1.86209 : 17
Gain conversions failed: 20 : 1.99526 : 19
Gain conversions failed: 21 : 2.06538 : 20
Gain conversions failed: 24 : 2.29087 : 23
And an extracted set of the test into godbolt that you can experiment
with is here:
- https://godbolt.org/z/PrWWh7d8z
To take a worked example. At present - lets say an application sets a
manual gain of x 1.36458 using
class CameraSensorHelperExponential : public CameraSensorHelper
{
public:
CameraSensorHelperExponential()
{
gainType_ = AnalogueGainExponential;
gainConstants_.exp = { 1.0, expGainDb(0.3) };
}
};
Which is the same as in the IMX335.
Without fixing with this patch the following can occur:
// Worked example:
float manualGain = 1.36458;
int code;
std::cout << "Set Manual gain of " << manualGain << std::endl;
code = exponential.gainCode(manualGain);
std::cout << "Camera being set to code: " << code << std::endl; /* = 8 */
/* So now we apply code to the camera, and return back the metadata */
float appliedGain = exponential.gain(code); /* appliedGain = 1.31826 */
std::cout << "Metadata reports this gain applied to the sensor "
<< appliedGain << std::endl;
/* Now the application might set back the 'quantised' value to the camera again */
code = exponential.gainCode(appliedGain); /* code = 7 */
std::cout << "Camera being set to code: " << code << std::endl;
/* we've gone from code 8 to code 7 already... */
/* Lets see what our current gain is... */
appliedGain = exponential.gain(code); /* appliedGain = 1.2735 */
std::cout << "Metadata reports this gain applied to the sensor "
<< appliedGain << std::endl;
/* Now the application might set back the 'quantised' value to the camera again */
code = exponential.gainCode(appliedGain); /* code = 6 */
std::cout << "Camera being set to code: " << code << std::endl;
/* Now we're down to code 6 which will return a yet smaller gain */
which produces:
Set Manual gain of 1.36458
Camera being set to code: 8
Metadata reports this gain applied to the sensor 1.31826
Camera being set to code: 7
Metadata reports this gain applied to the sensor 1.2735
Camera being set to code: 6
In otherwords, without the user changing anything the
clamping/quantising is now reducing further than it should.
David, Does the above make sense? In your opinion - maybe should only
one direction be rounded? or would you expect/not expect a given gain
value to translate directly to the same gain code and vice-versa?
Ultimately - what I want/expect is that the value we report as metadata
/precisely/ matches the gain applied on the sensor. So I expect if we
set the value applied on the sensor for a given quantised value (based
around the precise code values used) - in both directions the conversion
should generate the same result:
i.e:
a = code(gainCode(A);
assert(a==A);
b = gainCode(gain(B));
assert(b==B);
which is what the test checks for and what the fix corrects. Of course
the above assertions only hold true for values that match 1:1 with the
gaincodes which is why the test iterates based on those rather than
validating all float values.
Of course I could have easily made an error anywhere above! Please let
me know if you spot anything.
--
Kieran
> David
>
> >
> > Naush
> >
> >
> >
> >>
> >> This fixes the IMX290, IMX296, IMX327 and IMX335 which use the
> >> Exponential gain model helpers, as well as IMX219, IMX258, IMX283, and
> >> IMX477 which use the Linear gain model helpers.
> >>
> >>
> >> This updates the corresponding test which was previously marked as
> >> expected to fail.
> >>
> >> Fixes: 8ad9249fb09d ("libipa: camera_sensor_helper: Implement exponential gain model")
> >> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >> ---
> >> src/ipa/libipa/camera_sensor_helper.cpp | 6 +++---
> >> test/ipa/meson.build | 3 +--
> >> 2 files changed, 4 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
> >> index 2cd61fccfbb9..4a92ead88b23 100644
> >> --- a/src/ipa/libipa/camera_sensor_helper.cpp
> >> +++ b/src/ipa/libipa/camera_sensor_helper.cpp
> >> @@ -64,13 +64,13 @@ uint32_t CameraSensorHelper::gainCode(double gain) const
> >> case AnalogueGainLinear:
> >> ASSERT(k.linear.m0 == 0 || k.linear.m1 == 0);
> >>
> >> - return (k.linear.c0 - k.linear.c1 * gain) /
> >> - (k.linear.m1 * gain - k.linear.m0);
> >> + return std::round((k.linear.c0 - k.linear.c1 * gain) /
> >> + (k.linear.m1 * gain - k.linear.m0));
> >>
> >> case AnalogueGainExponential:
> >> ASSERT(k.exp.a != 0 && k.exp.m != 0);
> >>
> >> - return std::log2(gain / k.exp.a) / k.exp.m;
> >> + return std::round(std::log2(gain / k.exp.a) / k.exp.m);
> >>
> >> default:
> >> ASSERT(false);
> >> diff --git a/test/ipa/meson.build b/test/ipa/meson.build
> >> index abc2456fc341..0b22c7576e56 100644
> >> --- a/test/ipa/meson.build
> >> +++ b/test/ipa/meson.build
> >> @@ -1,8 +1,7 @@
> >> # SPDX-License-Identifier: CC0-1.0
> >>
> >> ipa_test = [
> >> - {'name': 'camera_sensor_helper', 'sources': ['camera_sensor_helper.cpp'],
> >> - 'should_fail': true},
> >> + {'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
> >>
More information about the libcamera-devel
mailing list