Attached to this mail are a couple of patches that should fix some issues with mouse cursors. It would be nice if some people could have a look and see if the patches break anything. Patches 1-4 move cursors into the server, 5 adds support for Xcursor cursors, 6 & 7 are cleanups, 8 fixes loading of .cur cursors, 9 adds support for animated cursors and 10 adds support for 32bpp cursors.
H. Verbeet wrote:
Attached to this mail are a couple of patches that should fix some issues with mouse cursors. It would be nice if some people could have a look and see if the patches break anything. Patches 1-4 move cursors into the server, 5 adds support for Xcursor cursors, 6 & 7 are cleanups, 8 fixes loading of .cur cursors, 9 adds support for animated cursors and 10 adds support for 32bpp cursors.
Overall, I'm very impressed with the patch set. However, I did find two things that I think need consideration or improvement: From 01_server_cursoricon.diff.txt:
+/* Destroy a cursor */ +DECL_HANDLER(destroy_cursor) +{ + unsigned int i; + cursor_t *cursor = free_user_handle( req->handle ); + + if (!cursor) return; + + for (i = 0; i < cursor->num_frames; ++i) + { + if (cursor->frames[i].bits) free( cursor->frames[i].bits ); + } + + free( cursor->frames ); +}
This appears to be the only place where a cursor is destroyed. Thus, a bad application could end up wineserver to leak memory. I think you need to investigate when Windows frees cursors that have not had DestroyCursor called on them. My guess is that it frees them on process destruction. From 09_user_ani_cursor.diff.txt:
--- a/dlls/user/Makefile.in +++ b/dlls/user/Makefile.in @@ -6,7 +6,7 @@ VPATH = @srcdir@ MODULE = user32.dll IMPORTLIB = libuser32.$(IMPLIBEXT) IMPORTS = gdi32 advapi32 kernel32 ntdll -DELAYIMPORTS = imm32 +DELAYIMPORTS = imm32 winmm EXTRALIBS = $(LIBUNICODE)
SPEC_SRCS16 = \
I'm not sure it is acceptable to import winmm from user32, even if it is a delay import. -- Rob Shearman
"Robert Shearman" <rob(a)codeweavers.com> wrote:
-DELAYIMPORTS = imm32 +DELAYIMPORTS = imm32 winmm EXTRALIBS = $(LIBUNICODE)
SPEC_SRCS16 = \
I'm not sure it is acceptable to import winmm from user32, even if it is a delay import.
Rob is right, native user32 doesn't import winmm by any means. Looking at user32 in a viewer it's easy to find chunk names such as RIFF, LIST, rate, anih although. That means that user32 has an internal implementation of mmioDescend which should be enough to parse animated cursor files. -- Dmitry.
On 07/07/06, Robert Shearman <rob(a)codeweavers.com> wrote:
This appears to be the only place where a cursor is destroyed. Thus, a bad application could end up wineserver to leak memory. I think you need to investigate when Windows frees cursors that have not had DestroyCursor called on them. My guess is that it frees them on process destruction. It does look like that's the case.
On 07/07/06, Dmitry Timoshkov <dmitry(a)codeweavers.com> wrote:
Rob is right, native user32 doesn't import winmm by any means. Looking at user32 in a viewer it's easy to find chunk names such as RIFF, LIST, rate, anih although. That means that user32 has an internal implementation of mmioDescend which should be enough to parse animated cursor files. That's a bit unfortunate. I guess that means there's no way around duplicating a significant part of that code into user32 then?
On 07/07/06, Robert Shearman <rob(a)codeweavers.com> wrote:
However, I did find two things that I think need consideration or improvement:
From 01_server_cursoricon.diff.txt:
+/* Destroy a cursor */ +DECL_HANDLER(destroy_cursor) +{ + unsigned int i; + cursor_t *cursor = free_user_handle( req->handle ); + + if (!cursor) return; + + for (i = 0; i < cursor->num_frames; ++i) + { + if (cursor->frames[i].bits) free( cursor->frames[i].bits ); + } + + free( cursor->frames ); +}
This appears to be the only place where a cursor is destroyed. Thus, a bad application could end up wineserver to leak memory. I think you need to investigate when Windows frees cursors that have not had DestroyCursor called on them. My guess is that it frees them on process destruction.
From 09_user_ani_cursor.diff.txt:
--- a/dlls/user/Makefile.in +++ b/dlls/user/Makefile.in @@ -6,7 +6,7 @@ VPATH = @srcdir@ MODULE = user32.dll IMPORTLIB = libuser32.$(IMPLIBEXT) IMPORTS = gdi32 advapi32 kernel32 ntdll -DELAYIMPORTS = imm32 +DELAYIMPORTS = imm32 winmm EXTRALIBS = $(LIBUNICODE)
SPEC_SRCS16 = \
I'm not sure it is acceptable to import winmm from user32, even if it is a delay import.
-- Rob Shearman
Attached is a slightly updated version of the patchset. I got rid of the winmm import, and it should now destroy cursors / icons when the process is destroyed. The changes are in parts 1 & 9, the rest should be pretty much unchanged.
"H. Verbeet" <hverbeet(a)gmail.com> wrote:
Attached is a slightly updated version of the patchset. I got rid of the winmm import, and it should now destroy cursors / icons when the process is destroyed. The changes are in parts 1 & 9, the rest should be pretty much unchanged.
I've looked only at 09_user_ani_cursor.diff.txt: 1. CURSORICON_LoadFromFile returns 0 on failure, so load_ani should return 0 on failure as well not INVALID_HANDLE_VALUE (which usually is returned only for kernel objects). 2. the loop in riff_find_chunk uses memcmp to compare DWORDs, comparing DWORD values directly is more efficient, and alignment is not a problem since all the fields and chunks are DWORD aligned. Making ptr of a type DWORD should help with that, although that makes pointer arithmetic slightly more tricky, that's still will look more readable IMO. -- Dmitry.
Hi, These are the warnings i get when i compile ur patch os kubuntu, gcc 4.0.3 cursoricon.c: In function 'CURSORICON_Load': cursoricon.c:1289: warning: 'frame_bits' may be used uninitialized in this function mouse.c: In function 'create_cursor': mouse.c:566: warning: pointer targets in passing argument 6 of 'XCreateImage' differ in signedness Wine build complete. cheers, VJ On 7/21/06, Dmitry Timoshkov <dmitry(a)codeweavers.com> wrote:
"H. Verbeet" <hverbeet(a)gmail.com> wrote:
Attached is a slightly updated version of the patchset. I got rid of the winmm import, and it should now destroy cursors / icons when the process is destroyed. The changes are in parts 1 & 9, the rest should be pretty much unchanged.
I've looked only at 09_user_ani_cursor.diff.txt:
1. CURSORICON_LoadFromFile returns 0 on failure, so load_ani should return 0 on failure as well not INVALID_HANDLE_VALUE (which usually is returned only for kernel objects).
2. the loop in riff_find_chunk uses memcmp to compare DWORDs, comparing DWORD values directly is more efficient, and alignment is not a problem since all the fields and chunks are DWORD aligned. Making ptr of a type DWORD should help with that, although that makes pointer arithmetic slightly more tricky, that's still will look more readable IMO.
-- Dmitry.
Another update. - The patches don't use INVALID_HANDLE_VALUE anymore (I was using that in some of the other patches as well). - riff_find_chunk() compares DWORDs - Code using get_cursor_frame() now has some error handling, in case the handle is invalid - Warnings should be fixed now, although I don't get the second one myself.
participants (4)
-
Dmitry Timoshkov -
H. Verbeet -
Robert Shearman -
Vijay Kiran Kamuju