[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-2d-dev
Subject:    [OpenJDK 2D-Dev] Please review patch for 7105461
From:       james.graham () oracle ! com (Jim Graham)
Date:       2012-09-21 20:40:21
Message-ID: 505CD0B5.8060706 () oracle ! com
[Download RAW message or body]

Hi Clemens,

Is the clip guaranteed to be rectangular here?  Or is the clipping code 
only meant to optimize the region of interest being processed?

In either case, I think the intersectsQuickCheckXYXY method might work 
better since it does the entire test in one go and it if this code has 
to deal with complex clipping then I think it would be a better choice 
of test in the first place (it won't be fooled by "the value is inside 
the bounds of the clip, but misses out on the specific sub-rects that 
are contained in it" and it will be faster than scanning a bunch of 
sub-rects only to approve the application of a basic bounds-intersection 
operation).

(A similar caveat may apply to drawLine unless we can guarantee that the 
comp clip is rectangular...)

Finally, why is the awtLock() moved inside the try/finally?  If it fails 
then you probably don't want to do an Unlock() do you?  Looking through 
the rest of the file, half of the methods call it inside the try-block 
and half call it outside - we should be consistent there (and according 
to the doc comments in SunToolkit, outside is probably the right choice)...

			...jim

On 9/21/2012 10:04 AM, Clemens Eisserer wrote:
> Hi Jim,
>
> Sorry it took so long - could you please have a look at
> http://cr.openjdk.java.net/~ceisserer/7105461/webrev.02
> It does now protect against integer overflow, using the Region.clipAdd.
>
> Thanks, Clemens
>
> 2012/4/17 Jim Graham<james.graham at oracle.com>:
>> This code doesn't protect against integer overflow.  We don't protect
>> against it in many places, but it couldn't hurt to get in the habit.  I
>> think the Region code has some methods that do safe addition of integers
>> with simple limit clipping that would work fine for rectangle dimensions.
>>
>> (JDK8 will be introducing new methods in Math for unsigned results and exact
>> non-overflowing integer results as well, but that would complicate any
>> backports to JDK7...)
>>
>>                          ...jim
>>
>>
>> On 4/13/12 9:46 AM, Clemens Eisserer wrote:
>>>
>>> Hi,
>>>
>>> Please take a look at the patch for bug 7105461, located at
>>> http://cr.openjdk.java.net/~ceisserer/7105461/webrev.00/
>>>
>>> The problem was caused by Swing calling drawLine/fillRect with
>>> coordinates outside the valid X11 coordinate space.
>>> I took the same approach of the original X11 pipeline to simply clamp
>>> the corrdinates to the min/max allowed value although its not enterly
>>> correct - as it doesn't adjust width/height in case it clamps x/y -
>>> triggered for exmaple by the following call:
>>>          g.fillRect(-32868, 0, 32968, 10);
>>>
>>> Should I take care of this special case, or is it ok to handle it the
>>> same way the X11 pipeline does?
>>>
>>> Thanks, Clemens

[prev in list] [next in list] [prev in thread] [next in thread]