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

[David Plowman]

Hi Naush

On Mon, 3 Apr 2023 at 08:58, Naushir Patuck <naush at raspberrypi.com> wrote:
>
> Hi David,
>
> Thanks for this, it looks good to me.  Just one question below:
>
> On Tue, 28 Mar 2023 at 09:55, David Plowman via libcamera-devel
> <libcamera-devel at lists.libcamera.org> 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 | 84 +++++++++++++++++++++++++++-------
> >  1 file changed, 67 insertions(+), 17 deletions(-)
> >
> > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> > index adea5f90..b472050c 100644
> > --- a/src/libcamera/control_ids.yaml
> > +++ b/src/libcamera/control_ids.yaml
> > @@ -156,6 +156,73 @@ 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.
> > +
> > +        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.
> > +
> > +      enum:
> > +        - name: FlickerOff
> > +          value: 0
> > +          description: No flicker avoidance is performed.
> > +        - name: FlickerFreq50Hz
> > +          value: 1
> > +          description: 50Hz flicker avoidance.
> > +            Suppress flicker effects caused by lighting running with a 100Hz
> > +            period (such as that produced by 50Hz mains electricity).
> > +        - name: FlickerFreq60Hz
> > +          value: 2
> > +          description: 60Hz flicker avoidance.
> > +            Suppress flicker effects caused by lighting running with a 120Hz
> > +            period (such as that produced by 60Hz mains electricity).
> > +        - name: FlickerCustom
> > +          value: 3
> > +          description: Custom flicker avoidance.
> > +            Suppress flicker effects caused by lighting running with a period
> > +            specified by the AeFlickerCustom control.
> > +            \sa AeFlickerCustom
> > +        - name: FlickerAuto
> > +          value: 4
> > +          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.
> > +
> > +  - AeFlickerCustom:
> > +      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.
> > +
> > +        If this control is not available, then the setting of custom flicker
> > +        periods is not supported.
> > +
> > +        \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.
> > +
> > +        So in the case of 50Hz mains flicker, the value would be 10000
> > +        (corresponding to 100Hz), or 8333 (120Hz) for 60Hz mains flicker. But
> > +        note that it is not required that any form of scene flicker is detected
> > +        at all, so depending on the implementation, this metadata may just
> > +        report zero or be entirely absent.
> > +
> > +        AeFlickerDetected may also report other values for non-standard flicker
> > +        periods.
> > +
> > +        \sa AeFlickerMode
>
> To clarify, AeFlickerDetected ought to be returned unconditionally on every
> frame, even if AeFlickerMode is set to something other than FlickerAuto, is that
> correct?

Actually I think that's a good question. A lot of this stuff _could_
be quite implementation dependent, I think, and we can't really
mandate that implementations will do auto-detection in all
circumstances (some may well not do it al all).

For example, if we're actually doing flicker avoidance at 50Hz, you
might expect that it gets tricky to detect 50Hz flicker. So what do we
report? Maybe we should require that the metadata is absent (or invent
a "can't tell" value) in these circumstances. We should probably be
clearer on this, so maybe another version is required

David.

>
> Other than that:
>
> Reviewed-by: Naushir Patuck <naush at raspberrypi.com>
>
> > +
> >    - Brightness:
> >        type: float
> >        description: |
> > @@ -843,23 +910,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
> >