[libcamera-devel] [PATCH 4/5] libcamera: raspberrypi: ALSC: Handle transform in colour tables

[Laurent Pinchart]

Hi David,

Thank you for the patch.

On Wed, Jul 29, 2020 at 10:31:53AM +0100, David Plowman wrote:
> This commit updates ALSC (Auto Lens Shading Correction) to handle the
> transform now passed in the SwitchMode method.
> 
> The transform is applied by the sensor, so the statistics we get
> already incorporates it, and the adpative algorithm is agnostic to
> it. That leaves only the calibrated tables (assumed measured without
> any user transform) that need to be flipped, and resample_cal_table -
> where we resample the calibrated table anyway - is an easy place to do
> it.
> ---
>  src/ipa/raspberrypi/controller/rpi/alsc.cpp | 22 ++++++++++++++-------
>  src/ipa/raspberrypi/controller/rpi/alsc.hpp |  3 ++-
>  2 files changed, 17 insertions(+), 8 deletions(-)
> 
> diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.cpp b/src/ipa/raspberrypi/controller/rpi/alsc.cpp
> index 5fac9dd..33c1a88 100644
> --- a/src/ipa/raspberrypi/controller/rpi/alsc.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/alsc.cpp
> @@ -151,8 +151,9 @@ static void get_cal_table(double ct,
>  			  std::vector<AlscCalibration> const &calibrations,
>  			  double cal_table[XY]);
>  static void resample_cal_table(double const cal_table_in[XY],
> -			       CameraMode const &camera_mode,
> -			       double cal_table_out[XY]);
> +							   CameraMode const &camera_mode,
> +							   Transform transform,
> +							   double cal_table_out[XY]);

:set tabstop=8

but you may not be using vim :-)

>  static void compensate_lambdas_for_cal(double const cal_table[XY],
>  				       double const old_lambdas[XY],
>  				       double new_lambdas[XY]);
> @@ -187,6 +188,7 @@ void Alsc::SwitchMode(CameraMode const &camera_mode, Transform transform, Metada
>  	// change, any effects should be transient, and if they're not transient
>  	// enough, we'll revisit the question then.
>  	camera_mode_ = camera_mode;
> +	transform_ = transform;
>  	if (first_time_) {
>  		// On the first time, arrange for something sensible in the
>  		// initial tables. Construct the tables for some default colour
> @@ -194,9 +196,9 @@ void Alsc::SwitchMode(CameraMode const &camera_mode, Transform transform, Metada
>  		// adaptive algorithm.
>  		double cal_table_r[XY], cal_table_b[XY], cal_table_tmp[XY];
>  		get_cal_table(4000, config_.calibrations_Cr, cal_table_tmp);
> -		resample_cal_table(cal_table_tmp, camera_mode_, cal_table_r);
> +		resample_cal_table(cal_table_tmp, camera_mode_, transform_, cal_table_r);
>  		get_cal_table(4000, config_.calibrations_Cb, cal_table_tmp);
> -		resample_cal_table(cal_table_tmp, camera_mode_, cal_table_b);
> +		resample_cal_table(cal_table_tmp, camera_mode_, transform_, cal_table_b);
>  		compensate_lambdas_for_cal(cal_table_r, lambda_r_,
>  					   async_lambda_r_);
>  		compensate_lambdas_for_cal(cal_table_b, lambda_b_,
> @@ -379,7 +381,9 @@ void get_cal_table(double ct, std::vector<AlscCalibration> const &calibrations,
>  }
>  
>  void resample_cal_table(double const cal_table_in[XY],
> -			CameraMode const &camera_mode, double cal_table_out[XY])
> +						CameraMode const &camera_mode,
> +						Transform transform,
> +						double cal_table_out[XY])
>  {
>  	// Precalculate and cache the x sampling locations and phases to save
>  	// recomputing them on every row.
> @@ -395,6 +399,8 @@ void resample_cal_table(double const cal_table_in[XY],
>  		xf[i] = x - x_lo[i];
>  		x_hi[i] = std::min(x_lo[i] + 1, X - 1);
>  		x_lo[i] = std::max(x_lo[i], 0);
> +		if (transform.contains(Transform::hflip()))
> +			x_lo[i] = X - 1 - x_lo[i], x_hi[i] = X - 1 - x_hi[i];

		if (transform.contains(Transform::hflip())) {
			x_lo[i] = X - 1 - x_lo[i];
			x_hi[i] = X - 1 - x_hi[i];
		}

No need to hide the code :-)

>  	}
>  	// Now march over the output table generating the new values.
>  	double scale_y = camera_mode.sensor_height /
> @@ -407,6 +413,8 @@ void resample_cal_table(double const cal_table_in[XY],
>  		double yf = y - y_lo;
>  		int y_hi = std::min(y_lo + 1, Y - 1);
>  		y_lo = std::max(y_lo, 0);
> +		if (transform.contains(Transform::vflip()))
> +			y_lo = Y - 1 - y_lo, y_hi = Y - 1 - y_hi;

Same here.

>  		double const *row_above = cal_table_in + X * y_lo;
>  		double const *row_below = cal_table_in + X * y_hi;
>  		for (int i = 0; i < X; i++) {
> @@ -671,9 +679,9 @@ void Alsc::doAlsc()
>  	// Fetch the new calibrations (if any) for this CT. Resample them in
>  	// case the camera mode is not full-frame.
>  	get_cal_table(ct_, config_.calibrations_Cr, cal_table_tmp);
> -	resample_cal_table(cal_table_tmp, async_camera_mode_, cal_table_r);
> +	resample_cal_table(cal_table_tmp, async_camera_mode_, transform_, cal_table_r);
>  	get_cal_table(ct_, config_.calibrations_Cb, cal_table_tmp);
> -	resample_cal_table(cal_table_tmp, async_camera_mode_, cal_table_b);
> +	resample_cal_table(cal_table_tmp, async_camera_mode_, transform_, cal_table_b);
>  	// You could print out the cal tables for this image here, if you're
>  	// tuning the algorithm...
>  	(void)print_cal_table;
> diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.hpp b/src/ipa/raspberrypi/controller/rpi/alsc.hpp
> index 9b54578..9001e8a 100644
> --- a/src/ipa/raspberrypi/controller/rpi/alsc.hpp
> +++ b/src/ipa/raspberrypi/controller/rpi/alsc.hpp
> @@ -60,7 +60,8 @@ private:
>  	// configuration is read-only, and available to both threads
>  	AlscConfig config_;
>  	bool first_time_;
> -	std::atomic<CameraMode> camera_mode_;
> +	CameraMode camera_mode_;

This seems unrelated. If you don't want to split it to a separate patch,
could you please mention it in the commit message with a rationale ?

> +	libcamera::Transform transform_;
>  	std::thread async_thread_;
>  	void asyncFunc(); // asynchronous thread function
>  	std::mutex mutex_;

-- 
Regards,

Laurent Pinchart