[PATCH 0/2] MR6401: uxtheme: Optimize drawing by avoiding unneeded TransparentBlt().
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/uxtheme/draw.c | 7 +++---- dlls/uxtheme/msstyles.h | 2 ++ 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/dlls/uxtheme/draw.c b/dlls/uxtheme/draw.c index 3286d70d4d1..eb400571197 100644 --- a/dlls/uxtheme/draw.c +++ b/dlls/uxtheme/draw.c @@ -502,7 +502,7 @@ static inline void get_transparency (HTHEME hTheme, int iPartId, int iStateId, if (hasImageAlpha) { *transparent = ALPHABLEND_FULL; - *transparentcolor = RGB (255, 0, 255); + *transparentcolor = DEFAULT_TRANSPARENT_COLOR; } else { @@ -514,8 +514,7 @@ static inline void get_transparency (HTHEME hTheme, int iPartId, int iStateId, if(FAILED(GetThemeColor(hTheme, iPartId, iStateId, glyph ? TMT_GLYPHTRANSPARENTCOLOR : TMT_TRANSPARENTCOLOR, transparentcolor))) { - /* If image is transparent, but no color was specified, use magenta */ - *transparentcolor = RGB(255, 0, 255); + *transparentcolor = DEFAULT_TRANSPARENT_COLOR; } } else @@ -1968,7 +1967,7 @@ static HRESULT create_image_bg_region(HTHEME theme, int part, int state, const R OffsetRect(&r, -r.left, -r.top); if (FAILED(GetThemeColor(theme, part, state, TMT_TRANSPARENTCOLOR, &transcolour))) - transcolour = RGB(255, 0, 255); /* defaults to magenta */ + transcolour = DEFAULT_TRANSPARENT_COLOR; dc = CreateCompatibleDC(NULL); if (!dc) { diff --git a/dlls/uxtheme/msstyles.h b/dlls/uxtheme/msstyles.h index b2b0e0d8f22..7de41340b86 100644 --- a/dlls/uxtheme/msstyles.h +++ b/dlls/uxtheme/msstyles.h @@ -27,6 +27,8 @@ #define MAX_THEME_CLASS_NAME 60 #define MAX_THEME_VALUE_NAME 60 +#define DEFAULT_TRANSPARENT_COLOR RGB(255, 0, 255) + typedef struct _THEME_PROPERTY { int iPrimitiveType; int iPropertyId; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6401
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/uxtheme/draw.c | 30 ++++++++++++++-------------- dlls/uxtheme/msstyles.c | 43 +++++++++++++++++++++++++++++++++++------ dlls/uxtheme/msstyles.h | 3 ++- 3 files changed, 55 insertions(+), 21 deletions(-) diff --git a/dlls/uxtheme/draw.c b/dlls/uxtheme/draw.c index eb400571197..83c1ce19655 100644 --- a/dlls/uxtheme/draw.c +++ b/dlls/uxtheme/draw.c @@ -250,7 +250,7 @@ static PTHEME_PROPERTY UXTHEME_SelectImage(HTHEME hTheme, int iPartId, int iStat BOOL hasAlpha; lstrcpynW(szPath, fileProp->lpValue, min(fileProp->dwValueLen+1, ARRAY_SIZE(szPath))); - hBmp = MSSTYLES_LoadBitmap(hTheme, szPath, &hasAlpha); + hBmp = MSSTYLES_LoadBitmap(hTheme, szPath, &hasAlpha, NULL); if(!hBmp) continue; GetThemeEnumValue(hTheme, iPartId, iStateId, TMT_IMAGELAYOUT, &imagelayout); @@ -289,7 +289,7 @@ static PTHEME_PROPERTY UXTHEME_SelectImage(HTHEME hTheme, int iPartId, int iStat */ static HRESULT UXTHEME_LoadImage(HTHEME hTheme, int iPartId, int iStateId, const RECT *pRect, BOOL glyph, HBITMAP *hBmp, RECT *bmpRect, BOOL *hasImageAlpha, - int *imageDpi) + BOOL *has_default_transparent_colour, int *imageDpi) { int imagelayout = IL_HORIZONTAL; int imagecount = 1; @@ -304,7 +304,7 @@ static HRESULT UXTHEME_LoadImage(HTHEME hTheme, int iPartId, int iStateId, const return E_PROP_ID_UNSUPPORTED; } lstrcpynW(szPath, tp->lpValue, min(tp->dwValueLen+1, ARRAY_SIZE(szPath))); - *hBmp = MSSTYLES_LoadBitmap(hTheme, szPath, hasImageAlpha); + *hBmp = MSSTYLES_LoadBitmap(hTheme, szPath, hasImageAlpha, has_default_transparent_colour); if(!*hBmp) { TRACE("Failed to load bitmap %s\n", debugstr_w(szPath)); return HRESULT_FROM_WIN32(GetLastError()); @@ -496,7 +496,7 @@ static inline BOOL UXTHEME_SizedBlt (HDC hdcDst, int nXOriginDst, int nYOriginDs * depend on whether the image has full alpha or whether it is * color-transparent or just opaque. */ static inline void get_transparency (HTHEME hTheme, int iPartId, int iStateId, - BOOL hasImageAlpha, INT* transparent, + BOOL hasImageAlpha, BOOL has_default_transparent_colour, INT* transparent, COLORREF* transparentcolor, BOOL glyph) { if (hasImageAlpha) @@ -515,6 +515,8 @@ static inline void get_transparency (HTHEME hTheme, int iPartId, int iStateId, glyph ? TMT_GLYPHTRANSPARENTCOLOR : TMT_TRANSPARENTCOLOR, transparentcolor))) { *transparentcolor = DEFAULT_TRANSPARENT_COLOR; + if (!has_default_transparent_colour) + *transparent = ALPHABLEND_NONE; } } else @@ -541,7 +543,7 @@ static void reset_dc_alpha_values(HTHEME htheme, HDC hdc, int part_id, int state return; if (FAILED(UXTHEME_LoadImage(htheme, part_id, state_id, rect, FALSE, &hbmp, &image_rect, - &has_alpha, NULL)) || has_alpha) + &has_alpha, NULL, NULL)) || has_alpha) return; bitmap_info.bmiHeader.biSize = sizeof(BITMAPINFOHEADER); @@ -576,10 +578,10 @@ static HRESULT UXTHEME_DrawImageGlyph(HTHEME hTheme, HDC hdc, int iPartId, POINT dstSize; POINT srcSize; POINT topleft; - BOOL hasAlpha; + BOOL hasAlpha, has_default_trans; hr = UXTHEME_LoadImage(hTheme, iPartId, iStateId, pRect, TRUE, &bmpSrc, &rcSrc, &hasAlpha, - NULL); + &has_default_trans, NULL); if(FAILED(hr)) return hr; hdcSrc = CreateCompatibleDC(hdc); if(!hdcSrc) { @@ -593,7 +595,7 @@ static HRESULT UXTHEME_DrawImageGlyph(HTHEME hTheme, HDC hdc, int iPartId, srcSize.x = rcSrc.right-rcSrc.left; srcSize.y = rcSrc.bottom-rcSrc.top; - get_transparency (hTheme, iPartId, iStateId, hasAlpha, &transparent, + get_transparency (hTheme, iPartId, iStateId, hasAlpha, has_default_trans, &transparent, &transparentcolor, TRUE); GetThemeEnumValue(hTheme, iPartId, iStateId, TMT_VALIGN, &valign); GetThemeEnumValue(hTheme, iPartId, iStateId, TMT_HALIGN, &halign); @@ -659,7 +661,7 @@ static HRESULT get_image_part_size(HTHEME hTheme, int iPartId, int iStateId, REC BOOL hasAlpha; hr = UXTHEME_LoadImage(hTheme, iPartId, iStateId, prc, FALSE, &bmpSrc, &rcSrc, &hasAlpha, - &imageDpi); + NULL, &imageDpi); if (FAILED(hr)) return hr; switch (eSize) @@ -760,10 +762,10 @@ static HRESULT UXTHEME_DrawImageBackground(HTHEME hTheme, HDC hdc, int iPartId, int sizingtype = ST_STRETCH; INT transparent; COLORREF transparentcolor = 0; - BOOL hasAlpha; + BOOL hasAlpha, has_default_trans; hr = UXTHEME_LoadImage(hTheme, iPartId, iStateId, pRect, FALSE, &bmpSrc, &rcSrc, &hasAlpha, - NULL); + &has_default_trans, NULL); if(FAILED(hr)) return hr; hdcSrc = CreateCompatibleDC(hdc); if(!hdcSrc) { @@ -774,7 +776,7 @@ static HRESULT UXTHEME_DrawImageBackground(HTHEME hTheme, HDC hdc, int iPartId, rcDst = *pRect; - get_transparency (hTheme, iPartId, iStateId, hasAlpha, &transparent, + get_transparency (hTheme, iPartId, iStateId, hasAlpha, has_default_trans, &transparent, &transparentcolor, FALSE); dstSize.x = rcDst.right-rcDst.left; @@ -2224,10 +2226,10 @@ BOOL WINAPI IsThemeBackgroundPartiallyTransparent(HTHEME hTheme, int iPartId, if (bgtype != BT_IMAGEFILE) return FALSE; if (FAILED(UXTHEME_LoadImage(hTheme, iPartId, iStateId, &rect, FALSE, &bmpSrc, &rcSrc, - &hasAlpha, NULL))) + &hasAlpha, NULL, NULL))) return FALSE; - get_transparency (hTheme, iPartId, iStateId, hasAlpha, &transparent, + get_transparency (hTheme, iPartId, iStateId, hasAlpha, TRUE, &transparent, &transparentcolor, FALSE); return (transparent != ALPHABLEND_NONE); } diff --git a/dlls/uxtheme/msstyles.c b/dlls/uxtheme/msstyles.c index 71603953ed2..8075f65d95f 100644 --- a/dlls/uxtheme/msstyles.c +++ b/dlls/uxtheme/msstyles.c @@ -1181,19 +1181,43 @@ PTHEME_PROPERTY MSSTYLES_FindProperty(PTHEME_CLASS tc, int iPartId, int iStateId } /* Prepare a bitmap to be used for alpha blending */ -static BOOL prepare_alpha (HBITMAP bmp, BOOL* hasAlpha) +static BOOL prepare_alpha (HBITMAP bmp, BOOL* hasAlpha, BOOL *has_default_transparent_colour) { DIBSECTION dib; - int n; + int n, stride; BYTE* p; *hasAlpha = FALSE; + *has_default_transparent_colour = FALSE; if (!bmp || GetObjectW( bmp, sizeof(dib), &dib ) != sizeof(dib)) return FALSE; - if (dib.dsBm.bmBitsPixel != 32 || dib.dsBmih.biCompression != BI_RGB) - /* nothing to do */ + if (dib.dsBmih.biCompression != BI_RGB) + return TRUE; + + if (dib.dsBm.bmBitsPixel == 24) + { + int y; + + stride = (dib.dsBmih.biWidth * 3 + 3) & ~3; + p = dib.dsBm.bmBits; + for (y = 0; y < dib.dsBmih.biHeight; ++y) + { + p = (BYTE *)dib.dsBm.bmBits + stride * y; + for (n = 0; n < dib.dsBmih.biWidth; ++n, p += 3) + { + if (RGB(p[0], p[1], p[2]) == DEFAULT_TRANSPARENT_COLOR) + { + *has_default_transparent_colour = TRUE; + break; + } + } + } + return TRUE; + } + + if (dib.dsBm.bmBitsPixel != 32) return TRUE; /* If all alpha values are 0xff, don't use alpha blending */ @@ -1219,11 +1243,13 @@ static BOOL prepare_alpha (HBITMAP bmp, BOOL* hasAlpha) return TRUE; } -HBITMAP MSSTYLES_LoadBitmap (PTHEME_CLASS tc, LPCWSTR lpFilename, BOOL* hasAlpha) +HBITMAP MSSTYLES_LoadBitmap (PTHEME_CLASS tc, LPCWSTR lpFilename, BOOL* hasAlpha, BOOL *has_default_transparent_colour) { WCHAR szFile[MAX_PATH]; LPWSTR tmp; PTHEME_IMAGE img; + BOOL has_default_trans; + lstrcpynW(szFile, lpFilename, ARRAY_SIZE(szFile)); tmp = szFile; do { @@ -1240,6 +1266,8 @@ HBITMAP MSSTYLES_LoadBitmap (PTHEME_CLASS tc, LPCWSTR lpFilename, BOOL* hasAlpha { TRACE ("found %p %s: %p\n", img, debugstr_w (img->name), img->image); *hasAlpha = img->hasAlpha; + if (has_default_transparent_colour) + *has_default_transparent_colour = img->has_default_transparent_colour; return img->image; } img = img->next; @@ -1247,8 +1275,11 @@ HBITMAP MSSTYLES_LoadBitmap (PTHEME_CLASS tc, LPCWSTR lpFilename, BOOL* hasAlpha /* Not found? Load from resources */ img = malloc(sizeof(*img)); img->image = LoadImageW(tc->hTheme, szFile, IMAGE_BITMAP, 0, 0, LR_CREATEDIBSECTION); - prepare_alpha (img->image, hasAlpha); + if (!has_default_transparent_colour) + has_default_transparent_colour = &has_default_trans; + prepare_alpha (img->image, hasAlpha, has_default_transparent_colour); img->hasAlpha = *hasAlpha; + img->has_default_transparent_colour = *has_default_transparent_colour; /* ...and stow away for later reuse. */ lstrcpyW (img->name, szFile); img->next = tc->tf->images; diff --git a/dlls/uxtheme/msstyles.h b/dlls/uxtheme/msstyles.h index 7de41340b86..264428eb30b 100644 --- a/dlls/uxtheme/msstyles.h +++ b/dlls/uxtheme/msstyles.h @@ -68,6 +68,7 @@ typedef struct _THEME_IMAGE { WCHAR name[MAX_PATH]; HBITMAP image; BOOL hasAlpha; + BOOL has_default_transparent_colour; struct _THEME_IMAGE *next; } THEME_IMAGE, *PTHEME_IMAGE; @@ -103,7 +104,7 @@ PTHEME_PARTSTATE MSSTYLES_FindPart(PTHEME_CLASS tc, int iPartId); PTHEME_PARTSTATE MSSTYLES_FindPartState(PTHEME_CLASS tc, int iPartId, int iStateId, PTHEME_CLASS *tcNext); PTHEME_PROPERTY MSSTYLES_FindProperty(PTHEME_CLASS tc, int iPartId, int iStateId, int iPropertyPrimitive, int iPropertyId); PTHEME_PROPERTY MSSTYLES_FindMetric(int iPropertyPrimitive, int iPropertyId); -HBITMAP MSSTYLES_LoadBitmap(PTHEME_CLASS tc, LPCWSTR lpFilename, BOOL* hasAlpha); +HBITMAP MSSTYLES_LoadBitmap(PTHEME_CLASS tc, LPCWSTR lpFilename, BOOL* hasAlpha, BOOL *has_default_transparent_colour); HRESULT MSSTYLES_GetPropertyBool(PTHEME_PROPERTY tp, BOOL *pfVal); HRESULT MSSTYLES_GetPropertyColor(PTHEME_PROPERTY tp, COLORREF *pColor); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6401
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=148080 Your paranoid android. === debian11b (64 bit WoW report) === mfmediaengine: mfmediaengine.c:2564: Test failed: Unexpected time 0.000000.
With default light theme UXTHEME_ScrollBarDraw() takes ~2.3ms here with the most slow part being TransparentBlt used multiple time. In fact, none of the theme's bitmap actually have the color key and are not transparent anyway. Utilizing this fact and avoiding TransparentBlt in such cases reduces the time of UXTHEME_ScrollBarDraw to 0.8ms, which is probably also not all that great but is a severe improvement with a simple change. Spotted while looking at Pharaoh Rebirth+ taking a long time to load into the game, while all the time spent in processing LB_ADDSTRING in listbox control for updating scroll bar. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6401#note_80490
In fact, none of the theme's bitmap actually have the color key and are not transparent anyway.
ALPHABLEND_BINARY is only used when the property TMT_GLYPHTRANSPARENT or TMT_TRANSPARENT is TRUE. Which theme part and state is the application trying to draw? If the theme part is not transparent then we should probably not set TMT_TRANSPARENT or TMT_GLYPHTRANSPARENT to TRUE.
while all the time spent in processing LB_ADDSTRING in listbox control for updating scroll bar.
Is the listbox from Wine's comctl32? Can this be fixed in comctl32 instead? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6401#note_80705
ALPHABLEND_BINARY is only used when the property TMT_GLYPHTRANSPARENT or TMT_TRANSPARENT is TRUE. Which theme part and state is the application trying to draw? If the theme part is not transparent then we should probably not set TMT_TRANSPARENT or TMT_GLYPHTRANSPARENT to TRUE.
This is from the default (light) theme, ScrollBar.ArrowBtn (blue_scrollbar_arrows.bmp). Removing Transparent attribute was my first thought (given the images which are actually transparent have alpha in the bmp and image transparency is used regardless of the theme attrubute) but there is a test which specifically checks that which is broken if doing so. Then, I could probably check the effect of just making all the theme images with alpha hoping that would improve this case too as blitting alpha-enabled bitmaps is probably not that catastrophic, but judged that maybe just handling it in the code avoiding just for any case (including non-Wine builtin theme) is a bit more interesting.
Is the listbox from Wine's comctl32? Can this be fixed in comctl32 instead?
Yes, it is comctl32 Wine listbox. There are already some criteria when the scroll box is not updated upon adding an item. The game's scroll bar configuration yields the redraw on each run. I didn't explore for sure if there is maybe something missing and in this specific case it shouldn't be updated too. But I concluded that the redraw time with default theme of ~2.5ms is so bad that it probably should be considered the core problem here; there are definitely cases when listbox's scrollbar should be updated and it looks more interesting reducing this time at this point. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6401#note_80736
Zhiyi Zhang (@zhiyi) commented about dlls/uxtheme/msstyles.c:
+ + if (dib.dsBm.bmBitsPixel == 24) + { + int y; + + stride = (dib.dsBmih.biWidth * 3 + 3) & ~3; + p = dib.dsBm.bmBits; + for (y = 0; y < dib.dsBmih.biHeight; ++y) + { + p = (BYTE *)dib.dsBm.bmBits + stride * y; + for (n = 0; n < dib.dsBmih.biWidth; ++n, p += 3) + { + if (RGB(p[0], p[1], p[2]) == DEFAULT_TRANSPARENT_COLOR) + { + *has_default_transparent_colour = TRUE; + break; This should be a return.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6401#note_80840
On Mon Sep 2 16:18:14 2024 +0000, Paul Gofman wrote:
ALPHABLEND_BINARY is only used when the property TMT_GLYPHTRANSPARENT or TMT_TRANSPARENT is TRUE. Which theme part and state is the application trying to draw? If the theme part is not transparent then we should probably not set TMT_TRANSPARENT or TMT_GLYPHTRANSPARENT to TRUE. This is from the default (light) theme, ScrollBar.ArrowBtn (blue_scrollbar_arrows.bmp). Removing Transparent attribute was my first thought (given the images which are actually transparent have alpha in the bmp and image transparency is used regardless of the theme attrubute) but there is a test which specifically checks that which is broken if doing so. Then, I could probably check the effect of just making all the theme images with alpha hoping that would improve this case too as blitting alpha-enabled bitmaps is probably not that catastrophic, but judged that maybe just handling it in the code avoiding just for any case (including non-Wine builtin theme) is a bit more interesting. Is the listbox from Wine's comctl32? Can this be fixed in comctl32 instead? Yes, it is comctl32 Wine listbox. There are already some criteria when the scroll box is not updated upon adding an item. The game's scroll bar configuration yields the redraw on each run. I didn't explore for sure if there is maybe something missing and in this specific case it shouldn't be updated too. But I concluded that the redraw time with default theme of ~2.5ms is so bad that it probably should be considered the core problem here; there are definitely cases when listbox's scrollbar should be updated and it looks more interesting reducing this time at this point. Does this MR completely remove the slowdown for the application? If not, I suggest fixing this for listbox as well. Try to avoid redraws for listbox whenever it's possible.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6401#note_80841
Zhiyi Zhang (@zhiyi) commented about dlls/uxtheme/draw.c:
*/ static HRESULT UXTHEME_LoadImage(HTHEME hTheme, int iPartId, int iStateId, const RECT *pRect, BOOL glyph, HBITMAP *hBmp, RECT *bmpRect, BOOL *hasImageAlpha, - int *imageDpi) + BOOL *has_default_transparent_colour, int *imageDpi) Let's keep using camel cases for consistency. Same for other places.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6401#note_80842
Zhiyi Zhang (@zhiyi) commented about dlls/uxtheme/draw.c:
POINT dstSize; POINT srcSize; POINT topleft; - BOOL hasAlpha; + BOOL hasAlpha, has_default_trans;
Let's use hasDefaultTransparentColour. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6401#note_80843
Zhiyi Zhang (@zhiyi) commented about dlls/uxtheme/draw.c:
glyph ? TMT_GLYPHTRANSPARENTCOLOR : TMT_TRANSPARENTCOLOR, transparentcolor))) { *transparentcolor = DEFAULT_TRANSPARENT_COLOR; + if (!has_default_transparent_colour) + *transparent = ALPHABLEND_NONE;
Let's extend this to the case when GetThemeColor() succeeds but the transparent color also happens to be DEFAULT_TRANSPARENT_COLOR. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6401#note_80844
Zhiyi Zhang (@zhiyi) commented about dlls/uxtheme/msstyles.h:
WCHAR name[MAX_PATH]; HBITMAP image; BOOL hasAlpha; + BOOL has_default_transparent_colour;
Let's use bitfields for hasAlpha and hasDefaultTransparentColour. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6401#note_80846
Zhiyi Zhang (@zhiyi) commented about dlls/uxtheme/msstyles.c:
/* Not found? Load from resources */ img = malloc(sizeof(*img)); img->image = LoadImageW(tc->hTheme, szFile, IMAGE_BITMAP, 0, 0, LR_CREATEDIBSECTION); - prepare_alpha (img->image, hasAlpha); + if (!has_default_transparent_colour) + has_default_transparent_colour = &has_default_trans; + prepare_alpha (img->image, hasAlpha, has_default_transparent_colour); img->hasAlpha = *hasAlpha; + img->has_default_transparent_colour = *has_default_transparent_colour;
Let's write it like the following. It's clearer this way. ``` prepare_alpha(img->image, &img->hasAlpha, &img->hasDefaultTransparentColour); if (hasAlpha) *hasAlpha = img->hasAlpha; if (has_default_transparent_colour) *has_default_transparent_colour = img->has_default_transparent_colour; ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6401#note_80845
participants (4)
-
Marvin -
Paul Gofman -
Paul Gofman (＠gofman)
-
Zhiyi Zhang (＠zhiyi)