[Spice-devel] [RFC v4 34/62] server/red_worker: remove more direct access to RedChannelClient.rcc

Alon Levy alevy at redhat.com
Thu May 5 06:09:18 PDT 2011


On Tue, May 03, 2011 at 01:55:50AM +0200, Marc-André Lureau wrote:
> Is the description matching the patch changes?
> 
> (if it really does, then it could be merged with the next patch 35/62 I suppose)
> 
> On Tue, Apr 26, 2011 at 12:54 PM, Alon Levy <alevy at redhat.com> wrote:
> > ---
> >  server/red_worker.c |   97 ++++++++++++++++++++++++++++++---------------------
> >  1 files changed, 57 insertions(+), 40 deletions(-)
> >
> > diff --git a/server/red_worker.c b/server/red_worker.c
> > index a700f7e..3581173 100644
> > --- a/server/red_worker.c
> > +++ b/server/red_worker.c
> > @@ -1190,7 +1190,7 @@ static void red_pipe_add_verb(RedChannelClient* rcc, uint16_t verb)
> >
> >  static inline void red_create_surface_item(RedWorker *worker,
> >                                            DisplayChannelClient *dcc, int surface_id);
> > -static void red_add_surface_image(RedWorker *worker, int surface_id);
> > +static void red_push_surface_image(DisplayChannelClient *dcc, int surface_id);
> >  static void red_pipes_add_verb(RedChannel *channel, uint16_t verb)
> >  {
> >     RedChannelClient *rcc = channel->rcc;
> > @@ -1226,7 +1226,7 @@ static inline void red_handle_drawable_surfaces_client_synced(
> >             }
> >             red_create_surface_item(worker, dcc, surface_id);
> >             red_current_flush(worker, &worker->surfaces, surface_id);
> > -            red_add_surface_image(worker, surface_id);
> > +            red_push_surface_image(dcc, surface_id);
> >         }
> >     }
> >
> > @@ -1236,7 +1236,7 @@ static inline void red_handle_drawable_surfaces_client_synced(
> >
> >     red_create_surface_item(worker, dcc, drawable->surface_id);
> >     red_current_flush(worker, &worker->surfaces, drawable->surface_id);
> > -    red_add_surface_image(worker, drawable->surface_id);
> > +    red_push_surface_image(dcc, drawable->surface_id);
> >  }
> >
> >  static int display_connected(RedWorker *worker)
> > @@ -1305,23 +1305,23 @@ static inline void red_pipe_remove_drawable(DisplayChannelClient *dcc, Drawable
> >     }
> >  }
> >
> > -static inline void red_pipe_add_image_item(RedWorker *worker, ImageItem *item)
> > +static inline void red_pipe_add_image_item(DisplayChannelClient *dcc, ImageItem *item)
> >  {
> > -    if (!display_connected(worker)) {
> > +    if (!dcc) {
> >         return;
> >     }
> 
> Why is it safe to remove display_connected() which was added in 04/62?
> 

here we have a direct pointer to the client - this should be NULL iff no client exists.

> >     item->refs++;
> > -    red_channel_client_pipe_add(worker->display_channel->common.base.rcc, &item->link);
> > +    red_channel_client_pipe_add(&dcc->common.base, &item->link);
> >  }
> >
> > -static inline void red_pipe_add_image_item_after(RedWorker *worker, ImageItem *item,
> > +static inline void red_pipe_add_image_item_after(DisplayChannelClient *dcc, ImageItem *item,
> >                                                  PipeItem *pos)
> >  {
> > -    if (!display_connected(worker)) {
> > +    if (!dcc) {
> >         return;
> >     }
> 
> same
> 
> >     item->refs++;
> > -    red_channel_client_pipe_add_after(worker->display_channel->common.base.rcc, &item->link, pos);
> > +    red_channel_client_pipe_add_after(&dcc->common.base, &item->link, pos);
> >  }
> >
> >  static void release_image_item(ImageItem *item)
> > @@ -2647,15 +2647,12 @@ static void reset_rate(DisplayChannelClient *dcc, StreamAgent *stream_agent)
> >     /* MJpeg has no rate limiting anyway, so do nothing */
> >  }
> >
> > -static int display_channel_is_low_bandwidth(DisplayChannel *display_channel)
> > +static int display_channel_is_low_bandwidth(DisplayChannelClient *dcc)
> >  {
> > -    if (display_channel->common.base.rcc) {
> > -        if (main_channel_client_is_low_bandwidth(
> > -                red_client_get_main(display_channel->common.base.rcc->client))) {
> > -            return TRUE;
> > -        }
> > -    }
> > -    return FALSE;
> > +    RedChannelClient *rcc = &dcc->common.base;
> > +
> > +    return dcc && main_channel_client_is_low_bandwidth(
> > +                red_client_get_main(red_channel_client_get_client(rcc)));
> >  }
> 
> This rewrite is less readable.
> 
> It should check dcc before dereferencing, or the dcc check is useless.
> Perhaps you meant rcc instead?
> 

Actually &dcc->common.base should be safe even if dcc == NULL, since it's actually just
an offset (and a 0 offset at that).

> {
>     RedChannelClient *rcc;
> 
>     return_val_if_fail(dcc != NULL, FALSE);
>     rcc = &dcc->common.base;
> 
>     return main_channel_client_is_low_bandwidth(red_client_get_main(red_channel_client_get_client(rcc)));
> }
> 
> Ideally, one would use a simple type system to do checked cast and
> simplify the code:
> 
> {
>     RedChannelClient *rcc = RED_CHANNEL_CLIENT(dcc);
>     return_val_if_fail(rcc != NULL, FALSE);
> ...
> }
> 

I'm already basically using macros everywhere (well, not all, but aiming there) for common
casts, to make it less error prone, and a side result is that it could be easily changed
to use such a system you suggest when it becomes available.

> 
> >  static inline void pre_stream_item_swap(RedWorker *worker, Surfaces *surfaces, Stream *stream)
> > @@ -2664,7 +2661,7 @@ static inline void pre_stream_item_swap(RedWorker *worker, Surfaces *surfaces, S
> >
> >     ASSERT(stream->current);
> >
> > -    if (!dcc || !display_channel_is_low_bandwidth(worker->display_channel)) {
> > +    if (!dcc || !display_channel_is_low_bandwidth(dcc)) {
> >         return;
> >     }
> >
> > @@ -4272,6 +4269,14 @@ static CursorItem *get_cursor_item(RedWorker *worker, RedCursorCmd *cmd, uint32_
> >     return cursor_item;
> >  }
> >
> > +static PipeItem *ref_cursor_pipe_item(RedChannelClient *rcc, void *data, int num)
> 
> Prefixing with "ref" is not that great...
> 
> The corresponding function argument is "new_pipe_item_t creator". So
> we have "ref", "new", "creator"...
> 
> It makes reading easier to use similar vocabulary, example:
> 
> Argument "RedChannelClientGetPipeItem getter" and function name
> "red_channel_client_cursor_item_ref".
> 
> I know this proposal is not that great either, so don't bother
> debating it. And let's leave it as it is for now.
> 
> > +{
> > +    CursorItem *cursor_item = data;
> > +
> > +    cursor_item->refs += (num != 0); /* we already reference the first use */
> 
> Ah! That's what "num" is for. Couldn't we just unref() later? so we
> can ref() unconditionally and avoid passing an extra "num"?
> 
> > +    return &cursor_item->pipe_data;
> > +}
> > +
> >  void qxl_process_cursor(RedWorker *worker, RedCursorCmd *cursor_cmd, uint32_t group_id)
> >  {
> >     CursorItem *item;
> > @@ -4306,9 +4311,10 @@ void qxl_process_cursor(RedWorker *worker, RedCursorCmd *cursor_cmd, uint32_t gr
> >         red_error("invalid cursor command %u", cursor_cmd->type);
> >     }
> >
> > -    if (worker->cursor_channel && (worker->mouse_mode == SPICE_MOUSE_MODE_SERVER ||
> > +    if (cursor_connected(worker) && (worker->mouse_mode == SPICE_MOUSE_MODE_SERVER ||
> >                                    cursor_cmd->type != QXL_CURSOR_MOVE || cursor_show)) {
> > -        red_channel_client_pipe_add(worker->cursor_channel->common.base.rcc, &item->pipe_data);
> > +        red_channel_pipes_new_add(&worker->cursor_channel->common.base, ref_cursor_pipe_item,
> > +                                  (void*)item);
> >     } else {
> >         red_release_cursor(worker, item);
> >     }
> > @@ -4482,29 +4488,35 @@ static void red_current_flush(RedWorker *worker, Surfaces *surfaces, int surface
> >  }
> >
> >  // adding the pipe item after pos. If pos == NULL, adding to head.
> > -static ImageItem *red_add_surface_area_image(RedWorker *worker, int surface_id, SpiceRect *area,
> > +static ImageItem *red_add_surface_area_image(Surfaces *surfaces, int surface_id, SpiceRect *area,
> >                                              PipeItem *pos, int can_lossy)
> >  {
> > +    DisplayChannelClient *dcc = surfaces->dcc;
> > +    RedWorker *worker = dcc ? DCC_TO_WORKER(dcc) : NULL;
> > +    DisplayChannel *display_channel = worker ? worker->display_channel : NULL;
> > +    RedChannel *channel = &display_channel->common.base;
> > +    RedSurface *surface = &surfaces->surfaces[surface_id];
> > +    SpiceCanvas *canvas = surface->context.canvas;
> >     ImageItem *item;
> >     int stride;
> >     int width;
> >     int height;
> > -    RedSurface *surface = &worker->surfaces.surfaces[surface_id];
> > -    SpiceCanvas *canvas = surface->context.canvas;
> >     int bpp;
> >     int all_set;
> >
> >     ASSERT(area);
> >
> > +    if (!dcc) {
> > +        return NULL;
> > +    }
> >     width = area->right - area->left;
> >     height = area->bottom - area->top;
> >     bpp = SPICE_SURFACE_FMT_DEPTH(surface->context.format) / 8;
> > -    stride = SPICE_ALIGN(width * bpp, 4);
> > -
> > +    stride = SPICE_ALIGN(width * bpp, 4);
> > +
> >     item = (ImageItem *)spice_malloc_n_m(height, stride, sizeof(ImageItem));
> >
> > -    red_channel_pipe_item_init(&worker->display_channel->common.base,
> > -                               &item->link, PIPE_ITEM_TYPE_IMAGE);
> > +    red_channel_pipe_item_init(channel, &item->link, PIPE_ITEM_TYPE_IMAGE);
> >
> >     item->refs = 1;
> >     item->surface_id = surface_id;
> > @@ -4534,9 +4546,9 @@ static ImageItem *red_add_surface_area_image(RedWorker *worker, int surface_id,
> >     }
> >
> >     if (!pos) {
> > -        red_pipe_add_image_item(worker, item);
> > +        red_pipe_add_image_item(dcc, item);
> >     } else {
> > -        red_pipe_add_image_item_after(worker, item, pos);
> > +        red_pipe_add_image_item_after(dcc, item, pos);
> >     }
> >
> >     release_image_item(item);
> > @@ -4544,25 +4556,28 @@ static ImageItem *red_add_surface_area_image(RedWorker *worker, int surface_id,
> >     return item;
> >  }
> >
> > -static void red_add_surface_image(RedWorker *worker, int surface_id)
> > +static void red_push_surface_image(DisplayChannelClient *dcc, int surface_id)
> >  {
> >     SpiceRect area;
> >     RedSurface *surface;
> > +    Surfaces *surfaces;
> >
> > -    surface = &worker->surfaces.surfaces[surface_id];
> > -
> > -    if (!display_connected(worker) || !surface->context.canvas) {
> > +    if (!dcc) {
> > +        return;
> > +    }
> > +    surfaces = dcc->common.surfaces;
> > +    surface = &surfaces->surfaces[surface_id];
> > +    if (!surface->context.canvas) {
> >         return;
> >     }
> > -
> >     area.top = area.left = 0;
> >     area.right = surface->context.width;
> >     area.bottom = surface->context.height;
> >
> >     /* not allowing lossy compression because probably, especially if it is a primary surface,
> >        it combines both "picture-like" areas with areas that are more "artificial"*/
> > -    red_add_surface_area_image(worker, surface_id, &area, NULL, FALSE);
> > -    red_channel_push(&worker->display_channel->common.base);
> > +    red_add_surface_area_image(surfaces, surface_id, &area, NULL, FALSE);
> > +    red_channel_client_push(&dcc->common.base);
> >  }
> >
> >  typedef struct {
> > @@ -6359,8 +6374,10 @@ static void red_pipe_replace_rendered_drawables_with_images(RedWorker *worker,
> >                                                             int first_surface_id,
> >                                                             SpiceRect *first_area)
> >  {
> > +    /* TODO: can't have those statics with multiple clients */
> >     static int resent_surface_ids[MAX_PIPE_SIZE];
> >     static SpiceRect resent_areas[MAX_PIPE_SIZE]; // not pointers since drawbales may be released
> > +    Surfaces *surfaces = dcc->common.surfaces;
> >     int num_resent;
> >     PipeItem *pipe_item;
> >     Ring *pipe;
> > @@ -6394,7 +6411,7 @@ static void red_pipe_replace_rendered_drawables_with_images(RedWorker *worker,
> >             continue;
> >         }
> >
> > -        image = red_add_surface_area_image(worker, drawable->red_drawable->surface_id,
> > +        image = red_add_surface_area_image(surfaces, drawable->red_drawable->surface_id,
> >                                            &drawable->red_drawable->bbox, pipe_item, TRUE);
> >         resent_surface_ids[num_resent] = drawable->red_drawable->surface_id;
> >         resent_areas[num_resent] = drawable->red_drawable->bbox;
> > @@ -6451,7 +6468,7 @@ static void red_add_lossless_drawable_dependencies(RedWorker *worker,
> >         // the surfaces areas will be sent as DRAW_COPY commands, that
> >         // will be executed before the current drawable
> >         for (i = 0; i < num_deps; i++) {
> > -            red_add_surface_area_image(worker, deps_surfaces_ids[i], deps_areas[i],
> > +            red_add_surface_area_image(surfaces, deps_surfaces_ids[i], deps_areas[i],
> >                                        red_pipe_get_tail(dcc), FALSE);
> >
> >         }
> > @@ -6472,7 +6489,7 @@ static void red_add_lossless_drawable_dependencies(RedWorker *worker,
> >                                                             &drawable->bbox);
> >         }
> >
> > -        red_add_surface_area_image(worker, drawable->surface_id, &drawable->bbox,
> > +        red_add_surface_area_image(surfaces, drawable->surface_id, &drawable->bbox,
> >                                    red_pipe_get_tail(dcc), TRUE);
> >     }
> >  }
> > @@ -8827,7 +8844,7 @@ static void on_new_display_channel_client(DisplayChannelClient *dcc)
> >     if (surfaces->surfaces[0].context.canvas) {
> >         red_current_flush(worker, surfaces, 0);
> >         push_new_primary_surface(dcc);
> > -        red_add_surface_image(worker, 0);
> > +        red_push_surface_image(dcc, 0);
> >         if (red_channel_is_connected(&display_channel->common.base)) {
> >             red_pipe_add_verb(rcc, SPICE_MSG_DISPLAY_MARK);
> >             red_disply_start_streams(dcc);
> > --
> > 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