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?
