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

[Kieran Bingham]

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.

Do you want to make changes to these patches and send a V2? or do you
mean patches on top ?

Is the deep copying an issue on this patch?




> >> Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>
> >>
> >>>    }
> >>>
> >>>    } /* namespace ipa::ipu3::aiq */