[libcamera-devel,v4,1/1] libcamera: controls: Controls for driving AF (autofocus) algorithms

15798 diff mbox series

This patch describes a series of controls that allow applications to
drive AF algorithms:

AfMode - manual, auto or continuous
AfRange - full, macro or normal
AfSpeed - fast or slow
AfMetering - how to choose where to measure focus
AfWindows - AF window locations
AfTrigger - start (trigger) an AF scan or cancel
AfPause - pause continuous AF
LensPosition - set or retrieve position of lens
AfState - reports whether scanning/success/failure
AfPauseState - reports whether continuous AF paused or not

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
---
 src/libcamera/control_ids.yaml | 345 +++++++++++++++++++++++++++------
 1 file changed, 285 insertions(+), 60 deletions(-)

Hi David,

On Thu, May 05, 2022 at 03:15:00PM +0100, David Plowman wrote:
> This patch describes a series of controls that allow applications to
> drive AF algorithms:
>
> AfMode - manual, auto or continuous
> AfRange - full, macro or normal
> AfSpeed - fast or slow
> AfMetering - how to choose where to measure focus
> AfWindows - AF window locations
> AfTrigger - start (trigger) an AF scan or cancel
> AfPause - pause continuous AF
> LensPosition - set or retrieve position of lens
> AfState - reports whether scanning/success/failure
> AfPauseState - reports whether continuous AF paused or not
>
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  src/libcamera/control_ids.yaml | 345 +++++++++++++++++++++++++++------
>  1 file changed, 285 insertions(+), 60 deletions(-)
>
> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> index 9d4638ae..5af1676e 100644
> --- a/src/libcamera/control_ids.yaml
> +++ b/src/libcamera/control_ids.yaml
> @@ -381,6 +381,291 @@ controls:
>          \todo Define how the sensor timestamp has to be used in the reprocessing
>          use case.
>
> +  - AfMode:
> +      type: int32_t
> +      description: |
> +        Control to set the mode of the AF (autofocus) algorithm. Applications
> +        are allowed to set a new mode, and to send additional controls for
> +        that new mode, in the same request.

nit: I find the:
"Applications are allowed to set a new mode, and to send additional
controls for that new mode, in the same request."

not necessary. This apply to all controls, doesn't it ? If that's the
case, I think that re-stating it here might do more harms than good as
one reading this would wonder why it is specified for this control and
not for other, and why is this different..


> +
> +        An implementation may choose not to implement all the modes.
> +
> +      enum:
> +        - name: AfModeManual
> +          value: 0
> +          description: |
> +            The AF algorithm is in manual mode. In this mode it will never
> +            perform any action nor move the lens of its own accord, but an
> +            application can specify the desired lens position using the
> +            LensPosition control.
> +
> +            In this mode the AfState will always report AfStateIdle.
> +        - name: AfModeAuto
> +          value: 1
> +          description: |
> +            The AF algorithm is in auto mode. This means that the algorithm
> +            will never move the lens or change state unless the AfTrigger
> +            control is used. The AfTrigger control can be used to initiate a
> +            focus scan, the results of which will be reported by AfState.
> +
> +            If the autofocus algorithm is moved from AfModeAuto to another
> +            mode while a scan is in progress, the scan is cancelled
> +            immediately, without waiting for the scan to finish.
> +
> +            When first entering this mode the AfState will report
> +            AfStateIdle. When a trigger control is sent, AfState will
> +            report AfStateScanning for a period before spontaneously
> +            changing to AfStateFocused or AfStateFailed, depending on
> +            the outcome of the scan. It will remain in this state until
> +            another scan is initiated by the AfTrigger control. If a scan is
> +            cancelled (without changing to another mode), AfState will return
> +            to AfStateIdle.
> +        - name: AfModeContinuous
> +          value: 2
> +          description: |
> +            The AF algorithm is in continuous mode. This means that the lens
> +            can re-start a scan spontaneously at any moment, without any user
> +            intervention. The AfState still reports whether the algorithm is
> +            currently scanning or not, though the application has no ability
> +            to initiate or cancel scans (though it can "pause" them), nor to
> +            move the lens for itself.

I would create a paragraph to describe the Pause thing, and maybe
remove "(though it can "pause" them") from the above phrase.

I would add
               Applications can pause the AF algorithm from
               continuously scanning by using the AfPause control to
               realize fixed-focus video recording or image capture.

(feel free to suggest improvements)

