<div dir="ltr"><div dir="ltr">Hi Paul,<div><br></div><div>Thank you for your work.  It's nice to see these changes to solidify the AE controls.</div><div>I do have a few thoughts/comments below.</div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 18 May 2022 at 14:47, Paul Elder via libcamera-devel <<a href="mailto:libcamera-devel@lists.libcamera.org">libcamera-devel@lists.libcamera.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">We have multiple goals:<br>
- we need a lock of some sort, to instruct the AEGC to not update output<br>
  results<br>
- we need manual modes, to override the values computed by the AEGC<br>
- we need to support seamless transitions from auto -> manual, and do so<br>
  without flickering<br>
- we need custom minimum values for the manual controls, that is no<br>
  magic values for enabling/disabling auto<br>
- all of these need to be done with AE sub-controls (exposure time,<br>
  analogue gain)<br>
<br>
To achieve these goals, we introduce mode controls for the AE<br>
sub-controls: ExposureTimeMode and AnalogueGainMode. These have an auto<br>
state, and a disabled state. The disabled state has an internal one-way<br>
state change from locked to manual, triggered by the presence of the<br>
value-controls (ExposureTime and AnalogueGain).<br>
<br>
We then remove the AeEnable control, as it is a redundant control in the<br>
face of these two mode controls.<br>
<br>
We also remove AeLocked, as it is insufficient for reporting the AE<br>
state, and we promote AeState to non-draft to fill its role. Notably,<br>
the locked state is removed, since this information can be obtained from<br>
the aforementioned mode controls.<br>
<br>
Bug: <a href="https://bugs.libcamera.org/show_bug.cgi?id=42" rel="noreferrer" target="_blank">https://bugs.libcamera.org/show_bug.cgi?id=42</a><br>
Bug: <a href="https://bugs.libcamera.org/show_bug.cgi?id=43" rel="noreferrer" target="_blank">https://bugs.libcamera.org/show_bug.cgi?id=43</a><br>
Bug: <a href="https://bugs.libcamera.org/show_bug.cgi?id=47" rel="noreferrer" target="_blank">https://bugs.libcamera.org/show_bug.cgi?id=47</a><br>
Signed-off-by: Paul Elder <<a href="mailto:paul.elder@ideasonboard.com" target="_blank">paul.elder@ideasonboard.com</a>><br>
<br>
---<br>
Changes in v4:<br>
- remove FlashRequired and Precapture from AeState<br>
- upgrade documentation of all the controls<br>
<br>
Changes in v3:<br>
- improve wording of the control descriptions<br>
  - make more succinct and clear<br>
- add description of how to do a flickerless transition<br>
<br>
Changes in v2:<br>
- No changes, just resubmitting at the head of this series so that it's<br>
  together and so that /people will actually see it/<br>
<br>
Initial version:<br>
Still RFC as I haven't updated the users of the control yet, and I want<br>
to check that these are the controls and docs that we want.<br>
<br>
We've decided that the "master AE control" will be implemented by a<br>
helper... but looking at uvcvideo and the V4L2 controls I'm wondering if<br>
such helper should come earlier than later?<br></blockquote><div><br></div><div>Yes, I agree having the "master AE control" earlier will be beneficial for</div><div>application developers.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
---<br>
 src/libcamera/control_ids.yaml | 262 +++++++++++++++++++++++++--------<br>
 1 file changed, 200 insertions(+), 62 deletions(-)<br>
<br>
diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml<br>
index 9d4638ae..9f5ce5e8 100644<br>
--- a/src/libcamera/control_ids.yaml<br>
+++ b/src/libcamera/control_ids.yaml<br>
@@ -7,23 +7,46 @@<br>
 # Unless otherwise stated, all controls are bi-directional, i.e. they can be<br>
 # set through Request::controls() and returned out through Request::metadata().<br>
 controls:<br>
-  - AeEnable:<br>
-      type: bool<br>
+  - AeState:<br>
+      type: int32_t<br>
       description: |<br>
-        Enable or disable the AE.<br>
+        Control to report the AE algorithm state associated with the capture<br>
+        result.<br>
<br>
-        \sa ExposureTime AnalogueGain<br>
+        The state is still reported even if ExposureTimeMode or<br>
+        AnalogueGainMode is set to Disabled.<br>
<br>
-  - AeLocked:<br>
-      type: bool<br>
-      description: |<br>
-        Report the lock status of a running AE algorithm.<br>
+        \sa AnalogueGain<br>
+        \sa AnalogueGainMode<br>
+        \sa ExposureTime<br>
+        \sa ExposureTimeMode<br>
<br>
-        If the AE algorithm is locked the value shall be set to true, if it's<br>
-        converging it shall be set to false. If the AE algorithm is not<br>
-        running the control shall not be present in the metadata control list.<br>
+      enum:<br>
+        - name: AeStateInactive<br>
+          value: 0<br>
+          description: |<br>
+            The AE algorithm is inactive.<br>
<br>
-        \sa AeEnable<br>
+            This state should be returned if both AnalogueGainMode and<br>
+            ExposureTimeMode are set to disabled (or one, if the camera only<br>
+            supports one of the two controls).<br>
+        - name: AeStateSearching<br>
+          value: 1<br>
+          description: |<br>
+            The AE algorithm has not converged yet.<br>
+<br>
+            This state should be returned if at least one of AnalogueGainMode<br>
+            or ExposureTimeMode is set to auto, and the AE algorithm hasn't<br>
+            converged yet. If the AE algorithm converges, the state shall go to<br>
+            AeStateConverged.<br>
+        - name: AeStateConverged<br>
+          value: 2<br>
+          description: |<br>
+            The AE algorithm has converged.<br>
+<br>
+            This state should be returned if at least one of AnalogueGainMode<br>
+            or ExposureTimeMode is set to auto, and the AE algorithm has<br>
+            converged.<br>
<br>
   # AeMeteringMode needs further attention:<br>
   # - Auto-generate max enum value.<br>
@@ -93,6 +116,13 @@ controls:<br>
         how the desired total exposure is divided between the shutter time<br>
         and the sensor's analogue gain. The exposure modes are platform<br>
         specific, and not all exposure modes may be supported.<br>
+<br>
+        When one of AnalogueGainMode or ExposureTimeMode is set to Disabled,<br>
+        the fixed values will override any choices made by AeExposureMode.<br>
+<br>
+        \sa AnalogueGainMode<br>
+        \sa ExposureTimeMode<br>
+<br>
       enum:<br>
         - name: ExposureNormal<br>
           value: 0<br>
@@ -111,13 +141,15 @@ controls:<br>
       type: float<br>
       description: |<br>
         Specify an Exposure Value (EV) parameter. The EV parameter will only be<br>
-        applied if the AE algorithm is currently enabled.<br>
+        applied if the AE algorithm is currently enabled, that is, at least one<br>
+        of AnalogueGainMode and ExposureTimeMode are auto.<br>
<br>
         By convention EV adjusts the exposure as log2. For example<br>
         EV = [-2, -1, 0.5, 0, 0.5, 1, 2] results in an exposure adjustment<br>
         of [1/4x, 1/2x, 1/sqrt(2)x, 1x, sqrt(2)x, 2x, 4x].<br>
<br>
-        \sa AeEnable<br>
+        \sa AnalogueGainMode<br>
+        \sa ExposureTimeMode<br>
<br>
   - ExposureTime:<br>
       type: int32_t<br>
@@ -125,17 +157,85 @@ controls:<br>
         Exposure time (shutter speed) for the frame applied in the sensor<br>
         device. This value is specified in micro-seconds.<br>
