[libcamera-devel] [RFC PATCH v3 05/16] controls: Replace AeLocked with AeState, and add AeLock
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Jul 12 00:36:19 CEST 2021
Hi Naush,
(now really CC'ing David)
Apologies for the late reply, I should have shared more information on
this topic earlier.
On Mon, Jul 05, 2021 at 06:34:07PM +0100, Naushir Patuck wrote:
> On Fri, 2 Jul 2021 at 11:38, Paul Elder wrote:
> > AeLocked alone isn't sufficient for reporting the AE state, so replace
> > it with AeState. Add an AeLock control for instructing the camera to
> > lock the AE values.
> >
> > Update the current users of AeLocked accordingly.
> >
> > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> >
> > ---
> > No change in v3
> > ---
> > src/ipa/raspberrypi/raspberrypi.cpp | 5 +-
> > src/ipa/rkisp1/rkisp1.cpp | 13 ++--
> > src/libcamera/control_ids.yaml | 96 ++++++++++++++++++-----------
> > 3 files changed, 71 insertions(+), 43 deletions(-)
> >
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > index 4d09a84f..4981aa29 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -469,7 +469,10 @@ void IPARPi::reportMetadata()
> >
> > AgcStatus *agcStatus = rpiMetadata_.GetLocked<AgcStatus>("agc.status");
> > if (agcStatus) {
> > - libcameraMetadata_.set(controls::AeLocked, agcStatus->locked);
> > + libcameraMetadata_.set(controls::AeState,
> > + agcStatus->locked ?
> > + controls::AeStateLocked :
> > + controls::AeStateSearching);
>
> Yes, this seems like the correct state mapping.
Is it ? I thought it should be AeStateConverged.
> > libcameraMetadata_.set(controls::DigitalGain, agcStatus->digital_gain);
> > }
> >
> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > index cdfb4d13..4eca26e2 100644
> > --- a/src/ipa/rkisp1/rkisp1.cpp
> > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > @@ -51,7 +51,7 @@ private:
> > const rkisp1_stat_buffer *stats);
> >
> > void setControls(unsigned int frame);
> > - void metadataReady(unsigned int frame, unsigned int aeState);
> > + void metadataReady(unsigned int frame, int aeState);
> >
> > std::map<unsigned int, FrameBuffer> buffers_;
> > std::map<unsigned int, void *> buffersMemory_;
> > @@ -227,7 +227,7 @@ void IPARkISP1::updateStatistics(unsigned int frame,
> > const rkisp1_stat_buffer *stats)
> > {
> > const rkisp1_cif_isp_stat *params = &stats->params;
> > - unsigned int aeState = 0;
> > + int aeState = controls::AeStateInactive;
> >
> > if (stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP) {
> > const rkisp1_cif_isp_ae_stat *ae = ¶ms->ae;
> > @@ -262,7 +262,9 @@ void IPARkISP1::updateStatistics(unsigned int frame,
> > setControls(frame + 1);
> > }
> >
> > - aeState = fabs(factor - 1.0f) < 0.05f ? 2 : 1;
> > + aeState = fabs(factor - 1.0f) < 0.05f ?
> > + controls::AeStateConverged :
> > + controls::AeStateSearching;
> > }
> >
> > metadataReady(frame, aeState);
> > @@ -281,12 +283,11 @@ void IPARkISP1::setControls(unsigned int frame)
> > queueFrameAction.emit(frame, op);
> > }
> >
> > -void IPARkISP1::metadataReady(unsigned int frame, unsigned int aeState)
> > +void IPARkISP1::metadataReady(unsigned int frame, int aeState)
> > {
> > ControlList ctrls(controls::controls);
> >
> > - if (aeState)
> > - ctrls.set(controls::AeLocked, aeState == 2);
> > + ctrls.set(controls::AeState, aeState);
> >
> > RkISP1Action op;
> > op.op = ActionMetadata;
> > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> > index 9d4638ae..5717bc1f 100644
> > --- a/src/libcamera/control_ids.yaml
> > +++ b/src/libcamera/control_ids.yaml
> > @@ -14,16 +14,70 @@ controls:
> >
> > \sa ExposureTime AnalogueGain
> >
> > - - AeLocked:
> > + - AeLock:
> > type: bool
> > description: |
> > - Report the lock status of a running AE algorithm.
> > + Control to lock the AE values.
> > + When set to true, the AE algorithm is locked to its latest parameters,
> > + and will not change exposure settings until set to false.
> > + \sa AeState
>
> I know I am repeating myself here, but I don't see the need to differentiate between
> AeLock and AeEnable == falses.
Please see below.
> > - If the AE algorithm is locked the value shall be set to true, if it's
> > - converging it shall be set to false. If the AE algorithm is not
> > - running the control shall not be present in the metadata control list.
> > + - AeState:
> > + type: int32_t
> > + description: |
> > + Control to report the current AE algorithm state. Enabling or disabling
> > + AE (AeEnable) always resets the AeState to AeStateInactive. The camera
> > + device can do several state transitions between two results, if it is
> > + allowed by the state transition table. For example, AeStateInactive may
> > + never actually be seen in a result.
> >
> > - \sa AeEnable
> > + The state in the result is the state for this image (in sync with this
> > + image). If AE state becomes AeStateConverged, then the image data
> > + associated with the result should be good to use.
> > +
> > + The state transitions mentioned below assume that AeEnable is on.
> > +
> > + \sa AeLock
> > + enum:
> > + - name: AeStateInactive
> > + value: 0
> > + description: |
> > + The AE algorithm is inactive.
> > + If the camera initiates an AE scan, the state shall go to Searching.
> > + If AeLock is on, the state shall go to Locked.
> >
>
> This state is also a bit ambiguous to me. Is this the state that must be reported
> if AeEnable == false? If so, is that not also conveyed by setting the latter?
>
> If the objective of the change is to wholesale do what Android does, then I don't
> really have any objection to this change as-is, and we can treat the above
> states as best we can in the Raspberry Pi IPA. Otherwise, I do see some scope
> to simplify some of this.
First of all, let me clarify the goal. The libcamera native API needs to
offer the features required to implement the Android camera HAL API, but
we don't need to just duplicate it. When there are reasons to depart
(for instance to fix historical mistakes made in Android, to provide
additional features, or just to expose an API we consider more
consistent), we should do so.
Then, a word about the process. Implementing support for an Android
control requires plumbing it through the camera stack: definition of a
libcamera control, translation between the Android and libcamera control
in the Android camera HAL, implementation of the libcamera control in
the pipeline handlesr, and, when applicable, in the IPAs. Defining
libcamera controls, as any API design effort, takes time. To allow
moving forward on all the other parts, we have a fast-track process to
define draft controls that mimick the Android controls. All the draft
controls will be redesigned before freezing the libcamera public API.
This means that AeLock and AeState should be marked as draft controls as
part of this patch.
This being said, we don't need to define controls as draft controls if
we want to already go through a complete design process. The discussion
that stemmed from this patch series goes in that direction.
It's useful, in this context, to understand why the Android AE (and AWB)
controls have been designed this way. Conceptually speaking, the
algorithms are considered, in Android, to be one level above manual
controls.
Controls Metadata
/---------- == OFF ---------\
| v
aeMode -/ +------+ |\
aeLock --> | AE | --> |0\ +--------+
aeExposureCompensation +------+ | | --> | Sensor | --> exposureTime
| | +--------+ sensitivity
exposureTime -----------------------> |1/
sensitivity |/
(The AE controls are in the adnroid.control class, while the sensor
manual control are in the android.sensor class.)
The aeMode control selects whether the AE algorithm (when != OFF) or the
manual controls (when == OFF) drive the sensor. The aeLock control tells
the AE algorithm to freeze its output (when ON) or operate normally
(when OFF). The metadata always report the values applied to the sensor,
regardless of whether they come from the AE algorithm or from manual
controls.
One important difference, compared to the model we currently implement,
is that the Android AE algorithm is independent from the manual sensor
controls. The values of the manual controls aren't affected by the AE
algorithm, and when AE is disabled, the last set manual control values
are applied. This thus require an aeLock control to transition
seamlessly between AE and manual mode, as the application needs to set
the manual controls to the values applied to the sensor on the last
frame that AE was on, which couldn't otherwise be done due to the
internal delays.
The existing libcamera AE controls are ill-defined. There's a \todo
comment to document the interactions between AeEnable and setting a
fixed value for ExposureTime or AnalogueGain. I'm actually not sure what
happens if AeEnable is set to false when ExposureTime and/or
AnalogueGain are 0, or when AeEnable is set to true when ExposureTime
and/or AnalogueGain are not 0.
We need a formal model for how libcamera algorithms interact with manual
controls. It doesn't have to duplicate the Android model, but it has to
offer at least the same features. I believe we need to take it one step
further, as we want to support exposure priority and aperture priority.
I think we should actually talk about gain/sensitivity priority instead
of aperture priority for the existing feature, and already prepare for
devices that have a controllable lens aperture.
Now, the hard question: how should libcamera model this ? :-) Naush,
David, your feedback from a RPi point of view would be very appreciated.
Please also let me know if the above explanation is clear or if you need
more information.
> > + - name: AeStateSearching
> > + value: 1
> > + description: |
> > + The AE algorithm has not converged yet.
> > + If the camera finishes an AE scan, the state shall go to Converged.
> > + If the camera finishes an AE scan, but flash is required, the state
> > + shall go to FlashRequired.
> > + If AeLock is on, the state shall go to Locked.
> > + - name: AeStateConverged
> > + value: 2
> > + description: |
> > + The AE algorithm has converged.
> > + If the camera initiates an AE scan, the state shall go to Searching.
> > + If AeLock is on, the state shall go to Locked.
> > + - name: AeStateLocked
> > + value: 3
> > + description: |
> > + The AE algorithm is locked.
> > + If AeLock is off, the state can go to Searching, Converged, or
> > + FlashRequired.
> > + - name: AeStateFlashRequired
> > + value: 4
> > + description: |
> > + The AE algorithm would need a flash for good results
> > + If the camera initiates an AE scan, the state shall go to Searching.
> > + If AeLock is on, the state shall go to Locked.
> > + - name: AeStatePrecapture
> > + value: 5
> > + description: |
> > + The AE algorithm has started a pre-capture metering session.
> > + After the sequence is finished, the state shall go to Converged if
> > + AeLock is off, and Locked if it is on.
> > + \sa AePrecaptureTrigger
> >
> > # AeMeteringMode needs further attention:
> > # - Auto-generate max enum value.
> > @@ -477,36 +531,6 @@ controls:
> > High quality aberration correction which might reduce the frame
> > rate.
> >
> > - - AeState:
> > - type: int32_t
> > - draft: true
> > - description: |
> > - Control to report the current AE algorithm state. Currently identical to
> > - ANDROID_CONTROL_AE_STATE.
> > -
> > - Current state of the AE algorithm.
> > - enum:
> > - - name: AeStateInactive
> > - value: 0
> > - description: The AE algorithm is inactive.
> > - - name: AeStateSearching
> > - value: 1
> > - description: The AE algorithm has not converged yet.
> > - - name: AeStateConverged
> > - value: 2
> > - description: The AE algorithm has converged.
> > - - name: AeStateLocked
> > - value: 3
> > - description: The AE algorithm is locked.
> > - - name: AeStateFlashRequired
> > - value: 4
> > - description: The AE algorithm would need a flash for good results
> > - - name: AeStatePrecapture
> > - value: 5
> > - description: |
> > - The AE algorithm has started a pre-capture metering session.
> > - \sa AePrecaptureTrigger
> > -
> > - AfState:
> > type: int32_t
> > draft: true
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list