The current YUV to RGB conversion provides values in studio-range [16-235], but full-range is required [0-255]. As a result, we currently write the value of 16 to represent black, and so we get gray instead.
This MR changes the coefficients used in the conversion so that the resultant output is in full-range. It actually uses two sets of coefficients as these differ between SD and HD.
Also included is a number of Direct Draw 7 tests.
The coefficients used are documented here: https://learn.microsoft.com/en-us/windows/win32/medfound/recommended-8-bit-y...
From: Brendan McGrath bmcgrath@codeweavers.com
--- dlls/ddraw/tests/ddraw7.c | 231 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 231 insertions(+)
diff --git a/dlls/ddraw/tests/ddraw7.c b/dlls/ddraw/tests/ddraw7.c index e7d66278ad7..b1ea54702cd 100644 --- a/dlls/ddraw/tests/ddraw7.c +++ b/dlls/ddraw/tests/ddraw7.c @@ -13652,6 +13652,236 @@ static void test_blt(void) DestroyWindow(window); }
+static void test_yuv_to_rgb_blt(void) +{ + IDirectDrawSurface7 *rgb_surface, *yuv_surface; + DWORD flags, chroma_pitch, chroma_height; + DDSURFACEDESC2 surface_desc, lock_desc; + unsigned int i, j, x, y; + IDirectDraw7 *ddraw; + ULONG refcount; + UINT32 *pixel; + HWND window; + HRESULT hr; + BYTE *ptr; + + static struct + { + const char *name; + DWORD fourCC; + BYTE h_subsampling; + BYTE v_subsampling; + BYTE u_plane; + BYTE v_plane; + BYTE y_offset; + BYTE u_offset; + BYTE v_offset; + BYTE y_increment; + BYTE u_increment; + BYTE v_increment; + } + test_textures[] = + { + {"uyvy", MAKEFOURCC('U', 'Y', 'V', 'Y'), 2, 1, 0, 0, 1, 0, 2, 2, 4, 4}, + {"yuy2", MAKEFOURCC('Y', 'U', 'Y', '2'), 2, 1, 0, 0, 0, 1, 3, 2, 4, 4}, + {"nv12", MAKEFOURCC('N', 'V', '1', '2'), 2, 2, 1, 1, 0, 0, 1, 1, 2, 2}, + {"yv12", MAKEFOURCC('Y', 'V', '1', '2'), 2, 2, 2, 1, 0, 0, 0, 1, 1, 1}, + }; + + static struct + { + const char *name; + DWORD height; + BYTE y; + BYTE u; + BYTE v; + UINT32 expected; + BOOL todo; + /* due to differing implementations, for some values, some systems + * will return an alternative result */ + BOOL allow_expected2; + UINT32 expected2; + /* some systems use SD values for HD. HD is skipped on these. */ + BOOL test_hd_broken; + UINT32 broken_hd; + } + test_colors[] = + { + {"black SD", 8, 0x10, 0x80, 0x80, 0x000000, TRUE}, + {"white SD", 8, 0xeb, 0x80, 0x80, 0xffffff, TRUE}, + {"red SD", 8, 0x51, 0x5a, 0xf0, 0xfe0000, TRUE}, + {"green SD", 8, 0x91, 0x36, 0x22, 0x00ff01, TRUE}, + {"blue SD", 8, 0x29, 0xf0, 0x6e, 0x0000ff, TRUE}, + {"gray SD", 8, 0x7e, 0x80, 0x80, 0x808080, TRUE}, + {"past black SD", 8, 0x00, 0x80, 0x80, 0x000000, TRUE}, + {"past white SD", 8, 0xff, 0x80, 0x80, 0xffffff, TRUE}, + {"zeros SD", 8, 0x00, 0x00, 0x00, 0x008700, TRUE, TRUE, 0x008800}, + /* Windows will treat anything with a height greater than 576 as HD and, + * as per ITU-R Recommendation BT.709, change the color scheme */ + {"black HD", 600, 0x10, 0x80, 0x80, 0x000000, TRUE}, + {"white HD", 600, 0xeb, 0x80, 0x80, 0xffffff, TRUE}, + {"red HD", 600, 0x3f, 0x66, 0xf0, 0xff0000, TRUE, TRUE, 0xff0100, TRUE, 0xe90002}, + {"green HD", 600, 0xad, 0x2a, 0x1a, 0x00ff01, TRUE}, + {"blue HD", 600, 0x20, 0xf0, 0x76, 0x0100ff, TRUE}, + {"gray HD", 600, 0x7e, 0x80, 0x80, 0x808080, TRUE}, + {"past black HD", 600, 0x00, 0x80, 0x80, 0x000000, TRUE}, + {"past white HD", 600, 0xff, 0x80, 0x80, 0xffffff, TRUE}, + {"zeros HD", 600, 0x00, 0x00, 0x00, 0x004d00, TRUE}, + }; + + window = create_window(); + if (!(ddraw = create_ddraw())) + { + skip("Failed to create a 3D device, skipping test.\n"); + DestroyWindow(window); + return; + } + + hr = IDirectDraw7_SetCooperativeLevel(ddraw, window, DDSCL_NORMAL); + ok(SUCCEEDED(hr), "Failed to set cooperative level, hr %#lx.\n", hr); + + memset(&surface_desc, 0, sizeof(surface_desc)); + surface_desc.dwSize = sizeof(surface_desc); + surface_desc.dwFlags = DDSD_WIDTH | DDSD_HEIGHT | DDSD_CAPS | DDSD_PIXELFORMAT; + surface_desc.dwWidth = 8; + surface_desc.ddsCaps.dwCaps = DDSCAPS_OFFSCREENPLAIN; + surface_desc.ddpfPixelFormat.dwSize = sizeof(surface_desc.ddpfPixelFormat); + surface_desc.ddpfPixelFormat.dwRGBBitCount = 32; + surface_desc.ddpfPixelFormat.dwRBitMask = 0xff0000; + surface_desc.ddpfPixelFormat.dwGBitMask = 0xff00; + surface_desc.ddpfPixelFormat.dwBBitMask = 0xff; + surface_desc.ddpfPixelFormat.dwRGBAlphaBitMask = 0; + + flags = DDSD_HEIGHT | DDSD_PITCH | DDSD_PIXELFORMAT | DDSD_WIDTH; + + memset(&lock_desc, 0, sizeof(lock_desc)); + lock_desc.dwSize = sizeof(lock_desc); + + for (i = 0; i < ARRAY_SIZE(test_textures); ++i) + { + surface_desc.ddpfPixelFormat.dwFourCC = test_textures[i].fourCC; + + for (j = 0; j < ARRAY_SIZE(test_colors); j++) + { + surface_desc.dwHeight = test_colors[j].height; + surface_desc.ddpfPixelFormat.dwFlags = DDPF_FOURCC; + hr = IDirectDraw7_CreateSurface(ddraw, &surface_desc, &yuv_surface, NULL); + if (FAILED(hr)) + { + skip("Failed to create %s surface, hr %#lx.\n", test_textures[i].name, hr); + break; + } + + surface_desc.ddpfPixelFormat.dwFlags = DDPF_RGB; + hr = IDirectDraw7_CreateSurface(ddraw, &surface_desc, &rgb_surface, NULL); + ok(SUCCEEDED(hr), "Failed to create rgb surface, hr %#lx.\n", hr); + + hr = IDirectDrawSurface7_Lock(yuv_surface, NULL, &lock_desc, DDLOCK_WRITEONLY | DDLOCK_WAIT | DDLOCK_SURFACEMEMORYPTR, NULL); + ok(hr == S_OK, "Test %u (%s): Got unexpected hr %#lx, expected S_OK.\n", i, test_textures[i].name, hr); + ok((lock_desc.dwFlags & flags) == flags, "Test %u (%s): Got unexpected flags %#lx, expected %#lx.\n", + i, test_textures[i].name, lock_desc.dwFlags, flags); + ok(lock_desc.ddpfPixelFormat.dwFlags == DDPF_FOURCC, + "Test %u (%s): Got unexpected pixel format flags %#lx, expected DDPF_FOURCC.\n", + i, test_textures[i].name, lock_desc.ddpfPixelFormat.dwFlags); + ok(lock_desc.ddpfPixelFormat.dwFourCC == test_textures[i].fourCC, + "Test %u (%s): Got unexpected pixel format %#lx, expected %#lx.\n", + i, test_textures[i].name, lock_desc.ddpfPixelFormat.dwFourCC, test_textures[i].fourCC); + + ptr = (BYTE*)lock_desc.lpSurface + test_textures[i].y_offset; + + for (y = 0; y < lock_desc.dwHeight; y++) + { + for (x = 0; x < lock_desc.lPitch / test_textures[i].y_increment; x++) + { + *ptr = test_colors[j].y; + ptr += test_textures[i].y_increment; + } + } + + chroma_pitch = lock_desc.lPitch / (test_textures[i].h_subsampling * test_textures[i].y_increment); + chroma_height = lock_desc.dwHeight / test_textures[i].v_subsampling; + + ptr = (BYTE*)lock_desc.lpSurface + test_textures[i].u_offset; + + if (test_textures[i].u_plane) + { + ptr += lock_desc.lPitch * lock_desc.dwHeight; + ptr += chroma_pitch * chroma_height * (test_textures[i].u_plane - 1); + } + + for (y = 0; y < chroma_height; y++) + { + for (x = 0; x < chroma_pitch; x++) + { + *ptr = test_colors[j].u; + ptr += test_textures[i].u_increment; + } + } + + ptr = (BYTE*)lock_desc.lpSurface + test_textures[i].v_offset; + if (test_textures[i].v_plane) + { + ptr += lock_desc.lPitch * lock_desc.dwHeight; + ptr += chroma_pitch * chroma_height * (test_textures[i].v_plane - 1); + } + + for (y = 0; y < chroma_height; y++) + { + for (x = 0; x < chroma_pitch; x++) + { + *ptr = test_colors[j].v; + ptr += test_textures[i].v_increment; + } + } + + hr = IDirectDrawSurface7_Unlock(yuv_surface, NULL); + ok(hr == S_OK, "Test %u (%s): Got unexpected hr %#lx, expected S_OK.\n", i, test_textures[i].name, hr); + + hr = IDirectDrawSurface7_Blt(rgb_surface, NULL, yuv_surface, NULL, DDBLT_WAIT, NULL); + ok(hr == S_OK, "Test %u (%s): Got unexpected hr %#lx, expected S_OK.\n", i, test_textures[i].name, hr); + + hr = IDirectDrawSurface7_Lock(rgb_surface, NULL, &lock_desc, DDLOCK_READONLY | DDLOCK_WAIT | DDLOCK_SURFACEMEMORYPTR, NULL); + ok(hr == S_OK, "Test %u (%s): Got unexpected hr %#lx, expected S_OK.\n", i, test_textures[i].name, hr); + ok((lock_desc.dwFlags & flags) == flags, "Test %u (%s): Got unexpected flags %#lx, expected %#lx.\n", + i, test_textures[i].name, lock_desc.dwFlags, flags); + ok(lock_desc.ddpfPixelFormat.dwFlags == DDPF_RGB, + "Test %u (%s): Got unexpected pixel format flags %#lx, expected DDPF_RGB.\n", + i, test_textures[i].name, lock_desc.ddpfPixelFormat.dwFlags); + + if (test_colors[j].test_hd_broken && (*(UINT32*)lock_desc.lpSurface & 0xffffff) == test_colors[j].broken_hd) + { + skip("System doesn't support HD values.\n"); + IDirectDrawSurface7_Unlock(rgb_surface, NULL); + IDirectDrawSurface7_Release(yuv_surface); + IDirectDrawSurface7_Release(rgb_surface); + break; + } + + /* check the top left 4x4 pixels */ + for (y = 0; y < 4; y++) + { + pixel = (UINT32*)((BYTE*)lock_desc.lpSurface + lock_desc.lPitch * y); + for (x = 0; x < 4; x++) + todo_wine_if(test_colors[j].todo) + ok((pixel[x] & 0xffffff) == test_colors[j].expected || + (test_colors[j].allow_expected2 && (pixel[x] & 0xffffff) == test_colors[j].expected2), + "Test %u (%s / %s): Got unexpected value at (%d,%d). Got %#lx, expected %#lx.\n", + i, test_textures[i].name, test_colors[j].name, x, y, (long unsigned)pixel[x] & 0xffffff, (long unsigned)test_colors[j].expected); + } + + hr = IDirectDrawSurface7_Unlock(rgb_surface, NULL); + ok(hr == S_OK, "Test %u (%s): Got unexpected hr %#lx, expected S_OK.\n", i, test_textures[i].name, hr); + + IDirectDrawSurface7_Release(yuv_surface); + IDirectDrawSurface7_Release(rgb_surface); + } + } + + refcount = IDirectDraw7_Release(ddraw); + ok(!refcount, "DirectDraw7 has %lu references left.\n", refcount); + DestroyWindow(window); +} + static void test_blt_z_alpha(void) { DWORD blt_flags[] = @@ -20609,6 +20839,7 @@ START_TEST(ddraw7) test_offscreen_overlay(); test_overlay_rect(); test_blt(); + test_yuv_to_rgb_blt(); test_blt_z_alpha(); test_cross_device_blt(); test_color_clamping();
From: Brendan McGrath bmcgrath@codeweavers.com
--- dlls/ddraw/tests/ddraw7.c | 38 ++++++++++++++++++-------------------- dlls/wined3d/glsl_shader.c | 25 +++++++++++++++++++------ 2 files changed, 37 insertions(+), 26 deletions(-)
diff --git a/dlls/ddraw/tests/ddraw7.c b/dlls/ddraw/tests/ddraw7.c index b1ea54702cd..ff3ba02ae52 100644 --- a/dlls/ddraw/tests/ddraw7.c +++ b/dlls/ddraw/tests/ddraw7.c @@ -13696,7 +13696,6 @@ static void test_yuv_to_rgb_blt(void) BYTE u; BYTE v; UINT32 expected; - BOOL todo; /* due to differing implementations, for some values, some systems * will return an alternative result */ BOOL allow_expected2; @@ -13707,26 +13706,26 @@ static void test_yuv_to_rgb_blt(void) } test_colors[] = { - {"black SD", 8, 0x10, 0x80, 0x80, 0x000000, TRUE}, - {"white SD", 8, 0xeb, 0x80, 0x80, 0xffffff, TRUE}, - {"red SD", 8, 0x51, 0x5a, 0xf0, 0xfe0000, TRUE}, - {"green SD", 8, 0x91, 0x36, 0x22, 0x00ff01, TRUE}, - {"blue SD", 8, 0x29, 0xf0, 0x6e, 0x0000ff, TRUE}, - {"gray SD", 8, 0x7e, 0x80, 0x80, 0x808080, TRUE}, - {"past black SD", 8, 0x00, 0x80, 0x80, 0x000000, TRUE}, - {"past white SD", 8, 0xff, 0x80, 0x80, 0xffffff, TRUE}, - {"zeros SD", 8, 0x00, 0x00, 0x00, 0x008700, TRUE, TRUE, 0x008800}, + {"black SD", 8, 0x10, 0x80, 0x80, 0x000000}, + {"white SD", 8, 0xeb, 0x80, 0x80, 0xffffff}, + {"red SD", 8, 0x51, 0x5a, 0xf0, 0xfe0000}, + {"green SD", 8, 0x91, 0x36, 0x22, 0x00ff01}, + {"blue SD", 8, 0x29, 0xf0, 0x6e, 0x0000ff}, + {"gray SD", 8, 0x7e, 0x80, 0x80, 0x808080}, + {"past black SD", 8, 0x00, 0x80, 0x80, 0x000000}, + {"past white SD", 8, 0xff, 0x80, 0x80, 0xffffff}, + {"zeros SD", 8, 0x00, 0x00, 0x00, 0x008700, TRUE, 0x008800}, /* Windows will treat anything with a height greater than 576 as HD and, * as per ITU-R Recommendation BT.709, change the color scheme */ - {"black HD", 600, 0x10, 0x80, 0x80, 0x000000, TRUE}, - {"white HD", 600, 0xeb, 0x80, 0x80, 0xffffff, TRUE}, - {"red HD", 600, 0x3f, 0x66, 0xf0, 0xff0000, TRUE, TRUE, 0xff0100, TRUE, 0xe90002}, - {"green HD", 600, 0xad, 0x2a, 0x1a, 0x00ff01, TRUE}, - {"blue HD", 600, 0x20, 0xf0, 0x76, 0x0100ff, TRUE}, - {"gray HD", 600, 0x7e, 0x80, 0x80, 0x808080, TRUE}, - {"past black HD", 600, 0x00, 0x80, 0x80, 0x000000, TRUE}, - {"past white HD", 600, 0xff, 0x80, 0x80, 0xffffff, TRUE}, - {"zeros HD", 600, 0x00, 0x00, 0x00, 0x004d00, TRUE}, + {"black HD", 600, 0x10, 0x80, 0x80, 0x000000}, + {"white HD", 600, 0xeb, 0x80, 0x80, 0xffffff}, + {"red HD", 600, 0x3f, 0x66, 0xf0, 0xff0000, TRUE, 0xff0100, TRUE, 0xe90002}, + {"green HD", 600, 0xad, 0x2a, 0x1a, 0x00ff01}, + {"blue HD", 600, 0x20, 0xf0, 0x76, 0x0100ff}, + {"gray HD", 600, 0x7e, 0x80, 0x80, 0x808080}, + {"past black HD", 600, 0x00, 0x80, 0x80, 0x000000}, + {"past white HD", 600, 0xff, 0x80, 0x80, 0xffffff}, + {"zeros HD", 600, 0x00, 0x00, 0x00, 0x004d00}, };
window = create_window(); @@ -13862,7 +13861,6 @@ static void test_yuv_to_rgb_blt(void) { pixel = (UINT32*)((BYTE*)lock_desc.lpSurface + lock_desc.lPitch * y); for (x = 0; x < 4; x++) - todo_wine_if(test_colors[j].todo) ok((pixel[x] & 0xffffff) == test_colors[j].expected || (test_colors[j].allow_expected2 && (pixel[x] & 0xffffff) == test_colors[j].expected2), "Test %u (%s / %s): Got unexpected value at (%d,%d). Got %#lx, expected %#lx.\n", diff --git a/dlls/wined3d/glsl_shader.c b/dlls/wined3d/glsl_shader.c index df60415f062..e41d64fc6aa 100644 --- a/dlls/wined3d/glsl_shader.c +++ b/dlls/wined3d/glsl_shader.c @@ -12677,10 +12677,12 @@ static void glsl_blitter_generate_yuv_shader(struct wined3d_string_buffer *buffe { enum complex_fixup complex_fixup = get_complex_fixup(args->fixup);
- shader_addline(buffer, "const vec4 yuv_coef = vec4(1.403, -0.344, -0.714, 1.770);\n"); + shader_addline(buffer, "const vec4 yuv_coef_sd = vec4(1.596, -0.392, -0.813, 2.017);\n"); + shader_addline(buffer, "const vec4 yuv_coef_hd = vec4(1.793, -0.213, -0.533, 2.112);\n"); shader_addline(buffer, "float luminance;\n"); shader_addline(buffer, "vec2 texcoord;\n"); shader_addline(buffer, "vec2 chroma;\n"); + shader_addline(buffer, "vec4 yuv_coef;\n"); shader_addline(buffer, "uniform vec2 size;\n");
shader_addline(buffer, "\nvoid main()\n{\n"); @@ -12715,14 +12717,25 @@ static void glsl_blitter_generate_yuv_shader(struct wined3d_string_buffer *buffe return; }
- /* Calculate the final result. Formula is taken from - * http://www.fourcc.org/fccyvrgb.php. Note that the chroma - * ranges from -0.5 to 0.5. */ - shader_addline(buffer, "\n chroma.xy -= 0.5;\n"); - + /* Calculate the final result. Formula is taken from: + * https://learn.microsoft.com/en-us/windows/win32/medfound/recommended-8-bit-y.... + * SD and HD textures use different coefficients. SD is anything with both width and height smaller or equal to 720x576. + * Values are converted from their normalized values [0.0, 1.0] to byte values [0, 255] before calculations begin to avoid off by one rounding errors. + * Note that input values are clamped for SD, but not HD. Luminance is clamped to [16,235] ([0.063, 0.922] when normalized). + * Chroma is clamped to [16,240] ([0.063, 0.941] when normalized). */ + shader_addline(buffer, "\n if (size.x <= 720 && size.y <= 576)\n {\n"); + shader_addline(buffer, " luminance = clamp(luminance, 0.063, 0.922);\n"); + shader_addline(buffer, " chroma.xy = clamp(chroma.xy, 0.063, 0.941);\n"); + shader_addline(buffer, " yuv_coef = yuv_coef_sd;\n"); + shader_addline(buffer, " }\n else\n {\n"); + shader_addline(buffer, " yuv_coef = yuv_coef_hd;\n"); + shader_addline(buffer, " }\n"); + shader_addline(buffer, " luminance = (luminance * 255 - 16) * 1.164;\n"); + shader_addline(buffer, " chroma.xy = chroma.xy * 255 - 128;\n"); shader_addline(buffer, " %s.x = luminance + chroma.x * yuv_coef.x;\n", output); shader_addline(buffer, " %s.y = luminance + chroma.y * yuv_coef.y + chroma.x * yuv_coef.z;\n", output); shader_addline(buffer, " %s.z = luminance + chroma.y * yuv_coef.w;\n", output); + shader_addline(buffer, " %s /= 255;\n", output); if (args->use_colour_key) shader_glsl_generate_colour_key_test(buffer, output, "colour_key.low", "colour_key.high");
``` + static struct ```
"static const"
``` + /* due to differing implementations, for some values, some systems + * will return an alternative result */ + BOOL allow_expected2; + UINT32 expected2; ```
All of these differ only by one. We should instead use compare_color(..., 1), which is what we do more generally.
``` + /* some systems use SD values for HD. HD is skipped on these. */ + BOOL test_hd_broken; ```
Which ones?
``` + {"zeros SD", 8, 0x00, 0x00, 0x00, 0x008700, TRUE, TRUE, 0x008800}, + /* Windows will treat anything with a height greater than 576 as HD and, + * as per ITU-R Recommendation BT.709, change the color scheme */ + {"black HD", 600, 0x10, 0x80, 0x80, 0x000000, TRUE}, ``` Since it's not any extra work, can we change these values to be, say, 576 and 572 instead of 8 and 600?
``` + ok(SUCCEEDED(hr), "Failed to create rgb surface, hr %#lx.\n", hr); ```
"hr == S_OK" in new code, please.
Also, we can move the surface creation to the outer loop, right?
``` + ok(hr == S_OK, "Test %u (%s): Got unexpected hr %#lx, expected S_OK.\n", i, test_textures[i].name, hr); ```
We have winetest_push_context() now; let's use that in new code instead of the "Test %u" prefix. [Also, specifying both an index and a name seems a bit redundant?]
``` + if (test_colors[j].test_hd_broken && (*(UINT32*)lock_desc.lpSurface & 0xffffff) == test_colors[j].broken_hd) + { + skip("System doesn't support HD values.\n"); ```
A skip in that case seems odd, why not just use broken() when comparing colours? [Though, again, I am curious which systems do this...]
`+ todo_wine_if(test_colors[j].todo)`
Not that it matters that much, but it seems this could be a regular todo_wine, rather than a todo_wine_if.
`+ * Values are converted from their normalized values [0.0, 1.0] to byte values [0, 255] before calculations begin to avoid off by one rounding errors.`
We're dealing with floats here, though, not integers. Also, as the tests show, being off by one isn't really a problem.
-
Brendan McGrath -
Brendan McGrath (＠redmcg)
-
Elizabeth Figura (＠zfigura)