[libcamera-devel] [PATCH v2 1/7] controls: Reorganize the AE-related controls
paul.elder at ideasonboard.com
paul.elder at ideasonboard.com
Mon Oct 4 08:17:33 CEST 2021
Hi David,
Thanks for the feedback.
On Fri, Oct 01, 2021 at 01:53:18PM +0100, David Plowman wrote:
> Hi Paul
>
> Thanks for all this work! I was fine with all the functionality being
> described, and mostly everything made sense to me, but there were just
> a couple of places where I thought we might be able to explain things
> more clearly. But this is just my opinion, others may disagree, so
> please take with the necessary quantities of salt!
>
> On Fri, 1 Oct 2021 at 11:33, Paul Elder <paul.elder at ideasonboard.com> wrote:
> >
> > We have multiple goals:
> > - we need a lock of some sort, to instruct the AEGC to not update output
> > results
> > - we need manual modes, to override the values computed by the AEGC
> > - we need to support seamless transitions from auto -> manual, and do so
> > without flickering
> > - we need custom minimum values for the manual controls, that is no
> > magic values for enabling/disabling auto
> > - all of these need to be done with AE sub-controls (exposure time,
> > analogue gain)
> >
> > To achieve these goals, we introduce mode controls for the AE
> > sub-controls: ExposureTimeMode and AnalogueGainMode. These have an auto
> > state, and a disabled state. The disabled state has an internal one-way
> > state change from locked to manual, triggered by the presence of the
> > value-controls (ExposureTime and AnalogueGain).
> >
> > We then remove the AeEnable control, as it is a redundant control in the
> > face of these two mode controls.
> >
> > We also remove AeLocked, as it is insufficient for reporting the AE
> > state, and we promote AeState to non-draft to fill its role. Notably,
> > the locked state is removed, since this information can be obtained from
> > the aforementioned mode controls.
> >
> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=42
> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=43
> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=47
> > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> >
> > ---
> > Changes in v2:
> > - No changes, just resubmitting at the head of this series so that it's
> > together and so that /people will actually see it/
> >
> > Initial version:
> > Still RFC as I haven't updated the users of the control yet, and I want
> > to check that these are the controls and docs that we want.
> >
> > We've decided that the "master AE control" will be implemented by a
> > helper... but looking at uvcvideo and the V4L2 controls I'm wondering if
> > such helper should come earlier than later?
> > ---
> > src/libcamera/control_ids.yaml | 215 +++++++++++++++++++++++----------
> > 1 file changed, 150 insertions(+), 65 deletions(-)
> >
> > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> > index 9d4638ae..18b186e8 100644
> > --- a/src/libcamera/control_ids.yaml
> > +++ b/src/libcamera/control_ids.yaml
> > @@ -7,23 +7,61 @@
> > # Unless otherwise stated, all controls are bi-directional, i.e. they can be
> > # set through Request::controls() and returned out through Request::metadata().
> > controls:
> > - - AeEnable:
> > - type: bool
> > - description: |
> > - Enable or disable the AE.
> > -
> > - \sa ExposureTime AnalogueGain
> > -
> > - - AeLocked:
> > - type: bool
> > + - AeState:
> > + type: int32_t
> > description: |
> > - Report the lock status of a running AE algorithm.
> > -
> > - If the AE algorithm is locked the value shall be set to true, if it's
> > - converging it shall be set to false. If the AE algorithm is not
> > - running the control shall not be present in the metadata control list.
> > + Control to report the current AE algorithm state. Enabling or disabling
> > + any of the AE-related mode controls (eg. AnalogueGainMode,
> > + ExposureTimeMode) is not required to reset the state to
> > + AeStateInactive. The camera device can do several state transitions
> > + between two results, if it is allowed by the state transition table.
> > + For example, AeStateInactive may never actually be seen in a result.
>
> I was fine until we got to that bit "Enabling or disabling". The rest
> of this paragraph struck me as a real burst of detail and corner cases
> (?) while I was still struggling with what AeStateInactive meant.
Ah, that was because the android description said "enabling or disabling
AE must reset the state to inactive" or something along those lines and
I just substituted that whole line with "no".
>
> Perhaps some of this specific stuff can be moved until after all the
> enum values have been explained? I found the enum descriptions quite
Not sure I can move it after the enums... but maybe at the end of the
overview at lesat?
> easy to follow and having read through those felt in a better place to
That's good!
> tackle the details. Having said that, I'm still a bit vague as to why
> "resetting the state to AeStateInactive" is a particular issue of
> concern?
>
> > +
> > + The state in the result is the state for this image (in sync with this
> > + image). If AE state becomes AeStateConverged, then the image data
> > + associated with the result should be good to use.
> > +
> > + As some AE algorithms may still be running when any of the AE-related
> > + controls are in manual mode, the states are valid even when the mode
> > + controls are set to Disabled. To know if this image image used AE or
>
> "image" is repeated.
>
> Do we need to say anything about what happens when all controls are
> disabled? (Does anything different happen??)
Maybe I should've said "if at least one is disabled".
>
> > + manual (or a mixture), check the relevant modes (eg. AnalogueGainMode,
> > + ExposureTimeMode).
> > +
> > + \sa AnalogueGain
> > + \sa AnalogueGainMode
> > + \sa ExposureTime
> > + \sa ExposureTimeMode
> >
> > - \sa AeEnable
> > + enum:
> > + - name: AeStateInactive
> > + value: 0
> > + description: |
> > + The AE algorithm is inactive.
> > + If the camera initiates an AE scan, the state shall go to Searching.
> > + - name: AeStateSearching
> > + value: 1
> > + description: |
> > + The AE algorithm has not converged yet.
> > + If the camera finishes an AE scan, the state shall go to Converged.
> > + If the camera finishes an AE scan, but flash is required, the state
> > + shall go to FlashRequired.
> > + - name: AeStateConverged
> > + value: 2
> > + description: |
> > + The AE algorithm has converged.
> > + If the camera initiates an AE scan, the state shall go to Searching.
> > + - name: AeStateFlashRequired
> > + value: 3
> > + description: |
> > + The AE algorithm would need a flash for good results
> > + If the camera initiates an AE scan, the state shall go to Searching.
> > + If AeLock is on, the state shall go to Locked.
> > + - name: AeStatePrecapture
> > + value: 4
> > + description: |
> > + The AE algorithm has started a pre-capture metering session.
> > + After the sequence is finished, the state shall go to Converged if
>
> This was all good, my only minor comment might be that using
> AeStateConverged and AeStateSearching in place of just
> Converged/Searching might be a little more precise?
Ah okay. Probably better to be explicit.
>
> > + \sa AePrecaptureTrigger
> >
> > # AeMeteringMode needs further attention:
> > # - Auto-generate max enum value.
> > @@ -93,6 +131,17 @@ controls:
> > how the desired total exposure is divided between the shutter time
> > and the sensor's analogue gain. The exposure modes are platform
> > specific, and not all exposure modes may be supported.
> > +
> > + When one of AnalogueGainMode or ExposureTimeMode is set to Disabled and
> > + a corresponding manual control is provided, AeExposureMode may be
>
> Actually I think the same is true even if you don't give a manual
> value. As soon as you fix any of them, the other(s) will vary in such
> a way as to ignore the exposure profile.
Ah yes you're right.
>
> > + ignored. This is because, for example, if AeExposureMode is set to
> > + ExposureLong but ExposureTimeMode is Disabled and a short ExposureTime
> > + is provided (AnalogueGainMode set to Auto), then the AeExposureMode
> > + doesn't make sense.
>
> The example is good, but seemed a bit wordy to me? Maybe the whole
> thing might be:
>
> "When any of AnalogueGainMode or ExposureTimeMode is set to Disabled.
> the fixed values will override any choices made by the AeExposureMode which may
> consequently be ignored."
That probably is sufficient.
>
> (Or perhaps I'm assuming a deeper understanding on the part of the
> reader than most will have?)
The example was me thinking aloud when coming up with the new rule :p
>
> > +
> > + \sa AnalogueGainMode
> > + \sa ExposureTimeMode
> > +
> > enum:
> > - name: ExposureNormal
> > value: 0
> > @@ -111,13 +160,15 @@ controls:
> > type: float
> > description: |
> > Specify an Exposure Value (EV) parameter. The EV parameter will only be
> > - applied if the AE algorithm is currently enabled.
> > + applied if the AE algorithm is currently enabled, that is, at least one
> > + of AnalogueGainMode and ExposureTimeMode are auto.
> >
> > By convention EV adjusts the exposure as log2. For example
> > EV = [-2, -1, 0.5, 0, 0.5, 1, 2] results in an exposure adjustment
> > of [1/4x, 1/2x, 1/sqrt(2)x, 1x, sqrt(2)x, 2x, 4x].
> >
> > - \sa AeEnable
> > + \sa AnalogueGainMode
> > + \sa ExposureTimeMode
> >
> > - ExposureTime:
> > type: int32_t
> > @@ -125,17 +176,49 @@ controls:
> > Exposure time (shutter speed) for the frame applied in the sensor
> > device. This value is specified in micro-seconds.
> >
> > - Setting this value means that it is now fixed and the AE algorithm may
> > - not change it. Setting it back to zero returns it to the control of the
> > - AE algorithm.
> > + This control will only take effect if ExposureTimeMode is Disabled. Its
> > + presence in a request acts as a trigger to switch to the internal
> > + manual mode within ExposureTimeModeDisabled.
> > +
> > + When reported in metadata, this control indicates what exposure time
> > + was used for the current request, regardless of ExposureTimeMode.
> > + ExposureTimeMode will indicate the source of the exposure time value,
> > + whether it came from the AE algorithm or not.
> >
> > - \sa AnalogueGain AeEnable
> > + \sa AnalogueGain
> > + \sa ExposureTimeMode
> >
> > - \todo Document the interactions between AeEnable and setting a fixed
> > - value for this control. Consider interactions with other AE features,
> > - such as aperture and aperture/shutter priority mode, and decide if
> > - control of which features should be automatically adjusted shouldn't
> > - better be handled through a separate AE mode control.
> > + - ExposureTimeMode:
> > + type: int32_t
> > + description: |
> > + Control to set and report the source of the exposure time that will be
> > + applied.
> > +
> > + \sa ExposureTime
> > + enum:
> > + - name: ExposureTimeModeAuto
> > + value: 0
> > + description: |
> > + The exposure time will be calculated automatically and set by the
> > + AE algorithm.
> > + - name: ExposureTimeModeDisabled
> > + value: 1
> > + description: |
> > + The exposure time will not be updated by the AE algorithm. It will
> > + come from the last calculated value when the mode was Auto, or from
> > + the value specified in ExposureTime.
> > +
> > + This Disabled state has two internal states; locked and manual.
>
> TBH I still struggle a bit with the word "locked". I think I prefer
> "fixed", it seems more obvious. But not to worry... if the Android
> world prefers it then I will get used to it!
I'm fine with either, I've just gotten used to saying "locked" since
that was the informal name of this whole ordeal :/
Thanks,
Paul
>
> Thanks again for doing all this!
>
> Best regards
> David
>
> > + When the Disabled state is first entered, the internal state will
> > + be locked, and the latest exposure time value set by the AE
> > + algorithm will be used (or the default ExposureTime value, if the
> > + mode started out as Disabled). When an ExposureTime is provided,
> > + then the internal state will go to manual, and the provided value
> > + will be used for the exposure time. Going from manual to locked is
> > + not possible. If an ExposureTime value is provided in the same
> > + request as going into the Disabled state, then the internal locked
> > + state will be skipped, and the internal state will start out as
> > + manual.
> >
> > - AnalogueGain:
> > type: float
> > @@ -144,17 +227,49 @@ controls:
> > The value of the control specifies the gain multiplier applied to all
> > colour channels. This value cannot be lower than 1.0.
> >
> > - Setting this value means that it is now fixed and the AE algorithm may
> > - not change it. Setting it back to zero returns it to the control of the
> > - AE algorithm.
> > + This control will only take effect if ExposureTimeMode is Disabled. Its
> > + presence in a request acts as a trigger to switch to the internal
> > + manual mode within ExposureTimeModeDisabled.
> >
> > - \sa ExposureTime AeEnable
> > + When reported in metadata, this control indicates what exposure time
> > + was used for the current request, regardless of ExposureTimeMode.
> > + ExposureTimeMode will indicate the source of the exposure time value,
> > + whether it came from the AE algorithm or not.
> > +
> > + \sa ExposureTime
> > + \sa AnalogueGainMode
> > +
> > + - AnalogueGainMode:
> > + type: int32_t
> > + description: |
> > + Control to set and report the source of the analogue gain that will be
> > + applied.
> >
> > - \todo Document the interactions between AeEnable and setting a fixed
> > - value for this control. Consider interactions with other AE features,
> > - such as aperture and aperture/shutter priority mode, and decide if
> > - control of which features should be automatically adjusted shouldn't
> > - better be handled through a separate AE mode control.
> > + \sa AnalogueGain
> > + enum:
> > + - name: AnalogueGainModeAuto
> > + value: 0
> > + description: |
> > + The analogue gain will be calculated automatically and set by the
> > + AE algorithm.
> > + - name: AnalogueGainModeDisabled
> > + value: 1
> > + description: |
> > + The analogue gain will not be updated by the AE algorithm. It will
> > + come from the last calculated value when the mode was Auto, or from
> > + the value specified in AnalogueGain.
> > +
> > + This Disabled state has two internal states; locked and manual.
> > + When the Disabled state is first entered, the internal state will
> > + be locked, and the latest analogue gain value set by the AE
> > + algorithm will be used (or the default AnalogueGain value, if the
> > + mode started out as Disabled). When an AnalogueGain is provided,
> > + then the internal state will go to manual, and the provided value
> > + will be used for the analogue gain. Going from manual to locked is
> > + not possible. If an AnalogueGain value is provided in the same
> > + request as going into the Disabled state, then the internal locked
> > + state will be skipped, and the internal state will start out as
> > + manual.
> >
> > - Brightness:
> > type: float
> > @@ -477,36 +592,6 @@ controls:
> > High quality aberration correction which might reduce the frame
> > rate.
> >
> > - - AeState:
> > - type: int32_t
> > - draft: true
> > - description: |
> > - Control to report the current AE algorithm state. Currently identical to
> > - ANDROID_CONTROL_AE_STATE.
> > -
> > - Current state of the AE algorithm.
> > - enum:
> > - - name: AeStateInactive
> > - value: 0
> > - description: The AE algorithm is inactive.
> > - - name: AeStateSearching
> > - value: 1
> > - description: The AE algorithm has not converged yet.
> > - - name: AeStateConverged
> > - value: 2
> > - description: The AE algorithm has converged.
> > - - name: AeStateLocked
> > - value: 3
> > - description: The AE algorithm is locked.
> > - - name: AeStateFlashRequired
> > - value: 4
> > - description: The AE algorithm would need a flash for good results
> > - - name: AeStatePrecapture
> > - value: 5
> > - description: |
> > - The AE algorithm has started a pre-capture metering session.
> > - \sa AePrecaptureTrigger
> > -
> > - AfState:
> > type: int32_t
> > draft: true
> > --
> > 2.27.0
> >
More information about the libcamera-devel
mailing list