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

[Kieran Bingham]

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?

--
Kieran


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