[libcamera-devel,v2,4/4] libcamera: ipa: raspberrypi: ALSC: Improve behaviour when camera mode changes
diff mbox series

Message ID 20200731140801.13253-5-david.plowman@raspberrypi.com
State Superseded
Headers show
Series
  • Raspberry Pi ALSC improvements
Related show

Commit Message

David Plowman July 31, 2020, 2:08 p.m. UTC 
Now that we stop the asynchronous thread on a SwitchMode, we would do
better to regenerate all the tables if the new camera mode crops in a
significantly different way to the old one. A few minor tweaks make
sense along with this:

* Reset the lambda values when we reset everything. It wouldn't make
  sense to re-start with the old mode's values.

* Use the last recorded colour temperature to generate new tables rather
  than any default value.

* Set the frame "phase" counter to ensure the adaptive procedure will
  run asap.

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/ipa/raspberrypi/controller/rpi/alsc.cpp | 54 ++++++++++++++-------
 1 file changed, 37 insertions(+), 17 deletions(-)

Comments

Laurent Pinchart July 31, 2020, 9:38 p.m. UTC | #1 
Hi David,

One more thing I'm afraid.

On Fri, Jul 31, 2020 at 03:08:01PM +0100, David Plowman wrote:
> Now that we stop the asynchronous thread on a SwitchMode, we would do
> better to regenerate all the tables if the new camera mode crops in a
> significantly different way to the old one. A few minor tweaks make
> sense along with this:
> 
> * Reset the lambda values when we reset everything. It wouldn't make
>   sense to re-start with the old mode's values.
> 
> * Use the last recorded colour temperature to generate new tables rather
>   than any default value.
> 
> * Set the frame "phase" counter to ensure the adaptive procedure will
>   run asap.
> 
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/ipa/raspberrypi/controller/rpi/alsc.cpp | 54 ++++++++++++++-------
>  1 file changed, 37 insertions(+), 17 deletions(-)
> 
> diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.cpp b/src/ipa/raspberrypi/controller/rpi/alsc.cpp
> index 4df9934..6e781bd 100644
> --- a/src/ipa/raspberrypi/controller/rpi/alsc.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/alsc.cpp
> @@ -166,11 +166,8 @@ void Alsc::Initialise()
>  	RPI_LOG("Alsc");
>  	frame_count2_ = frame_count_ = frame_phase_ = 0;
>  	first_time_ = true;
> -	// Initialise the lambdas. Each call to Process then restarts from the
> -	// previous results.  Also initialise the previous frame tables to the
> -	// same harmless values.
> -	for (int i = 0; i < XY; i++)
> -		lambda_r_[i] = lambda_b_[i] = 1.0;
> +	ct_ = config_.default_ct;
> +	// The lambdas are initialised in the SwitchMode.
>  }
>  
>  void Alsc::waitForAysncThread()
> @@ -185,31 +182,53 @@ void Alsc::waitForAysncThread()
>  	}
>  }
>  
> +static bool compare_modes(CameraMode const &cm0, CameraMode const &cm1)
> +{
> +	// Return true if the modes crop from the sensor significantly differently.
> +	int left_diff = std::abs(cm0.crop_x - cm1.crop_x);
> +	int top_diff = std::abs(cm0.crop_y - cm1.crop_y);
> +	int right_diff = std::abs(cm0.crop_x + cm0.scale_x * cm0.width -
> +				  cm1.crop_x - cm1.scale_x * cm1.width);
> +	int bottom_diff = std::abs(cm0.crop_y + cm0.scale_y * cm0.height -
> +				   cm1.crop_y - cm1.scale_y * cm1.height);

gcc5 complains here:

../../src/ipa/raspberrypi/controller/rpi/alsc.cpp: In function ‘bool compare_modes(const CameraMode&, const CameraMode&)’:
../../src/ipa/raspberrypi/controller/rpi/alsc.cpp:191:43: error: call of overloaded ‘abs(double)’ is ambiguous
       cm1.crop_x - cm1.scale_x * cm1.width);
                                           ^
