[libcamera-devel] [PATCH v4 2/2] ipa: rpi: common: Handle AEC/AGC flicker controls
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Tue Jul 18 14:29:19 CEST 2023
Just to throw more confusion to the mix
On Tue, Jul 18, 2023 at 12:16:02PM +0100, Kieran Bingham via libcamera-devel wrote:
> Quoting David Plowman (2023-07-18 10:13:42)
> > Hi Kieran
> >
> > On Tue, 18 Jul 2023 at 09:48, Kieran Bingham
> > <kieran.bingham at ideasonboard.com> wrote:
> > >
> > > 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?)
> >
> > Yes, I think we do still want a separate control.
> >
> > >
> > > 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.
> >
> > I agree that 'AeFlickerPeriod' is a better name, particularly if the
> > 'custom' mode becomes the 'manual' 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' ?
> >
> > I guess implementations get to choose their own limits though, for the
> > most part, I see no reason not to allow them to be very wide.
> > Extremely short or extremely long periods are going to be increasingly
> > unlikely IRL, of course.
>
> The part I was wondering about is if we 'defined' 0 as being equivalent
> to controls::FlickerOff, but that could make it harder to validate the
> control if it means any value from 0...MAX is 'acceptable'.
>
> But yes, I think this can all be handled by the pipeline handler/IPA.
>
>
> > > It might be reasonable to say setting AeFlickerMode to Manual/Custom
> > > will only take effect when the corresponding AeFlickerPeriod control is
> > > applied ?
> >
> > Yes, we could say that setting the mode to manual has no effect until
> > a period is also set. Though of course I assume the period is
> > "sticky", so once you've set it, you can go to and fro to manual mode
> > without re-sending the said period.
>
> <Time jump, now that I've writen the below> Does this mean you expect to
> go from AeFlickerMode=Manaul(20000) to AeFlickerMode=Auto(Detected
> 30000) and then go to AeFlickerMode=Off(0) and when you finally go back
> to AeFlickerMode=Manual(no period applied) you expect period to be
> 'remembered' as 20000?
>
>
>
> >
> > I guess we have to ask what happens if you go from auto to manual
> > mode, having never set a period. What happens then? TBH, I think I'd
> > be quite happy to say that selecting manual mode, without ever setting
> > a period, is the same as being "off". Thoughts?
>
> I suspect this might be something we should consider for all
> 'multi-control' settings, for how they react if they are only partially
> set. As long as the behaviour is documented I think it's fine though.
>
> I think I would consider setting the period to be 'sticky' as well so if
> you did:
>
> 1) AeFlickerMode = Off
> 2) AeFlickerPeriod = 10000
> 3) AeFlickerMode = Manual
I guess it depends if 1) 2) and 3) are separate requests ? If that's
the case, we discussed in the past how this should work for the AEGC
controls that I still wish someone review:
https://patchwork.libcamera.org/patch/17077/
A similar case is the one represented by 'ExposureTime' which is only
meaningful when the mode is set to Manual. From the description of the
control we drafted:
- ExposureTime:
This control will only take effect if ExposureTimeMode is Manual. If
this control is set when ExposureTimeMode is Auto, the value will be
ignored and will not be retained.
I guess the situation here is similar ?
>
> The Period would be defined at 10000
>
> I think for switching from Auto to Manual it would remain at whatever
> the autodetected value was...
>
> 1) AeFlickerMode = Off
> 2) AeFlickerMode = Auto #Metadata reports AeFlickerPeriod as 20000
> 3) AeFlickerMode = Manual # AeFlickerPeriod now 'locked' at manual 20000
Looking at the behaviour we defined for AEGC, this seems to match
+ When transitioning from Auto to Manual mode and no ExposureTime control
+ is provided by the application, the last value computed by the AE
+ algorithm when the mode was Auto will be used.
>
> Hrm... but that seems to differ with your suggestion above of setting it
> to 'off'
>
> To me 'sticky' to last actual value (where off = 0, auto == detected
> value) seems easiest to codify and explain? But maybe I'm missing
> something obvious.
>
> And now I feel like we should have helper classes in libipa to codify
> the behaviour of all these controls in a common way for each PH!?
> (later)
I wish
>
> --
> Kieran
>
> >
> > David
> >
> > >
> > >
> > > --
> > > 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