[libcamera-devel,v4,1/3] controls: Reorganize the AE-related controls

15970 diff mbox series

We have multiple goals:
- we need a lock of some sort, to instruct the AEGC to not update output
  results
- we need manual modes, to override the values computed by the AEGC
- we need to support seamless transitions from auto -> manual, and do so
  without flickering
- we need custom minimum values for the manual controls, that is no
  magic values for enabling/disabling auto
- all of these need to be done with AE sub-controls (exposure time,
  analogue gain)

To achieve these goals, we introduce mode controls for the AE
sub-controls: ExposureTimeMode and AnalogueGainMode. These have an auto
state, and a disabled state. The disabled state has an internal one-way
state change from locked to manual, triggered by the presence of the
value-controls (ExposureTime and AnalogueGain).

We then remove the AeEnable control, as it is a redundant control in the
face of these two mode controls.

We also remove AeLocked, as it is insufficient for reporting the AE
state, and we promote AeState to non-draft to fill its role. Notably,
the locked state is removed, since this information can be obtained from
the aforementioned mode controls.

Bug: https://bugs.libcamera.org/show_bug.cgi?id=42
Bug: https://bugs.libcamera.org/show_bug.cgi?id=43
Bug: https://bugs.libcamera.org/show_bug.cgi?id=47
Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

---
Changes in v4:
- remove FlashRequired and Precapture from AeState
- upgrade documentation of all the controls

Changes in v3:
- improve wording of the control descriptions
  - make more succinct and clear
- add description of how to do a flickerless transition

Changes in v2:
- No changes, just resubmitting at the head of this series so that it's
  together and so that /people will actually see it/

Initial version:
Still RFC as I haven't updated the users of the control yet, and I want
to check that these are the controls and docs that we want.

We've decided that the "master AE control" will be implemented by a
helper... but looking at uvcvideo and the V4L2 controls I'm wondering if
such helper should come earlier than later?
---
 src/libcamera/control_ids.yaml | 262 +++++++++++++++++++++++++--------
 1 file changed, 200 insertions(+), 62 deletions(-)

Hi Paul,

On Wed, May 18, 2022 at 03:47:26PM +0200, Paul Elder via libcamera-devel wrote:
> We have multiple goals:
> - we need a lock of some sort, to instruct the AEGC to not update output
>   results
> - we need manual modes, to override the values computed by the AEGC
> - we need to support seamless transitions from auto -> manual, and do so
>   without flickering
> - we need custom minimum values for the manual controls, that is no
>   magic values for enabling/disabling auto
> - all of these need to be done with AE sub-controls (exposure time,
>   analogue gain)
>
> To achieve these goals, we introduce mode controls for the AE
> sub-controls: ExposureTimeMode and AnalogueGainMode. These have an auto
> state, and a disabled state. The disabled state has an internal one-way
> state change from locked to manual, triggered by the presence of the
> value-controls (ExposureTime and AnalogueGain).
>
> We then remove the AeEnable control, as it is a redundant control in the
> face of these two mode controls.
>
> We also remove AeLocked, as it is insufficient for reporting the AE
> state, and we promote AeState to non-draft to fill its role. Notably,
> the locked state is removed, since this information can be obtained from
> the aforementioned mode controls.
>
> Bug: https://bugs.libcamera.org/show_bug.cgi?id=42
> Bug: https://bugs.libcamera.org/show_bug.cgi?id=43
> Bug: https://bugs.libcamera.org/show_bug.cgi?id=47
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
>

Be prepared: a lot of bikeshedding ahead

I think the content is good and these are the last refinements. Take
them as suggestions.

> ---
> Changes in v4:
> - remove FlashRequired and Precapture from AeState
> - upgrade documentation of all the controls
>
> Changes in v3:
> - improve wording of the control descriptions
>   - make more succinct and clear
> - add description of how to do a flickerless transition
>
> Changes in v2:
> - No changes, just resubmitting at the head of this series so that it's
>   together and so that /people will actually see it/
>
> Initial version:
> Still RFC as I haven't updated the users of the control yet, and I want
> to check that these are the controls and docs that we want.
>
> We've decided that the "master AE control" will be implemented by a
> helper... but looking at uvcvideo and the V4L2 controls I'm wondering if
> such helper should come earlier than later?
> ---
>  src/libcamera/control_ids.yaml | 262 +++++++++++++++++++++++++--------
>  1 file changed, 200 insertions(+), 62 deletions(-)
>
> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> index 9d4638ae..9f5ce5e8 100644
> --- a/src/libcamera/control_ids.yaml
> +++ b/src/libcamera/control_ids.yaml
> @@ -7,23 +7,46 @@
>  # Unless otherwise stated, all controls are bi-directional, i.e. they can be
>  # set through Request::controls() and returned out through Request::metadata().
>  controls:
> -  - AeEnable:
> -      type: bool
> +  - AeState:
> +      type: int32_t
>        description: |
> -        Enable or disable the AE.
> +        Control to report the AE algorithm state associated with the capture
> +        result.

I know you added the last part to highlight that this control is only
reported in metadata. To be honest I would defer this to a later
series where the 'direction' of controls will be more clearly defined
and just define this

          "Control to report the AE algorithm state'
>
> -        \sa ExposureTime AnalogueGain
> +        The state is still reported even if ExposureTimeMode or
> +        AnalogueGainMode is set to Disabled.
>
> -  - AeLocked:
> -      type: bool
> -      description: |
> -        Report the lock status of a running AE algorithm.
> +        \sa AnalogueGain
> +        \sa AnalogueGainMode
> +        \sa ExposureTime
> +        \sa ExposureTimeMode
>
> -        If the AE algorithm is locked the value shall be set to true, if it's
> -        converging it shall be set to false. If the AE algorithm is not
> -        running the control shall not be present in the metadata control list.
> +      enum:

The AF controls use the term "Idle" instead of inactive. I think it
would be nice if we use the same term to express the same concept.

> +        - name: AeStateInactive
> +          value: 0
> +          description: |
> +            The AE algorithm is inactive.
>
> -        \sa AeEnable
> +            This state should be returned if both AnalogueGainMode and
> +            ExposureTimeMode are set to disabled (or one, if the camera only
> +            supports one of the two controls).

Do you think it would be better to write this slightly differently, to
tell what it means to application, instead of suggesting to IPA
developers what they have to report (this part can be kept as well,
maybe implicitly suggested).

To explain what I mean, I would make this along the lines of:

               This state is returned when the AE algorithm is not
               actively computing new values for the exposure time and
               analogue gain. This happens when both AnalogueGainMode
               and ExposureTimeMode are set to disabled or when the
               algorithm has converged and has not yet initiated a new
               scan.

               If either ExposureTimeMode or AnalogueGainMode are set
               to auto, the AE algorithm might spontaneously initiate
               a new scan in which case the state is moved to be
               AeStateSearching.

> +        - name: AeStateSearching
> +          value: 1
> +          description: |
> +            The AE algorithm has not converged yet.

               The AE algorithm is actively computing new values and
               has not converged yet.

> +            This state should be returned if at least one of AnalogueGainMode
> +            or ExposureTimeMode is set to auto, and the AE algorithm hasn't
> +            converged yet. If the AE algorithm converges, the state shall go to
> +            AeStateConverged.

               This state is only returned when at least one of
               AnalogueGainMode or ExposureTimeMode is set to auto and
               the AE algorithm has spontaneously started a scan and
               has not convergd yet. If the AE algorithm converges,
               the state is moved to AeStateConverged.

For the same reason of telling applications how they should interepet
it instead of telling IPA developers how they should behave.

Do we need a AeStateFailed state to report failed scans or it
shouldn't happen at all ?

> +        - name: AeStateConverged
> +          value: 2
> +          description: |
> +            The AE algorithm has converged.
> +
> +            This state should be returned if at least one of AnalogueGainMode
> +            or ExposureTimeMode is set to auto, and the AE algorithm has
> +            converged.

Same "This state is returned if .." instead of "should be"

I hope my reasoning is right here and I'm not trying to correct a
native speaker with my bad english.

>
>    # AeMeteringMode needs further attention:
>    # - Auto-generate max enum value.
> @@ -93,6 +116,13 @@ controls:
>          how the desired total exposure is divided between the shutter time
>          and the sensor's analogue gain. The exposure modes are platform
>          specific, and not all exposure modes may be supported.
> +
> +        When one of AnalogueGainMode or ExposureTimeMode is set to Disabled,
> +        the fixed values will override any choices made by AeExposureMode.
> +
> +        \sa AnalogueGainMode
> +        \sa ExposureTimeMode
> +
>        enum:
>          - name: ExposureNormal
>            value: 0
> @@ -111,13 +141,15 @@ controls:
>        type: float
>        description: |
>          Specify an Exposure Value (EV) parameter. The EV parameter will only be
> -        applied if the AE algorithm is currently enabled.
> +        applied if the AE algorithm is currently enabled, that is, at least one
> +        of AnalogueGainMode and ExposureTimeMode are auto.
>
>          By convention EV adjusts the exposure as log2. For example
>          EV = [-2, -1, 0.5, 0, 0.5, 1, 2] results in an exposure adjustment
>          of [1/4x, 1/2x, 1/sqrt(2)x, 1x, sqrt(2)x, 2x, 4x].
>
> -        \sa AeEnable
> +        \sa AnalogueGainMode
> +        \sa ExposureTimeMode
>
>    - ExposureTime:
>        type: int32_t
> @@ -125,17 +157,85 @@ controls:
>          Exposure time (shutter speed) for the frame applied in the sensor
>          device. This value is specified in micro-seconds.
>
> -        Setting this value means that it is now fixed and the AE algorithm may
> -        not change it. Setting it back to zero returns it to the control of the
> -        AE algorithm.
> +        This control will only take effect if ExposureTimeMode is Disabled. If
> +        this control is set when ExposureTimeMode is Auto, the value will be
> +        ignored and will not be retained.
> +
> +        When reported in metadata, this control indicates what exposure time
> +        was used for the current request, regardless of ExposureTimeMode.

s/for the current request//

metadata are always associated to requests :)

> +        ExposureTimeMode will indicate the source of the exposure time value,
> +        whether it came from the AE algorithm or not.
> +
> +        \sa AnalogueGain
> +        \sa ExposureTimeMode
> +
> +  - ExposureTimeMode:
> +      type: int32_t
> +      description: |
> +        Controls the source of the exposure time that is applied to the image
> +        sensor. When set to Auto, the AE algorithm computes the exposure time
> +        and configures the image sensor accordingly. When set to Disabled,
> +        exposure time specified in ExposureTime is applied to the image sensor.
> +        If ExposureTime is not set, then the value last computed by the AE

I think you mean "When transitioning from Auto to Manual mode and no
ExposureTime is provided, then..."


> +        algorithm when the mode was Auto will be used.
> +
> +        If ExposureTime is not set and the mode is ExposureTimeModeDisabled and
> +        AE was never Auto (either because the camera started in Disabled mode,
> +        or Auto is not supported by the camera), the camera should use a
> +        best-effort default value.
> +
> +        When ExposureTimeMode is set Auto, the value set in ExposureTime is
> +        ignored and is not retained. This means that if ExposureTimeMode is set

"is later set to Disabled"

to make it clear it is a state transition

> +        to Disabled and ExposureTime is not also set, the exposure time that
> +        was last computed by the AE algorithm while the mode was Auto will be
> +        applied to the sensor.

Ah, isn't this also described above ?

> +
> +        If ExposureTimeModeDisabled is supported, the ExposureTime control must
> +        also be supported.

This is a note for implementer, it doesn't hurt though, but I would
rather move it to the design document you have prepared.

> +
> +        The set of ExposureTimeMode modes that are supported by the camera must
> +        have an intersection with the supported set of AnalogueGainMode modes.
>
> -        \sa AnalogueGain AeEnable
> +        As it takes a few frames to apply the exposure time, there is a period of
> +        time between submitting a request with ExposureTimeMode set to Disabled
> +        and the exposure time component of the AE actually being disabled,
> +        during which the AE algorithm can still update the exposure time. If an
> +        application is switching from automatic and manual control and wishes
> +        to eliminate any flicker during the switch, the following procedure is
> +        recommended.
>
> -        \todo Document the interactions between AeEnable and setting a fixed
> -        value for this control. Consider interactions with other AE features,
> -        such as aperture and aperture/shutter priority mode, and decide if
> -        control of which features should be automatically adjusted shouldn't
> -        better be handled through a separate AE mode control.
> +        1. Start with ExposureTimeMode set to Auto
> +
> +        2. Set ExposureTimeMode to Disabled
> +
> +        3. Wait for the first request to be output that has ExposureTimeMode
> +        set to Disabled
> +
> +        4. Copy the value reported in ExposureTime into a new request, and
> +        submit it
> +
> +        5. Proceed to run manual exposure time

I dare to repropose the text I suggested on v2:

             Applications that transition from auto ExposureTimeMode
             to the direct control of the exposure time should
             aim to do so by selecting an ExposureTime value as close
             as possible to the last value computed by the auto
             exposure algorithm in order to avoid any visible
             flickering.

             To select the correct value to use as ExposureTime value,
             applications should accommodate the natural delay in
             applying controls caused by the capture pipeline frame
             depth.

             When switching to manual exposure mode, applications
             should not immediately specify an ExposureTime value in
             the same request where ExposureTimeMode is set to
             Disabled. They should instead wait for the first Request
             where ExposureTimeMode is reported as Disabled in the
             Request metadata, and use the there reported exposure to
             populate the ExposureTime control value in the next
             Request to be queued to the Camera.

             The implementation of the auto-exposure algorithm
             should equally try to minimize flickering and when
             transitioning from manual exposure mode to auto
             exposure use the last value provided by the application
             as starting point.

This is not alternative to the list of steps you have proposed but
provides a more detailed introduction to them. Do you think it's not
necessary ?

> +
> +        \sa ExposureTime
> +      enum:
> +        - name: ExposureTimeModeAuto
> +          value: 0
> +          description: |
> +            The exposure time will be calculated automatically and set by the
> +            AE algorithm. If ExposureTime is set while this mode is active, it
> +            will be ignored, and it will also not be retained.
> +        - name: ExposureTimeModeDisabled
> +          value: 1
> +          description: |
> +            The exposure time will not be updated by the AE algorithm. It will
> +            come from the last calculated value when the mode was Auto, or from
> +            the value specified in ExposureTime.
> +
> +            When transitioning from Auto to Disabled mode, the last computed
> +            exposure value is used until a new value is specified through the
> +            ExposureTime control. If an ExposureTime value is specified in the
> +            same request where the ExposureTimeMode is changed from Auto to
> +            Disabled, the provided ExposureTime is applied.

"is applied immediately" ?

Thanks and sorry for being picky, we're almost there :)

