[PATCH v2 0/9] vnc: support some new extensions.

[PATCH v2 0/9] 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.

gtk-vnc (and anything building on top of it like virt-viewer and
virt-manager) has no support for these extensions.

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 (9):
  console: drop qemu_console_get_ui_info
  console: allow con==NULL in dpy_{get,set}_ui_info and
    dpy_ui_info_supported
  vnc: use enum for features
  vnc: drop unused copyrect feature
  vnc: add pseudo encodings
  vnc: add alpha cursor support
  vnc: force initial resize message
  vnc: add support for extended desktop resize
  vnc: move check into vnc_cursor_define

 include/ui/console.h |   1 -
 ui/vnc.h             |  32 ++++++++------
 ui/console.c         |  18 ++++----
 ui/vnc.c             | 103 +++++++++++++++++++++++++++++++++++++------
 4 files changed, 118 insertions(+), 36 deletions(-)

-- 
2.27.0

[PATCH v2 1/9] console: drop qemu_console_get_ui_info

[Gerd Hoffmann]

Unused and duplicate (there is dpy_get_ui_info).

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/ui/console.h | 1 -
 ui/console.c         | 6 ------
 2 files changed, 7 deletions(-)

diff --git a/include/ui/console.h b/include/ui/console.h
index e7303d8b98a8..5dd21976a376 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -390,7 +390,6 @@ bool qemu_console_is_gl_blocked(QemuConsole *con);
 char *qemu_console_get_label(QemuConsole *con);
 int qemu_console_get_index(QemuConsole *con);
 uint32_t qemu_console_get_head(QemuConsole *con);
-QemuUIInfo *qemu_console_get_ui_info(QemuConsole *con);
 int qemu_console_get_width(QemuConsole *con, int fallback);
 int qemu_console_get_height(QemuConsole *con, int fallback);
 /* Return the low-level window id for the console */
diff --git a/ui/console.c b/ui/console.c
index 53dee8e26b17..f995639e45f6 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -2122,12 +2122,6 @@ uint32_t qemu_console_get_head(QemuConsole *con)
     return con ? con->head : -1;
 }
 
