| Message ID | 20260830115120.255815-2-christian@themurphys.eu |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
On 30/08/2026 12:51, Christian Murphy wrote: > The bayer_unpacked fragment shader derives the Bayer parity of the > sampled pixel from one interpolated varying, in pixel units, while the > texture unit selects the nearest texel from another, in texture units. The commas in this stence make it stuttering and difficult to parse. > At native size every sample lands on a texel centre and the two agree. > When the render target is smaller than the source, as in the software > ISP GPU debayer and qcam's scaled viewfinder, sample positions land on > texel boundaries on a periodic subset of output rows and columns, and > rounding differences make the parity and the fetched texel disagree by > one. Such rows debayer with the wrong row parity, raising R and B over > G: regular magenta lines. > > On an imx471 (1928x1088 SRGGB10 through the Intel IPU7 ISYS, debayered > by Mesa 26.1 on Intel Arc B390 graphics) scaled to 640x480, output row > y samples source row (2y + 1) * 17 / 15, an exact texel boundary for > y = 7 (mod 15). Captures showed one magenta line every 15 output rows, > at those offsets. > > Select the source pixel with a single floor() in pixel space, as > bayer_1x_packed.frag already does, and derive the parity, the centre > fetch and the neighbour fetches from that one index, snapped to texel > centres. The two computations can then no longer disagree. The vertex > shader now only forwards the source position in pixels; the center, > xCoord and yCoord varyings and tex_step are dropped from the unpacked > programs, and stride_factor and tex_bayer_first_red move to the > fragment shader. DebayerEGL and qcam already set those uniforms, so no > C++ changes are needed beyond a comment. At native size the sampled > texels are unchanged. While there's a lot of detail in this, the main thing being done is moving the logic from the reading phase in the vertex shader to the writing phase in the fragment shader. That simple statement should appear at the top of a patch before a long explaination of technical detail. i.e. you shouldn't have to read a patch three times over to understand what it does, the commit log should make that plain from the outset. What is the justification to move from read to write ? > Measured with the mean of R + B - 2G per output row, in 8-bit output > levels: at 640x480 the mean excess of the y = 7 (mod 15) rows drops > from +13.1 (about 5% of full scale), with all other phases within > +/-0.05, to 0.00, within the spread of the other phases. 480x320, > 640x360, 1280x720 and the native output size of 1924x1088 stay clean > and the field of view is unchanged. > > Assisted by Claude Code (claude-fable-5). Asssited-by or Written-by, libcamera has a !AI policy for submissions. The commit log has lots of detail but is also simultaneously difficult to read. Clear non-rambling statements are what commit logs are for, code can be as detailed as necessary. For me it would be better if using an LLM to have to show with an example of how two given inputs produce a right and a wrong answer, stepping through each variable's value and its index into the source and how the new algorithm remediates it by shifting to the destination for the previous input examples. > > Signed-off-by: Christian Murphy <christian@themurphys.eu> > --- > src/libcamera/shaders/bayer_unpacked.frag | 21 ++++++++++++---- > src/libcamera/shaders/bayer_unpacked.vert | 28 +++------------------- > src/libcamera/software_isp/debayer_egl.cpp | 10 ++++---- > 3 files changed, 25 insertions(+), 34 deletions(-) > > diff --git a/src/libcamera/shaders/bayer_unpacked.frag b/src/libcamera/shaders/bayer_unpacked.frag > index 10c5e941b..455d1fe78 100644 > --- a/src/libcamera/shaders/bayer_unpacked.frag > +++ b/src/libcamera/shaders/bayer_unpacked.frag > @@ -21,9 +21,10 @@ precision highp float; > > /** Monochrome RGBA or GL_LUMINANCE Bayer encoded texture.*/ > uniform sampler2D tex_y; > -varying vec4 center; > -varying vec4 yCoord; > -varying vec4 xCoord; > +uniform vec2 tex_size; > +uniform float stride_factor; > +uniform vec2 tex_bayer_first_red; > +varying vec2 pixelPos; > uniform vec3 awb; > uniform mat3 ccm; > uniform vec3 blacklevel; > @@ -52,11 +53,23 @@ void main(void) { > #define fetch(x, y) texture2D(tex_y, vec2(x, y)).r > #endif > > + /* > + * Derive the parity and every fetch coordinate from one floor() of the > + * pixel position, snapped to texel centres. Computed separately they > + * can round to different texels when downscaling lands a sample on a > + * texel boundary. > + */ > + vec2 texStep = vec2(stride_factor / tex_size.x, 1.0 / tex_size.y); > + vec2 pix = floor(pixelPos); > + vec2 center = (pix + 0.5) * texStep; Why + 0.5 ? > + vec4 xCoord = center.x + vec4(-2.0, -1.0, 1.0, 2.0) * texStep.x; > + vec4 yCoord = center.y + vec4(-2.0, -1.0, 1.0, 2.0) * texStep.y; > + > float C = fetch(center.x, center.y); // ( 0, 0) > const vec4 kC = vec4( 4.0, 6.0, 5.0, 5.0) / 8.0; > > // Determine which of four types of pixels we are on. > - vec2 alternate = mod(floor(center.zw), 2.0); > + vec2 alternate = mod(pix + tex_bayer_first_red, 2.0); > > vec4 Dvec = vec4( > fetch(xCoord[1], yCoord[1]), // (-1,-1) > diff --git a/src/libcamera/shaders/bayer_unpacked.vert b/src/libcamera/shaders/bayer_unpacked.vert > index 423dde0fa..be1268b62 100644 > --- a/src/libcamera/shaders/bayer_unpacked.vert > +++ b/src/libcamera/shaders/bayer_unpacked.vert > @@ -22,34 +22,12 @@ attribute vec2 textureIn; > uniform mat4 proj_matrix; > > uniform vec2 tex_size; /* The texture size in pixels */ > -uniform vec2 tex_step; > > -/** Pixel position of the first red pixel in the */ > -/** Bayer pattern. [{0,1}, {0, 1}]*/ > -uniform vec2 tex_bayer_first_red; > - > -/** .xy = Pixel being sampled in the fragment shader on the range [0, 1] > - .zw = ...on the range [0, sourceSize], offset by firstRed */ > -varying vec4 center; > - > -/** center.x + (-2/w, -1/w, 1/w, 2/w); These are the x-positions */ > -/** of the adjacent pixels.*/ > -varying vec4 xCoord; > - > -/** center.y + (-2/h, -1/h, 1/h, 2/h); These are the y-positions */ > -/** of the adjacent pixels.*/ > -varying vec4 yCoord; > - > -uniform float stride_factor; > +/** Position of the pixel being sampled, in image pixels. */ > +varying vec2 pixelPos; > > void main(void) { > - center.xy = vec2(textureIn.x * stride_factor, textureIn.y); > - center.zw = textureIn * tex_size + tex_bayer_first_red; > - > - xCoord = center.x + vec4(-2.0 * tex_step.x, > - -tex_step.x, tex_step.x, 2.0 * tex_step.x); > - yCoord = center.y + vec4(-2.0 * tex_step.y, > - -tex_step.y, tex_step.y, 2.0 * tex_step.y); > + pixelPos = textureIn * tex_size; > > gl_Position = proj_matrix * vertexIn; > } Difficult to evaluate a change as complex as this. I'd appreciate a walk-through of existing indexing working/not-working and new indexing so that I can get a picture in my head of what is wrong and how this fixes, also then allowing us to understand if moving from sample to write is the appropriate change. > diff --git a/src/libcamera/software_isp/debayer_egl.cpp b/src/libcamera/software_isp/debayer_egl.cpp > index 97aa03793..6db07c284 100644 > --- a/src/libcamera/software_isp/debayer_egl.cpp > +++ b/src/libcamera/software_isp/debayer_egl.cpp > @@ -432,11 +432,11 @@ void DebayerEGL::setShaderVariableValues(eGLImage &eglImageIn, const DebayerPara > > /* > * These values are: > - * firstRed = tex_bayer_first_red - bayer_8.vert > - * imgSize = tex_size - bayer_8.vert > - * step = tex_step - bayer_8.vert > - * Stride = stride_factor identity.vert > - * textureUniformProjMatri = No scaling > + * firstRed = tex_bayer_first_red - bayer_unpacked.frag, bayer_1x_packed.frag > + * imgSize = tex_size - bayer_unpacked.vert, bayer_unpacked.frag, bayer_1x_packed.frag > + * step = tex_step - bayer_1x_packed.frag > + * Stride = stride_factor - identity.vert, bayer_unpacked.frag > + * projMatrix = proj_matrix - identity.vert, bayer_unpacked.vert > */ > glUniform2fv(textureUniformBayerFirstRed_, 1, firstRed); > glUniform2fv(textureUniformSize_, 1, imgSize); --- bod
Hi Bryan, Thanks for the review, and apologies for the slow reply. The AI drafted the code and commit messages. I observed the artifacting, captured and measured the output, tested on hardware, and reviewed the changes. I've now seen and understand Laurent's stated direction on AI-generated contributions, so I'm withdrawing both patches. For the finding itself, with 1928x1088 SRGGB10 scaled to 640x480, output row y samples the source row (2y + 1) * 17 / 15. This lands on exact texel boundaries at output rows 7, 22, 37, and every 15 rows after. The row parity is taken from center.w, in pixel units, while the texture unit picks the texel from center.y, in texture units, and they are interpolated separately. At those boundaries they can disagree by one row, so the fetched row is debayered with the wrong parity. Those rows averaged about 13 levels above the others in R + B - 2G (8-bit output). The excess fell to ~0 when the fragment shader used one floor(pixelPos) for both the parity and fetch coordinates, with +0.5 placing the fetch at the texel centre. Separately, qcam sets stridePixels to width for 8-bit Bayer, leaving stride_factor at 1.0 even when rows have padding. Happy to test a fix on this IPU7, imx471 and Arc B390 system! Kind regards, Christian
diff --git a/src/libcamera/shaders/bayer_unpacked.frag b/src/libcamera/shaders/bayer_unpacked.frag index 10c5e941b..455d1fe78 100644 --- a/src/libcamera/shaders/bayer_unpacked.frag +++ b/src/libcamera/shaders/bayer_unpacked.frag @@ -21,9 +21,10 @@ precision highp float; /** Monochrome RGBA or GL_LUMINANCE Bayer encoded texture.*/ uniform sampler2D tex_y; -varying vec4 center; -varying vec4 yCoord; -varying vec4 xCoord; +uniform vec2 tex_size; +uniform float stride_factor; +uniform vec2 tex_bayer_first_red; +varying vec2 pixelPos; uniform vec3 awb; uniform mat3 ccm; uniform vec3 blacklevel; @@ -52,11 +53,23 @@ void main(void) { #define fetch(x, y) texture2D(tex_y, vec2(x, y)).r #endif + /* + * Derive the parity and every fetch coordinate from one floor() of the + * pixel position, snapped to texel centres. Computed separately they + * can round to different texels when downscaling lands a sample on a + * texel boundary. + */ + vec2 texStep = vec2(stride_factor / tex_size.x, 1.0 / tex_size.y); + vec2 pix = floor(pixelPos); + vec2 center = (pix + 0.5) * texStep; + vec4 xCoord = center.x + vec4(-2.0, -1.0, 1.0, 2.0) * texStep.x; + vec4 yCoord = center.y + vec4(-2.0, -1.0, 1.0, 2.0) * texStep.y; + float C = fetch(center.x, center.y); // ( 0, 0) const vec4 kC = vec4( 4.0, 6.0, 5.0, 5.0) / 8.0; // Determine which of four types of pixels we are on. - vec2 alternate = mod(floor(center.zw), 2.0); + vec2 alternate = mod(pix + tex_bayer_first_red, 2.0); vec4 Dvec = vec4( fetch(xCoord[1], yCoord[1]), // (-1,-1) diff --git a/src/libcamera/shaders/bayer_unpacked.vert b/src/libcamera/shaders/bayer_unpacked.vert index 423dde0fa..be1268b62 100644 --- a/src/libcamera/shaders/bayer_unpacked.vert +++ b/src/libcamera/shaders/bayer_unpacked.vert @@ -22,34 +22,12 @@ attribute vec2 textureIn; uniform mat4 proj_matrix; uniform vec2 tex_size; /* The texture size in pixels */ -uniform vec2 tex_step; -/** Pixel position of the first red pixel in the */ -/** Bayer pattern. [{0,1}, {0, 1}]*/ -uniform vec2 tex_bayer_first_red; - -/** .xy = Pixel being sampled in the fragment shader on the range [0, 1] - .zw = ...on the range [0, sourceSize], offset by firstRed */ -varying vec4 center; - -/** center.x + (-2/w, -1/w, 1/w, 2/w); These are the x-positions */ -/** of the adjacent pixels.*/ -varying vec4 xCoord; - -/** center.y + (-2/h, -1/h, 1/h, 2/h); These are the y-positions */ -/** of the adjacent pixels.*/ -varying vec4 yCoord; - -uniform float stride_factor; +/** Position of the pixel being sampled, in image pixels. */ +varying vec2 pixelPos; void main(void) { - center.xy = vec2(textureIn.x * stride_factor, textureIn.y); - center.zw = textureIn * tex_size + tex_bayer_first_red; - - xCoord = center.x + vec4(-2.0 * tex_step.x, - -tex_step.x, tex_step.x, 2.0 * tex_step.x); - yCoord = center.y + vec4(-2.0 * tex_step.y, - -tex_step.y, tex_step.y, 2.0 * tex_step.y); + pixelPos = textureIn * tex_size; gl_Position = proj_matrix * vertexIn; } diff --git a/src/libcamera/software_isp/debayer_egl.cpp b/src/libcamera/software_isp/debayer_egl.cpp index 97aa03793..6db07c284 100644 --- a/src/libcamera/software_isp/debayer_egl.cpp +++ b/src/libcamera/software_isp/debayer_egl.cpp @@ -432,11 +432,11 @@ void DebayerEGL::setShaderVariableValues(eGLImage &eglImageIn, const DebayerPara /* * These values are: - * firstRed = tex_bayer_first_red - bayer_8.vert - * imgSize = tex_size - bayer_8.vert - * step = tex_step - bayer_8.vert - * Stride = stride_factor identity.vert - * textureUniformProjMatri = No scaling + * firstRed = tex_bayer_first_red - bayer_unpacked.frag, bayer_1x_packed.frag + * imgSize = tex_size - bayer_unpacked.vert, bayer_unpacked.frag, bayer_1x_packed.frag + * step = tex_step - bayer_1x_packed.frag + * Stride = stride_factor - identity.vert, bayer_unpacked.frag + * projMatrix = proj_matrix - identity.vert, bayer_unpacked.vert */ glUniform2fv(textureUniformBayerFirstRed_, 1, firstRed); glUniform2fv(textureUniformSize_, 1, imgSize);
The bayer_unpacked fragment shader derives the Bayer parity of the sampled pixel from one interpolated varying, in pixel units, while the texture unit selects the nearest texel from another, in texture units. At native size every sample lands on a texel centre and the two agree. When the render target is smaller than the source, as in the software ISP GPU debayer and qcam's scaled viewfinder, sample positions land on texel boundaries on a periodic subset of output rows and columns, and rounding differences make the parity and the fetched texel disagree by one. Such rows debayer with the wrong row parity, raising R and B over G: regular magenta lines. On an imx471 (1928x1088 SRGGB10 through the Intel IPU7 ISYS, debayered by Mesa 26.1 on Intel Arc B390 graphics) scaled to 640x480, output row y samples source row (2y + 1) * 17 / 15, an exact texel boundary for y = 7 (mod 15). Captures showed one magenta line every 15 output rows, at those offsets. Select the source pixel with a single floor() in pixel space, as bayer_1x_packed.frag already does, and derive the parity, the centre fetch and the neighbour fetches from that one index, snapped to texel centres. The two computations can then no longer disagree. The vertex shader now only forwards the source position in pixels; the center, xCoord and yCoord varyings and tex_step are dropped from the unpacked programs, and stride_factor and tex_bayer_first_red move to the fragment shader. DebayerEGL and qcam already set those uniforms, so no C++ changes are needed beyond a comment. At native size the sampled texels are unchanged. Measured with the mean of R + B - 2G per output row, in 8-bit output levels: at 640x480 the mean excess of the y = 7 (mod 15) rows drops from +13.1 (about 5% of full scale), with all other phases within +/-0.05, to 0.00, within the spread of the other phases. 480x320, 640x360, 1280x720 and the native output size of 1924x1088 stay clean and the field of view is unchanged. Assisted by Claude Code (claude-fable-5). Signed-off-by: Christian Murphy <christian@themurphys.eu> --- src/libcamera/shaders/bayer_unpacked.frag | 21 ++++++++++++---- src/libcamera/shaders/bayer_unpacked.vert | 28 +++------------------- src/libcamera/software_isp/debayer_egl.cpp | 10 ++++---- 3 files changed, 25 insertions(+), 34 deletions(-)