[PATCH 0/1] MR7227: winex11.drv: Choose alpha-enabled xrender format for argb drawables.
From: Paul Gofman pgofman@codeweavers.com
--- dlls/winex11.drv/opengl.c | 2 +- dlls/winex11.drv/window.c | 19 ++++++++++++------- dlls/winex11.drv/x11drv.h | 2 ++ dlls/winex11.drv/xrender.c | 38 +++++++++++++++++++++++++++++--------- 4 files changed, 44 insertions(+), 17 deletions(-)
diff --git a/dlls/winex11.drv/opengl.c b/dlls/winex11.drv/opengl.c index 74f78482b41..158f2f846f2 100644 --- a/dlls/winex11.drv/opengl.c +++ b/dlls/winex11.drv/opengl.c @@ -2292,7 +2292,7 @@ static BOOL X11DRV_wglDestroyPbufferARB( struct wgl_pbuffer *object ) */ static HDC X11DRV_wglGetPbufferDCARB( struct wgl_pbuffer *object ) { - struct x11drv_escape_set_drawable escape; + struct x11drv_escape_set_drawable escape = { 0 }; struct gl_drawable *prev; HDC hdc;
diff --git a/dlls/winex11.drv/window.c b/dlls/winex11.drv/window.c index b13c54531ac..d86321a4649 100644 --- a/dlls/winex11.drv/window.c +++ b/dlls/winex11.drv/window.c @@ -2694,7 +2694,7 @@ Window X11DRV_get_whole_window( HWND hwnd ) void X11DRV_GetDC( HDC hdc, HWND hwnd, HWND top, const RECT *win_rect, const RECT *top_rect, DWORD flags ) { - struct x11drv_escape_set_drawable escape; + struct x11drv_escape_set_drawable escape = { 0 };
escape.code = X11DRV_SET_DRAWABLE; escape.mode = IncludeInferiors; @@ -2709,11 +2709,16 @@ void X11DRV_GetDC( HDC hdc, HWND hwnd, HWND top, const RECT *win_rect, { struct x11drv_win_data *data = get_win_data( hwnd );
- escape.drawable = data ? data->whole_window : X11DRV_get_whole_window( hwnd ); - - /* special case: when repainting the root window, clip out top-level windows */ - if (data && data->whole_window == root_window) escape.mode = ClipByChildren; - release_win_data( data ); + if (data) + { + escape.drawable = data->whole_window; + escape.use_alpha = data->use_alpha; + escape.visual = data->vis; + /* special case: when repainting the root window, clip out top-level windows */ + if (data->whole_window == root_window) escape.mode = ClipByChildren; + release_win_data( data ); + } + else escape.drawable = X11DRV_get_whole_window( hwnd ); } else { @@ -2730,7 +2735,7 @@ void X11DRV_GetDC( HDC hdc, HWND hwnd, HWND top, const RECT *win_rect, */ void X11DRV_ReleaseDC( HWND hwnd, HDC hdc ) { - struct x11drv_escape_set_drawable escape; + struct x11drv_escape_set_drawable escape = { 0 };
escape.code = X11DRV_SET_DRAWABLE; escape.drawable = root_window; diff --git a/dlls/winex11.drv/x11drv.h b/dlls/winex11.drv/x11drv.h index ee5bbe07dd4..9e73e9d473f 100644 --- a/dlls/winex11.drv/x11drv.h +++ b/dlls/winex11.drv/x11drv.h @@ -344,6 +344,8 @@ struct x11drv_escape_set_drawable Drawable drawable; /* X drawable */ int mode; /* ClipByChildren or IncludeInferiors */ RECT dc_rect; /* DC rectangle relative to drawable */ + BOOL use_alpha; /* drawable use an alpha channel */ + XVisualInfo visual; /* X visual used by drawable, may be unspecified when alpha is not used. */ };
struct x11drv_escape_get_drawable diff --git a/dlls/winex11.drv/xrender.c b/dlls/winex11.drv/xrender.c index bac3e6fac4b..968b79b38d2 100644 --- a/dlls/winex11.drv/xrender.c +++ b/dlls/winex11.drv/xrender.c @@ -250,15 +250,14 @@ static BOOL get_xrender_template(const WineXRenderFormatTemplate *fmt, XRenderPi return TRUE; }
-static BOOL is_wxrformat_compatible_with_default_visual(const WineXRenderFormatTemplate *fmt) +static BOOL is_wxrformat_compatible_with_visual(const WineXRenderFormatTemplate *fmt, const XVisualInfo *visual, BOOL use_alpha) { - if(fmt->depth != default_visual.depth) return FALSE; - if( (fmt->redMask << fmt->red) != default_visual.red_mask) return FALSE; - if( (fmt->greenMask << fmt->green) != default_visual.green_mask) return FALSE; - if( (fmt->blueMask << fmt->blue) != default_visual.blue_mask) return FALSE; + if(fmt->depth != visual->depth) return FALSE; + if( (fmt->redMask << fmt->red) != visual->red_mask) return FALSE; + if( (fmt->greenMask << fmt->green) != visual->green_mask) return FALSE; + if( (fmt->blueMask << fmt->blue) != visual->blue_mask) return FALSE;
- /* We never select a default ARGB visual */ - if(fmt->alphaMask) return FALSE; + if(!fmt->alphaMask != !use_alpha) return FALSE; return TRUE; }
@@ -278,7 +277,7 @@ static int load_xrender_formats(void) TRACE( "Loaded root pict_format with id=%#lx\n", pict_formats[i]->id ); continue; } - if(is_wxrformat_compatible_with_default_visual(&wxr_formats_template[i])) + if(is_wxrformat_compatible_with_visual(&wxr_formats_template[i], &default_visual, FALSE)) { pict_formats[i] = pXRenderFindVisualFormat(gdi_display, default_visual.visual); if (!pict_formats[i]) @@ -985,8 +984,29 @@ static INT xrenderdrv_ExtEscape( PHYSDEV dev, INT escape, INT in_count, LPCVOID BOOL ret = dev->funcs->pExtEscape( dev, escape, in_count, in_data, out_count, out_data ); if (ret) { + const struct x11drv_escape_set_drawable *set = in_data; + enum wxr_format format = default_format; + unsigned int i; + + /* When alpha is not used visual format should be compatible with default and visual info may be + * not specified. */ + if (set->use_alpha) + { + for (i = 0; i < WXR_NB_FORMATS; ++i) + { + if (!pict_formats[i]) continue; + if (is_wxrformat_compatible_with_visual( &wxr_formats_template[i], &set->visual, TRUE )) + { + TRACE( "Found argb format %u.\n", i ); + format = i; + break; + } + } + if (i == WXR_NB_FORMATS) + WARN( "Format not found for argb visual.\n" ); + } free_xrender_picture( physdev ); - set_physdev_format( physdev, default_format ); + set_physdev_format( physdev, format ); } return ret; }
Hi,
It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated.
The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details:
The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=151151
Your paranoid android.
=== debian11b (64 bit WoW report) ===
Report validation errors: dxgi:dxgi timeout
=== debian11b (build log) ===
WineRunWineTest.pl:error: The task timed out
I debugged an X11 crash from coming from XRenderCreatePicture(), from move_window_bits_surface() -> NtGdiSetDIBitsToDeviceInternal -> nulldrv_SetDIBitsToDevice -> xrenderdrv_PutImage. That was happening for layered systray window when it was loosing WS_EX_LAYERED attribute during hiding icon (that's why this exact path reached xrender). xrender path currently doesn't expect argb drawables, phys_dev xrender format is always set to default_format and XRenderCreatePicture on phys_dev visual causes X error when the visual is argb. For systray window argb visual comes from a special systray visual handling in winex11.drv/visual.c (get_systray_visual_info).
Honestly, I am not sure that everything is fully correct on all that path, like not sure why do we need a special visual for systray window, or if we really need to reach move_window_bits_surface() in this specific case. But we probably may end up scaling argb images with xrender for layered windows now anyway, so maybe allowing that to work in xrender worth it.
While looking at unrelated bug I figured that move_window_bits_surface() called when surface becomes NULL is actually needed. Just SetPixelFormat() or creating Vulkan surface on a window without yet presenting anything will create client window, and when onscreen client window is used in winex11 that results in black window contents. In this case the surface is deleted, and move_window_bits_surface() restores the contents of the window onscreen while otherwise it will stay black (we also don't invalidate the window in this case which is also correct).
Then, it looks like currently systray window is the only case when we may end up with deleting surface and argb visual. For layered windows only UpdateLayeredWidnow - managed layered windows end up with argb visuals. And for such windows the surface is never destroyed even if 3d client window is present. So if it is possible to get rid of argb visuals for systray that would solve the present issue. Although, even if that is possible, I'd probably still suggest to do what this MR does, as the logic which guarantees that we never end up blitting from a going-away surface to argb visual never happens with layered windows looks very convoluted and relying on that seems flaky; that can maybe also change if we'll be improving support for layered windows in some way.
Actually, the issue I am reproducing is not with shell tray window but with shell tray window icon (which is ULW-managed layered window), so the specifics of shell tray window is likely unrelated. It looks like different things go wrong with it.
One thing is during any window state change win32u.create_window_surface() is called with 'create_layered' flag, which is TRUE only when that is called from UpdateLayeredWindow path. So during any other surface update during window pos change it is called with FALSE. Then, driver's pCreateWindowSurface is called with that flag only without taking into account if the window surface already had alpha_mask. Also, the handling below create_window_surface() in get_window_surface() resets alpha_mask in the surface even if the previous surface was layered (only minds 'create_layered').
Yet there is more to it, with modeset emulation enabled the window seems to loose ULW-layered surface when dealing with scaled surface.
All these looks like potentially something to fix, but yet maybe it still makes sense to have xrender change for the above reasons.
I opened https://gitlab.winehq.org/wine/wine/-/merge_requests/7255 with a couple of minor changes.
On Tue Feb 4 12:06:50 2025 +0000, Rémi Bernon wrote:
I opened https://gitlab.winehq.org/wine/wine/-/merge_requests/7255 with a couple of minor changes.
One thing that I'm not sure about is how we handle alpha in the visual. I don't think systray window visual necessarily has alpha channel, but I believe we need to use the exact visual of the embedder window for our systray windows.
Maybe we should check more precisely whether systray visual has alpha or not, and select the xrender format accordingly?
Specifically, this line https://gitlab.winehq.org/wine/wine/-/merge_requests/7255/diffs?commit_id=38... is different from your version.
On Tue Feb 4 12:08:42 2025 +0000, Rémi Bernon wrote:
One thing that I'm not sure about is how we handle alpha in the visual. I don't think systray window visual necessarily has alpha channel, but I believe we need to use the exact visual of the embedder window for our systray windows. Maybe we should check more precisely whether systray visual has alpha or not, and select the xrender format accordingly? Specifically, this line https://gitlab.winehq.org/wine/wine/-/merge_requests/7255/diffs?commit_id=38... is different from your version.
I am not sure if the change will work, why do you think it is better (and probably it won't work at least for a generic case)? Not sure why is_wxrformat_compatible_with_visual has to be so unobvious and depend on given visual id matching default for some reason, can't we just match argb visual with argb and non-argb with non-argb?
In the case when it is failing systray visual has alpha. But my idea of going this way was not making any assumptions on systray in xrender part and just not break on argb visuals.
On a separate note, the argb visual goes away now (and triggers blit to X window) for multiple reasons, at least: - any resize (get_default_visual will get dummy visual in this case and alpha_mask will be lost); - driver's CreateWindowSurface is called with only create new layered (meaning ULW) surface but previous surface status is not relayed; - win32u and winex11.drv do not agree a bit whether window should have surface or not. winex11.drv assumes that ULW window should have a surface regardless of child window presence (which I think is correct; on Windows 3d drawing doesn't go anywhere onscreen with ULW window and it should have a surface regardless of pixel format or 3d rendering present). But win32u disagrees and thinks if a window has pixel format it never should have / use a surface (and sometimes it thinks the same for Vulkan surface presence but those cases do not exactly align).
I tried making a patch of straightening this up but it pulls more things around and probably explicit tracking of style change and ULW / SLWP layered windows explicit distinction, I didn't finish that. But if we agree that having blits to argb visual is beneficial anyway (while solves the present regression), I guess it should not make assuptions in that part about how do we get the argb visual, would it be default argb visual or the one for systray.
On Tue Feb 4 16:30:45 2025 +0000, Paul Gofman wrote:
I am not sure if the change will work, why do you think it is better (and probably it won't work at least for a generic case)? Not sure why is_wxrformat_compatible_with_visual has to be so unobvious and depend on given visual id matching default for some reason, can't we just match argb visual with argb and non-argb with non-argb? In the case when it is failing systray visual has alpha. But my idea of going this way was not making any assumptions on systray in xrender part and just not break on argb visuals. On a separate note, the argb visual goes away now (and triggers blit to X window) for multiple reasons, at least:
- any resize (get_default_visual will get dummy visual in this case and
alpha_mask will be lost);
- driver's CreateWindowSurface is called with only create new layered
(meaning ULW) surface but previous surface status is not relayed;
- win32u and winex11.drv do not agree a bit whether window should have
surface or not. winex11.drv assumes that ULW window should have a surface regardless of child window presence (which I think is correct; on Windows 3d drawing doesn't go anywhere onscreen with ULW window and it should have a surface regardless of pixel format or 3d rendering present). But win32u disagrees and thinks if a window has pixel format it never should have / use a surface (and sometimes it thinks the same for Vulkan surface presence but those cases do not exactly align). I tried making a patch of straightening this up but it pulls more things around and probably explicit tracking of style change and ULW / SLWP layered windows explicit distinction, I didn't finish that. But if we agree that having blits to argb visual is beneficial anyway (while solves the present regression), I guess it should not make assuptions in that part about how do we get the argb visual, would it be default argb visual or the one for systray.
I. e., I think ```
if (fmt->alphaMask) return FALSE; if (fmt->alphaMask && visual->visualid == default_visual.visualid) return FALSE; return TRUE; ``` is very unobvious as best. If there should be an exception for handling default_visual which I missed in my patch that should probably be done in load_xrender_formats()?
Not sure why is_wxrformat_compatible_with_visual has to be so unobvious and depend on given visual id matching default for some reason, can't we just match argb visual with argb and non-argb with non-argb?
We know that `argb_visual` has alpha because it's selected like that. However, the systray visual can be anything, and it depends on what the systray owner window has been created with. The specification says that it "must either be the default visual for the screen or it must be a TrueColor visual. If this property is set to a visual with an alpha channel, ..." https://specifications.freedesktop.org/systemtray-spec/0.3/manager-hints.html
On Tue Feb 4 16:39:58 2025 +0000, Rémi Bernon wrote:
Not sure why is_wxrformat_compatible_with_visual has to be so
unobvious and depend on given visual id matching default for some reason, can't we just match argb visual with argb and non-argb with non-argb? We know that `argb_visual` has alpha because it's selected like that. However, the systray visual can be anything, and it depends on what the systray owner window has been created with. The specification says that it "must either be the default visual for the screen or it must be a TrueColor visual. If this property is set to a visual with an alpha channel, ..." https://specifications.freedesktop.org/systemtray-spec/0.3/manager-hints.html
I think we shouldn't think about systray gory details here, as soon as we match given visual and not assuming default_argb? What do you see wrong with checking for matching alpha presence as in my patch?
On Tue Feb 4 16:42:30 2025 +0000, Paul Gofman wrote:
I think we shouldn't think about systray gory details here, as soon as we match given visual and not assuming default_argb? What do you see wrong with checking for matching alpha presence as in my patch?
It's as misleading as this is, data->use_alpha is also incorrect and it's basically exactly the same as here: it is set to TRUE if visual != default_visual, which doesn't actually say anything about alpha.
On Tue Feb 4 16:44:01 2025 +0000, Rémi Bernon wrote:
It's as misleading as this is, data->use_alpha is also incorrect and it's basically exactly the same as here: it is set to TRUE if visual != default_visual, which doesn't actually say anything about alpha.
Well, the difference is that escape.use_alpha is set in the place where such things are managed, while xrender doesn't need to have logic tangled with the entirety of winex11.drv. data->use_alpha is set whenever we consider alpha-enabled visual, for ULW windows and conditionally for systray. If we set use_alpha because we wanted alpha but our visual doesn't have it then the proper xrender format will be selected based on visual depth check? In your variant what is going to make sure if we are going to actually select argb visual when we want it? depth can be 32 both for, e. g., WXR_FORMAT_A8R8G8B8 / WXR_FORMAT_X8R8G8B8. Any will be compatible as in avoiding X error probably, but we do want one or another depending on whether we do use alpha or not (and that is given by data->use_alpha)?
On Tue Feb 4 16:54:48 2025 +0000, Paul Gofman wrote:
Well, the difference is that escape.use_alpha is set in the place where such things are managed, while xrender doesn't need to have logic tangled with the entirety of winex11.drv. data->use_alpha is set whenever we consider alpha-enabled visual, for ULW windows and conditionally for systray. If we set use_alpha because we wanted alpha but our visual doesn't have it then the proper xrender format will be selected based on visual depth check? In your variant what is going to make sure if we are going to actually select argb visual when we want it? depth can be 32 both for, e. g., WXR_FORMAT_A8R8G8B8 / WXR_FORMAT_X8R8G8B8. Any will be compatible as in avoiding X error probably, but we do want one or another depending on whether we do use alpha or not (and that is given by data->use_alpha)?
I checked with my trigger case with Crysis 3 and the alternative variant also avoids the problem. It probably correctly selects alpha enabled formats because those formats are listed above matching non-alpha enabled ones. If you think that it is all clearer across the board maybe worth adding the comment in the alternative variant near that visual check? Like:
/* Non-default visual is only used when alpha is required. */
On Tue Feb 4 17:26:03 2025 +0000, Paul Gofman wrote:
I checked with my trigger case with Crysis 3 and the alternative variant also avoids the problem. It probably correctly selects alpha enabled formats because those formats are listed above matching non-alpha enabled ones. If you think that it is all clearer across the board maybe worth adding the comment in the alternative variant near that visual check? Like: /* Non-default visual is only used when alpha is required. */
I was progressing toward getting rid of that `use_alpha` flag and I think it could even be dropped already, it's not very useful anymore as most of the related logic has been moved to win32u.
Every layered window (and that includes systray windows) has an alpha channel in the window surface. What may be different then is how this alpha channel is being used when the pixels are sent to X(Render), they may or may not be used depending on the available visuals, and we should prefer using alpha when possible, but otherwise it's just getting ignored.
I've made a small tweak to the logic, to hopefully make it less misleading, checking `if (fmt->alphaMask && visual->class != TrueColor) return FALSE;` instead.
On Tue Feb 4 17:50:58 2025 +0000, Rémi Bernon wrote:
I was progressing toward getting rid of that `use_alpha` flag and I think it could even be dropped already, it's not very useful anymore as most of the related logic has been moved to win32u. Every layered window (and that includes systray windows) has an alpha channel in the window surface. What may be different then is how this alpha channel is being used when the pixels are sent to X(Render), they may or may not be used depending on the available visuals, and we should prefer using alpha when possible, but otherwise it's just getting ignored. I've made a small tweak to the logic, to hopefully make it less misleading, checking `if (fmt->alphaMask && visual->class != TrueColor) return FALSE;` instead (and an additional check to avoid selecting alpha for the default visual regardless if it is TrueColor).
Eh, but TrueColor is the common visual class both for argb and default visual?
On Tue Feb 4 17:52:45 2025 +0000, Paul Gofman wrote:
Eh, but TrueColor is the common visual class both for argb and default visual?
Sorry, I edited my comment to add a precision about that https://gitlab.winehq.org/wine/wine/-/merge_requests/7255/diffs?diff_id=1555...
The default visual may or may not be TrueColor, the argb visual (and any visual with alpha) is.
On Tue Feb 4 17:54:15 2025 +0000, Rémi Bernon wrote:
Sorry, I edited my comment to add a precision about that https://gitlab.winehq.org/wine/wine/-/merge_requests/7255/diffs?diff_id=1555... The default visual may or may not be TrueColor, the argb visual (and any visual with alpha) is.
Er... actually it's probably not right indeed.
On Tue Feb 4 17:57:06 2025 +0000, Rémi Bernon wrote:
Er... actually it's probably not right indeed.
Yeah, I am wondering what now will make sure xrender_ExtEscape will select non-alpha format for default visual?
On Tue Feb 4 17:58:07 2025 +0000, Paul Gofman wrote:
Yeah, I am wondering what now will make sure xrender_ExtEscape will select non-alpha format for default visual?
I reverted to checking visualid with a comment, should be good enough. Sorry for the mess.
-
Marvin -
Paul Gofman -
Paul Gofman (＠gofman)
-
Rémi Bernon (＠rbernon)