[PATCH v2 1/2] Refactory Glz RedDrawable retention code

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

 



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 |  53 +++++----
 server/image-encoders.c  | 300 +++++++++++++++--------------------------------
 server/image-encoders.h  |  27 +++--
 server/red-worker.c      |  11 +-
 server/red-worker.h      |   2 +-
 5 files changed, 146 insertions(+), 247 deletions(-)

diff --git a/server/display-channel.c b/server/display-channel.c
index 073d45e..6d25239 100644
--- a/server/display-channel.c
+++ b/server/display-channel.c
@@ -1167,7 +1167,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->current_list);
     Drawable *drawable;
@@ -1178,9 +1178,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;
 
@@ -1192,33 +1189,44 @@ 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->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;
     GList *link, *next;
 
-    spice_debug("#draw=%d, #glz_draw=%d", display->drawable_count,
-                display->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->drawable_count);
     FOREACH_CLIENT(display, link, next, dcc) {
-        // encoding using the dictionary is prevented since the following operations might
-        // change the dictionary
-        if (image_encoders_glz_encode_lock(&dcc->encoders)) {
-            n = image_encoders_free_some_independent_glz_drawables(&dcc->encoders);
-        }
+        left_to_free -= image_encoders_free_some_independent_glz_drawables(&dcc->encoders, left_to_free);
     }
 
-    while (!ring_is_empty(&display->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_CLIENT(display, link, next, dcc) {
-        image_encoders_glz_encode_unlock(&dcc->encoders);
+        while (left_to_free > 0 && !ring_is_empty(&display->current_list)) {
+            free_one_drawable(display);
+            --left_to_free;
+        }
+
+        /* 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_CLIENT(display, link, next, dcc) {
+            left_to_free -= image_encoders_free_some_independent_glz_drawables(&dcc->encoders, left_to_free);
+        }
     }
 }
 
@@ -1263,7 +1271,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;
     }
 
@@ -1341,7 +1349,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);
@@ -1845,9 +1853,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->drawable_count,
-                display->encoder_shared_data.glz_drawable_count);
+    spice_debug("#draw=%d",
+                display->drawable_count);
 }
 
 static int handle_migrate_flush_mark(RedChannelClient *rcc)
diff --git a/server/image-encoders.c b/server/image-encoders.c
index 5759230..0ade8c8 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 {
     RingItem base;
     GlzEncDictContext *dict;
@@ -45,37 +40,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);
@@ -382,23 +368,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);
     }
 }
 
@@ -457,9 +444,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);
@@ -489,121 +477,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;
 }
 
@@ -614,14 +544,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.
@@ -637,31 +565,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);
     }
 }
 
@@ -1156,52 +1078,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;
 }
 
@@ -1220,8 +1112,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;
 
@@ -1242,8 +1133,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;
@@ -1254,8 +1144,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 5bad0d4..c715f3e 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 {
@@ -136,14 +138,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;
@@ -183,9 +183,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 9238632..bce6d69 100644
--- a/server/red-worker.c
+++ b/server/red-worker.c
@@ -123,14 +123,15 @@ static void common_release_recv_buf(RedChannelClient *rcc, uint16_t type, uint32
     }
 }
 
-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)
@@ -821,9 +822,8 @@ static void handle_dev_oom(void *opaque, void *payload)
 
     spice_return_if_fail(worker->running);
     // streams? but without streams also leak
-    spice_debug("OOM1 #draw=%u, #glz_draw=%u current %u pipes %u",
+    spice_debug("OOM1 #draw=%u, current %u pipes %u",
                 display->drawable_count,
-                display->encoder_shared_data.glz_drawable_count,
                 display->current_size,
                 red_channel_sum_pipes_size(display_red_channel));
     while (red_process_display(worker, &ring_is_empty)) {
@@ -833,9 +833,8 @@ static void handle_dev_oom(void *opaque, void *payload)
         display_channel_free_some(worker->display_channel);
         red_qxl_flush_resources(worker->qxl);
     }
-    spice_debug("OOM2 #draw=%u, #glz_draw=%u current %u pipes %u",
+    spice_debug("OOM2 #draw=%u, current %u pipes %u",
                 display->drawable_count,
-                display->encoder_shared_data.glz_drawable_count,
                 display->current_size,
                 red_channel_sum_pipes_size(display_red_channel));
     red_qxl_clear_pending(worker->qxl->st, RED_DISPATCHER_PENDING_OOM);
diff --git a/server/red-worker.h b/server/red-worker.h
index 63be8b5..1639df8 100644
--- a/server/red-worker.h
+++ b/server/red-worker.h
@@ -92,7 +92,7 @@ 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);
 
 CommonGraphicsChannel *red_worker_new_channel(RedWorker *worker, int size,
                                               const char *name,
-- 
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]