[libcamera-devel] [PATCH] ipu3: ipa: Report correct exposure in request metadata

[Umang Jain]

Hi Kieran,

On 11/30/21 5:01 AM, Kieran Bingham wrote:
> Quoting Umang Jain (2021-11-29 18:15:52)
>> Hi Kieran
>>
>> On 11/29/21 10:58 PM, Kieran Bingham wrote:
>>> Quoting Umang Jain (2021-11-29 17:14:57)
>>>> Hi JM
>>>>
>>>> On 11/29/21 10:43 PM, Jean-Michel Hautbois wrote:
>>>>> Hi Umang,
>>>>>
>>>>> Thanks for the patch !
>>>> Thanks for fast review,  I was about to ping you on IRC
>>>>
>>>>> On 29/11/2021 18:10, Umang Jain wrote:
>>>>>> While populating the ControlList for the request's metadata,
>>>>>> the exposure value should be used computed from AGC algorithm
>>>>>> instead of sensor's exposure.
>>>>>>
>>>>>> The issue is caught while debugging a FULL-level CTS test.
>>>>>>
>>>>>> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
>>>>> Sounds like a bug indeed :-).
>>>> Spiraled my CTS work by 12ish hours :S
>>> Ouch, and I hate to not reply with the easy answer :-( but... ....
>>>
>>>
>>>
>>>>> Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
>>>>>
>>>>>> ---
>>>>>>     src/ipa/ipu3/ipu3.cpp | 2 +-
>>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
>>>>>> index a8d54a5d..9cd80a02 100644
>>>>>> --- a/src/ipa/ipu3/ipu3.cpp
>>>>>> +++ b/src/ipa/ipu3/ipu3.cpp
>>>>>> @@ -632,7 +632,7 @@ void IPAIPU3::parseStatistics(unsigned int frame,
>>> the steps of the IPA are:
>>>
>>>    queue requests
>>>       processControls ( incoming frame settings #1 )
>>>       fillParams ( Configure hardware for this frame #1 )
>>>       parseStatistics ( handle all results of finished frame #1,
>>>                      prepare for next one based on completed frame #2+ ).
>>>
>>> My worry - is that the metadata that should be returned when
>>> parseStatistics completes, is the metadata associated with frame #1.
>>>
>>> I.e. it's the frame that was requested first, and has just completed.
>>>
>>> The exposure set on that frame, is the exposure value that was passed in
>>> from DelayedControls during parseStatistics.
>>>
>>> We /could/ also keep all those values in a circular buffer/queue so we
>>> remember what was asked to be set in #1, when we come round to parsing
>>> the statistics and filling in the metadata...
>>>
>>>
>>>>>>           ctrls.set(controls::ColourTemperature,
>>>>>> context_.frameContext.awb.temperatureK);
>>>>>>     -    ctrls.set(controls::ExposureTime,
>>>>>> context_.frameContext.sensor.exposure *
>>>>>> lineDuration_.get<std::micro>());
>>>>>> +    ctrls.set(controls::ExposureTime,
>>>>>> context_.frameContext.agc.exposure * lineDuration_.get<std::micro>());
>>> However the change here, is now setting the exposure that we want to
>>> calculate, and we'll ask for in an upcoming frame. It's the output of
>>> the most recent algorithm calculation, and therefore is not the exposure
>>> value for the frame which has just completed, and therefore it's not the
>>> right value to be putting in the metadata for the completed request...
>>>
>>> Can you provide some more context of what led you to this fix please?
>>
>> The context is, a control - ExposureTimeMode (not present on master), if
>> set to "disabled" - should immediately stop changing exposure. This is
>> tested as a CTS test.
>>
>> what happens is :
>>
>> - Few requests(~1-4th for example) are queued initially with "auto" mode
>> - exposure can change
>> - Subsequent requests (4th - end_of_test) requests are queued with
>> "disabled" mode - exposure shouldn't change
> Ok, this should be handled during processControls().
>
> When the mode is set to disabled, we should be locking the exposure and
> gain values, and setting only those, (while still allowing the
> algorithms to run anyway...).


yes, this is already implemented on my side

>
>
>
>> - I can control the Algorithms (AGC in this case) to stop setting
>> IPAFramecontext.agc.exposure because we are in "disabled" mode; as early
>> as 4th request
> We should be able to lock them immediately. (Of course what we lock them
> to should be something that makes sense and is useful?)


implemented already

>
>
>> - Now the last exposure computed will be / should be used to set
>> exposure on the sensor for future frames
>> - Since delayed controls is involved, I only see sensor exposure getting
>> fixed (locked?) to a certain value only /after/ some _more) frames are
>> passed i.e. the lag between exposure gets "fixed"
> When processControls() tells us to lock/disable/whichever - we should be
> making sure at that point we store the current state, and set a flag to
> ensure that we only refer to the stored state for controls. That can


already implemented

> mean we never set any more control through delayed controls, and the
> state will stay where it was.


Ok, I think I need to place a check on delayed controls settings rather 
than on metadata.

>
> This sounds like something that was put on my plate to implement anyway,
> as I muttered something about it being 'easy', but haven't yet got round
> to it.
>
>> So it fails the test. There are more failures on the test, but those are
>> separate blocks of issues.
>>
>> Somehow we need to fix the exposure(in real or virtually**) /as soon as/
>> we get into ExposureTimeMode'Disabled' mode.
> It's a matter of handling the control when it is set. We simply don't do
> it yet. And I dont' think it's much to implement.


it already does, just not where I thought it should be. I might be 
mis-guided with IPAFrameContext::Sensor::Exposure with 
IPAFrameContext::Agc::Exposure and its non-sync use as denoted if you 
apply same concept of the patch on gain a few lines above.

I'll send a patch to properly report sensor gain as part of metadata 
instead of algorithm's

>
> I'll take a look at this tomorrow, and go through it with you if you
> want.
>
> Unless you can see what needs to be done from the above text and get
> there before I come online in the morning my time ;-)
>
>
>   - Upon receipt of a control disabling the AGC algorithm, we maintain a
>     flag to state if we're enabled or disabled (or locked? I haven't
>     checked the terminology or details yet).
>
>   - If we have locked AE/AGC ... we prevent setting any delayed controls
>     through setControls(). That may be either setting only values that we
>     have cached, or it may mean ... simply not calling to reset it.
>
>   - Manual settings of the Exposure/Gain then become values that also
>     override through setControls() so that only the manual values can be
>     used if provided. (We will likely track this with something like
>     std::optional, so we know if we have a value, we use it).
>
>> I agree with your rationale above, but looks like controlling the
>> algorithms is not enough to get it pass. The delayedControls lag of
>> stablising the exposure needs to be taken care of either by introducing
>> a ring-buffer of frames or using the algorithms value (since it can be
>> controlled) for now, as part of metadata.
> Anyway, those are my notes/thoughts on what I think we need to try to
> implement to resolve this. We shouldn't set the metadata from the
> 'wrong' frame. And I don't think we even need a ring buffer to solve
> this. It's about handling the request control at the processControls().


Cool, my problem is resolved for now. Most of the my implementatoin 
already compliant with what you noted down, so I think it's headed in 
the right direction.

> --
> Kieran
>
>
>>> --
>>> Kieran
>>>
>>>
>>>>>>           /*
>>>>>>          * \todo The Metadata provides a path to getting extended data
>>>>>>