-QemuUIInfo *qemu_console_get_ui_info(QemuConsole *con)
-{
-    assert(con != NULL);
-    return &con->ui_info;
-}
-
 int qemu_console_get_width(QemuConsole *con, int fallback)
 {
     if (con == NULL) {
-- 
2.27.0

Re: [PATCH v2 1/9] console: drop qemu_console_get_ui_info

[Marc-André Lureau]

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

On Tue, Dec 8, 2020 at 4:02 PM Gerd Hoffmann <kraxel@redhat.com> wrote:

> Unused and duplicate (there is dpy_get_ui_info).
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>

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

---
>  include/ui/console.h | 1 -
>  ui/console.c         | 6 ------
>  2 files changed, 7 deletions(-)
>
> diff --git a/include/ui/console.h b/include/ui/console.h
> index e7303d8b98a8..5dd21976a376 100644
> --- a/include/ui/console.h
> +++ b/include/ui/console.h
> @@ -390,7 +390,6 @@ bool qemu_console_is_gl_blocked(QemuConsole *con);
>  char *qemu_console_get_label(QemuConsole *con);
>  int qemu_console_get_index(QemuConsole *con);
>  uint32_t qemu_console_get_head(QemuConsole *con);
> -QemuUIInfo *qemu_console_get_ui_info(QemuConsole *con);
>  int qemu_console_get_width(QemuConsole *con, int fallback);
>  int qemu_console_get_height(QemuConsole *con, int fallback);
>  /* Return the low-level window id for the console */
> diff --git a/ui/console.c b/ui/console.c
> index 53dee8e26b17..f995639e45f6 100644
> --- a/ui/console.c
> +++ b/ui/console.c
> @@ -2122,12 +2122,6 @@ uint32_t qemu_console_get_head(QemuConsole *con)
>      return con ? con->head : -1;
>  }
>
> -QemuUIInfo *qemu_console_get_ui_info(QemuConsole *con)
> -{
> -    assert(con != NULL);
> -    return &con->ui_info;
> -}
> -
>  int qemu_console_get_width(QemuConsole *con, int fallback)
>  {
>      if (con == NULL) {
> --
> 2.27.0
>
>
>

-- 
Marc-André Lureau

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

Re: [PATCH v2 1/9] console: drop qemu_console_get_ui_info

[Daniel P. Berrangé]

On Tue, Dec 08, 2020 at 12:57:29PM +0100, Gerd Hoffmann wrote:
> Unused and duplicate (there is dpy_get_ui_info).
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/ui/console.h | 1 -
>  ui/console.c         | 6 ------
>  2 files changed, 7 deletions(-)

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 v2 2/9] console: allow con==NULL in dpy_{get, set}_ui_info and dpy_ui_info_supported

[Gerd Hoffmann]

Use active_console in that case like we do in many other places.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 ui/console.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/ui/console.c b/ui/console.c
index f995639e45f6..30e70be555db 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1544,19 +1544,27 @@ static void dpy_set_ui_info_timer(void *opaque)
 
 bool dpy_ui_info_supported(QemuConsole *con)
 {
+    if (con == NULL) {
+        con = active_console;
+    }
+
     return con->hw_ops->ui_info != NULL;
 }
 
 const QemuUIInfo *dpy_get_ui_info(const QemuConsole *con)
 {
-    assert(con != NULL);
+    if (con == NULL) {
+        con = active_console;
+    }
 
     return &con->ui_info;
 }
 
 int dpy_set_ui_info(QemuConsole *con, QemuUIInfo *info)
 {
-    assert(con != NULL);
+    if (con == NULL) {
+        con = active_console;
+    }
 
     if (!dpy_ui_info_supported(con)) {
         return -1;
-- 
2.27.0

Re: [PATCH v2 2/9] console: allow con==NULL in dpy_{get, set}_ui_info and dpy_ui_info_supported

[Marc-André Lureau]

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

On Tue, Dec 8, 2020 at 3:59 PM Gerd Hoffmann <kraxel@redhat.com> wrote:

> Use active_console in that case like we do in many other places.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>

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

---
>  ui/console.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/ui/console.c b/ui/console.c
> index f995639e45f6..30e70be555db 100644
> --- a/ui/console.c
> +++ b/ui/console.c
> @@ -1544,19 +1544,27 @@ static void dpy_set_ui_info_timer(void *opaque)
>
>  bool dpy_ui_info_supported(QemuConsole *con)
>  {
> +    if (con == NULL) {
> +        con = active_console;
> +    }
> +
>      return con->hw_ops->ui_info != NULL;
>  }
>
>  const QemuUIInfo *dpy_get_ui_info(const QemuConsole *con)
>  {
> -    assert(con != NULL);
> +    if (con == NULL) {
> +        con = active_console;
> +    }
>
>      return &con->ui_info;
>  }
>
>  int dpy_set_ui_info(QemuConsole *con, QemuUIInfo *info)
>  {
> -    assert(con != NULL);
> +    if (con == NULL) {
> +        con = active_console;
> +    }
>
>      if (!dpy_ui_info_supported(con)) {
>          return -1;
> --
> 2.27.0
>
>
>

-- 
Marc-André Lureau

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

Re: [PATCH v2 2/9] console: allow con==NULL in dpy_{get, set}_ui_info and dpy_ui_info_supported

[Daniel P. Berrangé]

On Tue, Dec 08, 2020 at 12:57:30PM +0100, Gerd Hoffmann wrote:
> Use active_console in that case like we do in many other places.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  ui/console.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)

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 v2 3/9] vnc: use enum for features

[Gerd Hoffmann]

Use an enum for the vnc feature bits.  That way they are enumerated
automatically and we don't have to do that manually when adding or
removing features.

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

diff --git a/ui/vnc.h b/ui/vnc.h
index 4e2637ce6c5c..262fcf179b44 100644
--- a/ui/vnc.h
+++ b/ui/vnc.h
@@ -438,18 +438,20 @@ enum {
  *
  *****************************************************************************/
 
-#define VNC_FEATURE_RESIZE                   0
-#define VNC_FEATURE_HEXTILE                  1
-#define VNC_FEATURE_POINTER_TYPE_CHANGE      2
-#define VNC_FEATURE_WMVI                     3
-#define VNC_FEATURE_TIGHT                    4
-#define VNC_FEATURE_ZLIB                     5
-#define VNC_FEATURE_COPYRECT                 6
-#define VNC_FEATURE_RICH_CURSOR              7
-#define VNC_FEATURE_TIGHT_PNG                8
-#define VNC_FEATURE_ZRLE                     9
-#define VNC_FEATURE_ZYWRLE                  10
-#define VNC_FEATURE_LED_STATE               11
+enum VncFeatures {
+    VNC_FEATURE_RESIZE,
+    VNC_FEATURE_HEXTILE,
+    VNC_FEATURE_POINTER_TYPE_CHANGE,
+    VNC_FEATURE_WMVI,
+    VNC_FEATURE_TIGHT,
+    VNC_FEATURE_ZLIB,
+    VNC_FEATURE_COPYRECT,
+    VNC_FEATURE_RICH_CURSOR,
+    VNC_FEATURE_TIGHT_PNG,
+    VNC_FEATURE_ZRLE,
+    VNC_FEATURE_ZYWRLE,
+    VNC_FEATURE_LED_STATE,
+};
 
 #define VNC_FEATURE_RESIZE_MASK              (1 << VNC_FEATURE_RESIZE)
 #define VNC_FEATURE_HEXTILE_MASK             (1 << VNC_FEATURE_HEXTILE)
-- 
2.27.0

[PATCH v2 4/9] vnc: drop unused copyrect feature

[Gerd Hoffmann]

vnc stopped using the copyrect pseudo encoding in 2017, in commit
50628d3479e4 ("cirrus/vnc: zap bitblit support from console code.")
So we can drop the now unused copyrect feature bit.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 ui/vnc.h | 2 --
 ui/vnc.c | 3 ---
 2 files changed, 5 deletions(-)

diff --git a/ui/vnc.h b/ui/vnc.h
index 262fcf179b44..a7fd38a82075 100644
--- a/ui/vnc.h
+++ b/ui/vnc.h
@@ -445,7 +445,6 @@ enum VncFeatures {
     VNC_FEATURE_WMVI,
     VNC_FEATURE_TIGHT,
     VNC_FEATURE_ZLIB,
-    VNC_FEATURE_COPYRECT,
     VNC_FEATURE_RICH_CURSOR,
     VNC_FEATURE_TIGHT_PNG,
     VNC_FEATURE_ZRLE,
@@ -459,7 +458,6 @@ enum VncFeatures {
 #define VNC_FEATURE_WMVI_MASK                (1 << VNC_FEATURE_WMVI)
 #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)
 #define VNC_FEATURE_TIGHT_PNG_MASK           (1 << VNC_FEATURE_TIGHT_PNG)
 #define VNC_FEATURE_ZRLE_MASK                (1 << VNC_FEATURE_ZRLE)
diff --git a/ui/vnc.c b/ui/vnc.c
index 49235056f7a8..8c2771c1ce3b 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -2061,9 +2061,6 @@ static void set_encodings(VncState *vs, int32_t *encodings, size_t n_encodings)
         case VNC_ENCODING_RAW:
             vs->vnc_encoding = enc;
             break;
-        case VNC_ENCODING_COPYRECT:
-            vs->features |= VNC_FEATURE_COPYRECT_MASK;
-            break;
         case VNC_ENCODING_HEXTILE:
             vs->features |= VNC_FEATURE_HEXTILE_MASK;
             vs->vnc_encoding = enc;
-- 
2.27.0

Re: [PATCH v2 4/9] vnc: drop unused copyrect feature

[Daniel P. Berrangé]

On Tue, Dec 08, 2020 at 12:57:32PM +0100, Gerd Hoffmann wrote:
> vnc stopped using the copyrect pseudo encoding in 2017, in commit
> 50628d3479e4 ("cirrus/vnc: zap bitblit support from console code.")
> So we can drop the now unused copyrect feature bit.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  ui/vnc.h | 2 --
>  ui/vnc.c | 3 ---
>  2 files changed, 5 deletions(-)

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 v2 5/9] vnc: add pseudo encodings

[Gerd Hoffmann]

Add #defines for two new pseudo encodings:
 * cursor with alpha channel.
 * extended desktop resize.

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

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

diff --git a/ui/vnc.h b/ui/vnc.h
index a7fd38a82075..6f5006da3593 100644
--- a/ui/vnc.h
+++ b/ui/vnc.h
@@ -411,6 +411,8 @@ enum {
 #define VNC_ENCODING_AUDIO                0XFFFFFEFD /* -259 */
 #define VNC_ENCODING_TIGHT_PNG            0xFFFFFEFC /* -260 */
 #define VNC_ENCODING_LED_STATE            0XFFFFFEFB /* -261 */
+#define VNC_ENCODING_DESKTOP_RESIZE_EXT   0XFFFFFECC /* -308 */
+#define VNC_ENCODING_ALPHA_CURSOR         0XFFFFFEC6 /* -314 */
 #define VNC_ENCODING_WMVi                 0x574D5669
 
 /*****************************************************************************
-- 
2.27.0

Re: [PATCH v2 5/9] vnc: add pseudo encodings

[Daniel P. Berrangé]

On Tue, Dec 08, 2020 at 12:57:33PM +0100, Gerd Hoffmann wrote:
> Add #defines for two new pseudo encodings:
>  * cursor with alpha channel.
>  * extended desktop resize.
> 
> https://github.com/rfbproto/rfbproto/blob/master/rfbproto.rst#pseudo-encodings
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  ui/vnc.h | 2 ++
>  1 file changed, 2 insertions(+)

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 v2 6/9] vnc: add alpha cursor support

[Gerd Hoffmann]

There is a new vnc extension for cursors with an alpha channel.  Use
it if supported by the vnc client, prefer it over the "rich cursor"
extension which supports only a bitmask for transparency.

This is a visible improvement especially on modern desktops which
actually use the alpha channel when defining cursors.

https://github.com/rfbproto/rfbproto/blob/master/rfbproto.rst#cursor-with-alpha-pseudo-encoding

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 ui/vnc.h |  2 ++
 ui/vnc.c | 21 ++++++++++++++++++---
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/ui/vnc.h b/ui/vnc.h
index 6f5006da3593..c8d3ad9ec496 100644
--- a/ui/vnc.h
+++ b/ui/vnc.h
@@ -448,6 +448,7 @@ enum VncFeatures {
     VNC_FEATURE_TIGHT,
     VNC_FEATURE_ZLIB,
     VNC_FEATURE_RICH_CURSOR,
+    VNC_FEATURE_ALPHA_CURSOR,
     VNC_FEATURE_TIGHT_PNG,
     VNC_FEATURE_ZRLE,
     VNC_FEATURE_ZYWRLE,
@@ -461,6 +462,7 @@ enum VncFeatures {
 #define VNC_FEATURE_TIGHT_MASK               (1 << VNC_FEATURE_TIGHT)
 #define VNC_FEATURE_ZLIB_MASK                (1 << VNC_FEATURE_ZLIB)
 #define VNC_FEATURE_RICH_CURSOR_MASK         (1 << VNC_FEATURE_RICH_CURSOR)
+#define VNC_FEATURE_ALPHA_CURSOR_MASK        (1 << VNC_FEATURE_ALPHA_CURSOR)
 #define VNC_FEATURE_TIGHT_PNG_MASK           (1 << VNC_FEATURE_TIGHT_PNG)
 #define VNC_FEATURE_ZRLE_MASK                (1 << VNC_FEATURE_ZRLE)
 #define VNC_FEATURE_ZYWRLE_MASK              (1 << VNC_FEATURE_ZYWRLE)
diff --git a/ui/vnc.c b/ui/vnc.c
index 8c2771c1ce3b..247e80d8f5c8 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -937,6 +937,18 @@ static int vnc_cursor_define(VncState *vs)
     QEMUCursor *c = vs->vd->cursor;
     int isize;
 
+    if (vnc_has_feature(vs, VNC_FEATURE_ALPHA_CURSOR)) {
+        vnc_lock_output(vs);
+        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, c->hot_x, c->hot_y, c->width, c->height,
+                               VNC_ENCODING_ALPHA_CURSOR);
+        vnc_write_s32(vs, VNC_ENCODING_RAW);
+        vnc_write(vs, c->data, c->width * c->height * 4);
+        vnc_unlock_output(vs);
+        return 0;
+    }
     if (vnc_has_feature(vs, VNC_FEATURE_RICH_CURSOR)) {
         vnc_lock_output(vs);
         vnc_write_u8(vs,  VNC_MSG_SERVER_FRAMEBUFFER_UPDATE);
@@ -2102,9 +2114,9 @@ static void set_encodings(VncState *vs, int32_t *encodings, size_t n_encodings)
             break;
         case VNC_ENCODING_RICH_CURSOR:
             vs->features |= VNC_FEATURE_RICH_CURSOR_MASK;
-            if (vs->vd->cursor) {
-                vnc_cursor_define(vs);
-            }
+            break;
+        case VNC_ENCODING_ALPHA_CURSOR:
+            vs->features |= VNC_FEATURE_ALPHA_CURSOR_MASK;
             break;
         case VNC_ENCODING_EXT_KEY_EVENT:
             send_ext_key_event_ack(vs);
@@ -2134,6 +2146,9 @@ 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);
+    }
 }
 
 static void set_pixel_conversion(VncState *vs)
-- 
2.27.0

Re: [PATCH v2 6/9] vnc: add alpha cursor support

[Daniel P. Berrangé]

On Tue, Dec 08, 2020 at 12:57:34PM +0100, Gerd Hoffmann wrote:
> There is a new vnc extension for cursors with an alpha channel.  Use
> it if supported by the vnc client, prefer it over the "rich cursor"
> extension which supports only a bitmask for transparency.
> 
> This is a visible improvement especially on modern desktops which
> actually use the alpha channel when defining cursors.
> 
> https://github.com/rfbproto/rfbproto/blob/master/rfbproto.rst#cursor-with-alpha-pseudo-encoding
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  ui/vnc.h |  2 ++
>  ui/vnc.c | 21 ++++++++++++++++++---
>  2 files changed, 20 insertions(+), 3 deletions(-)

Add implemented the client side in GTK-VNC and tested against
this QEMU patch:

  https://gitlab.gnome.org/GNOME/gtk-vnc/-/merge_requests/8

So

Tested-by: Daniel P. Berrangé <berrange@redhat.com>
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 v2 7/9] vnc: force initial resize message

[Gerd Hoffmann]

The vnc server should send desktop resize notifications unconditionally
on a new client connect, for feature negotiation reasons.  Add a bool
flag to vnc_desktop_resize() to force sending the message even in case
there is no size change.

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

diff --git a/ui/vnc.c b/ui/vnc.c
index 247e80d8f5c8..bdaf384f71a4 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -664,13 +664,14 @@ void vnc_framebuffer_update(VncState *vs, int x, int y, int w, int h,
 }
 
 
-static void vnc_desktop_resize(VncState *vs)
+static void vnc_desktop_resize(VncState *vs, bool force)
 {
     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)) {
+        vs->client_height == pixman_image_get_height(vs->vd->server) &&
+        !force) {
         return;
     }
 
@@ -800,7 +801,7 @@ static void vnc_dpy_switch(DisplayChangeListener *dcl,
 
     QTAILQ_FOREACH(vs, &vd->clients, next) {
         vnc_colordepth(vs);
-        vnc_desktop_resize(vs);
+        vnc_desktop_resize(vs, false);
         if (vs->vd->cursor) {
             vnc_cursor_define(vs);
         }
@@ -2143,7 +2144,7 @@ static void set_encodings(VncState *vs, int32_t *encodings, size_t n_encodings)
             break;
         }
     }
-    vnc_desktop_resize(vs);
+    vnc_desktop_resize(vs, true);
     check_pointer_type_change(&vs->mouse_mode_notifier, NULL);
     vnc_led_state_change(vs);
     if (vs->vd->cursor) {
-- 
2.27.0

Re: [PATCH v2 7/9] vnc: force initial resize message

[Daniel P. Berrangé]

On Tue, Dec 08, 2020 at 12:57:35PM +0100, Gerd Hoffmann wrote:
> The vnc server should send desktop resize notifications unconditionally
> on a new client connect, for feature negotiation reasons.  Add a bool
> flag to vnc_desktop_resize() to force sending the message even in case
> there is no size change.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  ui/vnc.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)

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 v2 8/9] 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 bdaf384f71a4..094ba6cd14cb 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -663,10 +663,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, bool force)
 {
-    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;
     }
     if (vs->client_width == pixman_image_get_width(vs->vd->server) &&
@@ -681,6 +706,12 @@ static void vnc_desktop_resize(VncState *vs, bool force)
            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);
@@ -2110,6 +2141,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;
@@ -2431,6 +2465,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.27.0

Re: [PATCH v2 8/9] vnc: add support for extended desktop resize

[Daniel P. Berrangé]

On Tue, Dec 08, 2020 at 12:57:36PM +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

The spec says

   "The server must send an ExtendedDesktopSize rectangle in response 
    to a FramebufferUpdateRequest with incremental set to zero, assuming
    the client has requested the ExtendedDesktopSize pseudo-encoding 
    using the SetEncodings message. This requirement is needed so that
    the client has a reliable way of fetching the initial screen 
    configuration, and to determine if the server supports the 
    SetDesktopSize message.

    A consequence of this is that a client must not respond to an
    ExtendedDesktopSize rectangle by sending a FramebufferUpdateRequest
    with incremental set to zero. Doing so would make the system go into
    an infinite loop."

Historically gtk-vnc always sets incremental=0 after a resize message,
so it should trigger an infinite loop. This doesn't happen with QEMU's
impl, so I think QEMU isn't correctly following the sec here. IIUC,
tt needs to force send an ExtendedDesktopSize message every time
incremental=0, not merely when a resize happens.

Having said that, I find this part of the spec rather crazy. I dont
see why clients need more than the first ExtendedDesktopSize message
in order to detect the feature, rather than after every single
incremental=0 update request.

None the less the spec says we should get an infinite loop and with
my intentional attempt to cause this, QEMU doesn't play ball.

> 
> 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 bdaf384f71a4..094ba6cd14cb 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -663,10 +663,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, bool force)
>  {
> -    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;
>      }
>      if (vs->client_width == pixman_image_get_width(vs->vd->server) &&
> @@ -681,6 +706,12 @@ static void vnc_desktop_resize(VncState *vs, bool force)
>             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);
> @@ -2110,6 +2141,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;
> @@ -2431,6 +2465,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.27.0
> 
> 

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 v2 8/9] vnc: add support for extended desktop resize

[Gerd Hoffmann]

On Tue, Dec 08, 2020 at 06:13:28PM +0000, Daniel P. Berrangé wrote:
> On Tue, Dec 08, 2020 at 12:57:36PM +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
> 
> The spec says
> 
>    "The server must send an ExtendedDesktopSize rectangle in response 
>     to a FramebufferUpdateRequest with incremental set to zero, assuming
>     the client has requested the ExtendedDesktopSize pseudo-encoding 
>     using the SetEncodings message. This requirement is needed so that
>     the client has a reliable way of fetching the initial screen 
>     configuration, and to determine if the server supports the 
>     SetDesktopSize message.
> 
>     A consequence of this is that a client must not respond to an
>     ExtendedDesktopSize rectangle by sending a FramebufferUpdateRequest
>     with incremental set to zero. Doing so would make the system go into
>     an infinite loop."
> 
> Historically gtk-vnc always sets incremental=0 after a resize message,
> so it should trigger an infinite loop. This doesn't happen with QEMU's
> impl, so I think QEMU isn't correctly following the sec here. IIUC,
> tt needs to force send an ExtendedDesktopSize message every time
> incremental=0, not merely when a resize happens.

There are three cases where qemu sends an ExtendedDesktopSize message:

  (1) during the initial handshake (to cover the feature detection).
  (2) as response to a SetDesktopSize request, and
  (3) when an resize happens.

So, yes, I skimmed the spec a bit to fast and didn't implement it
correctly.  Which also explains why you can't trigger the infinite
loop issue.

> Having said that, I find this part of the spec rather crazy. I dont
> see why clients need more than the first ExtendedDesktopSize message
> in order to detect the feature, rather than after every single
> incremental=0 update request.

Yes, it doesn't make much sense.

> None the less the spec says we should get an infinite loop and with
> my intentional attempt to cause this, QEMU doesn't play ball.

Not fully sure I should consider this a bug or a feature.
This patch doesn't conform to the spec.
But it clearly is more robust ...

take care,
  Gerd

Re: [PATCH v2 8/9] vnc: add support for extended desktop resize

[Daniel P. Berrangé]

On Tue, Dec 15, 2020 at 11:15:03AM +0100, Gerd Hoffmann wrote:
> On Tue, Dec 08, 2020 at 06:13:28PM +0000, Daniel P. Berrangé wrote:
> > On Tue, Dec 08, 2020 at 12:57:36PM +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
> > 
> > The spec says
> > 
> >    "The server must send an ExtendedDesktopSize rectangle in response 
> >     to a FramebufferUpdateRequest with incremental set to zero, assuming
> >     the client has requested the ExtendedDesktopSize pseudo-encoding 
> >     using the SetEncodings message. This requirement is needed so that
> >     the client has a reliable way of fetching the initial screen 
> >     configuration, and to determine if the server supports the 
> >     SetDesktopSize message.
> > 
> >     A consequence of this is that a client must not respond to an
> >     ExtendedDesktopSize rectangle by sending a FramebufferUpdateRequest
> >     with incremental set to zero. Doing so would make the system go into
> >     an infinite loop."
> > 
> > Historically gtk-vnc always sets incremental=0 after a resize message,
> > so it should trigger an infinite loop. This doesn't happen with QEMU's
> > impl, so I think QEMU isn't correctly following the sec here. IIUC,
> > tt needs to force send an ExtendedDesktopSize message every time
> > incremental=0, not merely when a resize happens.
> 
> There are three cases where qemu sends an ExtendedDesktopSize message:
> 
>   (1) during the initial handshake (to cover the feature detection).
>   (2) as response to a SetDesktopSize request, and
>   (3) when an resize happens.
> 
> So, yes, I skimmed the spec a bit to fast and didn't implement it
> correctly.  Which also explains why you can't trigger the infinite
> loop issue.
> 
> > Having said that, I find this part of the spec rather crazy. I dont
> > see why clients need more than the first ExtendedDesktopSize message
> > in order to detect the feature, rather than after every single
> > incremental=0 update request.
> 
> Yes, it doesn't make much sense.
> 
> > None the less the spec says we should get an infinite loop and with
> > my intentional attempt to cause this, QEMU doesn't play ball.
> 
> Not fully sure I should consider this a bug or a feature.
> This patch doesn't conform to the spec.
> But it clearly is more robust ...

I'm going to propose a spec change and see what response it gets

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 v2 8/9] vnc: add support for extended desktop resize

[Daniel P. Berrangé]

On Tue, Dec 15, 2020 at 10:20:40AM +0000, Daniel P. Berrangé wrote:
> On Tue, Dec 15, 2020 at 11:15:03AM +0100, Gerd Hoffmann wrote:
> > On Tue, Dec 08, 2020 at 06:13:28PM +0000, Daniel P. Berrangé wrote:
> > > On Tue, Dec 08, 2020 at 12:57:36PM +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
> > > 
> > > The spec says
> > > 
> > >    "The server must send an ExtendedDesktopSize rectangle in response 
> > >     to a FramebufferUpdateRequest with incremental set to zero, assuming
> > >     the client has requested the ExtendedDesktopSize pseudo-encoding 
> > >     using the SetEncodings message. This requirement is needed so that
> > >     the client has a reliable way of fetching the initial screen 
> > >     configuration, and to determine if the server supports the 
> > >     SetDesktopSize message.
> > > 
> > >     A consequence of this is that a client must not respond to an
> > >     ExtendedDesktopSize rectangle by sending a FramebufferUpdateRequest
> > >     with incremental set to zero. Doing so would make the system go into
> > >     an infinite loop."
> > > 
> > > Historically gtk-vnc always sets incremental=0 after a resize message,
> > > so it should trigger an infinite loop. This doesn't happen with QEMU's
> > > impl, so I think QEMU isn't correctly following the sec here. IIUC,
> > > tt needs to force send an ExtendedDesktopSize message every time
> > > incremental=0, not merely when a resize happens.
> > 
> > There are three cases where qemu sends an ExtendedDesktopSize message:
> > 
> >   (1) during the initial handshake (to cover the feature detection).
> >   (2) as response to a SetDesktopSize request, and
> >   (3) when an resize happens.
> > 
> > So, yes, I skimmed the spec a bit to fast and didn't implement it
> > correctly.  Which also explains why you can't trigger the infinite
> > loop issue.
> > 
> > > Having said that, I find this part of the spec rather crazy. I dont
> > > see why clients need more than the first ExtendedDesktopSize message
> > > in order to detect the feature, rather than after every single
> > > incremental=0 update request.
> > 
> > Yes, it doesn't make much sense.
> > 
> > > None the less the spec says we should get an infinite loop and with
> > > my intentional attempt to cause this, QEMU doesn't play ball.
> > 
> > Not fully sure I should consider this a bug or a feature.
> > This patch doesn't conform to the spec.
> > But it clearly is more robust ...
> 
> I'm going to propose a spec change and see what response it gets

I ended up opening an issue to query the spec instead:

  https://github.com/rfbproto/rfbproto/issues/37


Also, I've proposed a spec update to make the interaction between
ExtendedDesktopSize and WMVi encodings explicit:

   https://github.com/rfbproto/rfbproto/pull/36

QEMU is already  in compliance with this clarification, I just wanted
to make it explicitly required.

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 v2 9/9] 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 094ba6cd14cb..1f7dc545db7c 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -833,9 +833,7 @@ static void vnc_dpy_switch(DisplayChangeListener *dcl,
     QTAILQ_FOREACH(vs, &vd->clients, next) {
         vnc_colordepth(vs);
         vnc_desktop_resize(vs, false);
-        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),
@@ -969,6 +967,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);
@@ -2181,9 +2183,7 @@ static void set_encodings(VncState *vs, int32_t *encodings, size_t n_encodings)
     vnc_desktop_resize(vs, true);
     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.27.0

Re: [PATCH v2 0/9] vnc: support some new extensions.

[Daniel P. Berrangé]

On Tue, Dec 08, 2020 at 12:57:28PM +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.
> 
> gtk-vnc (and anything building on top of it like virt-viewer and
> virt-manager) has no support for these extensions.

I'm working on adding these two extensions to gtk-vnc as they're a notable
gap compared to spice.

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