[libcamera-devel,v3,5/6] src: ipa: raspberrypi: Estimate the colour temerature if starting with fixed colour gains

10617 diff mbox series

When the AWB is started from "cold" with fixed colour gains, we try to
estimate the colour temperature this corresponds to (if a calibrated
CT curve was supplied). When fixed colour gains are set after the AWB
has been running, we leave the CT estimate alone, as the one we have
is probably sensible.

This estimated colour is passed out in the metadata for other
algorithms - notably ALSC - to use.

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/ipa/raspberrypi/controller/rpi/alsc.cpp |  6 +++++-
 src/ipa/raspberrypi/controller/rpi/awb.cpp  | 14 ++++++++++++++
 src/ipa/raspberrypi/controller/rpi/awb.hpp  |  1 +
 3 files changed, 20 insertions(+), 1 deletion(-)

Hi David,

Thank you for the patch.

On Tue, Dec 08, 2020 at 08:44:40PM +0000, David Plowman wrote:
> When the AWB is started from "cold" with fixed colour gains, we try to
> estimate the colour temperature this corresponds to (if a calibrated
> CT curve was supplied). When fixed colour gains are set after the AWB
> has been running, we leave the CT estimate alone, as the one we have
> is probably sensible.
> 
> This estimated colour is passed out in the metadata for other
> algorithms - notably ALSC - to use.
> 
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/ipa/raspberrypi/controller/rpi/alsc.cpp |  6 +++++-
>  src/ipa/raspberrypi/controller/rpi/awb.cpp  | 14 ++++++++++++++
>  src/ipa/raspberrypi/controller/rpi/awb.hpp  |  1 +
>  3 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.cpp b/src/ipa/raspberrypi/controller/rpi/alsc.cpp
> index 183a0c95..c354c985 100644
> --- a/src/ipa/raspberrypi/controller/rpi/alsc.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/alsc.cpp
> @@ -146,6 +146,7 @@ void Alsc::Read(boost::property_tree::ptree const &params)
>  	config_.threshold = params.get<double>("threshold", 1e-3);
>  }
>  
> +static double get_ct(Metadata *metadata, double default_ct);
>  static void get_cal_table(double ct,
>  			  std::vector<AlscCalibration> const &calibrations,
>  			  double cal_table[XY]);
> @@ -210,6 +211,9 @@ void Alsc::SwitchMode(CameraMode const &camera_mode,
>  	// change.
>  	bool reset_tables = first_time_ || compare_modes(camera_mode_, camera_mode);
>  
> +	// Believe the colour temperature from the AWB, if there is one.
> +	ct_ = get_ct(metadata, ct_);
> +
>  	// Ensure the other thread isn't running while we do this.
>  	waitForAysncThread();
>  
> @@ -254,7 +258,7 @@ void Alsc::fetchAsyncResults()
>  	memcpy(sync_results_, async_results_, sizeof(sync_results_));
>  }
>  
> -static double get_ct(Metadata *metadata, double default_ct)
> +double get_ct(Metadata *metadata, double default_ct)

Is this required ? I see there are other internal functions that are
forward-declared as static and then defined as non-static. I thus won't
modify this when applying, but if it's not needed, a patch on top to
address all the occurrences could be nice. Bonus points if the functions
can be reordered to avoid statuc declarations.

>  {
>  	AwbStatus awb_status;
>  	awb_status.temperature_K = default_ct; // in case nothing found
> diff --git a/src/ipa/raspberrypi/controller/rpi/awb.cpp b/src/ipa/raspberrypi/controller/rpi/awb.cpp
> index 10600f75..f66c2b29 100644
> --- a/src/ipa/raspberrypi/controller/rpi/awb.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/awb.cpp
> @@ -19,6 +19,9 @@ using namespace RPiController;
>  
>  const double Awb::RGB::INVALID = -1.0;
>  
> +// todo - the locking in this algorithm needs some tidying up as has been done
> +// elsewhere (ALSC and AGC).
> +
>  void AwbMode::Read(boost::property_tree::ptree const &params)
>  {
>  	ct_lo = params.get<double>("lo");
> @@ -121,6 +124,7 @@ Awb::Awb(Controller *controller)
>  	async_abort_ = async_start_ = async_started_ = async_finished_ = false;
>  	mode_ = nullptr;
>  	manual_r_ = manual_b_ = 0.0;
> +	first_switch_mode_ = true;
>  	async_thread_ = std::thread(std::bind(&Awb::asyncFunc, this));
>  }
>  
> @@ -199,9 +203,19 @@ void Awb::SwitchMode([[maybe_unused]] CameraMode const &camera_mode,
>  		prev_sync_results_.gain_r = manual_r_;
>  		prev_sync_results_.gain_g = 1.0;
>  		prev_sync_results_.gain_b = manual_b_;
> +		// If we're starting up for the first time, try and
> +		// "dead reckon" the corresponding colour temperature.
> +		if (first_switch_mode_ && config_.bayes) {
> +			Pwl ct_r_inverse = config_.ct_r.Inverse();
> +			Pwl ct_b_inverse = config_.ct_b.Inverse();
> +			double ct_r = ct_r_inverse.Eval(ct_r_inverse.Domain().Clip(1 / manual_r_));
> +			double ct_b = ct_b_inverse.Eval(ct_b_inverse.Domain().Clip(1 / manual_b_));
> +			prev_sync_results_.temperature_K = (ct_r + ct_b) / 2;
> +		}
>  		sync_results_ = prev_sync_results_;
>  	}
>  	metadata->Set("awb.status", prev_sync_results_);
> +	first_switch_mode_ = false;
>  }
>  
>  void Awb::fetchAsyncResults()
> diff --git a/src/ipa/raspberrypi/controller/rpi/awb.hpp b/src/ipa/raspberrypi/controller/rpi/awb.hpp
> index 6fc045ce..83b81498 100644
> --- a/src/ipa/raspberrypi/controller/rpi/awb.hpp
> +++ b/src/ipa/raspberrypi/controller/rpi/awb.hpp
> @@ -159,6 +159,7 @@ private:
>  	double manual_r_;
>  	// manual b setting
>  	double manual_b_;
> +	bool first_switch_mode_; // is this the first call to SwitchMode?
>  };
>  
>  static inline Awb::RGB operator+(Awb::RGB const &a, Awb::RGB const &b)