[PATCH 0/3] MR3211: wined3d: Don't force going through a texture when downloading from renderbuffers.

[Matteo Bruni (＠Mystral)]

It can be unnecessary at best and unsupported at worst (e.g. no ARB_texture_multisample).

[PATCH 1/3] wined3d: Factor out a wined3d_context_gl_apply_blit_state_fbo() function.

[Matteo Bruni]

From: Matteo Bruni

---
 dlls/wined3d/context_gl.c | 51 +++++++++++++++++++++++++--------------
 1 file changed, 33 insertions(+), 18 deletions(-)

diff --git a/dlls/wined3d/context_gl.c b/dlls/wined3d/context_gl.c
index a8c5160a5b3..5b3630bf159 100644
--- a/dlls/wined3d/context_gl.c
+++ b/dlls/wined3d/context_gl.c
@@ -3236,6 +3236,34 @@ static uint32_t wined3d_context_gl_generate_rt_mask_no_fbo(const struct wined3d_
         return context_generate_rt_mask(wined3d_context_gl_get_offscreen_gl_buffer(context_gl));
 }

+void wined3d_context_gl_apply_blit_state_fbo(struct wined3d_context_gl *context_gl, GLenum target,
+        struct wined3d_texture *texture, unsigned int sub_resource_idx, DWORD location)
+{
+    const struct wined3d_format *format = texture->resource.format;
+    struct wined3d_context *context = &context_gl->c;
+
+    if (wined3d_settings.offscreen_rendering_mode == ORM_FBO)
+    {
+        if (location != WINED3D_LOCATION_DRAWABLE)
+        {
+            if (format->depth_size || format->stencil_size)
+                wined3d_context_gl_apply_fbo_state_blit(context_gl, target, NULL,
+                        0, &texture->resource, sub_resource_idx, location);
+            else
+                wined3d_context_gl_apply_fbo_state_blit(context_gl, target, &texture->resource,
+                        sub_resource_idx, NULL, 0, location);
+        }
+        else
+        {
+            context_gl->current_fbo = NULL;
+            wined3d_context_gl_bind_fbo(context_gl, target, 0);
+        }
+        wined3d_context_gl_check_fbo_status(context_gl, target);
+    }
+
+    context_invalidate_state(context, STATE_FRAMEBUFFER);
+}
+
 /* Context activation is done by the caller. */
 void wined3d_context_gl_apply_blit_state(struct wined3d_context_gl *context_gl, const struct wined3d_device *device)
 {
@@ -3251,14 +3279,13 @@ void wined3d_context_gl_apply_blit_state(struct wined3d_context_gl *context_gl,
     gl_info = context_gl->gl_info;
     rt = context->current_rt.texture;

+    wined3d_context_gl_apply_blit_state_fbo(context_gl, GL_FRAMEBUFFER, rt, context->current_rt.sub_resource_idx,
+            rt->resource.draw_binding);
+
     if (wined3d_settings.offscreen_rendering_mode == ORM_FBO)
     {
         if (context->render_offscreen)
         {
-            wined3d_texture_load(rt, context, FALSE);
-
-            wined3d_context_gl_apply_fbo_state_blit(context_gl, GL_FRAMEBUFFER, &rt->resource,
-                    context->current_rt.sub_resource_idx, NULL, 0, rt->resource.draw_binding);
             if (rt->resource.format->id != WINED3DFMT_NULL)
                 rt_mask = 1;
             else
@@ -3266,8 +3293,6 @@ void wined3d_context_gl_apply_blit_state(struct wined3d_context_gl *context_gl,
         }
         else
         {
-            context_gl->current_fbo = NULL;
-            wined3d_context_gl_bind_fbo(context_gl, GL_FRAMEBUFFER, 0);
             rt_mask = context_generate_rt_mask_from_resource(&rt->resource);
         }
     }
@@ -3284,10 +3309,6 @@ void wined3d_context_gl_apply_blit_state(struct wined3d_context_gl *context_gl,
         *cur_mask = rt_mask;
     }

-    if (wined3d_settings.offscreen_rendering_mode == ORM_FBO)
-        wined3d_context_gl_check_fbo_status(context_gl, GL_FRAMEBUFFER);
-    context_invalidate_state(context, STATE_FRAMEBUFFER);
-
     wined3d_context_gl_get_rt_size(context_gl, &rt_size);

if (context->last_was_blit)
@@ -3650,18 +3671,14 @@ void context_gl_apply_texture_draw_state(struct wined3d_context_gl *context_gl,
     if (wined3d_settings.offscreen_rendering_mode != ORM_FBO)
         return;

+    wined3d_context_gl_apply_blit_state_fbo(context_gl, GL_DRAW_FRAMEBUFFER,
+            texture, sub_resource_idx, location);
     if (format->depth_size || format->stencil_size)
     {
-        wined3d_context_gl_apply_fbo_state_blit(context_gl, GL_DRAW_FRAMEBUFFER,
-                NULL, 0, &texture->resource, sub_resource_idx, location);
-
         buffer = GL_NONE;
     }
     else
     {
-        wined3d_context_gl_apply_fbo_state_blit(context_gl, GL_DRAW_FRAMEBUFFER,
-                &texture->resource, sub_resource_idx, NULL, 0, location);
-
         if (location == WINED3D_LOCATION_DRAWABLE)
         {
             TRACE("Texture %p is onscreen.\n", texture);
@@ -3675,8 +3692,6 @@ void context_gl_apply_texture_draw_state(struct wined3d_context_gl *context_gl,
     }

wined3d_context_gl_set_draw_buffer(context_gl, buffer);
-    wined3d_context_gl_check_fbo_status(context_gl, GL_DRAW_FRAMEBUFFER);
-    context_invalidate_state(&context_gl->c, STATE_FRAMEBUFFER);
 }

/* Context activation is done by the caller. */

[PATCH 2/3] wined3d: Use wined3d_context_gl_apply_blit_state_fbo() elsewhere in wined3d.

[Matteo Bruni]

From: Matteo Bruni

Strict superset of the older wined3d_context_gl_apply_fbo_state_blit()
function it replaces, correctly handles depth/stencil textures.
---
 dlls/wined3d/context_gl.c      | 6 +++---
 dlls/wined3d/surface.c         | 6 ++----
 dlls/wined3d/texture.c         | 8 ++++----
 dlls/wined3d/wined3d_private.h | 5 ++---
 4 files changed, 11 insertions(+), 14 deletions(-)

diff --git a/dlls/wined3d/context_gl.c b/dlls/wined3d/context_gl.c
index 5b3630bf159..5ddb2fa3a93 100644
--- a/dlls/wined3d/context_gl.c
+++ b/dlls/wined3d/context_gl.c
@@ -849,7 +849,7 @@ static void wined3d_context_gl_apply_fbo_state(struct wined3d_context_gl *contex
 }

/* Context activation is done by the caller. */
-void wined3d_context_gl_apply_fbo_state_blit(struct wined3d_context_gl *context_gl, GLenum target,
+static void context_gl_apply_fbo_state_blit(struct wined3d_context_gl *context_gl, GLenum target,
         struct wined3d_resource *rt, unsigned int rt_sub_resource_idx,
         struct wined3d_resource *ds, unsigned int ds_sub_resource_idx, uint32_t location)
 {
@@ -3247,10 +3247,10 @@ void wined3d_context_gl_apply_blit_state_fbo(struct wined3d_context_gl *context_
         if (location != WINED3D_LOCATION_DRAWABLE)
         {
             if (format->depth_size || format->stencil_size)
-                wined3d_context_gl_apply_fbo_state_blit(context_gl, target, NULL,
+                context_gl_apply_fbo_state_blit(context_gl, target, NULL,
                         0, &texture->resource, sub_resource_idx, location);
             else
-                wined3d_context_gl_apply_fbo_state_blit(context_gl, target, &texture->resource,
+                context_gl_apply_fbo_state_blit(context_gl, target, &texture->resource,
                         sub_resource_idx, NULL, 0, location);
         }
         else
diff --git a/dlls/wined3d/surface.c b/dlls/wined3d/surface.c
index fd8db9ce60d..760682eae81 100644
--- a/dlls/wined3d/surface.c
+++ b/dlls/wined3d/surface.c
@@ -373,10 +373,8 @@ void texture2d_read_from_framebuffer(struct wined3d_texture *texture, unsigned i

if (src_location != resource->draw_binding)
     {
-        wined3d_context_gl_apply_fbo_state_blit(context_gl, GL_READ_FRAMEBUFFER,
-                resource, sub_resource_idx, NULL, 0, src_location);
-        wined3d_context_gl_check_fbo_status(context_gl, GL_READ_FRAMEBUFFER);
-        context_invalidate_state(context, STATE_FRAMEBUFFER);
+        wined3d_context_gl_apply_blit_state_fbo(context_gl, GL_READ_FRAMEBUFFER,
+                texture, sub_resource_idx, src_location);
     }
     else
     {
diff --git a/dlls/wined3d/texture.c b/dlls/wined3d/texture.c
index 32923349c6b..359e3719172 100644
--- a/dlls/wined3d/texture.c
+++ b/dlls/wined3d/texture.c
@@ -529,8 +529,8 @@ static void texture2d_blt_fbo(struct wined3d_device *device, struct wined3d_cont
         buffer = GL_COLOR_ATTACHMENT0;
     }

-    wined3d_context_gl_apply_fbo_state_blit(context_gl, GL_READ_FRAMEBUFFER,
-            &src_texture->resource, src_sub_resource_idx, NULL, 0, src_location);
+    wined3d_context_gl_apply_blit_state_fbo(context_gl, GL_READ_FRAMEBUFFER,
+            src_texture, src_sub_resource_idx, src_location);
     gl_info->gl_ops.gl.p_glReadBuffer(buffer);
     checkGLcall("glReadBuffer()");
     wined3d_context_gl_check_fbo_status(context_gl, GL_READ_FRAMEBUFFER);
@@ -630,8 +630,8 @@ static void texture2d_depth_blt_fbo(const struct wined3d_device *device, struct
     else
         wined3d_texture_prepare_location(dst_texture, dst_sub_resource_idx, context, dst_location);

-    wined3d_context_gl_apply_fbo_state_blit(context_gl, GL_READ_FRAMEBUFFER, NULL, 0,
-            &src_texture->resource, src_sub_resource_idx, src_location);
+    wined3d_context_gl_apply_blit_state_fbo(context_gl, GL_READ_FRAMEBUFFER,
+            src_texture, src_sub_resource_idx, src_location);
     wined3d_context_gl_check_fbo_status(context_gl, GL_READ_FRAMEBUFFER);

context_gl_apply_texture_draw_state(context_gl, dst_texture, dst_sub_resource_idx, dst_location);
diff --git a/dlls/wined3d/wined3d_private.h b/dlls/wined3d/wined3d_private.h
index 001534e8c38..6881868a2db 100644
--- a/dlls/wined3d/wined3d_private.h
+++ b/dlls/wined3d/wined3d_private.h
@@ -2467,11 +2467,10 @@ GLuint wined3d_context_gl_allocate_vram_chunk_buffer(struct wined3d_context_gl *
         unsigned int pool, size_t size) DECLSPEC_HIDDEN;
 void wined3d_context_gl_apply_blit_state(struct wined3d_context_gl *context_gl,
         const struct wined3d_device *device) DECLSPEC_HIDDEN;
+void wined3d_context_gl_apply_blit_state_fbo(struct wined3d_context_gl *context_gl, GLenum target,
+        struct wined3d_texture *texture, unsigned int sub_resource_idx, DWORD location) DECLSPEC_HIDDEN;
 BOOL wined3d_context_gl_apply_clear_state(struct wined3d_context_gl *context_gl, const struct wined3d_state *state,
         unsigned int rt_count, const struct wined3d_fb_state *fb) DECLSPEC_HIDDEN;
-void wined3d_context_gl_apply_fbo_state_blit(struct wined3d_context_gl *context_gl, GLenum target,
-        struct wined3d_resource *rt, unsigned int rt_sub_resource_idx,
-        struct wined3d_resource *ds, unsigned int ds_sub_resource_idx, uint32_t location) DECLSPEC_HIDDEN;
 void wined3d_context_gl_apply_ffp_blit_state(struct wined3d_context_gl *context_gl,
         const struct wined3d_device *device) DECLSPEC_HIDDEN;
 void wined3d_context_gl_bind_bo(struct wined3d_context_gl *context_gl, GLenum binding, GLuint name) DECLSPEC_HIDDEN;

[PATCH 3/3] wined3d: Don't force going through a texture when downloading from renderbuffers.

[Matteo Bruni]

From: Matteo Bruni

It can be unnecessary at best and unsupported at worst (e.g. no
ARB_texture_multisample).
---
 dlls/wined3d/surface.c | 23 +++++++++--------------
 dlls/wined3d/texture.c | 16 +++++++---------
 2 files changed, 16 insertions(+), 23 deletions(-)

diff --git a/dlls/wined3d/surface.c b/dlls/wined3d/surface.c
index 760682eae81..7b97bfcfe03 100644
--- a/dlls/wined3d/surface.c
+++ b/dlls/wined3d/surface.c
@@ -358,33 +358,28 @@ void texture2d_read_from_framebuffer(struct wined3d_texture *texture, unsigned i
     uint8_t *offset;
     unsigned int i;

+    TRACE("texture %p, sub_resource_idx %u, context %p, src_location %s, dst_location %s.\n",
+            texture, sub_resource_idx, context, wined3d_debug_location(src_location), wined3d_debug_location(dst_location));
+
     /* dst_location was already prepared by the caller. */
     wined3d_texture_get_bo_address(texture, sub_resource_idx, &data, dst_location);
     offset = data.addr;

restore_texture = context->current_rt.texture;
     restore_idx = context->current_rt.sub_resource_idx;
-    if (restore_texture != texture || restore_idx != sub_resource_idx)
+    if (!wined3d_resource_is_offscreen(resource) && (restore_texture != texture || restore_idx != sub_resource_idx))
         context = context_acquire(device, texture, sub_resource_idx);
     else
         restore_texture = NULL;
     context_gl = wined3d_context_gl(context);
     gl_info = context_gl->gl_info;

-    if (src_location != resource->draw_binding)
-    {
-        wined3d_context_gl_apply_blit_state_fbo(context_gl, GL_READ_FRAMEBUFFER,
-                texture, sub_resource_idx, src_location);
-    }
-    else
-    {
-        wined3d_context_gl_apply_blit_state(context_gl, device);
-    }
+    wined3d_context_gl_apply_blit_state_fbo(context_gl, GL_READ_FRAMEBUFFER,
+            texture, sub_resource_idx, src_location);

-    /* Select the correct read buffer, and give some debug output.
-     * There is no need to keep track of the current read buffer or reset it,
-     * every part of the code that reads sets the read buffer as desired.
-     */
+    /* Select the correct read buffer, and give some debug output. There is no
+     * need to keep track of the current read buffer or reset it, every part
+     * of the code that reads pixels sets the read buffer as desired. */
     if (src_location != WINED3D_LOCATION_DRAWABLE || wined3d_resource_is_offscreen(resource))
     {
         /* Mapping the primary render target which is not on a swapchain.
diff --git a/dlls/wined3d/texture.c b/dlls/wined3d/texture.c
index 359e3719172..01d7eaa14f1 100644
--- a/dlls/wined3d/texture.c
+++ b/dlls/wined3d/texture.c
@@ -2971,18 +2971,17 @@ static BOOL wined3d_texture_gl_load_sysmem(struct wined3d_texture_gl *texture_gl
     sub_resource = &texture_gl->t.sub_resources[sub_resource_idx];

/* We cannot download data from multisample textures directly. */
-    if (wined3d_texture_gl_is_multisample_location(texture_gl, WINED3D_LOCATION_TEXTURE_RGB))
-    {
+    if (wined3d_texture_gl_is_multisample_location(texture_gl, WINED3D_LOCATION_TEXTURE_RGB)
+            || sub_resource->locations & WINED3D_LOCATION_RB_MULTISAMPLE)
         wined3d_texture_load_location(&texture_gl->t, sub_resource_idx, &context_gl->c, WINED3D_LOCATION_RB_RESOLVED);
-        texture2d_read_from_framebuffer(&texture_gl->t, sub_resource_idx, &context_gl->c,
+
+    if (sub_resource->locations & WINED3D_LOCATION_RB_RESOLVED)
+    {
+            texture2d_read_from_framebuffer(&texture_gl->t, sub_resource_idx, &context_gl->c,
                 WINED3D_LOCATION_RB_RESOLVED, dst_location);
         return TRUE;
     }

-    if (sub_resource->locations & (WINED3D_LOCATION_RB_MULTISAMPLE | WINED3D_LOCATION_RB_RESOLVED))
-        wined3d_texture_load_location(&texture_gl->t, sub_resource_idx, &context_gl->c, WINED3D_LOCATION_TEXTURE_RGB);
-
-    /* Download the sub-resource to system memory. */
     if (sub_resource->locations & (WINED3D_LOCATION_TEXTURE_RGB | WINED3D_LOCATION_TEXTURE_SRGB))
     {
         unsigned int row_pitch, slice_pitch, level;
@@ -3003,8 +3002,7 @@ static BOOL wined3d_texture_gl_load_sysmem(struct wined3d_texture_gl *texture_gl
         return TRUE;
     }

-    if (!(texture_gl->t.resource.bind_flags & WINED3D_BIND_DEPTH_STENCIL)
-            && (sub_resource->locations & WINED3D_LOCATION_DRAWABLE))
+    if (sub_resource->locations & WINED3D_LOCATION_DRAWABLE)
     {
         texture2d_read_from_framebuffer(&texture_gl->t, sub_resource_idx, &context_gl->c,
                 texture_gl->t.resource.draw_binding, dst_location);

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about dlls/wined3d/context_gl.c:
>  gl_info = context_gl->gl_info;
>  rt = context->current_rt.texture;
>
> - wined3d_context_gl_apply_blit_state_fbo(context_gl, GL_FRAMEBUFFER, rt, context->current_rt.sub_resource_idx,
> -
>        rt->resource.draw_binding);
> - if (wined3d_settings.offscreen_rendering_mode == ORM_FBO)
>    {
>        if (context->render_offscreen)
>        {
> -
>        wined3d_texture_load(rt, context, FALSE);

I don't know why this is here in the first place, so is it safe to remove? (Should it be in its own patch?)

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about dlls/wined3d/context_gl.c:
> - {
> -
>    if (location != WINED3D_LOCATION_DRAWABLE)
> -
>    {
> -
>        if (format->depth_size || format->stencil_size)
> -
>            wined3d_context_gl_apply_fbo_state_blit(context_gl, target, NULL,
> -
>                    0, &texture->resource, sub_resource_idx, location);
> -
>        else
> -
>            wined3d_context_gl_apply_fbo_state_blit(context_gl, target, &texture->resource,
> -
>                    sub_resource_idx, NULL, 0, location);
> -
>    }
> -
>    else
> -
>    {
> -
>        context_gl->current_fbo = NULL;
> -
>        wined3d_context_gl_bind_fbo(context_gl, target, 0);
> -
>    }
> -
>    wined3d_context_gl_check_fbo_status(context_gl, target);

This commit moves the FBO status check before the glDrawBuffer() call. I'm not quite sure but reading through the definition of framebuffer completeness, I think it *does* indeed depend on the draw buffers?

And since this function already does at least one thing not directly related to FBOs, should we move the glDrawBuffer calls here too?

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about dlls/wined3d/context_gl.c:
> - if (wined3d_settings.offscreen_rendering_mode == ORM_FBO)
> - {
> -
>    if (location != WINED3D_LOCATION_DRAWABLE)
> -
>    {
> -
>        if (format->depth_size || format->stencil_size)
> -
>            wined3d_context_gl_apply_fbo_state_blit(context_gl, target, NULL,
> -
>                    0, &texture->resource, sub_resource_idx, location);
> -
>        else
> -
>            wined3d_context_gl_apply_fbo_state_blit(context_gl, target, &texture->resource,
> -
>                    sub_resource_idx, NULL, 0, location);
> -
>    }
> -
>    else
> -
>    {
> -
>        context_gl->current_fbo = NULL;
> -
>        wined3d_context_gl_bind_fbo(context_gl, target, 0);
> -
>    }

wined3d_context_gl_apply_fbo_state_blit() already does this. Do we need it? Perhaps more importantly, is it safe to skip that rebind_fbo logic?

I really am not familiar with this code [I don't even know what the rebind_fbo stuff is for] and haven't spent a lot of effort to bring myself up to speed, so I'm hoping you are more familiar, enough to be able to answer those questions more easily :-)

[Matteo Bruni (＠Mystral)]

On Mon Jul  3 22:28:53 2023 +0000, Zebediah Figura wrote:
> I don't know why this is here in the first place, so is it safe to
> remove? (Should it be in its own patch?)

I think the callers already do essentially the same if necessary, and this one call inside two branches seems unlikely to cover it in case they don't.

It makes sense to have this change in a separate patch though. I'll update and resend.

[Matteo Bruni (＠Mystral)]

On Mon Jul  3 22:28:54 2023 +0000, Zebediah Figura wrote:
> This commit moves the FBO status check before the glDrawBuffer() call.
> I'm not quite sure but reading through the definition of framebuffer
> completeness, I think it *does* indeed depend on the draw buffers?
> And since this function already does at least one thing not directly
> related to FBOs, should we move the glDrawBuffer calls here too?

You're correct WRT the FBO status check. I'll move the glDrawBuffer() calls into the function and see how that looks like.

[Matteo Bruni (＠Mystral)]

On Mon Jul  3 22:29:39 2023 +0000, Zebediah Figura wrote:
> wined3d_context_gl_apply_fbo_state_blit() already does this. Do we need
> it? Perhaps more importantly, is it safe to skip that rebind_fbo logic?
> I really am not familiar with this code [I don't even know what the
> rebind_fbo stuff is for] and haven't spent a lot of effort to bring
> myself up to speed, so I'm hoping you are more familiar, enough to be
> able to answer those questions more easily :-)

You mean wined3d_context_gl_apply_fbo_state_blit() -> wined3d_context_gl_apply_fbo_state() (yeah, naming is not great -_-) already binding FBO 0 for WINED3D_LOCATION_DRAWABLE? I could call wined3d_context_gl_apply_fbo_state[_blit]() instead for that case as well, true. It should be a bit more correct for rebind_fbo I guess, in the (very unlikely, but...) chance that a texture attached to the other FBO binding target was just updated AND we have the rebind quirk enabled.

I'll update this too.

[Zebediah Figura (＠zfigura)]

On Tue Jul  4 13:32:42 2023 +0000, Matteo Bruni wrote:
> I think the callers already do essentially the same if necessary, and
> this one call inside two branches seems unlikely to cover it in case
> they don't.
> It makes sense to have this change in a separate patch though. I'll
> update and resend.

Hmm, looking through this, this is called either from the blitter (in which case we are blitting *to* current_rt and shouldn't need to load it, though we may need to prepare it) or from texture2d_read_from_framebuffer(). In either case I'm not sure that we're actually preparing/loading the texture in the caller, though? I do think it should be the caller's responsibility, but I'm not sure that the callers are correctly handling things.

[Matteo Bruni (＠Mystral)]

> Hmm, looking through this, this is called either from the blitter (in which case we are blitting *to* current_rt and shouldn't need to load it, though we may need to prepare it)

We still need to load it in the general case, in case of a partial blit.
> In either case I'm not sure that we're actually preparing/loading the texture in the caller, though? I do think it should be the caller's responsibility, but I'm not sure that the callers are correctly handling things.

It looks like they are. Anyway, I ended up reworking things a bit and putting the removal of the `wined3d_texture_load()` call into its own patch.