[libcamera-devel] [PATCH v3 09/11] ipa: rkisp1: Introduce AGC
Kieran Bingham
kieran.bingham at ideasonboard.com
Tue Nov 23 21:13:52 CET 2021
Quoting Jean-Michel Hautbois (2021-11-23 17:38:42)
> Hi Kieran,
>
> On 23/11/2021 17:08, Kieran Bingham wrote:
> > Hi JM,
> >
> > Some question inline ...
> >
> > Quoting Jean-Michel Hautbois (2021-11-23 15:04:21)
> >> Now that we have IPAContext and Algorithm, we can implement a simple AGC
> >> based on the IPU3 one. It is very similar, except that there is no
> >> histogram used for an inter quantile mean. The RkISP1 is returning a 5x5
> >> array (for V10) of luminance means. Estimating the relative luminance is
> >> thus a simple mean of all the blocks already calculated by the ISP.
> >>
> >> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> >> ---
> >> src/ipa/rkisp1/algorithms/agc.cpp | 265 ++++++++++++++++++++++++++
> >> src/ipa/rkisp1/algorithms/agc.h | 55 ++++++
> >> src/ipa/rkisp1/algorithms/meson.build | 1 +
> >> src/ipa/rkisp1/ipa_context.cpp | 44 +++++
> >> src/ipa/rkisp1/ipa_context.h | 17 ++
> >> src/ipa/rkisp1/rkisp1.cpp | 72 +++----
> >> 6 files changed, 419 insertions(+), 35 deletions(-)
> >> create mode 100644 src/ipa/rkisp1/algorithms/agc.cpp
> >> create mode 100644 src/ipa/rkisp1/algorithms/agc.h
> >>
> >> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> >> new file mode 100644
> >> index 00000000..9c6d312e
> >> --- /dev/null
> >> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> >> @@ -0,0 +1,265 @@
> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >> +/*
> >> + * Copyright (C) 2021, Ideas On Board
> >> + *
> >> + * agc.cpp - AGC/AEC mean-based control algorithm
> >> + */
> >> +
> >> +#include "agc.h"
> >> +
> >> +#include <algorithm>
> >> +#include <chrono>
> >> +#include <cmath>
> >> +
> >> +#include <libcamera/base/log.h>
> >> +
> >> +#include <libcamera/ipa/core_ipa_interface.h>
> >> +
> >> +/**
> >> + * \file agc.h
> >> + */
> >> +
> >> +namespace libcamera {
> >> +
> >> +using namespace std::literals::chrono_literals;
> >> +
> >> +namespace ipa::rkisp1::algorithms {
> >> +
> >> +/**
> >> + * \class Agc
> >> + * \brief A mean-based auto-exposure algorithm
> >> + */
> >> +
> >> +LOG_DEFINE_CATEGORY(RkISP1Agc)
> >> +
> >> +/* Limits for analogue gain values */
> >> +static constexpr double kMinAnalogueGain = 1.0;
> >> +static constexpr double kMaxAnalogueGain = 8.0;
> >> +
> >> +/* \todo Honour the FrameDurationLimits control instead of hardcoding a limit */
> >> +static constexpr utils::Duration kMaxShutterSpeed = 60ms;
> >> +
> >> +/* Number of frames to wait before calculating stats on minimum exposure */
> >> +static constexpr uint32_t kNumStartupFrames = 10;
> >> +
> >> +/*
> >> + * Relative luminance target.
> >> + *
> >> + * It's a number that's chosen so that, when the camera points at a grey
> >> + * target, the resulting image brightness is considered right.
> >> + */
> >> +static constexpr double kRelativeLuminanceTarget = 0.4;
> >> +
> >> +Agc::Agc()
> >> + : frameCount_(0), lineDuration_(0s), minShutterSpeed_(0s),
> >> + maxShutterSpeed_(0s), filteredExposure_(0s), currentExposure_(0s)
> >> +{
> >> +}
> >> +
> >> +/**
> >> + * \brief Configure the AGC given a configInfo
> >> + * \param[in] context The shared IPA context
> >> + * \param[in] configInfo The IPA configuration data
> >> + *
> >> + * \return 0
> >> + */
> >> +int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
> >> +{
> >> + /* \todo use the IPAContext to provide the limits */
> >> + lineDuration_ = configInfo.lineLength * 1.0s / configInfo.pixelRate;
> >> +
> >> + minShutterSpeed_ = context.configuration.agc.minShutterSpeed;
> >> + maxShutterSpeed_ = std::min(context.configuration.agc.maxShutterSpeed,
> >> + kMaxShutterSpeed);
> >> +
> >> + minAnalogueGain_ = std::max(context.configuration.agc.minAnalogueGain, kMinAnalogueGain);
> >> + maxAnalogueGain_ = std::min(context.configuration.agc.maxAnalogueGain, kMaxAnalogueGain);
> >> +
> >> + /* Configure the default exposure and gain. */
> >> + context.frameContext.agc.gain = minAnalogueGain_;
> >> + context.frameContext.agc.exposure = 10ms / lineDuration_;
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +/**
> >> + * \brief Apply a filter on the exposure value to limit the speed of changes
> >> + */
> >> +void Agc::filterExposure()
> >> +{
> >> + double speed = 0.2;
> >> +
> >> + /* Adapt instantly if we are in startup phase */
> >> + if (frameCount_ < kNumStartupFrames)
> >> + speed = 1.0;
> >> +
> >> + if (filteredExposure_ == 0s) {
> >> + /* DG stands for digital gain.*/
> >
> > I think that comment is now stale.
> > If it's stale in the IPU3 as well, can you remove it there too please?
>
> Sure, in a dedicated patch for IPU3 ? In the same series ?
It would be a dedicated patch for the IPU3 now as it's an existing bug.
But here, it can simply be dropped.
> >> + filteredExposure_ = currentExposure_;
> >> + } else {
> >> + /*
> >> + * If we are close to the desired result, go faster to avoid making
> >
> > faster ? or slower?
> >
> > Oh - ok this checks out ;-) I almost got caught out forgetting that the
> > square root of a number less than one - increases towards one ;=)
>
> You auto-replied, that's an easy answer for me :-).
>
> >
> >
> >> + * multiple micro-adjustments.
> >> + * \todo Make this customisable?
> >> + */
> >> + if (filteredExposure_ < 1.2 * currentExposure_ &&
> >> + filteredExposure_ > 0.8 * currentExposure_)
> >> + speed = sqrt(speed);
> >> +
> >> + filteredExposure_ = speed * currentExposure_ +
> >> + filteredExposure_ * (1.0 - speed);
> >> + }
> >> +
> >> + LOG(RkISP1Agc, Debug) << "After filtering, total_exposure " << filteredExposure_;
> >> +}
> >> +
> >> +/**
> >> + * \brief Estimate the new exposure and gain values
> >> + * \param[inout] frameContext The shared IPA frame Context
> >> + * \param[in] yGain The gain calculated on the current brightness level
> >> + */
> >> +void Agc::computeExposure(IPAFrameContext &frameContext, double yGain)
> >> +{
> >> + /* Get the effective exposure and gain applied on the sensor. */
> >> + uint32_t exposure = frameContext.sensor.exposure;
> >> + double analogueGain = frameContext.sensor.gain;
> >> +
> >> + /* Consider within 1% of the target as correctly exposed */
> >> + if (std::abs(yGain - 1.0) < 0.01)
> >> + LOG(RkISP1Agc, Debug) << "We are well exposed (iqMean = "
> >> + << yGain << ")";
> >> +
> >> + /* extracted from Rpi::Agc::computeTargetExposure */
> >> +
> >> + /* Calculate the shutter time in seconds */
> >> + utils::Duration currentShutter = exposure * lineDuration_;
> >> +
> >> + /*
> >> + * Update the exposure value for the next computation using the values
> >> + * of exposure and gain really used by the sensor.
> >> + */
> >> + utils::Duration effectiveExposureValue = currentShutter * analogueGain;
> >> +
> >> + LOG(RkISP1Agc, Debug) << "Actual total exposure " << currentShutter * analogueGain
> >> + << " Shutter speed " << currentShutter
> >> + << " Gain " << analogueGain
> >> + << " Needed ev gain " << yGain;
> >> +
> >> + /*
> >> + * Calculate the current exposure value for the scene as the latest
> >> + * exposure value applied multiplied by the new estimated gain.
> >> + */
> >> + currentExposure_ = effectiveExposureValue * yGain;
> >> +
> >> + /* Clamp the exposure value to the min and max authorized */
> >> + utils::Duration maxTotalExposure = maxShutterSpeed_ * maxAnalogueGain_;
> >> + currentExposure_ = std::min(currentExposure_, maxTotalExposure);
> >> + LOG(RkISP1Agc, Debug) << "Target total exposure " << currentExposure_
> >> + << ", maximum is " << maxTotalExposure;
> >> +
> >> + /* \todo: estimate if we need to desaturate */
> >> + filterExposure();
> >
> > Would returning the filtedExposure value from the function make it
> > clearer or more explicit than it gets used ?
>
> Why not, it is also true for IPU3 then.
That's two patches (separate series) for the IPU3 already then.
That's the dilemma with having multiple similar versions. Change one,
and you need to change all the other places that do the same thing ;-)
> >> +
> >> + /* Divide the exposure value as new exposure and gain values */
> >> + utils::Duration exposureValue = filteredExposure_;
> >
> > i.e.
> > utils::Duration exposureValue = filteredExposure();
> > ?
> >
> >> + utils::Duration shutterTime;
> >> +
> >> + /*
> >> + * Push the shutter time up to the maximum first, and only then
> >> + * increase the gain.
> >> + */
> >> + shutterTime = std::clamp<utils::Duration>(exposureValue / minAnalogueGain_,
> >> + minShutterSpeed_, maxShutterSpeed_);
> >> + double stepGain = std::clamp(exposureValue / shutterTime,
> >> + minAnalogueGain_, maxAnalogueGain_);
> >> + LOG(RkISP1Agc, Debug) << "Divided up shutter and gain are "
> >> + << shutterTime << " and "
> >> + << stepGain;
> >> +
> >> + /* Update the estimated exposure and gain. */
> >> + frameContext.agc.exposure = shutterTime / lineDuration_;
> >> + frameContext.agc.gain = stepGain;
> >> +}
> >> +
> >> +/**
> >> + * \brief Estimate the relative luminance of the frame with a given gain
> >> + * \param[in] ae The RkISP1 statistics and ISP results
> >> + * \param[in] gain The gain calculated on the current brightness level
> >> + * \return The relative luminance
> >> + *
> >> + * This function estimates the average relative luminance of the frame that
> >> + * would be output by the sensor if an additional \a gain was applied.
> >> + *
> >> + * The estimation is based on the AE statistics for the current frame. Y
> >> + * averages for all cells are first multiplied by the gain, and then saturated
> >> + * to approximate the sensor behaviour at high brightness values. The
> >> + * approximation is quite rough, as it doesn't take into account non-linearities
> >> + * when approaching saturation.
> >> + *
> >> + * The values are normalized to the [0.0, 1.0] range, where 1.0 corresponds to a
> >> + * theoretical perfect reflector of 100% reference white.
> >> + *
> >> + * More detailed information can be found in:
> >> + * https://en.wikipedia.org/wiki/Relative_luminance
> >> + */
> >> +double Agc::estimateLuminance(const rkisp1_cif_isp_ae_stat *ae,
> >> + double gain)
> >> +{
> >> + double ySum = 0.0;
> >> + unsigned int num = 0;
> >> +
> >> + /* Sum the averages, saturated to 255. */
> >> + for (unsigned int aeCell = 0; aeCell < numCells_; aeCell++) {
> >> + ySum += std::min(ae->exp_mean[aeCell] * gain, 255.0);
> >> + num++;
> >> + }
> >> +
> >> + /* \todo Weight with the AWB gains */
> >> +
> >> + return ySum / num / 255;
> >> +}
> >> +
> >> +/**
> >> + * \brief Process RkISP1 statistics, and run AGC operations
> >> + * \param[in] context The shared IPA context
> >> + * \param[in] stats The RKIsp1 statistics and ISP results
> >> + *
> >> + * Identify the current image brightness, and use that to estimate the optimal
> >> + * new exposure and gain for the scene.
> >> + */
> >> +void Agc::process(IPAContext &context, const rkisp1_stat_buffer *stats)
> >> +{
> >> + const rkisp1_cif_isp_stat *params = &stats->params;
> >> + ASSERT(stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP);
> >> +
> >> + const rkisp1_cif_isp_ae_stat *ae = ¶ms->ae;
> >> +
> >> + /*
> >> + * Estimate the gain needed to achieve a relative luminance target. 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.
> >> + */
> >> + double yGain = 1.0;
> >> + double yTarget = kRelativeLuminanceTarget;
> >> +
> >> + for (unsigned int i = 0; i < 8; i++) {
> >> + double yValue = estimateLuminance(ae, yGain);
> >> + double extra_gain = std::min(10.0, yTarget / (yValue + .001));
> >> +
> >> + yGain *= extra_gain;
> >> + LOG(RkISP1Agc, Debug) << "Y value: " << yValue
> >> + << ", Y target: " << yTarget
> >> + << ", gives gain " << yGain;
> >> + if (extra_gain < 1.01)
> >> + break;
> >> + }
> >> +
> >> + computeExposure(context.frameContext, yGain);
> >
> > Does the algorithm need to track the frameCount internally? Can it get
> > it from the frame / index of the request / stats?
> >
>
> I have the introduction of frameId in the IPAFrameContext later, do you
> want it now too ? Same for IPU3 then (it makes this series deviate a bit
> of the initial subject probably but why not).
Whenever you like. It just really stands out - so I'll keep mentioning
it whenever I see it ;-)
Again, if this is like this in the IPU3 - a patch to clean up there
first makes sense.
> >> + frameCount_++;
> >> +}
> >> +
> >> +} /* namespace ipa::rkisp1::algorithms */
> >> +
> >> +} /* namespace libcamera */
> >> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h
> >> new file mode 100644
> >> index 00000000..83159743
> >> --- /dev/null
> >> +++ b/src/ipa/rkisp1/algorithms/agc.h
> >> @@ -0,0 +1,55 @@
> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >> +/*
> >> + * Copyright (C) 2021, Ideas On Board
> >> + *
> >> + * agc.h - RkISP1 AGC/AEC mean-based control algorithm
> >> + */
> >> +#ifndef __LIBCAMERA_RKISP1_ALGORITHMS_AGC_H__
> >> +#define __LIBCAMERA_RKISP1_ALGORITHMS_AGC_H__
> >> +
> >> +#include <linux/rkisp1-config.h>
> >> +
> >> +#include <libcamera/base/utils.h>
> >> +
> >> +#include <libcamera/geometry.h>
> >> +
> >> +#include "algorithm.h"
> >> +
> >> +namespace libcamera {
> >> +
> >> +struct IPACameraSensorInfo;
> >> +
> >> +namespace ipa::rkisp1::algorithms {
> >> +
> >> +class Agc : public Algorithm
> >> +{
> >> +public:
> >> + Agc();
> >> + ~Agc() = default;
> >> +
> >> + int configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override;
> >> + void process(IPAContext &context, const rkisp1_stat_buffer *stats) override;
> >> +
> >> +private:
> >> + void filterExposure();
> >> + void computeExposure(IPAFrameContext &frameContext, double yGain);
> >> + double estimateLuminance(const rkisp1_cif_isp_ae_stat *ae, double gain);
> >> +
> >> + uint64_t frameCount_;
> >> +
> >> + utils::Duration lineDuration_;
> >> + utils::Duration minShutterSpeed_;
> >> + utils::Duration maxShutterSpeed_;
> >> +
> >> + double minAnalogueGain_;
> >> + double maxAnalogueGain_;
> >
> > Do all these need to be duplicated from the context? Can't we use the
> > values from the IPASessionConfiguration?
>
> We can. It means we need to pass the IPASessionConfiguration to the
> computeExposure() call, and not only the IPAFrameContext.
So that's just passing the IPAContext then...
> Is there a real issue with having those cached (in C++ or memory
> footprint maybe) ?
It's more the 'which version should I reference/read' if it's
duplicated.
Sure it's a little bit more memory - but I don't think the cost of a few
vars is a big deal here.
To me - the IPAContext is the stateful storage area for all memory that
needs to be accessed by the 'core' and the 'algorithms'.
If it's only accessed by the algorithms, then it goes in private
members in the algo. But if it had to be passed in, or accessed in some
way, then it's in the IPAContext.
> >> +
> >> + utils::Duration filteredExposure_;
> >> + utils::Duration currentExposure_;
> >> +};
> >> +
> >> +} /* namespace ipa::rkisp1::algorithms */
> >> +
> >> +} /* namespace libcamera */
> >> +
> >> +#endif /* __LIBCAMERA_RKISP1_ALGORITHMS_AGC_H__ */
> >> diff --git a/src/ipa/rkisp1/algorithms/meson.build b/src/ipa/rkisp1/algorithms/meson.build
> >> index 1c6c59cf..a19c1a4f 100644
> >> --- a/src/ipa/rkisp1/algorithms/meson.build
> >> +++ b/src/ipa/rkisp1/algorithms/meson.build
> >> @@ -1,4 +1,5 @@
> >> # SPDX-License-Identifier: CC0-1.0
> >>
> >> rkisp1_ipa_algorithms = files([
> >> + 'agc.cpp',
> >> ])
> >> diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
> >> index 819b2c73..16fc248f 100644
> >> --- a/src/ipa/rkisp1/ipa_context.cpp
> >> +++ b/src/ipa/rkisp1/ipa_context.cpp
> >> @@ -55,4 +55,48 @@ namespace libcamera::ipa::rkisp1 {
> >> * are run. This needs to be turned into real per-frame data storage.
> >> */
> >>
> >> +/**
> >> + * \var IPASessionConfiguration::agc
> >> + * \brief AGC parameters configuration of the IPA
> >> + *
> >> + * \var IPASessionConfiguration::agc.minShutterSpeed
> >> + * \brief Minimum shutter speed supported with the configured sensor
> >> + *
> >> + * \var IPASessionConfiguration::agc.maxShutterSpeed
> >> + * \brief Maximum shutter speed supported with the configured sensor
> >> + *
> >> + * \var IPASessionConfiguration::agc.minAnalogueGain
> >> + * \brief Minimum analogue gain supported with the configured sensor
> >> + *
> >> + * \var IPASessionConfiguration::agc.maxAnalogueGain
> >> + * \brief Maximum analogue gain supported with the configured sensor
> >> + */
> >> +
> >> +/**
> >> + * \var IPAFrameContext::agc
> >> + * \brief Context for the Automatic Gain Control algorithm
> >> + *
> >> + * The exposure and gain determined are expected to be applied to the sensor
> >> + * at the earliest opportunity.
> >> + *
> >> + * \var IPAFrameContext::agc.exposure
> >> + * \brief Exposure time expressed as a number of lines
> >> + *
> >> + * \var IPAFrameContext::agc.gain
> >> + * \brief Analogue gain multiplier
> >> + *
> >> + * The gain should be adapted to the sensor specific gain code before applying.
> >> + */
> >> +
> >> +/**
> >> + * \var IPAFrameContext::sensor
> >> + * \brief Effective sensor values
> >> + *
> >> + * \var IPAFrameContext::sensor.exposure
> >> + * \brief Exposure time expressed as a number of lines
> >> + *
> >> + * \var IPAFrameContext::sensor.gain
> >> + * \brief Analogue gain multiplier
> >> + */
> >> +
> >> } /* namespace libcamera::ipa::rkisp1 */
> >> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> >> index ff40efe3..8e756fb1 100644
> >> --- a/src/ipa/rkisp1/ipa_context.h
> >> +++ b/src/ipa/rkisp1/ipa_context.h
> >> @@ -8,14 +8,31 @@
> >> #ifndef __LIBCAMERA_RKISP1_IPA_CONTEXT_H__
> >> #define __LIBCAMERA_RKISP1_IPA_CONTEXT_H__
> >>
> >> +#include <libcamera/base/utils.h>
> >> +
> >> namespace libcamera {
> >>
> >> namespace ipa::rkisp1 {
> >>
> >> struct IPASessionConfiguration {
> >> + struct {
> >> + utils::Duration minShutterSpeed;
> >> + utils::Duration maxShutterSpeed;
> >> + double minAnalogueGain;
> >> + double maxAnalogueGain;
> >> + } agc;
> >> };
> >>
> >> struct IPAFrameContext {
> >> + struct {
> >> + uint32_t exposure;
> >> + double gain;
> >> + } agc;
> >> +
> >> + struct {
> >> + uint32_t exposure;
> >> + double gain;
> >> + } sensor;
> >> };
> >>
> >> struct IPAContext {
> >> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> >> index b5aa93f8..89d98b6c 100644
> >> --- a/src/ipa/rkisp1/rkisp1.cpp
> >> +++ b/src/ipa/rkisp1/rkisp1.cpp
> >> @@ -25,6 +25,7 @@
> >>
> >> #include <libcamera/internal/mapped_framebuffer.h>
> >>
> >> +#include "algorithms/agc.h"
> >> #include "algorithms/algorithm.h"
> >> #include "libipa/camera_sensor_helper.h"
> >>
> >> @@ -32,6 +33,8 @@ namespace libcamera {
> >>
> >> LOG_DEFINE_CATEGORY(IPARkISP1)
> >>
> >> +using namespace std::literals::chrono_literals;
> >> +
> >> namespace ipa::rkisp1 {
> >>
> >> class IPARkISP1 : public IPARkISP1Interface
> >> @@ -77,6 +80,8 @@ private:
> >> unsigned int hwGammaOutMaxSamples_;
> >> unsigned int hwHistogramWeightGridsSize_;
> >>
> >> + utils::Duration lineDuration_;
> >> +
> >
> > Does this need to be stored privately? or can it be stored in the
> > Context?
> >
> > Or rather, does this get used by the algorithms directly? If not - then
> > it's fine here.
>
> I have, in IPU3, cached it because it was used at configure time and
> when updating controls after the algorithm have run.
>
> Are you trying to remove all the cached values by any chance :-) ?
No, not remove them, just considering /where/ to cache them ...
If it's only used in the IPARkISP1 core class, then it's fine here.
> >
> >> /* Interface to the Camera Helper */
> >> std::unique_ptr<CameraSensorHelper> camHelper_;
> >>
> >> @@ -120,6 +125,9 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision)
> >> return -ENODEV;
> >> }
> >>
> >> + /* Construct our Algorithms */
> >> + algorithms_.push_back(std::make_unique<algorithms::Agc>());
> >> +
> >> return 0;
> >> }
> >>
> >> @@ -171,9 +179,29 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,
> >> << "Exposure: " << minExposure_ << "-" << maxExposure_
> >> << " Gain: " << minGain_ << "-" << maxGain_;
> >>
> >> + lineDuration_ = info.lineLength * 1.0s / info.pixelRate;
> >> +
> >> /* Clean context at configuration */
> >> context_ = {};
> >>
> >> + /*
> >> + * When the AGC computes the new exposure values for a frame, it needs
> >> + * to know the limits for shutter speed and analogue gain.
> >> + * As it depends on the sensor, update it with the controls.
> >> + *
> >> + * \todo take VBLANK into account for maximum shutter speed
> >> + */
> >> + context_.configuration.agc.minShutterSpeed = minExposure_ * lineDuration_;
> >> + context_.configuration.agc.maxShutterSpeed = maxExposure_ * lineDuration_;
> >> + context_.configuration.agc.minAnalogueGain = camHelper_->gain(minGain_);
> >> + context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain_);
> >> +
> >> + for (auto const &algo : algorithms_) {
> >> + int ret = algo->configure(context_, info);
> >> + if (ret)
> >> + return ret;
> >> + }
> >> +
> >> return 0;
> >> }
> >>
> >> @@ -218,6 +246,9 @@ void IPARkISP1::processEvent(const RkISP1Event &event)
> >> reinterpret_cast<rkisp1_stat_buffer *>(
> >> mappedBuffers_.at(bufferId).planes()[0].data());
> >>
> >> + context_.frameContext.sensor.exposure = event.sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> >> + context_.frameContext.sensor.gain = camHelper_->gain(event.sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
> >> +
> >> updateStatistics(frame, stats);
> >> break;
> >> }
> >> @@ -262,44 +293,12 @@ void IPARkISP1::queueRequest(unsigned int frame, rkisp1_params_cfg *params,
> >> void IPARkISP1::updateStatistics(unsigned int frame,
> >> const rkisp1_stat_buffer *stats)
> >> {
> >> - const rkisp1_cif_isp_stat *params = &stats->params;
> >> unsigned int aeState = 0;
> >>
> >> - if (stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP) {
> >> - const rkisp1_cif_isp_ae_stat *ae = ¶ms->ae;
> >> -
> >> - const unsigned int target = 60;
> >> + for (auto const &algo : algorithms_)
> >> + algo->process(context_, stats);
> >>
> >> - unsigned int value = 0;
> >> - unsigned int num = 0;
> >> - for (unsigned int i = 0; i < hwAeMeanMax_; i++) {
> >> - if (ae->exp_mean[i] <= 15)
> >> - continue;
> >> -
> >> - value += ae->exp_mean[i];
> >> - num++;
> >> - }
> >> - value /= num;
> >> -
> >> - double factor = (double)target / value;
> >> -
> >> - if (frame % 3 == 0) {
> >> - double exposure;
> >> -
> >> - exposure = factor * exposure_ * gain_ / minGain_;
> >> - exposure_ = std::clamp<uint64_t>((uint64_t)exposure,
> >> - minExposure_,
> >> - maxExposure_);
> >> -
> >> - exposure = exposure / exposure_ * minGain_;
> >> - gain_ = std::clamp<uint64_t>((uint64_t)exposure,
> >> - minGain_, maxGain_);
> >> -
> >> - setControls(frame + 1);
> >> - }
> >> -
> >> - aeState = fabs(factor - 1.0f) < 0.05f ? 2 : 1;
> >> - }
> >> + setControls(frame + 1);
> >
> > Is + 1 the correct target frame?
> > Is that always true even if there are multiple frames queued?
> >
>
> I have to be honest here: I reused the existing code. Calling
> setControls(frame) gives the same output, so I can't really say if it is
> correct or not, I did not dig it.
What do you do in the IPU3?
setControls(frame + 1) sounds to me like a really bad attempt at
'aiming' for a specific time to set the controls, because we're in
parseStatistics, so we're processing the image and statistics that were
generated by frame number 'frame' ... so it's saying for the 'next'
frame I'll set these controls.
It sounds like it's just 'wrong' but works because delayedControls has
probably already passed that framenumber so sets them somewhat
immediately...
If this is what IPU3 does, then fine keep it as is. But I would say we
shouldn't diverge too much between the two implementations if we can.
If you're not sure which frame the controls should be written at, maybe
we should add a todo to investigate?
> >> metadataReady(frame, aeState);
> >> }
> >> @@ -309,6 +308,9 @@ void IPARkISP1::setControls(unsigned int frame)
> >> RkISP1Action op;
> >> op.op = ActionV4L2Set;
> >>
> >> + exposure_ = context_.frameContext.agc.exposure;
> >> + gain_ = camHelper_->gainCode(context_.frameContext.agc.gain);
> >> +
> >> ControlList ctrls(ctrls_);
> >> ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_));
> >> ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain_));
> >> --
> >> 2.32.0
> >>
More information about the libcamera-devel
mailing list