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

[Alexander Larsson]

On Fri, 2010-02-19 at 10:40 +0200, Izik Eidus wrote:
> On Thu, 18 Feb 2010 21:58:51 +0100 Alexander Larsson <alexl at redhat.com> wrote:
> 
> 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()

Ah, yeah, that sucks. Will fix.

> > +        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.

Yeah.

> 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?)

I agree, and it would be trivial to make SpicePoint and pixman_box32_t
identical. There are two differences atm, SpicePoint is packed, and the
coordinates are in another order. If we just changed the order and
removed the packed (which is unlikely to matter anyway in a struct with
4 int32_ts) then we could avoid all the conversions.

However, SpiceRect can't change, since its used as-is in the binary
format of the spice protocol...

Yet another reason we want to have a real demarshaller...


-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander Larsson                                            Red Hat, Inc 
       alexl at redhat.com            alexander.larsson at gmail.com 
He's a notorious Amish romance novelist from the 'hood. She's a high-kicking 
wisecracking barmaid in the wrong place at the wrong time. They fight crime!