In file included from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/cstdlib:72:0,
                 from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/ext/string_conversions.h:41,
                 from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/bits/basic_string.h:5249,
                 from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/string:52,
                 from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/stdexcept:39,
                 from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/array:38,
                 from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/tuple:39,
                 from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/mutex:38,
                 from ../../src/ipa/raspberrypi/controller/rpi/alsc.hpp:9,
                 from ../../src/ipa/raspberrypi/controller/rpi/alsc.cpp:10:
/usr/include/stdlib.h:840:12: note: candidate: int abs(int)
 extern int abs (int __x) __THROW __attribute__ ((__const__)) __wur;
            ^
In file included from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/ext/string_conversions.h:41:0,
                 from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/bits/basic_string.h:5249,
                 from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/string:52,
                 from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/stdexcept:39,
                 from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/array:38,
                 from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/tuple:39,
                 from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/mutex:38,
                 from ../../src/ipa/raspberrypi/controller/rpi/alsc.hpp:9,
                 from ../../src/ipa/raspberrypi/controller/rpi/alsc.cpp:10:
/usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/cstdlib:166:3: note: candidate: long int std::abs(long int)
   abs(long __i) { return __builtin_labs(__i); }
   ^
/usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/cstdlib:174:3: note: candidate: long long int std::abs(long long int)
   abs(long long __x) { return __builtin_llabs (__x); }
   ^
../../src/ipa/raspberrypi/controller/rpi/alsc.cpp:193:45: error: call of overloaded ‘abs(double)’ is ambiguous
        cm1.crop_y - cm1.scale_y * cm1.height);
                                             ^
In file included from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/cstdlib:72:0,
                 from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/ext/string_conversions.h:41,
                 from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/bits/basic_string.h:5249,
                 from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/string:52,
                 from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/stdexcept:39,
                 from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/array:38,
                 from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/tuple:39,
                 from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/mutex:38,
                 from ../../src/ipa/raspberrypi/controller/rpi/alsc.hpp:9,
                 from ../../src/ipa/raspberrypi/controller/rpi/alsc.cpp:10:
/usr/include/stdlib.h:840:12: note: candidate: int abs(int)
 extern int abs (int __x) __THROW __attribute__ ((__const__)) __wur;
            ^
In file included from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/ext/string_conversions.h:41:0,
                 from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/bits/basic_string.h:5249,
                 from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/string:52,
                 from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/stdexcept:39,
                 from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/array:38,
                 from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/tuple:39,
                 from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/mutex:38,
                 from ../../src/ipa/raspberrypi/controller/rpi/alsc.hpp:9,
                 from ../../src/ipa/raspberrypi/controller/rpi/alsc.cpp:10:
/usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/cstdlib:166:3: note: candidate: long int std::abs(long int)
   abs(long __i) { return __builtin_labs(__i); }
   ^
/usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/cstdlib:174:3: note: candidate: long long int std::abs(long long int)
   abs(long long __x) { return __builtin_llabs (__x); }
   ^

We may drop support for gcc 5 and 6 soon, but until we do, I would like
to avoid breaking the build. Should these two lines use std::fabs() ?

