[libcamera-devel] [RFC PATCH 03/14] controls: Replace AeLocked with AeState, and add AeLock

[Laurent Pinchart]

Hi David,

On Wed, Jun 23, 2021 at 04:19:55PM +0100, David Plowman wrote:
> On Wed, 23 Jun 2021 at 11:56, Naushir Patuck wrote:
> > On Wed, 23 Jun 2021 at 11:32, Jacopo Mondi wrote:
> >> On Wed, Jun 23, 2021 at 10:06:00AM +0100, Naushir Patuck wrote:
> >>> On Wed, 23 Jun 2021 at 09:12, Jacopo Mondi wrote:
> >>>
> >>> <snip>
> >>>
> >>>>> 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"?
> >>>>
> >>>> If AeLock is set on a Converged state then yes, the values are not
> >>>> changed until the lock is lifted.
> >>>>
> >>>> Does the transition table repoted here helps ?
> >>>>
> >>>> https://developer.android.com/reference/android/hardware/camera2/CaptureResult#CONTROL_AE_STATE
> >>>
> >>> Having a brief look at the above document, I think
> >>> Jacopo's description makes
> >>> sense now.  Enabling AeLock locks the current AE computed values, whether
> >>> the algorithm has converged or not.  However, with AeLock set to on, other
> >>> controls are effectively ignored by the AE (e.g. aePrecaptureTrigger, maybe
> >>> other things as well).  Not sure why this is the case, as the application
> >>> surely
> >>> knows what state it wants the AE to be in...?
> >>
> >> So my previous example is slightly wrong, as the AeLock was meant to
> >> be set only once the AE has converged, not in request #0.
> >>
> >>> We don't deal with pre-capture sequences in RPi AE, so for us AeLock and
> >>> AeDisable are actually the same thing.  Thinking about it, even with a
> >>> pre-capture
> >>> sequence, we could probably say the same.  This may also be the case for
> >>> other vendor AE algorithms, but Android makes these states explicit.
> >>
> >> I think that's fine.
> >>
> >> That's more of a philosophical question, but I don't think we should
> >> try to impose all the IPA implementation to behave exactly the same,
> >> as that would impede the implementation of the most advanced features.
> >> The means the most performant application won't behave exactly the
> >> same between different platform, but I think at least in the controls
> >> definition we should try to get to the minimum common denominator that
> >> satisfies most needs. After all I think Android does the same, the
> >> definition is there for all platforms, but each implementation is
> >> slightly different and indeed it's usually the phone manufacturer
> >> provided camera app that actually makes use of the most advanced
> >> features, while a generic camera app is usually limited to the
> >> simplest use cases.
> >>
> >> For this specific use case (the AECG lock/state/enable), let me try to
> >> summarize how we could move forward:
> >>
> >> - Replace controls::AeLocked with a more descriptive AeState where the
> >>   current 'locked' is represented by the 'Converged' state.
> >> - Introduce controls::AeLock (which for RPi won't be supported)
> >> - We have controls::AeEnable which more or less maps to
> >>   android.AeMode. AeMode has more states which include control of the
> >>   flash. We do not strictly have to support it now afaict but moving
> >>   controls::AeMode from being a boolean on/off to a list of states
> >>   might help that transition to happen in future.
> >>
> >> Do you agree with my understanding here ?
> >
> > That all sounds good to me, but I would like David to have his say as
> > well.  It will probably be the case that our IPA will treat controls::AeLock
> > the same as our "Paused" state.
> 
> I think we treat "lock" essentially the same as "enable". Maybe we
> ignore manual settings in the former but not the latter, but it's not
> sounding like a big deal. Will discuss with Naush when I'm next in the
> office.
> 
> >> The controls::AeLocked -> controls::AeState[Converged] will require us
> >> and the corresponding RPi camera app update to happen synchronously
> >> even if I admit I don't know how you distribute libcamera. IOW, are the
> >> camera apps compiled against the most recent master or against a
> >> frozen version? So that master can freely advance and the app updates can
> >> follow later once you update the version of libcamera you ship.
> >
> > We currently track master, so will have to update our apps separately when this
> > change goes through.  Eventually, we will freeze versions in our distribution to
> > avoid breakages, but we are not there yet.
> 
> I'd prefer to attach ourselves to a particular libcamera version once
> we have frozen a first version of the API, though maybe that's still
> too far off? I'd also like to sort out our sporadic crash
> (https://bugs.libcamera.org/show_bug.cgi?id=26) before doing that too.

That one is still on my todo list, it will get fixed.

> >>> Or it could be that I've just completely misunderstood that document :-)
> >>
> >> The fact we all keep asking that to ourselves makes me wonder if we're
> >> all a bunch of idiots or if we're actually exploring rather new
> >> territories in the open camera support landscape. I think time will
> >> tell :)
> >>
> >> PS As you might have noticed we're actually in the process of
> >> addressing the most advanced controls that android requires for
> >> high-end devices, so expect this kind of discussions to happen again
> >> in future :)

-- 
Regards,

Laurent Pinchart