<div dir="ltr"><div dir="ltr">Hi Laurent,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sun, 11 Jul 2021 at 23:37, 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">Hi Naush,<br>
<br>
(now really CC'ing David)<br>
<br>
Apologies for the late reply, I should have shared more information on<br>
this topic earlier.<br>
<br>
On Mon, Jul 05, 2021 at 06:34:07PM +0100, Naushir Patuck wrote:<br>
> On Fri, 2 Jul 2021 at 11:38, Paul Elder wrote:<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>
> > ---<br>
> > No change in v3<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 4d09a84f..4981aa29 100644<br>
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp<br>
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> > @@ -469,7 +469,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>
> Yes, this seems like the correct state mapping.<br>
<br>
Is it ? I thought it should be AeStateConverged.<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 cdfb4d13..4eca26e2 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>
> I know I am repeating myself here, but I don't see the need to differentiate between<br>
> AeLock and AeEnable == falses.<br>
<br>
Please see below.<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>
> ><br>
> <br>
> This state is also a bit ambiguous to me. Is this the state that must be reported<br>
> if AeEnable == false? If so, is that not also conveyed by setting the latter?<br>
> <br>
> If the objective of the change is to wholesale do what Android does, then I don't<br>
> really have any objection to this change as-is, and we can treat the above<br>
> states as best we can in the Raspberry Pi IPA. Otherwise, I do see some scope<br>
> to simplify some of this.<br>
<br>
First of all, let me clarify the goal. The libcamera native API needs to<br>
offer the features required to implement the Android camera HAL API, but<br>
we don't need to just duplicate it. When there are reasons to depart<br>
(for instance to fix historical mistakes made in Android, to provide<br>
additional features, or just to expose an API we consider more<br>
consistent), we should do so.<br>
<br>
Then, a word about the process. Implementing support for an Android<br>
control requires plumbing it through the camera stack: definition of a<br>
libcamera control, translation between the Android and libcamera control<br>
in the Android camera HAL, implementation of the libcamera control in<br>
the pipeline handlesr, and, when applicable, in the IPAs. Defining<br>
libcamera controls, as any API design effort, takes time. To allow<br>
moving forward on all the other parts, we have a fast-track process to<br>
define draft controls that mimick the Android controls. All the draft<br>
controls will be redesigned before freezing the libcamera public API.<br>
<br>
This means that AeLock and AeState should be marked as draft controls as<br>
part of this patch.<br>
<br>
This being said, we don't need to define controls as draft controls if<br>
we want to already go through a complete design process. The discussion<br>
that stemmed from this patch series goes in that direction.<br>
<br>
It's useful, in this context, to understand why the Android AE (and AWB)<br>
controls have been designed this way. Conceptually speaking, the<br>
algorithms are considered, in Android, to be one level above manual<br>
controls.<br>
<br>
Controls Metadata<br>
/---------- == OFF ---------\<br>
| v<br>
aeMode -/ +------+ |\<br>
aeLock --> | AE | --> |0\ +--------+<br>
aeExposureCompensation +------+ | | --> | Sensor | --> exposureTime<br>
| | +--------+ sensitivity<br>
exposureTime -----------------------> |1/<br>
sensitivity |/<br>
<br>
(The AE controls are in the adnroid.control class, while the sensor<br>
manual control are in the android.sensor class.)<br>
<br>
The aeMode control selects whether the AE algorithm (when != OFF) or the<br>
manual controls (when == OFF) drive the sensor. The aeLock control tells<br>
the AE algorithm to freeze its output (when ON) or operate normally<br>
(when OFF). The metadata always report the values applied to the sensor,<br>
regardless of whether they come from the AE algorithm or from manual<br>
controls.<br>
<br>
One important difference, compared to the model we currently implement,<br>
is that the Android AE algorithm is independent from the manual sensor<br>
controls. The values of the manual controls aren't affected by the AE<br>
algorithm, and when AE is disabled, the last set manual control values<br>
are applied. This thus require an aeLock control to transition<br>
seamlessly between AE and manual mode, as the application needs to set<br>
the manual controls to the values applied to the sensor on the last<br>
frame that AE was on, which couldn't otherwise be done due to the<br>
internal delays.<br></blockquote><div><br></div><div>Thank you for the detailed explanation here. I can now better understand</div><div>the Android AE state transitions and why they may be needed.</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>
The existing libcamera AE controls are ill-defined. There's a \todo<br>
comment to document the interactions between AeEnable and setting a<br>
fixed value for ExposureTime or AnalogueGain. I'm actually not sure what<br>
happens if AeEnable is set to false when ExposureTime and/or<br>
AnalogueGain are 0, or when AeEnable is set to true when ExposureTime<br>
and/or AnalogueGain are not 0.<br></blockquote><div><br></div><div>To provide some brief context on the Raspberry Pi AGC, we never really</div><div>go into a "disabled" state at all. This is because the AGC algorithm always</div><div>provides the shutter/gain values to pass to the sensor, even in manual mode</div><div>(more on this below). We use an internal Paused state to lock the shutter/gain</div><div>values so that they can never change, which essentially mimics a "disabled"</div><div>state.</div><div><br></div><div>For manual operation, we pass shutter/gain ctrl values (either or both) to the AGC,</div><div>and that is what the AGC requests off the sensor. This happens regardless of </div><div>whether the AGC is in it's internal paused state or not. If the AGC is not in</div><div>paused state, and we only provide one manual control value (e.g. shutter speed),</div><div>then the AGC will vary the gain as it sees fit to converge to the desired</div><div>brightness. Setting a manual shutter/gain ctrl value back to 0 hands back control</div><div>of the control to the AGC and it starts adapting it as necessary to converge to</div><div>the desired brightness.</div><div><br></div><div>This type of implementation does allow us to essentially do all that Android would<br></div><div>except of the AGC, but does also allow us the flexibility to do Aperture/Shutter/Gain</div><div>priority modes if needed.</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>
We need a formal model for how libcamera algorithms interact with manual<br>
controls. It doesn't have to duplicate the Android model, but it has to<br>
offer at least the same features. I believe we need to take it one step<br>
further, as we want to support exposure priority and aperture priority.<br>
I think we should actually talk about gain/sensitivity priority instead<br>
of aperture priority for the existing feature, and already prepare for<br>
devices that have a controllable lens aperture.<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>
Now, the hard question: how should libcamera model this ? :-) Naush,<br>
David, your feedback from a RPi point of view would be very appreciated.<br><br></blockquote><div><br></div><div>It would be easy for me to say do it the Raspberry Pi way :-)</div><div>But in reality, I don't think the Android AE model gains us an advantage over our existing</div><div>model. And having to track one less state (locked vs disabled) is always a good thing.</div><div><br></div><div>Explicitly defining AeModes to have (amongst others) ShutterPrioiry, GainPrioiry,</div><div>AperturePriority and Auto modes would be useful though. This avoids the messiness of</div><div>setting 0 to hand back manual control back to the AGC?</div><div><br></div><div>I'm sure David will also want to share his opinions on this, but he won't be able to</div><div>respond for a few days.</div><div><br></div><div>Regards,</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">
Please also let me know if the above explanation is clear or if you need<br>more information.</blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> > + - 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>