GNOME Bugzilla – Bug 741849
Add GdkScreen->is_composited() for GDK-Win32
Last modified: 2015-04-17 16:37:32 UTC
Hi, As we check whether the GdkWindow/GdkScreen is composited, for both the CSD and OpenGL support, I thought it might be a good idea that we implement support for ->is_composited() (instead of just returning FALSE) and ->set_composited() for the Windows backend, especially as GL support in the GDK-Win32 backend has landed. I do understand that there is work for this in bug 727316, but I thought that it might be better to open a new bug for it as: -This affects the OpenGL support in the GDK backend, in addition to CSD support. -I think we can do without the HAVE_W32_DWM check, but defining the types and macros until the point we bid farewell to XP support. I will post my patches on this in a bit.
Created attachment 293168 [details] [review] gdkmain-win32.c: Add a private utility function to check OS version info Hi, This patch adds a simple utility function to check whether we are running on at least a specified version of Windows, as GetVersion()/GetVersionEx() are being superceded since Windows 8.1. This takes the input major/minor version numbers in the standard Windows way, i.e.[1]: major minor Meaning ===== ===== ===== ... ... ... 6 1 Windows 7/Server 2008 R2 6 2 Windows 8/Server 2012 6 3 Windows 8.1/Server 2012 R2 With blessings, thank you! [1]: http://msdn.microsoft.com/zh-tw/library/windows/desktop/ms724833%28v=vs.85%29.aspx
Created attachment 293172 [details] [review] gdkcursor-win32.c: Simplify OS version check Hi, This simplifies the OS version check using the utility function in attachment 293168 [details] [review], and uses g_once_init_* APIs to make sure that the XP version check is only done once. With blessings, thank you!
Created attachment 293173 [details] [review] gdkmain-win32.c: Add a private utility function to check OS version info Hi, Apparently I forgot about the update in gdkprivate-win32.h, as the prototype is needed, so re-posting the patch. This, needs to come before attachment 293172 [details] [review]. With blessings, thank you!
Created attachment 293174 [details] [review] GDK-Win32: implement ->is_composited() and ->set_composited() on Windows Hi, As the title of the patch suggests, this implements ->is_composited() and set_composited() with the Windows GDK backend. Please see the patch comments for notes about this, as Windows 8+/2012+ does things a bit differently, and this is supported only for Vista and later. With blessings, thank you!
Review of attachment 293173 [details] [review]: ::: gdk/win32/gdkmain-win32.c @@ +152,3 @@ +gboolean +_gdk_win32_check_os_version (glong major, glong minor) just using guint for major and minor would make this fit better with similar apis, like glib_check_version or gtk_check_version
Review of attachment 293173 [details] [review]: ::: gdk/win32/gdkmain-win32.c @@ +152,3 @@ +gboolean +_gdk_win32_check_os_version (glong major, glong minor) there is no need to prefix the symbol with an underscore any more: all symbols are private unless explicitly marked for exporting.
Review of attachment 293174 [details] [review]: ::: gdk/win32/gdkmain-win32.c @@ +169,3 @@ + * we are on Windows 8/Server 2012 or later at that time + */ +GdkWin32DwmCapabilities we don't usually return structures by value. you can use a static GdkWin32DwmCapabilities and then return a pointer to it. @@ +170,3 @@ + */ +GdkWin32DwmCapabilities +_gdk_win32_get_dwm_capabilities (void) no need to prefix with '_'. ::: gdk/win32/gdkscreen-win32.c @@ +217,3 @@ + if (capabilities.dwmIsCompositionEnabled (&is_composited) != S_OK) + return FALSE; + else this is the last condition, so you can simply drop the `else` and let the flow fall through.
Review of attachment 293172 [details] [review]: looks generally okay to me.
Created attachment 293225 [details] [review] gdkcursor-win32.c: Simplify OS Version Check (take ii) Hi, I have added bug 741895 as a dependency of this bug, as: -We probably need a better mechanism for checking the Windows OS version, especially as GetVersion(), which is used in g_win32_windows_get_version() changed in the way it works in Windows 8.1+/Server 2012 R2+ (please see comment #2), and uses VerifyVersionInfo(), which is a more robust way. -Check OS-level functionalities (for those that don't need the GetProcAddress(), or those that the behavior changed, such as DWM, which we are talking about here changed) safer and easier. -This functionality might be useful to others, too. I have updated the gdkcursor-win32.c patch to make use of that API (but this means that API needs to land in GLib before this). With blessings, thank you!
Created attachment 293228 [details] [review] GDK-Win32: implement ->is_composited() and ->set_composited() on Windows (take ii) Hi, This is the updated version of the implementation of ->is_composited() and ->set_composited() on GDK-Win32, which: -Makes use of the new GLib API in bug 741895, g_win32_windows_is_at_least(), which takes Matthias' notes into account. -Have Emmanuele's suggestions taken into account as well. -Be more careful when doing LoadLibary()/GetProcAddress(). With blessings, thank you!
Hi, Are the newer patches better for consumption, along with the proposed simple API in bug 741895. With blessings, thank you!
Review of attachment 293225 [details] [review]: looks reasonable - I just wonder if maybe the caching should happen on the other side, so you can avoid spreading once_init_enter/leave across the various users
Created attachment 295076 [details] [review] Implement ->is_composited() and ->set_composited() for Windows Hello Matthias, Here's the new patch that should cover the functionality of the two previous patches here, and making the function and OS version check items in one single place, and also simplifying an existing OS version check for gdkwindow-win32.c I understand having to call that private function to set the OS versions and query for the functions in dwmapi.dll may not look that nice (although it makes sure that it only tries to do it once by g_once_*()), but I am not sure whether we could just do it once and for all in the GDK initing stage, in time for the items that need them. This patch will still depend on bug 741895 though, on the simple new GLib-Win32 API. With blessings, thank you!
Hi, Just wanted to know whether the patches here would be good to go in, or if there are any further improvements at this point that can and should be made about them, as the new GLib API that this uses is now in GLib master? With blessings, thank you!
Review of attachment 295076 [details] [review]: ::: gdk/win32/gdkprivate-win32.h @@ +510,3 @@ +/* for XP run-time compatibility, remove once XP support dropped */ +#ifndef DWM_BLURBEHIND Um...i think you want to check for DWMAPI or _DWMAPI_H_. DWM_BLURBEHIND, AFAICS, is not a macro (at least in mingw-w64 headers). No idea what its status is in mingw.org and MSVC. ::: gdk/win32/gdkwindow-win32.c @@ +3386,3 @@ + + _windowing_capabilities.dwmEnableComposition (action); +} According to https://developer.gnome.org/gdk3/stable/gdk3-Windows.html#gdk-window-set-composited , this function does not do what you think it does. I think it's better to leave it unimplemented, since gdk_display_supports_composite () is also unimplemented. This is completely unrelated to gdk_screen_is_composited().
Review of attachment 295076 [details] [review]: ::: gdk/win32/gdkmain-win32.c @@ +183,3 @@ + (PFN_DwmEnableBlurBehindWindow) GetProcAddress (dwmdll, "DwmEnableBlurBehindWindow"); + + FreeLibrary (dwmdll); You've just mapped the library into program's address space (earlier), then you've got (just now) addresses of functions in that library, pointing at the part of the program's address space where the library was mapped, and now you're freeing the library, *unmapping it from the program's address space*. Guess what is going to happen with these function pointers? I've failed to find documentation that would support my analysis, but that's what i would expect to happen. Folks at [1] seem to agree that you have to keep the library round for as long as you're using the functions from it. [1] http://stackoverflow.com/questions/25149927/loadlibrary-and-getprocaddress-in-a-function-c-c
Created attachment 301607 [details] [review] gdk-win32: Clean Up As We Are Always on On XP or Later Hi, I decided to split the patch into 2. The first one is to remove Windows XP checks, as we are bound to be on XP and later, since the dependent GLib already requires Windows XP for quite a while, as the patch comments indicate. (and the other patch will be the start of dropping XP support in GTK+-3.x), so this means items that are supported only on XP or later are always activated.
Created attachment 301609 [details] [review] gdk-win32: Really implement GdkScreen->is_composited(), and say bye to Windows XP Hello LRN, This is the new version of my patch. (In reply to LRN from comment #15) > > Um...i think you want to check for DWMAPI or _DWMAPI_H_. > DWM_BLURBEHIND, AFAICS, is not a macro (at least in mingw-w64 headers). No > idea what its status is in mingw.org and MSVC. I think we should start to say goodbye to Windows XP (as we are now in the 3.17.x cycle), as the general consensus I have would be to drop XP support in this cycle, so I think we can just include dwmapi.h and link to its library directly. This also means I am not going to check their presence via GetProcAddress() but assume that they are there, so probably you don't want to check for them in your RGBA window support for gdk-win32 anymore. > According to > https://developer.gnome.org/gdk3/stable/gdk3-Windows.html#gdk-window-set- > composited , this function does not do what you think it does. I think it's > better to leave it unimplemented, since gdk_display_supports_composite () is > also unimplemented... Thanks for the notes, and I think so too, as gdk_window_set_composited() is deprecated (so is gdk_display_supports_composite ()), so I think we should just forget about this part. p.s. I think Emmanuele is also getting GSK into GTK+ in this cycle, which depends on his graphene library. That library, as I asked him about it, does not run on XP at all, due to its use of NT 6.x (Vista/7) APIs. With blessings, thank you!
Review of attachment 301607 [details] [review]: Looks ok.
(In reply to Fan, Chun-wei from comment #18) > (In reply to LRN from comment #15) >> >> Um...i think you want to check for DWMAPI or _DWMAPI_H_. >> DWM_BLURBEHIND, AFAICS, is not a macro (at least in mingw-w64 headers). No >> idea what its status is in mingw.org and MSVC. > > I think we should start to say goodbye to Windows XP (as we are now in the > 3.17.x cycle), as the general consensus I have would be to drop XP support > in this cycle, so I think we can just include dwmapi.h and link to its > library directly. This also means I am not going to check their presence > via GetProcAddress() but assume that they are there, so probably you don't > want to check for them in your RGBA window support for gdk-win32 anymore. Okay, if glib maintainers bless the drop of XP support, then i support (no pun intended) that.
Review of attachment 301609 [details] [review]: Looks OK.
Review of attachment 301607 [details] [review]: ok
Review of attachment 301609 [details] [review]: ok
Hi, Thanks for the reviews and comments along the way. The patches have been committed as: -gdk-win32: Really Implement GdkScreen->is_composited(): b85f0cc (slightly updated, to update MSVC 2010+ projects properly) -gdk-win32: Clean Up As We Are Always on On XP or Later: bbac0eb (with modified commit message) I will close this bug now. --- Hello LRN, (In reply to LRN from comment #16) > You've just mapped the library into program's address space (earlier), then > you've got (just now) addresses of functions in that library, pointing at > the part of the program's address space where the library was mapped, and > now you're freeing the library, *unmapping it from the program's address > space*. Guess what is going to happen with these function pointers? On the contrary, I found that the code would run okay on my Visual Studio builds, meaning that I did test it when I came up with the initial patch for this bug. One such example would be in GLib's gio/gnetworking.c, where I would do LoadLibraryW() to get inet_pton() from ws2_32.dll (which is present in Vista and later), and do FreeLibrary() on the HMODULE right after doing GetProcAddress(). One can run the network-address test program in gio/tests/ on Windows Vista and later, and the proper inet_pton() acquired from ws2_32.dll will be used, and works. You might want to try the same on your MinGW builds, just to be more certain, if you'd like. Thanks though. --- With blessings, thank you!
Created attachment 301862 [details] libunload - A testcase for using functions from freed DLLs Fanc, here's a testcase for you. Please build with MSVC and run app.exe It crashes for me at f1(3).
One explanation why loading from ws2_32.dll works is that this DLL is already loaded into the address space of your program (indeed, glib uses tons of stuff from that library), so loading it *again* (to get the HMODULE, which then can be used to GetProcAddress) only increments the refcount, which gets decremented back once you free the library after getting the addresses, but does not reach zero, so the library doesn't go anywhere.