<br>
-        Setting this value means that it is now fixed and the AE algorithm may<br>
-        not change it. Setting it back to zero returns it to the control of the<br>
-        AE algorithm.<br>
+        This control will only take effect if ExposureTimeMode is Disabled. If<br>
+        this control is set when ExposureTimeMode is Auto, the value will be<br>
+        ignored and will not be retained.<br>
+<br>
+        When reported in metadata, this control indicates what exposure time<br>
+        was used for the current request, regardless of ExposureTimeMode.<br>
+        ExposureTimeMode will indicate the source of the exposure time value,<br>
+        whether it came from the AE algorithm or not.<br>
+<br>
+        \sa AnalogueGain<br>
+        \sa ExposureTimeMode<br>
+<br>
+  - ExposureTimeMode:<br>
+      type: int32_t<br>
+      description: |<br>
+        Controls the source of the exposure time that is applied to the image<br>
+        sensor. When set to Auto, the AE algorithm computes the exposure time<br>
+        and configures the image sensor accordingly. When set to Disabled,<br>
+        exposure time specified in ExposureTime is applied to the image sensor.<br>
+        If ExposureTime is not set, then the value last computed by the AE<br>
+        algorithm when the mode was Auto will be used.<br></blockquote><div><br></div><div>Can we un-set ExposureTime?  If it ever gets set once at any point in the application,</div><div>then ExposureTimeModeDisabled will always use the last set value for ExposureTime.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+<br>
+        If ExposureTime is not set and the mode is ExposureTimeModeDisabled and<br>
+        AE was never Auto (either because the camera started in Disabled mode,<br>
+        or Auto is not supported by the camera), the camera should use a<br>
+        best-effort default value.<br>
+<br>
+        When ExposureTimeMode is set Auto, the value set in ExposureTime is<br>
+        ignored and is not retained. This means that if ExposureTimeMode is set<br>
+        to Disabled and ExposureTime is not also set, the exposure time that<br>
+        was last computed by the AE algorithm while the mode was Auto will be<br>
+        applied to the sensor.<br>
+<br>
+        If ExposureTimeModeDisabled is supported, the ExposureTime control must<br>
+        also be supported.<br>
+<br>
+        The set of ExposureTimeMode modes that are supported by the camera must<br>
+        have an intersection with the supported set of AnalogueGainMode modes.<br>
<br>
-        \sa AnalogueGain AeEnable<br>
+        As it takes a few frames to apply the exposure time, there is a period of<br>
+        time between submitting a request with ExposureTimeMode set to Disabled<br>
+        and the exposure time component of the AE actually being disabled,<br>
+        during which the AE algorithm can still update the exposure time. If an<br>
+        application is switching from automatic and manual control and wishes<br>
+        to eliminate any flicker during the switch, the following procedure is<br>
+        recommended.<br></blockquote><div><br></div><div>I'm a bit confused by this bit to be honest.  If a user switches ExposureTimeMode from<br>Auto to Disabled with the intention of setting a manual ExposureTime, how can we ever</div><div>avoid a glitch in the brightness (unless we also change AnalogueGain appropriately)?</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
-        \todo Document the interactions between AeEnable and setting a fixed<br>
-        value for this control. Consider interactions with other AE features,<br>
-        such as aperture and aperture/shutter priority mode, and decide if<br>
-        control of which features should be automatically adjusted shouldn't<br>
-        better be handled through a separate AE mode control.<br>
+        1. Start with ExposureTimeMode set to Auto<br>
+<br>
+        2. Set ExposureTimeMode to Disabled<br>
+<br>
+        3. Wait for the first request to be output that has ExposureTimeMode<br>
+        set to Disabled<br></blockquote><div><br></div><div>How would the application know this time point?  Would the AE algorithm have to</div><div>count frames once it has been given a ExposureTimeModeDisabled ctrl then</div><div>return out the same in the metadata when it knows that it's last requested exposure</div><div>time change has been applied?</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+<br>
+        4. Copy the value reported in ExposureTime into a new request, and<br>
+        submit it<br>
+<br>
+        5. Proceed to run manual exposure time<br></blockquote><div><br></div><div>Again, I am unclear how this avoids glitches.  Say the AE chooses an exposure</div><div>time of 33ms, then the user wants to switch to 15ms.  There is always going to</div><div>be a jump in brightness.  Perhaps my interpretation of this glitch is not the same</div><div>as what you are describing?</div><div><br></div><div>Ditto comments for the AnalogueGain changes.</div><div><br></div><div>Regards,</div><div>Naush</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+<br>
+        \sa ExposureTime<br>
+      enum:<br>
+        - name: ExposureTimeModeAuto<br>
+          value: 0<br>
+          description: |<br>
+            The exposure time will be calculated automatically and set by the<br>
+            AE algorithm. If ExposureTime is set while this mode is active, it<br>
+            will be ignored, and it will also not be retained.<br>
+        - name: ExposureTimeModeDisabled<br>
+          value: 1<br>
+          description: |<br>
+            The exposure time will not be updated by the AE algorithm. It will<br>
+            come from the last calculated value when the mode was Auto, or from<br>
+            the value specified in ExposureTime.<br>
+<br>
+            When transitioning from Auto to Disabled mode, the last computed<br>
+            exposure value is used until a new value is specified through the<br>
+            ExposureTime control. If an ExposureTime value is specified in the<br>
+            same request where the ExposureTimeMode is changed from Auto to<br>
+            Disabled, the provided ExposureTime is applied.<br>
<br>
   - AnalogueGain:<br>
       type: float<br>
@@ -144,17 +244,85 @@ controls:<br>
         The value of the control specifies the gain multiplier applied to all<br>
         colour channels. This value cannot be lower than 1.0.<br>
