[v2] debayer_egl: Use untransposed matrix to fix GLES 2.0
diff mbox series

Message ID 20260829142709.222649-1-robert.mader@collabora.com
State Accepted
Headers show
Series
  • [v2] debayer_egl: Use untransposed matrix to fix GLES 2.0
Related show

Commit Message

Robert Mader Aug. 29, 2026, 2:27 p.m. UTC 
The softISP curently fails on Mali-400 on Mesa. It turns out that
transposed matrices are not allowed by the spec:
> The UniformMatrix{234}fv commands will load count 2 × 2, 3 × 3, or 4 × 4
> matrices (corresponding to 2, 3, or 4 in the command name) of floating-point values
> into a uniform location defined as a matrix or an array of matrices. The matrix is
> specified in column-major order. transpose must be FALSE.

Thus partially revert the commit mentioned below and manually transpose
the matrix.

Fixes: d780e285b593 ("shaders: bayer: Use native matrix multiplication")
Closes: https://gitlab.freedesktop.org/camera/libcamera/-/work_items/342
Signed-off-by: Robert Mader <robert.mader@collabora.com>

---

Changes in v2:
 - Actually transpose the matrix to not re-introduce issues with CCMs
 - Add comment about GLES 2.0
 - Add "Closes:" link
---
 src/libcamera/software_isp/debayer_egl.cpp | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

Comments

Barnabás Pőcze Aug. 31, 2026, 8:35 a.m. UTC | #1 
2026. 08. 29. 16:27 keltezéssel, Robert Mader írta:
> The softISP curently fails on Mali-400 on Mesa. It turns out that
> transposed matrices are not allowed by the spec:
>> The UniformMatrix{234}fv commands will load count 2 × 2, 3 × 3, or 4 × 4
>> matrices (corresponding to 2, 3, or 4 in the command name) of floating-point values
>> into a uniform location defined as a matrix or an array of matrices. The matrix is
>> specified in column-major order. transpose must be FALSE.
> 
> Thus partially revert the commit mentioned below and manually transpose
> the matrix.
> 
> Fixes: d780e285b593 ("shaders: bayer: Use native matrix multiplication")
> Closes: https://gitlab.freedesktop.org/camera/libcamera/-/work_items/342
> Signed-off-by: Robert Mader <robert.mader@collabora.com>
> 
> ---

On which device(s) have you tested this? The egl debayering now runs on the
pinephone, but the debayered image looks horrible (pink bands everywhere).

But I don't see how this can be the cause, so

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


