[libcamera-devel] [RFC PATCH 03/14] controls: Replace AeLocked with AeState, and add AeLock
Naushir Patuck
naush at raspberrypi.com
Fri Jun 18 15:58:52 CEST 2021
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?
>
> 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.
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
> >>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210618/0cc032a6/attachment-0001.htm>
More information about the libcamera-devel
mailing list