> > > > > > > > From: 乐义华 <yueyihua@xxxxxxxxxxx> > > > > > > https://gitlab.freedesktop.org/spice/spice-gtk/issues/72 > > > --- > > > src/channel-display.c | 5 ++++- > > > src/spice-channel-cache.h | 8 ++++++++ > > > 2 files changed, 12 insertions(+), 1 deletion(-) > > > > > > diff --git a/src/channel-display.c b/src/channel-display.c > > > index 44ba043..138cd8c 100644 > > > --- a/src/channel-display.c > > > +++ b/src/channel-display.c > > > @@ -788,7 +788,10 @@ static void image_put_lossy(SpiceImageCache *cache, > > > uint64_t id, > > > static void image_replace_lossy(SpiceImageCache *cache, uint64_t id, > > > pixman_image_t *surface) > > > { > > > - image_put(cache, id, surface); > > > + SpiceDisplayChannelPrivate *c = > > > + SPICE_CONTAINEROF(cache, SpiceDisplayChannelPrivate, > > > image_cache); > > > + > > > + cache_replace_lossy(c->images, id, pixman_image_ref(surface), > > > FALSE); > > > } > > > > > > > Maybe just add a cache_remove before image_put? > > image_put always uses a cache_add which add a reference so creating the > > leak > > as apparently image_replace_lossy is supposed to replace, not add a > > reference > > to the cache. > > In theory if the reference count of that item is already, for instance, 2 > > calling g_hash_table_replace directly will ignore the reference counting. > > Adding the call to cache_remove and then image_put will keep the > > reference count to 2. > > > > > static pixman_image_t* image_get_lossless(SpiceImageCache *cache, > > > uint64_t > > > id) > > > diff --git a/src/spice-channel-cache.h b/src/spice-channel-cache.h > > > index 75cc2cd..1df8f34 100644 > > > --- a/src/spice-channel-cache.h > > > +++ b/src/spice-channel-cache.h > > > @@ -101,6 +101,14 @@ static inline void cache_add_lossy(display_cache > > > *cache, > > > uint64_t id, > > > g_hash_table_replace(cache->table, item, value); > > > } > > > > > > +static inline void cache_replace_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->table, item, value); > > > +} > > > + > > > static inline void cache_add(display_cache *cache, uint64_t id, gpointer > > > value) > > > { > > > cache_add_lossy(cache, id, value, FALSE); > > > > Do we really need to use the reference counting on the image cache? > Looking at protocol and spice-gtk source I don't see the reason. > cache_add always call a g_hash_table_replace while cache_remove is only > called when server requires specific invalidate. I cannot imagine a > sequence of commands leading to a cache_remove that should not remove > the id from the cache. > After server send a CACHE_ME of an already existing ID all following > images with that ID are referring to last image inserted with that ID > so why server should send a INVAL_LIST with the ID referring to the old > ID and not the new one? > Ok, found the reason. The IDs to be removed are cached in a list and then sent as a batch. So you could have a sequence like - cached image 1 - remove image 1 - cached image 1 Which will lead to these messages - image 1 - image 1 - remove (INVAL_LIST) 1 If you remove the image from cache at remove you will remove an image still valid. Potentially could be optimized on the server but for compatibility we need to support this case on the client. I'll propose an updated patch from the initial one. Frediano _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel