Hi, > Actually, when a new image arrives with an id that is currently in the cache, > it will replace the older one. It will just be created with a +1 reference > count. > > In the case I've been debugging, the image in question is the Windows wallpaper. > As there is a driver instance for each monitor, each driver is sending > down it's own copy of the same image, with the same id. As one channel may be > busier than another there is no way to guarantee the order that the client > will handle commands from the server. What I'm seeing is this: > On the server side > Monitor 1 sends image A > Monitor 1 sends mulitple render commands for image A > Monitor 1 removes image A > Monitor 2 sends image A > Monitor 2 sends render command for image A ... > > What happens on the client end: > Monitor 1 adds Image A added to cache > Monitor 1 starts rendering from image A > Monitor 2 adds Image A to cache- has the same id, so just replaces it > Monitor 1 removes image A - same id, so the id is no longer in the cache > Monitor 2 hangs waiting to render image A - which is no longer in the cache > > In the above sequence, I don't think there is a way to prevent the image from > being sent 2x from the server. What helps keep this to a minimum is to > lock the server cache for the whole of the call to fill_bits. If only the > pixmap_cache_xxx calls are locked it is possible, in the case that two > channels are both executing in the fill_bits call, that the server will end > up sending the same image twice. > I think the above could go in the commit log, very well explained. > I know this seems like a lot of hand waving but I think the basic problem > is handling the Windows Multi-Monitor/multi-channel case where the channels > share a number of common images. > > > > Addresses bug: https://bugzilla.redhat.com/show_bug.cgi?id=1194354 > > > --- > > > src/spice-channel-cache.h | 60 > > > ++++++++++++++++++++++++++++++++++++----------- > > > src/spice-session.c | 2 +- > > > 2 files changed, 47 insertions(+), 15 deletions(-) > > > > > > diff --git a/src/spice-channel-cache.h b/src/spice-channel-cache.h > > > index 17775e6..968e661 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; > > > + uint32_t 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*) ¤t_item, > > > + (gpointer*) ¤t_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 ) { > > > + 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 778d82a..a252448 100644 > > > --- a/src/spice-session.c > > > +++ b/src/spice-session.c > > > @@ -257,7 +257,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); > > > } > > > -- > > > 1.9.5.msysgit.0 > > > > > > _______________________________________________ > > > Spice-devel mailing list > > > Spice-devel@xxxxxxxxxxxxxxxxxxxxx > > > http://lists.freedesktop.org/mailman/listinfo/spice-devel > > > > > > _______________________________________________ > Spice-devel mailing list > Spice-devel@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/spice-devel _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel