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{ { 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 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("agc.prepare_status"); - if (dgStatus) - applyDG(dgStatus, ctrls); + applyDG(dgStatus, awbStatus, ctrls); AlscStatus *lsStatus = rpiMetadata.getLocked("alsc.status"); if (lsStatus) @@ -329,10 +329,26 @@ void IpaVc4::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls) static_cast(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(dgStatus->digitalGain * 1000)); + static_cast(digitalGain * 1000)); } void IpaVc4::applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls)