[libcamera-devel] [PATCH v2 4/4] libcamera: ipa: raspberrypi: ALSC: Improve behaviour when camera mode changes
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Jul 31 23:42:30 CEST 2020
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 at raspberrypi.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at 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;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list