[libcamera-devel] [PATCH 4/9] fixup: Expand AeState documentation

[Jacopo Mondi]

Hi Paul

On Thu, Aug 04, 2022 at 12:04:10AM +0900, paul.elder at ideasonboard.com wrote:
> Hi Jacopo,
>
> On Fri, Jul 01, 2022 at 05:46:56PM +0200, Jacopo Mondi wrote:
> > The AeState control is tricky, it describes the state of the whole AEGC
> > block which is independently controlled by ExposureTimeMode and
> > AnalogueGainMode.
> >
> > Try to expand the documentation.
> >
> > Use "AEGC" when referring to the algorithm.
> >
> > Maybe the control should be named AEGCState ?
>
> It's uuuugly.
>

It is !

> But also maybe more correct. idk I'm neutral maybe people that have a
> bigger preference can do the bikeshedding :)
>

Let's see if we get more comments, I'll keep it as it is for now

> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/libcamera/control_ids.yaml | 64 ++++++++++++++++++++++++----------
> >  1 file changed, 46 insertions(+), 18 deletions(-)
> >
> > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> > index bb5eeb1507a9..d50df8bcad28 100644
> > --- a/src/libcamera/control_ids.yaml
> > +++ b/src/libcamera/control_ids.yaml
> > @@ -10,43 +10,71 @@ controls:
> >    - AeState:
> >        type: int32_t
> >        description: |
> > -        Control to report the AE algorithm state associated with the capture
> > -        result.
> > +        Control to report the AEGC algorithm state.
> >
> > -        The state is still reported even if ExposureTimeMode or
> > -        AnalogueGainMode is set to Manual.
> > +        The AEGC algorithm computes the exposure time and the analogue gain
> > +        values to be applied to the image sensor.
> > +
> > +        The AEGC algorithm behaviour is controlled by the ExposureTimeMode and
> > +        AnalogueGainMode controls, which allow applications to decide how
> > +        the exposure time and gain are computed, in Auto or Manual mode,
> > +        independently one from the other.
>
> s/one from the other/from one another/
>

Thanks :)

> > +
> > +        The AeState control reports the AEGC algorithm state through a single
> > +        value and describes it a single computation block which computes
>
> s/it //
>

I actually meant "describes it as a single computation block"

Do you think it's correct ?

> > +        both the exposure time and the analogue gain values.
> > +
> > +        When both the exposure time and analogue gain values are configured to
> > +        be in Manual mode, the AEGC algorithm is quiescent and does not actively
> > +        compute any value and the AeState control will report AeStateIdle.
> > +
> > +        When at least the exposure time or analogue gain are configured to be
> > +        computed by the AEGC algorithm, the AeState control will report if the
> > +        algorithm has converged to stable values for any of the controls set
>
> I would've s/any/all/, but now I'm wondering if it's even possible for
> AEGC with both E and G set to auto to have only one converged but not
> the other.

Good question

>
> Although maybe s/any/all/ is proper, because this is *one* state to
> describe *all* of AEGC, so I think if at least one is still converging
> it should be converging, and only if both are converged then it should
> be converged. Even if it's not really possible, that's probably the more
> correct description.
>

I think s/any/all as you suggested helps clarify it

> > +        to be computed in Auto mode.
> >
> > -        \sa AnalogueGain
> >          \sa AnalogueGainMode
> > -        \sa ExposureTime
> >          \sa ExposureTimeMode
> >
> >        enum:
> >          - name: AeStateIdle
> >            value: 0
> >            description: |
> > -            The AE algorithm is inactive.
> > +            The AEGC algorithm is inactive.
> > +
> > +            This state is returned when both AnalogueGainMode and
> > +            ExposureTimeMode are set to Manual and the algorithm is not
> > +            actively computing any value.
>
> Do we need to explicitly state an exception for if only one is
> manual-able?  Although due to the constraints that we've defined, if
> only one is manual-able then both are auto-able. So maybe it's implicit?
>

If I got your question right you suggest that only one might be
manual-able and the other is auto-only, hence the state will never be
reported as idle ?

Isn't this desired ? The AeState control will report the state of the
auto control, either if it is searching or it has converged, while the
other will be operated in manual mode.

I might have missed what constraints you are referring to here.

> >
> > -            This state should be returned if both AnalogueGainMode and
> > -            ExposureTimeMode are set to manual (or one, if the camera only
> > -            supports one of the two controls).
> > +            When ExposureTimeMode or AnalogueGainMode are set to Auto mode, the
> > +            AEGC algorithm might spontaneously initiate a new scan, in which
> > +            case the AeState control is moved to AeStateSearching.
> >          - name: AeStateSearching
> >            value: 1
> >            description: |
> > -            The AE algorithm has not converged yet.
> > +            The AEGC algorithm is actively computing new values, for either the
> > +            exposure time or the analogue gain, but has not converged to a
> > +            stable result yet.
> > +
> > +            This state is returned if at least one of AnalogueGainMode
> > +            or ExposureTimeMode is set to auto and the algorithm hasn't
> > +            converged yet.
> >
> > -            This state should be returned if at least one of AnalogueGainMode
> > -            or ExposureTimeMode is set to auto, and the AE algorithm hasn't
> > -            converged yet. If the AE algorithm converges, the state shall go to
> > -            AeStateConverged.
> > +            The AEGC algorithm converges once stable values are computed for
> > +            any of the controls set to be computed in Auto mode.
> > +
> > +            Once the algorithm converges the state is moved to AeStateConverged.
> >          - name: AeStateConverged
> >            value: 2
> >            description: |
> >              The AE algorithm has converged.
>
> s/AE/AEGC/?
>

Thanks!

>
> Otherwise looks good.
>
>
> Thanks,
>
> Paul
>
>
> >
> > -            This state should be returned if at least one of AnalogueGainMode
> > -            or ExposureTimeMode is set to auto, and the AE algorithm has
> > -            converged.
> > +            This state is returned if at least one of AnalogueGainMode
> > +            or ExposureTimeMode is set to Auto, and the AEGC algorithm has
> > +            converged to stable value.
> > +
> > +            The AEGC algorithm might spontaneously re-initiate an AE scan, in
> > +            which case the state is moved to AeStateSearching.
> >
> >    # AeMeteringMode needs further attention:
> >    # - Auto-generate max enum value.
> > --
> > 2.36.1
> >