[libcamera-devel] [PATCH 3/3] ipu3: Apply shading adapter as part of AIQ::run2a()

[Hanlin Chen]

On Thu, Oct 7, 2021 at 3:08 PM Han-lin Chen <hanlinchen at google.com> wrote:
>
> Hello Han-Lin,
>
> Thank you for the patch.
>
> Since it's a relatively simple patch to handle geared for SA only, my
> comments are mostly around  how it should handling it design-wise (for
> Kieran to double-check when he's back). It can be parallel-ly applied by
> me (or Kieran) and we can take care of handling it internally (unless
> you have an strong urge to work on this)  ;-)
>
> On 10/4/21 3:18 PM, Han-Lin Chen via libcamera-devel wrote:
> > From: hanlinchen<hanlinchen at google.com>
> >
> > Apply shading adapter to correct lens shading for both camera.
> >
> > Signed-off-by: Han-Lin Chen<hanlinchen at google.com>
> > ---
> >   aiq/aiq.cpp | 20 ++++++++++++++++++--
> >   aiq/aiq.h   |  4 +++-
> >   ipu3.cpp    |  2 +-
> >   3 files changed, 22 insertions(+), 4 deletions(-)
> >
> > diff --git a/aiq/aiq.cpp b/aiq/aiq.cpp
> > index 708e9d6..0a92eaf 100644
> > --- a/aiq/aiq.cpp
> > +++ b/aiq/aiq.cpp
> > @@ -20,7 +20,8 @@ LOG_DEFINE_CATEGORY(AIQ)
> >   namespaceipa::ipu3::aiq  {
> >
> >   AIQ::AIQ()
> > -     : aiq_(nullptr)
> > +     : aiq_(nullptr),
> > +             sensor_frame_params_{}
> >   {
> >       LOG(AIQ, Info) << "Creating IA AIQ Wrapper";
> >   }
> > @@ -105,10 +106,19 @@ intAIQ::init(BinaryData  &aiqb, BinaryData &nvm, BinaryData &aiqd)
> >       return 0;
> >   }
> >
> > -intAIQ::configure()
> > +intAIQ::configure(const  struct IPAConfigInfo &configInfo)
> >   {
> >       LOG(AIQ, Debug) << "Configure AIQ";
> >
> > +     sensor_frame_params_.horizontal_crop_offset = 0;
> > +     sensor_frame_params_.vertical_crop_offset = 0;
> > +     sensor_frame_params_.cropped_image_width = configInfo.sensorInfo.analogCrop.width;
> > +     sensor_frame_params_.cropped_image_height = configInfo.sensorInfo.analogCrop.height;
> > +     sensor_frame_params_.horizontal_scaling_numerator = 1;
> > +     sensor_frame_params_.horizontal_scaling_denominator = 1;
> > +     sensor_frame_params_.vertical_scaling_numerator = 1;
> > +     sensor_frame_params_.vertical_scaling_denominator = 1;
> > +
>
>
> I would apply sensor information inside AiqInputParameters::configure()
> since it already has IPAConfigInfo and sensorFrameParams. Currently we
> don't seem to set sensorFrameParams, so I would set the above change in
> there.
>
> >       return 0;
> >   }
> >
> > @@ -154,6 +164,11 @@ intAIQ::run2a(unsigned  int frame, AiqInputParameters &params,
> >       params.paParams.exposure_params = results.ae()->exposures[0].exposure;
> >       parameterAdapterRun(params.paParams, results);
> >
> > +     params.saParams.frame_use = params.aeInputParams.frame_use;
> > +     params.saParams.sensor_frame_params = &sensor_frame_params_;
>
>
> Broadly speaking, for setting input parameters for various algorithms,
> we have a dedicated class AiqInputParameters. The above two changes
> should be moved to AiqInputParameters::setAeAwbAfDefaults() (with scope
> of bikeshedding the name of function).
>
> > +     params.saParams.awb_results = results.awb();
>
>
> ... This can stay as is since it's dependent on results of awb
>
> > +     shadingAdapterRun(params.saParams, results);
> > +
> >       afRun(params.afParams, results);
> >
> >       return 0;
> > @@ -328,6 +343,7 @@ intAIQ::shadingAdapterRun(ia_aiq_sa_input_params  &saParams,
> >   {
> >       ia_aiq_sa_results *saResults = nullptr;
> >       ia_err err = ia_aiq_sa_run(aiq_, &saParams, &saResults);
> > +
> >       if (err) {
> >               LOG(AIQ, Error) << "Failed to run shading adapter: "
> >                               << decodeError(err);
> > diff --git a/aiq/aiq.h b/aiq/aiq.h
> > index fcd02d2..8a68827 100644
> > --- a/aiq/aiq.h
> > +++ b/aiq/aiq.h
> > @@ -35,7 +35,7 @@ public:
> >       ~AIQ();
> >
> >       int init(BinaryData &aiqb, BinaryData &nvm, BinaryData &aiqd);
> > -     int configure();
> > +     int configure(const struct IPAConfigInfo &configInfo);
> >       int setStatistics(unsigned int frame,
> >                         int64_t timestamp, AiqResults &results,
> >                         const ipu3_uapi_stats_3a *stats);
> > @@ -62,6 +62,8 @@ private:
> >       ia_cmc_t *iaCmc_;
> >       std::string  version_;
> >
> > +  ia_aiq_frame_params sensor_frame_params_;
> > +
> >       IPAIPU3Stats *aiqStats_;
> >   };
> >
> > diff --git a/ipu3.cpp b/ipu3.cpp
> > index b60c58c..ed3c516 100644
> > --- a/ipu3.cpp
> > +++ b/ipu3.cpp
> > @@ -232,7 +232,7 @@ intIPAIPU3::configure(const  IPAConfigInfo &configInfo)
> >
> >       int ret;
> >
> > -     ret = aiq_.configure();
> > +     ret = aiq_.configure(configInfo);
>
>
> As mentioned earlier AiqInputParameters::configure() already takes in
> configInfo, we should be using that instead to set sensorFrameParams and
> finally set SA input parameters (in AiqInputParameters::setAeAwbAfDefaults)


Thanks. I see it now. it's more reasonable.
>
>
> >       if (ret) {
> >               LOG(IPAIPU3, Error) << "Failed to configure the AIQ";
> >               return ret;
> > -- 2.33.0.800.g4c38ced690-goog
>
>
> --
> Cheers.
> Hanlin Chen