[PATCH v3 1/2] wined3d: Fix the odd line condition in gen_yv12_read().
Signed-off-by: Anton Baskanov <baskanov(a)gmail.com> --- The original condition is true for every other line, which is incorrect since the chroma has only half the resolution. Fixes comb artifacts around color transitions. v2: Added a comment. v3: Split condition simplification into a separate patch. --- dlls/wined3d/glsl_shader.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/dlls/wined3d/glsl_shader.c b/dlls/wined3d/glsl_shader.c index 5ae90148dc8..c760d519b30 100644 --- a/dlls/wined3d/glsl_shader.c +++ b/dlls/wined3d/glsl_shader.c @@ -12621,8 +12621,10 @@ static void gen_yv12_read(struct wined3d_string_buffer *buffer, * Don't forget to clamp the y values in into the range, otherwise we'll * get filtering bleeding. */ - /* Read odd lines from the right side (add 0.5 to the x coordinate). */ - shader_addline(buffer, " if (fract(floor(texcoord.y * size.y) * 0.5 + 1.0 / 6.0) >= 0.5)\n"); + /* Read odd lines from the right side (add 0.5 to the x coordinate). Keep + * in mind that each line of the chroma plane corresponds to 2 lines of the + * resulting image. */ + shader_addline(buffer, " if (fract(floor(texcoord.y * size.y) * 0.25 + 1.0 / 6.0) >= 0.5)\n"); shader_addline(buffer, " texcoord.x += 0.5;\n"); /* Clamp, keep the half pixel origin in mind. */ -- 2.25.1
Signed-off-by: Anton Baskanov <baskanov(a)gmail.com> --- dlls/wined3d/glsl_shader.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/wined3d/glsl_shader.c b/dlls/wined3d/glsl_shader.c index c760d519b30..fb809052d83 100644 --- a/dlls/wined3d/glsl_shader.c +++ b/dlls/wined3d/glsl_shader.c @@ -12624,7 +12624,7 @@ static void gen_yv12_read(struct wined3d_string_buffer *buffer, /* Read odd lines from the right side (add 0.5 to the x coordinate). Keep * in mind that each line of the chroma plane corresponds to 2 lines of the * resulting image. */ - shader_addline(buffer, " if (fract(floor(texcoord.y * size.y) * 0.25 + 1.0 / 6.0) >= 0.5)\n"); + shader_addline(buffer, " if (fract(texcoord.y * size.y * 0.25) >= 0.5)\n"); shader_addline(buffer, " texcoord.x += 0.5;\n"); /* Clamp, keep the half pixel origin in mind. */ -- 2.25.1
Am 31.08.2021 um 19:26 schrieb Anton Baskanov <baskanov(a)gmail.com>:
- shader_addline(buffer, " if (fract(floor(texcoord.y * size.y) * 0.25 + 1.0 / 6.0) >= 0.5)\n"); + shader_addline(buffer, " if (fract(texcoord.y * size.y * 0.25) >= 0.5)\n");
Some archeology 2c: the code in arb_program_shader.c has this to say about the 1/6 offset: shader_addline(buffer, "ADD texcrd2.x, texcrd2.y, yv12_coef.y;\n"); /* To avoid 0.5 == 0.5 comparisons */ I don't remember why I was scared of 0.5 == 0.5 comparisons or why exactly they might happen (at the exact edge of the texture probably? But even then I'd expect the texture coordinate to be not quite 1.0), why I was scared of equality here ('SGE' is set greater equal after all. Driver bugs?). It was 13 years ago when I wrote the ARB version of this. The ARB code does use 0.25 though, like the change your previous patch introduces. My gut feeling says that those shaders should be safe re magnifying filters, but if the blit scales the YUV image down while converting to RGB it'll probably break in interesting ways.
On среда, 1 сентября 2021 г. 22:53:14 +07 you wrote:
Am 31.08.2021 um 19:26 schrieb Anton Baskanov <baskanov(a)gmail.com>:
- shader_addline(buffer, " if (fract(floor(texcoord.y * size.y) * 0.25 + 1.0 / 6.0) >= 0.5)\n"); + shader_addline(buffer, " if (fract(texcoord.y * size.y * 0.25) >= 0.5)\n"); Some archeology 2c: the code in arb_program_shader.c has this to say about the 1/6 offset:
shader_addline(buffer, "ADD texcrd2.x, texcrd2.y, yv12_coef.y;\n"); /* To avoid 0.5 == 0.5 comparisons */
I don't remember why I was scared of 0.5 == 0.5 comparisons or why exactly they might happen (at the exact edge of the texture probably? But even then I'd expect the texture coordinate to be not quite 1.0), why I was scared of equality here ('SGE' is set greater equal after all. Driver bugs?). It was 13 years ago when I wrote the ARB version of this.
The ARB code does use 0.25 though, like the change your previous patch introduces.
Weirdly, the WINED3D_GL_RES_TYPE_TEX_2D branch in the ARB code does not multiply the floor result by anything at all. This means that the condition is always false. I tried to set shader_backend to arb, and I saw 4-pixel tall jaggies around the color transitions, which confirm this.
My gut feeling says that those shaders should be safe re magnifying filters, but if the blit scales the YUV image down while converting to RGB it'll probably break in interesting ways.
Signed-off-by: Henri Verbeet <hverbeet(a)codeweavers.com>
participants (3)
-
Anton Baskanov -
Henri Verbeet -
Stefan Dösinger