[Spice-devel] [RFC v4 45/62] server/red_worker: copy and free new surfaces after first client
Marc-André Lureau
marcandre.lureau at gmail.com
Mon May 2 16:55:04 PDT 2011
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 <alevy at redhat.com> 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
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>
--
Marc-André Lureau
More information about the Spice-devel
mailing list