| Message ID | 20240223155954.4139705-4-kieran.bingham@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, Thank you for the patch. On Fri, Feb 23, 2024 at 03:59:54PM +0000, Kieran Bingham wrote: > The implementation of gainCode for both Exponential and Linear gain s/gainCode/gainCode()/ > 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 rounding errors. It's not due to rounding errors, but to the fact that we only round down, isn't it ? > Correct the rounding error and ensure that gainCode translation > produces accurate bi-directional conversions from the perspective > of the gainCode. Even rounding to the closest integer (which I think is a good idea), are you sure you'll guarantee that gainCode(gain(code)) will always be equal to code, in *all* cases ? > This fixes the IMX290, IMX296, IMX327 and IMX335 which use the > Exponential gain model helpers, as well as IMX219 IMX258 and IMX477 Missing commas. > which use the Linear gain model helpers. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/ipa/libipa/camera_sensor_helper.cpp | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp > index a925b37b9f7c..39e10d86a2c7 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); The code change seems fine, but the commit message may need to be reworked. > > default: > ASSERT(false);
Quoting Laurent Pinchart (2024-02-27 14:58:47) > Hi Kieran, > > Thank you for the patch. > > On Fri, Feb 23, 2024 at 03:59:54PM +0000, Kieran Bingham wrote: > > The implementation of gainCode for both Exponential and Linear gain > > s/gainCode/gainCode()/ > > > 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 rounding errors. > > It's not due to rounding errors, but to the fact that we only round > down, isn't it ? Isn't that a rounding error? An error when rounding (to integer)? ... :-) > > Correct the rounding error and ensure that gainCode translation > > produces accurate bi-directional conversions from the perspective > > of the gainCode. > > Even rounding to the closest integer (which I think is a good idea), are > you sure you'll guarantee that gainCode(gain(code)) will always be equal > to code, in *all* cases ? Run the test with and without the fix... I haven't done a formal proof here no. But that's also why the test ... tests every CameraSensorHelper in more or less brute force. > > This fixes the IMX290, IMX296, IMX327 and IMX335 which use the > > Exponential gain model helpers, as well as IMX219 IMX258 and IMX477 > > Missing commas. Ok. > > > which use the Linear gain model helpers. > > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > --- > > src/ipa/libipa/camera_sensor_helper.cpp | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp > > index a925b37b9f7c..39e10d86a2c7 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); > > The code change seems fine, but the commit message may need to be > reworked. > > > > > default: > > ASSERT(false); > > -- > Regards, > > Laurent Pinchart
diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp index a925b37b9f7c..39e10d86a2c7 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);
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 rounding errors. 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 and IMX477 which use the Linear gain model helpers. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- src/ipa/libipa/camera_sensor_helper.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)