Re: [PATCH] ui/vnc: Update display update interval when VM state changes to RUNNING

[Peter Xu <peterx@redhat.com>]

On Wed, May 21, 2025 at 05:16:13PM +0200, Juraj Marcin wrote:
> From: Juraj Marcin <jmarcin@redhat.com>
> 
> If a virtual machine is paused for an extended period time, for example,
> due to an incoming migration, there are also no changes on the screen.
> VNC in such case increases the display update interval by
> VNC_REFRESH_INTERVAL_INC (50 ms). The update interval can then grow up
> to VNC_REFRESH_INTERVAL_MAX (3000 ms).
> 
> When the machine resumes, it can then take up to 3 seconds for the first
> display update. Furthermore, the update interval is then halved with
> each display update with changes on the screen. If there are moving
> elements on the screen, such as a video, this can be perceived as
> freezing and stuttering for few seconds before the movement is smooth
> again.
> 
> This patch resolves this issue, by adding a listener to VM state changes
> and changing the update interval when the VM state changes to RUNNING.
> The update_displaychangelistener() function updates the internal timer,
> and the display is refreshed immediately if the timer is expired.
> 
> Signed-off-by: Juraj Marcin <jmarcin@redhat.com>

Thanks for looking into it!

Reviewed-by: Peter Xu <peterx@redhat.com>

One trivial comment (and partly, pure question) below,

> ---
>  ui/vnc.c | 12 ++++++++++++
>  ui/vnc.h |  2 ++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 9e097dc4b4..32f8bfd1f9 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -3384,6 +3384,16 @@ static const DisplayChangeListenerOps dcl_ops = {
>      .dpy_cursor_define    = vnc_dpy_cursor_define,
>  };
>  
> +static void vmstate_change_handler(void *opaque, bool running, RunState state)
> +{
> +    VncDisplay *vd = opaque;
> +
> +    if (state != RUN_STATE_RUNNING) {

Just to mention in vm_prepare_start() it's possible we migrate a VM that
used to be suspended, if so it'll keep suspended after migration:

    RunState state = vm_was_suspended ? RUN_STATE_SUSPENDED : RUN_STATE_RUNNING;

Here I'm not sure whether SUSPENDED would also like to update the display
freq.  I don't think it matters hugely, but just to say, if we want we can
simply check "running=true" instead of checking the state to cover both
RUNNING|SUSPENDED cases.

> +        return;
> +    }
> +    update_displaychangelistener(&vd->dcl, VNC_REFRESH_INTERVAL_BASE);
> +}
> +
>  void vnc_display_init(const char *id, Error **errp)
>  {
>      VncDisplay *vd;
> @@ -3420,6 +3430,8 @@ void vnc_display_init(const char *id, Error **errp)
>      vd->dcl.ops = &dcl_ops;
>      register_displaychangelistener(&vd->dcl);
>      vd->kbd = qkbd_state_init(vd->dcl.con);
> +    vd->vmstate_handler_entry = qemu_add_vm_change_state_handler(
> +        &vmstate_change_handler, vd);
>  }
>  
>  
> diff --git a/ui/vnc.h b/ui/vnc.h
> index acc53a2cc1..3bb23acd34 100644
> --- a/ui/vnc.h
> +++ b/ui/vnc.h
> @@ -185,6 +185,8 @@ struct VncDisplay
>  #endif
>  
>      AudioState *audio_state;
> +
> +    VMChangeStateEntry *vmstate_handler_entry;
>  };
>  
>  typedef struct VncTight {
> -- 
> 2.49.0
> 

-- 
Peter Xu