[v4,13/21] ipa: rkisp1: cproc: Report metadata
diff mbox series

Message ID 20251114005428.90024-14-kieran.bingham@ideasonboard.com
State New
Headers show
Series
  • libipa: Introduce a Quantized type
Related show

Commit Message

Kieran Bingham Nov. 14, 2025, 12:54 a.m. UTC 
Presently the colour processing component exposes controls for
brightness, saturation, and contrast to the applications which are
handled and processed accordingly on the ISP.

The implementation lacks reporting the values that are set back to the
application.

Utilise the new Quantised types to provide the values that were applied
to the hardware and report them in the completed request metadata.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/ipa/rkisp1/algorithms/cproc.cpp | 12 ++++++++++++
 src/ipa/rkisp1/algorithms/cproc.h   |  4 ++++
 2 files changed, 16 insertions(+)

Comments

Barnabás Pőcze Nov. 14, 2025, 6:09 p.m. UTC | #1 
2025. 11. 14. 1:54 keltezéssel, Kieran Bingham írta:
> Presently the colour processing component exposes controls for
> brightness, saturation, and contrast to the applications which are
> handled and processed accordingly on the ISP.
> 
> The implementation lacks reporting the values that are set back to the
> application.
> 
> Utilise the new Quantised types to provide the values that were applied
> to the hardware and report them in the completed request metadata.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---

Looks ok to me.

Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>


>   src/ipa/rkisp1/algorithms/cproc.cpp | 12 ++++++++++++
>   src/ipa/rkisp1/algorithms/cproc.h   |  4 ++++
>   2 files changed, 16 insertions(+)
> 
> diff --git a/src/ipa/rkisp1/algorithms/cproc.cpp b/src/ipa/rkisp1/algorithms/cproc.cpp
> index 4637a824af03..7475e6c1b609 100644
> --- a/src/ipa/rkisp1/algorithms/cproc.cpp
> +++ b/src/ipa/rkisp1/algorithms/cproc.cpp
> @@ -145,6 +145,18 @@ void ColorProcessing::prepare([[maybe_unused]] IPAContext &context,
>   	config->sat = frameContext.cproc.saturation.quantized();
>   }
>   
> +/**
> + * \copydoc libcamera::ipa::Algorithm::process
> + */
> +void ColorProcessing::process([[maybe_unused]] IPAContext &context, [[maybe_unused]] const uint32_t frame,
> +			      IPAFrameContext &frameContext, [[maybe_unused]] const rkisp1_stat_buffer *stats,
> +			      ControlList &metadata)
> +{
> +	metadata.set(controls::Brightness, frameContext.cproc.brightness.value());
> +	metadata.set(controls::Contrast, frameContext.cproc.contrast.value());
> +	metadata.set(controls::Saturation, frameContext.cproc.saturation.value());
> +}
> +
>   REGISTER_IPA_ALGORITHM(ColorProcessing, "ColorProcessing")
>   
>   } /* namespace ipa::rkisp1::algorithms */
> diff --git a/src/ipa/rkisp1/algorithms/cproc.h b/src/ipa/rkisp1/algorithms/cproc.h
> index fd38fd17e8bb..9b589ebd4ad7 100644
> --- a/src/ipa/rkisp1/algorithms/cproc.h
> +++ b/src/ipa/rkisp1/algorithms/cproc.h
> @@ -30,6 +30,10 @@ public:
>   	void prepare(IPAContext &context, const uint32_t frame,
>   		     IPAFrameContext &frameContext,
>   		     RkISP1Params *params) override;
> +	void process(IPAContext &context, const uint32_t frame,
> +		     IPAFrameContext &frameContext,
> +		     const rkisp1_stat_buffer *stats,
> +		     ControlList &metadata) override;
>   };
>   
>   } /* namespace ipa::rkisp1::algorithms */
Isaac Scott Nov. 18, 2025, 12:40 p.m. UTC | #2 
Hi Kieran,

Thank you for the patch!

