[libcamera-devel] [PATCH v2 4/6] ipu3: Apply auto focus and send lens controls to pipeline handler

[Hanlin Chen]

Hi Umang,

On Wed, Nov 10, 2021 at 10:07 PM Umang Jain <umang.jain at ideasonboard.com> wrote:
>
> Hi,
>
> On 11/10/21 7:05 PM, Hanlin Chen wrote:
> > Hi Kieran,
> >
> > On Thu, Nov 4, 2021 at 6:23 AM Kieran Bingham
> > <kieran.bingham at ideasonboard.com> wrote:
> >> Quoting Han-Lin Chen (2021-10-29 12:59:59)
> >>> Apply auto focus and send lens controls to pipeline handler.
> >>>
> >>> Signed-off-by: Han-Lin Chen <hanlinchen at chromium.org>
> >>> ---
> >>>   aiq/aiq.cpp                  |  5 ++++-
> >>>   aiq/aiq.h                    |  2 +-
> >>>   aiq/aiq_input_parameters.cpp |  2 +-
> >>>   ipu3.cpp                     | 27 ++++++++++++++++++++++++++-
> >>>   4 files changed, 32 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/aiq/aiq.cpp b/aiq/aiq.cpp
> >>> index 24c61cb..9c7f9d6 100644
> >>> --- a/aiq/aiq.cpp
> >>> +++ b/aiq/aiq.cpp
> >>> @@ -139,7 +139,7 @@ int AIQ::setStatistics(unsigned int frame,
> >>>    * of the parameter buffer being the only part handled when called for.
> >>>    */
> >>>   int AIQ::run2a(unsigned int frame, AiqInputParameters &params,
> >>> -              AiqResults &results)
> >>> +                    AiqResults &results, int lensPosition, int64_t lensMovementStartTime)
> >> Indentation changed for the worse there I think.
> >>
> >> Do we really need to pass in individual parameters through the run
> >> function though? I don't think that's sustainable - as we'll likely have
> >> to pass in more parameters.
> >>
> >> Is the lens position / start time something that would already be
> >> expected to be passed in therou the AiqInputParametners? Or would we
> >> need to define a new parameter structure to pass into the run function?
> > Agreed. I should set it in AiqInputParametners directly.
>
>
> As stated in the v1, I guess we are due for a refactoring to handle
> various input parameters for the algorithms. Probably also need to work
> out a design plan and schedule. If it doesn't happen in this series or
> before this, I recommend adding a \todo here.
>
>
> v1
> https://lists.libcamera.org/pipermail/libcamera-devel/2021-October/026452.html
SGTM.
>
> >>
> >>
> >>>   {
> >>>          (void)frame;
> >>>
> >>> @@ -157,6 +157,9 @@ int AIQ::run2a(unsigned int frame, AiqInputParameters &params,
> >>>          params.saParams.awb_results = results.awb();
> >>>          shadingAdapterRun(params.saParams, results);
> >>>
> >>> +       params.afParams.lens_position = lensPosition;
> >>> +       params.afParams.lens_movement_start_timestamp = lensMovementStartTime;
> >>> +
> >>>          afRun(params.afParams, results);
> >>>
> >>>          return 0;
> >>> diff --git a/aiq/aiq.h b/aiq/aiq.h
> >>> index fcd02d2..4f5f868 100644
> >>> --- a/aiq/aiq.h
> >>> +++ b/aiq/aiq.h
> >>> @@ -41,7 +41,7 @@ public:
> >>>                            const ipu3_uapi_stats_3a *stats);
> >>>
> >>>          int run2a(unsigned int frame, AiqInputParameters &params,
> >>> -                 AiqResults &results);
> >>> +                 AiqResults &results, int lensPosition, int64_t lensMovementStartTime);
> >>>
> >>>   private:
> >>>          std::string decodeError(ia_err err);
> >>> diff --git a/aiq/aiq_input_parameters.cpp b/aiq/aiq_input_parameters.cpp
> >>> index 46553a6..5dd2f6c 100644
> >>> --- a/aiq/aiq_input_parameters.cpp
> >>> +++ b/aiq/aiq_input_parameters.cpp
> >>> @@ -166,7 +166,7 @@ void AiqInputParameters::setAeAwbAfDefaults()
> >>>                  ia_aiq_af_range_normal,
> >>>                  ia_aiq_af_metering_mode_auto,
> >>>                  ia_aiq_flash_mode_off,
> >>> -               NULL, NULL, false
> >>> +               &focusRect, &manualFocusParams, false
> >>>          };
> >>>
> >>>          /* GBCE Params */
> >>> diff --git a/ipu3.cpp b/ipu3.cpp
> >>> index f463805..c2dc754 100644
> >>> --- a/ipu3.cpp
> >>> +++ b/ipu3.cpp
> >>> @@ -77,6 +77,10 @@ private:
> >>>          uint32_t gain_;
> >>>          uint32_t minGain_;
> >>>          uint32_t maxGain_;
> >>> +       int32_t lensPosition_;
> >>> +
> >>> +       /* Intel AF library relies on timestamp to wait for lens movement */
> >>> +       uint64_t lensMovementStartTime_;
> >>>
> >>>          /* Intel Library Instances. */
> >>>          aiq::AIQ aiq_;
> >>> @@ -258,6 +262,9 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
> >>>          maxGain_ = itGain->second.max().get<int32_t>();
> >>>          gain_ = maxGain_;
> >>>
> >>> +       lensMovementStartTime_ = 0;
> >>> +       lensPosition_ = 0;
> >>> +
> >>>          int ret;
> >>>
> >>>          ret = aiq_.configure();
> >>> @@ -381,7 +388,7 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)
> >>>          */
> >>>
> >>>          /* Run algorithms into/using this context structure */
> >>> -       aiq_.run2a(frame, aiqInputParams_, results_);
> >>> +       aiq_.run2a(frame, aiqInputParams_, results_, lensPosition_, lensMovementStartTime_);
> >>>
> >>>          aic_.updateRuntimeParams(results_);
> >>>          aic_.run(params);
> >>> @@ -389,6 +396,19 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)
> >>>          exposure_ = results_.ae()->exposures[0].sensor_exposure->coarse_integration_time;
> >>>          gain_ = results_.ae()->exposures[0].sensor_exposure->analog_gain_code_global;
> >>>
> >>> +       /*
> >>> +        * Af algorithm compares the timestamp of start of lens movement and the
> >>> +        * that of the statistics generated to estimate whether next lens
> >>> +        * positon should be produced.
> >>> +        * Todo: Use the lens movement start time reported by the pipeline handler.
> >>> +        */
> >>> +       if (lensPosition_ != results_.af()->next_lens_position) {
> >>> +               utils::time_point time = utils::clock::now();
> >>> +               uint64_t msecs = std::chrono::duration_cast<std::chrono::microseconds>(time.time_since_epoch()).count();
> >>> +               lensMovementStartTime_ = msecs;
> >>> +       }
> >>> +       lensPosition_ = results_.af()->next_lens_position;
> >>> +
> >>>          resultsHistory_.Push(results_);
> >>>
> >>>          setControls(frame);
> >>> @@ -472,6 +492,11 @@ void IPAIPU3::setControls(unsigned int frame)
> >>>
> >>>          op.sensorControls = sensorCtrls;
> >>>
> >>> +       ControlList lensCtrls;
> >>> +       lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE, lensPosition_);
> >>> +
> >>> +       op.lensControls = lensCtrls;
> >> Does this need to create a ControlList just to copy/assign it?
> >> Can we call
> >>
> >>            op.lensControls.set(V4L2_CID_FOCUS_ABSOLUTE, lensPosition_);
> >>
> >> directly?
> > Indeed.
> >>
> >>> +
> >>>          queueFrameAction.emit(frame, op);
> >>>   }
> >>>
> >>> --
> >>> 2.33.1.1089.g2158813163f-goog
> >>>