[libcamera-devel,v2,1/7] controls: Reorganize the AE-related controls
diff mbox series

Message ID 20211001103325.1077590-2-paul.elder@ideasonboard.com
State New
Headers show
Series
  • The Great AE Changes
Related show

Commit Message

Paul Elder Oct. 1, 2021, 10:33 a.m. UTC
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 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 | 215 +++++++++++++++++++++++----------
 1 file changed, 150 insertions(+), 65 deletions(-)

Comments

David Plowman Oct. 1, 2021, 12:53 p.m. UTC | #1
Hi Paul

Thanks for all this work! I was fine with all the functionality being
described, and mostly everything made sense to me, but there were just
a couple of places where I thought we might be able to explain things
more clearly. But this is just my opinion, others may disagree, so
please take with the necessary quantities of salt!

On Fri, 1 Oct 2021 at 11:33, Paul Elder <paul.elder@ideasonboard.com> 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 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 | 215 +++++++++++++++++++++++----------
>  1 file changed, 150 insertions(+), 65 deletions(-)
>
> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> index 9d4638ae..18b186e8 100644
> --- a/src/libcamera/control_ids.yaml
> +++ b/src/libcamera/control_ids.yaml
> @@ -7,23 +7,61 @@
>  # 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
> -      description: |
> -        Enable or disable the AE.
> -
> -        \sa ExposureTime AnalogueGain
> -
> -  - AeLocked:
> -      type: bool
> +  - AeState:
> +      type: int32_t
>        description: |
> -        Report the lock status of a running AE algorithm.
> -
> -        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.
> +        Control to report the current AE algorithm state. Enabling or disabling
> +        any of the AE-related mode controls (eg. AnalogueGainMode,
> +        ExposureTimeMode) is not required to reset the state to
> +        AeStateInactive. The camera device can do several state transitions
> +        between two results, if it is allowed by the state transition table.
> +        For example, AeStateInactive may never actually be seen in a result.

I was fine until we got to that bit "Enabling or disabling". The rest
of this paragraph struck me as a real burst of detail and corner cases
(?) while I was still struggling with what AeStateInactive meant.

Perhaps some of this specific stuff can be moved until after all the
enum values have been explained? I found the enum descriptions quite
easy to follow and having read through those felt in a better place to
tackle the details. Having said that, I'm still a bit vague as to why
"resetting the state to AeStateInactive" is a particular issue of
concern?

> +
> +        The state in the result is the state for this image (in sync with this
> +        image). If AE state becomes AeStateConverged, then the image data
> +        associated with the result should be good to use.
> +
> +        As some AE algorithms may still be running when any of the AE-related
> +        controls are in manual mode, the states are valid even when the mode
> +        controls are set to Disabled. To know if this image image used AE or

"image" is repeated.

Do we need to say anything about what happens when all controls are
disabled? (Does anything different happen??)

> +        manual (or a mixture), check the relevant modes (eg. AnalogueGainMode,
> +        ExposureTimeMode).
> +
> +        \sa AnalogueGain
> +        \sa AnalogueGainMode
> +        \sa ExposureTime
> +        \sa ExposureTimeMode
>
> -        \sa AeEnable
> +      enum:
> +        - name: AeStateInactive
> +          value: 0
> +          description: |
> +            The AE algorithm is inactive.
> +            If the camera initiates an AE scan, the state shall go to Searching.
> +        - name: AeStateSearching
> +          value: 1
> +          description: |
> +            The AE algorithm has not converged yet.
> +            If the camera finishes an AE scan, the state shall go to Converged.
> +            If the camera finishes an AE scan, but flash is required, the state
> +            shall go to FlashRequired.
> +        - name: AeStateConverged
> +          value: 2
> +          description: |
> +            The AE algorithm has converged.
> +            If the camera initiates an AE scan, the state shall go to Searching.
> +        - name: AeStateFlashRequired
> +          value: 3
> +          description: |
> +            The AE algorithm would need a flash for good results
> +            If the camera initiates an AE scan, the state shall go to Searching.
> +            If AeLock is on, the state shall go to Locked.
> +        - name: AeStatePrecapture
> +          value: 4
> +          description: |
> +            The AE algorithm has started a pre-capture metering session.
> +            After the sequence is finished, the state shall go to Converged if

This was all good, my only minor comment might be that using
AeStateConverged and AeStateSearching in place of just
Converged/Searching might be a little more precise?

> +            \sa AePrecaptureTrigger
>
>    # AeMeteringMode needs further attention:
>    # - Auto-generate max enum value.
> @@ -93,6 +131,17 @@ 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 and
> +        a corresponding manual control is provided, AeExposureMode may be

Actually I think the same is true even if you don't give a manual
value. As soon as you fix any of them, the other(s) will vary in such
a way as to ignore the exposure profile.

> +        ignored. This is because, for example, if AeExposureMode is set to
> +        ExposureLong but ExposureTimeMode is Disabled and a short ExposureTime
> +        is provided (AnalogueGainMode set to Auto), then the AeExposureMode
> +        doesn't make sense.

The example is good, but seemed a bit wordy to me? Maybe the whole
thing might be:

"When any of AnalogueGainMode or ExposureTimeMode is set to Disabled.
the fixed values will override any choices made by the AeExposureMode which may
consequently be ignored."

(Or perhaps I'm assuming a deeper understanding on the part of the
reader than most will have?)

> +
> +        \sa AnalogueGainMode
> +        \sa ExposureTimeMode
> +
>        enum:
>          - name: ExposureNormal
>            value: 0
> @@ -111,13 +160,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 +176,49 @@ 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. Its
> +        presence in a request acts as a trigger to switch to the internal
> +        manual mode within ExposureTimeModeDisabled.
> +
> +        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 AeEnable
> +        \sa AnalogueGain
> +        \sa ExposureTimeMode
>
> -        \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.
> +  - ExposureTimeMode:
> +      type: int32_t
> +      description: |
> +        Control to set and report the source of the exposure time that will be
> +        applied.
> +
> +        \sa ExposureTime
> +      enum:
> +        - name: ExposureTimeModeAuto
> +          value: 0
> +          description: |
> +            The exposure time will be calculated automatically and set by the
> +            AE algorithm.
> +        - 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.
> +
> +            This Disabled state has two internal states; locked and manual.

TBH I still struggle a bit with the word "locked". I think I prefer
"fixed", it seems more obvious. But not to worry... if the Android
world prefers it then I will get used to it!

Thanks again for doing all this!

Best regards
David

