[libcamera-devel] [PATCH v4 2/2] ipa: rpi: common: Handle AEC/AGC flicker controls
Kieran Bingham
kieran.bingham at ideasonboard.com
Tue Jul 18 10:48:39 CEST 2023
Quoting David Plowman via libcamera-devel (2023-07-18 09:24:24)
> Hi Jacopo
>
> Thanks for the comments.
>
> On Tue, 18 Jul 2023 at 08:09, Jacopo Mondi
> <jacopo.mondi at ideasonboard.com> wrote:
> >
> > Hi David
> >
> > On Thu, Jul 13, 2023 at 04:12:18PM +0100, David Plowman via libcamera-devel wrote:
> > > We handle the flicker modes by passing the correct period to the
> > > AEC/AGC algorithm which already contains the necessary code.
> > >
> > > The "Auto" mode, as well as reporting the detected flicker period via
> >
> > If so you shouldn't register
> >
> > { &controls::AeFlickerMode, ControlInfo(controls::AeFlickerModeValues) },
> >
> > otherwise AeFlickerMode::FlickerAuto will be exposed to applications ?
>
> Indeed, it might be better not to expose the auto value!
>
> >
> >
> > > the "AeFlickerDetected" metadata, are unsupported for now.
> > >
> > > Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> > > ---
> > > src/ipa/rpi/common/ipa_base.cpp | 64 ++++++++++++++++++++++++++++++++-
> > > src/ipa/rpi/common/ipa_base.h | 6 ++++
> > > 2 files changed, 69 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
> > > index f40f2e71..81d65ea5 100644
> > > --- a/src/ipa/rpi/common/ipa_base.cpp
> > > +++ b/src/ipa/rpi/common/ipa_base.cpp
> > > @@ -61,6 +61,8 @@ const ControlInfoMap::Map ipaControls{
> > > { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },
> > > { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },
> > > { &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) },
> > > + { &controls::AeFlickerMode, ControlInfo(controls::AeFlickerModeValues) },
> > > + { &controls::AeFlickerCustom, ControlInfo(100, 1000000) },
> > > { &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },
> > > { &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },
> > > { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },
> > > @@ -97,7 +99,7 @@ namespace ipa::RPi {
> > >
> > > IpaBase::IpaBase()
> > > : controller_(), frameCount_(0), mistrustCount_(0), lastRunTimestamp_(0),
> > > - firstStart_(true)
> > > + firstStart_(true), flickerState_({ 0, 0s })
> > > {
> > > }
> > >
> > > @@ -812,6 +814,66 @@ void IpaBase::applyControls(const ControlList &controls)
> > > break;
> > > }
> > >
> > > + case controls::AE_FLICKER_MODE: {
> > > + RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> > > + controller_.getAlgorithm("agc"));
> > > + if (!agc) {
> > > + LOG(IPARPI, Warning)
> > > + << "Could not set AeFlickerMode - no AGC algorithm";
> > > + break;
> > > + }
> > > +
> > > + int32_t mode = ctrl.second.get<int32_t>();
> > > + bool modeValid = true;
> > > +
> > > + switch (mode) {
> > > + case controls::FlickerOff: {
> > > + agc->setFlickerPeriod(0us);
> > > +
> > > + break;
> > > + }
> >
> > Do you need braces ?
>
> Sure, I can remove them!
>
> >
> > > +
> > > + case controls::FlickerCustom: {
> > > + agc->setFlickerPeriod(flickerState_.customPeriod);
> > > +
> > > + break;
> > > + }
> > > +
> > > + default:
> > > + LOG(IPARPI, Error) << "Flicker mode " << mode << " is not supported";
> > > + modeValid = false;
> > > +
> > > + break;
> > > + }
> > > +
> > > + if (modeValid)
> > > + flickerState_.mode = mode;
> > > +
> > > + break;
> > > + }
> > > +
> > > + case controls::AE_FLICKER_CUSTOM: {
> > > + RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> > > + controller_.getAlgorithm("agc"));
> > > + if (!agc) {
> > > + LOG(IPARPI, Warning)
> > > + << "Could not set AeFlickerCustom - no AGC algorithm";
> > > + break;
> > > + }
> > > +
> > > + uint32_t customPeriod = ctrl.second.get<int32_t>();
> > > + flickerState_.customPeriod = customPeriod * 1.0us;
> > > +
> > > + /*
> > > + * We note that it makes no difference if the mode gets set to "custom"
> > > + * first, and the period updated after, or vice versa.
> > > + */
> >
> > True, but what if the app never provides a value for
> > controls::AE_FLICKER_CUSTOM ? Should you initialize customPeriod to
> > some sensible default ?
>
> In our implementation we use the value 0 to mean "no flicker
> cancellection", so setting the mode to custom/manual without a flicker
> period will cause it to do nothing (until you set one). I'm OK with
> this behaviour unless anyone objects, though I don't feel strongly!
> I'd rather document that setting custom/manual mode without a period
> leads to undefined behaviour rather than mandating a particular value,
> I think, as not every PH may wish to do the same. What do you think?
I had wondered if we could get this down to a single control by having
'0' as no flicker cancellation - but we'd have no way to express 'auto'
so I think we do still need two, and it lets us report in the metadata
if the value was determined automatically or explicitly I guess? (Though
the app should know that anyway as it would know if it set the manual
value?)
Throwing paint on the bike shed I feel like 'AeFlickerPeriod' would be
better suited for the control name as then it would be usable to report
the period back in the metadata when it's detected with 'Auto' mode.
What limits will be imposed for the period ? I think setting
AeFlickerMode to 'Custom' (or 'Manual'?) and not setting an
{AeFlickerCustom,AeFlickerPeriod} control should be the same as not
changing anything, but I expect that maybe the ControlInfo might have
limits that would preclude setting a period of '0' ?
It might be reasonable to say setting AeFlickerMode to Manual/Custom
will only take effect when the corresponding AeFlickerPeriod control is
applied ?
--
Kieran
>
> Thanks!
> David
>
> >
> > > + if (flickerState_.mode == controls::FlickerCustom)
> > > + agc->setFlickerPeriod(flickerState_.customPeriod);
> > > +
> > > + break;
> > > + }
> > > +
> > > case controls::AWB_ENABLE: {
> > > /* Silently ignore this control for a mono sensor. */
> > > if (monoSensor_)
> > > diff --git a/src/ipa/rpi/common/ipa_base.h b/src/ipa/rpi/common/ipa_base.h
> > > index 39d00760..22823176 100644
> > > --- a/src/ipa/rpi/common/ipa_base.h
> > > +++ b/src/ipa/rpi/common/ipa_base.h
> > > @@ -116,6 +116,12 @@ private:
> > > /* Frame duration (1/fps) limits. */
> > > utils::Duration minFrameDuration_;
> > > utils::Duration maxFrameDuration_;
> > > +
> > > + /* The current state of flicker avoidance. */
> > > + struct FlickerState {
> > > + int32_t mode;
> > > + utils::Duration customPeriod;
> > > + } flickerState_;
> > > };
> > >
> > > } /* namespace ipa::RPi */
> > > --
> > > 2.30.2
> > >
More information about the libcamera-devel
mailing list