[PULL] Various render changes

[Keith Packard]

On Fri, 15 Apr 2011 06:41:20 +0200, Soeren Sandmann <sandmann at cs.au.dk> wrote:
> Keith Packard <keithp at keithp.com> writes:
> 
> > On Thu, 07 Apr 2011 19:41:33 +0200, Soeren Sandmann <sandmann at cs.au.dk> wrote:
> >
> >> Søren Sandmann Pedersen (8):
> >>       Track damage for fbTrapezoids() and fbTriangles().
> >>       Add RegionInitBoxes(), and fix some buggy callers of RegionInit().
> >>       Make RegionInit() and RegionCreate() take just a box and no size
> >
> > Did you miss glx/glxdri2.c? I'm getting compile errors with that file.
> 
> Yes, apparently I did, and also a few in the rootless code. I'll send a
> new version pending the outcome of the discussion below.

Thanks.

> 
> > Also, if REGION_INIT is going to ignore an argument, you should simply
> > remove it from the API.
> 
> Well, the REGION_* macros were introduced in 
> 
>     7ef612de784daaed09ba13f4615c10714614033f
> 
> to preserve source compatibility when the pScreen argument went away, so
> changing their signature would make them pointless. If we are going to
> do that, why not delete them entirely? I would be fine with that, but it
> would belong in a different commit than this one.

Yeah, we should figure out how to get rid of these someday, but
separately. I instinctively suggested changing the REGION_INIT macro to
ensure that all uses were verified to comply with the new functionality,
but given that the argument wasn't really necessary for correct
semantics, it doesn't seem like we need to do that.

> > Do we want to just use the pixmap signatures here? RegionInitWithExtents
> > and RegionInit?
> 
> I don't have a strong opinion on that.

I'm good either way; just a suggestion on something that might make a
pixman transition a bit smaller.

> Yes, eventually, I think we should. There are currently a few features
> of the server regions that Pixman doesn't provide:
> 
> - There is no pixman_region_validate()

This was a semi-interesting optimization when we had toolkits creating
hundreds or thousands of windows. Having something which was explicitly
an invalid region would be a lot cleaner API than this mess.

> - Pixman doesn't have malloc'ed regions, like the ones povided by
>   RegionCreate().

That doesn't seem like a huge change...

> - The server uses 16 bit regions, but I think they do overflow in some
>   cases, such as when calling CopyArea on a large pixmap. We might want
>   to switch to 32 bit regions.

The server has a million checks all over the place to try and avoid
overflowing 16 bit regions. The only reason for using 16 bit regions was
memory, so we should probably do a bit of instrumentation to find out
what the memory impact would be. As above, I suspect with modern
toolkits using few windows that we just wouldn't see the same memory
usage anymore.

> - Pixman's region API is not all that great, in part because it was
>   inherited from the server, in part because it grew some idiosyncrasies
>   of its own. At some point rethinking the pixman API entirely will be
>   desirable.

If you're thinking of reworking that API, I'd like that to happen before
we switch the X server over...

> >>       Fix trapezoid and triangle rendering to windows
> >
> > My brain is full.
> 
> This one is getting urgent.
> 
> I don't want to force the issue, but I am going to release pixman 0.21.8
> with the corresponding pixman patch in it on Monday at the latest. When
> that happens, the combination of git X server and pixman 0.21.8 will
> have broken trapezoid rendering. Someone may bisect it down to
> 788ccb9a8bcf6a4f and ask to revert that, but the correct fix is the
> patch above. I'll send it a third time as a follow-up to this mail.

Yeah, we definitely need to figure this out, it's just a pile of code
that's non-trivial to review.

-- 
keith.packard at intel.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.x.org/archives/xorg-devel/attachments/20110415/30ae31b2/attachment.pgp>