[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 14:20:14 CEST 2023
Hi David
On Tue, Jul 18, 2023 at 12:12:15PM +0100, David Plowman wrote:
> Hi again!
>
> On Tue, 18 Jul 2023 at 11:38, Jacopo Mondi
> <jacopo.mondi at ideasonboard.com> wrote:
> >
> > 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...
>
> I'm not sure I agree that "all applications will enable it anyway".
> Certainly it will be very common for general purpose image capture
> applications, but we also have many industrial, scientific, and home
> users with specific purposes for their images.
>
> In these cases I think I'd rather the new behaviour was a conscious
> choice, rather than a default behaviour which might seem "hidden" to
> them. But like I say, I can live with this, though probably by adding
> code in our applications to undo it!
>
You certainly have more experience dealing with users than me, so I
guess you can frame the use case better. I'm fine with both options
> >
> > > > > +
> > > > > + 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 ?
>
> Sorry, I can probably explain this better. The case I have in mind is
> the case where you can set the mode to manual (because this may be
> hard to prevent if auto is available too), but you can't set the
> flicker period because this control is unavailable. In that case, I
> suppose we could say that the absence of this control means that
> setting the mode to manual would have no effect (or be the same as
> "off", or something).
I wish lc-compliance would catch situation like these and scold the
pipeline handler developers, as this is clearly an error on their
side, isn't it ?
>
> David
>
> >
> > >
> > > >
> > > > > +
> > > > > + \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