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

[Umang Jain]

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 at google.com>
>
> Apply shading adapter to correct lens shading for both camera.
>
> Signed-off-by: Han-Lin Chen<hanlinchen at 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