| Message ID | 20231110161433.413382-1-david.plowman@raspberrypi.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series |
|
| Related | show |
Quoting David Plowman via libcamera-devel (2023-11-10 16:14:33) > ColourTemperature is now exported as a writable control so that > algorithms can set it directly. The AWB algorithm class now requires a > method to be provided to perform this operation. The method should > clamp the passed value to the calibrated range known to the algorithm. > > The default range is set very wide to cover all conceivable future AWB > calibrations. It will always be clamped before use. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > --- > src/ipa/rpi/common/ipa_base.cpp | 20 ++++++++++++++++++++ > src/ipa/rpi/controller/awb_algorithm.h | 1 + > src/ipa/rpi/controller/rpi/awb.cpp | 18 ++++++++++++++++++ > src/ipa/rpi/controller/rpi/awb.h | 1 + > 4 files changed, 40 insertions(+) > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp > index 97a32522..cd1d6e3d 100644 > --- a/src/ipa/rpi/common/ipa_base.cpp > +++ b/src/ipa/rpi/common/ipa_base.cpp > @@ -80,6 +80,7 @@ const ControlInfoMap::Map ipaColourControls{ > { &controls::AwbEnable, ControlInfo(false, true) }, > { &controls::AwbMode, ControlInfo(controls::AwbModeValues) }, > { &controls::ColourGains, ControlInfo(0.0f, 32.0f) }, > + { &controls::ColourTemperature, ControlInfo(100, 100000) }, > { &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) }, > }; > > @@ -972,6 +973,25 @@ void IpaBase::applyControls(const ControlList &controls) > break; > } > > + case controls::COLOUR_TEMPERATURE: { > + /* Silently ignore this control for a mono sensor. */ > + if (monoSensor_) > + break; I wonder if this is where the construction of the control info map should be handled / supported by the algorithms so they can populate it correctly / dynamically based on what they can handle. But that's not for this patch - and I suspect this is ok. > + > + auto temperatureK = ctrl.second.get<int32_t>(); > + RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>( > + controller_.getAlgorithm("awb")); > + if (!awb) { > + LOG(IPARPI, Warning) > + << "Could not set COLOUR_TEMPERATURE - no AWB algorithm"; > + break; I guess it would also prevent exposing a control when there's no algorithm to implement/support it though. Still - for now this seems like all that could be done here then. But otherwise Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > + } > + > + awb->setColourTemperature(temperatureK); > + /* This metadata will get reported back automatically. */ > + break; > + } > + > case controls::BRIGHTNESS: { > RPiController::ContrastAlgorithm *contrast = dynamic_cast<RPiController::ContrastAlgorithm *>( > controller_.getAlgorithm("contrast")); > diff --git a/src/ipa/rpi/controller/awb_algorithm.h b/src/ipa/rpi/controller/awb_algorithm.h > index 8462c4db..d966dfa8 100644 > --- a/src/ipa/rpi/controller/awb_algorithm.h > +++ b/src/ipa/rpi/controller/awb_algorithm.h > @@ -18,6 +18,7 @@ public: > virtual unsigned int getConvergenceFrames() const = 0; > virtual void setMode(std::string const &modeName) = 0; > virtual void setManualGains(double manualR, double manualB) = 0; > + virtual void setColourTemperature(double temperatureK) = 0; > virtual void enableAuto() = 0; > virtual void disableAuto() = 0; > }; > diff --git a/src/ipa/rpi/controller/rpi/awb.cpp b/src/ipa/rpi/controller/rpi/awb.cpp > index 5ae0c2fa..0918e20d 100644 > --- a/src/ipa/rpi/controller/rpi/awb.cpp > +++ b/src/ipa/rpi/controller/rpi/awb.cpp > @@ -275,6 +275,24 @@ void Awb::setManualGains(double manualR, double manualB) > } > } > > +void Awb::setColourTemperature(double temperatureK) > +{ > + if (!config_.bayes) { > + LOG(RPiAwb, Warning) << "AWB uncalibrated - cannot set colour temperature"; > + return; > + } > + > + temperatureK = config_.ctR.domain().clip(temperatureK); > + manualR_ = 1 / config_.ctR.eval(temperatureK); > + manualB_ = 1 / config_.ctB.eval(temperatureK); > + > + syncResults_.temperatureK = temperatureK; > + syncResults_.gainR = manualR_; > + syncResults_.gainG = 1.0; > + syncResults_.gainB = manualB_; > + prevSyncResults_ = syncResults_; > +} > + > void Awb::switchMode([[maybe_unused]] CameraMode const &cameraMode, > Metadata *metadata) > { > diff --git a/src/ipa/rpi/controller/rpi/awb.h b/src/ipa/rpi/controller/rpi/awb.h > index e7d49cd8..dbd79eda 100644 > --- a/src/ipa/rpi/controller/rpi/awb.h > +++ b/src/ipa/rpi/controller/rpi/awb.h > @@ -97,6 +97,7 @@ public: > unsigned int getConvergenceFrames() const override; > void setMode(std::string const &name) override; > void setManualGains(double manualR, double manualB) override; > + void setColourTemperature(double temperatureK) override; > void enableAuto() override; > void disableAuto() override; > void switchMode(CameraMode const &cameraMode, Metadata *metadata) override; > -- > 2.39.2 >
Hi Naush, Are you happy with this patch? Quoting Kieran Bingham (2023-11-20 14:57:49) > Quoting David Plowman via libcamera-devel (2023-11-10 16:14:33) > > ColourTemperature is now exported as a writable control so that > > algorithms can set it directly. The AWB algorithm class now requires a > > method to be provided to perform this operation. The method should > > clamp the passed value to the calibrated range known to the algorithm. > > > > The default range is set very wide to cover all conceivable future AWB > > calibrations. It will always be clamped before use. > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > --- > > src/ipa/rpi/common/ipa_base.cpp | 20 ++++++++++++++++++++ > > src/ipa/rpi/controller/awb_algorithm.h | 1 + > > src/ipa/rpi/controller/rpi/awb.cpp | 18 ++++++++++++++++++ > > src/ipa/rpi/controller/rpi/awb.h | 1 + > > 4 files changed, 40 insertions(+) > > > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp > > index 97a32522..cd1d6e3d 100644 > > --- a/src/ipa/rpi/common/ipa_base.cpp > > +++ b/src/ipa/rpi/common/ipa_base.cpp > > @@ -80,6 +80,7 @@ const ControlInfoMap::Map ipaColourControls{ > > { &controls::AwbEnable, ControlInfo(false, true) }, > > { &controls::AwbMode, ControlInfo(controls::AwbModeValues) }, > > { &controls::ColourGains, ControlInfo(0.0f, 32.0f) }, > > + { &controls::ColourTemperature, ControlInfo(100, 100000) }, > > { &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) }, > > }; > > > > @@ -972,6 +973,25 @@ void IpaBase::applyControls(const ControlList &controls) > > break; > > } > > > > + case controls::COLOUR_TEMPERATURE: { > > + /* Silently ignore this control for a mono sensor. */ > > + if (monoSensor_) > > + break; > > I wonder if this is where the construction of the control info map > should be handled / supported by the algorithms so they can populate it > correctly / dynamically based on what they can handle. But that's not > for this patch - and I suspect this is ok. > > > > + > > + auto temperatureK = ctrl.second.get<int32_t>(); > > + RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>( > > + controller_.getAlgorithm("awb")); > > + if (!awb) { > > + LOG(IPARPI, Warning) > > + << "Could not set COLOUR_TEMPERATURE - no AWB algorithm"; > > + break; > > I guess it would also prevent exposing a control when there's no > algorithm to implement/support it though. Still - for now this seems > like all that could be done here then. > > > But otherwise > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Naush ... are you happy with this patch ? > > > + } > > + > > + awb->setColourTemperature(temperatureK); > > + /* This metadata will get reported back automatically. */ > > + break; > > + } > > + > > case controls::BRIGHTNESS: { > > RPiController::ContrastAlgorithm *contrast = dynamic_cast<RPiController::ContrastAlgorithm *>( > > controller_.getAlgorithm("contrast")); > > diff --git a/src/ipa/rpi/controller/awb_algorithm.h b/src/ipa/rpi/controller/awb_algorithm.h > > index 8462c4db..d966dfa8 100644 > > --- a/src/ipa/rpi/controller/awb_algorithm.h > > +++ b/src/ipa/rpi/controller/awb_algorithm.h > > @@ -18,6 +18,7 @@ public: > > virtual unsigned int getConvergenceFrames() const = 0; > > virtual void setMode(std::string const &modeName) = 0; > > virtual void setManualGains(double manualR, double manualB) = 0; > > + virtual void setColourTemperature(double temperatureK) = 0; > > virtual void enableAuto() = 0; > > virtual void disableAuto() = 0; > > }; > > diff --git a/src/ipa/rpi/controller/rpi/awb.cpp b/src/ipa/rpi/controller/rpi/awb.cpp > > index 5ae0c2fa..0918e20d 100644 > > --- a/src/ipa/rpi/controller/rpi/awb.cpp > > +++ b/src/ipa/rpi/controller/rpi/awb.cpp > > @@ -275,6 +275,24 @@ void Awb::setManualGains(double manualR, double manualB) > > } > > } > > > > +void Awb::setColourTemperature(double temperatureK) > > +{ > > + if (!config_.bayes) { > > + LOG(RPiAwb, Warning) << "AWB uncalibrated - cannot set colour temperature"; > > + return; > > + } > > + > > + temperatureK = config_.ctR.domain().clip(temperatureK); > > + manualR_ = 1 / config_.ctR.eval(temperatureK); > > + manualB_ = 1 / config_.ctB.eval(temperatureK); > > + > > + syncResults_.temperatureK = temperatureK; > > + syncResults_.gainR = manualR_; > > + syncResults_.gainG = 1.0; > > + syncResults_.gainB = manualB_; > > + prevSyncResults_ = syncResults_; > > +} > > + > > void Awb::switchMode([[maybe_unused]] CameraMode const &cameraMode, > > Metadata *metadata) > > { > > diff --git a/src/ipa/rpi/controller/rpi/awb.h b/src/ipa/rpi/controller/rpi/awb.h > > index e7d49cd8..dbd79eda 100644 > > --- a/src/ipa/rpi/controller/rpi/awb.h > > +++ b/src/ipa/rpi/controller/rpi/awb.h > > @@ -97,6 +97,7 @@ public: > > unsigned int getConvergenceFrames() const override; > > void setMode(std::string const &name) override; > > void setManualGains(double manualR, double manualB) override; > > + void setColourTemperature(double temperatureK) override; > > void enableAuto() override; > > void disableAuto() override; > > void switchMode(CameraMode const &cameraMode, Metadata *metadata) override; > > -- > > 2.39.2 > >
On Fri, 10 Nov 2023, 4:14 pm David Plowman via libcamera-devel, < libcamera-devel@lists.libcamera.org> wrote: > ColourTemperature is now exported as a writable control so that > algorithms can set it directly. The AWB algorithm class now requires a > method to be provided to perform this operation. The method should > clamp the passed value to the calibrated range known to the algorithm. > > The default range is set very wide to cover all conceivable future AWB > calibrations. It will always be clamped before use. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > Oops thought I had already reviewed this! Reviewed-by: Naushir Patuck <naush@raspberrypi.com> --- > src/ipa/rpi/common/ipa_base.cpp | 20 ++++++++++++++++++++ > src/ipa/rpi/controller/awb_algorithm.h | 1 + > src/ipa/rpi/controller/rpi/awb.cpp | 18 ++++++++++++++++++ > src/ipa/rpi/controller/rpi/awb.h | 1 + > 4 files changed, 40 insertions(+) > > diff --git a/src/ipa/rpi/common/ipa_base.cpp > b/src/ipa/rpi/common/ipa_base.cpp > index 97a32522..cd1d6e3d 100644 > --- a/src/ipa/rpi/common/ipa_base.cpp > +++ b/src/ipa/rpi/common/ipa_base.cpp > @@ -80,6 +80,7 @@ const ControlInfoMap::Map ipaColourControls{ > { &controls::AwbEnable, ControlInfo(false, true) }, > { &controls::AwbMode, ControlInfo(controls::AwbModeValues) }, > { &controls::ColourGains, ControlInfo(0.0f, 32.0f) }, > + { &controls::ColourTemperature, ControlInfo(100, 100000) }, > { &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) }, > }; > > @@ -972,6 +973,25 @@ void IpaBase::applyControls(const ControlList > &controls) > break; > } > > + case controls::COLOUR_TEMPERATURE: { > + /* Silently ignore this control for a mono sensor. > */ > + if (monoSensor_) > + break; > + > + auto temperatureK = ctrl.second.get<int32_t>(); > + RPiController::AwbAlgorithm *awb = > dynamic_cast<RPiController::AwbAlgorithm *>( > + controller_.getAlgorithm("awb")); > + if (!awb) { > + LOG(IPARPI, Warning) > + << "Could not set > COLOUR_TEMPERATURE - no AWB algorithm"; > + break; > + } > + > + awb->setColourTemperature(temperatureK); > + /* This metadata will get reported back > automatically. */ > + break; > + } > + > case controls::BRIGHTNESS: { > RPiController::ContrastAlgorithm *contrast = > dynamic_cast<RPiController::ContrastAlgorithm *>( > controller_.getAlgorithm("contrast")); > diff --git a/src/ipa/rpi/controller/awb_algorithm.h > b/src/ipa/rpi/controller/awb_algorithm.h > index 8462c4db..d966dfa8 100644 > --- a/src/ipa/rpi/controller/awb_algorithm.h > +++ b/src/ipa/rpi/controller/awb_algorithm.h > @@ -18,6 +18,7 @@ public: > virtual unsigned int getConvergenceFrames() const = 0; > virtual void setMode(std::string const &modeName) = 0; > virtual void setManualGains(double manualR, double manualB) = 0; > + virtual void setColourTemperature(double temperatureK) = 0; > virtual void enableAuto() = 0; > virtual void disableAuto() = 0; > }; > diff --git a/src/ipa/rpi/controller/rpi/awb.cpp > b/src/ipa/rpi/controller/rpi/awb.cpp > index 5ae0c2fa..0918e20d 100644 > --- a/src/ipa/rpi/controller/rpi/awb.cpp > +++ b/src/ipa/rpi/controller/rpi/awb.cpp > @@ -275,6 +275,24 @@ void Awb::setManualGains(double manualR, double > manualB) > } > } > > +void Awb::setColourTemperature(double temperatureK) > +{ > + if (!config_.bayes) { > + LOG(RPiAwb, Warning) << "AWB uncalibrated - cannot set > colour temperature"; > + return; > + } > + > + temperatureK = config_.ctR.domain().clip(temperatureK); > + manualR_ = 1 / config_.ctR.eval(temperatureK); > + manualB_ = 1 / config_.ctB.eval(temperatureK); > + > + syncResults_.temperatureK = temperatureK; > + syncResults_.gainR = manualR_; > + syncResults_.gainG = 1.0; > + syncResults_.gainB = manualB_; > + prevSyncResults_ = syncResults_; > +} > + > void Awb::switchMode([[maybe_unused]] CameraMode const &cameraMode, > Metadata *metadata) > { > diff --git a/src/ipa/rpi/controller/rpi/awb.h > b/src/ipa/rpi/controller/rpi/awb.h > index e7d49cd8..dbd79eda 100644 > --- a/src/ipa/rpi/controller/rpi/awb.h > +++ b/src/ipa/rpi/controller/rpi/awb.h > @@ -97,6 +97,7 @@ public: > unsigned int getConvergenceFrames() const override; > void setMode(std::string const &name) override; > void setManualGains(double manualR, double manualB) override; > + void setColourTemperature(double temperatureK) override; > void enableAuto() override; > void disableAuto() override; > void switchMode(CameraMode const &cameraMode, Metadata *metadata) > override; > -- > 2.39.2 > >
Hi David, Thank you for the patch. On Fri, Nov 10, 2023 at 04:14:33PM +0000, David Plowman via libcamera-devel wrote: > ColourTemperature is now exported as a writable control so that > algorithms can set it directly. I'm puzzled here. You mention algorithms, but the patch exposes the control as being writable from applications. Is it a typo in the commit message ? If so, what's the use case ? control_ids.yaml documents the control as - ColourTemperature: type: int32_t description: Report the current estimate of the colour temperature, in kelvin, for this frame. The ColourTemperature control can only be returned in metadata. so this would need to be updated as a prerequisite for this patch. > The AWB algorithm class now requires a > method to be provided to perform this operation. The method should > clamp the passed value to the calibrated range known to the algorithm. > > The default range is set very wide to cover all conceivable future AWB > calibrations. It will always be clamped before use. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > --- > src/ipa/rpi/common/ipa_base.cpp | 20 ++++++++++++++++++++ > src/ipa/rpi/controller/awb_algorithm.h | 1 + > src/ipa/rpi/controller/rpi/awb.cpp | 18 ++++++++++++++++++ > src/ipa/rpi/controller/rpi/awb.h | 1 + > 4 files changed, 40 insertions(+) > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp > index 97a32522..cd1d6e3d 100644 > --- a/src/ipa/rpi/common/ipa_base.cpp > +++ b/src/ipa/rpi/common/ipa_base.cpp > @@ -80,6 +80,7 @@ const ControlInfoMap::Map ipaColourControls{ > { &controls::AwbEnable, ControlInfo(false, true) }, > { &controls::AwbMode, ControlInfo(controls::AwbModeValues) }, > { &controls::ColourGains, ControlInfo(0.0f, 32.0f) }, > + { &controls::ColourTemperature, ControlInfo(100, 100000) }, > { &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) }, > }; > > @@ -972,6 +973,25 @@ void IpaBase::applyControls(const ControlList &controls) > break; > } > > + case controls::COLOUR_TEMPERATURE: { > + /* Silently ignore this control for a mono sensor. */ > + if (monoSensor_) > + break; > + > + auto temperatureK = ctrl.second.get<int32_t>(); > + RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>( > + controller_.getAlgorithm("awb")); > + if (!awb) { > + LOG(IPARPI, Warning) > + << "Could not set COLOUR_TEMPERATURE - no AWB algorithm"; > + break; > + } > + > + awb->setColourTemperature(temperatureK); > + /* This metadata will get reported back automatically. */ > + break; > + } > + > case controls::BRIGHTNESS: { > RPiController::ContrastAlgorithm *contrast = dynamic_cast<RPiController::ContrastAlgorithm *>( > controller_.getAlgorithm("contrast")); > diff --git a/src/ipa/rpi/controller/awb_algorithm.h b/src/ipa/rpi/controller/awb_algorithm.h > index 8462c4db..d966dfa8 100644 > --- a/src/ipa/rpi/controller/awb_algorithm.h > +++ b/src/ipa/rpi/controller/awb_algorithm.h > @@ -18,6 +18,7 @@ public: > virtual unsigned int getConvergenceFrames() const = 0; > virtual void setMode(std::string const &modeName) = 0; > virtual void setManualGains(double manualR, double manualB) = 0; > + virtual void setColourTemperature(double temperatureK) = 0; > virtual void enableAuto() = 0; > virtual void disableAuto() = 0; > }; > diff --git a/src/ipa/rpi/controller/rpi/awb.cpp b/src/ipa/rpi/controller/rpi/awb.cpp > index 5ae0c2fa..0918e20d 100644 > --- a/src/ipa/rpi/controller/rpi/awb.cpp > +++ b/src/ipa/rpi/controller/rpi/awb.cpp > @@ -275,6 +275,24 @@ void Awb::setManualGains(double manualR, double manualB) > } > } > > +void Awb::setColourTemperature(double temperatureK) > +{ > + if (!config_.bayes) { > + LOG(RPiAwb, Warning) << "AWB uncalibrated - cannot set colour temperature"; > + return; > + } > + > + temperatureK = config_.ctR.domain().clip(temperatureK); > + manualR_ = 1 / config_.ctR.eval(temperatureK); > + manualB_ = 1 / config_.ctB.eval(temperatureK); > + > + syncResults_.temperatureK = temperatureK; > + syncResults_.gainR = manualR_; > + syncResults_.gainG = 1.0; > + syncResults_.gainB = manualB_; > + prevSyncResults_ = syncResults_; > +} > + > void Awb::switchMode([[maybe_unused]] CameraMode const &cameraMode, > Metadata *metadata) > { > diff --git a/src/ipa/rpi/controller/rpi/awb.h b/src/ipa/rpi/controller/rpi/awb.h > index e7d49cd8..dbd79eda 100644 > --- a/src/ipa/rpi/controller/rpi/awb.h > +++ b/src/ipa/rpi/controller/rpi/awb.h > @@ -97,6 +97,7 @@ public: > unsigned int getConvergenceFrames() const override; > void setMode(std::string const &name) override; > void setManualGains(double manualR, double manualB) override; > + void setColourTemperature(double temperatureK) override; > void enableAuto() override; > void disableAuto() override; > void switchMode(CameraMode const &cameraMode, Metadata *metadata) override;
Hi Laurent Thanks for the review. On Thu, 23 Nov 2023 at 12:27, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi David, > > Thank you for the patch. > > On Fri, Nov 10, 2023 at 04:14:33PM +0000, David Plowman via libcamera-devel wrote: > > ColourTemperature is now exported as a writable control so that > > algorithms can set it directly. > > I'm puzzled here. You mention algorithms, but the patch exposes the > control as being writable from applications. Is it a typo in the commit > message ? If so, what's the use case ? control_ids.yaml documents the > control as Yes, I'll change "algorithms" to "applications". > > - ColourTemperature: > type: int32_t > description: Report the current estimate of the colour temperature, in > kelvin, for this frame. The ColourTemperature control can only be > returned in metadata. > > so this would need to be updated as a prerequisite for this patch. I'll add a patch before this one to change that description. Thanks! David > > > The AWB algorithm class now requires a > > method to be provided to perform this operation. The method should > > clamp the passed value to the calibrated range known to the algorithm. > > > > The default range is set very wide to cover all conceivable future AWB > > calibrations. It will always be clamped before use. > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > --- > > src/ipa/rpi/common/ipa_base.cpp | 20 ++++++++++++++++++++ > > src/ipa/rpi/controller/awb_algorithm.h | 1 + > > src/ipa/rpi/controller/rpi/awb.cpp | 18 ++++++++++++++++++ > > src/ipa/rpi/controller/rpi/awb.h | 1 + > > 4 files changed, 40 insertions(+) > > > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp > > index 97a32522..cd1d6e3d 100644 > > --- a/src/ipa/rpi/common/ipa_base.cpp > > +++ b/src/ipa/rpi/common/ipa_base.cpp > > @@ -80,6 +80,7 @@ const ControlInfoMap::Map ipaColourControls{ > > { &controls::AwbEnable, ControlInfo(false, true) }, > > { &controls::AwbMode, ControlInfo(controls::AwbModeValues) }, > > { &controls::ColourGains, ControlInfo(0.0f, 32.0f) }, > > + { &controls::ColourTemperature, ControlInfo(100, 100000) }, > > { &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) }, > > }; > > > > @@ -972,6 +973,25 @@ void IpaBase::applyControls(const ControlList &controls) > > break; > > } > > > > + case controls::COLOUR_TEMPERATURE: { > > + /* Silently ignore this control for a mono sensor. */ > > + if (monoSensor_) > > + break; > > + > > + auto temperatureK = ctrl.second.get<int32_t>(); > > + RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>( > > + controller_.getAlgorithm("awb")); > > + if (!awb) { > > + LOG(IPARPI, Warning) > > + << "Could not set COLOUR_TEMPERATURE - no AWB algorithm"; > > + break; > > + } > > + > > + awb->setColourTemperature(temperatureK); > > + /* This metadata will get reported back automatically. */ > > + break; > > + } > > + > > case controls::BRIGHTNESS: { > > RPiController::ContrastAlgorithm *contrast = dynamic_cast<RPiController::ContrastAlgorithm *>( > > controller_.getAlgorithm("contrast")); > > diff --git a/src/ipa/rpi/controller/awb_algorithm.h b/src/ipa/rpi/controller/awb_algorithm.h > > index 8462c4db..d966dfa8 100644 > > --- a/src/ipa/rpi/controller/awb_algorithm.h > > +++ b/src/ipa/rpi/controller/awb_algorithm.h > > @@ -18,6 +18,7 @@ public: > > virtual unsigned int getConvergenceFrames() const = 0; > > virtual void setMode(std::string const &modeName) = 0; > > virtual void setManualGains(double manualR, double manualB) = 0; > > + virtual void setColourTemperature(double temperatureK) = 0; > > virtual void enableAuto() = 0; > > virtual void disableAuto() = 0; > > }; > > diff --git a/src/ipa/rpi/controller/rpi/awb.cpp b/src/ipa/rpi/controller/rpi/awb.cpp > > index 5ae0c2fa..0918e20d 100644 > > --- a/src/ipa/rpi/controller/rpi/awb.cpp > > +++ b/src/ipa/rpi/controller/rpi/awb.cpp > > @@ -275,6 +275,24 @@ void Awb::setManualGains(double manualR, double manualB) > > } > > } > > > > +void Awb::setColourTemperature(double temperatureK) > > +{ > > + if (!config_.bayes) { > > + LOG(RPiAwb, Warning) << "AWB uncalibrated - cannot set colour temperature"; > > + return; > > + } > > + > > + temperatureK = config_.ctR.domain().clip(temperatureK); > > + manualR_ = 1 / config_.ctR.eval(temperatureK); > > + manualB_ = 1 / config_.ctB.eval(temperatureK); > > + > > + syncResults_.temperatureK = temperatureK; > > + syncResults_.gainR = manualR_; > > + syncResults_.gainG = 1.0; > > + syncResults_.gainB = manualB_; > > + prevSyncResults_ = syncResults_; > > +} > > + > > void Awb::switchMode([[maybe_unused]] CameraMode const &cameraMode, > > Metadata *metadata) > > { > > diff --git a/src/ipa/rpi/controller/rpi/awb.h b/src/ipa/rpi/controller/rpi/awb.h > > index e7d49cd8..dbd79eda 100644 > > --- a/src/ipa/rpi/controller/rpi/awb.h > > +++ b/src/ipa/rpi/controller/rpi/awb.h > > @@ -97,6 +97,7 @@ public: > > unsigned int getConvergenceFrames() const override; > > void setMode(std::string const &name) override; > > void setManualGains(double manualR, double manualB) override; > > + void setColourTemperature(double temperatureK) override; > > void enableAuto() override; > > void disableAuto() override; > > void switchMode(CameraMode const &cameraMode, Metadata *metadata) override; > > -- > Regards, > > Laurent Pinchart
On Thu, Nov 23, 2023 at 12:54:43PM +0000, David Plowman wrote: > On Thu, 23 Nov 2023 at 12:27, Laurent Pinchart wrote: > > On Fri, Nov 10, 2023 at 04:14:33PM +0000, David Plowman via libcamera-devel wrote: > > > ColourTemperature is now exported as a writable control so that > > > algorithms can set it directly. > > > > I'm puzzled here. You mention algorithms, but the patch exposes the > > control as being writable from applications. Is it a typo in the commit > > message ? If so, what's the use case ? control_ids.yaml documents the > > control as > > Yes, I'll change "algorithms" to "applications". > > > > > - ColourTemperature: > > type: int32_t > > description: Report the current estimate of the colour temperature, in > > kelvin, for this frame. The ColourTemperature control can only be > > returned in metadata. > > > > so this would need to be updated as a prerequisite for this patch. > > I'll add a patch before this one to change that description. OK, let's discuss the use case there. > > > The AWB algorithm class now requires a > > > method to be provided to perform this operation. The method should > > > clamp the passed value to the calibrated range known to the algorithm. > > > > > > The default range is set very wide to cover all conceivable future AWB > > > calibrations. It will always be clamped before use. > > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > > --- > > > src/ipa/rpi/common/ipa_base.cpp | 20 ++++++++++++++++++++ > > > src/ipa/rpi/controller/awb_algorithm.h | 1 + > > > src/ipa/rpi/controller/rpi/awb.cpp | 18 ++++++++++++++++++ > > > src/ipa/rpi/controller/rpi/awb.h | 1 + > > > 4 files changed, 40 insertions(+) > > > > > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp > > > index 97a32522..cd1d6e3d 100644 > > > --- a/src/ipa/rpi/common/ipa_base.cpp > > > +++ b/src/ipa/rpi/common/ipa_base.cpp > > > @@ -80,6 +80,7 @@ const ControlInfoMap::Map ipaColourControls{ > > > { &controls::AwbEnable, ControlInfo(false, true) }, > > > { &controls::AwbMode, ControlInfo(controls::AwbModeValues) }, > > > { &controls::ColourGains, ControlInfo(0.0f, 32.0f) }, > > > + { &controls::ColourTemperature, ControlInfo(100, 100000) }, > > > { &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) }, > > > }; > > > > > > @@ -972,6 +973,25 @@ void IpaBase::applyControls(const ControlList &controls) > > > break; > > > } > > > > > > + case controls::COLOUR_TEMPERATURE: { > > > + /* Silently ignore this control for a mono sensor. */ > > > + if (monoSensor_) > > > + break; > > > + > > > + auto temperatureK = ctrl.second.get<int32_t>(); > > > + RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>( > > > + controller_.getAlgorithm("awb")); > > > + if (!awb) { > > > + LOG(IPARPI, Warning) > > > + << "Could not set COLOUR_TEMPERATURE - no AWB algorithm"; > > > + break; > > > + } > > > + > > > + awb->setColourTemperature(temperatureK); > > > + /* This metadata will get reported back automatically. */ > > > + break; > > > + } > > > + > > > case controls::BRIGHTNESS: { > > > RPiController::ContrastAlgorithm *contrast = dynamic_cast<RPiController::ContrastAlgorithm *>( > > > controller_.getAlgorithm("contrast")); > > > diff --git a/src/ipa/rpi/controller/awb_algorithm.h b/src/ipa/rpi/controller/awb_algorithm.h > > > index 8462c4db..d966dfa8 100644 > > > --- a/src/ipa/rpi/controller/awb_algorithm.h > > > +++ b/src/ipa/rpi/controller/awb_algorithm.h > > > @@ -18,6 +18,7 @@ public: > > > virtual unsigned int getConvergenceFrames() const = 0; > > > virtual void setMode(std::string const &modeName) = 0; > > > virtual void setManualGains(double manualR, double manualB) = 0; > > > + virtual void setColourTemperature(double temperatureK) = 0; > > > virtual void enableAuto() = 0; > > > virtual void disableAuto() = 0; > > > }; > > > diff --git a/src/ipa/rpi/controller/rpi/awb.cpp b/src/ipa/rpi/controller/rpi/awb.cpp > > > index 5ae0c2fa..0918e20d 100644 > > > --- a/src/ipa/rpi/controller/rpi/awb.cpp > > > +++ b/src/ipa/rpi/controller/rpi/awb.cpp > > > @@ -275,6 +275,24 @@ void Awb::setManualGains(double manualR, double manualB) > > > } > > > } > > > > > > +void Awb::setColourTemperature(double temperatureK) > > > +{ > > > + if (!config_.bayes) { > > > + LOG(RPiAwb, Warning) << "AWB uncalibrated - cannot set colour temperature"; > > > + return; > > > + } > > > + > > > + temperatureK = config_.ctR.domain().clip(temperatureK); > > > + manualR_ = 1 / config_.ctR.eval(temperatureK); > > > + manualB_ = 1 / config_.ctB.eval(temperatureK); > > > + > > > + syncResults_.temperatureK = temperatureK; > > > + syncResults_.gainR = manualR_; > > > + syncResults_.gainG = 1.0; > > > + syncResults_.gainB = manualB_; > > > + prevSyncResults_ = syncResults_; > > > +} > > > + > > > void Awb::switchMode([[maybe_unused]] CameraMode const &cameraMode, > > > Metadata *metadata) > > > { > > > diff --git a/src/ipa/rpi/controller/rpi/awb.h b/src/ipa/rpi/controller/rpi/awb.h > > > index e7d49cd8..dbd79eda 100644 > > > --- a/src/ipa/rpi/controller/rpi/awb.h > > > +++ b/src/ipa/rpi/controller/rpi/awb.h > > > @@ -97,6 +97,7 @@ public: > > > unsigned int getConvergenceFrames() const override; > > > void setMode(std::string const &name) override; > > > void setManualGains(double manualR, double manualB) override; > > > + void setColourTemperature(double temperatureK) override; > > > void enableAuto() override; > > > void disableAuto() override; > > > void switchMode(CameraMode const &cameraMode, Metadata *metadata) override;
diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp index 97a32522..cd1d6e3d 100644 --- a/src/ipa/rpi/common/ipa_base.cpp +++ b/src/ipa/rpi/common/ipa_base.cpp @@ -80,6 +80,7 @@ const ControlInfoMap::Map ipaColourControls{ { &controls::AwbEnable, ControlInfo(false, true) }, { &controls::AwbMode, ControlInfo(controls::AwbModeValues) }, { &controls::ColourGains, ControlInfo(0.0f, 32.0f) }, + { &controls::ColourTemperature, ControlInfo(100, 100000) }, { &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) }, }; @@ -972,6 +973,25 @@ void IpaBase::applyControls(const ControlList &controls) break; } + case controls::COLOUR_TEMPERATURE: { + /* Silently ignore this control for a mono sensor. */ + if (monoSensor_) + break; + + auto temperatureK = ctrl.second.get<int32_t>(); + RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>( + controller_.getAlgorithm("awb")); + if (!awb) { + LOG(IPARPI, Warning) + << "Could not set COLOUR_TEMPERATURE - no AWB algorithm"; + break; + } + + awb->setColourTemperature(temperatureK); + /* This metadata will get reported back automatically. */ + break; + } + case controls::BRIGHTNESS: { RPiController::ContrastAlgorithm *contrast = dynamic_cast<RPiController::ContrastAlgorithm *>( controller_.getAlgorithm("contrast")); diff --git a/src/ipa/rpi/controller/awb_algorithm.h b/src/ipa/rpi/controller/awb_algorithm.h index 8462c4db..d966dfa8 100644 --- a/src/ipa/rpi/controller/awb_algorithm.h +++ b/src/ipa/rpi/controller/awb_algorithm.h @@ -18,6 +18,7 @@ public: virtual unsigned int getConvergenceFrames() const = 0; virtual void setMode(std::string const &modeName) = 0; virtual void setManualGains(double manualR, double manualB) = 0; + virtual void setColourTemperature(double temperatureK) = 0; virtual void enableAuto() = 0; virtual void disableAuto() = 0; }; diff --git a/src/ipa/rpi/controller/rpi/awb.cpp b/src/ipa/rpi/controller/rpi/awb.cpp index 5ae0c2fa..0918e20d 100644 --- a/src/ipa/rpi/controller/rpi/awb.cpp +++ b/src/ipa/rpi/controller/rpi/awb.cpp @@ -275,6 +275,24 @@ void Awb::setManualGains(double manualR, double manualB) } } +void Awb::setColourTemperature(double temperatureK) +{ + if (!config_.bayes) { + LOG(RPiAwb, Warning) << "AWB uncalibrated - cannot set colour temperature"; + return; + } + + temperatureK = config_.ctR.domain().clip(temperatureK); + manualR_ = 1 / config_.ctR.eval(temperatureK); + manualB_ = 1 / config_.ctB.eval(temperatureK); + + syncResults_.temperatureK = temperatureK; + syncResults_.gainR = manualR_; + syncResults_.gainG = 1.0; + syncResults_.gainB = manualB_; + prevSyncResults_ = syncResults_; +} + void Awb::switchMode([[maybe_unused]] CameraMode const &cameraMode, Metadata *metadata) { diff --git a/src/ipa/rpi/controller/rpi/awb.h b/src/ipa/rpi/controller/rpi/awb.h index e7d49cd8..dbd79eda 100644 --- a/src/ipa/rpi/controller/rpi/awb.h +++ b/src/ipa/rpi/controller/rpi/awb.h @@ -97,6 +97,7 @@ public: unsigned int getConvergenceFrames() const override; void setMode(std::string const &name) override; void setManualGains(double manualR, double manualB) override; + void setColourTemperature(double temperatureK) override; void enableAuto() override; void disableAuto() override; void switchMode(CameraMode const &cameraMode, Metadata *metadata) override;
ColourTemperature is now exported as a writable control so that algorithms can set it directly. The AWB algorithm class now requires a method to be provided to perform this operation. The method should clamp the passed value to the calibrated range known to the algorithm. The default range is set very wide to cover all conceivable future AWB calibrations. It will always be clamped before use. Signed-off-by: David Plowman <david.plowman@raspberrypi.com> --- src/ipa/rpi/common/ipa_base.cpp | 20 ++++++++++++++++++++ src/ipa/rpi/controller/awb_algorithm.h | 1 + src/ipa/rpi/controller/rpi/awb.cpp | 18 ++++++++++++++++++ src/ipa/rpi/controller/rpi/awb.h | 1 + 4 files changed, 40 insertions(+)