| Message ID | 20211013154125.133419-8-jeanmichel.hautbois@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Jean-Michel Hautbois (2021-10-13 16:41:19) > The gains is currently set as a uint32_t while the analogue gain is 'gains are currently' ? > passed as a double. We also have a default maximum analogue gain of 15 > which is quite high for a number of sensors. > > Use a maximum value of 8 which should really be configured by the IPA > and not fixed as it is now. While at it make it a double. > > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > --- > src/ipa/ipu3/algorithms/agc.cpp | 12 ++++-------- > 1 file changed, 4 insertions(+), 8 deletions(-) > > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp > index 2dd5c5c1..e58a8a8d 100644 > --- a/src/ipa/ipu3/algorithms/agc.cpp > +++ b/src/ipa/ipu3/algorithms/agc.cpp > @@ -30,14 +30,10 @@ static constexpr uint32_t kInitialFrameMinAECount = 4; > /* Number of frames to wait between new gain/exposure estimations */ > static constexpr uint32_t kFrameSkipCount = 6; > > -/* Maximum ISO value for analogue gain */ > -static constexpr uint32_t kMinISO = 100; > -static constexpr uint32_t kMaxISO = 1500; > - Do the ISO values no longer make sense to be used here? As this needs to come from a camera helper anyway, I guess it doesn't hurt to reduce it as it should be changed later all the same. Do you reduce it to prevent some overexposure issue? or because the sensors you've tested only have x8 as a maximum? Anyway, I don't object to this patch otherwise. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > /* Maximum analogue gain value > * \todo grab it from a camera helper */ > -static constexpr uint32_t kMinGain = kMinISO / 100; > -static constexpr uint32_t kMaxGain = kMaxISO / 100; > +static constexpr double kMinGain = 1.0; > +static constexpr double kMaxGain = 8.0; > > /* \todo use calculated value based on sensor */ > static constexpr libcamera::utils::Duration kMinShutterSpeed = 100us; > @@ -161,9 +157,9 @@ void Agc::lockExposureGain(uint32_t &exposure, double &gain) > if (currentShutter < kMaxShutterSpeed) { > exposure = std::clamp(static_cast<uint32_t>(exposure * currentExposure_ / currentExposureNoDg_), minExposureLines_, maxExposureLines_); > newExposure = currentExposure_ / exposure; > - gain = std::clamp(static_cast<uint32_t>(gain * currentExposure_ / newExposure), kMinGain, kMaxGain); > + gain = std::clamp(static_cast<double>(gain * currentExposure_ / newExposure), kMinGain, kMaxGain); > } else if (currentShutter >= kMaxShutterSpeed) { > - gain = std::clamp(static_cast<uint32_t>(gain * currentExposure_ / currentExposureNoDg_), kMinGain, kMaxGain); > + gain = std::clamp(static_cast<double>(gain * currentExposure_ / currentExposureNoDg_), kMinGain, kMaxGain); > newExposure = currentExposure_ / gain; > exposure = std::clamp(static_cast<uint32_t>(exposure * currentExposure_ / newExposure), minExposureLines_, maxExposureLines_); > } > -- > 2.30.2 >
On Thu, Oct 14, 2021 at 12:05:45PM +0100, Kieran Bingham wrote: > Quoting Jean-Michel Hautbois (2021-10-13 16:41:19) > > The gains is currently set as a uint32_t while the analogue gain is > > 'gains are currently' ? > > > passed as a double. We also have a default maximum analogue gain of 15 > > which is quite high for a number of sensors. > > > > Use a maximum value of 8 which should really be configured by the IPA > > and not fixed as it is now. While at it make it a double. > > > > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > > --- > > src/ipa/ipu3/algorithms/agc.cpp | 12 ++++-------- > > 1 file changed, 4 insertions(+), 8 deletions(-) > > > > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp > > index 2dd5c5c1..e58a8a8d 100644 > > --- a/src/ipa/ipu3/algorithms/agc.cpp > > +++ b/src/ipa/ipu3/algorithms/agc.cpp > > @@ -30,14 +30,10 @@ static constexpr uint32_t kInitialFrameMinAECount = 4; > > /* Number of frames to wait between new gain/exposure estimations */ > > static constexpr uint32_t kFrameSkipCount = 6; > > > > -/* Maximum ISO value for analogue gain */ > > -static constexpr uint32_t kMinISO = 100; > > -static constexpr uint32_t kMaxISO = 1500; > > - > > Do the ISO values no longer make sense to be used here? ISO values don't make much sense anywhere to be honest :-) It's mostly a marketing argument. Considering gain = ISO / 100 is as good as anything, because you can set a random conversion factor (within limits) and still be compliant with the specification. > As this needs to come from a camera helper anyway, I guess it doesn't > hurt to reduce it as it should be changed later all the same. > > Do you reduce it to prevent some overexposure issue? or because the > sensors you've tested only have x8 as a maximum? > > Anyway, I don't object to this patch otherwise. > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > /* Maximum analogue gain value > > * \todo grab it from a camera helper */ > > -static constexpr uint32_t kMinGain = kMinISO / 100; > > -static constexpr uint32_t kMaxGain = kMaxISO / 100; > > +static constexpr double kMinGain = 1.0; > > +static constexpr double kMaxGain = 8.0; > > > > /* \todo use calculated value based on sensor */ > > static constexpr libcamera::utils::Duration kMinShutterSpeed = 100us; > > @@ -161,9 +157,9 @@ void Agc::lockExposureGain(uint32_t &exposure, double &gain) > > if (currentShutter < kMaxShutterSpeed) { > > exposure = std::clamp(static_cast<uint32_t>(exposure * currentExposure_ / currentExposureNoDg_), minExposureLines_, maxExposureLines_); > > newExposure = currentExposure_ / exposure; > > - gain = std::clamp(static_cast<uint32_t>(gain * currentExposure_ / newExposure), kMinGain, kMaxGain); > > + gain = std::clamp(static_cast<double>(gain * currentExposure_ / newExposure), kMinGain, kMaxGain); This is also a good occasion to shorten lines, and you don't need the cast: gain = std::clamp(gain * currentExposure_ / newExposure, kMinGain, kMaxGain); Same below. By the way, in the previous patch, you could have used exposure = std::clamp<uint32_t>(exposure * currentExposure_ / currentExposureNoDg_, minExposureLines_, maxExposureLines_); Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > } else if (currentShutter >= kMaxShutterSpeed) { > > - gain = std::clamp(static_cast<uint32_t>(gain * currentExposure_ / currentExposureNoDg_), kMinGain, kMaxGain); > > + gain = std::clamp(static_cast<double>(gain * currentExposure_ / currentExposureNoDg_), kMinGain, kMaxGain); > > newExposure = currentExposure_ / gain; > > exposure = std::clamp(static_cast<uint32_t>(exposure * currentExposure_ / newExposure), minExposureLines_, maxExposureLines_); > > }
Hi Laurent, On 15/10/2021 02:48, Laurent Pinchart wrote: > On Thu, Oct 14, 2021 at 12:05:45PM +0100, Kieran Bingham wrote: >> Quoting Jean-Michel Hautbois (2021-10-13 16:41:19) >>> The gains is currently set as a uint32_t while the analogue gain is >> >> 'gains are currently' ? >> >>> passed as a double. We also have a default maximum analogue gain of 15 >>> which is quite high for a number of sensors. >>> >>> Use a maximum value of 8 which should really be configured by the IPA >>> and not fixed as it is now. While at it make it a double. >>> >>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> >>> --- >>> src/ipa/ipu3/algorithms/agc.cpp | 12 ++++-------- >>> 1 file changed, 4 insertions(+), 8 deletions(-) >>> >>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp >>> index 2dd5c5c1..e58a8a8d 100644 >>> --- a/src/ipa/ipu3/algorithms/agc.cpp >>> +++ b/src/ipa/ipu3/algorithms/agc.cpp >>> @@ -30,14 +30,10 @@ static constexpr uint32_t kInitialFrameMinAECount = 4; >>> /* Number of frames to wait between new gain/exposure estimations */ >>> static constexpr uint32_t kFrameSkipCount = 6; >>> >>> -/* Maximum ISO value for analogue gain */ >>> -static constexpr uint32_t kMinISO = 100; >>> -static constexpr uint32_t kMaxISO = 1500; >>> - >> >> Do the ISO values no longer make sense to be used here? > > ISO values don't make much sense anywhere to be honest :-) It's mostly a > marketing argument. Considering gain = ISO / 100 is as good as anything, > because you can set a random conversion factor (within limits) and still > be compliant with the specification. Ah ah you spent too much time in this spec, right ;-) ? > >> As this needs to come from a camera helper anyway, I guess it doesn't >> hurt to reduce it as it should be changed later all the same. >> >> Do you reduce it to prevent some overexposure issue? or because the >> sensors you've tested only have x8 as a maximum? >> >> Anyway, I don't object to this patch otherwise. >> >> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> >>> /* Maximum analogue gain value >>> * \todo grab it from a camera helper */ >>> -static constexpr uint32_t kMinGain = kMinISO / 100; >>> -static constexpr uint32_t kMaxGain = kMaxISO / 100; >>> +static constexpr double kMinGain = 1.0; >>> +static constexpr double kMaxGain = 8.0; >>> >>> /* \todo use calculated value based on sensor */ >>> static constexpr libcamera::utils::Duration kMinShutterSpeed = 100us; >>> @@ -161,9 +157,9 @@ void Agc::lockExposureGain(uint32_t &exposure, double &gain) >>> if (currentShutter < kMaxShutterSpeed) { >>> exposure = std::clamp(static_cast<uint32_t>(exposure * currentExposure_ / currentExposureNoDg_), minExposureLines_, maxExposureLines_); >>> newExposure = currentExposure_ / exposure; >>> - gain = std::clamp(static_cast<uint32_t>(gain * currentExposure_ / newExposure), kMinGain, kMaxGain); >>> + gain = std::clamp(static_cast<double>(gain * currentExposure_ / newExposure), kMinGain, kMaxGain); > > This is also a good occasion to shorten lines, and you don't need the > cast: > > gain = std::clamp(gain * currentExposure_ / newExposure, > kMinGain, kMaxGain); > > Same below. > > By the way, in the previous patch, you could have used > > exposure = std::clamp<uint32_t>(exposure * currentExposure_ / currentExposureNoDg_, > minExposureLines_, > maxExposureLines_); I did not think about passing the template type to std::clamp() because... I did not know we could do that, thanks ! > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>> } else if (currentShutter >= kMaxShutterSpeed) { >>> - gain = std::clamp(static_cast<uint32_t>(gain * currentExposure_ / currentExposureNoDg_), kMinGain, kMaxGain); >>> + gain = std::clamp(static_cast<double>(gain * currentExposure_ / currentExposureNoDg_), kMinGain, kMaxGain); >>> newExposure = currentExposure_ / gain; >>> exposure = std::clamp(static_cast<uint32_t>(exposure * currentExposure_ / newExposure), minExposureLines_, maxExposureLines_); >>> } >
diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp index 2dd5c5c1..e58a8a8d 100644 --- a/src/ipa/ipu3/algorithms/agc.cpp +++ b/src/ipa/ipu3/algorithms/agc.cpp @@ -30,14 +30,10 @@ static constexpr uint32_t kInitialFrameMinAECount = 4; /* Number of frames to wait between new gain/exposure estimations */ static constexpr uint32_t kFrameSkipCount = 6; -/* Maximum ISO value for analogue gain */ -static constexpr uint32_t kMinISO = 100; -static constexpr uint32_t kMaxISO = 1500; - /* Maximum analogue gain value * \todo grab it from a camera helper */ -static constexpr uint32_t kMinGain = kMinISO / 100; -static constexpr uint32_t kMaxGain = kMaxISO / 100; +static constexpr double kMinGain = 1.0; +static constexpr double kMaxGain = 8.0; /* \todo use calculated value based on sensor */ static constexpr libcamera::utils::Duration kMinShutterSpeed = 100us; @@ -161,9 +157,9 @@ void Agc::lockExposureGain(uint32_t &exposure, double &gain) if (currentShutter < kMaxShutterSpeed) { exposure = std::clamp(static_cast<uint32_t>(exposure * currentExposure_ / currentExposureNoDg_), minExposureLines_, maxExposureLines_); newExposure = currentExposure_ / exposure; - gain = std::clamp(static_cast<uint32_t>(gain * currentExposure_ / newExposure), kMinGain, kMaxGain); + gain = std::clamp(static_cast<double>(gain * currentExposure_ / newExposure), kMinGain, kMaxGain); } else if (currentShutter >= kMaxShutterSpeed) { - gain = std::clamp(static_cast<uint32_t>(gain * currentExposure_ / currentExposureNoDg_), kMinGain, kMaxGain); + gain = std::clamp(static_cast<double>(gain * currentExposure_ / currentExposureNoDg_), kMinGain, kMaxGain); newExposure = currentExposure_ / gain; exposure = std::clamp(static_cast<uint32_t>(exposure * currentExposure_ / newExposure), minExposureLines_, maxExposureLines_); }
The gains is currently set as a uint32_t while the analogue gain is passed as a double. We also have a default maximum analogue gain of 15 which is quite high for a number of sensors. Use a maximum value of 8 which should really be configured by the IPA and not fixed as it is now. While at it make it a double. Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> --- src/ipa/ipu3/algorithms/agc.cpp | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)