<AWT Dev> [8] Review request for 7153339: InternalError when drawLine with Xor and Antialiasing

[Oleg Pekhovskiy]

Hi Jim,

there are several places (e.g. in D3DRenderer and D3DSurfaceData) where 
InvalidPipeException could be thrown regardless the validity of SurfaceData.
As all SunGraphics2D draw-methods call revalidateAll() on IPE catch, 
getReplacement() could be potentially called for the valid SurfaceData.
Anyway, I made changes you proposed:
http://cr.openjdk.java.net/~bagiras/8/7153339.3/

Thanks,
Oleg

9/4/2012 10:50 PM, Jim Graham wrote:
> Hi Oleg,
>
> That looks much better.  One question - is there ever a case where 
> that method is called on a peer whose surface data is still valid?  
> (i.e. won't the first test always cause a replacement to be made?).  
> It's been a while since I wandered through this code so I don't 
> remember all of the conditions.
>
> If it is called a bit even for valid SDs then could you refactor it so 
> that we don't call getSurfaceData() twice in the "already valid" case?
>
>             ...jim
>
> On 9/4/12 11:32 AM, Oleg Pekhovskiy wrote:
>> Hi all,
>>
>> please review the second version of fix for CR:
>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7153339
>>
>> Webrev:
>> http://cr.openjdk.java.net/~bagiras/8/7153339.2/
>>
>> To avoid influence to the platforms other than Windows and guarantee
>> SurfaceData.getReplacement() returns valid surface
>> I made changes in ScreenUpdateManager.getReplacementScreenSurface()
>> where WComponentPeer.replaceSurfaceData()
>> is called to make peer's surfaceData valid.
>>
>> Thanks,
>> Oleg
>>
>> 8/25/2012 3:07 AM, Jim Graham wrote:
>>> Hi Oleg,
>>>
>>> Let me clarify, I'm not referring to a "better way" to fix this.
>>> Actually, the fix you have prepared has a problem. Once the SD is
>>> "invalid due to switching to software" and the SG2D decides to install
>>> a null SD instead, then it will never recover. But, no change to a
>>> drawable should happen (other than it going away) to cause that state.
>>> The SG2D's were designed to survive changes in state to the underlying
>>> drawable, as long as it remains viable as a drawing destination, but
>>> here you are giving up on it when that (externally not really very
>>> obvious) state change occurs. Thread A could be happily rendering to
>>> the surface on the screen, thread B decides to use XOR thereby
>>> changing the state, but thread A's G2D now stops working? Thread A
>>> would be very very confused.
>>>
>>> So, while there may be a fix we could add to SG2D to work around the
>>> problem, your fix isn't the right solution. I believe that the right
>>> solution is to have the D3DSurface return the correct new surface, but
>>> it being "what I think is the right solution" is just part of the
>>> issue with your currently proposed fix - the proposed fix violates a
>>> long-standing behavior of G2D objects to remain viable until the
>>> surface is gone...
>>>
>>> ...jim
>>>
>>> On 8/24/2012 3:01 AM, Oleg Pekhovskiy wrote:
>>>> Hi Jim,
>>>>
>>>> first of all I should say, that I prepared that fix for 7u as the most
>>>> safe, with minimum changes.
>>>> I agree that getReplacement() should return a valid sufrace or 
>>>> null, but
>>>> it doesn't happen during that switch.
>>>>
>>>> That CR was assigned to me because my previous changes for:
>>>> 7112642 Incorrect checking for graphics rendering object
>>>> 7121482 some sun/java2d and sun/awt tests failed with
>>>> InvalidPipeException
>>>> discovered the problem with getReplacement() and were somehow related.
>>>>
>>>> But maybe it would be better to create a separate CR for
>>>> getReplacement() issue and assign it to Java2d Team?
>>>> I also could add a comment for "!surfaceData.isValid()" reffering to
>>>> CR7153339.
>>>>
>>>> What do you think?
>>>>
>>>> Thanks,
>>>> Oleg
>>>>
>>>> 24.08.2012 4:04, Jim Graham wrote:
>>>>> Hi Oleg,
>>>>>
>>>>> getReplacement should not be returning an invalid surface. If the
>>>>> current D3DSD is invalid, then it should return a valid replacement.
>>>>> The only time it should return null is when the window is gone. It
>>>>> sounds like the window isn't gone here, it has just switched over 
>>>>> to a
>>>>> non-D3D type of SurfaceData and return that...
>>>>>
>>>>> ...jim
>>>>>
>>>>> On 8/23/2012 4:37 PM, Oleg Pekhovskiy wrote:
>>>>>> Hi Phil, Jim,
>>>>>>
>>>>>> thank you for pointing out the testing work that should be 
>>>>>> performed.
>>>>>> I tested my fix with the following regression tests:
>>>>>> test/java/awt/Graphics
>>>>>> test/java/awt/Graphics2D
>>>>>> test/java/awt/GraphicsDevice
>>>>>> test/java/awt/GraphicsEnvironment
>>>>>> test/sun/java2d
>>>>>>
>>>>>> Plus I tested performance differences on:
>>>>>> demo/jfc/Java2D/Java2Demo.jar
>>>>>>
>>>>>> Testing was done on Windows 7 & Ubuntu 12.04 LTS.
>>>>>> No differences were found.
>>>>>>
>>>>>> I also asked Yuri Nesterenko to test all that stuff on Mac.
>>>>>>
>>>>>> Thanks,
>>>>>> Oleg
>>>>>>
>>>>>>
>>>>>> 11.08.2012 3:10, Phil Race wrote:
>>>>>>> Oleg,
>>>>>>> This looks OK to me but since this is a shared code change I have
>>>>>>> to ask what testing you've done ?
>>>>>>>
>>>>>>> Also why not provide a regression test ? The provided test was
>>>>>>> interactive but I think it can be automated.
>>>>>>>
>>>>>>> You need another review and I'd like Jim to take a look.
>>>>>>>
>>>>>>> -phil.
>>>>>>>
>>>>>>> On 8/10/2012 2:26 PM, Oleg Pekhovskiy wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> Please review the fix for CR:
>>>>>>>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7153339
>>>>>>>>
>>>>>>>> Webrev:
>>>>>>>> http://cr.openjdk.java.net/~bagiras/8/7153339.1/
>>>>>>>>
>>>>>>>> Comments:
>>>>>>>> XOR is not supported for D3D (see comments inside
>>>>>>>> D3DSurfaceData.validatePipe()) and
>>>>>>>> software rendering is used invalidating current D3DSurfaceData.
>>>>>>>> So we have situation when component's peer has invalid SurfaceData
>>>>>>>> and it's retrieved in
>>>>>>>> SunGraphics2D.revalidateAll() through SurfaceData.getReplacement()
>>>>>>>> without any check.
>>>>>>>> That's why I added validity check there.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Oleg
>>>>>>>>
>>>>>>>> <http://cr.openjdk.java.net/%7Ebagiras/8/7153339.1/>
>>>>>>>
>>>>>>
>>>>>>
>>>>
>>