[Spice-devel] [PATCH 3/5] DisplayChannel: start documenting drawable tree
Frediano Ziglio
fziglio at redhat.com
Fri Feb 10 11:56:00 UTC 2017
>
> The code that manages pending QXL Drawable operations is fairly complex
> and difficult to understand. This is an attempt to start documenting
> that code to save time when we have to work on this code in the future.
> ---
> server/display-channel.c | 80
> ++++++++++++++++++++++++++++++++++++++++++++++++
> server/tree.c | 12 +++++++-
> server/tree.h | 6 ++++
> 3 files changed, 97 insertions(+), 1 deletion(-)
>
> diff --git a/server/display-channel.c b/server/display-channel.c
> index 7d3c6e4..e12751b 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -392,6 +392,8 @@ static void current_add_drawable(DisplayChannel *display,
> drawable->refs++;
> }
>
> +/* Unrefs the drawable and removes it from any rings that it's in, as well
> as
> + * removing any associated shadow item */
> static void current_remove_drawable(DisplayChannel *display, Drawable *item)
> {
> /* todo: move all to unref? */
> @@ -420,6 +422,7 @@ static void drawable_remove_from_pipes(Drawable
> *drawable)
> }
> }
>
> +/* This function should never be called for Shadow TreeItems */
> static void current_remove(DisplayChannel *display, TreeItem *item)
> {
> TreeItem *now = item;
> @@ -440,19 +443,32 @@ static void current_remove(DisplayChannel *display,
> TreeItem *item)
> spice_assert(now->type == TREE_ITEM_TYPE_CONTAINER);
>
> if ((ring_item = ring_get_head(&now_as_container->items))) {
> + /* descend into the container's child ring and continue
> + * iterating and removing those children */
> now = SPICE_CONTAINEROF(ring_item, TreeItem, siblings_link);
> continue;
> }
> + /* This item is a container but it has no children, so reset our
> + * iterator to the item's previous sibling and free this empty
> + * container */
> ring_item = now->siblings_link.prev;
> container_free(now_as_container);
> }
> if (now == item) {
> + /* This is true if the initial @item was a DRAWABLE, or if @item
> + * was a container and we've finished iterating over all of that
> + * container's children and returned back up to the parent and
> + * freed it (see below) */
> return;
> }
>
> + /* Increment the iterator to the next sibling. Note that if an item
> was
> + * removed above, ring_item will have been reset to the item before
> the
> + * item that was removed */
> if ((ring_item = ring_next(&container_of_now->items, ring_item))) {
> now = SPICE_CONTAINEROF(ring_item, TreeItem, siblings_link);
> } else {
> + /* there is no next sibling, so move one level up the tree */
> now = &container_of_now->base;
> }
> }
> @@ -465,10 +481,23 @@ static void current_remove_all(DisplayChannel *display,
> int surface_id)
>
> while ((ring_item = ring_get_head(ring))) {
> TreeItem *now = SPICE_CONTAINEROF(ring_item, TreeItem,
> siblings_link);
> + /* NOTE: current_remove() should never be called on Shadow type
> items
> + * or we will hit an assert. Fortunately, the 'current' ring is
> ordered
> + * in such a way that a DrawItem will always be placed before its
> + * associated Shadow in the tree. Since removing a DrawItem will
> also
> + * result in the associated Shadow item being removed from the tree,
> + * this loop will never call current_remove() on a Shadow item
> unless
> + * we change the order that items are inserted into the tree */
> current_remove(display, now);
> }
> }
>
> +/* Replace an existing Drawable in the tree with a new drawable that is
> + * equivalent. The new drawable is also added to the pipe.
> + *
> + * This function can fail if the items aren't actually equivalent (e.g.
> either
> + * item has a shadow, they have different effects, etc)
> + */
> static int current_add_equal(DisplayChannel *display, DrawItem *item,
> TreeItem *other)
> {
> DrawItem *other_draw_item;
> @@ -488,6 +517,9 @@ static int current_add_equal(DisplayChannel *display,
> DrawItem *item, TreeItem *
> other_drawable = SPICE_CONTAINEROF(other_draw_item, Drawable,
> tree_item);
>
> if (item->effect == QXL_EFFECT_OPAQUE) {
> + /* check whether the new item can safely replace the other drawable
> at
> + * the same position in the pipe, or whether it should be added to
> the
> + * end of the queue */
> int add_after = !!other_drawable->stream &&
> is_drawable_independent_from_surfaces(drawable);
> stream_maintenance(display, drawable, other_drawable);
> @@ -679,6 +711,8 @@ static void exclude_region(DisplayChannel *display, Ring
> *ring, RingItem *ring_i
> }
> }
>
> +/* Add a @drawable (with a shadow) to the current ring.
> + * The return value indicates whether the new item should be added to the
> pipe */
it's @drawable or @item ?
> static int current_add_with_shadow(DisplayChannel *display, Ring *ring,
> Drawable *item)
> {
> stat_start(&display->priv->add_stat, start_time);
> @@ -705,7 +739,11 @@ static int current_add_with_shadow(DisplayChannel
> *display, Ring *ring, Drawable
> stream_detach_behind(display, &shadow->base.rgn, NULL);
> }
>
> + /* Prepend the shadow to the beginning of the current ring */
> ring_add(ring, &shadow->base.siblings_link);
> + /* Prepend the draw item to the beginning of the current ring. NOTE:
> this
> + * means that the drawable is placed *before* its associated shadow in
> the
> + * tree. Changing this order will violate several unstated assumptions
> */
> current_add_drawable(display, item, ring);
> if (item->tree_item.effect == QXL_EFFECT_OPAQUE) {
> QRegion exclude_rgn;
> @@ -722,6 +760,8 @@ static int current_add_with_shadow(DisplayChannel
> *display, Ring *ring, Drawable
> return TRUE;
> }
>
> +/* Add a @drawable (without a shadow) to the current ring.
> + * The return value indicates whether the new item should be added to the
> pipe */
> static int current_add(DisplayChannel *display, Ring *ring, Drawable
> *drawable)
> {
> DrawItem *item = &drawable->tree_item;
> @@ -734,31 +774,49 @@ static int current_add(DisplayChannel *display, Ring
> *ring, Drawable *drawable)
> region_init(&exclude_rgn);
> now = ring_next(ring, ring);
>
> + /* check whether the new drawable region intersects any of the items
> + * already in the 'current' ring */
> while (now) {
> TreeItem *sibling = SPICE_CONTAINEROF(now, TreeItem, siblings_link);
> int test_res;
>
> if (!region_bounds_intersects(&item->base.rgn, &sibling->rgn)) {
> + /* the bounds of the two items are totally disjoint, so no need
> to
> + * check further. check the next item */
> now = ring_next(ring, now);
> continue;
> }
> + /* bounds overlap, but check whether the regions actually overlap */
> test_res = region_test(&item->base.rgn, &sibling->rgn,
> REGION_TEST_ALL);
> if (!(test_res & REGION_TEST_SHARED)) {
> + /* there's no overlap of the regions between these two items.
> Move
> + * on to the next one. */
> now = ring_next(ring, now);
> continue;
> } else if (sibling->type != TREE_ITEM_TYPE_SHADOW) {
> + /* there is an overlap between the two regions */
> + /* NOTE: Shadow types represent a source region for a COPY_BITS
> + * operation, they don't represent a region that will be drawn.
> + * Therefore, we don't check for overlap between the new
> + * DrawItem and any shadow items */
> if (!(test_res & REGION_TEST_RIGHT_EXCLUSIVE) &&
> !(test_res &
> REGION_TEST_LEFT_EXCLUSIVE)
> &&
> current_add_equal(display,
> item, sibling)) {
> + /* the regions were equivalent, so we just replaced the
> other
> + * drawable with the new one */
> stat_add(&display->priv->add_stat, start_time);
> + /* Caller doesn't need to add the new drawable to the pipe,
> + * since current_add_equal already added it to the pipe */
> return FALSE;
> }
>
> if (!(test_res & REGION_TEST_RIGHT_EXCLUSIVE) && item->effect ==
> QXL_EFFECT_OPAQUE) {
> + /* the new drawable is opaque and entirely contains the
> sibling item */
> Shadow *shadow;
> int skip = now == exclude_base;
>
> if ((shadow = tree_item_find_shadow(sibling))) {
> + /* The sibling item was the destination of a COPY_BITS
> operation */
> if (exclude_base) {
> TreeItem *next = sibling;
> exclude_region(display, ring, exclude_base,
> &exclude_rgn, &next, NULL);
> @@ -771,7 +829,10 @@ static int current_add(DisplayChannel *display, Ring
> *ring, Drawable *drawable)
> region_or(&exclude_rgn, &shadow->on_hold);
> }
> now = now->prev;
> + /* remove the obscured sibling from the 'current' tree,
> which
> + * will also remove its shadow (if any) */
> current_remove(display, sibling);
> + /* advance the loop variable */
> now = ring_next(ring, now);
> if (shadow || skip) {
> exclude_base = now;
> @@ -780,6 +841,7 @@ static int current_add(DisplayChannel *display, Ring
> *ring, Drawable *drawable)
> }
>
> if (!(test_res & REGION_TEST_LEFT_EXCLUSIVE) &&
> is_opaque_item(sibling)) {
> + /* the sibling item is opaque and entirely contains the new
> drawable */
> Container *container;
>
> if (exclude_base) {
> @@ -789,13 +851,20 @@ static int current_add(DisplayChannel *display, Ring
> *ring, Drawable *drawable)
> }
> if (sibling->type == TREE_ITEM_TYPE_CONTAINER) {
> container = CONTAINER(sibling);
> + /* NOTE: here, ring is reset to the ring of the
> container's children */
> ring = &container->items;
> + /* if the sibling item is a container, place the new
> + * drawable into that container */
> item->base.container = container;
> + /* Start iterating over the container's children to see
> if
> + * any of them intersect this new drawable */
> now = ring_next(ring, ring);
> continue;
> }
> spice_assert(IS_DRAW_ITEM(sibling));
> if (!DRAW_ITEM(sibling)->container_root) {
> + /* Create a new container to hold the sibling and the
> new
> + * drawable */
> container = container_new(DRAW_ITEM(sibling));
> if (!container) {
> spice_warning("create new container failed");
> @@ -803,6 +872,8 @@ static int current_add(DisplayChannel *display, Ring
> *ring, Drawable *drawable)
> return FALSE;
> }
> item->base.container = container;
> + /* reset 'ring' to the container's children ring, so
> that
> + * we can add the new drawable to this ring below */
> ring = &container->items;
> }
> }
> @@ -812,6 +883,8 @@ static int current_add(DisplayChannel *display, Ring
> *ring, Drawable *drawable)
> }
> break;
> }
> + /* 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) {
> region_or(&exclude_rgn, &item->base.rgn);
> exclude_region(display, ring, exclude_base, &exclude_rgn, NULL,
> drawable);
> @@ -1621,6 +1694,8 @@ static void surface_update_dest(RedSurface *surface,
> const SpiceRect *area)
> canvas->ops->read_bits(canvas, dest, -stride, area);
> }
>
> +/* Draws all drawables associated with @surface, starting from the tail of
> the
> + * ring, and stopping after it draws @last */
> static void draw_until(DisplayChannel *display, RedSurface *surface,
> Drawable *last)
> {
> RingItem *ring_item;
> @@ -1644,6 +1719,11 @@ static void draw_until(DisplayChannel *display,
> RedSurface *surface, Drawable *l
> } while (now != last);
> }
>
> +/* Find the first Drawable in the @current ring that intersects the given
> + * @area, starting at item @from (or the head of the ring if @from is NULL).
> + *
> + * NOTE: this function expects @current to be a ring of Drawables, and more
> + * specifically an instance of Surface::current_list (not Surface::current)
> */
> static Drawable* current_find_intersects_rect(Ring *current, RingItem *from,
> const SpiceRect *area)
> {
> diff --git a/server/tree.c b/server/tree.c
> index ea11000..cc3a96f 100644
> --- a/server/tree.c
> +++ b/server/tree.c
> @@ -185,6 +185,9 @@ void tree_item_dump(TreeItem *item)
> tree_foreach(item, dump_item, &di);
> }
>
> +/* Creates a new Shadow item for the given DrawItem, with an offset of
> @delta.
> + * A shadow represents a source region for a COPY_BITS operation, while the
> + * DrawItem represents the destination region for the operation */
> Shadow* shadow_new(DrawItem *item, const SpicePoint *delta)
> {
> spice_return_val_if_fail(item->shadow == NULL, NULL);
> @@ -205,6 +208,11 @@ Shadow* shadow_new(DrawItem *item, const SpicePoint
> *delta)
> return shadow;
> }
>
> +/* Create a new container to hold @item and insert @item into this
> container.
> + *
> + * NOTE: This function assumes that @item is already inside a different
> Ring,
> + * so it removes @item from that ring before inserting it into the new
> + * container */
> Container* container_new(DrawItem *item)
> {
> Container *container = spice_new(Container, 1);
> @@ -268,6 +276,8 @@ Shadow* tree_item_find_shadow(TreeItem *item)
> return DRAW_ITEM(item)->shadow;
> }
>
> +/* return the Ring containing siblings of item, falling back to @ring if
> @item
> + * does not have a container */
> Ring *tree_item_container_items(TreeItem *item, Ring *ring)
> {
> return (item->container) ? &item->container->items : ring;
> @@ -281,7 +291,7 @@ int tree_item_contained_by(TreeItem *item, Ring *ring)
> if (now == ring) {
> return TRUE;
> }
> - } while ((item = &item->container->base));
> + } while ((item = &item->container->base)); /* move up one level */
>
> return FALSE;
> }
Looks fine.
> diff --git a/server/tree.h b/server/tree.h
> index 35b78ab..e579dd0 100644
> --- a/server/tree.h
> +++ b/server/tree.h
> @@ -43,12 +43,18 @@ struct TreeItem {
> RingItem siblings_link;
> uint32_t type;
> Container *container;
> + /* rgn holds the region of the item. As additional items get added to
> the
> + * tree, this region may be modified to represent the portion of the
> item
> + * that is not obscured by other items */
I'm a bit confused by the "to represent the portion...". Does it mean that
the portion is removed, right? Maybe "this region may be modified to exclude
the portion of the item ..." ?
> QRegion rgn;
> };
>
> /* A region "below" a copy, or the src region of the copy */
> struct Shadow {
> TreeItem base;
> + /* holds the union of all parts of this source region that have been
> + * obscured by a drawable item that has been subsequently added to the
> tree
> + */
Not entirely sure of this.
There are 2 region of code that modify on_hold:
- excluding a region from an item with a shadow
region_and(&and_rgn, &shadow->on_hold);
if (!region_is_empty(&and_rgn)) {
region_exclude(&shadow->on_hold, &and_rgn);
this remove part of the region
- excluding a region from a shadow
region_or(&shadow->on_hold, &and_rgn);
this add a region.
Both in __exclude_region, a function we are not sure what's doing
and for which purpose.
> QRegion on_hold;
> };
>
Frediano
More information about the Spice-devel
mailing list