> +	// These thresholds are a rather arbitrary amount chosen to trigger
> +	// when carrying on with the previously calculated tables might be
> +	// worse than regenerating them (but without the adaptive algorithm).
> +	int threshold_x = cm0.sensor_width >> 4;
> +	int threshold_y = cm0.sensor_height >> 4;
> +	return left_diff > threshold_x || right_diff > threshold_x ||
> +	       top_diff > threshold_y || bottom_diff > threshold_y;
> +}
> +
>  void Alsc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)
>  {
>  	(void)metadata;
>  
> +	// We're going to start over with the tables if there's any "significant"
> +	// change.
> +	bool reset_tables = first_time_ || compare_modes(camera_mode_, camera_mode);
> +
>  	// 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. 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
> -		// temperature. This echoes the code in doAlsc, without the
> -		// adaptive algorithm.
> +	if (reset_tables) {
> +		// Upon every "table reset", arrange for something sensible to be
> +		// generated. Construct the tables for the previous recorded colour
> +		// temperature. In order to start over from scratch we initialise
> +		// the lambdas, but the rest of this code then echoes the code in
> +		// doAlsc, without the adaptive algorithm.
> +		for (int i = 0; i < XY; i++)
> +			lambda_r_[i] = lambda_b_[i] = 1.0;
>  		double cal_table_r[XY], cal_table_b[XY], cal_table_tmp[XY];
> -		get_cal_table(4000, config_.calibrations_Cr, cal_table_tmp);
> +		get_cal_table(ct_, config_.calibrations_Cr, cal_table_tmp);
>  		resample_cal_table(cal_table_tmp, camera_mode_, cal_table_r);
> -		get_cal_table(4000, config_.calibrations_Cb, cal_table_tmp);
> +		get_cal_table(ct_, config_.calibrations_Cb, cal_table_tmp);
>  		resample_cal_table(cal_table_tmp, camera_mode_, cal_table_b);
>  		compensate_lambdas_for_cal(cal_table_r, lambda_r_,
>  					   async_lambda_r_);
> @@ -220,6 +239,7 @@ void Alsc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)
>  					config_.luminance_strength);
>  		memcpy(prev_sync_results_, sync_results_,
>  		       sizeof(prev_sync_results_));
> +		frame_phase_ = config_.frame_period; // run the algo again asap
>  		first_time_ = false;
>  	}
>  }
> @@ -265,8 +285,8 @@ void Alsc::restartAsync(StatisticsPtr &stats, Metadata *image_metadata)
>  {
>  	RPI_LOG("Starting ALSC thread");
>  	// Get the current colour temperature. It's all we need from the
> -	// metadata.
> -	ct_ = get_ct(image_metadata, config_.default_ct);
> +	// metadata. Default to the last CT value (which could be the default).
> +	ct_ = get_ct(image_metadata, ct_);
>  	// We have to copy the statistics here, dividing out our best guess of
>  	// the LSC table that the pipeline applied to them.
>  	AlscStatus alsc_status;
Laurent Pinchart July 31, 2020, 9:42 p.m. UTC | #2 
On Sat, Aug 01, 2020 at 12:38:01AM +0300, Laurent Pinchart wrote:
> Hi David,
> 
> One more thing I'm afraid.
> 
> On Fri, Jul 31, 2020 at 03:08:01PM +0100, David Plowman wrote:
> > Now that we stop the asynchronous thread on a SwitchMode, we would do
> > better to regenerate all the tables if the new camera mode crops in a
> > significantly different way to the old one. A few minor tweaks make
> > sense along with this:
> > 
> > * Reset the lambda values when we reset everything. It wouldn't make
> >   sense to re-start with the old mode's values.
> > 
> > * Use the last recorded colour temperature to generate new tables rather
> >   than any default value.
> > 
> > * Set the frame "phase" counter to ensure the adaptive procedure will
> >   run asap.
> > 
> > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  src/ipa/raspberrypi/controller/rpi/alsc.cpp | 54 ++++++++++++++-------
> >  1 file changed, 37 insertions(+), 17 deletions(-)
> > 
> > diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.cpp b/src/ipa/raspberrypi/controller/rpi/alsc.cpp
> > index 4df9934..6e781bd 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/alsc.cpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/alsc.cpp
> > @@ -166,11 +166,8 @@ void Alsc::Initialise()
> >  	RPI_LOG("Alsc");
> >  	frame_count2_ = frame_count_ = frame_phase_ = 0;
> >  	first_time_ = true;
> > -	// Initialise the lambdas. Each call to Process then restarts from the
> > -	// previous results.  Also initialise the previous frame tables to the
> > -	// same harmless values.
> > -	for (int i = 0; i < XY; i++)
> > -		lambda_r_[i] = lambda_b_[i] = 1.0;
> > +	ct_ = config_.default_ct;
> > +	// The lambdas are initialised in the SwitchMode.
> >  }
> >  
> >  void Alsc::waitForAysncThread()
> > @@ -185,31 +182,53 @@ void Alsc::waitForAysncThread()
> >  	}
> >  }
> >  
> > +static bool compare_modes(CameraMode const &cm0, CameraMode const &cm1)
> > +{
> > +	// Return true if the modes crop from the sensor significantly differently.
> > +	int left_diff = std::abs(cm0.crop_x - cm1.crop_x);
> > +	int top_diff = std::abs(cm0.crop_y - cm1.crop_y);
> > +	int right_diff = std::abs(cm0.crop_x + cm0.scale_x * cm0.width -
> > +				  cm1.crop_x - cm1.scale_x * cm1.width);
> > +	int bottom_diff = std::abs(cm0.crop_y + cm0.scale_y * cm0.height -
> > +				   cm1.crop_y - cm1.scale_y * cm1.height);
> 
> gcc5 complains here:
> 
> ../../src/ipa/raspberrypi/controller/rpi/alsc.cpp: In function ‘bool compare_modes(const CameraMode&, const CameraMode&)’:
> ../../src/ipa/raspberrypi/controller/rpi/alsc.cpp:191:43: error: call of overloaded ‘abs(double)’ is ambiguous
>        cm1.crop_x - cm1.scale_x * cm1.width);
>                                            ^
> In file included from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/cstdlib:72:0,
>                  from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/ext/string_conversions.h:41,
>                  from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/bits/basic_string.h:5249,
>                  from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/string:52,
>                  from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/stdexcept:39,
>                  from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/array:38,
>                  from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/tuple:39,
>                  from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/mutex:38,
>                  from ../../src/ipa/raspberrypi/controller/rpi/alsc.hpp:9,
>                  from ../../src/ipa/raspberrypi/controller/rpi/alsc.cpp:10:
> /usr/include/stdlib.h:840:12: note: candidate: int abs(int)
>  extern int abs (int __x) __THROW __attribute__ ((__const__)) __wur;
>             ^
> In file included from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/ext/string_conversions.h:41:0,
>                  from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/bits/basic_string.h:5249,
>                  from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/string:52,
>                  from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/stdexcept:39,
>                  from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/array:38,
>                  from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/tuple:39,
>                  from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/mutex:38,
>                  from ../../src/ipa/raspberrypi/controller/rpi/alsc.hpp:9,
>                  from ../../src/ipa/raspberrypi/controller/rpi/alsc.cpp:10:
> /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/cstdlib:166:3: note: candidate: long int std::abs(long int)
>    abs(long __i) { return __builtin_labs(__i); }
>    ^
> /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/cstdlib:174:3: note: candidate: long long int std::abs(long long int)
>    abs(long long __x) { return __builtin_llabs (__x); }
>    ^
> ../../src/ipa/raspberrypi/controller/rpi/alsc.cpp:193:45: error: call of overloaded ‘abs(double)’ is ambiguous
>         cm1.crop_y - cm1.scale_y * cm1.height);
>                                              ^
> In file included from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/cstdlib:72:0,
>                  from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/ext/string_conversions.h:41,
>                  from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/bits/basic_string.h:5249,
>                  from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/string:52,
>                  from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/stdexcept:39,
>                  from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/array:38,
>                  from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/tuple:39,
>                  from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/mutex:38,
>                  from ../../src/ipa/raspberrypi/controller/rpi/alsc.hpp:9,
>                  from ../../src/ipa/raspberrypi/controller/rpi/alsc.cpp:10:
> /usr/include/stdlib.h:840:12: note: candidate: int abs(int)
>  extern int abs (int __x) __THROW __attribute__ ((__const__)) __wur;
>             ^
> In file included from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/ext/string_conversions.h:41:0,
>                  from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/bits/basic_string.h:5249,
>                  from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/string:52,
>                  from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/stdexcept:39,
>                  from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/array:38,
>                  from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/tuple:39,
>                  from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/mutex:38,
>                  from ../../src/ipa/raspberrypi/controller/rpi/alsc.hpp:9,
>                  from ../../src/ipa/raspberrypi/controller/rpi/alsc.cpp:10:
> /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/cstdlib:166:3: note: candidate: long int std::abs(long int)
>    abs(long __i) { return __builtin_labs(__i); }
>    ^
> /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/cstdlib:174:3: note: candidate: long long int std::abs(long long int)
>    abs(long long __x) { return __builtin_llabs (__x); }
>    ^
> 
> We may drop support for gcc 5 and 6 soon, but until we do, I would like
> to avoid breaking the build. Should these two lines use std::fabs() ?

