| Message ID | 20211029120001.2469018-3-hanlinchen@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Han-Lin Chen (2021-10-29 12:59:58) > Set the statistics with the latest AE AiqResults which has the same > exposure time and analog gain. The patch reduces the AE haunting during s/haunting/hunting/ Although appropriate for Halloween ;-) > the converging process. > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > --- > aiq/aiq_input_parameters.cpp | 2 +- > ipu3.cpp | 66 ++++++++++++++++++++++++++++++------ > 2 files changed, 56 insertions(+), 12 deletions(-) > > diff --git a/aiq/aiq_input_parameters.cpp b/aiq/aiq_input_parameters.cpp > index bc87b31..46553a6 100644 > --- a/aiq/aiq_input_parameters.cpp > +++ b/aiq/aiq_input_parameters.cpp > @@ -130,7 +130,7 @@ AiqInputParameters &AiqInputParameters::operator=(const AiqInputParameters &othe > > void AiqInputParameters::setAeAwbAfDefaults() > { > - /*Ae Params */ > + /* Ae Params */ > aeInputParams.num_exposures = NUM_EXPOSURES; > aeInputParams.frame_use = ia_aiq_frame_use_preview; > aeInputParams.flash_mode = ia_aiq_flash_mode_off; > diff --git a/ipu3.cpp b/ipu3.cpp > index 8126e9d..f463805 100644 > --- a/ipu3.cpp > +++ b/ipu3.cpp > @@ -59,7 +59,8 @@ private: > void fillParams(unsigned int frame, ipu3_uapi_params *params); > void parseStatistics(unsigned int frame, > int64_t frameTimestamp, > - const ipu3_uapi_stats_3a *stats); > + const ipu3_uapi_stats_3a *stats, > + const ControlList& sensorCtrls); > > void setControls(unsigned int frame); > > @@ -84,6 +85,7 @@ private: > /* Temporary storage until we have a FrameContext object / struct */ > aiq::AiqInputParameters aiqInputParams_; > aiq::AiqResults results_; Is this still needed (results_)? Isn't it just stored in the history ring buffer? > + aiq::AiqResultsRingBuffer resultsHistory_; > > BinaryData aiqb_; > BinaryData nvm_; > @@ -282,6 +284,8 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo, > /* Upate the camera controls using the new sensor settings. */ > updateControls(sensorInfo_, ctrls_, ipaControls); > > + resultsHistory_.Reset(); > + > return 0; > } > > @@ -327,7 +331,10 @@ void IPAIPU3::processEvent(const IPU3Event &event) > const ipu3_uapi_stats_3a *stats = > reinterpret_cast<ipu3_uapi_stats_3a *>(mem.data()); > > - parseStatistics(event.frame, event.frameTimestamp, stats); > + parseStatistics(event.frame, > + event.frameTimestamp, > + stats, > + event.sensorControls); > break; > } > case EventFillParams: { > @@ -374,14 +381,16 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params) > */ > > /* Run algorithms into/using this context structure */ > - if (frame % 10 == 0) > - aiq_.run2a(frame, aiqInputParams_, results_); > + aiq_.run2a(frame, aiqInputParams_, results_); Very happy to see the artifical run delay removed. > aic_.updateRuntimeParams(results_); > aic_.run(params); > > exposure_ = results_.ae()->exposures[0].sensor_exposure->coarse_integration_time; > gain_ = results_.ae()->exposures[0].sensor_exposure->analog_gain_code_global; > + > + resultsHistory_.Push(results_); > + Ah, ok so you are still using the results_ ... but is this introducing unnecessary copies? Can't we just decide / create a new entry in the resultsHistory_ (and later rename that to be 'results_') and directly run the run2a call into that active entry? > setControls(frame); > > IPU3Action op; > @@ -392,7 +401,8 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params) > > void IPAIPU3::parseStatistics(unsigned int frame, > int64_t frameTimestamp, > - const ipu3_uapi_stats_3a *stats) > + const ipu3_uapi_stats_3a *stats, > + const ControlList& sensorCtrls) > { > ControlList ctrls(controls::controls); > > @@ -403,10 +413,43 @@ void IPAIPU3::parseStatistics(unsigned int frame, > */ > > ASSERT (frameTimestamp > 0); > - aiq_.setStatistics(frame, frameTimestamp, results_, stats); > + > + /* Ae algorithm expects the statistics to be set with its corresponding > + * Ae result, i.e., the Ae result should match the exposure time and > + * analog gain with the the effective sensor controls of the statistics. > + * Search the required Ae result in the result history and combine it > + * with the latest result as the input to AIQ::setStatistics(). > + */ > + > + int32_t effectiveExpo = 0; > + int32_t effectiveGain = 0; > + ControlValue ctrlValue; > + > + ctrlValue = sensorCtrls.get(V4L2_CID_EXPOSURE); > + if (!ctrlValue.isNone()) > + effectiveExpo = ctrlValue.get<int32_t>(); > + > + ctrlValue = sensorCtrls.get(V4L2_CID_ANALOGUE_GAIN); > + if (!ctrlValue.isNone()) > + effectiveGain = ctrlValue.get<int32_t>(); > + > + auto pred = [effectiveExpo, effectiveGain] (aiq::AiqResults& result) { > + ia_aiq_exposure_sensor_parameters* sensorExposure = > + result.ae()->exposures[0].sensor_exposure; > + > + return (effectiveExpo == sensorExposure->coarse_integration_time || > + effectiveGain == sensorExposure->analog_gain_code_global); > + }; > + aiq::AiqResults& aeMatchedResults = resultsHistory_.searchBackward(pred, results_); This is an interesting implementation! (in a good way), though I wonder if it's over complicated. I thought we'd be able to do something simpler, because we know the frame number index, so we should be able to associate that with the resultsHistory_ and obtain the results from the frame that was queued with the same 'frame' number... Could you check the frame numbers of when the parameters are queued up, against the 'frame' that comes in here, and see if it matches up with your implementation as a suitable method to tie the results together? > + aiq::AiqResults combinedResults = results_; > + combinedResults.setAe(aeMatchedResults.ae()); Oh - that's interesting, so we take the latest calculated results, and the ae() from the matched results.. > + > + /* Aiq library expects timestamp in microseconds */ Perhaps std::chrono would help us here in the future. Doesn't have to be now. > + aiq_.setStatistics(frame, (frameTimestamp / 1000), combinedResults, stats); > > /* Set frame durations from exposure results */ > - ia_aiq_exposure_sensor_parameters *sensorExposure = results_.ae()->exposures->sensor_exposure; > + ia_aiq_exposure_sensor_parameters *sensorExposure = combinedResults.ae()->exposures->sensor_exposure; > int64_t frameDuration = (sensorExposure->line_length_pixels * sensorExposure->frame_length_lines) / > (sensorInfo_.pixelRate / 1e6); > ctrls.set(controls::FrameDuration, frameDuration); > @@ -423,10 +466,11 @@ void IPAIPU3::setControls(unsigned int frame) > IPU3Action op; > op.op = ActionSetSensorControls; > > - ControlList ctrls(ctrls_); > - ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_)); > - ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain_)); > - op.controls = ctrls; > + ControlList sensorCtrls(ctrls_); > + sensorCtrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_)); > + sensorCtrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain_)); > + > + op.sensorControls = sensorCtrls; > > queueFrameAction.emit(frame, op); > } > -- > 2.33.1.1089.g2158813163f-goog >
Hi Kieran, On Tue, Nov 2, 2021 at 9:58 PM Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Quoting Han-Lin Chen (2021-10-29 12:59:58) > > Set the statistics with the latest AE AiqResults which has the same > > exposure time and analog gain. The patch reduces the AE haunting during > > s/haunting/hunting/ > > Although appropriate for Halloween ;-) > > > the converging process. > > > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > > --- > > aiq/aiq_input_parameters.cpp | 2 +- > > ipu3.cpp | 66 ++++++++++++++++++++++++++++++------ > > 2 files changed, 56 insertions(+), 12 deletions(-) > > > > diff --git a/aiq/aiq_input_parameters.cpp b/aiq/aiq_input_parameters.cpp > > index bc87b31..46553a6 100644 > > --- a/aiq/aiq_input_parameters.cpp > > +++ b/aiq/aiq_input_parameters.cpp > > @@ -130,7 +130,7 @@ AiqInputParameters &AiqInputParameters::operator=(const AiqInputParameters &othe > > > > void AiqInputParameters::setAeAwbAfDefaults() > > { > > - /*Ae Params */ > > + /* Ae Params */ > > aeInputParams.num_exposures = NUM_EXPOSURES; > > aeInputParams.frame_use = ia_aiq_frame_use_preview; > > aeInputParams.flash_mode = ia_aiq_flash_mode_off; > > diff --git a/ipu3.cpp b/ipu3.cpp > > index 8126e9d..f463805 100644 > > --- a/ipu3.cpp > > +++ b/ipu3.cpp > > @@ -59,7 +59,8 @@ private: > > void fillParams(unsigned int frame, ipu3_uapi_params *params); > > void parseStatistics(unsigned int frame, > > int64_t frameTimestamp, > > - const ipu3_uapi_stats_3a *stats); > > + const ipu3_uapi_stats_3a *stats, > > + const ControlList& sensorCtrls); > > > > void setControls(unsigned int frame); > > > > @@ -84,6 +85,7 @@ private: > > /* Temporary storage until we have a FrameContext object / struct */ > > aiq::AiqInputParameters aiqInputParams_; > > aiq::AiqResults results_; > > Is this still needed (results_)? Isn't it just stored in the history ring buffer? > > > + aiq::AiqResultsRingBuffer resultsHistory_; > > > > BinaryData aiqb_; > > BinaryData nvm_; > > @@ -282,6 +284,8 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo, > > /* Upate the camera controls using the new sensor settings. */ > > updateControls(sensorInfo_, ctrls_, ipaControls); > > > > + resultsHistory_.Reset(); > > + > > return 0; > > } > > > > @@ -327,7 +331,10 @@ void IPAIPU3::processEvent(const IPU3Event &event) > > const ipu3_uapi_stats_3a *stats = > > reinterpret_cast<ipu3_uapi_stats_3a *>(mem.data()); > > > > - parseStatistics(event.frame, event.frameTimestamp, stats); > > + parseStatistics(event.frame, > > + event.frameTimestamp, > > + stats, > > + event.sensorControls); > > break; > > } > > case EventFillParams: { > > @@ -374,14 +381,16 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params) > > */ > > > > /* Run algorithms into/using this context structure */ > > - if (frame % 10 == 0) > > - aiq_.run2a(frame, aiqInputParams_, results_); > > + aiq_.run2a(frame, aiqInputParams_, results_); > > Very happy to see the artifical run delay removed. > > > > > aic_.updateRuntimeParams(results_); > > aic_.run(params); > > > > exposure_ = results_.ae()->exposures[0].sensor_exposure->coarse_integration_time; > > gain_ = results_.ae()->exposures[0].sensor_exposure->analog_gain_code_global; > > + > > + resultsHistory_.Push(results_); > > + > > Ah, ok so you are still using the results_ ... but is this introducing > unnecessary copies? Can't we just decide / create a new entry in the > resultsHistory_ (and later rename that to be 'results_') and directly > run the run2a call into that active entry? Yes. Will remove the "results_". > > > > setControls(frame); > > > > IPU3Action op; > > @@ -392,7 +401,8 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params) > > > > void IPAIPU3::parseStatistics(unsigned int frame, > > int64_t frameTimestamp, > > - const ipu3_uapi_stats_3a *stats) > > + const ipu3_uapi_stats_3a *stats, > > + const ControlList& sensorCtrls) > > { > > ControlList ctrls(controls::controls); > > > > @@ -403,10 +413,43 @@ void IPAIPU3::parseStatistics(unsigned int frame, > > */ > > > > ASSERT (frameTimestamp > 0); > > - aiq_.setStatistics(frame, frameTimestamp, results_, stats); > > + > > + /* Ae algorithm expects the statistics to be set with its corresponding > > + * Ae result, i.e., the Ae result should match the exposure time and > > + * analog gain with the the effective sensor controls of the statistics. > > + * Search the required Ae result in the result history and combine it > > + * with the latest result as the input to AIQ::setStatistics(). > > + */ > > + > > + int32_t effectiveExpo = 0; > > + int32_t effectiveGain = 0; > > + ControlValue ctrlValue; > > + > > + ctrlValue = sensorCtrls.get(V4L2_CID_EXPOSURE); > > + if (!ctrlValue.isNone()) > > + effectiveExpo = ctrlValue.get<int32_t>(); > > + > > + ctrlValue = sensorCtrls.get(V4L2_CID_ANALOGUE_GAIN); > > + if (!ctrlValue.isNone()) > > + effectiveGain = ctrlValue.get<int32_t>(); > > + > > + auto pred = [effectiveExpo, effectiveGain] (aiq::AiqResults& result) { > > + ia_aiq_exposure_sensor_parameters* sensorExposure = > > + result.ae()->exposures[0].sensor_exposure; > > + > > + return (effectiveExpo == sensorExposure->coarse_integration_time || > > + effectiveGain == sensorExposure->analog_gain_code_global); > > + }; > > + aiq::AiqResults& aeMatchedResults = resultsHistory_.searchBackward(pred, results_); > > This is an interesting implementation! (in a good way), though I wonder > if it's over complicated. > > I thought we'd be able to do something simpler, because we know the > frame number index, so we should be able to associate that with the > resultsHistory_ and obtain the results from the frame that was queued > with the same 'frame' number... > > Could you check the frame numbers of when the parameters are queued up, > against the 'frame' that comes in here, and see if it matches up with > your implementation as a suitable method to tie the results together? Yes, I struggled with it too :-). The effective AiqResults may be delayed for an interval which depends on when the pipeline handler applies controls when cio2 is queued up and the delay for the sensor controls to be effective, especially exposure and gain. In theory, the delay could be the delay on (applying controls delay + sensor effective delay). There were three approaches that were considered. 1. Let IPA assume the implementation and get the delay property of the sensor from the pipeline handler and do the calculation. It makes me a little uncomfortable to let IPA assume the implementation details of the pipeline handler, so I skipped this. 2. Associate the results with an ID and return it back to pipeline handler, and let pipeline send back the effective ID with statistics. This is how Intel HAL in ChromeOS is implemented, but this requires changes on the IPA interface and the DelayedControls class. 3. Current approach which finds the results backwards. > > > > + aiq::AiqResults combinedResults = results_; > > + combinedResults.setAe(aeMatchedResults.ae()); > > Oh - that's interesting, so we take the latest calculated results, and > the ae() from the matched results.. > > > + > > + /* Aiq library expects timestamp in microseconds */ > > Perhaps std::chrono would help us here in the future. Doesn't have to be > now. > > > + aiq_.setStatistics(frame, (frameTimestamp / 1000), combinedResults, stats); > > > > /* Set frame durations from exposure results */ > > - ia_aiq_exposure_sensor_parameters *sensorExposure = results_.ae()->exposures->sensor_exposure; > > + ia_aiq_exposure_sensor_parameters *sensorExposure = combinedResults.ae()->exposures->sensor_exposure; > > int64_t frameDuration = (sensorExposure->line_length_pixels * sensorExposure->frame_length_lines) / > > (sensorInfo_.pixelRate / 1e6); > > ctrls.set(controls::FrameDuration, frameDuration); > > @@ -423,10 +466,11 @@ void IPAIPU3::setControls(unsigned int frame) > > IPU3Action op; > > op.op = ActionSetSensorControls; > > > > - ControlList ctrls(ctrls_); > > - ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_)); > > - ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain_)); > > - op.controls = ctrls; > > + ControlList sensorCtrls(ctrls_); > > + sensorCtrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_)); > > + sensorCtrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain_)); > > + > > + op.sensorControls = sensorCtrls; > > > > queueFrameAction.emit(frame, op); > > } > > -- > > 2.33.1.1089.g2158813163f-goog > >
Quoting Hanlin Chen (2021-11-10 13:33:13) > Hi Kieran, > > On Tue, Nov 2, 2021 at 9:58 PM Kieran Bingham > <kieran.bingham@ideasonboard.com> wrote: > > > > Quoting Han-Lin Chen (2021-10-29 12:59:58) > > > Set the statistics with the latest AE AiqResults which has the same > > > exposure time and analog gain. The patch reduces the AE haunting during > > > > s/haunting/hunting/ > > > > Although appropriate for Halloween ;-) > > > > > the converging process. > > > > > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > > > --- > > > aiq/aiq_input_parameters.cpp | 2 +- > > > ipu3.cpp | 66 ++++++++++++++++++++++++++++++------ > > > 2 files changed, 56 insertions(+), 12 deletions(-) > > > > > > diff --git a/aiq/aiq_input_parameters.cpp b/aiq/aiq_input_parameters.cpp > > > index bc87b31..46553a6 100644 > > > --- a/aiq/aiq_input_parameters.cpp > > > +++ b/aiq/aiq_input_parameters.cpp > > > @@ -130,7 +130,7 @@ AiqInputParameters &AiqInputParameters::operator=(const AiqInputParameters &othe > > > > > > void AiqInputParameters::setAeAwbAfDefaults() > > > { > > > - /*Ae Params */ > > > + /* Ae Params */ > > > aeInputParams.num_exposures = NUM_EXPOSURES; > > > aeInputParams.frame_use = ia_aiq_frame_use_preview; > > > aeInputParams.flash_mode = ia_aiq_flash_mode_off; > > > diff --git a/ipu3.cpp b/ipu3.cpp > > > index 8126e9d..f463805 100644 > > > --- a/ipu3.cpp > > > +++ b/ipu3.cpp > > > @@ -59,7 +59,8 @@ private: > > > void fillParams(unsigned int frame, ipu3_uapi_params *params); > > > void parseStatistics(unsigned int frame, > > > int64_t frameTimestamp, > > > - const ipu3_uapi_stats_3a *stats); > > > + const ipu3_uapi_stats_3a *stats, > > > + const ControlList& sensorCtrls); > > > > > > void setControls(unsigned int frame); > > > > > > @@ -84,6 +85,7 @@ private: > > > /* Temporary storage until we have a FrameContext object / struct */ > > > aiq::AiqInputParameters aiqInputParams_; > > > aiq::AiqResults results_; > > > > Is this still needed (results_)? Isn't it just stored in the history ring buffer? > > > > > + aiq::AiqResultsRingBuffer resultsHistory_; > > > > > > BinaryData aiqb_; > > > BinaryData nvm_; > > > @@ -282,6 +284,8 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo, > > > /* Upate the camera controls using the new sensor settings. */ > > > updateControls(sensorInfo_, ctrls_, ipaControls); > > > > > > + resultsHistory_.Reset(); > > > + > > > return 0; > > > } > > > > > > @@ -327,7 +331,10 @@ void IPAIPU3::processEvent(const IPU3Event &event) > > > const ipu3_uapi_stats_3a *stats = > > > reinterpret_cast<ipu3_uapi_stats_3a *>(mem.data()); > > > > > > - parseStatistics(event.frame, event.frameTimestamp, stats); > > > + parseStatistics(event.frame, > > > + event.frameTimestamp, > > > + stats, > > > + event.sensorControls); > > > break; > > > } > > > case EventFillParams: { > > > @@ -374,14 +381,16 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params) > > > */ > > > > > > /* Run algorithms into/using this context structure */ > > > - if (frame % 10 == 0) > > > - aiq_.run2a(frame, aiqInputParams_, results_); > > > + aiq_.run2a(frame, aiqInputParams_, results_); > > > > Very happy to see the artifical run delay removed. > > > > > > > > > aic_.updateRuntimeParams(results_); > > > aic_.run(params); > > > > > > exposure_ = results_.ae()->exposures[0].sensor_exposure->coarse_integration_time; > > > gain_ = results_.ae()->exposures[0].sensor_exposure->analog_gain_code_global; > > > + > > > + resultsHistory_.Push(results_); > > > + > > > > Ah, ok so you are still using the results_ ... but is this introducing > > unnecessary copies? Can't we just decide / create a new entry in the > > resultsHistory_ (and later rename that to be 'results_') and directly > > run the run2a call into that active entry? > Yes. Will remove the "results_". > > > > > > > setControls(frame); > > > > > > IPU3Action op; > > > @@ -392,7 +401,8 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params) > > > > > > void IPAIPU3::parseStatistics(unsigned int frame, > > > int64_t frameTimestamp, > > > - const ipu3_uapi_stats_3a *stats) > > > + const ipu3_uapi_stats_3a *stats, > > > + const ControlList& sensorCtrls) > > > { > > > ControlList ctrls(controls::controls); > > > > > > @@ -403,10 +413,43 @@ void IPAIPU3::parseStatistics(unsigned int frame, > > > */ > > > > > > ASSERT (frameTimestamp > 0); > > > - aiq_.setStatistics(frame, frameTimestamp, results_, stats); > > > + > > > + /* Ae algorithm expects the statistics to be set with its corresponding > > > + * Ae result, i.e., the Ae result should match the exposure time and > > > + * analog gain with the the effective sensor controls of the statistics. > > > + * Search the required Ae result in the result history and combine it > > > + * with the latest result as the input to AIQ::setStatistics(). > > > + */ > > > + > > > + int32_t effectiveExpo = 0; > > > + int32_t effectiveGain = 0; > > > + ControlValue ctrlValue; > > > + > > > + ctrlValue = sensorCtrls.get(V4L2_CID_EXPOSURE); > > > + if (!ctrlValue.isNone()) > > > + effectiveExpo = ctrlValue.get<int32_t>(); > > > + > > > + ctrlValue = sensorCtrls.get(V4L2_CID_ANALOGUE_GAIN); > > > + if (!ctrlValue.isNone()) > > > + effectiveGain = ctrlValue.get<int32_t>(); > > > + > > > + auto pred = [effectiveExpo, effectiveGain] (aiq::AiqResults& result) { > > > + ia_aiq_exposure_sensor_parameters* sensorExposure = > > > + result.ae()->exposures[0].sensor_exposure; > > > + > > > + return (effectiveExpo == sensorExposure->coarse_integration_time || > > > + effectiveGain == sensorExposure->analog_gain_code_global); > > > + }; > > > + aiq::AiqResults& aeMatchedResults = resultsHistory_.searchBackward(pred, results_); > > > > This is an interesting implementation! (in a good way), though I wonder > > if it's over complicated. > > > > I thought we'd be able to do something simpler, because we know the > > frame number index, so we should be able to associate that with the > > resultsHistory_ and obtain the results from the frame that was queued > > with the same 'frame' number... > > > > Could you check the frame numbers of when the parameters are queued up, > > against the 'frame' that comes in here, and see if it matches up with > > your implementation as a suitable method to tie the results together? > Yes, I struggled with it too :-). > The effective AiqResults may be delayed for an interval which depends > on when the pipeline handler applies controls when cio2 is queued up > and the delay for the sensor controls to be effective, especially > exposure and gain. > In theory, the delay could be the delay on (applying controls delay + > sensor effective delay). > There were three approaches that were considered. > > 1. Let IPA assume the implementation and get the delay property of the > sensor from the pipeline handler and do the calculation. > It makes me a little uncomfortable to let IPA assume the > implementation details of the pipeline handler, so I skipped this. > 2. Associate the results with an ID and return it back to pipeline > handler, and let pipeline send back the effective ID with statistics. > This is how Intel HAL in ChromeOS is implemented, but this requires > changes on the IPA interface and the DelayedControls class. > 3. Current approach which finds the results backwards. As far as I'm aware, 2 is what we also already support. Every operation should have an id/frame number/sequence number which matches the request sequence number. Using your implementation, you should be able to verify (or vilify) the matching of the results you determine against the ones that were generated for the same index/sequence number. Re-reading your text though, am I right to infer that DelayedControls is throwing this option out? > > > > > > > > + aiq::AiqResults combinedResults = results_; > > > + combinedResults.setAe(aeMatchedResults.ae()); > > > > Oh - that's interesting, so we take the latest calculated results, and > > the ae() from the matched results.. > > > > > + > > > + /* Aiq library expects timestamp in microseconds */ > > > > Perhaps std::chrono would help us here in the future. Doesn't have to be > > now. > > > > > + aiq_.setStatistics(frame, (frameTimestamp / 1000), combinedResults, stats); > > > > > > /* Set frame durations from exposure results */ > > > - ia_aiq_exposure_sensor_parameters *sensorExposure = results_.ae()->exposures->sensor_exposure; > > > + ia_aiq_exposure_sensor_parameters *sensorExposure = combinedResults.ae()->exposures->sensor_exposure; > > > int64_t frameDuration = (sensorExposure->line_length_pixels * sensorExposure->frame_length_lines) / > > > (sensorInfo_.pixelRate / 1e6); > > > ctrls.set(controls::FrameDuration, frameDuration); > > > @@ -423,10 +466,11 @@ void IPAIPU3::setControls(unsigned int frame) > > > IPU3Action op; > > > op.op = ActionSetSensorControls; > > > > > > - ControlList ctrls(ctrls_); > > > - ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_)); > > > - ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain_)); > > > - op.controls = ctrls; > > > + ControlList sensorCtrls(ctrls_); > > > + sensorCtrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_)); > > > + sensorCtrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain_)); > > > + > > > + op.sensorControls = sensorCtrls; > > > > > > queueFrameAction.emit(frame, op); > > > } > > > -- > > > 2.33.1.1089.g2158813163f-goog > > >
On Wed, Nov 10, 2021 at 9:48 PM Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Quoting Hanlin Chen (2021-11-10 13:33:13) > > Hi Kieran, > > > > On Tue, Nov 2, 2021 at 9:58 PM Kieran Bingham > > <kieran.bingham@ideasonboard.com> wrote: > > > > > > Quoting Han-Lin Chen (2021-10-29 12:59:58) > > > > Set the statistics with the latest AE AiqResults which has the same > > > > exposure time and analog gain. The patch reduces the AE haunting during > > > > > > s/haunting/hunting/ > > > > > > Although appropriate for Halloween ;-) > > > > > > > the converging process. > > > > > > > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > > > > --- > > > > aiq/aiq_input_parameters.cpp | 2 +- > > > > ipu3.cpp | 66 ++++++++++++++++++++++++++++++------ > > > > 2 files changed, 56 insertions(+), 12 deletions(-) > > > > > > > > diff --git a/aiq/aiq_input_parameters.cpp b/aiq/aiq_input_parameters.cpp > > > > index bc87b31..46553a6 100644 > > > > --- a/aiq/aiq_input_parameters.cpp > > > > +++ b/aiq/aiq_input_parameters.cpp > > > > @@ -130,7 +130,7 @@ AiqInputParameters &AiqInputParameters::operator=(const AiqInputParameters &othe > > > > > > > > void AiqInputParameters::setAeAwbAfDefaults() > > > > { > > > > - /*Ae Params */ > > > > + /* Ae Params */ > > > > aeInputParams.num_exposures = NUM_EXPOSURES; > > > > aeInputParams.frame_use = ia_aiq_frame_use_preview; > > > > aeInputParams.flash_mode = ia_aiq_flash_mode_off; > > > > diff --git a/ipu3.cpp b/ipu3.cpp > > > > index 8126e9d..f463805 100644 > > > > --- a/ipu3.cpp > > > > +++ b/ipu3.cpp > > > > @@ -59,7 +59,8 @@ private: > > > > void fillParams(unsigned int frame, ipu3_uapi_params *params); > > > > void parseStatistics(unsigned int frame, > > > > int64_t frameTimestamp, > > > > - const ipu3_uapi_stats_3a *stats); > > > > + const ipu3_uapi_stats_3a *stats, > > > > + const ControlList& sensorCtrls); > > > > > > > > void setControls(unsigned int frame); > > > > > > > > @@ -84,6 +85,7 @@ private: > > > > /* Temporary storage until we have a FrameContext object / struct */ > > > > aiq::AiqInputParameters aiqInputParams_; > > > > aiq::AiqResults results_; > > > > > > Is this still needed (results_)? Isn't it just stored in the history ring buffer? > > > > > > > + aiq::AiqResultsRingBuffer resultsHistory_; > > > > > > > > BinaryData aiqb_; > > > > BinaryData nvm_; > > > > @@ -282,6 +284,8 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo, > > > > /* Upate the camera controls using the new sensor settings. */ > > > > updateControls(sensorInfo_, ctrls_, ipaControls); > > > > > > > > + resultsHistory_.Reset(); > > > > + > > > > return 0; > > > > } > > > > > > > > @@ -327,7 +331,10 @@ void IPAIPU3::processEvent(const IPU3Event &event) > > > > const ipu3_uapi_stats_3a *stats = > > > > reinterpret_cast<ipu3_uapi_stats_3a *>(mem.data()); > > > > > > > > - parseStatistics(event.frame, event.frameTimestamp, stats); > > > > + parseStatistics(event.frame, > > > > + event.frameTimestamp, > > > > + stats, > > > > + event.sensorControls); > > > > break; > > > > } > > > > case EventFillParams: { > > > > @@ -374,14 +381,16 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params) > > > > */ > > > > > > > > /* Run algorithms into/using this context structure */ > > > > - if (frame % 10 == 0) > > > > - aiq_.run2a(frame, aiqInputParams_, results_); > > > > + aiq_.run2a(frame, aiqInputParams_, results_); > > > > > > Very happy to see the artifical run delay removed. > > > > > > > > > > > > > aic_.updateRuntimeParams(results_); > > > > aic_.run(params); > > > > > > > > exposure_ = results_.ae()->exposures[0].sensor_exposure->coarse_integration_time; > > > > gain_ = results_.ae()->exposures[0].sensor_exposure->analog_gain_code_global; > > > > + > > > > + resultsHistory_.Push(results_); > > > > + > > > > > > Ah, ok so you are still using the results_ ... but is this introducing > > > unnecessary copies? Can't we just decide / create a new entry in the > > > resultsHistory_ (and later rename that to be 'results_') and directly > > > run the run2a call into that active entry? > > Yes. Will remove the "results_". > > > > > > > > > > setControls(frame); > > > > > > > > IPU3Action op; > > > > @@ -392,7 +401,8 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params) > > > > > > > > void IPAIPU3::parseStatistics(unsigned int frame, > > > > int64_t frameTimestamp, > > > > - const ipu3_uapi_stats_3a *stats) > > > > + const ipu3_uapi_stats_3a *stats, > > > > + const ControlList& sensorCtrls) > > > > { > > > > ControlList ctrls(controls::controls); > > > > > > > > @@ -403,10 +413,43 @@ void IPAIPU3::parseStatistics(unsigned int frame, > > > > */ > > > > > > > > ASSERT (frameTimestamp > 0); > > > > - aiq_.setStatistics(frame, frameTimestamp, results_, stats); > > > > + > > > > + /* Ae algorithm expects the statistics to be set with its corresponding > > > > + * Ae result, i.e., the Ae result should match the exposure time and > > > > + * analog gain with the the effective sensor controls of the statistics. > > > > + * Search the required Ae result in the result history and combine it > > > > + * with the latest result as the input to AIQ::setStatistics(). > > > > + */ > > > > + > > > > + int32_t effectiveExpo = 0; > > > > + int32_t effectiveGain = 0; > > > > + ControlValue ctrlValue; > > > > + > > > > + ctrlValue = sensorCtrls.get(V4L2_CID_EXPOSURE); > > > > + if (!ctrlValue.isNone()) > > > > + effectiveExpo = ctrlValue.get<int32_t>(); > > > > + > > > > + ctrlValue = sensorCtrls.get(V4L2_CID_ANALOGUE_GAIN); > > > > + if (!ctrlValue.isNone()) > > > > + effectiveGain = ctrlValue.get<int32_t>(); > > > > + > > > > + auto pred = [effectiveExpo, effectiveGain] (aiq::AiqResults& result) { > > > > + ia_aiq_exposure_sensor_parameters* sensorExposure = > > > > + result.ae()->exposures[0].sensor_exposure; > > > > + > > > > + return (effectiveExpo == sensorExposure->coarse_integration_time || > > > > + effectiveGain == sensorExposure->analog_gain_code_global); > > > > + }; > > > > + aiq::AiqResults& aeMatchedResults = resultsHistory_.searchBackward(pred, results_); > > > > > > This is an interesting implementation! (in a good way), though I wonder > > > if it's over complicated. > > > > > > I thought we'd be able to do something simpler, because we know the > > > frame number index, so we should be able to associate that with the > > > resultsHistory_ and obtain the results from the frame that was queued > > > with the same 'frame' number... > > > > > > Could you check the frame numbers of when the parameters are queued up, > > > against the 'frame' that comes in here, and see if it matches up with > > > your implementation as a suitable method to tie the results together? > > Yes, I struggled with it too :-). > > The effective AiqResults may be delayed for an interval which depends > > on when the pipeline handler applies controls when cio2 is queued up > > and the delay for the sensor controls to be effective, especially > > exposure and gain. > > In theory, the delay could be the delay on (applying controls delay + > > sensor effective delay). > > There were three approaches that were considered. > > > > 1. Let IPA assume the implementation and get the delay property of the > > sensor from the pipeline handler and do the calculation. > > It makes me a little uncomfortable to let IPA assume the > > implementation details of the pipeline handler, so I skipped this. > > 2. Associate the results with an ID and return it back to pipeline > > handler, and let pipeline send back the effective ID with statistics. > > This is how Intel HAL in ChromeOS is implemented, but this requires > > changes on the IPA interface and the DelayedControls class. > > 3. Current approach which finds the results backwards. > > As far as I'm aware, 2 is what we also already support. Every operation > should have an id/frame number/sequence number which matches the request > sequence number. > > Using your implementation, you should be able to verify (or vilify) the > matching of the results you determine against the ones that were > generated for the same index/sequence number. > > Re-reading your text though, am I right to infer that DelayedControls is > throwing this option out? Yes, it may introduce extra delay which makes frame number a little off, but we need that since the sensor controls may delay. For the ID in 2, I mean an extra ID field for Aiq (not the frame number), and let pipeline handlers return the ID back, since the pipeline handler could figure out the exact ID, by hacking into DelayedControls. ;-/ > > > > > > > > > > > > > > > + aiq::AiqResults combinedResults = results_; > > > > + combinedResults.setAe(aeMatchedResults.ae()); > > > > > > Oh - that's interesting, so we take the latest calculated results, and > > > the ae() from the matched results.. > > > > > > > + > > > > + /* Aiq library expects timestamp in microseconds */ > > > > > > Perhaps std::chrono would help us here in the future. Doesn't have to be > > > now. > > > > > > > + aiq_.setStatistics(frame, (frameTimestamp / 1000), combinedResults, stats); > > > > > > > > /* Set frame durations from exposure results */ > > > > - ia_aiq_exposure_sensor_parameters *sensorExposure = results_.ae()->exposures->sensor_exposure; > > > > + ia_aiq_exposure_sensor_parameters *sensorExposure = combinedResults.ae()->exposures->sensor_exposure; > > > > int64_t frameDuration = (sensorExposure->line_length_pixels * sensorExposure->frame_length_lines) / > > > > (sensorInfo_.pixelRate / 1e6); > > > > ctrls.set(controls::FrameDuration, frameDuration); > > > > @@ -423,10 +466,11 @@ void IPAIPU3::setControls(unsigned int frame) > > > > IPU3Action op; > > > > op.op = ActionSetSensorControls; > > > > > > > > - ControlList ctrls(ctrls_); > > > > - ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_)); > > > > - ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain_)); > > > > - op.controls = ctrls; > > > > + ControlList sensorCtrls(ctrls_); > > > > + sensorCtrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_)); > > > > + sensorCtrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain_)); > > > > + > > > > + op.sensorControls = sensorCtrls; > > > > > > > > queueFrameAction.emit(frame, op); > > > > } > > > > -- > > > > 2.33.1.1089.g2158813163f-goog > > > >
Quoting Hanlin Chen (2021-11-10 14:06:30) > On Wed, Nov 10, 2021 at 9:48 PM Kieran Bingham > <kieran.bingham@ideasonboard.com> wrote: > > > > Quoting Hanlin Chen (2021-11-10 13:33:13) > > > Hi Kieran, > > > > > > On Tue, Nov 2, 2021 at 9:58 PM Kieran Bingham > > > <kieran.bingham@ideasonboard.com> wrote: > > > > > > > > Quoting Han-Lin Chen (2021-10-29 12:59:58) > > > > > Set the statistics with the latest AE AiqResults which has the same > > > > > exposure time and analog gain. The patch reduces the AE haunting during > > > > > > > > s/haunting/hunting/ > > > > > > > > Although appropriate for Halloween ;-) > > > > > > > > > the converging process. > > > > > > > > > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > > > > > --- > > > > > aiq/aiq_input_parameters.cpp | 2 +- > > > > > ipu3.cpp | 66 ++++++++++++++++++++++++++++++------ > > > > > 2 files changed, 56 insertions(+), 12 deletions(-) > > > > > > > > > > diff --git a/aiq/aiq_input_parameters.cpp b/aiq/aiq_input_parameters.cpp > > > > > index bc87b31..46553a6 100644 > > > > > --- a/aiq/aiq_input_parameters.cpp > > > > > +++ b/aiq/aiq_input_parameters.cpp > > > > > @@ -130,7 +130,7 @@ AiqInputParameters &AiqInputParameters::operator=(const AiqInputParameters &othe > > > > > > > > > > void AiqInputParameters::setAeAwbAfDefaults() > > > > > { > > > > > - /*Ae Params */ > > > > > + /* Ae Params */ > > > > > aeInputParams.num_exposures = NUM_EXPOSURES; > > > > > aeInputParams.frame_use = ia_aiq_frame_use_preview; > > > > > aeInputParams.flash_mode = ia_aiq_flash_mode_off; > > > > > diff --git a/ipu3.cpp b/ipu3.cpp > > > > > index 8126e9d..f463805 100644 > > > > > --- a/ipu3.cpp > > > > > +++ b/ipu3.cpp > > > > > @@ -59,7 +59,8 @@ private: > > > > > void fillParams(unsigned int frame, ipu3_uapi_params *params); > > > > > void parseStatistics(unsigned int frame, > > > > > int64_t frameTimestamp, > > > > > - const ipu3_uapi_stats_3a *stats); > > > > > + const ipu3_uapi_stats_3a *stats, > > > > > + const ControlList& sensorCtrls); > > > > > > > > > > void setControls(unsigned int frame); > > > > > > > > > > @@ -84,6 +85,7 @@ private: > > > > > /* Temporary storage until we have a FrameContext object / struct */ > > > > > aiq::AiqInputParameters aiqInputParams_; > > > > > aiq::AiqResults results_; > > > > > > > > Is this still needed (results_)? Isn't it just stored in the history ring buffer? > > > > > > > > > + aiq::AiqResultsRingBuffer resultsHistory_; > > > > > > > > > > BinaryData aiqb_; > > > > > BinaryData nvm_; > > > > > @@ -282,6 +284,8 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo, > > > > > /* Upate the camera controls using the new sensor settings. */ > > > > > updateControls(sensorInfo_, ctrls_, ipaControls); > > > > > > > > > > + resultsHistory_.Reset(); > > > > > + > > > > > return 0; > > > > > } > > > > > > > > > > @@ -327,7 +331,10 @@ void IPAIPU3::processEvent(const IPU3Event &event) > > > > > const ipu3_uapi_stats_3a *stats = > > > > > reinterpret_cast<ipu3_uapi_stats_3a *>(mem.data()); > > > > > > > > > > - parseStatistics(event.frame, event.frameTimestamp, stats); > > > > > + parseStatistics(event.frame, > > > > > + event.frameTimestamp, > > > > > + stats, > > > > > + event.sensorControls); > > > > > break; > > > > > } > > > > > case EventFillParams: { > > > > > @@ -374,14 +381,16 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params) > > > > > */ > > > > > > > > > > /* Run algorithms into/using this context structure */ > > > > > - if (frame % 10 == 0) > > > > > - aiq_.run2a(frame, aiqInputParams_, results_); > > > > > + aiq_.run2a(frame, aiqInputParams_, results_); > > > > > > > > Very happy to see the artifical run delay removed. > > > > > > > > > > > > > > > > > aic_.updateRuntimeParams(results_); > > > > > aic_.run(params); > > > > > > > > > > exposure_ = results_.ae()->exposures[0].sensor_exposure->coarse_integration_time; > > > > > gain_ = results_.ae()->exposures[0].sensor_exposure->analog_gain_code_global; > > > > > + > > > > > + resultsHistory_.Push(results_); > > > > > + > > > > > > > > Ah, ok so you are still using the results_ ... but is this introducing > > > > unnecessary copies? Can't we just decide / create a new entry in the > > > > resultsHistory_ (and later rename that to be 'results_') and directly > > > > run the run2a call into that active entry? > > > Yes. Will remove the "results_". > > > > > > > > > > > > > setControls(frame); > > > > > > > > > > IPU3Action op; > > > > > @@ -392,7 +401,8 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params) > > > > > > > > > > void IPAIPU3::parseStatistics(unsigned int frame, > > > > > int64_t frameTimestamp, > > > > > - const ipu3_uapi_stats_3a *stats) > > > > > + const ipu3_uapi_stats_3a *stats, > > > > > + const ControlList& sensorCtrls) > > > > > { > > > > > ControlList ctrls(controls::controls); > > > > > > > > > > @@ -403,10 +413,43 @@ void IPAIPU3::parseStatistics(unsigned int frame, > > > > > */ > > > > > > > > > > ASSERT (frameTimestamp > 0); > > > > > - aiq_.setStatistics(frame, frameTimestamp, results_, stats); > > > > > + > > > > > + /* Ae algorithm expects the statistics to be set with its corresponding > > > > > + * Ae result, i.e., the Ae result should match the exposure time and > > > > > + * analog gain with the the effective sensor controls of the statistics. > > > > > + * Search the required Ae result in the result history and combine it > > > > > + * with the latest result as the input to AIQ::setStatistics(). > > > > > + */ > > > > > + > > > > > + int32_t effectiveExpo = 0; > > > > > + int32_t effectiveGain = 0; > > > > > + ControlValue ctrlValue; > > > > > + > > > > > + ctrlValue = sensorCtrls.get(V4L2_CID_EXPOSURE); > > > > > + if (!ctrlValue.isNone()) > > > > > + effectiveExpo = ctrlValue.get<int32_t>(); > > > > > + > > > > > + ctrlValue = sensorCtrls.get(V4L2_CID_ANALOGUE_GAIN); > > > > > + if (!ctrlValue.isNone()) > > > > > + effectiveGain = ctrlValue.get<int32_t>(); > > > > > + > > > > > + auto pred = [effectiveExpo, effectiveGain] (aiq::AiqResults& result) { > > > > > + ia_aiq_exposure_sensor_parameters* sensorExposure = > > > > > + result.ae()->exposures[0].sensor_exposure; > > > > > + > > > > > + return (effectiveExpo == sensorExposure->coarse_integration_time || > > > > > + effectiveGain == sensorExposure->analog_gain_code_global); > > > > > + }; > > > > > + aiq::AiqResults& aeMatchedResults = resultsHistory_.searchBackward(pred, results_); > > > > > > > > This is an interesting implementation! (in a good way), though I wonder > > > > if it's over complicated. > > > > > > > > I thought we'd be able to do something simpler, because we know the > > > > frame number index, so we should be able to associate that with the > > > > resultsHistory_ and obtain the results from the frame that was queued > > > > with the same 'frame' number... > > > > > > > > Could you check the frame numbers of when the parameters are queued up, > > > > against the 'frame' that comes in here, and see if it matches up with > > > > your implementation as a suitable method to tie the results together? > > > Yes, I struggled with it too :-). > > > The effective AiqResults may be delayed for an interval which depends > > > on when the pipeline handler applies controls when cio2 is queued up > > > and the delay for the sensor controls to be effective, especially > > > exposure and gain. > > > In theory, the delay could be the delay on (applying controls delay + > > > sensor effective delay). > > > There were three approaches that were considered. > > > > > > 1. Let IPA assume the implementation and get the delay property of the > > > sensor from the pipeline handler and do the calculation. > > > It makes me a little uncomfortable to let IPA assume the > > > implementation details of the pipeline handler, so I skipped this. > > > 2. Associate the results with an ID and return it back to pipeline > > > handler, and let pipeline send back the effective ID with statistics. > > > This is how Intel HAL in ChromeOS is implemented, but this requires > > > changes on the IPA interface and the DelayedControls class. > > > 3. Current approach which finds the results backwards. > > > > As far as I'm aware, 2 is what we also already support. Every operation > > should have an id/frame number/sequence number which matches the request > > sequence number. > > > > Using your implementation, you should be able to verify (or vilify) the > > matching of the results you determine against the ones that were > > generated for the same index/sequence number. > > > > Re-reading your text though, am I right to infer that DelayedControls is > > throwing this option out? > Yes, it may introduce extra delay which makes frame number a little > off, but we need that since the sensor controls may delay. > For the ID in 2, I mean an extra ID field for Aiq (not the frame > number), and let pipeline handlers return the ID back, since > the pipeline handler could figure out the exact ID, by hacking into > DelayedControls. ;-/ This sounds like the same thing that Jean-Michel is looking at too (which is probably evident as I think he used a couple of your patches). Discussing with him recently, we realised that sending back the effective sensor settings from DelayedControls along with the statistics buffer ensured that the IPA had the information it needed. Is there any other information/context that you need to ensure is kept from when the parameter buffers are filled in before capturing an image? -- Kieran > > > > > > > > > > > > > > > > > > > > > > + aiq::AiqResults combinedResults = results_; > > > > > + combinedResults.setAe(aeMatchedResults.ae()); > > > > > > > > Oh - that's interesting, so we take the latest calculated results, and > > > > the ae() from the matched results.. > > > > > > > > > + > > > > > + /* Aiq library expects timestamp in microseconds */ > > > > > > > > Perhaps std::chrono would help us here in the future. Doesn't have to be > > > > now. > > > > > > > > > + aiq_.setStatistics(frame, (frameTimestamp / 1000), combinedResults, stats); > > > > > > > > > > /* Set frame durations from exposure results */ > > > > > - ia_aiq_exposure_sensor_parameters *sensorExposure = results_.ae()->exposures->sensor_exposure; > > > > > + ia_aiq_exposure_sensor_parameters *sensorExposure = combinedResults.ae()->exposures->sensor_exposure; > > > > > int64_t frameDuration = (sensorExposure->line_length_pixels * sensorExposure->frame_length_lines) / > > > > > (sensorInfo_.pixelRate / 1e6); > > > > > ctrls.set(controls::FrameDuration, frameDuration); > > > > > @@ -423,10 +466,11 @@ void IPAIPU3::setControls(unsigned int frame) > > > > > IPU3Action op; > > > > > op.op = ActionSetSensorControls; > > > > > > > > > > - ControlList ctrls(ctrls_); > > > > > - ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_)); > > > > > - ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain_)); > > > > > - op.controls = ctrls; > > > > > + ControlList sensorCtrls(ctrls_); > > > > > + sensorCtrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_)); > > > > > + sensorCtrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain_)); > > > > > + > > > > > + op.sensorControls = sensorCtrls; > > > > > > > > > > queueFrameAction.emit(frame, op); > > > > > } > > > > > -- > > > > > 2.33.1.1089.g2158813163f-goog > > > > >
On Wed, Nov 10, 2021 at 10:14 PM Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Quoting Hanlin Chen (2021-11-10 14:06:30) > > On Wed, Nov 10, 2021 at 9:48 PM Kieran Bingham > > <kieran.bingham@ideasonboard.com> wrote: > > > > > > Quoting Hanlin Chen (2021-11-10 13:33:13) > > > > Hi Kieran, > > > > > > > > On Tue, Nov 2, 2021 at 9:58 PM Kieran Bingham > > > > <kieran.bingham@ideasonboard.com> wrote: > > > > > > > > > > Quoting Han-Lin Chen (2021-10-29 12:59:58) > > > > > > Set the statistics with the latest AE AiqResults which has the same > > > > > > exposure time and analog gain. The patch reduces the AE haunting during > > > > > > > > > > s/haunting/hunting/ > > > > > > > > > > Although appropriate for Halloween ;-) > > > > > > > > > > > the converging process. > > > > > > > > > > > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > > > > > > --- > > > > > > aiq/aiq_input_parameters.cpp | 2 +- > > > > > > ipu3.cpp | 66 ++++++++++++++++++++++++++++++------ > > > > > > 2 files changed, 56 insertions(+), 12 deletions(-) > > > > > > > > > > > > diff --git a/aiq/aiq_input_parameters.cpp b/aiq/aiq_input_parameters.cpp > > > > > > index bc87b31..46553a6 100644 > > > > > > --- a/aiq/aiq_input_parameters.cpp > > > > > > +++ b/aiq/aiq_input_parameters.cpp > > > > > > @@ -130,7 +130,7 @@ AiqInputParameters &AiqInputParameters::operator=(const AiqInputParameters &othe > > > > > > > > > > > > void AiqInputParameters::setAeAwbAfDefaults() > > > > > > { > > > > > > - /*Ae Params */ > > > > > > + /* Ae Params */ > > > > > > aeInputParams.num_exposures = NUM_EXPOSURES; > > > > > > aeInputParams.frame_use = ia_aiq_frame_use_preview; > > > > > > aeInputParams.flash_mode = ia_aiq_flash_mode_off; > > > > > > diff --git a/ipu3.cpp b/ipu3.cpp > > > > > > index 8126e9d..f463805 100644 > > > > > > --- a/ipu3.cpp > > > > > > +++ b/ipu3.cpp > > > > > > @@ -59,7 +59,8 @@ private: > > > > > > void fillParams(unsigned int frame, ipu3_uapi_params *params); > > > > > > void parseStatistics(unsigned int frame, > > > > > > int64_t frameTimestamp, > > > > > > - const ipu3_uapi_stats_3a *stats); > > > > > > + const ipu3_uapi_stats_3a *stats, > > > > > > + const ControlList& sensorCtrls); > > > > > > > > > > > > void setControls(unsigned int frame); > > > > > > > > > > > > @@ -84,6 +85,7 @@ private: > > > > > > /* Temporary storage until we have a FrameContext object / struct */ > > > > > > aiq::AiqInputParameters aiqInputParams_; > > > > > > aiq::AiqResults results_; > > > > > > > > > > Is this still needed (results_)? Isn't it just stored in the history ring buffer? > > > > > > > > > > > + aiq::AiqResultsRingBuffer resultsHistory_; > > > > > > > > > > > > BinaryData aiqb_; > > > > > > BinaryData nvm_; > > > > > > @@ -282,6 +284,8 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo, > > > > > > /* Upate the camera controls using the new sensor settings. */ > > > > > > updateControls(sensorInfo_, ctrls_, ipaControls); > > > > > > > > > > > > + resultsHistory_.Reset(); > > > > > > + > > > > > > return 0; > > > > > > } > > > > > > > > > > > > @@ -327,7 +331,10 @@ void IPAIPU3::processEvent(const IPU3Event &event) > > > > > > const ipu3_uapi_stats_3a *stats = > > > > > > reinterpret_cast<ipu3_uapi_stats_3a *>(mem.data()); > > > > > > > > > > > > - parseStatistics(event.frame, event.frameTimestamp, stats); > > > > > > + parseStatistics(event.frame, > > > > > > + event.frameTimestamp, > > > > > > + stats, > > > > > > + event.sensorControls); > > > > > > break; > > > > > > } > > > > > > case EventFillParams: { > > > > > > @@ -374,14 +381,16 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params) > > > > > > */ > > > > > > > > > > > > /* Run algorithms into/using this context structure */ > > > > > > - if (frame % 10 == 0) > > > > > > - aiq_.run2a(frame, aiqInputParams_, results_); > > > > > > + aiq_.run2a(frame, aiqInputParams_, results_); > > > > > > > > > > Very happy to see the artifical run delay removed. > > > > > > > > > > > > > > > > > > > > > aic_.updateRuntimeParams(results_); > > > > > > aic_.run(params); > > > > > > > > > > > > exposure_ = results_.ae()->exposures[0].sensor_exposure->coarse_integration_time; > > > > > > gain_ = results_.ae()->exposures[0].sensor_exposure->analog_gain_code_global; > > > > > > + > > > > > > + resultsHistory_.Push(results_); > > > > > > + > > > > > > > > > > Ah, ok so you are still using the results_ ... but is this introducing > > > > > unnecessary copies? Can't we just decide / create a new entry in the > > > > > resultsHistory_ (and later rename that to be 'results_') and directly > > > > > run the run2a call into that active entry? > > > > Yes. Will remove the "results_". > > > > > > > > > > > > > > > > setControls(frame); > > > > > > > > > > > > IPU3Action op; > > > > > > @@ -392,7 +401,8 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params) > > > > > > > > > > > > void IPAIPU3::parseStatistics(unsigned int frame, > > > > > > int64_t frameTimestamp, > > > > > > - const ipu3_uapi_stats_3a *stats) > > > > > > + const ipu3_uapi_stats_3a *stats, > > > > > > + const ControlList& sensorCtrls) > > > > > > { > > > > > > ControlList ctrls(controls::controls); > > > > > > > > > > > > @@ -403,10 +413,43 @@ void IPAIPU3::parseStatistics(unsigned int frame, > > > > > > */ > > > > > > > > > > > > ASSERT (frameTimestamp > 0); > > > > > > - aiq_.setStatistics(frame, frameTimestamp, results_, stats); > > > > > > + > > > > > > + /* Ae algorithm expects the statistics to be set with its corresponding > > > > > > + * Ae result, i.e., the Ae result should match the exposure time and > > > > > > + * analog gain with the the effective sensor controls of the statistics. > > > > > > + * Search the required Ae result in the result history and combine it > > > > > > + * with the latest result as the input to AIQ::setStatistics(). > > > > > > + */ > > > > > > + > > > > > > + int32_t effectiveExpo = 0; > > > > > > + int32_t effectiveGain = 0; > > > > > > + ControlValue ctrlValue; > > > > > > + > > > > > > + ctrlValue = sensorCtrls.get(V4L2_CID_EXPOSURE); > > > > > > + if (!ctrlValue.isNone()) > > > > > > + effectiveExpo = ctrlValue.get<int32_t>(); > > > > > > + > > > > > > + ctrlValue = sensorCtrls.get(V4L2_CID_ANALOGUE_GAIN); > > > > > > + if (!ctrlValue.isNone()) > > > > > > + effectiveGain = ctrlValue.get<int32_t>(); > > > > > > + > > > > > > + auto pred = [effectiveExpo, effectiveGain] (aiq::AiqResults& result) { > > > > > > + ia_aiq_exposure_sensor_parameters* sensorExposure = > > > > > > + result.ae()->exposures[0].sensor_exposure; > > > > > > + > > > > > > + return (effectiveExpo == sensorExposure->coarse_integration_time || > > > > > > + effectiveGain == sensorExposure->analog_gain_code_global); > > > > > > + }; > > > > > > + aiq::AiqResults& aeMatchedResults = resultsHistory_.searchBackward(pred, results_); > > > > > > > > > > This is an interesting implementation! (in a good way), though I wonder > > > > > if it's over complicated. > > > > > > > > > > I thought we'd be able to do something simpler, because we know the > > > > > frame number index, so we should be able to associate that with the > > > > > resultsHistory_ and obtain the results from the frame that was queued > > > > > with the same 'frame' number... > > > > > > > > > > Could you check the frame numbers of when the parameters are queued up, > > > > > against the 'frame' that comes in here, and see if it matches up with > > > > > your implementation as a suitable method to tie the results together? > > > > Yes, I struggled with it too :-). > > > > The effective AiqResults may be delayed for an interval which depends > > > > on when the pipeline handler applies controls when cio2 is queued up > > > > and the delay for the sensor controls to be effective, especially > > > > exposure and gain. > > > > In theory, the delay could be the delay on (applying controls delay + > > > > sensor effective delay). > > > > There were three approaches that were considered. > > > > > > > > 1. Let IPA assume the implementation and get the delay property of the > > > > sensor from the pipeline handler and do the calculation. > > > > It makes me a little uncomfortable to let IPA assume the > > > > implementation details of the pipeline handler, so I skipped this. > > > > 2. Associate the results with an ID and return it back to pipeline > > > > handler, and let pipeline send back the effective ID with statistics. > > > > This is how Intel HAL in ChromeOS is implemented, but this requires > > > > changes on the IPA interface and the DelayedControls class. > > > > 3. Current approach which finds the results backwards. > > > > > > As far as I'm aware, 2 is what we also already support. Every operation > > > should have an id/frame number/sequence number which matches the request > > > sequence number. > > > > > > Using your implementation, you should be able to verify (or vilify) the > > > matching of the results you determine against the ones that were > > > generated for the same index/sequence number. > > > > > > Re-reading your text though, am I right to infer that DelayedControls is > > > throwing this option out? > > Yes, it may introduce extra delay which makes frame number a little > > off, but we need that since the sensor controls may delay. > > For the ID in 2, I mean an extra ID field for Aiq (not the frame > > number), and let pipeline handlers return the ID back, since > > the pipeline handler could figure out the exact ID, by hacking into > > DelayedControls. ;-/ > > This sounds like the same thing that Jean-Michel is looking at too > (which is probably evident as I think he used a couple of your patches). > > Discussing with him recently, we realised that sending back the > effective sensor settings from DelayedControls along with the statistics > buffer ensured that the IPA had the information it needed. > > Is there any other information/context that you need to ensure is kept > from when the parameter buffers are filled in before capturing an image? Though I cannot give a full list for now. I suppose we will need more Controls exchange through implementing per-frame controls, and to extend Control definitions to support Android state machine transition. Also an event to pass eeprom content might be needed, which, in my understanding, contains lens correction per device. > > -- > Kieran > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > + aiq::AiqResults combinedResults = results_; > > > > > > + combinedResults.setAe(aeMatchedResults.ae()); > > > > > > > > > > Oh - that's interesting, so we take the latest calculated results, and > > > > > the ae() from the matched results.. > > > > > > > > > > > + > > > > > > + /* Aiq library expects timestamp in microseconds */ > > > > > > > > > > Perhaps std::chrono would help us here in the future. Doesn't have to be > > > > > now. > > > > > > > > > > > + aiq_.setStatistics(frame, (frameTimestamp / 1000), combinedResults, stats); > > > > > > > > > > > > /* Set frame durations from exposure results */ > > > > > > - ia_aiq_exposure_sensor_parameters *sensorExposure = results_.ae()->exposures->sensor_exposure; > > > > > > + ia_aiq_exposure_sensor_parameters *sensorExposure = combinedResults.ae()->exposures->sensor_exposure; > > > > > > int64_t frameDuration = (sensorExposure->line_length_pixels * sensorExposure->frame_length_lines) / > > > > > > (sensorInfo_.pixelRate / 1e6); > > > > > > ctrls.set(controls::FrameDuration, frameDuration); > > > > > > @@ -423,10 +466,11 @@ void IPAIPU3::setControls(unsigned int frame) > > > > > > IPU3Action op; > > > > > > op.op = ActionSetSensorControls; > > > > > > > > > > > > - ControlList ctrls(ctrls_); > > > > > > - ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_)); > > > > > > - ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain_)); > > > > > > - op.controls = ctrls; > > > > > > + ControlList sensorCtrls(ctrls_); > > > > > > + sensorCtrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_)); > > > > > > + sensorCtrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain_)); > > > > > > + > > > > > > + op.sensorControls = sensorCtrls; > > > > > > > > > > > > queueFrameAction.emit(frame, op); > > > > > > } > > > > > > -- > > > > > > 2.33.1.1089.g2158813163f-goog > > > > > >
diff --git a/aiq/aiq_input_parameters.cpp b/aiq/aiq_input_parameters.cpp index bc87b31..46553a6 100644 --- a/aiq/aiq_input_parameters.cpp +++ b/aiq/aiq_input_parameters.cpp @@ -130,7 +130,7 @@ AiqInputParameters &AiqInputParameters::operator=(const AiqInputParameters &othe void AiqInputParameters::setAeAwbAfDefaults() { - /*Ae Params */ + /* Ae Params */ aeInputParams.num_exposures = NUM_EXPOSURES; aeInputParams.frame_use = ia_aiq_frame_use_preview; aeInputParams.flash_mode = ia_aiq_flash_mode_off; diff --git a/ipu3.cpp b/ipu3.cpp index 8126e9d..f463805 100644 --- a/ipu3.cpp +++ b/ipu3.cpp @@ -59,7 +59,8 @@ private: void fillParams(unsigned int frame, ipu3_uapi_params *params); void parseStatistics(unsigned int frame, int64_t frameTimestamp, - const ipu3_uapi_stats_3a *stats); + const ipu3_uapi_stats_3a *stats, + const ControlList& sensorCtrls); void setControls(unsigned int frame); @@ -84,6 +85,7 @@ private: /* Temporary storage until we have a FrameContext object / struct */ aiq::AiqInputParameters aiqInputParams_; aiq::AiqResults results_; + aiq::AiqResultsRingBuffer resultsHistory_; BinaryData aiqb_; BinaryData nvm_; @@ -282,6 +284,8 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo, /* Upate the camera controls using the new sensor settings. */ updateControls(sensorInfo_, ctrls_, ipaControls); + resultsHistory_.Reset(); + return 0; } @@ -327,7 +331,10 @@ void IPAIPU3::processEvent(const IPU3Event &event) const ipu3_uapi_stats_3a *stats = reinterpret_cast<ipu3_uapi_stats_3a *>(mem.data()); - parseStatistics(event.frame, event.frameTimestamp, stats); + parseStatistics(event.frame, + event.frameTimestamp, + stats, + event.sensorControls); break; } case EventFillParams: { @@ -374,14 +381,16 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params) */ /* Run algorithms into/using this context structure */ - if (frame % 10 == 0) - aiq_.run2a(frame, aiqInputParams_, results_); + aiq_.run2a(frame, aiqInputParams_, results_); aic_.updateRuntimeParams(results_); aic_.run(params); exposure_ = results_.ae()->exposures[0].sensor_exposure->coarse_integration_time; gain_ = results_.ae()->exposures[0].sensor_exposure->analog_gain_code_global; + + resultsHistory_.Push(results_); + setControls(frame); IPU3Action op; @@ -392,7 +401,8 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params) void IPAIPU3::parseStatistics(unsigned int frame, int64_t frameTimestamp, - const ipu3_uapi_stats_3a *stats) + const ipu3_uapi_stats_3a *stats, + const ControlList& sensorCtrls) { ControlList ctrls(controls::controls); @@ -403,10 +413,43 @@ void IPAIPU3::parseStatistics(unsigned int frame, */ ASSERT (frameTimestamp > 0); - aiq_.setStatistics(frame, frameTimestamp, results_, stats); + + /* Ae algorithm expects the statistics to be set with its corresponding + * Ae result, i.e., the Ae result should match the exposure time and + * analog gain with the the effective sensor controls of the statistics. + * Search the required Ae result in the result history and combine it + * with the latest result as the input to AIQ::setStatistics(). + */ + + int32_t effectiveExpo = 0; + int32_t effectiveGain = 0; + ControlValue ctrlValue; + + ctrlValue = sensorCtrls.get(V4L2_CID_EXPOSURE); + if (!ctrlValue.isNone()) + effectiveExpo = ctrlValue.get<int32_t>(); + + ctrlValue = sensorCtrls.get(V4L2_CID_ANALOGUE_GAIN); + if (!ctrlValue.isNone()) + effectiveGain = ctrlValue.get<int32_t>(); + + auto pred = [effectiveExpo, effectiveGain] (aiq::AiqResults& result) { + ia_aiq_exposure_sensor_parameters* sensorExposure = + result.ae()->exposures[0].sensor_exposure; + + return (effectiveExpo == sensorExposure->coarse_integration_time || + effectiveGain == sensorExposure->analog_gain_code_global); + }; + aiq::AiqResults& aeMatchedResults = resultsHistory_.searchBackward(pred, results_); + + aiq::AiqResults combinedResults = results_; + combinedResults.setAe(aeMatchedResults.ae()); + + /* Aiq library expects timestamp in microseconds */ + aiq_.setStatistics(frame, (frameTimestamp / 1000), combinedResults, stats); /* Set frame durations from exposure results */ - ia_aiq_exposure_sensor_parameters *sensorExposure = results_.ae()->exposures->sensor_exposure; + ia_aiq_exposure_sensor_parameters *sensorExposure = combinedResults.ae()->exposures->sensor_exposure; int64_t frameDuration = (sensorExposure->line_length_pixels * sensorExposure->frame_length_lines) / (sensorInfo_.pixelRate / 1e6); ctrls.set(controls::FrameDuration, frameDuration); @@ -423,10 +466,11 @@ void IPAIPU3::setControls(unsigned int frame) IPU3Action op; op.op = ActionSetSensorControls; - ControlList ctrls(ctrls_); - ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_)); - ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain_)); - op.controls = ctrls; + ControlList sensorCtrls(ctrls_); + sensorCtrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_)); + sensorCtrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain_)); + + op.sensorControls = sensorCtrls; queueFrameAction.emit(frame, op); }
Set the statistics with the latest AE AiqResults which has the same exposure time and analog gain. The patch reduces the AE haunting during the converging process. Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> --- aiq/aiq_input_parameters.cpp | 2 +- ipu3.cpp | 66 ++++++++++++++++++++++++++++++------ 2 files changed, 56 insertions(+), 12 deletions(-)