[v3,2/3] ipa: libipa: agc: Change luminance target to piecewise linear function
diff mbox series

Message ID 20240517080802.3896531-3-paul.elder@ideasonboard.com
State Superseded
Headers show
Series
  • ipa: rkisp1: Improve AGC (plumbing)
Related show

Commit Message

Paul Elder May 17, 2024, 8:08 a.m. UTC 
Change the relative luminance target from a scalar value to a piecewise
linear function that needs to be sampled by the estimate lux value.

Also change the rkisp1 and ipu3 IPAs accordingly, as they use the libipa
agc. As they both don't yet have lux modules, hardcode them to a single
lux value for now.

This affects the format of the tuning files, but as there aren't yet any
this shouldn't be an issue.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>

---
No change in v3

Changes in v2:
- s/FPoint/PointF/
- add warning when using default relative luminance target when loading
  from the tuning file fails
---
 src/ipa/ipu3/algorithms/agc.cpp       |  5 ++++-
 src/ipa/libipa/agc_mean_luminance.cpp | 31 +++++++++++++++++++++------
 src/ipa/libipa/agc_mean_luminance.h   |  7 +++---
 src/ipa/rkisp1/algorithms/agc.cpp     |  5 ++++-
 4 files changed, 36 insertions(+), 12 deletions(-)

Comments

Dan Scally May 17, 2024, 10:29 a.m. UTC | #1 
Hi Paul

On 17/05/2024 10:08, Paul Elder wrote:
> Change the relative luminance target from a scalar value to a piecewise
> linear function that needs to be sampled by the estimate lux value.
>
> Also change the rkisp1 and ipu3 IPAs accordingly, as they use the libipa
> agc. As they both don't yet have lux modules, hardcode them to a single
> lux value for now.
>
> This affects the format of the tuning files, but as there aren't yet any
> this shouldn't be an issue.
>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>


Thanks for the patch, this looks good to me. We need to remember the same thing for the constraint 
target though I guess.


Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>