> +            When the Disabled state is first entered, the internal state will
> +            be locked, and the latest exposure time value set by the AE
> +            algorithm will be used (or the default ExposureTime value, if the
> +            mode started out as Disabled). When an ExposureTime is provided,
> +            then the internal state will go to manual, and the provided value
> +            will be used for the exposure time. Going from manual to locked is
> +            not possible. If an ExposureTime value is provided in the same
> +            request as going into the Disabled state, then the internal locked
> +            state will be skipped, and the internal state will start out as
> +            manual.
>
>    - AnalogueGain:
>        type: float
> @@ -144,17 +227,49 @@ 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 ExposureTimeMode is Disabled. Its
> +        presence in a request acts as a trigger to switch to the internal
> +        manual mode within ExposureTimeModeDisabled.
>
> -        \sa ExposureTime AeEnable
> +        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 ExposureTime
> +        \sa AnalogueGainMode
> +
> +  - AnalogueGainMode:
> +      type: int32_t
> +      description: |
> +        Control to set and report the source of the analogue gain that will be
> +        applied.
>
> -        \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.
> +        \sa AnalogueGain
> +      enum:
> +        - name: AnalogueGainModeAuto
> +          value: 0
> +          description: |
> +            The analogue gain will be calculated automatically and set by the
> +            AE algorithm.
> +        - 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.
> +
> +            This Disabled state has two internal states; locked and manual.
> +            When the Disabled state is first entered, the internal state will
> +            be locked, and the latest analogue gain value set by the AE
> +            algorithm will be used (or the default AnalogueGain value, if the
> +            mode started out as Disabled). When an AnalogueGain is provided,
> +            then the internal state will go to manual, and the provided value
> +            will be used for the analogue gain. Going from manual to locked is
> +            not possible. If an AnalogueGain value is provided in the same
> +            request as going into the Disabled state, then the internal locked
> +            state will be skipped, and the internal state will start out as
> +            manual.
>
>    - Brightness:
>        type: float
> @@ -477,36 +592,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.27.0
>
Jacopo Mondi Oct. 1, 2021, 4:08 p.m. UTC | #2
Hi Paul,

On Fri, Oct 01, 2021 at 07:33:19PM +0900, Paul Elder 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 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 | 215 +++++++++++++++++++++++----------
>  1 file changed, 150 insertions(+), 65 deletions(-)
>
> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> index 9d4638ae..18b186e8 100644
> --- a/src/libcamera/control_ids.yaml
> +++ b/src/libcamera/control_ids.yaml
> @@ -7,23 +7,61 @@
>  # 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
> -      description: |
> -        Enable or disable the AE.
> -
> -        \sa ExposureTime AnalogueGain
> -
> -  - AeLocked:
> -      type: bool
> +  - AeState:

Now that we have two separate controls for the exposure time and
analogue gain, do we want a single state for the whole AEGC block ?

> +      type: int32_t
>        description: |
> -        Report the lock status of a running AE algorithm.
> -
> -        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.
> +        Control to report the current AE algorithm state. Enabling or disabling
> +        any of the AE-related mode controls (eg. AnalogueGainMode,
> +        ExposureTimeMode) is not required to reset the state to
> +        AeStateInactive. The camera device can do several state transitions

I understand that, from where we come from the

"Enabling or disabling any of the AE-related mode controls (eg. AnalogueGainMode,
ExposureTimeMode) is not required to reset the state to
AeStateInactive."

makes sense, but you're reading this for the first time you would be
wondering why is this information here, even before what the inactive
state is has been defined. If you consider this relevant, I would
place this in the Inactive state description itself

> +        between two results, if it is allowed by the state transition table.

which table ? :)

> +        For example, AeStateInactive may never actually be seen in a result.

I would place this information in the transition table if we have one

> +
> +        The state in the result is the state for this image (in sync with this

The () part is a repetition, and doesn't this apply to all controls
part of a result ? Don't they all apply to the frames part of the
request they are part of ? (Also I would refer to requests, not
images)

> +        image). If AE state becomes AeStateConverged, then the image data
> +        associated with the result should be good to use.
> +
> +        As some AE algorithms may still be running when any of the AE-related
> +        controls are in manual mode, the states are valid even when the mode

What is manual mode ? Don't we have an "enabled"/"disabled" state only
?

I would just say that the state is reported even if the exposure time
and the analogue gain algorithms are disabled

> +        controls are set to Disabled. To know if this image image used AE or
> +        manual (or a mixture), check the relevant modes (eg. AnalogueGainMode,
> +        ExposureTimeMode).
> +
> +        \sa AnalogueGain
> +        \sa AnalogueGainMode
> +        \sa ExposureTime
> +        \sa ExposureTimeMode
>
> -        \sa AeEnable
> +      enum:
> +        - name: AeStateInactive
> +          value: 0
> +          description: |
> +            The AE algorithm is inactive.
> +            If the camera initiates an AE scan, the state shall go to Searching.
> +        - name: AeStateSearching
> +          value: 1
> +          description: |
> +            The AE algorithm has not converged yet.
> +            If the camera finishes an AE scan, the state shall go to Converged.
> +            If the camera finishes an AE scan, but flash is required, the state
> +            shall go to FlashRequired.
> +        - name: AeStateConverged
> +          value: 2
> +          description: |
> +            The AE algorithm has converged.
> +            If the camera initiates an AE scan, the state shall go to Searching.
> +        - name: AeStateFlashRequired
> +          value: 3
> +          description: |
> +            The AE algorithm would need a flash for good results
> +            If the camera initiates an AE scan, the state shall go to Searching.
> +            If AeLock is on, the state shall go to Locked.
> +        - name: AeStatePrecapture
> +          value: 4
> +          description: |
> +            The AE algorithm has started a pre-capture metering session.
> +            After the sequence is finished, the state shall go to Converged if
> +            \sa AePrecaptureTrigger

We don't have AE capture triggers, nor a transition table defined at
all. And as said above, now we have two controls for exposure and
analog gain, and a single state for the whole AE.

Do we want AeState at all ?

>
>    # AeMeteringMode needs further attention:
>    # - Auto-generate max enum value.
> @@ -93,6 +131,17 @@ 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 and
> +        a corresponding manual control is provided, AeExposureMode may be
> +        ignored. This is because, for example, if AeExposureMode is set to
> +        ExposureLong but ExposureTimeMode is Disabled and a short ExposureTime
> +        is provided (AnalogueGainMode set to Auto), then the AeExposureMode
> +        doesn't make sense.

Why not just:

          This controls takes effect only if ExposureTimeMode is set
          to ExposureTimeModeAuto and it's ignored otherwise

> +
> +        \sa AnalogueGainMode
> +        \sa ExposureTimeMode
> +
>        enum:
>          - name: ExposureNormal
>            value: 0
> @@ -111,13 +160,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.

Mostly a question for RPi who upstreamed this, but
I don't fully get how ExposureValue works. Seems like it is used as a
global multiplier for the analogue gain in AGC::computeGain. Is it
worth reworking it or is it just me that I'm missing some part ?

>
>          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 +176,49 @@ 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. Its
> +        presence in a request acts as a trigger to switch to the internal
> +        manual mode within ExposureTimeModeDisabled.

I find the concept of "internal manual mode" confusing, and I think we
should not insert it in the documentation.

        This control will only take effect if ExposureTimeMode is Disabled.

                                ^ take or takes ?

        and is ignored when ExposureTimeMode is set to Auto.


> +        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 AeEnable
> +        \sa AnalogueGain
> +        \sa ExposureTimeMode
>
> -        \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.
> +  - ExposureTimeMode:

So, I don't want to be too picky, but does it make sense to say a
"Mode" is either "Auto" or "Disabled" ? Aren't "AutoExposure = {Enable,
Disabled}" or "ExposureMode = {Auto, Manual}" better ? I'm asking
without having a real answer to the question.

> +      type: int32_t
> +      description: |
> +        Control to set and report the source of the exposure time that will be
> +        applied.

           Controls how the frame exposure time is computed. When set
           to "Auto" the auto-exposure algorithm computes the exposure
           time of the next frame and configures the image sensor
           accordingly. When set to "Disabled" the auto-exposure
           algorithm stops computing the exposure time.

