| Message ID | 20201207180121.6374-6-david.plowman@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi David, Thank you for your patch. On Mon, 7 Dec 2020 at 18:02, David Plowman <david.plowman@raspberrypi.com> wrote: > When the AWB is started from "cold" with fixed colour gains, we try to > estimate the colour temperature this corresponds to (if a calibrated > CT curve was supplied). When fixed colour gains are set after the AWB > has been running, we leave the CT estimate alone, as the one we have > is probably sensible. > > This estimated colour is passed out in the metadata for other > algorithms - notably ALSC - to use. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > --- > src/ipa/raspberrypi/controller/rpi/alsc.cpp | 6 +++++- > src/ipa/raspberrypi/controller/rpi/awb.cpp | 14 ++++++++++++++ > src/ipa/raspberrypi/controller/rpi/awb.hpp | 1 + > 3 files changed, 20 insertions(+), 1 deletion(-) > > diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.cpp > b/src/ipa/raspberrypi/controller/rpi/alsc.cpp > index 183a0c95..c354c985 100644 > --- a/src/ipa/raspberrypi/controller/rpi/alsc.cpp > +++ b/src/ipa/raspberrypi/controller/rpi/alsc.cpp > @@ -146,6 +146,7 @@ void Alsc::Read(boost::property_tree::ptree const > ¶ms) > config_.threshold = params.get<double>("threshold", 1e-3); > } > > +static double get_ct(Metadata *metadata, double default_ct); > static void get_cal_table(double ct, > std::vector<AlscCalibration> const &calibrations, > double cal_table[XY]); > @@ -210,6 +211,9 @@ void Alsc::SwitchMode(CameraMode const &camera_mode, > // change. > bool reset_tables = first_time_ || compare_modes(camera_mode_, > camera_mode); > > + // Believe the colour temperature from the AWB, if there is one. > + ct_ = get_ct(metadata, ct_); > + > // Ensure the other thread isn't running while we do this. > waitForAysncThread(); > > @@ -254,7 +258,7 @@ void Alsc::fetchAsyncResults() > memcpy(sync_results_, async_results_, sizeof(sync_results_)); > } > > -static double get_ct(Metadata *metadata, double default_ct) > +double get_ct(Metadata *metadata, double default_ct) > { > AwbStatus awb_status; > awb_status.temperature_K = default_ct; // in case nothing found > diff --git a/src/ipa/raspberrypi/controller/rpi/awb.cpp > b/src/ipa/raspberrypi/controller/rpi/awb.cpp > index 6b359ac5..2266c07b 100644 > --- a/src/ipa/raspberrypi/controller/rpi/awb.cpp > +++ b/src/ipa/raspberrypi/controller/rpi/awb.cpp > @@ -19,6 +19,9 @@ using namespace RPiController; > > const double Awb::RGB::INVALID = -1.0; > > +// todo - the locking in this algorithm needs some tidying up as has been > done > +// elsewhere (ALSC and AGC). > + > void AwbMode::Read(boost::property_tree::ptree const ¶ms) > { > ct_lo = params.get<double>("lo"); > @@ -121,6 +124,7 @@ Awb::Awb(Controller *controller) > async_abort_ = async_start_ = async_started_ = async_finished_ = > false; > mode_ = nullptr; > manual_r_ = manual_b_ = 0.0; > + first_switch_mode_ = true; > async_thread_ = std::thread(std::bind(&Awb::asyncFunc, this)); > } > > @@ -201,9 +205,19 @@ void Awb::SwitchMode([[maybe_unused]] CameraMode > const &camera_mode, > prev_sync_results_.gain_r = manual_r_; > prev_sync_results_.gain_g = 1.0; > prev_sync_results_.gain_b = manual_b_; > + // If we're starting up for the first time, try and > + // "dead reckon" the corresponding colour temperature. > + if (first_switch_mode_ && config_.bayes) { > + Pwl ct_r_inverse = > std::move(config_.ct_r.Inverse()); > + Pwl ct_b_inverse = > std::move(config_.ct_b.Inverse()); > Not a review comment just for my curiosity - will std::move do anything here? Would C++ Return Value Optimisation ensure that we don't make any extra copies without the std::move? Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > + double ct_r = > ct_r_inverse.Eval(ct_r_inverse.Domain().Clip(1 / manual_r_)); > + double ct_b = > ct_b_inverse.Eval(ct_b_inverse.Domain().Clip(1 / manual_b_)); > + prev_sync_results_.temperature_K = (ct_r + ct_b) / > 2; > + } > sync_results_ = prev_sync_results_; > } > metadata->Set("awb.status", prev_sync_results_); > + first_switch_mode_ = false; > } > > void Awb::fetchAsyncResults() > diff --git a/src/ipa/raspberrypi/controller/rpi/awb.hpp > b/src/ipa/raspberrypi/controller/rpi/awb.hpp > index d86b9598..8525af32 100644 > --- a/src/ipa/raspberrypi/controller/rpi/awb.hpp > +++ b/src/ipa/raspberrypi/controller/rpi/awb.hpp > @@ -159,6 +159,7 @@ private: > double manual_r_; > // manual b setting > double manual_b_; > + bool first_switch_mode_; // is this the first call to SwitchMode? > }; > > static inline Awb::RGB operator+(Awb::RGB const &a, Awb::RGB const &b) > -- > 2.20.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel >
Hi David, Thank you for the patch. On Mon, Dec 07, 2020 at 06:01:20PM +0000, David Plowman wrote: > When the AWB is started from "cold" with fixed colour gains, we try to > estimate the colour temperature this corresponds to (if a calibrated > CT curve was supplied). When fixed colour gains are set after the AWB > has been running, we leave the CT estimate alone, as the one we have > is probably sensible. > > This estimated colour is passed out in the metadata for other > algorithms - notably ALSC - to use. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > --- > src/ipa/raspberrypi/controller/rpi/alsc.cpp | 6 +++++- > src/ipa/raspberrypi/controller/rpi/awb.cpp | 14 ++++++++++++++ > src/ipa/raspberrypi/controller/rpi/awb.hpp | 1 + > 3 files changed, 20 insertions(+), 1 deletion(-) > > diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.cpp b/src/ipa/raspberrypi/controller/rpi/alsc.cpp > index 183a0c95..c354c985 100644 > --- a/src/ipa/raspberrypi/controller/rpi/alsc.cpp > +++ b/src/ipa/raspberrypi/controller/rpi/alsc.cpp > @@ -146,6 +146,7 @@ void Alsc::Read(boost::property_tree::ptree const ¶ms) > config_.threshold = params.get<double>("threshold", 1e-3); > } > > +static double get_ct(Metadata *metadata, double default_ct); > static void get_cal_table(double ct, > std::vector<AlscCalibration> const &calibrations, > double cal_table[XY]); > @@ -210,6 +211,9 @@ void Alsc::SwitchMode(CameraMode const &camera_mode, > // change. > bool reset_tables = first_time_ || compare_modes(camera_mode_, camera_mode); > > + // Believe the colour temperature from the AWB, if there is one. > + ct_ = get_ct(metadata, ct_); > + > // Ensure the other thread isn't running while we do this. > waitForAysncThread(); > > @@ -254,7 +258,7 @@ void Alsc::fetchAsyncResults() > memcpy(sync_results_, async_results_, sizeof(sync_results_)); > } > > -static double get_ct(Metadata *metadata, double default_ct) > +double get_ct(Metadata *metadata, double default_ct) Is this needed, does the compiler complain otherwise ? And on a side note, we could also move the function up to avoid forward declarations, but that doesn't have to be part of this patch. > { > AwbStatus awb_status; > awb_status.temperature_K = default_ct; // in case nothing found > diff --git a/src/ipa/raspberrypi/controller/rpi/awb.cpp b/src/ipa/raspberrypi/controller/rpi/awb.cpp > index 6b359ac5..2266c07b 100644 > --- a/src/ipa/raspberrypi/controller/rpi/awb.cpp > +++ b/src/ipa/raspberrypi/controller/rpi/awb.cpp > @@ -19,6 +19,9 @@ using namespace RPiController; > > const double Awb::RGB::INVALID = -1.0; > > +// todo - the locking in this algorithm needs some tidying up as has been done > +// elsewhere (ALSC and AGC). > + > void AwbMode::Read(boost::property_tree::ptree const ¶ms) > { > ct_lo = params.get<double>("lo"); > @@ -121,6 +124,7 @@ Awb::Awb(Controller *controller) > async_abort_ = async_start_ = async_started_ = async_finished_ = false; > mode_ = nullptr; > manual_r_ = manual_b_ = 0.0; > + first_switch_mode_ = true; > async_thread_ = std::thread(std::bind(&Awb::asyncFunc, this)); > } > > @@ -201,9 +205,19 @@ void Awb::SwitchMode([[maybe_unused]] CameraMode const &camera_mode, > prev_sync_results_.gain_r = manual_r_; > prev_sync_results_.gain_g = 1.0; > prev_sync_results_.gain_b = manual_b_; > + // If we're starting up for the first time, try and > + // "dead reckon" the corresponding colour temperature. > + if (first_switch_mode_ && config_.bayes) { > + Pwl ct_r_inverse = std::move(config_.ct_r.Inverse()); > + Pwl ct_b_inverse = std::move(config_.ct_b.Inverse()); I think you can drop std::move(), as Naush mentioned return value optimization will take care of this (and std::move() can actually prevent RVO in some cases). Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + double ct_r = ct_r_inverse.Eval(ct_r_inverse.Domain().Clip(1 / manual_r_)); > + double ct_b = ct_b_inverse.Eval(ct_b_inverse.Domain().Clip(1 / manual_b_)); > + prev_sync_results_.temperature_K = (ct_r + ct_b) / 2; > + } > sync_results_ = prev_sync_results_; > } > metadata->Set("awb.status", prev_sync_results_); > + first_switch_mode_ = false; > } > > void Awb::fetchAsyncResults() > diff --git a/src/ipa/raspberrypi/controller/rpi/awb.hpp b/src/ipa/raspberrypi/controller/rpi/awb.hpp > index d86b9598..8525af32 100644 > --- a/src/ipa/raspberrypi/controller/rpi/awb.hpp > +++ b/src/ipa/raspberrypi/controller/rpi/awb.hpp > @@ -159,6 +159,7 @@ private: > double manual_r_; > // manual b setting > double manual_b_; > + bool first_switch_mode_; // is this the first call to SwitchMode? > }; > > static inline Awb::RGB operator+(Awb::RGB const &a, Awb::RGB const &b)
Hi David, On Tue, Dec 08, 2020 at 01:59:18PM +0200, Laurent Pinchart wrote: > On Mon, Dec 07, 2020 at 06:01:20PM +0000, David Plowman wrote: > > When the AWB is started from "cold" with fixed colour gains, we try to > > estimate the colour temperature this corresponds to (if a calibrated > > CT curve was supplied). When fixed colour gains are set after the AWB > > has been running, we leave the CT estimate alone, as the one we have > > is probably sensible. > > > > This estimated colour is passed out in the metadata for other > > algorithms - notably ALSC - to use. > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > --- > > src/ipa/raspberrypi/controller/rpi/alsc.cpp | 6 +++++- > > src/ipa/raspberrypi/controller/rpi/awb.cpp | 14 ++++++++++++++ > > src/ipa/raspberrypi/controller/rpi/awb.hpp | 1 + > > 3 files changed, 20 insertions(+), 1 deletion(-) > > > > diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.cpp b/src/ipa/raspberrypi/controller/rpi/alsc.cpp > > index 183a0c95..c354c985 100644 > > --- a/src/ipa/raspberrypi/controller/rpi/alsc.cpp > > +++ b/src/ipa/raspberrypi/controller/rpi/alsc.cpp > > @@ -146,6 +146,7 @@ void Alsc::Read(boost::property_tree::ptree const ¶ms) > > config_.threshold = params.get<double>("threshold", 1e-3); > > } > > > > +static double get_ct(Metadata *metadata, double default_ct); > > static void get_cal_table(double ct, > > std::vector<AlscCalibration> const &calibrations, > > double cal_table[XY]); > > @@ -210,6 +211,9 @@ void Alsc::SwitchMode(CameraMode const &camera_mode, > > // change. > > bool reset_tables = first_time_ || compare_modes(camera_mode_, camera_mode); > > > > + // Believe the colour temperature from the AWB, if there is one. > > + ct_ = get_ct(metadata, ct_); > > + > > // Ensure the other thread isn't running while we do this. > > waitForAysncThread(); > > > > @@ -254,7 +258,7 @@ void Alsc::fetchAsyncResults() > > memcpy(sync_results_, async_results_, sizeof(sync_results_)); > > } > > > > -static double get_ct(Metadata *metadata, double default_ct) > > +double get_ct(Metadata *metadata, double default_ct) > > Is this needed, does the compiler complain otherwise ? And on a side > note, we could also move the function up to avoid forward declarations, > but that doesn't have to be part of this patch. > > > { > > AwbStatus awb_status; > > awb_status.temperature_K = default_ct; // in case nothing found > > diff --git a/src/ipa/raspberrypi/controller/rpi/awb.cpp b/src/ipa/raspberrypi/controller/rpi/awb.cpp > > index 6b359ac5..2266c07b 100644 > > --- a/src/ipa/raspberrypi/controller/rpi/awb.cpp > > +++ b/src/ipa/raspberrypi/controller/rpi/awb.cpp > > @@ -19,6 +19,9 @@ using namespace RPiController; > > > > const double Awb::RGB::INVALID = -1.0; > > > > +// todo - the locking in this algorithm needs some tidying up as has been done > > +// elsewhere (ALSC and AGC). > > + > > void AwbMode::Read(boost::property_tree::ptree const ¶ms) > > { > > ct_lo = params.get<double>("lo"); > > @@ -121,6 +124,7 @@ Awb::Awb(Controller *controller) > > async_abort_ = async_start_ = async_started_ = async_finished_ = false; > > mode_ = nullptr; > > manual_r_ = manual_b_ = 0.0; > > + first_switch_mode_ = true; > > async_thread_ = std::thread(std::bind(&Awb::asyncFunc, this)); > > } > > > > @@ -201,9 +205,19 @@ void Awb::SwitchMode([[maybe_unused]] CameraMode const &camera_mode, > > prev_sync_results_.gain_r = manual_r_; > > prev_sync_results_.gain_g = 1.0; > > prev_sync_results_.gain_b = manual_b_; > > + // If we're starting up for the first time, try and > > + // "dead reckon" the corresponding colour temperature. > > + if (first_switch_mode_ && config_.bayes) { > > + Pwl ct_r_inverse = std::move(config_.ct_r.Inverse()); > > + Pwl ct_b_inverse = std::move(config_.ct_b.Inverse()); > > I think you can drop std::move(), as Naush mentioned return value > optimization will take care of this (and std::move() can actually > prevent RVO in some cases). clang even warns about this: ../../src/ipa/raspberrypi/controller/rpi/awb.cpp:211:23: error: moving a temporary object prevents copy elision [-Werror,-Wpessimizing-move] Pwl ct_r_inverse = std::move(config_.ct_r.Inverse()); > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > + double ct_r = ct_r_inverse.Eval(ct_r_inverse.Domain().Clip(1 / manual_r_)); > > + double ct_b = ct_b_inverse.Eval(ct_b_inverse.Domain().Clip(1 / manual_b_)); > > + prev_sync_results_.temperature_K = (ct_r + ct_b) / 2; > > + } > > sync_results_ = prev_sync_results_; > > } > > metadata->Set("awb.status", prev_sync_results_); > > + first_switch_mode_ = false; > > } > > > > void Awb::fetchAsyncResults() > > diff --git a/src/ipa/raspberrypi/controller/rpi/awb.hpp b/src/ipa/raspberrypi/controller/rpi/awb.hpp > > index d86b9598..8525af32 100644 > > --- a/src/ipa/raspberrypi/controller/rpi/awb.hpp > > +++ b/src/ipa/raspberrypi/controller/rpi/awb.hpp > > @@ -159,6 +159,7 @@ private: > > double manual_r_; > > // manual b setting > > double manual_b_; > > + bool first_switch_mode_; // is this the first call to SwitchMode? > > }; > > > > static inline Awb::RGB operator+(Awb::RGB const &a, Awb::RGB const &b)
diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.cpp b/src/ipa/raspberrypi/controller/rpi/alsc.cpp index 183a0c95..c354c985 100644 --- a/src/ipa/raspberrypi/controller/rpi/alsc.cpp +++ b/src/ipa/raspberrypi/controller/rpi/alsc.cpp @@ -146,6 +146,7 @@ void Alsc::Read(boost::property_tree::ptree const ¶ms) config_.threshold = params.get<double>("threshold", 1e-3); } +static double get_ct(Metadata *metadata, double default_ct); static void get_cal_table(double ct, std::vector<AlscCalibration> const &calibrations, double cal_table[XY]); @@ -210,6 +211,9 @@ void Alsc::SwitchMode(CameraMode const &camera_mode, // change. bool reset_tables = first_time_ || compare_modes(camera_mode_, camera_mode); + // Believe the colour temperature from the AWB, if there is one. + ct_ = get_ct(metadata, ct_); + // Ensure the other thread isn't running while we do this. waitForAysncThread(); @@ -254,7 +258,7 @@ void Alsc::fetchAsyncResults() memcpy(sync_results_, async_results_, sizeof(sync_results_)); } -static double get_ct(Metadata *metadata, double default_ct) +double get_ct(Metadata *metadata, double default_ct) { AwbStatus awb_status; awb_status.temperature_K = default_ct; // in case nothing found diff --git a/src/ipa/raspberrypi/controller/rpi/awb.cpp b/src/ipa/raspberrypi/controller/rpi/awb.cpp index 6b359ac5..2266c07b 100644 --- a/src/ipa/raspberrypi/controller/rpi/awb.cpp +++ b/src/ipa/raspberrypi/controller/rpi/awb.cpp @@ -19,6 +19,9 @@ using namespace RPiController; const double Awb::RGB::INVALID = -1.0; +// todo - the locking in this algorithm needs some tidying up as has been done +// elsewhere (ALSC and AGC). + void AwbMode::Read(boost::property_tree::ptree const ¶ms) { ct_lo = params.get<double>("lo"); @@ -121,6 +124,7 @@ Awb::Awb(Controller *controller) async_abort_ = async_start_ = async_started_ = async_finished_ = false; mode_ = nullptr; manual_r_ = manual_b_ = 0.0; + first_switch_mode_ = true; async_thread_ = std::thread(std::bind(&Awb::asyncFunc, this)); } @@ -201,9 +205,19 @@ void Awb::SwitchMode([[maybe_unused]] CameraMode const &camera_mode, prev_sync_results_.gain_r = manual_r_; prev_sync_results_.gain_g = 1.0; prev_sync_results_.gain_b = manual_b_; + // If we're starting up for the first time, try and + // "dead reckon" the corresponding colour temperature. + if (first_switch_mode_ && config_.bayes) { + Pwl ct_r_inverse = std::move(config_.ct_r.Inverse()); + Pwl ct_b_inverse = std::move(config_.ct_b.Inverse()); + double ct_r = ct_r_inverse.Eval(ct_r_inverse.Domain().Clip(1 / manual_r_)); + double ct_b = ct_b_inverse.Eval(ct_b_inverse.Domain().Clip(1 / manual_b_)); + prev_sync_results_.temperature_K = (ct_r + ct_b) / 2; + } sync_results_ = prev_sync_results_; } metadata->Set("awb.status", prev_sync_results_); + first_switch_mode_ = false; } void Awb::fetchAsyncResults() diff --git a/src/ipa/raspberrypi/controller/rpi/awb.hpp b/src/ipa/raspberrypi/controller/rpi/awb.hpp index d86b9598..8525af32 100644 --- a/src/ipa/raspberrypi/controller/rpi/awb.hpp +++ b/src/ipa/raspberrypi/controller/rpi/awb.hpp @@ -159,6 +159,7 @@ private: double manual_r_; // manual b setting double manual_b_; + bool first_switch_mode_; // is this the first call to SwitchMode? }; static inline Awb::RGB operator+(Awb::RGB const &a, Awb::RGB const &b)
When the AWB is started from "cold" with fixed colour gains, we try to estimate the colour temperature this corresponds to (if a calibrated CT curve was supplied). When fixed colour gains are set after the AWB has been running, we leave the CT estimate alone, as the one we have is probably sensible. This estimated colour is passed out in the metadata for other algorithms - notably ALSC - to use. Signed-off-by: David Plowman <david.plowman@raspberrypi.com> --- src/ipa/raspberrypi/controller/rpi/alsc.cpp | 6 +++++- src/ipa/raspberrypi/controller/rpi/awb.cpp | 14 ++++++++++++++ src/ipa/raspberrypi/controller/rpi/awb.hpp | 1 + 3 files changed, 20 insertions(+), 1 deletion(-)