Message ID | 20250815102945.1602071-4-stefan.klug@ideasonboard.com |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
Quoting Stefan Klug (2025-08-15 19:29:23) > Add a small utility function that calculates the quantized gain that > gets applied by a sensor when the gain code is set to gainCode(gain). > This is needed by algorithms to calculate a digital correction gain that > gets applied to mitigate the error introduce by quantization. > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > Reviewed-by: Isaac Scott <isaac.scott@ideasonboard.com> > Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com> > > --- > > Changes in v3: > - Remove virtual from quantizeGain() > - Improved documentation > - Collected tags > --- > src/ipa/libipa/camera_sensor_helper.cpp | 23 +++++++++++++++++++++++ > src/ipa/libipa/camera_sensor_helper.h | 1 + > 2 files changed, 24 insertions(+) > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp > index dcd69d9f2bbb..a490161faf4c 100644 > --- a/src/ipa/libipa/camera_sensor_helper.cpp > +++ b/src/ipa/libipa/camera_sensor_helper.cpp > @@ -131,6 +131,29 @@ double CameraSensorHelper::gain(uint32_t gainCode) const > } > } > > +/** > + * \brief Quantize the given gain value > + * \param[in] _gain The real gain > + * \param[out] quantizationGain The gain that is lost due to quantization > + * > + * This function returns the actual gain that is applied when the sensors gain s/sensors/sensor's/ > + * is set to gainCode(gain). s/(gain)/(_gain)/ I think might be clearer? > + * > + * It shall be guaranteed that gainCode(gain) == gainCode(quantizeGain(gain)). Same here. > + * > + * If \a quantizationGain is provided it is filled with the gain that must be Personally I'd prefer s/filled/populated/, since filling feels like there could be an initial value that you "fill" until you reach the target value. It's just a feeling though :) > + * applied to correct the losses due to quantization. s/applied to/applied to \a _gain/ and s/the/for the/ I think is slightly clearer. Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > + * > + * \return The quantized real gain > + */ > +double CameraSensorHelper::quantizeGain(double _gain, double *quantizationGain) const > +{ > + double g = gain(gainCode(_gain)); > + if (quantizationGain) > + *quantizationGain = _gain / g; > + return g; > +} > + > /** > * \struct CameraSensorHelper::AnalogueGainLinear > * \brief Analogue gain constants for the linear gain model > diff --git a/src/ipa/libipa/camera_sensor_helper.h b/src/ipa/libipa/camera_sensor_helper.h > index a9300a64f1e7..bd3d0beec77f 100644 > --- a/src/ipa/libipa/camera_sensor_helper.h > +++ b/src/ipa/libipa/camera_sensor_helper.h > @@ -29,6 +29,7 @@ public: > std::optional<int16_t> blackLevel() const { return blackLevel_; } > virtual uint32_t gainCode(double gain) const; > virtual double gain(uint32_t gainCode) const; > + double quantizeGain(double gain, double *quantizationGain) const; > > protected: > struct AnalogueGainLinear { > -- > 2.48.1 >
Hi Paul, Thank you for the review. Quoting Paul Elder (2025-09-05 12:43:02) > Quoting Stefan Klug (2025-08-15 19:29:23) > > Add a small utility function that calculates the quantized gain that > > gets applied by a sensor when the gain code is set to gainCode(gain). > > This is needed by algorithms to calculate a digital correction gain that > > gets applied to mitigate the error introduce by quantization. > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > Reviewed-by: Isaac Scott <isaac.scott@ideasonboard.com> > > Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com> > > > > --- > > > > Changes in v3: > > - Remove virtual from quantizeGain() > > - Improved documentation > > - Collected tags > > --- > > src/ipa/libipa/camera_sensor_helper.cpp | 23 +++++++++++++++++++++++ > > src/ipa/libipa/camera_sensor_helper.h | 1 + > > 2 files changed, 24 insertions(+) > > > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp > > index dcd69d9f2bbb..a490161faf4c 100644 > > --- a/src/ipa/libipa/camera_sensor_helper.cpp > > +++ b/src/ipa/libipa/camera_sensor_helper.cpp > > @@ -131,6 +131,29 @@ double CameraSensorHelper::gain(uint32_t gainCode) const > > } > > } > > > > +/** > > + * \brief Quantize the given gain value > > + * \param[in] _gain The real gain > > + * \param[out] quantizationGain The gain that is lost due to quantization > > + * > > + * This function returns the actual gain that is applied when the sensors gain > > s/sensors/sensor's/ > > > + * is set to gainCode(gain). > > s/(gain)/(_gain)/ I think might be clearer? > > > + * > > + * It shall be guaranteed that gainCode(gain) == gainCode(quantizeGain(gain)). > > Same here. > > > + * > > + * If \a quantizationGain is provided it is filled with the gain that must be > > Personally I'd prefer s/filled/populated/, since filling feels like there could > be an initial value that you "fill" until you reach the target value. It's just > a feeling though :) > > > + * applied to correct the losses due to quantization. > > s/applied to/applied to \a _gain/ and s/the/for the/ I think is slightly clearer. I applied the previous fixes. Here I think it is not exactly right as quantizationGain is not applied to _gain but to the result of this function. I went for "... the gain that must be applied on top to correct for the losses...". This is also not ideal, but I already have the next rewrite of this function in the pipeline so I guess it's ok for now. > > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> Thanks, Stefan > > > + * > > + * \return The quantized real gain > > + */ > > +double CameraSensorHelper::quantizeGain(double _gain, double *quantizationGain) const > > +{ > > + double g = gain(gainCode(_gain)); > > + if (quantizationGain) > > + *quantizationGain = _gain / g; > > + return g; > > +} > > + > > /** > > * \struct CameraSensorHelper::AnalogueGainLinear > > * \brief Analogue gain constants for the linear gain model > > diff --git a/src/ipa/libipa/camera_sensor_helper.h b/src/ipa/libipa/camera_sensor_helper.h > > index a9300a64f1e7..bd3d0beec77f 100644 > > --- a/src/ipa/libipa/camera_sensor_helper.h > > +++ b/src/ipa/libipa/camera_sensor_helper.h > > @@ -29,6 +29,7 @@ public: > > std::optional<int16_t> blackLevel() const { return blackLevel_; } > > virtual uint32_t gainCode(double gain) const; > > virtual double gain(uint32_t gainCode) const; > > + double quantizeGain(double gain, double *quantizationGain) const; > > > > protected: > > struct AnalogueGainLinear { > > -- > > 2.48.1 > >
Quoting Stefan Klug (2025-09-18 22:16:28) > Hi Paul, > > Thank you for the review. > > Quoting Paul Elder (2025-09-05 12:43:02) > > Quoting Stefan Klug (2025-08-15 19:29:23) > > > Add a small utility function that calculates the quantized gain that > > > gets applied by a sensor when the gain code is set to gainCode(gain). > > > This is needed by algorithms to calculate a digital correction gain that > > > gets applied to mitigate the error introduce by quantization. > > > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > > Reviewed-by: Isaac Scott <isaac.scott@ideasonboard.com> > > > Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com> > > > > > > --- > > > > > > Changes in v3: > > > - Remove virtual from quantizeGain() > > > - Improved documentation > > > - Collected tags > > > --- > > > src/ipa/libipa/camera_sensor_helper.cpp | 23 +++++++++++++++++++++++ > > > src/ipa/libipa/camera_sensor_helper.h | 1 + > > > 2 files changed, 24 insertions(+) > > > > > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp > > > index dcd69d9f2bbb..a490161faf4c 100644 > > > --- a/src/ipa/libipa/camera_sensor_helper.cpp > > > +++ b/src/ipa/libipa/camera_sensor_helper.cpp > > > @@ -131,6 +131,29 @@ double CameraSensorHelper::gain(uint32_t gainCode) const > > > } > > > } > > > > > > +/** > > > + * \brief Quantize the given gain value > > > + * \param[in] _gain The real gain > > > + * \param[out] quantizationGain The gain that is lost due to quantization > > > + * > > > + * This function returns the actual gain that is applied when the sensors gain > > > > s/sensors/sensor's/ > > > > > + * is set to gainCode(gain). > > > > s/(gain)/(_gain)/ I think might be clearer? > > > > > + * > > > + * It shall be guaranteed that gainCode(gain) == gainCode(quantizeGain(gain)). > > > > Same here. > > > > > + * > > > + * If \a quantizationGain is provided it is filled with the gain that must be > > > > Personally I'd prefer s/filled/populated/, since filling feels like there could > > be an initial value that you "fill" until you reach the target value. It's just > > a feeling though :) > > > > > + * applied to correct the losses due to quantization. > > > > s/applied to/applied to \a _gain/ and s/the/for the/ I think is slightly clearer. > > I applied the previous fixes. Here I think it is not exactly right as > quantizationGain is not applied to _gain but to the result of this > function. I went for "... the gain that must be applied on top to > correct for the losses...". This is also not ideal, but I already have Ok, that's good too, I'll take that. Paul > the next rewrite of this function in the pipeline so I guess it's ok for > now. > > > > > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > > Thanks, > Stefan > > > > > > + * > > > + * \return The quantized real gain > > > + */ > > > +double CameraSensorHelper::quantizeGain(double _gain, double *quantizationGain) const > > > +{ > > > + double g = gain(gainCode(_gain)); > > > + if (quantizationGain) > > > + *quantizationGain = _gain / g; > > > + return g; > > > +} > > > + > > > /** > > > * \struct CameraSensorHelper::AnalogueGainLinear > > > * \brief Analogue gain constants for the linear gain model > > > diff --git a/src/ipa/libipa/camera_sensor_helper.h b/src/ipa/libipa/camera_sensor_helper.h > > > index a9300a64f1e7..bd3d0beec77f 100644 > > > --- a/src/ipa/libipa/camera_sensor_helper.h > > > +++ b/src/ipa/libipa/camera_sensor_helper.h > > > @@ -29,6 +29,7 @@ public: > > > std::optional<int16_t> blackLevel() const { return blackLevel_; } > > > virtual uint32_t gainCode(double gain) const; > > > virtual double gain(uint32_t gainCode) const; > > > + double quantizeGain(double gain, double *quantizationGain) const; > > > > > > protected: > > > struct AnalogueGainLinear { > > > -- > > > 2.48.1 > > >
diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp index dcd69d9f2bbb..a490161faf4c 100644 --- a/src/ipa/libipa/camera_sensor_helper.cpp +++ b/src/ipa/libipa/camera_sensor_helper.cpp @@ -131,6 +131,29 @@ double CameraSensorHelper::gain(uint32_t gainCode) const } } +/** + * \brief Quantize the given gain value + * \param[in] _gain The real gain + * \param[out] quantizationGain The gain that is lost due to quantization + * + * This function returns the actual gain that is applied when the sensors gain + * is set to gainCode(gain). + * + * It shall be guaranteed that gainCode(gain) == gainCode(quantizeGain(gain)). + * + * If \a quantizationGain is provided it is filled with the gain that must be + * applied to correct the losses due to quantization. + * + * \return The quantized real gain + */ +double CameraSensorHelper::quantizeGain(double _gain, double *quantizationGain) const +{ + double g = gain(gainCode(_gain)); + if (quantizationGain) + *quantizationGain = _gain / g; + return g; +} + /** * \struct CameraSensorHelper::AnalogueGainLinear * \brief Analogue gain constants for the linear gain model diff --git a/src/ipa/libipa/camera_sensor_helper.h b/src/ipa/libipa/camera_sensor_helper.h index a9300a64f1e7..bd3d0beec77f 100644 --- a/src/ipa/libipa/camera_sensor_helper.h +++ b/src/ipa/libipa/camera_sensor_helper.h @@ -29,6 +29,7 @@ public: std::optional<int16_t> blackLevel() const { return blackLevel_; } virtual uint32_t gainCode(double gain) const; virtual double gain(uint32_t gainCode) const; + double quantizeGain(double gain, double *quantizationGain) const; protected: struct AnalogueGainLinear {