[libcamera-devel] [PATCH v2 3/6] ipu3: Set statistics with the effective AE AiqResults
Kieran Bingham
kieran.bingham at ideasonboard.com
Wed Nov 10 15:14:48 CET 2021
Quoting Hanlin Chen (2021-11-10 14:06:30)
> On Wed, Nov 10, 2021 at 9:48 PM Kieran Bingham
> <kieran.bingham at 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 at 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 at 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
> > > > >
More information about the libcamera-devel
mailing list