Re: [08/23] wined3d: Allow viewport to be out-of-bounds if using offscreen target.

[Matteo Bruni]

2016-11-13 12:35 GMT-06:00 Andrew Wesie <awesie(a)gmail.com>:
> Signed-off-by: Andrew Wesie <awesie(a)gmail.com>
> ---
>  dlls/wined3d/state.c | 42 ++++++++++++++++++++++--------------------
>  1 file changed, 22 insertions(+), 20 deletions(-)
>
> diff --git a/dlls/wined3d/state.c b/dlls/wined3d/state.c
> index cd3763c..ed20e325 100644
> --- a/dlls/wined3d/state.c
> +++ b/dlls/wined3d/state.c
> @@ -4617,26 +4617,6 @@ static void viewport_miscpart(struct wined3d_context *context, const struct wine
>      struct wined3d_viewport vp = state->viewport;
>      unsigned int width, height;
>
> -    if (target)
> -    {
> -        if (vp.width > target->width)
> -            vp.width = target->width;
> -        if (vp.height > target->height)
> -            vp.height = target->height;
> -
> -        wined3d_rendertarget_view_get_drawable_size(target, context, &width, &height);
> -    }
> -    else if (depth_stencil)
> -    {
> -        width = depth_stencil->width;
> -        height = depth_stencil->height;
> -    }
> -    else
> -    {
> -        FIXME("No attachments draw calls not supported.\n");
> -        return;
> -    }
> -
>      gl_info->gl_ops.gl.p_glDepthRange(vp.min_z, vp.max_z);
>      checkGLcall("glDepthRange");
>      /* Note: GL requires lower left, DirectX supplies upper left. This is
> @@ -4644,7 +4624,29 @@ static void viewport_miscpart(struct wined3d_context *context, const struct wine
>      if (context->render_offscreen)
>          gl_info->gl_ops.gl.p_glViewport(vp.x, vp.y, vp.width, vp.height);
>      else
> +    {
> +        if (target)
> +        {
> +            if (vp.width > target->width)
> +                vp.width = target->width;
> +            if (vp.height > target->height)
> +                vp.height = target->height;
> +
> +            wined3d_rendertarget_view_get_drawable_size(target, context, &width, &height);
> +        }
> +        else if (depth_stencil)
> +        {
> +            width = depth_stencil->width;
> +            height = depth_stencil->height;
> +        }
> +        else
> +        {
> +            FIXME("No attachments draw calls not supported.\n");
> +            return;
> +        }
> +
>          gl_info->gl_ops.gl.p_glViewport(vp.x, (height - (vp.y + vp.height)), vp.width, vp.height);
> +    }
>      checkGLcall("glViewport");
>  }

You need to make the same change to viewport_miscpart_cc() (you can
blame me for that).

Again, it would be very nice to have a testcase for this. Extending
viewport_test() in d3d9/tests/visual.c and adding similar tests to the
other d3d versions shouldn't be too hard.

[08/23] wined3d: Allow viewport to be out-of-bounds if using offscreen target.

[Henri Verbeet]

On 13 November 2016 at 23:40, Matteo Bruni <matteo.mystral(a)gmail.com> wrote:
> You need to make the same change to viewport_miscpart_cc() (you can
> blame me for that).

Maybe a little. But yes, it would probably make sense to have a common
function for that.
> Again, it would be very nice to have a testcase for this. Extending
> viewport_test() in d3d9/tests/visual.c and adding similar tests to the
> other d3d versions shouldn't be too hard.

Well, the issue with that is that context->render_offscreen is always
true on common (modern) configurations. I.e., offscreen targets imply
context->render_offscreen, but the reverse isn't generally true.

Perhaps more importantly though, I suspect this change is more about
the wined3d_rendertarget_view_get_drawable_size() call than clamping
the viewport. There was an issue with calling
surface_get_drawable_size() for render targets that aren't 2D
textures, but commit 5cdb8f2486cf00a61c1aac20daef8c7cac0d8312 should
have mostly made that gone away.

We only need the drawable height in case context->render_offscreen is
false, so perhaps it makes sense to only retrieve it for that case,
but at this point that's only a (minor) optimisation.

[08/23] wined3d: Allow viewport to be out-of-bounds if using offscreen target.

[Matteo Bruni]

2016-11-14 8:52 GMT-06:00 Henri Verbeet <hverbeet(a)gmail.com>:
> On 13 November 2016 at 23:40, Matteo Bruni <matteo.mystral(a)gmail.com> wrote:
> > You need to make the same change to viewport_miscpart_cc() (you can
> > blame me for that).
>
> Maybe a little. But yes, it would probably make sense to have a common
> function for that.
> > Again, it would be very nice to have a testcase for this. Extending
> > viewport_test() in d3d9/tests/visual.c and adding similar tests to the
> > other d3d versions shouldn't be too hard.
>
> Well, the issue with that is that context->render_offscreen is always
> true on common (modern) configurations. I.e., offscreen targets imply
> context->render_offscreen, but the reverse isn't generally true.

Oh, you're right, I somehow overlooked it. That means this change is
quite suspicious, if the intention is really to avoid clamping
offscreen targets. But as you say...
> Perhaps more importantly though, I suspect this change is more about
> the wined3d_rendertarget_view_get_drawable_size() call than clamping
> the viewport. There was an issue with calling
> surface_get_drawable_size() for render targets that aren't 2D
> textures, but commit 5cdb8f2486cf00a61c1aac20daef8c7cac0d8312 should
> have mostly made that gone away.
>
> We only need the drawable height in case context->render_offscreen is
> false, so perhaps it makes sense to only retrieve it for that case,
> but at this point that's only a (minor) optimisation.

Yeah, that makes sense.