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

[Stefano Stabellini]

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?