[libcamera-devel] [RFC PATCH v3 05/16] controls: Replace AeLocked with AeState, and add AeLock
Naushir Patuck
naush at raspberrypi.com
Mon Jul 5 19:34:07 CEST 2021
Hi Paul,
Thank you for your work.
On Fri, 2 Jul 2021 at 11:38, 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>
>
> ---
> No change in v3
> ---
> 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 4d09a84f..4981aa29 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -469,7 +469,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);
>
Yes, this seems like the correct state mapping.
> libcameraMetadata_.set(controls::DigitalGain,
> agcStatus->digital_gain);
> }
>
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index cdfb4d13..4eca26e2 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
>
I know I am repeating myself here, but I don't see the need
to differentiate between
AeLock and AeEnable == falses.
> - 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.
>
This state is also a bit ambiguous to me. Is this the state that must be
reported
if AeEnable == false? If so, is that not also conveyed by setting the
latter?
If the objective of the change is to wholesale do what Android does, then I
don't
really have any objection to this change as-is, and we can treat the above
states as best we can in the Raspberry Pi IPA. Otherwise, I do see some
scope
to simplify some of this.
Regards,
Naush
> + - 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/20210705/ae9f2d02/attachment-0001.htm>
More information about the libcamera-devel
mailing list