[PATCH v4 6/8] ipa: rkisp1: Port to the new AEGC controls
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Dec 10 14:49:12 CET 2024
Hi Paul,
Thank you for the patch.
On Thu, Dec 05, 2024 at 08:22:39PM +0900, Paul Elder wrote:
> The newly introduced controls to drive the AEGC algorithm allow
> controlling the computation of the exposure time and analogue gain
> separately.
>
> Augument the RkISP1 AEGC implementation to handle the exposure and gain
> controls separately using the new controls.
>
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
>
> ---
> Changes in v4:
> - make auto the default value in the control infos
> - implement the expected behavior when transitioning between manual and
> auto modes
>
> New in v3
>
> Back 2 years ago in v2 RkISP1 didn't yet support AeEnable properly yet
> so the AeEnable control was simply removed.
> ---
> src/ipa/rkisp1/algorithms/agc.cpp | 102 +++++++++++++++++++++++++-----
> src/ipa/rkisp1/ipa_context.cpp | 22 +++++--
> src/ipa/rkisp1/ipa_context.h | 8 ++-
> 3 files changed, 110 insertions(+), 22 deletions(-)
>
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index 40e5a8f481b2..c92115e8cc0d 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -148,7 +148,14 @@ int Agc::init(IPAContext &context, const YamlObject &tuningData)
> if (ret)
> return ret;
>
> - context.ctrlMap[&controls::AeEnable] = ControlInfo(false, true);
> + context.ctrlMap[&controls::ExposureTimeMode] =
> + ControlInfo(static_cast<int32_t>(controls::ExposureTimeModeAuto),
> + static_cast<int32_t>(controls::ExposureTimeModeManual),
> + static_cast<int32_t>(controls::ExposureTimeModeAuto));
> + context.ctrlMap[&controls::AnalogueGainMode] =
> + ControlInfo(static_cast<int32_t>(controls::AnalogueGainModeAuto),
> + static_cast<int32_t>(controls::AnalogueGainModeManual),
> + static_cast<int32_t>(controls::AnalogueGainModeAuto));
> context.ctrlMap.merge(controls());
>
> return 0;
> @@ -169,7 +176,8 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
> 10ms / context.configuration.sensor.lineDuration;
> context.activeState.agc.manual.gain = context.activeState.agc.automatic.gain;
> context.activeState.agc.manual.exposure = context.activeState.agc.automatic.exposure;
> - context.activeState.agc.autoEnabled = !context.configuration.raw;
> + context.activeState.agc.autoExposureEnabled = !context.configuration.raw;
> + context.activeState.agc.autoGainEnabled = !context.configuration.raw;
>
> context.activeState.agc.constraintMode =
> static_cast<controls::AeConstraintModeEnum>(constraintModes().begin()->first);
> @@ -215,18 +223,44 @@ void Agc::queueRequest(IPAContext &context,
> auto &agc = context.activeState.agc;
>
> if (!context.configuration.raw) {
> - const auto &agcEnable = controls.get(controls::AeEnable);
> - if (agcEnable && *agcEnable != agc.autoEnabled) {
> - agc.autoEnabled = *agcEnable;
> + const auto &aeEnable = controls.get(controls::ExposureTimeMode);
> + if (aeEnable &&
> + (*aeEnable == controls::ExposureTimeModeAuto) != agc.autoExposureEnabled) {
> + agc.autoExposureEnabled = (*aeEnable == controls::ExposureTimeModeAuto);
>
> LOG(RkISP1Agc, Debug)
> - << (agc.autoEnabled ? "Enabling" : "Disabling")
> - << " AGC";
> + << (agc.autoExposureEnabled ? "Enabling" : "Disabling")
> + << " AGC (exposure)";
> +
> + /*
> + * If we go from auto -> manual with no manual control
> + * set, use the last computed value, which we don't
> + * know until prepare() so save this information
s/$/./
Same below where applicable.
> + */
> + if (!agc.autoExposureEnabled && !controls.get(controls::ExposureTime))
> + frameContext.agc.autoExposureModeChange = true;
frameContext.agc.autoExposureModeChange is documented as "Indicate if
autoExposureEnabled has changed between the previous and current frame",
but it records something more specific. The documentation should be
updated. Same for autoGainModeChange.
A long time ago I envisioned that this could be checked in prepare() by
looking at the context for the previous frame. We don't have a good
infrastructure to do so yet, so this is fine. I wonder if we should try
get there though. Maybe a \todo comment would be useful ?
> + }
> +
> + const auto &agEnable = controls.get(controls::AnalogueGainMode);
> + if (agEnable &&
> + (*agEnable == controls::AnalogueGainModeAuto) != agc.autoGainEnabled) {
> + agc.autoGainEnabled = (*agEnable == controls::AnalogueGainModeAuto);
> +
> + LOG(RkISP1Agc, Debug)
> + << (agc.autoGainEnabled ? "Enabling" : "Disabling")
> + << " AGC (gain)";
> + /*
> + * If we go from auto -> manual with no manual control
> + * set, use the last computed value, which we don't
> + * know until prepare() so save this information
> + */
> + if (!agc.autoGainEnabled && !controls.get(controls::AnalogueGain))
> + frameContext.agc.autoGainModeChange = true;
> }
> }
>
> const auto &exposure = controls.get(controls::ExposureTime);
> - if (exposure && !agc.autoEnabled) {
> + if (exposure && !agc.autoExposureEnabled) {
> agc.manual.exposure = *exposure * 1.0us
> / context.configuration.sensor.lineDuration;
>
> @@ -235,18 +269,19 @@ void Agc::queueRequest(IPAContext &context,
> }
>
> const auto &gain = controls.get(controls::AnalogueGain);
> - if (gain && !agc.autoEnabled) {
> + if (gain && !agc.autoGainEnabled) {
> agc.manual.gain = *gain;
>
> LOG(RkISP1Agc, Debug) << "Set gain to " << agc.manual.gain;
> }
>
> - frameContext.agc.autoEnabled = agc.autoEnabled;
> + frameContext.agc.autoExposureEnabled = agc.autoExposureEnabled;
> + frameContext.agc.autoGainEnabled = agc.autoGainEnabled;
>
> - if (!frameContext.agc.autoEnabled) {
> + if (!frameContext.agc.autoExposureEnabled)
> frameContext.agc.exposure = agc.manual.exposure;
> + if (!frameContext.agc.autoGainEnabled)
> frameContext.agc.gain = agc.manual.gain;
> - }
>
> const auto &meteringMode = controls.get(controls::AeMeteringMode);
> if (meteringMode) {
> @@ -283,9 +318,26 @@ void Agc::queueRequest(IPAContext &context,
> void Agc::prepare(IPAContext &context, const uint32_t frame,
> IPAFrameContext &frameContext, RkISP1Params *params)
> {
> - if (frameContext.agc.autoEnabled) {
> - frameContext.agc.exposure = context.activeState.agc.automatic.exposure;
> - frameContext.agc.gain = context.activeState.agc.automatic.gain;
> + uint32_t activeAutoExposure = context.activeState.agc.automatic.exposure;
> + double activeAutoGain = context.activeState.agc.automatic.gain;
> +
> + /* Populate exposure and gain in auto mode */
> + if (frameContext.agc.autoExposureEnabled)
> + frameContext.agc.exposure = activeAutoExposure;
> + if (frameContext.agc.autoGainEnabled)
> + frameContext.agc.gain = activeAutoGain;
> +
> + /*
> + * Populate manual exposure and gain from the active auto values when
> + * transitioning from auto to manual
> + */
> + if (!frameContext.agc.autoExposureEnabled && frameContext.agc.autoExposureModeChange) {
> + context.activeState.agc.manual.exposure = activeAutoExposure;
> + frameContext.agc.exposure = activeAutoExposure;
> + }
> + if (!frameContext.agc.autoGainEnabled && frameContext.agc.autoGainModeChange) {
> + context.activeState.agc.manual.gain = activeAutoGain;
> + frameContext.agc.gain = activeAutoGain;
> }
>
> if (frame > 0 && !frameContext.agc.updateMetering)
> @@ -333,7 +385,14 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,
> * frameContext.sensor.exposure;
> metadata.set(controls::AnalogueGain, frameContext.sensor.gain);
> metadata.set(controls::ExposureTime, exposureTime.get<std::micro>());
> - metadata.set(controls::AeEnable, frameContext.agc.autoEnabled);
> + metadata.set(controls::ExposureTimeMode,
> + frameContext.agc.autoExposureEnabled
> + ? controls::ExposureTimeModeAuto
> + : controls::ExposureTimeModeManual);
> + metadata.set(controls::AnalogueGainMode,
> + frameContext.agc.autoGainEnabled
> + ? controls::AnalogueGainModeAuto
> + : controls::AnalogueGainModeManual);
>
> /* \todo Use VBlank value calculated from each frame exposure. */
> uint32_t vTotal = context.configuration.sensor.size.height
> @@ -459,6 +518,17 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> / context.configuration.sensor.lineDuration;
> activeState.agc.automatic.gain = aGain;
>
> + /*
> + * Populate auto exposure and gain from the active manual values when
> + * transitioning from manual to auto
> + *
> + * The frame context doesn't need to be populated here as it will be populated in prepare()
Line wrap.
> + */
> + if (frameContext.agc.autoGainEnabled && frameContext.agc.autoGainModeChange)
> + context.activeState.agc.automatic.exposure = context.activeState.agc.manual.exposure;
> + if (frameContext.agc.autoGainEnabled && frameContext.agc.autoGainModeChange)
> + context.activeState.agc.automatic.gain = context.activeState.agc.manual.gain;
> +
Is that all that is needed ? When running in e.g. auto-exposure and
manual gain, shouldn't the AGC algorithm take that the manual gain into
account when dividing the total exposure between exposure time and gain
? With this code the algorithm will compute an exposure time and a gain,
and the gain will the be overridden, messing up the total exposure
value.
> fillMetadata(context, frameContext, metadata);
> expMeans_ = {};
> }
> diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
> index 80b99df8eaf8..c7ccc3093078 100644
> --- a/src/ipa/rkisp1/ipa_context.cpp
> +++ b/src/ipa/rkisp1/ipa_context.cpp
> @@ -165,8 +165,11 @@ namespace libcamera::ipa::rkisp1 {
> * \var IPAActiveState::agc.automatic.gain
> * \brief Automatic analogue gain multiplier
> *
> - * \var IPAActiveState::agc.autoEnabled
> - * \brief Manual/automatic AGC state as set by the AeEnable control
> + * \var IPAActiveState::agc.autoExposureEnabled
> + * \brief Manual/automatic AGC state (exposure) as set by the ExposureTimeMode control
> + *
> + * \var IPAActiveState::agc.autoGainEnabled
> + * \brief Manual/automatic AGC state (gain) as set by the AnalogueGainMode control
> *
> * \var IPAActiveState::agc.constraintMode
> * \brief Constraint mode as set by the AeConstraintMode control
> @@ -289,8 +292,11 @@ namespace libcamera::ipa::rkisp1 {
> *
> * The gain should be adapted to the sensor specific gain code before applying.
> *
> - * \var IPAFrameContext::agc.autoEnabled
> - * \brief Manual/automatic AGC state as set by the AeEnable control
> + * \var IPAFrameContext::agc.autoExposureEnabled
> + * \brief Manual/automatic AGC state (exposure) as set by the ExposureTimeMode control
> + *
> + * \var IPAFrameContext::agc.autoGainEnabled
> + * \brief Manual/automatic AGC state (gain) as set by the AnalogueGainMode control
> *
> * \var IPAFrameContext::agc.constraintMode
> * \brief Constraint mode as set by the AeConstraintMode control
> @@ -306,6 +312,14 @@ namespace libcamera::ipa::rkisp1 {
> *
> * \var IPAFrameContext::agc.updateMetering
> * \brief Indicate if new ISP AGC metering parameters need to be applied
> + *
> + * \var IPAFrameContext::agc.autoExposureModeChange
> + * \brief Indicate if autoExposureEnabled has changed between the previous and
> + * current frame
> + *
> + * \var IPAFrameContext::agc.autoGainModeChange
> + * \brief Indicate if autoGainEnabled has changed between the previous and
> + * current frame
> */
>
> /**
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index deb8c196f1b8..bc3d5a24be3a 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -79,7 +79,8 @@ struct IPAActiveState {
> double gain;
> } automatic;
>
> - bool autoEnabled;
> + bool autoExposureEnabled;
> + bool autoGainEnabled;
> controls::AeConstraintModeEnum constraintMode;
> controls::AeExposureModeEnum exposureMode;
> controls::AeMeteringModeEnum meteringMode;
> @@ -124,12 +125,15 @@ struct IPAFrameContext : public FrameContext {
> struct {
> uint32_t exposure;
> double gain;
> - bool autoEnabled;
> + bool autoExposureEnabled;
> + bool autoGainEnabled;
> controls::AeConstraintModeEnum constraintMode;
> controls::AeExposureModeEnum exposureMode;
> controls::AeMeteringModeEnum meteringMode;
> utils::Duration maxFrameDuration;
> bool updateMetering;
> + bool autoExposureModeChange;
> + bool autoGainModeChange;
> } agc;
>
> struct {
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list