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

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

Gerd Hoffmann (9):
  console: allow con==NULL in dpy_set_ui_info
  console: add check for ui_info pointer
  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
  qxl: add ui_info callback

 ui/vnc.h         | 32 +++++++++-------
 hw/display/qxl.c | 27 ++++++++++++++
 ui/console.c     |  8 +++-
 ui/vnc.c         | 97 ++++++++++++++++++++++++++++++++++++++++++------
 4 files changed, 138 insertions(+), 26 deletions(-)

-- 
2.27.0

[PATCH 1/9] console: allow con==NULL in dpy_set_ui_info

[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 | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/ui/console.c b/ui/console.c
index 53dee8e26b17..16b326854080 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1556,7 +1556,9 @@ const QemuUIInfo *dpy_get_ui_info(const QemuConsole *con)
 
 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 1/9] console: allow con==NULL in dpy_set_ui_info

[Marc-André Lureau]

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

Hi

On Thu, Dec 3, 2020 at 3:20 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>
>

Why not do it for the remaining functions?
At least dpy_get_ui_info() for consistency.

---
>  ui/console.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/ui/console.c b/ui/console.c
> index 53dee8e26b17..16b326854080 100644
> --- a/ui/console.c
> +++ b/ui/console.c
> @@ -1556,7 +1556,9 @@ const QemuUIInfo *dpy_get_ui_info(const QemuConsole
> *con)
>
>  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: 1574 bytes --]

[PATCH 2/9] console: add check for ui_info pointer

[Gerd Hoffmann]

Verify the hw_ops->ui_info function pointer is non-zero before
calling it.  Can be triggered by qxl which changes hw_ops when
switching between qxl-native and vga-compat modes.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 ui/console.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/ui/console.c b/ui/console.c
index 16b326854080..8930808d0b6d 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1539,7 +1539,9 @@ static void dpy_set_ui_info_timer(void *opaque)
 {
     QemuConsole *con = opaque;
 
-    con->hw_ops->ui_info(con->hw, con->head, &con->ui_info);
+    if (con->hw_ops->ui_info) {
+        con->hw_ops->ui_info(con->hw, con->head, &con->ui_info);
+    }
 }
 
 bool dpy_ui_info_supported(QemuConsole *con)
-- 
2.27.0

Re: [PATCH 2/9] console: add check for ui_info pointer

[Marc-André Lureau]

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

On Thu, Dec 3, 2020 at 3:08 PM Gerd Hoffmann <kraxel@redhat.com> wrote:

> Verify the hw_ops->ui_info function pointer is non-zero before
> calling it.  Can be triggered by qxl which changes hw_ops when
> switching between qxl-native and vga-compat modes.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  ui/console.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/ui/console.c b/ui/console.c
> index 16b326854080..8930808d0b6d 100644
> --- a/ui/console.c
> +++ b/ui/console.c
> @@ -1539,7 +1539,9 @@ static void dpy_set_ui_info_timer(void *opaque)
>  {
>      QemuConsole *con = opaque;
>
> -    con->hw_ops->ui_info(con->hw, con->head, &con->ui_info);
> +    if (con->hw_ops->ui_info) {
> +        con->hw_ops->ui_info(con->hw, con->head, &con->ui_info);
> +    }
>

That would ignore the last UI info change, right? Is there a place where it
is actually taken care off later? If yes, perhaps worth a comment. If not,
should it reschedule the timer or should there be a warning on the console?

 }
>
>  bool dpy_ui_info_supported(QemuConsole *con)
> --
> 2.27.0
>
>
>

-- 
Marc-André Lureau

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

[PATCH 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>
---
 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

Re: [PATCH 3/9] vnc: use enum for features

[Marc-André Lureau]

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

On Thu, Dec 3, 2020 at 3:08 PM Gerd Hoffmann <kraxel@redhat.com> wrote:

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

-- 
Marc-André Lureau

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

[PATCH 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>
---
 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 4/9] vnc: drop unused copyrect feature

[Marc-André Lureau]

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

On Thu, Dec 3, 2020 at 3:17 PM Gerd Hoffmann <kraxel@redhat.com> 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(-)
>
> 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
>
>
>

-- 
Marc-André Lureau

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

[PATCH 5/9] vnc: add pseudo encodings

[Gerd Hoffmann]

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

Signed-off-by: Gerd Hoffmann <kraxel@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 5/9] vnc: add pseudo encodings

