``` + 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.