From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1O8udW-0007Yj-L7 for qemu-devel@nongnu.org; Mon, 03 May 2010 08:20:34 -0400 Received: from [140.186.70.92] (port=40421 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1O8udU-0007Xb-24 for qemu-devel@nongnu.org; Mon, 03 May 2010 08:20:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1O8udQ-0006SU-Cw for qemu-devel@nongnu.org; Mon, 03 May 2010 08:20:31 -0400 Received: from mail-vw0-f45.google.com ([209.85.212.45]:54895) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1O8udQ-0006S1-4J for qemu-devel@nongnu.org; Mon, 03 May 2010 08:20:28 -0400 Received: by vws3 with SMTP id 3so1522402vws.4 for ; Mon, 03 May 2010 05:20:27 -0700 (PDT) Message-ID: <4BDEBF89.9020702@codemonkey.ws> Date: Mon, 03 May 2010 07:20:25 -0500 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] [RfC PATCH] vnc: rich cursor support. References: <1272887953-28305-1-git-send-email-kraxel@redhat.com> In-Reply-To: <1272887953-28305-1-git-send-email-kraxel@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Gerd Hoffmann Cc: qemu-devel@nongnu.org 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 */ >