[v3,31/39] libcamera: shaders: Fix neighbouring positions in 8-bit debayering

24676 diff mbox series

From: Milan Zamazal <mzamazal@redhat.com>

When accessing a texture position in a shader, the pixel with the
nearest centre to the specified texture coordinates (as mandated by
specifying GL_NEAREST parameter) is taken.  The current vertex shader
determines the positions of the neighbouring pixels by adding the
provided texture steps to the exact centre pixel coordinates.  But this
places the computed coordinates, from the point of view of GL_NEAREST,
exactly between the pixels and is thus prone to floating point
inaccuracies.  Wrong neighbouring pixel coordinates may be used,
resulting in artefacts in the output image.

Let's fix the problem by shifting the initial coordinates a bit from the
pixel border.

Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 include/libcamera/internal/shaders/bayer_unpacked.vert | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Bryan O'Donoghue <bryan.odonoghue@linaro.org> writes:

> From: Milan Zamazal <mzamazal@redhat.com>
>
> When accessing a texture position in a shader, the pixel with the
> nearest centre to the specified texture coordinates (as mandated by
> specifying GL_NEAREST parameter) is taken.  The current vertex shader
> determines the positions of the neighbouring pixels by adding the
> provided texture steps to the exact centre pixel coordinates.  But this
> places the computed coordinates, from the point of view of GL_NEAREST,
> exactly between the pixels and is thus prone to floating point
> inaccuracies.  Wrong neighbouring pixel coordinates may be used,
> resulting in artefacts in the output image.

I cannot reproduce this any more.  Quite opposite, artefacts are not
present iff the corrective step multiplication step is inside -0.5..0.5
range.  So the coordinates are apparently placed on pixel centres rather
than borders now.  And if I attempt to use a second set of shaders, the
coordinates shift introduced by this patch causes another trouble with
artefacts (still no idea why).

Honestly, I don't understand how it is supposed to work and what has
changed that it works differently now than long time before (pre-v1, I
think).  But based on my recent empiric observations, I'd say we should
drop this patch (unless independent testing tells otherwise).

> Let's fix the problem by shifting the initial coordinates a bit from the
> pixel border.
>
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  include/libcamera/internal/shaders/bayer_unpacked.vert | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/include/libcamera/internal/shaders/bayer_unpacked.vert b/include/libcamera/internal/shaders/bayer_unpacked.vert
> index fb5109ee..fc1cf89f 100644
> --- a/include/libcamera/internal/shaders/bayer_unpacked.vert
> +++ b/include/libcamera/internal/shaders/bayer_unpacked.vert
> @@ -44,10 +44,10 @@ void main(void) {
>      center.xy = textureIn;
>      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);
> +    xCoord = center.x + 0.1 * tex_step.x +
> +      vec4(-2.0 * tex_step.x, -tex_step.x, tex_step.x, 2.0 * tex_step.x);
> +    yCoord = center.y + 0.1 * tex_step.y +
> +      vec4(-2.0 * tex_step.y, -tex_step.y, tex_step.y, 2.0 * tex_step.y);
>  
>      gl_Position = proj_matrix * vertexIn;
>  }

On 24/10/2025 21:49, Milan Zamazal wrote:
> Bryan O'Donoghue<bryan.odonoghue@linaro.org> writes:
> 
>> From: Milan Zamazal<mzamazal@redhat.com>
>>
>> When accessing a texture position in a shader, the pixel with the
>> nearest centre to the specified texture coordinates (as mandated by
>> specifying GL_NEAREST parameter) is taken.  The current vertex shader
>> determines the positions of the neighbouring pixels by adding the
>> provided texture steps to the exact centre pixel coordinates.  But this
>> places the computed coordinates, from the point of view of GL_NEAREST,
>> exactly between the pixels and is thus prone to floating point
>> inaccuracies.  Wrong neighbouring pixel coordinates may be used,
>> resulting in artefacts in the output image.
> I cannot reproduce this any more.  Quite opposite, artefacts are not
> present iff the corrective step multiplication step is inside -0.5..0.5
> range.  So the coordinates are apparently placed on pixel centres rather
> than borders now.  And if I attempt to use a second set of shaders, the
> coordinates shift introduced by this patch causes another trouble with
> artefacts (still no idea why).
> 
> Honestly, I don't understand how it is supposed to work and what has
> changed that it works differently now than long time before (pre-v1, I
> think).  But based on my recent empiric observations, I'd say we should
> drop this patch (unless independent testing tells otherwise).

OK.

I trust your debug here and rely on your input for this setup anyway.

Dropped.

---
bod