[PATCH v3] ipa: libipa: Change constraint Y target to piecewise linear function
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Jun 12 00:48:10 CEST 2024
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 at 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_;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list