> +        \sa ExposureTime
> +      enum:
> +        - name: ExposureTimeModeAuto
> +          value: 0
> +          description: |
> +            The exposure time will be calculated automatically and set by the
> +            AE algorithm.
> +        - 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.
> +
> +            This Disabled state has two internal states; locked and manual.
> +            When the Disabled state is first entered, the internal state will
> +            be locked, and the latest exposure time value set by the AE
> +            algorithm will be used (or the default ExposureTime value, if the
> +            mode started out as Disabled). When an ExposureTime is provided,
> +            then the internal state will go to manual, and the provided value
> +            will be used for the exposure time. Going from manual to locked is
> +            not possible. If an ExposureTime value is provided in the same
> +            request as going into the Disabled state, then the internal locked
> +            state will be skipped, and the internal state will start out as
> +            manual.

Is it useful to define such internal states ? Isn't the same expressed
with ?

           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 set to Disabled, the provided
           ExposureTime is applied immediately.

I also think we need to describe how to perform a 'flickerless'
transition, but that depends on what we decide to do with AeState. If
I'm not mistaken we can deduce that the exposure value is 'locked' by
inspecting the ExposureTimeMode value in the result.

We can have a dedicated section in the control description. I'll make
an attempt but I'm sure this can be vastly improved.

           - Flicker-less manual exposure time transition:

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

             In order to select the correct value to use in the first
             Request with the ExposureTime control specified,
             applications should accommodate the natural delay in
             applying controls caused by the capture pipeline frame
             depth.

             When switching to manual exposure time control,
             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
             returned as Disabled in the Request metadata and use the
             there returned exposure time to populate the ExposureTime
             value in the next queued Request.

             The implementation of the auto-exposure time algorithm
             should equally try to minimize flickering and when
             transitioning from manual exposure time control to auto
             exposure, the last used value should be used as starting
             point.

I'm not sure about the last paragraph though, does it match the
outcome of our last discussion ?

>
>    - AnalogueGain:
>        type: float
> @@ -144,17 +227,49 @@ 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 ExposureTimeMode is Disabled. Its
> +        presence in a request acts as a trigger to switch to the internal
> +        manual mode within ExposureTimeModeDisabled.

s/ExposureTimeMode/AnalogueGainMode ?
>
> -        \sa ExposureTime AeEnable
> +        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.

Again, all the 'exposure' should be 'analogue gain' instead ?

> +
> +        \sa ExposureTime
> +        \sa AnalogueGainMode
> +
> +  - AnalogueGainMode:
> +      type: int32_t
> +      description: |
> +        Control to set and report the source of the analogue gain that will be
> +        applied.
>
> -        \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.
> +        \sa AnalogueGain
> +      enum:
> +        - name: AnalogueGainModeAuto
> +          value: 0
> +          description: |
> +            The analogue gain will be calculated automatically and set by the
> +            AE algorithm.
> +        - 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.
> +
> +            This Disabled state has two internal states; locked and manual.
> +            When the Disabled state is first entered, the internal state will
> +            be locked, and the latest analogue gain value set by the AE
> +            algorithm will be used (or the default AnalogueGain value, if the
> +            mode started out as Disabled). When an AnalogueGain is provided,
> +            then the internal state will go to manual, and the provided value
> +            will be used for the analogue gain. Going from manual to locked is
> +            not possible. If an AnalogueGain value is provided in the same
> +            request as going into the Disabled state, then the internal locked
> +            state will be skipped, and the internal state will start out as
> +            manual.

Same comments as per the ExposureTimeMode control.

>
>    - Brightness:
>        type: float
> @@ -477,36 +592,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.27.0
>
David Plowman Oct. 2, 2021, 4:10 a.m. UTC | #3
Hi everyone

Just to answer Jacopo's question that he asked us...

On Fri, 1 Oct 2021 at 17:07, Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hi Paul,
>
> On Fri, Oct 01, 2021 at 07:33:19PM +0900, Paul Elder 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 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 | 215 +++++++++++++++++++++++----------
> >  1 file changed, 150 insertions(+), 65 deletions(-)
> >
> > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> > index 9d4638ae..18b186e8 100644
> > --- a/src/libcamera/control_ids.yaml
> > +++ b/src/libcamera/control_ids.yaml
> > @@ -7,23 +7,61 @@
> >  # 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
> > -      description: |
> > -        Enable or disable the AE.
> > -
> > -        \sa ExposureTime AnalogueGain
> > -
> > -  - AeLocked:
> > -      type: bool
> > +  - AeState:
>
> Now that we have two separate controls for the exposure time and
> analogue gain, do we want a single state for the whole AEGC block ?
>
> > +      type: int32_t
> >        description: |
> > -        Report the lock status of a running AE algorithm.
> > -
> > -        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.
> > +        Control to report the current AE algorithm state. Enabling or disabling
> > +        any of the AE-related mode controls (eg. AnalogueGainMode,
> > +        ExposureTimeMode) is not required to reset the state to
> > +        AeStateInactive. The camera device can do several state transitions
>
> I understand that, from where we come from the
>
> "Enabling or disabling any of the AE-related mode controls (eg. AnalogueGainMode,
> ExposureTimeMode) is not required to reset the state to
> AeStateInactive."
>
> makes sense, but you're reading this for the first time you would be
> wondering why is this information here, even before what the inactive
> state is has been defined. If you consider this relevant, I would
> place this in the Inactive state description itself
>
> > +        between two results, if it is allowed by the state transition table.
>
> which table ? :)
>
> > +        For example, AeStateInactive may never actually be seen in a result.
>
> I would place this information in the transition table if we have one
>
> > +
> > +        The state in the result is the state for this image (in sync with this
>
> The () part is a repetition, and doesn't this apply to all controls
> part of a result ? Don't they all apply to the frames part of the
> request they are part of ? (Also I would refer to requests, not
> images)
>
> > +        image). If AE state becomes AeStateConverged, then the image data
> > +        associated with the result should be good to use.
> > +
> > +        As some AE algorithms may still be running when any of the AE-related
> > +        controls are in manual mode, the states are valid even when the mode
>
> What is manual mode ? Don't we have an "enabled"/"disabled" state only
> ?
>
> I would just say that the state is reported even if the exposure time
> and the analogue gain algorithms are disabled
>
> > +        controls are set to Disabled. To know if this image image used AE or
> > +        manual (or a mixture), check the relevant modes (eg. AnalogueGainMode,
> > +        ExposureTimeMode).
> > +
> > +        \sa AnalogueGain
> > +        \sa AnalogueGainMode
> > +        \sa ExposureTime
> > +        \sa ExposureTimeMode
> >
> > -        \sa AeEnable
> > +      enum:
> > +        - name: AeStateInactive
> > +          value: 0
> > +          description: |
> > +            The AE algorithm is inactive.
> > +            If the camera initiates an AE scan, the state shall go to Searching.
> > +        - name: AeStateSearching
> > +          value: 1
> > +          description: |
> > +            The AE algorithm has not converged yet.
> > +            If the camera finishes an AE scan, the state shall go to Converged.
> > +            If the camera finishes an AE scan, but flash is required, the state
> > +            shall go to FlashRequired.
> > +        - name: AeStateConverged
> > +          value: 2
> > +          description: |
> > +            The AE algorithm has converged.
> > +            If the camera initiates an AE scan, the state shall go to Searching.
> > +        - name: AeStateFlashRequired
> > +          value: 3
> > +          description: |
> > +            The AE algorithm would need a flash for good results
> > +            If the camera initiates an AE scan, the state shall go to Searching.
> > +            If AeLock is on, the state shall go to Locked.
> > +        - name: AeStatePrecapture
> > +          value: 4
> > +          description: |
> > +            The AE algorithm has started a pre-capture metering session.
> > +            After the sequence is finished, the state shall go to Converged if
> > +            \sa AePrecaptureTrigger
>
> We don't have AE capture triggers, nor a transition table defined at
> all. And as said above, now we have two controls for exposure and
> analog gain, and a single state for the whole AE.
>
> Do we want AeState at all ?
>
> >
> >    # AeMeteringMode needs further attention:
> >    # - Auto-generate max enum value.
> > @@ -93,6 +131,17 @@ 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 and
> > +        a corresponding manual control is provided, AeExposureMode may be
> > +        ignored. This is because, for example, if AeExposureMode is set to
> > +        ExposureLong but ExposureTimeMode is Disabled and a short ExposureTime
> > +        is provided (AnalogueGainMode set to Auto), then the AeExposureMode
> > +        doesn't make sense.
>
> Why not just:
>
>           This controls takes effect only if ExposureTimeMode is set
>           to ExposureTimeModeAuto and it's ignored otherwise
>
> > +
> > +        \sa AnalogueGainMode
> > +        \sa ExposureTimeMode
> > +
> >        enum:
> >          - name: ExposureNormal
> >            value: 0
> > @@ -111,13 +160,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.
>
> Mostly a question for RPi who upstreamed this, but
> I don't fully get how ExposureValue works. Seems like it is used as a
> global multiplier for the analogue gain in AGC::computeGain. Is it
> worth reworking it or is it just me that I'm missing some part ?

It's used as a global multiplier for the AGC's _target_ values, at
least in our implementation. For example, generally a 2x increase in
target values will give a 2x increase in combined exposure and
analogue gain, though exactly how much each increases by is determined
by the exposure profile.

If any levers are fixed, then any such increase has to go into the
ones that aren't fixed. Clearly if all are fixed, then changing the ev
can have no effect at all. Does that all make sense? I think Paul's
text seems accurate to me.

Thanks!

David

>
> >
> >          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 +176,49 @@ 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. Its
> > +        presence in a request acts as a trigger to switch to the internal
> > +        manual mode within ExposureTimeModeDisabled.
>
> I find the concept of "internal manual mode" confusing, and I think we
> should not insert it in the documentation.
>
>         This control will only take effect if ExposureTimeMode is Disabled.
>
>                                 ^ take or takes ?
>
>         and is ignored when ExposureTimeMode is set to Auto.
>
>
> > +        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 AeEnable
> > +        \sa AnalogueGain
> > +        \sa ExposureTimeMode
> >
> > -        \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.
> > +  - ExposureTimeMode:
>
> So, I don't want to be too picky, but does it make sense to say a
> "Mode" is either "Auto" or "Disabled" ? Aren't "AutoExposure = {Enable,
> Disabled}" or "ExposureMode = {Auto, Manual}" better ? I'm asking
> without having a real answer to the question.
>
> > +      type: int32_t
> > +      description: |
> > +        Control to set and report the source of the exposure time that will be
> > +        applied.
>
>            Controls how the frame exposure time is computed. When set
>            to "Auto" the auto-exposure algorithm computes the exposure
>            time of the next frame and configures the image sensor
>            accordingly. When set to "Disabled" the auto-exposure
>            algorithm stops computing the exposure time.
>
> > +        \sa ExposureTime
> > +      enum:
> > +        - name: ExposureTimeModeAuto
> > +          value: 0
> > +          description: |
> > +            The exposure time will be calculated automatically and set by the
> > +            AE algorithm.
> > +        - 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.
> > +
> > +            This Disabled state has two internal states; locked and manual.
> > +            When the Disabled state is first entered, the internal state will
> > +            be locked, and the latest exposure time value set by the AE
> > +            algorithm will be used (or the default ExposureTime value, if the
> > +            mode started out as Disabled). When an ExposureTime is provided,
> > +            then the internal state will go to manual, and the provided value
> > +            will be used for the exposure time. Going from manual to locked is
> > +            not possible. If an ExposureTime value is provided in the same
> > +            request as going into the Disabled state, then the internal locked
> > +            state will be skipped, and the internal state will start out as
> > +            manual.
>
> Is it useful to define such internal states ? Isn't the same expressed
> with ?
>
>            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 set to Disabled, the provided
>            ExposureTime is applied immediately.
>
> I also think we need to describe how to perform a 'flickerless'
> transition, but that depends on what we decide to do with AeState. If
> I'm not mistaken we can deduce that the exposure value is 'locked' by
> inspecting the ExposureTimeMode value in the result.
>
> We can have a dedicated section in the control description. I'll make
> an attempt but I'm sure this can be vastly improved.
>
>            - Flicker-less manual exposure time transition:
>
>              Applications that transition from ExposureTimeMode
>              Auto mode to the direct control of the exposure time through
>              the ExposureTime control should aim to do so by selecting
>              an ExposureTime value as close as possible to the last
>              value computed by the auto exposure algorithm to avoid
>              any visible flickering.
>
>              In order to select the correct value to use in the first
>              Request with the ExposureTime control specified,
>              applications should accommodate the natural delay in
>              applying controls caused by the capture pipeline frame
>              depth.
>
>              When switching to manual exposure time control,
>              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
>              returned as Disabled in the Request metadata and use the
>              there returned exposure time to populate the ExposureTime
>              value in the next queued Request.
>
>              The implementation of the auto-exposure time algorithm
>              should equally try to minimize flickering and when
>              transitioning from manual exposure time control to auto
>              exposure, the last used value should be used as starting
>              point.
>
> I'm not sure about the last paragraph though, does it match the
> outcome of our last discussion ?
>
> >
> >    - AnalogueGain:
> >        type: float
> > @@ -144,17 +227,49 @@ 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 ExposureTimeMode is Disabled. Its
> > +        presence in a request acts as a trigger to switch to the internal
> > +        manual mode within ExposureTimeModeDisabled.
>
> s/ExposureTimeMode/AnalogueGainMode ?
> >
> > -        \sa ExposureTime AeEnable
> > +        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.
>
> Again, all the 'exposure' should be 'analogue gain' instead ?
>
> > +
> > +        \sa ExposureTime
> > +        \sa AnalogueGainMode
> > +
> > +  - AnalogueGainMode:
> > +      type: int32_t
> > +      description: |
> > +        Control to set and report the source of the analogue gain that will be
> > +        applied.
> >
> > -        \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.
> > +        \sa AnalogueGain
> > +      enum:
> > +        - name: AnalogueGainModeAuto
> > +          value: 0
> > +          description: |
> > +            The analogue gain will be calculated automatically and set by the
> > +            AE algorithm.
> > +        - 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.
> > +
> > +            This Disabled state has two internal states; locked and manual.
> > +            When the Disabled state is first entered, the internal state will
> > +            be locked, and the latest analogue gain value set by the AE
> > +            algorithm will be used (or the default AnalogueGain value, if the
> > +            mode started out as Disabled). When an AnalogueGain is provided,
> > +            then the internal state will go to manual, and the provided value
> > +            will be used for the analogue gain. Going from manual to locked is
> > +            not possible. If an AnalogueGain value is provided in the same
> > +            request as going into the Disabled state, then the internal locked
> > +            state will be skipped, and the internal state will start out as
> > +            manual.
>
> Same comments as per the ExposureTimeMode control.
>
> >
> >    - Brightness:
> >        type: float
> > @@ -477,36 +592,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.27.0
> >
Paul Elder Oct. 4, 2021, 6:17 a.m. UTC | #4
Hi David,

