[v14,2/6] libcamera: software_isp: Add LSC data to DebayerParams
diff mbox series

Message ID 20260821125958.95928-3-mzamazal@redhat.com
State Superseded
Headers show
Series
  • LSC for SoftISP simple pipeline
Related show

Commit Message

Milan Zamazal Aug. 21, 2026, 12:59 p.m. UTC 
From: Xander Pronk <xander.c.pronk@gmail.com>

Add a lookup table for grid-based lens shading correction to debayer
params.  The lookup table is in the format of an RGBA texture to be
passed to debayering.  The alpha value serves only for padding and can
be arbitrary; OpenGL wants by default rows aligned to 4 bytes and using
RGBA format is the simplest way to achieve it.

The parameter is filled in by Lsc algorithm implemented in a follow-up
patch.  If the algorithm is not enabled, the table is filled in with
values corresponding to identity multiplication (1.0).

In order the image processing would be able to update its internal
data (e.g. the GPU texture) on and only on the lookup table changes,
lscLutVersion parameter is added, which is incremented on each lookup
table change.

The parameters are currently unused, their handling is implemented in
follow-up patches.

The grid size is selected to be the same as in the tuning files already
present in rkisp1.  Another popular grid size in other pipelines is 32.

Co-developed-by: Rick ten Wolde <rick_libcamera@wolde.info>
Signed-off-by: Rick ten Wolde <rick_libcamera@wolde.info>
Signed-off-by: Xander Pronk <xander.c.pronk@gmail.com>
Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
---
 .../internal/software_isp/debayer_params.h    | 12 ++++++++
 src/libcamera/software_isp/debayer.cpp        | 28 +++++++++++++++++++
 2 files changed, 40 insertions(+)

Comments

