On Wed, May 21, 2025 at 05:16:13PM +0200, Juraj Marcin wrote:
> From: Juraj Marcin <jmar...@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 <jmar...@redhat.com>
Thanks for looking into it!
Reviewed-by: Peter Xu <pet...@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
