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

[David Plowman]

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 at 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 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);
>
> 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