[libcamera-devel] [PATCH v3 4/4] libcamera: ipa: raspberrypi: ALSC: Improve behaviour when camera mode changes

[Naushir Patuck]

Hi David,

On Sat, 1 Aug 2020 at 09:02, David Plowman
<david.plowman at 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 at raspberrypi.com>

Reviewed-by: Naushir Patuck <naush at 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 at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel