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 <al...@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@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> >
>
>
>
> --
> Marc-André Lureau
_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel
