[PATCH 0/2] MR10190: winu32: Try harder to replace the old drawable everywhere.
From: Paul Gofman <pgofman@codeweavers.com> --- dlls/opengl32/unix_wgl.c | 3 +++ dlls/win32u/opengl.c | 8 ++++---- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/dlls/opengl32/unix_wgl.c b/dlls/opengl32/unix_wgl.c index 6d73b1cd735..09f51584dce 100644 --- a/dlls/opengl32/unix_wgl.c +++ b/dlls/opengl32/unix_wgl.c @@ -1388,6 +1388,9 @@ static void flush_context( TEB *teb, void (*flush)(void) ) if (flush) flush(); } + /* Drawables might have been replaced in wgl_context_flush(). */ + ctx = get_current_context( teb, &read, &draw ); + if (flags & GL_FLUSH_FORCE_SWAP) { GLenum mask = GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT | GL_STENCIL_BUFFER_BIT; diff --git a/dlls/win32u/opengl.c b/dlls/win32u/opengl.c index 486c3f45c2a..a3884ad41fb 100644 --- a/dlls/win32u/opengl.c +++ b/dlls/win32u/opengl.c @@ -1843,10 +1843,10 @@ static BOOL context_sync_drawables( struct opengl_context *context, HDC draw_hdc { NtCurrentTeb()->glContext = context; - if (old_draw && old_draw != new_draw && old_draw != new_read && old_draw->client) - set_window_opengl_drawable( old_draw->client->hwnd, old_draw, FALSE ); - if (old_read && old_read != new_draw && old_read != new_read && old_read->client) - set_window_opengl_drawable( old_read->client->hwnd, old_read, FALSE ); + if (old_draw && old_draw != new_draw && old_draw != new_read && old_draw->client && new_draw && new_draw->client) + set_window_opengl_drawable( old_draw->client->hwnd, new_draw, FALSE ); + if (old_read && old_read != new_draw && old_read != new_read && old_read->client && new_read && new_read->client) + set_window_opengl_drawable( old_read->client->hwnd, new_read, FALSE ); /* all good, release previous context drawables if any */ if (old_draw) opengl_drawable_release( old_draw ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/10190
From: Paul Gofman <pgofman@codeweavers.com> --- dlls/win32u/opengl.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/dlls/win32u/opengl.c b/dlls/win32u/opengl.c index a3884ad41fb..a9169ef15f8 100644 --- a/dlls/win32u/opengl.c +++ b/dlls/win32u/opengl.c @@ -1789,8 +1789,9 @@ static BOOL context_unset_current( struct opengl_context *context ) } /* return an updated drawable, recreating one if the window drawables have been invalidated (mostly wineandroid) */ -static struct opengl_drawable *get_updated_drawable( HDC hdc, int format, struct opengl_drawable *drawable ) +static struct opengl_drawable *get_updated_drawable( HDC hdc, int format, struct opengl_drawable *drawable, HDC update_dc ) { + struct opengl_drawable *ret; HWND hwnd = NULL; /* return the memory DCs drawables directly */ @@ -1809,7 +1810,9 @@ static struct opengl_drawable *get_updated_drawable( HDC hdc, int format, struct if (hdc && (drawable = get_dc_opengl_drawable( hdc ))) return drawable; /* get an updated drawable with the desired format */ - return get_window_unused_drawable( hwnd, format ); + ret = get_window_unused_drawable( hwnd, format ); + if (ret && update_dc) set_dc_opengl_drawable( update_dc, ret ); + return ret; } static BOOL context_sync_drawables( struct opengl_context *context, HDC draw_hdc, HDC read_hdc ) @@ -1818,10 +1821,12 @@ static BOOL context_sync_drawables( struct opengl_context *context, HDC draw_hdc struct opengl_context *previous = NtCurrentTeb()->glContext; BOOL ret = FALSE; - if (!(new_draw = get_updated_drawable( draw_hdc, context->format, context->draw ))) return FALSE; + if (!(new_draw = get_updated_drawable( draw_hdc, context->format, context->draw, + draw_hdc ? draw_hdc : NtCurrentTeb()->glReserved1[0] ))) return FALSE; if (!draw_hdc && context->draw == context->read) opengl_drawable_add_ref( (new_read = new_draw) ); else if (draw_hdc && draw_hdc == read_hdc) opengl_drawable_add_ref( (new_read = new_draw) ); - else new_read = get_updated_drawable( read_hdc, context->format, context->read ); + else new_read = get_updated_drawable( read_hdc, context->format, context->read, + read_hdc ? read_hdc : NtCurrentTeb()->glReserved1[1] ); TRACE( "context %p, new_draw %s, new_read %s\n", context, debugstr_opengl_drawable( new_draw ), debugstr_opengl_drawable( new_read ) ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/10190
Currently if the drawable gets updated the old one will stay as the window current drawable and on the DC (where it got when SetPixelFormat was called on it). Currently if drawable gets updated in get_updated_drawable() it is essentially leaked (will be destroyed only once the context and the window are destroyed), and also it may be occasionally updated and flicker onscreen / offscreen fighting with actual new surface (from update_client_surfaces()). I suppose what the first patch changes is essentially a typo and the intention was to update window to the new drawable. Then, I don't see why would we want to leave the old surface on the current context once we replaced it (fixed in patch 2). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10190#note_130552
Rémi Bernon (@rbernon) commented about dlls/win32u/opengl.c:
{ NtCurrentTeb()->glContext = context;
- if (old_draw && old_draw != new_draw && old_draw != new_read && old_draw->client) - set_window_opengl_drawable( old_draw->client->hwnd, old_draw, FALSE ); - if (old_read && old_read != new_draw && old_read != new_read && old_read->client) - set_window_opengl_drawable( old_read->client->hwnd, old_read, FALSE ); + if (old_draw && old_draw != new_draw && old_draw != new_read && old_draw->client && new_draw && new_draw->client) + set_window_opengl_drawable( old_draw->client->hwnd, new_draw, FALSE ); + if (old_read && old_read != new_draw && old_read != new_read && old_read->client && new_read && new_read->client) + set_window_opengl_drawable( old_read->client->hwnd, new_read, FALSE );
`set_window_opengl_drawable( ..., FALSE )` sets the window "unused drawable", which was there to keep for future reuse and avoid frequent drawable deallocation / reallocation. It's wrong to register the newly created and used drawable as unused, as it may then be reused by another thread which will try to make it current there too. This was mostly meant for D3D drawables as they often get switched back and forth but now that we use owned drawable per swapchain DC drawables it's probably not as necessary as it was and we could get rid of it. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10190#note_130596
Rémi Bernon (@rbernon) commented about dlls/opengl32/unix_wgl.c:
funcs->p_glBlitFramebuffer( 0, 0, rect.right, rect.bottom, 0, 0, rect.right, rect.bottom, mask, GL_NEAREST ); if (ctx->read_fbo) funcs->p_glBindFramebuffer( GL_READ_FRAMEBUFFER, ctx->read_fbo ); else funcs->p_glReadBuffer( drawable_buffer_from_buffer( read, ctx->pixel_mode.read_buffer ) ); }
We only use draw for `draw->client->hwnd`, could we hoist it earlier instead? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10190#note_130597
Rémi Bernon (@rbernon) commented about dlls/win32u/opengl.c:
if (hdc && (drawable = get_dc_opengl_drawable( hdc ))) return drawable;
/* get an updated drawable with the desired format */ - return get_window_unused_drawable( hwnd, format ); + ret = get_window_unused_drawable( hwnd, format ); + if (ret && update_dc) set_dc_opengl_drawable( update_dc, ret );
Only D3D swapchain and pbuffer DCs owns their drawables, and they are handled before this. This will register window-owned drawables with their DCs which is unnecessary, and caused some deadlock issues in the past when the DCs are released and destroyed and the drawable released. That issue might have been resolved, but I think it's still unnecessary and it'll cause any later use of the HDC to be treated as a D3D owning DC, which isn't the case. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10190#note_130598
participants (3)
-
Paul Gofman -
Paul Gofman (＠gofman)
-
Rémi Bernon (＠rbernon)