Thanks for the feedback.

On Fri, Oct 01, 2021 at 01:53:18PM +0100, David Plowman wrote:
> Hi Paul
> 
> Thanks for all this work! I was fine with all the functionality being
> described, and mostly everything made sense to me, but there were just
> a couple of places where I thought we might be able to explain things
> more clearly. But this is just my opinion, others may disagree, so
> please take with the necessary quantities of salt!
> 
> On Fri, 1 Oct 2021 at 11:33, Paul Elder <paul.elder@ideasonboard.com> 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 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 | 215 +++++++++++++++++++++++----------
> >  1 file changed, 150 insertions(+), 65 deletions(-)
> >
> > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> > index 9d4638ae..18b186e8 100644
> > --- a/src/libcamera/control_ids.yaml
> > +++ b/src/libcamera/control_ids.yaml
> > @@ -7,23 +7,61 @@
> >  # 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
> > -      description: |
> > -        Enable or disable the AE.
> > -
> > -        \sa ExposureTime AnalogueGain
> > -
> > -  - AeLocked:
> > -      type: bool
> > +  - AeState:
> > +      type: int32_t
> >        description: |
> > -        Report the lock status of a running AE algorithm.
> > -
> > -        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.
> > +        Control to report the current AE algorithm state. Enabling or disabling
> > +        any of the AE-related mode controls (eg. AnalogueGainMode,
> > +        ExposureTimeMode) is not required to reset the state to
> > +        AeStateInactive. The camera device can do several state transitions
> > +        between two results, if it is allowed by the state transition table.
> > +        For example, AeStateInactive may never actually be seen in a result.
> 
> I was fine until we got to that bit "Enabling or disabling". The rest
> of this paragraph struck me as a real burst of detail and corner cases
> (?) while I was still struggling with what AeStateInactive meant.

Ah, that was because the android description said "enabling or disabling
AE must reset the state to inactive" or something along those lines and
I just substituted that whole line with "no".

> 
> Perhaps some of this specific stuff can be moved until after all the
> enum values have been explained? I found the enum descriptions quite

Not sure I can move it after the enums... but maybe at the end of the
overview at lesat?

> easy to follow and having read through those felt in a better place to

That's good!

> tackle the details. Having said that, I'm still a bit vague as to why
> "resetting the state to AeStateInactive" is a particular issue of
> concern?
> 
> > +
> > +        The state in the result is the state for this image (in sync with this
> > +        image). If AE state becomes AeStateConverged, then the image data
> > +        associated with the result should be good to use.
> > +
> > +        As some AE algorithms may still be running when any of the AE-related
> > +        controls are in manual mode, the states are valid even when the mode
> > +        controls are set to Disabled. To know if this image image used AE or
> 
> "image" is repeated.
> 
> Do we need to say anything about what happens when all controls are
> disabled? (Does anything different happen??)

Maybe I should've said "if at least one is disabled".

> 
> > +        manual (or a mixture), check the relevant modes (eg. AnalogueGainMode,
> > +        ExposureTimeMode).
> > +
> > +        \sa AnalogueGain
> > +        \sa AnalogueGainMode
> > +        \sa ExposureTime
> > +        \sa ExposureTimeMode
> >
> > -        \sa AeEnable
> > +      enum:
> > +        - name: AeStateInactive
> > +          value: 0
> > +          description: |
> > +            The AE algorithm is inactive.
> > +            If the camera initiates an AE scan, the state shall go to Searching.
> > +        - name: AeStateSearching
> > +          value: 1
> > +          description: |
> > +            The AE algorithm has not converged yet.
> > +            If the camera finishes an AE scan, the state shall go to Converged.
> > +            If the camera finishes an AE scan, but flash is required, the state
> > +            shall go to FlashRequired.
> > +        - name: AeStateConverged
> > +          value: 2
> > +          description: |
> > +            The AE algorithm has converged.
> > +            If the camera initiates an AE scan, the state shall go to Searching.
> > +        - name: AeStateFlashRequired
> > +          value: 3
> > +          description: |
> > +            The AE algorithm would need a flash for good results
> > +            If the camera initiates an AE scan, the state shall go to Searching.
> > +            If AeLock is on, the state shall go to Locked.
> > +        - name: AeStatePrecapture
> > +          value: 4
> > +          description: |
> > +            The AE algorithm has started a pre-capture metering session.
> > +            After the sequence is finished, the state shall go to Converged if
> 
> This was all good, my only minor comment might be that using
> AeStateConverged and AeStateSearching in place of just
> Converged/Searching might be a little more precise?

Ah okay. Probably better to be explicit.

> 
> > +            \sa AePrecaptureTrigger
> >
> >    # AeMeteringMode needs further attention:
> >    # - Auto-generate max enum value.
> > @@ -93,6 +131,17 @@ 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 and
> > +        a corresponding manual control is provided, AeExposureMode may be
> 
> Actually I think the same is true even if you don't give a manual
> value. As soon as you fix any of them, the other(s) will vary in such
> a way as to ignore the exposure profile.

Ah yes you're right.

> 
> > +        ignored. This is because, for example, if AeExposureMode is set to
> > +        ExposureLong but ExposureTimeMode is Disabled and a short ExposureTime
> > +        is provided (AnalogueGainMode set to Auto), then the AeExposureMode
> > +        doesn't make sense.
> 
> The example is good, but seemed a bit wordy to me? Maybe the whole
> thing might be:
> 
> "When any of AnalogueGainMode or ExposureTimeMode is set to Disabled.
> the fixed values will override any choices made by the AeExposureMode which may
> consequently be ignored."

That probably is sufficient.

> 
> (Or perhaps I'm assuming a deeper understanding on the part of the
> reader than most will have?)

The example was me thinking aloud when coming up with the new rule :p

> 
> > +
> > +        \sa AnalogueGainMode
> > +        \sa ExposureTimeMode
> > +
> >        enum:
> >          - name: ExposureNormal
> >            value: 0
> > @@ -111,13 +160,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 +176,49 @@ 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. Its
> > +        presence in a request acts as a trigger to switch to the internal
> > +        manual mode within ExposureTimeModeDisabled.
> > +
> > +        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 AeEnable
> > +        \sa AnalogueGain
> > +        \sa ExposureTimeMode
> >
> > -        \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.
> > +  - ExposureTimeMode:
> > +      type: int32_t
> > +      description: |
> > +        Control to set and report the source of the exposure time that will be
> > +        applied.
> > +
> > +        \sa ExposureTime
> > +      enum:
> > +        - name: ExposureTimeModeAuto
> > +          value: 0
> > +          description: |
> > +            The exposure time will be calculated automatically and set by the
> > +            AE algorithm.
> > +        - 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.
> > +
> > +            This Disabled state has two internal states; locked and manual.
> 
> TBH I still struggle a bit with the word "locked". I think I prefer
> "fixed", it seems more obvious. But not to worry... if the Android
> world prefers it then I will get used to it!

