[PATCH 0/3] Mouse cursor improvements on macOS and VNC

[PATCH 0/3] Mouse cursor improvements on macOS and VNC

[Phil Dennis-Jordan]

This series of loosely related changes provides some minor improvements
in mouse cursor usability.

 1. This one-liner changes alpha downsampling when using a UI frontend
    which does not support alpha-blended mouse cursors. Previously,
    any pixel with an alpha value other than 255 was treated as fully
    transparent in this context. This looks pretty bad when the guest
    OS uses anti-aliased cursors. (e.g. macOS) This occurs with some
    VNC clients, for example.
 2. This change has nothing to do with cursors: there are two
    functionally identical implementations of an int_clamp() inline
    function in the Qemu codebase. This unifies them in the shared
    cutils.h header, as I'm about to use it in a third location in
    patch number 3.
 3. This sizeable patch implements cursor support in the (macOS) Cocoa
    UI frontend. This fixes the issue of no mouse pointer showing up
    when using virtio-vga on a macOS host, for example.
    It unfortunately introduces some complexity to the mouse movement
    event handling when using a relative pointing device in the guest,
    as teleporting the cursor on the host offsets the next mouse event
    delta by a corresponding amount. We therefore need to track this
    offset and counteract it when processing the event. For details,
    see the commit message and inline comments.

This work was sponsored by Sauce Labs Inc.

Phil Dennis-Jordan (3):
  Cursor: 8 -> 1 bit alpha downsampling improvement
  hw: Moves int_clamp() implementations to header
  ui/cocoa: Adds support for mouse cursors

 hw/input/hid.c        |  12 +--
 hw/usb/dev-wacom.c    |  11 +--
 include/qemu/cutils.h |  11 +++
 ui/cocoa.m            | 167 +++++++++++++++++++++++++++++++++++++++++-
 ui/cursor.c           |   2 +-
 ui/trace-events       |   7 ++
 6 files changed, 185 insertions(+), 25 deletions(-)

-- 
2.36.1

[PATCH 1/3] Cursor: 8 -> 1 bit alpha downsampling improvement

[Phil Dennis-Jordan]

Mouse cursors with 8 bit alpha were downsampled to 1-bit opacity maps by
turning alpha values of 255 into 1 and everything else into 0. This
means that mostly-opaque pixels ended up completely invisible.

This patch changes the behaviour so that only pixels with less than 50%
alpha (0-127) are treated as transparent when converted to 1-bit alpha.

This greatly improves the subjective appearance of anti-aliased mouse
cursors, such as those used by macOS, when using a front-end UI without
support for alpha-blended cursors, such as some VNC clients.

Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
---
 ui/cursor.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ui/cursor.c b/ui/cursor.c
