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