[Spice-devel] [RFC v4 34/62] server/red_worker: remove more direct access to RedChannelClient.rcc
Marc-André Lureau
marcandre.lureau at gmail.com
Mon May 2 16:55:50 PDT 2011
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?
> 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?
{
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);
...
}
> 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