<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 18 Jun 2021 at 14:13, David Plowman <<a href="mailto:david.plowman@raspberrypi.com">david.plowman@raspberrypi.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">Hi Naush, everyone<br>
<br>
On Fri, 18 Jun 2021 at 13:07, Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>> wrote:<br>
><br>
> Hi Paul,<br>
><br>
> Thank you for your patch.<br>
><br>
> On Fri, 18 Jun 2021 at 11:34, Paul Elder <<a href="mailto:paul.elder@ideasonboard.com" target="_blank">paul.elder@ideasonboard.com</a>> wrote:<br>
>><br>
>> 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>
><br>
><br>
> I think, strictly speaking, if agcStatus->locked == true, we should return<br>
> control::AeStateConverged by the definition below. David, do you agree?<br>
<br>
Hmm, indeed. My reading is that what we used to know as AeEnable has<br>
now been renamed/replaced by AeLock, is that right? And as you say,<br>
"locked" does not mean "converged" (any more).<br></blockquote><div><br></div><div>Yes I agree, but we now have both AeEnable and AeLock defined with this change.</div><div>Perhaps combining them to avoid redundancy makes sense? Or do folks think they</div><div>can they serve different purposes?</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>
So:<br>
<br>
We set controls::AeLocked in the metadata according to the value that<br>
we received in the corresponding control (which we now receive instead<br>
of AeEnable), and<br>
<br>
We set controls::AeState in the metadata to AeStateConverged when<br>
agcStatus->locked is true, otherwise to AeStateSearching. We should<br>
probably bite the bullet and rename that "locked" field to<br>
"converged", otherwise it's going to cause confusion for the rest of<br>
eternity.<br></blockquote><div><br></div><div>Agree, I think we should rename "locked" to "converged" in the controller</div><div>metadata struct.</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>
David<br>
<br>
><br>
> Thanks,<br>
> Naush<br>
><br>
>><br>
>> 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 = ¶ms->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>