Re: [Qemu-devel] [RfC PATCH] vnc: rich cursor support.

[Anthony Liguori <anthony@codemonkey.ws>]

On 05/03/2010 06:59 AM, Gerd Hoffmann wrote:
>    Hi,
>
> Simple patch.  Difficuilt matter.  Not really sure where to go from
> here ...
>    

It'll be a complicated patch :-)  I looked at this a while ago and there 
are a few gotchas.

> The whole thing is about local cursor support, i.e. just let the UI
> (sdl, vnc viewer, spice client, ...) draw the mouse pointer directly,
> without round trip to the guest for mouse pointer rendering.
>
> Current state in upstream qemu:  vmware svga supports it.  sdl supports
> it.  When running vmware vga on sdl you should get it.  In theory.  In
> practice it doesn't work correctly.
>    

Cirrus technically supports it do but almost nothing uses the hardware 
cursor support in Cirrus (only win2k configured in a certain way).

> SDL can only handle 1bit (black/white) mouse cursors (with mask)
>    

I personally don't think we should even bother with anything other than 
ARGB cursors.  Not enough things render 1bit cursors via hardware in my 
experience.

> I've seen vmware vga send only 1bit cursors (with mask, winxp guest),
> althougth it seems to be designed to support colored pointers too.
>    

VMware VGA sends full ARGB cursors.  That's what you get by default when 
you use vmware-vga and X.

> vnc has two extentions for it: xcursor (1bit too, using a mask, but also
> allows to specify foreground/backgrund color) and rich cursor (uses
> pixelformat of the display, i.e. allows color).
>    

The key problem with VNC is that it has no notion of disabling cursor 
offload.  This means that when the guest tries to disable it (like via a 
reset cycle), we have to make sure to send a NULL cursor to avoid the 
cursor being rendered.

This effectively disables any attempt by the client to draw a double 
cursor though.

> spice supports a whole bunch of formats, although I've seen only two of
> them used in practice:  1bit (with mask) and 32bit (rgb + alpha).
>
>
> The current hooks supporting local pointer (mouse_set, cursor_define)
> are in DisplayState, although I think they belong into
> DisplayChangeListener.
>
>
> Ok, how to sort this mess?
>
>
> I think we should put everything into a QEMUCursor struct, so we don't
> have to pass tons of parameters to the ->cursor_define() callback.
>    

Agreed.

> Does it make sense to reuse "struct PixelFormat" for the cursor?  I tend
> to think not.  I expect we'll see three different cases be used in
> practice:
>
>    (1) 1-bit image (and mask).
>    (2) Same pixelformat as DisplayState (and mask).
>    (3) 32bit RGB + alpha.
>    

I think always making a cursor 32bit ARGB would be reasonable.  Let the 
backend sort things out.  Since we have the PixelFormat structures, it's 
easy enough to add a routine to convert between formats.

> Current PixelFormat can cover only (2) and even in that case it is
> redundant with DisplayState->pf.
>
> I think we also better allow only certain sizes.  Minimum requirement
> should be that the width is a multiple of 8.  Handling bitmasks just
> become too ugly without that.  I'd tend to go further even and allow
> only 32x32 and 64x64 mouse pointers.  Maybe 48x48 too.
>    

I'm not sure it's necessary to be so restrictive.  If you assume ARGB, 
then masking isn't an issue.

Regards,

Anthony Liguori

> Comments?
>
>
> cheers,
>    Gerd
>
> ---
>   vnc.c |   35 +++++++++++++++++++++++++++++++++++
>   vnc.h |    2 +
>   2 files changed, 37 insertions(+), 0 deletions(-)
>
> diff --git a/vnc.c b/vnc.c
> index 9ba603c..5d9b9cb 100644
> --- a/vnc.c
> +++ b/vnc.c
> @@ -925,6 +925,36 @@ static void vnc_dpy_copy(DisplayState *ds, int src_x, int src_y, int dst_x, int
>       }
>   }
>
> +static void vnc_mouse_set(int x, int y, int visible)
> +{
> +    /* can we do that ??? */
> +}
> +
> +static void vnc_cursor_define(int width, int height, int bpp,
> +                              int hot_x, int hot_y,
> +                              uint8_t *image, uint8_t *mask)
> +{
> +    VncDisplay *vd = vnc_display;
> +    int pixels, isize, msize;
> +    VncState *vs;
> +
> +    pixels = width * height;
> +    isize = pixels * bpp / 8;
> +    msize = pixels / 8;
> +
> +    QTAILQ_FOREACH(vs,&vd->clients, next) {
> +        if (!vnc_has_feature(vs, VNC_FEATURE_RICH_CURSOR))
> +            continue;
> +        vnc_write_u8(vs,  VNC_MSG_SERVER_FRAMEBUFFER_UPDATE);
> +        vnc_write_u8(vs,  0);  /*  padding     */
> +        vnc_write_u16(vs, 1);  /*  # of rects  */
> +        vnc_framebuffer_update(vs, hot_x, hot_y, width, height,
> +                               VNC_ENCODING_RICH_CURSOR);
> +        vnc_write(vs, image, isize);
> +        vnc_write(vs, mask, msize);
> +    }
> +}
> +
>   static int find_and_clear_dirty_height(struct VncState *vs,
>                                          int y, int last_x, int x)
>   {
> @@ -1800,6 +1830,9 @@ static void set_encodings(VncState *vs, int32_t *encodings, size_t n_encodings)
>           case VNC_ENCODING_POINTER_TYPE_CHANGE:
>               vs->features |= VNC_FEATURE_POINTER_TYPE_CHANGE_MASK;
>               break;
> +        case VNC_ENCODING_RICH_CURSOR:
> +            vs->features |= VNC_FEATURE_RICH_CURSOR_MASK;
> +            break;
>           case VNC_ENCODING_EXT_KEY_EVENT:
>               send_ext_key_event_ack(vs);
>               break;
> @@ -2487,6 +2520,8 @@ void vnc_display_init(DisplayState *ds)
>       dcl->dpy_resize = vnc_dpy_resize;
>       dcl->dpy_setdata = vnc_dpy_setdata;
>       register_displaychangelistener(ds, dcl);
> +    ds->mouse_set = vnc_mouse_set;
> +    ds->cursor_define = vnc_cursor_define;
>   }
>
>
> diff --git a/vnc.h b/vnc.h
> index b593608..ede9040 100644
> --- a/vnc.h
> +++ b/vnc.h
> @@ -266,6 +266,7 @@ enum {
>   #define VNC_FEATURE_TIGHT                    4
>   #define VNC_FEATURE_ZLIB                     5
>   #define VNC_FEATURE_COPYRECT                 6
> +#define VNC_FEATURE_RICH_CURSOR              7
>
>   #define VNC_FEATURE_RESIZE_MASK              (1<<  VNC_FEATURE_RESIZE)
>   #define VNC_FEATURE_HEXTILE_MASK             (1<<  VNC_FEATURE_HEXTILE)
> @@ -274,6 +275,7 @@ enum {
>   #define VNC_FEATURE_TIGHT_MASK               (1<<  VNC_FEATURE_TIGHT)
>   #define VNC_FEATURE_ZLIB_MASK                (1<<  VNC_FEATURE_ZLIB)
>   #define VNC_FEATURE_COPYRECT_MASK            (1<<  VNC_FEATURE_COPYRECT)
> +#define VNC_FEATURE_RICH_CURSOR_MASK         (1<<  VNC_FEATURE_RICH_CURSOR)
>
>
>   /* Client ->  Server message IDs */
>