[PATCH v2 2/2] libipa: camera_sensor_helper: Fix rounding of gainCode
Kieran Bingham
kieran.bingham at ideasonboard.com
Wed May 29 00:56:08 CEST 2024
Hi David,
Quoting David Plowman (2024-05-28 16:51:52)
> 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.
Ack,
> * Converting the gain code back to the gain needs to give you true gain value.
Definite ack. I haven't made changes to those calculations and presently
I don't believe there are any precision faults there. It's only
converting from gain to code as far as I can see.
> So long as I have those, I don't really care.
So - I worked through (harder this time) to really see what's going on -
and with some help from chatGPT (I'm still surprised at how useful that
can really be) I've learnt how to use octave, a linux matlab equivalent
which so far seems quite useful.
The net result is - I can see your concern. Particularly given your
first point above.
The issue I'm trying to solve can be visualised here:
- https://git.uk.ideasonboard.com/kbingham/octave-gains/src/commit/ffe0ae5b2a77d2f36bfd7a9642b9ed89c977fe3c/linear_gain_model_plot.png
- https://git.uk.ideasonboard.com/kbingham/octave-gains/src/commit/ffe0ae5b2a77d2f36bfd7a9642b9ed89c977fe3c/logarithmic_gain_model_plot.png
Where those 'blips' rounding down to the next lowest code are what
concern me when I try to expose the gain model to application space.
And of course my implementation in this series /does/ violate your first
requirement as can be seen when I turn those graphs into a fine-grained
resolution of the gain->code:
- https://git.uk.ideasonboard.com/kbingham/octave-gains/src/commit/e275f529e436fc0f8b48028090ae6d421b9dffec/linear_gain_model_plot.png
- https://git.uk.ideasonboard.com/kbingham/octave-gains/src/commit/4dc39a8dd3906cb2e4077e3e7bc00cbd2756ee4f/logarithmic_gain_model_plot.png
where it can be seen that the red lines (gainCode(gain)) only ever round
*down* while the green lines (roundedCode(gain)) are visibly rounding to
the nearest which covers 50% of the interval rounding up, and 50% of the
interval rounding down.
Given your requirement number one I think we can call this a clear fail.
> 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
Indeed, I think I need to stop calling this a rounding problem, and call
it a floating point precision problem, as that's what I'm really facing.
I think I have incorrectly attempted to solve it as if it were an integer
rounding issue ...
Interestingly chatgpt also suggested adding a very small epsilon to
solve this. I'm not yet sure how to determine the what the smallest
possible valid epsilon would be for a given camera sensor helper though
and I don't trust chatgpt's suggestions here ... (back to Laurent's
"does this solve for every case" question).
A quick check shows that adding a small value (0.000001) does solve the
issue for me for 'a single test point so far'.
- https://git.uk.ideasonboard.com/kbingham/octave-gains/src/commit/176750e25639f5e0c521adf27c7264924a62cec7/linear_gain_model_plot.png
now shows the green gain+epsilon on the correct path, and the
fine-grained view:
- https://git.uk.ideasonboard.com/kbingham/octave-gains/src/commit/39fa3ebaa7fad95fc96ba6ac58dc576728cab4c5/linear_gain_model_plot.png
shows no more 'rounding up'...
> 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!
I've already got a user who /doesn't/ want to use the internal AGC. That
means they want to control and set the (exposure and) gains manually,
but right now - it's really difficult for them to determine what gain
will be set for a given ... gain. The only way libcamera lets that be
known is to set it and wait for the result to complete in the metadata.
And if that 'corrected' gain is then used to apply the gain ... it would
change! That's the bug I want to solve with this series.
I hope that explains the real-life need for it that I perceive. It could
also be solved perhaps in otherways by exposing the camera sensor
helpers to applications in some way so that they can directly perform
the calculations too ... but even then in the current form the 'downward
loop' would potentially still be there, so that's not really solving the
issue.
Back to the drawing board. Perhaps this one gets shelved until I can
figure out a way to determine/calculate a valid epsilon value for all
cases in the helpers ...
--
Kieran
> 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
> > > >>
More information about the libcamera-devel
mailing list