[libcamera-devel] [RFC PATCH 03/14] controls: Replace AeLocked with AeState, and add AeLock
Naushir Patuck
naush at raspberrypi.com
Mon Jun 28 11:14:02 CEST 2021
Hi Laurent,
On Mon, 28 Jun 2021 at 01:49, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:
> Hello everybody,
>
> On Tue, Jun 22, 2021 at 06:12:26PM +0200, Jacopo Mondi wrote:
> > On Mon, Jun 21, 2021 at 10:15:21AM +0100, David Plowman wrote:
> > > On Mon, 21 Jun 2021 at 10:05, <paul.elder at ideasonboard.com> wrote:
> > >> On Fri, Jun 18, 2021 at 02:58:52PM +0100, Naushir Patuck wrote:
> > >>> On Fri, 18 Jun 2021 at 14:13, David Plowman wrote:
> > >>>> On Fri, 18 Jun 2021 at 13:07, Naushir Patuck wrote:
> > >>>>> On Fri, 18 Jun 2021 at 11:34, Paul Elder 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 ?
>
> I'm still a bit puzzled by the need for an AeLock control. I do
> understand that it's used in Android to switch to manual exposure
> without glitches, as detailed above. However, couldn't the same result
> be achieved by switching AeMode from on to off (thus switching to manual
> mode), without specifying manual values for the sensitivity, exposure
> and frame duration ? The camera would then keep the last values computed
> by the AEGC, which would be reported in metadata, and could be then used
> as a basis to set other manual values.
>
I agree with the sentiment that AeLock and AeEnable sound like the same
thing,
and the Raspberry Pi AGC will almost certainly be treating it the same to
achieve
what the user expects without glitching.
>
> This may be related to defining what happens to manual control values
> when running in auto mode. The camera will report computed values for
> the sensitivity, exposure time and frame duration in metadata, but
> should those values be implicitly copied to the request controls
> (effectively only, not really adding them to the Request instances) when
> the algorithm is switched off, or should the last value that was set
> manually be used in that case ?
In my opinion, the values returned in the metadata, particularly
controls::ExposureTime and controls::AnalogueGain should be the values
used for that frame. So if the AE is switched off, we should still get
these
values returned in the Request, as the currently used values (which may
be set manually, or frozen to what the AE last selected).
I did have one bit of confusion in the paragraph above. If we have manual
controls set, does that not imply that AE is not running in auto mode, so
the
two operations are mutually exclusive?
Regards,
Naush
> I think we need to clearly specify this,
> ideally in a consistent fashion across all controls. If we decide that
> the camera behaviour should be to implicitly copy the metadata value to
> the control value, then AeLock doesn't seem to be needed. Otherwise,
> switching seamlessly from auto to manual mode requires a mechanism
> similar to AeLock.
>
> Is there any other need for the AeLock control that I may have missed ?
>
> > >> 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.
> > >>
> > >>> 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.
> > >>>
> > >>>>>> 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
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210628/38b4a2b6/attachment-0001.htm>
More information about the libcamera-devel
mailing list