Re: [Qemu-devel] [PATCH] vnc: shared buffer: skip some optimizations.

[Stefano Stabellini <stefano.stabellini@eu.citrix.com>]

Gerd Hoffmann wrote:

>diff --git a/vnc.c b/vnc.c
>index 81c842a..66f8946 100644
>--- a/vnc.c
>+++ b/vnc.c
>@@ -649,7 +649,8 @@ static void vnc_dpy_copy(DisplayState *ds, int src_x, int src_y, int dst_x, int
>     VncDisplay *vd = ds->opaque;
>     VncState *vs = vd->clients;
>     while (vs != NULL) {
>-        if (vnc_has_feature(vs, VNC_FEATURE_COPYRECT))
>+        if (vnc_has_feature(vs, VNC_FEATURE_COPYRECT) &&
>+            !is_buffer_shared(ds->surface))
>             vnc_copy(vs, src_x, src_y, dst_x, dst_y, w, h);
>         else /* TODO */
>             vnc_update(vs, dst_x, dst_y, w, h);
>



I don't think this is needed (on qemu and qemu-xen, I don't know about
kvm): vnc_copy does not do any actual copy, only sends a copyrect update
to the vnc client.
Why should we prevent this when the buffer is shared?

>@@ -688,36 +689,51 @@ static void vnc_update_client(void *opaque)
> 
>         vnc_set_bits(width_mask, (ds_get_width(vs->ds) / 16), VNC_DIRTY_WORDS);
> 
>-	/* Walk through the dirty map and eliminate tiles that
>-	   really aren't dirty */
>-	row = ds_get_data(vs->ds);
>-	old_row = vs->old_data;
>-
>-	for (y = 0; y < ds_get_height(vs->ds); y++) {
>-	    if (vnc_and_bits(vs->dirty_row[y], width_mask, VNC_DIRTY_WORDS)) {
>-		int x;
>-		uint8_t *ptr;
>-		char *old_ptr;
>-
>-		ptr = row;
>-		old_ptr = (char*)old_row;
>-
>-		for (x = 0; x < ds_get_width(vs->ds); x += 16) {
>-		    if (memcmp(old_ptr, ptr, 16 * ds_get_bytes_per_pixel(vs->ds)) == 0) {
>-			vnc_clear_bit(vs->dirty_row[y], (x / 16));
>-		    } else {
>-			has_dirty = 1;
>-			memcpy(old_ptr, ptr, 16 * ds_get_bytes_per_pixel(vs->ds));
>-		    }
>-
>-		    ptr += 16 * ds_get_bytes_per_pixel(vs->ds);
>-		    old_ptr += 16 * ds_get_bytes_per_pixel(vs->ds);
>-		}
>-	    }
>


    
This loop filters the dirty_row bitmap using a memcmp against the
framebuffer, that could be more up to date than the bitmap itself.
In any case we are getting another vnc_dpy_update call next time
with the correct updated area.
So I don't think we risk losing updates here, the worst that could
happen is sending together portions of the screen more updated than
others to the client.
I would keep this loop even with the buffer shared.
Gerd, am I missing something?