> +
> +            When set to AfModeContinuous, the system will immediately initiate a
> +            scan so AfState will report AfStateScanning, and will settle on one
> +            of AfStateFocused or AfStateFailed, depending on the scan result.
> +
> +  - AfRange:
> +      type: int32_t
> +      description: |
> +        Control to set the range of focus distances that is scanned. An
> +        implementation may choose not to implement all the options here.
> +      enum:
> +        - name: AfRangeNormal
> +          value: 0
> +          description: |
> +            A wide range of focus distances is scanned, all the way from
> +            infinity down to close distances, though depending on the
> +            implementation, possibly not including the very closest macro
> +            positions.
> +        - name: AfRangeMacro
> +          value: 1
> +          description: Only close distances are scanned.
> +        - name: AfRangeFull
> +          value: 2
> +          description: |
> +            The full range of focus distances is scanned just as with
> +            AfRangeNormal but this time including the very closest macro
> +            positions.
> +
> +  - AfSpeed:
> +      type: int32_t
> +      description: |
> +        Control that determines whether the AF algorithm is to move the lens
> +        as quickly as possible or more steadily. For example, during video
> +        recording it may be desirable not to move the lens too abruptly, but
> +        when in a preview mode (waiting for a still capture) it may be
> +        helpful to move the lens as quickly as is reasonably possible.
> +      enum:
> +        - name: AfSpeedNormal
> +          value: 0
> +          description: Move the lens at its usual speed.
> +        - name: AfSpeedFast
> +          value: 1
> +          description: Move the lens more quickly.
> +
> +  - AfMetering:
> +      type: int32_t
> +      description: |
> +        Instruct the AF algorithm how it should decide which parts of the image
> +        should be used to measure focus.
> +      enum:
> +        - name: AfMeteringAuto
> +          value: 0
> +          description: The AF algorithm should decide for itself where it will
> +            measure focus.
> +        - name: AfMeteringWindows
> +          value: 1
> +          description: The AF algorithm should use the rectangles defined by
> +            the AfWindows control to measure focus.

If no AfWindows control is specified the focus area will cover the
whole visible picture ? Is it worth mentioning ?

> +
> +  - AfWindows:
> +      type: Rectangle
> +      description: |
> +        Sets the focus windows used by the AF algorithm when AfMetering is set
> +        to AfMeteringWindows. The units used are pixels within the rectangle
> +        returned by the ScalerCropMaximum property. Applications should ignore
> +        any offsets reported for the ScalerCropMaximum rectangle as we are only
> +        defining locations within it.

I think you mean that the ScalerCropMaximum top-left corner distance
from the active pixel array doesn't matter, but isn't this implied by
the fact AfWindows is defined within ScalerCropMaximum itself ?

I like the idea of using ScalerCropMaximum as it clearly defines what
is the visible area and makes sure that (unless any crop is specified)
no window can fall out of the sensor's field of view. I still wonder
what happens if the ph doesn't register any ScalerCropMaximum but I
think it's fair to assume that if AfWindows is supported, than
ScalerCropMaximum shall be reported.

> +
> +        In order to be activated, a rectangle must be programmed with non-zero
> +        width and height. The rectangles are also implicitly bounded to the
> +        ScalerCropMaximum rectangle size. If no pixels remain after this
> +        operation for any of the defined windows, then the behaviour is platform
> +        dependent.

Does this mean to clarify what happens if an app specifies rectangles
completely -outside- of the ScalerCropMaximum rectangle ? Shouldn't we
just ignore them instead of deferring to a platform-specific
behaviour?

Also Isn't it more clear to describe this by pointing out that only
intersections are considered ? The below, with a small graphical
example, might be appropritate in your opinion ?


        In order to be activated, a rectangle must be programmed with non-zero
        width and height. The rectangles are also implicitly bounded to the
        ScalerCropMaximum rectangle size and only intersections of the
        two rectangles are considered by the AF algorithm.

        As an example

        + SCM ---------------+
        |                    |
        |                    |
        |                    |
        |         +- AW ---------+
        |         |          |   |
        |         |          |   |
        |         |          |   |
        |         |          |   |
        |         |          |   |
        |         +--------------+
        |                    |
        +--------------------+

        Will result in

        + SCM ---------------+
        |                    |
        |                    |
        |                    |
        |         +- AW -----+
        |         |          |
        |         |          |
        |         |          |
        |         |          |
        |         |          |
        |         +----------+
        |                    |
        +--------------------+

        While if any of the AfWindows rectangles lies completely
        outside of the ScalerCropMaximum rectangle, [it is ignored by
        the AF algorithm./the behaviour is platform dependent.]

> +
> +        On platforms that support the ScalerCrop control (for implementing
> +        digital zoom, for example), no automatic recalculation or adjustment of
> +        AF windows is performed internally if the ScalerCrop is changed.

I would then add:

           If any window lies outside of the output image after a new
           cropping rectangle has been applied, it is up to
           application to re-calculate the correct focusing windows.

Or it isn't necessary in your opinion ?

> +
> +        The details of how the windows are used are platform dependent. We note
> +        that when there is more than one AF window, a typical implementation
> +        might find the optimal focus position for each one and finally select
> +        the window where the focal distance for the objects shown in that part
> +        of the image are closest to the camera.
> +
> +        size: [n]

Wrong indentation ? The control will be created as a scalar control
not as a Span<> one

from $build/include/libcamera/control_ids.h:

        extern const Control<Rectangle> AfWindows;

