[OpenJDK 2D-Dev] [9] request for review: 8036022: D3D: rendering with XOR composite causes InternalError.

[Sergey Bylokhov]

Hi, Andrew.
The fix looks good to me too.

On 3/20/14 4:51 AM, Jim Graham wrote:
> Hi Andrew,
>
> revalidateAll() unconditionally calls validatePipe() which will do all 
> of the work for choosing pipelines again anyway (I don't think any 
> implementations of validatePipe try to share too much, do they?)  
> Also, this tends to be called when the surface was invalidated and an 
> exception was thrown because the old surface was being used, so I 
> don't think there can be much that happens as a result of an 
> unconditional total revalidation that might cost any more than an 
> exception throw and an SD replacement...
>
> This looks good.  Approved...
>
>             ...jim
>
> On 3/19/14 6:05 AM, Andrew Brygin wrote:
>> Hi Jim,
>>
>>    we probably may want to avoid the unconditional invalidation here
>>    due to performance reasons. Potentially it saves some work if the
>>    replacement type is exactly same, and the current set of pipes can
>>    be re-used.
>>
>>    However, as soon as there seems to be no reliable way to check
>>    whether current set of pipes corresponds to the replacement surface,
>>    it makes sense to invalidate it unconditionally.
>>
>>    Please take a loot to updated webrev:
>>    http://cr.openjdk.java.net/~bae/8036022/9/webrev.02/
>>
>> Thanks,
>> Andrew
>>
>> On 3/19/2014 4:39 AM, Jim Graham wrote:
>>> Interesting. I don't know how this impacts the original issue, but I
>>> think we do need some form of invalidation there.  Is there any reason
>>> not to make it unconditional?  I think that code was logically
>>> intending to start over from scratch so an unconditional reset of all
>>> of the pipes makes sense.
>>>
>>> The following comment is probably obsolete given what I just said in
>>> the last paragraph, but I wanted to also point out that equals() on a
>>> SurfaceType may not be the right thing to use since we may have
>>> multiple ST's with the same "type", but they are distinct objects
>>> because they inherit from different types at a higher level. In
>>> essense the ST is a chain of related types, not just a single
>>> description of one way of looking at the surface (which is all an
>>> individual object can do is describe it one way and link to another
>>> description).  In the end, I probably should have distinguished
>>> between ST and "collection of STs to describe a surface", but got
>>> clever with chaining instead.  As a result, I think that == is the
>>> more proper way to compare STs?  (Or outlaw comparing them?) But,
>>> given that I think it makes sense to make the invalidate
>>> unconditional, I don't think this matters...
>>>
>>>             ...jim
>>>
>>> On 3/17/14 2:22 AM, Andrew Brygin wrote:
>>>> Hello Jim,
>>>>
>>>>   I have completely changed the fix while trying to answer your
>>>> questions :)
>>>>
>>>>   I agree that the check for composite type is incorrect: in fact we
>>>> have to check
>>>>   whether the gdi surface data can be used as a destination for
>>>> rendering primitives
>>>>   which the 'loops' filed refers to.
>>>>
>>>>   However, the main question is why the 'loops' is non-null after the
>>>> d3d->gdi fallback.
>>>>   It happens because d3d surface leaves d3d-specific loops and pipes
>>>> registered in
>>>>   the graphics instance in the case of XOR, and just disables the
>>>> acceleration for given
>>>>   peer. In particular, it leaves rendering loops as is, and it 
>>>> leads to
>>>> fall into generic (any-to-any)
>>>>   loop in the case of gdi surface. To avoid this, we have to 
>>>> invalidate
>>>> the graphics
>>>>   at some point.
>>>>
>>>>   This invalidation probably can be done in 'revalidateAll' method: if
>>>> original surface data,
>>>>   and it's replacement have different types, we probably have to
>>>> invalidate the graphics object
>>>>   in order to avoid the situation described above.
>>>>
>>>>   Please take a look to updated fix:
>>>> http://cr.openjdk.java.net/~bae/8036022/9/webrev.01/
>>>>
>>>> Thanks,
>>>> Andrew
>>>>
>>>> On 3/14/2014 4:36 AM, Jim Graham wrote:
>>>>> Hi Andrew,
>>>>>
>>>>> It looks like you are covering the case of the existing loops being
>>>>> Solid and we switch to XOR so you get new loops.  What about the case
>>>>> where we used to be XOR and then we switch to Solid and the loops
>>>>> might be stale, but in the other way (i.e. XOR when we want Solid
>>>>> rather than Solid when we want XOR)?  Also, if we were XOR and we
>>>>> remain XOR, does this force us to fetch the loops on every validate?
>>>>>
>>>>> I forget the strategy for the loops variable, was it supposed to be
>>>>> set to null to force a refetch somewhere, but some invalidation case
>>>>> missed setting the loops=null for XOR?
>>>>>
>>>>>         ...jim
>>>>>
>>>>> On 3/13/14 4:54 AM, Andrew Brygin wrote:
>>>>>> Hello,
>>>>>>
>>>>>> could you please review a fix for CR 8036022?
>>>>>>
>>>>>> Bug: http://bugs.sun.com/view_bug.do?bug_id=8036022
>>>>>> Webrev: http://cr.openjdk.java.net/~bae/8036022/9/webrev.00/
>>>>>>
>>>>>> In case of XOR composite, rendering pipeline falls back from
>>>>>> d3d to gdi. However, gdi surface data does not re-set rendering
>>>>>> loops during validation, that leads to usage of the software
>>>>>> loops (due to dst type mismatch), and results in observed internal
>>>>>> error.
>>>>>>
>>>>>> Suggested fix forces the re-set of the rendering loops,
>>>>>> at least for the case of XOR composite.
>>>>>> This change does not trigger any existing regression test.
>>>>>>
>>>>>> Supplied regression test demonstrates the problem.
>>>>>>
>>>>>> Please take a look.
>>>>>>
>>>>>> Thanks,
>>>>>> Andrew.
>>>>
>>


-- 
Best regards, Sergey.