<Swing Dev> [9] Review request for 8162350 RepaintManager shifts repainted region when the floating point UI scale is used

[Jim Graham]

I took another look at the use of clipScale/clipRound in the SG2D.drawHiDPI method.

The sub-image parameters are given relative to the base image and the intention (ignoring scaling and DPI variants) is 
to extract those complete pixels from the (base) source images and do the rendering operation restricted to those 
pixels.  For the default case of using NEAREST_NEIGHBOR interpolation, the concept of "extracting those pixels" isn't 
really interesting because those coordinates fall on some pixel and we only sample those pixels.  For the case of 
LINEAR_INTERPOLATION, though, we can sometimes sample from pixels that are to the outside of the sample coordinates we 
see.  To illustrate this, consider the following linear interpolation math:

- location 0.5 is 100% sampled from the pixel at location 0
- location 1.5 is 100% sampled from the pixel at location 1
- location 0.75 is 75% sampled from pixel 0 and 25% from pixel 1
- location 1.0 is 50% of the pixels at location 0 and 1
- location 1.25 is 25% of pixel 0 and 75% of pixel 1

So, while it might not be surprising that sample coordinates from 0.5 -> 0.99 take some color information from pixel 0, 
it would be somewhat surprising that a sample coordinate of 1.25 (or even 1.0) takes some color information from pixel 0 
because it seems like we've started the sub-image "on pixel 1", so why isn't that the limit of which pixels we sample?

If we expand the drawImage() loops to handle floating point sub-pixel coordinates for the source rectangle and linear 
interpolation is enabled then a sub-image rectangle that is scaled and starts at 1.25 might include some of the color 
from pixel 0 if we aren't careful, which appears to violate the concept of extracting some pixels.

On the other hand, if we round the sub-image coordinates to integers so that the current loops physically limit the 
pixels that are considered for sampling without having to rewrite them, then we apply a stretch/compression to the 
source rectangle we were given which changes which pixels are sampled for a given output pixel all across the image 
operation.

Neither of those outcomes is ideal, but the latter involves fewer code changes in the short term.

My vote would be for eventually modifying the drawImage pipeline and primitives to take float sub-image coordinates and 
apply them using an algorithm that:

- first pretends that the following pixels from the image have been extracted:
     extracted_x1 = floor(x1)
     extracted_y1 = floor(y1)
     extracted_x2 = ceil(x2)
     extracted_y2 = ceil(y2)
(clipped against the bounds of the image as the current x1y1x2y2 are)
- then sample relative to those extracted pixels using the sampling coordinates:
     x1 - extracted_x1
     y1 - extracted_y1
     x2 - extracted_x1
     y2 - extracted_y1
(note that all 4 have "extracted_xy1" subtracted from them because that is the new "effective origin" of the image being 
sampled)

Until we put in that work to modify the drawImage pipe and primitives, though, all we have at our disposal is to choose 
new integer sub-image coordinates and I don't think the subtle differences between clipScale() and clipRound() matter in 
this near-term work-around.

clipScale() does a basic round which means that left and top edges that scale to a pixel center will not include that 
pixel and right and bottom edges that scale to a pixel center will include those pixels.

clipRound() (which I think needs to be renamed because the name implies that it does what clipScale does) does a 
ceil(v-0.5) operation which is consistent with our fill rule and means that the decisions I mentioned for clipScale() 
about scaled sub-image edges falling on a pixel boundary are reversed.

For example, if the base image is 10x10 and the DPI variant is for 1.5 scale and it is 15x15, then a sub-image request 
for x1,y1,x2,y2 = (1, 1, 3, 3) maps to (1.5, 1.5, 4.5, 4.5).  Do we want to use the pixels at (1, 1, 4, 4) or (2, 2, 5, 
5) or (1, 1, 5, 5) or (2, 2, 4, 4)?  The first represents what the fill policy would do with a rectangle scaled like 
that (and matches clipRound).  The second represents a simple round (i.e. clipScale).  The third includes any pixel that 
has any part of it in the requested region - floor, floor, ceil, ceil, and the 4th includes only pixels that are 
completely in the region - ceil, ceil, floor, floor.

One thing to consider is that as long as the DPI variants are all scales >1.0 then any non-empty sub-image rectangle 
will map to a non-empty scaled sub-image rectangle for any of the variants, but if we ever have a DPI variant that is 
smaller than the base image then only the 3rd variant (floor, floor, ceil, ceil) will map to a non-empty source rectangle.

Have I confused enough people at this point with the subtleties of this code?  :(

I should submit a bug to improve our image pipeline for scaled sub-images...

			...jim


On 11/29/16 3:11 PM, Jim Graham wrote:
> clipRound() has appropriate logic for matching the results of filling a path.  clipScale() is a more basic "just scale
> this value and clip it" type of operation that has uses - not everything needs to match the rasterization logic of
> fills.  I did a quick grep to see where clipScale() is used:
>
> - SG2D.drawHiDPI() - this fix changes one of the instances of using clipScale() in that method, but not the other one.
> They should probably both match.  I don't have a good answer for which is the best rounding method to use here in any
> case since the original concept was that we'd extract the integer pixels and pretend that they were an isolated image,
> but if we end up with a fractional scale then that isn't really possible.  It may be better to just leave this alone for
> now.  Does anything about the rest of this fix depend on that change?  Hopefully if RepaintManager does its job right we
> end up painting the back buffer to the destination all on integer pixel boundaries on both the source and dest and so
> rounding isn't an issue?