[Mesa-dev] [PATCH] i965/blorp: Implement destination clipping and scissoring

[Paul Berry]

On 22 May 2012 17:19, Eric Anholt <eric at anholt.net> wrote:

> On Mon, 14 May 2012 15:47:35 -0700, Paul Berry <stereotype441 at gmail.com>
> wrote:
> > This patch implements clipping and scissoring of the destination rect
> > for blits that use the blorp engine (e.g. MSAA blits).
>
> There's _mesa_clip_blit() used in swrast and state_tracker, which looks
> like we should be using instead of rolling our own.  Probably, we should
> just be doing that in core instead of pawning it off on drivers.
>

(Ian, I've added you to the Cc: list in case you have any thoughts, since
the two of us have been talking about this off-list)

Thanks for the pointer, Eric.  I wasn't aware of _mesa_clip_blit()'s
existence, since it isn't currently used by the i965 driver.

Now that I've read through it, I'm concerned that _mesa_clip_blit() may not
be spec compliant because when clipping a non-1:1 blit, it introduces
rounding errors.  For example, if you're blitting from a source rectangle
of (0, 0, 5, 5) to a destination rectangle of (0, 0, 10, 10), and your
scissor rectangle is (0, 0, 5, 5), in principle this should be clipped to a
blit from (0, 0, 2.5, 2.5) to (0, 0, 5, 5).  But since source coordinates
have to be integers, _mesa_clip_blit() instead causes the blit to be from
(0, 0, 3, 3) to (0, 0, 5, 5).  As a result, the image is expanded by a
factor of 5/3 instead of 2.  It's pretty clear from the spec that this
isn't the intended behaviour--p276 of the GL 3.0 spec (4.3.3 Copying
Pixels: Blitting Pixel Rectangles) says:

  "Whether or not the source or destination regions are altered due to
these limits, the scaling and offset applied to pixels being transferred is
performed as though no such limits were present."

If we do rounding as _mesa_clip_blit() does, we're likely to create
shimmering artefacts in programs that rely on clipping or scissoring their
blits.

Ordinarily I would go ahead and use _mesa_clip_blit(), and consider this a
bug in _mesa_clip_blit() to be fixed in a future patch, but in this case
the bug can't be fixed by changing _mesa_clip_blit().  The bug lies in the
incorrect assumption that for any clipped blit with integer source
coordinates, there's an equivalent unclipped blit with integer source
coordinates; that's simply not true for the general case of scaling blits.

Granted, at the moment brw_blorp_blit only does 1:1 blits, so using
_mesa_clip_blit() won't introduce any incorrect behaviour, but I'd rather
not proliferate re-use of code that doesn't comply with the spec.

There's also the unresolved question about what to do with clipping of
source coordinates, which we talked about in another thread ("Spec
interpretation question: glBlitFramebuffer behavior when src rectangle out
of range").  What we discovered in that thread was that nVidia behaviour is
to clip out source pixels that are completely out of range, and all other
implementations we investigated (including Mesa's blitting meta-op) behave
in a manner consistent with CLAMP_TO_EDGE texture sampling.  We tentatively
decided to assume that the non-nVidia behaviour is the correct one, though
I believe Ian is talking to Khronos folks to try to get a more definitive
answer.  _mesa_clip_blit()'s behaviour doesn't match any of the other
implementations we've investigated--it clips the source coordinates (as
nVidia does) but it alters the scaling and offset applied to pixels being
transferred, which none of the other implementations do and which the spec
explicitly forbids.

My preference would be to either replace or modify the _mesa_clip_blit()
function so that it's capable of reproducing the behaviour we see in other
GL implementations.  For instance, if we changed the type of its
src{X,Y}{0,1} arguments to GLdouble * instead of GLint *, then we could
ensure that clipping doesn't change the scaling and offset applied to
pixels (to within floating point rounding error), and we could choose at a
later time whether to do clipping or clamping for out-of-range source
rectangles.  Alternatively, we could make the caller of _mesa_clip_blit()
responsible for computing the correct scaling and offset based on the
unclipped source and destination coordinates, and then call
_mesa_clip_blit() just to determine which destination pixels to render (so
the src{X,Y}{0,1} arguments would be passed by value instead of by
reference).  Either of these alternatives, unfortunately, would probably
require some surgery in swrast and state_tracker, and I'm not familiar with
either of those codebases.

I'm open to other suggestions, but unless we have a strong reason to do
otherwise, I'd prefer to do something that moves us toward spec compliance,
and it seems like using _mesa_clip_blit() will move us in the opposite
direction.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20120523/a020d4f8/attachment.htm>