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

[Jim Graham]

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