Re: [graphics] [9] Review request for 8155903: Crash while running imported/w3c/canvas/2d.gradient.interpolate.overlap2.html
Hello Jim, Thank you. Incorporated your review comments in http://cr.openjdk.java.net/~arajkumar/8155903/webrev.03 Please take a look. -- Arun On 5/9/2016 11:51 PM, Jim Graham wrote: That looks good for the case where Imin is zero, but it appears that we could also have overflow as well, with a single very tiny Imin the accumulation of estimatedSize with an "int" type could easily overflow and become essentially a random number. Changing the estimatedSize variable to a float should prevent that related issue... ...jim On 5/9/16 6:16 AM, Arunprasad Rajkumar wrote: Hello Jim, Thanks for your suggestions. As of now I taking an easy way to fix the issue, New changes are available at http://cr.openjdk.java.net/~arajkumar/8155903/webrev.02 I couldn't write a reliable test case using public javafx APIs, the behavior is intermittent. However I could consistently produce the issue using our DRT internal test case which is based on W3C functional test[1]. [1] http://w3c-test.org/2dcontext/fill-and-stroke-styles/2d.gradient.interpolate.overlap2.html Thanks, Arun On 5/5/2016 11:51 PM, Jim Graham wrote: Hi Arun, The change you made to the calculateSingleArray method looks like it produces a bad array of color stops for the case where Imin is 0. You should fall into the calculateMultipleArray method instead which should not have any trouble with zero length intervals. At that point you don't have to have any code in calculateSingleArray that deals with Imin being zero because that can be part of its calling contract. That is the quick fix. The harder fix that would let us maintain speed when there is a zero interval would be to teach the code what to do in that special case. Basically a zero interval means that we have a case where approaching the interval point from the left is interpolating towards colorA, but leaving that point from the right is interpolating from colorB, with a sudden transition between those 2. In that case, a zero interval shouldn't affect the Imin, since the Imin is trying to determine the size of each interpolated region and we don't interpolate across a zero-sized interval. So, the scan for Imin would simply ignore zero length intervals. Later, the code that populates the array in the calculateSingleArray function would know that the zero length interval simply means swap in a new "from color" without any actual interpolation. Getting that harder fix right would require a lot of testing, so it may be better to do the quick fix now to stop the exceptions and then deal with the optimization as a tweak filed for later... ...jim On 05/05/2016 01:57 AM, Arunprasad Rajkumar wrote: Hello Jim, Please review the below patch. JIRA: https://bugs.openjdk.java.net/browse/JDK-8155903 Webrev: http://cr.openjdk.java.net/~arajkumar/8155903/webrev.01 Issue: Divide by zero while adding multiple gradient stops at same offset. Regards, Arun
Re: [graphics] [9] Review request for 8155903: Crash while running imported/w3c/canvas/2d.gradient.interpolate.overlap2.html
That looks good for the case where Imin is zero, but it appears that we could also have overflow as well, with a single very tiny Imin the accumulation of estimatedSize with an "int" type could easily overflow and become essentially a random number. Changing the estimatedSize variable to a float should prevent that related issue... ...jim On 5/9/16 6:16 AM, Arunprasad Rajkumar wrote: Hello Jim, Thanks for your suggestions. As of now I taking an easy way to fix the issue, New changes are available at http://cr.openjdk.java.net/~arajkumar/8155903/webrev.02 I couldn't write a reliable test case using public javafx APIs, the behavior is intermittent. However I could consistently produce the issue using our DRT internal test case which is based on W3C functional test[1]. [1] http://w3c-test.org/2dcontext/fill-and-stroke-styles/2d.gradient.interpolate.overlap2.html Thanks, Arun On 5/5/2016 11:51 PM, Jim Graham wrote: Hi Arun, The change you made to the calculateSingleArray method looks like it produces a bad array of color stops for the case where Imin is 0. You should fall into the calculateMultipleArray method instead which should not have any trouble with zero length intervals. At that point you don't have to have any code in calculateSingleArray that deals with Imin being zero because that can be part of its calling contract. That is the quick fix. The harder fix that would let us maintain speed when there is a zero interval would be to teach the code what to do in that special case. Basically a zero interval means that we have a case where approaching the interval point from the left is interpolating towards colorA, but leaving that point from the right is interpolating from colorB, with a sudden transition between those 2. In that case, a zero interval shouldn't affect the Imin, since the Imin is trying to determine the size of each interpolated region and we don't interpolate across a zero-sized interval. So, the scan for Imin would simply ignore zero length intervals. Later, the code that populates the array in the calculateSingleArray function would know that the zero length interval simply means swap in a new "from color" without any actual interpolation. Getting that harder fix right would require a lot of testing, so it may be better to do the quick fix now to stop the exceptions and then deal with the optimization as a tweak filed for later... ...jim On 05/05/2016 01:57 AM, Arunprasad Rajkumar wrote: Hello Jim, Please review the below patch. JIRA: https://bugs.openjdk.java.net/browse/JDK-8155903 Webrev: http://cr.openjdk.java.net/~arajkumar/8155903/webrev.01 Issue: Divide by zero while adding multiple gradient stops at same offset. Regards, Arun
Re: [graphics] [9] Review request for 8155903: Crash while running imported/w3c/canvas/2d.gradient.interpolate.overlap2.html
Hello Jim, Thanks for your suggestions. As of now I taking an easy way to fix the issue, New changes are available at http://cr.openjdk.java.net/~arajkumar/8155903/webrev.02 I couldn't write a reliable test case using public javafx APIs, the behavior is intermittent. However I could consistently produce the issue using our DRT internal test case which is based on W3C functional test[1]. [1] http://w3c-test.org/2dcontext/fill-and-stroke-styles/2d.gradient.interpolate.overlap2.html Thanks, Arun On 5/5/2016 11:51 PM, Jim Graham wrote: Hi Arun, The change you made to the calculateSingleArray method looks like it produces a bad array of color stops for the case where Imin is 0. You should fall into the calculateMultipleArray method instead which should not have any trouble with zero length intervals. At that point you don't have to have any code in calculateSingleArray that deals with Imin being zero because that can be part of its calling contract. That is the quick fix. The harder fix that would let us maintain speed when there is a zero interval would be to teach the code what to do in that special case. Basically a zero interval means that we have a case where approaching the interval point from the left is interpolating towards colorA, but leaving that point from the right is interpolating from colorB, with a sudden transition between those 2. In that case, a zero interval shouldn't affect the Imin, since the Imin is trying to determine the size of each interpolated region and we don't interpolate across a zero-sized interval. So, the scan for Imin would simply ignore zero length intervals. Later, the code that populates the array in the calculateSingleArray function would know that the zero length interval simply means swap in a new "from color" without any actual interpolation. Getting that harder fix right would require a lot of testing, so it may be better to do the quick fix now to stop the exceptions and then deal with the optimization as a tweak filed for later... ...jim On 05/05/2016 01:57 AM, Arunprasad Rajkumar wrote: Hello Jim, Please review the below patch. JIRA: https://bugs.openjdk.java.net/browse/JDK-8155903 Webrev: http://cr.openjdk.java.net/~arajkumar/8155903/webrev.01 Issue: Divide by zero while adding multiple gradient stops at same offset. Regards, Arun
[graphics] [9] Review request for 8155903: Crash while running imported/w3c/canvas/2d.gradient.interpolate.overlap2.html
Hello Jim, Please review the below patch. JIRA: https://bugs.openjdk.java.net/browse/JDK-8155903 Webrev: http://cr.openjdk.java.net/~arajkumar/8155903/webrev.01 Issue: Divide by zero while adding multiple gradient stops at same offset. Regards, Arun
