<div dir="ltr"><div dir="ltr">Hi Paul,<div><br></div><div>Thank you for your patch.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 18 Jun 2021 at 11:34, Paul Elder <<a href="mailto:paul.elder@ideasonboard.com">paul.elder@ideasonboard.com</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">AeLocked alone isn't sufficient for reporting the AE state, so replace<br>
it with AeState. Add an AeLock control for instructing the camera to<br>
lock the AE values.<br>
<br>
Update the current users of AeLocked accordingly.<br>
<br>
Signed-off-by: Paul Elder <<a href="mailto:paul.elder@ideasonboard.com" target="_blank">paul.elder@ideasonboard.com</a>><br>
---<br>
 src/ipa/raspberrypi/raspberrypi.cpp |  5 +-<br>
 src/ipa/rkisp1/rkisp1.cpp  Â  Â  Â  Â  Â | 13 ++--<br>
 src/libcamera/control_ids.yaml  Â  Â  | 96 ++++++++++++++++++-----------<br>
 3 files changed, 71 insertions(+), 43 deletions(-)<br>
<br>
diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp<br>
index 1c1e802a..ad0132c0 100644<br>
--- a/src/ipa/raspberrypi/raspberrypi.cpp<br>
+++ b/src/ipa/raspberrypi/raspberrypi.cpp<br>
@@ -468,7 +468,10 @@ void IPARPi::reportMetadata()<br>
<br>
  Â  Â  Â  AgcStatus *agcStatus = rpiMetadata_.GetLocked<AgcStatus>("agc.status");<br>
  Â  Â  Â  if (agcStatus) {<br>
-  Â  Â  Â  Â  Â  Â  Â libcameraMetadata_.set(controls::AeLocked, agcStatus->locked);<br>
+  Â  Â  Â  Â  Â  Â  Â libcameraMetadata_.set(controls::AeState,<br>
+  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  agcStatus->locked ?<br>
+  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  controls::AeStateLocked :<br>
+  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  controls::AeStateSearching);<br></blockquote><div><br></div><div>I think, strictly speaking, if agcStatus->locked == true, we should return</div><div>control::AeStateConverged by the definition below.  David, do you agree?<br></div><div><br></div><div>Thanks,</div><div>Naush</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">
  Â  Â  Â  Â  Â  Â  Â  libcameraMetadata_.set(controls::DigitalGain, agcStatus->digital_gain);<br>
  Â  Â  Â  }<br>
<br>
diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp<br>
index b47ea324..88a562a8 100644<br>
--- a/src/ipa/rkisp1/rkisp1.cpp<br>
+++ b/src/ipa/rkisp1/rkisp1.cpp<br>
@@ -51,7 +51,7 @@ private:<br>
  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  const rkisp1_stat_buffer *stats);<br>
<br>
  Â  Â  Â  void setControls(unsigned int frame);<br>
-  Â  Â  Â void metadataReady(unsigned int frame, unsigned int aeState);<br>
+  Â  Â  Â void metadataReady(unsigned int frame, int aeState);<br>
<br>
  Â  Â  Â  std::map<unsigned int, FrameBuffer> buffers_;<br>
  Â  Â  Â  std::map<unsigned int, void *> buffersMemory_;<br>
@@ -227,7 +227,7 @@ void IPARkISP1::updateStatistics(unsigned int frame,<br>
  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â const rkisp1_stat_buffer *stats)<br>
 {<br>
  Â  Â  Â  const rkisp1_cif_isp_stat *params = &stats->params;<br>
-  Â  Â  Â unsigned int aeState = 0;<br>
+  Â  Â  Â int aeState = controls::AeStateInactive;<br>
<br>
  Â  Â  Â  if (stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP) {<br>
  Â  Â  Â  Â  Â  Â  Â  const rkisp1_cif_isp_ae_stat *ae = &params->ae;<br>
@@ -262,7 +262,9 @@ void IPARkISP1::updateStatistics(unsigned int frame,<br>
  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  setControls(frame + 1);<br>
  Â  Â  Â  Â  Â  Â  Â  }<br>
<br>
-  Â  Â  Â  Â  Â  Â  Â aeState = fabs(factor - 1.0f) < 0.05f ? 2 : 1;<br>
+  Â  Â  Â  Â  Â  Â  Â aeState = fabs(factor - 1.0f) < 0.05f ?<br>
+  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â controls::AeStateConverged :<br>
+  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â controls::AeStateSearching;<br>
  Â  Â  Â  }<br>
<br>
  Â  Â  Â  metadataReady(frame, aeState);<br>
@@ -281,12 +283,11 @@ void IPARkISP1::setControls(unsigned int frame)<br>
  Â  Â  Â  queueFrameAction.emit(frame, op);<br>
 }<br>
