[PATCH v2 2/2] libipa: camera_sensor_helper: Fix rounding of gainCode

[David Plowman]

Hi Kieran

So I have only two requirements here:

* The gain "rounds down", so that the gain code always gives you
exactly the gain you asked for, or less.

* Converting the gain code back to the gain needs to give you true gain value.

So long as I have those, I don't really care.

Slightly longer answer. The above all happens to within some
precision, of course. So if folks wanted to go round adding (for
example) 1e-6 to gains just to avoid the rounding down "problem", then
I don't really mind because I'd never notice. But I'm still in the
"why bother to do that?" camp because it's a bit ugly, annoying to
explain, and I see no real-life need for it!

David

On Mon, 27 May 2024 at 13:46, Kieran Bingham
<kieran.bingham at ideasonboard.com> wrote:
>
> 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
> > >>