[Marc-André Lureau]

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

On Thu, Dec 3, 2020 at 3:15 PM Gerd Hoffmann <kraxel@redhat.com> wrote:

> Add #defines for two new pseudo encodings:
>  * cursor with alpha channel.
>  * extended desktop resize.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>

It might be worth documenting somewhere where those values come from.

My understanding is that the official document is
https://tools.ietf.org/html/rfc6143, and the community maintained version
is https://github.com/rfbproto/rfbproto/blob/master/rfbproto.rst

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

-- 
Marc-André Lureau

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

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

[Marc-André Lureau]

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

Hi

On Thu, Dec 3, 2020 at 3:11 PM Gerd Hoffmann <kraxel@redhat.com> 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>
> ---
>  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);
> +    }
>

Looks good. Minor nit, I would suggest to do the "if (!vs->vd->cursor)
return" in vnc_cursor_define().

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

 }
>
>  static void set_pixel_conversion(VncState *vs)
> --
> 2.27.0
>
>
>

-- 
Marc-André Lureau

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

[PATCH 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 7/9] vnc: force initial resize message

[Marc-André Lureau]

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

Hi

On Thu, Dec 3, 2020 at 3:12 PM Gerd Hoffmann <kraxel@redhat.com> 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>
>

In principle, this looks harmless. But the spec says:

"The server should only send a *DesktopSize* pseudo-rectangle when an
actual change of the framebuffer dimensions has occurred. Some clients
respond to a *DesktopSize* pseudo-rectangle in a way that could send the
system into an infinite loop if the server sent out the pseudo-rectangle
for anything other than an actual change."
(
https://github.com/rfbproto/rfbproto/blob/master/rfbproto.rst#server-semantics
)

I can't say if sending desktop resize during the initial SetEncoding phase
is really compliant with the specification. Also, it's unclear to me if the
client is allowed to SetEncoding multiple times (in which there would be no
dimension change occurring).

What did you fix with this? Is it worth a clarification in the
specification?

thanks

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

-- 
Marc-André Lureau

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

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

[Gerd Hoffmann]

On Fri, Dec 04, 2020 at 03:57:23PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Thu, Dec 3, 2020 at 3:12 PM Gerd Hoffmann <kraxel@redhat.com> 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>
> >
> 
> In principle, this looks harmless. But the spec says:
> 
> "The server should only send a *DesktopSize* pseudo-rectangle when an
> actual change of the framebuffer dimensions has occurred. Some clients
> respond to a *DesktopSize* pseudo-rectangle in a way that could send the
> system into an infinite loop if the server sent out the pseudo-rectangle
> for anything other than an actual change."
> (
> https://github.com/rfbproto/rfbproto/blob/master/rfbproto.rst#server-semantics
> )
> 
> I can't say if sending desktop resize during the initial SetEncoding phase
> is really compliant with the specification. Also, it's unclear to me if the
> client is allowed to SetEncoding multiple times (in which there would be no
> dimension change occurring).
> 
> What did you fix with this? Is it worth a clarification in the
> specification?

Well, for ExtendedDesktopResize the spec explicitly asked for this.
But, yes, for DesktopResize this is not needed.  But it also shouldn't
cause much trouble.  It is sent before any actual display updates, so
concerns whenever the client should consider the screen content invalid
or not are moot.

I could squash this into patch #8 and do it for ExtendedDesktopResize
only ...

take care,
  Gerd

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

[Marc-André Lureau]

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

Hi

On Tue, Dec 8, 2020 at 10:57 AM Gerd Hoffmann <kraxel@redhat.com> wrote:

> On Fri, Dec 04, 2020 at 03:57:23PM +0400, Marc-André Lureau wrote:
> > Hi
> >
> > On Thu, Dec 3, 2020 at 3:12 PM Gerd Hoffmann <kraxel@redhat.com> 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>
> > >
> >
> > In principle, this looks harmless. But the spec says:
> >
> > "The server should only send a *DesktopSize* pseudo-rectangle when an
> > actual change of the framebuffer dimensions has occurred. Some clients
> > respond to a *DesktopSize* pseudo-rectangle in a way that could send the
> > system into an infinite loop if the server sent out the pseudo-rectangle
> > for anything other than an actual change."
> > (
> >
> https://github.com/rfbproto/rfbproto/blob/master/rfbproto.rst#server-semantics
> > )
> >
> > I can't say if sending desktop resize during the initial SetEncoding
> phase
> > is really compliant with the specification. Also, it's unclear to me if
> the
> > client is allowed to SetEncoding multiple times (in which there would be
> no
> > dimension change occurring).
> >
> > What did you fix with this? Is it worth a clarification in the
> > specification?
>
> Well, for ExtendedDesktopResize the spec explicitly asked for this.
> But, yes, for DesktopResize this is not needed.  But it also shouldn't
> cause much trouble.  It is sent before any actual display updates, so
> concerns whenever the client should consider the screen content invalid
> or not are moot.
>
> I could squash this into patch #8 and do it for ExtendedDesktopResize
> only ...
>

Ok, it's probably fine (dunno), you could also capture that in the commit
message, or as code comment.


-- 
Marc-André Lureau

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

[PATCH 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..a15132faa96f 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, bool reject)
+{
+    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 ? 1 : 0,
+                           reject ? 3 : 0,
+                           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, false);
+        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;
+        uint16_t width;
+        uint16_t height;
+        QemuUIInfo info;
+
+        if (len < 8) {
+            return 8;
+        }
+
+        screens = read_u8(data, 6);
+        size    = 8 + screens * 16;
+        if (len < size) {
+            return size;
+        }
+
+        width   = read_u16(data, 2);
+        height  = read_u16(data, 4);
+        vnc_desktop_resize_ext(vs, true);
+
+        memset(&info, 0, sizeof(info));
+        info.width = width;
+        info.height = height;
+        dpy_set_ui_info(vs->vd->dcl.con, &info);
+        break;
+    }
     default:
         VNC_DEBUG("Msg: %d\n", data[0]);
         vnc_client_error(vs);
-- 
2.27.0

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

[Daniel P. Berrangé]

On Thu, Dec 03, 2020 at 12:08:04PM +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(-)
> 
> 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..a15132faa96f 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, bool reject)
> +{
> +    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 ? 1 : 0,
> +                           reject ? 3 : 0,
> +                           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, false);
> +        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;

IIUC, we shouldn't set this flag unless all current displays adapters
associated with the VNC server support the "ui_info" callbacks,
otherwise the client will think it can send resize requests
but they'll never be honoured.

> +            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;
> +        uint16_t width;
> +        uint16_t height;
> +        QemuUIInfo info;
> +
> +        if (len < 8) {
> +            return 8;
> +        }
> +
> +        screens = read_u8(data, 6);
> +        size    = 8 + screens * 16;
> +        if (len < size) {
> +            return size;
> +        }
> +
> +        width   = read_u16(data, 2);
> +        height  = read_u16(data, 4);
> +        vnc_desktop_resize_ext(vs, true);
> +
> +        memset(&info, 0, sizeof(info));
> +        info.width = width;
> +        info.height = height;
> +        dpy_set_ui_info(vs->vd->dcl.con, &info);
> +        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 8/9] vnc: add support for extended desktop resize

[Gerd Hoffmann]

  Hi,

> > +        case VNC_ENCODING_DESKTOP_RESIZE_EXT:
> > +            vs->features |= VNC_FEATURE_RESIZE_EXT_MASK;
> 
> IIUC, we shouldn't set this flag unless all current displays adapters
> associated with the VNC server support the "ui_info" callbacks,
> otherwise the client will think it can send resize requests
> but they'll never be honoured.

Well, that can happen anyway as honoring the request is in the hands of
the guest and not something qemu can guarantee.  So vnc clients must be
able to deal with that no matter what.  The spec even explicitly states
that rejecting all resize requests from the client is perfectly valid
behavior for a server.

For tigervnc it seems to make no difference whenever the server supports
extended desktop resize or not.

I doubt making this conditional buys us anything ...

take care,
  Gerd

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

