[Spice-commits] src/spice-channel-cache.h src/spice-session.c

[Victor Toso de Carvalho]

 src/spice-channel-cache.h |   60 +++++++++++++++++++++++++++++++++++-----------
 src/spice-session.c       |    2 -
 2 files changed, 47 insertions(+), 15 deletions(-)

New commits:
commit c9d4773a666e00f347969981d2176708471ec0c7
Author: Sandy Stutsman <sstutsma at redhat.com>
Date:   Thu Jul 9 10:10:06 2015 -0400

    This adds reference counting to cached images.
    
    Windows guests with multi-monitors will often send down the same image
    via different channels.  If these instances are not counted,  one channel
    can delete an image before all the rest of the channels are finished with
    it. In this case, the client will hang.
    
    This can happen, for instance, when the Windows guest is updating the
    monitor's wallpaper. There is a driver instance for each monitor, so each
    one is sending down its own copy of the same image with the same id.  As
    one channel may be busier than another, the order that the client executes
    commands may not be the same order as the server issued them.  Here's what
    can happen:
    
      On the server side:
        Monitor 1 sends image A
        Monitor 1 sends multiple rendering commands for image A
        Monitor 1 removes image A
        Monitor 2 sends image A
        Monitor 2 sends rendering commands for image A
      On the client side:
        Monitor 1 adds image A to the cache
        Monitor 1 starts rendering with image A
        Monitor 2 adds image A to the cache - has same id, so image is replaced
        Monitor 1 removes image A - image A is completely removed from cache
        Monitor 2 render command hangs waiting for image A
    
    Addresses bug: https://bugzilla.redhat.com/show_bug.cgi?id=1194354

diff --git a/src/spice-channel-cache.h b/src/spice-channel-cache.h
index 17775e6..6e2d084 100644
--- a/src/spice-channel-cache.h
+++ b/src/spice-channel-cache.h
@@ -27,15 +27,20 @@ G_BEGIN_DECLS
 typedef struct display_cache_item {
     guint64                     id;
     gboolean                    lossy;
+    guint32                     ref_count;
 } display_cache_item;
 
-typedef GHashTable display_cache;
+typedef struct display_cache {
+    GHashTable  *table;
+    gboolean    ref_counted;
+}display_cache;
 
 static inline display_cache_item* cache_item_new(guint64 id, gboolean lossy)
 {
     display_cache_item *self = g_slice_new(display_cache_item);
     self->id = id;
     self->lossy = lossy;
+    self->ref_count = 1;
     return self;
 }
 
@@ -46,18 +51,24 @@ static inline void cache_item_free(display_cache_item *self)
 
 static inline display_cache* cache_new(GDestroyNotify value_destroy)
 {
-    GHashTable* self;
-
-    self = g_hash_table_new_full(g_int64_hash, g_int64_equal,
-                                 (GDestroyNotify)cache_item_free,
-                                 value_destroy);
-
+    display_cache * self = g_slice_new(display_cache);
+    self->table = g_hash_table_new_full(g_int64_hash, g_int64_equal,
+                                       (GDestroyNotify) cache_item_free,
+                                       value_destroy);
+    self->ref_counted = FALSE;
     return self;
 }
 
+static inline display_cache * cache_image_new(GDestroyNotify value_destroy)
+{
+    display_cache * self = cache_new(value_destroy);
+    self->ref_counted = TRUE;
+    return self;
+};
+
 static inline gpointer cache_find(display_cache *cache, uint64_t id)
 {
-    return g_hash_table_lookup(cache, &id);
+    return g_hash_table_lookup(cache->table, &id);
 }
 
 static inline gpointer cache_find_lossy(display_cache *cache, uint64_t id, gboolean *lossy)
@@ -65,7 +76,7 @@ static inline gpointer cache_find_lossy(display_cache *cache, uint64_t id, gbool
     gpointer value;
     display_cache_item *item;
 
-    if (!g_hash_table_lookup_extended(cache, &id, (gpointer*)&item, &value))
+    if (!g_hash_table_lookup_extended(cache->table, &id, (gpointer*)&item, &value))
         return NULL;
 
     *lossy = item->lossy;
@@ -77,8 +88,17 @@ static inline void cache_add_lossy(display_cache *cache, uint64_t id,
                                    gpointer value, gboolean lossy)
 {
     display_cache_item *item = cache_item_new(id, lossy);
-
-    g_hash_table_replace(cache, item, value);
+    display_cache_item *current_item;
+    gpointer            current_image;
+
+    //If image is currently in the table add its reference count before replacing it
+    if(cache->ref_counted) {
+        if(g_hash_table_lookup_extended(cache->table, &id, (gpointer*) &current_item,
+                                        (gpointer*) &current_image)) {
+            item->ref_count = current_item->ref_count + 1;
+        }
+    }
+    g_hash_table_replace(cache->table, item, value);
 }
 
 static inline void cache_add(display_cache *cache, uint64_t id, gpointer value)
@@ -88,17 +108,29 @@ static inline void cache_add(display_cache *cache, uint64_t id, gpointer value)
 
 static inline gboolean cache_remove(display_cache *cache, uint64_t id)
 {
-    return g_hash_table_remove(cache, &id);
+    display_cache_item * item;
+    gpointer value;
+
+    if( g_hash_table_lookup_extended(cache->table, &id, (gpointer*) &item, &value)) {
+        --item->ref_count;
+        if(!cache->ref_counted || item->ref_count == 0 ) {
+            return g_hash_table_remove(cache->table, &id);
+        }
+    }
+    else {
+        return FALSE;
+    }
+    return TRUE;
 }
 
 static inline void cache_clear(display_cache *cache)
 {
-    g_hash_table_remove_all(cache);
+    g_hash_table_remove_all(cache->table);
 }
 
 static inline void cache_unref(display_cache *cache)
 {
-    g_hash_table_unref(cache);
+    g_hash_table_unref(cache->table);
 }
 
 G_END_DECLS
diff --git a/src/spice-session.c b/src/spice-session.c
index 0edfa42..f1261fd 100644
--- a/src/spice-session.c
+++ b/src/spice-session.c
@@ -291,7 +291,7 @@ static void spice_session_init(SpiceSession *session)
     g_free(channels);
 
     ring_init(&s->channels);
-    s->images = cache_new((GDestroyNotify)pixman_image_unref);
+    s->images = cache_image_new((GDestroyNotify)pixman_image_unref);
     s->glz_window = glz_decoder_window_new();
     update_proxy(session, NULL);
 }