Re: [PATCH spice-server 1/2] Refactory Glz RedDrawable retention code

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



ping

this patch is about 6 months old

Frediano

----- Original Message -----
> From: "Frediano Ziglio" <fziglio@xxxxxxxxxx>
> To: spice-devel@xxxxxxxxxxxxxxxxxxxxx
> Cc: "Frediano Ziglio" <fziglio@xxxxxxxxxx>
> Sent: Tuesday, October 18, 2016 10:28:25 AM
> Subject: [PATCH spice-server 1/2] Refactory Glz RedDrawable retention code
> 
> The code was quite complicated.
> - a GlzEncDictImageContext was bound to a GlzDrawableInstanceItem
>   (1-1 relation);
> - multiple GlzDrawableInstanceItem were attached to RedGlzDrawable;
> - multiple RedGlzDrawable (one for RedClient) were attached to Drawable
>   using GlzImageRetention;
> - OOM code require all Glz dictionary used by DisplayChannel to be locked
>   at the same time;
> - drawable count computation during OOM was not corrent in case of
>   multiple clients;
> - not clear why to call glz_retention_free_drawables or
>   glz_retention_detach_drawables.
> 
> Now:
> - a GlzEncDictImageContext is bound to a GlzDictItem (still 1-1 relation);
> - multiple GlzDictItem are attached to a Drawable using GlzImageRetention;
> - there is only a glz_retention_free to be called when the structure
>   must be destroyed.
> 
> Implementation detail:
> - red_drawable_unref returns a gboolean to indicate if the object
>   was really freed;
> - glz_drawable_count was removed as there are no more RedGlzDrawable;
> - image_encoders_glz_encode_lock and image_encoders_glz_encode_unlock
>   were removed and now the locking is handled entirely by ImageEncoders;
> - instead of marking GlzDictItem/GlzDrawableInstanceItem changing
>   has_drawable flag contexts are moved to a glz_orphans ring;
> - code is smaller (100 lines less in image-encoders.c) and I hope easier
>   to read.
> 
> Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> ---
>  server/display-channel.c |  57 +++++----
>  server/image-encoders.c  | 300
>  +++++++++++++++--------------------------------
>  server/image-encoders.h  |  27 +++--
>  server/red-worker.c      |   5 +-
>  server/red-worker.h      |   2 +-
>  5 files changed, 145 insertions(+), 246 deletions(-)
> 
> diff --git a/server/display-channel.c b/server/display-channel.c
> index 69edd35..9a56080 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -1185,7 +1185,7 @@ void display_channel_free_glz_drawables(DisplayChannel
> *display)
>      }
>  }
>  
> -static bool free_one_drawable(DisplayChannel *display, int force_glz_free)
> +static bool free_one_drawable(DisplayChannel *display)
>  {
>      RingItem *ring_item = ring_get_tail(&display->priv->current_list);
>      Drawable *drawable;
> @@ -1196,9 +1196,6 @@ static bool free_one_drawable(DisplayChannel *display,
> int force_glz_free)
>      }
>  
>      drawable = SPICE_CONTAINEROF(ring_item, Drawable, list_link);
> -    if (force_glz_free) {
> -        glz_retention_free_drawables(&drawable->glz_retention);
> -    }
>      drawable_draw(display, drawable);
>      container = drawable->tree_item.base.container;
>  
> @@ -1210,37 +1207,47 @@ static bool free_one_drawable(DisplayChannel
> *display, int force_glz_free)
>  void display_channel_current_flush(DisplayChannel *display, int surface_id)
>  {
>      while
>      (!ring_is_empty(&display->priv->surfaces[surface_id].current_list)) {
> -        free_one_drawable(display, FALSE);
> +        free_one_drawable(display);
>      }
>      current_remove_all(display, surface_id);
>  }
>  
>  void display_channel_free_some(DisplayChannel *display)
>  {
> -    int n = 0;
> +    int left_to_free = RED_RELEASE_BUNCH_SIZE;
>      DisplayChannelClient *dcc;
>      GListIter iter;
>  
> -    spice_debug("#draw=%d, #glz_draw=%d", display->priv->drawable_count,
> -                display->priv->encoder_shared_data.glz_drawable_count);
> +    /* As we are trying to release some memory as requested by guest driver
> +     * loop througth Glz freeing drawables which have no corresponding
> Drawable
> +     * as this will cause guest memory to be freed. */
> +    spice_debug("#draw=%d", display->priv->drawable_count);
>      FOREACH_DCC(display, iter, dcc) {
>          ImageEncoders *encoders = dcc_get_encoders(dcc);
>  
> -        // encoding using the dictionary is prevented since the following
> operations might
> -        // change the dictionary
> -        if (image_encoders_glz_encode_lock(encoders)) {
> -            n =
> image_encoders_free_some_independent_glz_drawables(encoders);
> -        }
> +        left_to_free -=
> image_encoders_free_some_independent_glz_drawables(encoders, left_to_free);
>      }
>  
> -    while (!ring_is_empty(&display->priv->current_list) && n++ <
> RED_RELEASE_BUNCH_SIZE) {
> -        free_one_drawable(display, TRUE);
> -    }
> +    if (left_to_free > 0) {
> +        int saved_to_free = left_to_free;
>  
> -    FOREACH_DCC(display, iter, dcc) {
> -        ImageEncoders *encoders = dcc_get_encoders(dcc);
> +        while (left_to_free > 0 &&
> !ring_is_empty(&display->priv->current_list)) {
> +            free_one_drawable(display);
> +            --left_to_free;
> +        }
>  
> -        image_encoders_glz_encode_unlock(encoders);
> +        /* We freed Drawables in the loop above but the underlaying
> RedDrawables
> +         * could still be referenced by Glz so scan again the Glz for
> independent
> +         * drawables. Note that there is a 1-to-1 relatioship between
> Drawable
> +         * and RedDrawable so in theory there will be a maximum of
> saved_to_free
> +         * RedDrawables to free. Use this limit in any case, this should not
> be
> +         * a problem and will make sure we won't free more items that we are
> +         * supposed to free. */
> +        left_to_free = saved_to_free;
> +        FOREACH_DCC(display, iter, dcc) {
> +            ImageEncoders *encoders = dcc_get_encoders(dcc);
> +            left_to_free -=
> image_encoders_free_some_independent_glz_drawables(encoders, left_to_free);
> +        }
>      }
>  }
>  
> @@ -1285,7 +1292,7 @@ static Drawable
> *display_channel_drawable_try_new(DisplayChannel *display,
>      Drawable *drawable;
>  
>      while (!(drawable = drawable_try_new(display))) {
> -        if (!free_one_drawable(display, FALSE))
> +        if (!free_one_drawable(display))
>              return NULL;
>      }
>  
> @@ -1362,7 +1369,7 @@ void drawable_unref(Drawable *drawable)
>      drawable_unref_surface_deps(display, drawable);
>      display_channel_surface_unref(display, drawable->surface_id);
>  
> -    glz_retention_detach_drawables(&drawable->glz_retention);
> +    glz_retention_destroy(&drawable->glz_retention);
>  
>      if (drawable->red_drawable) {
>          red_drawable_unref(drawable->red_drawable);
> @@ -1866,9 +1873,8 @@ static void on_disconnect(RedChannelClient *rcc)
>      display_channel_compress_stats_print(display);
>  
>      // this was the last channel client
> -    spice_debug("#draw=%d, #glz_draw=%d",
> -                display->priv->drawable_count,
> -                display->priv->encoder_shared_data.glz_drawable_count);
> +    spice_debug("#draw=%d",
> +                display->priv->drawable_count);
>  }
>  
>  static int handle_migrate_flush_mark(RedChannelClient *rcc)
> @@ -2117,10 +2123,9 @@ void display_channel_debug_oom(DisplayChannel
> *display, const char *msg)
>  {
>      RedChannel *channel = RED_CHANNEL(display);
>  
> -    spice_debug("%s #draw=%u, #glz_draw=%u current %u pipes %u",
> +    spice_debug("%s #draw=%u, current %u pipes %u",
>                  msg,
>                  display->priv->drawable_count,
> -                display->priv->encoder_shared_data.glz_drawable_count,
>                  display->priv->current_size,
>                  red_channel_sum_pipes_size(channel));
>  }
> diff --git a/server/image-encoders.c b/server/image-encoders.c
> index 39aca6c..951fe96 100644
> --- a/server/image-encoders.c
> +++ b/server/image-encoders.c
> @@ -30,11 +30,6 @@
>  
>  #define ENCODER_MESSAGE_SIZE 512
>  
> -#define MAX_GLZ_DRAWABLE_INSTANCES 2
> -
> -typedef struct RedGlzDrawable RedGlzDrawable;
> -typedef struct GlzDrawableInstanceItem GlzDrawableInstanceItem;
> -
>  struct GlzSharedDictionary {
>      GlzEncDictContext *dict;
>      uint32_t refs;
> @@ -44,37 +39,28 @@ struct GlzSharedDictionary {
>      RedClient *client; // channel clients of the same client share the dict
>  };
>  
> -/* for each qxl drawable, there may be several instances of lz drawables */
> -/* TODO - reuse this stuff for the top level. I just added a second level of
> multiplicity
> - * at the Drawable by keeping a ring, so:
> - * Drawable -> (ring of) RedGlzDrawable -> (up to 2) GlzDrawableInstanceItem
> - * and it should probably (but need to be sure...) be
> - * Drawable -> ring of GlzDrawableInstanceItem.
> +/**
> + * This structure is used to retain RedDrawable objects so they don't
> + * disappear while we are using their memory.
> + * This is because Glz will keep using image memory for future compressions.
>   */
> -struct GlzDrawableInstanceItem {
> -    RingItem glz_link;
> +typedef struct GlzDictItem {
> +    /** linked to ImageEncoders::glz_dict_items. Always inserted */
> +    RingItem glz_context_link;
> +    /** linked to GlzImageRetention::ring or ImageEncoders::orphans. Always
> inserted */
> +    RingItem retention_link;
> +    /** linked to ImageEncoders::glz_dict_items_to_free */
>      RingItem free_link;
> -    GlzEncDictImageContext *context;
> -    RedGlzDrawable         *glz_drawable;
> -};
> -
> -struct RedGlzDrawable {
> -    RingItem link;    // ordered by the time it was encoded
> -    RingItem drawable_link;
> +    /** context pointer from Glz encoder, used to free the drawable */
> +    GlzEncDictImageContext *dict_image_context;
> +    /** where the drawable came from */
> +    ImageEncoders *enc;
> +    /** hold a reference to this object, this to make sure Glz
> +     *  can continue to use its memory */
>      RedDrawable *red_drawable;
> -    GlzDrawableInstanceItem instances_pool[MAX_GLZ_DRAWABLE_INSTANCES];
> -    Ring instances;
> -    uint8_t instances_count;
> -    gboolean has_drawable;
> -    ImageEncoders *encoders;
> -};
> -
> -#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_retention.ring, glz,
> LINK_TO_GLZ(link))
> +} GlzDictItem;
>  
> -static void glz_drawable_instance_item_free(GlzDrawableInstanceItem
> *instance);
> +static gboolean glz_context_free(GlzDictItem *glz_item);
>  static void encoder_data_init(EncoderData *data);
>  static void encoder_data_reset(EncoderData *data);
>  static void image_encoders_release_glz(ImageEncoders *enc);
> @@ -381,23 +367,24 @@ static void image_encoders_init_lz(ImageEncoders *enc)
>  static void glz_usr_free_image(GlzEncoderUsrContext *usr, GlzUsrImageContext
>  *image)
>  {
>      GlzData *lz_data = (GlzData *)usr;
> -    GlzDrawableInstanceItem *glz_drawable_instance =
> (GlzDrawableInstanceItem *)image;
> -    ImageEncoders *drawable_enc =
> glz_drawable_instance->glz_drawable->encoders;
> +    GlzDictItem *glz_item = (GlzDictItem *)image;
> +    ImageEncoders *drawable_enc = glz_item->enc;
>      ImageEncoders *this_enc = SPICE_CONTAINEROF(lz_data, ImageEncoders,
>      glz_data);
> +    glz_item->dict_image_context = NULL;
>      if (this_enc == drawable_enc) {
> -        glz_drawable_instance_item_free(glz_drawable_instance);
> +        glz_context_free(glz_item);
>      } else {
>          /* The glz dictionary is shared between all DisplayChannelClient
> -         * instances that belong to the same client, and glz_usr_free_image
> +         * items that belong to the same client, and glz_usr_free_image
>           * can be called by the dictionary code
>           * (glz_dictionary_window_remove_head). Thus this function can be
>           * called from any DisplayChannelClient thread, hence the need for
>           * this check.
>           */
> -        pthread_mutex_lock(&drawable_enc->glz_drawables_inst_to_free_lock);
> -        ring_add_before(&glz_drawable_instance->free_link,
> -                        &drawable_enc->glz_drawables_inst_to_free);
> -
> pthread_mutex_unlock(&drawable_enc->glz_drawables_inst_to_free_lock);
> +        pthread_mutex_lock(&drawable_enc->glz_dict_items_to_free_lock);
> +        ring_add_before(&glz_item->free_link,
> +                        &drawable_enc->glz_dict_items_to_free);
> +        pthread_mutex_unlock(&drawable_enc->glz_dict_items_to_free_lock);
>      }
>  }
>  
> @@ -456,9 +443,10 @@ void image_encoders_init(ImageEncoders *enc,
> ImageEncoderSharedData *shared_data
>      spice_assert(shared_data);
>      enc->shared_data = shared_data;
>  
> -    ring_init(&enc->glz_drawables);
> -    ring_init(&enc->glz_drawables_inst_to_free);
> -    pthread_mutex_init(&enc->glz_drawables_inst_to_free_lock, NULL);
> +    ring_init(&enc->glz_orphans);
> +    ring_init(&enc->glz_dict_items);
> +    ring_init(&enc->glz_dict_items_to_free);
> +    pthread_mutex_init(&enc->glz_dict_items_to_free_lock, NULL);
>  
>      image_encoders_init_glz_data(enc);
>      image_encoders_init_quic(enc);
> @@ -488,121 +476,63 @@ void image_encoders_free(ImageEncoders *enc)
>  #endif
>      zlib_encoder_destroy(enc->zlib);
>      enc->zlib = NULL;
> -    pthread_mutex_destroy(&enc->glz_drawables_inst_to_free_lock);
> +    pthread_mutex_destroy(&enc->glz_dict_items_to_free_lock);
>  }
>  
>  /* Remove from the to_free list and the instances_list.
> -   When no instance is left - the RedGlzDrawable is released too. (and the
> qxl drawable too, if
> -   it is not used by Drawable).
> -   NOTE - 1) can be called only by the display channel that created the
> drawable
> -          2) it is assumed that the instance was already removed from the
> dictionary*/
> -static void glz_drawable_instance_item_free(GlzDrawableInstanceItem
> *instance)
> +   Returns TRUE if a RedDrawable was freed.
> +   NOTE - can be called only by the display channel that created the
> drawable */
> +static gboolean glz_context_free(GlzDictItem *glz_item)
>  {
> -    RedGlzDrawable *glz_drawable;
> -
> -    spice_assert(instance);
> -    spice_assert(instance->glz_drawable);
> +    gboolean ret;
>  
> -    glz_drawable = instance->glz_drawable;
> +    spice_assert(glz_item);
>  
> -    spice_assert(glz_drawable->instances_count > 0);
> -
> -    ring_remove(&instance->glz_link);
> -    glz_drawable->instances_count--;
> +    ring_remove(&glz_item->glz_context_link);
> +    ring_remove(&glz_item->retention_link);
>  
>      // when the remove callback is performed from the channel that the
> -    // drawable belongs to, the instance is not added to the 'to_free' list
> -    if (ring_item_is_linked(&instance->free_link)) {
> -        ring_remove(&instance->free_link);
> -    }
> -
> -    if (ring_is_empty(&glz_drawable->instances)) {
> -        spice_assert(glz_drawable->instances_count == 0);
> -
> -        if (glz_drawable->has_drawable) {
> -            ring_remove(&glz_drawable->drawable_link);
> -        }
> -        red_drawable_unref(glz_drawable->red_drawable);
> -        glz_drawable->encoders->shared_data->glz_drawable_count--;
> -        if (ring_item_is_linked(&glz_drawable->link)) {
> -            ring_remove(&glz_drawable->link);
> -        }
> -        free(glz_drawable);
> -    }
> -}
> -
> -/*
> - * Releases all the instances of the drawable from the dictionary and the
> display channel client.
> - * The release of the last instance will also release the drawable itself
> and the qxl drawable
> - * if possible.
> - * NOTE - the caller should prevent encoding using the dictionary during
> this operation
> - */
> -static void red_glz_drawable_free(RedGlzDrawable *glz_drawable)
> -{
> -    ImageEncoders *enc = glz_drawable->encoders;
> -    RingItem *head_instance = ring_get_head(&glz_drawable->instances);
> -    int cont = (head_instance != NULL);
> -
> -    while (cont) {
> -        if (glz_drawable->instances_count == 1) {
> -            /* Last instance: glz_drawable_instance_item_free will free the
> glz_drawable */
> -            cont = FALSE;
> -        }
> -        GlzDrawableInstanceItem *instance = SPICE_CONTAINEROF(head_instance,
> -
> GlzDrawableInstanceItem,
> -                                                        glz_link);
> -        if (!ring_item_is_linked(&instance->free_link)) {
> -            // the instance didn't get out from window yet
> -            glz_enc_dictionary_remove_image(enc->glz_dict->dict,
> -                                            instance->context,
> -                                            &enc->glz_data.usr);
> -        }
> -        glz_drawable_instance_item_free(instance);
> -
> -        if (cont) {
> -            head_instance = ring_get_head(&glz_drawable->instances);
> -        }
> +    // drawable belongs to, the item is not added to the 'to_free' list
> +    if (ring_item_is_linked(&glz_item->free_link)) {
> +        ring_remove(&glz_item->free_link);
> +    } else if (glz_item->dict_image_context) {
> +        ImageEncoders *enc = glz_item->enc;
> +
> +        // the item didn't get out from window yet
> +        glz_enc_dictionary_remove_image(enc->glz_dict->dict,
> +                                        glz_item->dict_image_context,
> +                                        &enc->glz_data.usr);
>      }
> -}
>  
> -gboolean image_encoders_glz_encode_lock(ImageEncoders *enc)
> -{
> -    if (enc->glz_dict) {
> -        pthread_rwlock_wrlock(&enc->glz_dict->encode_lock);
> -        return TRUE;
> -    }
> -    return FALSE;
> -}
> +    ret = red_drawable_unref(glz_item->red_drawable);
> +    free(glz_item);
>  
> -void image_encoders_glz_encode_unlock(ImageEncoders *enc)
> -{
> -    if (enc->glz_dict) {
> -        pthread_rwlock_unlock(&enc->glz_dict->encode_lock);
> -    }
> +    return ret;
>  }
>  
>  /*
> - * Remove from the global lz dictionary some glz_drawables that have no
> reference to
> + * Remove from the global lz dictionary some glz_dict_items that have no
> reference to
>   * Drawable (their qxl drawables are released too).
> - * NOTE - the caller should prevent encoding using the dictionary during the
> operation
>   */
> -int image_encoders_free_some_independent_glz_drawables(ImageEncoders *enc)
> +int image_encoders_free_some_independent_glz_drawables(ImageEncoders *enc,
> int max_to_free)
>  {
>      RingItem *ring_link;
>      int n = 0;
>  
> -    if (!enc) {
> -        return 0;
> +    if (!enc || !enc->glz_dict || max_to_free <= 0) {
> +        return n;
>      }
> -    ring_link = ring_get_head(&enc->glz_drawables);
> -    while ((n < RED_RELEASE_BUNCH_SIZE) && (ring_link != NULL)) {
> -        RedGlzDrawable *glz_drawable = SPICE_CONTAINEROF(ring_link,
> RedGlzDrawable, link);
> -        ring_link = ring_next(&enc->glz_drawables, ring_link);
> -        if (!glz_drawable->has_drawable) {
> -            red_glz_drawable_free(glz_drawable);
> -            n++;
> +
> +    // prevent encoding using the dictionary during the operation
> +    pthread_rwlock_wrlock(&enc->glz_dict->encode_lock);
> +    while (max_to_free > n
> +           && (ring_link = ring_get_head(&enc->glz_orphans)) != NULL) {
> +        GlzDictItem *glz_item = SPICE_CONTAINEROF(ring_link, GlzDictItem,
> retention_link);
> +        if (glz_context_free(glz_item)) {
> +            ++n;
>          }
>      }
> +    pthread_rwlock_unlock(&enc->glz_dict->encode_lock);
>      return n;
>  }
>  
> @@ -613,14 +543,12 @@ void
> image_encoders_free_glz_drawables_to_free(ImageEncoders* enc)
>      if (!enc->glz_dict) {
>          return;
>      }
> -    pthread_mutex_lock(&enc->glz_drawables_inst_to_free_lock);
> -    while ((ring_link = ring_get_head(&enc->glz_drawables_inst_to_free))) {
> -        GlzDrawableInstanceItem *drawable_instance =
> SPICE_CONTAINEROF(ring_link,
> -
> GlzDrawableInstanceItem,
> -                                                                 free_link);
> -        glz_drawable_instance_item_free(drawable_instance);
> +    pthread_mutex_lock(&enc->glz_dict_items_to_free_lock);
> +    while ((ring_link = ring_get_head(&enc->glz_dict_items_to_free))) {
> +        GlzDictItem * glz_item = SPICE_CONTAINEROF(ring_link, GlzDictItem,
> free_link);
> +        glz_context_free(glz_item);
>      }
> -    pthread_mutex_unlock(&enc->glz_drawables_inst_to_free_lock);
> +    pthread_mutex_unlock(&enc->glz_dict_items_to_free_lock);
>  }
>  
>  /* Clear all lz drawables - enforce their removal from the global
>  dictionary.
> @@ -636,31 +564,25 @@ void image_encoders_free_glz_drawables(ImageEncoders
> *enc)
>  
>      // assure no display channel is during global lz encoding
>      pthread_rwlock_wrlock(&glz_dict->encode_lock);
> -    while ((ring_link = ring_get_head(&enc->glz_drawables))) {
> -        RedGlzDrawable *drawable = SPICE_CONTAINEROF(ring_link,
> RedGlzDrawable, link);
> +    while ((ring_link = ring_get_head(&enc->glz_dict_items))) {
> +        GlzDictItem *glz_item = SPICE_CONTAINEROF(ring_link, GlzDictItem,
> glz_context_link);
>          // no need to lock the to_free list, since we assured no other
>          thread is encoding and
>          // thus not other thread access the to_free list of the channel
> -        red_glz_drawable_free(drawable);
> +        glz_context_free(glz_item);
>      }
>      pthread_rwlock_unlock(&glz_dict->encode_lock);
>  }
>  
> -void glz_retention_free_drawables(GlzImageRetention *ret)
> +void glz_retention_destroy(GlzImageRetention *retention)
>  {
> -    RingItem *glz_item, *next_item;
> -    RedGlzDrawable *glz;
> -    SAFE_FOREACH(glz_item, next_item, TRUE, &ret->ring, glz,
> LINK_TO_GLZ(glz_item)) {
> -        red_glz_drawable_free(glz);
> -    }
> -}
> +    RingItem *head;
>  
> -void glz_retention_detach_drawables(GlzImageRetention *ret)
> -{
> -    RingItem *item, *next;
> +    /* append all ring to orphans one */
> +    while ((head = ring_get_head(&retention->ring)) != NULL) {
> +        GlzDictItem *glz_item = SPICE_CONTAINEROF(head, GlzDictItem,
> retention_link);
>  
> -    RING_FOREACH_SAFE(item, next, &ret->ring) {
> -        SPICE_CONTAINEROF(item, RedGlzDrawable, drawable_link)->has_drawable
> = FALSE;
> -        ring_remove(item);
> +        ring_remove(head);
> +        ring_add_before(head, &glz_item->enc->glz_orphans);
>      }
>  }
>  
> @@ -1153,52 +1075,22 @@ int image_encoders_compress_lz4(ImageEncoders *enc,
> SpiceImage *dest,
>  
>  /* if already exists, returns it. Otherwise allocates and adds it (1) to the
>  ring tail
>     in the channel (2) to the Drawable*/
> -static RedGlzDrawable *get_glz_drawable(ImageEncoders *enc, RedDrawable
> *red_drawable,
> -                                        GlzImageRetention *glz_retention)
> +static GlzDictItem *get_glz_context(ImageEncoders *enc, RedDrawable
> *red_drawable,
> +                                   GlzImageRetention *glz_retention)
>  {
> -    RedGlzDrawable *ret;
> -    RingItem *item, *next;
> -
> -    // TODO - I don't really understand what's going on here, so doing the
> technical equivalent
> -    // now that we have multiple glz_dicts, so the only way to go from dcc
> to drawable glz is to go
> -    // over the glz_ring (unless adding some better data structure then a
> ring)
> -    SAFE_FOREACH(item, next, TRUE, &glz_retention->ring, ret,
> LINK_TO_GLZ(item)) {
> -        if (ret->encoders == enc) {
> -            return ret;
> -        }
> -    }
> -
> -    ret = spice_new(RedGlzDrawable, 1);
> -
> -    ret->encoders = enc;
> -    ret->red_drawable = red_drawable_ref(red_drawable);
> -    ret->has_drawable = TRUE;
> -    ret->instances_count = 0;
> -    ring_init(&ret->instances);
> -
> -    ring_item_init(&ret->link);
> -    ring_item_init(&ret->drawable_link);
> -    ring_add_before(&ret->link, &enc->glz_drawables);
> -    ring_add(&glz_retention->ring, &ret->drawable_link);
> -    enc->shared_data->glz_drawable_count++;
> -    return ret;
> -}
> +    GlzDictItem *ret;
>  
> -/* allocates new instance and adds it to instances in the given drawable.
> -   NOTE - the caller should set the glz_instance returned by the encoder by
> itself.*/
> -static GlzDrawableInstanceItem *add_glz_drawable_instance(RedGlzDrawable
> *glz_drawable)
> -{
> -    spice_assert(glz_drawable->instances_count <
> MAX_GLZ_DRAWABLE_INSTANCES);
> -    // NOTE: We assume the additions are performed consecutively, without
> removals in the middle
> -    GlzDrawableInstanceItem *ret = glz_drawable->instances_pool +
> glz_drawable->instances_count;
> -    glz_drawable->instances_count++;
> +    ret = spice_new(GlzDictItem, 1);
>  
> +    ring_item_init(&ret->glz_context_link);
> +    ring_item_init(&ret->retention_link);
>      ring_item_init(&ret->free_link);
> -    ring_item_init(&ret->glz_link);
> -    ring_add(&glz_drawable->instances, &ret->glz_link);
> -    ret->context = NULL;
> -    ret->glz_drawable = glz_drawable;
>  
> +    ret->enc = enc;
> +    ret->red_drawable = red_drawable_ref(red_drawable);
> +
> +    ring_add_before(&ret->glz_context_link, &enc->glz_dict_items);
> +    ring_add(&glz_retention->ring, &ret->retention_link);
>      return ret;
>  }
>  
> @@ -1217,8 +1109,7 @@ int image_encoders_compress_glz(ImageEncoders *enc,
>      GlzData *glz_data = &enc->glz_data;
>      ZlibData *zlib_data;
>      LzImageType type = bitmap_fmt_to_lz_image_type[src->format];
> -    RedGlzDrawable *glz_drawable;
> -    GlzDrawableInstanceItem *glz_drawable_instance;
> +    GlzDictItem *glz_item;
>      int glz_size;
>      int zlib_size;
>  
> @@ -1239,8 +1130,7 @@ int image_encoders_compress_glz(ImageEncoders *enc,
>  
>      encoder_data_init(&glz_data->data);
>  
> -    glz_drawable = get_glz_drawable(enc, red_drawable, glz_retention);
> -    glz_drawable_instance = add_glz_drawable_instance(glz_drawable);
> +    glz_item = get_glz_context(enc, red_drawable, glz_retention);
>  
>      glz_data->data.u.lines_data.chunks = src->data;
>      glz_data->data.u.lines_data.stride = src->stride;
> @@ -1251,8 +1141,8 @@ int image_encoders_compress_glz(ImageEncoders *enc,
>                            (src->flags & SPICE_BITMAP_FLAGS_TOP_DOWN), NULL,
>                            0,
>                            src->stride, glz_data->data.bufs_head->buf.bytes,
>                            sizeof(glz_data->data.bufs_head->buf),
> -                          glz_drawable_instance,
> -                          &glz_drawable_instance->context);
> +                          glz_item,
> +                          &glz_item->dict_image_context);
>  
>      stat_compress_add(&enc->shared_data->glz_stat, start_time, src->stride *
>      src->y, glz_size);
>  
> diff --git a/server/image-encoders.h b/server/image-encoders.h
> index 6542edd..4bac606 100644
> --- a/server/image-encoders.h
> +++ b/server/image-encoders.h
> @@ -45,16 +45,18 @@ void image_encoder_shared_stat_print(const
> ImageEncoderSharedData *shared_data);
>  
>  void image_encoders_init(ImageEncoders *enc, ImageEncoderSharedData
>  *shared_data);
>  void image_encoders_free(ImageEncoders *enc);
> -int image_encoders_free_some_independent_glz_drawables(ImageEncoders *enc);
> +int image_encoders_free_some_independent_glz_drawables(ImageEncoders *enc,
> int max_to_free);
>  void image_encoders_free_glz_drawables(ImageEncoders *enc);
>  void image_encoders_free_glz_drawables_to_free(ImageEncoders* enc);
>  gboolean image_encoders_glz_create(ImageEncoders *enc, uint8_t id);
>  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 glz_retention_free_drawables(GlzImageRetention *ret);
> -void glz_retention_detach_drawables(GlzImageRetention *ret);
> +
> +/**
> + * Free retention structure so Glz code that drawables
> + * are no more owned by something else.
> + */
> +void glz_retention_destroy(GlzImageRetention *retention);
>  
>  #define RED_COMPRESS_BUF_SIZE (1024 * 64)
>  struct RedCompressBuf {
> @@ -137,14 +139,12 @@ struct GlzImageRetention {
>      Ring ring;
>  };
>  
> -static inline void glz_retention_init(GlzImageRetention *ret)
> +static inline void glz_retention_init(GlzImageRetention *retention)
>  {
> -    ring_init(&ret->ring);
> +    ring_init(&retention->ring);
>  }
>  
>  struct ImageEncoderSharedData {
> -    uint32_t glz_drawable_count;
> -
>      stat_info_t off_stat;
>      stat_info_t lz_stat;
>      stat_info_t glz_stat;
> @@ -184,9 +184,12 @@ struct ImageEncoders {
>      GlzEncoderContext *glz;
>      GlzData glz_data;
>  
> -    Ring glz_drawables;               // all the living lz drawable, ordered
> by encoding time
> -    Ring glz_drawables_inst_to_free;               // list of instances to
> be freed
> -    pthread_mutex_t glz_drawables_inst_to_free_lock;
> +    /** list to all Glz contexts that are owned only by Glz and could
> +     *  be freed in case of OOM */
> +    Ring glz_orphans;
> +    Ring glz_dict_items;               // all the living lz drawable,
> ordered by encoding time
> +    Ring glz_dict_items_to_free;               // list of items to be freed
> +    pthread_mutex_t glz_dict_items_to_free_lock;
>  };
>  
>  typedef struct compress_send_data_t {
> diff --git a/server/red-worker.c b/server/red-worker.c
> index 678856b..b419f91 100644
> --- a/server/red-worker.c
> +++ b/server/red-worker.c
> @@ -97,14 +97,15 @@ static int display_is_connected(RedWorker *worker)
>          &worker->display_channel->common.base));
>  }
>  
> -void red_drawable_unref(RedDrawable *red_drawable)
> +gboolean red_drawable_unref(RedDrawable *red_drawable)
>  {
>      if (--red_drawable->refs) {
> -        return;
> +        return FALSE;
>      }
>      red_qxl_release_resource(red_drawable->qxl,
>      red_drawable->release_info_ext);
>      red_put_drawable(red_drawable);
>      free(red_drawable);
> +    return TRUE;
>  }
>  
>  static int red_process_cursor(RedWorker *worker, int *ring_is_empty)
> diff --git a/server/red-worker.h b/server/red-worker.h
> index dc2ff24..6d3be94 100644
> --- a/server/red-worker.h
> +++ b/server/red-worker.h
> @@ -30,6 +30,6 @@ RedWorker* red_worker_new(QXLInstance *qxl,
>                            const ClientCbs *client_display_cbs);
>  bool       red_worker_run(RedWorker *worker);
>  
> -void red_drawable_unref(RedDrawable *red_drawable);
> +gboolean red_drawable_unref(RedDrawable *red_drawable);
>  
>  #endif
> --
> 2.7.4
> 
> 
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]