[Spice-devel] [PATCH 06/11] worker: move some tree container functions

[Frediano Ziglio]

> 
> On Wed, Nov 11, 2015 at 5:10 PM, Frediano Ziglio <fziglio at redhat.com> wrote:
> >>
> >> On Wed, Nov 11, 2015 at 1:20 PM, Frediano Ziglio <fziglio at redhat.com>
> >> wrote:
> >> > From: Marc-André Lureau <marcandre.lureau at gmail.com>
> >> >
> >> > ---
> >> >  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 at lists.freedesktop.org
> >> > 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
> 

Merged

Frediano