index 29717b3ecb..4c05e5555c 100644
--- a/ui/cursor.c
+++ b/ui/cursor.c
@@ -232,7 +232,7 @@ void cursor_get_mono_mask(QEMUCursor *c, int transparent, uint8_t *mask)
     for (y = 0; y < c->height; y++) {
         bit = 0x80;
         for (x = 0; x < c->width; x++, data++) {
-            if ((*data & 0xff000000) != 0xff000000) {
+            if ((*data & 0xff000000) < 0x80000000) {
                 if (transparent != 0) {
                     mask[x/8] |= bit;
                 }
-- 
2.36.1

[PATCH 2/3] hw: Moves int_clamp() implementations to header

[Phil Dennis-Jordan]

Both hw/input/hid.c and hw/usb/dev-wacom.c define identical versions
(aside from code formatting) of a clamping function, int_clamp().
(marked inline) To avoid duplication and to enable further re-use, this
change moves the function into qemu/cutils.h.

Signed-off-by: Phil Dennis-Jordan<phil@philjordan.eu>
---
 hw/input/hid.c        | 12 +-----------
 hw/usb/dev-wacom.c    | 11 +----------
 include/qemu/cutils.h | 11 +++++++++++
 3 files changed, 13 insertions(+), 21 deletions(-)

diff --git a/hw/input/hid.c b/hw/input/hid.c
index 76bedc1844..89f37f1bbb 100644
--- a/hw/input/hid.c
+++ b/hw/input/hid.c
@@ -26,6 +26,7 @@
 #include "qemu/osdep.h"
 #include "ui/console.h"
 #include "qemu/timer.h"
+#include "qemu/cutils.h"
 #include "hw/input/hid.h"
 #include "migration/vmstate.h"
 #include "trace.h"
@@ -336,17 +337,6 @@ static void hid_keyboard_process_keycode(HIDState *hs)
     }
 }
 
-static inline int int_clamp(int val, int vmin, int vmax)
-{
-    if (val < vmin) {
-        return vmin;
-    } else if (val > vmax) {
-        return vmax;
-    } else {
-        return val;
-    }
-}
-
 void hid_pointer_activate(HIDState *hs)
 {
     if (!hs->ptr.mouse_grabbed) {
diff --git a/hw/usb/dev-wacom.c b/hw/usb/dev-wacom.c
index 7177c17f03..539581010e 100644
--- a/hw/usb/dev-wacom.c
+++ b/hw/usb/dev-wacom.c
@@ -32,6 +32,7 @@
 #include "hw/usb/hid.h"
 #include "migration/vmstate.h"
 #include "qemu/module.h"
+#include "qemu/cutils.h"
 #include "desc.h"
 #include "qom/object.h"
 
@@ -215,16 +216,6 @@ static void usb_wacom_event(void *opaque,
     usb_wakeup(s->intr, 0);
 }
 
-static inline int int_clamp(int val, int vmin, int vmax)
-{
-    if (val < vmin)
-        return vmin;
-    else if (val > vmax)
-        return vmax;
-    else
-        return val;
-}
-
 static int usb_mouse_poll(USBWacomState *s, uint8_t *buf, int len)
 {
     int dx, dy, dz, b, l;
diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
index da15547bfb..4394f03326 100644
--- a/include/qemu/cutils.h
+++ b/include/qemu/cutils.h
@@ -277,6 +277,17 @@ static inline const char *yes_no(bool b)
      return b ? "yes" : "no";
 }
 
+static inline int int_clamp(int val, int vmin, int vmax)
+{
+    if (val < vmin) {
+        return vmin;
+    } else if (val > vmax) {
+        return vmax;
+    } else {
+        return val;
+    }
+}
+
 /*
  * helper to parse debug environment variables
  */
-- 
2.36.1

[PATCH 3/3] ui/cocoa: Adds support for mouse cursors

[Phil Dennis-Jordan]

This change implements the callbacks dpy_cursor_define and dpy_mouse_set
for the Cocoa UI. The incoming mouse cursor image is converted into an
NSCursor object, allowing the guest mouse cursor to be rendered as the
host's native OS cursor on macOS.

This is straightforward in absolute pointing mode, but rather trickier
with a relative pointing device:

 1. The cursor position in Qemu's coordinate system must be translated
    and converted into macOS's Core Graphics/Quartz coordinates when
    positioning the cursor. Additionally, the position already includes
    the hotspot offset; we'd prefer to use the host OS's hotspot support
    so we need subtract the hotspot vector off again.
 2. Setting the cursor position programmatically on macOS biases the
    next mouse movement event by the amount the cursor was shifted.
    If we didn't reverse that bias when forwarding the next event
    back into Qemu's input stack, this would create a feedback loop.
    (The behaviour of affecting mouse events makes sense for e.g.
    setting the cursor position in a remote access system.)

This change slightly improves the user experience when using virtual
display adapter implementations which check for UI back-end cursor
support, and fixes the issue of no visible mouse cursor when using one
which does not. (Such as virtio-vga)

Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
---
 ui/cocoa.m      | 167 +++++++++++++++++++++++++++++++++++++++++++++++-
 ui/trace-events |   7 ++
 2 files changed, 171 insertions(+), 3 deletions(-)

diff --git a/ui/cocoa.m b/ui/cocoa.m
index 981615a8b9..0c71533835 100644
--- a/ui/cocoa.m
+++ b/ui/cocoa.m
@@ -49,6 +49,7 @@
 #include "qemu/error-report.h"
 #include <Carbon/Carbon.h>
 #include "hw/core/cpu.h"
+#include "trace.h"
 
 #ifndef MAC_OS_VERSION_14_0
 #define MAC_OS_VERSION_14_0 140000
@@ -80,11 +81,17 @@ static void cocoa_switch(DisplayChangeListener *dcl,
 
 static void cocoa_refresh(DisplayChangeListener *dcl);
 
+static void cocoa_cursor_define(DisplayChangeListener *dcl, QEMUCursor *cursor);
+
+static void cocoa_mouse_set(DisplayChangeListener *dcl, int x, int y, int on);
+
 static const DisplayChangeListenerOps dcl_ops = {
     .dpy_name          = "cocoa",
     .dpy_gfx_update = cocoa_update,
     .dpy_gfx_switch = cocoa_switch,
     .dpy_refresh = cocoa_refresh,
+    .dpy_cursor_define = cocoa_cursor_define,
+    .dpy_mouse_set = cocoa_mouse_set,
 };
 static DisplayChangeListener dcl = {
     .ops = &dcl_ops,
@@ -299,6 +306,11 @@ @interface QemuCocoaView : NSView
     BOOL isMouseGrabbed;
     BOOL isAbsoluteEnabled;
     CFMachPortRef eventsTap;
+    NSCursor *current_cursor;
+    int cursor_hot_x;
+    int cursor_hot_y;
+    int offset_delta_x;
+    int offset_delta_y;
 }
 - (void) switchSurface:(pixman_image_t *)image;
 - (void) grabMouse;
@@ -320,6 +332,9 @@ - (BOOL) isMouseGrabbed;
 - (BOOL) isAbsoluteEnabled;
 - (QEMUScreen) gscreen;
 - (void) raiseAllKeys;
+- (void) setCursor:(NSCursor*)newCursor hotspotX:(int)hotX y:(int)hotY;
+- (void) setMouseX:(int)x y:(int)y showCursor:(BOOL)showCursor;
+
 @end
 
 QemuCocoaView *cocoaView;
@@ -376,6 +391,9 @@ - (void) dealloc
         pixman_image_unref(pixman_image);
     }
 
+    [self->current_cursor release];
+    self->current_cursor = nil;
+
     if (eventsTap) {
         CFRelease(eventsTap);
     }
@@ -407,6 +425,68 @@ - (void) selectConsoleLocked:(unsigned int)index
     [self updateUIInfo];
 }
 
+- (void) setCursor:(NSCursor*)newCursor hotspotX:(int)hotX y:(int)hotY
+{
+    [self->current_cursor release];
+    [newCursor retain];
+    self->current_cursor = newCursor;
+
+    cocoaView->cursor_hot_x = hotX;
+    cocoaView->cursor_hot_y = hotY;
+
+    [self.window invalidateCursorRectsForView:self];
+}
+
+- (void) resetCursorRects
+{
+    if (self->current_cursor == nil) {
+        [super resetCursorRects];
+    } else {
+        [self addCursorRect:NSMakeRect(0.0, 0.0, self->screen.width, self->screen.height) cursor:self->current_cursor];
+    }
+}
+
+- (void) setMouseX:(int)x y:(int)y showCursor:(BOOL)showCursor
+{
+    if (isAbsoluteEnabled) {
+        offset_delta_x = 0;
+        offset_delta_y = 0;
+        return;
+    } else if (!isMouseGrabbed) {
+        return;
+    }
+
+    NSWindow* window = [self window];
+
+    /* Coordinates seem to come in already offset by hotspot; undo that. Also
+     * avoid out-of-window coordinates. */
+    x += cursor_hot_x;
+    y += cursor_hot_y;
+    x = int_clamp(x, 0, screen.width);
+    y = int_clamp(y, 0, screen.height);
+    /* Flip coordinates so origin is bottom left (Cocoa), not top left (Qemu),
+     * before translating into window and then desktop coordinate systems. */
+    y = screen.height - y;
+
+    NSPoint new_pos_window = [self convertPoint:NSMakePoint(x, y) toView:nil];
+    NSPoint prev_pos_window = window.mouseLocationOutsideOfEventStream;
+
+    CGPoint screen_pos = [window convertPointToScreen:new_pos_window];
+
+    /* Translate from Cocoa (origin: main screen bottom left, +y up)
+     * to Quartz (origin: top left, +y down) coordinate system. */
+    screen_pos.y = NSScreen.mainScreen.frame.size.height - screen_pos.y;
+
+    /* Warp moves the host cursor to the new position. This doesn't generate a
+     * spurious move event, but it does offset the delta for next genuine event,
+     * which we need to take into account when that event comes in. */
+    CGWarpMouseCursorPosition(screen_pos);
+
+    offset_delta_x += (prev_pos_window.x - new_pos_window.x);
+    /* -ve due to Cocoa -> Qemu/Quartz Y axis conversion: */
+    offset_delta_y -= (prev_pos_window.y - new_pos_window.y);
+}
+
 - (void) hideCursor
 {
     if (!cursor_hide) {
@@ -1005,9 +1085,21 @@ - (void) handleMouseEvent:(NSEvent *)event
             /* Note that the origin for Cocoa mouse coords is bottom left, not top left. */
             qemu_input_queue_abs(dcl.con, INPUT_AXIS_X, p.x * d, 0, screen.width);
             qemu_input_queue_abs(dcl.con, INPUT_AXIS_Y, screen.height - p.y * d, 0, screen.height);
+            trace_cocoa_handle_mouse_event_absolute(p.x, p.y, screen.width, screen.height, p.x * d, screen.height - p.y * d);
         } else {
-            qemu_input_queue_rel(dcl.con, INPUT_AXIS_X, [event deltaX]);
-            qemu_input_queue_rel(dcl.con, INPUT_AXIS_Y, [event deltaY]);
+            /* Programmatically moving the Cocoa mouse cursor also offsets the
+             * next mouse event, so we offset by the amount we moved the cursor
+             * to avoid a feedback loop. */
+            int delta_x = [event deltaX] + offset_delta_x;
+            int delta_y = [event deltaY] + offset_delta_y;
+            trace_cocoa_handle_mouse_event_relative(
+                [event deltaX], [event deltaY],
+                offset_delta_x, offset_delta_y,
+                delta_x, delta_y);
+            qemu_input_queue_rel(dcl.con, INPUT_AXIS_X, /*[event deltaX]/*/delta_x/**/);
+            qemu_input_queue_rel(dcl.con, INPUT_AXIS_Y, /*[event deltaY]/*/delta_y/**/);
+            offset_delta_x = 0;
+            offset_delta_y = 0;
         }
 
         qemu_input_event_sync();
@@ -1084,19 +1176,26 @@ - (void) otherMouseUp:(NSEvent *)event
 
 - (void) grabMouse
 {
+    trace_cocoa_grab_mouse();
     COCOA_DEBUG("QemuCocoaView: grabMouse\n");
 
     if (qemu_name)
         [[self window] setTitle:[NSString stringWithFormat:@"QEMU %s - (Press  " UC_CTRL_KEY " " UC_ALT_KEY " G  to release Mouse)", qemu_name]];
     else
         [[self window] setTitle:@"QEMU - (Press  " UC_CTRL_KEY " " UC_ALT_KEY " G  to release Mouse)"];
-    [self hideCursor];
+
+    if (current_cursor == nil) {
+        [self hideCursor];
+    }
     CGAssociateMouseAndMouseCursorPosition(isAbsoluteEnabled);
     isMouseGrabbed = TRUE; // while isMouseGrabbed = TRUE, QemuCocoaApp sends all events to [cocoaView handleEvent:]
+    offset_delta_x = 0;
+    offset_delta_y = 0;
 }
 
 - (void) ungrabMouse
 {
+    trace_cocoa_ungrab_mouse();
     COCOA_DEBUG("QemuCocoaView: ungrabMouse\n");
 
     if (qemu_name)
@@ -1104,6 +1203,11 @@ - (void) ungrabMouse
     else
         [[self window] setTitle:@"QEMU"];
     [self unhideCursor];
+
+    if (current_cursor == nil) {
+        [self unhideCursor];
+    }
+
     CGAssociateMouseAndMouseCursorPosition(TRUE);
     isMouseGrabbed = FALSE;
     [self raiseAllButtons];
@@ -2000,6 +2104,63 @@ static void cocoa_refresh(DisplayChangeListener *dcl)
     [pool release];
 }
 
+static NSImage *cocoa_create_image_argb32(size_t width, size_t height, const void *pixel_data)
+{
+    size_t buffer_size = width * height * 4lu;
+    CGDataProviderRef provider = CGDataProviderCreateWithData(
+        NULL, pixel_data, buffer_size, NULL);
+    size_t bpc = 8;
+    size_t bpp = 32;
+    size_t bytes_per_row = 4u * width;
+    CGColorSpaceRef color_space = CGColorSpaceCreateDeviceRGB();
+    CGBitmapInfo bitmap_info =
+        kCGBitmapByteOrder32Little | kCGImageAlphaFirst;
+    CGColorRenderingIntent intent = kCGRenderingIntentDefault;
+
+    CGImageRef cg_image = CGImageCreate(
+        width,
+        height,
+        bpc,
+        bpp,
+        bytes_per_row,
+        color_space,
+        bitmap_info,
+        provider,
+        NULL,       // decode
+        YES,        // should interpolate
+        intent);
+
+    NSImage *image = [[NSImage alloc] initWithCGImage:cg_image size:NSMakeSize(width, height)];
+    CGImageRelease(cg_image);
+    CGColorSpaceRelease(color_space);
+    CGDataProviderRelease(provider);
+    return image;
+}
+
+static void cocoa_cursor_define(DisplayChangeListener *dcl, QEMUCursor *cursor)
+{
+    NSImage *cursor_image = nil;
+    NSPoint hotspot = { cursor->hot_x, cursor->hot_y };
+    trace_cocoa_cursor_define(cursor->hot_x, cursor->hot_y,
+                              cursor->width, cursor->height);
+    if (cursor == NULL || cursor->width <= 0 || cursor->height <= 0) {
+        cursor_image = [[NSImage alloc] initWithSize:NSMakeSize(1.0, 1.0)];
+    } else {
+        cursor_image = cocoa_create_image_argb32(cursor->width, cursor->height,
+                                                 cursor->data);
+    }
+    NSCursor *cocoa_cursor =
+        [[NSCursor alloc] initWithImage:cursor_image hotSpot:hotspot];
+    [cursor_image release];
+    [cocoaView setCursor:cocoa_cursor hotspotX:cursor->hot_x y:cursor->hot_y];
+    [cocoa_cursor release];
+}
+
+static void cocoa_mouse_set(DisplayChangeListener *dcl, int x, int y, int on)
+{
+    [cocoaView setMouseX:x y:y showCursor:(on != 0)];
+}
+
 static void cocoa_display_init(DisplayState *ds, DisplayOptions *opts)
 {
     NSAutoreleasePool * pool = [[NSAutoreleasePool alloc] init];
diff --git a/ui/trace-events b/ui/trace-events
index 69ff22955d..e53601693d 100644
--- a/ui/trace-events
+++ b/ui/trace-events
@@ -166,3 +166,10 @@ dbus_filter(unsigned int serial, unsigned int filter) "serial=%u (<= %u)"
 
 # egl-helpers.c
 egl_init_d3d11_device(void *p) "d3d device: %p"
+
+# cocoa.m
+cocoa_cursor_define(int hot_x, int hot_y, uint16_t width, uint16_t height) "Cursor hotspot: (%d, %d), size: %" PRIu16 "x%" PRIu16
+cocoa_grab_mouse(void) ""
+cocoa_ungrab_mouse(void) ""
+cocoa_handle_mouse_event_relative(int raw_dx, int raw_dy, int offset_dx, int offset_dy, int net_dx, int net_dy) "raw delta: %d, %d; offset for delta: %d, %d; net delta: %d, %d"
+cocoa_handle_mouse_event_absolute(int event_x, int event_y, int screen_width, int screen_height, int window_x, int window_y) "event: %d, %d; screen: %d x %d; position: %d, %d"
-- 
2.36.1

Re: [PATCH 1/3] Cursor: 8 -> 1 bit alpha downsampling improvement

[Akihiko Odaki]

On 2024/06/09 5:20, Phil Dennis-Jordan wrote:
> Mouse cursors with 8 bit alpha were downsampled to 1-bit opacity maps by
> turning alpha values of 255 into 1 and everything else into 0. This
> means that mostly-opaque pixels ended up completely invisible.
> 
> This patch changes the behaviour so that only pixels with less than 50%
> alpha (0-127) are treated as transparent when converted to 1-bit alpha.
> 
> This greatly improves the subjective appearance of anti-aliased mouse
> cursors, such as those used by macOS, when using a front-end UI without
> support for alpha-blended cursors, such as some VNC clients.
> 
> Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
> ---
>   ui/cursor.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/ui/cursor.c b/ui/cursor.c
> index 29717b3ecb..4c05e5555c 100644
> --- a/ui/cursor.c
> +++ b/ui/cursor.c
> @@ -232,7 +232,7 @@ void cursor_get_mono_mask(QEMUCursor *c, int transparent, uint8_t *mask)
>       for (y = 0; y < c->height; y++) {
>           bit = 0x80;
>           for (x = 0; x < c->width; x++, data++) {
> -            if ((*data & 0xff000000) != 0xff000000) {
> +            if ((*data & 0xff000000) < 0x80000000) {

You can just evaluate: !(*data & 0x80000000)

Re: [PATCH 2/3] hw: Moves int_clamp() implementations to header

[Akihiko Odaki]

On 2024/06/09 5:20, Phil Dennis-Jordan wrote:
> Both hw/input/hid.c and hw/usb/dev-wacom.c define identical versions
> (aside from code formatting) of a clamping function, int_clamp().
> (marked inline) To avoid duplication and to enable further re-use, this
> change moves the function into qemu/cutils.h.

Wht about replacing int_clamp(a, b, c) with MIN(MAX(a, b), c)? 
MIN(MAX(a, b), c) has a few advantages:
- It works with any integer types
   (so you can replace even uint_clamp() in hw/display/vga.c)
- It makes clear that b is the minimum value and c is the maximum value
   while it is not with int_clamp()
- It is already used in other places.

Re: [PATCH 3/3] ui/cocoa: Adds support for mouse cursors

[Akihiko Odaki]

On 2024/06/09 5:20, Phil Dennis-Jordan wrote:
> This change implements the callbacks dpy_cursor_define and dpy_mouse_set
> for the Cocoa UI. The incoming mouse cursor image is converted into an
> NSCursor object, allowing the guest mouse cursor to be rendered as the
> host's native OS cursor on macOS.
> 
> This is straightforward in absolute pointing mode, but rather trickier
> with a relative pointing device:
> 
>   1. The cursor position in Qemu's coordinate system must be translated
>      and converted into macOS's Core Graphics/Quartz coordinates when
>      positioning the cursor. Additionally, the position already includes
>      the hotspot offset; we'd prefer to use the host OS's hotspot support
>      so we need subtract the hotspot vector off again.
>   2. Setting the cursor position programmatically on macOS biases the
>      next mouse movement event by the amount the cursor was shifted.
>      If we didn't reverse that bias when forwarding the next event
>      back into Qemu's input stack, this would create a feedback loop.
>      (The behaviour of affecting mouse events makes sense for e.g.
>      setting the cursor position in a remote access system.)
> 
> This change slightly improves the user experience when using virtual
> display adapter implementations which check for UI back-end cursor
> support, and fixes the issue of no visible mouse cursor when using one
> which does not. (Such as virtio-vga)

Thanks for working on ui/cocoa, but I already have submitted a patch for 
this particular problem:
https://patchew.org/QEMU/20240318-cursor-v1-0-0bbe6c382217@daynix.com/

The difference between these patches is that my patch does not use 
warping at all. I thought reversing the mouse movement bias is a fragile 
approach that depends on the details of how Quartz works.

Re: [PATCH 2/3] hw: Moves int_clamp() implementations to header

[Phil Dennis-Jordan]

On Sun, 9 Jun 2024 at 11:00, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> On 2024/06/09 5:20, Phil Dennis-Jordan wrote:
> > Both hw/input/hid.c and hw/usb/dev-wacom.c define identical versions
> > (aside from code formatting) of a clamping function, int_clamp().
> > (marked inline) To avoid duplication and to enable further re-use, this
> > change moves the function into qemu/cutils.h.
>
> Wht about replacing int_clamp(a, b, c) with MIN(MAX(a, b), c)?
> MIN(MAX(a, b), c) has a few advantages:
> - It works with any integer types
>    (so you can replace even uint_clamp() in hw/display/vga.c)

Well, that can of course also be achieved by wrapping MIN(MAX(v, min),
max) in a new CLAMP(v, min, max) macro.

> - It makes clear that b is the minimum value and c is the maximum value
>    while it is not with int_clamp()

I guess this aspect is rather subjective. When I see a MIN(MAX(
construct I generally feel the need to triple-check to make sure which
way around it is. (The minimum value is passed to MAX, the maximum to
MIN.)

On the other hand, there is precedent and convention for a clamp()
function with that order of parameters in other APIs and programming
languages: C++ std::clamp, Java Math.clamp(), OpenCL kernels and
OpenGL shaders can both use a built-in clamp() function, etc.

> - It is already used in other places.


The CLAMP macro would be my preferred compromise, although I don't
know how keen everyone is on introducing a new macro named after a
short, generic word.

Re: [PATCH 2/3] hw: Moves int_clamp() implementations to header

[Akihiko Odaki]

On 2024/06/10 17:50, Phil Dennis-Jordan wrote:
> On Sun, 9 Jun 2024 at 11:00, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>> On 2024/06/09 5:20, Phil Dennis-Jordan wrote:
>>> Both hw/input/hid.c and hw/usb/dev-wacom.c define identical versions
>>> (aside from code formatting) of a clamping function, int_clamp().
>>> (marked inline) To avoid duplication and to enable further re-use, this
>>> change moves the function into qemu/cutils.h.
>>
>> Wht about replacing int_clamp(a, b, c) with MIN(MAX(a, b), c)?
>> MIN(MAX(a, b), c) has a few advantages:
>> - It works with any integer types
>>     (so you can replace even uint_clamp() in hw/display/vga.c)
> 
> Well, that can of course also be achieved by wrapping MIN(MAX(v, min),
> max) in a new CLAMP(v, min, max) macro.
> 
>> - It makes clear that b is the minimum value and c is the maximum value
>>     while it is not with int_clamp()
> 
> I guess this aspect is rather subjective. When I see a MIN(MAX(
> construct I generally feel the need to triple-check to make sure which
> way around it is. (The minimum value is passed to MAX, the maximum to
> MIN.)
> 
> On the other hand, there is precedent and convention for a clamp()
> function with that order of parameters in other APIs and programming
> languages: C++ std::clamp, Java Math.clamp(), OpenCL kernels and
> OpenGL shaders can both use a built-in clamp() function, etc.

That sounds convincing. Please add the CLAMP macro.

Re: [PATCH 3/3] ui/cocoa: Adds support for mouse cursors

[Phil Dennis-Jordan]

[-- Attachment #1: Type: text/plain, Size: 2612 bytes --]

On Sun, 9 Jun 2024 at 11:06, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:

> Thanks for working on ui/cocoa, but I already have submitted a patch for
> this particular problem:
> https://patchew.org/QEMU/20240318-cursor-v1-0-0bbe6c382217@daynix.com/
>

Sorry, I missed this patch set - thanks for bringing it to my attention.


> The difference between these patches is that my patch does not use
> warping at all. I thought reversing the mouse movement bias is a fragile
> approach that depends on the details of how Quartz works.
>

Hmm. So, I agree that the relative cursor implementation with NSCursor is
somewhat awkward. I'm not sure it's as fragile as you make out as the
behaviour of the APIs used hasn't changed in decades and has plenty of
existing software depending on it. Still, it might behave awkwardly in the
context of other apps warping the cursor at the same time. I also
definitely think host cursor integration is useful and valuable, at least
in absolute pointing mode - for example, when the host system is itself
being remote controlled, and also to avoid the cursor being cropped near
the edges of the guest viewport.

The CALayer based rendering makes sense to me in relative mode though. For
one, it avoids the complicated event offsets. The cursor cropping actually
makes sense as a visual cue when the cursor is actually constrained to the
guest viewport while mouse input is grabbed. And because the guest cursor
is decoupled from the host cursor even after ungrabbing, it makes sense to
continue rendering it even when Qemu has relinquished the host cursor.

I've therefore reworked my NSCursor code on top of your CALayer cursor and
change notifier work so that the NSCursor is visible and updated with the
guest's cursor image when in absolute mode, and the CALayer draws the
cursor in relative mode. Because of the chain of patch dependencies I've
staged it on gitlab here for initial feedback/testing/review:
https://gitlab.com/pmdj/qemu-upstreaming/-/compare/master...m-cocoa-cursors-2.0?from_project_id=53960510&straight=false

Let me know what you think. If we decide to go with this approach we can
post our respective patches as a combined v2 patchset to the list.

Incidentally, that version of my NSCursor patch includes a few
refinements/fixes to the CALayer patch which I'll tease out into a separate
commit, and which I'd recommend applying even if consensus settles on a
CALayer-only approach. (setCursor: was rather long and messy; it also
leaked a colour space object; layer and cursor objects would leak if the
view was hypothetically dealloc'd)

Thanks,
Phil

[-- Attachment #2: Type: text/html, Size: 3660 bytes --]

Re: [PATCH 3/3] ui/cocoa: Adds support for mouse cursors

[Akihiko Odaki]

On 2024/06/10 23:00, Phil Dennis-Jordan wrote:
> On Sun, 9 Jun 2024 at 11:06, Akihiko Odaki <akihiko.odaki@daynix.com 
> <mailto:akihiko.odaki@daynix.com>> wrote:
> 
>     Thanks for working on ui/cocoa, but I already have submitted a patch
>     for
>     this particular problem:
>     https://patchew.org/QEMU/20240318-cursor-v1-0-0bbe6c382217@daynix.com/ <https://patchew.org/QEMU/20240318-cursor-v1-0-0bbe6c382217@daynix.com/>
> 
> 
> Sorry, I missed this patch set - thanks for bringing it to my attention.
> 
>     The difference between these patches is that my patch does not use
>     warping at all. I thought reversing the mouse movement bias is a
>     fragile
>     approach that depends on the details of how Quartz works.
> 
> 
> Hmm. So, I agree that the relative cursor implementation with NSCursor 
> is somewhat awkward. I'm not sure it's as fragile as you make out as the 
> behaviour of the APIs used hasn't changed in decades and has plenty of 
> existing software depending on it. Still, it might behave awkwardly in 
> the context of other apps warping the cursor at the same time. I also 
> definitely think host cursor integration is useful and valuable, at 
> least in absolute pointing mode - for example, when the host system is 
> itself being remote controlled, and also to avoid the cursor being 
> cropped near the edges of the guest viewport.

Can you elaborate more about the remote control scenario? I don't think 
having extra code just to fix cropped cursor is not worthwhile (I even 
feel a bit awkward to make the cursor overflow.)

> 
> The CALayer based rendering makes sense to me in relative mode though. 
> For one, it avoids the complicated event offsets. The cursor cropping 
> actually makes sense as a visual cue when the cursor is actually 
> constrained to the guest viewport while mouse input is grabbed. And 
> because the guest cursor is decoupled from the host cursor even after 
> ungrabbing, it makes sense to continue rendering it even when Qemu has 
> relinquished the host cursor.
> 
> I've therefore reworked my NSCursor code on top of your CALayer cursor 
> and change notifier work so that the NSCursor is visible and updated 
> with the guest's cursor image when in absolute mode, and the CALayer 
> draws the cursor in relative mode. Because of the chain of patch 
> dependencies I've staged it on gitlab here for initial 
> feedback/testing/review:

You can add Based-on: to the cover letter to clarify the dependency, and 
add "RFC" to the subject if the code is not ready to pull. Please look 
at docs/devel/submitting-a-patch.rst for more info.

Regards,
Akihiko Odaki

Re: [PATCH 3/3] ui/cocoa: Adds support for mouse cursors

[Phil Dennis-Jordan]

On Tue, 11 Jun 2024 at 09:36, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> > […] I also
> > definitely think host cursor integration is useful and valuable, at
> > least in absolute pointing mode - for example, when the host system is
> > itself being remote controlled, and also to avoid the cursor being
> > cropped near the edges of the guest viewport.
>
> Can you elaborate more about the remote control scenario? I don't think
> having extra code just to fix cropped cursor is not worthwhile (I even
> feel a bit awkward to make the cursor overflow.)

If you're remote-controlling the host Mac, many VNC/RDP/whatever
clients will use a local cursor and simply request cursor image
updates from the server and apply them to the local native cursor.
That way, there's no lag when moving the cursor even on slower
connections, which you'd otherwise get if you had to wait for the
regular screen image update, and which can make precise positioning
difficult.

At any rate, most other Qemu UI frontends likewise implement guest
cursors by setting the guest-supplied cursor image on the host's
native windowing system's cursor, e.g. gdk_window_set_cursor(). I
don't really see a good reason why macOS should be different? Qemu
would also hardly be the first VMM on the Mac to pass guest pointers
through as NSPointers - Parallels Desktop appears to do the same, for
example.

> You can add Based-on: to the cover letter to clarify the dependency, and
> add "RFC" to the subject if the code is not ready to pull. Please look
> at docs/devel/submitting-a-patch.rst for more info.

I've done that now, thanks.
https://patchew.org/QEMU/20240625134931.92279-1-phil@philjordan.eu/

Phil