[libcamera-devel,RFC,03/14] controls: Replace AeLocked with AeState, and add AeLock
diff mbox series

Message ID 20210618103351.1642060-4-paul.elder@ideasonboard.com
State Superseded
Delegated to: Paul Elder
Headers show
Series
  • Preliminary FULL plumbing
Related show

Commit Message

Paul Elder June 18, 2021, 10:33 a.m. UTC 
AeLocked alone isn't sufficient for reporting the AE state, so replace
it with AeState. Add an AeLock control for instructing the camera to
lock the AE values.

Update the current users of AeLocked accordingly.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
---
 src/ipa/raspberrypi/raspberrypi.cpp |  5 +-
 src/ipa/rkisp1/rkisp1.cpp           | 13 ++--
 src/libcamera/control_ids.yaml      | 96 ++++++++++++++++++-----------
 3 files changed, 71 insertions(+), 43 deletions(-)

Comments

Naushir Patuck June 18, 2021, 12:07 p.m. UTC | #1 
Hi Paul,

Thank you for your patch.

On Fri, 18 Jun 2021 at 11:34, Paul Elder <paul.elder@ideasonboard.com>
wrote:

> AeLocked alone isn't sufficient for reporting the AE state, so replace
> it with AeState. Add an AeLock control for instructing the camera to
> lock the AE values.
>
> Update the current users of AeLocked accordingly.
>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
>  src/ipa/raspberrypi/raspberrypi.cpp |  5 +-
>  src/ipa/rkisp1/rkisp1.cpp           | 13 ++--
>  src/libcamera/control_ids.yaml      | 96 ++++++++++++++++++-----------
>  3 files changed, 71 insertions(+), 43 deletions(-)
>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> index 1c1e802a..ad0132c0 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -468,7 +468,10 @@ void IPARPi::reportMetadata()
>
>         AgcStatus *agcStatus =
> rpiMetadata_.GetLocked<AgcStatus>("agc.status");
>         if (agcStatus) {
> -               libcameraMetadata_.set(controls::AeLocked,
> agcStatus->locked);
> +               libcameraMetadata_.set(controls::AeState,
> +                                      agcStatus->locked ?
> +                                      controls::AeStateLocked :
> +                                      controls::AeStateSearching);
>

I think, strictly speaking, if agcStatus->locked == true, we should return
control::AeStateConverged by the definition below.  David, do you agree?

Thanks,
Naush


>                 libcameraMetadata_.set(controls::DigitalGain,
> agcStatus->digital_gain);
>         }
>
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index b47ea324..88a562a8 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -51,7 +51,7 @@ private:
>                               const rkisp1_stat_buffer *stats);
>
>         void setControls(unsigned int frame);
> -       void metadataReady(unsigned int frame, unsigned int aeState);
> +       void metadataReady(unsigned int frame, int aeState);
>
>         std::map<unsigned int, FrameBuffer> buffers_;
>         std::map<unsigned int, void *> buffersMemory_;
> @@ -227,7 +227,7 @@ void IPARkISP1::updateStatistics(unsigned int frame,
>                                  const rkisp1_stat_buffer *stats)
>  {
>         const rkisp1_cif_isp_stat *params = &stats->params;
> -       unsigned int aeState = 0;
> +       int aeState = controls::AeStateInactive;
>
>         if (stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP) {
>                 const rkisp1_cif_isp_ae_stat *ae = &params->ae;
> @@ -262,7 +262,9 @@ void IPARkISP1::updateStatistics(unsigned int frame,
>                         setControls(frame + 1);
>                 }
>
> -               aeState = fabs(factor - 1.0f) < 0.05f ? 2 : 1;
> +               aeState = fabs(factor - 1.0f) < 0.05f ?
> +                         controls::AeStateConverged :
> +                         controls::AeStateSearching;
>         }
>
>         metadataReady(frame, aeState);
> @@ -281,12 +283,11 @@ void IPARkISP1::setControls(unsigned int frame)
>         queueFrameAction.emit(frame, op);
>  }
>
> -void IPARkISP1::metadataReady(unsigned int frame, unsigned int aeState)
> +void IPARkISP1::metadataReady(unsigned int frame, int aeState)
>  {
>         ControlList ctrls(controls::controls);
>
> -       if (aeState)
> -               ctrls.set(controls::AeLocked, aeState == 2);
> +       ctrls.set(controls::AeState, aeState);
>
>         RkISP1Action op;
>         op.op = ActionMetadata;
> diff --git a/src/libcamera/control_ids.yaml
> b/src/libcamera/control_ids.yaml
> index 9d4638ae..5717bc1f 100644
> --- a/src/libcamera/control_ids.yaml
> +++ b/src/libcamera/control_ids.yaml
> @@ -14,16 +14,70 @@ controls:
>
>          \sa ExposureTime AnalogueGain
>
> -  - AeLocked:
> +  - AeLock:
>        type: bool
>        description: |
> -        Report the lock status of a running AE algorithm.
> +        Control to lock the AE values.
> +        When set to true, the AE algorithm is locked to its latest
> parameters,
> +        and will not change exposure settings until set to false.
> +        \sa AeState
>
> -        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.
> +  - AeState:
> +      type: int32_t
> +      description: |
> +        Control to report the current AE algorithm state. Enabling or
> disabling
> +        AE (AeEnable) always resets the AeState 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.
>
> -        \sa AeEnable
> +        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.
> +
> +        The state transitions mentioned below assume that AeEnable is on.
> +
> +        \sa AeLock
> +      enum:
> +        - name: AeStateInactive
> +          value: 0
> +          description: |
> +            The AE algorithm is inactive.
> +            If the camera initiates an AE scan, the state shall go to
> Searching.
> +            If AeLock is on, the state shall go to Locked.
> +        - 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.
> +            If AeLock is on, the state shall go to Locked.
> +        - name: AeStateConverged
> +          value: 2
> +          description: |
> +            The AE algorithm has converged.
> +            If the camera initiates an AE scan, the state shall go to
> Searching.
> +            If AeLock is on, the state shall go to Locked.
> +        - name: AeStateLocked
> +          value: 3
> +          description: |
> +            The AE algorithm is locked.
> +            If AeLock is off, the state can go to Searching, Converged, or
> +            FlashRequired.
> +        - name: AeStateFlashRequired
> +          value: 4
> +          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: 5
> +          description: |
> +            The AE algorithm has started a pre-capture metering session.
> +            After the sequence is finished, the state shall go to
> Converged if
> +            AeLock is off, and Locked if it is on.
> +            \sa AePrecaptureTrigger
>
>    # AeMeteringMode needs further attention:
>    # - Auto-generate max enum value.
> @@ -477,36 +531,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 June 18, 2021, 1:13 p.m. UTC | #2 
Hi Naush, everyone

On Fri, 18 Jun 2021 at 13:07, Naushir Patuck <naush@raspberrypi.com> wrote:
>
> Hi Paul,
>
> Thank you for your patch.
>
> On Fri, 18 Jun 2021 at 11:34, Paul Elder <paul.elder@ideasonboard.com> wrote:
>>
>> AeLocked alone isn't sufficient for reporting the AE state, so replace
>> it with AeState. Add an AeLock control for instructing the camera to
>> lock the AE values.
>>
>> Update the current users of AeLocked accordingly.
>>
>> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
>> ---
>>  src/ipa/raspberrypi/raspberrypi.cpp |  5 +-
>>  src/ipa/rkisp1/rkisp1.cpp           | 13 ++--
>>  src/libcamera/control_ids.yaml      | 96 ++++++++++++++++++-----------
>>  3 files changed, 71 insertions(+), 43 deletions(-)
>>
>> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
>> index 1c1e802a..ad0132c0 100644
>> --- a/src/ipa/raspberrypi/raspberrypi.cpp
>> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
>> @@ -468,7 +468,10 @@ void IPARPi::reportMetadata()
>>
>>         AgcStatus *agcStatus = rpiMetadata_.GetLocked<AgcStatus>("agc.status");
>>         if (agcStatus) {
>> -               libcameraMetadata_.set(controls::AeLocked, agcStatus->locked);
>> +               libcameraMetadata_.set(controls::AeState,
>> +                                      agcStatus->locked ?
>> +                                      controls::AeStateLocked :
>> +                                      controls::AeStateSearching);
>
>
> I think, strictly speaking, if agcStatus->locked == true, we should return
> control::AeStateConverged by the definition below.  David, do you agree?

Hmm, indeed. My reading is that what we used to know as AeEnable has
now been renamed/replaced by AeLock, is that right? And as you say,
"locked" does not mean "converged" (any more).

So:

We set controls::AeLocked in the metadata according to the value that
we received in the corresponding control (which we now receive instead
of AeEnable), and

We set controls::AeState in the metadata to AeStateConverged when
agcStatus->locked is true, otherwise to AeStateSearching. We should
probably bite the bullet and rename that "locked" field to
"converged", otherwise it's going to cause confusion for the rest of
eternity.

David

>
> Thanks,
> Naush
>
>>
>>                 libcameraMetadata_.set(controls::DigitalGain, agcStatus->digital_gain);
>>         }
>>
>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
>> index b47ea324..88a562a8 100644
>> --- a/src/ipa/rkisp1/rkisp1.cpp
>> +++ b/src/ipa/rkisp1/rkisp1.cpp
>> @@ -51,7 +51,7 @@ private:
>>                               const rkisp1_stat_buffer *stats);
>>
>>         void setControls(unsigned int frame);
>> -       void metadataReady(unsigned int frame, unsigned int aeState);
>> +       void metadataReady(unsigned int frame, int aeState);
>>
>>         std::map<unsigned int, FrameBuffer> buffers_;
>>         std::map<unsigned int, void *> buffersMemory_;
>> @@ -227,7 +227,7 @@ void IPARkISP1::updateStatistics(unsigned int frame,
>>                                  const rkisp1_stat_buffer *stats)
>>  {
>>         const rkisp1_cif_isp_stat *params = &stats->params;
>> -       unsigned int aeState = 0;
>> +       int aeState = controls::AeStateInactive;
>>
>>         if (stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP) {
>>                 const rkisp1_cif_isp_ae_stat *ae = &params->ae;
>> @@ -262,7 +262,9 @@ void IPARkISP1::updateStatistics(unsigned int frame,
>>                         setControls(frame + 1);
>>                 }
>>
>> -               aeState = fabs(factor - 1.0f) < 0.05f ? 2 : 1;
>> +               aeState = fabs(factor - 1.0f) < 0.05f ?
>> +                         controls::AeStateConverged :
>> +                         controls::AeStateSearching;
>>         }
>>
>>         metadataReady(frame, aeState);
>> @@ -281,12 +283,11 @@ void IPARkISP1::setControls(unsigned int frame)
>>         queueFrameAction.emit(frame, op);
>>  }
>>
>> -void IPARkISP1::metadataReady(unsigned int frame, unsigned int aeState)
>> +void IPARkISP1::metadataReady(unsigned int frame, int aeState)
>>  {
>>         ControlList ctrls(controls::controls);
>>
>> -       if (aeState)
>> -               ctrls.set(controls::AeLocked, aeState == 2);
>> +       ctrls.set(controls::AeState, aeState);
>>
>>         RkISP1Action op;
>>         op.op = ActionMetadata;
>> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
>> index 9d4638ae..5717bc1f 100644
>> --- a/src/libcamera/control_ids.yaml
>> +++ b/src/libcamera/control_ids.yaml
>> @@ -14,16 +14,70 @@ controls:
>>
>>          \sa ExposureTime AnalogueGain
>>
>> -  - AeLocked:
>> +  - AeLock:
>>        type: bool
>>        description: |
>> -        Report the lock status of a running AE algorithm.
>> +        Control to lock the AE values.
>> +        When set to true, the AE algorithm is locked to its latest parameters,
>> +        and will not change exposure settings until set to false.
>> +        \sa AeState
>>
>> -        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.
>> +  - AeState:
>> +      type: int32_t
>> +      description: |
>> +        Control to report the current AE algorithm state. Enabling or disabling
>> +        AE (AeEnable) always resets the AeState 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.
>>
>> -        \sa AeEnable
>> +        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.
>> +
>> +        The state transitions mentioned below assume that AeEnable is on.
>> +
>> +        \sa AeLock
>> +      enum:
>> +        - name: AeStateInactive
>> +          value: 0
>> +          description: |
>> +            The AE algorithm is inactive.
>> +            If the camera initiates an AE scan, the state shall go to Searching.
>> +            If AeLock is on, the state shall go to Locked.
>> +        - 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.
>> +            If AeLock is on, the state shall go to Locked.
>> +        - name: AeStateConverged
>> +          value: 2
>> +          description: |
>> +            The AE algorithm has converged.
>> +            If the camera initiates an AE scan, the state shall go to Searching.
>> +            If AeLock is on, the state shall go to Locked.
>> +        - name: AeStateLocked
>> +          value: 3
>> +          description: |
>> +            The AE algorithm is locked.
>> +            If AeLock is off, the state can go to Searching, Converged, or
>> +            FlashRequired.
>> +        - name: AeStateFlashRequired
>> +          value: 4
>> +          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: 5
>> +          description: |
>> +            The AE algorithm has started a pre-capture metering session.
>> +            After the sequence is finished, the state shall go to Converged if
>> +            AeLock is off, and Locked if it is on.
>> +            \sa AePrecaptureTrigger
>>
>>    # AeMeteringMode needs further attention:
>>    # - Auto-generate max enum value.
>> @@ -477,36 +531,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
>>
Naushir Patuck June 18, 2021, 1:58 p.m. UTC | #3 
On Fri, 18 Jun 2021 at 14:13, David Plowman <david.plowman@raspberrypi.com>
wrote:

> Hi Naush, everyone
>
> On Fri, 18 Jun 2021 at 13:07, Naushir Patuck <naush@raspberrypi.com>
> wrote:
> >
> > Hi Paul,
> >
> > Thank you for your patch.
> >
> > On Fri, 18 Jun 2021 at 11:34, Paul Elder <paul.elder@ideasonboard.com>
> wrote:
> >>
> >> AeLocked alone isn't sufficient for reporting the AE state, so replace
> >> it with AeState. Add an AeLock control for instructing the camera to
> >> lock the AE values.
> >>
> >> Update the current users of AeLocked accordingly.
> >>
> >> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> >> ---
> >>  src/ipa/raspberrypi/raspberrypi.cpp |  5 +-
> >>  src/ipa/rkisp1/rkisp1.cpp           | 13 ++--
> >>  src/libcamera/control_ids.yaml      | 96 ++++++++++++++++++-----------
> >>  3 files changed, 71 insertions(+), 43 deletions(-)
> >>
> >> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> >> index 1c1e802a..ad0132c0 100644
> >> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> >> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> >> @@ -468,7 +468,10 @@ void IPARPi::reportMetadata()
> >>
> >>         AgcStatus *agcStatus =
> rpiMetadata_.GetLocked<AgcStatus>("agc.status");
> >>         if (agcStatus) {
> >> -               libcameraMetadata_.set(controls::AeLocked,
> agcStatus->locked);
> >> +               libcameraMetadata_.set(controls::AeState,
> >> +                                      agcStatus->locked ?
> >> +                                      controls::AeStateLocked :
> >> +                                      controls::AeStateSearching);
> >
> >
> > I think, strictly speaking, if agcStatus->locked == true, we should
> return
> > control::AeStateConverged by the definition below.  David, do you agree?
>
> Hmm, indeed. My reading is that what we used to know as AeEnable has
> now been renamed/replaced by AeLock, is that right? And as you say,
> "locked" does not mean "converged" (any more).
>

Yes I agree, but we now have both AeEnable and AeLock defined with this
change.
Perhaps combining them to avoid redundancy makes sense?  Or do folks think
they
can they serve different purposes?


>
> So:
>
> We set controls::AeLocked in the metadata according to the value that
> we received in the corresponding control (which we now receive instead
> of AeEnable), and
>
> We set controls::AeState in the metadata to AeStateConverged when
> agcStatus->locked is true, otherwise to AeStateSearching. We should
> probably bite the bullet and rename that "locked" field to
> "converged", otherwise it's going to cause confusion for the rest of
> eternity.
>

Agree, I think we should rename "locked" to "converged" in the controller
metadata struct.

Regards,
Naush


>
> David
>
> >
> > Thanks,
> > Naush
> >
> >>
> >>                 libcameraMetadata_.set(controls::DigitalGain,
> agcStatus->digital_gain);
> >>         }
> >>
> >> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> >> index b47ea324..88a562a8 100644
> >> --- a/src/ipa/rkisp1/rkisp1.cpp
> >> +++ b/src/ipa/rkisp1/rkisp1.cpp
> >> @@ -51,7 +51,7 @@ private:
> >>                               const rkisp1_stat_buffer *stats);
> >>
> >>         void setControls(unsigned int frame);
> >> -       void metadataReady(unsigned int frame, unsigned int aeState);
> >> +       void metadataReady(unsigned int frame, int aeState);
> >>
> >>         std::map<unsigned int, FrameBuffer> buffers_;
> >>         std::map<unsigned int, void *> buffersMemory_;
> >> @@ -227,7 +227,7 @@ void IPARkISP1::updateStatistics(unsigned int frame,
> >>                                  const rkisp1_stat_buffer *stats)
> >>  {
> >>         const rkisp1_cif_isp_stat *params = &stats->params;
> >> -       unsigned int aeState = 0;
> >> +       int aeState = controls::AeStateInactive;
> >>
> >>         if (stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP) {
> >>                 const rkisp1_cif_isp_ae_stat *ae = &params->ae;
> >> @@ -262,7 +262,9 @@ void IPARkISP1::updateStatistics(unsigned int frame,
> >>                         setControls(frame + 1);
> >>                 }
> >>
> >> -               aeState = fabs(factor - 1.0f) < 0.05f ? 2 : 1;
> >> +               aeState = fabs(factor - 1.0f) < 0.05f ?
> >> +                         controls::AeStateConverged :
> >> +                         controls::AeStateSearching;
> >>         }
> >>
> >>         metadataReady(frame, aeState);
> >> @@ -281,12 +283,11 @@ void IPARkISP1::setControls(unsigned int frame)
> >>         queueFrameAction.emit(frame, op);
> >>  }
> >>
> >> -void IPARkISP1::metadataReady(unsigned int frame, unsigned int aeState)
> >> +void IPARkISP1::metadataReady(unsigned int frame, int aeState)
> >>  {
> >>         ControlList ctrls(controls::controls);
> >>
> >> -       if (aeState)
> >> -               ctrls.set(controls::AeLocked, aeState == 2);
> >> +       ctrls.set(controls::AeState, aeState);
> >>
> >>         RkISP1Action op;
> >>         op.op = ActionMetadata;
> >> diff --git a/src/libcamera/control_ids.yaml
> b/src/libcamera/control_ids.yaml
> >> index 9d4638ae..5717bc1f 100644
> >> --- a/src/libcamera/control_ids.yaml
> >> +++ b/src/libcamera/control_ids.yaml
> >> @@ -14,16 +14,70 @@ controls:
> >>
> >>          \sa ExposureTime AnalogueGain
> >>
> >> -  - AeLocked:
> >> +  - AeLock:
> >>        type: bool
> >>        description: |
> >> -        Report the lock status of a running AE algorithm.
> >> +        Control to lock the AE values.
> >> +        When set to true, the AE algorithm is locked to its latest
> parameters,
> >> +        and will not change exposure settings until set to false.
> >> +        \sa AeState
> >>
> >> -        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.
> >> +  - AeState:
> >> +      type: int32_t
> >> +      description: |
> >> +        Control to report the current AE algorithm state. Enabling or
> disabling
> >> +        AE (AeEnable) always resets the AeState 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.
> >>
> >> -        \sa AeEnable
> >> +        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.
> >> +
> >> +        The state transitions mentioned below assume that AeEnable is
> on.
> >> +
> >> +        \sa AeLock
> >> +      enum:
> >> +        - name: AeStateInactive
> >> +          value: 0
> >> +          description: |
> >> +            The AE algorithm is inactive.
> >> +            If the camera initiates an AE scan, the state shall go to
> Searching.
> >> +            If AeLock is on, the state shall go to Locked.
> >> +        - 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.
> >> +            If AeLock is on, the state shall go to Locked.
> >> +        - name: AeStateConverged
> >> +          value: 2
> >> +          description: |
> >> +            The AE algorithm has converged.
> >> +            If the camera initiates an AE scan, the state shall go to
> Searching.
> >> +            If AeLock is on, the state shall go to Locked.
> >> +        - name: AeStateLocked
> >> +          value: 3
> >> +          description: |
> >> +            The AE algorithm is locked.
> >> +            If AeLock is off, the state can go to Searching,
> Converged, or
> >> +            FlashRequired.
> >> +        - name: AeStateFlashRequired
> >> +          value: 4
> >> +          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: 5
> >> +          description: |
> >> +            The AE algorithm has started a pre-capture metering
> session.
> >> +            After the sequence is finished, the state shall go to
> Converged if
> >> +            AeLock is off, and Locked if it is on.
> >> +            \sa AePrecaptureTrigger
> >>
> >>    # AeMeteringMode needs further attention:
> >>    # - Auto-generate max enum value.
> >> @@ -477,36 +531,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 June 21, 2021, 9:05 a.m. UTC | #4 
Hi Naush,

Thank you for the feedback.

On Fri, Jun 18, 2021 at 02:58:52PM +0100, Naushir Patuck wrote:
> 
> 
> On Fri, 18 Jun 2021 at 14:13, David Plowman <david.plowman@raspberrypi.com>
> wrote:
> 
>     Hi Naush, everyone
> 
>     On Fri, 18 Jun 2021 at 13:07, Naushir Patuck <naush@raspberrypi.com> wrote:
>     >
>     > Hi Paul,
>     >
>     > Thank you for your patch.
>     >
>     > On Fri, 18 Jun 2021 at 11:34, Paul Elder <paul.elder@ideasonboard.com>
>     wrote:
>     >>
>     >> AeLocked alone isn't sufficient for reporting the AE state, so replace
>     >> it with AeState. Add an AeLock control for instructing the camera to
>     >> lock the AE values.
>     >>
>     >> Update the current users of AeLocked accordingly.
>     >>
>     >> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
>     >> ---
>     >>  src/ipa/raspberrypi/raspberrypi.cpp |  5 +-
>     >>  src/ipa/rkisp1/rkisp1.cpp           | 13 ++--
>     >>  src/libcamera/control_ids.yaml      | 96 ++++++++++++++++++-----------
>     >>  3 files changed, 71 insertions(+), 43 deletions(-)
>     >>
>     >> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/
>     raspberrypi.cpp
>     >> index 1c1e802a..ad0132c0 100644
>     >> --- a/src/ipa/raspberrypi/raspberrypi.cpp
>     >> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
>     >> @@ -468,7 +468,10 @@ void IPARPi::reportMetadata()
>     >>
>     >>         AgcStatus *agcStatus = rpiMetadata_.GetLocked<AgcStatus>
>     ("agc.status");
>     >>         if (agcStatus) {
>     >> -               libcameraMetadata_.set(controls::AeLocked, agcStatus->
>     locked);
>     >> +               libcameraMetadata_.set(controls::AeState,
>     >> +                                      agcStatus->locked ?
>     >> +                                      controls::AeStateLocked :
>     >> +                                      controls::AeStateSearching);
>     >
>     >
>     > I think, strictly speaking, if agcStatus->locked == true, we should
>     return
>     > control::AeStateConverged by the definition below.  David, do you agree?

Oh shoot, that is indeed what I meant. I got mixed up with the status
name :/

> 
>     Hmm, indeed. My reading is that what we used to know as AeEnable has
>     now been renamed/replaced by AeLock, is that right? And as you say,
>     "locked" does not mean "converged" (any more).
> 
> 
> Yes I agree, but we now have both AeEnable and AeLock defined with this change.

AeEnable is for enabling/disabling the AE routine. AeLock is to lock the
AE values... which would normally be the same as turning off AE, but you
want to be able to unlock AE without having to restart the routine.
That's the idea afaict.

Supporting AeLock is optional, of course. Same for AeState, not all of
the states need to be supported. You can specify which ones are
supported in pipeline's ControlInfoMap.


Thanks,

Paul

> Perhaps combining them to avoid redundancy makes sense?  Or do folks think they
> can they serve different purposes?
>  
> 
> 
>     So:
> 
>     We set controls::AeLocked in the metadata according to the value that
>     we received in the corresponding control (which we now receive instead
>     of AeEnable), and
> 
>     We set controls::AeState in the metadata to AeStateConverged when
>     agcStatus->locked is true, otherwise to AeStateSearching. We should
>     probably bite the bullet and rename that "locked" field to
>     "converged", otherwise it's going to cause confusion for the rest of
>     eternity.
> 
> 
> Agree, I think we should rename "locked" to "converged" in the controller
> metadata struct.
> 
> Regards,
> Naush
>  
> 
> 
>     David
> 
>     >
>     > Thanks,
>     > Naush
>     >
>     >>
>     >>                 libcameraMetadata_.set(controls::DigitalGain,
>     agcStatus->digital_gain);
>     >>         }
>     >>
>     >> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
>     >> index b47ea324..88a562a8 100644
>     >> --- a/src/ipa/rkisp1/rkisp1.cpp
>     >> +++ b/src/ipa/rkisp1/rkisp1.cpp
>     >> @@ -51,7 +51,7 @@ private:
>     >>                               const rkisp1_stat_buffer *stats);
>     >>
>     >>         void setControls(unsigned int frame);
>     >> -       void metadataReady(unsigned int frame, unsigned int aeState);
>     >> +       void metadataReady(unsigned int frame, int aeState);
>     >>
>     >>         std::map<unsigned int, FrameBuffer> buffers_;
>     >>         std::map<unsigned int, void *> buffersMemory_;
>     >> @@ -227,7 +227,7 @@ void IPARkISP1::updateStatistics(unsigned int frame,
>     >>                                  const rkisp1_stat_buffer *stats)
>     >>  {
>     >>         const rkisp1_cif_isp_stat *params = &stats->params;
>     >> -       unsigned int aeState = 0;
>     >> +       int aeState = controls::AeStateInactive;
>     >>
>     >>         if (stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP) {
>     >>                 const rkisp1_cif_isp_ae_stat *ae = &params->ae;
>     >> @@ -262,7 +262,9 @@ void IPARkISP1::updateStatistics(unsigned int frame,
>     >>                         setControls(frame + 1);
>     >>                 }
>     >>
>     >> -               aeState = fabs(factor - 1.0f) < 0.05f ? 2 : 1;
>     >> +               aeState = fabs(factor - 1.0f) < 0.05f ?
>     >> +                         controls::AeStateConverged :
>     >> +                         controls::AeStateSearching;
>     >>         }
>     >>
>     >>         metadataReady(frame, aeState);
>     >> @@ -281,12 +283,11 @@ void IPARkISP1::setControls(unsigned int frame)
>     >>         queueFrameAction.emit(frame, op);
>     >>  }
>     >>
>     >> -void IPARkISP1::metadataReady(unsigned int frame, unsigned int aeState)
>     >> +void IPARkISP1::metadataReady(unsigned int frame, int aeState)
>     >>  {
>     >>         ControlList ctrls(controls::controls);
>     >>
>     >> -       if (aeState)
>     >> -               ctrls.set(controls::AeLocked, aeState == 2);
>     >> +       ctrls.set(controls::AeState, aeState);
>     >>
>     >>         RkISP1Action op;
>     >>         op.op = ActionMetadata;
>     >> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/
>     control_ids.yaml
>     >> index 9d4638ae..5717bc1f 100644
>     >> --- a/src/libcamera/control_ids.yaml
>     >> +++ b/src/libcamera/control_ids.yaml
>     >> @@ -14,16 +14,70 @@ controls:
>     >>
>     >>          \sa ExposureTime AnalogueGain
>     >>
>     >> -  - AeLocked:
>     >> +  - AeLock:
>     >>        type: bool
>     >>        description: |
>     >> -        Report the lock status of a running AE algorithm.
>     >> +        Control to lock the AE values.
>     >> +        When set to true, the AE algorithm is locked to its latest
>     parameters,
>     >> +        and will not change exposure settings until set to false.
>     >> +        \sa AeState
>     >>
>     >> -        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.
>     >> +  - AeState:
>     >> +      type: int32_t
>     >> +      description: |
>     >> +        Control to report the current AE algorithm state. Enabling or
>     disabling
>     >> +        AE (AeEnable) always resets the AeState 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.
>     >>
>     >> -        \sa AeEnable
>     >> +        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.
>     >> +
>     >> +        The state transitions mentioned below assume that AeEnable is
>     on.
>     >> +
>     >> +        \sa AeLock
>     >> +      enum:
>     >> +        - name: AeStateInactive
>     >> +          value: 0
>     >> +          description: |
>     >> +            The AE algorithm is inactive.
>     >> +            If the camera initiates an AE scan, the state shall go to
>     Searching.
>     >> +            If AeLock is on, the state shall go to Locked.
>     >> +        - 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.
>     >> +            If AeLock is on, the state shall go to Locked.
>     >> +        - name: AeStateConverged
>     >> +          value: 2
>     >> +          description: |
>     >> +            The AE algorithm has converged.
>     >> +            If the camera initiates an AE scan, the state shall go to
>     Searching.
>     >> +            If AeLock is on, the state shall go to Locked.
>     >> +        - name: AeStateLocked
>     >> +          value: 3
>     >> +          description: |
>     >> +            The AE algorithm is locked.
>     >> +            If AeLock is off, the state can go to Searching, Converged,
>     or
>     >> +            FlashRequired.
>     >> +        - name: AeStateFlashRequired
>     >> +          value: 4
>     >> +          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: 5
>     >> +          description: |
>     >> +            The AE algorithm has started a pre-capture metering
>     session.
>     >> +            After the sequence is finished, the state shall go to
>     Converged if
>     >> +            AeLock is off, and Locked if it is on.
>     >> +            \sa AePrecaptureTrigger
>     >>
>     >>    # AeMeteringMode needs further attention:
>     >>    # - Auto-generate max enum value.
>     >> @@ -477,36 +531,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 June 21, 2021, 9:15 a.m. UTC | #5 
Hi Paul

Thanks for the extra explanations.

On Mon, 21 Jun 2021 at 10:05, <paul.elder@ideasonboard.com> wrote:
>
> Hi Naush,
>
> Thank you for the feedback.
>
> On Fri, Jun 18, 2021 at 02:58:52PM +0100, Naushir Patuck wrote:
> >
> >
> > On Fri, 18 Jun 2021 at 14:13, David Plowman <david.plowman@raspberrypi.com>
> > wrote:
> >
> >     Hi Naush, everyone
> >
> >     On Fri, 18 Jun 2021 at 13:07, Naushir Patuck <naush@raspberrypi.com> wrote:
> >     >
> >     > Hi Paul,
> >     >
> >     > Thank you for your patch.
> >     >
> >     > On Fri, 18 Jun 2021 at 11:34, Paul Elder <paul.elder@ideasonboard.com>
> >     wrote:
> >     >>
> >     >> AeLocked alone isn't sufficient for reporting the AE state, so replace
> >     >> it with AeState. Add an AeLock control for instructing the camera to
> >     >> lock the AE values.
> >     >>
> >     >> Update the current users of AeLocked accordingly.
> >     >>
> >     >> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> >     >> ---
> >     >>  src/ipa/raspberrypi/raspberrypi.cpp |  5 +-
> >     >>  src/ipa/rkisp1/rkisp1.cpp           | 13 ++--
> >     >>  src/libcamera/control_ids.yaml      | 96 ++++++++++++++++++-----------
> >     >>  3 files changed, 71 insertions(+), 43 deletions(-)
> >     >>
> >     >> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/
> >     raspberrypi.cpp
> >     >> index 1c1e802a..ad0132c0 100644
> >     >> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> >     >> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> >     >> @@ -468,7 +468,10 @@ void IPARPi::reportMetadata()
> >     >>
> >     >>         AgcStatus *agcStatus = rpiMetadata_.GetLocked<AgcStatus>
> >     ("agc.status");
> >     >>         if (agcStatus) {
> >     >> -               libcameraMetadata_.set(controls::AeLocked, agcStatus->
> >     locked);
> >     >> +               libcameraMetadata_.set(controls::AeState,
> >     >> +                                      agcStatus->locked ?
> >     >> +                                      controls::AeStateLocked :
> >     >> +                                      controls::AeStateSearching);
> >     >
> >     >
> >     > I think, strictly speaking, if agcStatus->locked == true, we should
> >     return
> >     > control::AeStateConverged by the definition below.  David, do you agree?
>
> Oh shoot, that is indeed what I meant. I got mixed up with the status
> name :/
>
> >
> >     Hmm, indeed. My reading is that what we used to know as AeEnable has
> >     now been renamed/replaced by AeLock, is that right? And as you say,
> >     "locked" does not mean "converged" (any more).
> >
> >
> > Yes I agree, but we now have both AeEnable and AeLock defined with this change.
>
> AeEnable is for enabling/disabling the AE routine. AeLock is to lock the
> AE values... which would normally be the same as turning off AE, but you
> want to be able to unlock AE without having to restart the routine.

I'm still struggling a bit with this, however. What would it mean to
unlock the AE without restarting it? Isn't it then still fixed at
whatever values it had? Perhaps you could provide an example of this
behaviour which would show how it is different.

Sorry for still being puzzled...

Thanks!
David

> That's the idea afaict.
>
> Supporting AeLock is optional, of course. Same for AeState, not all of
> the states need to be supported. You can specify which ones are
> supported in pipeline's ControlInfoMap.
>
>
> Thanks,
>
> Paul
>
> > Perhaps combining them to avoid redundancy makes sense?  Or do folks think they
> > can they serve different purposes?
> >
> >
> >
> >     So:
> >
> >     We set controls::AeLocked in the metadata according to the value that
> >     we received in the corresponding control (which we now receive instead
> >     of AeEnable), and
> >
> >     We set controls::AeState in the metadata to AeStateConverged when
> >     agcStatus->locked is true, otherwise to AeStateSearching. We should
> >     probably bite the bullet and rename that "locked" field to
> >     "converged", otherwise it's going to cause confusion for the rest of
> >     eternity.
> >
> >
> > Agree, I think we should rename "locked" to "converged" in the controller
> > metadata struct.
> >
> > Regards,
> > Naush
> >
> >
> >
> >     David
> >
> >     >
> >     > Thanks,
> >     > Naush
> >     >
> >     >>
> >     >>                 libcameraMetadata_.set(controls::DigitalGain,
> >     agcStatus->digital_gain);
> >     >>         }
> >     >>
> >     >> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> >     >> index b47ea324..88a562a8 100644
> >     >> --- a/src/ipa/rkisp1/rkisp1.cpp
> >     >> +++ b/src/ipa/rkisp1/rkisp1.cpp
> >     >> @@ -51,7 +51,7 @@ private:
> >     >>                               const rkisp1_stat_buffer *stats);
> >     >>
> >     >>         void setControls(unsigned int frame);
> >     >> -       void metadataReady(unsigned int frame, unsigned int aeState);
> >     >> +       void metadataReady(unsigned int frame, int aeState);
> >     >>
> >     >>         std::map<unsigned int, FrameBuffer> buffers_;
> >     >>         std::map<unsigned int, void *> buffersMemory_;
> >     >> @@ -227,7 +227,7 @@ void IPARkISP1::updateStatistics(unsigned int frame,
> >     >>                                  const rkisp1_stat_buffer *stats)
> >     >>  {
> >     >>         const rkisp1_cif_isp_stat *params = &stats->params;
> >     >> -       unsigned int aeState = 0;
> >     >> +       int aeState = controls::AeStateInactive;
> >     >>
> >     >>         if (stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP) {
> >     >>                 const rkisp1_cif_isp_ae_stat *ae = &params->ae;
> >     >> @@ -262,7 +262,9 @@ void IPARkISP1::updateStatistics(unsigned int frame,
> >     >>                         setControls(frame + 1);
> >     >>                 }
> >     >>
> >     >> -               aeState = fabs(factor - 1.0f) < 0.05f ? 2 : 1;
> >     >> +               aeState = fabs(factor - 1.0f) < 0.05f ?
> >     >> +                         controls::AeStateConverged :
> >     >> +                         controls::AeStateSearching;
> >     >>         }
> >     >>
> >     >>         metadataReady(frame, aeState);
> >     >> @@ -281,12 +283,11 @@ void IPARkISP1::setControls(unsigned int frame)
> >     >>         queueFrameAction.emit(frame, op);
> >     >>  }
> >     >>
> >     >> -void IPARkISP1::metadataReady(unsigned int frame, unsigned int aeState)
> >     >> +void IPARkISP1::metadataReady(unsigned int frame, int aeState)
> >     >>  {
> >     >>         ControlList ctrls(controls::controls);
> >     >>
> >     >> -       if (aeState)
> >     >> -               ctrls.set(controls::AeLocked, aeState == 2);
> >     >> +       ctrls.set(controls::AeState, aeState);
> >     >>
> >     >>         RkISP1Action op;
> >     >>         op.op = ActionMetadata;
> >     >> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/
> >     control_ids.yaml
> >     >> index 9d4638ae..5717bc1f 100644
> >     >> --- a/src/libcamera/control_ids.yaml
> >     >> +++ b/src/libcamera/control_ids.yaml
> >     >> @@ -14,16 +14,70 @@ controls:
> >     >>
> >     >>          \sa ExposureTime AnalogueGain
> >     >>
> >     >> -  - AeLocked:
> >     >> +  - AeLock:
> >     >>        type: bool
> >     >>        description: |
> >     >> -        Report the lock status of a running AE algorithm.
> >     >> +        Control to lock the AE values.
> >     >> +        When set to true, the AE algorithm is locked to its latest
> >     parameters,
> >     >> +        and will not change exposure settings until set to false.
> >     >> +        \sa AeState
> >     >>
> >     >> -        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.
> >     >> +  - AeState:
> >     >> +      type: int32_t
> >     >> +      description: |
> >     >> +        Control to report the current AE algorithm state. Enabling or
> >     disabling
> >     >> +        AE (AeEnable) always resets the AeState 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.
> >     >>
> >     >> -        \sa AeEnable
> >     >> +        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.
> >     >> +
> >     >> +        The state transitions mentioned below assume that AeEnable is
> >     on.
> >     >> +
> >     >> +        \sa AeLock
> >     >> +      enum:
> >     >> +        - name: AeStateInactive
> >     >> +          value: 0
> >     >> +          description: |
> >     >> +            The AE algorithm is inactive.
> >     >> +            If the camera initiates an AE scan, the state shall go to
> >     Searching.
> >     >> +            If AeLock is on, the state shall go to Locked.
> >     >> +        - 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.
> >     >> +            If AeLock is on, the state shall go to Locked.
> >     >> +        - name: AeStateConverged
> >     >> +          value: 2
> >     >> +          description: |
> >     >> +            The AE algorithm has converged.
> >     >> +            If the camera initiates an AE scan, the state shall go to
> >     Searching.
> >     >> +            If AeLock is on, the state shall go to Locked.
> >     >> +        - name: AeStateLocked
> >     >> +          value: 3
> >     >> +          description: |
> >     >> +            The AE algorithm is locked.
> >     >> +            If AeLock is off, the state can go to Searching, Converged,
> >     or
> >     >> +            FlashRequired.
> >     >> +        - name: AeStateFlashRequired
> >     >> +          value: 4
> >     >> +          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: 5
> >     >> +          description: |
> >     >> +            The AE algorithm has started a pre-capture metering
> >     session.
> >     >> +            After the sequence is finished, the state shall go to
> >     Converged if
> >     >> +            AeLock is off, and Locked if it is on.
> >     >> +            \sa AePrecaptureTrigger
> >     >>
> >     >>    # AeMeteringMode needs further attention:
> >     >>    # - Auto-generate max enum value.
> >     >> @@ -477,36 +531,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 June 22, 2021, 4:12 p.m. UTC | #6 
Hi David,

On Mon, Jun 21, 2021 at 10:15:21AM +0100, David Plowman wrote:
> Hi Paul
>
> Thanks for the extra explanations.
>
> On Mon, 21 Jun 2021 at 10:05, <paul.elder@ideasonboard.com> wrote:
> >
> > Hi Naush,
> >
> > Thank you for the feedback.
> >
> > On Fri, Jun 18, 2021 at 02:58:52PM +0100, Naushir Patuck wrote:
> > >
> > >
> > > On Fri, 18 Jun 2021 at 14:13, David Plowman <david.plowman@raspberrypi.com>
> > > wrote:
> > >
> > >     Hi Naush, everyone
> > >
> > >     On Fri, 18 Jun 2021 at 13:07, Naushir Patuck <naush@raspberrypi.com> wrote:
> > >     >
> > >     > Hi Paul,
> > >     >
> > >     > Thank you for your patch.
> > >     >
> > >     > On Fri, 18 Jun 2021 at 11:34, Paul Elder <paul.elder@ideasonboard.com>
> > >     wrote:
> > >     >>
> > >     >> AeLocked alone isn't sufficient for reporting the AE state, so replace
> > >     >> it with AeState. Add an AeLock control for instructing the camera to
> > >     >> lock the AE values.
> > >     >>
> > >     >> Update the current users of AeLocked accordingly.
> > >     >>
> > >     >> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > >     >> ---
> > >     >>  src/ipa/raspberrypi/raspberrypi.cpp |  5 +-
> > >     >>  src/ipa/rkisp1/rkisp1.cpp           | 13 ++--
> > >     >>  src/libcamera/control_ids.yaml      | 96 ++++++++++++++++++-----------
> > >     >>  3 files changed, 71 insertions(+), 43 deletions(-)
> > >     >>
> > >     >> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/
> > >     raspberrypi.cpp
> > >     >> index 1c1e802a..ad0132c0 100644
> > >     >> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > >     >> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > >     >> @@ -468,7 +468,10 @@ void IPARPi::reportMetadata()
> > >     >>
> > >     >>         AgcStatus *agcStatus = rpiMetadata_.GetLocked<AgcStatus>
> > >     ("agc.status");
> > >     >>         if (agcStatus) {
> > >     >> -               libcameraMetadata_.set(controls::AeLocked, agcStatus->
> > >     locked);
> > >     >> +               libcameraMetadata_.set(controls::AeState,
> > >     >> +                                      agcStatus->locked ?
> > >     >> +                                      controls::AeStateLocked :
> > >     >> +                                      controls::AeStateSearching);
> > >     >
> > >     >
> > >     > I think, strictly speaking, if agcStatus->locked == true, we should
> > >     return
> > >     > control::AeStateConverged by the definition below.  David, do you agree?
> >
> > Oh shoot, that is indeed what I meant. I got mixed up with the status
> > name :/
> >
> > >
> > >     Hmm, indeed. My reading is that what we used to know as AeEnable has
> > >     now been renamed/replaced by AeLock, is that right? And as you say,
> > >     "locked" does not mean "converged" (any more).
> > >
> > >
> > > Yes I agree, but we now have both AeEnable and AeLock defined with this change.
> >
> > AeEnable is for enabling/disabling the AE routine. AeLock is to lock the
> > AE values... which would normally be the same as turning off AE, but you
> > want to be able to unlock AE without having to restart the routine.
>
> I'm still struggling a bit with this, however. What would it mean to
> unlock the AE without restarting it? Isn't it then still fixed at
> whatever values it had? Perhaps you could provide an example of this
> behaviour which would show how it is different.
>
> Sorry for still being puzzled...

And I'm sorry as well for asking possibly stupid questions, but:

- It seems to me the RPi AGC routine has two Pause/Resume functions that:
 - Pause: sets fixed gain and shutter time to the last computed
   shutter and gain values
 - Resume: re-sets them to 0

 - In the RPi AEGC implementation, if fixed shutter and gains are set
   then those values are used

 - The AEGC routine is paused/resumed on controls::AeEnable on/off

 - When AeEnable == Off, then the only way to override the fixed
   shutter/gain is by using the ExposureTime and AnalogueGain controls
   (ie manual shutter/gain control)

So in you implementation disabling AE has the effect that the AEGC
routine uses the last computed values until a new shutter/gain value
is set by the application (through what is usually called "manual" mode).

The difference between your implementation and a new one that uses
AeLocked and AeEnable is, in my understanding, that when the AeLocked
is set the AEGC routine is still in control of the shutter/gain which
gets "fixed" as you're doing, but it does not accept manual control of
the shutter/gain like you're doing unless the AEGC is disabled.

This seems to be suggested by the following paragraph in the Android
documentation:

-------------------------------------------------------------------------------

If an application is switching between automatic and manual control
and wishes to eliminate any flicker during the switch, the following
procedure is recommended:

Starting in auto-AE mode:
- Lock AE
- Wait for the first result to be output that has the AE locked
- Copy exposure settings from that result into a request, set the request to manual AE
- Submit the capture request, proceed to run manual AE as desired.

See android.control.aeState for AE lock related state transition details.
-------------------------------------------------------------------------------

If we were able to report the AEGC convergence state I think the same
could be done without AeLock, by disabling AEGC once it has converged
and continue from there with manual exposure. But my understanding is that
having a lock controls guarantees that once the AEGC has
converged it won't change, so that the in-flight requests that are
completed before the first one with manual AE control is processed
won't have floating AEGC values,

(Let me try to draw it down, hoping it render ok with your client,
otherwise https://paste.debian.net/1202043/)

Queue of
submitted               Completed               AeState in the
Requests                Requests                completed Request
-----------------------------------------------------------------

0 (AeLock)
1
2
3                       0                       Searching
4                       1                       Converged
5 (AeDisable +          2                       Locked  <- COPY AE VALUES
   Manual AE with       3                       Locked
   settings from #2)    4                       Locked
                        5                       Use Manual AE

Without a Lock the AEGC routine would keep adjusting its values for
requests 3 and 4. Request 5 will contain settings copied from Request
2 so a small hiccup might happen. Of course, if the number of frames
required to get to a converged state is longer, the duration of the
possible AE adjustment period is longer, making the flickering more
visible ?

Does this make sense ?

The android.control.aeMode documentation also reports that:

-----------------------------------------------------------------
Note that auto-white balance (AWB) and auto-focus (AF) behavior is
device dependent when AE is in OFF mode. To have consistent behavior
across different devices, it is recommended to either set AWB and AF to
OFF mode or lock AWB and AF before setting AE to OFF.See
android.control.awbMode,
android.control.afMode,android.control.awbLock, and
android.control.afTrigger for more details.
-----------------------------------------------------------------

Suggesting that the AF AWB and AE locks might play a role on the interactions
between the three algorithms.

Does this make sense and provide a bit more of context about what
AeLock could be used for ?

Thanks
   j


>
> Thanks!
> David
>
> > That's the idea afaict.
> >
> > Supporting AeLock is optional, of course. Same for AeState, not all of
> > the states need to be supported. You can specify which ones are
> > supported in pipeline's ControlInfoMap.
> >
> >
> > Thanks,
> >
> > Paul
> >
> > > Perhaps combining them to avoid redundancy makes sense?  Or do folks think they
> > > can they serve different purposes?
> > >
> > >
> > >
> > >     So:
> > >
> > >     We set controls::AeLocked in the metadata according to the value that
> > >     we received in the corresponding control (which we now receive instead
> > >     of AeEnable), and
> > >
> > >     We set controls::AeState in the metadata to AeStateConverged when
> > >     agcStatus->locked is true, otherwise to AeStateSearching. We should
> > >     probably bite the bullet and rename that "locked" field to
> > >     "converged", otherwise it's going to cause confusion for the rest of
> > >     eternity.
> > >
> > >
> > > Agree, I think we should rename "locked" to "converged" in the controller
> > > metadata struct.
> > >
> > > Regards,
> > > Naush
> > >
> > >
> > >
> > >     David
> > >
> > >     >
> > >     > Thanks,
> > >     > Naush
> > >     >
> > >     >>
> > >     >>                 libcameraMetadata_.set(controls::DigitalGain,
> > >     agcStatus->digital_gain);
> > >     >>         }
> > >     >>
> > >     >> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > >     >> index b47ea324..88a562a8 100644
> > >     >> --- a/src/ipa/rkisp1/rkisp1.cpp
> > >     >> +++ b/src/ipa/rkisp1/rkisp1.cpp
> > >     >> @@ -51,7 +51,7 @@ private:
> > >     >>                               const rkisp1_stat_buffer *stats);
> > >     >>
> > >     >>         void setControls(unsigned int frame);
> > >     >> -       void metadataReady(unsigned int frame, unsigned int aeState);
> > >     >> +       void metadataReady(unsigned int frame, int aeState);
> > >     >>
> > >     >>         std::map<unsigned int, FrameBuffer> buffers_;
> > >     >>         std::map<unsigned int, void *> buffersMemory_;
> > >     >> @@ -227,7 +227,7 @@ void IPARkISP1::updateStatistics(unsigned int frame,
> > >     >>                                  const rkisp1_stat_buffer *stats)
> > >     >>  {
> > >     >>         const rkisp1_cif_isp_stat *params = &stats->params;
> > >     >> -       unsigned int aeState = 0;
> > >     >> +       int aeState = controls::AeStateInactive;
> > >     >>
> > >     >>         if (stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP) {
> > >     >>                 const rkisp1_cif_isp_ae_stat *ae = &params->ae;
> > >     >> @@ -262,7 +262,9 @@ void IPARkISP1::updateStatistics(unsigned int frame,
> > >     >>                         setControls(frame + 1);
> > >     >>                 }
> > >     >>
> > >     >> -               aeState = fabs(factor - 1.0f) < 0.05f ? 2 : 1;
> > >     >> +               aeState = fabs(factor - 1.0f) < 0.05f ?
> > >     >> +                         controls::AeStateConverged :
> > >     >> +                         controls::AeStateSearching;
> > >     >>         }
> > >     >>
> > >     >>         metadataReady(frame, aeState);
> > >     >> @@ -281,12 +283,11 @@ void IPARkISP1::setControls(unsigned int frame)
> > >     >>         queueFrameAction.emit(frame, op);
> > >     >>  }
> > >     >>
> > >     >> -void IPARkISP1::metadataReady(unsigned int frame, unsigned int aeState)
> > >     >> +void IPARkISP1::metadataReady(unsigned int frame, int aeState)
> > >     >>  {
> > >     >>         ControlList ctrls(controls::controls);
> > >     >>
> > >     >> -       if (aeState)
> > >     >> -               ctrls.set(controls::AeLocked, aeState == 2);
> > >     >> +       ctrls.set(controls::AeState, aeState);
> > >     >>
> > >     >>         RkISP1Action op;
> > >     >>         op.op = ActionMetadata;
> > >     >> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/
> > >     control_ids.yaml
> > >     >> index 9d4638ae..5717bc1f 100644
> > >     >> --- a/src/libcamera/control_ids.yaml
> > >     >> +++ b/src/libcamera/control_ids.yaml
> > >     >> @@ -14,16 +14,70 @@ controls:
> > >     >>
> > >     >>          \sa ExposureTime AnalogueGain
> > >     >>
> > >     >> -  - AeLocked:
> > >     >> +  - AeLock:
> > >     >>        type: bool
> > >     >>        description: |
> > >     >> -        Report the lock status of a running AE algorithm.
> > >     >> +        Control to lock the AE values.
> > >     >> +        When set to true, the AE algorithm is locked to its latest
> > >     parameters,
> > >     >> +        and will not change exposure settings until set to false.
> > >     >> +        \sa AeState
> > >     >>
> > >     >> -        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.
> > >     >> +  - AeState:
> > >     >> +      type: int32_t
> > >     >> +      description: |
> > >     >> +        Control to report the current AE algorithm state. Enabling or
> > >     disabling
> > >     >> +        AE (AeEnable) always resets the AeState 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.
> > >     >>
> > >     >> -        \sa AeEnable
> > >     >> +        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.
> > >     >> +
> > >     >> +        The state transitions mentioned below assume that AeEnable is
> > >     on.
> > >     >> +
> > >     >> +        \sa AeLock
> > >     >> +      enum:
> > >     >> +        - name: AeStateInactive
> > >     >> +          value: 0
> > >     >> +          description: |
> > >     >> +            The AE algorithm is inactive.
> > >     >> +            If the camera initiates an AE scan, the state shall go to
> > >     Searching.
> > >     >> +            If AeLock is on, the state shall go to Locked.
> > >     >> +        - 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.
> > >     >> +            If AeLock is on, the state shall go to Locked.
> > >     >> +        - name: AeStateConverged
> > >     >> +          value: 2
> > >     >> +          description: |
> > >     >> +            The AE algorithm has converged.
> > >     >> +            If the camera initiates an AE scan, the state shall go to
> > >     Searching.
> > >     >> +            If AeLock is on, the state shall go to Locked.
> > >     >> +        - name: AeStateLocked
> > >     >> +          value: 3
> > >     >> +          description: |
> > >     >> +            The AE algorithm is locked.
> > >     >> +            If AeLock is off, the state can go to Searching, Converged,
> > >     or
> > >     >> +            FlashRequired.
> > >     >> +        - name: AeStateFlashRequired
> > >     >> +          value: 4
> > >     >> +          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: 5
> > >     >> +          description: |
> > >     >> +            The AE algorithm has started a pre-capture metering
> > >     session.
> > >     >> +            After the sequence is finished, the state shall go to
> > >     Converged if
> > >     >> +            AeLock is off, and Locked if it is on.
> > >     >> +            \sa AePrecaptureTrigger
> > >     >>
> > >     >>    # AeMeteringMode needs further attention:
> > >     >>    # - Auto-generate max enum value.
> > >     >> @@ -477,36 +531,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 June 23, 2021, 7:57 a.m. UTC | #7 
Hi Jacopo

Thanks for taking more time to explain this!

On Tue, 22 Jun 2021 at 17:11, Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hi David,
>
> On Mon, Jun 21, 2021 at 10:15:21AM +0100, David Plowman wrote:
> > Hi Paul
> >
> > Thanks for the extra explanations.
> >
> > On Mon, 21 Jun 2021 at 10:05, <paul.elder@ideasonboard.com> wrote:
> > >
> > > Hi Naush,
> > >
> > > Thank you for the feedback.
> > >
> > > On Fri, Jun 18, 2021 at 02:58:52PM +0100, Naushir Patuck wrote:
> > > >
> > > >
> > > > On Fri, 18 Jun 2021 at 14:13, David Plowman <david.plowman@raspberrypi.com>
> > > > wrote:
> > > >
> > > >     Hi Naush, everyone
> > > >
> > > >     On Fri, 18 Jun 2021 at 13:07, Naushir Patuck <naush@raspberrypi.com> wrote:
> > > >     >
> > > >     > Hi Paul,
> > > >     >
> > > >     > Thank you for your patch.
> > > >     >
> > > >     > On Fri, 18 Jun 2021 at 11:34, Paul Elder <paul.elder@ideasonboard.com>
> > > >     wrote:
> > > >     >>
> > > >     >> AeLocked alone isn't sufficient for reporting the AE state, so replace
> > > >     >> it with AeState. Add an AeLock control for instructing the camera to
> > > >     >> lock the AE values.
> > > >     >>
> > > >     >> Update the current users of AeLocked accordingly.
> > > >     >>
> > > >     >> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > >     >> ---
> > > >     >>  src/ipa/raspberrypi/raspberrypi.cpp |  5 +-
> > > >     >>  src/ipa/rkisp1/rkisp1.cpp           | 13 ++--
> > > >     >>  src/libcamera/control_ids.yaml      | 96 ++++++++++++++++++-----------
> > > >     >>  3 files changed, 71 insertions(+), 43 deletions(-)
> > > >     >>
> > > >     >> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/
> > > >     raspberrypi.cpp
> > > >     >> index 1c1e802a..ad0132c0 100644
> > > >     >> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > > >     >> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > > >     >> @@ -468,7 +468,10 @@ void IPARPi::reportMetadata()
> > > >     >>
> > > >     >>         AgcStatus *agcStatus = rpiMetadata_.GetLocked<AgcStatus>
> > > >     ("agc.status");
> > > >     >>         if (agcStatus) {
> > > >     >> -               libcameraMetadata_.set(controls::AeLocked, agcStatus->
> > > >     locked);
> > > >     >> +               libcameraMetadata_.set(controls::AeState,
> > > >     >> +                                      agcStatus->locked ?
> > > >     >> +                                      controls::AeStateLocked :
> > > >     >> +                                      controls::AeStateSearching);
> > > >     >
> > > >     >
> > > >     > I think, strictly speaking, if agcStatus->locked == true, we should
> > > >     return
> > > >     > control::AeStateConverged by the definition below.  David, do you agree?
> > >
> > > Oh shoot, that is indeed what I meant. I got mixed up with the status
> > > name :/
> > >
> > > >
> > > >     Hmm, indeed. My reading is that what we used to know as AeEnable has
> > > >     now been renamed/replaced by AeLock, is that right? And as you say,
> > > >     "locked" does not mean "converged" (any more).
> > > >
> > > >
> > > > Yes I agree, but we now have both AeEnable and AeLock defined with this change.
> > >
> > > AeEnable is for enabling/disabling the AE routine. AeLock is to lock the
> > > AE values... which would normally be the same as turning off AE, but you
> > > want to be able to unlock AE without having to restart the routine.
> >
> > I'm still struggling a bit with this, however. What would it mean to
> > unlock the AE without restarting it? Isn't it then still fixed at
> > whatever values it had? Perhaps you could provide an example of this
> > behaviour which would show how it is different.
> >
> > Sorry for still being puzzled...
>
> And I'm sorry as well for asking possibly stupid questions, but:
>
> - It seems to me the RPi AGC routine has two Pause/Resume functions that:
>  - Pause: sets fixed gain and shutter time to the last computed
>    shutter and gain values
>  - Resume: re-sets them to 0
>
>  - In the RPi AEGC implementation, if fixed shutter and gains are set
>    then those values are used
>
>  - The AEGC routine is paused/resumed on controls::AeEnable on/off
>
>  - When AeEnable == Off, then the only way to override the fixed
>    shutter/gain is by using the ExposureTime and AnalogueGain controls
>    (ie manual shutter/gain control)
>
> So in you implementation disabling AE has the effect that the AEGC
> routine uses the last computed values until a new shutter/gain value
> is set by the application (through what is usually called "manual" mode).
>
> The difference between your implementation and a new one that uses
> AeLocked and AeEnable is, in my understanding, that when the AeLocked
> is set the AEGC routine is still in control of the shutter/gain which
> gets "fixed" as you're doing, but it does not accept manual control of
> the shutter/gain like you're doing unless the AEGC is disabled.
>
> This seems to be suggested by the following paragraph in the Android
> documentation:
>
> -------------------------------------------------------------------------------
>
> If an application is switching between automatic and manual control
> and wishes to eliminate any flicker during the switch, the following
> procedure is recommended:
>
> Starting in auto-AE mode:
> - Lock AE
> - Wait for the first result to be output that has the AE locked
> - Copy exposure settings from that result into a request, set the request to manual AE
> - Submit the capture request, proceed to run manual AE as desired.
>
> See android.control.aeState for AE lock related state transition details.
> -------------------------------------------------------------------------------
>
> If we were able to report the AEGC convergence state I think the same
> could be done without AeLock, by disabling AEGC once it has converged
> and continue from there with manual exposure. But my understanding is that
> having a lock controls guarantees that once the AEGC has
> converged it won't change, so that the in-flight requests that are
> completed before the first one with manual AE control is processed
> won't have floating AEGC values,
>
> (Let me try to draw it down, hoping it render ok with your client,
> otherwise https://paste.debian.net/1202043/)
>
> Queue of
> submitted               Completed               AeState in the
> Requests                Requests                completed Request
> -----------------------------------------------------------------
>
> 0 (AeLock)
> 1
> 2
> 3                       0                       Searching
> 4                       1                       Converged
> 5 (AeDisable +          2                       Locked  <- COPY AE VALUES
>    Manual AE with       3                       Locked
>    settings from #2)    4                       Locked
>                         5                       Use Manual AE

Are we saying that AeDisable means "stop adjusting the AEC/AGC
immediately, whether it's converged or not", but AeLock means "wait
until it's converged and then immediately stop adjusting the AEC/AGC"?

Thanks!
David

>
> Without a Lock the AEGC routine would keep adjusting its values for
> requests 3 and 4. Request 5 will contain settings copied from Request
> 2 so a small hiccup might happen. Of course, if the number of frames
> required to get to a converged state is longer, the duration of the
> possible AE adjustment period is longer, making the flickering more
> visible ?
>
> Does this make sense ?
>
> The android.control.aeMode documentation also reports that:
>
> -----------------------------------------------------------------
> Note that auto-white balance (AWB) and auto-focus (AF) behavior is
> device dependent when AE is in OFF mode. To have consistent behavior
> across different devices, it is recommended to either set AWB and AF to
> OFF mode or lock AWB and AF before setting AE to OFF.See
> android.control.awbMode,
> android.control.afMode,android.control.awbLock, and
> android.control.afTrigger for more details.
> -----------------------------------------------------------------
>
> Suggesting that the AF AWB and AE locks might play a role on the interactions
> between the three algorithms.
>
> Does this make sense and provide a bit more of context about what
> AeLock could be used for ?
>
> Thanks
>    j
>
>
> >
> > Thanks!
> > David
> >
> > > That's the idea afaict.
> > >
> > > Supporting AeLock is optional, of course. Same for AeState, not all of
> > > the states need to be supported. You can specify which ones are
> > > supported in pipeline's ControlInfoMap.
> > >
> > >
> > > Thanks,
> > >
> > > Paul
> > >
> > > > Perhaps combining them to avoid redundancy makes sense?  Or do folks think they
> > > > can they serve different purposes?
> > > >
> > > >
> > > >
> > > >     So:
> > > >
> > > >     We set controls::AeLocked in the metadata according to the value that
> > > >     we received in the corresponding control (which we now receive instead
> > > >     of AeEnable), and
> > > >
> > > >     We set controls::AeState in the metadata to AeStateConverged when
> > > >     agcStatus->locked is true, otherwise to AeStateSearching. We should
> > > >     probably bite the bullet and rename that "locked" field to
> > > >     "converged", otherwise it's going to cause confusion for the rest of
> > > >     eternity.
> > > >
> > > >
> > > > Agree, I think we should rename "locked" to "converged" in the controller
> > > > metadata struct.
> > > >
> > > > Regards,
> > > > Naush
> > > >
> > > >
> > > >
> > > >     David
> > > >
> > > >     >
> > > >     > Thanks,
> > > >     > Naush
> > > >     >
> > > >     >>
> > > >     >>                 libcameraMetadata_.set(controls::DigitalGain,
> > > >     agcStatus->digital_gain);
> > > >     >>         }
> > > >     >>
> > > >     >> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > > >     >> index b47ea324..88a562a8 100644
> > > >     >> --- a/src/ipa/rkisp1/rkisp1.cpp
> > > >     >> +++ b/src/ipa/rkisp1/rkisp1.cpp
> > > >     >> @@ -51,7 +51,7 @@ private:
> > > >     >>                               const rkisp1_stat_buffer *stats);
> > > >     >>
> > > >     >>         void setControls(unsigned int frame);
> > > >     >> -       void metadataReady(unsigned int frame, unsigned int aeState);
> > > >     >> +       void metadataReady(unsigned int frame, int aeState);
> > > >     >>
> > > >     >>         std::map<unsigned int, FrameBuffer> buffers_;
> > > >     >>         std::map<unsigned int, void *> buffersMemory_;
> > > >     >> @@ -227,7 +227,7 @@ void IPARkISP1::updateStatistics(unsigned int frame,
> > > >     >>                                  const rkisp1_stat_buffer *stats)
> > > >     >>  {
> > > >     >>         const rkisp1_cif_isp_stat *params = &stats->params;
> > > >     >> -       unsigned int aeState = 0;
> > > >     >> +       int aeState = controls::AeStateInactive;
> > > >     >>
> > > >     >>         if (stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP) {
> > > >     >>                 const rkisp1_cif_isp_ae_stat *ae = &params->ae;
> > > >     >> @@ -262,7 +262,9 @@ void IPARkISP1::updateStatistics(unsigned int frame,
> > > >     >>                         setControls(frame + 1);
> > > >     >>                 }
> > > >     >>
> > > >     >> -               aeState = fabs(factor - 1.0f) < 0.05f ? 2 : 1;
> > > >     >> +               aeState = fabs(factor - 1.0f) < 0.05f ?
> > > >     >> +                         controls::AeStateConverged :
> > > >     >> +                         controls::AeStateSearching;
> > > >     >>         }
> > > >     >>
> > > >     >>         metadataReady(frame, aeState);
> > > >     >> @@ -281,12 +283,11 @@ void IPARkISP1::setControls(unsigned int frame)
> > > >     >>         queueFrameAction.emit(frame, op);
> > > >     >>  }
> > > >     >>
> > > >     >> -void IPARkISP1::metadataReady(unsigned int frame, unsigned int aeState)
> > > >     >> +void IPARkISP1::metadataReady(unsigned int frame, int aeState)
> > > >     >>  {
> > > >     >>         ControlList ctrls(controls::controls);
> > > >     >>
> > > >     >> -       if (aeState)
> > > >     >> -               ctrls.set(controls::AeLocked, aeState == 2);
> > > >     >> +       ctrls.set(controls::AeState, aeState);
> > > >     >>
> > > >     >>         RkISP1Action op;
> > > >     >>         op.op = ActionMetadata;
> > > >     >> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/
> > > >     control_ids.yaml
> > > >     >> index 9d4638ae..5717bc1f 100644
> > > >     >> --- a/src/libcamera/control_ids.yaml
> > > >     >> +++ b/src/libcamera/control_ids.yaml
> > > >     >> @@ -14,16 +14,70 @@ controls:
> > > >     >>
> > > >     >>          \sa ExposureTime AnalogueGain
> > > >     >>
> > > >     >> -  - AeLocked:
> > > >     >> +  - AeLock:
> > > >     >>        type: bool
> > > >     >>        description: |
> > > >     >> -        Report the lock status of a running AE algorithm.
> > > >     >> +        Control to lock the AE values.
> > > >     >> +        When set to true, the AE algorithm is locked to its latest
> > > >     parameters,
> > > >     >> +        and will not change exposure settings until set to false.
> > > >     >> +        \sa AeState
> > > >     >>
> > > >     >> -        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.
> > > >     >> +  - AeState:
> > > >     >> +      type: int32_t
> > > >     >> +      description: |
> > > >     >> +        Control to report the current AE algorithm state. Enabling or
> > > >     disabling
> > > >     >> +        AE (AeEnable) always resets the AeState 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.
> > > >     >>
> > > >     >> -        \sa AeEnable
> > > >     >> +        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.
> > > >     >> +
> > > >     >> +        The state transitions mentioned below assume that AeEnable is
> > > >     on.
> > > >     >> +
> > > >     >> +        \sa AeLock
> > > >     >> +      enum:
> > > >     >> +        - name: AeStateInactive
> > > >     >> +          value: 0
> > > >     >> +          description: |
> > > >     >> +            The AE algorithm is inactive.
> > > >     >> +            If the camera initiates an AE scan, the state shall go to
> > > >     Searching.
> > > >     >> +            If AeLock is on, the state shall go to Locked.
> > > >     >> +        - 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.
> > > >     >> +            If AeLock is on, the state shall go to Locked.
> > > >     >> +        - name: AeStateConverged
> > > >     >> +          value: 2
> > > >     >> +          description: |
> > > >     >> +            The AE algorithm has converged.
> > > >     >> +            If the camera initiates an AE scan, the state shall go to
> > > >     Searching.
> > > >     >> +            If AeLock is on, the state shall go to Locked.
> > > >     >> +        - name: AeStateLocked
> > > >     >> +          value: 3
> > > >     >> +          description: |
> > > >     >> +            The AE algorithm is locked.
> > > >     >> +            If AeLock is off, the state can go to Searching, Converged,
> > > >     or
> > > >     >> +            FlashRequired.
> > > >     >> +        - name: AeStateFlashRequired
> > > >     >> +          value: 4
> > > >     >> +          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: 5
> > > >     >> +          description: |
> > > >     >> +            The AE algorithm has started a pre-capture metering
> > > >     session.
> > > >     >> +            After the sequence is finished, the state shall go to
> > > >     Converged if
> > > >     >> +            AeLock is off, and Locked if it is on.
> > > >     >> +            \sa AePrecaptureTrigger
> > > >     >>
> > > >     >>    # AeMeteringMode needs further attention:
> > > >     >>    # - Auto-generate max enum value.
> > > >     >> @@ -477,36 +531,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 June 23, 2021, 8:13 a.m. UTC | #8 
Hi David,

On Wed, Jun 23, 2021 at 08:57:04AM +0100, David Plowman wrote:
> Hi Jacopo
>
> Thanks for taking more time to explain this!

I'm just trying to wrap my head around this as well, it's far from being an
"explanation" :)
>
> On Tue, 22 Jun 2021 at 17:11, Jacopo Mondi <jacopo@jmondi.org> wrote:
> >
> > Hi David,
> >
> > On Mon, Jun 21, 2021 at 10:15:21AM +0100, David Plowman wrote:
> > > Hi Paul
> > >
> > > Thanks for the extra explanations.
> > >
> > > On Mon, 21 Jun 2021 at 10:05, <paul.elder@ideasonboard.com> wrote:
> > > >
> > > > Hi Naush,
> > > >
> > > > Thank you for the feedback.
> > > >
> > > > On Fri, Jun 18, 2021 at 02:58:52PM +0100, Naushir Patuck wrote:
> > > > >
> > > > >
> > > > > On Fri, 18 Jun 2021 at 14:13, David Plowman <david.plowman@raspberrypi.com>
> > > > > wrote:
> > > > >
> > > > >     Hi Naush, everyone
> > > > >
> > > > >     On Fri, 18 Jun 2021 at 13:07, Naushir Patuck <naush@raspberrypi.com> wrote:
> > > > >     >
> > > > >     > Hi Paul,
> > > > >     >
> > > > >     > Thank you for your patch.
> > > > >     >
> > > > >     > On Fri, 18 Jun 2021 at 11:34, Paul Elder <paul.elder@ideasonboard.com>
> > > > >     wrote:
> > > > >     >>
> > > > >     >> AeLocked alone isn't sufficient for reporting the AE state, so replace
> > > > >     >> it with AeState. Add an AeLock control for instructing the camera to
> > > > >     >> lock the AE values.
> > > > >     >>
> > > > >     >> Update the current users of AeLocked accordingly.
> > > > >     >>
> > > > >     >> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > > >     >> ---
> > > > >     >>  src/ipa/raspberrypi/raspberrypi.cpp |  5 +-
> > > > >     >>  src/ipa/rkisp1/rkisp1.cpp           | 13 ++--
> > > > >     >>  src/libcamera/control_ids.yaml      | 96 ++++++++++++++++++-----------
> > > > >     >>  3 files changed, 71 insertions(+), 43 deletions(-)
> > > > >     >>
> > > > >     >> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/
> > > > >     raspberrypi.cpp
> > > > >     >> index 1c1e802a..ad0132c0 100644
> > > > >     >> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > > > >     >> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > > > >     >> @@ -468,7 +468,10 @@ void IPARPi::reportMetadata()
> > > > >     >>
> > > > >     >>         AgcStatus *agcStatus = rpiMetadata_.GetLocked<AgcStatus>
> > > > >     ("agc.status");
> > > > >     >>         if (agcStatus) {
> > > > >     >> -               libcameraMetadata_.set(controls::AeLocked, agcStatus->
> > > > >     locked);
> > > > >     >> +               libcameraMetadata_.set(controls::AeState,
> > > > >     >> +                                      agcStatus->locked ?
> > > > >     >> +                                      controls::AeStateLocked :
> > > > >     >> +                                      controls::AeStateSearching);
> > > > >     >
> > > > >     >
> > > > >     > I think, strictly speaking, if agcStatus->locked == true, we should
> > > > >     return
> > > > >     > control::AeStateConverged by the definition below.  David, do you agree?
> > > >
> > > > Oh shoot, that is indeed what I meant. I got mixed up with the status
> > > > name :/
> > > >
> > > > >
> > > > >     Hmm, indeed. My reading is that what we used to know as AeEnable has
> > > > >     now been renamed/replaced by AeLock, is that right? And as you say,
> > > > >     "locked" does not mean "converged" (any more).
> > > > >
> > > > >
> > > > > Yes I agree, but we now have both AeEnable and AeLock defined with this change.
> > > >
> > > > AeEnable is for enabling/disabling the AE routine. AeLock is to lock the
> > > > AE values... which would normally be the same as turning off AE, but you
> > > > want to be able to unlock AE without having to restart the routine.
> > >
> > > I'm still struggling a bit with this, however. What would it mean to
> > > unlock the AE without restarting it? Isn't it then still fixed at
> > > whatever values it had? Perhaps you could provide an example of this
> > > behaviour which would show how it is different.
> > >
> > > Sorry for still being puzzled...
> >
> > And I'm sorry as well for asking possibly stupid questions, but:
> >
> > - It seems to me the RPi AGC routine has two Pause/Resume functions that:
> >  - Pause: sets fixed gain and shutter time to the last computed
> >    shutter and gain values
> >  - Resume: re-sets them to 0
> >
> >  - In the RPi AEGC implementation, if fixed shutter and gains are set
> >    then those values are used
> >
> >  - The AEGC routine is paused/resumed on controls::AeEnable on/off
> >
> >  - When AeEnable == Off, then the only way to override the fixed
> >    shutter/gain is by using the ExposureTime and AnalogueGain controls
> >    (ie manual shutter/gain control)
> >
> > So in you implementation disabling AE has the effect that the AEGC
> > routine uses the last computed values until a new shutter/gain value
> > is set by the application (through what is usually called "manual" mode).
> >
> > The difference between your implementation and a new one that uses
> > AeLocked and AeEnable is, in my understanding, that when the AeLocked
> > is set the AEGC routine is still in control of the shutter/gain which
> > gets "fixed" as you're doing, but it does not accept manual control of
> > the shutter/gain like you're doing unless the AEGC is disabled.
> >
> > This seems to be suggested by the following paragraph in the Android
> > documentation:
> >
> > -------------------------------------------------------------------------------
> >
> > If an application is switching between automatic and manual control
> > and wishes to eliminate any flicker during the switch, the following
> > procedure is recommended:
> >
> > Starting in auto-AE mode:
> > - Lock AE
> > - Wait for the first result to be output that has the AE locked
> > - Copy exposure settings from that result into a request, set the request to manual AE
> > - Submit the capture request, proceed to run manual AE as desired.
> >
> > See android.control.aeState for AE lock related state transition details.
> > -------------------------------------------------------------------------------
> >
> > If we were able to report the AEGC convergence state I think the same
> > could be done without AeLock, by disabling AEGC once it has converged
> > and continue from there with manual exposure. But my understanding is that
> > having a lock controls guarantees that once the AEGC has
> > converged it won't change, so that the in-flight requests that are
> > completed before the first one with manual AE control is processed
> > won't have floating AEGC values,
> >
> > (Let me try to draw it down, hoping it render ok with your client,
> > otherwise https://paste.debian.net/1202043/)
> >
> > Queue of
> > submitted               Completed               AeState in the
> > Requests                Requests                completed Request
> > -----------------------------------------------------------------
> >
> > 0 (AeLock)
> > 1
> > 2
> > 3                       0                       Searching
> > 4                       1                       Converged
> > 5 (AeDisable +          2                       Locked  <- COPY AE VALUES
> >    Manual AE with       3                       Locked
> >    settings from #2)    4                       Locked
> >                         5                       Use Manual AE
>
> Are we saying that AeDisable means "stop adjusting the AEC/AGC
> immediately, whether it's converged or not", but AeLock means "wait
> until it's converged and then immediately stop adjusting the AEC/AGC"?

If AeLock is set on a Converged state then yes, the values are not
changed until the lock is lifted.

Does the transition table repoted here helps ?
https://developer.android.com/reference/android/hardware/camera2/CaptureResult#CONTROL_AE_STATE

Thanks
   j

>
> Thanks!
> David
>
> >
> > Without a Lock the AEGC routine would keep adjusting its values for
> > requests 3 and 4. Request 5 will contain settings copied from Request
> > 2 so a small hiccup might happen. Of course, if the number of frames
> > required to get to a converged state is longer, the duration of the
> > possible AE adjustment period is longer, making the flickering more
> > visible ?
> >
> > Does this make sense ?
> >
> > The android.control.aeMode documentation also reports that:
> >
> > -----------------------------------------------------------------
> > Note that auto-white balance (AWB) and auto-focus (AF) behavior is
> > device dependent when AE is in OFF mode. To have consistent behavior
> > across different devices, it is recommended to either set AWB and AF to
> > OFF mode or lock AWB and AF before setting AE to OFF.See
> > android.control.awbMode,
> > android.control.afMode,android.control.awbLock, and
> > android.control.afTrigger for more details.
> > -----------------------------------------------------------------
> >
> > Suggesting that the AF AWB and AE locks might play a role on the interactions
> > between the three algorithms.
> >
> > Does this make sense and provide a bit more of context about what
> > AeLock could be used for ?
> >
> > Thanks
> >    j
> >
> >
> > >
> > > Thanks!
> > > David
> > >
> > > > That's the idea afaict.
> > > >
> > > > Supporting AeLock is optional, of course. Same for AeState, not all of
> > > > the states need to be supported. You can specify which ones are
> > > > supported in pipeline's ControlInfoMap.
> > > >
> > > >
> > > > Thanks,
> > > >
> > > > Paul
> > > >
> > > > > Perhaps combining them to avoid redundancy makes sense?  Or do folks think they
> > > > > can they serve different purposes?
> > > > >
> > > > >
> > > > >
> > > > >     So:
> > > > >
> > > > >     We set controls::AeLocked in the metadata according to the value that
> > > > >     we received in the corresponding control (which we now receive instead
> > > > >     of AeEnable), and
> > > > >
> > > > >     We set controls::AeState in the metadata to AeStateConverged when
> > > > >     agcStatus->locked is true, otherwise to AeStateSearching. We should
> > > > >     probably bite the bullet and rename that "locked" field to
> > > > >     "converged", otherwise it's going to cause confusion for the rest of
> > > > >     eternity.
> > > > >
> > > > >
> > > > > Agree, I think we should rename "locked" to "converged" in the controller
> > > > > metadata struct.
> > > > >
> > > > > Regards,
> > > > > Naush
> > > > >
> > > > >
> > > > >
> > > > >     David
> > > > >
> > > > >     >
> > > > >     > Thanks,
> > > > >     > Naush
> > > > >     >
> > > > >     >>
> > > > >     >>                 libcameraMetadata_.set(controls::DigitalGain,
> > > > >     agcStatus->digital_gain);
> > > > >     >>         }
> > > > >     >>
> > > > >     >> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > > > >     >> index b47ea324..88a562a8 100644
> > > > >     >> --- a/src/ipa/rkisp1/rkisp1.cpp
> > > > >     >> +++ b/src/ipa/rkisp1/rkisp1.cpp
> > > > >     >> @@ -51,7 +51,7 @@ private:
> > > > >     >>                               const rkisp1_stat_buffer *stats);
> > > > >     >>
> > > > >     >>         void setControls(unsigned int frame);
> > > > >     >> -       void metadataReady(unsigned int frame, unsigned int aeState);
> > > > >     >> +       void metadataReady(unsigned int frame, int aeState);
> > > > >     >>
> > > > >     >>         std::map<unsigned int, FrameBuffer> buffers_;
> > > > >     >>         std::map<unsigned int, void *> buffersMemory_;
> > > > >     >> @@ -227,7 +227,7 @@ void IPARkISP1::updateStatistics(unsigned int frame,
> > > > >     >>                                  const rkisp1_stat_buffer *stats)
> > > > >     >>  {
> > > > >     >>         const rkisp1_cif_isp_stat *params = &stats->params;
> > > > >     >> -       unsigned int aeState = 0;
> > > > >     >> +       int aeState = controls::AeStateInactive;
> > > > >     >>
> > > > >     >>         if (stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP) {
> > > > >     >>                 const rkisp1_cif_isp_ae_stat *ae = &params->ae;
> > > > >     >> @@ -262,7 +262,9 @@ void IPARkISP1::updateStatistics(unsigned int frame,
> > > > >     >>                         setControls(frame + 1);
> > > > >     >>                 }
> > > > >     >>
> > > > >     >> -               aeState = fabs(factor - 1.0f) < 0.05f ? 2 : 1;
> > > > >     >> +               aeState = fabs(factor - 1.0f) < 0.05f ?
> > > > >     >> +                         controls::AeStateConverged :
> > > > >     >> +                         controls::AeStateSearching;
> > > > >     >>         }
> > > > >     >>
> > > > >     >>         metadataReady(frame, aeState);
> > > > >     >> @@ -281,12 +283,11 @@ void IPARkISP1::setControls(unsigned int frame)
> > > > >     >>         queueFrameAction.emit(frame, op);
> > > > >     >>  }
> > > > >     >>
> > > > >     >> -void IPARkISP1::metadataReady(unsigned int frame, unsigned int aeState)
> > > > >     >> +void IPARkISP1::metadataReady(unsigned int frame, int aeState)
> > > > >     >>  {
> > > > >     >>         ControlList ctrls(controls::controls);
> > > > >     >>
> > > > >     >> -       if (aeState)
> > > > >     >> -               ctrls.set(controls::AeLocked, aeState == 2);
> > > > >     >> +       ctrls.set(controls::AeState, aeState);
> > > > >     >>
> > > > >     >>         RkISP1Action op;
> > > > >     >>         op.op = ActionMetadata;
> > > > >     >> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/
> > > > >     control_ids.yaml
> > > > >     >> index 9d4638ae..5717bc1f 100644
> > > > >     >> --- a/src/libcamera/control_ids.yaml
> > > > >     >> +++ b/src/libcamera/control_ids.yaml
> > > > >     >> @@ -14,16 +14,70 @@ controls:
> > > > >     >>
> > > > >     >>          \sa ExposureTime AnalogueGain
> > > > >     >>
> > > > >     >> -  - AeLocked:
> > > > >     >> +  - AeLock:
> > > > >     >>        type: bool
> > > > >     >>        description: |
> > > > >     >> -        Report the lock status of a running AE algorithm.
> > > > >     >> +        Control to lock the AE values.
> > > > >     >> +        When set to true, the AE algorithm is locked to its latest
> > > > >     parameters,
> > > > >     >> +        and will not change exposure settings until set to false.
> > > > >     >> +        \sa AeState
> > > > >     >>
> > > > >     >> -        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.
> > > > >     >> +  - AeState:
> > > > >     >> +      type: int32_t
> > > > >     >> +      description: |
> > > > >     >> +        Control to report the current AE algorithm state. Enabling or
> > > > >     disabling
> > > > >     >> +        AE (AeEnable) always resets the AeState 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.
> > > > >     >>
> > > > >     >> -        \sa AeEnable
> > > > >     >> +        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.
> > > > >     >> +
> > > > >     >> +        The state transitions mentioned below assume that AeEnable is
> > > > >     on.
> > > > >     >> +
> > > > >     >> +        \sa AeLock
> > > > >     >> +      enum:
> > > > >     >> +        - name: AeStateInactive
> > > > >     >> +          value: 0
> > > > >     >> +          description: |
> > > > >     >> +            The AE algorithm is inactive.
> > > > >     >> +            If the camera initiates an AE scan, the state shall go to
> > > > >     Searching.
> > > > >     >> +            If AeLock is on, the state shall go to Locked.
> > > > >     >> +        - 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.
> > > > >     >> +            If AeLock is on, the state shall go to Locked.
> > > > >     >> +        - name: AeStateConverged
> > > > >     >> +          value: 2
> > > > >     >> +          description: |
> > > > >     >> +            The AE algorithm has converged.
> > > > >     >> +            If the camera initiates an AE scan, the state shall go to
> > > > >     Searching.
> > > > >     >> +            If AeLock is on, the state shall go to Locked.
> > > > >     >> +        - name: AeStateLocked
> > > > >     >> +          value: 3
> > > > >     >> +          description: |
> > > > >     >> +            The AE algorithm is locked.
> > > > >     >> +            If AeLock is off, the state can go to Searching, Converged,
> > > > >     or
> > > > >     >> +            FlashRequired.
> > > > >     >> +        - name: AeStateFlashRequired
> > > > >     >> +          value: 4
> > > > >     >> +          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: 5
> > > > >     >> +          description: |
> > > > >     >> +            The AE algorithm has started a pre-capture metering
> > > > >     session.
> > > > >     >> +            After the sequence is finished, the state shall go to
> > > > >     Converged if
> > > > >     >> +            AeLock is off, and Locked if it is on.
> > > > >     >> +            \sa AePrecaptureTrigger
> > > > >     >>
> > > > >     >>    # AeMeteringMode needs further attention:
> > > > >     >>    # - Auto-generate max enum value.
> > > > >     >> @@ -477,36 +531,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
> > > > >     >>
> > > > >
Naushir Patuck June 23, 2021, 9:06 a.m. UTC | #9 
Hi Jacopo and David,

On Wed, 23 Jun 2021 at 09:12, Jacopo Mondi <jacopo@jmondi.org> wrote:

<snip>


> >
> > Are we saying that AeDisable means "stop adjusting the AEC/AGC
> > immediately, whether it's converged or not", but AeLock means "wait
> > until it's converged and then immediately stop adjusting the AEC/AGC"?
>
> If AeLock is set on a Converged state then yes, the values are not
> changed until the lock is lifted.
>
> Does the transition table repoted here helps ?
>
> https://developer.android.com/reference/android/hardware/camera2/CaptureResult#CONTROL_AE_STATE
>
> Thanks
>    j
>

Having a brief look at the above document, I think
Jacopo's description makes
sense now.  Enabling AeLock locks the current AE computed values, whether
the algorithm has converged or not.  However, with AeLock set to on, other
controls are effectively ignored by the AE (e.g. aePrecaptureTrigger, maybe
other things as well).  Not sure why this is the case, as the application
surely
knows what state it wants the AE to be in...?

We don't deal with pre-capture sequences in RPi AE, so for us AeLock and
AeDisable are actually the same thing.  Thinking about it, even with a
pre-capture
sequence, we could probably say the same.  This may also be the case for
other vendor AE algorithms, but Android makes these states explicit.

 Or it could be that I've just completely misunderstood that document :-)

Naush
Jacopo Mondi June 23, 2021, 10:33 a.m. UTC | #10 
Hi Naush,

On Wed, Jun 23, 2021 at 10:06:00AM +0100, Naushir Patuck wrote:
> Hi Jacopo and David,
>
> On Wed, 23 Jun 2021 at 09:12, Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> <snip>
>
>
> > >
> > > Are we saying that AeDisable means "stop adjusting the AEC/AGC
> > > immediately, whether it's converged or not", but AeLock means "wait
> > > until it's converged and then immediately stop adjusting the AEC/AGC"?
> >
> > If AeLock is set on a Converged state then yes, the values are not
> > changed until the lock is lifted.
> >
> > Does the transition table repoted here helps ?
> >
> > https://developer.android.com/reference/android/hardware/camera2/CaptureResult#CONTROL_AE_STATE
> >
> > Thanks
> >    j
> >
>
> Having a brief look at the above document, I think
> Jacopo's description makes
> sense now.  Enabling AeLock locks the current AE computed values, whether
> the algorithm has converged or not.  However, with AeLock set to on, other
> controls are effectively ignored by the AE (e.g. aePrecaptureTrigger, maybe
> other things as well).  Not sure why this is the case, as the application
> surely
> knows what state it wants the AE to be in...?

So my previous example is slightly wrong, as the AeLock was meant to
be set only once the AE has converged, not in request #0.

>
> We don't deal with pre-capture sequences in RPi AE, so for us AeLock and
> AeDisable are actually the same thing.  Thinking about it, even with a
> pre-capture
> sequence, we could probably say the same.  This may also be the case for
> other vendor AE algorithms, but Android makes these states explicit.

I think that's fine.

That's more of a philosophical question, but I don't think we should
try to impose all the IPA implementation to behave exactly the same,
as that would impede the implementation of the most advanced features.
The means the most performant application won't behave exactly the
same between different platform, but I think at least in the controls
definition we should try to get to the minimum common denominator that
satisfies most needs. After all I think Android does the same, the
definition is there for all platforms, but each implementation is
slightly different and indeed it's usually the phone manufacturer
provided camera app that actually makes use of the most advanced
features, while a generic camera app is usually limited to the
simplest use cases.

For this specific use case (the AECG lock/state/enable), let me try to
summarize how we could move forward:

- Replace controls::AeLocked with a more descriptive AeState where the
  current 'locked' is represented by the 'Converged' state.
- Introduce controls::AeLock (which for RPi won't be supported)
- We have controls::AeEnable which more or less maps to
  android.AeMode. AeMode has more states which include control of the
  flash. We do not strictly have to support it now afaict but moving
  controls::AeMode from being a boolean on/off to a list of states
  might help that transition to happen in future.

Do you agree with my understanding here ?

The controls::AeLocked -> controls::AeState[Converged] will require us
and the corresponding RPi camera app update to happen synchronously
even if I admit I don't know how you distribute libcamera. IOW, are the
camera apps compiled against the most recent master or against a
frozen version? So that master can freely advance and the app updates can
follow later once you update the version of libcamera you ship.

>
>  Or it could be that I've just completely misunderstood that document :-)

The fact we all keep asking that to ourselves makes me wonder if we're
all a bunch of idiots or if we're actually exploring rather new
territories in the open camera support landscape. I think time will
tell :)

Thanks
   j

PS As you might have noticed we're actually in the process of
addressing the most advanced controls that android requires for
high-end devices, so expect this kind of discussions to happen again
in future :)

>
> Naush
Naushir Patuck June 23, 2021, 10:56 a.m. UTC | #11 
Hi Jacopo,


On Wed, 23 Jun 2021 at 11:32, Jacopo Mondi <jacopo@jmondi.org> wrote:

> Hi Naush,
>
> On Wed, Jun 23, 2021 at 10:06:00AM +0100, Naushir Patuck wrote:
> > Hi Jacopo and David,
> >
> > On Wed, 23 Jun 2021 at 09:12, Jacopo Mondi <jacopo@jmondi.org> wrote:
> >
> > <snip>
> >
> >
> > > >
> > > > Are we saying that AeDisable means "stop adjusting the AEC/AGC
> > > > immediately, whether it's converged or not", but AeLock means "wait
> > > > until it's converged and then immediately stop adjusting the
> AEC/AGC"?
> > >
> > > If AeLock is set on a Converged state then yes, the values are not
> > > changed until the lock is lifted.
> > >
> > > Does the transition table repoted here helps ?
> > >
> > >
> https://developer.android.com/reference/android/hardware/camera2/CaptureResult#CONTROL_AE_STATE
> > >
> > > Thanks
> > >    j
> > >
> >
> > Having a brief look at the above document, I think
> > Jacopo's description makes
> > sense now.  Enabling AeLock locks the current AE computed values, whether
> > the algorithm has converged or not.  However, with AeLock set to on,
> other
> > controls are effectively ignored by the AE (e.g. aePrecaptureTrigger,
> maybe
> > other things as well).  Not sure why this is the case, as the application
> > surely
> > knows what state it wants the AE to be in...?
>
> So my previous example is slightly wrong, as the AeLock was meant to
> be set only once the AE has converged, not in request #0.
>
> >
> > We don't deal with pre-capture sequences in RPi AE, so for us AeLock and
> > AeDisable are actually the same thing.  Thinking about it, even with a
> > pre-capture
> > sequence, we could probably say the same.  This may also be the case for
> > other vendor AE algorithms, but Android makes these states explicit.
>
> I think that's fine.
>
> That's more of a philosophical question, but I don't think we should
> try to impose all the IPA implementation to behave exactly the same,
> as that would impede the implementation of the most advanced features.
> The means the most performant application won't behave exactly the
> same between different platform, but I think at least in the controls
> definition we should try to get to the minimum common denominator that
> satisfies most needs. After all I think Android does the same, the
> definition is there for all platforms, but each implementation is
> slightly different and indeed it's usually the phone manufacturer
> provided camera app that actually makes use of the most advanced
> features, while a generic camera app is usually limited to the
> simplest use cases.
>
> For this specific use case (the AECG lock/state/enable), let me try to
> summarize how we could move forward:
>
> - Replace controls::AeLocked with a more descriptive AeState where the
>   current 'locked' is represented by the 'Converged' state.
> - Introduce controls::AeLock (which for RPi won't be supported)
> - We have controls::AeEnable which more or less maps to
>   android.AeMode. AeMode has more states which include control of the
>   flash. We do not strictly have to support it now afaict but moving
>   controls::AeMode from being a boolean on/off to a list of states
>   might help that transition to happen in future.
>

> Do you agree with my understanding here ?
>

That all sounds good to me, but I would like David to have his say as
well.  It will probably be the case that our IPA will treat controls::AeLock
the same as our "Paused" state.


> The controls::AeLocked -> controls::AeState[Converged] will require us
> and the corresponding RPi camera app update to happen synchronously
> even if I admit I don't know how you distribute libcamera. IOW, are the
> camera apps compiled against the most recent master or against a
> frozen version? So that master can freely advance and the app updates can
> follow later once you update the version of libcamera you ship.
>

We currently track master, so will have to update our apps separately when
this
change goes through.  Eventually, we will freeze versions in our
distribution to
avoid breakages, but we are not there yet.

Regards,
Naush


> >
> >  Or it could be that I've just completely misunderstood that document :-)
>
> The fact we all keep asking that to ourselves makes me wonder if we're
> all a bunch of idiots or if we're actually exploring rather new
> territories in the open camera support landscape. I think time will
> tell :)
>
> Thanks
>    j
>
> PS As you might have noticed we're actually in the process of
> addressing the most advanced controls that android requires for
> high-end devices, so expect this kind of discussions to happen again
> in future :)
>
> >
> > Naush
>
David Plowman June 23, 2021, 3:19 p.m. UTC | #12 
Hi Naush, Jacopo

On Wed, 23 Jun 2021 at 11:56, Naushir Patuck <naush@raspberrypi.com> wrote:
>
> Hi Jacopo,
>
>
> On Wed, 23 Jun 2021 at 11:32, Jacopo Mondi <jacopo@jmondi.org> wrote:
>>
>> Hi Naush,
>>
>> On Wed, Jun 23, 2021 at 10:06:00AM +0100, Naushir Patuck wrote:
>> > Hi Jacopo and David,
>> >
>> > On Wed, 23 Jun 2021 at 09:12, Jacopo Mondi <jacopo@jmondi.org> wrote:
>> >
>> > <snip>
>> >
>> >
>> > > >
>> > > > Are we saying that AeDisable means "stop adjusting the AEC/AGC
>> > > > immediately, whether it's converged or not", but AeLock means "wait
>> > > > until it's converged and then immediately stop adjusting the AEC/AGC"?
>> > >
>> > > If AeLock is set on a Converged state then yes, the values are not
>> > > changed until the lock is lifted.
>> > >
>> > > Does the transition table repoted here helps ?
>> > >
>> > > https://developer.android.com/reference/android/hardware/camera2/CaptureResult#CONTROL_AE_STATE
>> > >
>> > > Thanks
>> > >    j
>> > >
>> >
>> > Having a brief look at the above document, I think
>> > Jacopo's description makes
>> > sense now.  Enabling AeLock locks the current AE computed values, whether
>> > the algorithm has converged or not.  However, with AeLock set to on, other
>> > controls are effectively ignored by the AE (e.g. aePrecaptureTrigger, maybe
>> > other things as well).  Not sure why this is the case, as the application
>> > surely
>> > knows what state it wants the AE to be in...?
>>
>> So my previous example is slightly wrong, as the AeLock was meant to
>> be set only once the AE has converged, not in request #0.
>>
>> >
>> > We don't deal with pre-capture sequences in RPi AE, so for us AeLock and
>> > AeDisable are actually the same thing.  Thinking about it, even with a
>> > pre-capture
>> > sequence, we could probably say the same.  This may also be the case for
>> > other vendor AE algorithms, but Android makes these states explicit.
>>
>> I think that's fine.
>>
>> That's more of a philosophical question, but I don't think we should
>> try to impose all the IPA implementation to behave exactly the same,
>> as that would impede the implementation of the most advanced features.
>> The means the most performant application won't behave exactly the
>> same between different platform, but I think at least in the controls
>> definition we should try to get to the minimum common denominator that
>> satisfies most needs. After all I think Android does the same, the
>> definition is there for all platforms, but each implementation is
>> slightly different and indeed it's usually the phone manufacturer
>> provided camera app that actually makes use of the most advanced
>> features, while a generic camera app is usually limited to the
>> simplest use cases.
>>
>> For this specific use case (the AECG lock/state/enable), let me try to
>> summarize how we could move forward:
>>
>> - Replace controls::AeLocked with a more descriptive AeState where the
>>   current 'locked' is represented by the 'Converged' state.
>> - Introduce controls::AeLock (which for RPi won't be supported)
>> - We have controls::AeEnable which more or less maps to
>>   android.AeMode. AeMode has more states which include control of the
>>   flash. We do not strictly have to support it now afaict but moving
>>   controls::AeMode from being a boolean on/off to a list of states
>>   might help that transition to happen in future.
>>
>>
>> Do you agree with my understanding here ?
>
>
> That all sounds good to me, but I would like David to have his say as
> well.  It will probably be the case that our IPA will treat controls::AeLock
> the same as our "Paused" state.

I think we treat "lock" essentially the same as "enable". Maybe we
ignore manual settings in the former but not the latter, but it's not
sounding like a big deal. Will discuss with Naush when I'm next in the
office.

>
>>
>> The controls::AeLocked -> controls::AeState[Converged] will require us
>> and the corresponding RPi camera app update to happen synchronously
>> even if I admit I don't know how you distribute libcamera. IOW, are the
>> camera apps compiled against the most recent master or against a
>> frozen version? So that master can freely advance and the app updates can
>> follow later once you update the version of libcamera you ship.
>
>
> We currently track master, so will have to update our apps separately when this
> change goes through.  Eventually, we will freeze versions in our distribution to
> avoid breakages, but we are not there yet.

I'd prefer to attach ourselves to a particular libcamera version once
we have frozen a first version of the API, though maybe that's still
too far off? I'd also like to sort out our sporadic crash
(https://bugs.libcamera.org/show_bug.cgi?id=26) before doing that too.

Thanks
David

>
> Regards,
> Naush
>
>>
>> >
>> >  Or it could be that I've just completely misunderstood that document :-)
>>
>> The fact we all keep asking that to ourselves makes me wonder if we're
>> all a bunch of idiots or if we're actually exploring rather new
>> territories in the open camera support landscape. I think time will
>> tell :)
>>
>> Thanks
>>    j
>>
>> PS As you might have noticed we're actually in the process of
>> addressing the most advanced controls that android requires for
>> high-end devices, so expect this kind of discussions to happen again
>> in future :)
>>
>> >
>> > Naush
Laurent Pinchart June 28, 2021, 12:06 a.m. UTC | #13 
Hi David,

On Wed, Jun 23, 2021 at 04:19:55PM +0100, David Plowman wrote:
> On Wed, 23 Jun 2021 at 11:56, Naushir Patuck wrote:
> > On Wed, 23 Jun 2021 at 11:32, Jacopo Mondi wrote:
> >> On Wed, Jun 23, 2021 at 10:06:00AM +0100, Naushir Patuck wrote:
> >>> On Wed, 23 Jun 2021 at 09:12, Jacopo Mondi wrote:
> >>>
> >>> <snip>
> >>>
> >>>>> Are we saying that AeDisable means "stop adjusting the AEC/AGC
> >>>>> immediately, whether it's converged or not", but AeLock means "wait
> >>>>> until it's converged and then immediately stop adjusting the AEC/AGC"?
> >>>>
> >>>> If AeLock is set on a Converged state then yes, the values are not
> >>>> changed until the lock is lifted.
> >>>>
> >>>> Does the transition table repoted here helps ?
> >>>>
> >>>> https://developer.android.com/reference/android/hardware/camera2/CaptureResult#CONTROL_AE_STATE
> >>>
> >>> Having a brief look at the above document, I think
> >>> Jacopo's description makes
> >>> sense now.  Enabling AeLock locks the current AE computed values, whether
> >>> the algorithm has converged or not.  However, with AeLock set to on, other
> >>> controls are effectively ignored by the AE (e.g. aePrecaptureTrigger, maybe
> >>> other things as well).  Not sure why this is the case, as the application
> >>> surely
> >>> knows what state it wants the AE to be in...?
> >>
> >> So my previous example is slightly wrong, as the AeLock was meant to
> >> be set only once the AE has converged, not in request #0.
> >>
> >>> We don't deal with pre-capture sequences in RPi AE, so for us AeLock and
> >>> AeDisable are actually the same thing.  Thinking about it, even with a
> >>> pre-capture
> >>> sequence, we could probably say the same.  This may also be the case for
> >>> other vendor AE algorithms, but Android makes these states explicit.
> >>
> >> I think that's fine.
> >>
> >> That's more of a philosophical question, but I don't think we should
> >> try to impose all the IPA implementation to behave exactly the same,
> >> as that would impede the implementation of the most advanced features.
> >> The means the most performant application won't behave exactly the
> >> same between different platform, but I think at least in the controls
> >> definition we should try to get to the minimum common denominator that
> >> satisfies most needs. After all I think Android does the same, the
> >> definition is there for all platforms, but each implementation is
> >> slightly different and indeed it's usually the phone manufacturer
> >> provided camera app that actually makes use of the most advanced
> >> features, while a generic camera app is usually limited to the
> >> simplest use cases.
> >>
> >> For this specific use case (the AECG lock/state/enable), let me try to
> >> summarize how we could move forward:
> >>
> >> - Replace controls::AeLocked with a more descriptive AeState where the
> >>   current 'locked' is represented by the 'Converged' state.
> >> - Introduce controls::AeLock (which for RPi won't be supported)
> >> - We have controls::AeEnable which more or less maps to
> >>   android.AeMode. AeMode has more states which include control of the
> >>   flash. We do not strictly have to support it now afaict but moving
> >>   controls::AeMode from being a boolean on/off to a list of states
> >>   might help that transition to happen in future.
> >>
> >> Do you agree with my understanding here ?
> >
> > That all sounds good to me, but I would like David to have his say as
> > well.  It will probably be the case that our IPA will treat controls::AeLock
> > the same as our "Paused" state.
> 
> I think we treat "lock" essentially the same as "enable". Maybe we
> ignore manual settings in the former but not the latter, but it's not
> sounding like a big deal. Will discuss with Naush when I'm next in the
> office.
> 
> >> The controls::AeLocked -> controls::AeState[Converged] will require us
> >> and the corresponding RPi camera app update to happen synchronously
> >> even if I admit I don't know how you distribute libcamera. IOW, are the
> >> camera apps compiled against the most recent master or against a
> >> frozen version? So that master can freely advance and the app updates can
> >> follow later once you update the version of libcamera you ship.
> >
> > We currently track master, so will have to update our apps separately when this
> > change goes through.  Eventually, we will freeze versions in our distribution to
> > avoid breakages, but we are not there yet.
> 
> I'd prefer to attach ourselves to a particular libcamera version once
> we have frozen a first version of the API, though maybe that's still
> too far off? I'd also like to sort out our sporadic crash
> (https://bugs.libcamera.org/show_bug.cgi?id=26) before doing that too.

That one is still on my todo list, it will get fixed.

> >>> Or it could be that I've just completely misunderstood that document :-)
> >>
> >> The fact we all keep asking that to ourselves makes me wonder if we're
> >> all a bunch of idiots or if we're actually exploring rather new
> >> territories in the open camera support landscape. I think time will
> >> tell :)
> >>
> >> PS As you might have noticed we're actually in the process of
> >> addressing the most advanced controls that android requires for
> >> high-end devices, so expect this kind of discussions to happen again
> >> in future :)
Laurent Pinchart June 28, 2021, 12:49 a.m. UTC | #14 
Hello everybody,

On Tue, Jun 22, 2021 at 06:12:26PM +0200, Jacopo Mondi wrote:
> On Mon, Jun 21, 2021 at 10:15:21AM +0100, David Plowman wrote:
> > On Mon, 21 Jun 2021 at 10:05, <paul.elder@ideasonboard.com> wrote:
> >> On Fri, Jun 18, 2021 at 02:58:52PM +0100, Naushir Patuck wrote:
> >>> On Fri, 18 Jun 2021 at 14:13, David Plowman wrote:
> >>>> On Fri, 18 Jun 2021 at 13:07, Naushir Patuck wrote:
> >>>>> On Fri, 18 Jun 2021 at 11:34, Paul Elder wrote:
> >>>>>>
> >>>>>> AeLocked alone isn't sufficient for reporting the AE state, so replace
> >>>>>> it with AeState. Add an AeLock control for instructing the camera to
> >>>>>> lock the AE values.
> >>>>>>
> >>>>>> Update the current users of AeLocked accordingly.
> >>>>>>
> >>>>>> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> >>>>>> ---
> >>>>>>  src/ipa/raspberrypi/raspberrypi.cpp |  5 +-
> >>>>>>  src/ipa/rkisp1/rkisp1.cpp           | 13 ++--
> >>>>>>  src/libcamera/control_ids.yaml      | 96 ++++++++++++++++++-----------
> >>>>>>  3 files changed, 71 insertions(+), 43 deletions(-)
> >>>>>>
> >>>>>> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> >>>>>> index 1c1e802a..ad0132c0 100644
> >>>>>> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> >>>>>> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> >>>>>> @@ -468,7 +468,10 @@ void IPARPi::reportMetadata()
> >>>>>>
> >>>>>>         AgcStatus *agcStatus = rpiMetadata_.GetLocked<AgcStatus>("agc.status");
> >>>>>>         if (agcStatus) {
> >>>>>> -               libcameraMetadata_.set(controls::AeLocked, agcStatus->locked);
> >>>>>> +               libcameraMetadata_.set(controls::AeState,
> >>>>>> +                                      agcStatus->locked ?
> >>>>>> +                                      controls::AeStateLocked :
> >>>>>> +                                      controls::AeStateSearching);
> >>>>>
> >>>>> I think, strictly speaking, if agcStatus->locked == true, we should return
> >>>>> control::AeStateConverged by the definition below.  David, do you agree?
> >>
> >> Oh shoot, that is indeed what I meant. I got mixed up with the status
> >> name :/
> >>
> >>>> Hmm, indeed. My reading is that what we used to know as AeEnable has
> >>>> now been renamed/replaced by AeLock, is that right? And as you say,
> >>>> "locked" does not mean "converged" (any more).
> >>>
> >>> Yes I agree, but we now have both AeEnable and AeLock defined with this change.
> >>
> >> AeEnable is for enabling/disabling the AE routine. AeLock is to lock the
> >> AE values... which would normally be the same as turning off AE, but you
> >> want to be able to unlock AE without having to restart the routine.
> >
> > I'm still struggling a bit with this, however. What would it mean to
> > unlock the AE without restarting it? Isn't it then still fixed at
> > whatever values it had? Perhaps you could provide an example of this
> > behaviour which would show how it is different.
> >
> > Sorry for still being puzzled...
> 
> And I'm sorry as well for asking possibly stupid questions, but:
> 
> - It seems to me the RPi AGC routine has two Pause/Resume functions that:
>  - Pause: sets fixed gain and shutter time to the last computed
>    shutter and gain values
>  - Resume: re-sets them to 0
> 
>  - In the RPi AEGC implementation, if fixed shutter and gains are set
>    then those values are used
> 
>  - The AEGC routine is paused/resumed on controls::AeEnable on/off
> 
>  - When AeEnable == Off, then the only way to override the fixed
>    shutter/gain is by using the ExposureTime and AnalogueGain controls
>    (ie manual shutter/gain control)
> 
> So in you implementation disabling AE has the effect that the AEGC
> routine uses the last computed values until a new shutter/gain value
> is set by the application (through what is usually called "manual" mode).
> 
> The difference between your implementation and a new one that uses
> AeLocked and AeEnable is, in my understanding, that when the AeLocked
> is set the AEGC routine is still in control of the shutter/gain which
> gets "fixed" as you're doing, but it does not accept manual control of
> the shutter/gain like you're doing unless the AEGC is disabled.
> 
> This seems to be suggested by the following paragraph in the Android
> documentation:
> 
> -------------------------------------------------------------------------------
> 
> If an application is switching between automatic and manual control
> and wishes to eliminate any flicker during the switch, the following
> procedure is recommended:
> 
> Starting in auto-AE mode:
> - Lock AE
> - Wait for the first result to be output that has the AE locked
> - Copy exposure settings from that result into a request, set the request to manual AE
> - Submit the capture request, proceed to run manual AE as desired.
> 
> See android.control.aeState for AE lock related state transition details.
> -------------------------------------------------------------------------------
> 
> If we were able to report the AEGC convergence state I think the same
> could be done without AeLock, by disabling AEGC once it has converged
> and continue from there with manual exposure. But my understanding is that
> having a lock controls guarantees that once the AEGC has
> converged it won't change, so that the in-flight requests that are
> completed before the first one with manual AE control is processed
> won't have floating AEGC values,
> 
> (Let me try to draw it down, hoping it render ok with your client,
> otherwise https://paste.debian.net/1202043/)
> 
> Queue of
> submitted               Completed               AeState in the
> Requests                Requests                completed Request
> -----------------------------------------------------------------
> 
> 0 (AeLock)
> 1
> 2
> 3                       0                       Searching
> 4                       1                       Converged
> 5 (AeDisable +          2                       Locked  <- COPY AE VALUES
>    Manual AE with       3                       Locked
>    settings from #2)    4                       Locked
>                         5                       Use Manual AE
> 
> Without a Lock the AEGC routine would keep adjusting its values for
> requests 3 and 4. Request 5 will contain settings copied from Request
> 2 so a small hiccup might happen. Of course, if the number of frames
> required to get to a converged state is longer, the duration of the
> possible AE adjustment period is longer, making the flickering more
> visible ?
> 
> Does this make sense ?
> 
> The android.control.aeMode documentation also reports that:
> 
> -----------------------------------------------------------------
> Note that auto-white balance (AWB) and auto-focus (AF) behavior is
> device dependent when AE is in OFF mode. To have consistent behavior
> across different devices, it is recommended to either set AWB and AF to
> OFF mode or lock AWB and AF before setting AE to OFF.See
> android.control.awbMode,
> android.control.afMode,android.control.awbLock, and
> android.control.afTrigger for more details.
> -----------------------------------------------------------------
> 
> Suggesting that the AF AWB and AE locks might play a role on the interactions
> between the three algorithms.
> 
> Does this make sense and provide a bit more of context about what
> AeLock could be used for ?

I'm still a bit puzzled by the need for an AeLock control. I do
understand that it's used in Android to switch to manual exposure
without glitches, as detailed above. However, couldn't the same result
be achieved by switching AeMode from on to off (thus switching to manual
mode), without specifying manual values for the sensitivity, exposure
and frame duration ? The camera would then keep the last values computed
by the AEGC, which would be reported in metadata, and could be then used
as a basis to set other manual values.

This may be related to defining what happens to manual control values
when running in auto mode. The camera will report computed values for
the sensitivity, exposure time and frame duration in metadata, but
should those values be implicitly copied to the request controls
(effectively only, not really adding them to the Request instances) when
the algorithm is switched off, or should the last value that was set
manually be used in that case ? I think we need to clearly specify this,
ideally in a consistent fashion across all controls. If we decide that
the camera behaviour should be to implicitly copy the metadata value to
the control value, then AeLock doesn't seem to be needed. Otherwise,
switching seamlessly from auto to manual mode requires a mechanism
similar to AeLock.

Is there any other need for the AeLock control that I may have missed ?

> >> That's the idea afaict.
> >>
> >> Supporting AeLock is optional, of course. Same for AeState, not all of
> >> the states need to be supported. You can specify which ones are
> >> supported in pipeline's ControlInfoMap.
> >>
> >>> Perhaps combining them to avoid redundancy makes sense?  Or do folks think they
> >>> can they serve different purposes?
> >>>
> >>>> So:
> >>>>
> >>>> We set controls::AeLocked in the metadata according to the value that
> >>>> we received in the corresponding control (which we now receive instead
> >>>> of AeEnable), and
> >>>>
> >>>> We set controls::AeState in the metadata to AeStateConverged when
> >>>> agcStatus->locked is true, otherwise to AeStateSearching. We should
> >>>> probably bite the bullet and rename that "locked" field to
> >>>> "converged", otherwise it's going to cause confusion for the rest of
> >>>> eternity.
> >>>
> >>> Agree, I think we should rename "locked" to "converged" in the controller
> >>> metadata struct.
> >>>
> >>>>>>                 libcameraMetadata_.set(controls::DigitalGain, agcStatus->digital_gain);
> >>>>>>         }
> >>>>>>
> >>>>>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> >>>>>> index b47ea324..88a562a8 100644
> >>>>>> --- a/src/ipa/rkisp1/rkisp1.cpp
> >>>>>> +++ b/src/ipa/rkisp1/rkisp1.cpp
> >>>>>> @@ -51,7 +51,7 @@ private:
> >>>>>>                               const rkisp1_stat_buffer *stats);
> >>>>>>
> >>>>>>         void setControls(unsigned int frame);
> >>>>>> -       void metadataReady(unsigned int frame, unsigned int aeState);
> >>>>>> +       void metadataReady(unsigned int frame, int aeState);
> >>>>>>
> >>>>>>         std::map<unsigned int, FrameBuffer> buffers_;
> >>>>>>         std::map<unsigned int, void *> buffersMemory_;
> >>>>>> @@ -227,7 +227,7 @@ void IPARkISP1::updateStatistics(unsigned int frame,
> >>>>>>                                  const rkisp1_stat_buffer *stats)
> >>>>>>  {
> >>>>>>         const rkisp1_cif_isp_stat *params = &stats->params;
> >>>>>> -       unsigned int aeState = 0;
> >>>>>> +       int aeState = controls::AeStateInactive;
> >>>>>>
> >>>>>>         if (stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP) {
> >>>>>>                 const rkisp1_cif_isp_ae_stat *ae = &params->ae;
> >>>>>> @@ -262,7 +262,9 @@ void IPARkISP1::updateStatistics(unsigned int frame,
> >>>>>>                         setControls(frame + 1);
> >>>>>>                 }
> >>>>>>
> >>>>>> -               aeState = fabs(factor - 1.0f) < 0.05f ? 2 : 1;
> >>>>>> +               aeState = fabs(factor - 1.0f) < 0.05f ?
> >>>>>> +                         controls::AeStateConverged :
> >>>>>> +                         controls::AeStateSearching;
> >>>>>>         }
> >>>>>>
> >>>>>>         metadataReady(frame, aeState);
> >>>>>> @@ -281,12 +283,11 @@ void IPARkISP1::setControls(unsigned int frame)
> >>>>>>         queueFrameAction.emit(frame, op);
> >>>>>>  }
> >>>>>>
> >>>>>> -void IPARkISP1::metadataReady(unsigned int frame, unsigned int aeState)
> >>>>>> +void IPARkISP1::metadataReady(unsigned int frame, int aeState)
> >>>>>>  {
> >>>>>>         ControlList ctrls(controls::controls);
> >>>>>>
> >>>>>> -       if (aeState)
> >>>>>> -               ctrls.set(controls::AeLocked, aeState == 2);
> >>>>>> +       ctrls.set(controls::AeState, aeState);
> >>>>>>
> >>>>>>         RkISP1Action op;
> >>>>>>         op.op = ActionMetadata;
> >>>>>> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> >>>>>> index 9d4638ae..5717bc1f 100644
> >>>>>> --- a/src/libcamera/control_ids.yaml
> >>>>>> +++ b/src/libcamera/control_ids.yaml
> >>>>>> @@ -14,16 +14,70 @@ controls:
> >>>>>>
> >>>>>>          \sa ExposureTime AnalogueGain
> >>>>>>
> >>>>>> -  - AeLocked:
> >>>>>> +  - AeLock:
> >>>>>>        type: bool
> >>>>>>        description: |
> >>>>>> -        Report the lock status of a running AE algorithm.
> >>>>>> +        Control to lock the AE values.
> >>>>>> +        When set to true, the AE algorithm is locked to its latest parameters,
> >>>>>> +        and will not change exposure settings until set to false.
> >>>>>> +        \sa AeState
> >>>>>>
> >>>>>> -        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.
> >>>>>> +  - AeState:
> >>>>>> +      type: int32_t
> >>>>>> +      description: |
> >>>>>> +        Control to report the current AE algorithm state. Enabling or disabling
> >>>>>> +        AE (AeEnable) always resets the AeState 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.
> >>>>>>
> >>>>>> -        \sa AeEnable
> >>>>>> +        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.
> >>>>>> +
> >>>>>> +        The state transitions mentioned below assume that AeEnable is on.
> >>>>>> +
> >>>>>> +        \sa AeLock
> >>>>>> +      enum:
> >>>>>> +        - name: AeStateInactive
> >>>>>> +          value: 0
> >>>>>> +          description: |
> >>>>>> +            The AE algorithm is inactive.
> >>>>>> +            If the camera initiates an AE scan, the state shall go to Searching.
> >>>>>> +            If AeLock is on, the state shall go to Locked.
> >>>>>> +        - 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.
> >>>>>> +            If AeLock is on, the state shall go to Locked.
> >>>>>> +        - name: AeStateConverged
> >>>>>> +          value: 2
> >>>>>> +          description: |
> >>>>>> +            The AE algorithm has converged.
> >>>>>> +            If the camera initiates an AE scan, the state shall go to Searching.
> >>>>>> +            If AeLock is on, the state shall go to Locked.
> >>>>>> +        - name: AeStateLocked
> >>>>>> +          value: 3
> >>>>>> +          description: |
> >>>>>> +            The AE algorithm is locked.
> >>>>>> +            If AeLock is off, the state can go to Searching, Converged, or
> >>>>>> +            FlashRequired.
> >>>>>> +        - name: AeStateFlashRequired
> >>>>>> +          value: 4
> >>>>>> +          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: 5
> >>>>>> +          description: |
> >>>>>> +            The AE algorithm has started a pre-capture metering session.
> >>>>>> +            After the sequence is finished, the state shall go to Converged if
> >>>>>> +            AeLock is off, and Locked if it is on.
> >>>>>> +            \sa AePrecaptureTrigger
> >>>>>>
> >>>>>>    # AeMeteringMode needs further attention:
> >>>>>>    # - Auto-generate max enum value.
> >>>>>> @@ -477,36 +531,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
Naushir Patuck June 28, 2021, 9:14 a.m. UTC | #15 
Hi Laurent,

On Mon, 28 Jun 2021 at 01:49, Laurent Pinchart <
laurent.pinchart@ideasonboard.com> wrote:

> Hello everybody,
>
> On Tue, Jun 22, 2021 at 06:12:26PM +0200, Jacopo Mondi wrote:
> > On Mon, Jun 21, 2021 at 10:15:21AM +0100, David Plowman wrote:
> > > On Mon, 21 Jun 2021 at 10:05, <paul.elder@ideasonboard.com> wrote:
> > >> On Fri, Jun 18, 2021 at 02:58:52PM +0100, Naushir Patuck wrote:
> > >>> On Fri, 18 Jun 2021 at 14:13, David Plowman wrote:
> > >>>> On Fri, 18 Jun 2021 at 13:07, Naushir Patuck wrote:
> > >>>>> On Fri, 18 Jun 2021 at 11:34, Paul Elder wrote:
> > >>>>>>
> > >>>>>> AeLocked alone isn't sufficient for reporting the AE state, so
> replace
> > >>>>>> it with AeState. Add an AeLock control for instructing the camera
> to
> > >>>>>> lock the AE values.
> > >>>>>>
> > >>>>>> Update the current users of AeLocked accordingly.
> > >>>>>>
> > >>>>>> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > >>>>>> ---
> > >>>>>>  src/ipa/raspberrypi/raspberrypi.cpp |  5 +-
> > >>>>>>  src/ipa/rkisp1/rkisp1.cpp           | 13 ++--
> > >>>>>>  src/libcamera/control_ids.yaml      | 96
> ++++++++++++++++++-----------
> > >>>>>>  3 files changed, 71 insertions(+), 43 deletions(-)
> > >>>>>>
> > >>>>>> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> > >>>>>> index 1c1e802a..ad0132c0 100644
> > >>>>>> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > >>>>>> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > >>>>>> @@ -468,7 +468,10 @@ void IPARPi::reportMetadata()
> > >>>>>>
> > >>>>>>         AgcStatus *agcStatus =
> rpiMetadata_.GetLocked<AgcStatus>("agc.status");
> > >>>>>>         if (agcStatus) {
> > >>>>>> -               libcameraMetadata_.set(controls::AeLocked,
> agcStatus->locked);
> > >>>>>> +               libcameraMetadata_.set(controls::AeState,
> > >>>>>> +                                      agcStatus->locked ?
> > >>>>>> +                                      controls::AeStateLocked :
> > >>>>>> +
> controls::AeStateSearching);
> > >>>>>
> > >>>>> I think, strictly speaking, if agcStatus->locked == true, we
> should return
> > >>>>> control::AeStateConverged by the definition below.  David, do you
> agree?
> > >>
> > >> Oh shoot, that is indeed what I meant. I got mixed up with the status
> > >> name :/
> > >>
> > >>>> Hmm, indeed. My reading is that what we used to know as AeEnable has
> > >>>> now been renamed/replaced by AeLock, is that right? And as you say,
> > >>>> "locked" does not mean "converged" (any more).
> > >>>
> > >>> Yes I agree, but we now have both AeEnable and AeLock defined with
> this change.
> > >>
> > >> AeEnable is for enabling/disabling the AE routine. AeLock is to lock
> the
> > >> AE values... which would normally be the same as turning off AE, but
> you
> > >> want to be able to unlock AE without having to restart the routine.
> > >
> > > I'm still struggling a bit with this, however. What would it mean to
> > > unlock the AE without restarting it? Isn't it then still fixed at
> > > whatever values it had? Perhaps you could provide an example of this
> > > behaviour which would show how it is different.
> > >
> > > Sorry for still being puzzled...
> >
> > And I'm sorry as well for asking possibly stupid questions, but:
> >
> > - It seems to me the RPi AGC routine has two Pause/Resume functions that:
> >  - Pause: sets fixed gain and shutter time to the last computed
> >    shutter and gain values
> >  - Resume: re-sets them to 0
> >
> >  - In the RPi AEGC implementation, if fixed shutter and gains are set
> >    then those values are used
> >
> >  - The AEGC routine is paused/resumed on controls::AeEnable on/off
> >
> >  - When AeEnable == Off, then the only way to override the fixed
> >    shutter/gain is by using the ExposureTime and AnalogueGain controls
> >    (ie manual shutter/gain control)
> >
> > So in you implementation disabling AE has the effect that the AEGC
> > routine uses the last computed values until a new shutter/gain value
> > is set by the application (through what is usually called "manual" mode).
> >
> > The difference between your implementation and a new one that uses
> > AeLocked and AeEnable is, in my understanding, that when the AeLocked
> > is set the AEGC routine is still in control of the shutter/gain which
> > gets "fixed" as you're doing, but it does not accept manual control of
> > the shutter/gain like you're doing unless the AEGC is disabled.
> >
> > This seems to be suggested by the following paragraph in the Android
> > documentation:
> >
> >
> -------------------------------------------------------------------------------
> >
> > If an application is switching between automatic and manual control
> > and wishes to eliminate any flicker during the switch, the following
> > procedure is recommended:
> >
> > Starting in auto-AE mode:
> > - Lock AE
> > - Wait for the first result to be output that has the AE locked
> > - Copy exposure settings from that result into a request, set the
> request to manual AE
> > - Submit the capture request, proceed to run manual AE as desired.
> >
> > See android.control.aeState for AE lock related state transition details.
> >
> -------------------------------------------------------------------------------
> >
> > If we were able to report the AEGC convergence state I think the same
> > could be done without AeLock, by disabling AEGC once it has converged
> > and continue from there with manual exposure. But my understanding is
> that
> > having a lock controls guarantees that once the AEGC has
> > converged it won't change, so that the in-flight requests that are
> > completed before the first one with manual AE control is processed
> > won't have floating AEGC values,
> >
> > (Let me try to draw it down, hoping it render ok with your client,
> > otherwise https://paste.debian.net/1202043/)
> >
> > Queue of
> > submitted               Completed               AeState in the
> > Requests                Requests                completed Request
> > -----------------------------------------------------------------
> >
> > 0 (AeLock)
> > 1
> > 2
> > 3                       0                       Searching
> > 4                       1                       Converged
> > 5 (AeDisable +          2                       Locked  <- COPY AE VALUES
> >    Manual AE with       3                       Locked
> >    settings from #2)    4                       Locked
> >                         5                       Use Manual AE
> >
> > Without a Lock the AEGC routine would keep adjusting its values for
> > requests 3 and 4. Request 5 will contain settings copied from Request
> > 2 so a small hiccup might happen. Of course, if the number of frames
> > required to get to a converged state is longer, the duration of the
> > possible AE adjustment period is longer, making the flickering more
> > visible ?
> >
> > Does this make sense ?
> >
> > The android.control.aeMode documentation also reports that:
> >
> > -----------------------------------------------------------------
> > Note that auto-white balance (AWB) and auto-focus (AF) behavior is
> > device dependent when AE is in OFF mode. To have consistent behavior
> > across different devices, it is recommended to either set AWB and AF to
> > OFF mode or lock AWB and AF before setting AE to OFF.See
> > android.control.awbMode,
> > android.control.afMode,android.control.awbLock, and
> > android.control.afTrigger for more details.
> > -----------------------------------------------------------------
> >
> > Suggesting that the AF AWB and AE locks might play a role on the
> interactions
> > between the three algorithms.
> >
> > Does this make sense and provide a bit more of context about what
> > AeLock could be used for ?
>
> I'm still a bit puzzled by the need for an AeLock control. I do
> understand that it's used in Android to switch to manual exposure
> without glitches, as detailed above. However, couldn't the same result
> be achieved by switching AeMode from on to off (thus switching to manual
> mode), without specifying manual values for the sensitivity, exposure
> and frame duration ? The camera would then keep the last values computed
> by the AEGC, which would be reported in metadata, and could be then used
> as a basis to set other manual values.
>

I agree with the sentiment that AeLock and AeEnable sound like the same
thing,
and the Raspberry Pi AGC will almost certainly be treating it the same to
achieve
what the user expects without glitching.


>
> This may be related to defining what happens to manual control values
> when running in auto mode. The camera will report computed values for
> the sensitivity, exposure time and frame duration in metadata, but
> should those values be implicitly copied to the request controls
> (effectively only, not really adding them to the Request instances) when
> the algorithm is switched off, or should the last value that was set
> manually be used in that case ?


In my opinion, the values returned in the metadata, particularly
controls::ExposureTime and controls::AnalogueGain should be the values
used for that frame.  So if the AE is switched off, we should still get
these
values returned in the Request, as the currently used values (which may
be set manually, or frozen to what the AE last selected).

I did have one bit of confusion in the paragraph above.  If we have manual
controls set, does that not imply that AE is not running in auto mode, so
the
two operations are mutually exclusive?

Regards,
Naush


> I think we need to clearly specify this,
> ideally in a consistent fashion across all controls. If we decide that
> the camera behaviour should be to implicitly copy the metadata value to
> the control value, then AeLock doesn't seem to be needed. Otherwise,
> switching seamlessly from auto to manual mode requires a mechanism
> similar to AeLock.
>
> Is there any other need for the AeLock control that I may have missed ?
>
> > >> That's the idea afaict.
> > >>
> > >> Supporting AeLock is optional, of course. Same for AeState, not all of
> > >> the states need to be supported. You can specify which ones are
> > >> supported in pipeline's ControlInfoMap.
> > >>
> > >>> Perhaps combining them to avoid redundancy makes sense?  Or do folks
> think they
> > >>> can they serve different purposes?
> > >>>
> > >>>> So:
> > >>>>
> > >>>> We set controls::AeLocked in the metadata according to the value
> that
> > >>>> we received in the corresponding control (which we now receive
> instead
> > >>>> of AeEnable), and
> > >>>>
> > >>>> We set controls::AeState in the metadata to AeStateConverged when
> > >>>> agcStatus->locked is true, otherwise to AeStateSearching. We should
> > >>>> probably bite the bullet and rename that "locked" field to
> > >>>> "converged", otherwise it's going to cause confusion for the rest of
> > >>>> eternity.
> > >>>
> > >>> Agree, I think we should rename "locked" to "converged" in the
> controller
> > >>> metadata struct.
> > >>>
> > >>>>>>                 libcameraMetadata_.set(controls::DigitalGain,
> agcStatus->digital_gain);
> > >>>>>>         }
> > >>>>>>
> > >>>>>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > >>>>>> index b47ea324..88a562a8 100644
> > >>>>>> --- a/src/ipa/rkisp1/rkisp1.cpp
> > >>>>>> +++ b/src/ipa/rkisp1/rkisp1.cpp
> > >>>>>> @@ -51,7 +51,7 @@ private:
> > >>>>>>                               const rkisp1_stat_buffer *stats);
> > >>>>>>
> > >>>>>>         void setControls(unsigned int frame);
> > >>>>>> -       void metadataReady(unsigned int frame, unsigned int
> aeState);
> > >>>>>> +       void metadataReady(unsigned int frame, int aeState);
> > >>>>>>
> > >>>>>>         std::map<unsigned int, FrameBuffer> buffers_;
> > >>>>>>         std::map<unsigned int, void *> buffersMemory_;
> > >>>>>> @@ -227,7 +227,7 @@ void IPARkISP1::updateStatistics(unsigned int
> frame,
> > >>>>>>                                  const rkisp1_stat_buffer *stats)
> > >>>>>>  {
> > >>>>>>         const rkisp1_cif_isp_stat *params = &stats->params;
> > >>>>>> -       unsigned int aeState = 0;
> > >>>>>> +       int aeState = controls::AeStateInactive;
> > >>>>>>
> > >>>>>>         if (stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP) {
> > >>>>>>                 const rkisp1_cif_isp_ae_stat *ae = &params->ae;
> > >>>>>> @@ -262,7 +262,9 @@ void IPARkISP1::updateStatistics(unsigned int
> frame,
> > >>>>>>                         setControls(frame + 1);
> > >>>>>>                 }
> > >>>>>>
> > >>>>>> -               aeState = fabs(factor - 1.0f) < 0.05f ? 2 : 1;
> > >>>>>> +               aeState = fabs(factor - 1.0f) < 0.05f ?
> > >>>>>> +                         controls::AeStateConverged :
> > >>>>>> +                         controls::AeStateSearching;
> > >>>>>>         }
> > >>>>>>
> > >>>>>>         metadataReady(frame, aeState);
> > >>>>>> @@ -281,12 +283,11 @@ void IPARkISP1::setControls(unsigned int
> frame)
> > >>>>>>         queueFrameAction.emit(frame, op);
> > >>>>>>  }
> > >>>>>>
> > >>>>>> -void IPARkISP1::metadataReady(unsigned int frame, unsigned int
> aeState)
> > >>>>>> +void IPARkISP1::metadataReady(unsigned int frame, int aeState)
> > >>>>>>  {
> > >>>>>>         ControlList ctrls(controls::controls);
> > >>>>>>
> > >>>>>> -       if (aeState)
> > >>>>>> -               ctrls.set(controls::AeLocked, aeState == 2);
> > >>>>>> +       ctrls.set(controls::AeState, aeState);
> > >>>>>>
> > >>>>>>         RkISP1Action op;
> > >>>>>>         op.op = ActionMetadata;
> > >>>>>> diff --git a/src/libcamera/control_ids.yaml
> b/src/libcamera/control_ids.yaml
> > >>>>>> index 9d4638ae..5717bc1f 100644
> > >>>>>> --- a/src/libcamera/control_ids.yaml
> > >>>>>> +++ b/src/libcamera/control_ids.yaml
> > >>>>>> @@ -14,16 +14,70 @@ controls:
> > >>>>>>
> > >>>>>>          \sa ExposureTime AnalogueGain
> > >>>>>>
> > >>>>>> -  - AeLocked:
> > >>>>>> +  - AeLock:
> > >>>>>>        type: bool
> > >>>>>>        description: |
> > >>>>>> -        Report the lock status of a running AE algorithm.
> > >>>>>> +        Control to lock the AE values.
> > >>>>>> +        When set to true, the AE algorithm is locked to its
> latest parameters,
> > >>>>>> +        and will not change exposure settings until set to false.
> > >>>>>> +        \sa AeState
> > >>>>>>
> > >>>>>> -        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.
> > >>>>>> +  - AeState:
> > >>>>>> +      type: int32_t
> > >>>>>> +      description: |
> > >>>>>> +        Control to report the current AE algorithm state.
> Enabling or disabling
> > >>>>>> +        AE (AeEnable) always resets the AeState 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.
> > >>>>>>
> > >>>>>> -        \sa AeEnable
> > >>>>>> +        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.
> > >>>>>> +
> > >>>>>> +        The state transitions mentioned below assume that
> AeEnable is on.
> > >>>>>> +
> > >>>>>> +        \sa AeLock
> > >>>>>> +      enum:
> > >>>>>> +        - name: AeStateInactive
> > >>>>>> +          value: 0
> > >>>>>> +          description: |
> > >>>>>> +            The AE algorithm is inactive.
> > >>>>>> +            If the camera initiates an AE scan, the state shall
> go to Searching.
> > >>>>>> +            If AeLock is on, the state shall go to Locked.
> > >>>>>> +        - 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.
> > >>>>>> +            If AeLock is on, the state shall go to Locked.
> > >>>>>> +        - name: AeStateConverged
> > >>>>>> +          value: 2
> > >>>>>> +          description: |
> > >>>>>> +            The AE algorithm has converged.
> > >>>>>> +            If the camera initiates an AE scan, the state shall
> go to Searching.
> > >>>>>> +            If AeLock is on, the state shall go to Locked.
> > >>>>>> +        - name: AeStateLocked
> > >>>>>> +          value: 3
> > >>>>>> +          description: |
> > >>>>>> +            The AE algorithm is locked.
> > >>>>>> +            If AeLock is off, the state can go to Searching,
> Converged, or
> > >>>>>> +            FlashRequired.
> > >>>>>> +        - name: AeStateFlashRequired
> > >>>>>> +          value: 4
> > >>>>>> +          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: 5
> > >>>>>> +          description: |
> > >>>>>> +            The AE algorithm has started a pre-capture metering
> session.
> > >>>>>> +            After the sequence is finished, the state shall go
> to Converged if
> > >>>>>> +            AeLock is off, and Locked if it is on.
> > >>>>>> +            \sa AePrecaptureTrigger
> > >>>>>>
> > >>>>>>    # AeMeteringMode needs further attention:
> > >>>>>>    # - Auto-generate max enum value.
> > >>>>>> @@ -477,36 +531,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
>
> --
> Regards,
>
> Laurent Pinchart
>

Patch
diff mbox series 

diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
index 1c1e802a..ad0132c0 100644
--- a/src/ipa/raspberrypi/raspberrypi.cpp
+++ b/src/ipa/raspberrypi/raspberrypi.cpp
@@ -468,7 +468,10 @@  void IPARPi::reportMetadata()
 
 	AgcStatus *agcStatus = rpiMetadata_.GetLocked<AgcStatus>("agc.status");
 	if (agcStatus) {
-		libcameraMetadata_.set(controls::AeLocked, agcStatus->locked);
+		libcameraMetadata_.set(controls::AeState,
+				       agcStatus->locked ?
+				       controls::AeStateLocked :
+				       controls::AeStateSearching);
 		libcameraMetadata_.set(controls::DigitalGain, agcStatus->digital_gain);
 	}
 
diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
index b47ea324..88a562a8 100644
--- a/src/ipa/rkisp1/rkisp1.cpp
+++ b/src/ipa/rkisp1/rkisp1.cpp
@@ -51,7 +51,7 @@  private:
 			      const rkisp1_stat_buffer *stats);
 
 	void setControls(unsigned int frame);
-	void metadataReady(unsigned int frame, unsigned int aeState);
+	void metadataReady(unsigned int frame, int aeState);
 
 	std::map<unsigned int, FrameBuffer> buffers_;
 	std::map<unsigned int, void *> buffersMemory_;
@@ -227,7 +227,7 @@  void IPARkISP1::updateStatistics(unsigned int frame,
 				 const rkisp1_stat_buffer *stats)
 {
 	const rkisp1_cif_isp_stat *params = &stats->params;
-	unsigned int aeState = 0;
+	int aeState = controls::AeStateInactive;
 
 	if (stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP) {
 		const rkisp1_cif_isp_ae_stat *ae = &params->ae;
@@ -262,7 +262,9 @@  void IPARkISP1::updateStatistics(unsigned int frame,
 			setControls(frame + 1);
 		}
 
-		aeState = fabs(factor - 1.0f) < 0.05f ? 2 : 1;
+		aeState = fabs(factor - 1.0f) < 0.05f ?
+			  controls::AeStateConverged :
+			  controls::AeStateSearching;
 	}
 
 	metadataReady(frame, aeState);
@@ -281,12 +283,11 @@  void IPARkISP1::setControls(unsigned int frame)
 	queueFrameAction.emit(frame, op);
 }
 
-void IPARkISP1::metadataReady(unsigned int frame, unsigned int aeState)
+void IPARkISP1::metadataReady(unsigned int frame, int aeState)
 {
 	ControlList ctrls(controls::controls);
 
-	if (aeState)
-		ctrls.set(controls::AeLocked, aeState == 2);
+	ctrls.set(controls::AeState, aeState);
 
 	RkISP1Action op;
 	op.op = ActionMetadata;
diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
index 9d4638ae..5717bc1f 100644
--- a/src/libcamera/control_ids.yaml
+++ b/src/libcamera/control_ids.yaml
@@ -14,16 +14,70 @@  controls:
 
         \sa ExposureTime AnalogueGain
 
-  - AeLocked:
+  - AeLock:
       type: bool
       description: |
-        Report the lock status of a running AE algorithm.
+        Control to lock the AE values.
+        When set to true, the AE algorithm is locked to its latest parameters,
+        and will not change exposure settings until set to false.
+        \sa AeState
 
-        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.
+  - AeState:
+      type: int32_t
+      description: |
+        Control to report the current AE algorithm state. Enabling or disabling
+        AE (AeEnable) always resets the AeState 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.
 
-        \sa AeEnable
+        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.
+
+        The state transitions mentioned below assume that AeEnable is on.
+
+        \sa AeLock
+      enum:
+        - name: AeStateInactive
+          value: 0
+          description: |
+            The AE algorithm is inactive.
+            If the camera initiates an AE scan, the state shall go to Searching.
+            If AeLock is on, the state shall go to Locked.
+        - 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.
+            If AeLock is on, the state shall go to Locked.
+        - name: AeStateConverged
+          value: 2
+          description: |
+            The AE algorithm has converged.
+            If the camera initiates an AE scan, the state shall go to Searching.
+            If AeLock is on, the state shall go to Locked.
+        - name: AeStateLocked
+          value: 3
+          description: |
+            The AE algorithm is locked.
+            If AeLock is off, the state can go to Searching, Converged, or
+            FlashRequired.
+        - name: AeStateFlashRequired
+          value: 4
+          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: 5
+          description: |
+            The AE algorithm has started a pre-capture metering session.
+            After the sequence is finished, the state shall go to Converged if
+            AeLock is off, and Locked if it is on.
+            \sa AePrecaptureTrigger
 
   # AeMeteringMode needs further attention:
   # - Auto-generate max enum value.
@@ -477,36 +531,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