<div dir="ltr"><div dir="ltr">Hi Laurent,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, 28 Jun 2021 at 01:49, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@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">Hello everybody,<br>
<br>
On Tue, Jun 22, 2021 at 06:12:26PM +0200, Jacopo Mondi wrote:<br>
> On Mon, Jun 21, 2021 at 10:15:21AM +0100, David Plowman wrote:<br>
> > On Mon, 21 Jun 2021 at 10:05, <<a href="mailto:paul.elder@ideasonboard.com" target="_blank">paul.elder@ideasonboard.com</a>> wrote:<br>
> >> On Fri, Jun 18, 2021 at 02:58:52PM +0100, Naushir Patuck wrote:<br>
> >>> On Fri, 18 Jun 2021 at 14:13, David Plowman wrote:<br>
> >>>> On Fri, 18 Jun 2021 at 13:07, Naushir Patuck wrote:<br>
> >>>>> On Fri, 18 Jun 2021 at 11:34, Paul Elder 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>
> >>>>> I think, strictly speaking, if agcStatus->locked == true, we should return<br>
> >>>>> control::AeStateConverged by the definition below. David, do you agree?<br>
> >><br>
> >> Oh shoot, that is indeed what I meant. I got mixed up with the status<br>
> >> name :/<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>
> >>><br>
> >>> Yes I agree, but we now have both AeEnable and AeLock defined with this change.<br>
> >><br>
> >> AeEnable is for enabling/disabling the AE routine. AeLock is to lock the<br>
> >> AE values... which would normally be the same as turning off AE, but you<br>
> >> want to be able to unlock AE without having to restart the routine.<br>
> ><br>
> > I'm still struggling a bit with this, however. What would it mean to<br>
> > unlock the AE without restarting it? Isn't it then still fixed at<br>
> > whatever values it had? Perhaps you could provide an example of this<br>
> > behaviour which would show how it is different.<br>
> ><br>
> > Sorry for still being puzzled...<br>
> <br>
> And I'm sorry as well for asking possibly stupid questions, but:<br>
> <br>
> - It seems to me the RPi AGC routine has two Pause/Resume functions that:<br>
> - Pause: sets fixed gain and shutter time to the last computed<br>
> shutter and gain values<br>
> - Resume: re-sets them to 0<br>
> <br>
> - In the RPi AEGC implementation, if fixed shutter and gains are set<br>
> then those values are used<br>
> <br>
> - The AEGC routine is paused/resumed on controls::AeEnable on/off<br>
> <br>
> - When AeEnable == Off, then the only way to override the fixed<br>
> shutter/gain is by using the ExposureTime and AnalogueGain controls<br>
> (ie manual shutter/gain control)<br>
> <br>
> So in you implementation disabling AE has the effect that the AEGC<br>
> routine uses the last computed values until a new shutter/gain value<br>
> is set by the application (through what is usually called "manual" mode).<br>
> <br>
> The difference between your implementation and a new one that uses<br>
> AeLocked and AeEnable is, in my understanding, that when the AeLocked<br>
> is set the AEGC routine is still in control of the shutter/gain which<br>
> gets "fixed" as you're doing, but it does not accept manual control of<br>
> the shutter/gain like you're doing unless the AEGC is disabled.<br>
> <br>
> This seems to be suggested by the following paragraph in the Android<br>
> documentation:<br>
> <br>
> -------------------------------------------------------------------------------<br>
> <br>
> If an application is switching between automatic and manual control<br>
> and wishes to eliminate any flicker during the switch, the following<br>
> procedure is recommended:<br>
> <br>
> Starting in auto-AE mode:<br>
> - Lock AE<br>
> - Wait for the first result to be output that has the AE locked<br>
> - Copy exposure settings from that result into a request, set the request to manual AE<br>
> - Submit the capture request, proceed to run manual AE as desired.<br>
> <br>
> See android.control.aeState for AE lock related state transition details.<br>
> -------------------------------------------------------------------------------<br>
> <br>
> If we were able to report the AEGC convergence state I think the same<br>
> could be done without AeLock, by disabling AEGC once it has converged<br>
> and continue from there with manual exposure. But my understanding is that<br>
> having a lock controls guarantees that once the AEGC has<br>
> converged it won't change, so that the in-flight requests that are<br>
> completed before the first one with manual AE control is processed<br>
> won't have floating AEGC values,<br>
> <br>
> (Let me try to draw it down, hoping it render ok with your client,<br>
> otherwise <a href="https://paste.debian.net/1202043/" rel="noreferrer" target="_blank">https://paste.debian.net/1202043/</a>)<br>
> <br>
> Queue of<br>
> submitted Completed AeState in the<br>
> Requests Requests completed Request<br>
> -----------------------------------------------------------------<br>
> <br>
> 0 (AeLock)<br>
> 1<br>
> 2<br>
> 3 0 Searching<br>
> 4 1 Converged<br>
> 5 (AeDisable + 2 Locked <- COPY AE VALUES<br>
> Manual AE with 3 Locked<br>
> settings from #2) 4 Locked<br>
> 5 Use Manual AE<br>
> <br>
> Without a Lock the AEGC routine would keep adjusting its values for<br>
> requests 3 and 4. Request 5 will contain settings copied from Request<br>
> 2 so a small hiccup might happen. Of course, if the number of frames<br>
> required to get to a converged state is longer, the duration of the<br>
> possible AE adjustment period is longer, making the flickering more<br>
> visible ?<br>
> <br>
> Does this make sense ?<br>
> <br>
> The android.control.aeMode documentation also reports that:<br>
> <br>
> -----------------------------------------------------------------<br>
> Note that auto-white balance (AWB) and auto-focus (AF) behavior is<br>
> device dependent when AE is in OFF mode. To have consistent behavior<br>
> across different devices, it is recommended to either set AWB and AF to<br>
> OFF mode or lock AWB and AF before setting AE to OFF.See<br>
> android.control.awbMode,<br>
> android.control.afMode,android.control.awbLock, and<br>
> android.control.afTrigger for more details.<br>
> -----------------------------------------------------------------<br>
> <br>
> Suggesting that the AF AWB and AE locks might play a role on the interactions<br>
> between the three algorithms.<br>
> <br>
> Does this make sense and provide a bit more of context about what<br>
> AeLock could be used for ?<br>
<br>
I'm still a bit puzzled by the need for an AeLock control. I do<br>
understand that it's used in Android to switch to manual exposure<br>
without glitches, as detailed above. However, couldn't the same result<br>
be achieved by switching AeMode from on to off (thus switching to manual<br>
mode), without specifying manual values for the sensitivity, exposure<br>
and frame duration ? The camera would then keep the last values computed<br>
by the AEGC, which would be reported in metadata, and could be then used<br>
as a basis to set other manual values.<br></blockquote><div><br></div><div>I agree with the sentiment that AeLock and AeEnable sound like the same thing,</div><div>and the Raspberry Pi AGC will almost certainly be treating it the same to achieve</div><div>what the user expects without glitching.</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>
This may be related to defining what happens to manual control values<br>
when running in auto mode. The camera will report computed values for<br>
the sensitivity, exposure time and frame duration in metadata, but<br>
should those values be implicitly copied to the request controls<br>
(effectively only, not really adding them to the Request instances) when<br>
the algorithm is switched off, or should the last value that was set<br>
manually be used in that case ? </blockquote><div><br></div><div>In my opinion, the values returned in the metadata, particularly</div><div>controls::ExposureTime and controls::AnalogueGain should be the values</div><div>used for that frame. So if the AE is switched off, we should still get these</div><div>values returned in the Request, as the currently used values (which may</div><div>be set manually, or frozen to what the AE last selected).</div><div><br></div><div>I did have one bit of confusion in the paragraph above. If we have manual</div><div>controls set, does that not imply that AE is not running in auto mode, so the</div><div>two operations are mutually exclusive?</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">I think we need to clearly specify this,<br>
ideally in a consistent fashion across all controls. If we decide that<br>
the camera behaviour should be to implicitly copy the metadata value to<br>
the control value, then AeLock doesn't seem to be needed. Otherwise,<br>
switching seamlessly from auto to manual mode requires a mechanism<br>
similar to AeLock.<br>
<br>
Is there any other need for the AeLock control that I may have missed ?<br>
<br>
> >> That's the idea afaict.<br>
> >><br>
> >> Supporting AeLock is optional, of course. Same for AeState, not all of<br>
> >> the states need to be supported. You can specify which ones are<br>
> >> supported in pipeline's ControlInfoMap.<br>
> >><br>
> >>> Perhaps combining them to avoid redundancy makes sense? Or do folks think they<br>
> >>> can they serve different purposes?<br>
> >>><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>
> >>><br>
> >>> Agree, I think we should rename "locked" to "converged" in the controller<br>
> >>> metadata struct.<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>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>