Re: [Qemu-devel] [PATCH] ui/cocoa: Adding cursor support

[Chen Zhang]

> On Mar 14, 2019, at 1:35 AM, BALATON Zoltan <address@hidden> wrote:
> 
> On Wed, 13 Mar 2019, Chen Zhang wrote:
>> I sympathize with your situation, but the things on macOS seems a little 
>> different.
>> 
>> The QEMU Cocoa UI starts in the `main` thread and detach a `qemu_main` 
>> thread which runs stuff in vl.c etc. On the contrary, QEMU with gtk and 
>> other UIs just scheduled its event loop in the qemu_main thread without 
>> detaching a dedicated thread. These two kinds of event loop(, the cocoa one 
>> and the glib one,) do not mix well.
> 
> I was following the recent ui/cocoa.m changes so I'm aware of this.
> 
>> A second problem is that recently Cocoa UI event handling routine is locking 
>> io threads with qemu_mutex_lock_iothread(), and this may delay bottom halves 
>> and subsequently the dpy_mouse_set calls, as glib_pollfds_poll() is also 
>> protected by qemu_mutex_lock_iothread().
>> 
>> My preliminary guess is that all mouse NSEvents must be queued and processed 
>> in a way not breaking any causal order.  Maybe the next NSEvent shall not be 
>> processed before the corresponding dpy_mouse_set call.
> 
> Sorry if this was not clear but the cursor change to black square problem is 
> not cocoa UI related and we've actually seen it on Linux with gtk (I think, 
> it was reported to me, not reproduced by me). Maybe I should not have mixed 
> that in here and report independently.
> 
> Based on my experience with the ati-vga model where I've implemented hardware 
> cursor both with dpy_cursor_define()/dpy_mouse_set() and with 
> cursor_invalidate()/cursor_draw_line() callbacks I've found the latter to 
> work more smoothly. I don't know why jumpy mouse pointer happens with the 
> dpy_mouse_set() way but that's what I see. With mouse drawn from screen 
> refresh the pointer may stop when guest is busy but it won't jump around 
> unlike when it's updated on the host side on register writes not tied to 
> screen refresh. I don't know how much does it relate to your case with Cocoa 
> UI but there may be some similarities that's why I've brought this up here. 
> If it's not related just ignore my comment.
For the Cocoa UI, I tried to isolate the cursor movement issue after calling 
CGWarpMouseCursorPosition(). 

In an empty Cocoa window app with a minimal overridden -sendEvent:, a minimal 
-handleEvent:, calling CGWarpMouseCursorPosition calls upon mouse-moved event 
showed the similar cursor movement issue as in QEMU.

It was suggested that Apple throttled how frequently the cursor could be moved 
in earlier version of macOS. [1][2] However none of these workaround methods I 
found worked on Mojave so far.

Best Regards,

[1] 
https://stackoverflow.com/questions/10196603/using-cgeventsourcesetlocaleventssuppressioninterval-instead-of-the-deprecated

[2] https://lists.apple.com/archives/cocoa-dev/2012/Feb/msg00372.html
> 
> Regards,
> BALATON Zoltan
> 
>> 
>>> On Mar 12, 2019, at 8:32 PM, BALATON Zoltan <address@hidden> wrote:
>>> 
>>> On Tue, 12 Mar 2019, Chen Zhang via Qemu-devel wrote:
>>>> Hi,
>>>> 
>>>> I did try to utilize NSCursor and CGWarpMouseCursorPosition API before 
>>>> this compromise. In cocoa_mouse_set, the position of cursor should to be 
>>>> modified, but the bottom half that called it was not scheduled on main 
>>>> thread. UI operations have to be queued on main thread asynchronously 
>>>> thereafter. This introduced troubles.
>>>> 
>>>> One issue was that once CGWarpMouseCursorPosition(), which should not 
>>>> trigger any mouse events, was called from cocoa_mouse_set(), the cursor 
>>>> position accelerated in a positive feedback manner. This was independent 
>>>> from the association state between mouse movement and cursor.
>>>> 
>>>> Another issue was that the cursor moved several steps later than the Cocoa 
>>>> mouse events.
>>>> 
>>>> All these phenomena made me wonder if I was messing up with the host input 
>>>> source, runloop, the bottom halves and the asynchronous changes made to 
>>>> cursor positions.
>>>> 
>>>> On the other hand, rendering the cursor in the window frame buffer only 
>>>> introduce a few more dirty rectangles per second and this does not seem to 
>>>> add up any significant amount of overhead. At least it keeps the troubles 
>>>> above away.
>>> 
>>> We've also found similar mouse jumping problems with the ati-vga after 
>>> using define_cursor and host side cursor but also another problem where 
>>> cursor turned into a black square sometimes. I think the latter happens 
>>> when guest sets the memory offset to the cursor (which is when I call 
>>> cursor_define) but only writes cursor data there later. This works if 
>>> cursor data is read only when rendering the cursor but fails in QEMU with 
>>> cursor_define. Problem does not happen with guest_hwcursor option of 
>>> ati-vga that uses the cirrus callbacks that read cursor data when rendering 
>>> but that needs another display surface to render cursor into. I cannot fix 
>>> this by calling define_cursor on all register writes because guest writes 
>>> offset on every mouse move even if it's not changed and calling 
>>> cursor_define causes flickering of the pointer.
>> Is it possible to compare the cursor pixel data before calling define_cursor 
>> and avoid some redundant calls?
>>> 
>>> So I wonder if this defining cursor in the host is a good idea after all 
>>> given all the above problems with it. (It might be wanted for remote 
>>> displays as an optimisation with tradeoffs for accuracy but not as a 
>>> default on local displays.) Could it be modified to be called from screen 
>>> update at least instead of being completely async which might fix some of 
>>> the problems. (I can't do that from ati-vga itself because I don't want to 
>>> have custom screen refresh function just for this which would need 
>>> reimplementing all of vga.) Or maybe the cirrus callbacks could be fixed to 
>>> render cursor in a similar way like this patch and then use that?
>>> 
>>> I think my point is that there seems to be a problem with host side cursor 
>>> which would need to be solved more generally within QEMU instead of 
>>> inventing workarounds individually within device models and ui backends.
>>> 
>>> Regards,
>>> BALATON Zoltan
>>> 
>>>> 
>>>> Best regards,
>>>> 
>>>>> On Mar 12, 2019, at 3:26 PM, Gerd Hoffmann <address@hidden> wrote:
>>>>> 
>>>>> Hi,
>>>>> 
>>>>>> +        if (cursorVisible && cursorImage && NSIntersectsRect(rect, 
>>>>>> cursorRect)) {
>>>>>> +            CGContextDrawImage (viewContextRef, cursorRect, 
>>>>>> cursorImage);
>>>>> 
>>>>> So you are rendering the cursor to the window.
>>>>> 
>>>>> Better approach would be to just set the cursor of the host window,
>>>>> like the gtk UI does, using gdk_window_set_cursor().
>>>>> 
>>>>> cheers,
>>>>> Gerd