[libcamera-devel] [PATCH v4 1/2] libcamera: controls: Add controls for AEC/AGC flicker avoidance

[David Plowman]

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?

> > +
> > +      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.
* 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?

>
> > +
> > +  - 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!

>
> > +
> > +        \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!
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
> >