The H264 implementation only sends frames when it detects changes in
the server's framebuffer. This leads to artifacts when there are no
further changes, as the internal H264 encoder may still contain data.

This patch modifies the code to send a few additional frames in such
situations to flush the H264 encoder data.

Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>
---
 ui/vnc.c | 25 ++++++++++++++++++++++++-
 ui/vnc.h |  3 +++
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index aed25b0183..badc7912c0 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -3239,7 +3239,30 @@ static void vnc_refresh(DisplayChangeListener *dcl)
     vnc_unlock_display(vd);
 
     QTAILQ_FOREACH_SAFE(vs, &vd->clients, next, vn) {
-        rects += vnc_update_client(vs, has_dirty);
+        int client_dirty = has_dirty;
+        if (vs->h264) {
+            if (client_dirty) {
+                vs->h264->keep_dirty = VNC_H264_KEEP_DIRTY;
+            } else {
+                if (vs->h264->keep_dirty > 0) {
+                    client_dirty = 1;
+                    vs->h264->keep_dirty--;
+                }
+            }
+        }
+
+        int count = vnc_update_client(vs, client_dirty);
+        rects += count;
+
+        if (vs->h264 && !count && vs->h264->keep_dirty) {
+            VncJob *job = vnc_job_new(vs);
+            int height = pixman_image_get_height(vd->server);
+            int width = pixman_image_get_width(vd->server);
+            vs->job_update = vs->update;
+            vs->update = VNC_STATE_UPDATE_NONE;
+            vnc_job_add_rect(job, 0, 0, width, height);
+            vnc_job_push(job);
+        }
         /* vs might be free()ed here */
     }
 
diff --git a/ui/vnc.h b/ui/vnc.h
index a0d336738d..a5ea134de8 100644
--- a/ui/vnc.h
+++ b/ui/vnc.h
@@ -236,10 +236,13 @@ typedef struct VncZywrle {
 } VncZywrle;
 
 #ifdef CONFIG_GSTREAMER
+/* Number of frames we send after the display is clean. */
+#define VNC_H264_KEEP_DIRTY 10
 typedef struct VncH264 {
     GstElement *pipeline, *source, *gst_encoder, *sink, *convert;
     size_t width;
     size_t height;
+    guint keep_dirty;
 } VncH264;
 #endif
 
-- 
2.39.5

On Fri, Apr 18, 2025 at 01:29:47PM +0200, Dietmar Maurer wrote:
> The H264 implementation only sends frames when it detects changes in
> the server's framebuffer. This leads to artifacts when there are no
> further changes, as the internal H264 encoder may still contain data.
> 
> This patch modifies the code to send a few additional frames in such
> situations to flush the H264 encoder data.
> 
> Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>
> ---
>  ui/vnc.c | 25 ++++++++++++++++++++++++-
>  ui/vnc.h |  3 +++
>  2 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/ui/vnc.c b/ui/vnc.c
> index aed25b0183..badc7912c0 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -3239,7 +3239,30 @@ static void vnc_refresh(DisplayChangeListener *dcl)
>      vnc_unlock_display(vd);
>  
>      QTAILQ_FOREACH_SAFE(vs, &vd->clients, next, vn) {
> -        rects += vnc_update_client(vs, has_dirty);
> +        int client_dirty = has_dirty;
> +        if (vs->h264) {
> +            if (client_dirty) {
> +                vs->h264->keep_dirty = VNC_H264_KEEP_DIRTY;
> +            } else {
> +                if (vs->h264->keep_dirty > 0) {
> +                    client_dirty = 1;
> +                    vs->h264->keep_dirty--;
> +                }
> +            }
> +        }
> +
> +        int count = vnc_update_client(vs, client_dirty);
> +        rects += count;
> +
> +        if (vs->h264 && !count && vs->h264->keep_dirty) {
> +            VncJob *job = vnc_job_new(vs);
> +            int height = pixman_image_get_height(vd->server);
> +            int width = pixman_image_get_width(vd->server);
> +            vs->job_update = vs->update;
> +            vs->update = VNC_STATE_UPDATE_NONE;
> +            vnc_job_add_rect(job, 0, 0, width, height);
> +            vnc_job_push(job);
> +        }
>          /* vs might be free()ed here */

This comment is telling you that 'vnc_update_client' may have
released 'vs', and yet this code is referencing 'vs->...'
and so will be liable to crash.

>      }
>  

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

On Fri, Apr 18, 2025 at 3:54 PM Dietmar Maurer <dietmar@proxmox.com> wrote:
>
> The H264 implementation only sends frames when it detects changes in
> the server's framebuffer. This leads to artifacts when there are no
> further changes, as the internal H264 encoder may still contain data.
>
> This patch modifies the code to send a few additional frames in such
> situations to flush the H264 encoder data.
>
> Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>

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

> ---
>  ui/vnc.c | 25 ++++++++++++++++++++++++-
>  ui/vnc.h |  3 +++
>  2 files changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/ui/vnc.c b/ui/vnc.c
> index aed25b0183..badc7912c0 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -3239,7 +3239,30 @@ static void vnc_refresh(DisplayChangeListener *dcl)
>      vnc_unlock_display(vd);
>
>      QTAILQ_FOREACH_SAFE(vs, &vd->clients, next, vn) {
> -        rects += vnc_update_client(vs, has_dirty);
> +        int client_dirty = has_dirty;
> +        if (vs->h264) {
> +            if (client_dirty) {
> +                vs->h264->keep_dirty = VNC_H264_KEEP_DIRTY;
> +            } else {
> +                if (vs->h264->keep_dirty > 0) {
> +                    client_dirty = 1;
> +                    vs->h264->keep_dirty--;
> +                }
> +            }
> +        }
> +
> +        int count = vnc_update_client(vs, client_dirty);
> +        rects += count;
> +
> +        if (vs->h264 && !count && vs->h264->keep_dirty) {
> +            VncJob *job = vnc_job_new(vs);
> +            int height = pixman_image_get_height(vd->server);
> +            int width = pixman_image_get_width(vd->server);
> +            vs->job_update = vs->update;
> +            vs->update = VNC_STATE_UPDATE_NONE;
> +            vnc_job_add_rect(job, 0, 0, width, height);
> +            vnc_job_push(job);
> +        }
>          /* vs might be free()ed here */
>      }
>
> diff --git a/ui/vnc.h b/ui/vnc.h
> index a0d336738d..a5ea134de8 100644
> --- a/ui/vnc.h
> +++ b/ui/vnc.h
> @@ -236,10 +236,13 @@ typedef struct VncZywrle {
>  } VncZywrle;
>
>  #ifdef CONFIG_GSTREAMER
> +/* Number of frames we send after the display is clean. */
> +#define VNC_H264_KEEP_DIRTY 10
>  typedef struct VncH264 {
>      GstElement *pipeline, *source, *gst_encoder, *sink, *convert;
>      size_t width;
>      size_t height;
> +    guint keep_dirty;
>  } VncH264;
>  #endif
>
> --
> 2.39.5
>
>


-- 
Marc-André Lureau