>
>    - AnalogueGain:
>        type: float
> @@ -144,17 +244,85 @@ controls:
>          The value of the control specifies the gain multiplier applied to all
>          colour channels. This value cannot be lower than 1.0.
>
> -        Setting this value means that it is now fixed and the AE algorithm may
> -        not change it. Setting it back to zero returns it to the control of the
> -        AE algorithm.
> +        This control will only take effect if AnalogueGainMode is Disabled. If
> +        this control is set when AnalogueGainMode is Auto, the value will be
> +        ignored and will not be retained.
> +
> +        When reported in metadata, this control indicates what analogue gain
> +        was used for the current request, regardless of AnalogueGainMode.
> +        AnalogueGainMode will indicate the source of the analogue gain value,
> +        whether it came from the AE algorithm or not.
> +
> +        \sa ExposureTime
> +        \sa AnalogueGainMode
> +
> +  - AnalogueGainMode:
> +      type: int32_t
> +      description: |
> +        Controls the source of the analogue gain that is applied to the image
> +        sensor. When set to Auto, the AE algorithm computes the analogue gain
> +        and configures the image sensor accordingly. When set to Disabled,
> +        analogue gain specified in AnalogueGain is applied to the image sensor.
> +        If AnalogueGain is not set, then the value last computed by the AE
> +        algorithm when the mode was Auto will be used.
> +
> +        If AnalogueGain is not set and the mode is AnalogueGainModeDisabled and
> +        AE was never Auto (either because the camera started in Disabled mode,
> +        or Auto is not supported by the camera), the camera should use a
> +        best-effort default value.
> +
> +        When AnalogueGainMode is set Auto, the value set in AnalogueGain is
> +        ignored and is not retained. This means that if AnalogueGainMode is set
> +        to Disabled and AnalogueGain is not also set, the analogue gain that
> +        was last computed by the AE algorithm while the mode was Auto will be
> +        applied to the sensor.
>
> -        \sa ExposureTime AeEnable
> +        If AnalogueGainModeDisabled is supported, the AnalogueGain control must
> +        also be supported.
> +
> +        The set of AnalogueGainMode modes that are supported by the camera must
> +        have an intersection with the supported set of ExposureTimeMode modes.
> +
> +        As it takes a few frames to apply the analogue gain, there is a period of
> +        time between submitting a request with AnalogueGainMode set to Disabled
> +        and the analogue gain component of the AE actually being disabled,
> +        during which the AE algorithm can still update the analogue gain. If an
> +        application is switching from automatic and manual control and wishes
> +        to eliminate any flicker during the switch, the following procedure is
> +        recommended.
> +
> +        1. Start with AnalogueGainMode set to Auto
> +
> +        2. Set AnalogueGainMode to Disabled
> +
> +        3. Wait for the first request to be output that has AnalogueGainMode
> +        set to Disabled
> +
> +        4. Copy the value reported in AnalogueGain into a new request, and
> +        submit it
> +
> +        5. Proceed to run manual analogue gain
> +
> +        \sa AnalogueGain
> +      enum:
> +        - name: AnalogueGainModeAuto
> +          value: 0
> +          description: |
> +            The analogue gain will be calculated automatically and set by the
> +            AE algorithm. If AnalogueGain is set while this mode is active, it
> +            will be ignored, and it will also not be retained.
> +        - name: AnalogueGainModeDisabled
> +          value: 1
> +          description: |
> +            The analogue gain will not be updated by the AE algorithm. It will
> +            come from the last calculated value when the mode was Auto, or from
> +            the value specified in AnalogueGain.
>
> -        \todo Document the interactions between AeEnable and setting a fixed
> -        value for this control. Consider interactions with other AE features,
> -        such as aperture and aperture/shutter priority mode, and decide if
> -        control of which features should be automatically adjusted shouldn't
> -        better be handled through a separate AE mode control.
> +            When transitioning from Auto to Disabled mode the last computed
> +            gain value is used until a new value is specified through the
> +            AnalogueGain control. If an AnalogueGain value is specified in the
> +            same request where the AnalogueGainMode is set to Disabled, the
> +            provided AnalogueGain is applied.
>
>    - Brightness:
>        type: float
> @@ -477,36 +645,6 @@ controls:
>              High quality aberration correction which might reduce the frame
>              rate.
>
> -  - AeState:
> -      type: int32_t
> -      draft: true
> -      description: |
> -       Control to report the current AE algorithm state. Currently identical to
> -       ANDROID_CONTROL_AE_STATE.
> -
> -        Current state of the AE algorithm.
> -      enum:
> -        - name: AeStateInactive
> -          value: 0
> -          description: The AE algorithm is inactive.
> -        - name: AeStateSearching
> -          value: 1
> -          description: The AE algorithm has not converged yet.
> -        - name: AeStateConverged
> -          value: 2
> -          description: The AE algorithm has converged.
> -        - name: AeStateLocked
> -          value: 3
> -          description: The AE algorithm is locked.
> -        - name: AeStateFlashRequired
> -          value: 4
> -          description: The AE algorithm would need a flash for good results
> -        - name: AeStatePrecapture
> -          value: 5
> -          description: |
> -            The AE algorithm has started a pre-capture metering session.
> -            \sa AePrecaptureTrigger
> -
>    - AfState:
>        type: int32_t
>        draft: true
> --
> 2.30.2
>

Hi Paul,

Thank you for your work.  It's nice to see these changes to solidify the AE
controls.
I do have a few thoughts/comments below.


On Wed, 18 May 2022 at 14:47, Paul Elder via libcamera-devel <
libcamera-devel@lists.libcamera.org> wrote:

> We have multiple goals:
> - we need a lock of some sort, to instruct the AEGC to not update output
>   results
> - we need manual modes, to override the values computed by the AEGC
> - we need to support seamless transitions from auto -> manual, and do so
>   without flickering
> - we need custom minimum values for the manual controls, that is no
>   magic values for enabling/disabling auto
> - all of these need to be done with AE sub-controls (exposure time,
>   analogue gain)
>
> To achieve these goals, we introduce mode controls for the AE
> sub-controls: ExposureTimeMode and AnalogueGainMode. These have an auto
> state, and a disabled state. The disabled state has an internal one-way
> state change from locked to manual, triggered by the presence of the
> value-controls (ExposureTime and AnalogueGain).
>
> We then remove the AeEnable control, as it is a redundant control in the
> face of these two mode controls.
>
> We also remove AeLocked, as it is insufficient for reporting the AE
> state, and we promote AeState to non-draft to fill its role. Notably,
> the locked state is removed, since this information can be obtained from
> the aforementioned mode controls.
>
> Bug: https://bugs.libcamera.org/show_bug.cgi?id=42
> Bug: https://bugs.libcamera.org/show_bug.cgi?id=43
> Bug: https://bugs.libcamera.org/show_bug.cgi?id=47
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
>
> ---
> Changes in v4:
> - remove FlashRequired and Precapture from AeState
> - upgrade documentation of all the controls
>
> Changes in v3:
> - improve wording of the control descriptions
>   - make more succinct and clear
> - add description of how to do a flickerless transition
>
> Changes in v2:
> - No changes, just resubmitting at the head of this series so that it's
>   together and so that /people will actually see it/
>
> Initial version:
> Still RFC as I haven't updated the users of the control yet, and I want
> to check that these are the controls and docs that we want.
>
> We've decided that the "master AE control" will be implemented by a
> helper... but looking at uvcvideo and the V4L2 controls I'm wondering if
> such helper should come earlier than later?
>

Yes, I agree having the "master AE control" earlier will be beneficial for
application developers.


> ---
>  src/libcamera/control_ids.yaml | 262 +++++++++++++++++++++++++--------
>  1 file changed, 200 insertions(+), 62 deletions(-)
>
> diff --git a/src/libcamera/control_ids.yaml
> b/src/libcamera/control_ids.yaml
> index 9d4638ae..9f5ce5e8 100644
> --- a/src/libcamera/control_ids.yaml
> +++ b/src/libcamera/control_ids.yaml
> @@ -7,23 +7,46 @@
>  # Unless otherwise stated, all controls are bi-directional, i.e. they can
> be
>  # set through Request::controls() and returned out through
> Request::metadata().
>  controls:
> -  - AeEnable:
> -      type: bool
> +  - AeState:
> +      type: int32_t
>        description: |
> -        Enable or disable the AE.
> +        Control to report the AE algorithm state associated with the
> capture
> +        result.
>
> -        \sa ExposureTime AnalogueGain
> +        The state is still reported even if ExposureTimeMode or
> +        AnalogueGainMode is set to Disabled.
>
> -  - AeLocked:
> -      type: bool
> -      description: |
> -        Report the lock status of a running AE algorithm.
> +        \sa AnalogueGain
> +        \sa AnalogueGainMode
> +        \sa ExposureTime
> +        \sa ExposureTimeMode
>
> -        If the AE algorithm is locked the value shall be set to true, if
> it's
> -        converging it shall be set to false. If the AE algorithm is not
> -        running the control shall not be present in the metadata control
> list.
> +      enum:
> +        - name: AeStateInactive
> +          value: 0
> +          description: |
> +            The AE algorithm is inactive.
>
> -        \sa AeEnable
> +            This state should be returned if both AnalogueGainMode and
> +            ExposureTimeMode are set to disabled (or one, if the camera
> only
> +            supports one of the two controls).
> +        - name: AeStateSearching
> +          value: 1
> +          description: |
> +            The AE algorithm has not converged yet.
> +
> +            This state should be returned if at least one of
> AnalogueGainMode
> +            or ExposureTimeMode is set to auto, and the AE algorithm
> hasn't
> +            converged yet. If the AE algorithm converges, the state shall
> go to
> +            AeStateConverged.
> +        - name: AeStateConverged
> +          value: 2
> +          description: |
> +            The AE algorithm has converged.
> +
> +            This state should be returned if at least one of
> AnalogueGainMode
> +            or ExposureTimeMode is set to auto, and the AE algorithm has
> +            converged.
>
>    # AeMeteringMode needs further attention:
>    # - Auto-generate max enum value.
> @@ -93,6 +116,13 @@ controls:
>          how the desired total exposure is divided between the shutter time
>          and the sensor's analogue gain. The exposure modes are platform
>          specific, and not all exposure modes may be supported.
> +
> +        When one of AnalogueGainMode or ExposureTimeMode is set to
> Disabled,
> +        the fixed values will override any choices made by AeExposureMode.
> +
> +        \sa AnalogueGainMode
> +        \sa ExposureTimeMode
> +
>        enum:
>          - name: ExposureNormal
>            value: 0
> @@ -111,13 +141,15 @@ controls:
>        type: float
>        description: |
>          Specify an Exposure Value (EV) parameter. The EV parameter will
> only be
> -        applied if the AE algorithm is currently enabled.
> +        applied if the AE algorithm is currently enabled, that is, at
> least one
> +        of AnalogueGainMode and ExposureTimeMode are auto.
>
>          By convention EV adjusts the exposure as log2. For example
>          EV = [-2, -1, 0.5, 0, 0.5, 1, 2] results in an exposure adjustment
>          of [1/4x, 1/2x, 1/sqrt(2)x, 1x, sqrt(2)x, 2x, 4x].
>
> -        \sa AeEnable
> +        \sa AnalogueGainMode
> +        \sa ExposureTimeMode
>
>    - ExposureTime:
>        type: int32_t
> @@ -125,17 +157,85 @@ controls:
>          Exposure time (shutter speed) for the frame applied in the sensor
>          device. This value is specified in micro-seconds.
>
> -        Setting this value means that it is now fixed and the AE
> algorithm may
> -        not change it. Setting it back to zero returns it to the control
> of the
> -        AE algorithm.
> +        This control will only take effect if ExposureTimeMode is
> Disabled. If
> +        this control is set when ExposureTimeMode is Auto, the value will
> be
> +        ignored and will not be retained.
> +
> +        When reported in metadata, this control indicates what exposure
> time
> +        was used for the current request, regardless of ExposureTimeMode.
> +        ExposureTimeMode will indicate the source of the exposure time
> value,
> +        whether it came from the AE algorithm or not.
> +
> +        \sa AnalogueGain
> +        \sa ExposureTimeMode
> +
> +  - ExposureTimeMode:
> +      type: int32_t
> +      description: |
> +        Controls the source of the exposure time that is applied to the
> image
> +        sensor. When set to Auto, the AE algorithm computes the exposure
> time
> +        and configures the image sensor accordingly. When set to Disabled,
> +        exposure time specified in ExposureTime is applied to the image
> sensor.
> +        If ExposureTime is not set, then the value last computed by the AE
> +        algorithm when the mode was Auto will be used.
>

Can we un-set ExposureTime?  If it ever gets set once at any point in the
application,
then ExposureTimeModeDisabled will always use the last set value for
ExposureTime.


> +
> +        If ExposureTime is not set and the mode is
> ExposureTimeModeDisabled and
> +        AE was never Auto (either because the camera started in Disabled
> mode,
> +        or Auto is not supported by the camera), the camera should use a
> +        best-effort default value.
> +
> +        When ExposureTimeMode is set Auto, the value set in ExposureTime
> is
> +        ignored and is not retained. This means that if ExposureTimeMode
> is set
> +        to Disabled and ExposureTime is not also set, the exposure time
> that
> +        was last computed by the AE algorithm while the mode was Auto
> will be
> +        applied to the sensor.
> +
> +        If ExposureTimeModeDisabled is supported, the ExposureTime
> control must
> +        also be supported.
> +
> +        The set of ExposureTimeMode modes that are supported by the
> camera must
> +        have an intersection with the supported set of AnalogueGainMode
> modes.
>
> -        \sa AnalogueGain AeEnable
> +        As it takes a few frames to apply the exposure time, there is a
> period of
> +        time between submitting a request with ExposureTimeMode set to
> Disabled
> +        and the exposure time component of the AE actually being disabled,
> +        during which the AE algorithm can still update the exposure time.
> If an
> +        application is switching from automatic and manual control and
> wishes
> +        to eliminate any flicker during the switch, the following
> procedure is
> +        recommended.
>

I'm a bit confused by this bit to be honest.  If a user switches
ExposureTimeMode from
Auto to Disabled with the intention of setting a manual ExposureTime, how
can we ever
avoid a glitch in the brightness (unless we also change AnalogueGain
appropriately)?


> -        \todo Document the interactions between AeEnable and setting a
> fixed
> -        value for this control. Consider interactions with other AE
> features,
> -        such as aperture and aperture/shutter priority mode, and decide if
> -        control of which features should be automatically adjusted
> shouldn't
> -        better be handled through a separate AE mode control.
> +        1. Start with ExposureTimeMode set to Auto
> +
> +        2. Set ExposureTimeMode to Disabled
> +
> +        3. Wait for the first request to be output that has
> ExposureTimeMode
> +        set to Disabled
>

How would the application know this time point?  Would the AE algorithm
have to
count frames once it has been given a ExposureTimeModeDisabled ctrl then
return out the same in the metadata when it knows that it's last requested
exposure
time change has been applied?


> +
> +        4. Copy the value reported in ExposureTime into a new request, and
> +        submit it
> +
> +        5. Proceed to run manual exposure time
>

Again, I am unclear how this avoids glitches.  Say the AE chooses an
exposure
time of 33ms, then the user wants to switch to 15ms.  There is always going
to
be a jump in brightness.  Perhaps my interpretation of this glitch is not
the same
as what you are describing?

Ditto comments for the AnalogueGain changes.

Regards,
Naush


> +
> +        \sa ExposureTime
> +      enum:
> +        - name: ExposureTimeModeAuto
> +          value: 0
> +          description: |
> +            The exposure time will be calculated automatically and set by
> the
> +            AE algorithm. If ExposureTime is set while this mode is
> active, it
> +            will be ignored, and it will also not be retained.
> +        - name: ExposureTimeModeDisabled
> +          value: 1
> +          description: |
> +            The exposure time will not be updated by the AE algorithm. It
> will
> +            come from the last calculated value when the mode was Auto,
> or from
> +            the value specified in ExposureTime.
> +
> +            When transitioning from Auto to Disabled mode, the last
> computed
> +            exposure value is used until a new value is specified through
> the
> +            ExposureTime control. If an ExposureTime value is specified
> in the
> +            same request where the ExposureTimeMode is changed from Auto
> to
> +            Disabled, the provided ExposureTime is applied.
>
>    - AnalogueGain:
>        type: float
> @@ -144,17 +244,85 @@ controls:
>          The value of the control specifies the gain multiplier applied to
> all
>          colour channels. This value cannot be lower than 1.0.
>
> -        Setting this value means that it is now fixed and the AE
> algorithm may
> -        not change it. Setting it back to zero returns it to the control
> of the
> -        AE algorithm.
> +        This control will only take effect if AnalogueGainMode is
> Disabled. If
> +        this control is set when AnalogueGainMode is Auto, the value will
> be
> +        ignored and will not be retained.
> +
> +        When reported in metadata, this control indicates what analogue
> gain
> +        was used for the current request, regardless of AnalogueGainMode.
> +        AnalogueGainMode will indicate the source of the analogue gain
> value,
> +        whether it came from the AE algorithm or not.
> +
> +        \sa ExposureTime
> +        \sa AnalogueGainMode
> +
> +  - AnalogueGainMode:
> +      type: int32_t
> +      description: |
> +        Controls the source of the analogue gain that is applied to the
> image
> +        sensor. When set to Auto, the AE algorithm computes the analogue
> gain
> +        and configures the image sensor accordingly. When set to Disabled,
> +        analogue gain specified in AnalogueGain is applied to the image
> sensor.
> +        If AnalogueGain is not set, then the value last computed by the AE
> +        algorithm when the mode was Auto will be used.
> +
> +        If AnalogueGain is not set and the mode is
> AnalogueGainModeDisabled and
> +        AE was never Auto (either because the camera started in Disabled
> mode,
> +        or Auto is not supported by the camera), the camera should use a
> +        best-effort default value.
> +
> +        When AnalogueGainMode is set Auto, the value set in AnalogueGain
> is
> +        ignored and is not retained. This means that if AnalogueGainMode
> is set
> +        to Disabled and AnalogueGain is not also set, the analogue gain
> that
> +        was last computed by the AE algorithm while the mode was Auto
> will be
> +        applied to the sensor.
>
> -        \sa ExposureTime AeEnable
> +        If AnalogueGainModeDisabled is supported, the AnalogueGain
> control must
> +        also be supported.
> +
> +        The set of AnalogueGainMode modes that are supported by the
> camera must
> +        have an intersection with the supported set of ExposureTimeMode
> modes.
> +
> +        As it takes a few frames to apply the analogue gain, there is a
> period of
> +        time between submitting a request with AnalogueGainMode set to
> Disabled
> +        and the analogue gain component of the AE actually being disabled,
> +        during which the AE algorithm can still update the analogue gain.
> If an
> +        application is switching from automatic and manual control and
> wishes
> +        to eliminate any flicker during the switch, the following
> procedure is
> +        recommended.
> +
> +        1. Start with AnalogueGainMode set to Auto
> +
> +        2. Set AnalogueGainMode to Disabled
> +
> +        3. Wait for the first request to be output that has
> AnalogueGainMode
> +        set to Disabled
> +
> +        4. Copy the value reported in AnalogueGain into a new request, and
> +        submit it
> +
> +        5. Proceed to run manual analogue gain
>
+
> +        \sa AnalogueGain
> +      enum:
> +        - name: AnalogueGainModeAuto
> +          value: 0
> +          description: |
> +            The analogue gain will be calculated automatically and set by
> the
> +            AE algorithm. If AnalogueGain is set while this mode is
> active, it
> +            will be ignored, and it will also not be retained.
> +        - name: AnalogueGainModeDisabled
> +          value: 1
> +          description: |
> +            The analogue gain will not be updated by the AE algorithm. It
> will
> +            come from the last calculated value when the mode was Auto,
> or from
> +            the value specified in AnalogueGain.
>
> -        \todo Document the interactions between AeEnable and setting a
> fixed
> -        value for this control. Consider interactions with other AE
> features,
> -        such as aperture and aperture/shutter priority mode, and decide if
> -        control of which features should be automatically adjusted
> shouldn't
> -        better be handled through a separate AE mode control.
> +            When transitioning from Auto to Disabled mode the last
> computed
> +            gain value is used until a new value is specified through the
> +            AnalogueGain control. If an AnalogueGain value is specified
> in the
> +            same request where the AnalogueGainMode is set to Disabled,
> the
> +            provided AnalogueGain is applied.
>
>    - Brightness:
>        type: float
> @@ -477,36 +645,6 @@ controls:
>              High quality aberration correction which might reduce the
> frame
>              rate.
>
> -  - AeState:
> -      type: int32_t
> -      draft: true
> -      description: |
> -       Control to report the current AE algorithm state. Currently
> identical to
> -       ANDROID_CONTROL_AE_STATE.
> -
> -        Current state of the AE algorithm.
> -      enum:
> -        - name: AeStateInactive
> -          value: 0
> -          description: The AE algorithm is inactive.
> -        - name: AeStateSearching
> -          value: 1
> -          description: The AE algorithm has not converged yet.
> -        - name: AeStateConverged
> -          value: 2
> -          description: The AE algorithm has converged.
> -        - name: AeStateLocked
> -          value: 3
> -          description: The AE algorithm is locked.
> -        - name: AeStateFlashRequired
> -          value: 4
> -          description: The AE algorithm would need a flash for good
> results
> -        - name: AeStatePrecapture
> -          value: 5
> -          description: |
> -            The AE algorithm has started a pre-capture metering session.
> -            \sa AePrecaptureTrigger
> -
>    - AfState:
>        type: int32_t
>        draft: true
> --
> 2.30.2
>
>

