diff mbox

[RfC] vnc: rich cursor support.

Message ID 1272887953-28305-1-git-send-email-kraxel@redhat.com
State New
Headers show

Commit Message

Gerd Hoffmann May 3, 2010, 11:59 a.m. UTC 
Hi,

Simple patch.  Difficuilt matter.  Not really sure where to go from
here ...

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.


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

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

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).

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.

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.

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.

Comments?


cheers,
  Gerd

---
 vnc.c |   35 +++++++++++++++++++++++++++++++++++
 vnc.h |    2 ++
 2 files changed, 37 insertions(+), 0 deletions(-)

Comments

Anthony Liguori May 3, 2010, 12:20 p.m. UTC | #1 
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 */
>
Gerd Hoffmann May 3, 2010, 12:46 p.m. UTC | #2 
On 05/03/10 14:20, Anthony Liguori wrote:
> 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.

Yea, /me isn't surprised after digging there.

>> 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.

qemu doesn't need to care how it is actually rendered, that job is 
offloaded anyway ;)

I'm more concerned about network bandwith and host cpu usage.  Using 
ARGB everythere will result in 1bit -> ARGB -> 1bit conversion in 
several cases.  Changing the mouse pointer doesn't happen *that* 
frequently though, so I think there is no point in being worried too 
much.  Except maybe for animated pointers ...

Long-term ARGB cursors will be the only thing used, and limiting cursors 
to just that single format will certainly simplify all the cursor handling.

>> 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.

Didn't try X11, but windows xp vmware vga driver *does* send 1bit cursors.

> 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.

Easy.

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

This isn't nice indeed.

>> 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.

Good.

>> 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.

Or use existing ones such as vnc_convert_pixels().
Makes sense.

>> 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.

For the backends it might be in case they have to convert the alpha 
channel to a mask.

cheers,
   Gerd
diff mbox

Patch

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 */