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 <[email protected]> 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 > [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
