| Message ID | 20240524135256.649406-3-kieran.bingham@ideasonboard.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, On Fri, 24 May 2024, 2:53 pm Kieran Bingham, < kieran.bingham@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. 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@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 > >
On Fri, 24 May 2024 at 15:32, Naushir Patuck <naush@raspberrypi.com> wrote: > > Hi Kieran, > > On Fri, 24 May 2024, 2:53 pm Kieran Bingham, <kieran.bingham@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... Maybe it's a case of fixing the test? 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@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 >>
Quoting David Plowman (2024-05-24 16:00:31) > On Fri, 24 May 2024 at 15:32, Naushir Patuck <naush@raspberrypi.com> wrote: > > > > Hi Kieran, > > > > On Fri, 24 May 2024, 2:53 pm Kieran Bingham, <kieran.bingham@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@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 > >>
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@ideasonboard.com> wrote: > > Quoting David Plowman (2024-05-24 16:00:31) > > On Fri, 24 May 2024 at 15:32, Naushir Patuck <naush@raspberrypi.com> wrote: > > > > > > Hi Kieran, > > > > > > On Fri, 24 May 2024, 2:53 pm Kieran Bingham, <kieran.bingham@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@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 > > >>
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@ideasonboard.com> wrote: > > > > Quoting David Plowman (2024-05-24 16:00:31) > > > On Fri, 24 May 2024 at 15:32, Naushir Patuck <naush@raspberrypi.com> wrote: > > > > > > > > Hi Kieran, > > > > > > > > On Fri, 24 May 2024, 2:53 pm Kieran Bingham, <kieran.bingham@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@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 > > > >>
Hi Kieran Thanks for the thorough reply! On Tue, 28 May 2024 at 23:56, Kieran Bingham <kieran.bingham@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@ideasonboard.com> wrote: > > > > > > Quoting David Plowman (2024-05-24 16:00:31) > > > > On Fri, 24 May 2024 at 15:32, Naushir Patuck <naush@raspberrypi.com> wrote: > > > > > > > > > > Hi Kieran, > > > > > > > > > > On Fri, 24 May 2024, 2:53 pm Kieran Bingham, <kieran.bingham@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@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 > > > > >>
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@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@ideasonboard.com> wrote: > > > > > > > > Quoting David Plowman (2024-05-24 16:00:31) > > > > > On Fri, 24 May 2024 at 15:32, Naushir Patuck <naush@raspberrypi.com> wrote: > > > > > > > > > > > > Hi Kieran, > > > > > > > > > > > > On Fri, 24 May 2024, 2:53 pm Kieran Bingham, <kieran.bingham@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@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 > > > > > >>
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']}, ]
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. 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@ideasonboard.com> --- src/ipa/libipa/camera_sensor_helper.cpp | 6 +++--- test/ipa/meson.build | 3 +-- 2 files changed, 4 insertions(+), 5 deletions(-)