I'm fine with either, I've just gotten used to saying "locked" since
that was the informal name of this whole ordeal :/


Thanks,

Paul

> 
> Thanks again for doing all this!
> 
> Best regards
> David
> 
> > +            When the Disabled state is first entered, the internal state will
> > +            be locked, and the latest exposure time value set by the AE
> > +            algorithm will be used (or the default ExposureTime value, if the
> > +            mode started out as Disabled). When an ExposureTime is provided,
> > +            then the internal state will go to manual, and the provided value
> > +            will be used for the exposure time. Going from manual to locked is
> > +            not possible. If an ExposureTime value is provided in the same
> > +            request as going into the Disabled state, then the internal locked
> > +            state will be skipped, and the internal state will start out as
> > +            manual.
> >
> >    - AnalogueGain:
> >        type: float
> > @@ -144,17 +227,49 @@ 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 ExposureTimeMode is Disabled. Its
> > +        presence in a request acts as a trigger to switch to the internal
> > +        manual mode within ExposureTimeModeDisabled.
> >
> > -        \sa ExposureTime AeEnable
> > +        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 ExposureTime
> > +        \sa AnalogueGainMode
> > +
> > +  - AnalogueGainMode:
> > +      type: int32_t
> > +      description: |
> > +        Control to set and report the source of the analogue gain that will be
> > +        applied.
> >
> > -        \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.
> > +        \sa AnalogueGain
> > +      enum:
> > +        - name: AnalogueGainModeAuto
> > +          value: 0
> > +          description: |
> > +            The analogue gain will be calculated automatically and set by the
> > +            AE algorithm.
> > +        - 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.
> > +
> > +            This Disabled state has two internal states; locked and manual.
> > +            When the Disabled state is first entered, the internal state will
> > +            be locked, and the latest analogue gain value set by the AE
> > +            algorithm will be used (or the default AnalogueGain value, if the
> > +            mode started out as Disabled). When an AnalogueGain is provided,
> > +            then the internal state will go to manual, and the provided value
> > +            will be used for the analogue gain. Going from manual to locked is
> > +            not possible. If an AnalogueGain value is provided in the same
> > +            request as going into the Disabled state, then the internal locked
> > +            state will be skipped, and the internal state will start out as
> > +            manual.
> >
> >    - Brightness:
> >        type: float
> > @@ -477,36 +592,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.27.0
> >
Paul Elder Oct. 4, 2021, 6:54 a.m. UTC | #5
Hi Jacopo,

Thanks for the feedback.

On Fri, Oct 01, 2021 at 06:08:19PM +0200, Jacopo Mondi wrote:
> Hi Paul,
> 
> On Fri, Oct 01, 2021 at 07:33:19PM +0900, Paul Elder 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 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 | 215 +++++++++++++++++++++++----------
> >  1 file changed, 150 insertions(+), 65 deletions(-)
> >
> > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> > index 9d4638ae..18b186e8 100644
> > --- a/src/libcamera/control_ids.yaml
> > +++ b/src/libcamera/control_ids.yaml
> > @@ -7,23 +7,61 @@
> >  # 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
> > -      description: |
> > -        Enable or disable the AE.
> > -
> > -        \sa ExposureTime AnalogueGain
> > -
> > -  - AeLocked:
> > -      type: bool
> > +  - AeState:
> 
> Now that we have two separate controls for the exposure time and
> analogue gain, do we want a single state for the whole AEGC block ?

I think so. I learned a while back that the whole AEGC block takes both
E and G and outputs both E and G and that AEC and AGC are not separate
blocks. Thus we have separate controls for the input and output, but a
unified state for the state.

> 
> > +      type: int32_t
> >        description: |
> > -        Report the lock status of a running AE algorithm.
> > -
> > -        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.
> > +        Control to report the current AE algorithm state. Enabling or disabling
> > +        any of the AE-related mode controls (eg. AnalogueGainMode,
> > +        ExposureTimeMode) is not required to reset the state to
> > +        AeStateInactive. The camera device can do several state transitions
> 
> I understand that, from where we come from the
> 
> "Enabling or disabling any of the AE-related mode controls (eg. AnalogueGainMode,
> ExposureTimeMode) is not required to reset the state to
> AeStateInactive."
> 
> makes sense, but you're reading this for the first time you would be
> wondering why is this information here, even before what the inactive
> state is has been defined. If you consider this relevant, I would
> place this in the Inactive state description itself

I think it's a rule for the control in general, so I'll move it to the
end of the overview.

> 
> > +        between two results, if it is allowed by the state transition table.
> 
> which table ? :)

It's just a different representation of a table :p

> 
> > +        For example, AeStateInactive may never actually be seen in a result.
> 
> I would place this information in the transition table if we have one

It's a general rule about the control, so I think it belongs in the
overview. It can be any state, it's just that inactive is the most
trivial example.

> 
> > +
> > +        The state in the result is the state for this image (in sync with this
> 
> The () part is a repetition, and doesn't this apply to all controls
> part of a result ? Don't they all apply to the frames part of the
> request they are part of ? (Also I would refer to requests, not
> images)

Whoops, yeah it is. I just wanted to be explicit (not sure why).

> 
> > +        image). If AE state becomes AeStateConverged, then the image data
> > +        associated with the result should be good to use.
> > +
> > +        As some AE algorithms may still be running when any of the AE-related
> > +        controls are in manual mode, the states are valid even when the mode
> 
> What is manual mode ? Don't we have an "enabled"/"disabled" state only
> ?

Oh yeah s/manual/disabled/ might be better here.

> 
> I would just say that the state is reported even if the exposure time
> and the analogue gain algorithms are disabled

Yeah.

> 
> > +        controls are set to Disabled. To know if this image image used AE or
> > +        manual (or a mixture), check the relevant modes (eg. AnalogueGainMode,
> > +        ExposureTimeMode).
> > +
> > +        \sa AnalogueGain
> > +        \sa AnalogueGainMode
> > +        \sa ExposureTime
> > +        \sa ExposureTimeMode
> >
> > -        \sa AeEnable
> > +      enum:
> > +        - name: AeStateInactive
> > +          value: 0
> > +          description: |
> > +            The AE algorithm is inactive.
> > +            If the camera initiates an AE scan, the state shall go to Searching.
> > +        - name: AeStateSearching
> > +          value: 1
> > +          description: |
> > +            The AE algorithm has not converged yet.
> > +            If the camera finishes an AE scan, the state shall go to Converged.
> > +            If the camera finishes an AE scan, but flash is required, the state
> > +            shall go to FlashRequired.
> > +        - name: AeStateConverged
> > +          value: 2
> > +          description: |
> > +            The AE algorithm has converged.
> > +            If the camera initiates an AE scan, the state shall go to Searching.
> > +        - name: AeStateFlashRequired
> > +          value: 3
> > +          description: |
> > +            The AE algorithm would need a flash for good results
> > +            If the camera initiates an AE scan, the state shall go to Searching.
> > +            If AeLock is on, the state shall go to Locked.
> > +        - name: AeStatePrecapture
> > +          value: 4
> > +          description: |
> > +            The AE algorithm has started a pre-capture metering session.
> > +            After the sequence is finished, the state shall go to Converged if
> > +            \sa AePrecaptureTrigger
> 
> We don't have AE capture triggers, nor a transition table defined at

