[PATCH] Bug #25136: Revert "Fix clipping when windows are used as sources"

[Soeren Sandmann]

Keith Packard <keithp at keithp.com> writes:

> On 27 Nov 2009 12:33:24 +0100, Soeren Sandmann <sandmann at daimi.au.dk> wrote:
> 
> > (a) The window can be partically offscreen and partially covered by
> >     other windows, which means the set of pixels in it that can be
> >     legally accessed is a possibly complex region, not just a
> >     rectangle.
> 
> The only question is what color we provide for pixels not within the
> window.
> 
> According to the spec, we're supposed to make them all be
> 0,0,0,0. Copying from an RGB to an ARGB pixmap is the only way to make
> this work according to the spec; making this efficient would involve
> computing the bounding box of the source as projected back through the
> transform (which isn't all that hard; we've got a transform function
> that computes the correct box).
> 
> However, I'd have to say that applications doing Render operations from
> partially obscured windows pretty much deserve whatever we provide for
> them; I'd bet they mostly don't care if we follow the spec (given that
> we never have).  The cheapest pixel values we can provide are those
> already present in whatever pixmap the source drawable is part of;
> clipping to the bounds of that pixmap will keep applications from
> crashing and do "something" similar to what they've always gotten. In
> particular, I'd bet they'd be happier with this than the current
> performance surprise.

I changed the spec to say that:

        - pixels *outside* the bounds are determined by the repeat
          property.

        - pixels *inside* the bounds that are obscured by other
          windows or are outside the screen have _undefined_ values.

This was precisely so that a driver could adjust the transformation
instead of copying if it wanted to.

It's certainly possible that noone cares whether X follows the render
spec, but with the copying, it actually does follow the spec as far as
I know. It is just unusably slow in the case where someone used 1
pixel from the root window as the source.

> > (d) We cannot completely ignore source clipping because there are
> >     applications that make use of it.
> 
> Are they using source clipping and transforms together? That's the hard
> case as we cannot simply convert one clipped operation into a sequence
> of smaller unclipped ones.

I don't think they are.

> >    How do you use this window as a source, producing correct results
> >    while not reading outside the screen boundaries?
> 
> I'd say the first issue is to ensure that we don't read outside of the
> screen boundaries, and that seems pretty easy; just clip the source
> reading operations to the size of the screen, a single rectangle which
> should be supported by hardware pretty easily.
> 
> Getting better results for clipped pixels is a secondary issue.

The bugs that will show up, if any, are that notification icons become
blank.

> > Copying the window to a temporary pixmap is simple and gets the right
> > answer in all cases, except that making accelerated callbacks from
> > within fb doesn't work, and that software copying is too slow for
> > large windows.
> 
> Why is this being done at the fb level? If we're going to use a
> temporary pixmap, we should push the problem up to the DIX level where
> we can actually accelerate the entire sequence of operations.

The reason I didn't was that that would require either mucking around
with the drawable inside the pictures, or creating new temporary
pictures, then passing those on to the driver.

It didn't seem worth it considering how rare it is to use windows as
sources.

> > Reverting the copying will allow out-of-bound reads by creating a
> > pixman image that points outside the screen.
> 
> How does the copying version ensure that the reads will all be
> in-bounds? We clearly clip to a pixmap at some point, and that code
> could just as easily clip to the screen pixmap as a temporary
> pixmap.

The copying version clips to the temporary pixmap thereby avoiding
out-of-bound reads.

If you clip to the screen pixmap, then you will need to adjust the
transformation to account for the coordinates of the drawable no
longer being right. Or ignore it, as you say.

> Yes, we'd get pixel values outside of the window; let's fix the crash
> and then argue about what the 'right' answer is.

The crash (and security hole - because you can write outside the
screen too) was introduced by reverting the copying patch. Before
that, with the Fedora patch, it was correct, just really slow in one
case.

It seems to me that the right fix is copy only the bounding box of the
transformed destination area, and just leave the other pixels in the
temporary pixmap undefined.



Soren