[PATCH v2 2/2] libipa: camera_sensor_helper: Fix rounding of gainCode
David Plowman
david.plowman at raspberrypi.com
Wed May 29 10:41:18 CEST 2024
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
"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.
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".
David
>
> 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