Hi Naush,
  sorry Paul if I reply in your place, but I just got through this so
I might have it slightly fresher

On Mon, Jul 04, 2022 at 11:02:47AM +0100, Naushir Patuck via libcamera-devel wrote:
> Hi Paul,
>
> Thank you for your work.  It's nice to see these changes to solidify the AE
> controls.
> I do have a few thoughts/comments below.

Can I ask you to have a look at the final result with the fixups I
sent in reply applied on top of this patch ?
>
>
> On Wed, 18 May 2022 at 14:47, Paul Elder via libcamera-devel <
> libcamera-devel@lists.libcamera.org> wrote:
>
> > We have multiple goals:
> > - we need a lock of some sort, to instruct the AEGC to not update output
> >   results
> > - we need manual modes, to override the values computed by the AEGC
> > - we need to support seamless transitions from auto -> manual, and do so
> >   without flickering
> > - we need custom minimum values for the manual controls, that is no
> >   magic values for enabling/disabling auto
> > - all of these need to be done with AE sub-controls (exposure time,
> >   analogue gain)
> >
> > To achieve these goals, we introduce mode controls for the AE
> > sub-controls: ExposureTimeMode and AnalogueGainMode. These have an auto
> > state, and a disabled state. The disabled state has an internal one-way
> > state change from locked to manual, triggered by the presence of the
> > value-controls (ExposureTime and AnalogueGain).
> >
> > We then remove the AeEnable control, as it is a redundant control in the
> > face of these two mode controls.
> >
> > We also remove AeLocked, as it is insufficient for reporting the AE
> > state, and we promote AeState to non-draft to fill its role. Notably,
> > the locked state is removed, since this information can be obtained from
> > the aforementioned mode controls.
> >
> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=42
> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=43
> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=47
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> >
> > ---
> > Changes in v4:
> > - remove FlashRequired and Precapture from AeState
> > - upgrade documentation of all the controls
> >
> > Changes in v3:
> > - improve wording of the control descriptions
> >   - make more succinct and clear
> > - add description of how to do a flickerless transition
> >
> > Changes in v2:
> > - No changes, just resubmitting at the head of this series so that it's
> >   together and so that /people will actually see it/
> >
> > Initial version:
> > Still RFC as I haven't updated the users of the control yet, and I want
> > to check that these are the controls and docs that we want.
> >
> > We've decided that the "master AE control" will be implemented by a
> > helper... but looking at uvcvideo and the V4L2 controls I'm wondering if
> > such helper should come earlier than later?
> >
>
> Yes, I agree having the "master AE control" earlier will be beneficial for
> application developers.
>

Do you envision this as something that could be part of your
applications in example, or in a layer part of libcamera itself ?

>
> > ---
> >  src/libcamera/control_ids.yaml | 262 +++++++++++++++++++++++++--------
> >  1 file changed, 200 insertions(+), 62 deletions(-)
> >
> > diff --git a/src/libcamera/control_ids.yaml
> > b/src/libcamera/control_ids.yaml
> > index 9d4638ae..9f5ce5e8 100644
> > --- a/src/libcamera/control_ids.yaml
> > +++ b/src/libcamera/control_ids.yaml
> > @@ -7,23 +7,46 @@
> >  # Unless otherwise stated, all controls are bi-directional, i.e. they can
> > be
> >  # set through Request::controls() and returned out through
> > Request::metadata().
> >  controls:
> > -  - AeEnable:
> > -      type: bool
> > +  - AeState:
> > +      type: int32_t
> >        description: |
> > -        Enable or disable the AE.
> > +        Control to report the AE algorithm state associated with the
> > capture
> > +        result.
> >
> > -        \sa ExposureTime AnalogueGain
> > +        The state is still reported even if ExposureTimeMode or
> > +        AnalogueGainMode is set to Disabled.
> >
> > -  - AeLocked:
> > -      type: bool
> > -      description: |
> > -        Report the lock status of a running AE algorithm.
> > +        \sa AnalogueGain
> > +        \sa AnalogueGainMode
> > +        \sa ExposureTime
> > +        \sa ExposureTimeMode
> >
> > -        If the AE algorithm is locked the value shall be set to true, if
> > it's
> > -        converging it shall be set to false. If the AE algorithm is not
> > -        running the control shall not be present in the metadata control
> > list.
> > +      enum:
> > +        - name: AeStateInactive
> > +          value: 0
> > +          description: |
> > +            The AE algorithm is inactive.
> >
> > -        \sa AeEnable
> > +            This state should be returned if both AnalogueGainMode and
> > +            ExposureTimeMode are set to disabled (or one, if the camera
> > only
> > +            supports one of the two controls).
> > +        - name: AeStateSearching
> > +          value: 1
> > +          description: |
> > +            The AE algorithm has not converged yet.
> > +
> > +            This state should be returned if at least one of
> > AnalogueGainMode
> > +            or ExposureTimeMode is set to auto, and the AE algorithm
> > hasn't
> > +            converged yet. If the AE algorithm converges, the state shall
> > go to
> > +            AeStateConverged.
> > +        - name: AeStateConverged
> > +          value: 2
> > +          description: |
> > +            The AE algorithm has converged.
> > +
> > +            This state should be returned if at least one of
> > AnalogueGainMode
> > +            or ExposureTimeMode is set to auto, and the AE algorithm has
> > +            converged.
> >
> >    # AeMeteringMode needs further attention:
> >    # - Auto-generate max enum value.
> > @@ -93,6 +116,13 @@ controls:
> >          how the desired total exposure is divided between the shutter time
> >          and the sensor's analogue gain. The exposure modes are platform
> >          specific, and not all exposure modes may be supported.
> > +
> > +        When one of AnalogueGainMode or ExposureTimeMode is set to
> > Disabled,
> > +        the fixed values will override any choices made by AeExposureMode.
> > +
> > +        \sa AnalogueGainMode
> > +        \sa ExposureTimeMode
> > +
> >        enum:
> >          - name: ExposureNormal
> >            value: 0
> > @@ -111,13 +141,15 @@ controls:
> >        type: float
> >        description: |
> >          Specify an Exposure Value (EV) parameter. The EV parameter will
> > only be
> > -        applied if the AE algorithm is currently enabled.
> > +        applied if the AE algorithm is currently enabled, that is, at
> > least one
> > +        of AnalogueGainMode and ExposureTimeMode are auto.
> >
> >          By convention EV adjusts the exposure as log2. For example
> >          EV = [-2, -1, 0.5, 0, 0.5, 1, 2] results in an exposure adjustment
> >          of [1/4x, 1/2x, 1/sqrt(2)x, 1x, sqrt(2)x, 2x, 4x].
> >
> > -        \sa AeEnable
> > +        \sa AnalogueGainMode
> > +        \sa ExposureTimeMode
> >
> >    - ExposureTime:
> >        type: int32_t
> > @@ -125,17 +157,85 @@ controls:
> >          Exposure time (shutter speed) for the frame applied in the sensor
> >          device. This value is specified in micro-seconds.
> >
> > -        Setting this value means that it is now fixed and the AE
> > algorithm may
> > -        not change it. Setting it back to zero returns it to the control
> > of the
> > -        AE algorithm.
> > +        This control will only take effect if ExposureTimeMode is
> > Disabled. If
> > +        this control is set when ExposureTimeMode is Auto, the value will
> > be
> > +        ignored and will not be retained.
> > +
> > +        When reported in metadata, this control indicates what exposure
> > time
> > +        was used for the current request, regardless of ExposureTimeMode.
> > +        ExposureTimeMode will indicate the source of the exposure time
> > value,
> > +        whether it came from the AE algorithm or not.
> > +
> > +        \sa AnalogueGain
> > +        \sa ExposureTimeMode
> > +
> > +  - ExposureTimeMode:
> > +      type: int32_t
> > +      description: |
> > +        Controls the source of the exposure time that is applied to the
> > image
> > +        sensor. When set to Auto, the AE algorithm computes the exposure
> > time
> > +        and configures the image sensor accordingly. When set to Disabled,
> > +        exposure time specified in ExposureTime is applied to the image
> > sensor.
> > +        If ExposureTime is not set, then the value last computed by the AE
> > +        algorithm when the mode was Auto will be used.
> >
>
> Can we un-set ExposureTime?  If it ever gets set once at any point in the
> application,
> then ExposureTimeModeDisabled will always use the last set value for
> ExposureTime.
>

If I interpret your question right, are you wondering if the
ExposureTime value is retained if an application sends it when the
AEGC is actually in auto mode (and so the ExposureTime from application
is not applied) ?

We discussed this, and I think Paul tried to clarify it in the
ExposureTime documentation by saying:

  - name: ExposureTimeModeDisabled
       If ExposureTime is set while this mode is active, it
       will be ignored, and it will also not be retained.

which means that by design, the ExposureTime is just ignored if sent
when the AEGC is in auto mode.

Do you think that's not the expected behaviour ?

>
> > +
> > +        If ExposureTime is not set and the mode is
> > ExposureTimeModeDisabled and
> > +        AE was never Auto (either because the camera started in Disabled
> > mode,
> > +        or Auto is not supported by the camera), the camera should use a
> > +        best-effort default value.
> > +
> > +        When ExposureTimeMode is set Auto, the value set in ExposureTime
> > is
> > +        ignored and is not retained. This means that if ExposureTimeMode
> > is set
> > +        to Disabled and ExposureTime is not also set, the exposure time
> > that
> > +        was last computed by the AE algorithm while the mode was Auto
> > will be
> > +        applied to the sensor.
> > +
> > +        If ExposureTimeModeDisabled is supported, the ExposureTime
> > control must
> > +        also be supported.
> > +
> > +        The set of ExposureTimeMode modes that are supported by the
> > camera must
> > +        have an intersection with the supported set of AnalogueGainMode
> > modes.
> >
> > -        \sa AnalogueGain AeEnable
> > +        As it takes a few frames to apply the exposure time, there is a
> > period of
> > +        time between submitting a request with ExposureTimeMode set to
> > Disabled
> > +        and the exposure time component of the AE actually being disabled,
> > +        during which the AE algorithm can still update the exposure time.
> > If an
> > +        application is switching from automatic and manual control and
> > wishes
> > +        to eliminate any flicker during the switch, the following
> > procedure is
> > +        recommended.
> >
>
> I'm a bit confused by this bit to be honest.  If a user switches
> ExposureTimeMode from
> Auto to Disabled with the intention of setting a manual ExposureTime, how
> can we ever
> avoid a glitch in the brightness (unless we also change AnalogueGain
> appropriately)?
>

See below

>
> > -        \todo Document the interactions between AeEnable and setting a
> > fixed
> > -        value for this control. Consider interactions with other AE
> > features,
> > -        such as aperture and aperture/shutter priority mode, and decide if
> > -        control of which features should be automatically adjusted
> > shouldn't
> > -        better be handled through a separate AE mode control.
> > +        1. Start with ExposureTimeMode set to Auto
> > +
> > +        2. Set ExposureTimeMode to Disabled
> > +
> > +        3. Wait for the first request to be output that has
> > ExposureTimeMode
> > +        set to Disabled
> >
>
> How would the application know this time point?  Would the AE algorithm
> have to
> count frames once it has been given a ExposureTimeModeDisabled ctrl then
> return out the same in the metadata when it knows that it's last requested
> exposure
> time change has been applied?
>

Not sure this is going to answer your question, but let's start by
defining what a "glitch" is for us in this definition. I think it's
useful to validate our understanding against your experience of providing
this features to the vast number of users you have.

The idea is that applications willing to control the exposure time
explicitly might want to do so by minimizing the difference between
the last value computed by the AEGC algorithm and their newly set
value, to avoid a sudden change in exposure and gain which result in a
visible "glitch". En example, suddenly moving the exposure time and
gain to the opposite of the spectrum of what the AEGC was computing
will result in images going very bright or very dark in just a few
frames.

The way to implement a smooth transition is to start from the values
lastly computed by the AEGC (as available in metadata) and then "slowly"
moving towards the desired manual value. Of course this is not
mandatory, application might desire a sudden change of exposure, or simply
won't care about smooth transitions. If they do, however, they have to
consider that there will always be a number of requests in the queue
that will be processed by the camera before the one with
"ExposureTimeDisabled" gets to be processed.

During the processing of those requests in the queue, the AEGC will still
be active and might still change the exposure time (and gain) to values quite
different from the ones visible at the application at the time it
queued the request with "ExposureTimeModeDisabled".

The steps proposed here suggest to applications to wait until the
"ExposureTimeModeDisabled" request is returned and the AEGC is
actually off. From the definitions we gave here, this mean the
exposure time (and gain) won't be updated by the now inactive AEGC
block until an "ExposureTime" value is submitted by applications (more
or less like your agc::pause()/resume() work, if I recall correctly).

When the "ExposureTimeModeDisabled" request has completed,
applications know that the exposure time won't be updated from that
point on, and can use the ExposureTime and AnalogueGain metadata values
as a "stable" starting point for their values.

Does this make sense to you ?

In my fixups I proposed a rework of the introduction section of this
part, could you have a look to see if that's more clear ?

>
> > +
> > +        4. Copy the value reported in ExposureTime into a new request, and
> > +        submit it
> > +
> > +        5. Proceed to run manual exposure time
> >
>
> Again, I am unclear how this avoids glitches.  Say the AE chooses an
> exposure
> time of 33ms, then the user wants to switch to 15ms.  There is always going
> to
> be a jump in brightness.  Perhaps my interpretation of this glitch is not
> the same
> as what you are describing?

If an application decides not to care and halves the exposure time
from one request to the following one, the above procedure is useless
indeed.

But as explained above, an application might want to approach 15ms more
smoothly and the above text suggests how to do so.

I feel like this is mostly directed to applications that wants to
drive the AEGC with some sort of algorithm instead of application
that simply take a value in from users and apply it directly. In
this latter case the values input from the user might very well be 1ms
hence approaching it slowly might not even be desired.

Thanks for commenting!

