[Spice-devel] [PATCH v6 5/7] Use new GlzImageRetention instead of accessing Drawable internals

[Jonathon Jongsma]

On Thu, 2016-06-16 at 10:41 +0100, Frediano Ziglio wrote:
> Remove some coupling, we mainly need to store a list of RedGlzDrawables.
> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  server/dcc-encoders.c    | 12 ++++++------
>  server/dcc-encoders.h    | 14 ++++++++++++--
>  server/display-channel.c |  6 +++---
>  server/display-channel.h |  2 +-
>  4 files changed, 22 insertions(+), 12 deletions(-)
> 
> diff --git a/server/dcc-encoders.c b/server/dcc-encoders.c
> index ea68ac1..42affd5 100644
> --- a/server/dcc-encoders.c
> +++ b/server/dcc-encoders.c
> @@ -70,7 +70,7 @@ struct RedGlzDrawable {
>  #define LINK_TO_GLZ(ptr) SPICE_CONTAINEROF((ptr), RedGlzDrawable, \
>                                             drawable_link)
>  #define DRAWABLE_FOREACH_GLZ_SAFE(drawable, link, next, glz) \
> -    SAFE_FOREACH(link, next, drawable, &(drawable)->glz_ring, glz,
> LINK_TO_GLZ(link))
> +    SAFE_FOREACH(link, next, drawable, &(drawable)->glz_retention.ring, glz,
> LINK_TO_GLZ(link))
>  
>  static void glz_drawable_instance_item_free(GlzDrawableInstanceItem
> *instance);
>  static void encoder_data_init(EncoderData *data);
> @@ -642,20 +642,20 @@ void image_encoders_free_glz_drawables(ImageEncoders
> *enc)
>      pthread_rwlock_unlock(&glz_dict->encode_lock);
>  }
>  
> -void drawable_free_glz_drawables(struct Drawable *drawable)
> +void glz_retention_free_drawables(GlzImageRetention *ret)
>  {
>      RingItem *glz_item, *next_item;
>      RedGlzDrawable *glz;
> -    DRAWABLE_FOREACH_GLZ_SAFE(drawable, glz_item, next_item, glz) {
> +    SAFE_FOREACH(glz_item, next_item, TRUE, &ret->ring, glz,
> LINK_TO_GLZ(glz_item)) {
>          red_glz_drawable_free(glz);
>      }
>  }
>  
> -void drawable_detach_glz_drawables(struct Drawable *drawable)
> +void glz_retention_detach_drawables(GlzImageRetention *ret)
>  {
>      RingItem *item, *next;
>  
> -    RING_FOREACH_SAFE(item, next, &drawable->glz_ring) {
> +    RING_FOREACH_SAFE(item, next, &ret->ring) {
>          SPICE_CONTAINEROF(item, RedGlzDrawable, drawable_link)->has_drawable
> = FALSE;
>          ring_remove(item);
>      }
> @@ -1176,7 +1176,7 @@ static RedGlzDrawable *get_glz_drawable(ImageEncoders
> *enc, Drawable *drawable)
>      ring_item_init(&ret->link);
>      ring_item_init(&ret->drawable_link);
>      ring_add_before(&ret->link, &enc->glz_drawables);
> -    ring_add(&drawable->glz_ring, &ret->drawable_link);
> +    ring_add(&drawable->glz_retention.ring, &ret->drawable_link);
>      enc->shared_data->glz_drawable_count++;
>      return ret;
>  }
> diff --git a/server/dcc-encoders.h b/server/dcc-encoders.h
> index 114dacd..38e6c5a 100644
> --- a/server/dcc-encoders.h
> +++ b/server/dcc-encoders.h
> @@ -36,6 +36,7 @@ typedef struct RedGlzDrawable RedGlzDrawable;
>  typedef struct ImageEncoders ImageEncoders;
>  typedef struct ImageEncoderSharedData ImageEncoderSharedData;
>  typedef struct GlzSharedDictionary GlzSharedDictionary;
> +typedef struct GlzImageRetention GlzImageRetention;
>  
>  void image_encoder_shared_init(ImageEncoderSharedData *shared_data);
>  void image_encoder_shared_stat_reset(ImageEncoderSharedData *shared_data);
> @@ -51,8 +52,8 @@ void image_encoders_glz_get_restore_data(ImageEncoders *enc,
>                                           uint8_t *out_id,
> GlzEncDictRestoreData *out_data);
>  gboolean image_encoders_glz_encode_lock(ImageEncoders *enc);
>  void image_encoders_glz_encode_unlock(ImageEncoders *enc);
> -void drawable_free_glz_drawables(struct Drawable *drawable);
> -void drawable_detach_glz_drawables(struct Drawable *drawable);
> +void glz_retention_free_drawables(GlzImageRetention *ret);
> +void glz_retention_detach_drawables(GlzImageRetention *ret);
>  
>  #define RED_COMPRESS_BUF_SIZE (1024 * 64)
>  struct RedCompressBuf {
> @@ -130,6 +131,15 @@ typedef struct {
>      EncoderData data;
>  } GlzData;
>  
> +struct GlzImageRetention {
> +    Ring ring;
> +};
> +
> +static inline void glz_retention_init(GlzImageRetention *ret)
> +{
> +    ring_init(&ret->ring);
> +}
> +
>  struct ImageEncoderSharedData {
>      uint32_t glz_drawable_count;
>  
> diff --git a/server/display-channel.c b/server/display-channel.c
> index 2366a98..073d45e 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -1179,7 +1179,7 @@ static bool free_one_drawable(DisplayChannel *display,
> int force_glz_free)
>  
>      drawable = SPICE_CONTAINEROF(ring_item, Drawable, list_link);
>      if (force_glz_free) {
> -        drawable_free_glz_drawables(drawable);
> +        glz_retention_free_drawables(&drawable->glz_retention);
>      }
>      drawable_draw(display, drawable);
>      container = drawable->tree_item.base.container;
> @@ -1280,7 +1280,7 @@ static Drawable
> *display_channel_drawable_try_new(DisplayChannel *display,
>      drawable->tree_item.base.type = TREE_ITEM_TYPE_DRAWABLE;
>      region_init(&drawable->tree_item.base.rgn);
>      ring_init(&drawable->pipes);
> -    ring_init(&drawable->glz_ring);
> +    glz_retention_init(&drawable->glz_retention);
>      drawable->process_commands_generation = process_commands_generation;
>  
>      return drawable;
> @@ -1341,7 +1341,7 @@ void drawable_unref(Drawable *drawable)
>      drawable_unref_surface_deps(display, drawable);
>      display_channel_surface_unref(display, drawable->surface_id);
>  
> -    drawable_detach_glz_drawables(drawable);
> +    glz_retention_detach_drawables(&drawable->glz_retention);
>  
>      if (drawable->red_drawable) {
>          red_drawable_unref(drawable->red_drawable);
> diff --git a/server/display-channel.h b/server/display-channel.h
> index 14c0b90..5ccc8cb 100644
> --- a/server/display-channel.h
> +++ b/server/display-channel.h
> @@ -62,7 +62,7 @@ struct Drawable {
>      uint32_t size_pipe_item_rest;
>      RedDrawable *red_drawable;
>  
> -    Ring glz_ring;
> +    GlzImageRetention glz_retention;
>  
>      red_time_t creation_time;
>      red_time_t first_frame_time;


In general, this looks pretty good. The only hesitation that I have is that from
the Drawable point of view, there's no symmetry between adding and removing
items from the GlzImageRetention. In other words, Drawable never explicitly adds
things to the GlzImageRetention structure [1], but it does explicitly "detach"
or "free" things from it. This makes it a bit of a confusing interface. 

[1] items are only added to the GlzImageRetention struct as a side-effect of
calling get_glz_drawable(), whose implementation is opaque to Drawable

***UPDATE***: I just remembered that the next patch changes the
get_glz_drawable() function so that GlzImageRetention is passed as an argument.
This makes it more explicit that items are potentially being added to
GlzImageRetention as a side-effect of that function. So I think that satisfies
my concerns. I wonder if we shouldn't just squash that change into this one.
Doesn't matter too much.

Acked-by: Jonathon Jongsma <jjongsma at redhat.com>