libcamera: matrix: Add read-only accessor to internal data
diff mbox series

Message ID 20250127064745.5740-1-laurent.pinchart@ideasonboard.com
State Superseded
Headers show
Series
  • libcamera: matrix: Add read-only accessor to internal data
Related show

Commit Message

Laurent Pinchart Jan. 27, 2025, 6:47 a.m. UTC
Add a data() function to the Matrix class to access the internal data.
This is useful for code that needs to use the matrix contents as a
linear array, as shows by the RkISP1::Ccm::process() function that needs
to copy the matrix data to a local variable. Simplify that function by
using the new accessor.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 include/libcamera/internal/matrix.h | 2 ++
 src/ipa/rkisp1/algorithms/ccm.cpp   | 7 +------
 src/libcamera/matrix.cpp            | 6 ++++++
 3 files changed, 9 insertions(+), 6 deletions(-)


base-commit: 8aef7b4dfb12f2f9bf0513625231b85a5b8f5440

Comments

Kieran Bingham Jan. 27, 2025, 10:49 a.m. UTC | #1
Quoting Laurent Pinchart (2025-01-27 06:47:45)
> Add a data() function to the Matrix class to access the internal data.
> This is useful for code that needs to use the matrix contents as a
> linear array, as shows by the RkISP1::Ccm::process() function that needs

s/shows/shown/