This is a table in list form :p

idk how capture triggers work yet. I pulled the state in from android.

> all. And as said above, now we have two controls for exposure and
> analog gain, and a single state for the whole AE.
> 
> Do we want AeState at all ?

I think we do. We still need a way to report at least AE converged vs
not-converged, and the state is shared between AEC and AGC as they're
linked together.

> 
> >
> >    # AeMeteringMode needs further attention:
> >    # - Auto-generate max enum value.
> > @@ -93,6 +131,17 @@ 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 and
> > +        a corresponding manual control is provided, AeExposureMode may be
> > +        ignored. This is because, for example, if AeExposureMode is set to
> > +        ExposureLong but ExposureTimeMode is Disabled and a short ExposureTime
> > +        is provided (AnalogueGainMode set to Auto), then the AeExposureMode
> > +        doesn't make sense.
> 
> Why not just:
> 
>           This controls takes effect only if ExposureTimeMode is set
>           to ExposureTimeModeAuto and it's ignored otherwise

AnalogueGainMode also affects this afaict.

In any case, you also think that the example is unnecessary? (I suppose
it was just me reasoning aloud...)

> 
> > +
> > +        \sa AnalogueGainMode
> > +        \sa ExposureTimeMode
> > +
> >        enum:
> >          - name: ExposureNormal
> >            value: 0
> > @@ -111,13 +160,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.
> 
> Mostly a question for RPi who upstreamed this, but
> I don't fully get how ExposureValue works. Seems like it is used as a
> global multiplier for the analogue gain in AGC::computeGain. Is it
> worth reworking it or is it just me that I'm missing some part ?
> 
> >
> >          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 +176,49 @@ 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. Its
> > +        presence in a request acts as a trigger to switch to the internal
> > +        manual mode within ExposureTimeModeDisabled.
> 
> I find the concept of "internal manual mode" confusing, and I think we
> should not insert it in the documentation.

We have a premise that "controls that are submitted in one request but
not resubmitted in subsequent requests still have their value
maintained", and we have these control values that act as triggers for
locked -> manual which violates this premise. Thus we either need to
document that exception (which I think mentioning the internal state is
useful), or get rid of the premise and require all unchanging control
values to still require resubmission.

> 
>         This control will only take effect if ExposureTimeMode is Disabled.
> 
>                                 ^ take or takes ?

The control will -> take

The control (no will) -> takes

The controls will -> take

The controls (no will) -> take

> 
>         and is ignored when ExposureTimeMode is set to Auto.
> 
> 
> > +        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 AeEnable
> > +        \sa AnalogueGain
> > +        \sa ExposureTimeMode
> >
> > -        \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.
> > +  - ExposureTimeMode:
> 
> So, I don't want to be too picky, but does it make sense to say a
> "Mode" is either "Auto" or "Disabled" ? Aren't "AutoExposure = {Enable,
> Disabled}" or "ExposureMode = {Auto, Manual}" better ? I'm asking
> without having a real answer to the question.

I don't have a real answer either. I heard objections against "Mode" but
nobody proposed a better alternative :D

Same thing with "Disabled". It's not "manual" or "locked" or "fixed"
alone, it's a superposition of the two, so we need a word that can
encompass those :/

> 
> > +      type: int32_t
> > +      description: |
> > +        Control to set and report the source of the exposure time that will be
> > +        applied.
> 
>            Controls how the frame exposure time is computed. When set
>            to "Auto" the auto-exposure algorithm computes the exposure
>            time of the next frame and configures the image sensor
>            accordingly. When set to "Disabled" the auto-exposure
>            algorithm stops computing the exposure time.

Oh that's good.

> 
> > +        \sa ExposureTime
> > +      enum:
> > +        - name: ExposureTimeModeAuto
> > +          value: 0
> > +          description: |
> > +            The exposure time will be calculated automatically and set by the
> > +            AE algorithm.
> > +        - 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.
> > +
> > +            This Disabled state has two internal states; locked and manual.
> > +            When the Disabled state is first entered, the internal state will
> > +            be locked, and the latest exposure time value set by the AE
> > +            algorithm will be used (or the default ExposureTime value, if the
> > +            mode started out as Disabled). When an ExposureTime is provided,
> > +            then the internal state will go to manual, and the provided value
> > +            will be used for the exposure time. Going from manual to locked is
> > +            not possible. If an ExposureTime value is provided in the same
> > +            request as going into the Disabled state, then the internal locked
> > +            state will be skipped, and the internal state will start out as
> > +            manual.
> 
> Is it useful to define such internal states ? Isn't the same expressed
> with ?
> 
>            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 set to Disabled, the provided
>            ExposureTime is applied immediately.

True, that is sufficient. I was thinking that there was some value in
being explicit with states...

> 
> I also think we need to describe how to perform a 'flickerless'

Ah yeah we should.

> transition, but that depends on what we decide to do with AeState. If
> I'm not mistaken we can deduce that the exposure value is 'locked' by
> inspecting the ExposureTimeMode value in the result.

Yeah, AeState doesn't contribute to determining if AEC or AGC are
locked. Those are determined by the two Mode controls. AeState is for
determining if the combined AEGC has converged or not.

However, the two Mode controls are not sufficient for determining if the
internal AEC/AGC states are actually in the "locked" state (for the
purpose of reporting Lock to android). But that's in a later patch, and
we just tag that state onto the request descriptor.

> 
> We can have a dedicated section in the control description. I'll make
> an attempt but I'm sure this can be vastly improved.
> 
>            - Flicker-less manual exposure time transition:
> 
>              Applications that transition from ExposureTimeMode
>              Auto mode to the direct control of the exposure time through
>              the ExposureTime control should aim to do so by selecting
>              an ExposureTime value as close as possible to the last
>              value computed by the auto exposure algorithm to avoid
>              any visible flickering.
> 
>              In order to select the correct value to use in the first
>              Request with the ExposureTime control specified,
>              applications should accommodate the natural delay in
>              applying controls caused by the capture pipeline frame
>              depth.
> 
>              When switching to manual exposure time control,
>              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
>              returned as Disabled in the Request metadata and use the
>              there returned exposure time to populate the ExposureTime
>              value in the next queued Request.

Ooh that's good.

> 
>              The implementation of the auto-exposure time algorithm
>              should equally try to minimize flickering and when
>              transitioning from manual exposure time control to auto
>              exposure, the last used value should be used as starting
>              point.
> 
> I'm not sure about the last paragraph though, does it match the
> outcome of our last discussion ?

