[libcamera-devel] [PATCH v4 1/2] libcamera: controls: Add controls for AEC/AGC flicker avoidance
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Tue Jul 18 12:38:49 CEST 2023
Hi David
On Tue, Jul 18, 2023 at 09:15:00AM +0100, David Plowman via libcamera-devel wrote:
> Hi Jacopo
>
> Thanks for the comments.
>
> On Tue, 18 Jul 2023 at 08:04, Jacopo Mondi
> <jacopo.mondi at ideasonboard.com> wrote:
> >
> > Hi David
> >
> > On Thu, Jul 13, 2023 at 04:12:17PM +0100, David Plowman via libcamera-devel wrote:
> > > Flicker is the term used to describe brightness banding or oscillation
> > > of images caused typically by artificial lighting driven by a 50 or
> > > 60Hz mains supply. We add three controls intended to be used by
> > > AEC/AGC algorithms:
> > >
> > > AeFlickerMode to enable flicker avoidance.
> > >
> > > AeFlickerCustom to set custom flicker periods.
> > >
> > > AeFlickerDetected to report any flicker that is currently detected.
> > >
> > > Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> > > ---
> > > src/libcamera/control_ids.yaml | 79 ++++++++++++++++++++++++++--------
> > > 1 file changed, 62 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> > > index 056886e6..f1629b89 100644
> > > --- a/src/libcamera/control_ids.yaml
> > > +++ b/src/libcamera/control_ids.yaml
> > > @@ -156,6 +156,68 @@ controls:
> > > control of which features should be automatically adjusted shouldn't
> > > better be handled through a separate AE mode control.
> > >
> > > + - AeFlickerMode:
> > > + type: int32_t
> > > + description: |
> > > + Set the flicker mode, which determines whether, and how, the AGC/AEC
> > > + algorithm attempts to hide flicker effects caused by the duty cycle of
> > > + artificial lighting.
> >
> > I would here add that if the AeEnable control is set to false
> > ("manual mode") this setting has no effect.
>
> Yes, I think that seems reasonable!
>
> >
> > > +
> > > + Although implementation dependent, many algorithms for "flicker
> > > + avoidance" work by restricting this exposure time to integer multiples
> > > + of the cycle period, wherever possible.
> > > +
> > > + Implementations may not support all of the flicker modes listed below.
> > > +
> > > + By default the system will start in FlickerAuto mode if this is
> > > + supported, otherwise the flicker mode will be set to FlickerOff.
>
> This is what we said in Prague, though I'm not sure about it now. I
> think I'd rather have it be off by default because that's the existing
> behaviour. If Android wants this behaviour, it should perhaps go in
> the Android adaptation layer. Thoughts?
>
To be honest, if anything like auto is available, I would expect it to
be used, as I don't see why an application wouldn't want to have it
enabled.
IOW it seems to me all applications will enable it anyway, if
available...
> > > +
> > > + enum:
> > > + - name: FlickerOff
> > > + value: 0
> > > + description: No flicker avoidance is performed.
> > > + - name: FlickerCustom
> > > + value: 1
> > > + description: Custom flicker avoidance.
> > > + Suppress flicker effects caused by lighting running with a period
> > > + specified by the AeFlickerCustom control.
> > > + \sa AeFlickerCustom
> > > + - name: FlickerAuto
> > > + value: 2
> > > + description: Automatic flicker period detection and avoidance.
> > > + The system will automatically determine the most likely value of
> > > + flicker period, and avoid flicker of this frequency. Once flicker
> > > + is being corrected, it is implementation dependent whether the
> > > + system is still able to detect a change in the flicker period.
> >
> > "... during a single streaming session" ?
> >
> > I presume a camera stop/start sequence re-init the algorithms (so that
> > if you travel between countries far apart in the world (or different
> > parts of Japan) while using the camera the new flicker period will be
> > detected).
>
> We don't normally re-initialise control algorithms between
> stop/configure/start calls. This is pretty critical to everything we
> do - AE, AWB, ALSC and other things are all preserved.
>
> But re-opening the camera obviously does restart everything from scratch.
>
> We did briefly talk about places like Japan where it is more plausible
> that you could go from a 50Hz to a 60Hz environment with the camera
> left running. If anyone recalls exactly what we said that would be
> interesting. My own feelings on this include:
>
> * It's a fairly marginal use-case, even in Japan. Let's make the basics work.
Yeah, my comment was half a joke
> * The detection algorithm could plausibly detect changes to
> frequencies other than the one being handled, so this could probably
> be supported (though it would be implementation dependent).
>
> What do others think?
It seems a rather marginal use case to me. Ignore my comment.
>
> >
> > > +
> > > + - AeFlickerCustom:
> >
> > Nit: Custom or Manual ? I don't recall if we discussed it or not
>
> I agree, maybe "manual" is better. "custom" was the term left over
> from when we had explicit 50/60Hz modes.
>
> >
> > > + type: int32_t
> > > + description: Custom flicker period in microseconds.
> > > + This value sets the current flicker period to avoid. It is used when
> > > + AeFlickerMode is set to FlickerCustom.
> > > +
> > > + To cancel 50Hz mains flicker, this should be set to 10000 (corresponding
> > > + to 100Hz), or 8333 (120Hz) for 60Hz mains.
> > > +
> > > + If this control is not available, then the setting of custom flicker
> > > + periods is not supported.
> >
> > How can you set the control if it is not available ? This seems more
> > like a note for PH implementer not to register this control if
> > FlickerCustom is not available ?
>
> I guess it means you wouldn't be able to set the flicker period
> ("manually") yourself. I suppose you might still have "off" and
> perhaps "auto" settings available. Though it feels unlikely that an
> implementation would support "auto" but not "manual", though you never
> know for sure!
What I meant is that your phrasing sounded to me like like: "if this
control is not available you cannot set it", which seems... expected ?
Did I get it wrong ?
>
> >
> > > +
> > > + \sa AeFlickerMode
> > > +
> > > + - AeFlickerDetected:
> > > + type: int32_t
> > > + description: Flicker period detected in microseconds.
> > > + The value reported here indicates the currently detected flicker
> > > + period, or zero if no flicker at all is detected.
> > > +
> > > + In the case of 50Hz mains flicker, the value would be 10000
> > > + (corresponding to 100Hz), or 8333 (120Hz) for 60Hz mains flicker.
> > > +
> > > + It is implementation dependent exactly when the system is able
> > > + to detect and report the flicker period.
> >
> > Does flicker period detection need a warm-up phase where we can expect
> > the first results to be innacurate or as soon as we receive this value
> > in metadata we can assume it is correct in your experience ?
>
> Good point. I think in practice an algorithm would want to see "a few
> frames" to confirm the detected flicker period before reporting and
> cancelling it, so it would make sense to set this once it's
> "confirmed" and the flicker is now going to be cancelled.
>
> Anyway, I'll put together another version shortly.
Thanks
>
> Thanks!
> David
>
> >
> > Thanks
> > j
> >
> > > +
> > > + \sa AeFlickerMode
> > > +
> > > - Brightness:
> > > type: float
> > > description: |
> > > @@ -850,23 +912,6 @@ controls:
> > > value: 1
> > > description: The lens shading map mode is available.
> > >
> > > - - SceneFlicker:
> > > - type: int32_t
> > > - draft: true
> > > - description: |
> > > - Control to report the detected scene light frequency. Currently
> > > - identical to ANDROID_STATISTICS_SCENE_FLICKER.
> > > - enum:
> > > - - name: SceneFickerOff
> > > - value: 0
> > > - description: No flickering detected.
> > > - - name: SceneFicker50Hz
> > > - value: 1
> > > - description: 50Hz flickering detected.
> > > - - name: SceneFicker60Hz
> > > - value: 2
> > > - description: 60Hz flickering detected.
> > > -
> > > - PipelineDepth:
> > > type: int32_t
> > > draft: true
> > > --
> > > 2.30.2
> > >
More information about the libcamera-devel
mailing list