Should be fabs() as this file includes math.h.

> > +	// These thresholds are a rather arbitrary amount chosen to trigger
> > +	// when carrying on with the previously calculated tables might be
> > +	// worse than regenerating them (but without the adaptive algorithm).
> > +	int threshold_x = cm0.sensor_width >> 4;
> > +	int threshold_y = cm0.sensor_height >> 4;
> > +	return left_diff > threshold_x || right_diff > threshold_x ||
> > +	       top_diff > threshold_y || bottom_diff > threshold_y;
> > +}
> > +
> >  void Alsc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)
> >  {
> >  	(void)metadata;
> >  
> > +	// We're going to start over with the tables if there's any "significant"
> > +	// change.
> > +	bool reset_tables = first_time_ || compare_modes(camera_mode_, camera_mode);
> > +
> >  	// 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. 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
> > -		// temperature. This echoes the code in doAlsc, without the
> > -		// adaptive algorithm.
> > +	if (reset_tables) {
> > +		// Upon every "table reset", arrange for something sensible to be
> > +		// generated. Construct the tables for the previous recorded colour
> > +		// temperature. In order to start over from scratch we initialise
> > +		// the lambdas, but the rest of this code then echoes the code in
> > +		// doAlsc, without the adaptive algorithm.
> > +		for (int i = 0; i < XY; i++)
> > +			lambda_r_[i] = lambda_b_[i] = 1.0;
> >  		double cal_table_r[XY], cal_table_b[XY], cal_table_tmp[XY];
> > -		get_cal_table(4000, config_.calibrations_Cr, cal_table_tmp);
> > +		get_cal_table(ct_, config_.calibrations_Cr, cal_table_tmp);
> >  		resample_cal_table(cal_table_tmp, camera_mode_, cal_table_r);
> > -		get_cal_table(4000, config_.calibrations_Cb, cal_table_tmp);
> > +		get_cal_table(ct_, config_.calibrations_Cb, cal_table_tmp);
> >  		resample_cal_table(cal_table_tmp, camera_mode_, cal_table_b);
> >  		compensate_lambdas_for_cal(cal_table_r, lambda_r_,
> >  					   async_lambda_r_);
> > @@ -220,6 +239,7 @@ void Alsc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)
> >  					config_.luminance_strength);
> >  		memcpy(prev_sync_results_, sync_results_,
> >  		       sizeof(prev_sync_results_));
> > +		frame_phase_ = config_.frame_period; // run the algo again asap
> >  		first_time_ = false;
> >  	}
> >  }
> > @@ -265,8 +285,8 @@ void Alsc::restartAsync(StatisticsPtr &stats, Metadata *image_metadata)
> >  {
> >  	RPI_LOG("Starting ALSC thread");
> >  	// Get the current colour temperature. It's all we need from the
> > -	// metadata.
> > -	ct_ = get_ct(image_metadata, config_.default_ct);
> > +	// metadata. Default to the last CT value (which could be the default).
> > +	ct_ = get_ct(image_metadata, ct_);
> >  	// We have to copy the statistics here, dividing out our best guess of
> >  	// the LSC table that the pipeline applied to them.
> >  	AlscStatus alsc_status;

Patch
diff mbox series 

diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.cpp b/src/ipa/raspberrypi/controller/rpi/alsc.cpp
index 4df9934..6e781bd 100644
--- a/src/ipa/raspberrypi/controller/rpi/alsc.cpp
+++ b/src/ipa/raspberrypi/controller/rpi/alsc.cpp
@@ -166,11 +166,8 @@  void Alsc::Initialise()
 	RPI_LOG("Alsc");
 	frame_count2_ = frame_count_ = frame_phase_ = 0;
 	first_time_ = true;
-	// Initialise the lambdas. Each call to Process then restarts from the
-	// previous results.  Also initialise the previous frame tables to the
-	// same harmless values.
-	for (int i = 0; i < XY; i++)
-		lambda_r_[i] = lambda_b_[i] = 1.0;
+	ct_ = config_.default_ct;
+	// The lambdas are initialised in the SwitchMode.
 }
 
 void Alsc::waitForAysncThread()