Reviewed-by: Isaac Scott <isaac.scott@ideasonboard.com>

Quoting Kieran Bingham (2025-11-14 00:54:17)
> Presently the colour processing component exposes controls for
> brightness, saturation, and contrast to the applications which are
> handled and processed accordingly on the ISP.
> 
> The implementation lacks reporting the values that are set back to the
> application.
> 
> Utilise the new Quantised types to provide the values that were applied
> to the hardware and report them in the completed request metadata.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  src/ipa/rkisp1/algorithms/cproc.cpp | 12 ++++++++++++
>  src/ipa/rkisp1/algorithms/cproc.h   |  4 ++++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/src/ipa/rkisp1/algorithms/cproc.cpp b/src/ipa/rkisp1/algorithms/cproc.cpp
> index 4637a824af03..7475e6c1b609 100644
> --- a/src/ipa/rkisp1/algorithms/cproc.cpp
> +++ b/src/ipa/rkisp1/algorithms/cproc.cpp
> @@ -145,6 +145,18 @@ void ColorProcessing::prepare([[maybe_unused]] IPAContext &context,
>         config->sat = frameContext.cproc.saturation.quantized();
>  }
>  
> +/**
> + * \copydoc libcamera::ipa::Algorithm::process
> + */
> +void ColorProcessing::process([[maybe_unused]] IPAContext &context, [[maybe_unused]] const uint32_t frame,
> +                             IPAFrameContext &frameContext, [[maybe_unused]] const rkisp1_stat_buffer *stats,
> +                             ControlList &metadata)
> +{
> +       metadata.set(controls::Brightness, frameContext.cproc.brightness.value());
> +       metadata.set(controls::Contrast, frameContext.cproc.contrast.value());
> +       metadata.set(controls::Saturation, frameContext.cproc.saturation.value());
> +}
> +
>  REGISTER_IPA_ALGORITHM(ColorProcessing, "ColorProcessing")
>  
>  } /* namespace ipa::rkisp1::algorithms */
> diff --git a/src/ipa/rkisp1/algorithms/cproc.h b/src/ipa/rkisp1/algorithms/cproc.h
> index fd38fd17e8bb..9b589ebd4ad7 100644
> --- a/src/ipa/rkisp1/algorithms/cproc.h
> +++ b/src/ipa/rkisp1/algorithms/cproc.h
> @@ -30,6 +30,10 @@ public:
>         void prepare(IPAContext &context, const uint32_t frame,
>                      IPAFrameContext &frameContext,
>                      RkISP1Params *params) override;
> +       void process(IPAContext &context, const uint32_t frame,
> +                    IPAFrameContext &frameContext,
> +                    const rkisp1_stat_buffer *stats,
> +                    ControlList &metadata) override;
>  };
>  
>  } /* namespace ipa::rkisp1::algorithms */
> -- 
> 2.51.1
>
Laurent Pinchart Nov. 19, 2025, 4:30 a.m. UTC | #3 
Hi Kieran,

Thank you for the patch.

On Fri, Nov 14, 2025 at 12:54:17AM +0000, Kieran Bingham wrote:
> Presently the colour processing component exposes controls for
> brightness, saturation, and contrast to the applications which are
> handled and processed accordingly on the ISP.
> 
> The implementation lacks reporting the values that are set back to the
> application.
> 
> Utilise the new Quantised types to provide the values that were applied
> to the hardware and report them in the completed request metadata.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  src/ipa/rkisp1/algorithms/cproc.cpp | 12 ++++++++++++
>  src/ipa/rkisp1/algorithms/cproc.h   |  4 ++++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/src/ipa/rkisp1/algorithms/cproc.cpp b/src/ipa/rkisp1/algorithms/cproc.cpp
> index 4637a824af03..7475e6c1b609 100644
> --- a/src/ipa/rkisp1/algorithms/cproc.cpp
> +++ b/src/ipa/rkisp1/algorithms/cproc.cpp
> @@ -145,6 +145,18 @@ void ColorProcessing::prepare([[maybe_unused]] IPAContext &context,
>  	config->sat = frameContext.cproc.saturation.quantized();
>  }
>  
> +/**
> + * \copydoc libcamera::ipa::Algorithm::process
> + */
> +void ColorProcessing::process([[maybe_unused]] IPAContext &context, [[maybe_unused]] const uint32_t frame,
> +			      IPAFrameContext &frameContext, [[maybe_unused]] const rkisp1_stat_buffer *stats,
> +			      ControlList &metadata)

