| Message ID | 20230728133700.3713-3-david.plowman@raspberrypi.com |
|---|---|
| State | Accepted |
| Commit | 84b6327789fcf5be37a990d5be27f305e8514621 |
| Headers | show |
| Series |
|
| Related | show |
Hi David, Nice simplification! On Fri, 28 Jul 2023 at 14:37, David Plowman via libcamera-devel <libcamera-devel@lists.libcamera.org> wrote: > > We now time-filter the exposure before sorting out how much digital > gain is required. This is actually a little more natural and > simplifies the code. It also prepares us for some future work where > this arrangement will be helpful. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> Reviewed-by: Naushir Patuck <naush@raspberrpyi.com> > --- > src/ipa/rpi/controller/rpi/agc.cpp | 30 +++++------------------------- > src/ipa/rpi/controller/rpi/agc.h | 2 +- > 2 files changed, 6 insertions(+), 26 deletions(-) > > diff --git a/src/ipa/rpi/controller/rpi/agc.cpp b/src/ipa/rpi/controller/rpi/agc.cpp > index e8526355..6087fc60 100644 > --- a/src/ipa/rpi/controller/rpi/agc.cpp > +++ b/src/ipa/rpi/controller/rpi/agc.cpp > @@ -469,14 +469,14 @@ void Agc::process(StatisticsPtr &stats, Metadata *imageMetadata) > computeGain(stats, imageMetadata, gain, targetY); > /* Now compute the target (final) exposure which we think we want. */ > computeTargetExposure(gain); > + /* The results have to be filtered so as not to change too rapidly. */ > + filterExposure(); > /* > * Some of the exposure has to be applied as digital gain, so work out > * what that is. This function also tells us whether it's decided to > * "desaturate" the image more quickly. > */ > bool desaturate = applyDigitalGain(gain, targetY); > - /* The results have to be filtered so as not to change too rapidly. */ > - filterExposure(desaturate); > /* > * The last thing is to divide up the exposure value into a shutter time > * and analogue gain, according to the current exposure mode. > @@ -757,12 +757,12 @@ bool Agc::applyDigitalGain(double gain, double targetY) > if (desaturate) > dg /= config_.fastReduceThreshold; > LOG(RPiAgc, Debug) << "Digital gain " << dg << " desaturate? " << desaturate; > - target_.totalExposureNoDG = target_.totalExposure / dg; > - LOG(RPiAgc, Debug) << "Target totalExposureNoDG " << target_.totalExposureNoDG; > + filtered_.totalExposureNoDG = filtered_.totalExposure / dg; > + LOG(RPiAgc, Debug) << "Target totalExposureNoDG " << filtered_.totalExposureNoDG; > return desaturate; > } > > -void Agc::filterExposure(bool desaturate) > +void Agc::filterExposure() > { > double speed = config_.speed; > /* > @@ -774,7 +774,6 @@ void Agc::filterExposure(bool desaturate) > speed = 1.0; > if (!filtered_.totalExposure) { > filtered_.totalExposure = target_.totalExposure; > - filtered_.totalExposureNoDG = target_.totalExposureNoDG; > } else { > /* > * If close to the result go faster, to save making so many > @@ -785,26 +784,7 @@ void Agc::filterExposure(bool desaturate) > speed = sqrt(speed); > filtered_.totalExposure = speed * target_.totalExposure + > filtered_.totalExposure * (1.0 - speed); > - /* > - * When desaturing, take a big jump down in totalExposureNoDG, > - * which we'll hide with digital gain. > - */ > - if (desaturate) > - filtered_.totalExposureNoDG = > - target_.totalExposureNoDG; > - else > - filtered_.totalExposureNoDG = > - speed * target_.totalExposureNoDG + > - filtered_.totalExposureNoDG * (1.0 - speed); > } > - /* > - * We can't let the totalExposureNoDG exposure deviate too far below the > - * total exposure, as there might not be enough digital gain available > - * in the ISP to hide it (which will cause nasty oscillation). > - */ > - if (filtered_.totalExposureNoDG < > - filtered_.totalExposure * config_.fastReduceThreshold) > - filtered_.totalExposureNoDG = filtered_.totalExposure * config_.fastReduceThreshold; > LOG(RPiAgc, Debug) << "After filtering, totalExposure " << filtered_.totalExposure > << " no dg " << filtered_.totalExposureNoDG; > } > diff --git a/src/ipa/rpi/controller/rpi/agc.h b/src/ipa/rpi/controller/rpi/agc.h > index 939f9729..b7122de3 100644 > --- a/src/ipa/rpi/controller/rpi/agc.h > +++ b/src/ipa/rpi/controller/rpi/agc.h > @@ -93,8 +93,8 @@ private: > void computeGain(StatisticsPtr &statistics, Metadata *imageMetadata, > double &gain, double &targetY); > void computeTargetExposure(double gain); > + void filterExposure(); > bool applyDigitalGain(double gain, double targetY); > - void filterExposure(bool desaturate); > void divideUpExposure(); > void writeAndFinish(Metadata *imageMetadata, bool desaturate); > libcamera::utils::Duration limitShutter(libcamera::utils::Duration shutter); > -- > 2.30.2 >
Hi David On Fri, Jul 28, 2023 at 02:36:59PM +0100, David Plowman via libcamera-devel wrote: > We now time-filter the exposure before sorting out how much digital > gain is required. This is actually a little more natural and > simplifies the code. It also prepares us for some future work where > this arrangement will be helpful. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > --- > src/ipa/rpi/controller/rpi/agc.cpp | 30 +++++------------------------- > src/ipa/rpi/controller/rpi/agc.h | 2 +- > 2 files changed, 6 insertions(+), 26 deletions(-) > > diff --git a/src/ipa/rpi/controller/rpi/agc.cpp b/src/ipa/rpi/controller/rpi/agc.cpp > index e8526355..6087fc60 100644 > --- a/src/ipa/rpi/controller/rpi/agc.cpp > +++ b/src/ipa/rpi/controller/rpi/agc.cpp > @@ -469,14 +469,14 @@ void Agc::process(StatisticsPtr &stats, Metadata *imageMetadata) > computeGain(stats, imageMetadata, gain, targetY); > /* Now compute the target (final) exposure which we think we want. */ > computeTargetExposure(gain); > + /* The results have to be filtered so as not to change too rapidly. */ > + filterExposure(); > /* > * Some of the exposure has to be applied as digital gain, so work out > * what that is. This function also tells us whether it's decided to > * "desaturate" the image more quickly. > */ > bool desaturate = applyDigitalGain(gain, targetY); > - /* The results have to be filtered so as not to change too rapidly. */ > - filterExposure(desaturate); > /* > * The last thing is to divide up the exposure value into a shutter time > * and analogue gain, according to the current exposure mode. > @@ -757,12 +757,12 @@ bool Agc::applyDigitalGain(double gain, double targetY) > if (desaturate) > dg /= config_.fastReduceThreshold; > LOG(RPiAgc, Debug) << "Digital gain " << dg << " desaturate? " << desaturate; > - target_.totalExposureNoDG = target_.totalExposure / dg; > - LOG(RPiAgc, Debug) << "Target totalExposureNoDG " << target_.totalExposureNoDG; > + filtered_.totalExposureNoDG = filtered_.totalExposure / dg; > + LOG(RPiAgc, Debug) << "Target totalExposureNoDG " << filtered_.totalExposureNoDG; > return desaturate; > } > > -void Agc::filterExposure(bool desaturate) > +void Agc::filterExposure() > { > double speed = config_.speed; > /* > @@ -774,7 +774,6 @@ void Agc::filterExposure(bool desaturate) > speed = 1.0; > if (!filtered_.totalExposure) { > filtered_.totalExposure = target_.totalExposure; > - filtered_.totalExposureNoDG = target_.totalExposureNoDG; > } else { > /* > * If close to the result go faster, to save making so many > @@ -785,26 +784,7 @@ void Agc::filterExposure(bool desaturate) > speed = sqrt(speed); > filtered_.totalExposure = speed * target_.totalExposure + > filtered_.totalExposure * (1.0 - speed); > - /* > - * When desaturing, take a big jump down in totalExposureNoDG, > - * which we'll hide with digital gain. > - */ > - if (desaturate) > - filtered_.totalExposureNoDG = > - target_.totalExposureNoDG; > - else > - filtered_.totalExposureNoDG = > - speed * target_.totalExposureNoDG + > - filtered_.totalExposureNoDG * (1.0 - speed); > } > - /* > - * We can't let the totalExposureNoDG exposure deviate too far below the > - * total exposure, as there might not be enough digital gain available > - * in the ISP to hide it (which will cause nasty oscillation). > - */ > - if (filtered_.totalExposureNoDG < > - filtered_.totalExposure * config_.fastReduceThreshold) > - filtered_.totalExposureNoDG = filtered_.totalExposure * config_.fastReduceThreshold; I won't pretend I've fully understood why this check can now be dropped, but I the flow seems indeed more natural Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > LOG(RPiAgc, Debug) << "After filtering, totalExposure " << filtered_.totalExposure > << " no dg " << filtered_.totalExposureNoDG; > } > diff --git a/src/ipa/rpi/controller/rpi/agc.h b/src/ipa/rpi/controller/rpi/agc.h > index 939f9729..b7122de3 100644 > --- a/src/ipa/rpi/controller/rpi/agc.h > +++ b/src/ipa/rpi/controller/rpi/agc.h > @@ -93,8 +93,8 @@ private: > void computeGain(StatisticsPtr &statistics, Metadata *imageMetadata, > double &gain, double &targetY); > void computeTargetExposure(double gain); > + void filterExposure(); > bool applyDigitalGain(double gain, double targetY); > - void filterExposure(bool desaturate); > void divideUpExposure(); > void writeAndFinish(Metadata *imageMetadata, bool desaturate); > libcamera::utils::Duration limitShutter(libcamera::utils::Duration shutter); > -- > 2.30.2 >
diff --git a/src/ipa/rpi/controller/rpi/agc.cpp b/src/ipa/rpi/controller/rpi/agc.cpp index e8526355..6087fc60 100644 --- a/src/ipa/rpi/controller/rpi/agc.cpp +++ b/src/ipa/rpi/controller/rpi/agc.cpp @@ -469,14 +469,14 @@ void Agc::process(StatisticsPtr &stats, Metadata *imageMetadata) computeGain(stats, imageMetadata, gain, targetY); /* Now compute the target (final) exposure which we think we want. */ computeTargetExposure(gain); + /* The results have to be filtered so as not to change too rapidly. */ + filterExposure(); /* * Some of the exposure has to be applied as digital gain, so work out * what that is. This function also tells us whether it's decided to * "desaturate" the image more quickly. */ bool desaturate = applyDigitalGain(gain, targetY); - /* The results have to be filtered so as not to change too rapidly. */ - filterExposure(desaturate); /* * The last thing is to divide up the exposure value into a shutter time * and analogue gain, according to the current exposure mode. @@ -757,12 +757,12 @@ bool Agc::applyDigitalGain(double gain, double targetY) if (desaturate) dg /= config_.fastReduceThreshold; LOG(RPiAgc, Debug) << "Digital gain " << dg << " desaturate? " << desaturate; - target_.totalExposureNoDG = target_.totalExposure / dg; - LOG(RPiAgc, Debug) << "Target totalExposureNoDG " << target_.totalExposureNoDG; + filtered_.totalExposureNoDG = filtered_.totalExposure / dg; + LOG(RPiAgc, Debug) << "Target totalExposureNoDG " << filtered_.totalExposureNoDG; return desaturate; } -void Agc::filterExposure(bool desaturate) +void Agc::filterExposure() { double speed = config_.speed; /* @@ -774,7 +774,6 @@ void Agc::filterExposure(bool desaturate) speed = 1.0; if (!filtered_.totalExposure) { filtered_.totalExposure = target_.totalExposure; - filtered_.totalExposureNoDG = target_.totalExposureNoDG; } else { /* * If close to the result go faster, to save making so many @@ -785,26 +784,7 @@ void Agc::filterExposure(bool desaturate) speed = sqrt(speed); filtered_.totalExposure = speed * target_.totalExposure + filtered_.totalExposure * (1.0 - speed); - /* - * When desaturing, take a big jump down in totalExposureNoDG, - * which we'll hide with digital gain. - */ - if (desaturate) - filtered_.totalExposureNoDG = - target_.totalExposureNoDG; - else - filtered_.totalExposureNoDG = - speed * target_.totalExposureNoDG + - filtered_.totalExposureNoDG * (1.0 - speed); } - /* - * We can't let the totalExposureNoDG exposure deviate too far below the - * total exposure, as there might not be enough digital gain available - * in the ISP to hide it (which will cause nasty oscillation). - */ - if (filtered_.totalExposureNoDG < - filtered_.totalExposure * config_.fastReduceThreshold) - filtered_.totalExposureNoDG = filtered_.totalExposure * config_.fastReduceThreshold; LOG(RPiAgc, Debug) << "After filtering, totalExposure " << filtered_.totalExposure << " no dg " << filtered_.totalExposureNoDG; } diff --git a/src/ipa/rpi/controller/rpi/agc.h b/src/ipa/rpi/controller/rpi/agc.h index 939f9729..b7122de3 100644 --- a/src/ipa/rpi/controller/rpi/agc.h +++ b/src/ipa/rpi/controller/rpi/agc.h @@ -93,8 +93,8 @@ private: void computeGain(StatisticsPtr &statistics, Metadata *imageMetadata, double &gain, double &targetY); void computeTargetExposure(double gain); + void filterExposure(); bool applyDigitalGain(double gain, double targetY); - void filterExposure(bool desaturate); void divideUpExposure(); void writeAndFinish(Metadata *imageMetadata, bool desaturate); libcamera::utils::Duration limitShutter(libcamera::utils::Duration shutter);
We now time-filter the exposure before sorting out how much digital gain is required. This is actually a little more natural and simplifies the code. It also prepares us for some future work where this arrangement will be helpful. Signed-off-by: David Plowman <david.plowman@raspberrypi.com> --- src/ipa/rpi/controller/rpi/agc.cpp | 30 +++++------------------------- src/ipa/rpi/controller/rpi/agc.h | 2 +- 2 files changed, 6 insertions(+), 26 deletions(-)