[libcamera-devel] [RFC PATCH 03/14] controls: Replace AeLocked with AeState, and add AeLock
David Plowman
david.plowman at raspberrypi.com
Mon Jun 21 11:15:21 CEST 2021
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...
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