| Message ID | 20250617082956.5699-2-david.plowman@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi David, Quoting David Plowman (2025-06-17 09:29:49) > 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. > > Signed-off-by: David Plowman <david.plowman@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 a5562760..a87dc194 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.gainR, awb.gainB } }; There's a merge conflict here because we've fixed the above to be RGB insted of RRB. > + 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.gainR, awb.gainB } }; Which I have applied manually as a correction to this line ^ but unfortunately - [PATCH 4/7] ipa: rpi: Advance the delay context counter even when IPAs don't run isn't applying at all for me. It might be safer for me to ask you to rebase this series please? Then I can push it through CI and merge. -- Regards Kieran
Hi David, Naush, Quoting Kieran Bingham (2025-07-10 17:04:31) > Hi David, > > Quoting David Plowman (2025-06-17 09:29:49) > > 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. > > > > Signed-off-by: David Plowman <david.plowman@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 a5562760..a87dc194 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.gainR, awb.gainB } }; > > There's a merge conflict here because we've fixed the above to be RGB > insted of RRB. > > > + 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.gainR, awb.gainB } }; > > Which I have applied manually as a correction to this line ^ > > but unfortunately - [PATCH 4/7] ipa: rpi: Advance the delay context > counter even when IPAs don't run isn't applying at all for me. > > It might be safer for me to ask you to rebase this series please? Then I > can push it through CI and merge. Do either of you have a rebased version of this I can push through CI? -- Regards Kieran
diff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp b/src/ipa/rpi/controller/rpi/agc_channel.cpp index a5562760..a87dc194 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.gainR, 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.gainR, 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 bb50a9e0..e1a804f5 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 ba43e474..8a7a37c8 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)
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. Signed-off-by: David Plowman <david.plowman@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(-)