[PATCH v3 0/3] vnc: support some new extensions.

[PATCH v3 0/3] vnc: support some new extensions.

[Gerd Hoffmann]

The rfb standard keeps envolving.  While the official spec is kind of
frozen since a decade or so the community maintains an updated version
of the spec at:
	https://github.com/rfbproto/rfbproto/

This series adds support for two new extensions from that spec: alpha
cursor and extended desktop resize.

alpha cursor allows a full alpha channel for the cursor image (unlike
the rich cursor extension which has only a bitmask for transparency).

extended desktop resize makes the desktop-resize work both ways, i.e. we
can not only signal guest display resolution changes to the vnc client
but also vnc client window size changes to the guest.

Tested with tigervnc.

Support for gtk-vnc is in progress.

v3:
 - dropped cursor patches (already merged).
 - fix client initialization (reply to update request not set encoding).

v2:
 - dropped qxl bits (will be a separate patch series).
 - use error codes for desktop resize responses.
 - little tweaks here and there.
 - pick up some review tags.

Gerd Hoffmann (3):
  vnc: move check into vnc_cursor_define
  vnc: move initialization to framebuffer_update_request
  vnc: add support for extended desktop resize

 ui/vnc.h |  2 ++
 ui/vnc.c | 85 +++++++++++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 74 insertions(+), 13 deletions(-)

-- 
2.29.2

[PATCH v3 1/3] vnc: move check into vnc_cursor_define

[Gerd Hoffmann]

Move the check whenever a cursor exists into the vnc_cursor_define()
function so callers don't have to do it.