> +
> +  - AfTrigger:
> +      type: int32_t
> +      description: |
> +        This control starts an autofocus scan when AfMode is set to AfModeAuto,
> +        and can also be used to terminate a scan early.
> +
> +        It is ignored if AfMode is set to AfModeManual or AfModeContinuous.
> +
> +      enum:
> +        - name: AfTriggerStart
> +          value: 0
> +          description: Start an AF scan. Ignored if a scan is in progress.
> +        - name: AfTriggerCancel
> +          value: 1
> +          description: Cancel an AF scan. This does not cause the lens to move
> +            anywhere else. Ignored if no scan is in progress.
> +
> +  - AfPause:
> +      type: int32_t
> +      description: |
> +        This control has no effect except when in continuous autofocus mode
> +        (AfModeContinuous). It can be used to pause any lens movements while
> +        (for example) images are captured. The algorithm remains inactive
> +        until it is instructed to resume.
> +
> +      enum:
> +        - name: AfPauseImmediate
> +          value: 0
> +          description: |
> +            Pause the continuous autofocus algorithm immediately, whether or not
> +            any kind of scan is underway. AfPauseState will subsequently report
> +            AfPauseStatePaused. AfState may report any of AfStateScanning,
> +            AfStateFocused or AfStateFailed, depending on the algorithm's state
> +            when it received this control.
> +        - name: AfPauseDeferred
> +          value: 1
> +          description: |
> +            This is similar to AfPauseImmediate, and if the AfState is currently
> +            reporting AfStateFocused or AfStateFailed it will remain in that
> +            state and AfPauseState will report AfPauseStatePaused.
> +
> +            However, if the algorithm is scanning (AfStateScanning),
> +            AfPauseState will report AfPauseStatePausing until the scan is
> +            finished, at which point AfState will report one of AfStateFocused
> +            or AfStateFailed, and AfPauseState will change to
> +            AfPauseStatePaused.
> +
> +        - name: AfPauseResume
> +          value: 2
> +          description: |
> +            Resume continuous autofocus operation. The algorithm starts again
> +            from exactly where it left off, and AfPauseState will report
> +            AfPauseStateRunning.
> +
> +  - LensPosition:
> +      type: float
> +      description: |
> +        Acts as a control to instruct the lens to move to a particular position
> +        and also reports back the position of the lens for each frame.

Reported unconditionally or only if in Manual mode ?

> +
> +        The LensPosition control is ignored unless the AfMode is set to
> +        AfModeManual.
> +
> +        The units are dioptres divided by the hyperfocal distance. Non-integer
> +        values are permitted. For example:
> +        0 moves the lens to infinity.
> +        0.5 moves the lens to twice the hyperfocal distance.
> +        1 moves the lens to the hyperfocal position.
> +        And larger values will focus the lens ever closer.

Can we add a

           \todo Define a property to report the Hyperfocal distance of
           calibrated lenses.

> +
> +  - AfState:
> +      type: int32_t
> +      description: |
> +        Reports the current state of the AF algorithm in conjunction with the
> +        reported AfMode value and (in continuous AF mode) the AfPauseState
> +        value.  The possible state changes are described below, though we note
                 ^ double space

> +        the following state transitions that occur when the AfMode is changed.
> +
> +        If the AfMode is set to AfModeManual then the AfState will report
> +        AfStateIdle. This action on its own does not cause the lens to move; the
                        ^ What action ?

The AfMode being moved to AfModeManual ? If that's the case I would
omit the second part of the phrase (from "This action.." on) as the
fact the lens doesn't move is specified by the AfModeManual definition
and it is unrelated to the reported state.

I would instead say that AfState will -always- report Idle when in
Manual mode, even if the lens is moved.

           Whan AfMode is set to AfModeManual then AfState will always report
           AfStateIdle.

> +        LensPosition control must subsequently be used to achieve this.
> +
> +        If the AfMode is set to AfModeAuto then the AfState will report
> +        AfStateIdle. However, if AfModeAuto and AfTriggerStart are sent together
> +        then AfState will omit AfStateIdle and move straight to AfStateScanning
> +        (and start a scan).
> +
> +        If the AfMode is set to AfModeContinuous then the AfState will initially
> +        report AfStateScanning.
> +
> +      enum:
> +        - name: AfStateIdle
> +          value: 0
> +          description: |
> +            The AF algorithm is in manual mode (AfModeManual) or in auto mode
> +            (AfModeAuto) and a scan has not yet been triggered, or an
> +            in-progress scan was cancelled.
> +        - name: AfStateScanning
> +          value: 1
> +          description: |
> +            The AF algorithm is in auto mode (AfModeAuto), and a scan has been
> +            started using the AfTrigger control. The scan can be cancelled by
> +            sending AfTriggerCancel at which point the algorithm will either
> +            move back to AfStateIdle or, if the scan actually completes
> +            before the cancel request is processed, to one of
> +            AfStateFocused or AfStateFailed.
> +
> +            Alternatively the AF algorithm could be in continuous mode
> +            (AfModeContinuous) at which point it may enter this state
> +            spontaneously whenever it determines that a rescan is needed.
> +        - name: AfStateFocused
> +          value: 2
> +          description: |
> +            The AF algorithm is in auto (AfModeAuto) or continuous
> +            (AfModeContinuous) mode and a scan has completed with the result
> +            that the algorithm believes the image is now in focus.
> +        - name: AfStateFailed
> +          value: 3
> +          description: |
> +            The AF algorithm is in auto (AfModeAuto) or continuous
> +            (AfModeContinuous) mode and a scan has completed with the result
> +            that the algorithm did not find a good focus position.
> +
> +  - AfPauseState:
> +      type: int32_t
> +      description: |
> +        Only applicable in continuous (AfModeContinuous) mode, this reports
> +        whether the algorithm is currently running, paused or pausing (that is,
> +        will pause as soon as any in-progress scan completes).
> +
> +        Any change to AfMode will cause AfPauseStateRunning to be reported.
> +
> +      enum:
> +        - name: AfPauseStateRunning
> +          value: 0
> +          description: |
> +            Continuous AF is running and the algorithm may restart a scan
> +            spontaneously.
> +        - name: AfPauseStatePausing
> +          value: 1
> +          description: |
> +            Continuous AF has been sent an AfPauseDeferred control, and will
> +            pause as soon as any in-progress scan completes (and then report
> +            AfPauseStatePaused). No new scans will be start spontaneously until
> +            the AfPauseResume control is sent.
> +        - name: AfPauseStatePaused
> +          value: 2
> +          description: |
> +            Continuous AF is paused. No further state changes or lens movements
> +            will occur until the AfPauseResume control is sent.
> +