@@ -185,31 +182,53 @@  void Alsc::waitForAysncThread()
 	}
 }
 
+static bool compare_modes(CameraMode const &cm0, CameraMode const &cm1)
+{
+	// Return true if the modes crop from the sensor significantly differently.
+	int left_diff = std::abs(cm0.crop_x - cm1.crop_x);
+	int top_diff = std::abs(cm0.crop_y - cm1.crop_y);
+	int right_diff = std::abs(cm0.crop_x + cm0.scale_x * cm0.width -
+				  cm1.crop_x - cm1.scale_x * cm1.width);
+	int bottom_diff = std::abs(cm0.crop_y + cm0.scale_y * cm0.height -
+				   cm1.crop_y - cm1.scale_y * cm1.height);
+	// These thresholds are a rather arbitrary amount chosen to trigger
+	// when carrying on with the previously calculated tables might be
+	// worse than regenerating them (but without the adaptive algorithm).
+	int threshold_x = cm0.sensor_width >> 4;
+	int threshold_y = cm0.sensor_height >> 4;
+	return left_diff > threshold_x || right_diff > threshold_x ||
+	       top_diff > threshold_y || bottom_diff > threshold_y;
+}
+
 void Alsc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)
 {
 	(void)metadata;
 
+	// We're going to start over with the tables if there's any "significant"
+	// change.
+	bool reset_tables = first_time_ || compare_modes(camera_mode_, camera_mode);
+
 	// 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. 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
-		// temperature. This echoes the code in doAlsc, without the
-		// adaptive algorithm.
+	if (reset_tables) {
+		// Upon every "table reset", arrange for something sensible to be
+		// generated. Construct the tables for the previous recorded colour
+		// temperature. In order to start over from scratch we initialise
+		// the lambdas, but the rest of this code then echoes the code in
+		// doAlsc, without the adaptive algorithm.
+		for (int i = 0; i < XY; i++)
+			lambda_r_[i] = lambda_b_[i] = 1.0;
 		double cal_table_r[XY], cal_table_b[XY], cal_table_tmp[XY];
-		get_cal_table(4000, config_.calibrations_Cr, cal_table_tmp);
+		get_cal_table(ct_, config_.calibrations_Cr, cal_table_tmp);
 		resample_cal_table(cal_table_tmp, camera_mode_, cal_table_r);
-		get_cal_table(4000, config_.calibrations_Cb, cal_table_tmp);
+		get_cal_table(ct_, config_.calibrations_Cb, cal_table_tmp);
 		resample_cal_table(cal_table_tmp, camera_mode_, cal_table_b);
 		compensate_lambdas_for_cal(cal_table_r, lambda_r_,
 					   async_lambda_r_);
@@ -220,6 +239,7 @@  void Alsc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)
 					config_.luminance_strength);
 		memcpy(prev_sync_results_, sync_results_,
 		       sizeof(prev_sync_results_));
+		frame_phase_ = config_.frame_period; // run the algo again asap
 		first_time_ = false;
 	}
 }
@@ -265,8 +285,8 @@  void Alsc::restartAsync(StatisticsPtr &stats, Metadata *image_metadata)
 {
 	RPI_LOG("Starting ALSC thread");
 	// Get the current colour temperature. It's all we need from the
-	// metadata.
-	ct_ = get_ct(image_metadata, config_.default_ct);
+	// metadata. Default to the last CT value (which could be the default).
+	ct_ = get_ct(image_metadata, ct_);
 	// We have to copy the statistics here, dividing out our best guess of
 	// the LSC table that the pipeline applied to them.
 	AlscStatus alsc_status;