[PATCH 0/3] MR3211: wined3d: Don't force going through a texture when downloading from renderbuffers.
It can be unnecessary at best and unsupported at worst (e.g. no ARB_texture_multisample). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3211
From: Matteo Bruni <mbruni(a)codeweavers.com> --- 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. */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3211
From: Matteo Bruni <mbruni(a)codeweavers.com> 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; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3211
From: Matteo Bruni <mbruni(a)codeweavers.com> 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); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3211
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? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3211#note_37702
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?) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3211#note_37701
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 :-) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3211#note_37704
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. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3211#note_37785
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.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3211#note_37786
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. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3211#note_37808
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.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3211#note_37921
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. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3211#note_38161
participants (3)
-
Matteo Bruni -
Matteo Bruni (＠Mystral)
-
Zebediah Figura (＠zfigura)