Re: [OpenJDK 2D-Dev] [9] Review Request: 8167310 The graphics clip is incorrectly rounded for some fractional scales
Tweaking might reduce the number of cases where the reverse transform makes a different edge condition decision than the forward transform, but it will never completely eliminate it and so the refine code (which exists because forward computations don't always match reverse computations) would still be needed. Both could be part of the same fix - make the refinement more accurate and also consider if a better approximation of the transform would make the refinement actions more rare... ...jim On 11/1/2016 8:30 AM, Sergey Bylokhov wrote: But probably it is possible to tweak the transform? I mean that we can apply the "round", then calculate the diff between resulted destination and destination based on transform, and shift/rescale the transform so it will map exactly the pixels from source edges to destination edges. On 29.10.16 0:47, Jim Graham wrote: I suppose that there could be a 4th bug about the fact that the trimming/refining code in the native TransformHelper is based on an inverse transform and subject to bit error, but it's not easy to figure out how to refine it further. Note that it could also cause us to omit a row of pixels in some cases if the rounding went the other way. Refining it further could be easier than the 2nd two solutions, but not really noticeable if we just get do the more conservative calculations on the bounds equations in the setup code... ...jim On 10/28/16 2:33 PM, Jim Graham wrote: Hi Alexandr, That code is coming up with maximum extents for the operation, not the exact edges that will be rendered. It passes them down to the native TransformHelper method which refines them using the exact calculations it will use to render the pixels. Admittedly, the calculations you point out below in the DrawImage pipeline would more accurately note that the right/bottom edges fall on a .5 pixel location and shouldn't be included, but I didn't want to make that decision using one set of equations while the actual rendering uses different math down below. Note that if you had a rotation then there are lots of unused pixels on each scanline which are computed and refined by the native code. Those calculations would also be performing the "included/excluded" calculations using the actual rendering equations rather than these higher level boundary equations so even if we switched to "ceil(N - 0.5)" calculations up here for the bounding box to cure the scaled drawImage we'd still potentially have overshoot due to the fixed point/floating point bit errors on those rotated edges. With a general transform it would be difficult to detect the occasional extra pixel being rendered due to bit errors, but it is much easier with these scale-only transforms. One change we could make would be to do the ceil(N-0.5) calculation in the bounds setup code which would "fix" the scale only case. Another might be to fix the scale pipeline to handle arbitrary formats - we could write a related "ScaleHelper" native method that would do the same thing that TransformHelper does. TransformHelper takes a transform loop that transforms pixels to ARGB, and a separate Blit or MaskBlit that blends ARGB pixels into the destination and it runs the first loop into a temporary local buffer on the C call stack and the second loop to blend the pixels into the dest. A ScaleHelper loop would do the same, but with a "Scale(XXX to ARGB)" loop for the first half. Finally, we could also add more ScaleBlit loops for more source/dest pairs so these scales would happen more directly. Unfortunately, many of the missing cases would require creating scale loops that also do blending and the only macros we currently have for direct src->dest scaling are opaque-to-opaque pixel store operations. The first solution is simpler and easier to add, the second one would help with all scales that fall outside our loops. The last solution would arguably provide the highest performance for any source/dest case that we care about (and with the advent of HiDPI we now care about a lot more cases). I'd file 3 bugs: - Scaled and transformed drawImage can overshoot with some scales (1.5) - Add more ScaledBlit graphics primitive instances for common HiDPI cases - Create a general purpose Scale helper method along the lines of TransformHelper The first could be fixed soon with the proposed change you talk about below... ...jim On 10/28/16 6:40 AM, Alexandr Scherbatiy wrote: On 10/11/2016 10:13 PM, Sergey Bylokhov wrote: On 11.10.16 21:54, Jim Graham wrote: Additionally, for AA rendering, there is no such thing as "setClip(someShape)" and "fill(sameShape)" being identical no matter how much we predict which rules they may want because clips are inherently non-AA and so the fuzzy pixels along the boundary of an AA shape will be randomly clipped or included. When all is said and done I feel (not very strongly) it would be best to have clipping remain unaffected by the biasing
Re: [OpenJDK 2D-Dev] [9] Review Request: 8167310 The graphics clip is incorrectly rounded for some fractional scales
But probably it is possible to tweak the transform? I mean that we can apply the "round", then calculate the diff between resulted destination and destination based on transform, and shift/rescale the transform so it will map exactly the pixels from source edges to destination edges. On 29.10.16 0:47, Jim Graham wrote: I suppose that there could be a 4th bug about the fact that the trimming/refining code in the native TransformHelper is based on an inverse transform and subject to bit error, but it's not easy to figure out how to refine it further. Note that it could also cause us to omit a row of pixels in some cases if the rounding went the other way. Refining it further could be easier than the 2nd two solutions, but not really noticeable if we just get do the more conservative calculations on the bounds equations in the setup code... ...jim On 10/28/16 2:33 PM, Jim Graham wrote: Hi Alexandr, That code is coming up with maximum extents for the operation, not the exact edges that will be rendered. It passes them down to the native TransformHelper method which refines them using the exact calculations it will use to render the pixels. Admittedly, the calculations you point out below in the DrawImage pipeline would more accurately note that the right/bottom edges fall on a .5 pixel location and shouldn't be included, but I didn't want to make that decision using one set of equations while the actual rendering uses different math down below. Note that if you had a rotation then there are lots of unused pixels on each scanline which are computed and refined by the native code. Those calculations would also be performing the "included/excluded" calculations using the actual rendering equations rather than these higher level boundary equations so even if we switched to "ceil(N - 0.5)" calculations up here for the bounding box to cure the scaled drawImage we'd still potentially have overshoot due to the fixed point/floating point bit errors on those rotated edges. With a general transform it would be difficult to detect the occasional extra pixel being rendered due to bit errors, but it is much easier with these scale-only transforms. One change we could make would be to do the ceil(N-0.5) calculation in the bounds setup code which would "fix" the scale only case. Another might be to fix the scale pipeline to handle arbitrary formats - we could write a related "ScaleHelper" native method that would do the same thing that TransformHelper does. TransformHelper takes a transform loop that transforms pixels to ARGB, and a separate Blit or MaskBlit that blends ARGB pixels into the destination and it runs the first loop into a temporary local buffer on the C call stack and the second loop to blend the pixels into the dest. A ScaleHelper loop would do the same, but with a "Scale(XXX to ARGB)" loop for the first half. Finally, we could also add more ScaleBlit loops for more source/dest pairs so these scales would happen more directly. Unfortunately, many of the missing cases would require creating scale loops that also do blending and the only macros we currently have for direct src->dest scaling are opaque-to-opaque pixel store operations. The first solution is simpler and easier to add, the second one would help with all scales that fall outside our loops. The last solution would arguably provide the highest performance for any source/dest case that we care about (and with the advent of HiDPI we now care about a lot more cases). I'd file 3 bugs: - Scaled and transformed drawImage can overshoot with some scales (1.5) - Add more ScaledBlit graphics primitive instances for common HiDPI cases - Create a general purpose Scale helper method along the lines of TransformHelper The first could be fixed soon with the proposed change you talk about below... ...jim On 10/28/16 6:40 AM, Alexandr Scherbatiy wrote: On 10/11/2016 10:13 PM, Sergey Bylokhov wrote: On 11.10.16 21:54, Jim Graham wrote: Additionally, for AA rendering, there is no such thing as "setClip(someShape)" and "fill(sameShape)" being identical no matter how much we predict which rules they may want because clips are inherently non-AA and so the fuzzy pixels along the boundary of an AA shape will be randomly clipped or included. When all is said and done I feel (not very strongly) it would be best to have clipping remain unaffected by the biasing of STROKE hints, but part of that is driven by the fact that I think it can be fixed with a couple of lines of code in SG2D/LoopPipe... I guess a bottom line is that it is require an additional investigation. I am not exactly sure, but my expectation is that fillRect(x,y,w,h)/drawImage(x,y,w,h) + setClip(x,y,w,h) should work in a similar way(covers the same pixels). And the question should this behavior be exactly the same as fill(RectShape) + setClip(RectShape) is unclear (I am not sure is it a critical issue or not) Right now I tried to fix overlapping, which produ
Re: [OpenJDK 2D-Dev] [9] Review Request: 8167310 The graphics clip is incorrectly rounded for some fractional scales
I suppose that there could be a 4th bug about the fact that the trimming/refining code in the native TransformHelper is based on an inverse transform and subject to bit error, but it's not easy to figure out how to refine it further. Note that it could also cause us to omit a row of pixels in some cases if the rounding went the other way. Refining it further could be easier than the 2nd two solutions, but not really noticeable if we just get do the more conservative calculations on the bounds equations in the setup code... ...jim On 10/28/16 2:33 PM, Jim Graham wrote: Hi Alexandr, That code is coming up with maximum extents for the operation, not the exact edges that will be rendered. It passes them down to the native TransformHelper method which refines them using the exact calculations it will use to render the pixels. Admittedly, the calculations you point out below in the DrawImage pipeline would more accurately note that the right/bottom edges fall on a .5 pixel location and shouldn't be included, but I didn't want to make that decision using one set of equations while the actual rendering uses different math down below. Note that if you had a rotation then there are lots of unused pixels on each scanline which are computed and refined by the native code. Those calculations would also be performing the "included/excluded" calculations using the actual rendering equations rather than these higher level boundary equations so even if we switched to "ceil(N - 0.5)" calculations up here for the bounding box to cure the scaled drawImage we'd still potentially have overshoot due to the fixed point/floating point bit errors on those rotated edges. With a general transform it would be difficult to detect the occasional extra pixel being rendered due to bit errors, but it is much easier with these scale-only transforms. One change we could make would be to do the ceil(N-0.5) calculation in the bounds setup code which would "fix" the scale only case. Another might be to fix the scale pipeline to handle arbitrary formats - we could write a related "ScaleHelper" native method that would do the same thing that TransformHelper does. TransformHelper takes a transform loop that transforms pixels to ARGB, and a separate Blit or MaskBlit that blends ARGB pixels into the destination and it runs the first loop into a temporary local buffer on the C call stack and the second loop to blend the pixels into the dest. A ScaleHelper loop would do the same, but with a "Scale(XXX to ARGB)" loop for the first half. Finally, we could also add more ScaleBlit loops for more source/dest pairs so these scales would happen more directly. Unfortunately, many of the missing cases would require creating scale loops that also do blending and the only macros we currently have for direct src->dest scaling are opaque-to-opaque pixel store operations. The first solution is simpler and easier to add, the second one would help with all scales that fall outside our loops. The last solution would arguably provide the highest performance for any source/dest case that we care about (and with the advent of HiDPI we now care about a lot more cases). I'd file 3 bugs: - Scaled and transformed drawImage can overshoot with some scales (1.5) - Add more ScaledBlit graphics primitive instances for common HiDPI cases - Create a general purpose Scale helper method along the lines of TransformHelper The first could be fixed soon with the proposed change you talk about below... ...jim On 10/28/16 6:40 AM, Alexandr Scherbatiy wrote: On 10/11/2016 10:13 PM, Sergey Bylokhov wrote: On 11.10.16 21:54, Jim Graham wrote: Additionally, for AA rendering, there is no such thing as "setClip(someShape)" and "fill(sameShape)" being identical no matter how much we predict which rules they may want because clips are inherently non-AA and so the fuzzy pixels along the boundary of an AA shape will be randomly clipped or included. When all is said and done I feel (not very strongly) it would be best to have clipping remain unaffected by the biasing of STROKE hints, but part of that is driven by the fact that I think it can be fixed with a couple of lines of code in SG2D/LoopPipe... I guess a bottom line is that it is require an additional investigation. I am not exactly sure, but my expectation is that fillRect(x,y,w,h)/drawImage(x,y,w,h) + setClip(x,y,w,h) should work in a similar way(covers the same pixels). And the question should this behavior be exactly the same as fill(RectShape) + setClip(RectShape) is unclear (I am not sure is it a critical issue or not) Right now I tried to fix overlapping, which produce visible artifacts and were considered as a bugs. The next issue which I would like to fix is a overlapping of drawImage(). I just created a small sample [1] which fills a rectangle (x, y, w, y), creates an image with size (w, h) and draws the image into the area (x, y, w, h).
Re: [OpenJDK 2D-Dev] [9] Review Request: 8167310 The graphics clip is incorrectly rounded for some fractional scales
Hi Alexandr, That code is coming up with maximum extents for the operation, not the exact edges that will be rendered. It passes them down to the native TransformHelper method which refines them using the exact calculations it will use to render the pixels. Admittedly, the calculations you point out below in the DrawImage pipeline would more accurately note that the right/bottom edges fall on a .5 pixel location and shouldn't be included, but I didn't want to make that decision using one set of equations while the actual rendering uses different math down below. Note that if you had a rotation then there are lots of unused pixels on each scanline which are computed and refined by the native code. Those calculations would also be performing the "included/excluded" calculations using the actual rendering equations rather than these higher level boundary equations so even if we switched to "ceil(N - 0.5)" calculations up here for the bounding box to cure the scaled drawImage we'd still potentially have overshoot due to the fixed point/floating point bit errors on those rotated edges. With a general transform it would be difficult to detect the occasional extra pixel being rendered due to bit errors, but it is much easier with these scale-only transforms. One change we could make would be to do the ceil(N-0.5) calculation in the bounds setup code which would "fix" the scale only case. Another might be to fix the scale pipeline to handle arbitrary formats - we could write a related "ScaleHelper" native method that would do the same thing that TransformHelper does. TransformHelper takes a transform loop that transforms pixels to ARGB, and a separate Blit or MaskBlit that blends ARGB pixels into the destination and it runs the first loop into a temporary local buffer on the C call stack and the second loop to blend the pixels into the dest. A ScaleHelper loop would do the same, but with a "Scale(XXX to ARGB)" loop for the first half. Finally, we could also add more ScaleBlit loops for more source/dest pairs so these scales would happen more directly. Unfortunately, many of the missing cases would require creating scale loops that also do blending and the only macros we currently have for direct src->dest scaling are opaque-to-opaque pixel store operations. The first solution is simpler and easier to add, the second one would help with all scales that fall outside our loops. The last solution would arguably provide the highest performance for any source/dest case that we care about (and with the advent of HiDPI we now care about a lot more cases). I'd file 3 bugs: - Scaled and transformed drawImage can overshoot with some scales (1.5) - Add more ScaledBlit graphics primitive instances for common HiDPI cases - Create a general purpose Scale helper method along the lines of TransformHelper The first could be fixed soon with the proposed change you talk about below... ...jim On 10/28/16 6:40 AM, Alexandr Scherbatiy wrote: On 10/11/2016 10:13 PM, Sergey Bylokhov wrote: On 11.10.16 21:54, Jim Graham wrote: Additionally, for AA rendering, there is no such thing as "setClip(someShape)" and "fill(sameShape)" being identical no matter how much we predict which rules they may want because clips are inherently non-AA and so the fuzzy pixels along the boundary of an AA shape will be randomly clipped or included. When all is said and done I feel (not very strongly) it would be best to have clipping remain unaffected by the biasing of STROKE hints, but part of that is driven by the fact that I think it can be fixed with a couple of lines of code in SG2D/LoopPipe... I guess a bottom line is that it is require an additional investigation. I am not exactly sure, but my expectation is that fillRect(x,y,w,h)/drawImage(x,y,w,h) + setClip(x,y,w,h) should work in a similar way(covers the same pixels). And the question should this behavior be exactly the same as fill(RectShape) + setClip(RectShape) is unclear (I am not sure is it a critical issue or not) Right now I tried to fix overlapping, which produce visible artifacts and were considered as a bugs. The next issue which I would like to fix is a overlapping of drawImage(). I just created a small sample [1] which fills a rectangle (x, y, w, y), creates an image with size (w, h) and draws the image into the area (x, y, w, h). With the floating point scale like 1.5 the image does not exactly fits the area filled by the rectangle (see image [2] generated by the code [1]). This is because the Graphics.drawImage(...) calls DrawImage.renderImageXform() which uses floor and ceil methods for the region corner rounding: final int dx1 = Math.max((int) Math.floor(ddx1), clip.getLoX()); final int dy1 = Math.max((int) Math.floor(ddy1), clip.getLoY()); final int dx2 = Math.min((int) Math.ceil(ddx2), clip.getHiX()); final int dy2 = Math.min((int) Math.ceil(ddy2), clip.getHiY());
Re: [OpenJDK 2D-Dev] [9] Review Request: 8167310 The graphics clip is incorrectly rounded for some fractional scales
On 10/11/2016 10:13 PM, Sergey Bylokhov wrote: On 11.10.16 21:54, Jim Graham wrote: Additionally, for AA rendering, there is no such thing as "setClip(someShape)" and "fill(sameShape)" being identical no matter how much we predict which rules they may want because clips are inherently non-AA and so the fuzzy pixels along the boundary of an AA shape will be randomly clipped or included. When all is said and done I feel (not very strongly) it would be best to have clipping remain unaffected by the biasing of STROKE hints, but part of that is driven by the fact that I think it can be fixed with a couple of lines of code in SG2D/LoopPipe... I guess a bottom line is that it is require an additional investigation. I am not exactly sure, but my expectation is that fillRect(x,y,w,h)/drawImage(x,y,w,h) + setClip(x,y,w,h) should work in a similar way(covers the same pixels). And the question should this behavior be exactly the same as fill(RectShape) + setClip(RectShape) is unclear (I am not sure is it a critical issue or not) Right now I tried to fix overlapping, which produce visible artifacts and were considered as a bugs. The next issue which I would like to fix is a overlapping of drawImage(). I just created a small sample [1] which fills a rectangle (x, y, w, y), creates an image with size (w, h) and draws the image into the area (x, y, w, h). With the floating point scale like 1.5 the image does not exactly fits the area filled by the rectangle (see image [2] generated by the code [1]). This is because the Graphics.drawImage(...) calls DrawImage.renderImageXform() which uses floor and ceil methods for the region corner rounding: final int dx1 = Math.max((int) Math.floor(ddx1), clip.getLoX()); final int dy1 = Math.max((int) Math.floor(ddy1), clip.getLoY()); final int dx2 = Math.min((int) Math.ceil(ddx2), clip.getHiX()); final int dy2 = Math.min((int) Math.ceil(ddy2), clip.getHiY()); But the Graphics.fillRect() falls down to the code which just cast the transformed coordinates to int. Why the floor/ceil methods are used for the image region rounding? Is it possible to change this to fit the rule for the filled rectangles rounding? [1] http://cr.openjdk.java.net/~alexsch/fpapi/code/FillRectAndImageTest.java [2] http://cr.openjdk.java.net/~alexsch/fpapi/screenshots/rect-and-image.png Thanks, Alexandr.
Re: [OpenJDK 2D-Dev] [9] Review Request: 8167310 The graphics clip is incorrectly rounded for some fractional scales
On 10/11/16 12:49 PM, Jim Graham wrote: Right now I tried to fix overlapping, which produce visible artifacts and were considered as a bugs. The next issue which I would like to fix is a overlapping of drawImage(). Yes, that bears investigation... I looked into it and the issue is basically floating point bit error. Basically, in the test program we don't use a ScaledBlit because we don't have one defined for the combination of (IntArgb, SrcOver, IntArgb), since our ScaledBlit implementations are all for opaque images. The loop macros can't even create a non-opaque ScaledBlit without a bit of work. So, we end up in TransformHelper where we perform the blit by doing an inverse transform on the graphics transform and then use that to map back from destination pixels to the source pixels. The issue here is that with a scale of 1.5, the forward transform is a nice IEEE-floating-point spec compatible bit pattern, but inverting it gives a scale of 2/3s which doesn't have an exact IEEE representation. The result is that it marches through the image and the very last edge of that image maps to 10.5 which is an exact center of the pixel. If the math were perfect then we'd back-transform that coordinate into the source image and get 7.0 and that would be "past the right edge of the image" and we'd bail at that point. But, we back-transform it and get 6.9 and we think "OK, this pixel just squeaks by and gets one last sample of the image before we fall off the edge of the image". But, this should not happen with 1:1 image copies. And it shouldn't happen when we have a ScaledBlit function since that uses more exact math without inverting the scale, but I haven't written a test case to verify that. And, it shouldn't happen with integer scales and scales where we don't have a situation of an image mapping to exactly (pixel+0.5). But, I don't think we can improve the pixel accuracy of our inverse transforms to handle this exact case without a lot of work... ...jim
Re: [OpenJDK 2D-Dev] [9] Review Request: 8167310 The graphics clip is incorrectly rounded for some fractional scales
That looks good. +1 ...jim On 10/11/16 4:32 PM, Sergey Bylokhov wrote: On 11.10.16 23:12, Jim Graham wrote: Also, is it worth having a protective test for Rectangle in the intersect(Rectangle2D) method to avoid all of the double math when the rectangle was already an integer one? Yes, this code can be moved from SunGraphics2D: http://cr.openjdk.java.net/~serb/8167310/webrev.05 + isNaN checks were added ...jim On 10/10/16 4:37 PM, Sergey Bylokhov wrote: On 10.10.16 23:42, Jim Graham wrote: Can we also not use MAX_INT for the drawImage test case? Either have the drawImage follow the clip around or choose an appropriate non-limit size to cover the worst case scale with some margin for error... Something like this? http://cr.openjdk.java.net/~serb/8167310/webrev.04 On 10/10/16 12:45 PM, Sergey Bylokhov wrote: An updated version: http://cr.openjdk.java.net/~serb/8167310/webrev.03 - STROKE_PURE is used in the test, the line width is set to 2.01. This also fixed the difference between clips(Shape vs Rectangle). - The if statement is changed as suggested. The additional questions: - In the current fix we change behavior of the clip. Before the fix if we set the clip to the nearest areas they can overlaps in the destination. Should we change the drawImage as well? Currently if I draw image to the nearest areas in the user space, the images can overlap in the destination(so the actual result in destination depends on what image was painted first). - Should the clip be affected by the stroke(if it was set by the shape)? Right now if the clip was set by the shape it will produce different result than if it was set via rectangle. On 10.10.16 22:29, Jim Graham wrote: That does sound like a problem. Does it do the same thing with new Path2D(Rectangle)? The Area class does some processing on the path and it would be nice to eliminate that as a potential source of this problem. I don't have a buildable JDK9 repo right now that I can fire off some quick tests on so I'll have to look at this later... ...jim On 10/10/16 12:04 PM, Sergey Bylokhov wrote: On 10.10.16 21:55, Sergey Bylokhov wrote: Will give us a pass on the test as long as we made the same mistake for both the rect clip and the shape clip. I think you want "(rgb != goldRGB) || (rgb != GREEN && rgb != RED)"...? Correct, I will update the test. surprisingly but it is produce the different results And I think that the clip which is set via Shape is shifted, because the first and last fillRects cover only the half of expected area. But in case of clip=rectangle all fillRects produce the same areas.
Re: [OpenJDK 2D-Dev] [9] Review Request: 8167310 The graphics clip is incorrectly rounded for some fractional scales
On 11.10.16 23:12, Jim Graham wrote: Also, is it worth having a protective test for Rectangle in the intersect(Rectangle2D) method to avoid all of the double math when the rectangle was already an integer one? Yes, this code can be moved from SunGraphics2D: http://cr.openjdk.java.net/~serb/8167310/webrev.05 + isNaN checks were added ...jim On 10/10/16 4:37 PM, Sergey Bylokhov wrote: On 10.10.16 23:42, Jim Graham wrote: Can we also not use MAX_INT for the drawImage test case? Either have the drawImage follow the clip around or choose an appropriate non-limit size to cover the worst case scale with some margin for error... Something like this? http://cr.openjdk.java.net/~serb/8167310/webrev.04 On 10/10/16 12:45 PM, Sergey Bylokhov wrote: An updated version: http://cr.openjdk.java.net/~serb/8167310/webrev.03 - STROKE_PURE is used in the test, the line width is set to 2.01. This also fixed the difference between clips(Shape vs Rectangle). - The if statement is changed as suggested. The additional questions: - In the current fix we change behavior of the clip. Before the fix if we set the clip to the nearest areas they can overlaps in the destination. Should we change the drawImage as well? Currently if I draw image to the nearest areas in the user space, the images can overlap in the destination(so the actual result in destination depends on what image was painted first). - Should the clip be affected by the stroke(if it was set by the shape)? Right now if the clip was set by the shape it will produce different result than if it was set via rectangle. On 10.10.16 22:29, Jim Graham wrote: That does sound like a problem. Does it do the same thing with new Path2D(Rectangle)? The Area class does some processing on the path and it would be nice to eliminate that as a potential source of this problem. I don't have a buildable JDK9 repo right now that I can fire off some quick tests on so I'll have to look at this later... ...jim On 10/10/16 12:04 PM, Sergey Bylokhov wrote: On 10.10.16 21:55, Sergey Bylokhov wrote: Will give us a pass on the test as long as we made the same mistake for both the rect clip and the shape clip. I think you want "(rgb != goldRGB) || (rgb != GREEN && rgb != RED)"...? Correct, I will update the test. surprisingly but it is produce the different results And I think that the clip which is set via Shape is shifted, because the first and last fillRects cover only the half of expected area. But in case of clip=rectangle all fillRects produce the same areas. -- Best regards, Sergey.
Re: [OpenJDK 2D-Dev] [9] Review Request: 8167310 The graphics clip is incorrectly rounded for some fractional scales
Also, is it worth having a protective test for Rectangle in the intersect(Rectangle2D) method to avoid all of the double math when the rectangle was already an integer one? ...jim On 10/10/16 4:37 PM, Sergey Bylokhov wrote: On 10.10.16 23:42, Jim Graham wrote: Can we also not use MAX_INT for the drawImage test case? Either have the drawImage follow the clip around or choose an appropriate non-limit size to cover the worst case scale with some margin for error... Something like this? http://cr.openjdk.java.net/~serb/8167310/webrev.04 On 10/10/16 12:45 PM, Sergey Bylokhov wrote: An updated version: http://cr.openjdk.java.net/~serb/8167310/webrev.03 - STROKE_PURE is used in the test, the line width is set to 2.01. This also fixed the difference between clips(Shape vs Rectangle). - The if statement is changed as suggested. The additional questions: - In the current fix we change behavior of the clip. Before the fix if we set the clip to the nearest areas they can overlaps in the destination. Should we change the drawImage as well? Currently if I draw image to the nearest areas in the user space, the images can overlap in the destination(so the actual result in destination depends on what image was painted first). - Should the clip be affected by the stroke(if it was set by the shape)? Right now if the clip was set by the shape it will produce different result than if it was set via rectangle. On 10.10.16 22:29, Jim Graham wrote: That does sound like a problem. Does it do the same thing with new Path2D(Rectangle)? The Area class does some processing on the path and it would be nice to eliminate that as a potential source of this problem. I don't have a buildable JDK9 repo right now that I can fire off some quick tests on so I'll have to look at this later... ...jim On 10/10/16 12:04 PM, Sergey Bylokhov wrote: On 10.10.16 21:55, Sergey Bylokhov wrote: Will give us a pass on the test as long as we made the same mistake for both the rect clip and the shape clip. I think you want "(rgb != goldRGB) || (rgb != GREEN && rgb != RED)"...? Correct, I will update the test. surprisingly but it is produce the different results And I think that the clip which is set via Shape is shifted, because the first and last fillRects cover only the half of expected area. But in case of clip=rectangle all fillRects produce the same areas.
Re: [OpenJDK 2D-Dev] [9] Review Request: 8167310 The graphics clip is incorrectly rounded for some fractional scales
One last thing. In the clipRound() method you mention that NaN evaluates to 0. I assume that is just an observation that we'd end up in the "(int) ceil()" case and the cast returns a 0. In the case of a path we omit NaN coordinates, which means skipping segments in a path until they become sane again. That would likely turn a rectangle into a triangle if it happened in a shape, but in a Rectangle object the NaN ends up affecting more than one corner (minX is the coordinate of 2 corners, maxX is the coordinate for 2 corners, same for Y) and it degenerates further into an empty shape in all cases. For example: (x, y, w, h) 10, 10, NaN, 20 path is 10,10 => NaN,10 => NaN,30 => 10,30 => 2 points eliminated => devolves to a vertical line => no pixels included 10, 10, 20, NaN path is 10,10 => 30,10 => 30,NaN => 10,NaN => 2 points, devolves NaN, 10, 20, 20 path is NaN,10 => NaN,10 => NaN,30 => NaN,30 => no points left => completely degenerate 10, NaN, 20, 20 path is 10,NaN => 30,NaN => 30,NaN => 10,NaN => no points, degenerate In the case of NaN in X and Y, the clipRound() method would end up returning 0 for both min and max of X or Y which would hopefully produce an empty region when we do the intersection. In the case of NaN as a W or H, clipRound() would only produce a 0 for maxX or maxY, but the min would still be computed correctly. I guess this could technically be considered an empty shape in practice since we tend to use Regions to only encompass pixels in a non-negative space, so it is fine in practice, but if we ever use translated Regions for some sort of use (what about shaped components which might use Region?) then 0 for maxX or maxY might be paired with a negative value for minX or minY and end up with a non-empty Region in the negative coordinates (and potentially translated later to positive coordinates and encompass some pixels on a destination drawable). All of those would (should?) return a completely empty region if we rasterized the shape, so we should set an empty clip if we get a NaN in any coordinate... ...jim On 10/10/16 4:37 PM, Sergey Bylokhov wrote: On 10.10.16 23:42, Jim Graham wrote: Can we also not use MAX_INT for the drawImage test case? Either have the drawImage follow the clip around or choose an appropriate non-limit size to cover the worst case scale with some margin for error... Something like this? http://cr.openjdk.java.net/~serb/8167310/webrev.04 On 10/10/16 12:45 PM, Sergey Bylokhov wrote: An updated version: http://cr.openjdk.java.net/~serb/8167310/webrev.03 - STROKE_PURE is used in the test, the line width is set to 2.01. This also fixed the difference between clips(Shape vs Rectangle). - The if statement is changed as suggested. The additional questions: - In the current fix we change behavior of the clip. Before the fix if we set the clip to the nearest areas they can overlaps in the destination. Should we change the drawImage as well? Currently if I draw image to the nearest areas in the user space, the images can overlap in the destination(so the actual result in destination depends on what image was painted first). - Should the clip be affected by the stroke(if it was set by the shape)? Right now if the clip was set by the shape it will produce different result than if it was set via rectangle. On 10.10.16 22:29, Jim Graham wrote: That does sound like a problem. Does it do the same thing with new Path2D(Rectangle)? The Area class does some processing on the path and it would be nice to eliminate that as a potential source of this problem. I don't have a buildable JDK9 repo right now that I can fire off some quick tests on so I'll have to look at this later... ...jim On 10/10/16 12:04 PM, Sergey Bylokhov wrote: On 10.10.16 21:55, Sergey Bylokhov wrote: Will give us a pass on the test as long as we made the same mistake for both the rect clip and the shape clip. I think you want "(rgb != goldRGB) || (rgb != GREEN && rgb != RED)"...? Correct, I will update the test. surprisingly but it is produce the different results And I think that the clip which is set via Shape is shifted, because the first and last fillRects cover only the half of expected area. But in case of clip=rectangle all fillRects produce the same areas.
Re: [OpenJDK 2D-Dev] [9] Review Request: 8167310 The graphics clip is incorrectly rounded for some fractional scales
On 10/11/16 12:13 PM, Sergey Bylokhov wrote: On 11.10.16 21:54, Jim Graham wrote: Additionally, for AA rendering, there is no such thing as "setClip(someShape)" and "fill(sameShape)" being identical no matter how much we predict which rules they may want because clips are inherently non-AA and so the fuzzy pixels along the boundary of an AA shape will be randomly clipped or included. When all is said and done I feel (not very strongly) it would be best to have clipping remain unaffected by the biasing of STROKE hints, but part of that is driven by the fact that I think it can be fixed with a couple of lines of code in SG2D/LoopPipe... I guess a bottom line is that it is require an additional investigation. I am not exactly sure, but my expectation is that fillRect(x,y,w,h)/drawImage(x,y,w,h) + setClip(x,y,w,h) should work in a similar way(covers the same pixels). And the question should this behavior be exactly the same as fill(RectShape) + setClip(RectShape) is unclear (I am not sure is it a critical issue or not) In STROKE_PURE, setClip/fillRect/drawImage(x,y,w,h) should all hit the same pixels. I think we currently have a bug with drawImage() that needs to be investigated further, though. The fact that it overlaps with itself is problematic and likely an indication that it won't match fillRect() either. Under STROKE_NORMALIZE, fillRect() and fill(Rect) are affected due to their relationship to drawRect()/fill(Rect). It's not clear if it should be identical to clipRect() and clip(Rect), though as I don't know where we'd have developers relying on that. In particular, damage repair would usually involve clip(dmgxywh) followed by fill(compxywh) and so they wouldn't necessarily need to match. drawImage() brings up additional questions. For now I think we can ignore this issue for this fix, but I still have a question on the fix which I'll ask in response to webrev.04. We should file the "clipRect() vs clip(RectShape)" bug for follow-on work. Right now I tried to fix overlapping, which produce visible artifacts and were considered as a bugs. The next issue which I would like to fix is a overlapping of drawImage(). Yes, that bears investigation... ...jim
Re: [OpenJDK 2D-Dev] [9] Review Request: 8167310 The graphics clip is incorrectly rounded for some fractional scales
On 11.10.16 21:54, Jim Graham wrote: Additionally, for AA rendering, there is no such thing as "setClip(someShape)" and "fill(sameShape)" being identical no matter how much we predict which rules they may want because clips are inherently non-AA and so the fuzzy pixels along the boundary of an AA shape will be randomly clipped or included. When all is said and done I feel (not very strongly) it would be best to have clipping remain unaffected by the biasing of STROKE hints, but part of that is driven by the fact that I think it can be fixed with a couple of lines of code in SG2D/LoopPipe... I guess a bottom line is that it is require an additional investigation. I am not exactly sure, but my expectation is that fillRect(x,y,w,h)/drawImage(x,y,w,h) + setClip(x,y,w,h) should work in a similar way(covers the same pixels). And the question should this behavior be exactly the same as fill(RectShape) + setClip(RectShape) is unclear (I am not sure is it a critical issue or not) Right now I tried to fix overlapping, which produce visible artifacts and were considered as a bugs. The next issue which I would like to fix is a overlapping of drawImage(). -- Best regards, Sergey.
Re: [OpenJDK 2D-Dev] [9] Review Request: 8167310 The graphics clip is incorrectly rounded for some fractional scales
On 10/10/16 3:23 PM, Phil Race wrote: That last sentence sounds like the right answer in principle but I don't know if we'll be unpleasantly surprised by some consequence of "... that setClip(Shape) and fill(Shape) might disagree .." -phil. As I was thinking more about this last night we have additional issues to consider. Our rasterization rules bend a bit under STROKE_CONTROL and they do so differently for AA and non-AA. Really STROKE_CONTROL was a bandaid for the fact that the suddenly specific rasterization rules we created with the 2D API didn't necessarily match what developers had been used to and so we'd need some time for them to adjust. In particular, a common misunderstanding is that "drawLine(X,0,X,h)" would fill in all of the pixels in the X column, but in AA this would not work right as it would fill half of (coverage-wise 50% blending) the pixels to the right of X and half of the pixels to the left of X and look fuzzy. Similarly, exact insideness rules meant that even with non-AA and a line width of 1, technically the outline for that "line" would suggest that the pixels in the column to the left of X would be drawn. In retrospect, we should have been advising developers to wean themselves off of STROKE_NORMALIZE adjustments from the release of Java 2, but we dropped that ball. :( To be fair, though, these issues had been common in dealing with other rendering systems, like Postscript - but how many developers have ever written Postscript code themselves? Now with JavaFX we no longer have STROKE_CONTROL at all and we honor the strict rasterization rules that Java2D should be honoring under STROKE_PURE and so developers must learn how to be more selective in how the render things from their first exposure to FX. So, the ins and outs of dealing with the specific rasterization rules (things like where to center lines, and manually snapping to pixels because "ints" don't do it pseudo-automatically for you) should be becoming part of the developer experience with FX. I'm not sure how much that will help in Swing/AWT when Java2D has had NORMALIZE rules as default for a couple of decades. The rules for STROKE_NORMALIZE were created so that fills and strokes would match and be non-surprising for both AA and non-AA modes, but the rules needed were different for the 2 modes. Having different rules was fine because nobody would expect someone to draw a shape with non-AA and then fill it with AA or vice versa. It is very much expected that the AA hint would stay the same for both a fill and a draw of a shape. But, will the AA hint (or even the STROKE hint) stay the same for both the setting of a clip shape and the rendering of items in it? How often is clip(someShape) followed by draw(sameShape) or fill(sameShape)? Would a developer believe that the hints would affect the clip and so they would do the clip adjustment first and then set all of their "rendering hints"? (Note that these are called "RenderingHints", though if I were to come up with a clip-specific hint I would have no qualms adding it to the bag of hints in the RH class...) Additionally, for AA rendering, there is no such thing as "setClip(someShape)" and "fill(sameShape)" being identical no matter how much we predict which rules they may want because clips are inherently non-AA and so the fuzzy pixels along the boundary of an AA shape will be randomly clipped or included. When all is said and done I feel (not very strongly) it would be best to have clipping remain unaffected by the biasing of STROKE hints, but part of that is driven by the fact that I think it can be fixed with a couple of lines of code in SG2D/LoopPipe... ...jim
Re: [OpenJDK 2D-Dev] [9] Review Request: 8167310 The graphics clip is incorrectly rounded for some fractional scales
On 10/10/16 3:53 PM, Sergey Bylokhov wrote: On 11.10.16 1:23, Phil Race wrote: Yes, I think adjacent drawImage requests should not overlap. We should look into this separately. So previously, overlapping of the clip bounds ensured adjacent images were not drawn over-lapped .. but with this fix they might be ? The fix change only behavior of the rectangle clip. So using the setClip we can prevent overlaping of the images. But i think that even without clip such images should not overlap. I'm pretty sure that setClip(x,y,w,h) would ensure adjacency (no overlaps and no gaps) with another call to itself with adjacent parameters. And setClip(RectShape) would also ensure proper adjacency with itself. But, I think we had cases before this fix where setClip(x,y,w,h) could violate proper adjacency with setClip(RectShape). This fix addresses most of that, particularly it works if we have STROKE_PURE. I believe that it still has issues with STROKE_NORMALIZE, though, because one of them honors it and the other doesn't. So, we need to have them both honor the STROKE_CONTROL in the same way. On the other hand, we normalize differently for AA and non-AA. Calling getFillSSI() on LoopPipe basically performs normalization only for non-AA fills. Arguably, though, clipping produces non-AA results in that it chooses whole pixels to include or exclude. This might mean that it should never follow AA normalization. That last sentence sounds like the right answer in principle but I don't know if we'll be unpleasantly surprised by some consequence of "... that setClip(Shape) and fill(Shape) might disagree .." I just tested fillRect vs fill(RectShape), and both work differently in some cases as well-=((. So we have a few similar methods which works differently(even if VALUE_STROKE_PURE is set), which became visible on fractional(1.5) scales - setClip(Rectangle) - setClip(Shape) - fillRect(Rectangle) - fill(Shape) - DrawImage(Rectangle) I'd like to see which cases cause differences. The main one that I can see by a brief examination of the code is the case of non-integer translations (but no scale). I think we end up using the simplified FillRect primitives in the case of a non-integer translate, and that can cause a difference in effect. We could fix SurfaceData's validation to not use the primitive pipeline unless the translation is integer, but that could cost performance for that case. I'm not sure how often a non-integer translation with no other transform elements (i.e. scale) occurs, though. We could teach the FillRect primitive (or its caller) to adjust for the STROKE_CONTROL more properly in the case of a non-integer translation, but I'm not sure how often that will come into play and benefit us. Also, this might be fixable in the code that comes up with sg2d.transXY, which probably doesn't take STROKE hint into account... ...jim
Re: [OpenJDK 2D-Dev] [9] Review Request: 8167310 The graphics clip is incorrectly rounded for some fractional scales
On 10.10.16 23:42, Jim Graham wrote: Can we also not use MAX_INT for the drawImage test case? Either have the drawImage follow the clip around or choose an appropriate non-limit size to cover the worst case scale with some margin for error... Something like this? http://cr.openjdk.java.net/~serb/8167310/webrev.04 On 10/10/16 12:45 PM, Sergey Bylokhov wrote: An updated version: http://cr.openjdk.java.net/~serb/8167310/webrev.03 - STROKE_PURE is used in the test, the line width is set to 2.01. This also fixed the difference between clips(Shape vs Rectangle). - The if statement is changed as suggested. The additional questions: - In the current fix we change behavior of the clip. Before the fix if we set the clip to the nearest areas they can overlaps in the destination. Should we change the drawImage as well? Currently if I draw image to the nearest areas in the user space, the images can overlap in the destination(so the actual result in destination depends on what image was painted first). - Should the clip be affected by the stroke(if it was set by the shape)? Right now if the clip was set by the shape it will produce different result than if it was set via rectangle. On 10.10.16 22:29, Jim Graham wrote: That does sound like a problem. Does it do the same thing with new Path2D(Rectangle)? The Area class does some processing on the path and it would be nice to eliminate that as a potential source of this problem. I don't have a buildable JDK9 repo right now that I can fire off some quick tests on so I'll have to look at this later... ...jim On 10/10/16 12:04 PM, Sergey Bylokhov wrote: On 10.10.16 21:55, Sergey Bylokhov wrote: Will give us a pass on the test as long as we made the same mistake for both the rect clip and the shape clip. I think you want "(rgb != goldRGB) || (rgb != GREEN && rgb != RED)"...? Correct, I will update the test. surprisingly but it is produce the different results And I think that the clip which is set via Shape is shifted, because the first and last fillRects cover only the half of expected area. But in case of clip=rectangle all fillRects produce the same areas. -- Best regards, Sergey.
Re: [OpenJDK 2D-Dev] [9] Review Request: 8167310 The graphics clip is incorrectly rounded for some fractional scales
On 11.10.16 1:23, Phil Race wrote: Yes, I think adjacent drawImage requests should not overlap. We should look into this separately. So previously, overlapping of the clip bounds ensured adjacent images were not drawn over-lapped .. but with this fix they might be ? The fix change only behavior of the rectangle clip. So using the setClip we can prevent overlaping of the images. But i think that even without clip such images should not overlap. On the other hand, we normalize differently for AA and non-AA. Calling getFillSSI() on LoopPipe basically performs normalization only for non-AA fills. Arguably, though, clipping produces non-AA results in that it chooses whole pixels to include or exclude. This might mean that it should never follow AA normalization. That last sentence sounds like the right answer in principle but I don't know if we'll be unpleasantly surprised by some consequence of "... that setClip(Shape) and fill(Shape) might disagree .." I just tested fillRect vs fill(RectShape), and both work differently in some cases as well-=((. So we have a few similar methods which works differently(even if VALUE_STROKE_PURE is set), which became visible on fractional(1.5) scales - setClip(Rectangle) - setClip(Shape) - fillRect(Rectangle) - fill(Shape) - DrawImage(Rectangle) -- Best regards, Sergey.
Re: [OpenJDK 2D-Dev] [9] Review Request: 8167310 The graphics clip is incorrectly rounded for some fractional scales
On 10/10/2016 01:34 PM, Jim Graham wrote: [CC'ing Phil in case he has some suggestions for how the API should work...] Responding just to additional questions in this email... On 10/10/16 12:45 PM, Sergey Bylokhov wrote: The additional questions: - In the current fix we change behavior of the clip. Before the fix if we set the clip to the nearest areas they can overlaps in the destination. Should we change the drawImage as well? Currently if I draw image to the nearest areas in the user space, the images can overlap in the destination(so the actual result in destination depends on what image was painted first). Yes, I think adjacent drawImage requests should not overlap. We should look into this separately. So previously, overlapping of the clip bounds ensured adjacent images were not drawn over-lapped .. but with this fix they might be ? - Should the clip be affected by the stroke(if it was set by the shape)? Right now if the clip was set by the shape it will produce different result than if it was set via rectangle. I think we could have the clip never be affected by STROKE_NORMALIZE. That might make sense because the NORMALIZE code is meant to deal with aesthetic issues, not rendering math issues. We'd have to add a getClipSSI() method that hard-codes the "adjust" boolean to "false" to do that, but that is an easy fix. The question is then that would meant that setClip(Shape) and fill(Shape) might disagree, though. This would argue that we should have it affected by the normalization. This is also a fairly easy fix, you could have LoopPipe have a method that would return the appropriate Rectangle representing which pixels are covered for a given Rectangle2D depending on the normalize parameters. On the other hand, we normalize differently for AA and non-AA. Calling getFillSSI() on LoopPipe basically performs normalization only for non-AA fills. Arguably, though, clipping produces non-AA results in that it chooses whole pixels to include or exclude. This might mean that it should never follow AA normalization. That last sentence sounds like the right answer in principle but I don't know if we'll be unpleasantly surprised by some consequence of "... that setClip(Shape) and fill(Shape) might disagree .." -phil. But, it also points out the silliness of normalizing to match the results of fill when it can never do that for AA mode. (Potentially we could add an AA-clip mode, but the implementation of that would require capturing every rendering operation to an intermediate buffer and then pixel-filtering it through a mask created by AA-filling the clip shape - we do that in FX, but it would require major surgery to add that to the SG2D pipeline so I don't think we would ever consider that at this stage.) Any of those solutions are fairly easy to fix (except adding an AA clipping mode), but the question is which is more useful and/or surprises the developers the least. So we have the current case: setClip(integers or int-Rectangle) => pure coverage setClip(other shape) => non-AA normalized coverage If we decide that clip should normalize always to non-AA parameters because it is an intrinsically non-AA type of operation then we have to fix the setClip(integer rectangle) code which isn't hard. If we decide that clip should normalize to the current AA parameters then it's basically the same, but we need to find a way to query the right objects to perform the AA according to their chosen bias. LoopPipe/ShapeSpanIterator appear to own non-AA normalization and I think AA normalization is hidden behind the TileGenerator API somewhere that Ductus/Pisces/Marlin all implement. If we decide that clip should not normalize then we only have to fix setClip(other shape) and it's just a question of hard-coding the adjust boolean in the call to instantiate the ShapeSpanIterator. Phil? ...jim
Re: [OpenJDK 2D-Dev] [9] Review Request: 8167310 The graphics clip is incorrectly rounded for some fractional scales
Can we also not use MAX_INT for the drawImage test case? Either have the drawImage follow the clip around or choose an appropriate non-limit size to cover the worst case scale with some margin for error... ...jim On 10/10/16 12:45 PM, Sergey Bylokhov wrote: An updated version: http://cr.openjdk.java.net/~serb/8167310/webrev.03 - STROKE_PURE is used in the test, the line width is set to 2.01. This also fixed the difference between clips(Shape vs Rectangle). - The if statement is changed as suggested. The additional questions: - In the current fix we change behavior of the clip. Before the fix if we set the clip to the nearest areas they can overlaps in the destination. Should we change the drawImage as well? Currently if I draw image to the nearest areas in the user space, the images can overlap in the destination(so the actual result in destination depends on what image was painted first). - Should the clip be affected by the stroke(if it was set by the shape)? Right now if the clip was set by the shape it will produce different result than if it was set via rectangle. On 10.10.16 22:29, Jim Graham wrote: That does sound like a problem. Does it do the same thing with new Path2D(Rectangle)? The Area class does some processing on the path and it would be nice to eliminate that as a potential source of this problem. I don't have a buildable JDK9 repo right now that I can fire off some quick tests on so I'll have to look at this later... ...jim On 10/10/16 12:04 PM, Sergey Bylokhov wrote: On 10.10.16 21:55, Sergey Bylokhov wrote: Will give us a pass on the test as long as we made the same mistake for both the rect clip and the shape clip. I think you want "(rgb != goldRGB) || (rgb != GREEN && rgb != RED)"...? Correct, I will update the test. surprisingly but it is produce the different results And I think that the clip which is set via Shape is shifted, because the first and last fillRects cover only the half of expected area. But in case of clip=rectangle all fillRects produce the same areas.
Re: [OpenJDK 2D-Dev] [9] Review Request: 8167310 The graphics clip is incorrectly rounded for some fractional scales
[CC'ing Phil in case he has some suggestions for how the API should work...] Responding just to additional questions in this email... On 10/10/16 12:45 PM, Sergey Bylokhov wrote: The additional questions: - In the current fix we change behavior of the clip. Before the fix if we set the clip to the nearest areas they can overlaps in the destination. Should we change the drawImage as well? Currently if I draw image to the nearest areas in the user space, the images can overlap in the destination(so the actual result in destination depends on what image was painted first). Yes, I think adjacent drawImage requests should not overlap. We should look into this separately. - Should the clip be affected by the stroke(if it was set by the shape)? Right now if the clip was set by the shape it will produce different result than if it was set via rectangle. I think we could have the clip never be affected by STROKE_NORMALIZE. That might make sense because the NORMALIZE code is meant to deal with aesthetic issues, not rendering math issues. We'd have to add a getClipSSI() method that hard-codes the "adjust" boolean to "false" to do that, but that is an easy fix. The question is then that would meant that setClip(Shape) and fill(Shape) might disagree, though. This would argue that we should have it affected by the normalization. This is also a fairly easy fix, you could have LoopPipe have a method that would return the appropriate Rectangle representing which pixels are covered for a given Rectangle2D depending on the normalize parameters. On the other hand, we normalize differently for AA and non-AA. Calling getFillSSI() on LoopPipe basically performs normalization only for non-AA fills. Arguably, though, clipping produces non-AA results in that it chooses whole pixels to include or exclude. This might mean that it should never follow AA normalization. But, it also points out the silliness of normalizing to match the results of fill when it can never do that for AA mode. (Potentially we could add an AA-clip mode, but the implementation of that would require capturing every rendering operation to an intermediate buffer and then pixel-filtering it through a mask created by AA-filling the clip shape - we do that in FX, but it would require major surgery to add that to the SG2D pipeline so I don't think we would ever consider that at this stage.) Any of those solutions are fairly easy to fix (except adding an AA clipping mode), but the question is which is more useful and/or surprises the developers the least. So we have the current case: setClip(integers or int-Rectangle) => pure coverage setClip(other shape) => non-AA normalized coverage If we decide that clip should normalize always to non-AA parameters because it is an intrinsically non-AA type of operation then we have to fix the setClip(integer rectangle) code which isn't hard. If we decide that clip should normalize to the current AA parameters then it's basically the same, but we need to find a way to query the right objects to perform the AA according to their chosen bias. LoopPipe/ShapeSpanIterator appear to own non-AA normalization and I think AA normalization is hidden behind the TileGenerator API somewhere that Ductus/Pisces/Marlin all implement. If we decide that clip should not normalize then we only have to fix setClip(other shape) and it's just a question of hard-coding the adjust boolean in the call to instantiate the ShapeSpanIterator. Phil? ...jim
Re: [OpenJDK 2D-Dev] [9] Review Request: 8167310 The graphics clip is incorrectly rounded for some fractional scales
An updated version: http://cr.openjdk.java.net/~serb/8167310/webrev.03 - STROKE_PURE is used in the test, the line width is set to 2.01. This also fixed the difference between clips(Shape vs Rectangle). - The if statement is changed as suggested. The additional questions: - In the current fix we change behavior of the clip. Before the fix if we set the clip to the nearest areas they can overlaps in the destination. Should we change the drawImage as well? Currently if I draw image to the nearest areas in the user space, the images can overlap in the destination(so the actual result in destination depends on what image was painted first). - Should the clip be affected by the stroke(if it was set by the shape)? Right now if the clip was set by the shape it will produce different result than if it was set via rectangle. On 10.10.16 22:29, Jim Graham wrote: That does sound like a problem. Does it do the same thing with new Path2D(Rectangle)? The Area class does some processing on the path and it would be nice to eliminate that as a potential source of this problem. I don't have a buildable JDK9 repo right now that I can fire off some quick tests on so I'll have to look at this later... ...jim On 10/10/16 12:04 PM, Sergey Bylokhov wrote: On 10.10.16 21:55, Sergey Bylokhov wrote: Will give us a pass on the test as long as we made the same mistake for both the rect clip and the shape clip. I think you want "(rgb != goldRGB) || (rgb != GREEN && rgb != RED)"...? Correct, I will update the test. surprisingly but it is produce the different results And I think that the clip which is set via Shape is shifted, because the first and last fillRects cover only the half of expected area. But in case of clip=rectangle all fillRects produce the same areas. -- Best regards, Sergey.
Re: [OpenJDK 2D-Dev] [9] Review Request: 8167310 The graphics clip is incorrectly rounded for some fractional scales
Yes, we do bias both. So, the code we just added doesn't match the biasing that is performed by the fill operations, we should probably make it be the same. You can see that it computes a bias boolean when it calls getFillSSI(), but the bias is applied natively in the native ShapeSpanIterator code... ...jim On 10/10/16 12:26 PM, Sergey Bylokhov wrote: On 10.10.16 22:04, Sergey Bylokhov wrote: On 10.10.16 21:55, Sergey Bylokhov wrote: Will give us a pass on the test as long as we made the same mistake for both the rect clip and the shape clip. I think you want "(rgb != goldRGB) || (rgb != GREEN && rgb != RED)"...? Correct, I will update the test. surprisingly but it is produce the different results And I think that the clip which is set via Shape is shifted, because the first and last fillRects cover only the half of expected area. But in case of clip=rectangle all fillRects produce the same areas. It was solved by stroke=PURE, but I am not sure why fillRect(when clip is set via Shape) is affected by this hint. Is it ok?
Re: [OpenJDK 2D-Dev] [9] Review Request: 8167310 The graphics clip is incorrectly rounded for some fractional scales
That does sound like a problem. Does it do the same thing with new Path2D(Rectangle)? The Area class does some processing on the path and it would be nice to eliminate that as a potential source of this problem. I don't have a buildable JDK9 repo right now that I can fire off some quick tests on so I'll have to look at this later... ...jim On 10/10/16 12:04 PM, Sergey Bylokhov wrote: On 10.10.16 21:55, Sergey Bylokhov wrote: Will give us a pass on the test as long as we made the same mistake for both the rect clip and the shape clip. I think you want "(rgb != goldRGB) || (rgb != GREEN && rgb != RED)"...? Correct, I will update the test. surprisingly but it is produce the different results And I think that the clip which is set via Shape is shifted, because the first and last fillRects cover only the half of expected area. But in case of clip=rectangle all fillRects produce the same areas.
Re: [OpenJDK 2D-Dev] [9] Review Request: 8167310 The graphics clip is incorrectly rounded for some fractional scales
On 10.10.16 22:04, Sergey Bylokhov wrote: On 10.10.16 21:55, Sergey Bylokhov wrote: Will give us a pass on the test as long as we made the same mistake for both the rect clip and the shape clip. I think you want "(rgb != goldRGB) || (rgb != GREEN && rgb != RED)"...? Correct, I will update the test. surprisingly but it is produce the different results And I think that the clip which is set via Shape is shifted, because the first and last fillRects cover only the half of expected area. But in case of clip=rectangle all fillRects produce the same areas. It was solved by stroke=PURE, but I am not sure why fillRect(when clip is set via Shape) is affected by this hint. Is it ok? -- Best regards, Sergey.
Re: [OpenJDK 2D-Dev] [9] Review Request: 8167310 The graphics clip is incorrectly rounded for some fractional scales
On 10/10/16 11:55 AM, Sergey Bylokhov wrote: On 10.10.16 21:31, Jim Graham wrote: OK, but you only need a line width of 2.0 to cover the gap regardless of scale. Line width scales in user space so larger scales scale up the line width along with the clip region being rendered. With STROKE_CONTROL on, 2.0 is plenty because the line is normalized (though I'm not sure the test should assume that). WITH STROKE_PURE, 2.0 is precisely exactly the right amount. Round-off error might theoretically bite us, so maybe 2.01 just to be safe. When I tried to use w=2/3/4/5 I got a situation when no lines are drawn to the image. I guess to paint something we need to cover at least half of the pixel, this is not the case when line is w=2 and small scale is used. I think I see the problem. With STROKE_NORMALIZE and a very tiny scale and a small line width you will not reach the center of a pixel from the x,y=0.25 offset that our implementation normalizes lines to. At .1 scale you'd need to cover .25 units to reach that pixel center so you'd need a larger line width. Since only half of the line happens on either side of the line, a line width of 5 at .1 scale only draws .25 on either side and even though that reaches the pixel center, it doesn't light up the pixel center because that would be the right (or bottom) side of the fillable area and a pixel is not rendered if its center falls on the right or bottom edge of a fillable region, so you would need to go to 6 to see something at .1 scale. If you turn off NORMALIZE and use PURE, then the line would track along with the clip location. I'm not sure why you are testing NORMALIZE behavior since the NORMALIZE behavior is not a formal spec since there are several ways to achieve its goals. The way we implement it 6 is enough for the lines to appear at .1 scale, but someone else might decide that normalizing lines to the pixel edges is better for some reason and then you'd need to have a line width of 11 to light up the pixels. Note that we use 0.5,0.5 for AA rendering because its needs are different, so our own implementation isn't even consistent about the bias used for NORMALIZE (though since you aren't testing AA, you will only encounter our 0.25 bias). If you switch to PURE then 2 (or 2.01 to be safe) would suffice... ...jim
Re: [OpenJDK 2D-Dev] [9] Review Request: 8167310 The graphics clip is incorrectly rounded for some fractional scales
On 10.10.16 21:55, Sergey Bylokhov wrote: Will give us a pass on the test as long as we made the same mistake for both the rect clip and the shape clip. I think you want "(rgb != goldRGB) || (rgb != GREEN && rgb != RED)"...? Correct, I will update the test. surprisingly but it is produce the different results And I think that the clip which is set via Shape is shifted, because the first and last fillRects cover only the half of expected area. But in case of clip=rectangle all fillRects produce the same areas. -- Best regards, Sergey.
Re: [OpenJDK 2D-Dev] [9] Review Request: 8167310 The graphics clip is incorrectly rounded for some fractional scales
On 10.10.16 21:31, Jim Graham wrote:
OK, but you only need a line width of 2.0 to cover the gap regardless of
scale. Line width scales in user space so larger scales scale up the
line width along with the clip region being rendered. With
STROKE_CONTROL on, 2.0 is plenty because the line is normalized (though
I'm not sure the test should assume that). WITH STROKE_PURE, 2.0 is
precisely exactly the right amount. Round-off error might theoretically
bite us, so maybe 2.01 just to be safe.
When I tried to use w=2/3/4/5 I got a situation when no lines are drawn
to the image. I guess to paint something we need to cover at least half
of the pixel, this is not the case when line is w=2 and small scale is used.
In the drawImage case in the test, is there a reason to stretch the
image to MAX_INT size? Isn't (img,0,0,null) enough? The issue with the
MAX_INT arguments is that this then becomes not only a test of clipping,
but a test for how our image scaling handles huge scales that might
overflow. Those should be tested independently if we fear there is a
problem with image scaling.
This is extreme case which I tried to test, intersect the clip + the
scale on graphics + the scale from drawImage, since this huge
coordinates also can affect the clip, if intersection will be done
incorrectly.
Also, This line in the test case:
161 if (rgb != goldRGB && rgb != GREEN.getRGB()
162 && rgb != RED.getRGB()) {
Will give us a pass on the test as long as we made the same mistake for
both the rect clip and the shape clip. I think you want "(rgb !=
goldRGB) || (rgb != GREEN && rgb != RED)"...?
Correct, I will update the test.
--
Best regards, Sergey.
Re: [OpenJDK 2D-Dev] [9] Review Request: 8167310 The graphics clip is incorrectly rounded for some fractional scales
Hi Sergey,
Comments inline...
On 10/8/16 2:15 PM, Sergey Bylokhov wrote:
On 08.10.16 0:58, Jim Graham wrote:
That looks great. A couple of questions.
An updated version and a comments inline:
http://cr.openjdk.java.net/~serb/8167310/webrev.01
Test case:
Where do you get white gaps? Is it in the line test? If so, then
consider setting the line width higher (or drawing 2 lines per iteration
as I mention in the last paragraph). I can illustrate this with the
following:
Yes, it was in the lines. In the new version I changed the line width to 6.
This value will fill the whole clip in all
tested scales[0.1 - 4].
I did not use two lines because I use "srcover+transparentColor" and two lines
can produce one more color after
overlapping.
OK, but you only need a line width of 2.0 to cover the gap regardless of scale. Line width scales in user space so
larger scales scale up the line width along with the clip region being rendered. With STROKE_CONTROL on, 2.0 is plenty
because the line is normalized (though I'm not sure the test should assume that). WITH STROKE_PURE, 2.0 is precisely
exactly the right amount. Round-off error might theoretically bite us, so maybe 2.01 just to be safe.
Other comments...
In the drawImage case in the test, is there a reason to stretch the image to MAX_INT size? Isn't (img,0,0,null) enough?
The issue with the MAX_INT arguments is that this then becomes not only a test of clipping, but a test for how our
image scaling handles huge scales that might overflow. Those should be tested independently if we fear there is a
problem with image scaling.
Also, This line in the test case:
161 if (rgb != goldRGB && rgb != GREEN.getRGB()
162 && rgb != RED.getRGB()) {
Will give us a pass on the test as long as we made the same mistake for both the rect clip and the shape clip. I think
you want "(rgb != goldRGB) || (rgb != GREEN && rgb != RED)"...?
...jim
Re: [OpenJDK 2D-Dev] [9] Review Request: 8167310 The graphics clip is incorrectly rounded for some fractional scales
On 08.10.16 0:58, Jim Graham wrote: That looks great. A couple of questions. An updated version and a comments inline: http://cr.openjdk.java.net/~serb/8167310/webrev.01 Region, line 132: why are you testing for newv > coordinate? That was an attempt to catch an overflow like in case of int/longs. This is an unnecessary for double type, the code is removed and a spec of the method is also updated. Test case: Where do you get white gaps? Is it in the line test? If so, then consider setting the line width higher (or drawing 2 lines per iteration as I mention in the last paragraph). I can illustrate this with the following: Yes, it was in the lines. In the new version I changed the line width to 6. This value will fill the whole clip in all tested scales[0.1 - 4]. I did not use two lines because I use "srcover+transparentColor" and two lines can produce one more color after overlapping. Note that this will cover some pixels to the left of the clip (which should be clipped out), but it will leave a gap between the right side of the line and the right side of the clip, i.e. the next step (of about 0.75 pixels - which are likely to include a pixel center and thus leave a white pixel). At higher scales, the gap will be higher and more likely to include white pixels. A line width of 2 (actually 2*stepsize where stepsize=1 here) means that the line will cover an entire step regardless of the scale. Hopefully that will remove the need for the WHITE test in the validate method. Note that STROKE_CONTROL may come into play here as well. The above analysis is for STROKE_PURE semantics, but the normalization/biasing of STROKE_NORMALIZE might increase or decrease the chance that the above will fix the issue and may require an even larger line width. Another option without setting line width would be to draw two lines, one at step and another at step+1, which would test for gaps between adjacent lines as well as ensure that the test covers the "right half" of the line at step and the "left half" of the line at step+1... ...jim On 10/7/16 6:18 AM, Sergey Bylokhov wrote: Hello. Please review the fix for jdk9. This is bug which was found when the fractional scale is used in Swing. The problem is that if we save a usrClip as Rectangle2D then we incorrectly intersect it with device clip. The problem is in the RectangularShape.getBounds() method, see more details: http://mail.openjdk.java.net/pipermail/2d-dev/2016-July/007299.html So getBounds() produces the bigger Rectangle than is necessary and our clip became bigger as well. This means that in some fractional scales such clips are overlapping. The test will test fillRect, DrawImage, DrawLine methods and validates that there are no any overlapping if we set the clip.(The gaps between touched pixels are allowed only for lines). Note that as I understand the code this fix should affect the DrawLines, because in some situations it is possible that two lines are overlapped without clip. And two other cases(fillRect, drawImage) should work without clip, but only fillRect works. It is discussed here: http://mail.openjdk.java.net/pipermail/2d-dev/2016-October/007766.html Bug: https://bugs.openjdk.java.net/browse/JDK-8167310 Webrev can be found at: http://cr.openjdk.java.net/~serb/8167310/webrev.00 -- Best regards, Sergey.
Re: [OpenJDK 2D-Dev] [9] Review Request: 8167310 The graphics clip is incorrectly rounded for some fractional scales
That looks great. A couple of questions. Region, line 132: why are you testing for newv > coordinate? Test case: Where do you get white gaps? Is it in the line test? If so, then consider setting the line width higher (or drawing 2 lines per iteration as I mention in the last paragraph). I can illustrate this with the following: scale = 1.5 vertical line drawn with step, 0, step, SIZE covers the area: (step-0.5)*1.5, -0.5, (step+0.5)*1.5, SIZE+0.5 step*1.5-.75, ..., step*1.5+0.75 clipLoX-0.75, ..., clipHiX-0.75 Note that this will cover some pixels to the left of the clip (which should be clipped out), but it will leave a gap between the right side of the line and the right side of the clip, i.e. the next step (of about 0.75 pixels - which are likely to include a pixel center and thus leave a white pixel). At higher scales, the gap will be higher and more likely to include white pixels. A line width of 2 (actually 2*stepsize where stepsize=1 here) means that the line will cover an entire step regardless of the scale. Hopefully that will remove the need for the WHITE test in the validate method. Note that STROKE_CONTROL may come into play here as well. The above analysis is for STROKE_PURE semantics, but the normalization/biasing of STROKE_NORMALIZE might increase or decrease the chance that the above will fix the issue and may require an even larger line width. Another option without setting line width would be to draw two lines, one at step and another at step+1, which would test for gaps between adjacent lines as well as ensure that the test covers the "right half" of the line at step and the "left half" of the line at step+1... ...jim On 10/7/16 6:18 AM, Sergey Bylokhov wrote: Hello. Please review the fix for jdk9. This is bug which was found when the fractional scale is used in Swing. The problem is that if we save a usrClip as Rectangle2D then we incorrectly intersect it with device clip. The problem is in the RectangularShape.getBounds() method, see more details: http://mail.openjdk.java.net/pipermail/2d-dev/2016-July/007299.html So getBounds() produces the bigger Rectangle than is necessary and our clip became bigger as well. This means that in some fractional scales such clips are overlapping. The test will test fillRect, DrawImage, DrawLine methods and validates that there are no any overlapping if we set the clip.(The gaps between touched pixels are allowed only for lines). Note that as I understand the code this fix should affect the DrawLines, because in some situations it is possible that two lines are overlapped without clip. And two other cases(fillRect, drawImage) should work without clip, but only fillRect works. It is discussed here: http://mail.openjdk.java.net/pipermail/2d-dev/2016-October/007766.html Bug: https://bugs.openjdk.java.net/browse/JDK-8167310 Webrev can be found at: http://cr.openjdk.java.net/~serb/8167310/webrev.00
