[PATCH v2 2/2] libipa: camera_sensor_helper: Fix rounding of gainCode
Kieran Bingham
kieran.bingham at ideasonboard.com
Wed May 29 16:32:39 CEST 2024
Quoting David Plowman (2024-05-29 09:41:18)
> Hi Kieran
>
> Thanks for the thorough reply!
>
> On Tue, 28 May 2024 at 23:56, Kieran Bingham
> <kieran.bingham at ideasonboard.com> wrote:
> >
> > 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.
>
> Yes, this is the "problem", I think. My view is "don't do this". There
> are floating point precision issues, you're not guaranteed to get the
> same gain code back.
>
> But as I said, adding 1e-6 (or whatever) is not unmanageable if folks
> prefer that behaviour. I think I'd probably rather add this when
> converting the gain to a code, rather than converting the code to a
> (slightly) larger gain, but I'm not sure I feel strongly. You need an
Yes, I think going that was is the correct behaviour too in what I can
see.
> "epsilon" value that doesn't cause you ever to jump up to the next
> code, but I don't think that's a problem in practice. Probably I'd
> make the helper class add the epsilon value transparently rather than
> requiring the application to do it, then the helper could always
> choose its own epsilon value, but am open to persuasion otherwise.
Oh indeed, I don't think I need to pursuade you otherwise - I would
expect the same. I don't think applciations should know about the
epsilon. It's just about how the gain value gets quantised to a gain
code in a (more) predictable and reversable way in my opinion,
> Ironically the legacy firmware based camera stack on the Pi had
> exactly this problem too, but we made sure to avoid those mistakes in
> our libcamera work, and not rely on those conversion "round-trips".
I agree, the round trips are to be avoided. Some how I think I have to
figure out an API extension that would let applications know what 'will'
happen for a given value though, but I have /no idea/ how that could fit
yet.
> David
Thank you for the feedback! I'll make sure we don't break your
requirements here whatever happens.
--
Kieran
>
> >
> > 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