[PATCH 1/3] wined3d: Test if a nudge is needed to get the correct filling convention.

[Henri Verbeet]

On Mon, 6 Sept 2021 at 21:41, Stefan Dösinger <stefan at codeweavers.com> wrote:
> diff --git a/dlls/wined3d/adapter_gl.c b/dlls/wined3d/adapter_gl.c
> index 902b9620f30..72f46693274 100644
> --- a/dlls/wined3d/adapter_gl.c
> +++ b/dlls/wined3d/adapter_gl.c
> @@ -5130,6 +5130,7 @@ static void wined3d_adapter_gl_init_d3d_info(struct wined3d_adapter_gl *adapter_
>      d3d_info->full_ffp_varyings = !!(shader_caps.wined3d_caps & WINED3D_SHADER_CAP_FULL_FFP_VARYINGS);
>      d3d_info->scaled_resolve = !!gl_info->supported[EXT_FRAMEBUFFER_MULTISAMPLE_BLIT_SCALED];
>      d3d_info->feature_level = feature_level_from_caps(gl_info, &shader_caps, &fragment_caps);
> +    d3d_info->filling_convention_nudge = gl_info->filling_convention_nudge;
>
It's perhaps worth explicitly initialising "filling_convention_nudge"
in wined3d_adapter_vk_init_d3d_info() as well, perhaps with a comment
to explain why we're using 0.0f there.

> +static float wined3d_adapter_find_fill_nudge(struct wined3d_caps_gl_ctx *ctx)
> +{
> +    static const float test_array[] =
> +    {
> +        0.0f,
> +        -1.0f / 1024.0f,
> +        -1.0f / 512.0f,
> +        -1.0f / 256.0f,
> +        -1.0f / 128.0f,
> +        -1.0f / 64.0f /* Accept this without a warning if it gets the right result. */
> +    };
> +    /* This value was used unconditionally before the dynamic test function was
> +     * introduced. */
> +    static const float no_clue = -1.0f / 64.0f;
> +    unsigned int i;
> +    float value;
> +
> +    if (wined3d_settings.offscreen_rendering_mode != ORM_FBO)
> +        return no_clue;
> +
> +    for (i = 0; i < ARRAY_SIZE(test_array); ++i)
> +    {
> +        value = test_array[i];
> +        if (wined3d_caps_gl_ctx_test_filling_convention(ctx, value))
> +        {
> +            if (value)
> +                WARN("Using a filling convention fixup nudge of -1/%f\n", -1.0f / value);
> +            else
> +                TRACE("No need for a filling convetion nudge.\n");
> +            return value;
> +        }
> +    }
> +
> +    FIXME("Did not find a way to get the filling convention we want.\n");
> +    return no_clue;
> +}
> +
I don't think "no_clue" is a great name, and you could probably just
"return -1.0f / 64.0f;" at the end, and add a label there to jump to
in case we're not using FBOs.

Would it make sense to do a binary search here instead of trying every
value? (Similar to what we do in
wined3d_adapter_find_polyoffset_scale())

"convetion" -> "convention"

The WARN could do with a full stop.

> +BOOL wined3d_caps_gl_ctx_test_filling_convention(struct wined3d_caps_gl_ctx *ctx, float nudge)
> +{
> +    static const struct wined3d_color red = {1.0f, 0.0f, 0.0f, 1.0f};
> +    const struct wined3d_gl_info *gl_info = ctx->gl_info;
> +    GLuint texture, fbo;
> +    DWORD readback[8][8];
> +    unsigned int x, y;
> +
We're generally using "bool" and "uint32_t" instead of "BOOL" and
"DWORD" these days; doesn't matter too much though.

> +    /* This is a very simple test to find out how GL handles pixel edges:
> +     * Draw a quad exactly between 4 pixels in an 8x8 viewport and see
> +     * which pixel it ends up in. So far we've seen top left and bottom
> +     * left conventions. This test may produce unexpected results if the
> +     * driver forces multisampling on us.
> +     *
Technically, this is about rasterisation of polygon edges; the pixels
stay where they are. :)
We're also not drawing "exactly between 4 pixels"; that case would be
well defined. Instead, we do quite the opposite, and align the corners
of the quad with the pixel centres. (Incidentally, it may not be bad
to draw a slightly larger quad, although I suppose it would complicate
the maths a little.)

Can the driver force multi-sampling on us here? My understanding was
that generally speaking those controls only affect the backbuffer, and
not FBOs; that was part of the reason for having the "SampleCount"
registry setting.

> +     * If we find a bottom-left filling behavior we also nudge the x axis
> +     * by the same amount. This is necessary to keep diagonals that go
> +     * through the pixel center intact.
> +     *
"x-axis", probably. (I'd also argue for "behaviour" and "centre", but
I suppose those are spelling variants more than they are spelling
errors.)

> +     * Note that we are ignoring some settings that might influence the
> +     * driver: How we switch GL to a upper-left coordinate system, if we
> +     * are drawing into the on-screen framebuffer (we don't have one here
> +     * because our window is hidden), shaders vs fixed function GL.
> +     * Testing these isn't possible with the current draw_test_quad()
> +     * infrastructure.
> +     *
Well, we do have an on-screen framebuffer here, we may just fail the
pixel ownership test. That's true regardless of whether we're drawing
to a hidden window though; an otherwise visible window may be
partially obscured by some other window, for example. I don't think
this is generally an issue on modern Linux. It's also somewhat moot;
if we're using FBOs, the application never draws directly to the
on-screen/default framebuffer, and if we're not using FBOs we don't
use wined3d_caps_gl_ctx_test_filling_convention().

"an upper-left coordinate system"

> +    if (readback[3][3] == 0xffff0000 && readback[3][4] == 0xff0000ff
> +            && readback[4][3] == 0xff0000ff && readback[4][4] == 0xff0000ff)
> +    {
> +        TRACE("Got the expected filling result.\n");
> +        return TRUE;
> +    }
> +    else if (readback[3][3] == 0xff0000ff && readback[3][4] == 0xff0000ff
> +            && readback[4][3] == 0xffff0000 && readback[4][4] == 0xff0000ff)
> +    {
> +        WARN("GPU uses a bottom-left filling convention in FBOs.\n");
> +        return FALSE;
> +    }
> +
> +    FIXME("Unexpected filling convention test result\n");
> +
> +    for (y = 0; y < ARRAY_SIZE(readback); ++y)
> +    {
> +        for (x = 0; x < ARRAY_SIZE(readback[0]); ++x)
> +        {
> +            FIXME("0x%08x ", readback[y][x]);
> +        }
> +        FIXME("\n");
> +    }
> +
Do we really need a FIXME for that? Getting e.g. a bottom-right result
would perhaps be unexpected, but it doesn't seem particularly
concerning.

> +     * In order to handle the pixel center, we translate by 0.5 / VPw and
> +     * 0.5 / VPh. We test the filling convention during adapter init and
> +     * add a small offset to correct it if necessary. See
> +     * wined3d_caps_gl_ctx_test_filling_convention for more details on how
> +     * we test GL and considerations regarding the added nudge value.
> +     *
"wined3d_caps_gl_ctx_test_filling_convention()"

>      clip_control = d3d_info->clip_control;
>      flip = !clip_control && context->render_offscreen;
> -    if (!clip_control && d3d_info->wined3d_creation_flags & WINED3D_PIXEL_CENTER_INTEGER)
> -        center_offset = 63.0f / 64.0f;
> +    if (!clip_control)
> +        center_offset = 1.0f + d3d_info->filling_convention_nudge;
>      else
> -        center_offset = -1.0f / 64.0f;
> +        center_offset = 0.0f;
>
This changes the logic for which offset is applied in which case.
Perhaps that makes sense, but it seems like an independent change.