On 22 May 2012 17:19, Eric Anholt <<a href="mailto:eric@anholt.net" target="_blank">eric@anholt.net</a>> wrote: <div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div class="im">On Mon, 14 May 2012 15:47:35 -0700, Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>> wrote: > This patch implements clipping and scissoring of the destination rect > for blits that use the blorp engine (e.g. MSAA blits). </div>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. </blockquote></div> (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.