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.
