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

[Umang Jain]

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
- 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
- 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"

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.

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.


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