[Spice-devel] [PATCH 5/5] DisplayChannel: document exclude_region() functions

Jonathon Jongsma jjongsma at redhat.com
Fri Feb 3 22:55:14 UTC 2017


This is a particularly opaque part of the code for managing pending
Drawable operations. This patch adds documentation explaining these
functions.
---
NOTE: this patch still contains a lot of open questions. It's not intended to
be submitted as-is, but is posted here maintly to elicit discussion


 server/display-channel.c | 173 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 173 insertions(+)

diff --git a/server/display-channel.c b/server/display-channel.c
index ed00740..5d6c530 100644
--- a/server/display-channel.c
+++ b/server/display-channel.c
@@ -583,6 +583,47 @@ static int current_add_equal(DisplayChannel *display, DrawItem *item, TreeItem *
     return FALSE;
 }
 
+/* This function excludes the region from a single TreeItem
+ *
+ * If there is overlap between @rgn and the @item region, remove the
+ *
+ * overlapping intersection from both @rgn and the item's region (XXX WHY???)
+ *
+ * However, if the item is a DrawItem that has a shadow, we add an additional
+ * region to @rgn: the intersection of the shadow item's region with @rgn if
+ * @rgn was shifted over by the delta between the DrawItem and the Shadow.
+ * [WORKING THEORY: since the destination region for a COPY_BITS operation was
+ * excluded, we no longer need the source region that corresponds with that
+ * copy operation, so we can also exclude any drawables that affect that
+ * region. Not sure if that actually makes sense... ]
+ *
+ * If the item is a Shadow, we store the intersection between @rgn and the
+ * Shadow's region in Shadow::on_hold and remove that region from @rgn. This is
+ * done since a Shadow represents the source region for a COPY_BITS operation,
+ * and we need to make sure that this source region stays up-to-date until the
+ * copy operation is executed.
+ *
+ * Consider the following case:
+ *  1) the surface is fully black at the beginning
+ *  2) we add a new item to the tree which paints region A white
+ *  3) we add a new item to the tree which copies region A to region B
+ *  4) we add another new item to the tree painting region A blue.
+ *
+ * After all operations are completed, region A should be blue, and region B
+ * should be white. If there were no copy operation (step 3), we could simply
+ * eliminate step 2 when we add item 4 to the tree, since step 4 overwrites the
+ * same region with a different color. However, if we did eliminate step 2,
+ * region B would be black after all operations were completed. So these
+ * regions that would normally be excluded are put "on hold" if they are part
+ * of a source region for a copy operation.
+ *
+ * @display: the display channel
+ * @ring: a fallback toplevel ring???
+ * @item: the tree item to exclude from @rgn
+ * @rgn: the region to exclude (?)
+ * @top_ring: ?
+ * @frame_candidate: ?
+ */
 static void __exclude_region(DisplayChannel *display, Ring *ring, TreeItem *item, QRegion *rgn,
                              Ring **top_ring, Drawable *frame_candidate)
 {
@@ -590,51 +631,79 @@ static void __exclude_region(DisplayChannel *display, Ring *ring, TreeItem *item
     stat_start(&display->priv->__exclude_stat, start_time);
 
     region_clone(&and_rgn, rgn);
+    /* find intersection of the @rgn argument with the region of the @item arg */
     region_and(&and_rgn, &item->rgn);
     if (!region_is_empty(&and_rgn)) {
         if (IS_DRAW_ITEM(item)) {
             DrawItem *draw = DRAW_ITEM(item);
 
             if (draw->effect == QXL_EFFECT_OPAQUE) {
+                /* remove the intersection from the original @rgn */
                 region_exclude(rgn, &and_rgn);
             }
 
             if (draw->shadow) {
+                /* this item represents the destination of a COPY_BITS
+                 * operation. 'shadow' represents the source item for the copy
+                 * operation */
                 Shadow *shadow;
                 int32_t x = item->rgn.extents.x1;
                 int32_t y = item->rgn.extents.y1;
 
+                /* remove the intersection from the item's region */
                 region_exclude(&draw->base.rgn, &and_rgn);
                 shadow = draw->shadow;
+                /* shift the intersected region by the difference between the
+                 * source and destination regions */
                 region_offset(&and_rgn, shadow->base.rgn.extents.x1 - x,
                               shadow->base.rgn.extents.y1 - y);
+                /* remove the shifted intersection region from the source
+                 * (shadow) item's region. If the destination is excluded, we
+                 * can also exclude the corresponding area from the source */
                 region_exclude(&shadow->base.rgn, &and_rgn);
+                /* find the intersection between the shifted intersection
+                 * region and the Shadow's 'on_hold' region. This represents
+                 * the portion of the Shadow's region that we just removed that
+                 * is currently stored in on_hold. */
                 region_and(&and_rgn, &shadow->on_hold);
                 if (!region_is_empty(&and_rgn)) {
+                    /* Since we removed a portion of the Shadow's region, we
+                     * can also remove that portion from on_hold */
                     region_exclude(&shadow->on_hold, &and_rgn);
+                    /* Since this region is no longer "on hold", add it back to
+                     * the @rgn argument */
                     region_or(rgn, &and_rgn);
                     // in flat representation of current, shadow is always his owner next
+                    // XXX: what does that mean?
                     if (!tree_item_contained_by(&shadow->base, *top_ring)) {
                         *top_ring = tree_item_container_items(&shadow->base, ring);
                     }
                 }
             } else {
+                /* XXX: ??? */
                 if (frame_candidate) {
                     Drawable *drawable = SPICE_CONTAINEROF(draw, Drawable, tree_item);
                     stream_maintenance(display, frame_candidate, drawable);
                 }
+                /* Remove the intersection from the DrawItem's region */
                 region_exclude(&draw->base.rgn, &and_rgn);
             }
         } else if (item->type == TREE_ITEM_TYPE_CONTAINER) {
+            /* excludes the intersection between 'rgn' and item->rgn from the
+             * item's region */
             region_exclude(&item->rgn, &and_rgn);
 
             if (region_is_empty(&item->rgn)) {  //assume container removal will follow
                 Shadow *shadow;
 
+                /* exclude the intersection from the 'rgn' argument as well,
+                 * but only if the item is now empty... WHY??? */
                 region_exclude(rgn, &and_rgn);
                 if ((shadow = tree_item_find_shadow(item))) {
+                    /* add the shadow's on_hold region back to the 'rgn' argument */
                     region_or(rgn, &shadow->on_hold);
                     if (!tree_item_contained_by(&shadow->base, *top_ring)) {
+                        /* what is top_ring used for??? */
                         *top_ring = tree_item_container_items(&shadow->base, ring);
                     }
                 }
@@ -644,14 +713,43 @@ static void __exclude_region(DisplayChannel *display, Ring *ring, TreeItem *item
 
             spice_assert(item->type == TREE_ITEM_TYPE_SHADOW);
             shadow = SHADOW(item);
+            /* Since a Shadow represents the source region for a COPY_BITS
+             * operation, we need to make sure that we don't remove existing
+             * drawables that draw to this source region. If we did, it would
+             * affect the copy operation. So we remove the intersection between
+             * @rgn and item->rgn from the @rgn argument to avoid excluding
+             * these drawables */
             region_exclude(rgn, &and_rgn);
+            /* adds this intersection to on_hold */
             region_or(&shadow->on_hold, &and_rgn);
         }
     }
+    /* clean up memory */
     region_destroy(&and_rgn);
     stat_add(&display->priv->__exclude_stat, start_time);
 }
 
+/* This function iterates through the given @ring starting at @ring_item and
+ * continuing until reaching @last, and calls __exclude_item() on each item.
+ * Any items that have an empty region as a result of the __exclude_region()
+ * call are removed from the tree.
+ *
+ * XXX: I still don't have a great conceptual understanding of what we're
+ * trying to do by calling this function.
+ *
+ * @ring: every time this function is called, @ring is a Surface's 'current'
+ *      ring, or to the ring of children of a container within that ring.
+ * @ring_item: callers usually call this argument 'exclude_base'. We will
+ *      iterate through the tree starting at this item
+ * @rgn: callers usually call this 'exclude_rgn' -- it appears to be the region
+ *      we want to exclude from existing items in the tree. It is an in/out
+ *      parameter and it may be modified as the result of calling this function
+ * @last: We will stop iterating at this item, and the function will return the
+ *      next item after iteration is complete (which may be different than the
+ *      passed value if that item was removed from the tree
+ * @frame_candidate: usually callers pass NULL, sometimes it's the drawable
+ *      that's being added to the 'current' ring. What is its purpose?
+ */
 static void exclude_region(DisplayChannel *display, Ring *ring, RingItem *ring_item,
                            QRegion *rgn, TreeItem **last, Drawable *frame_candidate)
 {
@@ -670,40 +768,60 @@ static void exclude_region(DisplayChannel *display, Ring *ring, RingItem *ring_i
 
         spice_assert(!region_is_empty(&now->rgn));
 
+        /* check whether the ring_item item intersects the passed-in region */
         if (region_intersects(rgn, &now->rgn)) {
+            /* remove the overlapping portions of region and now->rgn, among
+             * other things. See documentation for __exclude_region() */
             __exclude_region(display, ring, now, rgn, &top_ring, frame_candidate);
 
             if (region_is_empty(&now->rgn)) {
+                /* __exclude_region() does not remove the region of shadow-type
+                 * items */
                 spice_assert(now->type != TREE_ITEM_TYPE_SHADOW);
                 ring_item = now->siblings_link.prev;
+                /* if __exclude_region() removed the entire region for this
+                 * sibling item, remove it from the 'current' tree */
                 current_remove(display, now);
                 if (last && *last == now) {
+                    /* the caller wanted to stop at this item, but this item
+                     * has been removed, so we set @last to the next item */
                     SPICE_VERIFY(SPICE_OFFSETOF(TreeItem, siblings_link) == 0);
                     *last = (TreeItem *)ring_next(ring, ring_item);
                 }
             } else if (now->type == TREE_ITEM_TYPE_CONTAINER) {
+                /* if this sibling is a container type, descend into the
+                 * container's child ring and continue iterating */
                 Container *container = CONTAINER(now);
                 if ((ring_item = ring_get_head(&container->items))) {
                     ring = &container->items;
                     spice_assert(SPICE_CONTAINEROF(ring_item, TreeItem, siblings_link)->container);
                     continue;
                 }
+                /* container had no children, so reset ring_item to the
+                 * container itself */
                 ring_item = &now->siblings_link;
             }
 
             if (region_is_empty(rgn)) {
+                /* __exclude_region() removed the entire region from 'rgn', so
+                 * no need to continue checking further items in the tree */
                 stat_add(&display->priv->exclude_stat, start_time);
                 return;
             }
         }
 
         SPICE_VERIFY(SPICE_OFFSETOF(TreeItem, siblings_link) == 0);
+        /* if this is the last item to check, or if the current ring is
+         * completed, don't go any further */
         while ((last && *last == (TreeItem *)ring_item) ||
                !(ring_item = ring_next(ring, ring_item))) {
+            /* we're currently iterating the top ring, so we're done */
             if (ring == top_ring) {
                 stat_add(&display->priv->exclude_stat, start_time);
                 return;
             }
+            /* we're iterating through a container child ring, so climb one
+             * level up the heirarchy and continue iterating that ring */
             ring_item = &container->base.siblings_link;
             container = container->base.container;
             ring = (container) ? &container->items : top_ring;
@@ -748,6 +866,10 @@ static int current_add_with_shadow(DisplayChannel *display, Ring *ring, Drawable
     if (item->tree_item.effect == QXL_EFFECT_OPAQUE) {
         QRegion exclude_rgn;
         region_clone(&exclude_rgn, &item->tree_item.base.rgn);
+        /* Since the new drawable is opaque, remove overlapped regions from all
+         * items already in the tree.  Start iterating through the tree
+         * starting with the shadow item to avoid excluding the new item
+         * itself */
         exclude_region(display, ring, &shadow->base.siblings_link, &exclude_rgn, NULL, NULL);
         region_destroy(&exclude_rgn);
         streams_update_visible_region(display, item);
@@ -818,14 +940,42 @@ static int current_add(DisplayChannel *display, Ring *ring, Drawable *drawable)
                 if ((shadow = tree_item_find_shadow(sibling))) {
                     /* The sibling item was the destination of a COPY_BITS operation */
                     if (exclude_base) {
+                        /* During a previous iteration through this loop, an
+                         * obscured sibling item was removed from the tree, and
+                         * exclude_base was set to the item immediately after
+                         * the removed item (see below). This time through the
+                         * loop, we encountered another sibling that was
+                         * completely obscured, so we call exclude_region()
+                         * using the previously saved item as our starting
+                         * point. @exlude_rgn will be the union of any previous
+                         * 'on_hold' regions from the shadows of previous
+                         * iterations
+                         *
+                         * XXX: why do we only only call exclude_region() for
+                         * the previous item if the next item is obscured and
+                         * has a shadow???
+                         */
                         TreeItem *next = sibling;
+                        /* XXX: What is the intent of this call? */
                         exclude_region(display, ring, exclude_base, &exclude_rgn, &next, NULL);
                         if (next != sibling) {
+                            /* the @next param is only changed if the given item
+                             * was removed as a side-effect of calling
+                             * exclude_region(), so update our loop variable */
                             now = next ? &next->siblings_link : NULL;
                             exclude_base = NULL;
                             continue;
                         }
                     }
+                    /* Since the destination item (sibling) of the COPY_BITS
+                     * operation is fully obscured, we no longer need the
+                     * source item (shadow) anymore. shadow->on_hold represents
+                     * a region that would normally have been excluded by a
+                     * previous call to __exclude_region() (see documentation
+                     * for that function), but was put on hold to make sure we
+                     * kept the source region up to date. Now that we no longer
+                     * need this source region, this "on hold" region can be
+                     * safely excluded again. */
                     region_or(&exclude_rgn, &shadow->on_hold);
                 }
                 now = now->prev;
@@ -835,6 +985,11 @@ static int current_add(DisplayChannel *display, Ring *ring, Drawable *drawable)
                 /* advance the loop variable */
                 now = ring_next(ring, now);
                 if (shadow || skip) {
+                    /* XXX: don't understand this exclude_base stuff
+                     * 'now' is currently set to the the item immediately AFTER
+                     * the obscured sibling that we just removed. Why is this
+                     * item used as an 'exclude_base'??
+                     */
                     exclude_base = now;
                 }
                 continue;
@@ -844,6 +999,13 @@ static int current_add(DisplayChannel *display, Ring *ring, Drawable *drawable)
                 /* the sibling item is opaque and entirely contains the new drawable */
                 Container *container;
 
+                /* XXX: still don't understand this exclude_base stuff. The
+                 * first time through, @exclude_base will be NULL, but
+                 * subsequent loops may set it to something.
+                 * In addition, @exclude_rgn starts out empty, but previous
+                 * iterations of this loop may have added various
+                 * Shadow::on_hold regions to it.
+                 */
                 if (exclude_base) {
                     exclude_region(display, ring, exclude_base, &exclude_rgn, NULL, NULL);
                     region_clear(&exclude_rgn);
@@ -878,6 +1040,11 @@ static int current_add(DisplayChannel *display, Ring *ring, Drawable *drawable)
                 }
             }
         }
+        /* If we've gotten here, that means that:
+         *  - the new item is not opaque
+         *  - We just created a container to hold the new drawable and the
+         *    sibling that encloses it
+         *  - ??? */
         if (!exclude_base) {
             exclude_base = now;
         }
@@ -886,7 +1053,13 @@ static int current_add(DisplayChannel *display, Ring *ring, Drawable *drawable)
     /* we've removed any obscured siblings and figured out which ring the new
      * drawable needs to be added to, so let's add it. */
     if (item->effect == QXL_EFFECT_OPAQUE) {
+        /* @exclude_rgn may contain the union of on_hold regions from any
+         * Shadows that were associated with DrawItems that were removed from
+         * the tree.  Add the new item's region to that */
         region_or(&exclude_rgn, &item->base.rgn);
+        /* XXX: Why do we need to call exclude_region() here again? I thought
+         * we had just iterated the whole list above to check for region
+         * intersections between this new item and existing items... */
         exclude_region(display, ring, exclude_base, &exclude_rgn, NULL, drawable);
         stream_trace_update(display, drawable);
         streams_update_visible_region(display, drawable);
-- 
2.9.3



More information about the Spice-devel mailing list