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

[Jim Graham]

I've filed JDK-8170514 to track this issue:

https://bugs.openjdk.java.net/browse/JDK-8170514

			...jim

On 11/29/16 10:11 PM, Jim Graham wrote:
> 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?