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

[Oleg Pekhovskiy]

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/>
>>>>>
>>>>
>>>>
>>