[PATCH v5 3/3] ipa: rkisp1: agc: Plumb mode-selection and frame duration controls
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Jun 11 22:24:58 CEST 2024
On Tue, Jun 11, 2024 at 05:21:44PM +0300, Laurent Pinchart wrote:
> On Mon, Jun 10, 2024 at 03:35:28PM +0100, Kieran Bingham wrote:
> > Quoting Paul Elder (2024-06-07 09:03:30)
> > > 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>
> > > Reviewed-by: Stefan Klug <stefan.klug at ideasonboard.com>
> > >
> > > ---
> > > No change in v5
> > >
> > > No change in v4
> > >
> > > 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 e9f1f2095..c6d03c3b2 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 think you can avoid the availableMeteringModes variable by using
>
> Algorithm::controls_[&controls::AeMeteringMode] =
> ControlInfo(utils::map_keys(meteringModes_));
>
> > > +
> > > 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);
> >
> > I'm worried about any potential race being introduced here if requests
> > are queued late (after the frame is already set to be processed).
> >
> > I know that scenario is not unique to this patch - but I can 'see' it
> > being something that would be impacted here because of the reference
> > above potentially being uninitialised?
> >
> > I wonder if we need to catch that during the top level and if we are
> > about to call prepare on a frameContext for a sequence that has not had
> > queueRequest called on it - we should probably copy the previous
> > frameContext or such.
> >
> > But that's probably something to handle during the PFC reworks not this
> > patch specifically as the issue will apply to all algorithms, not just
> > this one.
>
> Yes, there's an issue here, and no, Paul doesn't have to address it now
> (Paul says "yay thanks!" during our live review session :-)). It will be
> addressed as part of the PFC work.
>
> > > 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);
>
> This looks a bit weird. Could we instead initialize
> activeState.agc.maxShutterSpeed somewhere (in configure() maybe ?) to a
> sensible default value ? I think it should be initialized to whatever
> default value the IPA module reports for the FrameDurationLimits.
Another comment on FrameDurationLimits. At the moment, the control is
inserted in the control info map in IPARkISP1::updateControls(). I think
it should be moved to the AGC class, as well as the
controls::ExposureTime and controls::AnalogueGain controls. This means
that the Algorithm::configure() function will need to update the control
info map. I haven't checked if that would be straightforward or would
require more refactoring.
As this is a good chunk of additional rework it doesn't belong to this
patch, and isn't a blocker to merge this series. A \todo comment would
be here too, and a patch on top of this series would be splendid :-)
> > > + 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 715cfcd82..93a4b548c 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 bd02a7a24..1cad512e1 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;
>
> I don't think those could take negative values. They're essentially
> enums, which we store as int32_t internally. It would be nice if the
> compiler could verify that we're using them correctly. We seem to have,
> for instance, enum AeConstraintModeEnum in the control generated
> headers. Should we use that ?
>
> > > + 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 6687c91e7..12b3aedd2 100644
> > > --- a/src/ipa/rkisp1/rkisp1.cpp
> > > +++ b/src/ipa/rkisp1/rkisp1.cpp
> >
> > All the changes to rkisp1 here should really be in a standalone commit.
> >
> > I'm surprised we don't already have this handled, but it's core to
> > rkisp1 IPA - not the AGC algorithm.
> >
> > And ideally that patch should be in a series that also drops
> > rkisp1Controls and moves those to their respective algo implementations!
> >
> > Err ... wait - I think it /is/ already being handled by the core...
> >
> > > @@ -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 */
> >
> > I think that's already the case. algoControls_ can't merge duplicates,
> > it can either overwrite or drop the incoming ones (based on a
> > parameter).
> >
> > In this case, we /shouldn't/ have duplicates - so I'd keep this as is
> > with the default MergePolicy which I believe will shout out a warning if
> > someone tries to merge a control that's already added.
> >
> > In other words, I think this todo comment can be dropped now.
> >
> >
> > So in fact - I think this should all be dropped here as there is already
> > a way to register additional controls by adding them to the IPA Context
> > ctlrMap.
> >
> > See Stefan's recent "ipa: rkisp1: Add GammaOutCorrection algorithm" as
> > an example.
> >
> > Or did I miss something different about what this is doing here?
> >
> > > + if (!algo->controls_.empty())
> > > + algoControls_.merge(ControlInfoMap::Map(algo->controls_));
> > > + }
> > > +
> > > /* 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
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list