>
> Ditto comments for the AnalogueGain changes.
>
> Regards,
> Naush
>
>
> > +
> > +        \sa ExposureTime
> > +      enum:
> > +        - name: ExposureTimeModeAuto
> > +          value: 0
> > +          description: |
> > +            The exposure time will be calculated automatically and set by
> > the
> > +            AE algorithm. If ExposureTime is set while this mode is
> > active, it
> > +            will be ignored, and it will also not be retained.
> > +        - name: ExposureTimeModeDisabled
> > +          value: 1
> > +          description: |
> > +            The exposure time will not be updated by the AE algorithm. It
> > will
> > +            come from the last calculated value when the mode was Auto,
> > or from
> > +            the value specified in ExposureTime.
> > +
> > +            When transitioning from Auto to Disabled mode, the last
> > computed
> > +            exposure value is used until a new value is specified through
> > the
> > +            ExposureTime control. If an ExposureTime value is specified
> > in the
> > +            same request where the ExposureTimeMode is changed from Auto
> > to
> > +            Disabled, the provided ExposureTime is applied.
> >
> >    - AnalogueGain:
> >        type: float
> > @@ -144,17 +244,85 @@ controls:
> >          The value of the control specifies the gain multiplier applied to
> > all
> >          colour channels. This value cannot be lower than 1.0.
> >
> > -        Setting this value means that it is now fixed and the AE
> > algorithm may
> > -        not change it. Setting it back to zero returns it to the control
> > of the
> > -        AE algorithm.
> > +        This control will only take effect if AnalogueGainMode is
> > Disabled. If
> > +        this control is set when AnalogueGainMode is Auto, the value will
> > be
> > +        ignored and will not be retained.
> > +
> > +        When reported in metadata, this control indicates what analogue
> > gain
> > +        was used for the current request, regardless of AnalogueGainMode.
> > +        AnalogueGainMode will indicate the source of the analogue gain
> > value,
> > +        whether it came from the AE algorithm or not.
> > +
> > +        \sa ExposureTime
> > +        \sa AnalogueGainMode
> > +
> > +  - AnalogueGainMode:
> > +      type: int32_t
> > +      description: |
> > +        Controls the source of the analogue gain that is applied to the
> > image
> > +        sensor. When set to Auto, the AE algorithm computes the analogue
> > gain
> > +        and configures the image sensor accordingly. When set to Disabled,
> > +        analogue gain specified in AnalogueGain is applied to the image
> > sensor.
> > +        If AnalogueGain is not set, then the value last computed by the AE
> > +        algorithm when the mode was Auto will be used.
> > +
> > +        If AnalogueGain is not set and the mode is
> > AnalogueGainModeDisabled and
> > +        AE was never Auto (either because the camera started in Disabled
> > mode,
> > +        or Auto is not supported by the camera), the camera should use a
> > +        best-effort default value.
> > +
> > +        When AnalogueGainMode is set Auto, the value set in AnalogueGain
> > is
> > +        ignored and is not retained. This means that if AnalogueGainMode
> > is set
> > +        to Disabled and AnalogueGain is not also set, the analogue gain
> > that
> > +        was last computed by the AE algorithm while the mode was Auto
> > will be
> > +        applied to the sensor.
> >
> > -        \sa ExposureTime AeEnable
> > +        If AnalogueGainModeDisabled is supported, the AnalogueGain
> > control must
> > +        also be supported.
> > +
> > +        The set of AnalogueGainMode modes that are supported by the
> > camera must
> > +        have an intersection with the supported set of ExposureTimeMode
> > modes.
> > +
> > +        As it takes a few frames to apply the analogue gain, there is a
> > period of
> > +        time between submitting a request with AnalogueGainMode set to
> > Disabled
> > +        and the analogue gain component of the AE actually being disabled,
> > +        during which the AE algorithm can still update the analogue gain.
> > If an
> > +        application is switching from automatic and manual control and
> > wishes
> > +        to eliminate any flicker during the switch, the following
> > procedure is
> > +        recommended.
> > +
> > +        1. Start with AnalogueGainMode set to Auto
> > +
> > +        2. Set AnalogueGainMode to Disabled
> > +
> > +        3. Wait for the first request to be output that has
> > AnalogueGainMode
> > +        set to Disabled
> > +
> > +        4. Copy the value reported in AnalogueGain into a new request, and
> > +        submit it
> > +
> > +        5. Proceed to run manual analogue gain
> >
> +
> > +        \sa AnalogueGain
> > +      enum:
> > +        - name: AnalogueGainModeAuto
> > +          value: 0
> > +          description: |
> > +            The analogue gain will be calculated automatically and set by
> > the
> > +            AE algorithm. If AnalogueGain is set while this mode is
> > active, it
> > +            will be ignored, and it will also not be retained.
> > +        - name: AnalogueGainModeDisabled
> > +          value: 1
> > +          description: |
> > +            The analogue gain will not be updated by the AE algorithm. It
> > will
> > +            come from the last calculated value when the mode was Auto,
> > or from
> > +            the value specified in AnalogueGain.
> >
> > -        \todo Document the interactions between AeEnable and setting a
> > fixed
> > -        value for this control. Consider interactions with other AE
> > features,
> > -        such as aperture and aperture/shutter priority mode, and decide if
> > -        control of which features should be automatically adjusted
> > shouldn't
> > -        better be handled through a separate AE mode control.
> > +            When transitioning from Auto to Disabled mode the last
> > computed
> > +            gain value is used until a new value is specified through the
> > +            AnalogueGain control. If an AnalogueGain value is specified
> > in the
> > +            same request where the AnalogueGainMode is set to Disabled,
> > the
> > +            provided AnalogueGain is applied.
> >
> >    - Brightness:
> >        type: float
> > @@ -477,36 +645,6 @@ controls:
> >              High quality aberration correction which might reduce the
> > frame
> >              rate.
> >
> > -  - AeState:
> > -      type: int32_t
> > -      draft: true
> > -      description: |
> > -       Control to report the current AE algorithm state. Currently
> > identical to
> > -       ANDROID_CONTROL_AE_STATE.
> > -
> > -        Current state of the AE algorithm.
> > -      enum:
> > -        - name: AeStateInactive
> > -          value: 0
> > -          description: The AE algorithm is inactive.
> > -        - name: AeStateSearching
> > -          value: 1
> > -          description: The AE algorithm has not converged yet.
> > -        - name: AeStateConverged
> > -          value: 2
> > -          description: The AE algorithm has converged.
> > -        - name: AeStateLocked
> > -          value: 3
> > -          description: The AE algorithm is locked.
> > -        - name: AeStateFlashRequired
> > -          value: 4
> > -          description: The AE algorithm would need a flash for good
> > results
> > -        - name: AeStatePrecapture
> > -          value: 5
> > -          description: |
> > -            The AE algorithm has started a pre-capture metering session.
> > -            \sa AePrecaptureTrigger
> > -
> >    - AfState:
> >        type: int32_t
> >        draft: true
> > --
> > 2.30.2
> >
> >

Hi Jacopo and Paul,

On Mon, 4 Jul 2022 at 16:30, Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hi Naush,
>   sorry Paul if I reply in your place, but I just got through this so
> I might have it slightly fresher
>
> On Mon, Jul 04, 2022 at 11:02:47AM +0100, Naushir Patuck via
libcamera-devel wrote:
> > Hi Paul,
> >
> > Thank you for your work.  It's nice to see these changes to solidify
the AE
> > controls.
> > I do have a few thoughts/comments below.
>
> Can I ask you to have a look at the final result with the fixups I
> sent in reply applied on top of this patch ?


Yes will do!


>
> >
> >
> > On Wed, 18 May 2022 at 14:47, Paul Elder via libcamera-devel <
> > libcamera-devel@lists.libcamera.org> wrote:
> >
> > > We have multiple goals:
> > > - we need a lock of some sort, to instruct the AEGC to not update
output
> > >   results
> > > - we need manual modes, to override the values computed by the AEGC
>
> > > - we need to support seamless transitions from auto -> manual, and do
so
> > >   without flickering
> > > - we need custom minimum values for the manual controls, that is no
> > >   magic values for enabling/disabling auto
> > > - all of these need to be done with AE sub-controls (exposure time,
> > >   analogue gain)
> > >
> > > To achieve these goals, we introduce mode controls for the AE
> > > sub-controls: ExposureTimeMode and AnalogueGainMode. These have an
auto
> > > state, and a disabled state. The disabled state has an internal
one-way
> > > state change from locked to manual, triggered by the presence of the
> > > value-controls (ExposureTime and AnalogueGain).
> > >
> > > We then remove the AeEnable control, as it is a redundant control in
the
> > > face of these two mode controls.
> > >
> > > We also remove AeLocked, as it is insufficient for reporting the AE
> > > state, and we promote AeState to non-draft to fill its role. Notably,
> > > the locked state is removed, since this information can be obtained
from
> > > the aforementioned mode controls.
> > >
> > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=42
> > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=43
> > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=47
> > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > >
> > > ---
> > > Changes in v4:
> > > - remove FlashRequired and Precapture from AeState
> > > - upgrade documentation of all the controls
> > >
> > > Changes in v3:
> > > - improve wording of the control descriptions
> > >   - make more succinct and clear
> > > - add description of how to do a flickerless transition
> > >
> > > Changes in v2:
> > > - No changes, just resubmitting at the head of this series so that
it's
> > >   together and so that /people will actually see it/
> > >
> > > Initial version:
> > > Still RFC as I haven't updated the users of the control yet, and I
want
> > > to check that these are the controls and docs that we want.
> > >
> > > We've decided that the "master AE control" will be implemented by a
> > > helper... but looking at uvcvideo and the V4L2 controls I'm wondering
if
> > > such helper should come earlier than later?
> > >
> >
> > Yes, I agree having the "master AE control" earlier will be beneficial
for
> > application developers.
> >
>
> Do you envision this as something that could be part of your
> applications in example, or in a layer part of libcamera itself ?


My preference would be to have a helper in libcamera do this if possible.
This way, applications don't need to duplicate code to set all AE controls.
But either way, it's not too big of a deal to put it in the application.

>
>
> >
> > > ---
> > >  src/libcamera/control_ids.yaml | 262
+++++++++++++++++++++++++--------
> > >  1 file changed, 200 insertions(+), 62 deletions(-)
> > >
> > > diff --git a/src/libcamera/control_ids.yaml
> > > b/src/libcamera/control_ids.yaml
> > > index 9d4638ae..9f5ce5e8 100644
> > > --- a/src/libcamera/control_ids.yaml
> > > +++ b/src/libcamera/control_ids.yaml
> > > @@ -7,23 +7,46 @@
> > >  # Unless otherwise stated, all controls are bi-directional, i.e.
they can
> > > be
> > >  # set through Request::controls() and returned out through
> > > Request::metadata().
> > >  controls:
> > > -  - AeEnable:
> > > -      type: bool
> > > +  - AeState:
> > > +      type: int32_t
> > >        description: |
> > > -        Enable or disable the AE.
> > > +        Control to report the AE algorithm state associated with the
> > > capture
> > > +        result.
> > >
> > > -        \sa ExposureTime AnalogueGain
> > > +        The state is still reported even if ExposureTimeMode or
> > > +        AnalogueGainMode is set to Disabled.
> > >
> > > -  - AeLocked:
> > > -      type: bool
> > > -      description: |
> > > -        Report the lock status of a running AE algorithm.
> > > +        \sa AnalogueGain
> > > +        \sa AnalogueGainMode
> > > +        \sa ExposureTime
> > > +        \sa ExposureTimeMode
> > >
> > > -        If the AE algorithm is locked the value shall be set to
true, if
> > > it's
> > > -        converging it shall be set to false. If the AE algorithm is
not
> > > -        running the control shall not be present in the metadata
control
> > > list.
> > > +      enum:
> > > +        - name: AeStateInactive
> > > +          value: 0
> > > +          description: |
> > > +            The AE algorithm is inactive.
> > >
> > > -        \sa AeEnable
> > > +            This state should be returned if both AnalogueGainMode
and
> > > +            ExposureTimeMode are set to disabled (or one, if the
camera
> > > only
> > > +            supports one of the two controls).
> > > +        - name: AeStateSearching
> > > +          value: 1
> > > +          description: |
> > > +            The AE algorithm has not converged yet.
> > > +
> > > +            This state should be returned if at least one of
> > > AnalogueGainMode
> > > +            or ExposureTimeMode is set to auto, and the AE algorithm
> > > hasn't
> > > +            converged yet. If the AE algorithm converges, the state
shall
> > > go to
> > > +            AeStateConverged.
> > > +        - name: AeStateConverged
> > > +          value: 2
> > > +          description: |
> > > +            The AE algorithm has converged.
> > > +
> > > +            This state should be returned if at least one of
> > > AnalogueGainMode
> > > +            or ExposureTimeMode is set to auto, and the AE algorithm
has
> > > +            converged.
> > >
> > >    # AeMeteringMode needs further attention:
> > >    # - Auto-generate max enum value.
> > > @@ -93,6 +116,13 @@ controls:
> > >          how the desired total exposure is divided between the
shutter time
> > >          and the sensor's analogue gain. The exposure modes are
platform
> > >          specific, and not all exposure modes may be supported.
> > > +
> > > +        When one of AnalogueGainMode or ExposureTimeMode is set to
> > > Disabled,
> > > +        the fixed values will override any choices made by
AeExposureMode.
> > > +
> > > +        \sa AnalogueGainMode
> > > +        \sa ExposureTimeMode
> > > +
> > >        enum:
> > >          - name: ExposureNormal
> > >            value: 0
> > > @@ -111,13 +141,15 @@ controls:
> > >        type: float
> > >        description: |
> > >          Specify an Exposure Value (EV) parameter. The EV parameter
will
> > > only be
> > > -        applied if the AE algorithm is currently enabled.
> > > +        applied if the AE algorithm is currently enabled, that is, at
> > > least one
> > > +        of AnalogueGainMode and ExposureTimeMode are auto.
> > >
> > >          By convention EV adjusts the exposure as log2. For example
> > >          EV = [-2, -1, 0.5, 0, 0.5, 1, 2] results in an exposure
adjustment
> > >          of [1/4x, 1/2x, 1/sqrt(2)x, 1x, sqrt(2)x, 2x, 4x].
> > >
> > > -        \sa AeEnable
> > > +        \sa AnalogueGainMode
> > > +        \sa ExposureTimeMode
> > >
> > >    - ExposureTime:
> > >        type: int32_t
> > > @@ -125,17 +157,85 @@ controls:
> > >          Exposure time (shutter speed) for the frame applied in the
sensor
> > >          device. This value is specified in micro-seconds.
> > >
> > > -        Setting this value means that it is now fixed and the AE
> > > algorithm may
> > > -        not change it. Setting it back to zero returns it to the
control
> > > of the
> > > -        AE algorithm.
> > > +        This control will only take effect if ExposureTimeMode is
> > > Disabled. If
> > > +        this control is set when ExposureTimeMode is Auto, the value
will
> > > be
> > > +        ignored and will not be retained.
> > > +
> > > +        When reported in metadata, this control indicates what
exposure
> > > time
> > > +        was used for the current request, regardless of
ExposureTimeMode.
> > > +        ExposureTimeMode will indicate the source of the exposure
time
> > > value,
> > > +        whether it came from the AE algorithm or not.
> > > +
> > > +        \sa AnalogueGain
> > > +        \sa ExposureTimeMode
> > > +
> > > +  - ExposureTimeMode:
> > > +      type: int32_t
> > > +      description: |
> > > +        Controls the source of the exposure time that is applied to
the
> > > image
> > > +        sensor. When set to Auto, the AE algorithm computes the
exposure
> > > time
> > > +        and configures the image sensor accordingly. When set to
Disabled,
> > > +        exposure time specified in ExposureTime is applied to the
image
> > > sensor.
> > > +        If ExposureTime is not set, then the value last computed by
the AE
> > > +        algorithm when the mode was Auto will be used.
> > >
> >
> > Can we un-set ExposureTime?  If it ever gets set once at any point in
the
> > application,
> > then ExposureTimeModeDisabled will always use the last set value for
> > ExposureTime.
> >
>
> If I interpret your question right, are you wondering if the
> ExposureTime value is retained if an application sends it when the
> AEGC is actually in auto mode (and so the ExposureTime from application
> is not applied) ?
>
> We discussed this, and I think Paul tried to clarify it in the
> ExposureTime documentation by saying:
>
>   - name: ExposureTimeModeDisabled
>        If ExposureTime is set while this mode is active, it
>        will be ignored, and it will also not be retained.
>
> which means that by design, the ExposureTime is just ignored if sent
> when the AEGC is in auto mode.
>
> Do you think that's not the expected behaviour ?


Not exactly... I was considering the following sequence:

1) ExposureTimeMode is Auto - AE adjusts exposure time as needed.
2) ExposureTimeMode set to Disabled - AE stops adjusting exposure time.
No ExposureTime value set yet, so keep the last AE exposure time.
3) Set ExposureTime to some desired value.
4) ExposureTimeMode set to Auto - AE adjusts exposure time as needed again.
5) ExposureTimeMode  set to  Disabled - AE stops adjusting exposure time.

In step 5, does the IPA switch back to the ExposureTime set in step 3, or
does
setting ExposureTimeMode to Auto invalidate the existing ExposureTime
value? If
the ExposureTime value is still valid, should we consider a way to
invalidate
(unset) it if the application wanted to just use the AE selected exposure
time
at step 5?

