| Message ID | 20250721074853.1463358-2-naush@raspberrypi.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush, David, On Mon, Jul 21, 2025 at 08:47:21AM +0100, Naushir Patuck wrote: > From: David Plowman <david.plowman@raspberrypi.com> > > Previously these were handled in the AGC/AEC exposure update > calculations by explicitly driving a higher digital gain to "cancel > out" any colour gains that were less than 1. > > Now we're ignoring this in the AGC and leaving it to the IPA code to > normalise all the gains so that the smallest is 1. We don't regard > this as a "real" increase because one of the colour channels (just not > necessarily the green one) still gets the minimum gain possible. > > We do, however, update the statistics calculations so that they > reflect any such digital gain increase, so that images are driven to > the correct level. I'm checking if this is something we should replicate in other IPA modules. The commit message doesn't document the reason for this change. Could you briefly explain the rationale ? > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/ipa/rpi/controller/rpi/agc_channel.cpp | 41 +++++++--------------- > src/ipa/rpi/pisp/pisp.cpp | 26 ++++++++++---- > src/ipa/rpi/vc4/vc4.cpp | 26 +++++++++++--- > 3 files changed, 54 insertions(+), 39 deletions(-) > > diff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp b/src/ipa/rpi/controller/rpi/agc_channel.cpp > index b2999364bf27..ae0cb148893a 100644 > --- a/src/ipa/rpi/controller/rpi/agc_channel.cpp > +++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp > @@ -435,14 +435,9 @@ void AgcChannel::switchMode(CameraMode const &cameraMode, > > Duration fixedExposureTime = limitExposureTime(fixedExposureTime_); > if (fixedExposureTime && fixedAnalogueGain_) { > - /* We're going to reset the algorithm here with these fixed values. */ > - fetchAwbStatus(metadata); > - double minColourGain = std::min({ awb_.gainR, awb_.gainG, awb_.gainB, 1.0 }); > - ASSERT(minColourGain != 0.0); > - > /* This is the equivalent of computeTargetExposure and applyDigitalGain. */ > target_.totalExposureNoDG = fixedExposureTime_ * fixedAnalogueGain_; > - target_.totalExposure = target_.totalExposureNoDG / minColourGain; > + target_.totalExposure = target_.totalExposureNoDG; > > /* Equivalent of filterExposure. This resets any "history". */ > filtered_ = target_; > @@ -462,10 +457,10 @@ void AgcChannel::switchMode(CameraMode const &cameraMode, > */ > > double ratio = lastSensitivity / cameraMode.sensitivity; > - target_.totalExposureNoDG *= ratio; > target_.totalExposure *= ratio; > - filtered_.totalExposureNoDG *= ratio; > + target_.totalExposureNoDG = target_.totalExposure; > filtered_.totalExposure *= ratio; > + filtered_.totalExposureNoDG = filtered_.totalExposure; > > divideUpExposure(); > } else { > @@ -716,8 +711,13 @@ static double computeInitialY(StatisticsPtr &stats, AwbStatus const &awb, > } > > /* Factor in the AWB correction if needed. */ > - if (stats->agcStatsPos == Statistics::AgcStatsPos::PreWb) > - sum *= RGB<double>{ { awb.gainR, awb.gainG, awb.gainB } }; > + if (stats->agcStatsPos == Statistics::AgcStatsPos::PreWb) { > + double minColourGain = std::min({ awb.gainR, awb.gainG, awb.gainB, 1.0 }); > + minColourGain = std::max(minColourGain, 1.0); > + RGB<double> colourGains{ { awb.gainR, awb.gainG, awb.gainB } }; > + colourGains /= minColourGain; > + sum *= colourGains; > + } > > double ySum = ipa::rec601LuminanceFromRGB(sum); > > @@ -797,16 +797,8 @@ void AgcChannel::computeGain(StatisticsPtr &statistics, Metadata *imageMetadata, > void AgcChannel::computeTargetExposure(double gain) > { > if (status_.fixedExposureTime && status_.fixedAnalogueGain) { > - /* > - * When analogue gain and exposure time are both fixed, we need > - * to drive the total exposure so that we end up with a digital > - * gain of at least 1/minColourGain. Otherwise we'd desaturate > - * channels causing white to go cyan or magenta. > - */ > - double minColourGain = std::min({ awb_.gainR, awb_.gainG, awb_.gainB, 1.0 }); > - ASSERT(minColourGain != 0.0); > target_.totalExposure = > - status_.fixedExposureTime * status_.fixedAnalogueGain / minColourGain; > + status_.fixedExposureTime * status_.fixedAnalogueGain; > } else { > /* > * The statistics reflect the image without digital gain, so the final > @@ -867,15 +859,8 @@ bool AgcChannel::applyChannelConstraints(const AgcChannelTotalExposures &channel > > bool AgcChannel::applyDigitalGain(double gain, double targetY, bool channelBound) > { > - double minColourGain = std::min({ awb_.gainR, awb_.gainG, awb_.gainB, 1.0 }); > - ASSERT(minColourGain != 0.0); > - double dg = 1.0 / minColourGain; > - /* > - * I think this pipeline subtracts black level and rescales before we > - * get the stats, so no need to worry about it. > - */ > - LOG(RPiAgc, Debug) << "after AWB, target dg " << dg << " gain " << gain > - << " target_Y " << targetY; > + double dg = 1.0; > + > /* > * Finally, if we're trying to reduce exposure but the target_Y is > * "close" to 1.0, then the gain computed for that constraint will be > diff --git a/src/ipa/rpi/pisp/pisp.cpp b/src/ipa/rpi/pisp/pisp.cpp > index bb50a9e05904..e1a804f533bb 100644 > --- a/src/ipa/rpi/pisp/pisp.cpp > +++ b/src/ipa/rpi/pisp/pisp.cpp > @@ -521,10 +521,24 @@ void IpaPiSP::applyWBG(const AwbStatus *awbStatus, const AgcPrepareStatus *agcPr > pisp_wbg_config wbg; > pisp_fe_rgby_config rgby = {}; > double dg = agcPrepareStatus ? agcPrepareStatus->digitalGain : 1.0; > + double minColourGain = std::min({ awbStatus->gainR, awbStatus->gainG, awbStatus->gainB, 1.0 }); > + /* The 0.1 here doesn't mean much, but just stops arithmetic errors and extreme behaviour. */ > + double extraGain = 1.0 / std::max({ minColourGain, 0.1 }); > > - wbg.gain_r = clampField(dg * awbStatus->gainR, 14, 10); > - wbg.gain_g = clampField(dg * awbStatus->gainG, 14, 10); > - wbg.gain_b = clampField(dg * awbStatus->gainB, 14, 10); > + /* > + * Apply an extra gain of 1 / minColourGain so as not to apply < 1 gains to any > + * channels (which would cause saturated pixels to go cyan or magenta). > + * Doing this doesn't really apply more gain than necessary, because one of the > + * channels is always getting the minimum gain possible. For this reason we also > + * don't change the values that we report externally. > + */ > + double gainR = awbStatus->gainR * extraGain; > + double gainG = awbStatus->gainG * extraGain; > + double gainB = awbStatus->gainB * extraGain; > + > + wbg.gain_r = clampField(dg * gainR, 14, 10); > + wbg.gain_g = clampField(dg * gainG, 14, 10); > + wbg.gain_b = clampField(dg * gainB, 14, 10); > > /* > * The YCbCr conversion block should contain the appropriate YCbCr > @@ -535,9 +549,9 @@ void IpaPiSP::applyWBG(const AwbStatus *awbStatus, const AgcPrepareStatus *agcPr > be_->GetYcbcr(csc); > > /* The CSC coefficients already have the << 10 scaling applied. */ > - rgby.gain_r = clampField(csc.coeffs[0] * awbStatus->gainR, 14); > - rgby.gain_g = clampField(csc.coeffs[1] * awbStatus->gainG, 14); > - rgby.gain_b = clampField(csc.coeffs[2] * awbStatus->gainB, 14); > + rgby.gain_r = clampField(csc.coeffs[0] * gainR, 14); > + rgby.gain_g = clampField(csc.coeffs[1] * gainG, 14); > + rgby.gain_b = clampField(csc.coeffs[2] * gainB, 14); > > LOG(IPARPI, Debug) << "Applying WB R: " << awbStatus->gainR << " B: " > << awbStatus->gainB; > diff --git a/src/ipa/rpi/vc4/vc4.cpp b/src/ipa/rpi/vc4/vc4.cpp > index ba43e4741584..8a7a37c870ed 100644 > --- a/src/ipa/rpi/vc4/vc4.cpp > +++ b/src/ipa/rpi/vc4/vc4.cpp > @@ -63,7 +63,8 @@ private: > bool validateIspControls(); > > void applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls); > - void applyDG(const struct AgcPrepareStatus *dgStatus, ControlList &ctrls); > + void applyDG(const struct AgcPrepareStatus *dgStatus, > + const struct AwbStatus *awbStatus, ControlList &ctrls); > void applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls); > void applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, ControlList &ctrls); > void applyGamma(const struct ContrastStatus *contrastStatus, ControlList &ctrls); > @@ -152,8 +153,7 @@ void IpaVc4::platformPrepareIsp([[maybe_unused]] const PrepareParams ¶ms, > applyCCM(ccmStatus, ctrls); > > AgcPrepareStatus *dgStatus = rpiMetadata.getLocked<AgcPrepareStatus>("agc.prepare_status"); > - if (dgStatus) > - applyDG(dgStatus, ctrls); > + applyDG(dgStatus, awbStatus, ctrls); > > AlscStatus *lsStatus = rpiMetadata.getLocked<AlscStatus>("alsc.status"); > if (lsStatus) > @@ -329,10 +329,26 @@ void IpaVc4::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls) > static_cast<int32_t>(awbStatus->gainB * 1000)); > } > > -void IpaVc4::applyDG(const struct AgcPrepareStatus *dgStatus, ControlList &ctrls) > +void IpaVc4::applyDG(const struct AgcPrepareStatus *dgStatus, > + const struct AwbStatus *awbStatus, ControlList &ctrls) > { > + double digitalGain = dgStatus ? dgStatus->digitalGain : 1.0; > + > + if (awbStatus) { > + /* > + * We must apply sufficient extra digital gain to stop any of the channel gains being > + * less than 1, which would cause saturation artifacts. Note that one of the colour > + * channels is still getting the minimum possible gain, so it's not a "real" gain > + * increase. > + */ > + double minColourGain = std::min({ awbStatus->gainR, awbStatus->gainG, awbStatus->gainB, 1.0 }); > + /* The 0.1 here doesn't mean much, but just stops arithmetic errors and extreme behaviour. */ > + double extraGain = 1.0 / std::max({ minColourGain, 0.1 }); > + digitalGain *= extraGain; > + } > + > ctrls.set(V4L2_CID_DIGITAL_GAIN, > - static_cast<int32_t>(dgStatus->digitalGain * 1000)); > + static_cast<int32_t>(digitalGain * 1000)); > } > > void IpaVc4::applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls)
Hi Laurent Well, I think it's all a matter of personal taste really, and to be honest, this was so long ago my recollections are a bit hazy. I think it's just a slight simplification of the AGC. Previously, if AWB decided the red gain was 0.8, then the AGC had to see this, and compensate by applying a global digital gain of 1/0.8 = 1.25, so that multiplied together, the red gain is now 1. Arguably, this would mean the AGC should possibly target a point only 0.8x its nominal target, because that extra digital gain would make up for it. It seems I slightly changed my view on that. and decided I didn't care about it. I preferred to simplify the AGC so as not to worry about that, and make the AWB normalise the gains to the red gain comes out as 1. So the image comes out a teeny bit brighter, but it's a very marginal effect. In practice, the only cameras we have that suffer from all this (which is when the red component goes very high in very low CT illumination like 3000K), are Omnivision ones, where even then the gain never goes below about 0.95 - so it's a 5% effect maximum. From my point of view, it's a take-it-or-leave-it thing, go with personal preference. Dunno if that helps! David On Wed, 23 Jul 2025 at 05:53, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Naush, David, > > On Mon, Jul 21, 2025 at 08:47:21AM +0100, Naushir Patuck wrote: > > From: David Plowman <david.plowman@raspberrypi.com> > > > > Previously these were handled in the AGC/AEC exposure update > > calculations by explicitly driving a higher digital gain to "cancel > > out" any colour gains that were less than 1. > > > > Now we're ignoring this in the AGC and leaving it to the IPA code to > > normalise all the gains so that the smallest is 1. We don't regard > > this as a "real" increase because one of the colour channels (just not > > necessarily the green one) still gets the minimum gain possible. > > > > We do, however, update the statistics calculations so that they > > reflect any such digital gain increase, so that images are driven to > > the correct level. > > I'm checking if this is something we should replicate in other IPA > modules. The commit message doesn't document the reason for this change. > Could you briefly explain the rationale ? > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > src/ipa/rpi/controller/rpi/agc_channel.cpp | 41 +++++++--------------- > > src/ipa/rpi/pisp/pisp.cpp | 26 ++++++++++---- > > src/ipa/rpi/vc4/vc4.cpp | 26 +++++++++++--- > > 3 files changed, 54 insertions(+), 39 deletions(-) > > > > diff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp > b/src/ipa/rpi/controller/rpi/agc_channel.cpp > > index b2999364bf27..ae0cb148893a 100644 > > --- a/src/ipa/rpi/controller/rpi/agc_channel.cpp > > +++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp > > @@ -435,14 +435,9 @@ void AgcChannel::switchMode(CameraMode const > &cameraMode, > > > > Duration fixedExposureTime = limitExposureTime(fixedExposureTime_); > > if (fixedExposureTime && fixedAnalogueGain_) { > > - /* We're going to reset the algorithm here with these > fixed values. */ > > - fetchAwbStatus(metadata); > > - double minColourGain = std::min({ awb_.gainR, awb_.gainG, > awb_.gainB, 1.0 }); > > - ASSERT(minColourGain != 0.0); > > - > > /* This is the equivalent of computeTargetExposure and > applyDigitalGain. */ > > target_.totalExposureNoDG = fixedExposureTime_ * > fixedAnalogueGain_; > > - target_.totalExposure = target_.totalExposureNoDG / > minColourGain; > > + target_.totalExposure = target_.totalExposureNoDG; > > > > /* Equivalent of filterExposure. This resets any > "history". */ > > filtered_ = target_; > > @@ -462,10 +457,10 @@ void AgcChannel::switchMode(CameraMode const > &cameraMode, > > */ > > > > double ratio = lastSensitivity / cameraMode.sensitivity; > > - target_.totalExposureNoDG *= ratio; > > target_.totalExposure *= ratio; > > - filtered_.totalExposureNoDG *= ratio; > > + target_.totalExposureNoDG = target_.totalExposure; > > filtered_.totalExposure *= ratio; > > + filtered_.totalExposureNoDG = filtered_.totalExposure; > > > > divideUpExposure(); > > } else { > > @@ -716,8 +711,13 @@ static double computeInitialY(StatisticsPtr &stats, > AwbStatus const &awb, > > } > > > > /* Factor in the AWB correction if needed. */ > > - if (stats->agcStatsPos == Statistics::AgcStatsPos::PreWb) > > - sum *= RGB<double>{ { awb.gainR, awb.gainG, awb.gainB } }; > > + if (stats->agcStatsPos == Statistics::AgcStatsPos::PreWb) { > > + double minColourGain = std::min({ awb.gainR, awb.gainG, > awb.gainB, 1.0 }); > > + minColourGain = std::max(minColourGain, 1.0); > > + RGB<double> colourGains{ { awb.gainR, awb.gainG, awb.gainB > } }; > > + colourGains /= minColourGain; > > + sum *= colourGains; > > + } > > > > double ySum = ipa::rec601LuminanceFromRGB(sum); > > > > @@ -797,16 +797,8 @@ void AgcChannel::computeGain(StatisticsPtr > &statistics, Metadata *imageMetadata, > > void AgcChannel::computeTargetExposure(double gain) > > { > > if (status_.fixedExposureTime && status_.fixedAnalogueGain) { > > - /* > > - * When analogue gain and exposure time are both fixed, we > need > > - * to drive the total exposure so that we end up with a > digital > > - * gain of at least 1/minColourGain. Otherwise we'd > desaturate > > - * channels causing white to go cyan or magenta. > > - */ > > - double minColourGain = std::min({ awb_.gainR, awb_.gainG, > awb_.gainB, 1.0 }); > > - ASSERT(minColourGain != 0.0); > > target_.totalExposure = > > - status_.fixedExposureTime * > status_.fixedAnalogueGain / minColourGain; > > + status_.fixedExposureTime * > status_.fixedAnalogueGain; > > } else { > > /* > > * The statistics reflect the image without digital gain, > so the final > > @@ -867,15 +859,8 @@ bool AgcChannel::applyChannelConstraints(const > AgcChannelTotalExposures &channel > > > > bool AgcChannel::applyDigitalGain(double gain, double targetY, bool > channelBound) > > { > > - double minColourGain = std::min({ awb_.gainR, awb_.gainG, > awb_.gainB, 1.0 }); > > - ASSERT(minColourGain != 0.0); > > - double dg = 1.0 / minColourGain; > > - /* > > - * I think this pipeline subtracts black level and rescales before > we > > - * get the stats, so no need to worry about it. > > - */ > > - LOG(RPiAgc, Debug) << "after AWB, target dg " << dg << " gain " << > gain > > - << " target_Y " << targetY; > > + double dg = 1.0; > > + > > /* > > * Finally, if we're trying to reduce exposure but the target_Y is > > * "close" to 1.0, then the gain computed for that constraint will > be > > diff --git a/src/ipa/rpi/pisp/pisp.cpp b/src/ipa/rpi/pisp/pisp.cpp > > index bb50a9e05904..e1a804f533bb 100644 > > --- a/src/ipa/rpi/pisp/pisp.cpp > > +++ b/src/ipa/rpi/pisp/pisp.cpp > > @@ -521,10 +521,24 @@ void IpaPiSP::applyWBG(const AwbStatus *awbStatus, > const AgcPrepareStatus *agcPr > > pisp_wbg_config wbg; > > pisp_fe_rgby_config rgby = {}; > > double dg = agcPrepareStatus ? agcPrepareStatus->digitalGain : 1.0; > > + double minColourGain = std::min({ awbStatus->gainR, > awbStatus->gainG, awbStatus->gainB, 1.0 }); > > + /* The 0.1 here doesn't mean much, but just stops arithmetic > errors and extreme behaviour. */ > > + double extraGain = 1.0 / std::max({ minColourGain, 0.1 }); > > > > - wbg.gain_r = clampField(dg * awbStatus->gainR, 14, 10); > > - wbg.gain_g = clampField(dg * awbStatus->gainG, 14, 10); > > - wbg.gain_b = clampField(dg * awbStatus->gainB, 14, 10); > > + /* > > + * Apply an extra gain of 1 / minColourGain so as not to apply < 1 > gains to any > > + * channels (which would cause saturated pixels to go cyan or > magenta). > > + * Doing this doesn't really apply more gain than necessary, > because one of the > > + * channels is always getting the minimum gain possible. For this > reason we also > > + * don't change the values that we report externally. > > + */ > > + double gainR = awbStatus->gainR * extraGain; > > + double gainG = awbStatus->gainG * extraGain; > > + double gainB = awbStatus->gainB * extraGain; > > + > > + wbg.gain_r = clampField(dg * gainR, 14, 10); > > + wbg.gain_g = clampField(dg * gainG, 14, 10); > > + wbg.gain_b = clampField(dg * gainB, 14, 10); > > > > /* > > * The YCbCr conversion block should contain the appropriate YCbCr > > @@ -535,9 +549,9 @@ void IpaPiSP::applyWBG(const AwbStatus *awbStatus, > const AgcPrepareStatus *agcPr > > be_->GetYcbcr(csc); > > > > /* The CSC coefficients already have the << 10 scaling applied. */ > > - rgby.gain_r = clampField(csc.coeffs[0] * awbStatus->gainR, 14); > > - rgby.gain_g = clampField(csc.coeffs[1] * awbStatus->gainG, 14); > > - rgby.gain_b = clampField(csc.coeffs[2] * awbStatus->gainB, 14); > > + rgby.gain_r = clampField(csc.coeffs[0] * gainR, 14); > > + rgby.gain_g = clampField(csc.coeffs[1] * gainG, 14); > > + rgby.gain_b = clampField(csc.coeffs[2] * gainB, 14); > > > > LOG(IPARPI, Debug) << "Applying WB R: " << awbStatus->gainR << " > B: " > > << awbStatus->gainB; > > diff --git a/src/ipa/rpi/vc4/vc4.cpp b/src/ipa/rpi/vc4/vc4.cpp > > index ba43e4741584..8a7a37c870ed 100644 > > --- a/src/ipa/rpi/vc4/vc4.cpp > > +++ b/src/ipa/rpi/vc4/vc4.cpp > > @@ -63,7 +63,8 @@ private: > > bool validateIspControls(); > > > > void applyAWB(const struct AwbStatus *awbStatus, ControlList > &ctrls); > > - void applyDG(const struct AgcPrepareStatus *dgStatus, ControlList > &ctrls); > > + void applyDG(const struct AgcPrepareStatus *dgStatus, > > + const struct AwbStatus *awbStatus, ControlList > &ctrls); > > void applyCCM(const struct CcmStatus *ccmStatus, ControlList > &ctrls); > > void applyBlackLevel(const struct BlackLevelStatus > *blackLevelStatus, ControlList &ctrls); > > void applyGamma(const struct ContrastStatus *contrastStatus, > ControlList &ctrls); > > @@ -152,8 +153,7 @@ void IpaVc4::platformPrepareIsp([[maybe_unused]] > const PrepareParams ¶ms, > > applyCCM(ccmStatus, ctrls); > > > > AgcPrepareStatus *dgStatus = > rpiMetadata.getLocked<AgcPrepareStatus>("agc.prepare_status"); > > - if (dgStatus) > > - applyDG(dgStatus, ctrls); > > + applyDG(dgStatus, awbStatus, ctrls); > > > > AlscStatus *lsStatus = > rpiMetadata.getLocked<AlscStatus>("alsc.status"); > > if (lsStatus) > > @@ -329,10 +329,26 @@ void IpaVc4::applyAWB(const struct AwbStatus > *awbStatus, ControlList &ctrls) > > static_cast<int32_t>(awbStatus->gainB * 1000)); > > } > > > > -void IpaVc4::applyDG(const struct AgcPrepareStatus *dgStatus, > ControlList &ctrls) > > +void IpaVc4::applyDG(const struct AgcPrepareStatus *dgStatus, > > + const struct AwbStatus *awbStatus, ControlList &ctrls) > > { > > + double digitalGain = dgStatus ? dgStatus->digitalGain : 1.0; > > + > > + if (awbStatus) { > > + /* > > + * We must apply sufficient extra digital gain to stop any > of the channel gains being > > + * less than 1, which would cause saturation artifacts. > Note that one of the colour > > + * channels is still getting the minimum possible gain, so > it's not a "real" gain > > + * increase. > > + */ > > + double minColourGain = std::min({ awbStatus->gainR, > awbStatus->gainG, awbStatus->gainB, 1.0 }); > > + /* The 0.1 here doesn't mean much, but just stops > arithmetic errors and extreme behaviour. */ > > + double extraGain = 1.0 / std::max({ minColourGain, 0.1 }); > > + digitalGain *= extraGain; > > + } > > + > > ctrls.set(V4L2_CID_DIGITAL_GAIN, > > - static_cast<int32_t>(dgStatus->digitalGain * 1000)); > > + static_cast<int32_t>(digitalGain * 1000)); > > } > > > > void IpaVc4::applyCCM(const struct CcmStatus *ccmStatus, ControlList > &ctrls) > > -- > Regards, > > Laurent Pinchart >
Hi David, On Wed, Jul 23, 2025 at 08:24:58AM +0100, David Plowman wrote: > Hi Laurent > > Well, I think it's all a matter of personal taste really, and to be honest, > this was so long ago my recollections are a bit hazy. > > I think it's just a slight simplification of the AGC. Previously, if AWB > decided the red gain was 0.8, then the AGC had to see this, and compensate > by applying a global digital gain of 1/0.8 = 1.25, so that multiplied > together, the red gain is now 1. Arguably, this would mean the AGC should > possibly target a point only 0.8x its nominal target, because that extra > digital gain would make up for it. > > It seems I slightly changed my view on that. and decided I didn't care > about it. I preferred to simplify the AGC so as not to worry about that, > and make the AWB normalise the gains to the red gain comes out as 1. So the > image comes out a teeny bit brighter, but it's a very marginal effect. In > practice, the only cameras we have that suffer from all this (which is when > the red component goes very high in very low CT illumination like 3000K), > are Omnivision ones, where even then the gain never goes below about 0.95 - > so it's a 5% effect maximum. > > From my point of view, it's a take-it-or-leave-it thing, go with personal > preference. Dunno if that helps! It does, thanks. > On Wed, 23 Jul 2025 at 05:53, Laurent Pinchart wrote: > > On Mon, Jul 21, 2025 at 08:47:21AM +0100, Naushir Patuck wrote: > > > From: David Plowman <david.plowman@raspberrypi.com> > > > > > > Previously these were handled in the AGC/AEC exposure update > > > calculations by explicitly driving a higher digital gain to "cancel > > > out" any colour gains that were less than 1. > > > > > > Now we're ignoring this in the AGC and leaving it to the IPA code to > > > normalise all the gains so that the smallest is 1. We don't regard > > > this as a "real" increase because one of the colour channels (just not > > > necessarily the green one) still gets the minimum gain possible. > > > > > > We do, however, update the statistics calculations so that they > > > reflect any such digital gain increase, so that images are driven to > > > the correct level. > > > > I'm checking if this is something we should replicate in other IPA > > modules. The commit message doesn't document the reason for this change. > > Could you briefly explain the rationale ? > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > > > --- > > > src/ipa/rpi/controller/rpi/agc_channel.cpp | 41 +++++++--------------- > > > src/ipa/rpi/pisp/pisp.cpp | 26 ++++++++++---- > > > src/ipa/rpi/vc4/vc4.cpp | 26 +++++++++++--- > > > 3 files changed, 54 insertions(+), 39 deletions(-) > > > > > > diff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp b/src/ipa/rpi/controller/rpi/agc_channel.cpp > > > index b2999364bf27..ae0cb148893a 100644 > > > --- a/src/ipa/rpi/controller/rpi/agc_channel.cpp > > > +++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp > > > @@ -435,14 +435,9 @@ void AgcChannel::switchMode(CameraMode const &cameraMode, > > > > > > Duration fixedExposureTime = limitExposureTime(fixedExposureTime_); > > > if (fixedExposureTime && fixedAnalogueGain_) { > > > - /* We're going to reset the algorithm here with these fixed values. */ > > > - fetchAwbStatus(metadata); > > > - double minColourGain = std::min({ awb_.gainR, awb_.gainG, awb_.gainB, 1.0 }); > > > - ASSERT(minColourGain != 0.0); > > > - > > > /* This is the equivalent of computeTargetExposure and applyDigitalGain. */ > > > target_.totalExposureNoDG = fixedExposureTime_ * fixedAnalogueGain_; > > > - target_.totalExposure = target_.totalExposureNoDG / minColourGain; > > > + target_.totalExposure = target_.totalExposureNoDG; > > > > > > /* Equivalent of filterExposure. This resets any "history". */ > > > filtered_ = target_; > > > @@ -462,10 +457,10 @@ void AgcChannel::switchMode(CameraMode const &cameraMode, > > > */ > > > > > > double ratio = lastSensitivity / cameraMode.sensitivity; > > > - target_.totalExposureNoDG *= ratio; > > > target_.totalExposure *= ratio; > > > - filtered_.totalExposureNoDG *= ratio; > > > + target_.totalExposureNoDG = target_.totalExposure; > > > filtered_.totalExposure *= ratio; > > > + filtered_.totalExposureNoDG = filtered_.totalExposure; > > > > > > divideUpExposure(); > > > } else { > > > @@ -716,8 +711,13 @@ static double computeInitialY(StatisticsPtr &stats, AwbStatus const &awb, > > > } > > > > > > /* Factor in the AWB correction if needed. */ > > > - if (stats->agcStatsPos == Statistics::AgcStatsPos::PreWb) > > > - sum *= RGB<double>{ { awb.gainR, awb.gainG, awb.gainB } }; > > > + if (stats->agcStatsPos == Statistics::AgcStatsPos::PreWb) { > > > + double minColourGain = std::min({ awb.gainR, awb.gainG, awb.gainB, 1.0 }); > > > + minColourGain = std::max(minColourGain, 1.0); > > > + RGB<double> colourGains{ { awb.gainR, awb.gainG, awb.gainB } }; > > > + colourGains /= minColourGain; > > > + sum *= colourGains; > > > + } > > > > > > double ySum = ipa::rec601LuminanceFromRGB(sum); > > > > > > @@ -797,16 +797,8 @@ void AgcChannel::computeGain(StatisticsPtr &statistics, Metadata *imageMetadata, > > > void AgcChannel::computeTargetExposure(double gain) > > > { > > > if (status_.fixedExposureTime && status_.fixedAnalogueGain) { > > > - /* > > > - * When analogue gain and exposure time are both fixed, we need > > > - * to drive the total exposure so that we end up with a digital > > > - * gain of at least 1/minColourGain. Otherwise we'd desaturate > > > - * channels causing white to go cyan or magenta. > > > - */ > > > - double minColourGain = std::min({ awb_.gainR, awb_.gainG, awb_.gainB, 1.0 }); > > > - ASSERT(minColourGain != 0.0); > > > target_.totalExposure = > > > - status_.fixedExposureTime * status_.fixedAnalogueGain / minColourGain; > > > + status_.fixedExposureTime * status_.fixedAnalogueGain; > > > } else { > > > /* > > > * The statistics reflect the image without digital gain, so the final > > > @@ -867,15 +859,8 @@ bool AgcChannel::applyChannelConstraints(const AgcChannelTotalExposures &channel > > > > > > bool AgcChannel::applyDigitalGain(double gain, double targetY, bool channelBound) > > > { > > > - double minColourGain = std::min({ awb_.gainR, awb_.gainG, awb_.gainB, 1.0 }); > > > - ASSERT(minColourGain != 0.0); > > > - double dg = 1.0 / minColourGain; > > > - /* > > > - * I think this pipeline subtracts black level and rescales before we > > > - * get the stats, so no need to worry about it. > > > - */ > > > - LOG(RPiAgc, Debug) << "after AWB, target dg " << dg << " gain " << gain > > > - << " target_Y " << targetY; > > > + double dg = 1.0; > > > + > > > /* > > > * Finally, if we're trying to reduce exposure but the target_Y is > > > * "close" to 1.0, then the gain computed for that constraint will be > > > diff --git a/src/ipa/rpi/pisp/pisp.cpp b/src/ipa/rpi/pisp/pisp.cpp > > > index bb50a9e05904..e1a804f533bb 100644 > > > --- a/src/ipa/rpi/pisp/pisp.cpp > > > +++ b/src/ipa/rpi/pisp/pisp.cpp > > > @@ -521,10 +521,24 @@ void IpaPiSP::applyWBG(const AwbStatus *awbStatus, const AgcPrepareStatus *agcPr > > > pisp_wbg_config wbg; > > > pisp_fe_rgby_config rgby = {}; > > > double dg = agcPrepareStatus ? agcPrepareStatus->digitalGain : 1.0; > > > + double minColourGain = std::min({ awbStatus->gainR, awbStatus->gainG, awbStatus->gainB, 1.0 }); > > > + /* The 0.1 here doesn't mean much, but just stops arithmetic errors and extreme behaviour. */ > > > + double extraGain = 1.0 / std::max({ minColourGain, 0.1 }); > > > > > > - wbg.gain_r = clampField(dg * awbStatus->gainR, 14, 10); > > > - wbg.gain_g = clampField(dg * awbStatus->gainG, 14, 10); > > > - wbg.gain_b = clampField(dg * awbStatus->gainB, 14, 10); > > > + /* > > > + * Apply an extra gain of 1 / minColourGain so as not to apply < 1 gains to any > > > + * channels (which would cause saturated pixels to go cyan or magenta). > > > + * Doing this doesn't really apply more gain than necessary, because one of the > > > + * channels is always getting the minimum gain possible. For this reason we also > > > + * don't change the values that we report externally. > > > + */ > > > + double gainR = awbStatus->gainR * extraGain; > > > + double gainG = awbStatus->gainG * extraGain; > > > + double gainB = awbStatus->gainB * extraGain; > > > + > > > + wbg.gain_r = clampField(dg * gainR, 14, 10); > > > + wbg.gain_g = clampField(dg * gainG, 14, 10); > > > + wbg.gain_b = clampField(dg * gainB, 14, 10); > > > > > > /* > > > * The YCbCr conversion block should contain the appropriate YCbCr > > > @@ -535,9 +549,9 @@ void IpaPiSP::applyWBG(const AwbStatus *awbStatus, const AgcPrepareStatus *agcPr > > > be_->GetYcbcr(csc); > > > > > > /* The CSC coefficients already have the << 10 scaling applied. */ > > > - rgby.gain_r = clampField(csc.coeffs[0] * awbStatus->gainR, 14); > > > - rgby.gain_g = clampField(csc.coeffs[1] * awbStatus->gainG, 14); > > > - rgby.gain_b = clampField(csc.coeffs[2] * awbStatus->gainB, 14); > > > + rgby.gain_r = clampField(csc.coeffs[0] * gainR, 14); > > > + rgby.gain_g = clampField(csc.coeffs[1] * gainG, 14); > > > + rgby.gain_b = clampField(csc.coeffs[2] * gainB, 14); > > > > > > LOG(IPARPI, Debug) << "Applying WB R: " << awbStatus->gainR << " B: " > > > << awbStatus->gainB; > > > diff --git a/src/ipa/rpi/vc4/vc4.cpp b/src/ipa/rpi/vc4/vc4.cpp > > > index ba43e4741584..8a7a37c870ed 100644 > > > --- a/src/ipa/rpi/vc4/vc4.cpp > > > +++ b/src/ipa/rpi/vc4/vc4.cpp > > > @@ -63,7 +63,8 @@ private: > > > bool validateIspControls(); > > > > > > void applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls); > > > - void applyDG(const struct AgcPrepareStatus *dgStatus, ControlList &ctrls); > > > + void applyDG(const struct AgcPrepareStatus *dgStatus, > > > + const struct AwbStatus *awbStatus, ControlList &ctrls); > > > void applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls); > > > void applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, ControlList &ctrls); > > > void applyGamma(const struct ContrastStatus *contrastStatus, ControlList &ctrls); > > > @@ -152,8 +153,7 @@ void IpaVc4::platformPrepareIsp([[maybe_unused]] const PrepareParams ¶ms, > > > applyCCM(ccmStatus, ctrls); > > > > > > AgcPrepareStatus *dgStatus = rpiMetadata.getLocked<AgcPrepareStatus>("agc.prepare_status"); > > > - if (dgStatus) > > > - applyDG(dgStatus, ctrls); > > > + applyDG(dgStatus, awbStatus, ctrls); > > > > > > AlscStatus *lsStatus = rpiMetadata.getLocked<AlscStatus>("alsc.status"); > > > if (lsStatus) > > > @@ -329,10 +329,26 @@ void IpaVc4::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls) > > > static_cast<int32_t>(awbStatus->gainB * 1000)); > > > } > > > > > > -void IpaVc4::applyDG(const struct AgcPrepareStatus *dgStatus, ControlList &ctrls) > > > +void IpaVc4::applyDG(const struct AgcPrepareStatus *dgStatus, > > > + const struct AwbStatus *awbStatus, ControlList &ctrls) > > > { > > > + double digitalGain = dgStatus ? dgStatus->digitalGain : 1.0; > > > + > > > + if (awbStatus) { > > > + /* > > > + * We must apply sufficient extra digital gain to stop any of the channel gains being > > > + * less than 1, which would cause saturation artifacts. Note that one of the colour > > > + * channels is still getting the minimum possible gain, so it's not a "real" gain > > > + * increase. > > > + */ > > > + double minColourGain = std::min({ awbStatus->gainR, awbStatus->gainG, awbStatus->gainB, 1.0 }); > > > + /* The 0.1 here doesn't mean much, but just stops arithmetic errors and extreme behaviour. */ > > > + double extraGain = 1.0 / std::max({ minColourGain, 0.1 }); > > > + digitalGain *= extraGain; > > > + } > > > + > > > ctrls.set(V4L2_CID_DIGITAL_GAIN, > > > - static_cast<int32_t>(dgStatus->digitalGain * 1000)); > > > + static_cast<int32_t>(digitalGain * 1000)); > > > } > > > > > > void IpaVc4::applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls)
diff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp b/src/ipa/rpi/controller/rpi/agc_channel.cpp index b2999364bf27..ae0cb148893a 100644 --- a/src/ipa/rpi/controller/rpi/agc_channel.cpp +++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp @@ -435,14 +435,9 @@ void AgcChannel::switchMode(CameraMode const &cameraMode, Duration fixedExposureTime = limitExposureTime(fixedExposureTime_); if (fixedExposureTime && fixedAnalogueGain_) { - /* We're going to reset the algorithm here with these fixed values. */ - fetchAwbStatus(metadata); - double minColourGain = std::min({ awb_.gainR, awb_.gainG, awb_.gainB, 1.0 }); - ASSERT(minColourGain != 0.0); - /* This is the equivalent of computeTargetExposure and applyDigitalGain. */ target_.totalExposureNoDG = fixedExposureTime_ * fixedAnalogueGain_; - target_.totalExposure = target_.totalExposureNoDG / minColourGain; + target_.totalExposure = target_.totalExposureNoDG; /* Equivalent of filterExposure. This resets any "history". */ filtered_ = target_; @@ -462,10 +457,10 @@ void AgcChannel::switchMode(CameraMode const &cameraMode, */ double ratio = lastSensitivity / cameraMode.sensitivity; - target_.totalExposureNoDG *= ratio; target_.totalExposure *= ratio; - filtered_.totalExposureNoDG *= ratio; + target_.totalExposureNoDG = target_.totalExposure; filtered_.totalExposure *= ratio; + filtered_.totalExposureNoDG = filtered_.totalExposure; divideUpExposure(); } else { @@ -716,8 +711,13 @@ static double computeInitialY(StatisticsPtr &stats, AwbStatus const &awb, } /* Factor in the AWB correction if needed. */ - if (stats->agcStatsPos == Statistics::AgcStatsPos::PreWb) - sum *= RGB<double>{ { awb.gainR, awb.gainG, awb.gainB } }; + if (stats->agcStatsPos == Statistics::AgcStatsPos::PreWb) { + double minColourGain = std::min({ awb.gainR, awb.gainG, awb.gainB, 1.0 }); + minColourGain = std::max(minColourGain, 1.0); + RGB<double> colourGains{ { awb.gainR, awb.gainG, awb.gainB } }; + colourGains /= minColourGain; + sum *= colourGains; + } double ySum = ipa::rec601LuminanceFromRGB(sum); @@ -797,16 +797,8 @@ void AgcChannel::computeGain(StatisticsPtr &statistics, Metadata *imageMetadata, void AgcChannel::computeTargetExposure(double gain) { if (status_.fixedExposureTime && status_.fixedAnalogueGain) { - /* - * When analogue gain and exposure time are both fixed, we need - * to drive the total exposure so that we end up with a digital - * gain of at least 1/minColourGain. Otherwise we'd desaturate - * channels causing white to go cyan or magenta. - */ - double minColourGain = std::min({ awb_.gainR, awb_.gainG, awb_.gainB, 1.0 }); - ASSERT(minColourGain != 0.0); target_.totalExposure = - status_.fixedExposureTime * status_.fixedAnalogueGain / minColourGain; + status_.fixedExposureTime * status_.fixedAnalogueGain; } else { /* * The statistics reflect the image without digital gain, so the final @@ -867,15 +859,8 @@ bool AgcChannel::applyChannelConstraints(const AgcChannelTotalExposures &channel bool AgcChannel::applyDigitalGain(double gain, double targetY, bool channelBound) { - double minColourGain = std::min({ awb_.gainR, awb_.gainG, awb_.gainB, 1.0 }); - ASSERT(minColourGain != 0.0); - double dg = 1.0 / minColourGain; - /* - * I think this pipeline subtracts black level and rescales before we - * get the stats, so no need to worry about it. - */ - LOG(RPiAgc, Debug) << "after AWB, target dg " << dg << " gain " << gain - << " target_Y " << targetY; + double dg = 1.0; + /* * Finally, if we're trying to reduce exposure but the target_Y is * "close" to 1.0, then the gain computed for that constraint will be diff --git a/src/ipa/rpi/pisp/pisp.cpp b/src/ipa/rpi/pisp/pisp.cpp index bb50a9e05904..e1a804f533bb 100644 --- a/src/ipa/rpi/pisp/pisp.cpp +++ b/src/ipa/rpi/pisp/pisp.cpp @@ -521,10 +521,24 @@ void IpaPiSP::applyWBG(const AwbStatus *awbStatus, const AgcPrepareStatus *agcPr pisp_wbg_config wbg; pisp_fe_rgby_config rgby = {}; double dg = agcPrepareStatus ? agcPrepareStatus->digitalGain : 1.0; + double minColourGain = std::min({ awbStatus->gainR, awbStatus->gainG, awbStatus->gainB, 1.0 }); + /* The 0.1 here doesn't mean much, but just stops arithmetic errors and extreme behaviour. */ + double extraGain = 1.0 / std::max({ minColourGain, 0.1 }); - wbg.gain_r = clampField(dg * awbStatus->gainR, 14, 10); - wbg.gain_g = clampField(dg * awbStatus->gainG, 14, 10); - wbg.gain_b = clampField(dg * awbStatus->gainB, 14, 10); + /* + * Apply an extra gain of 1 / minColourGain so as not to apply < 1 gains to any + * channels (which would cause saturated pixels to go cyan or magenta). + * Doing this doesn't really apply more gain than necessary, because one of the + * channels is always getting the minimum gain possible. For this reason we also + * don't change the values that we report externally. + */ + double gainR = awbStatus->gainR * extraGain; + double gainG = awbStatus->gainG * extraGain; + double gainB = awbStatus->gainB * extraGain; + + wbg.gain_r = clampField(dg * gainR, 14, 10); + wbg.gain_g = clampField(dg * gainG, 14, 10); + wbg.gain_b = clampField(dg * gainB, 14, 10); /* * The YCbCr conversion block should contain the appropriate YCbCr @@ -535,9 +549,9 @@ void IpaPiSP::applyWBG(const AwbStatus *awbStatus, const AgcPrepareStatus *agcPr be_->GetYcbcr(csc); /* The CSC coefficients already have the << 10 scaling applied. */ - rgby.gain_r = clampField(csc.coeffs[0] * awbStatus->gainR, 14); - rgby.gain_g = clampField(csc.coeffs[1] * awbStatus->gainG, 14); - rgby.gain_b = clampField(csc.coeffs[2] * awbStatus->gainB, 14); + rgby.gain_r = clampField(csc.coeffs[0] * gainR, 14); + rgby.gain_g = clampField(csc.coeffs[1] * gainG, 14); + rgby.gain_b = clampField(csc.coeffs[2] * gainB, 14); LOG(IPARPI, Debug) << "Applying WB R: " << awbStatus->gainR << " B: " << awbStatus->gainB; diff --git a/src/ipa/rpi/vc4/vc4.cpp b/src/ipa/rpi/vc4/vc4.cpp index ba43e4741584..8a7a37c870ed 100644 --- a/src/ipa/rpi/vc4/vc4.cpp +++ b/src/ipa/rpi/vc4/vc4.cpp @@ -63,7 +63,8 @@ private: bool validateIspControls(); void applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls); - void applyDG(const struct AgcPrepareStatus *dgStatus, ControlList &ctrls); + void applyDG(const struct AgcPrepareStatus *dgStatus, + const struct AwbStatus *awbStatus, ControlList &ctrls); void applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls); void applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, ControlList &ctrls); void applyGamma(const struct ContrastStatus *contrastStatus, ControlList &ctrls); @@ -152,8 +153,7 @@ void IpaVc4::platformPrepareIsp([[maybe_unused]] const PrepareParams ¶ms, applyCCM(ccmStatus, ctrls); AgcPrepareStatus *dgStatus = rpiMetadata.getLocked<AgcPrepareStatus>("agc.prepare_status"); - if (dgStatus) - applyDG(dgStatus, ctrls); + applyDG(dgStatus, awbStatus, ctrls); AlscStatus *lsStatus = rpiMetadata.getLocked<AlscStatus>("alsc.status"); if (lsStatus) @@ -329,10 +329,26 @@ void IpaVc4::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls) static_cast<int32_t>(awbStatus->gainB * 1000)); } -void IpaVc4::applyDG(const struct AgcPrepareStatus *dgStatus, ControlList &ctrls) +void IpaVc4::applyDG(const struct AgcPrepareStatus *dgStatus, + const struct AwbStatus *awbStatus, ControlList &ctrls) { + double digitalGain = dgStatus ? dgStatus->digitalGain : 1.0; + + if (awbStatus) { + /* + * We must apply sufficient extra digital gain to stop any of the channel gains being + * less than 1, which would cause saturation artifacts. Note that one of the colour + * channels is still getting the minimum possible gain, so it's not a "real" gain + * increase. + */ + double minColourGain = std::min({ awbStatus->gainR, awbStatus->gainG, awbStatus->gainB, 1.0 }); + /* The 0.1 here doesn't mean much, but just stops arithmetic errors and extreme behaviour. */ + double extraGain = 1.0 / std::max({ minColourGain, 0.1 }); + digitalGain *= extraGain; + } + ctrls.set(V4L2_CID_DIGITAL_GAIN, - static_cast<int32_t>(dgStatus->digitalGain * 1000)); + static_cast<int32_t>(digitalGain * 1000)); } void IpaVc4::applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls)