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

[Hanlin Chen]

Hi Umang,
Many thanks for the review.

On Fri, Oct 8, 2021 at 2:08 AM Umang Jain <umang.jain at ideasonboard.com> wrote:
>
> Hi Han-Lin
>
> Thank you for the patch.
>
> On 10/7/21 12:51 PM, Han-Lin Chen wrote:
> > Apply shading adapter to correct lens shading for both camera.
>
>
> both?
>
> The SA will be applied to whichever camera is running with the IPA
> right? I guess we only need a generic statement here like:
>
>      Populate shading adapter input parameters and run it as a part of
> run2a()
>      to correct lens shading for the camera.
>
Sounds great.
> >
> > Signed-off-by: Han-Lin Chen <hanlinchen at chromium.org>
> > ---
> >   aiq/aiq.cpp                  |  3 +++
> >   aiq/aiq_input_parameters.cpp | 12 ++++++++++++
> >   2 files changed, 15 insertions(+)
> >
> > diff --git a/aiq/aiq.cpp b/aiq/aiq.cpp
> > index 708e9d6..24c61cb 100644
> > --- a/aiq/aiq.cpp
> > +++ b/aiq/aiq.cpp
> > @@ -154,6 +154,9 @@ int AIQ::run2a(unsigned int frame, AiqInputParameters &params,
> >       params.paParams.exposure_params = results.ae()->exposures[0].exposure;
> >       parameterAdapterRun(params.paParams, results);
> >
> > +     params.saParams.awb_results = results.awb();
> > +     shadingAdapterRun(params.saParams, results);
> > +
> >       afRun(params.afParams, results);
> >
> >       return 0;
> > diff --git a/aiq/aiq_input_parameters.cpp b/aiq/aiq_input_parameters.cpp
> > index 8a53849..36e2b07 100644
> > --- a/aiq/aiq_input_parameters.cpp
> > +++ b/aiq/aiq_input_parameters.cpp
> > @@ -89,6 +89,15 @@ int AiqInputParameters::configure(const IPAConfigInfo &configInfo)
> >       /* Guess from hal-configs-nautilus/files/camera3_profiles.xml#263 */
> >       sensorDescriptor.coarse_integration_time_max_margin = 10;
> >
> > +     sensorFrameParams.horizontal_crop_offset = 0;
> > +     sensorFrameParams.vertical_crop_offset = 0;
> > +     sensorFrameParams.cropped_image_width = configInfo.sensorInfo.analogCrop.width;
> > +     sensorFrameParams.cropped_image_height = configInfo.sensorInfo.analogCrop.height;
> > +     sensorFrameParams.horizontal_scaling_numerator = 1;
> > +     sensorFrameParams.horizontal_scaling_denominator = 1;
> > +     sensorFrameParams.vertical_scaling_numerator = 1;
> > +     sensorFrameParams.vertical_scaling_denominator = 1;
> > +
> >       return 0;
> >   }
> >
> > @@ -165,6 +174,9 @@ void AiqInputParameters::setAeAwbAfDefaults()
> >       gbceParams.tone_map_level = ia_aiq_tone_map_level_default;
> >       gbceParams.frame_use = ia_aiq_frame_use_still;
> >       gbceParams.ev_shift = 0;
> > +
> > +     /* SA Params */
> > +     saParams.frame_use = ia_aiq_frame_use_still;
>
>
> I think we also need here:
>
>      +  saParams.sensor_frame_params = &sensorFrameParams;
>
> To actually pass sensor frame params to the SA run.
>
It's bound to &sensorFrameParams in reset() as part of the init().
It does occur to me. Because the pointer contents are allocated in
private members already, other pointers,
for example awbResults and colorGians, may be deep copied as well.

> Other than that, I think we are almost there for merging this series.
> I'll let Kieran come back and take a look and we can handle the merge
> (fixing the above suggestions as well)
Thanks. I will wait for Kieran's back before I create the next patch
set in case I spam too much in the mailing list -).
>
> Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>
>
> >   }
> >
> >   } /* namespace ipa::ipu3::aiq */