>
>
> >
> > > +
> > > +        If ExposureTime is not set and the mode is
> > > ExposureTimeModeDisabled and
> > > +        AE was never Auto (either because the camera started in
Disabled
> > > mode,
> > > +        or Auto is not supported by the camera), the camera should
use a
> > > +        best-effort default value.
> > > +
> > > +        When ExposureTimeMode is set Auto, the value set in
ExposureTime
> > > is
> > > +        ignored and is not retained. This means that if
ExposureTimeMode
> > > is set
> > > +        to Disabled and ExposureTime is not also set, the exposure
time
> > > that
> > > +        was last computed by the AE algorithm while the mode was Auto
> > > will be
> > > +        applied to the sensor.
> > > +
> > > +        If ExposureTimeModeDisabled is supported, the ExposureTime
> > > control must
> > > +        also be supported.
> > > +
> > > +        The set of ExposureTimeMode modes that are supported by the
> > > camera must
> > > +        have an intersection with the supported set of
AnalogueGainMode
> > > modes.
> > >
> > > -        \sa AnalogueGain AeEnable
> > > +        As it takes a few frames to apply the exposure time, there
is a
> > > period of
> > > +        time between submitting a request with ExposureTimeMode set
to
> > > Disabled
> > > +        and the exposure time component of the AE actually being
disabled,
> > > +        during which the AE algorithm can still update the exposure
time.
> > > If an
> > > +        application is switching from automatic and manual control
and
> > > wishes
> > > +        to eliminate any flicker during the switch, the following
> > > procedure is
> > > +        recommended.
> > >
> >
> > I'm a bit confused by this bit to be honest.  If a user switches
> > ExposureTimeMode from
> > Auto to Disabled with the intention of setting a manual ExposureTime,
how
> > can we ever
> > avoid a glitch in the brightness (unless we also change AnalogueGain
> > appropriately)?
> >
>
> See below
>
> >
> > > -        \todo Document the interactions between AeEnable and setting
a
> > > fixed
> > > -        value for this control. Consider interactions with other AE
> > > features,
> > > -        such as aperture and aperture/shutter priority mode, and
decide if
> > > -        control of which features should be automatically adjusted
> > > shouldn't
> > > -        better be handled through a separate AE mode control.
> > > +        1. Start with ExposureTimeMode set to Auto
> > > +
> > > +        2. Set ExposureTimeMode to Disabled
> > > +
> > > +        3. Wait for the first request to be output that has
> > > ExposureTimeMode
> > > +        set to Disabled
> > >
> >
> > How would the application know this time point?  Would the AE algorithm
> > have to
> > count frames once it has been given a ExposureTimeModeDisabled ctrl then
> > return out the same in the metadata when it knows that it's last
requested
> > exposure
> > time change has been applied?
> >
>
> Not sure this is going to answer your question, but let's start by
> defining what a "glitch" is for us in this definition. I think it's
> useful to validate our understanding against your experience of providing
> this features to the vast number of users you have.
>
> The idea is that applications willing to control the exposure time
> explicitly might want to do so by minimizing the difference between
> the last value computed by the AEGC algorithm and their newly set
> value, to avoid a sudden change in exposure and gain which result in a
> visible "glitch". En example, suddenly moving the exposure time and
> gain to the opposite of the spectrum of what the AEGC was computing
> will result in images going very bright or very dark in just a few
> frames.
>
> The way to implement a smooth transition is to start from the values
> lastly computed by the AEGC (as available in metadata) and then "slowly"
> moving towards the desired manual value. Of course this is not
> mandatory, application might desire a sudden change of exposure, or simply
> won't care about smooth transitions. If they do, however, they have to
> consider that there will always be a number of requests in the queue
> that will be processed by the camera before the one with
> "ExposureTimeDisabled" gets to be processed.
>
> During the processing of those requests in the queue, the AEGC will still
> be active and might still change the exposure time (and gain) to values
quite
> different from the ones visible at the application at the time it
> queued the request with "ExposureTimeModeDisabled".
>
> The steps proposed here suggest to applications to wait until the
> "ExposureTimeModeDisabled" request is returned and the AEGC is
> actually off. From the definitions we gave here, this mean the
> exposure time (and gain) won't be updated by the now inactive AEGC
> block until an "ExposureTime" value is submitted by applications (more
> or less like your agc::pause()/resume() work, if I recall correctly).
>
> When the "ExposureTimeModeDisabled" request has completed,
> applications know that the exposure time won't be updated from that
> point on, and can use the ExposureTime and AnalogueGain metadata values
> as a "stable" starting point for their values.
>
> Does this make sense to you ?

Yes it does, thanks for the clarification!

One question still remains - how does the application know when the AE has
actullay finished - i.e. all the AE adjusted frames have been delivered?
Should
the IPA report "ExposureTimeModeDisabled" only when the AE adjust frames
have
been completed, or report it immediately?  The former will require AE to
hold
extra state and keep "running" even when it might be disabled, but it's not
too
big of a problem I suppose.

>
> In my fixups I proposed a rework of the introduction section of this
> part, could you have a look to see if that's more clear ?
>
> >
> > > +
> > > +        4. Copy the value reported in ExposureTime into a new
request, and
> > > +        submit it
> > > +
> > > +        5. Proceed to run manual exposure time
> > >
> >
> > Again, I am unclear how this avoids glitches.  Say the AE chooses an
> > exposure
> > time of 33ms, then the user wants to switch to 15ms.  There is always
going
> > to
> > be a jump in brightness.  Perhaps my interpretation of this glitch is
not
> > the same
> > as what you are describing?
>
> If an application decides not to care and halves the exposure time
> from one request to the following one, the above procedure is useless
> indeed.
>
> But as explained above, an application might want to approach 15ms more
> smoothly and the above text suggests how to do so.
>
> I feel like this is mostly directed to applications that wants to
> drive the AEGC with some sort of algorithm instead of application
> that simply take a value in from users and apply it directly. In
> this latter case the values input from the user might very well be 1ms
> hence approaching it slowly might not even be desired.

Thanks, I do understand when this might be needed now.  But I struggle to
see
why any application might want to do something like this, but that's not to
say
they won't :-)

Regards,
Naush

>
> Thanks for commenting!
>
> >
> > Ditto comments for the AnalogueGain changes.
> >
> > Regards,
> > Naush
> >
> >
> > > +
> > > +        \sa ExposureTime
> > > +      enum:
> > > +        - name: ExposureTimeModeAuto
> > > +          value: 0
> > > +          description: |
> > > +            The exposure time will be calculated automatically and
set by
> > > the
> > > +            AE algorithm. If ExposureTime is set while this mode is
> > > active, it
> > > +            will be ignored, and it will also not be retained.
> > > +        - name: ExposureTimeModeDisabled
> > > +          value: 1
> > > +          description: |
> > > +            The exposure time will not be updated by the AE
algorithm. It
> > > will
> > > +            come from the last calculated value when the mode was
Auto,
> > > or from
> > > +            the value specified in ExposureTime.
> > > +
> > > +            When transitioning from Auto to Disabled mode, the last
> > > computed
> > > +            exposure value is used until a new value is specified
through
> > > the
> > > +            ExposureTime control. If an ExposureTime value is
specified
> > > in the
> > > +            same request where the ExposureTimeMode is changed from
Auto
> > > to
> > > +            Disabled, the provided ExposureTime is applied.
> > >
> > >    - AnalogueGain:
> > >        type: float
> > > @@ -144,17 +244,85 @@ controls:
> > >          The value of the control specifies the gain multiplier
applied to
> > > all
> > >          colour channels. This value cannot be lower than 1.0.
> > >
> > > -        Setting this value means that it is now fixed and the AE
> > > algorithm may
> > > -        not change it. Setting it back to zero returns it to the
control
> > > of the
> > > -        AE algorithm.
> > > +        This control will only take effect if AnalogueGainMode is
> > > Disabled. If
> > > +        this control is set when AnalogueGainMode is Auto, the value
will
> > > be
> > > +        ignored and will not be retained.
> > > +
> > > +        When reported in metadata, this control indicates what
analogue
> > > gain
> > > +        was used for the current request, regardless of
AnalogueGainMode.
> > > +        AnalogueGainMode will indicate the source of the analogue
gain
> > > value,
> > > +        whether it came from the AE algorithm or not.
> > > +
> > > +        \sa ExposureTime
> > > +        \sa AnalogueGainMode
> > > +
> > > +  - AnalogueGainMode:
> > > +      type: int32_t
> > > +      description: |
> > > +        Controls the source of the analogue gain that is applied to
the
> > > image
> > > +        sensor. When set to Auto, the AE algorithm computes the
analogue
> > > gain
> > > +        and configures the image sensor accordingly. When set to
Disabled,
> > > +        analogue gain specified in AnalogueGain is applied to the
image
> > > sensor.
> > > +        If AnalogueGain is not set, then the value last computed by
the AE
> > > +        algorithm when the mode was Auto will be used.
> > > +
> > > +        If AnalogueGain is not set and the mode is
> > > AnalogueGainModeDisabled and
> > > +        AE was never Auto (either because the camera started in
Disabled
> > > mode,
> > > +        or Auto is not supported by the camera), the camera should
use a
> > > +        best-effort default value.
> > > +
> > > +        When AnalogueGainMode is set Auto, the value set in
AnalogueGain
> > > is
> > > +        ignored and is not retained. This means that if
AnalogueGainMode
> > > is set
> > > +        to Disabled and AnalogueGain is not also set, the analogue
gain
> > > that
> > > +        was last computed by the AE algorithm while the mode was Auto
> > > will be
> > > +        applied to the sensor.
> > >
> > > -        \sa ExposureTime AeEnable
> > > +        If AnalogueGainModeDisabled is supported, the AnalogueGain
> > > control must
> > > +        also be supported.
> > > +
> > > +        The set of AnalogueGainMode modes that are supported by the
> > > camera must
> > > +        have an intersection with the supported set of
ExposureTimeMode
> > > modes.
> > > +
> > > +        As it takes a few frames to apply the analogue gain, there
is a
> > > period of
> > > +        time between submitting a request with AnalogueGainMode set
to
> > > Disabled
> > > +        and the analogue gain component of the AE actually being
disabled,
> > > +        during which the AE algorithm can still update the analogue
gain.
> > > If an
> > > +        application is switching from automatic and manual control
and
> > > wishes
> > > +        to eliminate any flicker during the switch, the following
> > > procedure is
> > > +        recommended.
> > > +
> > > +        1. Start with AnalogueGainMode set to Auto
> > > +
> > > +        2. Set AnalogueGainMode to Disabled
> > > +
> > > +        3. Wait for the first request to be output that has
> > > AnalogueGainMode
> > > +        set to Disabled
> > > +
> > > +        4. Copy the value reported in AnalogueGain into a new
request, and
> > > +        submit it
> > > +
> > > +        5. Proceed to run manual analogue gain
> > >
> > +
> > > +        \sa AnalogueGain
> > > +      enum:
> > > +        - name: AnalogueGainModeAuto
> > > +          value: 0
> > > +          description: |
> > > +            The analogue gain will be calculated automatically and
set by
> > > the
> > > +            AE algorithm. If AnalogueGain is set while this mode is
> > > active, it
> > > +            will be ignored, and it will also not be retained.
> > > +        - name: AnalogueGainModeDisabled
> > > +          value: 1
> > > +          description: |
> > > +            The analogue gain will not be updated by the AE
algorithm. It
> > > will
> > > +            come from the last calculated value when the mode was
Auto,
> > > or from
> > > +            the value specified in AnalogueGain.
> > >
> > > -        \todo Document the interactions between AeEnable and setting
a
> > > fixed
> > > -        value for this control. Consider interactions with other AE
> > > features,
> > > -        such as aperture and aperture/shutter priority mode, and
decide if
> > > -        control of which features should be automatically adjusted
> > > shouldn't
> > > -        better be handled through a separate AE mode control.
> > > +            When transitioning from Auto to Disabled mode the last
> > > computed
> > > +            gain value is used until a new value is specified
through the
> > > +            AnalogueGain control. If an AnalogueGain value is
specified
> > > in the
> > > +            same request where the AnalogueGainMode is set to
Disabled,
> > > the
> > > +            provided AnalogueGain is applied.
> > >
> > >    - Brightness:
> > >        type: float
> > > @@ -477,36 +645,6 @@ controls:
> > >              High quality aberration correction which might reduce the
> > > frame
> > >              rate.
> > >
> > > -  - AeState:
> > > -      type: int32_t
> > > -      draft: true
> > > -      description: |
> > > -       Control to report the current AE algorithm state. Currently
> > > identical to
> > > -       ANDROID_CONTROL_AE_STATE.
> > > -
> > > -        Current state of the AE algorithm.
> > > -      enum:
> > > -        - name: AeStateInactive
> > > -          value: 0
> > > -          description: The AE algorithm is inactive.
> > > -        - name: AeStateSearching
> > > -          value: 1
> > > -          description: The AE algorithm has not converged yet.
> > > -        - name: AeStateConverged
> > > -          value: 2
> > > -          description: The AE algorithm has converged.
> > > -        - name: AeStateLocked
> > > -          value: 3
> > > -          description: The AE algorithm is locked.
> > > -        - name: AeStateFlashRequired
> > > -          value: 4
> > > -          description: The AE algorithm would need a flash for good
> > > results
> > > -        - name: AeStatePrecapture
> > > -          value: 5
> > > -          description: |
> > > -            The AE algorithm has started a pre-capture metering
session.
> > > -            \sa AePrecaptureTrigger
> > > -
> > >    - AfState:
> > >        type: int32_t
> > >        draft: true
> > > --
> > > 2.30.2
> > >
> > >

Hi Naush,