[Daniel P. Berrangé]

On Fri, Dec 04, 2020 at 07:37:50AM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> > > +        case VNC_ENCODING_DESKTOP_RESIZE_EXT:
> > > +            vs->features |= VNC_FEATURE_RESIZE_EXT_MASK;
> > 
> > IIUC, we shouldn't set this flag unless all current displays adapters
> > associated with the VNC server support the "ui_info" callbacks,
> > otherwise the client will think it can send resize requests
> > but they'll never be honoured.
> 
> Well, that can happen anyway as honoring the request is in the hands of
> the guest and not something qemu can guarantee.  So vnc clients must be
> able to deal with that no matter what.  The spec even explicitly states
> that rejecting all resize requests from the client is perfectly valid
> behavior for a server.

Yes, I see we are rejecting requests unconditionally now.

I still think it is valuable to clients to see a difference between
something that is rejected because there is zero chance it will be
honoured, vs something that we are likely honour albeit asynchronously.

So I suggested we have a new error reason to indicate it is being
processed async.  If we don't have ui_info, then we should reject
with reason 1 - Resize is administratively prohibited, but if we
can probably honour it, then reject with a new reason 4 to indicate
async handling.

> For tigervnc it seems to make no difference whenever the server supports
> extended desktop resize or not.
> 
> I doubt making this conditional buys us anything ...

If we know there is zero chance of this working, then clients can
take different behaviour. For example, we can make the window
non-resizable, or activate scaling of the graphics, instead of
waiting for a resize.  So this distinction will be useful to
improve the user experiance of virt-viewer/remote-viewer IMHO.

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

[Daniel P. Berrangé]

On Thu, Dec 03, 2020 at 12:08:04PM +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(-)
> 
> 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..a15132faa96f 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, bool reject)
> +{
> +    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 ? 1 : 0,
> +                           reject ? 3 : 0,

So there are a number of reject reasons defined

Code 	Description
0 	No error
1 	Resize is administratively prohibited
2 	Out of resources
3 	Invalid screen layout

none of them is an ideal fit, because we are actually attempting
to honour the request, but it is asynchronous so we can't
confirm this to the client yet.

I feel like we could propose a new reason 4 to the spec, since
it explicitly allows for adding new reasons

  "Request under consideration"

to make it clear this is not actually an invalid layout.

This can be useful for clients to decide how they want to
handle the failure.

> +                           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, false);
> +        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;
> +        uint16_t width;
> +        uint16_t height;
> +        QemuUIInfo info;
> +
> +        if (len < 8) {
> +            return 8;
> +        }
> +
> +        screens = read_u8(data, 6);
> +        size    = 8 + screens * 16;
> +        if (len < size) {
> +            return size;
> +        }
> +
> +        width   = read_u16(data, 2);
> +        height  = read_u16(data, 4);
> +        vnc_desktop_resize_ext(vs, true);

I think it is worth a comment to say why we are rejecting the request...


> +
> +        memset(&info, 0, sizeof(info));
> +        info.width = width;
> +        info.height = height;
> +        dpy_set_ui_info(vs->vd->dcl.con, &info);

...while still (attempting to) honour it.

> +        break;
> +    }
>      default:
>          VNC_DEBUG("Msg: %d\n", data[0]);
>          vnc_client_error(vs);

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

[Marc-André Lureau]

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

Hi

On Thu, Dec 3, 2020 at 3:13 PM Gerd Hoffmann <kraxel@redhat.com> 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(-)
>
> 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..a15132faa96f 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, bool reject)
> +{
> +    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 ? 1 : 0,


1 "The client receiving this message requested a change of the screen
layout. The change may or may not have happened depending on server policy
or available resources. The status code in the *y-position* field must be
used to determine which."
ok

0 "The screen layout was changed via non-RFB means on the server."
ok

+                           reject ? 3 : 0,
>

3 "Invalid screen layout"
ok

+                           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, false);
> +        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;
> +        uint16_t width;
> +        uint16_t height;
> +        QemuUIInfo info;
> +
> +        if (len < 8) {
> +            return 8;
> +        }
> +
> +        screens = read_u8(data, 6);
> +        size    = 8 + screens * 16;
> +        if (len < size) {
> +            return size;
> +        }
>