Great, just a minor refinements and from my side we're good to go.
If you don't want to send a new version and if anyone has not other
comments, feel free to reply my comments here and I can change things
when applying if there's nothing too controversial.

Thanks, nice work!

>    # ----------------------------------------------------------------------------
>    # Draft controls section
>
> @@ -406,27 +691,6 @@ controls:
>              The camera will cancel any active or completed metering sequence.
>              The AE algorithm is reset to its initial state.
>
> -  - AfTrigger:
> -      type: int32_t
> -      draft: true
> -      description: |
> -       Control for AF trigger. Currently identical to
> -       ANDROID_CONTROL_AF_TRIGGER.
> -
> -        Whether the camera device will trigger autofocus for this request.
> -      enum:
> -        - name: AfTriggerIdle
> -          value: 0
> -          description: The trigger is idle.
> -        - name: AfTriggerStart
> -          value: 1
> -          description: The AF routine is started by the camera.
> -        - name: AfTriggerCancel
> -          value: 2
> -          description: |
> -            The camera will cancel any active trigger and the AF routine is
> -            reset to its initial state.
> -
>    - NoiseReductionMode:
>        type: int32_t
>        draft: true
> @@ -507,45 +771,6 @@ controls:
>              The AE algorithm has started a pre-capture metering session.
>              \sa AePrecaptureTrigger
>
> -  - AfState:
> -      type: int32_t
> -      draft: true
> -      description: |
> -       Control to report the current AF algorithm state. Currently identical to
> -       ANDROID_CONTROL_AF_STATE.
> -
> -        Current state of the AF algorithm.
> -      enum:
> -        - name: AfStateInactive
> -          value: 0
> -          description: The AF algorithm is inactive.
> -        - name: AfStatePassiveScan
> -          value: 1
> -          description: |
> -            AF is performing a passive scan of the scene in continuous
> -            auto-focus mode.
> -        - name: AfStatePassiveFocused
> -          value: 2
> -          description: |
> -            AF believes the scene is in focus, but might restart scanning.
> -        - name: AfStateActiveScan
> -          value: 3
> -          description: |
> -            AF is performing a scan triggered by an AF trigger request.
> -            \sa AfTrigger
> -        - name: AfStateFocusedLock
> -          value: 4
> -          description: |
> -            AF believes has focused correctly and has locked focus.
> -        - name: AfStateNotFocusedLock
> -          value: 5
> -          description: |
> -            AF has not been able to focus and has locked.
> -        - name: AfStatePassiveUnfocused
> -          value: 6
> -          description: |
> -            AF has completed a passive scan without finding focus.
> -
>    - AwbState:
>        type: int32_t
>        draft: true
> --
> 2.30.2
>

Hi Jacopo

Thanks for the comments. Let me zip through them, and then I'll get
another update ready!

On Fri, 6 May 2022 at 09:40, Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hi David,
>
> On Thu, May 05, 2022 at 03:15:00PM +0100, David Plowman wrote:
> > This patch describes a series of controls that allow applications to
> > drive AF algorithms:
> >
> > AfMode - manual, auto or continuous
> > AfRange - full, macro or normal
> > AfSpeed - fast or slow
> > AfMetering - how to choose where to measure focus
> > AfWindows - AF window locations
> > AfTrigger - start (trigger) an AF scan or cancel
> > AfPause - pause continuous AF
> > LensPosition - set or retrieve position of lens
> > AfState - reports whether scanning/success/failure
> > AfPauseState - reports whether continuous AF paused or not
> >
> > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > ---
> >  src/libcamera/control_ids.yaml | 345 +++++++++++++++++++++++++++------
> >  1 file changed, 285 insertions(+), 60 deletions(-)
> >
> > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> > index 9d4638ae..5af1676e 100644
> > --- a/src/libcamera/control_ids.yaml
> > +++ b/src/libcamera/control_ids.yaml
> > @@ -381,6 +381,291 @@ controls:
> >          \todo Define how the sensor timestamp has to be used in the reprocessing
> >          use case.
> >
> > +  - AfMode:
> > +      type: int32_t
> > +      description: |
> > +        Control to set the mode of the AF (autofocus) algorithm. Applications
> > +        are allowed to set a new mode, and to send additional controls for
> > +        that new mode, in the same request.
>
> nit: I find the:
> "Applications are allowed to set a new mode, and to send additional
> controls for that new mode, in the same request."
>
> not necessary. This apply to all controls, doesn't it ? If that's the
> case, I think that re-stating it here might do more harms than good as
> one reading this would wonder why it is specified for this control and
> not for other, and why is this different..

Sure, I can remove that!

>
>
> > +
> > +        An implementation may choose not to implement all the modes.
> > +
> > +      enum:
> > +        - name: AfModeManual
> > +          value: 0
> > +          description: |
> > +            The AF algorithm is in manual mode. In this mode it will never
> > +            perform any action nor move the lens of its own accord, but an
> > +            application can specify the desired lens position using the
> > +            LensPosition control.
> > +
> > +            In this mode the AfState will always report AfStateIdle.
> > +        - name: AfModeAuto
> > +          value: 1
> > +          description: |
> > +            The AF algorithm is in auto mode. This means that the algorithm
> > +            will never move the lens or change state unless the AfTrigger
> > +            control is used. The AfTrigger control can be used to initiate a
> > +            focus scan, the results of which will be reported by AfState.
> > +
> > +            If the autofocus algorithm is moved from AfModeAuto to another
> > +            mode while a scan is in progress, the scan is cancelled
> > +            immediately, without waiting for the scan to finish.
> > +
> > +            When first entering this mode the AfState will report
> > +            AfStateIdle. When a trigger control is sent, AfState will
> > +            report AfStateScanning for a period before spontaneously
> > +            changing to AfStateFocused or AfStateFailed, depending on
> > +            the outcome of the scan. It will remain in this state until
> > +            another scan is initiated by the AfTrigger control. If a scan is
> > +            cancelled (without changing to another mode), AfState will return
> > +            to AfStateIdle.
> > +        - name: AfModeContinuous
> > +          value: 2
> > +          description: |
> > +            The AF algorithm is in continuous mode. This means that the lens
> > +            can re-start a scan spontaneously at any moment, without any user
> > +            intervention. The AfState still reports whether the algorithm is
> > +            currently scanning or not, though the application has no ability
> > +            to initiate or cancel scans (though it can "pause" them), nor to
> > +            move the lens for itself.
>
> I would create a paragraph to describe the Pause thing, and maybe
> remove "(though it can "pause" them") from the above phrase.
>
> I would add
>                Applications can pause the AF algorithm from
>                continuously scanning by using the AfPause control to
>                realize fixed-focus video recording or image capture.
>
> (feel free to suggest improvements)

Will do, I can add something to that effect.

>
> > +
> > +            When set to AfModeContinuous, the system will immediately initiate a
> > +            scan so AfState will report AfStateScanning, and will settle on one
> > +            of AfStateFocused or AfStateFailed, depending on the scan result.
> > +
> > +  - AfRange:
> > +      type: int32_t
> > +      description: |
> > +        Control to set the range of focus distances that is scanned. An
> > +        implementation may choose not to implement all the options here.
> > +      enum:
> > +        - name: AfRangeNormal
> > +          value: 0
> > +          description: |
> > +            A wide range of focus distances is scanned, all the way from
> > +            infinity down to close distances, though depending on the
> > +            implementation, possibly not including the very closest macro
> > +            positions.
> > +        - name: AfRangeMacro
> > +          value: 1
> > +          description: Only close distances are scanned.
> > +        - name: AfRangeFull
> > +          value: 2
> > +          description: |
> > +            The full range of focus distances is scanned just as with
> > +            AfRangeNormal but this time including the very closest macro
> > +            positions.
> > +
> > +  - AfSpeed:
> > +      type: int32_t
> > +      description: |
> > +        Control that determines whether the AF algorithm is to move the lens
> > +        as quickly as possible or more steadily. For example, during video
> > +        recording it may be desirable not to move the lens too abruptly, but
> > +        when in a preview mode (waiting for a still capture) it may be
> > +        helpful to move the lens as quickly as is reasonably possible.
> > +      enum:
> > +        - name: AfSpeedNormal
> > +          value: 0
> > +          description: Move the lens at its usual speed.
> > +        - name: AfSpeedFast
> > +          value: 1
> > +          description: Move the lens more quickly.
> > +
> > +  - AfMetering:
> > +      type: int32_t
> > +      description: |
> > +        Instruct the AF algorithm how it should decide which parts of the image
> > +        should be used to measure focus.
> > +      enum:
> > +        - name: AfMeteringAuto
> > +          value: 0
> > +          description: The AF algorithm should decide for itself where it will
> > +            measure focus.
> > +        - name: AfMeteringWindows
> > +          value: 1
> > +          description: The AF algorithm should use the rectangles defined by
> > +            the AfWindows control to measure focus.
>
> If no AfWindows control is specified the focus area will cover the
> whole visible picture ? Is it worth mentioning ?

