[PATCH v3 3/3] ipa: rkisp1: agc: Plumb mode-selection and frame duration controls
Dan Scally
dan.scally at ideasonboard.com
Mon May 20 14:34:56 CEST 2024
Hi Paul, thanks for the patch
On 17/05/2024 09:08, Paul Elder wrote:
> Plumb controls for setting metering mode, exposure mode, constraint
> mode, and frame duration limits. Also report them as available controls,
> as well as in metadata.
>
> While at it, add the missing #include for tuple, as a std::tie is used.
>
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
>
> ---
> Changes in v3:
> - prevent maxShutterSpeed (for setLimits) from choosing 0 shutter speed
> if one of the inputs std::min() is not set (this fixes an assertion
> failure in the exposure mode helper)
>
> Changes in v2:
> - add #include <tuple>
> - don't overwrite metering/exposure/constraint mode controls to default
> if no control is supplied, and retain the old value instead (same for
> frame duration limits)
> ---
> src/ipa/rkisp1/algorithms/agc.cpp | 55 ++++++++++++++++++++++++---
> src/ipa/rkisp1/algorithms/algorithm.h | 2 +
> src/ipa/rkisp1/ipa_context.h | 10 ++++-
> src/ipa/rkisp1/rkisp1.cpp | 10 +++++
> 4 files changed, 70 insertions(+), 7 deletions(-)
>
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index 1c9872d02..ce8beae24 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -10,6 +10,8 @@
> #include <algorithm>
> #include <chrono>
> #include <cmath>
> +#include <tuple>
> +#include <vector>
>
> #include <libcamera/base/log.h>
> #include <libcamera/base/utils.h>
> @@ -48,6 +50,7 @@ int Agc::parseMeteringModes(IPAContext &context, const YamlObject &tuningData,
> return -EINVAL;
> }
>
> + std::vector<ControlValue> availableMeteringModes;
> for (const auto &[key, value] : yamlMeteringModes.asDict()) {
> if (controls::AeMeteringModeNameValueMap.find(key) ==
> controls::AeMeteringModeNameValueMap.end()) {
> @@ -66,17 +69,23 @@ int Agc::parseMeteringModes(IPAContext &context, const YamlObject &tuningData,
> << "Matched metering matrix mode "
> << key << ", version " << version;
>
> - meteringModes_[controls::AeMeteringModeNameValueMap.at(key)] = weights;
> + int32_t control = controls::AeMeteringModeNameValueMap.at(key);
> + meteringModes_[control] = weights;
> + availableMeteringModes.push_back(control);
> }
> }
>
> - if (meteringModes_.empty()) {
> + if (availableMeteringModes.empty()) {
> int32_t meteringModeId = controls::AeMeteringModeNameValueMap.at("MeteringMatrix");
> std::vector<uint8_t> weights(context.hw->numHistogramWeights, 1);
>
> meteringModes_[meteringModeId] = weights;
> + availableMeteringModes.push_back(meteringModeId);
> }
>
> + Algorithm::controls_[&controls::AeMeteringMode] =
> + ControlInfo(availableMeteringModes);
> +
I was going to say "shouldn't this be controls()[&controls::AeMeteringMode] instead?", but I see
that the patch adds a new ControlInfoMap to the Algorithm base class. I did a similar thing in the
central AGC series with a ControlInfoMap in the context that algorithms fill with control data - we
need to merge those methods and just do one or the other...I don't particularly mind which method we
settle on, the only difference I can see at the moment is that the context.ctrlMap allows the
algorithms to all fill the same ControlInfoMap and so avoids the needs to merge them all into
IPARkISP1::algoControls_ with the loop in IPARkISP1::init()
> return 0;
> }
>
> @@ -137,6 +146,8 @@ int Agc::init(IPAContext &context, const YamlObject &tuningData)
>
> context.ctrlMap.merge(controls());
>
> + Algorithm::controls_.merge(ControlInfoMap::Map(controls()));
> +
> return 0;
> }
>
> @@ -159,6 +170,7 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
>
> context.activeState.agc.constraintMode = constraintModes().begin()->first;
> context.activeState.agc.exposureMode = exposureModeHelpers().begin()->first;
> + context.activeState.agc.meteringMode = meteringModes_.begin()->first;
>
> /*
> * Define the measurement window for AGC as a centered rectangle
> @@ -169,7 +181,6 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
> context.configuration.agc.measureWindow.h_size = 3 * configInfo.outputSize.width / 4;
> context.configuration.agc.measureWindow.v_size = 3 * configInfo.outputSize.height / 4;
>
> - /* \todo Run this again when FrameDurationLimits is passed in */
> setLimits(context.configuration.sensor.minShutterSpeed,
> context.configuration.sensor.maxShutterSpeed,
> context.configuration.sensor.minAnalogueGain,
> @@ -223,6 +234,26 @@ void Agc::queueRequest(IPAContext &context,
> frameContext.agc.exposure = agc.manual.exposure;
> frameContext.agc.gain = agc.manual.gain;
> }
> +
> + const auto &meteringMode = controls.get(controls::AeMeteringMode);
> + if (meteringMode)
> + agc.meteringMode = *meteringMode;
> + frameContext.agc.meteringMode = agc.meteringMode;
> +
> + const auto &exposureMode = controls.get(controls::AeExposureMode);
> + if (exposureMode)
> + agc.exposureMode = *exposureMode;
> + frameContext.agc.exposureMode = agc.exposureMode;
> +
> + const auto &constraintMode = controls.get(controls::AeConstraintMode);
> + if (constraintMode)
> + agc.constraintMode = *constraintMode;
> + frameContext.agc.constraintMode = agc.constraintMode;
> +
> + const auto &frameDurationLimits = controls.get(controls::FrameDurationLimits);
> + if (frameDurationLimits)
> + agc.maxShutterSpeed = std::chrono::milliseconds((*frameDurationLimits).back());
> + frameContext.agc.maxShutterSpeed = agc.maxShutterSpeed;
> }
>
> /**
> @@ -261,8 +292,7 @@ void Agc::prepare(IPAContext &context, const uint32_t frame,
> params->meas.hst_config.hist_weight,
> context.hw->numHistogramWeights
> };
> - /* \todo Get this from control */
> - std::vector<uint8_t> &modeWeights = meteringModes_.at(controls::MeteringMatrix);
> + std::vector<uint8_t> &modeWeights = meteringModes_.at(frameContext.agc.meteringMode);
> std::copy(modeWeights.begin(), modeWeights.end(), weights.begin());
>
> std::stringstream str;
> @@ -289,6 +319,7 @@ 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);
>
> /* \todo Use VBlank value calculated from each frame exposure. */
> uint32_t vTotal = context.configuration.sensor.size.height
> @@ -296,6 +327,10 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,
> utils::Duration frameDuration = context.configuration.sensor.lineDuration
> * vTotal;
> metadata.set(controls::FrameDuration, frameDuration.get<std::micro>());
> +
> + metadata.set(controls::AeMeteringMode, frameContext.agc.meteringMode);
> + metadata.set(controls::AeExposureMode, frameContext.agc.exposureMode);
> + metadata.set(controls::AeConstraintMode, frameContext.agc.constraintMode);
> }
>
> /**
> @@ -370,6 +405,16 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> [](uint32_t x) { return x >> 4; });
> expMeans_ = { params->ae.exp_mean, context.hw->numAeCells };
>
> + utils::Duration maxShutterSpeed = std::min(context.configuration.sensor.maxShutterSpeed,
> + frameContext.agc.maxShutterSpeed);
> + if (!maxShutterSpeed)
> + maxShutterSpeed = std::max(context.configuration.sensor.maxShutterSpeed,
> + frameContext.agc.maxShutterSpeed);
> + setLimits(context.configuration.sensor.minShutterSpeed,
> + maxShutterSpeed,
> + context.configuration.sensor.minAnalogueGain,
> + context.configuration.sensor.maxAnalogueGain);
> +
> /*
> * The Agc algorithm needs to know the effective exposure value that was
> * applied to the sensor when the statistics were collected.
> diff --git a/src/ipa/rkisp1/algorithms/algorithm.h b/src/ipa/rkisp1/algorithms/algorithm.h
> index 9454c9a1f..c3a002b85 100644
> --- a/src/ipa/rkisp1/algorithms/algorithm.h
> +++ b/src/ipa/rkisp1/algorithms/algorithm.h
> @@ -25,6 +25,8 @@ public:
>
> bool disabled_;
> bool supportsRaw_;
> +
> + ControlInfoMap::Map controls_;
> };
>
> } /* namespace ipa::rkisp1 */
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index 256b75ebc..26c395704 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -68,8 +68,10 @@ struct IPAActiveState {
> } automatic;
>
> bool autoEnabled;
> - uint32_t constraintMode;
> - uint32_t exposureMode;
> + int32_t constraintMode;
> + int32_t exposureMode;
> + int32_t meteringMode;
> + utils::Duration maxShutterSpeed;
> } agc;
>
> struct {
> @@ -111,6 +113,10 @@ struct IPAFrameContext : public FrameContext {
> uint32_t exposure;
> double gain;
> bool autoEnabled;
> + int32_t exposureMode;
> + int32_t constraintMode;
> + int32_t meteringMode;
> + utils::Duration maxShutterSpeed;
> } agc;
>
> struct {
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 963cdbab4..7716c11bb 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -80,6 +80,7 @@ private:
> std::map<unsigned int, MappedFrameBuffer> mappedBuffers_;
>
> ControlInfoMap sensorControls_;
> + ControlInfoMap::Map algoControls_;
>
> /* Interface to the Camera Helper */
> std::unique_ptr<CameraSensorHelper> camHelper_;
> @@ -193,6 +194,14 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,
> if (ret)
> return ret;
>
> + for (auto const &a : algorithms()) {
> + Algorithm *algo = static_cast<Algorithm *>(a.get());
> +
> + /* \todo Avoid merging duplicate controls */
> + if (!algo->controls_.empty())
> + algoControls_.merge(ControlInfoMap::Map(algo->controls_));
> + }
> +
If we settle on having a ControlInfoMap in the Algorithm class then I think this loop should move to
updateControls() and just merge to ctrlMap directly, and algoControls_ can be dropped.
> /* Initialize controls. */
> updateControls(sensorInfo, sensorControls, ipaControls);
>
> @@ -377,6 +386,7 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo,
> ControlInfoMap *ipaControls)
> {
> ControlInfoMap::Map ctrlMap = rkisp1Controls;
> + ctrlMap.merge(algoControls_);
>
> /*
> * Compute exposure time limits from the V4L2_CID_EXPOSURE control
More information about the libcamera-devel
mailing list