| Message ID | 20240607075914.2655019-1-paul.elder@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Quoting Paul Elder (2024-06-07 08:59:14) > Change the constraint luminance target from a scalar value to a > piecewise linear function that needs to be sampled by the estimated lux > value. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > This patch depends on v5 of "ipa:: Move Pwl from Raspberry Pi" > > Changes in v3: > - use new PointF that's a typedef of Vector > > Changes in v2: > - s/FPoint/PointF/ > - construct default Pwl with *two* points so that it actually constructs > properly > --- > src/ipa/libipa/agc_mean_luminance.cpp | 15 +++++++++------ > src/ipa/libipa/agc_mean_luminance.h | 4 ++-- > 2 files changed, 11 insertions(+), 8 deletions(-) > > diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp > index 10c16850d..575143610 100644 > --- a/src/ipa/libipa/agc_mean_luminance.cpp > +++ b/src/ipa/libipa/agc_mean_luminance.cpp > @@ -168,8 +168,10 @@ void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id) > AgcConstraint::Bound bound = static_cast<AgcConstraint::Bound>(idx); > double qLo = content["qLo"].get<double>().value_or(0.98); > double qHi = content["qHi"].get<double>().value_or(1.0); > - double yTarget = > - content["yTarget"].getList<double>().value_or(std::vector<double>{ 0.5 }).at(0); > + Pwl yTarget; > + int ret = yTarget.readYaml(content["yTarget"]); > + if (ret < 0) > + yTarget = Pwl({ Pwl::PointF({ 0, 0.5 }), Pwl::PointF({ 1000, 0.5 }) }); > > AgcConstraint constraint = { bound, qLo, qHi, yTarget }; > > @@ -221,7 +223,7 @@ int AgcMeanLuminance::parseConstraintModes(const YamlObject &tuningData) > AgcConstraint::Bound::lower, > 0.98, > 1.0, > - 0.5 > + Pwl({ Pwl::PointF({ 0, 0.5 }), Pwl::PointF({ 1000, 0.5 }) }) In V2, I asked: > Should this be some pre-constructed global const if it's going to be > used in multiple places? And you replied: Probably? It's used three times; twice here and once more for relativeLuminance. But I don't see that action taken for v3? If you do so, it would be a good opportunity to define and document the value choices for 0,0.5 -> 1000,0.5 and state clearly that this is a single horizontal linear default Pwl used to express a mid-point brightness level regardless of the input colour temperature? > }; > > constraintModes_[controls::ConstraintNormal].insert( > @@ -471,16 +473,17 @@ double AgcMeanLuminance::estimateInitialGain(double lux) const > * \param[in] constraintModeIndex The index of the constraint to adhere to > * \param[in] hist A histogram over which to calculate inter-quantile means > * \param[in] gain The gain to clamp > + * \param[in] lux The lux value at which to sample the constraint luminance target pwl > * > * \return The gain clamped within the constraint bounds > */ > double AgcMeanLuminance::constraintClampGain(uint32_t constraintModeIndex, > const Histogram &hist, > - double gain) > + double gain, double lux) > { > std::vector<AgcConstraint> &constraints = constraintModes_[constraintModeIndex]; > for (const AgcConstraint &constraint : constraints) { > - double newGain = constraint.yTarget * hist.bins() / > + double newGain = constraint.yTarget.eval(constraint.yTarget.domain().clamp(lux)) * hist.bins() / > hist.interQuantileMean(constraint.qLo, constraint.qHi); > > if (constraint.bound == AgcConstraint::Bound::lower && > @@ -559,7 +562,7 @@ AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex, > exposureModeHelpers_.at(exposureModeIndex); > > double gain = estimateInitialGain(lux); > - gain = constraintClampGain(constraintModeIndex, yHist, gain); > + gain = constraintClampGain(constraintModeIndex, yHist, gain, lux); > > /* > * We don't check whether we're already close to the target, because > diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h > index 6ec2a0dc9..d43f673dd 100644 > --- a/src/ipa/libipa/agc_mean_luminance.h > +++ b/src/ipa/libipa/agc_mean_luminance.h > @@ -38,7 +38,7 @@ public: > Bound bound; > double qLo; > double qHi; > - double yTarget; > + Pwl yTarget; > }; > > int parseTuningData(const YamlObject &tuningData); > @@ -80,7 +80,7 @@ private: > double estimateInitialGain(double lux) const; > double constraintClampGain(uint32_t constraintModeIndex, > const Histogram &hist, > - double gain); > + double gain, double lux); > utils::Duration filterExposure(utils::Duration exposureValue); > > uint64_t frameCount_; > -- > 2.39.2 >
On Mon, Jun 10, 2024 at 11:31:32AM +0100, Kieran Bingham wrote: > Quoting Paul Elder (2024-06-07 08:59:14) > > Change the constraint luminance target from a scalar value to a > > piecewise linear function that needs to be sampled by the estimated lux > > value. > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > > > --- > > This patch depends on v5 of "ipa:: Move Pwl from Raspberry Pi" > > > > Changes in v3: > > - use new PointF that's a typedef of Vector > > > > Changes in v2: > > - s/FPoint/PointF/ > > - construct default Pwl with *two* points so that it actually constructs > > properly > > --- > > src/ipa/libipa/agc_mean_luminance.cpp | 15 +++++++++------ > > src/ipa/libipa/agc_mean_luminance.h | 4 ++-- > > 2 files changed, 11 insertions(+), 8 deletions(-) > > > > diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp > > index 10c16850d..575143610 100644 > > --- a/src/ipa/libipa/agc_mean_luminance.cpp > > +++ b/src/ipa/libipa/agc_mean_luminance.cpp > > @@ -168,8 +168,10 @@ void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id) > > AgcConstraint::Bound bound = static_cast<AgcConstraint::Bound>(idx); > > double qLo = content["qLo"].get<double>().value_or(0.98); > > double qHi = content["qHi"].get<double>().value_or(1.0); > > - double yTarget = > > - content["yTarget"].getList<double>().value_or(std::vector<double>{ 0.5 }).at(0); > > + Pwl yTarget; > > + int ret = yTarget.readYaml(content["yTarget"]); > > + if (ret < 0) > > + yTarget = Pwl({ Pwl::PointF({ 0, 0.5 }), Pwl::PointF({ 1000, 0.5 }) }); > > > > AgcConstraint constraint = { bound, qLo, qHi, yTarget }; > > > > @@ -221,7 +223,7 @@ int AgcMeanLuminance::parseConstraintModes(const YamlObject &tuningData) > > AgcConstraint::Bound::lower, > > 0.98, > > 1.0, > > - 0.5 > > + Pwl({ Pwl::PointF({ 0, 0.5 }), Pwl::PointF({ 1000, 0.5 }) }) > > In V2, I asked: > > > Should this be some pre-constructed global const if it's going to be > > used in multiple places? > > And you replied: > > Probably? It's used three times; twice here and once more for > relativeLuminance. > > But I don't see that action taken for v3? It's split between two patches and I don't really want to introduce another depencency at this stage... Paul > > If you do so, it would be a good opportunity to define and document the > value choices for 0,0.5 -> 1000,0.5 and state clearly that this is a > single horizontal linear default Pwl used to express a mid-point > brightness level regardless of the input colour temperature? > > > > > > }; > > > > constraintModes_[controls::ConstraintNormal].insert( > > @@ -471,16 +473,17 @@ double AgcMeanLuminance::estimateInitialGain(double lux) const > > * \param[in] constraintModeIndex The index of the constraint to adhere to > > * \param[in] hist A histogram over which to calculate inter-quantile means > > * \param[in] gain The gain to clamp > > + * \param[in] lux The lux value at which to sample the constraint luminance target pwl > > * > > * \return The gain clamped within the constraint bounds > > */ > > double AgcMeanLuminance::constraintClampGain(uint32_t constraintModeIndex, > > const Histogram &hist, > > - double gain) > > + double gain, double lux) > > { > > std::vector<AgcConstraint> &constraints = constraintModes_[constraintModeIndex]; > > for (const AgcConstraint &constraint : constraints) { > > - double newGain = constraint.yTarget * hist.bins() / > > + double newGain = constraint.yTarget.eval(constraint.yTarget.domain().clamp(lux)) * hist.bins() / > > hist.interQuantileMean(constraint.qLo, constraint.qHi); > > > > if (constraint.bound == AgcConstraint::Bound::lower && > > @@ -559,7 +562,7 @@ AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex, > > exposureModeHelpers_.at(exposureModeIndex); > > > > double gain = estimateInitialGain(lux); > > - gain = constraintClampGain(constraintModeIndex, yHist, gain); > > + gain = constraintClampGain(constraintModeIndex, yHist, gain, lux); > > > > /* > > * We don't check whether we're already close to the target, because > > diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h > > index 6ec2a0dc9..d43f673dd 100644 > > --- a/src/ipa/libipa/agc_mean_luminance.h > > +++ b/src/ipa/libipa/agc_mean_luminance.h > > @@ -38,7 +38,7 @@ public: > > Bound bound; > > double qLo; > > double qHi; > > - double yTarget; > > + Pwl yTarget; > > }; > > > > int parseTuningData(const YamlObject &tuningData); > > @@ -80,7 +80,7 @@ private: > > double estimateInitialGain(double lux) const; > > double constraintClampGain(uint32_t constraintModeIndex, > > const Histogram &hist, > > - double gain); > > + double gain, double lux); > > utils::Duration filterExposure(utils::Duration exposureValue); > > > > uint64_t frameCount_; > > -- > > 2.39.2 > >
Quoting Paul Elder (2024-06-10 15:22:04) > On Mon, Jun 10, 2024 at 11:31:32AM +0100, Kieran Bingham wrote: > > Quoting Paul Elder (2024-06-07 08:59:14) > > > Change the constraint luminance target from a scalar value to a > > > piecewise linear function that needs to be sampled by the estimated lux > > > value. > > > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > > > > > --- > > > This patch depends on v5 of "ipa:: Move Pwl from Raspberry Pi" > > > > > > Changes in v3: > > > - use new PointF that's a typedef of Vector > > > > > > Changes in v2: > > > - s/FPoint/PointF/ > > > - construct default Pwl with *two* points so that it actually constructs > > > properly > > > --- > > > src/ipa/libipa/agc_mean_luminance.cpp | 15 +++++++++------ > > > src/ipa/libipa/agc_mean_luminance.h | 4 ++-- > > > 2 files changed, 11 insertions(+), 8 deletions(-) > > > > > > diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp > > > index 10c16850d..575143610 100644 > > > --- a/src/ipa/libipa/agc_mean_luminance.cpp > > > +++ b/src/ipa/libipa/agc_mean_luminance.cpp > > > @@ -168,8 +168,10 @@ void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id) > > > AgcConstraint::Bound bound = static_cast<AgcConstraint::Bound>(idx); > > > double qLo = content["qLo"].get<double>().value_or(0.98); > > > double qHi = content["qHi"].get<double>().value_or(1.0); > > > - double yTarget = > > > - content["yTarget"].getList<double>().value_or(std::vector<double>{ 0.5 }).at(0); > > > + Pwl yTarget; > > > + int ret = yTarget.readYaml(content["yTarget"]); > > > + if (ret < 0) > > > + yTarget = Pwl({ Pwl::PointF({ 0, 0.5 }), Pwl::PointF({ 1000, 0.5 }) }); > > > > > > AgcConstraint constraint = { bound, qLo, qHi, yTarget }; > > > > > > @@ -221,7 +223,7 @@ int AgcMeanLuminance::parseConstraintModes(const YamlObject &tuningData) > > > AgcConstraint::Bound::lower, > > > 0.98, > > > 1.0, > > > - 0.5 > > > + Pwl({ Pwl::PointF({ 0, 0.5 }), Pwl::PointF({ 1000, 0.5 }) }) > > > > In V2, I asked: > > > > > Should this be some pre-constructed global const if it's going to be > > > used in multiple places? > > > > And you replied: > > > > Probably? It's used three times; twice here and once more for > > relativeLuminance. > > > > But I don't see that action taken for v3? > > It's split between two patches and I don't really want to introduce > another depencency at this stage... So - what action can we take to earn the RB tag? Pwl::PointF({ 0, 0.5 }), Pwl::PointF({ 1000, 0.5 }) is opaque, underdocumented, and used in multiple places. Not documenting the meaning behind "0, 0.5"->"1000, 0.5" is currently blocking me giving a tag... Furthermore I see "ipa: libipa: agc: Change luminance target to piecewise linear function" does this: + std::vector<Pwl::PointF> points = { Pwl::PointF({ 0, kDefaultRelativeLuminanceTarget }), + Pwl::PointF({ 10000, kDefaultRelativeLuminanceTarget }) }; which is suddenly 0->10000. Is that the one you mean ? kDefaultRelativeLuminanceTarget is set to 0.16; ? So they're not the same. Why aren't they the same? Should they be? -- Kieran > > > Paul > > > > > If you do so, it would be a good opportunity to define and document the > > value choices for 0,0.5 -> 1000,0.5 and state clearly that this is a > > single horizontal linear default Pwl used to express a mid-point > > brightness level regardless of the input colour temperature? > > > > > > > > > > > }; > > > > > > constraintModes_[controls::ConstraintNormal].insert( > > > @@ -471,16 +473,17 @@ double AgcMeanLuminance::estimateInitialGain(double lux) const > > > * \param[in] constraintModeIndex The index of the constraint to adhere to > > > * \param[in] hist A histogram over which to calculate inter-quantile means > > > * \param[in] gain The gain to clamp > > > + * \param[in] lux The lux value at which to sample the constraint luminance target pwl > > > * > > > * \return The gain clamped within the constraint bounds > > > */ > > > double AgcMeanLuminance::constraintClampGain(uint32_t constraintModeIndex, > > > const Histogram &hist, > > > - double gain) > > > + double gain, double lux) > > > { > > > std::vector<AgcConstraint> &constraints = constraintModes_[constraintModeIndex]; > > > for (const AgcConstraint &constraint : constraints) { > > > - double newGain = constraint.yTarget * hist.bins() / > > > + double newGain = constraint.yTarget.eval(constraint.yTarget.domain().clamp(lux)) * hist.bins() / > > > hist.interQuantileMean(constraint.qLo, constraint.qHi); > > > > > > if (constraint.bound == AgcConstraint::Bound::lower && > > > @@ -559,7 +562,7 @@ AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex, > > > exposureModeHelpers_.at(exposureModeIndex); > > > > > > double gain = estimateInitialGain(lux); > > > - gain = constraintClampGain(constraintModeIndex, yHist, gain); > > > + gain = constraintClampGain(constraintModeIndex, yHist, gain, lux); > > > > > > /* > > > * We don't check whether we're already close to the target, because > > > diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h > > > index 6ec2a0dc9..d43f673dd 100644 > > > --- a/src/ipa/libipa/agc_mean_luminance.h > > > +++ b/src/ipa/libipa/agc_mean_luminance.h > > > @@ -38,7 +38,7 @@ public: > > > Bound bound; > > > double qLo; > > > double qHi; > > > - double yTarget; > > > + Pwl yTarget; > > > }; > > > > > > int parseTuningData(const YamlObject &tuningData); > > > @@ -80,7 +80,7 @@ private: > > > double estimateInitialGain(double lux) const; > > > double constraintClampGain(uint32_t constraintModeIndex, > > > const Histogram &hist, > > > - double gain); > > > + double gain, double lux); > > > utils::Duration filterExposure(utils::Duration exposureValue); > > > > > > uint64_t frameCount_; > > > -- > > > 2.39.2 > > >
On Mon, Jun 10, 2024 at 03:47:23PM +0100, Kieran Bingham wrote: > Quoting Paul Elder (2024-06-10 15:22:04) > > On Mon, Jun 10, 2024 at 11:31:32AM +0100, Kieran Bingham wrote: > > > Quoting Paul Elder (2024-06-07 08:59:14) > > > > Change the constraint luminance target from a scalar value to a > > > > piecewise linear function that needs to be sampled by the estimated lux > > > > value. > > > > > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > > > > > > > --- > > > > This patch depends on v5 of "ipa:: Move Pwl from Raspberry Pi" > > > > > > > > Changes in v3: > > > > - use new PointF that's a typedef of Vector > > > > > > > > Changes in v2: > > > > - s/FPoint/PointF/ > > > > - construct default Pwl with *two* points so that it actually constructs > > > > properly > > > > --- > > > > src/ipa/libipa/agc_mean_luminance.cpp | 15 +++++++++------ > > > > src/ipa/libipa/agc_mean_luminance.h | 4 ++-- > > > > 2 files changed, 11 insertions(+), 8 deletions(-) > > > > > > > > diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp > > > > index 10c16850d..575143610 100644 > > > > --- a/src/ipa/libipa/agc_mean_luminance.cpp > > > > +++ b/src/ipa/libipa/agc_mean_luminance.cpp > > > > @@ -168,8 +168,10 @@ void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id) > > > > AgcConstraint::Bound bound = static_cast<AgcConstraint::Bound>(idx); > > > > double qLo = content["qLo"].get<double>().value_or(0.98); > > > > double qHi = content["qHi"].get<double>().value_or(1.0); > > > > - double yTarget = > > > > - content["yTarget"].getList<double>().value_or(std::vector<double>{ 0.5 }).at(0); > > > > + Pwl yTarget; > > > > + int ret = yTarget.readYaml(content["yTarget"]); > > > > + if (ret < 0) > > > > + yTarget = Pwl({ Pwl::PointF({ 0, 0.5 }), Pwl::PointF({ 1000, 0.5 }) }); > > > > > > > > AgcConstraint constraint = { bound, qLo, qHi, yTarget }; > > > > > > > > @@ -221,7 +223,7 @@ int AgcMeanLuminance::parseConstraintModes(const YamlObject &tuningData) > > > > AgcConstraint::Bound::lower, > > > > 0.98, > > > > 1.0, > > > > - 0.5 > > > > + Pwl({ Pwl::PointF({ 0, 0.5 }), Pwl::PointF({ 1000, 0.5 }) }) > > > > > > In V2, I asked: > > > > > > > Should this be some pre-constructed global const if it's going to be > > > > used in multiple places? > > > > > > And you replied: > > > > > > Probably? It's used three times; twice here and once more for > > > relativeLuminance. > > > > > > But I don't see that action taken for v3? > > > > It's split between two patches and I don't really want to introduce > > another depencency at this stage... Let's get the dependencies merged, we're nearly there. > So - what action can we take to earn the RB tag? > > Pwl::PointF({ 0, 0.5 }), Pwl::PointF({ 1000, 0.5 }) is opaque, > underdocumented, and used in multiple places. > > Not documenting the meaning behind "0, 0.5"->"1000, 0.5" is currently > blocking me giving a tag... > > > Furthermore I see "ipa: libipa: agc: Change luminance target to > piecewise linear function" does this: > > + std::vector<Pwl::PointF> points = { Pwl::PointF({ 0, kDefaultRelativeLuminanceTarget }), > + Pwl::PointF({ 10000, kDefaultRelativeLuminanceTarget }) }; > > which is suddenly 0->10000. That won't make a difference in practice, as the function is flat, but I agree it would be nice to be consistent. For a default curve, I would use typical lux levels. 0 is complete darkness, I think that's as good as anything, even if we could replace it with 1 (full moonlight) or 10 (street lighting at night). 10000 is the lower end of ambient daylight, while 1000 is the lower end of overcast daylight (or a really well lit room). > Is that the one you mean ? > > kDefaultRelativeLuminanceTarget is set to 0.16; ? So they're not the > same. Why aren't they the same? Should they be? That one is a bit more concerning. > > > If you do so, it would be a good opportunity to define and document the > > > value choices for 0,0.5 -> 1000,0.5 and state clearly that this is a > > > single horizontal linear default Pwl used to express a mid-point > > > brightness level regardless of the input colour temperature? > > > > > > > }; > > > > > > > > constraintModes_[controls::ConstraintNormal].insert( > > > > @@ -471,16 +473,17 @@ double AgcMeanLuminance::estimateInitialGain(double lux) const > > > > * \param[in] constraintModeIndex The index of the constraint to adhere to > > > > * \param[in] hist A histogram over which to calculate inter-quantile means > > > > * \param[in] gain The gain to clamp > > > > + * \param[in] lux The lux value at which to sample the constraint luminance target pwl > > > > * > > > > * \return The gain clamped within the constraint bounds > > > > */ > > > > double AgcMeanLuminance::constraintClampGain(uint32_t constraintModeIndex, > > > > const Histogram &hist, > > > > - double gain) > > > > + double gain, double lux) > > > > { > > > > std::vector<AgcConstraint> &constraints = constraintModes_[constraintModeIndex]; > > > > for (const AgcConstraint &constraint : constraints) { > > > > - double newGain = constraint.yTarget * hist.bins() / > > > > + double newGain = constraint.yTarget.eval(constraint.yTarget.domain().clamp(lux)) * hist.bins() / > > > > hist.interQuantileMean(constraint.qLo, constraint.qHi); Let's shorten that a bit: double newGain = constraint.yTarget.eval(constraint.yTarget.domain().clamp(lux)) * hist.bins() / hist.interQuantileMean(constraint.qLo, constraint.qHi); But I recall you mentioned that the Pwl class models a function that is flat outside of the domain, so do you need to clamp ? If not, double newGain = constraint.yTarget.eval(lux) * hist.bins() / hist.interQuantileMean(constraint.qLo, constraint.qHi); looks nicer. > > > > > > > > if (constraint.bound == AgcConstraint::Bound::lower && > > > > @@ -559,7 +562,7 @@ AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex, > > > > exposureModeHelpers_.at(exposureModeIndex); > > > > > > > > double gain = estimateInitialGain(lux); > > > > - gain = constraintClampGain(constraintModeIndex, yHist, gain); > > > > + gain = constraintClampGain(constraintModeIndex, yHist, gain, lux); > > > > > > > > /* > > > > * We don't check whether we're already close to the target, because > > > > diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h > > > > index 6ec2a0dc9..d43f673dd 100644 > > > > --- a/src/ipa/libipa/agc_mean_luminance.h > > > > +++ b/src/ipa/libipa/agc_mean_luminance.h > > > > @@ -38,7 +38,7 @@ public: > > > > Bound bound; > > > > double qLo; > > > > double qHi; > > > > - double yTarget; > > > > + Pwl yTarget; > > > > }; > > > > > > > > int parseTuningData(const YamlObject &tuningData); > > > > @@ -80,7 +80,7 @@ private: > > > > double estimateInitialGain(double lux) const; > > > > double constraintClampGain(uint32_t constraintModeIndex, > > > > const Histogram &hist, > > > > - double gain); > > > > + double gain, double lux); > > > > utils::Duration filterExposure(utils::Duration exposureValue); > > > > > > > > uint64_t frameCount_;
Hello On 11/06/2024 23:48, Laurent Pinchart wrote: > On Mon, Jun 10, 2024 at 03:47:23PM +0100, Kieran Bingham wrote: >> Quoting Paul Elder (2024-06-10 15:22:04) >>> On Mon, Jun 10, 2024 at 11:31:32AM +0100, Kieran Bingham wrote: >>>> Quoting Paul Elder (2024-06-07 08:59:14) >>>>> Change the constraint luminance target from a scalar value to a >>>>> piecewise linear function that needs to be sampled by the estimated lux >>>>> value. >>>>> >>>>> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> >>>>> >>>>> --- >>>>> This patch depends on v5 of "ipa:: Move Pwl from Raspberry Pi" >>>>> >>>>> Changes in v3: >>>>> - use new PointF that's a typedef of Vector >>>>> >>>>> Changes in v2: >>>>> - s/FPoint/PointF/ >>>>> - construct default Pwl with *two* points so that it actually constructs >>>>> properly >>>>> --- >>>>> src/ipa/libipa/agc_mean_luminance.cpp | 15 +++++++++------ >>>>> src/ipa/libipa/agc_mean_luminance.h | 4 ++-- >>>>> 2 files changed, 11 insertions(+), 8 deletions(-) >>>>> >>>>> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp >>>>> index 10c16850d..575143610 100644 >>>>> --- a/src/ipa/libipa/agc_mean_luminance.cpp >>>>> +++ b/src/ipa/libipa/agc_mean_luminance.cpp >>>>> @@ -168,8 +168,10 @@ void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id) >>>>> AgcConstraint::Bound bound = static_cast<AgcConstraint::Bound>(idx); >>>>> double qLo = content["qLo"].get<double>().value_or(0.98); >>>>> double qHi = content["qHi"].get<double>().value_or(1.0); >>>>> - double yTarget = >>>>> - content["yTarget"].getList<double>().value_or(std::vector<double>{ 0.5 }).at(0); >>>>> + Pwl yTarget; >>>>> + int ret = yTarget.readYaml(content["yTarget"]); >>>>> + if (ret < 0) >>>>> + yTarget = Pwl({ Pwl::PointF({ 0, 0.5 }), Pwl::PointF({ 1000, 0.5 }) }); >>>>> >>>>> AgcConstraint constraint = { bound, qLo, qHi, yTarget }; >>>>> >>>>> @@ -221,7 +223,7 @@ int AgcMeanLuminance::parseConstraintModes(const YamlObject &tuningData) >>>>> AgcConstraint::Bound::lower, >>>>> 0.98, >>>>> 1.0, >>>>> - 0.5 >>>>> + Pwl({ Pwl::PointF({ 0, 0.5 }), Pwl::PointF({ 1000, 0.5 }) }) >>>> In V2, I asked: >>>> >>>>> Should this be some pre-constructed global const if it's going to be >>>>> used in multiple places? >>>> >>>> And you replied: >>>> >>>> Probably? It's used three times; twice here and once more for >>>> relativeLuminance. >>>> >>>> But I don't see that action taken for v3? >>> It's split between two patches and I don't really want to introduce >>> another depencency at this stage... > Let's get the dependencies merged, we're nearly there. > >> So - what action can we take to earn the RB tag? >> >> Pwl::PointF({ 0, 0.5 }), Pwl::PointF({ 1000, 0.5 }) is opaque, >> underdocumented, and used in multiple places. >> >> Not documenting the meaning behind "0, 0.5"->"1000, 0.5" is currently >> blocking me giving a tag... >> >> >> Furthermore I see "ipa: libipa: agc: Change luminance target to >> piecewise linear function" does this: >> >> + std::vector<Pwl::PointF> points = { Pwl::PointF({ 0, kDefaultRelativeLuminanceTarget }), >> + Pwl::PointF({ 10000, kDefaultRelativeLuminanceTarget }) }; >> >> which is suddenly 0->10000. > That won't make a difference in practice, as the function is flat, but I > agree it would be nice to be consistent. For a default curve, I would > use typical lux levels. 0 is complete darkness, I think that's as good > as anything, even if we could replace it with 1 (full moonlight) or 10 > (street lighting at night). 10000 is the lower end of ambient daylight, > while 1000 is the lower end of overcast daylight (or a really well lit > room). > >> Is that the one you mean ? >> >> kDefaultRelativeLuminanceTarget is set to 0.16; ? So they're not the >> same. Why aren't they the same? Should they be? > That one is a bit more concerning. They're targets for different things; they shouldn't be the same. kDefaultRelativeLuminanceTarget is the default target we chose for the image as a whole - the mean luminance agc method tries to drive the mean luminance of the entire image towards that target. The yTarget for the constraints here are just to clamp the mean luminance for a particular portion of the histogram to a value. The effect of the defaults is to drive the mean luminance of the entire image to 0.16, but to clamp that of the top 2% of the histogram to at least 0.5 - that's how it currently behaves too, though the default is expressed as a single value rather than a flat piecewise linear function. > >>>> If you do so, it would be a good opportunity to define and document the >>>> value choices for 0,0.5 -> 1000,0.5 and state clearly that this is a >>>> single horizontal linear default Pwl used to express a mid-point >>>> brightness level regardless of the input colour temperature? >>>> >>>>> }; >>>>> >>>>> constraintModes_[controls::ConstraintNormal].insert( >>>>> @@ -471,16 +473,17 @@ double AgcMeanLuminance::estimateInitialGain(double lux) const >>>>> * \param[in] constraintModeIndex The index of the constraint to adhere to >>>>> * \param[in] hist A histogram over which to calculate inter-quantile means >>>>> * \param[in] gain The gain to clamp >>>>> + * \param[in] lux The lux value at which to sample the constraint luminance target pwl >>>>> * >>>>> * \return The gain clamped within the constraint bounds >>>>> */ >>>>> double AgcMeanLuminance::constraintClampGain(uint32_t constraintModeIndex, >>>>> const Histogram &hist, >>>>> - double gain) >>>>> + double gain, double lux) >>>>> { >>>>> std::vector<AgcConstraint> &constraints = constraintModes_[constraintModeIndex]; >>>>> for (const AgcConstraint &constraint : constraints) { >>>>> - double newGain = constraint.yTarget * hist.bins() / >>>>> + double newGain = constraint.yTarget.eval(constraint.yTarget.domain().clamp(lux)) * hist.bins() / >>>>> hist.interQuantileMean(constraint.qLo, constraint.qHi); > Let's shorten that a bit: > > double newGain = constraint.yTarget.eval(constraint.yTarget.domain().clamp(lux)) > * hist.bins() > / hist.interQuantileMean(constraint.qLo, constraint.qHi); > > But I recall you mentioned that the Pwl class models a function that is > flat outside of the domain, so do you need to clamp ? If not, > > double newGain = constraint.yTarget.eval(lux) * hist.bins() > / hist.interQuantileMean(constraint.qLo, constraint.qHi); > > looks nicer. > >>>>> >>>>> if (constraint.bound == AgcConstraint::Bound::lower && >>>>> @@ -559,7 +562,7 @@ AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex, >>>>> exposureModeHelpers_.at(exposureModeIndex); >>>>> >>>>> double gain = estimateInitialGain(lux); >>>>> - gain = constraintClampGain(constraintModeIndex, yHist, gain); >>>>> + gain = constraintClampGain(constraintModeIndex, yHist, gain, lux); >>>>> >>>>> /* >>>>> * We don't check whether we're already close to the target, because >>>>> diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h >>>>> index 6ec2a0dc9..d43f673dd 100644 >>>>> --- a/src/ipa/libipa/agc_mean_luminance.h >>>>> +++ b/src/ipa/libipa/agc_mean_luminance.h >>>>> @@ -38,7 +38,7 @@ public: >>>>> Bound bound; >>>>> double qLo; >>>>> double qHi; >>>>> - double yTarget; >>>>> + Pwl yTarget; >>>>> }; >>>>> >>>>> int parseTuningData(const YamlObject &tuningData); >>>>> @@ -80,7 +80,7 @@ private: >>>>> double estimateInitialGain(double lux) const; >>>>> double constraintClampGain(uint32_t constraintModeIndex, >>>>> const Histogram &hist, >>>>> - double gain); >>>>> + double gain, double lux); >>>>> utils::Duration filterExposure(utils::Duration exposureValue); >>>>> >>>>> uint64_t frameCount_;
Hi Dan, On Thu, Jun 13, 2024 at 10:39:00PM +0100, Daniel Scally wrote: > On 11/06/2024 23:48, Laurent Pinchart wrote: > > On Mon, Jun 10, 2024 at 03:47:23PM +0100, Kieran Bingham wrote: > >> Quoting Paul Elder (2024-06-10 15:22:04) > >>> On Mon, Jun 10, 2024 at 11:31:32AM +0100, Kieran Bingham wrote: > >>>> Quoting Paul Elder (2024-06-07 08:59:14) > >>>>> Change the constraint luminance target from a scalar value to a > >>>>> piecewise linear function that needs to be sampled by the estimated lux > >>>>> value. > >>>>> > >>>>> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > >>>>> > >>>>> --- > >>>>> This patch depends on v5 of "ipa:: Move Pwl from Raspberry Pi" > >>>>> > >>>>> Changes in v3: > >>>>> - use new PointF that's a typedef of Vector > >>>>> > >>>>> Changes in v2: > >>>>> - s/FPoint/PointF/ > >>>>> - construct default Pwl with *two* points so that it actually constructs > >>>>> properly > >>>>> --- > >>>>> src/ipa/libipa/agc_mean_luminance.cpp | 15 +++++++++------ > >>>>> src/ipa/libipa/agc_mean_luminance.h | 4 ++-- > >>>>> 2 files changed, 11 insertions(+), 8 deletions(-) > >>>>> > >>>>> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp > >>>>> index 10c16850d..575143610 100644 > >>>>> --- a/src/ipa/libipa/agc_mean_luminance.cpp > >>>>> +++ b/src/ipa/libipa/agc_mean_luminance.cpp > >>>>> @@ -168,8 +168,10 @@ void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id) > >>>>> AgcConstraint::Bound bound = static_cast<AgcConstraint::Bound>(idx); > >>>>> double qLo = content["qLo"].get<double>().value_or(0.98); > >>>>> double qHi = content["qHi"].get<double>().value_or(1.0); > >>>>> - double yTarget = > >>>>> - content["yTarget"].getList<double>().value_or(std::vector<double>{ 0.5 }).at(0); > >>>>> + Pwl yTarget; > >>>>> + int ret = yTarget.readYaml(content["yTarget"]); > >>>>> + if (ret < 0) > >>>>> + yTarget = Pwl({ Pwl::PointF({ 0, 0.5 }), Pwl::PointF({ 1000, 0.5 }) }); > >>>>> > >>>>> AgcConstraint constraint = { bound, qLo, qHi, yTarget }; > >>>>> > >>>>> @@ -221,7 +223,7 @@ int AgcMeanLuminance::parseConstraintModes(const YamlObject &tuningData) > >>>>> AgcConstraint::Bound::lower, > >>>>> 0.98, > >>>>> 1.0, > >>>>> - 0.5 > >>>>> + Pwl({ Pwl::PointF({ 0, 0.5 }), Pwl::PointF({ 1000, 0.5 }) }) > >>>> > >>>> In V2, I asked: > >>>> > >>>>> Should this be some pre-constructed global const if it's going to be > >>>>> used in multiple places? > >>>> > >>>> And you replied: > >>>> > >>>> Probably? It's used three times; twice here and once more for > >>>> relativeLuminance. > >>>> > >>>> But I don't see that action taken for v3? > >>> > >>> It's split between two patches and I don't really want to introduce > >>> another depencency at this stage... > > > > Let's get the dependencies merged, we're nearly there. > > > >> So - what action can we take to earn the RB tag? > >> > >> Pwl::PointF({ 0, 0.5 }), Pwl::PointF({ 1000, 0.5 }) is opaque, > >> underdocumented, and used in multiple places. > >> > >> Not documenting the meaning behind "0, 0.5"->"1000, 0.5" is currently > >> blocking me giving a tag... > >> > >> > >> Furthermore I see "ipa: libipa: agc: Change luminance target to > >> piecewise linear function" does this: > >> > >> + std::vector<Pwl::PointF> points = { Pwl::PointF({ 0, kDefaultRelativeLuminanceTarget }), > >> + Pwl::PointF({ 10000, kDefaultRelativeLuminanceTarget }) }; > >> > >> which is suddenly 0->10000. > > > > That won't make a difference in practice, as the function is flat, but I > > agree it would be nice to be consistent. For a default curve, I would > > use typical lux levels. 0 is complete darkness, I think that's as good > > as anything, even if we could replace it with 1 (full moonlight) or 10 > > (street lighting at night). 10000 is the lower end of ambient daylight, > > while 1000 is the lower end of overcast daylight (or a really well lit > > room). > > > >> Is that the one you mean ? > >> > >> kDefaultRelativeLuminanceTarget is set to 0.16; ? So they're not the > >> same. Why aren't they the same? Should they be? > > That one is a bit more concerning. > > They're targets for different things; they shouldn't be the same. kDefaultRelativeLuminanceTarget is > the default target we chose for the image as a whole - the mean luminance agc method tries to drive > the mean luminance of the entire image towards that target. The yTarget for the constraints here are > just to clamp the mean luminance for a particular portion of the histogram to a value. The effect of > the defaults is to drive the mean luminance of the entire image to 0.16, but to clamp that of the > top 2% of the histogram to at least 0.5 - that's how it currently behaves too, though the default is > expressed as a single value rather than a flat piecewise linear function. You're right, my bad. One day it would be very nice to document the AGC algorithm. I understand that we combine two different mechanisms, but I don't understand why that produces the desired result :-) > >>>> If you do so, it would be a good opportunity to define and document the > >>>> value choices for 0,0.5 -> 1000,0.5 and state clearly that this is a > >>>> single horizontal linear default Pwl used to express a mid-point > >>>> brightness level regardless of the input colour temperature? > >>>> > >>>>> }; > >>>>> > >>>>> constraintModes_[controls::ConstraintNormal].insert( > >>>>> @@ -471,16 +473,17 @@ double AgcMeanLuminance::estimateInitialGain(double lux) const > >>>>> * \param[in] constraintModeIndex The index of the constraint to adhere to > >>>>> * \param[in] hist A histogram over which to calculate inter-quantile means > >>>>> * \param[in] gain The gain to clamp > >>>>> + * \param[in] lux The lux value at which to sample the constraint luminance target pwl > >>>>> * > >>>>> * \return The gain clamped within the constraint bounds > >>>>> */ > >>>>> double AgcMeanLuminance::constraintClampGain(uint32_t constraintModeIndex, > >>>>> const Histogram &hist, > >>>>> - double gain) > >>>>> + double gain, double lux) > >>>>> { > >>>>> std::vector<AgcConstraint> &constraints = constraintModes_[constraintModeIndex]; > >>>>> for (const AgcConstraint &constraint : constraints) { > >>>>> - double newGain = constraint.yTarget * hist.bins() / > >>>>> + double newGain = constraint.yTarget.eval(constraint.yTarget.domain().clamp(lux)) * hist.bins() / > >>>>> hist.interQuantileMean(constraint.qLo, constraint.qHi); > > > > Let's shorten that a bit: > > > > double newGain = constraint.yTarget.eval(constraint.yTarget.domain().clamp(lux)) > > * hist.bins() > > / hist.interQuantileMean(constraint.qLo, constraint.qHi); > > > > But I recall you mentioned that the Pwl class models a function that is > > flat outside of the domain, so do you need to clamp ? If not, > > > > double newGain = constraint.yTarget.eval(lux) * hist.bins() > > / hist.interQuantileMean(constraint.qLo, constraint.qHi); > > > > looks nicer. > > > >>>>> > >>>>> if (constraint.bound == AgcConstraint::Bound::lower && > >>>>> @@ -559,7 +562,7 @@ AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex, > >>>>> exposureModeHelpers_.at(exposureModeIndex); > >>>>> > >>>>> double gain = estimateInitialGain(lux); > >>>>> - gain = constraintClampGain(constraintModeIndex, yHist, gain); > >>>>> + gain = constraintClampGain(constraintModeIndex, yHist, gain, lux); > >>>>> > >>>>> /* > >>>>> * We don't check whether we're already close to the target, because > >>>>> diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h > >>>>> index 6ec2a0dc9..d43f673dd 100644 > >>>>> --- a/src/ipa/libipa/agc_mean_luminance.h > >>>>> +++ b/src/ipa/libipa/agc_mean_luminance.h > >>>>> @@ -38,7 +38,7 @@ public: > >>>>> Bound bound; > >>>>> double qLo; > >>>>> double qHi; > >>>>> - double yTarget; > >>>>> + Pwl yTarget; > >>>>> }; > >>>>> > >>>>> int parseTuningData(const YamlObject &tuningData); > >>>>> @@ -80,7 +80,7 @@ private: > >>>>> double estimateInitialGain(double lux) const; > >>>>> double constraintClampGain(uint32_t constraintModeIndex, > >>>>> const Histogram &hist, > >>>>> - double gain); > >>>>> + double gain, double lux); > >>>>> utils::Duration filterExposure(utils::Duration exposureValue); > >>>>> > >>>>> uint64_t frameCount_;
Hi Paul On 07/06/2024 08:59, Paul Elder wrote: > Change the constraint luminance target from a scalar value to a > piecewise linear function that needs to be sampled by the estimated lux > value. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> For me, the only problem now is that the documentation for parseTuningData() now needs updating, as it still suggests a single value for the constraint mode yTarget rather than an array. The rest of the patch I think is fine. > --- > This patch depends on v5 of "ipa:: Move Pwl from Raspberry Pi" > > Changes in v3: > - use new PointF that's a typedef of Vector > > Changes in v2: > - s/FPoint/PointF/ > - construct default Pwl with *two* points so that it actually constructs > properly > --- > src/ipa/libipa/agc_mean_luminance.cpp | 15 +++++++++------ > src/ipa/libipa/agc_mean_luminance.h | 4 ++-- > 2 files changed, 11 insertions(+), 8 deletions(-) > > diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp > index 10c16850d..575143610 100644 > --- a/src/ipa/libipa/agc_mean_luminance.cpp > +++ b/src/ipa/libipa/agc_mean_luminance.cpp > @@ -168,8 +168,10 @@ void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id) > AgcConstraint::Bound bound = static_cast<AgcConstraint::Bound>(idx); > double qLo = content["qLo"].get<double>().value_or(0.98); > double qHi = content["qHi"].get<double>().value_or(1.0); > - double yTarget = > - content["yTarget"].getList<double>().value_or(std::vector<double>{ 0.5 }).at(0); > + Pwl yTarget; > + int ret = yTarget.readYaml(content["yTarget"]); > + if (ret < 0) > + yTarget = Pwl({ Pwl::PointF({ 0, 0.5 }), Pwl::PointF({ 1000, 0.5 }) }); > > AgcConstraint constraint = { bound, qLo, qHi, yTarget }; > > @@ -221,7 +223,7 @@ int AgcMeanLuminance::parseConstraintModes(const YamlObject &tuningData) > AgcConstraint::Bound::lower, > 0.98, > 1.0, > - 0.5 > + Pwl({ Pwl::PointF({ 0, 0.5 }), Pwl::PointF({ 1000, 0.5 }) }) > }; > > constraintModes_[controls::ConstraintNormal].insert( > @@ -471,16 +473,17 @@ double AgcMeanLuminance::estimateInitialGain(double lux) const > * \param[in] constraintModeIndex The index of the constraint to adhere to > * \param[in] hist A histogram over which to calculate inter-quantile means > * \param[in] gain The gain to clamp > + * \param[in] lux The lux value at which to sample the constraint luminance target pwl > * > * \return The gain clamped within the constraint bounds > */ > double AgcMeanLuminance::constraintClampGain(uint32_t constraintModeIndex, > const Histogram &hist, > - double gain) > + double gain, double lux) > { > std::vector<AgcConstraint> &constraints = constraintModes_[constraintModeIndex]; > for (const AgcConstraint &constraint : constraints) { > - double newGain = constraint.yTarget * hist.bins() / > + double newGain = constraint.yTarget.eval(constraint.yTarget.domain().clamp(lux)) * hist.bins() / > hist.interQuantileMean(constraint.qLo, constraint.qHi); > > if (constraint.bound == AgcConstraint::Bound::lower && > @@ -559,7 +562,7 @@ AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex, > exposureModeHelpers_.at(exposureModeIndex); > > double gain = estimateInitialGain(lux); > - gain = constraintClampGain(constraintModeIndex, yHist, gain); > + gain = constraintClampGain(constraintModeIndex, yHist, gain, lux); > > /* > * We don't check whether we're already close to the target, because > diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h > index 6ec2a0dc9..d43f673dd 100644 > --- a/src/ipa/libipa/agc_mean_luminance.h > +++ b/src/ipa/libipa/agc_mean_luminance.h > @@ -38,7 +38,7 @@ public: > Bound bound; > double qLo; > double qHi; > - double yTarget; > + Pwl yTarget; > }; > > int parseTuningData(const YamlObject &tuningData); > @@ -80,7 +80,7 @@ private: > double estimateInitialGain(double lux) const; > double constraintClampGain(uint32_t constraintModeIndex, > const Histogram &hist, > - double gain); > + double gain, double lux); > utils::Duration filterExposure(utils::Duration exposureValue); > > uint64_t frameCount_;
diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp index 10c16850d..575143610 100644 --- a/src/ipa/libipa/agc_mean_luminance.cpp +++ b/src/ipa/libipa/agc_mean_luminance.cpp @@ -168,8 +168,10 @@ void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id) AgcConstraint::Bound bound = static_cast<AgcConstraint::Bound>(idx); double qLo = content["qLo"].get<double>().value_or(0.98); double qHi = content["qHi"].get<double>().value_or(1.0); - double yTarget = - content["yTarget"].getList<double>().value_or(std::vector<double>{ 0.5 }).at(0); + Pwl yTarget; + int ret = yTarget.readYaml(content["yTarget"]); + if (ret < 0) + yTarget = Pwl({ Pwl::PointF({ 0, 0.5 }), Pwl::PointF({ 1000, 0.5 }) }); AgcConstraint constraint = { bound, qLo, qHi, yTarget }; @@ -221,7 +223,7 @@ int AgcMeanLuminance::parseConstraintModes(const YamlObject &tuningData) AgcConstraint::Bound::lower, 0.98, 1.0, - 0.5 + Pwl({ Pwl::PointF({ 0, 0.5 }), Pwl::PointF({ 1000, 0.5 }) }) }; constraintModes_[controls::ConstraintNormal].insert( @@ -471,16 +473,17 @@ double AgcMeanLuminance::estimateInitialGain(double lux) const * \param[in] constraintModeIndex The index of the constraint to adhere to * \param[in] hist A histogram over which to calculate inter-quantile means * \param[in] gain The gain to clamp + * \param[in] lux The lux value at which to sample the constraint luminance target pwl * * \return The gain clamped within the constraint bounds */ double AgcMeanLuminance::constraintClampGain(uint32_t constraintModeIndex, const Histogram &hist, - double gain) + double gain, double lux) { std::vector<AgcConstraint> &constraints = constraintModes_[constraintModeIndex]; for (const AgcConstraint &constraint : constraints) { - double newGain = constraint.yTarget * hist.bins() / + double newGain = constraint.yTarget.eval(constraint.yTarget.domain().clamp(lux)) * hist.bins() / hist.interQuantileMean(constraint.qLo, constraint.qHi); if (constraint.bound == AgcConstraint::Bound::lower && @@ -559,7 +562,7 @@ AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex, exposureModeHelpers_.at(exposureModeIndex); double gain = estimateInitialGain(lux); - gain = constraintClampGain(constraintModeIndex, yHist, gain); + gain = constraintClampGain(constraintModeIndex, yHist, gain, lux); /* * We don't check whether we're already close to the target, because diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h index 6ec2a0dc9..d43f673dd 100644 --- a/src/ipa/libipa/agc_mean_luminance.h +++ b/src/ipa/libipa/agc_mean_luminance.h @@ -38,7 +38,7 @@ public: Bound bound; double qLo; double qHi; - double yTarget; + Pwl yTarget; }; int parseTuningData(const YamlObject &tuningData); @@ -80,7 +80,7 @@ private: double estimateInitialGain(double lux) const; double constraintClampGain(uint32_t constraintModeIndex, const Histogram &hist, - double gain); + double gain, double lux); utils::Duration filterExposure(utils::Duration exposureValue); uint64_t frameCount_;
Change the constraint luminance target from a scalar value to a piecewise linear function that needs to be sampled by the estimated lux value. Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> --- This patch depends on v5 of "ipa:: Move Pwl from Raspberry Pi" Changes in v3: - use new PointF that's a typedef of Vector Changes in v2: - s/FPoint/PointF/ - construct default Pwl with *two* points so that it actually constructs properly --- src/ipa/libipa/agc_mean_luminance.cpp | 15 +++++++++------ src/ipa/libipa/agc_mean_luminance.h | 4 ++-- 2 files changed, 11 insertions(+), 8 deletions(-)