[Spice-devel] [PATCH spice-gtk 1/2] display: fix palette regression

[Christophe Fergeau]

On Thu, Oct 03, 2013 at 08:51:15AM -0400, Marc-André Lureau wrote:
> 
> 
> ----- Original Message -----
> > On Thu, Oct 03, 2013 at 01:53:55PM +0200, Marc-André Lureau wrote:
> > > palette_get() used to return a ref, and palette_release() used to
> > > release that ref.
> > > 
> > > Since ed877341, the palette is no longer refcount'ed, since its usage is
> > > exclusively local in common/canvas code.
> > > 
> > > palette_release() shouldn't remove the palette from the cache.
> > 
> > Doesn't this mean we will keep some palettes in the cache forever? If yes,
> > some guarantee that the memory used by these palettes is bounded?
> 
> palette_get() / palette_release() always come in pair

Ah right, looks like we can even go with removal then:

diff --git a/common/canvas_base.c b/common/canvas_base.c
index 2753fae..1fb89d2 100644
--- a/common/canvas_base.c
+++ b/common/canvas_base.c
@@ -942,10 +942,6 @@ static pixman_image_t *canvas_get_bits(CanvasBase
*canvas, 
 
     surface = canvas_bitmap_to_surface(canvas, bitmap, palette,
want_original);
 
-    if (palette && (bitmap->flags & SPICE_BITMAP_FLAGS_PAL_FROM_CACHE)) {
-        canvas->palette_cache->ops->release(canvas->palette_cache,
         palette);
-    }
-
     return surface;
 }
 
diff --git a/common/canvas_base.h b/common/canvas_base.h
index 637cdc1..10b855c 100644
--- a/common/canvas_base.h
+++ b/common/canvas_base.h
@@ -77,8 +77,6 @@ typedef struct {
                 SpicePalette *palette);
     SpicePalette *(*get)(SpicePaletteCache *cache,
                          uint64_t id);
-    void (*release)(SpicePaletteCache *cache,
-                    SpicePalette *palette);
 } SpicePaletteCacheOps;
 
 struct _SpicePaletteCache {

diff --git a/gtk/channel-display.c b/gtk/channel-display.c
index 794f4eb..0a91a5e 100644
--- a/gtk/channel-display.c
+++ b/gtk/channel-display.c
@@ -559,11 +559,6 @@ static void palette_remove(SpicePaletteCache *cache,
uint32
     cache_remove(c->palettes, id);
 }
 
-static void palette_release(SpicePaletteCache *cache, SpicePalette
*palette)
-{
-    palette_remove(cache, palette->unique);
-}
-
 #ifdef SW_CANVAS_CACHE
 static void image_put_lossy(SpiceImageCache *cache, uint64_t id,
                             pixman_image_t *surface)
@@ -625,7 +620,6 @@ static SpiceImageCacheOps image_cache_ops = {
 static SpicePaletteCacheOps palette_cache_ops = {
     .put     = palette_put,
     .get     = palette_get,
-    .release = palette_release,
 };


> And a palette is remove from cache with
> SPICE_MSG_DISPLAY_INVAL_{ALL_}PALETTE

Hopefully canvas_get_bits() and handling of
SPICE_MSG_DISPLAY_INVAL_ALL_PALETTE can't race with each other?

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20131003/10df4572/attachment.pgp>