[libcamera-devel,3/3] ipu3: Apply shading adapter as part of AIQ::run2a()
diff mbox series

Message ID mailman.446.1633340920.837.libcamera-devel@lists.libcamera.org
State Superseded
Headers show
Series
  • Fix dark caputred image with close sourced IPU3 IPA
Related show

Commit Message

Han-lin Chen Oct. 4, 2021, 9:48 a.m. UTC 
From: hanlinchen <hanlinchen@google.com>

Apply shading adapter to correct lens shading for both camera.

Signed-off-by: Han-Lin Chen <hanlinchen@google.com>
---
 aiq/aiq.cpp | 20 ++++++++++++++++++--
 aiq/aiq.h   |  4 +++-
 ipu3.cpp    |  2 +-
 3 files changed, 22 insertions(+), 4 deletions(-)

Comments

Umang Jain Oct. 5, 2021, 6:12 a.m. UTC | #1 
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@google.com>
>
> Apply shading adapter to correct lens shading for both camera.
>
> Signed-off-by: Han-Lin Chen<hanlinchen@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)

>   	if (ret) {
>   		LOG(IPAIPU3, Error) << "Failed to configure the AIQ";
>   		return ret;
> -- 2.33.0.800.g4c38ced690-goog
Hanlin Chen Oct. 7, 2021, 7:20 a.m. UTC | #2 
On Thu, Oct 7, 2021 at 3:08 PM Han-lin Chen <hanlinchen@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@google.com>
> >
> > Apply shading adapter to correct lens shading for both camera.
> >
> > Signed-off-by: Han-Lin Chen<hanlinchen@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

Patch
diff mbox series 

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)
 namespace ipa::ipu3::aiq {
 
 AIQ::AIQ()
-	: aiq_(nullptr)
+	: aiq_(nullptr),
+		sensor_frame_params_{}
 {
 	LOG(AIQ, Info) << "Creating IA AIQ Wrapper";
 }
@@ -105,10 +106,19 @@  int AIQ::init(BinaryData &aiqb, BinaryData &nvm, BinaryData &aiqd)
 	return 0;
 }
 
-int AIQ::configure()
+int AIQ::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;
+
 	return 0;
 }
 
@@ -154,6 +164,11 @@  int AIQ::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_;
+	params.saParams.awb_results = results.awb();
+	shadingAdapterRun(params.saParams, results);
+
 	afRun(params.afParams, results);
 
 	return 0;
@@ -328,6 +343,7 @@  int AIQ::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 @@  int IPAIPU3::configure(const IPAConfigInfo &configInfo)
 
 	int ret;
 
-	ret = aiq_.configure();
+	ret = aiq_.configure(configInfo);
 	if (ret) {
 		LOG(IPAIPU3, Error) << "Failed to configure the AIQ";
 		return ret;