[libcamera-devel] [PATCH 2/3] libcamera: ipa: raspberrypi: ALSC: Resample luminance table

[Laurent Pinchart]

Hi David,

Thank you for the patch.

On Thu, Jul 30, 2020 at 12:11:33PM +0100, David Plowman wrote:
> This fixes a bug where the luminance correction table was not being
> resampled according to the camera mode, in the same way as the colour
> tables. This could be noticeable if any camera modes crop
> aggressively.
> 
> This resampling can be done "up front" in the SwitchMode, as we have
> only a single fixed luminance table. In order to protect the
> recalculation of the table from the asynchronous thread (which reads
> it) I've elected to wait for that thread to go idle (though I doubt it
> would have mattered much). As a by-product of stopping the thread, it
> no longer needs its own copy of the camera mode (async_camera_mode_).

This looks good to me. There are however a couple of potential issues
I've noticed when reading the ALSC code and that you may want to check:

- In Alsc::restartAsync(), async_start_ and async_started_ are set
  without holding the mutex_. They could thus potentially be reordered,
  possibly causing a race condition.

- In Alsc::asyncFunc(), sync_signal_.notify_one() is called with the
  mutex_ held. The function will cause the thread waiting on the
  sync_signal_ condition variable to wake up, to then go back to sleep
  immediatelly when it tries to lock the mutex_. The notify_one() call
  should be moved after the locked scope.

- sync_signal_ is actually never used :-) Or rather was never used, as
  this patch now waits on it in Alsc::waitForAysncThread(). The
  sync_signal_ in the AWB algorithm is however completely unused.

> Signed-off-by: David Plowman <david.plowman at raspberrypi.com>

For this patch,

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> ---
>  src/ipa/raspberrypi/controller/rpi/alsc.cpp | 39 ++++++++++++++-------
>  src/ipa/raspberrypi/controller/rpi/alsc.hpp |  3 +-
>  2 files changed, 28 insertions(+), 14 deletions(-)
> 
> diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.cpp b/src/ipa/raspberrypi/controller/rpi/alsc.cpp
> index 76e2f04..8f7720b 100644
> --- a/src/ipa/raspberrypi/controller/rpi/alsc.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/alsc.cpp
> @@ -173,19 +173,34 @@ void Alsc::Initialise()
>  		lambda_r_[i] = lambda_b_[i] = 1.0;
>  }
>  
> +void Alsc::waitForAysncThread()
> +{
> +	std::unique_lock<std::mutex> lock(mutex_);
> +	if (async_started_) {
> +		sync_signal_.wait(lock, [&] {
> +			return async_finished_;
> +		});
> +		async_started_ = false;
> +		async_finished_ = false;
> +	}
> +}
> +
>  void Alsc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)
>  {
>  	(void)metadata;
>  
> +	// Ensure the other thread isn't running while we do this.
> +	waitForAysncThread();
> +
>  	// There's a bit of a question what we should do if the "crop" of the
> -	// camera mode has changed.  Any calculation currently in flight would
> -	// not be useful to the new mode, so arguably we should abort it, and
> -	// generate a new table (like the "first_time" code already here).  When
> -	// the crop doesn't change, we can presumably just leave things
> -	// alone. For now, I think we'll just wait and see. When the crop does
> -	// change, any effects should be transient, and if they're not transient
> -	// enough, we'll revisit the question then.
> +	// camera mode has changed. Should we effectively reset the algorithm
> +	// and start over?
>  	camera_mode_ = camera_mode;
> +
> +	// We must resample the luminance table like we do the others, but it's
> +	// fixed so we can simply do it up front here.
> +	resample_cal_table(config_.luminance_lut, camera_mode_, luminance_table_);
> +
>  	if (first_time_) {
>  		// On the first time, arrange for something sensible in the
>  		// initial tables. Construct the tables for some default colour
> @@ -201,7 +216,7 @@ void Alsc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)
>  		compensate_lambdas_for_cal(cal_table_b, lambda_b_,
>  					   async_lambda_b_);
>  		add_luminance_to_tables(sync_results_, async_lambda_r_, 1.0,
> -					async_lambda_b_, config_.luminance_lut,
> +					async_lambda_b_, luminance_table_,
>  					config_.luminance_strength);
>  		memcpy(prev_sync_results_, sync_results_,
>  		       sizeof(prev_sync_results_));
> @@ -266,8 +281,6 @@ void Alsc::restartAsync(StatisticsPtr &stats, Metadata *image_metadata)
>  	}
>  	copy_stats(statistics_, stats, alsc_status);
>  	frame_phase_ = 0;
> -	// copy the camera mode so it won't change during the calculations
> -	async_camera_mode_ = camera_mode_;
>  	async_start_ = true;
>  	async_started_ = true;
>  	async_signal_.notify_one();
> @@ -670,9 +683,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, camera_mode_, 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, camera_mode_, cal_table_b);
>  	// You could print out the cal tables for this image here, if you're
>  	// tuning the algorithm...
>  	(void)print_cal_table;
> @@ -695,7 +708,7 @@ void Alsc::doAlsc()
>  	compensate_lambdas_for_cal(cal_table_b, lambda_b_, async_lambda_b_);
>  	// Fold in the luminance table at the appropriate strength.
>  	add_luminance_to_tables(async_results_, async_lambda_r_, 1.0,
> -				async_lambda_b_, config_.luminance_lut,
> +				async_lambda_b_, luminance_table_,
>  				config_.luminance_strength);
>  }
>  
> diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.hpp b/src/ipa/raspberrypi/controller/rpi/alsc.hpp
> index e895913..95572af 100644
> --- a/src/ipa/raspberrypi/controller/rpi/alsc.hpp
> +++ b/src/ipa/raspberrypi/controller/rpi/alsc.hpp
> @@ -60,10 +60,10 @@ private:
>  	AlscConfig config_;
>  	bool first_time_;
>  	CameraMode camera_mode_;
> +	double luminance_table_[ALSC_CELLS_X * ALSC_CELLS_Y];
>  	std::thread async_thread_;
>  	void asyncFunc(); // asynchronous thread function
>  	std::mutex mutex_;
> -	CameraMode async_camera_mode_;
>  	// condvar for async thread to wait on
>  	std::condition_variable async_signal_;
>  	// condvar for synchronous thread to wait on
> @@ -86,6 +86,7 @@ private:
>  	int frame_count2_;
>  	double sync_results_[3][ALSC_CELLS_Y][ALSC_CELLS_X];
>  	double prev_sync_results_[3][ALSC_CELLS_Y][ALSC_CELLS_X];
> +	void waitForAysncThread();
>  	// The following are for the asynchronous thread to use, though the main
>  	// thread can set/reset them if the async thread is known to be idle:
>  	void restartAsync(StatisticsPtr &stats, Metadata *image_metadata);

-- 
Regards,

Laurent Pinchart