| Message ID | 20211007072147.1289490-3-hanlinchen@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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. > > Signed-off-by: Han-Lin Chen <hanlinchen@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 ¶ms, > 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. 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) Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > } > > } /* namespace ipa::ipu3::aiq */
Hi Umang, Many thanks for the review. On Fri, Oct 8, 2021 at 2:08 AM Umang Jain <umang.jain@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@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 ¶ms, > > 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@ideasonboard.com> > > > } > > > > } /* namespace ipa::ipu3::aiq */
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@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@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 ¶ms, >>> 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 -). >> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> >> >>> } >>> >>> } /* namespace ipa::ipu3::aiq */
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@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@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 ¶ms, > >>> 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@ideasonboard.com> > >> > >>> } > >>> > >>> } /* namespace ipa::ipu3::aiq */
On Tue, Oct 12, 2021 at 8:00 PM Kieran Bingham <kieran.bingham@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@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@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 ¶ms, > > >>> 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@ideasonboard.com> > > >> > > >>> } > > >>> > > >>> } /* namespace ipa::ipu3::aiq */
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 ¶ms, 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; } } /* namespace ipa::ipu3::aiq */
Apply shading adapter to correct lens shading for both camera. Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> --- aiq/aiq.cpp | 3 +++ aiq/aiq_input_parameters.cpp | 12 ++++++++++++ 2 files changed, 15 insertions(+)