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

[Hanlin Chen]

On Tue, Oct 12, 2021 at 8:00 PM Kieran Bingham
<kieran.bingham at ideasonboard.com> wrote:
>
> Quoting Umang Jain (2021-10-11 09:23:41)
> > Hi Hanlin,
> >
> > On 10/8/21 3:55 PM, Hanlin Chen wrote:
> > > 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().
> >
> >
> > Ah yes, I missed that. It's been a while I looked at that code.
> >
> > > 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.
> >
> >
> > Yes, we need to re-look at those parts.
> >
> > I think we need to have a discussion to link AiqInputParameters and
> > AiqResults somehow. Since results' pointers are input params for some
> > operations. This might require some rework but the end result will be
> > clearer in my opinion.
> >
> >
> > >
> > >> 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 -).
>
> I'm back ;-) Sorry for the delay.
Thanks Kieran, and welcome back -:)
>
> Do you want to make changes to these patches and send a V2? or do you
> mean patches on top ?
I was thinking of changing the commit messages as Umang suggested, and
re-post the changes.
>
> Is the deep copying an issue on this patch?
No, I suppose it could/should be a separate discussion.
>
>
>
>
> > >> Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>
> > >>
> > >>>    }
> > >>>
> > >>>    } /* namespace ipa::ipu3::aiq */