<br>
-        Setting this value means that it is now fixed and the AE algorithm may<br>
-        not change it. Setting it back to zero returns it to the control of the<br>
-        AE algorithm.<br>
+        This control will only take effect if AnalogueGainMode is Disabled. If<br>
+        this control is set when AnalogueGainMode is Auto, the value will be<br>
+        ignored and will not be retained.<br>
+<br>
+        When reported in metadata, this control indicates what analogue gain<br>
+        was used for the current request, regardless of AnalogueGainMode.<br>
+        AnalogueGainMode will indicate the source of the analogue gain value,<br>
+        whether it came from the AE algorithm or not.<br>
+<br>
+        \sa ExposureTime<br>
+        \sa AnalogueGainMode<br>
+<br>
+  - AnalogueGainMode:<br>
+      type: int32_t<br>
+      description: |<br>
+        Controls the source of the analogue gain that is applied to the image<br>
+        sensor. When set to Auto, the AE algorithm computes the analogue gain<br>
+        and configures the image sensor accordingly. When set to Disabled,<br>
+        analogue gain specified in AnalogueGain is applied to the image sensor.<br>
+        If AnalogueGain is not set, then the value last computed by the AE<br>
+        algorithm when the mode was Auto will be used.<br>
+<br>
+        If AnalogueGain is not set and the mode is AnalogueGainModeDisabled and<br>
+        AE was never Auto (either because the camera started in Disabled mode,<br>
+        or Auto is not supported by the camera), the camera should use a<br>
+        best-effort default value.<br>
+<br>
+        When AnalogueGainMode is set Auto, the value set in AnalogueGain is<br>
+        ignored and is not retained. This means that if AnalogueGainMode is set<br>
+        to Disabled and AnalogueGain is not also set, the analogue gain that<br>
+        was last computed by the AE algorithm while the mode was Auto will be<br>
+        applied to the sensor.<br>
<br>
-        \sa ExposureTime AeEnable<br>
+        If AnalogueGainModeDisabled is supported, the AnalogueGain control must<br>
+        also be supported.<br>
+<br>
+        The set of AnalogueGainMode modes that are supported by the camera must<br>
+        have an intersection with the supported set of ExposureTimeMode modes.<br>
+<br>
+        As it takes a few frames to apply the analogue gain, there is a period of<br>
+        time between submitting a request with AnalogueGainMode set to Disabled<br>
+        and the analogue gain component of the AE actually being disabled,<br>
+        during which the AE algorithm can still update the analogue gain. If an<br>
+        application is switching from automatic and manual control and wishes<br>
+        to eliminate any flicker during the switch, the following procedure is<br>
+        recommended.<br>
+<br>
+        1. Start with AnalogueGainMode set to Auto<br>
+<br>
+        2. Set AnalogueGainMode to Disabled<br>
+<br>
+        3. Wait for the first request to be output that has AnalogueGainMode<br>
+        set to Disabled<br>
+<br>
+        4. Copy the value reported in AnalogueGain into a new request, and<br>
+        submit it<br>
+<br>
+        5. Proceed to run manual analogue gain<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">+<br>
+        \sa AnalogueGain<br>
+      enum:<br>
+        - name: AnalogueGainModeAuto<br>
+          value: 0<br>
+          description: |<br>
+            The analogue gain will be calculated automatically and set by the<br>
+            AE algorithm. If AnalogueGain is set while this mode is active, it<br>
+            will be ignored, and it will also not be retained.<br>
+        - name: AnalogueGainModeDisabled<br>
+          value: 1<br>
+          description: |<br>
+            The analogue gain will not be updated by the AE algorithm. It will<br>
+            come from the last calculated value when the mode was Auto, or from<br>
+            the value specified in AnalogueGain.<br>
<br>
-        \todo Document the interactions between AeEnable and setting a fixed<br>
-        value for this control. Consider interactions with other AE features,<br>
-        such as aperture and aperture/shutter priority mode, and decide if<br>
-        control of which features should be automatically adjusted shouldn't<br>
-        better be handled through a separate AE mode control.<br>
+            When transitioning from Auto to Disabled mode the last computed<br>
+            gain value is used until a new value is specified through the<br>
+            AnalogueGain control. If an AnalogueGain value is specified in the<br>
+            same request where the AnalogueGainMode is set to Disabled, the<br>
+            provided AnalogueGain is applied.<br>
<br>
   - Brightness:<br>
       type: float<br>
@@ -477,36 +645,6 @@ controls:<br>
             High quality aberration correction which might reduce the frame<br>
             rate.<br>
<br>
-  - AeState:<br>
-      type: int32_t<br>
-      draft: true<br>
-      description: |<br>
-       Control to report the current AE algorithm state. Currently identical to<br>
-       ANDROID_CONTROL_AE_STATE.<br>
-<br>
-        Current state of the AE algorithm.<br>
-      enum:<br>
-        - name: AeStateInactive<br>
-          value: 0<br>
-          description: The AE algorithm is inactive.<br>
-        - name: AeStateSearching<br>
-          value: 1<br>
-          description: The AE algorithm has not converged yet.<br>
-        - name: AeStateConverged<br>
-          value: 2<br>
-          description: The AE algorithm has converged.<br>
-        - name: AeStateLocked<br>
-          value: 3<br>
-          description: The AE algorithm is locked.<br>
-        - name: AeStateFlashRequired<br>
-          value: 4<br>
-          description: The AE algorithm would need a flash for good results<br>
-        - name: AeStatePrecapture<br>
-          value: 5<br>
-          description: |<br>
-            The AE algorithm has started a pre-capture metering session.<br>
-            \sa AePrecaptureTrigger<br>
-<br>
   - AfState:<br>
       type: int32_t<br>
       draft: true<br>
-- <br>
2.30.2<br>
<br>
</blockquote></div></div>