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

[Jim Graham]

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