> to copy the matrix data to a local variable. Simplify that function by
> using the new accessor.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  include/libcamera/internal/matrix.h | 2 ++
>  src/ipa/rkisp1/algorithms/ccm.cpp   | 7 +------
>  src/libcamera/matrix.cpp            | 6 ++++++
>  3 files changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/include/libcamera/internal/matrix.h b/include/libcamera/internal/matrix.h
> index 7a71028c473a..a055e6926c94 100644
> --- a/include/libcamera/internal/matrix.h
> +++ b/include/libcamera/internal/matrix.h
> @@ -66,6 +66,8 @@ public:
>                 return out.str();
>         }
>  
> +       Span<const T, Rows * Cols> data() const { return data_; }
> +
>         Span<const T, Cols> operator[](size_t i) const
>         {
>                 return Span<const T, Cols>{ &data_.data()[i * Cols], Cols };
> diff --git a/src/ipa/rkisp1/algorithms/ccm.cpp b/src/ipa/rkisp1/algorithms/ccm.cpp
> index e2b5cf4d313e..eb8ca39e56a8 100644
> --- a/src/ipa/rkisp1/algorithms/ccm.cpp
> +++ b/src/ipa/rkisp1/algorithms/ccm.cpp
> @@ -120,12 +120,7 @@ void Ccm::process([[maybe_unused]] IPAContext &context,
>                   [[maybe_unused]] const rkisp1_stat_buffer *stats,
>                   ControlList &metadata)
>  {
> -       float m[9];
> -       for (unsigned int i = 0; i < 3; i++) {
> -               for (unsigned int j = 0; j < 3; j++)
> -                       m[i * 3 + j] = frameContext.ccm.ccm[i][j];
> -       }
> -       metadata.set(controls::ColourCorrectionMatrix, m);
> +       metadata.set(controls::ColourCorrectionMatrix, frameContext.ccm.ccm.data());

That's better than iterating 3x3.


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

>  }
>  
>  REGISTER_IPA_ALGORITHM(Ccm, "Ccm")
> diff --git a/src/libcamera/matrix.cpp b/src/libcamera/matrix.cpp
> index 4d95a19bfbb9..d9d34867b0a3 100644
> --- a/src/libcamera/matrix.cpp
> +++ b/src/libcamera/matrix.cpp
> @@ -52,6 +52,12 @@ LOG_DEFINE_CATEGORY(Matrix)
>   * \return A string describing the matrix
>   */
>  
> +/**
> + * \fn Matrix::data()
> + * \brief Access the internal data as a linear array
> + * \return A span referencing the internal data as a linear array
> + */
> +
>  /**
>   * \fn Span<const T, Cols> Matrix::operator[](size_t i) const
>   * \brief Index to a row in the matrix
> 
> base-commit: 8aef7b4dfb12f2f9bf0513625231b85a5b8f5440
> -- 
> Regards,
> 
> Laurent Pinchart
>
Milan Zamazal Jan. 27, 2025, 12:36 p.m. UTC | #2
Hi Laurent,

thank you for the patch.

Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:

> Add a data() function to the Matrix class to access the internal data.
> This is useful for code that needs to use the matrix contents as a
> linear array, as shows by the RkISP1::Ccm::process() function that needs
> to copy the matrix data to a local variable. Simplify that function by
> using the new accessor.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  include/libcamera/internal/matrix.h | 2 ++
>  src/ipa/rkisp1/algorithms/ccm.cpp   | 7 +------
>  src/libcamera/matrix.cpp            | 6 ++++++
>  3 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/include/libcamera/internal/matrix.h b/include/libcamera/internal/matrix.h
> index 7a71028c473a..a055e6926c94 100644
> --- a/include/libcamera/internal/matrix.h
> +++ b/include/libcamera/internal/matrix.h
> @@ -66,6 +66,8 @@ public:
>  		return out.str();
>  	}
>  
> +	Span<const T, Rows * Cols> data() const { return data_; }
> +
>  	Span<const T, Cols> operator[](size_t i) const
>  	{
>  		return Span<const T, Cols>{ &data_.data()[i * Cols], Cols };
> diff --git a/src/ipa/rkisp1/algorithms/ccm.cpp b/src/ipa/rkisp1/algorithms/ccm.cpp
> index e2b5cf4d313e..eb8ca39e56a8 100644
> --- a/src/ipa/rkisp1/algorithms/ccm.cpp
> +++ b/src/ipa/rkisp1/algorithms/ccm.cpp
> @@ -120,12 +120,7 @@ void Ccm::process([[maybe_unused]] IPAContext &context,
>  		  [[maybe_unused]] const rkisp1_stat_buffer *stats,
>  		  ControlList &metadata)
>  {
> -	float m[9];
> -	for (unsigned int i = 0; i < 3; i++) {
> -		for (unsigned int j = 0; j < 3; j++)
> -			m[i * 3 + j] = frameContext.ccm.ccm[i][j];
> -	}
> -	metadata.set(controls::ColourCorrectionMatrix, m);
> +	metadata.set(controls::ColourCorrectionMatrix, frameContext.ccm.ccm.data());
>  }
>  
>  REGISTER_IPA_ALGORITHM(Ccm, "Ccm")
> diff --git a/src/libcamera/matrix.cpp b/src/libcamera/matrix.cpp
> index 4d95a19bfbb9..d9d34867b0a3 100644
> --- a/src/libcamera/matrix.cpp
> +++ b/src/libcamera/matrix.cpp
> @@ -52,6 +52,12 @@ LOG_DEFINE_CATEGORY(Matrix)
>   * \return A string describing the matrix
>   */
>  
> +/**
> + * \fn Matrix::data()
> + * \brief Access the internal data as a linear array
> + * \return A span referencing the internal data as a linear array

"Internal data" can be anything and if it is supposed to be used in
places like metadata or for any "real" purpose, it should be better
specified.  Something like the documentation of `data' argument in the
constructor?

> + */
> +
>  /**
>   * \fn Span<const T, Cols> Matrix::operator[](size_t i) const
>   * \brief Index to a row in the matrix
>
> base-commit: 8aef7b4dfb12f2f9bf0513625231b85a5b8f5440

Patch
diff mbox series

diff --git a/include/libcamera/internal/matrix.h b/include/libcamera/internal/matrix.h
index 7a71028c473a..a055e6926c94 100644
--- a/include/libcamera/internal/matrix.h
+++ b/include/libcamera/internal/matrix.h
@@ -66,6 +66,8 @@  public:
 		return out.str();
 	}
 
+	Span<const T, Rows * Cols> data() const { return data_; }
+
 	Span<const T, Cols> operator[](size_t i) const
 	{
 		return Span<const T, Cols>{ &data_.data()[i * Cols], Cols };
diff --git a/src/ipa/rkisp1/algorithms/ccm.cpp b/src/ipa/rkisp1/algorithms/ccm.cpp
index e2b5cf4d313e..eb8ca39e56a8 100644
--- a/src/ipa/rkisp1/algorithms/ccm.cpp
+++ b/src/ipa/rkisp1/algorithms/ccm.cpp
@@ -120,12 +120,7 @@  void Ccm::process([[maybe_unused]] IPAContext &context,
 		  [[maybe_unused]] const rkisp1_stat_buffer *stats,
 		  ControlList &metadata)
 {
-	float m[9];
-	for (unsigned int i = 0; i < 3; i++) {
-		for (unsigned int j = 0; j < 3; j++)
-			m[i * 3 + j] = frameContext.ccm.ccm[i][j];
-	}
-	metadata.set(controls::ColourCorrectionMatrix, m);
+	metadata.set(controls::ColourCorrectionMatrix, frameContext.ccm.ccm.data());
 }
 
 REGISTER_IPA_ALGORITHM(Ccm, "Ccm")
diff --git a/src/libcamera/matrix.cpp b/src/libcamera/matrix.cpp
index 4d95a19bfbb9..d9d34867b0a3 100644
--- a/src/libcamera/matrix.cpp
+++ b/src/libcamera/matrix.cpp
@@ -52,6 +52,12 @@  LOG_DEFINE_CATEGORY(Matrix)
  * \return A string describing the matrix
  */
 
+/**
+ * \fn Matrix::data()
+ * \brief Access the internal data as a linear array
+ * \return A span referencing the internal data as a linear array
+ */
+
 /**
  * \fn Span<const T, Cols> Matrix::operator[](size_t i) const
  * \brief Index to a row in the matrix