| Message ID | 20200801080151.4282-5-david.plowman@raspberrypi.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi David, Thank you for the patch. On Sat, Aug 01, 2020 at 09:01:51AM +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> > --- > 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..5e55954 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 = fabs(cm0.crop_x - cm1.crop_x); > + int top_diff = fabs(cm0.crop_y - cm1.crop_y); Note that these two could still use abs(), it's only the two lines below that are given a double argument. It doesn't matter much though, and if you think abs() is better than fabs() here, I can fix when applying, no need to send a new version. > + int right_diff = fabs(cm0.crop_x + cm0.scale_x * cm0.width - > + cm1.crop_x - cm1.scale_x * cm1.width); > + int bottom_diff = fabs(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;
Ah. I think maybe abs is better than fabs for ints, no point turning everything into floats for no reason, but don't particularly mind. But thanks for taking care of this! Best regards David On Sat, 1 Aug 2020 at 13:34, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi David, > > Thank you for the patch. > > On Sat, Aug 01, 2020 at 09:01:51AM +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> > > --- > > 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..5e55954 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 = fabs(cm0.crop_x - cm1.crop_x); > > + int top_diff = fabs(cm0.crop_y - cm1.crop_y); > > Note that these two could still use abs(), it's only the two lines below > that are given a double argument. It doesn't matter much though, and if > you think abs() is better than fabs() here, I can fix when applying, no > need to send a new version. > > > + int right_diff = fabs(cm0.crop_x + cm0.scale_x * cm0.width - > > + cm1.crop_x - cm1.scale_x * cm1.width); > > + int bottom_diff = fabs(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; > > -- > Regards, > > Laurent Pinchart
Hi David, Thank you for the patch. On Sat, 1 Aug 2020 at 09:02, David Plowman <david.plowman@raspberrypi.com> 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> All looks good to me. Apart from the fabs -> abs change: Reviewed-by: Naushir Patuck <naush@raspberrypi.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..5e55954 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 = fabs(cm0.crop_x - cm1.crop_x); > + int top_diff = fabs(cm0.crop_y - cm1.crop_y); > + int right_diff = fabs(cm0.crop_x + cm0.scale_x * cm0.width - > + cm1.crop_x - cm1.scale_x * cm1.width); > + int bottom_diff = fabs(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; > -- > 2.20.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi David, On Sat, 1 Aug 2020 at 09:02, David Plowman <david.plowman@raspberrypi.com> 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: Naushir Patuck <naush@raspberrypi.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..5e55954 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 = fabs(cm0.crop_x - cm1.crop_x); > + int top_diff = fabs(cm0.crop_y - cm1.crop_y); > + int right_diff = fabs(cm0.crop_x + cm0.scale_x * cm0.width - > + cm1.crop_x - cm1.scale_x * cm1.width); > + int bottom_diff = fabs(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; > -- > 2.20.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Naush, On Wed, Aug 05, 2020 at 07:51:54AM +0100, Naushir Patuck wrote: > On Sat, 1 Aug 2020 at 09:02, 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: Naushir Patuck <naush@raspberrypi.com> Thank you. I've pushed the series to the master branch. > > --- > > 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..5e55954 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 = fabs(cm0.crop_x - cm1.crop_x); > > + int top_diff = fabs(cm0.crop_y - cm1.crop_y); > > + int right_diff = fabs(cm0.crop_x + cm0.scale_x * cm0.width - > > + cm1.crop_x - cm1.scale_x * cm1.width); > > + int bottom_diff = fabs(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;
diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.cpp b/src/ipa/raspberrypi/controller/rpi/alsc.cpp index 4df9934..5e55954 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 = fabs(cm0.crop_x - cm1.crop_x); + int top_diff = fabs(cm0.crop_y - cm1.crop_y); + int right_diff = fabs(cm0.crop_x + cm0.scale_x * cm0.width - + cm1.crop_x - cm1.scale_x * cm1.width); + int bottom_diff = fabs(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;
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> --- src/ipa/raspberrypi/controller/rpi/alsc.cpp | 54 ++++++++++++++------- 1 file changed, 37 insertions(+), 17 deletions(-)