[libcamera-devel] [RFC PATCH 03/14] controls: Replace AeLocked with AeState, and add AeLock
Jacopo Mondi
jacopo at jmondi.org
Tue Jun 22 18:12:26 CEST 2021
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
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