Re: [OpenJDK 2D-Dev] Request for review: Additional fix for JDK-7159455
Hi Jim, Thanks for taking the time to review this in-depth. I guess that also explains why the source coordinates were multiplied by the scale, though that seems a rather odd way to handle these issues. Indeed, the offsets could be also stored as part of the transformation (as it is done for TransformBlit), which would be more elegant. A new question/issue - on line 314, what happens if the case is both quadrant rotated and extra-alpha'd? Either of those will get us into that code block, but also both of them could be true at the same time in which case it looks like you forego the retreival of the EA mask...? In this case a 1x1 mask with EA prepared in advance by XRCompositionManager will be used, returned by xrMgr.getExtraAlphaMask(). However, thinking about it again, it seems to me the code could be simplified a bit more. As soon as a TransformBlit is quadrant rotated, the else-branch (line 321) can handle all cases even with extra-alpha (compositeBlit handles EA in a generic way) - which would make the conditional at line 314 redundant. However, I tended to versimplify this part, so maybe this is something for JDK9 ;) - Clemens
Re: [OpenJDK 2D-Dev] Request for review: Additional fix for JDK-7159455
Hi Clemens, On 11/6/13 9:38 AM, Clemens Eisserer wrote: A new question/issue - on line 314, what happens if the case is both quadrant rotated and extra-alpha'd? Either of those will get us into that code block, but also both of them could be true at the same time in which case it looks like you forego the retreival of the EA mask...? In this case a 1x1 mask with EA prepared in advance by XRCompositionManager will be used, returned by xrMgr.getExtraAlphaMask(). I think I had that backwards. I was referring to the other case which does not call xrMgr.getExtraAlphaMask(): 312 // For quadrant-transformed blits geometry is not stored inside the mask 313 // therefore we can use a repeating 1x1 mask for applying extra alpha. 314 int maskPicture = isQuadrantRotated ? xrMgr.getExtraAlphaMask() 315 : mask.prepareBlitMask(x11sdDst, maskTX, width, height); In the mask.prepareBlitMask case I don't see where extra alpha is processed, but perhaps I am missing something and it is already registered in some data structure somewhere? ...jim
Re: [OpenJDK 2D-Dev] Request for review: Additional fix for JDK-7159455
Hi Clemens, I sincerely apologize for not seeing this sooner, my email sorting is a little primitive, but I should check these alternate folders more often. The one thing I am concerned about here is the rounding that is going on for scaled images. You removed a comment about non-integer coordinates that I think is germane. In particular, if a scale is animating and an image is rendered to 0,0=10,10 and then 0,0=10.25,10.25 then the pixels chosen will subtly change, but the code there rounds the coordinates to integers so you would see no change. While the bounds of the result would be correct, the exact pixels chosen from the source image for each pixel in the destination would not be quite right. Where can you communicate to the maskBuffer.compositeBlit() method that, while the coordinates given represent the bounds of interest, they aren't the exact definition of the precise scaling parameters in play? ...jim On 10/18/13 4:45 AM, Clemens Eisserer wrote: Another revision was required, available at: http://cr.openjdk.java.net/~ceisserer/fix12/webrev.04/ When the destination-area is quadratic, no geometry is stored inside the mask and therefore the composition-window is used for clipping and it's size has to be rounded properly. In cases where it is not quadrant transformed, the code makes now sure no pixels are clipped away unintentionally. Thanks for your patience, Clemens
Re: [OpenJDK 2D-Dev] Request for review: Additional fix for JDK-7159455
Ah, I see. And, I was about to rant about MT issues, but I see now that this is all inside an AWTLock. I guess that also explains why the source coordinates were multiplied by the scale, though that seems a rather odd way to handle these issues. A new question/issue - on line 314, what happens if the case is both quadrant rotated and extra-alpha'd? Either of those will get us into that code block, but also both of them could be true at the same time in which case it looks like you forego the retreival of the EA mask...? ...jim On 11/5/13 1:24 PM, Clemens Eisserer wrote: Hi Jim, I sincerely apologize for not seeing this sooner, my email sorting is a little primitive, but I should check these alternate folders more often. Never mind :) The one thing I am concerned about here is the rounding that is going on for scaled images. You removed a comment about non-integer coordinates that I think is germane. In particular, if a scale is animating and an image is rendered to 0,0=10,10 and then 0,0=10.25,10.25 then the pixels chosen will subtly change, but the code there rounds the coordinates to integers so you would see no change. While the bounds of the result would be correct, the exact pixels chosen from the source image for each pixel in the destination would not be quite right. Where can you communicate to the maskBuffer.compositeBlit() method that, while the coordinates given represent the bounds of interest, they aren't the exact definition of the precise scaling parameters in play? The scaling parameters are stored as properties of the xrender source surface and are set within the validateAsSource() method, while the parameters passed to the compositeBlit()-function only specify the size of the composition window. Regards, Clemens
Re: [OpenJDK 2D-Dev] Request for review: Additional fix for JDK-7159455
Hello Clemens, the change in XRPMBlitLoops.java looks good to me. The part of the fix in XRUtils.java should probably be ignored now. Thanks, Andrew On 10/18/2013 3:45 PM, Clemens Eisserer wrote: Another revision was required, available at: http://cr.openjdk.java.net/~ceisserer/fix12/webrev.04/ When the destination-area is quadratic, no geometry is stored inside the mask and therefore the composition-window is used for clipping and it's size has to be rounded properly. In cases where it is not quadrant transformed, the code makes now sure no pixels are clipped away unintentionally. Thanks for your patience, Clemens
Re: [OpenJDK 2D-Dev] Request for review: Additional fix for JDK-7159455
Another revision was required, available at: http://cr.openjdk.java.net/~ceisserer/fix12/webrev.04/ When the destination-area is quadratic, no geometry is stored inside the mask and therefore the composition-window is used for clipping and it's size has to be rounded properly. In cases where it is not quadrant transformed, the code makes now sure no pixels are clipped away unintentionally. Thanks for your patience, Clemens
Re: [OpenJDK 2D-Dev] Request for review: Additional fix for JDK-7159455
Clemens, This looks good to me. I used the Java 2D demo to test it. What were you using as a test ? I'll need to file a new bug to commit this additional fix. I'll push once Jim has OK'd this too. -phil. On 10/18/2013 4:45 AM, Clemens Eisserer wrote: Another revision was required, available at: http://cr.openjdk.java.net/~ceisserer/fix12/webrev.04/ When the destination-area is quadratic, no geometry is stored inside the mask and therefore the composition-window is used for clipping and it's size has to be rounded properly. In cases where it is not quadrant transformed, the code makes now sure no pixels are clipped away unintentionally. Thanks for your patience, Clemens
Re: [OpenJDK 2D-Dev] Request for review: Additional fix for JDK-7159455
Hi, Please find an updated webrev with your suggestions at: http://cr.openjdk.java.net/~ceisserer/fix12/webrev.01/ Am I reading this right. You are saying that it is used in a mixture of cases and some of those will support full transforms? Exactly. What I was suggesting is that the adjust method can do more optimal work if the transform is quadrant-rotated and so it needs to know that information as well. I'm not suggesting changing the technique for determining the type of rotation, Done. If you prefer I could also write a single-if version, which would be more pretty but also contain some duplicate code. This would depend on the semantics of the draw operation. I believe that other places in our other pipelines are consistent with round-down semantics and so this pipeline should be consistent. This may be worth writing a test that slowly translates an image to see when it jumps pixels, especially if it can be automated, so that we can keep all of the pipelines in synch. I can confirm the round-down bahaviour at least for the software-loops. Once I did a test slowly scaling an image, but jump-points were different between the ogl/d3d, xrender and software pipelines. I agree that floor/ceil sounds more correct for this case. Perhaps we need a boolean for the adjust method? Or is there a case where the caller may use it in different ways and doesn't know which way is appropriate before it calls the method? No there isn't, so I implemented it this way ;) Thanks, Clemens
Re: [OpenJDK 2D-Dev] Request for review: Additional fix for JDK-7159455
As usual, seconds after sending a review request I somewhere find another issue - sorry. Please instead of webrev.01, review http://cr.openjdk.java.net/~ceisserer/fix12/webrev.02/ In the previous version, I assumed not using a mask means a quadrant-rotated transform, which is not correct as the mask can also be ommited in any case when nearest neighbour interpolation is used. Thanks, Clemens
[OpenJDK 2D-Dev] Request for review: Additional fix for JDK-7159455
Hi, As Jim pointed out, my earlier fix for bug JDK-7159455 wasn't entirely correct as it allowed some shear-transformations to pass a check for a fast-path although they should have been rejected. He proposed a way of using the capabilities already provided by AffineTransform, which is correct, shorter and most likely more efficient (thanks!). Please find the follow-up fix for JDK-7159455 at: http://cr.openjdk.java.net/~ceisserer/fix12/webrev.00/ Thanks, Clemens
Re: [OpenJDK 2D-Dev] Request for review: Additional fix for JDK-7159455
Hi Clemens, Now that we've narrowed it down to a quadrant rotation at best, can't we get by with only transforming x,y and x+w,y+h? Or is the adjust method used from other locations that might have a non-quadrant transform? (Or do any callers depend on srcCoords and dstCoords being fully populated?). If it's a mixture of quadrant and general transforms, then I think it may make sense to have the adjust method do the test, determine how much it needs to transform, and then return the result, but that is probably a minor optimization compared to other work going on (not sure if benchmarks would notice the difference). I also noticed that the bounds are rounded. If the intent is to catch every single pixel, then shouldn't those be floor/ceil operations? If the intent is to rasterize the transformed rectangle then filled rectangles (and shapes) use a round down calculation, but Math.round() is a round up calculation. How is the resulting compositeBounds used? Also, I noticed that you added a new wild-card import into one of the files. I believe we deprecated that practice in the FX source base, but I don't know what the standard practice is in OpenJDK and didn't see any statements about it in the OpenJDK style guildelines. ...jim On 10/16/13 10:55 AM, Clemens Eisserer wrote: Hi, As Jim pointed out, my earlier fix for bug JDK-7159455 wasn't entirely correct as it allowed some shear-transformations to pass a check for a fast-path although they should have been rejected. He proposed a way of using the capabilities already provided by AffineTransform, which is correct, shorter and most likely more efficient (thanks!). Please find the follow-up fix for JDK-7159455 at: http://cr.openjdk.java.net/~ceisserer/fix12/webrev.00/ Thanks, Clemens