With line wraps,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +{
> +	metadata.set(controls::Brightness, frameContext.cproc.brightness.value());
> +	metadata.set(controls::Contrast, frameContext.cproc.contrast.value());
> +	metadata.set(controls::Saturation, frameContext.cproc.saturation.value());
> +}
> +
>  REGISTER_IPA_ALGORITHM(ColorProcessing, "ColorProcessing")
>  
>  } /* namespace ipa::rkisp1::algorithms */
> diff --git a/src/ipa/rkisp1/algorithms/cproc.h b/src/ipa/rkisp1/algorithms/cproc.h
> index fd38fd17e8bb..9b589ebd4ad7 100644
> --- a/src/ipa/rkisp1/algorithms/cproc.h
> +++ b/src/ipa/rkisp1/algorithms/cproc.h
> @@ -30,6 +30,10 @@ public:
>  	void prepare(IPAContext &context, const uint32_t frame,
>  		     IPAFrameContext &frameContext,
>  		     RkISP1Params *params) override;
> +	void process(IPAContext &context, const uint32_t frame,
> +		     IPAFrameContext &frameContext,
> +		     const rkisp1_stat_buffer *stats,
> +		     ControlList &metadata) override;
>  };
>  
>  } /* namespace ipa::rkisp1::algorithms */

Patch
diff mbox series 

diff --git a/src/ipa/rkisp1/algorithms/cproc.cpp b/src/ipa/rkisp1/algorithms/cproc.cpp
index 4637a824af03..7475e6c1b609 100644
--- a/src/ipa/rkisp1/algorithms/cproc.cpp
+++ b/src/ipa/rkisp1/algorithms/cproc.cpp
@@ -145,6 +145,18 @@  void ColorProcessing::prepare([[maybe_unused]] IPAContext &context,
 	config->sat = frameContext.cproc.saturation.quantized();
 }
 
+/**
+ * \copydoc libcamera::ipa::Algorithm::process
+ */
+void ColorProcessing::process([[maybe_unused]] IPAContext &context, [[maybe_unused]] const uint32_t frame,
+			      IPAFrameContext &frameContext, [[maybe_unused]] const rkisp1_stat_buffer *stats,
+			      ControlList &metadata)
+{
+	metadata.set(controls::Brightness, frameContext.cproc.brightness.value());
+	metadata.set(controls::Contrast, frameContext.cproc.contrast.value());
+	metadata.set(controls::Saturation, frameContext.cproc.saturation.value());
+}
+
 REGISTER_IPA_ALGORITHM(ColorProcessing, "ColorProcessing")
 
 } /* namespace ipa::rkisp1::algorithms */
diff --git a/src/ipa/rkisp1/algorithms/cproc.h b/src/ipa/rkisp1/algorithms/cproc.h
index fd38fd17e8bb..9b589ebd4ad7 100644
--- a/src/ipa/rkisp1/algorithms/cproc.h
+++ b/src/ipa/rkisp1/algorithms/cproc.h
@@ -30,6 +30,10 @@  public:
 	void prepare(IPAContext &context, const uint32_t frame,
 		     IPAFrameContext &frameContext,
 		     RkISP1Params *params) override;
+	void process(IPAContext &context, const uint32_t frame,
+		     IPAFrameContext &frameContext,
+		     const rkisp1_stat_buffer *stats,
+		     ControlList &metadata) override;
 };
 
 } /* namespace ipa::rkisp1::algorithms */