| Message ID | 20230713151218.26045-2-david.plowman@raspberrypi.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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@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. > + > + 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. > + > + 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). > + > + - AeFlickerCustom: Nit: Custom or Manual ? I don't recall if we discussed it or not > + 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 ? > + > + \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 ? 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 >
Hi Jacopo Thanks for the comments. On Tue, 18 Jul 2023 at 08:04, Jacopo Mondi <jacopo.mondi@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@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 > >
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@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@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 > > >
Hi again! On Tue, 18 Jul 2023 at 11:38, Jacopo Mondi <jacopo.mondi@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@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@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! > > > > > + > > > > + 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). 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 > > > >
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@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@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@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 > > > > >
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. + + 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. + + 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. + + - 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. + + 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. + + \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. + + \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
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@raspberrypi.com> --- src/libcamera/control_ids.yaml | 79 ++++++++++++++++++++++++++-------- 1 file changed, 62 insertions(+), 17 deletions(-)