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

[Kieran Bingham]

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...).



> - 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?)


> - 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
mean we never set any more control through delayed controls, and the
state will stay where it was.

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.

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().
--
Kieran


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