Or maybe it would just behave as if it was still "auto"? Perhaps it's
a "platform dependent" thing? I don't have any particularly strong
feelings on this, how do folks feel about specifying a particular
behaviour?

>
> > +
> > +  - AfWindows:
> > +      type: Rectangle
> > +      description: |
> > +        Sets the focus windows used by the AF algorithm when AfMetering is set
> > +        to AfMeteringWindows. The units used are pixels within the rectangle
> > +        returned by the ScalerCropMaximum property. Applications should ignore
> > +        any offsets reported for the ScalerCropMaximum rectangle as we are only
> > +        defining locations within it.
>
> I think you mean that the ScalerCropMaximum top-left corner distance
> from the active pixel array doesn't matter, but isn't this implied by
> the fact AfWindows is defined within ScalerCropMaximum itself ?

I agree... but I wanted to be really clear, I could imagine myself
having the "I assume they mean this, but is that really true?" kind of
worry. Maybe I just worry too much.

>
> I like the idea of using ScalerCropMaximum as it clearly defines what
> is the visible area and makes sure that (unless any crop is specified)
> no window can fall out of the sensor's field of view. I still wonder
> what happens if the ph doesn't register any ScalerCropMaximum but I
> think it's fair to assume that if AfWindows is supported, than
> ScalerCropMaximum shall be reported.
>
> > +
> > +        In order to be activated, a rectangle must be programmed with non-zero
> > +        width and height. The rectangles are also implicitly bounded to the
> > +        ScalerCropMaximum rectangle size. If no pixels remain after this
> > +        operation for any of the defined windows, then the behaviour is platform
> > +        dependent.
>
> Does this mean to clarify what happens if an app specifies rectangles
> completely -outside- of the ScalerCropMaximum rectangle ? Shouldn't we
> just ignore them instead of deferring to a platform-specific
> behaviour?

I guess I wanted to cover the case when *all* the rectangles lie
outside. So we are ignoring them... it's just we have to decide what
to do if we ignore *everything*! We should perhaps make this behaviour
the same as the behaviour we have chosen (above) when there are no
windows at all.

>
> Also Isn't it more clear to describe this by pointing out that only
> intersections are considered ? The below, with a small graphical
> example, might be appropritate in your opinion ?
>
>
>         In order to be activated, a rectangle must be programmed with non-zero
>         width and height. The rectangles are also implicitly bounded to the
>         ScalerCropMaximum rectangle size and only intersections of the
>         two rectangles are considered by the AF algorithm.

I can go with that. TBH, maybe I should just use "intersected with"
instead of "implicitly bounded to".

>
>         As an example
>
>         + SCM ---------------+
>         |                    |
>         |                    |
>         |                    |
>         |         +- AW ---------+
>         |         |          |   |
>         |         |          |   |
>         |         |          |   |
>         |         |          |   |
>         |         |          |   |
>         |         +--------------+
>         |                    |
>         +--------------------+
>
>         Will result in
>
>         + SCM ---------------+
>         |                    |
>         |                    |
>         |                    |
>         |         +- AW -----+
>         |         |          |
>         |         |          |
>         |         |          |
>         |         |          |
>         |         |          |
>         |         +----------+
>         |                    |
>         +--------------------+
>
>         While if any of the AfWindows rectangles lies completely
>         outside of the ScalerCropMaximum rectangle, [it is ignored by
>         the AF algorithm./the behaviour is platform dependent.]

I think describing this implicitly as intersection is probably clear
enough. I'm not sure I can bring myself to draw lots of ASCII art...

>
> > +
> > +        On platforms that support the ScalerCrop control (for implementing
> > +        digital zoom, for example), no automatic recalculation or adjustment of
> > +        AF windows is performed internally if the ScalerCrop is changed.
>
> I would then add:
>
>            If any window lies outside of the output image after a new
>            cropping rectangle has been applied, it is up to
>            application to re-calculate the correct focusing windows.
>
> Or it isn't necessary in your opinion ?

I think adding something like that is quite helpful.

>
> > +
> > +        The details of how the windows are used are platform dependent. We note
> > +        that when there is more than one AF window, a typical implementation
> > +        might find the optimal focus position for each one and finally select
> > +        the window where the focal distance for the objects shown in that part
> > +        of the image are closest to the camera.
> > +
> > +        size: [n]
>
> Wrong indentation ? The control will be created as a scalar control
> not as a Span<> one
>
> from $build/include/libcamera/control_ids.h:
>
>         extern const Control<Rectangle> AfWindows;

Thanks, will check that!