Barnabás Pőcze Aug. 25, 2026, 11:18 a.m. UTC | #1 
2026. 08. 21. 14:59 keltezéssel, Milan Zamazal írta:
> From: Xander Pronk <xander.c.pronk@gmail.com>
> 
> Add a lookup table for grid-based lens shading correction to debayer
> params.  The lookup table is in the format of an RGBA texture to be
> passed to debayering.  The alpha value serves only for padding and can
> be arbitrary; OpenGL wants by default rows aligned to 4 bytes and using
> RGBA format is the simplest way to achieve it.
> 
> The parameter is filled in by Lsc algorithm implemented in a follow-up
> patch.  If the algorithm is not enabled, the table is filled in with
> values corresponding to identity multiplication (1.0).
> 
> In order the image processing would be able to update its internal
> data (e.g. the GPU texture) on and only on the lookup table changes,
> lscLutVersion parameter is added, which is incremented on each lookup
> table change.
> 
> The parameters are currently unused, their handling is implemented in
> follow-up patches.
> 
> The grid size is selected to be the same as in the tuning files already
> present in rkisp1.  Another popular grid size in other pipelines is 32.
> 
> Co-developed-by: Rick ten Wolde <rick_libcamera@wolde.info>
> Signed-off-by: Rick ten Wolde <rick_libcamera@wolde.info>
> Signed-off-by: Xander Pronk <xander.c.pronk@gmail.com>
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> ---
>   .../internal/software_isp/debayer_params.h    | 12 ++++++++
>   src/libcamera/software_isp/debayer.cpp        | 28 +++++++++++++++++++
>   2 files changed, 40 insertions(+)
> 
> diff --git a/include/libcamera/internal/software_isp/debayer_params.h b/include/libcamera/internal/software_isp/debayer_params.h
> index 1074720d7..93dcf42e6 100644
> --- a/include/libcamera/internal/software_isp/debayer_params.h
> +++ b/include/libcamera/internal/software_isp/debayer_params.h
> @@ -10,6 +10,7 @@
>   
>   #pragma once
>   
> +#include <array>
>   #include <stdint.h>
>   
>   #include "libcamera/internal/matrix.h"
> @@ -25,6 +26,17 @@ struct DebayerParams {
>   	float gamma = 1.0;
>   	float contrastExp = 1.0;
>   	RGB<double> gains = RGB<double>({ 1.0, 1.0, 1.0 });
> +
> +	/**
> +	 * To prevent OpenGL alignment issues, the number of bytes in each row
> +	 * should be a multiple of 4.
> +	 **/

I'm not sure that's sufficient. I'm assuming this is talking about `GL_UNPACK_ALIGNMENT`,
whose initial value is 4. But there is nothing enforcing that `lscLut` is aligned to 4
bytes (and the parameter can be as high as 8).

So `lscLut` either needs an explicit `alignas(4)`, or `glPixelStorei(GL_UNPACK_ALIGNMENT, 1)`
possibly needs to be used (not sure about the performance implications).


> +	static constexpr unsigned int kLscGridSize = 17;
> +	static constexpr unsigned int kLscValuesPerCell = 4;
> +	using LscLookupTable =
> +		std::array<uint8_t, kLscGridSize * kLscGridSize * kLscValuesPerCell>;
> +	LscLookupTable lscLut{};
> +	uint64_t lscLutVersion = 0;
>   };
>   
>   } /* namespace libcamera */
> diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp
> index a4854e51b..82cdf1dc2 100644
> --- a/src/libcamera/software_isp/debayer.cpp
> +++ b/src/libcamera/software_isp/debayer.cpp
> @@ -43,6 +43,34 @@ namespace libcamera {
>    * \brief Contrast value to be used as an exponent
>    */
>   
> +/**
> + * \var DebayerParams::kLscGridSize
> + * \brief Number of lens shading grid areas in one direction
> + */
> +
> +/**
> + * \var DebayerParams::kLscValuesPerCell
> + * \brief Number of pixel values per each of the lens shading grid areas
> + */
> +
> +/**
> + * \typedef DebayerParams::LscLookupTable
> + * \brief Lookup table for lens shading correction
> + *
> + * It's an array of values to be later used as a texture.
> + * The values are in row - column - RGB order.
> + */
> +
> +/**
> + * \var DebayerParams::lscLut
> + * \brief Lens shading lookup table
> + */
> +
> +/**
> + * \var DebayerParams::lscLutVersion
> + * \brief Incremented on each \a lscLut change
> + */
> +
>   /**
>    * \class Debayer
>    * \brief Base debayering class
Milan Zamazal Aug. 25, 2026, 2:50 p.m. UTC | #2 
Hi Barnabás,

thank you for reviews.

Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes:

> 2026. 08. 21. 14:59 keltezéssel, Milan Zamazal írta:
>> From: Xander Pronk <xander.c.pronk@gmail.com>
>> Add a lookup table for grid-based lens shading correction to debayer
>> params.  The lookup table is in the format of an RGBA texture to be
>> passed to debayering.  The alpha value serves only for padding and can
>> be arbitrary; OpenGL wants by default rows aligned to 4 bytes and using
>> RGBA format is the simplest way to achieve it.
>> The parameter is filled in by Lsc algorithm implemented in a follow-up
>> patch.  If the algorithm is not enabled, the table is filled in with
>> values corresponding to identity multiplication (1.0).
>> In order the image processing would be able to update its internal
>> data (e.g. the GPU texture) on and only on the lookup table changes,
>> lscLutVersion parameter is added, which is incremented on each lookup
>> table change.
>> The parameters are currently unused, their handling is implemented in
>> follow-up patches.
>> The grid size is selected to be the same as in the tuning files already
>> present in rkisp1.  Another popular grid size in other pipelines is 32.
>> Co-developed-by: Rick ten Wolde <rick_libcamera@wolde.info>
>> Signed-off-by: Rick ten Wolde <rick_libcamera@wolde.info>
>> Signed-off-by: Xander Pronk <xander.c.pronk@gmail.com>
>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
>> ---
>>   .../internal/software_isp/debayer_params.h    | 12 ++++++++
>>   src/libcamera/software_isp/debayer.cpp        | 28 +++++++++++++++++++
>>   2 files changed, 40 insertions(+)
>> diff --git a/include/libcamera/internal/software_isp/debayer_params.h b/include/libcamera/internal/software_isp/debayer_params.h
>> index 1074720d7..93dcf42e6 100644
>> --- a/include/libcamera/internal/software_isp/debayer_params.h
>> +++ b/include/libcamera/internal/software_isp/debayer_params.h
>> @@ -10,6 +10,7 @@
>>     #pragma once
>>   +#include <array>
>>   #include <stdint.h>
>>     #include "libcamera/internal/matrix.h"
>> @@ -25,6 +26,17 @@ struct DebayerParams {
>>   	float gamma = 1.0;
>>   	float contrastExp = 1.0;
>>   	RGB<double> gains = RGB<double>({ 1.0, 1.0, 1.0 });
>> +
>> +	/**
>> +	 * To prevent OpenGL alignment issues, the number of bytes in each row
>> +	 * should be a multiple of 4.
>> +	 **/
>
> I'm not sure that's sufficient. I'm assuming this is talking about `GL_UNPACK_ALIGNMENT`,
> whose initial value is 4. But there is nothing enforcing that `lscLut` is aligned to 4
> bytes (and the parameter can be as high as 8).

We don't change GL_UNPACK_ALIGNMENT so I suppose it's OK to assume it's
4?  It's not clear to me whether lscLut must be aligned too but it
doesn't harm to align it, so I add alignas(4) as you suggest.

> So `lscLut` either needs an explicit `alignas(4)`, or `glPixelStorei(GL_UNPACK_ALIGNMENT, 1)`
> possibly needs to be used (not sure about the performance implications).

I'd say let's use the default alignment, it's not a big issue.

>
>> +	static constexpr unsigned int kLscGridSize = 17;
>> +	static constexpr unsigned int kLscValuesPerCell = 4;
>> +	using LscLookupTable =
>> +		std::array<uint8_t, kLscGridSize * kLscGridSize * kLscValuesPerCell>;
>> +	LscLookupTable lscLut{};
>> +	uint64_t lscLutVersion = 0;
>>   };
>>     } /* namespace libcamera */
>> diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp
>> index a4854e51b..82cdf1dc2 100644
>> --- a/src/libcamera/software_isp/debayer.cpp
>> +++ b/src/libcamera/software_isp/debayer.cpp
>> @@ -43,6 +43,34 @@ namespace libcamera {
>>    * \brief Contrast value to be used as an exponent
>>    */
>>   +/**
>> + * \var DebayerParams::kLscGridSize
>> + * \brief Number of lens shading grid areas in one direction
>> + */
>> +
>> +/**
>> + * \var DebayerParams::kLscValuesPerCell
>> + * \brief Number of pixel values per each of the lens shading grid areas
>> + */
>> +
>> +/**
>> + * \typedef DebayerParams::LscLookupTable
>> + * \brief Lookup table for lens shading correction
>> + *
>> + * It's an array of values to be later used as a texture.
>> + * The values are in row - column - RGB order.
>> + */
>> +
>> +/**
>> + * \var DebayerParams::lscLut
>> + * \brief Lens shading lookup table
>> + */
>> +
>> +/**
>> + * \var DebayerParams::lscLutVersion
>> + * \brief Incremented on each \a lscLut change
>> + */
>> +
>>   /**
>>    * \class Debayer
>>    * \brief Base debayering class

Patch
diff mbox series 

diff --git a/include/libcamera/internal/software_isp/debayer_params.h b/include/libcamera/internal/software_isp/debayer_params.h
index 1074720d7..93dcf42e6 100644
--- a/include/libcamera/internal/software_isp/debayer_params.h
+++ b/include/libcamera/internal/software_isp/debayer_params.h
@@ -10,6 +10,7 @@ 
 
 #pragma once
 
+#include <array>
 #include <stdint.h>
 
 #include "libcamera/internal/matrix.h"
@@ -25,6 +26,17 @@  struct DebayerParams {
 	float gamma = 1.0;
 	float contrastExp = 1.0;
 	RGB<double> gains = RGB<double>({ 1.0, 1.0, 1.0 });
+
+	/**
+	 * To prevent OpenGL alignment issues, the number of bytes in each row
+	 * should be a multiple of 4.
+	 **/
+	static constexpr unsigned int kLscGridSize = 17;
+	static constexpr unsigned int kLscValuesPerCell = 4;
+	using LscLookupTable =
+		std::array<uint8_t, kLscGridSize * kLscGridSize * kLscValuesPerCell>;
+	LscLookupTable lscLut{};
+	uint64_t lscLutVersion = 0;
 };
 
 } /* namespace libcamera */
diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp
index a4854e51b..82cdf1dc2 100644
--- a/src/libcamera/software_isp/debayer.cpp
+++ b/src/libcamera/software_isp/debayer.cpp
@@ -43,6 +43,34 @@  namespace libcamera {
  * \brief Contrast value to be used as an exponent
  */
 
+/**
+ * \var DebayerParams::kLscGridSize
+ * \brief Number of lens shading grid areas in one direction
+ */
+
+/**
+ * \var DebayerParams::kLscValuesPerCell
+ * \brief Number of pixel values per each of the lens shading grid areas
+ */
+
+/**
+ * \typedef DebayerParams::LscLookupTable
+ * \brief Lookup table for lens shading correction
+ *
+ * It's an array of values to be later used as a texture.
+ * The values are in row - column - RGB order.
+ */
+
+/**
+ * \var DebayerParams::lscLut
+ * \brief Lens shading lookup table
+ */
+
+/**
+ * \var DebayerParams::lscLutVersion
+ * \brief Incremented on each \a lscLut change
+ */
+
 /**
  * \class Debayer
  * \brief Base debayering class