[libcamera-devel] [PATCH v3 1/2] libcamera: controls: Add controls for AEC/AGC flicker avoidance
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Apr 5 04:06:56 CEST 2023
Hi David,
On Tue, Apr 04, 2023 at 11:40:53AM +0100, David Plowman wrote:
> On Tue, 4 Apr 2023 at 07:06, Laurent Pinchart wrote:
> > On Mon, Apr 03, 2023 at 09:20:39AM +0100, David Plowman via libcamera-devel wrote:
> > > On Mon, 3 Apr 2023 at 08:58, Naushir Patuck wrote:
> > > > On Tue, 28 Mar 2023 at 09:55, 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 | 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).
> >
> > Have you considered having 3 values here (off, manual and auto), and a
> > AeFlickerPeriod control to set the manual value ? 50Hz and 60Hz are
> > common values, but do they require special cases ?
>
> Interesting thought, maybe I was following Android too slavishly. I
> believe it to be the case that almost no one will care about anything
> other than 50/60Hz so making those convenient seems helpful, but a
> manual mode like this would be fine too. Too many choices... need to
> think!
With the world (or at least the parts I'm used to) switching to LED
lighting, won't automatic frequency detection become more relevant than
50/60Hz manual selection ?
> > > > > + - 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
> >
> > To answer this question I'd like to look at it from an application's
> > point of view. How do you envision this control being used in
> > applications ?
>
> Yes, always a good idea to do this. Let me try and list a number of scenarios:
>
> 1. Flicker avoidance permanently off. For users who don't care!
And for controlled environment where the user knows there's no lighting
flicker.
> 2. A user or application might know what country they're in and set
> Flicker avoidance to 50/60Hz immediately, and leave it forever. These
> folks would just use the "manual" mode.
>
> 3. A user might want to rely on auto-detection to discover 50/60Hz
> flicker, then they set flicker avoidance to that frequency and leave
> it forever. Here we'd watch the "flicker detected" value, and as soon
> as we see 50 or 60Hz, we'd set the flicker avoidance manually and
> never touch it again.
Wouldn't using auto mode be simpler for applications in this case ?
> After this, we might consider users with non-50/60Hz lights, or where
> the flicker period might change. I think these use cases are way more
> niche.
As mentioned in a reply to the cover letter, won't non-50/60Hz
frequencies become increasingly important with LED lighting ?
> 4. A user has some special lighting with a known period, and they want
> to set it manually. After that they leave it alone. Though somewhat
> niche, this sounds plausible to me, and is also something that is
> easily implemented so we should probably cover it.
I agree with both points, it's a bit niche, but I see no reason not to
support it.
> 5. User has unknown lighting (that may not be 50/60Hz) and wants to be
> told the flicker period so that they can cancel it. Obviously depends
> on whether the implementation can support this. Niche certainly, but
> perhaps it could happen.
>
> After this I think there are cases where someone might want to detect
> flicker even when one particular frequency is being cancelled, perhaps
> so that they can change the flicker period. This sounds like the least
> realistic end of the spectrum, and will definitely depend a lot on
> what the capabilities of the platform are.
>
> I also haven't talked about what "auto" means. There will be a class
> of users who just want to set "auto" and forget about it. It may
> depend on the implementation what "auto" means, but I would expect
> something like case 3 (above) might be an obvious candidate. If a
> platform can detect other flicker frequencies than the one it is
> cancelling, then "auto" could potentially be cleverer, though most of
> the time there's no benefit to this.
The way you've specified the behaviour of "auto" in your patch seems
fine to me. Implementations can be cleverer than that if they want to.
> Sorry if that's got rather long! I guess the thing that perhaps leaves
> me the most questions is exactly what flicker detection means when a
> particular period is already being cancelled. Do we need to
> distinguish between "I've detected another flicker frequency" from
> "I've detected that there is no flicker at all" from "I can't tell" or
> even "I can't detect a different frequency but I can't tell if the one
> I'm cancelling is still there"?
That's also the part that bothers me a bit, I'm not sure what would be
best for applications. Detecting and handling multiple frequencies seems
to me like an advanced use case that may be best left to applications to
manage.
Regarding reporting no flicker vs. unknown flicker frequency, is that
even something we can do ? If you set the exposure time to a multiple of
the flicker half period, my understanding is that libcamera wouldn't be
able to distinguish the two cases. We should then document that a zero
value for the detected flicker frequency could mean either when in
non-off mode.
> > > > 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
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list