On Wed, Nov 11, 2015 at 5:10 PM, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
>>
>> On Wed, Nov 11, 2015 at 1:20 PM, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
>> > From: Marc-André Lureau <marcandre.lureau@xxxxxxxxx>
>> >
>> > ---
>> > server/red_worker.c | 32 ++++----------------------------
>> > server/tree.c | 27 +++++++++++++++++++++++++++
>> > server/tree.h | 2 ++
>> > 3 files changed, 33 insertions(+), 28 deletions(-)
>> >
>> > diff --git a/server/red_worker.c b/server/red_worker.c
>> > index 5cdb348..e82317c 100644
>> > --- a/server/red_worker.c
>> > +++ b/server/red_worker.c
>> > @@ -933,30 +933,6 @@ static inline void remove_shadow(DrawItem *item)
>> > free(shadow);
>> > }
>> >
>> > -static inline void current_remove_container(DisplayChannel *display,
>> > Container *container)
>> > -{
>> > - spice_assert(ring_is_empty(&container->items));
>> > - ring_remove(&container->base.siblings_link);
>> > - region_destroy(&container->base.rgn);
>> > - free(container);
>> > -}
>> > -
>> > -static inline void container_cleanup(DisplayChannel *display, Container
>> > *container)
>> > -{
>> > - while (container && container->items.next == container->items.prev) {
>> > - Container *next = container->base.container;
>> > - if (container->items.next != &container->items) {
>> > - TreeItem *item = (TreeItem *)ring_get_head(&container->items);
>> > - spice_assert(item);
>> > - ring_remove(&item->siblings_link);
>> > - ring_add_after(&item->siblings_link,
>> > &container->base.siblings_link);
>> > - item->container = container->base.container;
>> > - }
>> > - current_remove_container(display, container);
>> > - container = next;
>> > - }
>> > -}
>> > -
>> > static void display_stream_trace_add_drawable(DisplayChannel *display,
>> > Drawable *item)
>> > {
>> > ItemTrace *trace;
>> > @@ -1034,7 +1010,7 @@ static inline void current_remove(DisplayChannel
>> > *display, TreeItem *item)
>> > continue;
>> > }
>> > ring_item = now->siblings_link.prev;
>> > - current_remove_container(display, container);
>> > + container_free(container);
>> > }
>> > if (now == item) {
>> > return;
>> > @@ -2613,7 +2589,7 @@ static bool free_one_drawable(DisplayChannel
>> > *display, int force_glz_free)
>> > container = drawable->tree_item.base.container;
>> >
>> > current_remove_drawable(display, drawable);
>> > - container_cleanup(display, container);
>> > + container_cleanup(container);
>> > return TRUE;
>> > }
>> >
>> > @@ -3106,7 +3082,7 @@ static void red_update_area_till(DisplayChannel
>> > *display, const SpiceRect *area,
>> > now->refs++;
>> > container = now->tree_item.base.container;
>> > current_remove_drawable(display, now);
>> > - container_cleanup(display, container);
>> > + container_cleanup(container);
>> > /* red_draw_drawable may call red_update_area for the surfaces
>> > 'now' depends on. Notice,
>> > that it is valid to call red_update_area in this case and not
>> > red_update_area_till:
>> > It is impossible that there was newer item then 'last' in one
>> > of the surfaces
>> > @@ -3164,7 +3140,7 @@ static void red_update_area(DisplayChannel *display,
>> > const SpiceRect *area, int
>> > now->refs++;
>> > container = now->tree_item.base.container;
>> > current_remove_drawable(display, now);
>> > - container_cleanup(display, container);
>> > + container_cleanup(container);
>> > red_draw_drawable(display, now);
>> > display_channel_drawable_unref(display, now);
>> > } while (now != last);
>> > diff --git a/server/tree.c b/server/tree.c
>> > index bf50edf..ad31f09 100644
>> > --- a/server/tree.c
>> > +++ b/server/tree.c
>> > @@ -223,3 +223,30 @@ Container* container_new(DrawItem *item)
>> >
>> > return container;
>> > }
>> > +
>> > +void container_free(Container *container)
>> > +{
>>
>> Better add a check if container is valid as well.
>>
>> > + spice_return_if_fail(ring_is_empty(&container->items));
>> > +
>> > + ring_remove(&container->base.siblings_link);
>> > + region_destroy(&container->base.rgn);
>> > + free(container);
>> > +}
>> > +
>> > +void container_cleanup(Container *container)
>> > +{
>> > + /* visit upward, removing containers */
>> > + /* non-empty container get its element moving up ?? */
>> > + while (container && container->items.next == container->items.prev) {
>> > + Container *next = container->base.container;
>> > + if (container->items.next != &container->items) {
>> > + TreeItem *item = (TreeItem *)ring_get_head(&container->items);
>> > + spice_assert(item);
>> > + ring_remove(&item->siblings_link);
>> > + ring_add_after(&item->siblings_link,
>> > &container->base.siblings_link);
>> > + item->container = container->base.container;
>> > + }
>> > + container_free(container);
>> > + container = next;
>> > + }
>> > +}
>> > diff --git a/server/tree.h b/server/tree.h
>> > index 6249c28..01d4ff9 100644
>> > --- a/server/tree.h
>> > +++ b/server/tree.h
>> > @@ -82,5 +82,7 @@ static inline int is_opaque_item(TreeItem *item)
>> > void tree_item_dump (TreeItem *item);
>> > Shadow* shadow_new (DrawItem *item, const
>> > SpicePoint *delta);
>> > Container* container_new (DrawItem *item);
>> > +void container_free (Container
>> > *container);
>> > +void container_cleanup (Container
>> > *container);
>> >
>> > #endif /* TREE_H_ */
>> > --
>> > 2.4.3
>> >
>> > _______________________________________________
>> > Spice-devel mailing list
>> > Spice-devel@xxxxxxxxxxxxxxxxxxxxx
>> > http://lists.freedesktop.org/mailman/listinfo/spice-devel
>>
>> ACK!
>>
>
> The comment is a TODO for you or should I add the check?
> Personally this is just a move so I would not add anything.
I agree that, as it's just moving the code, is better to not change
anything on this patch.
>
> OT on a change: this check on the code
>
> while (container && container->items.next == container->items.prev) {
>
> (the ==) looks odd. Is basically checking if container has 0 or 1 items.
> Looks like that more than cleaning is collapsing/optimizing the container.
>
> Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel