[libcamera-devel,v2] ipa: rkisp1: Add manual color gains
diff mbox series

Message ID 20220818090108.3004198-1-paul.elder@ideasonboard.com
State Accepted
Headers show
Series
  • [libcamera-devel,v2] ipa: rkisp1: Add manual color gains
Related show

Commit Message

Paul Elder Aug. 18, 2022, 9:01 a.m. UTC 
Add support for manually controlling the color gains on the rkisp1 IPA.
To that end, add and plumb the AwbEnable and ColourGains controls. As
per-frame controls aren't supported yet in the rkisp1 IPA, simply apply
and perform checks on the controls immediately.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

---
Changes in v2:
- s/enabled/autoEnabled
  - add documentation
- fixup construction of the ControlInfo
---
 src/ipa/rkisp1/algorithms/awb.cpp | 36 +++++++++++++++++++++++++++++--
 src/ipa/rkisp1/algorithms/awb.h   |  3 +++
 src/ipa/rkisp1/ipa_context.cpp    |  3 +++
 src/ipa/rkisp1/ipa_context.h      |  1 +
 src/ipa/rkisp1/rkisp1.cpp         |  2 ++
 5 files changed, 43 insertions(+), 2 deletions(-)

Comments

Jacopo Mondi Aug. 19, 2022, 1:11 p.m. UTC | #1 
Hi Paul

On Thu, Aug 18, 2022 at 06:01:08PM +0900, Paul Elder via libcamera-devel wrote:
> Add support for manually controlling the color gains on the rkisp1 IPA.
> To that end, add and plumb the AwbEnable and ColourGains controls. As
> per-frame controls aren't supported yet in the rkisp1 IPA, simply apply
> and perform checks on the controls immediately.
>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>

The patch looks sane to me!
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

> ---
> Changes in v2:
> - s/enabled/autoEnabled
>   - add documentation
> - fixup construction of the ControlInfo
> ---
>  src/ipa/rkisp1/algorithms/awb.cpp | 36 +++++++++++++++++++++++++++++--
>  src/ipa/rkisp1/algorithms/awb.h   |  3 +++
>  src/ipa/rkisp1/ipa_context.cpp    |  3 +++
>  src/ipa/rkisp1/ipa_context.h      |  1 +
>  src/ipa/rkisp1/rkisp1.cpp         |  2 ++
>  5 files changed, 43 insertions(+), 2 deletions(-)
>
> diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> index 9f00364d..a3c14a9a 100644
> --- a/src/ipa/rkisp1/algorithms/awb.cpp
> +++ b/src/ipa/rkisp1/algorithms/awb.cpp
> @@ -12,6 +12,7 @@
>
>  #include <libcamera/base/log.h>
>
> +#include <libcamera/control_ids.h>
>  #include <libcamera/ipa/core_ipa_interface.h>
>
>  /**
> @@ -38,6 +39,7 @@ int Awb::configure(IPAContext &context,
>  	context.frameContext.awb.gains.red = 1.0;
>  	context.frameContext.awb.gains.blue = 1.0;
>  	context.frameContext.awb.gains.green = 1.0;
> +	context.frameContext.awb.autoEnabled = true;
>
>  	/*
>  	 * Define the measurement window for AWB as a centered rectangle
> @@ -116,6 +118,34 @@ void Awb::prepare(IPAContext &context, rkisp1_params_cfg *params)
>  	params->module_ens |= RKISP1_CIF_ISP_MODULE_AWB;
>  }
>
> +/**
> + * \copydoc libcamera::ipa::Algorithm::queueRequest
> + */
> +void Awb::queueRequest(IPAContext &context,
> +		       [[maybe_unused]] const uint32_t frame,
> +		       const ControlList &controls)
> +{
> +	auto &awb = context.frameContext.awb;
> +
> +	const auto &awbEnable = controls.get(controls::AwbEnable);
> +	if (awbEnable && *awbEnable != awb.autoEnabled) {
> +		awb.autoEnabled = *awbEnable;
> +
> +		LOG(RkISP1Awb, Debug)
> +			<< (*awbEnable ? "Enabling" : "Disabling") << " AWB";
> +	}
> +
> +	const auto &colourGains = controls.get(controls::ColourGains);
> +	if (colourGains && !awb.autoEnabled) {
> +		awb.gains.red = (*colourGains)[0];
> +		awb.gains.blue = (*colourGains)[1];
> +
> +		LOG(RkISP1Awb, Debug)
> +			<< "Set colour gains to red: " << awb.gains.red
> +			<< ", blue: " << awb.gains.blue;
> +	}
> +}
> +
>  /**
>   * \copydoc libcamera::ipa::Algorithm::process
>   */
> @@ -164,8 +194,10 @@ void Awb::process([[maybe_unused]] IPAContext &context,
>  	 * Gain values are unsigned integer value, range 0 to 4 with 8 bit
>  	 * fractional part.
>  	 */
> -	frameContext.awb.gains.red = std::clamp(redGain, 0.0, 1023.0 / 256);
> -	frameContext.awb.gains.blue = std::clamp(blueGain, 0.0, 1023.0 / 256);
> +	if (frameContext.awb.autoEnabled) {
> +		frameContext.awb.gains.red = std::clamp(redGain, 0.0, 1023.0 / 256);
> +		frameContext.awb.gains.blue = std::clamp(blueGain, 0.0, 1023.0 / 256);
> +	}
>  	/* Hardcode the green gain to 1.0. */
>  	frameContext.awb.gains.green = 1.0;
>
> diff --git a/src/ipa/rkisp1/algorithms/awb.h b/src/ipa/rkisp1/algorithms/awb.h
> index 667a8beb..4332fac7 100644
> --- a/src/ipa/rkisp1/algorithms/awb.h
> +++ b/src/ipa/rkisp1/algorithms/awb.h
> @@ -21,6 +21,9 @@ public:
>
>  	int configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override;
>  	void prepare(IPAContext &context, rkisp1_params_cfg *params) override;
> +	void queueRequest(IPAContext &context,
> +			  const uint32_t frame,
> +			  const ControlList &controls);
>  	void process(IPAContext &context, IPAFrameContext *frameCtx,
>  		     const rkisp1_stat_buffer *stats) override;
>
> diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
> index ef8bb8e9..290569d0 100644
> --- a/src/ipa/rkisp1/ipa_context.cpp
> +++ b/src/ipa/rkisp1/ipa_context.cpp
> @@ -134,6 +134,9 @@ namespace libcamera::ipa::rkisp1 {
>   *
>   * \var IPAFrameContext::awb.temperatureK
>   * \brief Estimated color temperature
> + *
> + * \var IPAFrameContext::awb.autoEnabled
> + * \brief Whether the Auto White Balance algorithm is enabled
>   */
>
>  /**
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index 2bdb6a81..68f92297 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -55,6 +55,7 @@ struct IPAFrameContext {
>  		} gains;
>
>  		double temperatureK;
> +		bool autoEnabled;
>  	} awb;
>
>  	struct {
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 17d42d38..27b4212b 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -92,6 +92,8 @@ namespace {
>  /* List of controls handled by the RkISP1 IPA */
>  const ControlInfoMap::Map rkisp1Controls{
>  	{ &controls::AeEnable, ControlInfo(false, true) },
> +	{ &controls::AwbEnable, ControlInfo(false, true) },
> +	{ &controls::ColourGains, ControlInfo(0.0f, 3.996f, 1.0f) },
>  	{ &controls::Brightness, ControlInfo(-1.0f, 0.993f) },
>  	{ &controls::Contrast, ControlInfo(0.0f, 1.993f) },
>  	{ &controls::Saturation, ControlInfo(0.0f, 1.993f) },
> --
> 2.30.2
>
Laurent Pinchart Aug. 19, 2022, 1:46 p.m. UTC | #2 
Hi Paul,

Thank you for the patch.

On Thu, Aug 18, 2022 at 06:01:08PM +0900, Paul Elder wrote:
> Add support for manually controlling the color gains on the rkisp1 IPA.
> To that end, add and plumb the AwbEnable and ColourGains controls. As
> per-frame controls aren't supported yet in the rkisp1 IPA, simply apply
> and perform checks on the controls immediately.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> ---
> Changes in v2:
> - s/enabled/autoEnabled
>   - add documentation
> - fixup construction of the ControlInfo
> ---
>  src/ipa/rkisp1/algorithms/awb.cpp | 36 +++++++++++++++++++++++++++++--
>  src/ipa/rkisp1/algorithms/awb.h   |  3 +++
>  src/ipa/rkisp1/ipa_context.cpp    |  3 +++
>  src/ipa/rkisp1/ipa_context.h      |  1 +
>  src/ipa/rkisp1/rkisp1.cpp         |  2 ++
>  5 files changed, 43 insertions(+), 2 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> index 9f00364d..a3c14a9a 100644
> --- a/src/ipa/rkisp1/algorithms/awb.cpp
> +++ b/src/ipa/rkisp1/algorithms/awb.cpp
> @@ -12,6 +12,7 @@
>  
>  #include <libcamera/base/log.h>
>  
> +#include <libcamera/control_ids.h>
>  #include <libcamera/ipa/core_ipa_interface.h>
>  
>  /**
> @@ -38,6 +39,7 @@ int Awb::configure(IPAContext &context,
>  	context.frameContext.awb.gains.red = 1.0;
>  	context.frameContext.awb.gains.blue = 1.0;
>  	context.frameContext.awb.gains.green = 1.0;
> +	context.frameContext.awb.autoEnabled = true;
>  
>  	/*
>  	 * Define the measurement window for AWB as a centered rectangle
> @@ -116,6 +118,34 @@ void Awb::prepare(IPAContext &context, rkisp1_params_cfg *params)
>  	params->module_ens |= RKISP1_CIF_ISP_MODULE_AWB;
>  }
>  
> +/**
> + * \copydoc libcamera::ipa::Algorithm::queueRequest
> + */
> +void Awb::queueRequest(IPAContext &context,
> +		       [[maybe_unused]] const uint32_t frame,
> +		       const ControlList &controls)
> +{
> +	auto &awb = context.frameContext.awb;
> +
> +	const auto &awbEnable = controls.get(controls::AwbEnable);
> +	if (awbEnable && *awbEnable != awb.autoEnabled) {
> +		awb.autoEnabled = *awbEnable;
> +
> +		LOG(RkISP1Awb, Debug)
> +			<< (*awbEnable ? "Enabling" : "Disabling") << " AWB";
> +	}
> +
> +	const auto &colourGains = controls.get(controls::ColourGains);
> +	if (colourGains && !awb.autoEnabled) {
> +		awb.gains.red = (*colourGains)[0];
> +		awb.gains.blue = (*colourGains)[1];
> +
> +		LOG(RkISP1Awb, Debug)
> +			<< "Set colour gains to red: " << awb.gains.red
> +			<< ", blue: " << awb.gains.blue;
> +	}
> +}
> +
>  /**
>   * \copydoc libcamera::ipa::Algorithm::process
>   */
> @@ -164,8 +194,10 @@ void Awb::process([[maybe_unused]] IPAContext &context,
>  	 * Gain values are unsigned integer value, range 0 to 4 with 8 bit
>  	 * fractional part.
>  	 */
> -	frameContext.awb.gains.red = std::clamp(redGain, 0.0, 1023.0 / 256);
> -	frameContext.awb.gains.blue = std::clamp(blueGain, 0.0, 1023.0 / 256);
> +	if (frameContext.awb.autoEnabled) {
> +		frameContext.awb.gains.red = std::clamp(redGain, 0.0, 1023.0 / 256);
> +		frameContext.awb.gains.blue = std::clamp(blueGain, 0.0, 1023.0 / 256);
> +	}
>  	/* Hardcode the green gain to 1.0. */
>  	frameContext.awb.gains.green = 1.0;
>  
> diff --git a/src/ipa/rkisp1/algorithms/awb.h b/src/ipa/rkisp1/algorithms/awb.h
> index 667a8beb..4332fac7 100644
> --- a/src/ipa/rkisp1/algorithms/awb.h
> +++ b/src/ipa/rkisp1/algorithms/awb.h
> @@ -21,6 +21,9 @@ public:
>  
>  	int configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override;
>  	void prepare(IPAContext &context, rkisp1_params_cfg *params) override;
> +	void queueRequest(IPAContext &context,
> +			  const uint32_t frame,
> +			  const ControlList &controls);

Missing override. I'll fix it when applying.

>  	void process(IPAContext &context, IPAFrameContext *frameCtx,
>  		     const rkisp1_stat_buffer *stats) override;
>  
> diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
> index ef8bb8e9..290569d0 100644
> --- a/src/ipa/rkisp1/ipa_context.cpp
> +++ b/src/ipa/rkisp1/ipa_context.cpp
> @@ -134,6 +134,9 @@ namespace libcamera::ipa::rkisp1 {
>   *
>   * \var IPAFrameContext::awb.temperatureK
>   * \brief Estimated color temperature
> + *
> + * \var IPAFrameContext::awb.autoEnabled
> + * \brief Whether the Auto White Balance algorithm is enabled
>   */
>  
>  /**
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index 2bdb6a81..68f92297 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -55,6 +55,7 @@ struct IPAFrameContext {
>  		} gains;
>  
>  		double temperatureK;
> +		bool autoEnabled;
>  	} awb;
>  
>  	struct {
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 17d42d38..27b4212b 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -92,6 +92,8 @@ namespace {
>  /* List of controls handled by the RkISP1 IPA */
>  const ControlInfoMap::Map rkisp1Controls{
>  	{ &controls::AeEnable, ControlInfo(false, true) },
> +	{ &controls::AwbEnable, ControlInfo(false, true) },
> +	{ &controls::ColourGains, ControlInfo(0.0f, 3.996f, 1.0f) },
>  	{ &controls::Brightness, ControlInfo(-1.0f, 0.993f) },
>  	{ &controls::Contrast, ControlInfo(0.0f, 1.993f) },
>  	{ &controls::Saturation, ControlInfo(0.0f, 1.993f) },

Patch
diff mbox series 

diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
index 9f00364d..a3c14a9a 100644
--- a/src/ipa/rkisp1/algorithms/awb.cpp
+++ b/src/ipa/rkisp1/algorithms/awb.cpp
@@ -12,6 +12,7 @@ 
 
 #include <libcamera/base/log.h>
 
+#include <libcamera/control_ids.h>
 #include <libcamera/ipa/core_ipa_interface.h>
 
 /**
@@ -38,6 +39,7 @@  int Awb::configure(IPAContext &context,
 	context.frameContext.awb.gains.red = 1.0;
 	context.frameContext.awb.gains.blue = 1.0;
 	context.frameContext.awb.gains.green = 1.0;
+	context.frameContext.awb.autoEnabled = true;
 
 	/*
 	 * Define the measurement window for AWB as a centered rectangle
@@ -116,6 +118,34 @@  void Awb::prepare(IPAContext &context, rkisp1_params_cfg *params)
 	params->module_ens |= RKISP1_CIF_ISP_MODULE_AWB;
 }
 
+/**
+ * \copydoc libcamera::ipa::Algorithm::queueRequest
+ */
+void Awb::queueRequest(IPAContext &context,
+		       [[maybe_unused]] const uint32_t frame,
+		       const ControlList &controls)
+{
+	auto &awb = context.frameContext.awb;
+
+	const auto &awbEnable = controls.get(controls::AwbEnable);
+	if (awbEnable && *awbEnable != awb.autoEnabled) {
+		awb.autoEnabled = *awbEnable;
+
+		LOG(RkISP1Awb, Debug)
+			<< (*awbEnable ? "Enabling" : "Disabling") << " AWB";
+	}
+
+	const auto &colourGains = controls.get(controls::ColourGains);
+	if (colourGains && !awb.autoEnabled) {
+		awb.gains.red = (*colourGains)[0];
+		awb.gains.blue = (*colourGains)[1];
+
+		LOG(RkISP1Awb, Debug)
+			<< "Set colour gains to red: " << awb.gains.red
+			<< ", blue: " << awb.gains.blue;
+	}
+}
+
 /**
  * \copydoc libcamera::ipa::Algorithm::process
  */
@@ -164,8 +194,10 @@  void Awb::process([[maybe_unused]] IPAContext &context,
 	 * Gain values are unsigned integer value, range 0 to 4 with 8 bit
 	 * fractional part.
 	 */
-	frameContext.awb.gains.red = std::clamp(redGain, 0.0, 1023.0 / 256);
-	frameContext.awb.gains.blue = std::clamp(blueGain, 0.0, 1023.0 / 256);
+	if (frameContext.awb.autoEnabled) {
+		frameContext.awb.gains.red = std::clamp(redGain, 0.0, 1023.0 / 256);
+		frameContext.awb.gains.blue = std::clamp(blueGain, 0.0, 1023.0 / 256);
+	}
 	/* Hardcode the green gain to 1.0. */
 	frameContext.awb.gains.green = 1.0;
 
diff --git a/src/ipa/rkisp1/algorithms/awb.h b/src/ipa/rkisp1/algorithms/awb.h
index 667a8beb..4332fac7 100644
--- a/src/ipa/rkisp1/algorithms/awb.h
+++ b/src/ipa/rkisp1/algorithms/awb.h
@@ -21,6 +21,9 @@  public:
 
 	int configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override;
 	void prepare(IPAContext &context, rkisp1_params_cfg *params) override;
+	void queueRequest(IPAContext &context,
+			  const uint32_t frame,
+			  const ControlList &controls);
 	void process(IPAContext &context, IPAFrameContext *frameCtx,
 		     const rkisp1_stat_buffer *stats) override;
 
diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
index ef8bb8e9..290569d0 100644
--- a/src/ipa/rkisp1/ipa_context.cpp
+++ b/src/ipa/rkisp1/ipa_context.cpp
@@ -134,6 +134,9 @@  namespace libcamera::ipa::rkisp1 {
  *
  * \var IPAFrameContext::awb.temperatureK
  * \brief Estimated color temperature
+ *
+ * \var IPAFrameContext::awb.autoEnabled
+ * \brief Whether the Auto White Balance algorithm is enabled
  */
 
 /**
diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
index 2bdb6a81..68f92297 100644
--- a/src/ipa/rkisp1/ipa_context.h
+++ b/src/ipa/rkisp1/ipa_context.h
@@ -55,6 +55,7 @@  struct IPAFrameContext {
 		} gains;
 
 		double temperatureK;
+		bool autoEnabled;
 	} awb;
 
 	struct {
diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
index 17d42d38..27b4212b 100644
--- a/src/ipa/rkisp1/rkisp1.cpp
+++ b/src/ipa/rkisp1/rkisp1.cpp
@@ -92,6 +92,8 @@  namespace {
 /* List of controls handled by the RkISP1 IPA */
 const ControlInfoMap::Map rkisp1Controls{
 	{ &controls::AeEnable, ControlInfo(false, true) },
+	{ &controls::AwbEnable, ControlInfo(false, true) },
+	{ &controls::ColourGains, ControlInfo(0.0f, 3.996f, 1.0f) },
 	{ &controls::Brightness, ControlInfo(-1.0f, 0.993f) },
 	{ &controls::Contrast, ControlInfo(0.0f, 1.993f) },
 	{ &controls::Saturation, ControlInfo(0.0f, 1.993f) },