> ---
> No change in v3
>
> Changes in v2:
> - s/FPoint/PointF/
> - add warning when using default relative luminance target when loading
>    from the tuning file fails
> ---
>   src/ipa/ipu3/algorithms/agc.cpp       |  5 ++++-
>   src/ipa/libipa/agc_mean_luminance.cpp | 31 +++++++++++++++++++++------
>   src/ipa/libipa/agc_mean_luminance.h   |  7 +++---
>   src/ipa/rkisp1/algorithms/agc.cpp     |  5 ++++-
>   4 files changed, 36 insertions(+), 12 deletions(-)
>
> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> index c9b5548c4..984ed0874 100644
> --- a/src/ipa/ipu3/algorithms/agc.cpp
> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> @@ -222,12 +222,15 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>   	double analogueGain = frameContext.sensor.gain;
>   	utils::Duration effectiveExposureValue = exposureTime * analogueGain;
>   
> +	/* \todo Plumb in the lux value. Requires a lux algo + tuning */
> +	double lux = 400;
> +
>   	utils::Duration shutterTime;
>   	double aGain, dGain;
>   	std::tie(shutterTime, aGain, dGain) =
>   		calculateNewEv(context.activeState.agc.constraintMode,
>   			       context.activeState.agc.exposureMode, hist,
> -			       effectiveExposureValue);
> +			       effectiveExposureValue, lux);
>   
>   	LOG(IPU3Agc, Debug)
>   		<< "Divided up shutter, analogue gain and digital gain are "
> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
> index 2bf84d05b..fe07777dc 100644
> --- a/src/ipa/libipa/agc_mean_luminance.cpp
> +++ b/src/ipa/libipa/agc_mean_luminance.cpp
> @@ -134,7 +134,7 @@ static constexpr double kDefaultRelativeLuminanceTarget = 0.16;
>    */
>   
>   AgcMeanLuminance::AgcMeanLuminance()
> -	: frameCount_(0), filteredExposure_(0s), relativeLuminanceTarget_(0)
> +	: frameCount_(0), filteredExposure_(0s)
>   {
>   }
>   
> @@ -142,8 +142,16 @@ AgcMeanLuminance::~AgcMeanLuminance() = default;
>   
>   void AgcMeanLuminance::parseRelativeLuminanceTarget(const YamlObject &tuningData)
>   {
> -	relativeLuminanceTarget_ =
> -		tuningData["relativeLuminanceTarget"].get<double>(kDefaultRelativeLuminanceTarget);
> +	int ret = relativeLuminanceTarget_.readYaml(tuningData["relativeLuminanceTarget"]);
> +	if (ret == 0)
> +		return;
> +
> +	LOG(AgcMeanLuminance, Warning)
> +		<< "Failed to load tuning parameter 'relativeLuminanceTarget', "
> +		<< "using default [0, " << kDefaultRelativeLuminanceTarget << "]";
> +
> +	std::vector<PointF> points = { { 0, kDefaultRelativeLuminanceTarget } };
> +	relativeLuminanceTarget_ = Pwl(points);
>   }
>   
>   void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)
> @@ -421,11 +429,18 @@ void AgcMeanLuminance::setLimits(utils::Duration minShutter,
>   /**
>    * \brief Estimate the initial gain needed to achieve a relative luminance
>    * target
> + * \param[in] lux The lux value at which to sample the luminance target pwl
> + *
> + * To account for non-linearity caused by saturation, the value needs to be
> + * estimated in an iterative process, as multiplying by a gain will not increase
> + * the relative luminance by the same factor if some image regions are saturated
> + *
>    * \return The calculated initial gain
>    */
> -double AgcMeanLuminance::estimateInitialGain() const
> +double AgcMeanLuminance::estimateInitialGain(double lux) const
>   {
> -	double yTarget = relativeLuminanceTarget_;
> +	double yTarget =
> +		relativeLuminanceTarget_.eval(relativeLuminanceTarget_.domain().clamp(lux));
>   	double yGain = 1.0;
>   
>   	/*
> @@ -520,6 +535,7 @@ utils::Duration AgcMeanLuminance::filterExposure(utils::Duration exposureValue)
>    * the calculated gain
>    * \param[in] effectiveExposureValue The EV applied to the frame from which the
>    * statistics in use derive
> + * \param[in] lux The lux value at which to sample the luminance target pwl
>    *
>    * Calculate a new exposure value to try to obtain the target. The calculated
>    * exposure value is filtered to prevent rapid changes from frame to frame, and
> @@ -531,7 +547,8 @@ std::tuple<utils::Duration, double, double>
>   AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex,
>   				 uint32_t exposureModeIndex,
>   				 const Histogram &yHist,
> -				 utils::Duration effectiveExposureValue)
> +				 utils::Duration effectiveExposureValue,
> +				 double lux)
>   {
>   	/*
>   	 * The pipeline handler should validate that we have received an allowed
> @@ -540,7 +557,7 @@ AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex,
>   	std::shared_ptr<ExposureModeHelper> exposureModeHelper =
>   		exposureModeHelpers_.at(exposureModeIndex);
>   
> -	double gain = estimateInitialGain();
> +	double gain = estimateInitialGain(lux);
>   	gain = constraintClampGain(constraintModeIndex, yHist, gain);
>   
>   	/*
> diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h
> index 0a81c6d28..6ec2a0dc9 100644
> --- a/src/ipa/libipa/agc_mean_luminance.h
> +++ b/src/ipa/libipa/agc_mean_luminance.h
> @@ -18,6 +18,7 @@
>   
>   #include "exposure_mode_helper.h"
>   #include "histogram.h"
> +#include "pwl.h"
>   
>   namespace libcamera {
>   
> @@ -62,7 +63,7 @@ public:
>   
>   	std::tuple<utils::Duration, double, double>
>   	calculateNewEv(uint32_t constraintModeIndex, uint32_t exposureModeIndex,
> -		       const Histogram &yHist, utils::Duration effectiveExposureValue);
> +		       const Histogram &yHist, utils::Duration effectiveExposureValue, double lux);
>   
>   	void resetFrameCount()
>   	{
> @@ -76,7 +77,7 @@ private:
>   	void parseConstraint(const YamlObject &modeDict, int32_t id);
>   	int parseConstraintModes(const YamlObject &tuningData);
>   	int parseExposureModes(const YamlObject &tuningData);
> -	double estimateInitialGain() const;
> +	double estimateInitialGain(double lux) const;
>   	double constraintClampGain(uint32_t constraintModeIndex,
>   				   const Histogram &hist,
>   				   double gain);
> @@ -84,7 +85,7 @@ private:
>   
>   	uint64_t frameCount_;
>   	utils::Duration filteredExposure_;
> -	double relativeLuminanceTarget_;
> +	Pwl relativeLuminanceTarget_;
>   
>   	std::map<int32_t, std::vector<AgcConstraint>> constraintModes_;
>   	std::map<int32_t, std::shared_ptr<ExposureModeHelper>> exposureModeHelpers_;
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index 4af397bdc..1c9872d02 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -379,12 +379,15 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>   	double analogueGain = frameContext.sensor.gain;
>   	utils::Duration effectiveExposureValue = exposureTime * analogueGain;
>   
> +	/* \todo Plumb in the lux value. Requires a lux algo + tuning */
> +	double lux = 400;
> +
>   	utils::Duration shutterTime;
>   	double aGain, dGain;
>   	std::tie(shutterTime, aGain, dGain) =
>   		calculateNewEv(context.activeState.agc.constraintMode,
>   			       context.activeState.agc.exposureMode,
> -			       hist, effectiveExposureValue);
> +			       hist, effectiveExposureValue, lux);
>   
>   	LOG(RkISP1Agc, Debug)
>   		<< "Divided up shutter, analogue gain and digital gain are "
Paul Elder May 17, 2024, 11:47 a.m. UTC | #2 
On Fri, May 17, 2024 at 12:29:16PM +0200, Dan Scally wrote:
> Hi Paul
> 
> On 17/05/2024 10:08, Paul Elder wrote:
> > Change the relative luminance target from a scalar value to a piecewise
> > linear function that needs to be sampled by the estimate lux value.
> > 
> > Also change the rkisp1 and ipu3 IPAs accordingly, as they use the libipa
> > agc. As they both don't yet have lux modules, hardcode them to a single
> > lux value for now.
> > 
> > This affects the format of the tuning files, but as there aren't yet any
> > this shouldn't be an issue.
> > 
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>
> 
> 
> Thanks for the patch, this looks good to me. We need to remember the same
> thing for the constraint target though I guess.

Yeah I just remembered that I had meant to squash that patch in with
this, but ig I can send it separately.


Thanks,

Paul

> 
> 
> Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
> 
> > ---
> > No change in v3
> > 
> > Changes in v2:
> > - s/FPoint/PointF/
> > - add warning when using default relative luminance target when loading
> >    from the tuning file fails
> > ---
> >   src/ipa/ipu3/algorithms/agc.cpp       |  5 ++++-
> >   src/ipa/libipa/agc_mean_luminance.cpp | 31 +++++++++++++++++++++------
> >   src/ipa/libipa/agc_mean_luminance.h   |  7 +++---
> >   src/ipa/rkisp1/algorithms/agc.cpp     |  5 ++++-
> >   4 files changed, 36 insertions(+), 12 deletions(-)
> > 
> > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> > index c9b5548c4..984ed0874 100644
> > --- a/src/ipa/ipu3/algorithms/agc.cpp
> > +++ b/src/ipa/ipu3/algorithms/agc.cpp
> > @@ -222,12 +222,15 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> >   	double analogueGain = frameContext.sensor.gain;
> >   	utils::Duration effectiveExposureValue = exposureTime * analogueGain;
> > +	/* \todo Plumb in the lux value. Requires a lux algo + tuning */
> > +	double lux = 400;
> > +
> >   	utils::Duration shutterTime;
> >   	double aGain, dGain;
> >   	std::tie(shutterTime, aGain, dGain) =
> >   		calculateNewEv(context.activeState.agc.constraintMode,
> >   			       context.activeState.agc.exposureMode, hist,
> > -			       effectiveExposureValue);
> > +			       effectiveExposureValue, lux);
> >   	LOG(IPU3Agc, Debug)
> >   		<< "Divided up shutter, analogue gain and digital gain are "
> > diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
> > index 2bf84d05b..fe07777dc 100644
> > --- a/src/ipa/libipa/agc_mean_luminance.cpp
> > +++ b/src/ipa/libipa/agc_mean_luminance.cpp
> > @@ -134,7 +134,7 @@ static constexpr double kDefaultRelativeLuminanceTarget = 0.16;
> >    */
> >   AgcMeanLuminance::AgcMeanLuminance()
> > -	: frameCount_(0), filteredExposure_(0s), relativeLuminanceTarget_(0)
> > +	: frameCount_(0), filteredExposure_(0s)
> >   {
> >   }
> > @@ -142,8 +142,16 @@ AgcMeanLuminance::~AgcMeanLuminance() = default;
> >   void AgcMeanLuminance::parseRelativeLuminanceTarget(const YamlObject &tuningData)
> >   {
> > -	relativeLuminanceTarget_ =
> > -		tuningData["relativeLuminanceTarget"].get<double>(kDefaultRelativeLuminanceTarget);
> > +	int ret = relativeLuminanceTarget_.readYaml(tuningData["relativeLuminanceTarget"]);
> > +	if (ret == 0)
> > +		return;
> > +
> > +	LOG(AgcMeanLuminance, Warning)
> > +		<< "Failed to load tuning parameter 'relativeLuminanceTarget', "
> > +		<< "using default [0, " << kDefaultRelativeLuminanceTarget << "]";
> > +
> > +	std::vector<PointF> points = { { 0, kDefaultRelativeLuminanceTarget } };
> > +	relativeLuminanceTarget_ = Pwl(points);
> >   }
> >   void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)
> > @@ -421,11 +429,18 @@ void AgcMeanLuminance::setLimits(utils::Duration minShutter,
> >   /**
> >    * \brief Estimate the initial gain needed to achieve a relative luminance
> >    * target
> > + * \param[in] lux The lux value at which to sample the luminance target pwl
> > + *
> > + * To account for non-linearity caused by saturation, the value needs to be
> > + * estimated in an iterative process, as multiplying by a gain will not increase
> > + * the relative luminance by the same factor if some image regions are saturated
> > + *
> >    * \return The calculated initial gain
> >    */
> > -double AgcMeanLuminance::estimateInitialGain() const
> > +double AgcMeanLuminance::estimateInitialGain(double lux) const
> >   {
> > -	double yTarget = relativeLuminanceTarget_;
> > +	double yTarget =
> > +		relativeLuminanceTarget_.eval(relativeLuminanceTarget_.domain().clamp(lux));
> >   	double yGain = 1.0;
> >   	/*
> > @@ -520,6 +535,7 @@ utils::Duration AgcMeanLuminance::filterExposure(utils::Duration exposureValue)
> >    * the calculated gain
> >    * \param[in] effectiveExposureValue The EV applied to the frame from which the
> >    * statistics in use derive
> > + * \param[in] lux The lux value at which to sample the luminance target pwl
> >    *
> >    * Calculate a new exposure value to try to obtain the target. The calculated
> >    * exposure value is filtered to prevent rapid changes from frame to frame, and
> > @@ -531,7 +547,8 @@ std::tuple<utils::Duration, double, double>
> >   AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex,
> >   				 uint32_t exposureModeIndex,
> >   				 const Histogram &yHist,
> > -				 utils::Duration effectiveExposureValue)
> > +				 utils::Duration effectiveExposureValue,
> > +				 double lux)
> >   {
> >   	/*
> >   	 * The pipeline handler should validate that we have received an allowed
> > @@ -540,7 +557,7 @@ AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex,
> >   	std::shared_ptr<ExposureModeHelper> exposureModeHelper =
> >   		exposureModeHelpers_.at(exposureModeIndex);
> > -	double gain = estimateInitialGain();
> > +	double gain = estimateInitialGain(lux);
> >   	gain = constraintClampGain(constraintModeIndex, yHist, gain);
> >   	/*
> > diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h
> > index 0a81c6d28..6ec2a0dc9 100644
> > --- a/src/ipa/libipa/agc_mean_luminance.h
> > +++ b/src/ipa/libipa/agc_mean_luminance.h
> > @@ -18,6 +18,7 @@
> >   #include "exposure_mode_helper.h"
> >   #include "histogram.h"
> > +#include "pwl.h"
> >   namespace libcamera {
> > @@ -62,7 +63,7 @@ public:
> >   	std::tuple<utils::Duration, double, double>
> >   	calculateNewEv(uint32_t constraintModeIndex, uint32_t exposureModeIndex,
> > -		       const Histogram &yHist, utils::Duration effectiveExposureValue);
> > +		       const Histogram &yHist, utils::Duration effectiveExposureValue, double lux);
> >   	void resetFrameCount()
> >   	{
> > @@ -76,7 +77,7 @@ private:
> >   	void parseConstraint(const YamlObject &modeDict, int32_t id);
> >   	int parseConstraintModes(const YamlObject &tuningData);
> >   	int parseExposureModes(const YamlObject &tuningData);
> > -	double estimateInitialGain() const;
> > +	double estimateInitialGain(double lux) const;
> >   	double constraintClampGain(uint32_t constraintModeIndex,
> >   				   const Histogram &hist,
> >   				   double gain);
> > @@ -84,7 +85,7 @@ private:
> >   	uint64_t frameCount_;
> >   	utils::Duration filteredExposure_;
> > -	double relativeLuminanceTarget_;
> > +	Pwl relativeLuminanceTarget_;
> >   	std::map<int32_t, std::vector<AgcConstraint>> constraintModes_;
> >   	std::map<int32_t, std::shared_ptr<ExposureModeHelper>> exposureModeHelpers_;
> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > index 4af397bdc..1c9872d02 100644
> > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > @@ -379,12 +379,15 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> >   	double analogueGain = frameContext.sensor.gain;
> >   	utils::Duration effectiveExposureValue = exposureTime * analogueGain;
> > +	/* \todo Plumb in the lux value. Requires a lux algo + tuning */
> > +	double lux = 400;
> > +
> >   	utils::Duration shutterTime;
> >   	double aGain, dGain;
> >   	std::tie(shutterTime, aGain, dGain) =
> >   		calculateNewEv(context.activeState.agc.constraintMode,
> >   			       context.activeState.agc.exposureMode,
> > -			       hist, effectiveExposureValue);
> > +			       hist, effectiveExposureValue, lux);
> >   	LOG(RkISP1Agc, Debug)
> >   		<< "Divided up shutter, analogue gain and digital gain are "
Dan Scally May 20, 2024, 1:12 p.m. UTC | #3 
On 17/05/2024 12:47, Paul Elder wrote:
> On Fri, May 17, 2024 at 12:29:16PM +0200, Dan Scally wrote:
>> Hi Paul
>>
>> On 17/05/2024 10:08, Paul Elder wrote:
>>> Change the relative luminance target from a scalar value to a piecewise
>>> linear function that needs to be sampled by the estimate lux value.
>>>
>>> Also change the rkisp1 and ipu3 IPAs accordingly, as they use the libipa
>>> agc. As they both don't yet have lux modules, hardcode them to a single
>>> lux value for now.
>>>
>>> This affects the format of the tuning files, but as there aren't yet any
>>> this shouldn't be an issue.
>>>
>>> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
>>> Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>
>>
>> Thanks for the patch, this looks good to me. We need to remember the same
>> thing for the constraint target though I guess.
> Yeah I just remembered that I had meant to squash that patch in with
> this, but ig I can send it separately.


Actually, one quick thought below...

>
>
> Thanks,
>
> Paul
>
>>
>> Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
>>
>>> ---
>>> No change in v3
>>>
>>> Changes in v2:
>>> - s/FPoint/PointF/
>>> - add warning when using default relative luminance target when loading
>>>     from the tuning file fails
>>> ---
>>>    src/ipa/ipu3/algorithms/agc.cpp       |  5 ++++-
>>>    src/ipa/libipa/agc_mean_luminance.cpp | 31 +++++++++++++++++++++------
>>>    src/ipa/libipa/agc_mean_luminance.h   |  7 +++---
>>>    src/ipa/rkisp1/algorithms/agc.cpp     |  5 ++++-
>>>    4 files changed, 36 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
>>> index c9b5548c4..984ed0874 100644
>>> --- a/src/ipa/ipu3/algorithms/agc.cpp
>>> +++ b/src/ipa/ipu3/algorithms/agc.cpp
>>> @@ -222,12 +222,15 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>>>    	double analogueGain = frameContext.sensor.gain;
>>>    	utils::Duration effectiveExposureValue = exposureTime * analogueGain;
>>> +	/* \todo Plumb in the lux value. Requires a lux algo + tuning */
>>> +	double lux = 400;
>>> +
>>>    	utils::Duration shutterTime;
>>>    	double aGain, dGain;
>>>    	std::tie(shutterTime, aGain, dGain) =
>>>    		calculateNewEv(context.activeState.agc.constraintMode,
>>>    			       context.activeState.agc.exposureMode, hist,
>>> -			       effectiveExposureValue);
>>> +			       effectiveExposureValue, lux);
>>>    	LOG(IPU3Agc, Debug)
>>>    		<< "Divided up shutter, analogue gain and digital gain are "
>>> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
>>> index 2bf84d05b..fe07777dc 100644
>>> --- a/src/ipa/libipa/agc_mean_luminance.cpp
>>> +++ b/src/ipa/libipa/agc_mean_luminance.cpp
>>> @@ -134,7 +134,7 @@ static constexpr double kDefaultRelativeLuminanceTarget = 0.16;
>>>     */
>>>    AgcMeanLuminance::AgcMeanLuminance()
>>> -	: frameCount_(0), filteredExposure_(0s), relativeLuminanceTarget_(0)
>>> +	: frameCount_(0), filteredExposure_(0s)
>>>    {
>>>    }
>>> @@ -142,8 +142,16 @@ AgcMeanLuminance::~AgcMeanLuminance() = default;
>>>    void AgcMeanLuminance::parseRelativeLuminanceTarget(const YamlObject &tuningData)
>>>    {
>>> -	relativeLuminanceTarget_ =
>>> -		tuningData["relativeLuminanceTarget"].get<double>(kDefaultRelativeLuminanceTarget);
>>> +	int ret = relativeLuminanceTarget_.readYaml(tuningData["relativeLuminanceTarget"]);
>>> +	if (ret == 0)
>>> +		return;
>>> +
>>> +	LOG(AgcMeanLuminance, Warning)
>>> +		<< "Failed to load tuning parameter 'relativeLuminanceTarget', "
>>> +		<< "using default [0, " << kDefaultRelativeLuminanceTarget << "]";
>>> +
>>> +	std::vector<PointF> points = { { 0, kDefaultRelativeLuminanceTarget } };
>>> +	relativeLuminanceTarget_ = Pwl(points);


Does this not need to have two points? The Pwl implementation looks like it expects an even number 
of points with a minimum of 2 - validation would fail in the readYaml() if that weren't met.

>>>    }
>>>    void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)
>>> @@ -421,11 +429,18 @@ void AgcMeanLuminance::setLimits(utils::Duration minShutter,
>>>    /**
>>>     * \brief Estimate the initial gain needed to achieve a relative luminance
>>>     * target
>>> + * \param[in] lux The lux value at which to sample the luminance target pwl
>>> + *
>>> + * To account for non-linearity caused by saturation, the value needs to be
>>> + * estimated in an iterative process, as multiplying by a gain will not increase
>>> + * the relative luminance by the same factor if some image regions are saturated
>>> + *
>>>     * \return The calculated initial gain
>>>     */
>>> -double AgcMeanLuminance::estimateInitialGain() const
>>> +double AgcMeanLuminance::estimateInitialGain(double lux) const
>>>    {
>>> -	double yTarget = relativeLuminanceTarget_;
>>> +	double yTarget =
>>> +		relativeLuminanceTarget_.eval(relativeLuminanceTarget_.domain().clamp(lux));
>>>    	double yGain = 1.0;
>>>    	/*
>>> @@ -520,6 +535,7 @@ utils::Duration AgcMeanLuminance::filterExposure(utils::Duration exposureValue)
>>>     * the calculated gain
>>>     * \param[in] effectiveExposureValue The EV applied to the frame from which the
>>>     * statistics in use derive
>>> + * \param[in] lux The lux value at which to sample the luminance target pwl
>>>     *
>>>     * Calculate a new exposure value to try to obtain the target. The calculated
>>>     * exposure value is filtered to prevent rapid changes from frame to frame, and
>>> @@ -531,7 +547,8 @@ std::tuple<utils::Duration, double, double>
>>>    AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex,
>>>    				 uint32_t exposureModeIndex,
>>>    				 const Histogram &yHist,
>>> -				 utils::Duration effectiveExposureValue)
>>> +				 utils::Duration effectiveExposureValue,
>>> +				 double lux)
>>>    {
>>>    	/*
>>>    	 * The pipeline handler should validate that we have received an allowed
>>> @@ -540,7 +557,7 @@ AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex,
>>>    	std::shared_ptr<ExposureModeHelper> exposureModeHelper =
>>>    		exposureModeHelpers_.at(exposureModeIndex);
>>> -	double gain = estimateInitialGain();
>>> +	double gain = estimateInitialGain(lux);
>>>    	gain = constraintClampGain(constraintModeIndex, yHist, gain);
>>>    	/*
>>> diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h
>>> index 0a81c6d28..6ec2a0dc9 100644
>>> --- a/src/ipa/libipa/agc_mean_luminance.h
>>> +++ b/src/ipa/libipa/agc_mean_luminance.h
>>> @@ -18,6 +18,7 @@
>>>    #include "exposure_mode_helper.h"
>>>    #include "histogram.h"
>>> +#include "pwl.h"
>>>    namespace libcamera {
>>> @@ -62,7 +63,7 @@ public:
>>>    	std::tuple<utils::Duration, double, double>
>>>    	calculateNewEv(uint32_t constraintModeIndex, uint32_t exposureModeIndex,
>>> -		       const Histogram &yHist, utils::Duration effectiveExposureValue);
>>> +		       const Histogram &yHist, utils::Duration effectiveExposureValue, double lux);
>>>    	void resetFrameCount()
>>>    	{
>>> @@ -76,7 +77,7 @@ private:
>>>    	void parseConstraint(const YamlObject &modeDict, int32_t id);
>>>    	int parseConstraintModes(const YamlObject &tuningData);
>>>    	int parseExposureModes(const YamlObject &tuningData);
>>> -	double estimateInitialGain() const;
>>> +	double estimateInitialGain(double lux) const;
>>>    	double constraintClampGain(uint32_t constraintModeIndex,
>>>    				   const Histogram &hist,
>>>    				   double gain);
>>> @@ -84,7 +85,7 @@ private:
>>>    	uint64_t frameCount_;
>>>    	utils::Duration filteredExposure_;
>>> -	double relativeLuminanceTarget_;
>>> +	Pwl relativeLuminanceTarget_;
>>>    	std::map<int32_t, std::vector<AgcConstraint>> constraintModes_;
>>>    	std::map<int32_t, std::shared_ptr<ExposureModeHelper>> exposureModeHelpers_;
>>> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
>>> index 4af397bdc..1c9872d02 100644
>>> --- a/src/ipa/rkisp1/algorithms/agc.cpp
>>> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
>>> @@ -379,12 +379,15 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>>>    	double analogueGain = frameContext.sensor.gain;
>>>    	utils::Duration effectiveExposureValue = exposureTime * analogueGain;
>>> +	/* \todo Plumb in the lux value. Requires a lux algo + tuning */
>>> +	double lux = 400;
>>> +
>>>    	utils::Duration shutterTime;
>>>    	double aGain, dGain;
>>>    	std::tie(shutterTime, aGain, dGain) =
>>>    		calculateNewEv(context.activeState.agc.constraintMode,
>>>    			       context.activeState.agc.exposureMode,
>>> -			       hist, effectiveExposureValue);
>>> +			       hist, effectiveExposureValue, lux);
>>>    	LOG(RkISP1Agc, Debug)
>>>    		<< "Divided up shutter, analogue gain and digital gain are "
Paul Elder May 29, 2024, 7:42 a.m. UTC | #4 
On Mon, May 20, 2024 at 02:12:26PM +0100, Dan Scally wrote:
> 
> On 17/05/2024 12:47, Paul Elder wrote:
> > On Fri, May 17, 2024 at 12:29:16PM +0200, Dan Scally wrote:
> > > Hi Paul
> > > 
> > > On 17/05/2024 10:08, Paul Elder wrote:
> > > > Change the relative luminance target from a scalar value to a piecewise
> > > > linear function that needs to be sampled by the estimate lux value.
> > > > 
> > > > Also change the rkisp1 and ipu3 IPAs accordingly, as they use the libipa
> > > > agc. As they both don't yet have lux modules, hardcode them to a single
> > > > lux value for now.
> > > > 
> > > > This affects the format of the tuning files, but as there aren't yet any
> > > > this shouldn't be an issue.
> > > > 
> > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > > Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > > 
> > > Thanks for the patch, this looks good to me. We need to remember the same
> > > thing for the constraint target though I guess.
> > Yeah I just remembered that I had meant to squash that patch in with
> > this, but ig I can send it separately.
> 
> 
> Actually, one quick thought below...
> 
> > 
> > 
> > Thanks,
> > 
> > Paul
> > 
> > > 
> > > Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
> > > 
> > > > ---
> > > > No change in v3
> > > > 
> > > > Changes in v2:
> > > > - s/FPoint/PointF/
> > > > - add warning when using default relative luminance target when loading
> > > >     from the tuning file fails
> > > > ---
> > > >    src/ipa/ipu3/algorithms/agc.cpp       |  5 ++++-
> > > >    src/ipa/libipa/agc_mean_luminance.cpp | 31 +++++++++++++++++++++------
> > > >    src/ipa/libipa/agc_mean_luminance.h   |  7 +++---
> > > >    src/ipa/rkisp1/algorithms/agc.cpp     |  5 ++++-
> > > >    4 files changed, 36 insertions(+), 12 deletions(-)
> > > > 
> > > > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> > > > index c9b5548c4..984ed0874 100644
> > > > --- a/src/ipa/ipu3/algorithms/agc.cpp
> > > > +++ b/src/ipa/ipu3/algorithms/agc.cpp
> > > > @@ -222,12 +222,15 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> > > >    	double analogueGain = frameContext.sensor.gain;
> > > >    	utils::Duration effectiveExposureValue = exposureTime * analogueGain;
> > > > +	/* \todo Plumb in the lux value. Requires a lux algo + tuning */
> > > > +	double lux = 400;
> > > > +
> > > >    	utils::Duration shutterTime;
> > > >    	double aGain, dGain;
> > > >    	std::tie(shutterTime, aGain, dGain) =
> > > >    		calculateNewEv(context.activeState.agc.constraintMode,
> > > >    			       context.activeState.agc.exposureMode, hist,
> > > > -			       effectiveExposureValue);
> > > > +			       effectiveExposureValue, lux);
> > > >    	LOG(IPU3Agc, Debug)
> > > >    		<< "Divided up shutter, analogue gain and digital gain are "
> > > > diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
> > > > index 2bf84d05b..fe07777dc 100644
> > > > --- a/src/ipa/libipa/agc_mean_luminance.cpp
> > > > +++ b/src/ipa/libipa/agc_mean_luminance.cpp
> > > > @@ -134,7 +134,7 @@ static constexpr double kDefaultRelativeLuminanceTarget = 0.16;
> > > >     */
> > > >    AgcMeanLuminance::AgcMeanLuminance()
> > > > -	: frameCount_(0), filteredExposure_(0s), relativeLuminanceTarget_(0)
> > > > +	: frameCount_(0), filteredExposure_(0s)
> > > >    {
> > > >    }
> > > > @@ -142,8 +142,16 @@ AgcMeanLuminance::~AgcMeanLuminance() = default;
> > > >    void AgcMeanLuminance::parseRelativeLuminanceTarget(const YamlObject &tuningData)
> > > >    {
> > > > -	relativeLuminanceTarget_ =
> > > > -		tuningData["relativeLuminanceTarget"].get<double>(kDefaultRelativeLuminanceTarget);
> > > > +	int ret = relativeLuminanceTarget_.readYaml(tuningData["relativeLuminanceTarget"]);
> > > > +	if (ret == 0)
> > > > +		return;
> > > > +
> > > > +	LOG(AgcMeanLuminance, Warning)
> > > > +		<< "Failed to load tuning parameter 'relativeLuminanceTarget', "
> > > > +		<< "using default [0, " << kDefaultRelativeLuminanceTarget << "]";
> > > > +
> > > > +	std::vector<PointF> points = { { 0, kDefaultRelativeLuminanceTarget } };
> > > > +	relativeLuminanceTarget_ = Pwl(points);
> 
> 
> Does this not need to have two points? The Pwl implementation looks like it
> expects an even number of points with a minimum of 2 - validation would fail
> in the readYaml() if that weren't met.

Indeed it does. I was getting mixed up with my matrix interpolator which
doesn't.


Thanks,

Paul

> 
> > > >    }
> > > >    void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)
> > > > @@ -421,11 +429,18 @@ void AgcMeanLuminance::setLimits(utils::Duration minShutter,
> > > >    /**
> > > >     * \brief Estimate the initial gain needed to achieve a relative luminance
> > > >     * target
> > > > + * \param[in] lux The lux value at which to sample the luminance target pwl
> > > > + *
> > > > + * To account for non-linearity caused by saturation, the value needs to be
> > > > + * estimated in an iterative process, as multiplying by a gain will not increase
> > > > + * the relative luminance by the same factor if some image regions are saturated
> > > > + *
> > > >     * \return The calculated initial gain
> > > >     */
> > > > -double AgcMeanLuminance::estimateInitialGain() const
> > > > +double AgcMeanLuminance::estimateInitialGain(double lux) const
> > > >    {
> > > > -	double yTarget = relativeLuminanceTarget_;
> > > > +	double yTarget =
> > > > +		relativeLuminanceTarget_.eval(relativeLuminanceTarget_.domain().clamp(lux));
> > > >    	double yGain = 1.0;
> > > >    	/*
> > > > @@ -520,6 +535,7 @@ utils::Duration AgcMeanLuminance::filterExposure(utils::Duration exposureValue)
> > > >     * the calculated gain
> > > >     * \param[in] effectiveExposureValue The EV applied to the frame from which the
> > > >     * statistics in use derive
> > > > + * \param[in] lux The lux value at which to sample the luminance target pwl
> > > >     *
> > > >     * Calculate a new exposure value to try to obtain the target. The calculated
> > > >     * exposure value is filtered to prevent rapid changes from frame to frame, and
> > > > @@ -531,7 +547,8 @@ std::tuple<utils::Duration, double, double>
> > > >    AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex,
> > > >    				 uint32_t exposureModeIndex,
> > > >    				 const Histogram &yHist,
> > > > -				 utils::Duration effectiveExposureValue)
> > > > +				 utils::Duration effectiveExposureValue,
> > > > +				 double lux)
> > > >    {
> > > >    	/*
> > > >    	 * The pipeline handler should validate that we have received an allowed
> > > > @@ -540,7 +557,7 @@ AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex,
> > > >    	std::shared_ptr<ExposureModeHelper> exposureModeHelper =
> > > >    		exposureModeHelpers_.at(exposureModeIndex);
> > > > -	double gain = estimateInitialGain();
> > > > +	double gain = estimateInitialGain(lux);
> > > >    	gain = constraintClampGain(constraintModeIndex, yHist, gain);
> > > >    	/*
> > > > diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h
> > > > index 0a81c6d28..6ec2a0dc9 100644
> > > > --- a/src/ipa/libipa/agc_mean_luminance.h
> > > > +++ b/src/ipa/libipa/agc_mean_luminance.h
> > > > @@ -18,6 +18,7 @@
> > > >    #include "exposure_mode_helper.h"
> > > >    #include "histogram.h"
> > > > +#include "pwl.h"
> > > >    namespace libcamera {
> > > > @@ -62,7 +63,7 @@ public:
> > > >    	std::tuple<utils::Duration, double, double>
> > > >    	calculateNewEv(uint32_t constraintModeIndex, uint32_t exposureModeIndex,
> > > > -		       const Histogram &yHist, utils::Duration effectiveExposureValue);
> > > > +		       const Histogram &yHist, utils::Duration effectiveExposureValue, double lux);
> > > >    	void resetFrameCount()
> > > >    	{
> > > > @@ -76,7 +77,7 @@ private:
> > > >    	void parseConstraint(const YamlObject &modeDict, int32_t id);
> > > >    	int parseConstraintModes(const YamlObject &tuningData);
> > > >    	int parseExposureModes(const YamlObject &tuningData);
> > > > -	double estimateInitialGain() const;
> > > > +	double estimateInitialGain(double lux) const;
> > > >    	double constraintClampGain(uint32_t constraintModeIndex,
> > > >    				   const Histogram &hist,
> > > >    				   double gain);
> > > > @@ -84,7 +85,7 @@ private:
> > > >    	uint64_t frameCount_;
> > > >    	utils::Duration filteredExposure_;
> > > > -	double relativeLuminanceTarget_;
> > > > +	Pwl relativeLuminanceTarget_;
> > > >    	std::map<int32_t, std::vector<AgcConstraint>> constraintModes_;
> > > >    	std::map<int32_t, std::shared_ptr<ExposureModeHelper>> exposureModeHelpers_;
> > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > > > index 4af397bdc..1c9872d02 100644
> > > > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > > > @@ -379,12 +379,15 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> > > >    	double analogueGain = frameContext.sensor.gain;
> > > >    	utils::Duration effectiveExposureValue = exposureTime * analogueGain;
> > > > +	/* \todo Plumb in the lux value. Requires a lux algo + tuning */
> > > > +	double lux = 400;
> > > > +
> > > >    	utils::Duration shutterTime;
> > > >    	double aGain, dGain;
> > > >    	std::tie(shutterTime, aGain, dGain) =
> > > >    		calculateNewEv(context.activeState.agc.constraintMode,
> > > >    			       context.activeState.agc.exposureMode,
> > > > -			       hist, effectiveExposureValue);
> > > > +			       hist, effectiveExposureValue, lux);
> > > >    	LOG(RkISP1Agc, Debug)
> > > >    		<< "Divided up shutter, analogue gain and digital gain are "

Patch
diff mbox series 

diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
index c9b5548c4..984ed0874 100644
--- a/src/ipa/ipu3/algorithms/agc.cpp
+++ b/src/ipa/ipu3/algorithms/agc.cpp
@@ -222,12 +222,15 @@  void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
 	double analogueGain = frameContext.sensor.gain;
 	utils::Duration effectiveExposureValue = exposureTime * analogueGain;
 
+	/* \todo Plumb in the lux value. Requires a lux algo + tuning */
+	double lux = 400;
+
 	utils::Duration shutterTime;
 	double aGain, dGain;
 	std::tie(shutterTime, aGain, dGain) =
 		calculateNewEv(context.activeState.agc.constraintMode,
 			       context.activeState.agc.exposureMode, hist,
-			       effectiveExposureValue);
+			       effectiveExposureValue, lux);
 
 	LOG(IPU3Agc, Debug)
 		<< "Divided up shutter, analogue gain and digital gain are "
diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
index 2bf84d05b..fe07777dc 100644
--- a/src/ipa/libipa/agc_mean_luminance.cpp
+++ b/src/ipa/libipa/agc_mean_luminance.cpp
@@ -134,7 +134,7 @@  static constexpr double kDefaultRelativeLuminanceTarget = 0.16;
  */
 
 AgcMeanLuminance::AgcMeanLuminance()
-	: frameCount_(0), filteredExposure_(0s), relativeLuminanceTarget_(0)
+	: frameCount_(0), filteredExposure_(0s)
 {
 }
 
@@ -142,8 +142,16 @@  AgcMeanLuminance::~AgcMeanLuminance() = default;
 
 void AgcMeanLuminance::parseRelativeLuminanceTarget(const YamlObject &tuningData)
 {
-	relativeLuminanceTarget_ =
-		tuningData["relativeLuminanceTarget"].get<double>(kDefaultRelativeLuminanceTarget);
+	int ret = relativeLuminanceTarget_.readYaml(tuningData["relativeLuminanceTarget"]);
+	if (ret == 0)
+		return;
+
+	LOG(AgcMeanLuminance, Warning)
+		<< "Failed to load tuning parameter 'relativeLuminanceTarget', "
+		<< "using default [0, " << kDefaultRelativeLuminanceTarget << "]";
+
+	std::vector<PointF> points = { { 0, kDefaultRelativeLuminanceTarget } };
+	relativeLuminanceTarget_ = Pwl(points);
 }
 
 void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)
@@ -421,11 +429,18 @@  void AgcMeanLuminance::setLimits(utils::Duration minShutter,
 /**
  * \brief Estimate the initial gain needed to achieve a relative luminance
  * target
+ * \param[in] lux The lux value at which to sample the luminance target pwl
+ *
+ * To account for non-linearity caused by saturation, the value needs to be
+ * estimated in an iterative process, as multiplying by a gain will not increase
+ * the relative luminance by the same factor if some image regions are saturated
+ *
  * \return The calculated initial gain
  */
-double AgcMeanLuminance::estimateInitialGain() const
+double AgcMeanLuminance::estimateInitialGain(double lux) const
 {
-	double yTarget = relativeLuminanceTarget_;
+	double yTarget =
+		relativeLuminanceTarget_.eval(relativeLuminanceTarget_.domain().clamp(lux));
 	double yGain = 1.0;
 
 	/*
@@ -520,6 +535,7 @@  utils::Duration AgcMeanLuminance::filterExposure(utils::Duration exposureValue)
  * the calculated gain
  * \param[in] effectiveExposureValue The EV applied to the frame from which the
  * statistics in use derive
+ * \param[in] lux The lux value at which to sample the luminance target pwl
  *
  * Calculate a new exposure value to try to obtain the target. The calculated
  * exposure value is filtered to prevent rapid changes from frame to frame, and
@@ -531,7 +547,8 @@  std::tuple<utils::Duration, double, double>
 AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex,
 				 uint32_t exposureModeIndex,
 				 const Histogram &yHist,
-				 utils::Duration effectiveExposureValue)
+				 utils::Duration effectiveExposureValue,
+				 double lux)
 {
 	/*
 	 * The pipeline handler should validate that we have received an allowed
@@ -540,7 +557,7 @@  AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex,
 	std::shared_ptr<ExposureModeHelper> exposureModeHelper =
 		exposureModeHelpers_.at(exposureModeIndex);
 
-	double gain = estimateInitialGain();
+	double gain = estimateInitialGain(lux);
 	gain = constraintClampGain(constraintModeIndex, yHist, gain);
 
 	/*
diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h
index 0a81c6d28..6ec2a0dc9 100644
--- a/src/ipa/libipa/agc_mean_luminance.h
+++ b/src/ipa/libipa/agc_mean_luminance.h
@@ -18,6 +18,7 @@ 
 
 #include "exposure_mode_helper.h"
 #include "histogram.h"
+#include "pwl.h"
 
 namespace libcamera {
 
@@ -62,7 +63,7 @@  public:
 
 	std::tuple<utils::Duration, double, double>
 	calculateNewEv(uint32_t constraintModeIndex, uint32_t exposureModeIndex,
-		       const Histogram &yHist, utils::Duration effectiveExposureValue);
+		       const Histogram &yHist, utils::Duration effectiveExposureValue, double lux);
 
 	void resetFrameCount()
 	{
@@ -76,7 +77,7 @@  private:
 	void parseConstraint(const YamlObject &modeDict, int32_t id);
 	int parseConstraintModes(const YamlObject &tuningData);
 	int parseExposureModes(const YamlObject &tuningData);
-	double estimateInitialGain() const;
+	double estimateInitialGain(double lux) const;
 	double constraintClampGain(uint32_t constraintModeIndex,
 				   const Histogram &hist,
 				   double gain);
@@ -84,7 +85,7 @@  private:
 
 	uint64_t frameCount_;
 	utils::Duration filteredExposure_;
-	double relativeLuminanceTarget_;
+	Pwl relativeLuminanceTarget_;
 
 	std::map<int32_t, std::vector<AgcConstraint>> constraintModes_;
 	std::map<int32_t, std::shared_ptr<ExposureModeHelper>> exposureModeHelpers_;
diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
index 4af397bdc..1c9872d02 100644
--- a/src/ipa/rkisp1/algorithms/agc.cpp
+++ b/src/ipa/rkisp1/algorithms/agc.cpp
@@ -379,12 +379,15 @@  void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
 	double analogueGain = frameContext.sensor.gain;
 	utils::Duration effectiveExposureValue = exposureTime * analogueGain;
 
+	/* \todo Plumb in the lux value. Requires a lux algo + tuning */
+	double lux = 400;
+
 	utils::Duration shutterTime;
 	double aGain, dGain;
 	std::tie(shutterTime, aGain, dGain) =
 		calculateNewEv(context.activeState.agc.constraintMode,
 			       context.activeState.agc.exposureMode,
-			       hist, effectiveExposureValue);
+			       hist, effectiveExposureValue, lux);
 
 	LOG(RkISP1Agc, Debug)
 		<< "Divided up shutter, analogue gain and digital gain are "