On Tue, Jul 05, 2022 at 09:06:39AM +0100, Naushir Patuck wrote:
> Hi Jacopo and Paul,
>
> On Mon, 4 Jul 2022 at 16:30, Jacopo Mondi <jacopo@jmondi.org> wrote:
> >
> > Hi Naush,
> >   sorry Paul if I reply in your place, but I just got through this so
> > I might have it slightly fresher
> >
> > On Mon, Jul 04, 2022 at 11:02:47AM +0100, Naushir Patuck via
> libcamera-devel wrote:
> > > Hi Paul,
> > >
> > > Thank you for your work.  It's nice to see these changes to solidify
> the AE
> > > controls.
> > > I do have a few thoughts/comments below.
> >
> > Can I ask you to have a look at the final result with the fixups I
> > sent in reply applied on top of this patch ?
>
>
> Yes will do!
>
>
> >
> > >
> > >
> > > On Wed, 18 May 2022 at 14:47, Paul Elder via libcamera-devel <
> > > libcamera-devel@lists.libcamera.org> wrote:
> > >
> > > > We have multiple goals:
> > > > - we need a lock of some sort, to instruct the AEGC to not update
> output
> > > >   results
> > > > - we need manual modes, to override the values computed by the AEGC
> >
> > > > - we need to support seamless transitions from auto -> manual, and do
> so
> > > >   without flickering
> > > > - we need custom minimum values for the manual controls, that is no
> > > >   magic values for enabling/disabling auto
> > > > - all of these need to be done with AE sub-controls (exposure time,
> > > >   analogue gain)
> > > >
> > > > To achieve these goals, we introduce mode controls for the AE
> > > > sub-controls: ExposureTimeMode and AnalogueGainMode. These have an
> auto
> > > > state, and a disabled state. The disabled state has an internal
> one-way
> > > > state change from locked to manual, triggered by the presence of the
> > > > value-controls (ExposureTime and AnalogueGain).
> > > >
> > > > We then remove the AeEnable control, as it is a redundant control in
> the
> > > > face of these two mode controls.
> > > >
> > > > We also remove AeLocked, as it is insufficient for reporting the AE
> > > > state, and we promote AeState to non-draft to fill its role. Notably,
> > > > the locked state is removed, since this information can be obtained
> from
> > > > the aforementioned mode controls.
> > > >
> > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=42
> > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=43
> > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=47
> > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > >
> > > > ---
> > > > Changes in v4:
> > > > - remove FlashRequired and Precapture from AeState
> > > > - upgrade documentation of all the controls
> > > >
> > > > Changes in v3:
> > > > - improve wording of the control descriptions
> > > >   - make more succinct and clear
> > > > - add description of how to do a flickerless transition
> > > >
> > > > Changes in v2:
> > > > - No changes, just resubmitting at the head of this series so that
> it's
> > > >   together and so that /people will actually see it/
> > > >
> > > > Initial version:
> > > > Still RFC as I haven't updated the users of the control yet, and I
> want
> > > > to check that these are the controls and docs that we want.
> > > >
> > > > We've decided that the "master AE control" will be implemented by a
> > > > helper... but looking at uvcvideo and the V4L2 controls I'm wondering
> if
> > > > such helper should come earlier than later?
> > > >
> > >
> > > Yes, I agree having the "master AE control" earlier will be beneficial
> for
> > > application developers.
> > >
> >
> > Do you envision this as something that could be part of your
> > applications in example, or in a layer part of libcamera itself ?
>
>
> My preference would be to have a helper in libcamera do this if possible.
> This way, applications don't need to duplicate code to set all AE controls.
> But either way, it's not too big of a deal to put it in the application.
>
> >
> >
> > >
> > > > ---
> > > >  src/libcamera/control_ids.yaml | 262
> +++++++++++++++++++++++++--------
> > > >  1 file changed, 200 insertions(+), 62 deletions(-)
> > > >
> > > > diff --git a/src/libcamera/control_ids.yaml
> > > > b/src/libcamera/control_ids.yaml
> > > > index 9d4638ae..9f5ce5e8 100644
> > > > --- a/src/libcamera/control_ids.yaml
> > > > +++ b/src/libcamera/control_ids.yaml
> > > > @@ -7,23 +7,46 @@
> > > >  # Unless otherwise stated, all controls are bi-directional, i.e.
> they can
> > > > be
> > > >  # set through Request::controls() and returned out through
> > > > Request::metadata().
> > > >  controls:
> > > > -  - AeEnable:
> > > > -      type: bool
> > > > +  - AeState:
> > > > +      type: int32_t
> > > >        description: |
> > > > -        Enable or disable the AE.
> > > > +        Control to report the AE algorithm state associated with the
> > > > capture
> > > > +        result.
> > > >
> > > > -        \sa ExposureTime AnalogueGain
> > > > +        The state is still reported even if ExposureTimeMode or
> > > > +        AnalogueGainMode is set to Disabled.
> > > >
> > > > -  - AeLocked:
> > > > -      type: bool
> > > > -      description: |
> > > > -        Report the lock status of a running AE algorithm.
> > > > +        \sa AnalogueGain
> > > > +        \sa AnalogueGainMode
> > > > +        \sa ExposureTime
> > > > +        \sa ExposureTimeMode
> > > >
> > > > -        If the AE algorithm is locked the value shall be set to
> true, if
> > > > it's
> > > > -        converging it shall be set to false. If the AE algorithm is
> not
> > > > -        running the control shall not be present in the metadata
> control
> > > > list.
> > > > +      enum:
> > > > +        - name: AeStateInactive
> > > > +          value: 0
> > > > +          description: |
> > > > +            The AE algorithm is inactive.
> > > >
> > > > -        \sa AeEnable
> > > > +            This state should be returned if both AnalogueGainMode
> and
> > > > +            ExposureTimeMode are set to disabled (or one, if the
> camera
> > > > only
> > > > +            supports one of the two controls).
> > > > +        - name: AeStateSearching
> > > > +          value: 1
> > > > +          description: |
> > > > +            The AE algorithm has not converged yet.
> > > > +
> > > > +            This state should be returned if at least one of
> > > > AnalogueGainMode
> > > > +            or ExposureTimeMode is set to auto, and the AE algorithm
> > > > hasn't
> > > > +            converged yet. If the AE algorithm converges, the state
> shall
> > > > go to
> > > > +            AeStateConverged.
> > > > +        - name: AeStateConverged
> > > > +          value: 2
> > > > +          description: |
> > > > +            The AE algorithm has converged.
> > > > +
> > > > +            This state should be returned if at least one of
> > > > AnalogueGainMode
> > > > +            or ExposureTimeMode is set to auto, and the AE algorithm
> has
> > > > +            converged.
> > > >
> > > >    # AeMeteringMode needs further attention:
> > > >    # - Auto-generate max enum value.
> > > > @@ -93,6 +116,13 @@ controls:
> > > >          how the desired total exposure is divided between the
> shutter time
> > > >          and the sensor's analogue gain. The exposure modes are
> platform
> > > >          specific, and not all exposure modes may be supported.
> > > > +
> > > > +        When one of AnalogueGainMode or ExposureTimeMode is set to
> > > > Disabled,
> > > > +        the fixed values will override any choices made by
> AeExposureMode.
> > > > +
> > > > +        \sa AnalogueGainMode
> > > > +        \sa ExposureTimeMode
> > > > +
> > > >        enum:
> > > >          - name: ExposureNormal
> > > >            value: 0
> > > > @@ -111,13 +141,15 @@ controls:
> > > >        type: float
> > > >        description: |
> > > >          Specify an Exposure Value (EV) parameter. The EV parameter
> will
> > > > only be
> > > > -        applied if the AE algorithm is currently enabled.
> > > > +        applied if the AE algorithm is currently enabled, that is, at
> > > > least one
> > > > +        of AnalogueGainMode and ExposureTimeMode are auto.
> > > >
> > > >          By convention EV adjusts the exposure as log2. For example
> > > >          EV = [-2, -1, 0.5, 0, 0.5, 1, 2] results in an exposure
> adjustment
> > > >          of [1/4x, 1/2x, 1/sqrt(2)x, 1x, sqrt(2)x, 2x, 4x].
> > > >
> > > > -        \sa AeEnable
> > > > +        \sa AnalogueGainMode
> > > > +        \sa ExposureTimeMode
> > > >
> > > >    - ExposureTime:
> > > >        type: int32_t
> > > > @@ -125,17 +157,85 @@ controls:
> > > >          Exposure time (shutter speed) for the frame applied in the
> sensor
> > > >          device. This value is specified in micro-seconds.
> > > >
> > > > -        Setting this value means that it is now fixed and the AE
> > > > algorithm may
> > > > -        not change it. Setting it back to zero returns it to the
> control
> > > > of the
> > > > -        AE algorithm.
> > > > +        This control will only take effect if ExposureTimeMode is
> > > > Disabled. If
> > > > +        this control is set when ExposureTimeMode is Auto, the value
> will
> > > > be
> > > > +        ignored and will not be retained.
> > > > +
> > > > +        When reported in metadata, this control indicates what
> exposure
> > > > time
> > > > +        was used for the current request, regardless of
> ExposureTimeMode.
> > > > +        ExposureTimeMode will indicate the source of the exposure
> time
> > > > value,
> > > > +        whether it came from the AE algorithm or not.
> > > > +
> > > > +        \sa AnalogueGain
> > > > +        \sa ExposureTimeMode
> > > > +
> > > > +  - ExposureTimeMode:
> > > > +      type: int32_t
> > > > +      description: |
> > > > +        Controls the source of the exposure time that is applied to
> the
> > > > image
> > > > +        sensor. When set to Auto, the AE algorithm computes the
> exposure
> > > > time
> > > > +        and configures the image sensor accordingly. When set to
> Disabled,
> > > > +        exposure time specified in ExposureTime is applied to the
> image
> > > > sensor.
> > > > +        If ExposureTime is not set, then the value last computed by
> the AE
> > > > +        algorithm when the mode was Auto will be used.
> > > >
> > >
> > > Can we un-set ExposureTime?  If it ever gets set once at any point in
> the
> > > application,
> > > then ExposureTimeModeDisabled will always use the last set value for
> > > ExposureTime.
> > >
> >
> > If I interpret your question right, are you wondering if the
> > ExposureTime value is retained if an application sends it when the
> > AEGC is actually in auto mode (and so the ExposureTime from application
> > is not applied) ?
> >
> > We discussed this, and I think Paul tried to clarify it in the
> > ExposureTime documentation by saying:
> >
> >   - name: ExposureTimeModeDisabled
> >        If ExposureTime is set while this mode is active, it
> >        will be ignored, and it will also not be retained.
> >
> > which means that by design, the ExposureTime is just ignored if sent
> > when the AEGC is in auto mode.
> >
> > Do you think that's not the expected behaviour ?
>
>
> Not exactly... I was considering the following sequence:
>
> 1) ExposureTimeMode is Auto - AE adjusts exposure time as needed.
> 2) ExposureTimeMode set to Disabled - AE stops adjusting exposure time.
> No ExposureTime value set yet, so keep the last AE exposure time.
> 3) Set ExposureTime to some desired value.
> 4) ExposureTimeMode set to Auto - AE adjusts exposure time as needed again.
> 5) ExposureTimeMode  set to  Disabled - AE stops adjusting exposure time.
>
> In step 5, does the IPA switch back to the ExposureTime set in step 3, or
> does
> setting ExposureTimeMode to Auto invalidate the existing ExposureTime
> value? If

I would expect step 3 to re-cycle the state machine and invalidate the
manually programmed exposure time

Is this what application developers would expect in your opinion ?

> the ExposureTime value is still valid, should we consider a way to
> invalidate
> (unset) it if the application wanted to just use the AE selected exposure
> time
> at step 5?
>
> >
> >
> > >
> > > > +
> > > > +        If ExposureTime is not set and the mode is
> > > > ExposureTimeModeDisabled and
> > > > +        AE was never Auto (either because the camera started in
> Disabled
> > > > mode,
> > > > +        or Auto is not supported by the camera), the camera should
> use a
> > > > +        best-effort default value.
> > > > +
> > > > +        When ExposureTimeMode is set Auto, the value set in
> ExposureTime
> > > > is
> > > > +        ignored and is not retained. This means that if
> ExposureTimeMode
> > > > is set
> > > > +        to Disabled and ExposureTime is not also set, the exposure
> time
> > > > that
> > > > +        was last computed by the AE algorithm while the mode was Auto
> > > > will be
> > > > +        applied to the sensor.
> > > > +
> > > > +        If ExposureTimeModeDisabled is supported, the ExposureTime
> > > > control must
> > > > +        also be supported.
> > > > +
> > > > +        The set of ExposureTimeMode modes that are supported by the
> > > > camera must
> > > > +        have an intersection with the supported set of
> AnalogueGainMode
> > > > modes.
> > > >
> > > > -        \sa AnalogueGain AeEnable
> > > > +        As it takes a few frames to apply the exposure time, there
> is a
> > > > period of
> > > > +        time between submitting a request with ExposureTimeMode set
> to
> > > > Disabled
> > > > +        and the exposure time component of the AE actually being
> disabled,
> > > > +        during which the AE algorithm can still update the exposure
> time.
> > > > If an
> > > > +        application is switching from automatic and manual control
> and
> > > > wishes
> > > > +        to eliminate any flicker during the switch, the following
> > > > procedure is
> > > > +        recommended.
> > > >
> > >
> > > I'm a bit confused by this bit to be honest.  If a user switches
> > > ExposureTimeMode from
> > > Auto to Disabled with the intention of setting a manual ExposureTime,
> how
> > > can we ever
> > > avoid a glitch in the brightness (unless we also change AnalogueGain
> > > appropriately)?
> > >
> >
> > See below
> >
> > >
> > > > -        \todo Document the interactions between AeEnable and setting
> a
> > > > fixed
> > > > -        value for this control. Consider interactions with other AE
> > > > features,
> > > > -        such as aperture and aperture/shutter priority mode, and
> decide if
> > > > -        control of which features should be automatically adjusted
> > > > shouldn't
> > > > -        better be handled through a separate AE mode control.
> > > > +        1. Start with ExposureTimeMode set to Auto
> > > > +
> > > > +        2. Set ExposureTimeMode to Disabled
> > > > +
> > > > +        3. Wait for the first request to be output that has
> > > > ExposureTimeMode
> > > > +        set to Disabled
> > > >
> > >
> > > How would the application know this time point?  Would the AE algorithm
> > > have to
> > > count frames once it has been given a ExposureTimeModeDisabled ctrl then
> > > return out the same in the metadata when it knows that it's last
> requested
> > > exposure
> > > time change has been applied?
> > >
> >
> > Not sure this is going to answer your question, but let's start by
> > defining what a "glitch" is for us in this definition. I think it's
> > useful to validate our understanding against your experience of providing
> > this features to the vast number of users you have.
> >
> > The idea is that applications willing to control the exposure time
> > explicitly might want to do so by minimizing the difference between
> > the last value computed by the AEGC algorithm and their newly set
> > value, to avoid a sudden change in exposure and gain which result in a
> > visible "glitch". En example, suddenly moving the exposure time and
> > gain to the opposite of the spectrum of what the AEGC was computing
> > will result in images going very bright or very dark in just a few
> > frames.
> >
> > The way to implement a smooth transition is to start from the values
> > lastly computed by the AEGC (as available in metadata) and then "slowly"
> > moving towards the desired manual value. Of course this is not
> > mandatory, application might desire a sudden change of exposure, or simply
> > won't care about smooth transitions. If they do, however, they have to
> > consider that there will always be a number of requests in the queue
> > that will be processed by the camera before the one with
> > "ExposureTimeDisabled" gets to be processed.
> >
> > During the processing of those requests in the queue, the AEGC will still
> > be active and might still change the exposure time (and gain) to values
> quite
> > different from the ones visible at the application at the time it
> > queued the request with "ExposureTimeModeDisabled".
> >
> > The steps proposed here suggest to applications to wait until the
> > "ExposureTimeModeDisabled" request is returned and the AEGC is
> > actually off. From the definitions we gave here, this mean the
> > exposure time (and gain) won't be updated by the now inactive AEGC
> > block until an "ExposureTime" value is submitted by applications (more
> > or less like your agc::pause()/resume() work, if I recall correctly).
> >
> > When the "ExposureTimeModeDisabled" request has completed,
> > applications know that the exposure time won't be updated from that
> > point on, and can use the ExposureTime and AnalogueGain metadata values
> > as a "stable" starting point for their values.
> >
> > Does this make sense to you ?
>
> Yes it does, thanks for the clarification!
>
> One question still remains - how does the application know when the AE has
> actullay finished - i.e. all the AE adjusted frames have been delivered?
> Should
> the IPA report "ExposureTimeModeDisabled" only when the AE adjust frames
> have
> been completed, or report it immediately?  The former will require AE to

I do expect IPAs to process all requests with AE activated until they
don't get to crunch the one that has "ExposureTimeModeDisabled" in the
controls list, then "pause" their AGC and set
"ExposureTimeModeDisabled" in metadata (with the currently programmed
exposure time in "ExposureTime"). From that point on the
exposure time won't be updated anymore by AEGC.

Between the time an application sends a request with
"ExposureTimeModeDisabled" and the time it should send one with an
"ExposureTime" specified there will be a number of requests completed
with the AEGC still reported as active.

> hold
> extra state and keep "running" even when it might be disabled, but it's not
> too
> big of a problem I suppose.
>

I'm missing why the AEGC should keep a state, so I'm probably missing
part of your reasoning

> >
> > In my fixups I proposed a rework of the introduction section of this
> > part, could you have a look to see if that's more clear ?
> >
> > >
> > > > +
> > > > +        4. Copy the value reported in ExposureTime into a new
> request, and
> > > > +        submit it
> > > > +
> > > > +        5. Proceed to run manual exposure time
> > > >
> > >
> > > Again, I am unclear how this avoids glitches.  Say the AE chooses an
> > > exposure
> > > time of 33ms, then the user wants to switch to 15ms.  There is always
> going
> > > to
> > > be a jump in brightness.  Perhaps my interpretation of this glitch is
> not
> > > the same
> > > as what you are describing?
> >
> > If an application decides not to care and halves the exposure time
> > from one request to the following one, the above procedure is useless
> > indeed.
> >
> > But as explained above, an application might want to approach 15ms more
> > smoothly and the above text suggests how to do so.
> >
> > I feel like this is mostly directed to applications that wants to
> > drive the AEGC with some sort of algorithm instead of application
> > that simply take a value in from users and apply it directly. In
> > this latter case the values input from the user might very well be 1ms
> > hence approaching it slowly might not even be desired.
>
> Thanks, I do understand when this might be needed now.  But I struggle to
> see
> why any application might want to do something like this, but that's not to
> say
> they won't :-)

To be honest the idea to have such a section in documentation comes
from the fact android does describe it, so I presumed it was kind of a
common parameter.

Thanks for review