<br>
-void IPARkISP1::metadataReady(unsigned int frame, unsigned int aeState)<br>
+void IPARkISP1::metadataReady(unsigned int frame, int aeState)<br>
 {<br>
  Â  Â  Â  ControlList ctrls(controls::controls);<br>
<br>
-  Â  Â  Â if (aeState)<br>
-  Â  Â  Â  Â  Â  Â  Â ctrls.set(controls::AeLocked, aeState == 2);<br>
+  Â  Â  Â ctrls.set(controls::AeState, aeState);<br>
<br>
  Â  Â  Â  RkISP1Action op;<br>
  Â  Â  Â  op.op = ActionMetadata;<br>
diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml<br>
index 9d4638ae..5717bc1f 100644<br>
--- a/src/libcamera/control_ids.yaml<br>
+++ b/src/libcamera/control_ids.yaml<br>
@@ -14,16 +14,70 @@ controls:<br>
<br>
  Â  Â  Â  Â \sa ExposureTime AnalogueGain<br>
<br>
-  - AeLocked:<br>
+  - AeLock:<br>
  Â  Â  Â type: bool<br>
  Â  Â  Â description: |<br>
-  Â  Â  Â  Report the lock status of a running AE algorithm.<br>
+  Â  Â  Â  Control to lock the AE values.<br>
+  Â  Â  Â  When set to true, the AE algorithm is locked to its latest parameters,<br>
+  Â  Â  Â  and will not change exposure settings until set to false.<br>
+  Â  Â  Â  \sa AeState<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>
+  - AeState:<br>
+  Â  Â  type: int32_t<br>
+  Â  Â  description: |<br>
+  Â  Â  Â  Control to report the current AE algorithm state. Enabling or disabling<br>
+  Â  Â  Â  AE (AeEnable) always resets the AeState to AeStateInactive. The camera<br>
+  Â  Â  Â  device can do several state transitions between two results, if it is<br>
+  Â  Â  Â  allowed by the state transition table. For example, AeStateInactive may<br>
+  Â  Â  Â  never actually be seen in a result.<br>
<br>
-  Â  Â  Â  \sa AeEnable<br>
+  Â  Â  Â  The state in the result is the state for this image (in sync with this<br>
+  Â  Â  Â  image). If AE state becomes AeStateConverged, then the image data<br>
+  Â  Â  Â  associated with the result should be good to use.<br>
+<br>
+  Â  Â  Â  The state transitions mentioned below assume that AeEnable is on.<br>
+<br>
+  Â  Â  Â  \sa AeLock<br>
+  Â  Â  enum:<br>
+  Â  Â  Â  - name: AeStateInactive<br>
+  Â  Â  Â  Â  value: 0<br>
+  Â  Â  Â  Â  description: |<br>
+  Â  Â  Â  Â  Â  The AE algorithm is inactive.<br>
+  Â  Â  Â  Â  Â  If the camera initiates an AE scan, the state shall go to Searching.<br>
+  Â  Â  Â  Â  Â  If AeLock is on, the state shall go to Locked.<br>
+  Â  Â  Â  - name: AeStateSearching<br>
+  Â  Â  Â  Â  value: 1<br>
+  Â  Â  Â  Â  description: |<br>
+  Â  Â  Â  Â  Â  The AE algorithm has not converged yet.<br>
+  Â  Â  Â  Â  Â  If the camera finishes an AE scan, the state shall go to Converged.<br>
+  Â  Â  Â  Â  Â  If the camera finishes an AE scan, but flash is required, the state<br>
+  Â  Â  Â  Â  Â  shall go to FlashRequired.<br>
+  Â  Â  Â  Â  Â  If AeLock is on, the state shall go to Locked.<br>
+  Â  Â  Â  - name: AeStateConverged<br>
+  Â  Â  Â  Â  value: 2<br>
+  Â  Â  Â  Â  description: |<br>
+  Â  Â  Â  Â  Â  The AE algorithm has converged.<br>
+  Â  Â  Â  Â  Â  If the camera initiates an AE scan, the state shall go to Searching.<br>
+  Â  Â  Â  Â  Â  If AeLock is on, the state shall go to Locked.<br>
+  Â  Â  Â  - name: AeStateLocked<br>
+  Â  Â  Â  Â  value: 3<br>
+  Â  Â  Â  Â  description: |<br>
+  Â  Â  Â  Â  Â  The AE algorithm is locked.<br>
+  Â  Â  Â  Â  Â  If AeLock is off, the state can go to Searching, Converged, or<br>
+  Â  Â  Â  Â  Â  FlashRequired.<br>
+  Â  Â  Â  - name: AeStateFlashRequired<br>
+  Â  Â  Â  Â  value: 4<br>
+  Â  Â  Â  Â  description: |<br>
+  Â  Â  Â  Â  Â  The AE algorithm would need a flash for good results<br>
+  Â  Â  Â  Â  Â  If the camera initiates an AE scan, the state shall go to Searching.<br>
+  Â  Â  Â  Â  Â  If AeLock is on, the state shall go to Locked.<br>
+  Â  Â  Â  - name: AeStatePrecapture<br>
+  Â  Â  Â  Â  value: 5<br>
+  Â  Â  Â  Â  description: |<br>
+  Â  Â  Â  Â  Â  The AE algorithm has started a pre-capture metering session.<br>
+  Â  Â  Â  Â  Â  After the sequence is finished, the state shall go to Converged if<br>
+  Â  Â  Â  Â  Â  AeLock is off, and Locked if it is on.<br>
+  Â  Â  Â  Â  Â  \sa AePrecaptureTrigger<br>
<br>
  Â # AeMeteringMode needs further attention:<br>
  Â # - Auto-generate max enum value.<br>
@@ -477,36 +531,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.27.0<br>
<br>
</blockquote></div></div>