[Spice-devel] [PATCH 25/30] Replace custom region implementation with pixman_region32_t

[Izik Eidus]

On Thu, 18 Feb 2010 21:58:51 +0100
Alexander Larsson <alexl at redhat.com> wrote:

void red_pipe_item_init(PipeItem *item, int type)
> @@ -1904,13 +1904,13 @@ static inline void __exclude_region(RedWorker *worker, TreeItem *item, QRegion *
>  
>              if (draw->shadow) {
>                  Shadow *shadow;
> -                int32_t x = item->rgn.bbox.left;
> -                int32_t y = item->rgn.bbox.top;
> +                int32_t x = item->rgn.extents.x1;
> +                int32_t y = item->rgn.extents.y1;
>  
>                  region_exclude(&draw->base.rgn, &and_rgn);
>                  shadow = draw->shadow;
> -                region_offset(&and_rgn, shadow->base.rgn.bbox.left - x,
> -                              shadow->base.rgn.bbox.top - y);
> +                region_offset(&and_rgn, shadow->base.rgn.extents.x1 - x,
> +                              shadow->base.rgn.extents.y1 - y);
>                  region_exclude(&shadow->base.rgn, &and_rgn);
>                  region_and(&and_rgn, &shadow->on_hold);
>                  if (!region_is_empty(&and_rgn)) {
> @@ -4304,8 +4304,7 @@ static void red_draw_drawable(RedWorker *worker, Drawable *drawable)
>  #ifdef UPDATE_AREA_BY_TREE
>      //todo: add need top mask flag
>      worker->draw_funcs.set_top_mask(worker->surface.context.canvas,
> -                                    drawable->tree_item.base.rgn.num_rects,
> -                                    drawable->tree_item.base.rgn.rects);
> +                                    &drawable->tree_item.base.rgn);
>  #endif
>      red_draw_qxl_drawable(worker, drawable);
>  #ifdef UPDATE_AREA_BY_TREE
> @@ -6965,6 +6964,9 @@ static void red_display_send_upgrade(DisplayChannel *display_channel, UpgradeIte
>      RedChannel *channel;
>      QXLDrawable *qxl_drawable;
>      SpiceMsgDisplayDrawCopy *copy = &display_channel->send_data.u.copy;
> +    SpiceRect *rects;
> +    int num_rects;
> +    uint32_t num_rects32;
>  
>      ASSERT(display_channel && item && item->drawable);
>      channel = &display_channel->base;
> @@ -6980,8 +6982,11 @@ static void red_display_send_upgrade(DisplayChannel *display_channel, UpgradeIte
>      copy->base.box = qxl_drawable->bbox;
>      copy->base.clip.type = SPICE_CLIP_TYPE_RECTS;
>      copy->base.clip.data = channel->send_data.header.size;
> -    add_buf(channel, BUF_TYPE_RAW, &item->region.num_rects, sizeof(uint32_t), 0, 0);
> -    add_buf(channel, BUF_TYPE_RAW, item->region.rects, sizeof(SpiceRect) * item->region.num_rects, 0, 0);
> +    rects = region_dup_rects(&item->region, &num_rects);
> +    num_rects32 = num_rects;
> +    add_buf(channel, BUF_TYPE_RAW, &num_rects32, sizeof(uint32_t), 0, 0);
> +    add_buf(channel, BUF_TYPE_RAW, rects, sizeof(SpiceRect) * num_rects, 0, 0);
> +    free(rects);


So this is wrong, add_buf() will add the address of what rects is pointing to the
sending vector, but then you free() it before the server send it.

The way to deal with this would be freeing it at red_display_release_stream_clip()
and allocating the memory at the callers of __new_stream_clip()


>      copy->data = qxl_drawable->u.copy;
>      fill_bits(display_channel, &copy->data.src_bitmap, item->drawable);
>  
> @@ -7034,6 +7039,9 @@ static void red_display_send_stream_clip(DisplayChannel *display_channel,
>  
>      StreamAgent *agent = item->stream_agent;
>      Stream *stream = agent->stream;
> +    SpiceRect *rects;
> +    int num_rects;
> +    uint32_t num_rects32;
>  
>      ASSERT(stream);
>  
> @@ -7046,9 +7054,11 @@ static void red_display_send_stream_clip(DisplayChannel *display_channel,
>      } else {
>          ASSERT(stream_clip->clip.type == SPICE_CLIP_TYPE_RECTS);
>          stream_clip->clip.data = channel->send_data.header.size;
> -        add_buf(channel, BUF_TYPE_RAW, &item->region.num_rects, sizeof(uint32_t), 0, 0);
> -        add_buf(channel, BUF_TYPE_RAW, item->region.rects,
> -                item->region.num_rects * sizeof(SpiceRect), 0, 0);
> +	rects = region_dup_rects(&item->region, &num_rects);
> +	num_rects32 = num_rects;
> +        add_buf(channel, BUF_TYPE_RAW, &num_rects32, sizeof(uint32_t), 0, 0);
> +        add_buf(channel, BUF_TYPE_RAW, rects, num_rects * sizeof(SpiceRect), 0, 0);
> +	free(rects);

The same problem.

>      }
>      display_begin_send_massage(display_channel, item);
>  }



So about this patch just one more question:
It look to me like there are some cases we translate rects from one type into another
where we could have use one type in the begining?,
(I did see many cases that it is a most, But some looked like we could avoid this?)

Thanks