| Message ID | 20200730111134.641-4-david.plowman@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi David, (CC'ing Naush) Thank you for the patch. On Thu, Jul 30, 2020 at 12:11:34PM +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> The patch looks fine to me. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> I would appreciate a review from Naush for the parts that deal with the algorithm itself. One question for my general education, why is the algorithm using the average scene temperature ? Is that because it is an indicator of the average frequency contents of the scene lightning, and because the lens' shading response is frequency-dependent ? > --- > 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 8f7720b..1e70637 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;
diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.cpp b/src/ipa/raspberrypi/controller/rpi/alsc.cpp index 8f7720b..1e70637 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;
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(-)