[PATCH v5 2/3] ipa: libipa: agc: Change luminance target to piecewise linear function
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Jun 11 18:31:34 CEST 2024
Hi Paul,
Thank you for the patch.
On Fri, Jun 07, 2024 at 05:03:29PM +0900, 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.
One thing I would like to capture somewhere is the rationale for this.
Why should the target luminance (essentially the average Y value across
the image if we simplify it) depend on how bright the scene is ? I
looked at the RPi tuning files, and they push the target luminance
slightly up the brighter the scene is. Is this because we want to image
to appear brighter to a human eye when we know that the scene is brigher
?
> 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.
Sounds fine, but I would still like to see documentation of the format
:-) Or, as a first step, at least a sample tuning file in-tree.
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> Reviewed-by: Stefan Klug <stefan.klug at ideasonboard.com>
> Reviewed-by: Daniel Scally <dan.scally at ideasonboard.com>
>
> ---
> Changes in v5:
> - use new PointF that's a typedef of Vector
>
> Changes in v4:
> - construct default pwl with *two* points so that it succeeds
>
> 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 | 32 +++++++++++++++++++++------
> src/ipa/libipa/agc_mean_luminance.h | 7 +++---
> src/ipa/rkisp1/algorithms/agc.cpp | 5 ++++-
> 4 files changed, 37 insertions(+), 12 deletions(-)
>
> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> index 0e0114f6d..7f71bec06 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 271b5ae4b..10c16850d 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,17 @@ 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<Pwl::PointF> points = { Pwl::PointF({ 0, kDefaultRelativeLuminanceTarget }),
> + Pwl::PointF({ 10000, kDefaultRelativeLuminanceTarget }) };
> + relativeLuminanceTarget_ = Pwl(points);
> }
>
> void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)
> @@ -421,11 +430,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
s/$/./
I would *really* like checkstyle to catch this, but parsing
documentation isn't easy :-S
This being said, the same comment is already present inside the
function. Is there a reason to duplicate it ?
> + *
> * \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 +536,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 +548,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 +558,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 3bbafd978..e9f1f2095 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 "
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list