> 
> Changes in v2:
>   - Actually transpose the matrix to not re-introduce issues with CCMs
>   - Add comment about GLES 2.0
>   - Add "Closes:" link
> ---
>   src/libcamera/software_isp/debayer_egl.cpp | 17 +++++++++++++++--
>   1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libcamera/software_isp/debayer_egl.cpp b/src/libcamera/software_isp/debayer_egl.cpp
> index 97aa03793574..6df1ce1a8fb6 100644
> --- a/src/libcamera/software_isp/debayer_egl.cpp
> +++ b/src/libcamera/software_isp/debayer_egl.cpp
> @@ -460,8 +460,21 @@ void DebayerEGL::setShaderVariableValues(eGLImage &eglImageIn, const DebayerPara
>   			    << " textureUniformStep_.y " << Step[1]
>   			    << " textureUniformStrideFactor_ " << Stride
>   			    << " textureUniformProjMatrix_ " << textureUniformProjMatrix_;
> -
> -	glUniformMatrix3fv(ccmUniformDataIn_, 1, GL_TRUE, params.combinedMatrix.data().data());
> +	/*
> +	 * Pre-transpose matrix for GLES 2.0
> +	 */
> +	GLfloat ccm[9] = {
> +		params.combinedMatrix[0][0],
> +		params.combinedMatrix[1][0],
> +		params.combinedMatrix[2][0],
> +		params.combinedMatrix[0][1],
> +		params.combinedMatrix[1][1],
> +		params.combinedMatrix[2][1],
> +		params.combinedMatrix[0][2],
> +		params.combinedMatrix[1][2],
> +		params.combinedMatrix[2][2],
> +	};
> +	glUniformMatrix3fv(ccmUniformDataIn_, 1, GL_FALSE, ccm);
>   	LOG(Debayer, Debug) << " ccmUniformDataIn_ " << ccmUniformDataIn_ << " data " << params.combinedMatrix;
>   
>   	/*
Bryan O'Donoghue Aug. 31, 2026, 9:55 a.m. UTC | #2 
On 29/08/2026 15:27, Robert Mader wrote:
> The softISP curently fails on Mali-400 on Mesa. It turns out that
> transposed matrices are not allowed by the spec:
>> The UniformMatrix{234}fv commands will load count 2 × 2, 3 × 3, or 4 × 4
>> matrices (corresponding to 2, 3, or 4 in the command name) of floating-point values
>> into a uniform location defined as a matrix or an array of matrices. The matrix is
>> specified in column-major order. transpose must be FALSE.
> 
> Thus partially revert the commit mentioned below and manually transpose
> the matrix.
> 
> Fixes: d780e285b593 ("shaders: bayer: Use native matrix multiplication")
> Closes: https://gitlab.freedesktop.org/camera/libcamera/-/work_items/342
> Signed-off-by: Robert Mader <robert.mader@collabora.com>
> 
> ---
> 
> Changes in v2:
>   - Actually transpose the matrix to not re-introduce issues with CCMs
>   - Add comment about GLES 2.0
>   - Add "Closes:" link
> ---
>   src/libcamera/software_isp/debayer_egl.cpp | 17 +++++++++++++++--
>   1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libcamera/software_isp/debayer_egl.cpp b/src/libcamera/software_isp/debayer_egl.cpp
> index 97aa03793574..6df1ce1a8fb6 100644
> --- a/src/libcamera/software_isp/debayer_egl.cpp
> +++ b/src/libcamera/software_isp/debayer_egl.cpp
> @@ -460,8 +460,21 @@ void DebayerEGL::setShaderVariableValues(eGLImage &eglImageIn, const DebayerPara
>   			    << " textureUniformStep_.y " << Step[1]
>   			    << " textureUniformStrideFactor_ " << Stride
>   			    << " textureUniformProjMatrix_ " << textureUniformProjMatrix_;
> -
> -	glUniformMatrix3fv(ccmUniformDataIn_, 1, GL_TRUE, params.combinedMatrix.data().data());
> +	/*
> +	 * Pre-transpose matrix for GLES 2.0
> +	 */
> +	GLfloat ccm[9] = {
> +		params.combinedMatrix[0][0],
> +		params.combinedMatrix[1][0],
> +		params.combinedMatrix[2][0],
> +		params.combinedMatrix[0][1],
> +		params.combinedMatrix[1][1],
> +		params.combinedMatrix[2][1],
> +		params.combinedMatrix[0][2],
> +		params.combinedMatrix[1][2],
> +		params.combinedMatrix[2][2],
> +	};
> +	glUniformMatrix3fv(ccmUniformDataIn_, 1, GL_FALSE, ccm);
>   	LOG(Debayer, Debug) << " ccmUniformDataIn_ " << ccmUniformDataIn_ << " data " << params.combinedMatrix;
> 
>   	/*
> --
> 2.55.0
> 
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Milan Zamazal Aug. 31, 2026, 10:25 a.m. UTC | #3 
Robert Mader <robert.mader@collabora.com> writes:

> The softISP curently fails on Mali-400 on Mesa. It turns out that
> transposed matrices are not allowed by the spec:
>> The UniformMatrix{234}fv commands will load count 2 × 2, 3 × 3, or 4 × 4
>> matrices (corresponding to 2, 3, or 4 in the command name) of floating-point values
>> into a uniform location defined as a matrix or an array of matrices. The matrix is
>> specified in column-major order. transpose must be FALSE.
>
> Thus partially revert the commit mentioned below and manually transpose
> the matrix.
>
> Fixes: d780e285b593 ("shaders: bayer: Use native matrix multiplication")
> Closes: https://gitlab.freedesktop.org/camera/libcamera/-/work_items/342
> Signed-off-by: Robert Mader <robert.mader@collabora.com>

Reviewed-by: Milan Zamazal <mzamazal@redhat.com>

>
> ---
>
> Changes in v2:
>  - Actually transpose the matrix to not re-introduce issues with CCMs
>  - Add comment about GLES 2.0
>  - Add "Closes:" link
> ---
>  src/libcamera/software_isp/debayer_egl.cpp | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/src/libcamera/software_isp/debayer_egl.cpp b/src/libcamera/software_isp/debayer_egl.cpp
> index 97aa03793574..6df1ce1a8fb6 100644
> --- a/src/libcamera/software_isp/debayer_egl.cpp
> +++ b/src/libcamera/software_isp/debayer_egl.cpp
> @@ -460,8 +460,21 @@ void DebayerEGL::setShaderVariableValues(eGLImage &eglImageIn, const DebayerPara
>  			    << " textureUniformStep_.y " << Step[1]
>  			    << " textureUniformStrideFactor_ " << Stride
>  			    << " textureUniformProjMatrix_ " << textureUniformProjMatrix_;
> -
> -	glUniformMatrix3fv(ccmUniformDataIn_, 1, GL_TRUE, params.combinedMatrix.data().data());
> +	/*
> +	 * Pre-transpose matrix for GLES 2.0
> +	 */
> +	GLfloat ccm[9] = {
> +		params.combinedMatrix[0][0],
> +		params.combinedMatrix[1][0],
> +		params.combinedMatrix[2][0],
> +		params.combinedMatrix[0][1],
> +		params.combinedMatrix[1][1],
> +		params.combinedMatrix[2][1],
> +		params.combinedMatrix[0][2],
> +		params.combinedMatrix[1][2],
> +		params.combinedMatrix[2][2],
> +	};
> +	glUniformMatrix3fv(ccmUniformDataIn_, 1, GL_FALSE, ccm);
>  	LOG(Debayer, Debug) << " ccmUniformDataIn_ " << ccmUniformDataIn_ << " data " << params.combinedMatrix;
>  
>  	/*
Robert Mader Aug. 31, 2026, 10:39 a.m. UTC | #4 
Hi,

On 31.08.26 10:35, Barnabás Pőcze wrote:
> 2026. 08. 29. 16:27 keltezéssel, Robert Mader írta:
>> The softISP curently fails on Mali-400 on Mesa. It turns out that
>> transposed matrices are not allowed by the spec:
>>> The UniformMatrix{234}fv commands will load count 2 × 2, 3 × 3, or 4 
>>> × 4
>>> matrices (corresponding to 2, 3, or 4 in the command name) of 
>>> floating-point values
>>> into a uniform location defined as a matrix or an array of matrices. 
>>> The matrix is
>>> specified in column-major order. transpose must be FALSE.
>>
>> Thus partially revert the commit mentioned below and manually transpose
>> the matrix.
>>
>> Fixes: d780e285b593 ("shaders: bayer: Use native matrix multiplication")
>> Closes: https://gitlab.freedesktop.org/camera/libcamera/-/work_items/342
>> Signed-off-by: Robert Mader <robert.mader@collabora.com>
>>
>> ---
>
> On which device(s) have you tested this? The egl debayering now runs 
> on the
> pinephone, but the debayered image looks horrible (pink bands 
> everywhere).
On the PinePhone - also seeing glitches in a bunch of cases, however 
IIRC that's always been the case with DebayerEGL on that device - and 
the librem5 where I don't see any glitches (and managed to test a CCM 
for the first time).
>
> But I don't see how this can be the cause, so
>
> Tested-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> # pinephone
> Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>
>
>>
>> Changes in v2:
>>   - Actually transpose the matrix to not re-introduce issues with CCMs
>>   - Add comment about GLES 2.0
>>   - Add "Closes:" link
>> ---
>>   src/libcamera/software_isp/debayer_egl.cpp | 17 +++++++++++++++--
>>   1 file changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/libcamera/software_isp/debayer_egl.cpp 
>> b/src/libcamera/software_isp/debayer_egl.cpp
>> index 97aa03793574..6df1ce1a8fb6 100644
>> --- a/src/libcamera/software_isp/debayer_egl.cpp
>> +++ b/src/libcamera/software_isp/debayer_egl.cpp
>> @@ -460,8 +460,21 @@ void 
>> DebayerEGL::setShaderVariableValues(eGLImage &eglImageIn, const 
>> DebayerPara
>>                   << " textureUniformStep_.y " << Step[1]
>>                   << " textureUniformStrideFactor_ " << Stride
>>                   << " textureUniformProjMatrix_ " << 
>> textureUniformProjMatrix_;
>> -
>> -    glUniformMatrix3fv(ccmUniformDataIn_, 1, GL_TRUE, 
>> params.combinedMatrix.data().data());
>> +    /*
>> +     * Pre-transpose matrix for GLES 2.0
>> +     */
>> +    GLfloat ccm[9] = {
>> +        params.combinedMatrix[0][0],
>> +        params.combinedMatrix[1][0],
>> +        params.combinedMatrix[2][0],
>> +        params.combinedMatrix[0][1],
>> +        params.combinedMatrix[1][1],
>> +        params.combinedMatrix[2][1],
>> +        params.combinedMatrix[0][2],
>> +        params.combinedMatrix[1][2],
>> +        params.combinedMatrix[2][2],
>> +    };
>> +    glUniformMatrix3fv(ccmUniformDataIn_, 1, GL_FALSE, ccm);
>>       LOG(Debayer, Debug) << " ccmUniformDataIn_ " << 
>> ccmUniformDataIn_ << " data " << params.combinedMatrix;
>>         /*
>

Patch
diff mbox series 

diff --git a/src/libcamera/software_isp/debayer_egl.cpp b/src/libcamera/software_isp/debayer_egl.cpp
index 97aa03793574..6df1ce1a8fb6 100644
--- a/src/libcamera/software_isp/debayer_egl.cpp
+++ b/src/libcamera/software_isp/debayer_egl.cpp
@@ -460,8 +460,21 @@  void DebayerEGL::setShaderVariableValues(eGLImage &eglImageIn, const DebayerPara
 			    << " textureUniformStep_.y " << Step[1]
 			    << " textureUniformStrideFactor_ " << Stride
 			    << " textureUniformProjMatrix_ " << textureUniformProjMatrix_;
-
-	glUniformMatrix3fv(ccmUniformDataIn_, 1, GL_TRUE, params.combinedMatrix.data().data());
+	/*
+	 * Pre-transpose matrix for GLES 2.0
+	 */
+	GLfloat ccm[9] = {
+		params.combinedMatrix[0][0],
+		params.combinedMatrix[1][0],
+		params.combinedMatrix[2][0],
+		params.combinedMatrix[0][1],
+		params.combinedMatrix[1][1],
+		params.combinedMatrix[2][1],
+		params.combinedMatrix[0][2],
+		params.combinedMatrix[1][2],
+		params.combinedMatrix[2][2],
+	};
+	glUniformMatrix3fv(ccmUniformDataIn_, 1, GL_FALSE, ccm);
 	LOG(Debayer, Debug) << " ccmUniformDataIn_ " << ccmUniformDataIn_ << " data " << params.combinedMatrix;
 
 	/*