>
> On Thu, 2016-04-21 at 12:41 +0100, Frediano Ziglio wrote:
> > From: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
> >
> > 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.
> > ---
> > server/dcc.c | 18 +-----------------
> > server/display-channel.c | 4 +---
> > server/display-channel.h | 7 ++++++-
> > server/stream.c | 15 +++++++++++++--
> > 4 files changed, 21 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..00e8a53 100644
> > --- a/server/display-channel.h
> > +++ b/server/display-channel.h
> > @@ -244,7 +244,12 @@ typedef struct SurfaceDestroyItem {
> >
> > typedef struct UpgradeItem {
> > PipeItem base;
> > - int refs;
> > + /**
> > + * Pointer to the display from which the drawable is allocated.
> > + * This pointer is safe to be retained as DisplayChannel
> > + * lifespan is bigger than all drawables.
> > + */
> > + DisplayChannel *display;
>
> OK, that seems reasonable to me with minor comment change: s/bigger/longer/
>
> Although now I'm wondering if we shouldn't just modify Drawable to store the
> DisplayChannel* instead. And then change
>
> display_channel_drawable_unref(DisplayChannel*, Drawable*)
> {
> /* do stuff */
> }
>
> to
>
> drawable_unref(Drawable*)
> {
> DisplayChannel *display = drawable->display;
> /* do stuff */
> }
>
Make sense too. More aggressive (more work to do).
If you want to try I approve the idea.
> > Drawable *drawable;
> > SpiceClipRects *rects;
> > } UpgradeItem;
> > diff --git a/server/stream.c b/server/stream.c
> > index ae37a62..f8d0abe 100644
> > --- a/server/stream.c
> > +++ b/server/stream.c
> > @@ -758,6 +758,16 @@ void stream_agent_stop(StreamAgent *agent)
> > }
> > }
> >
> > +static void upgrade_item_free(UpgradeItem *item)
> > +{
> > + g_return_if_fail(item != NULL);
> > + g_return_if_fail(item->base.refcount != 0);
> > +
> > + display_channel_drawable_unref(item->display, item->drawable);
> > + free(item->rects);
> > + 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 +808,11 @@ 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->display = display;
> > 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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel