Re: Cursor patches
On 25/07/06, H. Verbeet <[EMAIL PROTECTED]> wrote: On 25/07/06, H. Verbeet <[EMAIL PROTECTED]> wrote: > Another update. Ignore that. It doesn't actually work. In riff_find_chunk(): *((DWORD *)ptr) + 2 == chunk_id Should be: *((DWORD *)ptr + 2) == chunk_id
Re: Cursor patches
On 25/07/06, H. Verbeet <[EMAIL PROTECTED]> wrote: Another update. Ignore that. It doesn't actually work.
Re: Cursor patches
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. cursor_patches_20060724.tar.bz2 Description: BZip2 compressed data
Re: Cursor patches
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 <[EMAIL PROTECTED]> wrote: "H. Verbeet" <[EMAIL PROTECTED]> 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.
Re: Cursor patches
"H. Verbeet" <[EMAIL PROTECTED]> 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.
Re: Cursor patches
On 07/07/06, Robert Shearman <[EMAIL PROTECTED]> 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.
cursor_patches_20060721.tar.bz2
Description: BZip2 compressed data
Re: Cursor patches
On 07/07/06, Robert Shearman <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> 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?
Re: Cursor patches
"Robert Shearman" <[EMAIL PROTECTED]> 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.
Re: Cursor patches
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
