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

[Marc-André Lureau]

----- Original Message -----
> 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:

Sure, but you'll have to fix server code too 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?

Not in spice-gtk, since canvas operations are not async/reordered or multi-threaded.
> Christophe
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>