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

[David Plowman]

Hi Naush, Jacopo

On Wed, 23 Jun 2021 at 11:56, Naushir Patuck <naush at raspberrypi.com> wrote:
>
> Hi Jacopo,
>
>
> On Wed, 23 Jun 2021 at 11:32, Jacopo Mondi <jacopo at jmondi.org> wrote:
>>
>> Hi Naush,
>>
>> On Wed, Jun 23, 2021 at 10:06:00AM +0100, Naushir Patuck wrote:
>> > Hi Jacopo and David,
>> >
>> > On Wed, 23 Jun 2021 at 09:12, Jacopo Mondi <jacopo at jmondi.org> 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
>> > >
>> > > Thanks
>> > >    j
>> > >
>> >
>> > 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.

Thanks
David

>
> Regards,
> Naush
>
>>
>> >
>> >  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 :)
>>
>> Thanks
>>    j
>>
>> 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 :)
>>
>> >
>> > Naush