>
> Regards,
> Naush
>
> >
> > Thanks for commenting!
> >
> > >
> > > Ditto comments for the AnalogueGain changes.
> > >
> > > Regards,
> > > Naush
> > >
> > >
> > > > +
> > > > +        \sa ExposureTime
> > > > +      enum:
> > > > +        - name: ExposureTimeModeAuto
> > > > +          value: 0
> > > > +          description: |
> > > > +            The exposure time will be calculated automatically and
> set by
> > > > the
> > > > +            AE algorithm. If ExposureTime is set while this mode is
> > > > active, it
> > > > +            will be ignored, and it will also not be retained.
> > > > +        - name: ExposureTimeModeDisabled
> > > > +          value: 1
> > > > +          description: |
> > > > +            The exposure time will not be updated by the AE
> algorithm. It
> > > > will
> > > > +            come from the last calculated value when the mode was
> Auto,
> > > > or from
> > > > +            the value specified in ExposureTime.
> > > > +
> > > > +            When transitioning from Auto to Disabled mode, the last
> > > > computed
> > > > +            exposure value is used until a new value is specified
> through
> > > > the
> > > > +            ExposureTime control. If an ExposureTime value is
> specified
> > > > in the
> > > > +            same request where the ExposureTimeMode is changed from
> Auto
> > > > to
> > > > +            Disabled, the provided ExposureTime is applied.
> > > >
> > > >    - AnalogueGain:
> > > >        type: float
> > > > @@ -144,17 +244,85 @@ controls:
> > > >          The value of the control specifies the gain multiplier
> applied to
> > > > all
> > > >          colour channels. This value cannot be lower than 1.0.
> > > >
> > > > -        Setting this value means that it is now fixed and the AE
> > > > algorithm may
> > > > -        not change it. Setting it back to zero returns it to the
> control
> > > > of the
> > > > -        AE algorithm.
> > > > +        This control will only take effect if AnalogueGainMode is
> > > > Disabled. If
> > > > +        this control is set when AnalogueGainMode is Auto, the value
> will
> > > > be
> > > > +        ignored and will not be retained.
> > > > +
> > > > +        When reported in metadata, this control indicates what
> analogue
> > > > gain
> > > > +        was used for the current request, regardless of
> AnalogueGainMode.
> > > > +        AnalogueGainMode will indicate the source of the analogue
> gain
> > > > value,
> > > > +        whether it came from the AE algorithm or not.
> > > > +
> > > > +        \sa ExposureTime
> > > > +        \sa AnalogueGainMode
> > > > +
> > > > +  - AnalogueGainMode:
> > > > +      type: int32_t
> > > > +      description: |
> > > > +        Controls the source of the analogue gain that is applied to
> the
> > > > image
> > > > +        sensor. When set to Auto, the AE algorithm computes the
> analogue
> > > > gain
> > > > +        and configures the image sensor accordingly. When set to
> Disabled,
> > > > +        analogue gain specified in AnalogueGain is applied to the
> image
> > > > sensor.
> > > > +        If AnalogueGain is not set, then the value last computed by
> the AE
> > > > +        algorithm when the mode was Auto will be used.
> > > > +
> > > > +        If AnalogueGain is not set and the mode is
> > > > AnalogueGainModeDisabled and
> > > > +        AE was never Auto (either because the camera started in
> Disabled
> > > > mode,
> > > > +        or Auto is not supported by the camera), the camera should
> use a
> > > > +        best-effort default value.
> > > > +
> > > > +        When AnalogueGainMode is set Auto, the value set in
> AnalogueGain
> > > > is
> > > > +        ignored and is not retained. This means that if
> AnalogueGainMode
> > > > is set
> > > > +        to Disabled and AnalogueGain is not also set, the analogue
> gain
> > > > that
> > > > +        was last computed by the AE algorithm while the mode was Auto
> > > > will be
> > > > +        applied to the sensor.
> > > >
> > > > -        \sa ExposureTime AeEnable
> > > > +        If AnalogueGainModeDisabled is supported, the AnalogueGain
> > > > control must
> > > > +        also be supported.
> > > > +
> > > > +        The set of AnalogueGainMode modes that are supported by the
> > > > camera must
> > > > +        have an intersection with the supported set of
> ExposureTimeMode
> > > > modes.
> > > > +
> > > > +        As it takes a few frames to apply the analogue gain, there
> is a
> > > > period of
> > > > +        time between submitting a request with AnalogueGainMode set
> to
> > > > Disabled
> > > > +        and the analogue gain component of the AE actually being
> disabled,
> > > > +        during which the AE algorithm can still update the analogue
> gain.
> > > > If an
> > > > +        application is switching from automatic and manual control
> and
> > > > wishes
> > > > +        to eliminate any flicker during the switch, the following
> > > > procedure is
> > > > +        recommended.
> > > > +
> > > > +        1. Start with AnalogueGainMode set to Auto
> > > > +
> > > > +        2. Set AnalogueGainMode to Disabled
> > > > +
> > > > +        3. Wait for the first request to be output that has
> > > > AnalogueGainMode
> > > > +        set to Disabled
> > > > +
> > > > +        4. Copy the value reported in AnalogueGain into a new
> request, and
> > > > +        submit it
> > > > +
> > > > +        5. Proceed to run manual analogue gain
> > > >
> > > +
> > > > +        \sa AnalogueGain
> > > > +      enum:
> > > > +        - name: AnalogueGainModeAuto
> > > > +          value: 0
> > > > +          description: |
> > > > +            The analogue gain will be calculated automatically and
> set by
> > > > the
> > > > +            AE algorithm. If AnalogueGain is set while this mode is
> > > > active, it
> > > > +            will be ignored, and it will also not be retained.
> > > > +        - name: AnalogueGainModeDisabled
> > > > +          value: 1
> > > > +          description: |
> > > > +            The analogue gain will not be updated by the AE
> algorithm. It
> > > > will
> > > > +            come from the last calculated value when the mode was
> Auto,
> > > > or from
> > > > +            the value specified in AnalogueGain.
> > > >
> > > > -        \todo Document the interactions between AeEnable and setting
> a
> > > > fixed
> > > > -        value for this control. Consider interactions with other AE
> > > > features,
> > > > -        such as aperture and aperture/shutter priority mode, and
> decide if
> > > > -        control of which features should be automatically adjusted
> > > > shouldn't
> > > > -        better be handled through a separate AE mode control.
> > > > +            When transitioning from Auto to Disabled mode the last
> > > > computed
> > > > +            gain value is used until a new value is specified
> through the
> > > > +            AnalogueGain control. If an AnalogueGain value is
> specified
> > > > in the
> > > > +            same request where the AnalogueGainMode is set to
> Disabled,
> > > > the
> > > > +            provided AnalogueGain is applied.
> > > >
> > > >    - Brightness:
> > > >        type: float
> > > > @@ -477,36 +645,6 @@ controls:
> > > >              High quality aberration correction which might reduce the
> > > > frame
> > > >              rate.
> > > >
> > > > -  - AeState:
> > > > -      type: int32_t
> > > > -      draft: true
> > > > -      description: |
> > > > -       Control to report the current AE algorithm state. Currently
> > > > identical to
> > > > -       ANDROID_CONTROL_AE_STATE.
> > > > -
> > > > -        Current state of the AE algorithm.
> > > > -      enum:
> > > > -        - name: AeStateInactive
> > > > -          value: 0
> > > > -          description: The AE algorithm is inactive.
> > > > -        - name: AeStateSearching
> > > > -          value: 1
> > > > -          description: The AE algorithm has not converged yet.
> > > > -        - name: AeStateConverged
> > > > -          value: 2
> > > > -          description: The AE algorithm has converged.
> > > > -        - name: AeStateLocked
> > > > -          value: 3
> > > > -          description: The AE algorithm is locked.
> > > > -        - name: AeStateFlashRequired
> > > > -          value: 4
> > > > -          description: The AE algorithm would need a flash for good
> > > > results
> > > > -        - name: AeStatePrecapture
> > > > -          value: 5
> > > > -          description: |
> > > > -            The AE algorithm has started a pre-capture metering
> session.
> > > > -            \sa AePrecaptureTrigger
> > > > -
> > > >    - AfState:
> > > >        type: int32_t
> > > >        draft: true
> > > > --
> > > > 2.30.2
> > > >
> > > >

Hi Jacopo

On Tue, 5 Jul 2022 at 10:17, Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hi Naush,
>
> On Tue, Jul 05, 2022 at 09:06:39AM +0100, Naushir Patuck wrote:
> > Hi Jacopo and Paul,
> >
> > On Mon, 4 Jul 2022 at 16:30, Jacopo Mondi <jacopo@jmondi.org> wrote:
> > >
> > > Hi Naush,
> > >   sorry Paul if I reply in your place, but I just got through this so
> > > I might have it slightly fresher
> > >
> > > On Mon, Jul 04, 2022 at 11:02:47AM +0100, Naushir Patuck via
> > libcamera-devel wrote:
> > > > Hi Paul,
> > > >
> > > > Thank you for your work.  It's nice to see these changes to solidify
> > the AE
> > > > controls.
> > > > I do have a few thoughts/comments below.
> > >
> > > Can I ask you to have a look at the final result with the fixups I
> > > sent in reply applied on top of this patch ?
> >
> >
> > Yes will do!
> >
> >
> > >
> > > >
> > > >
> > > > On Wed, 18 May 2022 at 14:47, Paul Elder via libcamera-devel <
> > > > libcamera-devel@lists.libcamera.org> wrote:
> > > >
> > > > > We have multiple goals:
> > > > > - we need a lock of some sort, to instruct the AEGC to not update
> > output
> > > > >   results
> > > > > - we need manual modes, to override the values computed by the
AEGC
> > >
> > > > > - we need to support seamless transitions from auto -> manual,
and do
> > so
> > > > >   without flickering
> > > > > - we need custom minimum values for the manual controls, that is
no
> > > > >   magic values for enabling/disabling auto
> > > > > - all of these need to be done with AE sub-controls (exposure
time,
> > > > >   analogue gain)
> > > > >
> > > > > To achieve these goals, we introduce mode controls for the AE
> > > > > sub-controls: ExposureTimeMode and AnalogueGainMode. These have an
> > auto
> > > > > state, and a disabled state. The disabled state has an internal
> > one-way
> > > > > state change from locked to manual, triggered by the presence of
the
> > > > > value-controls (ExposureTime and AnalogueGain).
> > > > >
> > > > > We then remove the AeEnable control, as it is a redundant control
in
> > the
> > > > > face of these two mode controls.
> > > > >
> > > > > We also remove AeLocked, as it is insufficient for reporting the
AE
> > > > > state, and we promote AeState to non-draft to fill its role.
Notably,
> > > > > the locked state is removed, since this information can be
obtained
> > from
> > > > > the aforementioned mode controls.
> > > > >
> > > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=42
> > > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=43
> > > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=47
> > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > > >
> > > > > ---
> > > > > Changes in v4:
> > > > > - remove FlashRequired and Precapture from AeState
> > > > > - upgrade documentation of all the controls
> > > > >
> > > > > Changes in v3:
> > > > > - improve wording of the control descriptions
> > > > >   - make more succinct and clear
> > > > > - add description of how to do a flickerless transition
> > > > >
> > > > > Changes in v2:
> > > > > - No changes, just resubmitting at the head of this series so that
> > it's
> > > > >   together and so that /people will actually see it/
> > > > >
> > > > > Initial version:
> > > > > Still RFC as I haven't updated the users of the control yet, and I
> > want
> > > > > to check that these are the controls and docs that we want.
> > > > >
> > > > > We've decided that the "master AE control" will be implemented by
a
> > > > > helper... but looking at uvcvideo and the V4L2 controls I'm
wondering
> > if
> > > > > such helper should come earlier than later?
> > > > >
> > > >
> > > > Yes, I agree having the "master AE control" earlier will be
beneficial
> > for
> > > > application developers.
> > > >
> > >
> > > Do you envision this as something that could be part of your
> > > applications in example, or in a layer part of libcamera itself ?
> >
> >
> > My preference would be to have a helper in libcamera do this if
possible.
> > This way, applications don't need to duplicate code to set all AE
controls.
> > But either way, it's not too big of a deal to put it in the application.
> >
> > >
> > >
> > > >
> > > > > ---
> > > > >  src/libcamera/control_ids.yaml | 262
> > +++++++++++++++++++++++++--------
> > > > >  1 file changed, 200 insertions(+), 62 deletions(-)
> > > > >
> > > > > diff --git a/src/libcamera/control_ids.yaml
> > > > > b/src/libcamera/control_ids.yaml
> > > > > index 9d4638ae..9f5ce5e8 100644
> > > > > --- a/src/libcamera/control_ids.yaml
> > > > > +++ b/src/libcamera/control_ids.yaml
> > > > > @@ -7,23 +7,46 @@
> > > > >  # Unless otherwise stated, all controls are bi-directional, i.e.
> > they can
> > > > > be
> > > > >  # set through Request::controls() and returned out through
> > > > > Request::metadata().
> > > > >  controls:
> > > > > -  - AeEnable:
> > > > > -      type: bool
> > > > > +  - AeState:
> > > > > +      type: int32_t
> > > > >        description: |
> > > > > -        Enable or disable the AE.
> > > > > +        Control to report the AE algorithm state associated with
the
> > > > > capture
> > > > > +        result.
> > > > >
> > > > > -        \sa ExposureTime AnalogueGain
> > > > > +        The state is still reported even if ExposureTimeMode or
> > > > > +        AnalogueGainMode is set to Disabled.
> > > > >
> > > > > -  - AeLocked:
> > > > > -      type: bool
> > > > > -      description: |
> > > > > -        Report the lock status of a running AE algorithm.
> > > > > +        \sa AnalogueGain
> > > > > +        \sa AnalogueGainMode
> > > > > +        \sa ExposureTime
> > > > > +        \sa ExposureTimeMode
> > > > >
> > > > > -        If the AE algorithm is locked the value shall be set to
> > true, if
> > > > > it's
> > > > > -        converging it shall be set to false. If the AE algorithm
is
> > not
> > > > > -        running the control shall not be present in the metadata
> > control
> > > > > list.
> > > > > +      enum:
> > > > > +        - name: AeStateInactive
> > > > > +          value: 0
> > > > > +          description: |
> > > > > +            The AE algorithm is inactive.
> > > > >
> > > > > -        \sa AeEnable
> > > > > +            This state should be returned if both
AnalogueGainMode
> > and
> > > > > +            ExposureTimeMode are set to disabled (or one, if the
> > camera
> > > > > only
> > > > > +            supports one of the two controls).
> > > > > +        - name: AeStateSearching
> > > > > +          value: 1
> > > > > +          description: |
> > > > > +            The AE algorithm has not converged yet.
> > > > > +
> > > > > +            This state should be returned if at least one of
> > > > > AnalogueGainMode
> > > > > +            or ExposureTimeMode is set to auto, and the AE
algorithm
> > > > > hasn't
> > > > > +            converged yet. If the AE algorithm converges, the
state
> > shall
> > > > > go to
> > > > > +            AeStateConverged.
> > > > > +        - name: AeStateConverged
> > > > > +          value: 2
> > > > > +          description: |
> > > > > +            The AE algorithm has converged.
> > > > > +
> > > > > +            This state should be returned if at least one of
> > > > > AnalogueGainMode
> > > > > +            or ExposureTimeMode is set to auto, and the AE
algorithm
> > has
> > > > > +            converged.
> > > > >
> > > > >    # AeMeteringMode needs further attention:
> > > > >    # - Auto-generate max enum value.
> > > > > @@ -93,6 +116,13 @@ controls:
> > > > >          how the desired total exposure is divided between the
> > shutter time
> > > > >          and the sensor's analogue gain. The exposure modes are
> > platform
> > > > >          specific, and not all exposure modes may be supported.
> > > > > +
> > > > > +        When one of AnalogueGainMode or ExposureTimeMode is set
to
> > > > > Disabled,
> > > > > +        the fixed values will override any choices made by
> > AeExposureMode.
> > > > > +
> > > > > +        \sa AnalogueGainMode
> > > > > +        \sa ExposureTimeMode
> > > > > +
> > > > >        enum:
> > > > >          - name: ExposureNormal
> > > > >            value: 0
> > > > > @@ -111,13 +141,15 @@ controls:
> > > > >        type: float
> > > > >        description: |
> > > > >          Specify an Exposure Value (EV) parameter. The EV
parameter
> > will
> > > > > only be
> > > > > -        applied if the AE algorithm is currently enabled.
> > > > > +        applied if the AE algorithm is currently enabled, that
is, at
> > > > > least one
> > > > > +        of AnalogueGainMode and ExposureTimeMode are auto.
> > > > >
> > > > >          By convention EV adjusts the exposure as log2. For
example
> > > > >          EV = [-2, -1, 0.5, 0, 0.5, 1, 2] results in an exposure
> > adjustment
> > > > >          of [1/4x, 1/2x, 1/sqrt(2)x, 1x, sqrt(2)x, 2x, 4x].
> > > > >
> > > > > -        \sa AeEnable
> > > > > +        \sa AnalogueGainMode
> > > > > +        \sa ExposureTimeMode
> > > > >
> > > > >    - ExposureTime:
> > > > >        type: int32_t
> > > > > @@ -125,17 +157,85 @@ controls:
> > > > >          Exposure time (shutter speed) for the frame applied in
the
> > sensor
> > > > >          device. This value is specified in micro-seconds.
> > > > >
> > > > > -        Setting this value means that it is now fixed and the AE
> > > > > algorithm may
> > > > > -        not change it. Setting it back to zero returns it to the
> > control
> > > > > of the
> > > > > -        AE algorithm.
> > > > > +        This control will only take effect if ExposureTimeMode is
> > > > > Disabled. If
> > > > > +        this control is set when ExposureTimeMode is Auto, the
value
> > will
> > > > > be
> > > > > +        ignored and will not be retained.
> > > > > +
> > > > > +        When reported in metadata, this control indicates what
> > exposure
> > > > > time
> > > > > +        was used for the current request, regardless of
> > ExposureTimeMode.
> > > > > +        ExposureTimeMode will indicate the source of the exposure
> > time
> > > > > value,
> > > > > +        whether it came from the AE algorithm or not.
> > > > > +
> > > > > +        \sa AnalogueGain
> > > > > +        \sa ExposureTimeMode
> > > > > +
> > > > > +  - ExposureTimeMode:
> > > > > +      type: int32_t
> > > > > +      description: |
> > > > > +        Controls the source of the exposure time that is applied
to
> > the
> > > > > image
> > > > > +        sensor. When set to Auto, the AE algorithm computes the
> > exposure
> > > > > time
> > > > > +        and configures the image sensor accordingly. When set to
> > Disabled,
> > > > > +        exposure time specified in ExposureTime is applied to the
> > image
> > > > > sensor.
> > > > > +        If ExposureTime is not set, then the value last computed
by
> > the AE
> > > > > +        algorithm when the mode was Auto will be used.
> > > > >
> > > >
> > > > Can we un-set ExposureTime?  If it ever gets set once at any point
in
> > the
> > > > application,
> > > > then ExposureTimeModeDisabled will always use the last set value for
> > > > ExposureTime.
> > > >
> > >
> > > If I interpret your question right, are you wondering if the
> > > ExposureTime value is retained if an application sends it when the
> > > AEGC is actually in auto mode (and so the ExposureTime from
application
> > > is not applied) ?
> > >
> > > We discussed this, and I think Paul tried to clarify it in the
> > > ExposureTime documentation by saying:
> > >
> > >   - name: ExposureTimeModeDisabled
> > >        If ExposureTime is set while this mode is active, it
> > >        will be ignored, and it will also not be retained.
> > >
> > > which means that by design, the ExposureTime is just ignored if sent
> > > when the AEGC is in auto mode.
> > >
> > > Do you think that's not the expected behaviour ?
> >
> >
> > Not exactly... I was considering the following sequence:
> >
> > 1) ExposureTimeMode is Auto - AE adjusts exposure time as needed.
> > 2) ExposureTimeMode set to Disabled - AE stops adjusting exposure time.
> > No ExposureTime value set yet, so keep the last AE exposure time.
> > 3) Set ExposureTime to some desired value.
> > 4) ExposureTimeMode set to Auto - AE adjusts exposure time as needed
again.
> > 5) ExposureTimeMode  set to  Disabled - AE stops adjusting exposure
time.
> >
> > In step 5, does the IPA switch back to the ExposureTime set in step 3,
or
> > does
> > setting ExposureTimeMode to Auto invalidate the existing ExposureTime
> > value? If
>
> I would expect step 3 to re-cycle the state machine and invalidate the
> manually programmed exposure time
>
> Is this what application developers would expect in your opinion ?