>
> > +
> > +  - AfTrigger:
> > +      type: int32_t
> > +      description: |
> > +        This control starts an autofocus scan when AfMode is set to AfModeAuto,
> > +        and can also be used to terminate a scan early.
> > +
> > +        It is ignored if AfMode is set to AfModeManual or AfModeContinuous.
> > +
> > +      enum:
> > +        - name: AfTriggerStart
> > +          value: 0
> > +          description: Start an AF scan. Ignored if a scan is in progress.
> > +        - name: AfTriggerCancel
> > +          value: 1
> > +          description: Cancel an AF scan. This does not cause the lens to move
> > +            anywhere else. Ignored if no scan is in progress.
> > +
> > +  - AfPause:
> > +      type: int32_t
> > +      description: |
> > +        This control has no effect except when in continuous autofocus mode
> > +        (AfModeContinuous). It can be used to pause any lens movements while
> > +        (for example) images are captured. The algorithm remains inactive
> > +        until it is instructed to resume.
> > +
> > +      enum:
> > +        - name: AfPauseImmediate
> > +          value: 0
> > +          description: |
> > +            Pause the continuous autofocus algorithm immediately, whether or not
> > +            any kind of scan is underway. AfPauseState will subsequently report
> > +            AfPauseStatePaused. AfState may report any of AfStateScanning,
> > +            AfStateFocused or AfStateFailed, depending on the algorithm's state
> > +            when it received this control.
> > +        - name: AfPauseDeferred
> > +          value: 1
> > +          description: |
> > +            This is similar to AfPauseImmediate, and if the AfState is currently
> > +            reporting AfStateFocused or AfStateFailed it will remain in that
> > +            state and AfPauseState will report AfPauseStatePaused.
> > +
> > +            However, if the algorithm is scanning (AfStateScanning),
> > +            AfPauseState will report AfPauseStatePausing until the scan is
> > +            finished, at which point AfState will report one of AfStateFocused
> > +            or AfStateFailed, and AfPauseState will change to
> > +            AfPauseStatePaused.
> > +
> > +        - name: AfPauseResume
> > +          value: 2
> > +          description: |
> > +            Resume continuous autofocus operation. The algorithm starts again
> > +            from exactly where it left off, and AfPauseState will report
> > +            AfPauseStateRunning.
> > +
> > +  - LensPosition:
> > +      type: float
> > +      description: |
> > +        Acts as a control to instruct the lens to move to a particular position
> > +        and also reports back the position of the lens for each frame.
>
> Reported unconditionally or only if in Manual mode ?

I think reported unconditionally so that you can always see where it
is. I'll make that clearer.

>
> > +
> > +        The LensPosition control is ignored unless the AfMode is set to
> > +        AfModeManual.
> > +
> > +        The units are dioptres divided by the hyperfocal distance. Non-integer
> > +        values are permitted. For example:
> > +        0 moves the lens to infinity.
> > +        0.5 moves the lens to twice the hyperfocal distance.
> > +        1 moves the lens to the hyperfocal position.
> > +        And larger values will focus the lens ever closer.
>
> Can we add a
>
>            \todo Define a property to report the Hyperfocal distance of
>            calibrated lenses.

Will do.

>
> > +
> > +  - AfState:
> > +      type: int32_t
> > +      description: |
> > +        Reports the current state of the AF algorithm in conjunction with the
> > +        reported AfMode value and (in continuous AF mode) the AfPauseState
> > +        value.  The possible state changes are described below, though we note
>                  ^ double space

Will fix/

>
> > +        the following state transitions that occur when the AfMode is changed.
> > +
> > +        If the AfMode is set to AfModeManual then the AfState will report
> > +        AfStateIdle. This action on its own does not cause the lens to move; the
>                         ^ What action ?
>
> The AfMode being moved to AfModeManual ? If that's the case I would
> omit the second part of the phrase (from "This action.." on) as the
> fact the lens doesn't move is specified by the AfModeManual definition
> and it is unrelated to the reported state.
>
> I would instead say that AfState will -always- report Idle when in
> Manual mode, even if the lens is moved.
>
>            Whan AfMode is set to AfModeManual then AfState will always report
>            AfStateIdle.

Yes, I'll adjust this. I'm still slightly tempted to add that doing
this doesn't move the lens... though it was a while ago I'm sure that
at some point someone asked about that so probably good to be clear
about that.

