[libcamera-devel] [RFC PATCH 03/14] controls: Replace AeLocked with AeState, and add AeLock
David Plowman
david.plowman at raspberrypi.com
Wed Jun 23 09:57:04 CEST 2021
Hi Jacopo
Thanks for taking more time to explain this!
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"?
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