On Mon, 1 Nov 2021 20:03:42 GMT, Sergey Bylokhov <s...@openjdk.org> wrote:

> Please take a look to one more wagon in this train:
> [JDK-8176795](https://bugs.openjdk.java.net/browse/JDK-8176795)->[JDK-8204931](https://bugs.openjdk.java.net/browse/JDK-8204931)->[JDK-8214579](https://bugs.openjdk.java.net/browse/JDK-8214579)->[JDK-8227392](https://bugs.openjdk.java.net/browse/JDK-8227392)->[Current
>  issue](https://bugs.openjdk.java.net/browse/JDK-8275843). 
> 
> #### The short description:
> 
> We have an inconsistent between xrender java code and the x11 window. The 
> java code thinks that all our windows support only two types of color models: 
> IntRgbX11 and IntArgbPreX11, but the x11 window supports more types. If such 
> inconsistency occurs we render some garbage, or just crash since our "blits" 
> will assume the wrong data layout. This issue was reported and fixed 
> [before](http://hg.openjdk.java.net/jdk/jdk/rev/c53905e7dc57) but it was not 
> realized why the rendering was wrong and that it can cause a crash. The fix 
> is similar to the old one.
> 
> #### The long description:
> 
>  * Once upon a time this bug was filed 
> [JDK-8176795](https://bugs.openjdk.java.net/browse/JDK-8176795) it was about 
> the rendering of colors to the volatile image, the description can be found 
> [here](https://bugs.openjdk.java.net/browse/JDK-8176795?focusedCommentId=14138323&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14138323)
>  In short: the volatile images in the xrender use intArgbPre color model, it 
> means that the SunGraphics2D object 
> [convert](https://github.com/openjdk/jdk/blob/603bba282a089928fd23f8da23a7c1b2d52944ef/src/java.desktop/share/classes/sun/java2d/SunGraphics2D.java#L1753)
>  the argb value to the argbPre before passing it to the pipeline, and the 
> xrender pipeline 
> [convert](http://hg.openjdk.java.net/jdk/jdk/rev/e4b03365ddbf) this value to 
> the argbPre one more time -> double-conversion-> the bug is reproduced. The 
> fix for that bug disabled the second conversion in the xrender. but it does 
> not take into account that this code path is u
 sed for rendering to the ARGB X11 window where the shared code does not 
convert the color, so the next bug was filed.
>  * The second bug is 
> [JDK-8204931](https://bugs.openjdk.java.net/browse/JDK-8204931), the 
> description can be found 
> [here](https://bugs.openjdk.java.net/browse/JDK-8204931?focusedCommentId=14199800&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14199800).
>  In short the bug was about rendering directly to the window and missing 
> color conversion removed previously. The fix for that bug changed the surface 
> type used by the window, so the shared code start to convert the color from 
> ARGB to argbPre, but it does not take into account that we do not control the 
> color model of the window, see how many of them we actually 
> [support](https://github.com/openjdk/jdk/blob/603bba282a089928fd23f8da23a7c1b2d52944ef/src/java.desktop/unix/classes/sun/java2d/x11/X11SurfaceData.java#L501)
>  Also note in the link above that if we do not recognize the type we do not 
> fallback to something but throw InvalidPipeException, this is because the 
> blits(and other primitives) may use t
 his type to access the data directly. So the next bug was reported:
>  * This is the third issue 
> [JDK-8214579](https://bugs.openjdk.java.net/browse/JDK-8214579). The fix for 
> that issue tried to roll back the previous one but does not take into account 
> that the conversion removed by the first issue should be rollback as well, so 
> the last bug was filed.
>  * The "last" issue is 
> [JDK-8227392](https://bugs.openjdk.java.net/browse/JDK-8227392) rollback the 
> previous one. So the bug reported in the third report come back.
> 
> #### Fix description
> 
> The fix for the current issue contains a few parts:
>  * Rollback of the 
> [JDK-8204931](https://bugs.openjdk.java.net/browse/JDK-8204931) so we will 
> use the proper surface data for the window.
>  * Return conversion in the xrender pipeline removed by the 
> [JDK-8176795](https://bugs.openjdk.java.net/browse/JDK-8176795) So we will 
> convert it when we render to the volatile image(argbPre) and to the 
> window(argb). We need to convert in both cases since the xrender API which we 
> use expects the argbPre colors.
>  * Start to use the 
> [eargb](https://github.com/openjdk/jdk/blob/603bba282a089928fd23f8da23a7c1b2d52944ef/src/java.desktop/share/classes/sun/java2d/SunGraphics2D.java#L1752)
>  value( instead of pixel) stored in the SunGraphics2D so we did not get the 
> double conversion.
> 
> Some tests are updated and one more is added.
> 
> ### Note
> I have checked the performance implication of this change and it looks like 
> after this change the sequence window.setvisible(true)/window.dispose() will 
> work 30% faster, this may affect the tests which does not wait enough time to 
> make the window visible. Probably this is the reason why some of our tests 
> became more stable at some point in jdk11.
> 
> 
> I have tested this by the jck and jdk_desktop tests. But any additional 
> verification/testing is welcome.

I can confirm that testing looks good, however I didn't manage to get is 
crashed even once before the fix even with updated tests. (on personal, VM or 
multiple CI runs)

How often it fails for you and on what config?

src/java.desktop/unix/classes/sun/java2d/xr/XRSurfaceData.java line 403:

> 401:         SurfaceType sType = null;
> 402: 
> 403:         switch (transparency) {

Since you are touching this, you might want to fix indent for cases in this 
switch.

-------------

PR: https://git.openjdk.java.net/jdk/pull/6199

Reply via email to