[Spice-devel] [PATCH 2/2] DisplayChannel: document exclude_region() functions
Jonathon Jongsma
jjongsma at redhat.com
Wed Mar 8 21:50:04 UTC 2017
This is a particularly opaque part of the code for managing pending
Drawable operations. This patch adds documentation atempting to explain
these functions.
---
server/display-channel.c | 174 +++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 174 insertions(+)
diff --git a/server/display-channel.c b/server/display-channel.c
index dd0bb6b..fbb66fe 100644
--- a/server/display-channel.c
+++ b/server/display-channel.c
@@ -587,6 +587,47 @@ static int current_add_equal(DisplayChannel *display, DrawItem *item, TreeItem *
return FALSE;
}
+/* This function excludes the given region from a single TreeItem. Both @rgn
+ * and @item may be modified.
+ *
+ * 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 when
+ * @rgn is 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)
{
@@ -594,51 +635,78 @@ 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) {
+ /* @draw 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);
}
}
@@ -648,14 +716,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_region() 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)
{
@@ -674,40 +771,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;
@@ -752,6 +869,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);
@@ -822,14 +943,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;
@@ -839,6 +988,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;
@@ -848,6 +1002,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);
@@ -882,6 +1043,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;
}
@@ -890,7 +1056,15 @@ 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...
+ * NOTE: this is the only place where we call exclude_region() with a
+ * non-NULL frame_candidate argument. */
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