>
> > +        LensPosition control must subsequently be used to achieve this.
> > +
> > +        If the AfMode is set to AfModeAuto then the AfState will report
> > +        AfStateIdle. However, if AfModeAuto and AfTriggerStart are sent together
> > +        then AfState will omit AfStateIdle and move straight to AfStateScanning
> > +        (and start a scan).
> > +
> > +        If the AfMode is set to AfModeContinuous then the AfState will initially
> > +        report AfStateScanning.
> > +
> > +      enum:
> > +        - name: AfStateIdle
> > +          value: 0
> > +          description: |
> > +            The AF algorithm is in manual mode (AfModeManual) or in auto mode
> > +            (AfModeAuto) and a scan has not yet been triggered, or an
> > +            in-progress scan was cancelled.
> > +        - name: AfStateScanning
> > +          value: 1
> > +          description: |
> > +            The AF algorithm is in auto mode (AfModeAuto), and a scan has been
> > +            started using the AfTrigger control. The scan can be cancelled by
> > +            sending AfTriggerCancel at which point the algorithm will either
> > +            move back to AfStateIdle or, if the scan actually completes
> > +            before the cancel request is processed, to one of
> > +            AfStateFocused or AfStateFailed.
> > +
> > +            Alternatively the AF algorithm could be in continuous mode
> > +            (AfModeContinuous) at which point it may enter this state
> > +            spontaneously whenever it determines that a rescan is needed.
> > +        - name: AfStateFocused
> > +          value: 2
> > +          description: |
> > +            The AF algorithm is in auto (AfModeAuto) or continuous
> > +            (AfModeContinuous) mode and a scan has completed with the result
> > +            that the algorithm believes the image is now in focus.
> > +        - name: AfStateFailed
> > +          value: 3
> > +          description: |
> > +            The AF algorithm is in auto (AfModeAuto) or continuous
> > +            (AfModeContinuous) mode and a scan has completed with the result
> > +            that the algorithm did not find a good focus position.
> > +
> > +  - AfPauseState:
> > +      type: int32_t
> > +      description: |
> > +        Only applicable in continuous (AfModeContinuous) mode, this reports
> > +        whether the algorithm is currently running, paused or pausing (that is,
> > +        will pause as soon as any in-progress scan completes).
> > +
> > +        Any change to AfMode will cause AfPauseStateRunning to be reported.
> > +
> > +      enum:
> > +        - name: AfPauseStateRunning
> > +          value: 0
> > +          description: |
> > +            Continuous AF is running and the algorithm may restart a scan
> > +            spontaneously.
> > +        - name: AfPauseStatePausing
> > +          value: 1
> > +          description: |
> > +            Continuous AF has been sent an AfPauseDeferred control, and will
> > +            pause as soon as any in-progress scan completes (and then report
> > +            AfPauseStatePaused). No new scans will be start spontaneously until
> > +            the AfPauseResume control is sent.
> > +        - name: AfPauseStatePaused
> > +          value: 2
> > +          description: |
> > +            Continuous AF is paused. No further state changes or lens movements
> > +            will occur until the AfPauseResume control is sent.
> > +
>
> Great, just a minor refinements and from my side we're good to go.
> If you don't want to send a new version and if anyone has not other
> comments, feel free to reply my comments here and I can change things
> when applying if there's nothing too controversial.
>
> Thanks, nice work!

I'll do another version and re-send.

Thanks!
David

>
> >    # ----------------------------------------------------------------------------
> >    # Draft controls section
> >
> > @@ -406,27 +691,6 @@ controls:
> >              The camera will cancel any active or completed metering sequence.
> >              The AE algorithm is reset to its initial state.
> >
> > -  - AfTrigger:
> > -      type: int32_t
> > -      draft: true
> > -      description: |
> > -       Control for AF trigger. Currently identical to
> > -       ANDROID_CONTROL_AF_TRIGGER.
> > -
> > -        Whether the camera device will trigger autofocus for this request.
> > -      enum:
> > -        - name: AfTriggerIdle
> > -          value: 0
> > -          description: The trigger is idle.
> > -        - name: AfTriggerStart
> > -          value: 1
> > -          description: The AF routine is started by the camera.
> > -        - name: AfTriggerCancel
> > -          value: 2
> > -          description: |
> > -            The camera will cancel any active trigger and the AF routine is
> > -            reset to its initial state.
> > -
> >    - NoiseReductionMode:
> >        type: int32_t
> >        draft: true
> > @@ -507,45 +771,6 @@ controls:
> >              The AE algorithm has started a pre-capture metering session.
> >              \sa AePrecaptureTrigger
> >
> > -  - AfState:
> > -      type: int32_t
> > -      draft: true
> > -      description: |
> > -       Control to report the current AF algorithm state. Currently identical to
> > -       ANDROID_CONTROL_AF_STATE.
> > -
> > -        Current state of the AF algorithm.
> > -      enum:
> > -        - name: AfStateInactive
> > -          value: 0
> > -          description: The AF algorithm is inactive.
> > -        - name: AfStatePassiveScan
> > -          value: 1
> > -          description: |
> > -            AF is performing a passive scan of the scene in continuous
> > -            auto-focus mode.
> > -        - name: AfStatePassiveFocused
> > -          value: 2
> > -          description: |
> > -            AF believes the scene is in focus, but might restart scanning.
> > -        - name: AfStateActiveScan
> > -          value: 3
> > -          description: |
> > -            AF is performing a scan triggered by an AF trigger request.
> > -            \sa AfTrigger
> > -        - name: AfStateFocusedLock
> > -          value: 4
> > -          description: |
> > -            AF believes has focused correctly and has locked focus.
> > -        - name: AfStateNotFocusedLock
> > -          value: 5
> > -          description: |
> > -            AF has not been able to focus and has locked.
> > -        - name: AfStatePassiveUnfocused
> > -          value: 6
> > -          description: |
> > -            AF has completed a passive scan without finding focus.
> > -
> >    - AwbState:
> >        type: int32_t
> >        draft: true
> > --
> > 2.30.2
> >