Suggested-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 ui/vnc.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index 7452ac7df2ce..84c4972b895b 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -792,9 +792,7 @@ static void vnc_dpy_switch(DisplayChangeListener *dcl,
     QTAILQ_FOREACH(vs, &vd->clients, next) {
         vnc_colordepth(vs);
         vnc_desktop_resize(vs);
-        if (vs->vd->cursor) {
-            vnc_cursor_define(vs);
-        }
+        vnc_cursor_define(vs);
         memset(vs->dirty, 0x00, sizeof(vs->dirty));
         vnc_set_area_dirty(vs->dirty, vd, 0, 0,
                            vnc_width(vd),
@@ -928,6 +926,10 @@ static int vnc_cursor_define(VncState *vs)
     QEMUCursor *c = vs->vd->cursor;
     int isize;
 
+    if (!vs->vd->cursor) {
+        return -1;
+    }
+
     if (vnc_has_feature(vs, VNC_FEATURE_ALPHA_CURSOR)) {
         vnc_lock_output(vs);
         vnc_write_u8(vs,  VNC_MSG_SERVER_FRAMEBUFFER_UPDATE);
@@ -2137,9 +2139,7 @@ static void set_encodings(VncState *vs, int32_t *encodings, size_t n_encodings)
     vnc_desktop_resize(vs);
     check_pointer_type_change(&vs->mouse_mode_notifier, NULL);
     vnc_led_state_change(vs);
-    if (vs->vd->cursor) {
-        vnc_cursor_define(vs);
-    }
+    vnc_cursor_define(vs);
 }
 
 static void set_pixel_conversion(VncState *vs)
-- 
2.29.2

Re: [PATCH v3 1/3] vnc: move check into vnc_cursor_define

[Marc-André Lureau]

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

On Tue, Jan 12, 2021 at 5:41 PM Gerd Hoffmann <kraxel@redhat.com> wrote:

> Move the check whenever a cursor exists into the vnc_cursor_define()
> function so callers don't have to do it.
>
> Suggested-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>

thanks
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

---
>  ui/vnc.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 7452ac7df2ce..84c4972b895b 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -792,9 +792,7 @@ static void vnc_dpy_switch(DisplayChangeListener *dcl,
>      QTAILQ_FOREACH(vs, &vd->clients, next) {
>          vnc_colordepth(vs);
>          vnc_desktop_resize(vs);
> -        if (vs->vd->cursor) {
> -            vnc_cursor_define(vs);
> -        }
> +        vnc_cursor_define(vs);
>          memset(vs->dirty, 0x00, sizeof(vs->dirty));
>          vnc_set_area_dirty(vs->dirty, vd, 0, 0,
>                             vnc_width(vd),
> @@ -928,6 +926,10 @@ static int vnc_cursor_define(VncState *vs)
>      QEMUCursor *c = vs->vd->cursor;
>      int isize;
>
> +    if (!vs->vd->cursor) {
> +        return -1;
> +    }
> +
>      if (vnc_has_feature(vs, VNC_FEATURE_ALPHA_CURSOR)) {
>          vnc_lock_output(vs);
>          vnc_write_u8(vs,  VNC_MSG_SERVER_FRAMEBUFFER_UPDATE);
> @@ -2137,9 +2139,7 @@ static void set_encodings(VncState *vs, int32_t
> *encodings, size_t n_encodings)
>      vnc_desktop_resize(vs);
>      check_pointer_type_change(&vs->mouse_mode_notifier, NULL);
>      vnc_led_state_change(vs);
> -    if (vs->vd->cursor) {
> -        vnc_cursor_define(vs);
> -    }
> +    vnc_cursor_define(vs);
>  }
>
>  static void set_pixel_conversion(VncState *vs)
> --
> 2.29.2
>
>

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

Re: [PATCH v3 1/3] vnc: move check into vnc_cursor_define

[Daniel P. Berrangé]

On Tue, Jan 12, 2021 at 02:41:18PM +0100, Gerd Hoffmann wrote:
> Move the check whenever a cursor exists into the vnc_cursor_define()
> function so callers don't have to do it.
> 
> Suggested-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  ui/vnc.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

> 
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 7452ac7df2ce..84c4972b895b 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -792,9 +792,7 @@ static void vnc_dpy_switch(DisplayChangeListener *dcl,
>      QTAILQ_FOREACH(vs, &vd->clients, next) {
>          vnc_colordepth(vs);
>          vnc_desktop_resize(vs);
> -        if (vs->vd->cursor) {
> -            vnc_cursor_define(vs);
> -        }
> +        vnc_cursor_define(vs);
>          memset(vs->dirty, 0x00, sizeof(vs->dirty));
>          vnc_set_area_dirty(vs->dirty, vd, 0, 0,
>                             vnc_width(vd),
> @@ -928,6 +926,10 @@ static int vnc_cursor_define(VncState *vs)
>      QEMUCursor *c = vs->vd->cursor;
>      int isize;
>  
> +    if (!vs->vd->cursor) {
> +        return -1;
> +    }

None of the callers check the return value, so we could just
make this function void, as a future cleanup patch.

> +
>      if (vnc_has_feature(vs, VNC_FEATURE_ALPHA_CURSOR)) {
>          vnc_lock_output(vs);
>          vnc_write_u8(vs,  VNC_MSG_SERVER_FRAMEBUFFER_UPDATE);
> @@ -2137,9 +2139,7 @@ static void set_encodings(VncState *vs, int32_t *encodings, size_t n_encodings)
>      vnc_desktop_resize(vs);
>      check_pointer_type_change(&vs->mouse_mode_notifier, NULL);
>      vnc_led_state_change(vs);
> -    if (vs->vd->cursor) {
> -        vnc_cursor_define(vs);
> -    }
> +    vnc_cursor_define(vs);
>  }
>  
>  static void set_pixel_conversion(VncState *vs)
> -- 
> 2.29.2
> 
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

[PATCH v3 2/3] vnc: move initialization to framebuffer_update_request

[Gerd Hoffmann]

qemu sends various state info like current cursor shape to newly connected
clients in response to a set_encoding message.  This is not correct according
to the rfb spec.  Send that information in response to a full (incremental=0)
framebuffer update request instead.  Also send the resize information
unconditionally, not only in case of an actual server-side change.

This makes the qemu vnc server conform to the spec and allows clients to
request the complete vnc server state without reconnect.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 ui/vnc.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index 84c4972b895b..8df63b349b38 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -660,10 +660,6 @@ static void vnc_desktop_resize(VncState *vs)
     if (vs->ioc == NULL || !vnc_has_feature(vs, VNC_FEATURE_RESIZE)) {
         return;
     }
-    if (vs->client_width == pixman_image_get_width(vs->vd->server) &&
-        vs->client_height == pixman_image_get_height(vs->vd->server)) {
-        return;
-    }
 
     assert(pixman_image_get_width(vs->vd->server) < 65536 &&
            pixman_image_get_width(vs->vd->server) >= 0);
@@ -2013,6 +2009,10 @@ static void framebuffer_update_request(VncState *vs, int incremental,
     } else {
         vs->update = VNC_STATE_UPDATE_FORCE;
         vnc_set_area_dirty(vs->dirty, vs->vd, x, y, w, h);
+        vnc_colordepth(vs);
+        vnc_desktop_resize(vs);
+        vnc_led_state_change(vs);
+        vnc_cursor_define(vs);
     }
 }
 
@@ -2136,10 +2136,7 @@ static void set_encodings(VncState *vs, int32_t *encodings, size_t n_encodings)
             break;
         }
     }
-    vnc_desktop_resize(vs);
     check_pointer_type_change(&vs->mouse_mode_notifier, NULL);
-    vnc_led_state_change(vs);
-    vnc_cursor_define(vs);
 }
 
 static void set_pixel_conversion(VncState *vs)
-- 
2.29.2

Re: [PATCH v3 2/3] vnc: move initialization to framebuffer_update_request

[Daniel P. Berrangé]

On Tue, Jan 12, 2021 at 02:41:19PM +0100, Gerd Hoffmann wrote:
> qemu sends various state info like current cursor shape to newly connected
> clients in response to a set_encoding message.  This is not correct according
> to the rfb spec.  Send that information in response to a full (incremental=0)
> framebuffer update request instead.  Also send the resize information
> unconditionally, not only in case of an actual server-side change.
> 
> This makes the qemu vnc server conform to the spec and allows clients to
> request the complete vnc server state without reconnect.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  ui/vnc.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 84c4972b895b..8df63b349b38 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -660,10 +660,6 @@ static void vnc_desktop_resize(VncState *vs)
>      if (vs->ioc == NULL || !vnc_has_feature(vs, VNC_FEATURE_RESIZE)) {
>          return;
>      }
> -    if (vs->client_width == pixman_image_get_width(vs->vd->server) &&
> -        vs->client_height == pixman_image_get_height(vs->vd->server)) {
> -        return;
> -    }
>  
>      assert(pixman_image_get_width(vs->vd->server) < 65536 &&
>             pixman_image_get_width(vs->vd->server) >= 0);
> @@ -2013,6 +2009,10 @@ static void framebuffer_update_request(VncState *vs, int incremental,
>      } else {
>          vs->update = VNC_STATE_UPDATE_FORCE;
>          vnc_set_area_dirty(vs->dirty, vs->vd, x, y, w, h);
> +        vnc_colordepth(vs);
> +        vnc_desktop_resize(vs);
> +        vnc_led_state_change(vs);
> +        vnc_cursor_define(vs);

If I'm nit-picking this still isn't spec compliant as it is sending
4 separate framebuffer update messages each with 1 rectangle, in
response to 1 single framebuffer update request.

The strictly spec compliant approach would be to put these on a queue
and send them as rectangles in the frame buffer update message that's
sending the framebuffer contents. IIUC, this is the approach tigervnc
uses.  This would require a major change in the way QEMU sends FB
update messages though.

In reality I doubt there are VNC clients that really care about this
distinction, and this change is still more spec compliant
that the current impl.

>      }
>  }
>  
> @@ -2136,10 +2136,7 @@ static void set_encodings(VncState *vs, int32_t *encodings, size_t n_encodings)
>              break;
>          }
>      }
> -    vnc_desktop_resize(vs);
>      check_pointer_type_change(&vs->mouse_mode_notifier, NULL);
> -    vnc_led_state_change(vs);
> -    vnc_cursor_define(vs);
>  }
>  
>  static void set_pixel_conversion(VncState *vs)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

[PATCH v3 3/3] vnc: add support for extended desktop resize

[Gerd Hoffmann]

The extended desktop resize encoding adds support for (a) clients
sending resize requests to the server, and (b) multihead support.

This patch implements (a).  All resize requests are rejected by qemu.
Qemu can't resize the framebuffer on its own, this is in the hands of
the guest, so all qemu can do is forward the request to the guest.
Should the guest actually resize the framebuffer we can notify the vnc
client later with a separate message.

This requires support in the display device.  Works with virtio-gpu.

https://github.com/rfbproto/rfbproto/blob/master/rfbproto.rst#extendeddesktopsize-pseudo-encoding

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 ui/vnc.h |  2 ++
 ui/vnc.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 65 insertions(+), 1 deletion(-)

diff --git a/ui/vnc.h b/ui/vnc.h
index c8d3ad9ec496..77a310947bd6 100644
--- a/ui/vnc.h
+++ b/ui/vnc.h
@@ -442,6 +442,7 @@ enum {
 
 enum VncFeatures {
     VNC_FEATURE_RESIZE,
+    VNC_FEATURE_RESIZE_EXT,
     VNC_FEATURE_HEXTILE,
     VNC_FEATURE_POINTER_TYPE_CHANGE,
     VNC_FEATURE_WMVI,
@@ -456,6 +457,7 @@ enum VncFeatures {
 };
 
 #define VNC_FEATURE_RESIZE_MASK              (1 << VNC_FEATURE_RESIZE)
+#define VNC_FEATURE_RESIZE_EXT_MASK          (1 << VNC_FEATURE_RESIZE_EXT)
 #define VNC_FEATURE_HEXTILE_MASK             (1 << VNC_FEATURE_HEXTILE)
 #define VNC_FEATURE_POINTER_TYPE_CHANGE_MASK (1 << VNC_FEATURE_POINTER_TYPE_CHANGE)
 #define VNC_FEATURE_WMVI_MASK                (1 << VNC_FEATURE_WMVI)
diff --git a/ui/vnc.c b/ui/vnc.c
index 8df63b349b38..d4a161ac1393 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -654,10 +654,35 @@ void vnc_framebuffer_update(VncState *vs, int x, int y, int w, int h,
     vnc_write_s32(vs, encoding);
 }
 
+static void vnc_desktop_resize_ext(VncState *vs, int reject_reason)
+{
+    vnc_lock_output(vs);
+    vnc_write_u8(vs, VNC_MSG_SERVER_FRAMEBUFFER_UPDATE);
+    vnc_write_u8(vs, 0);
+    vnc_write_u16(vs, 1); /* number of rects */
+    vnc_framebuffer_update(vs,
+                           reject_reason ? 1 : 0,
+                           reject_reason,
+                           vs->client_width, vs->client_height,
+                           VNC_ENCODING_DESKTOP_RESIZE_EXT);
+    vnc_write_u8(vs, 1);  /* number of screens */
+    vnc_write_u8(vs, 0);  /* padding */
+    vnc_write_u8(vs, 0);  /* padding */
+    vnc_write_u8(vs, 0);  /* padding */
+    vnc_write_u32(vs, 0); /* screen id */
+    vnc_write_u16(vs, 0); /* screen x-pos */
+    vnc_write_u16(vs, 0); /* screen y-pos */
+    vnc_write_u16(vs, vs->client_width);
+    vnc_write_u16(vs, vs->client_height);
+    vnc_write_u32(vs, 0); /* screen flags */
+    vnc_unlock_output(vs);
+    vnc_flush(vs);
+}
 
 static void vnc_desktop_resize(VncState *vs)
 {
-    if (vs->ioc == NULL || !vnc_has_feature(vs, VNC_FEATURE_RESIZE)) {
+    if (vs->ioc == NULL || (!vnc_has_feature(vs, VNC_FEATURE_RESIZE) &&
+                            !vnc_has_feature(vs, VNC_FEATURE_RESIZE_EXT))) {
         return;
     }
 
@@ -667,6 +692,12 @@ static void vnc_desktop_resize(VncState *vs)
            pixman_image_get_height(vs->vd->server) >= 0);
     vs->client_width = pixman_image_get_width(vs->vd->server);
     vs->client_height = pixman_image_get_height(vs->vd->server);
+
+    if (vnc_has_feature(vs, VNC_FEATURE_RESIZE_EXT)) {
+        vnc_desktop_resize_ext(vs, 0);
+        return;
+    }
+
     vnc_lock_output(vs);
     vnc_write_u8(vs, VNC_MSG_SERVER_FRAMEBUFFER_UPDATE);
     vnc_write_u8(vs, 0);
@@ -2102,6 +2133,9 @@ static void set_encodings(VncState *vs, int32_t *encodings, size_t n_encodings)
         case VNC_ENCODING_DESKTOPRESIZE:
             vs->features |= VNC_FEATURE_RESIZE_MASK;
             break;
+        case VNC_ENCODING_DESKTOP_RESIZE_EXT:
+            vs->features |= VNC_FEATURE_RESIZE_EXT_MASK;
+            break;
         case VNC_ENCODING_POINTER_TYPE_CHANGE:
             vs->features |= VNC_FEATURE_POINTER_TYPE_CHANGE_MASK;
             break;
@@ -2420,6 +2454,34 @@ static int protocol_client_msg(VncState *vs, uint8_t *data, size_t len)
             break;
         }
         break;
+    case VNC_MSG_CLIENT_SET_DESKTOP_SIZE:
+    {
+        size_t size;
+        uint8_t screens;
+
+        if (len < 8) {
+            return 8;
+        }
+
+        screens = read_u8(data, 6);
+        size    = 8 + screens * 16;
+        if (len < size) {
+            return size;
+        }
+
+        if (dpy_ui_info_supported(vs->vd->dcl.con)) {
+            QemuUIInfo info;
+            memset(&info, 0, sizeof(info));
+            info.width = read_u16(data, 2);
+            info.height = read_u16(data, 4);
+            dpy_set_ui_info(vs->vd->dcl.con, &info);
+            vnc_desktop_resize_ext(vs, 4 /* Request forwarded */);
+        } else {
+            vnc_desktop_resize_ext(vs, 3 /* Invalid screen layout */);
+        }
+
+        break;
+    }
     default:
         VNC_DEBUG("Msg: %d\n", data[0]);
         vnc_client_error(vs);
-- 
2.29.2

Re: [PATCH v3 3/3] vnc: add support for extended desktop resize

[Daniel P. Berrangé]

On Tue, Jan 12, 2021 at 02:41:20PM +0100, Gerd Hoffmann wrote:
> The extended desktop resize encoding adds support for (a) clients
> sending resize requests to the server, and (b) multihead support.
> 
> This patch implements (a).  All resize requests are rejected by qemu.
> Qemu can't resize the framebuffer on its own, this is in the hands of
> the guest, so all qemu can do is forward the request to the guest.
> Should the guest actually resize the framebuffer we can notify the vnc
> client later with a separate message.
> 
> This requires support in the display device.  Works with virtio-gpu.
> 
> https://github.com/rfbproto/rfbproto/blob/master/rfbproto.rst#extendeddesktopsize-pseudo-encoding
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  ui/vnc.h |  2 ++
>  ui/vnc.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 65 insertions(+), 1 deletion(-)


Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v3 0/3] vnc: support some new extensions.

[Daniel P. Berrangé]

On Tue, Jan 12, 2021 at 02:41:17PM +0100, Gerd Hoffmann wrote:
> The rfb standard keeps envolving.  While the official spec is kind of
> frozen since a decade or so the community maintains an updated version
> of the spec at:
> 	https://github.com/rfbproto/rfbproto/
> 
> This series adds support for two new extensions from that spec: alpha
> cursor and extended desktop resize.
> 
> alpha cursor allows a full alpha channel for the cursor image (unlike
> the rich cursor extension which has only a bitmask for transparency).
> 
> extended desktop resize makes the desktop-resize work both ways, i.e. we
> can not only signal guest display resolution changes to the vnc client
> but also vnc client window size changes to the guest.
> 
> Tested with tigervnc.
> 
> Support for gtk-vnc is in progress.

FWIW, basic support (ie no multi-head) is implemented and testable
via the examples/gvncviewer program in gtk-vnc.

virt-viewer/remote-viewer are still todo.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|