Maybe leave a TODO note for handling multiple screens etc?

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

+
> +        width   = read_u16(data, 2);
> +        height  = read_u16(data, 4);
> +        vnc_desktop_resize_ext(vs, true);
> +
> +        memset(&info, 0, sizeof(info));
> +        info.width = width;
> +        info.height = height;
> +        dpy_set_ui_info(vs->vd->dcl.con, &info);
> +        break;
> +    }
>      default:
>          VNC_DEBUG("Msg: %d\n", data[0]);
>          vnc_client_error(vs);
> --
> 2.27.0
>
>
>

-- 
Marc-André Lureau

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

[PATCH 9/9] qxl: add ui_info callback

[Gerd Hoffmann]

This makes qxl respond to user interface window resizes
when not using spice, so it works with gtk and vnc too.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/qxl.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 431c1070967a..e1df95c3e8a9 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -1177,8 +1177,35 @@ static const QXLInterface qxl_interface = {
     .client_monitors_config = interface_client_monitors_config,
 };
 
+static int qxl_ui_info(void *opaque, uint32_t idx, QemuUIInfo *info)
+{
+    PCIQXLDevice *qxl = opaque;
+    VDAgentMonitorsConfig *cfg;
+    size_t size;
+
+    if (using_spice) {
+        /* spice agent will handle display resize */
+        return -1;
+    }
+    if (idx > 0) {
+        /* supporting only single head for now */
+        return -1;
+    }
+
+    /* go fake a spice agent message */
+    size = sizeof(VDAgentMonitorsConfig) + sizeof(VDAgentMonConfig);
+    cfg = g_malloc0(size);
+    cfg->num_of_monitors = 1;
+    cfg->monitors[0].width = info->width;
+    cfg->monitors[0].height = info->height;
+    interface_client_monitors_config(&qxl->ssd.qxl, cfg);
+    g_free(cfg);
+    return 0;
+}
+
 static const GraphicHwOps qxl_ops = {
     .gfx_update  = qxl_hw_update,
+    .ui_info     = qxl_ui_info,
     .gfx_update_async = true,
 };
 
-- 
2.27.0

Re: [PATCH 9/9] qxl: add ui_info callback

[Daniel P. Berrangé]

On Thu, Dec 03, 2020 at 12:08:05PM +0100, Gerd Hoffmann wrote:
> This makes qxl respond to user interface window resizes
> when not using spice, so it works with gtk and vnc too.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/display/qxl.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> index 431c1070967a..e1df95c3e8a9 100644
> --- a/hw/display/qxl.c
> +++ b/hw/display/qxl.c
> @@ -1177,8 +1177,35 @@ static const QXLInterface qxl_interface = {
>      .client_monitors_config = interface_client_monitors_config,
>  };
>  
> +static int qxl_ui_info(void *opaque, uint32_t idx, QemuUIInfo *info)
> +{
> +    PCIQXLDevice *qxl = opaque;
> +    VDAgentMonitorsConfig *cfg;
> +    size_t size;
> +
> +    if (using_spice) {
> +        /* spice agent will handle display resize */
> +        return -1;
> +    }

Does this break VNC resizes if  both  VNC + SPICE are enabled
but the client is connected with VNC ?

> +    if (idx > 0) {
> +        /* supporting only single head for now */
> +        return -1;
> +    }
> +
> +    /* go fake a spice agent message */
> +    size = sizeof(VDAgentMonitorsConfig) + sizeof(VDAgentMonConfig);
> +    cfg = g_malloc0(size);
> +    cfg->num_of_monitors = 1;
> +    cfg->monitors[0].width = info->width;
> +    cfg->monitors[0].height = info->height;
> +    interface_client_monitors_config(&qxl->ssd.qxl, cfg);
> +    g_free(cfg);
> +    return 0;
> +}
> +
>  static const GraphicHwOps qxl_ops = {
>      .gfx_update  = qxl_hw_update,
> +    .ui_info     = qxl_ui_info,
>      .gfx_update_async = true,
>  };
>  
> -- 
> 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 9/9] qxl: add ui_info callback

[Marc-André Lureau]

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

