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