Can you explain the choice to select arbitrarily some client as the "primary"? (why are the clients not treated the same way?)
On Tue, Apr 26, 2011 at 12:55 PM, Alon Levy <[email protected]> wrote: > --- > server/red_worker.c | 138 > +++++++++++++++++++++++++++++++++++++++++++++++++-- > 1 files changed, 134 insertions(+), 4 deletions(-) > > diff --git a/server/red_worker.c b/server/red_worker.c > index 6014cb3..399ebea 100644 > --- a/server/red_worker.c > +++ b/server/red_worker.c > @@ -8610,6 +8610,103 @@ static void init_surfaces(Surfaces *surfaces) > image_surface_init(surfaces); > } > > +static void free_surfaces(RedWorker *worker, Surfaces *surfaces) I would name it red_worker_free_surfaces() > + RingItem *link; > + RingItem *next; > + int count = 0; > + int i; > + > + // TODO: this prevents remove_drawable from sending anything. Is that > really > + // required? > + surfaces->dcc = NULL; > + > + RING_FOREACH_SAFE(link, next, &surfaces->current_list) { > + Drawable *drawable = SPICE_CONTAINEROF(link, Drawable, list_link); > + remove_drawable(worker, surfaces, drawable); > + count++; > + } > + red_printf("released %d drawables", count); > + // TODO - we should ensure the reference counts on the surfaces > + // have zeroed, and that there are no contexts > + for (i = 0 ; i < surfaces->n_surfaces ; ++i) { > + RedSurface *s = &surfaces->surfaces[i]; > + if (s->refs == 0) { > + continue; > + } > + ASSERT(s->context.canvas); > + red_destroy_surface(worker, surfaces, i); > + } > + free(surfaces); > +} > + > +static Surfaces *copy_surfaces(RedWorker *worker, DisplayChannelClient *dcc, > + Surfaces *orig) Could be named more descriptively: void display_channel_client_set_surfaces(DisplayChannelClient *dcc, Surfaces *surfaces); > +{ > + int i; > + Surfaces *surfaces = spice_malloc0(sizeof(Surfaces)); > + > + init_surfaces(surfaces); > + dcc->common.surfaces = surfaces; > + surfaces->dcc = dcc; > + surfaces->n_surfaces = orig->n_surfaces; > + surfaces->image_surfaces = orig->image_surfaces; // just ops table > + > + // must init streams before rest of copy, since red_add_current tries > + // to attach to stream. > + red_init_streams(dcc->common.surfaces); // TODO - move surfaces to dcc > + red_display_client_init_streams(dcc); > + > + for (i = 0 ; i < orig->n_surfaces ; ++i) { > + RedSurface *s = &orig->surfaces[i]; > + if (s->refs == 0) { // stop when we reached the last surface, > + // TODO - any better way then refs? > + continue; > + } > + ASSERT(s->context.canvas); > + red_create_surface(worker, surfaces, i, > + s->context.width, s->context.height, s->context.stride, > + s->context.format, s->context.line_0, > + TRUE /* surface is valid */, s->release_info); > + } > +#ifdef PIPE_DEBUG > + surfaces->last_id = 0; // orig->last_id; // TODO: does this make any > sense? > +#endif > + // clone the current_list, adding references to everything > + //current_list - we update all current_list's (both the main > + //and the per surface one) in this loop: > + //TODO: we definitely need to copy all the surfaces, but how do we copy > surface trees? > + // A. replay all current operations (which are by definition ordered > from oldest to > + // newest). This is what we currently do. > + // B. flatten each tree to a surface image, and send that. That's what > we actually > + // send to the client. Any reason to have the new client's tree equal to > the > + // worker tree? > + RingItem *link; > + int count = 0; > + RING_FOREACH(link, &orig->current_list) { > + Drawable *orig_drawable = SPICE_CONTAINEROF(link, Drawable, > list_link); > + uint32_t group_id = orig_drawable->group_id; > + RedDrawable *red_drawable = orig_drawable->red_drawable; > + red_process_drawable_surfaces(worker, surfaces, red_drawable, > + group_id); > + count++; > + } > + ASSERT(count == orig->drawable_count); > + ASSERT(surfaces->current_size <= orig->current_size); > + ASSERT(surfaces->drawable_count <= orig->drawable_count); > + ASSERT(surfaces->transparent_count <= orig->transparent_count); > + ASSERT(surfaces->shadows_count <= orig->shadows_count); > + ASSERT(surfaces->containers_count <= orig->containers_count); > + red_printf("current/drawable/transparent/shadows/containers: " > + "(%d,%d,%d,%d,%d)->(%d,%d,%d,%d,%d)", > + orig->current_size, orig->drawable_count, orig->transparent_count, > + orig->shadows_count, orig->containers_count, > + surfaces->current_size, surfaces->drawable_count, > + surfaces->transparent_count, surfaces->shadows_count, > + surfaces->containers_count); > + return surfaces; > +} > + > static void display_channel_client_disconnect(RedChannelClient *rcc) > { > // TODO: MC: right now we assume single channel > @@ -8641,7 +8738,36 @@ static void > display_channel_client_disconnect(RedChannelClient *rcc) > free(dcc->send_data.free_list.res); > red_display_destroy_streams(dcc); > red_channel_client_pipe_clear(rcc); // do this before deleting surfaces > - worker->surfaces.dcc = NULL; > + if (dcc->common.surfaces != &worker->surfaces) { > + free_surfaces(worker, dcc->common.surfaces); > + } else { > + // if this isn't the last client, free one client's surfaces > + // and let it have the worker's surfaces (this makes it the > "primary", > + // and we should (TODO) let him know new channels like sound are > + // now available. I wonder what the client will do :). > + if (display_channel->common.base.clients_num > 1) { > + // since this may have happened already, the client with the > + // worker surfaces can be anybody - since this is a doubly linked > + // list - we can check next and prev, one can't be the head, > + // and so will be our other. > + red_printf("swapping surfaces"); Perhaps a separate swap function would make sense for this operation. Or teach display_channel_client_set_surfaces() to deal with this case. > + RingItem *dcc_link = &dcc->common.base.channel_link; > + Ring *clients = &display_channel->common.base.clients; > + DisplayChannelClient *other = SPICE_CONTAINEROF( > + ring_next(clients, dcc_link), DisplayChannelClient, > + common.base.channel_link); > + if (other == NULL) { > + other = SPICE_CONTAINEROF(ring_prev(clients, dcc_link), > + DisplayChannelClient, common.base.channel_link); > + } > + ASSERT(other != dcc && other != NULL); > + free_surfaces(worker, other->common.surfaces); > + worker->surfaces.dcc = other; > + other->common.surfaces = &worker->surfaces; > + } else { > + worker->surfaces.dcc = NULL; > + } > + } > red_channel_client_disconnect(rcc); > } > > @@ -9746,9 +9872,13 @@ static void handle_new_display_channel(RedWorker > *worker, RedClient *client, Red > if (!listen_to_new_client_channel(&display_channel->common, &dcc->common, > stream)) { > goto error; > } > - dcc->common.surfaces = &worker->surfaces; > - dcc->common.surfaces->dcc = dcc; > - red_display_client_init_streams(dcc); > + if (display_channel->common.base.clients_num == 1) { > + dcc->common.surfaces = &worker->surfaces; > + dcc->common.surfaces->dcc = dcc; > + red_display_client_init_streams(dcc); > + } else { > + copy_surfaces(worker, dcc, &worker->surfaces); > + } Can't we handle first and other clients the same way? > on_new_display_channel_client(dcc); > return; > > -- > 1.7.4.4 > > _______________________________________________ > Spice-devel mailing list > [email protected] > http://lists.freedesktop.org/mailman/listinfo/spice-devel > -- Marc-André Lureau _______________________________________________ Spice-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/spice-devel
