[PATCH v3 3/3] ipa: rkisp1: agc: Plumb mode-selection and frame duration controls
Paul Elder
paul.elder at ideasonboard.com
Wed May 29 09:44:29 CEST 2024
On Mon, May 20, 2024 at 01:34:56PM +0100, Dan Scally wrote:
> 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()
>
Yeah... I needed it for rkisp1's algorithms for reporting and merging
the valid controls and values, so to consolidate it with the one that
you added in agc I have them merged in ipa::rkisp1::algorithms::Agc::init():
Algorithm::controls_.merge(ControlInfoMap::Map(controls()));
Paul
> > 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