| Message ID | 20211129171032.469605-1-umang.jain@ideasonboard.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, Thanks for the patch ! 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@ideasonboard.com> Sounds like a bug indeed :-). Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@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, > > 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>()); > > /* > * \todo The Metadata provides a path to getting extended data >
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@ideasonboard.com> > > Sounds like a bug indeed :-). Spiraled my CTS work by 12ish hours :S > Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@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, >> 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>()); >> /* >> * \todo The Metadata provides a path to getting extended data >>
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@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@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 > >>
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@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@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 >>>>
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@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@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 > >>>>
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@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@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 >>>>>>
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, 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>()); /* * \todo The Metadata provides a path to getting extended data
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@ideasonboard.com> --- src/ipa/ipu3/ipu3.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)