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

["Daniel P. Berrangé" <berrange@redhat.com>]

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