That seems reasonable to me, but perhaps worth adding some text to the
documention clarifying this.

>
> > the ExposureTime value is still valid, should we consider a way to
> > invalidate
> > (unset) it if the application wanted to just use the AE selected
exposure
> > time
> > at step 5?
> >
> > >
> > >
> > > >
> > > > > +
> > > > > +        If ExposureTime is not set and the mode is
> > > > > ExposureTimeModeDisabled and
> > > > > +        AE was never Auto (either because the camera started in
> > Disabled
> > > > > mode,
> > > > > +        or Auto is not supported by the camera), the camera
should
> > use a
> > > > > +        best-effort default value.
> > > > > +
> > > > > +        When ExposureTimeMode is set Auto, the value set in
> > ExposureTime
> > > > > is
> > > > > +        ignored and is not retained. This means that if
> > ExposureTimeMode
> > > > > is set
> > > > > +        to Disabled and ExposureTime is not also set, the
exposure
> > time
> > > > > that
> > > > > +        was last computed by the AE algorithm while the mode was
Auto
> > > > > will be
> > > > > +        applied to the sensor.
> > > > > +
> > > > > +        If ExposureTimeModeDisabled is supported, the
ExposureTime
> > > > > control must
> > > > > +        also be supported.
> > > > > +
> > > > > +        The set of ExposureTimeMode modes that are supported by
the
> > > > > camera must
> > > > > +        have an intersection with the supported set of
> > AnalogueGainMode
> > > > > modes.
> > > > >
> > > > > -        \sa AnalogueGain AeEnable
> > > > > +        As it takes a few frames to apply the exposure time,
there
> > is a
> > > > > period of
> > > > > +        time between submitting a request with ExposureTimeMode
set
> > to
> > > > > Disabled
> > > > > +        and the exposure time component of the AE actually being
> > disabled,
> > > > > +        during which the AE algorithm can still update the
exposure
> > time.
> > > > > If an
> > > > > +        application is switching from automatic and manual
control
> > and
> > > > > wishes
> > > > > +        to eliminate any flicker during the switch, the following
> > > > > procedure is
> > > > > +        recommended.
> > > > >
> > > >
> > > > I'm a bit confused by this bit to be honest.  If a user switches
> > > > ExposureTimeMode from
> > > > Auto to Disabled with the intention of setting a manual
ExposureTime,
> > how
> > > > can we ever
> > > > avoid a glitch in the brightness (unless we also change AnalogueGain
> > > > appropriately)?
> > > >
> > >
> > > See below
> > >
> > > >
> > > > > -        \todo Document the interactions between AeEnable and
setting
> > a
> > > > > fixed
> > > > > -        value for this control. Consider interactions with other
AE
> > > > > features,
> > > > > -        such as aperture and aperture/shutter priority mode, and
> > decide if
> > > > > -        control of which features should be automatically
adjusted
> > > > > shouldn't
> > > > > -        better be handled through a separate AE mode control.
> > > > > +        1. Start with ExposureTimeMode set to Auto
> > > > > +
> > > > > +        2. Set ExposureTimeMode to Disabled
> > > > > +
> > > > > +        3. Wait for the first request to be output that has
> > > > > ExposureTimeMode
> > > > > +        set to Disabled
> > > > >
> > > >
> > > > How would the application know this time point?  Would the AE
algorithm
> > > > have to
> > > > count frames once it has been given a ExposureTimeModeDisabled ctrl
then
> > > > return out the same in the metadata when it knows that it's last
> > requested
> > > > exposure
> > > > time change has been applied?
> > > >
> > >
> > > Not sure this is going to answer your question, but let's start by
> > > defining what a "glitch" is for us in this definition. I think it's
> > > useful to validate our understanding against your experience of
providing
> > > this features to the vast number of users you have.
> > >
> > > The idea is that applications willing to control the exposure time
> > > explicitly might want to do so by minimizing the difference between
> > > the last value computed by the AEGC algorithm and their newly set
> > > value, to avoid a sudden change in exposure and gain which result in a
> > > visible "glitch". En example, suddenly moving the exposure time and
> > > gain to the opposite of the spectrum of what the AEGC was computing
> > > will result in images going very bright or very dark in just a few
> > > frames.
> > >
> > > The way to implement a smooth transition is to start from the values
> > > lastly computed by the AEGC (as available in metadata) and then
"slowly"
> > > moving towards the desired manual value. Of course this is not
> > > mandatory, application might desire a sudden change of exposure, or
simply
> > > won't care about smooth transitions. If they do, however, they have to
> > > consider that there will always be a number of requests in the queue
> > > that will be processed by the camera before the one with
> > > "ExposureTimeDisabled" gets to be processed.
> > >
> > > During the processing of those requests in the queue, the AEGC will
still
> > > be active and might still change the exposure time (and gain) to
values
> > quite
> > > different from the ones visible at the application at the time it
> > > queued the request with "ExposureTimeModeDisabled".
> > >
> > > The steps proposed here suggest to applications to wait until the
> > > "ExposureTimeModeDisabled" request is returned and the AEGC is
> > > actually off. From the definitions we gave here, this mean the
> > > exposure time (and gain) won't be updated by the now inactive AEGC
> > > block until an "ExposureTime" value is submitted by applications (more
> > > or less like your agc::pause()/resume() work, if I recall correctly).
> > >
> > > When the "ExposureTimeModeDisabled" request has completed,
> > > applications know that the exposure time won't be updated from that
> > > point on, and can use the ExposureTime and AnalogueGain metadata
values
> > > as a "stable" starting point for their values.
> > >
> > > Does this make sense to you ?
> >
> > Yes it does, thanks for the clarification!
> >
> > One question still remains - how does the application know when the AE
has
> > actullay finished - i.e. all the AE adjusted frames have been delivered?
> > Should
> > the IPA report "ExposureTimeModeDisabled" only when the AE adjust frames
> > have
> > been completed, or report it immediately?  The former will require AE to
>
> I do expect IPAs to process all requests with AE activated until they
> don't get to crunch the one that has "ExposureTimeModeDisabled" in the
> controls list, then "pause" their AGC and set
> "ExposureTimeModeDisabled" in metadata (with the currently programmed
> exposure time in "ExposureTime"). From that point on the
> exposure time won't be updated anymore by AEGC.
>
> Between the time an application sends a request with
> "ExposureTimeModeDisabled" and the time it should send one with an
> "ExposureTime" specified there will be a number of requests completed
> with the AEGC still reported as active.

That answers my question!

>
> > hold
> > extra state and keep "running" even when it might be disabled, but it's
not
> > too
> > big of a problem I suppose.
> >
>
> I'm missing why the AEGC should keep a state, so I'm probably missing
> part of your reasoning

Actually, I was wrong, it's not EGC that keeps state, it is the IPA that
does to
know when to send "ExposureTimeModeDisabled" metadat.

Thanks for the clarification.

Regards,
Naush


>
> > >
> > > In my fixups I proposed a rework of the introduction section of this
> > > part, could you have a look to see if that's more clear ?
> > >
> > > >
> > > > > +
> > > > > +        4. Copy the value reported in ExposureTime into a new
> > request, and
> > > > > +        submit it
> > > > > +
> > > > > +        5. Proceed to run manual exposure time
> > > > >
> > > >
> > > > Again, I am unclear how this avoids glitches.  Say the AE chooses an
> > > > exposure
> > > > time of 33ms, then the user wants to switch to 15ms.  There is
always
> > going
> > > > to
> > > > be a jump in brightness.  Perhaps my interpretation of this glitch
is
> > not
> > > > the same
> > > > as what you are describing?
> > >
> > > If an application decides not to care and halves the exposure time
> > > from one request to the following one, the above procedure is useless
> > > indeed.
> > >
> > > But as explained above, an application might want to approach 15ms
more
> > > smoothly and the above text suggests how to do so.
> > >
> > > I feel like this is mostly directed to applications that wants to
> > > drive the AEGC with some sort of algorithm instead of application
> > > that simply take a value in from users and apply it directly. In
> > > this latter case the values input from the user might very well be 1ms
> > > hence approaching it slowly might not even be desired.
> >
> > Thanks, I do understand when this might be needed now.  But I struggle
to
> > see
> > why any application might want to do something like this, but that's
not to
> > say
> > they won't :-)
>
> To be honest the idea to have such a section in documentation comes
> from the fact android does describe it, so I presumed it was kind of a
> common parameter.
>
> Thanks for review
>
> >
> > Regards,
> > Naush
> >
> > >
> > > Thanks for commenting!
> > >
> > > >
> > > > Ditto comments for the AnalogueGain changes.
> > > >
> > > > Regards,
> > > > Naush
> > > >
> > > >
> > > > > +
> > > > > +        \sa ExposureTime
> > > > > +      enum:
> > > > > +        - name: ExposureTimeModeAuto
> > > > > +          value: 0
> > > > > +          description: |
> > > > > +            The exposure time will be calculated automatically
and
> > set by
> > > > > the
> > > > > +            AE algorithm. If ExposureTime is set while this mode
is
> > > > > active, it
> > > > > +            will be ignored, and it will also not be retained.
> > > > > +        - name: ExposureTimeModeDisabled
> > > > > +          value: 1
> > > > > +          description: |
> > > > > +            The exposure time will not be updated by the AE
> > algorithm. It
> > > > > will
> > > > > +            come from the last calculated value when the mode was
> > Auto,
> > > > > or from
> > > > > +            the value specified in ExposureTime.
> > > > > +
> > > > > +            When transitioning from Auto to Disabled mode, the
last
> > > > > computed
> > > > > +            exposure value is used until a new value is specified
> > through
> > > > > the
> > > > > +            ExposureTime control. If an ExposureTime value is
> > specified
> > > > > in the
> > > > > +            same request where the ExposureTimeMode is changed
from
> > Auto
> > > > > to
> > > > > +            Disabled, the provided ExposureTime is applied.
> > > > >
> > > > >    - AnalogueGain:
> > > > >        type: float
> > > > > @@ -144,17 +244,85 @@ controls:
> > > > >          The value of the control specifies the gain multiplier
> > applied to
> > > > > all
> > > > >          colour channels. This value cannot be lower than 1.0.
> > > > >
> > > > > -        Setting this value means that it is now fixed and the AE
> > > > > algorithm may
> > > > > -        not change it. Setting it back to zero returns it to the
> > control
> > > > > of the
> > > > > -        AE algorithm.
> > > > > +        This control will only take effect if AnalogueGainMode is
> > > > > Disabled. If
> > > > > +        this control is set when AnalogueGainMode is Auto, the
value
> > will
> > > > > be
> > > > > +        ignored and will not be retained.
> > > > > +
> > > > > +        When reported in metadata, this control indicates what
> > analogue
> > > > > gain
> > > > > +        was used for the current request, regardless of
> > AnalogueGainMode.
> > > > > +        AnalogueGainMode will indicate the source of the analogue
> > gain
> > > > > value,
> > > > > +        whether it came from the AE algorithm or not.
> > > > > +
> > > > > +        \sa ExposureTime
> > > > > +        \sa AnalogueGainMode
> > > > > +
> > > > > +  - AnalogueGainMode:
> > > > > +      type: int32_t
> > > > > +      description: |
> > > > > +        Controls the source of the analogue gain that is applied
to
> > the
> > > > > image
> > > > > +        sensor. When set to Auto, the AE algorithm computes the
> > analogue
> > > > > gain
> > > > > +        and configures the image sensor accordingly. When set to
> > Disabled,
> > > > > +        analogue gain specified in AnalogueGain is applied to the
> > image
> > > > > sensor.
> > > > > +        If AnalogueGain is not set, then the value last computed
by
> > the AE
> > > > > +        algorithm when the mode was Auto will be used.
> > > > > +
> > > > > +        If AnalogueGain is not set and the mode is
> > > > > AnalogueGainModeDisabled and
> > > > > +        AE was never Auto (either because the camera started in
> > Disabled
> > > > > mode,
> > > > > +        or Auto is not supported by the camera), the camera
should
> > use a
> > > > > +        best-effort default value.
> > > > > +
> > > > > +        When AnalogueGainMode is set Auto, the value set in
> > AnalogueGain
> > > > > is
> > > > > +        ignored and is not retained. This means that if
> > AnalogueGainMode
> > > > > is set
> > > > > +        to Disabled and AnalogueGain is not also set, the
analogue
> > gain
> > > > > that
> > > > > +        was last computed by the AE algorithm while the mode was
Auto
> > > > > will be
> > > > > +        applied to the sensor.
> > > > >
> > > > > -        \sa ExposureTime AeEnable
> > > > > +        If AnalogueGainModeDisabled is supported, the
AnalogueGain
> > > > > control must
> > > > > +        also be supported.
> > > > > +
> > > > > +        The set of AnalogueGainMode modes that are supported by
the
> > > > > camera must
> > > > > +        have an intersection with the supported set of
> > ExposureTimeMode
> > > > > modes.
> > > > > +
> > > > > +        As it takes a few frames to apply the analogue gain,
there
> > is a
> > > > > period of
> > > > > +        time between submitting a request with AnalogueGainMode
set
> > to
> > > > > Disabled
> > > > > +        and the analogue gain component of the AE actually being
> > disabled,
> > > > > +        during which the AE algorithm can still update the
analogue
> > gain.
> > > > > If an
> > > > > +        application is switching from automatic and manual
control
> > and
> > > > > wishes
> > > > > +        to eliminate any flicker during the switch, the following
> > > > > procedure is
> > > > > +        recommended.
> > > > > +
> > > > > +        1. Start with AnalogueGainMode set to Auto
> > > > > +
> > > > > +        2. Set AnalogueGainMode to Disabled
> > > > > +
> > > > > +        3. Wait for the first request to be output that has
> > > > > AnalogueGainMode
> > > > > +        set to Disabled
> > > > > +
> > > > > +        4. Copy the value reported in AnalogueGain into a new
> > request, and
> > > > > +        submit it
> > > > > +
> > > > > +        5. Proceed to run manual analogue gain
> > > > >
> > > > +
> > > > > +        \sa AnalogueGain
> > > > > +      enum:
> > > > > +        - name: AnalogueGainModeAuto
> > > > > +          value: 0
> > > > > +          description: |
> > > > > +            The analogue gain will be calculated automatically
and
> > set by
> > > > > the
> > > > > +            AE algorithm. If AnalogueGain is set while this mode
is
> > > > > active, it
> > > > > +            will be ignored, and it will also not be retained.
> > > > > +        - name: AnalogueGainModeDisabled
> > > > > +          value: 1
> > > > > +          description: |
> > > > > +            The analogue gain will not be updated by the AE
> > algorithm. It
> > > > > will
> > > > > +            come from the last calculated value when the mode was
> > Auto,
> > > > > or from
> > > > > +            the value specified in AnalogueGain.
> > > > >
> > > > > -        \todo Document the interactions between AeEnable and
setting
> > a
> > > > > fixed
> > > > > -        value for this control. Consider interactions with other
AE
> > > > > features,
> > > > > -        such as aperture and aperture/shutter priority mode, and
> > decide if
> > > > > -        control of which features should be automatically
adjusted
> > > > > shouldn't
> > > > > -        better be handled through a separate AE mode control.
> > > > > +            When transitioning from Auto to Disabled mode the
last
> > > > > computed
> > > > > +            gain value is used until a new value is specified
> > through the
> > > > > +            AnalogueGain control. If an AnalogueGain value is
> > specified
> > > > > in the
> > > > > +            same request where the AnalogueGainMode is set to
> > Disabled,
> > > > > the
> > > > > +            provided AnalogueGain is applied.
> > > > >
> > > > >    - Brightness:
> > > > >        type: float
> > > > > @@ -477,36 +645,6 @@ controls:
> > > > >              High quality aberration correction which might
reduce the
> > > > > frame
> > > > >              rate.
> > > > >
> > > > > -  - AeState:
> > > > > -      type: int32_t
> > > > > -      draft: true
> > > > > -      description: |
> > > > > -       Control to report the current AE algorithm state.
Currently
> > > > > identical to
> > > > > -       ANDROID_CONTROL_AE_STATE.
> > > > > -
> > > > > -        Current state of the AE algorithm.
> > > > > -      enum:
> > > > > -        - name: AeStateInactive
> > > > > -          value: 0
> > > > > -          description: The AE algorithm is inactive.
> > > > > -        - name: AeStateSearching
> > > > > -          value: 1
> > > > > -          description: The AE algorithm has not converged yet.
> > > > > -        - name: AeStateConverged
> > > > > -          value: 2
> > > > > -          description: The AE algorithm has converged.
> > > > > -        - name: AeStateLocked
> > > > > -          value: 3
> > > > > -          description: The AE algorithm is locked.
> > > > > -        - name: AeStateFlashRequired
> > > > > -          value: 4
> > > > > -          description: The AE algorithm would need a flash for
good
> > > > > results
> > > > > -        - name: AeStatePrecapture
> > > > > -          value: 5
> > > > > -          description: |
> > > > > -            The AE algorithm has started a pre-capture metering
> > session.
> > > > > -            \sa AePrecaptureTrigger
> > > > > -
> > > > >    - AfState:
> > > > >        type: int32_t
> > > > >        draft: true
> > > > > --
> > > > > 2.30.2
> > > > >
> > > > >