[Spice-devel] [PATCH 01/16] UpgradeItem: use base PipeItem for refcounting

[Jonathon Jongsma]

On Wed, 2016-04-20 at 04:18 -0400, Frediano Ziglio wrote:
> > 
> > No need to re-implement refcounting in this subclass. However, I needed
> > to add a new 'dcc' member to UpgradeItem to be able to unref properly.
> > ---
> > Change since last patch: take ref on dcc
> > 
> >  server/dcc.c             | 18 +-----------------
> >  server/display-channel.c |  4 +---
> >  server/display-channel.h |  2 +-
> >  server/stream.c          | 18 ++++++++++++++++--
> >  4 files changed, 19 insertions(+), 23 deletions(-)
> > 
> > diff --git a/server/dcc.c b/server/dcc.c
> > index 91c3f82..5193c17 100644
> > --- a/server/dcc.c
> > +++ b/server/dcc.c
> > @@ -1591,29 +1591,15 @@ int dcc_handle_migrate_data(DisplayChannelClient
> > *dcc, uint32_t size, void *mess
> >      return TRUE;
> >  }
> >  
> > -static void upgrade_item_unref(DisplayChannel *display, UpgradeItem *item)
> > -{
> > -    if (--item->refs != 0)
> > -        return;
> > -
> > -    display_channel_drawable_unref(display, item->drawable);
> > -    free(item->rects);
> > -    free(item);
> > -}
> > -
> >  static void release_item_after_push(DisplayChannelClient *dcc, PipeItem
> >  *item)
> >  {
> > -    DisplayChannel *display = DCC_TO_DC(dcc);
> > -
> >      switch (item->type) {
> >      case PIPE_ITEM_TYPE_DRAW:
> >      case PIPE_ITEM_TYPE_IMAGE:
> >      case PIPE_ITEM_TYPE_STREAM_CLIP:
> >      case PIPE_ITEM_TYPE_MONITORS_CONFIG:
> > -        pipe_item_unref(item);
> > -        break;
> >      case PIPE_ITEM_TYPE_UPGRADE:
> > -        upgrade_item_unref(display, (UpgradeItem *)item);
> > +        pipe_item_unref(item);
> >          break;
> >      case PIPE_ITEM_TYPE_GL_SCANOUT:
> >      case PIPE_ITEM_TYPE_GL_DRAW:
> > @@ -1651,8 +1637,6 @@ static void
> > release_item_before_push(DisplayChannelClient *dcc, PipeItem *item)
> >      }
> >      case PIPE_ITEM_TYPE_STREAM_CLIP:
> >      case PIPE_ITEM_TYPE_UPGRADE:
> > -        upgrade_item_unref(display, (UpgradeItem *)item);
> > -        break;
> >      case PIPE_ITEM_TYPE_IMAGE:
> >      case PIPE_ITEM_TYPE_MONITORS_CONFIG:
> >          pipe_item_unref(item);
> > diff --git a/server/display-channel.c b/server/display-channel.c
> > index 88dbc74..98b4a43 100644
> > --- a/server/display-channel.c
> > +++ b/server/display-channel.c
> > @@ -1973,10 +1973,8 @@ static void hold_item(RedChannelClient *rcc, PipeItem
> > *item)
> >      case PIPE_ITEM_TYPE_DRAW:
> >      case PIPE_ITEM_TYPE_IMAGE:
> >      case PIPE_ITEM_TYPE_STREAM_CLIP:
> > -        pipe_item_ref(item);
> > -        break;
> >      case PIPE_ITEM_TYPE_UPGRADE:
> > -        ((UpgradeItem *)item)->refs++;
> > +        pipe_item_ref(item);
> >          break;
> >      default:
> >          spice_warn_if_reached();
> > diff --git a/server/display-channel.h b/server/display-channel.h
> > index 6b053de..8944f06 100644
> > --- a/server/display-channel.h
> > +++ b/server/display-channel.h
> > @@ -244,7 +244,7 @@ typedef struct SurfaceDestroyItem {
> >  
> >  typedef struct UpgradeItem {
> >      PipeItem base;
> > -    int refs;
> > +    DisplayChannelClient *dcc;
> 
> Here a display channel is enough I think.
> 
> >      Drawable *drawable;
> >      SpiceClipRects *rects;
> >  } UpgradeItem;
> > diff --git a/server/stream.c b/server/stream.c
> > index ae37a62..80cfe2e 100644
> > --- a/server/stream.c
> > +++ b/server/stream.c
> > @@ -758,6 +758,18 @@ void stream_agent_stop(StreamAgent *agent)
> >      }
> >  }
> >  
> > +static void upgrade_item_free(UpgradeItem *item)
> > +{
> > +    g_return_if_fail(item != NULL);
> > +    DisplayChannel *display = DCC_TO_DC(item->dcc);
> > +    g_return_if_fail(item->base.refcount != 0);
> > +
> > +    display_channel_drawable_unref(display, item->drawable);
> 
> I'm still pondering this.
> My initial question was mainly a question (even to me) than a
> proposal. Here the problem is that this can create a circular
> reference counting. The problem is quite hidden by the fact that
> we don't still have a way to free part of our stuff (DisplayChannel
> is one). This make hard to verify this, I should have pushed the
> inclusion of my cleanup branch when was refused.
> 
> Jonathon... did you test for leaks?

No I didn't. I did just now and didn't notice any leaks. But you're right that
it basically creates a circular ref, which could be problematic. It would
probably be better to use a weak ref, although I can't do until after the
GObject conversions have been merged. What if I remove the ref and add a FIXME
to use a weak ref in the future?

> 
> > +    free(item->rects);
> > +    red_channel_client_unref(RED_CHANNEL_CLIENT(item->dcc));
> > +    free(item);
> > +}
> > +
> >  /*
> >   * after dcc_detach_stream_gracefully is called for all the display channel
> >   clients,
> >   * detach_stream should be called. See comment (1).
> > @@ -798,10 +810,12 @@ static void
> > dcc_detach_stream_gracefully(DisplayChannelClient *dcc,
> >          rect_debug(&stream->current->red_drawable->bbox);
> >          rcc = RED_CHANNEL_CLIENT(dcc);
> >          upgrade_item = spice_new(UpgradeItem, 1);
> > -        upgrade_item->refs = 1;
> > -        pipe_item_init(&upgrade_item->base, PIPE_ITEM_TYPE_UPGRADE);
> > +        pipe_item_init_full(&upgrade_item->base, PIPE_ITEM_TYPE_UPGRADE,
> > +                            (GDestroyNotify)upgrade_item_free);
> >          upgrade_item->drawable = stream->current;
> >          upgrade_item->drawable->refs++;
> > +        upgrade_item->dcc = dcc;
> > +        red_channel_client_ref(RED_CHANNEL_CLIENT(upgrade_item->dcc));
> >          n_rects =
> >          pixman_region32_n_rects(&upgrade_item->drawable
> > ->tree_item.base.rgn);
> >          upgrade_item->rects = spice_malloc_n_m(n_rects, sizeof(SpiceRect),
> >          sizeof(SpiceClipRects));
> >          upgrade_item->rects->num_rects = n_rects;
> 
> Frediano