[libcamera-devel] [RFC PATCH 03/14] controls: Replace AeLocked with AeState, and add AeLock
Jacopo Mondi
jacopo at jmondi.org
Wed Jun 23 10:13:03 CEST 2021
Hi David,
On Wed, Jun 23, 2021 at 08:57:04AM +0100, David Plowman wrote:
> Hi Jacopo
>
> Thanks for taking more time to explain this!
I'm just trying to wrap my head around this as well, it's far from being an
"explanation" :)
>
> On Tue, 22 Jun 2021 at 17:11, Jacopo Mondi <jacopo at jmondi.org> wrote:
> >
> > Hi David,
> >
> > On Mon, Jun 21, 2021 at 10:15:21AM +0100, David Plowman wrote:
> > > Hi Paul
> > >
> > > Thanks for the extra explanations.
> > >
> > > On Mon, 21 Jun 2021 at 10:05, <paul.elder at ideasonboard.com> wrote:
> > > >
> > > > Hi Naush,
> > > >
> > > > Thank you for the feedback.
> > > >
> > > > On Fri, Jun 18, 2021 at 02:58:52PM +0100, Naushir Patuck wrote:
> > > > >
> > > > >
> > > > > On Fri, 18 Jun 2021 at 14:13, David Plowman <david.plowman at raspberrypi.com>
> > > > > wrote:
> > > > >
> > > > > Hi Naush, everyone
> > > > >
> > > > > On Fri, 18 Jun 2021 at 13:07, Naushir Patuck <naush at raspberrypi.com> wrote:
> > > > > >
> > > > > > Hi Paul,
> > > > > >
> > > > > > Thank you for your patch.
> > > > > >
> > > > > > On Fri, 18 Jun 2021 at 11:34, Paul Elder <paul.elder at ideasonboard.com>
> > > > > 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>
> > > > > >> ---
> > > > > >> 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 1c1e802a..ad0132c0 100644
> > > > > >> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > > > > >> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > > > > >> @@ -468,7 +468,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);
> > > > > >
> > > > > >
> > > > > > I think, strictly speaking, if agcStatus->locked == true, we should
> > > > > return
> > > > > > control::AeStateConverged by the definition below. David, do you agree?
> > > >
> > > > Oh shoot, that is indeed what I meant. I got mixed up with the status
> > > > name :/
> > > >
> > > > >
> > > > > Hmm, indeed. My reading is that what we used to know as AeEnable has
> > > > > now been renamed/replaced by AeLock, is that right? And as you say,
> > > > > "locked" does not mean "converged" (any more).
> > > > >
> > > > >
> > > > > Yes I agree, but we now have both AeEnable and AeLock defined with this change.
> > > >
> > > > AeEnable is for enabling/disabling the AE routine. AeLock is to lock the
> > > > AE values... which would normally be the same as turning off AE, but you
> > > > want to be able to unlock AE without having to restart the routine.
> > >
> > > I'm still struggling a bit with this, however. What would it mean to
> > > unlock the AE without restarting it? Isn't it then still fixed at
> > > whatever values it had? Perhaps you could provide an example of this
> > > behaviour which would show how it is different.
> > >
> > > Sorry for still being puzzled...
> >
> > And I'm sorry as well for asking possibly stupid questions, but:
> >
> > - It seems to me the RPi AGC routine has two Pause/Resume functions that:
> > - Pause: sets fixed gain and shutter time to the last computed
> > shutter and gain values
> > - Resume: re-sets them to 0
> >
> > - In the RPi AEGC implementation, if fixed shutter and gains are set
> > then those values are used
> >
> > - The AEGC routine is paused/resumed on controls::AeEnable on/off
> >
> > - When AeEnable == Off, then the only way to override the fixed
> > shutter/gain is by using the ExposureTime and AnalogueGain controls
> > (ie manual shutter/gain control)
> >
> > So in you implementation disabling AE has the effect that the AEGC
> > routine uses the last computed values until a new shutter/gain value
> > is set by the application (through what is usually called "manual" mode).
> >
> > The difference between your implementation and a new one that uses
> > AeLocked and AeEnable is, in my understanding, that when the AeLocked
> > is set the AEGC routine is still in control of the shutter/gain which
> > gets "fixed" as you're doing, but it does not accept manual control of
> > the shutter/gain like you're doing unless the AEGC is disabled.
> >
> > This seems to be suggested by the following paragraph in the Android
> > documentation:
> >
> > -------------------------------------------------------------------------------
> >
> > If an application is switching between automatic and manual control
> > and wishes to eliminate any flicker during the switch, the following
> > procedure is recommended:
> >
> > Starting in auto-AE mode:
> > - Lock AE
> > - Wait for the first result to be output that has the AE locked
> > - Copy exposure settings from that result into a request, set the request to manual AE
> > - Submit the capture request, proceed to run manual AE as desired.
> >
> > See android.control.aeState for AE lock related state transition details.
> > -------------------------------------------------------------------------------
> >
> > If we were able to report the AEGC convergence state I think the same
> > could be done without AeLock, by disabling AEGC once it has converged
> > and continue from there with manual exposure. But my understanding is that
> > having a lock controls guarantees that once the AEGC has
> > converged it won't change, so that the in-flight requests that are
> > completed before the first one with manual AE control is processed
> > won't have floating AEGC values,
> >
> > (Let me try to draw it down, hoping it render ok with your client,
> > otherwise https://paste.debian.net/1202043/)
> >
> > Queue of
> > submitted Completed AeState in the
> > Requests Requests completed Request
> > -----------------------------------------------------------------
> >
> > 0 (AeLock)
> > 1
> > 2
> > 3 0 Searching
> > 4 1 Converged
> > 5 (AeDisable + 2 Locked <- COPY AE VALUES
> > Manual AE with 3 Locked
> > settings from #2) 4 Locked
> > 5 Use Manual AE
>
> Are we saying that AeDisable means "stop adjusting the AEC/AGC
> immediately, whether it's converged or not", but AeLock means "wait
> until it's converged and then immediately stop adjusting the AEC/AGC"?
If AeLock is set on a Converged state then yes, the values are not
changed until the lock is lifted.
Does the transition table repoted here helps ?
https://developer.android.com/reference/android/hardware/camera2/CaptureResult#CONTROL_AE_STATE
Thanks
j
>
> Thanks!
> David
>
> >
> > Without a Lock the AEGC routine would keep adjusting its values for
> > requests 3 and 4. Request 5 will contain settings copied from Request
> > 2 so a small hiccup might happen. Of course, if the number of frames
> > required to get to a converged state is longer, the duration of the
> > possible AE adjustment period is longer, making the flickering more
> > visible ?
> >
> > Does this make sense ?
> >
> > The android.control.aeMode documentation also reports that:
> >
> > -----------------------------------------------------------------
> > Note that auto-white balance (AWB) and auto-focus (AF) behavior is
> > device dependent when AE is in OFF mode. To have consistent behavior
> > across different devices, it is recommended to either set AWB and AF to
> > OFF mode or lock AWB and AF before setting AE to OFF.See
> > android.control.awbMode,
> > android.control.afMode,android.control.awbLock, and
> > android.control.afTrigger for more details.
> > -----------------------------------------------------------------
> >
> > Suggesting that the AF AWB and AE locks might play a role on the interactions
> > between the three algorithms.
> >
> > Does this make sense and provide a bit more of context about what
> > AeLock could be used for ?
> >
> > Thanks
> > j
> >
> >
> > >
> > > Thanks!
> > > David
> > >
> > > > That's the idea afaict.
> > > >
> > > > Supporting AeLock is optional, of course. Same for AeState, not all of
> > > > the states need to be supported. You can specify which ones are
> > > > supported in pipeline's ControlInfoMap.
> > > >
> > > >
> > > > Thanks,
> > > >
> > > > Paul
> > > >
> > > > > Perhaps combining them to avoid redundancy makes sense? Or do folks think they
> > > > > can they serve different purposes?
> > > > >
> > > > >
> > > > >
> > > > > So:
> > > > >
> > > > > We set controls::AeLocked in the metadata according to the value that
> > > > > we received in the corresponding control (which we now receive instead
> > > > > of AeEnable), and
> > > > >
> > > > > We set controls::AeState in the metadata to AeStateConverged when
> > > > > agcStatus->locked is true, otherwise to AeStateSearching. We should
> > > > > probably bite the bullet and rename that "locked" field to
> > > > > "converged", otherwise it's going to cause confusion for the rest of
> > > > > eternity.
> > > > >
> > > > >
> > > > > Agree, I think we should rename "locked" to "converged" in the controller
> > > > > metadata struct.
> > > > >
> > > > > Regards,
> > > > > Naush
> > > > >
> > > > >
> > > > >
> > > > > David
> > > > >
> > > > > >
> > > > > > Thanks,
> > > > > > Naush
> > > > > >
> > > > > >>
> > > > > >> libcameraMetadata_.set(controls::DigitalGain,
> > > > > agcStatus->digital_gain);
> > > > > >> }
> > > > > >>
> > > > > >> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > > > > >> index b47ea324..88a562a8 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
> > > > > >>
> > > > > >> - 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.
> > > > > >> + - 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
> > > > > >> --
> > > > > >> 2.27.0
> > > > > >>
> > > > >
More information about the libcamera-devel
mailing list