I'm not sure I understand the purpose of the last paragraph. It seems
more directed to IPA developers, that their AEGC should minimize
flickering (afaict that's in their interest already), and that they need
to use the last used value as a stating point when switching from
manual to auto. Maybe the latter point would be good to mention here?

> 
> >
> >    - AnalogueGain:
> >        type: float
> > @@ -144,17 +227,49 @@ 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 ExposureTimeMode is Disabled. Its
> > +        presence in a request acts as a trigger to switch to the internal
> > +        manual mode within ExposureTimeModeDisabled.
> 
> s/ExposureTimeMode/AnalogueGainMode ?
> >
> > -        \sa ExposureTime AeEnable
> > +        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.
> 
> Again, all the 'exposure' should be 'analogue gain' instead ?

Oops, yeah :p

I forgot to fix it in the copy-paste...


Thanks,

Paul

> 
> > +
> > +        \sa ExposureTime
> > +        \sa AnalogueGainMode
> > +
> > +  - AnalogueGainMode:
> > +      type: int32_t
> > +      description: |
> > +        Control to set and report the source of the analogue gain that will be
> > +        applied.
> >
> > -        \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.
> > +        \sa AnalogueGain
> > +      enum:
> > +        - name: AnalogueGainModeAuto
> > +          value: 0
> > +          description: |
> > +            The analogue gain will be calculated automatically and set by the
> > +            AE algorithm.
> > +        - 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.
> > +
> > +            This Disabled state has two internal states; locked and manual.
> > +            When the Disabled state is first entered, the internal state will
> > +            be locked, and the latest analogue gain value set by the AE
> > +            algorithm will be used (or the default AnalogueGain value, if the
> > +            mode started out as Disabled). When an AnalogueGain is provided,
> > +            then the internal state will go to manual, and the provided value
> > +            will be used for the analogue gain. Going from manual to locked is
> > +            not possible. If an AnalogueGain value is provided in the same
> > +            request as going into the Disabled state, then the internal locked
> > +            state will be skipped, and the internal state will start out as
> > +            manual.
> 
> Same comments as per the ExposureTimeMode control.
> 
> >
> >    - Brightness:
> >        type: float
> > @@ -477,36 +592,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.27.0
> >

Patch
diff mbox series

diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
index 9d4638ae..18b186e8 100644
--- a/src/libcamera/control_ids.yaml
+++ b/src/libcamera/control_ids.yaml
@@ -7,23 +7,61 @@ 
 # 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
-      description: |
-        Enable or disable the AE.
-
-        \sa ExposureTime AnalogueGain
-
-  - AeLocked:
-      type: bool
+  - AeState:
+      type: int32_t
       description: |
-        Report the lock status of a running AE algorithm.
-
-        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.
+        Control to report the current AE algorithm state. Enabling or disabling
+        any of the AE-related mode controls (eg. AnalogueGainMode,
+        ExposureTimeMode) is not required to reset the state to
+        AeStateInactive. The camera device can do several state transitions
+        between two results, if it is allowed by the state transition table.
+        For example, AeStateInactive may never actually be seen in a result.
+
+        The state in the result is the state for this image (in sync with this
+        image). If AE state becomes AeStateConverged, then the image data
+        associated with the result should be good to use.
+
+        As some AE algorithms may still be running when any of the AE-related
+        controls are in manual mode, the states are valid even when the mode
+        controls are set to Disabled. To know if this image image used AE or
+        manual (or a mixture), check the relevant modes (eg. AnalogueGainMode,
+        ExposureTimeMode).
+
+        \sa AnalogueGain
+        \sa AnalogueGainMode
+        \sa ExposureTime
+        \sa ExposureTimeMode
 
-        \sa AeEnable
+      enum:
+        - name: AeStateInactive
+          value: 0
+          description: |
+            The AE algorithm is inactive.
+            If the camera initiates an AE scan, the state shall go to Searching.
+        - name: AeStateSearching
+          value: 1
+          description: |
+            The AE algorithm has not converged yet.
+            If the camera finishes an AE scan, the state shall go to Converged.
+            If the camera finishes an AE scan, but flash is required, the state
+            shall go to FlashRequired.
+        - name: AeStateConverged
+          value: 2
+          description: |
+            The AE algorithm has converged.
+            If the camera initiates an AE scan, the state shall go to Searching.
+        - name: AeStateFlashRequired
+          value: 3
+          description: |
+            The AE algorithm would need a flash for good results
+            If the camera initiates an AE scan, the state shall go to Searching.
+            If AeLock is on, the state shall go to Locked.
+        - name: AeStatePrecapture
+          value: 4
+          description: |
+            The AE algorithm has started a pre-capture metering session.
+            After the sequence is finished, the state shall go to Converged if
+            \sa AePrecaptureTrigger
 
   # AeMeteringMode needs further attention:
   # - Auto-generate max enum value.
@@ -93,6 +131,17 @@  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 and
+        a corresponding manual control is provided, AeExposureMode may be
+        ignored. This is because, for example, if AeExposureMode is set to
+        ExposureLong but ExposureTimeMode is Disabled and a short ExposureTime
+        is provided (AnalogueGainMode set to Auto), then the AeExposureMode
+        doesn't make sense.
+
+        \sa AnalogueGainMode
+        \sa ExposureTimeMode
+
       enum:
         - name: ExposureNormal
           value: 0
@@ -111,13 +160,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 +176,49 @@  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. Its
+        presence in a request acts as a trigger to switch to the internal
+        manual mode within ExposureTimeModeDisabled.
+
+        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 AeEnable
+        \sa AnalogueGain
+        \sa ExposureTimeMode
 
-        \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.
+  - ExposureTimeMode:
+      type: int32_t
+      description: |
+        Control to set and report the source of the exposure time that will be
+        applied.
+
+        \sa ExposureTime
+      enum:
+        - name: ExposureTimeModeAuto
+          value: 0
+          description: |
+            The exposure time will be calculated automatically and set by the
+            AE algorithm.
+        - 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.
+
+            This Disabled state has two internal states; locked and manual.
+            When the Disabled state is first entered, the internal state will
+            be locked, and the latest exposure time value set by the AE
+            algorithm will be used (or the default ExposureTime value, if the
+            mode started out as Disabled). When an ExposureTime is provided,
+            then the internal state will go to manual, and the provided value
+            will be used for the exposure time. Going from manual to locked is
+            not possible. If an ExposureTime value is provided in the same
+            request as going into the Disabled state, then the internal locked
+            state will be skipped, and the internal state will start out as
+            manual.
 
   - AnalogueGain:
       type: float
@@ -144,17 +227,49 @@  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 ExposureTimeMode is Disabled. Its
+        presence in a request acts as a trigger to switch to the internal
+        manual mode within ExposureTimeModeDisabled.
 
-        \sa ExposureTime AeEnable
+        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 ExposureTime
+        \sa AnalogueGainMode
+
+  - AnalogueGainMode:
+      type: int32_t
+      description: |
+        Control to set and report the source of the analogue gain that will be
+        applied.
 
-        \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.
+        \sa AnalogueGain
+      enum:
+        - name: AnalogueGainModeAuto
+          value: 0
+          description: |
+            The analogue gain will be calculated automatically and set by the
+            AE algorithm.
+        - 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.
+
+            This Disabled state has two internal states; locked and manual.
+            When the Disabled state is first entered, the internal state will
+            be locked, and the latest analogue gain value set by the AE
+            algorithm will be used (or the default AnalogueGain value, if the
+            mode started out as Disabled). When an AnalogueGain is provided,
+            then the internal state will go to manual, and the provided value
+            will be used for the analogue gain. Going from manual to locked is
+            not possible. If an AnalogueGain value is provided in the same
+            request as going into the Disabled state, then the internal locked
+            state will be skipped, and the internal state will start out as
+            manual.
 
   - Brightness:
       type: float
@@ -477,36 +592,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