On Fri, Dec 4, 2020 at 4:21 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Thu, Dec 03, 2020 at 12:08:05PM +0100, Gerd Hoffmann wrote:
> > This makes qxl respond to user interface window resizes
> > when not using spice, so it works with gtk and vnc too.
> >
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > ---
> >  hw/display/qxl.c | 27 +++++++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> >
> > diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> > index 431c1070967a..e1df95c3e8a9 100644
> > --- a/hw/display/qxl.c
> > +++ b/hw/display/qxl.c
> > @@ -1177,8 +1177,35 @@ static const QXLInterface qxl_interface = {
> >      .client_monitors_config = interface_client_monitors_config,
> >  };
> >
> > +static int qxl_ui_info(void *opaque, uint32_t idx, QemuUIInfo *info)
> > +{
> > +    PCIQXLDevice *qxl = opaque;
> > +    VDAgentMonitorsConfig *cfg;
> > +    size_t size;
> > +
> > +    if (using_spice) {
> > +        /* spice agent will handle display resize */
> > +        return -1;
> > +    }
>
> Does this break VNC resizes if  both  VNC + SPICE are enabled
> but the client is connected with VNC ?
>

Yes. I am not sure we should worry about that case much, either way.
Perhaps it's best to handle both QEMU-originated resize and spice-agent
based resizes, even if the former can lose details from the later for
multiple screens.


> > +    if (idx > 0) {
> > +        /* supporting only single head for now */
> > +        return -1;
> > +    }
> > +
> > +    /* go fake a spice agent message */
> > +    size = sizeof(VDAgentMonitorsConfig) + sizeof(VDAgentMonConfig);
> > +    cfg = g_malloc0(size);
> > +    cfg->num_of_monitors = 1;
> > +    cfg->monitors[0].width = info->width;
> > +    cfg->monitors[0].height = info->height;
> > +    interface_client_monitors_config(&qxl->ssd.qxl, cfg);
> > +    g_free(cfg);
> > +    return 0;
> > +}
> > +
> >  static const GraphicHwOps qxl_ops = {
> >      .gfx_update  = qxl_hw_update,
> > +    .ui_info     = qxl_ui_info,
> >      .gfx_update_async = true,
> >  };
> >
> > --
> > 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 :|
>
>
>

-- 
Marc-André Lureau

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

Re: [PATCH 9/9] qxl: add ui_info callback

[Daniel P. Berrangé]

On Fri, Dec 04, 2020 at 04:45:41PM +0400, Marc-André Lureau wrote:
> On Fri, Dec 4, 2020 at 4:21 PM Daniel P. Berrangé <berrange@redhat.com>
> wrote:
> 
> > On Thu, Dec 03, 2020 at 12:08:05PM +0100, Gerd Hoffmann wrote:
> > > This makes qxl respond to user interface window resizes
> > > when not using spice, so it works with gtk and vnc too.
> > >
> > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > > ---
> > >  hw/display/qxl.c | 27 +++++++++++++++++++++++++++
> > >  1 file changed, 27 insertions(+)
> > >
> > > diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> > > index 431c1070967a..e1df95c3e8a9 100644
> > > --- a/hw/display/qxl.c
> > > +++ b/hw/display/qxl.c
> > > @@ -1177,8 +1177,35 @@ static const QXLInterface qxl_interface = {
> > >      .client_monitors_config = interface_client_monitors_config,
> > >  };
> > >
> > > +static int qxl_ui_info(void *opaque, uint32_t idx, QemuUIInfo *info)
> > > +{
> > > +    PCIQXLDevice *qxl = opaque;
> > > +    VDAgentMonitorsConfig *cfg;
> > > +    size_t size;
> > > +
> > > +    if (using_spice) {
> > > +        /* spice agent will handle display resize */
> > > +        return -1;
> > > +    }
> >
> > Does this break VNC resizes if  both  VNC + SPICE are enabled
> > but the client is connected with VNC ?
> >
> 
> Yes. I am not sure we should worry about that case much, either way.
> Perhaps it's best to handle both QEMU-originated resize and spice-agent
> based resizes, even if the former can lose details from the later for
> multiple screens.

Or at least worth  comment to say that we're intentionally breaking
VNC resizes when both are enabled.


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