[PATCH v3] ipa: libipa: Change constraint Y target to piecewise linear function
Dan Scally
dan.scally at ideasonboard.com
Thu Jun 13 23:39:00 CEST 2024
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 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.
>
>>>> 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_;
More information about the libcamera-devel
mailing list