Hello Prahalad,
Thanks for your inputs.
Regarding code change mentioned by you in source:
Initially I also just replaced xrCol.setColorValues(pixelVal, false) with
xrCol.setColorValues(pixelVal, true) in XRSolidSrcPict.java to fix the issue.
After that I listed call hierarchy to XRColor.setColorValues(int, boolean) and
found that after my fix at all the places we are passing second argument as
true and it is resulting in dead/unused code in XRColor.setColorValues(int,
boolean).
Dead code in XRColor.setColorValues(int, boolean) which will be never reached:
if (!pre) {
double alphaMult = XRUtils.XFixedToDouble(alpha);
this.red = (int) (red * alphaMult);
this.green = (int) (green * alphaMult);
this.blue = (int) (blue * alphaMult);
}
Before removing this dead code for review I researched and found that it is
always advised to remove the dead/unused
code(https://www.infoq.com/news/2017/02/dead-code ,
https://stackoverflow.com/questions/15699995/why-unused-code-should-be-deleted
). Also if we don't remove the dead code and just change second argument passed
in setColorValues() for our fix any static analyzer tool will show it as a
bug/unreachable code.
>From future use case perspective we have version control to get back the logic
>so it should not be a problem. So I think it's better to clean up the unused
>code even if we need to do small changes like removing the second argument in
>other files.
Regarding code change mentioned by you in test case:
I think you are referring to VolatileImage.validate(GraphicsConfiguration) and
checking whether it returns VolatileImage.IMAGE_INCOMPATIBLE.
In the test case present in this bug we are creating a Compatible Volatile
Image from a GraphicsConfiguration so I think there is no need to validate the
compatibility as GC. createCompatibleVolatileImage(int, int ) mentions that it
" Returns a VolatileImage with a data layout and color model compatible with
this GraphicsConfiguration ".
If I am missing something regarding the test case change you have mentioned
please guide me.
Regards,
Jay
-----Original Message-----
From: Prahalad Kumar Narayanan
Sent: Thursday, December 14, 2017 5:19 PM
To: Philip Race; Jayathirth D V; 2d-dev
Subject: RE: [OpenJDK 2D-Dev] [10] RFR JDK-8176795: Wrong color drawn when
painting translucent colors on volatile images using XRender.
Hello Jay
I looked into the changes. Here are my views.
> X Rendering extension expects pre-multiplied alpha color values, so we need
> to convert the non-premultiplied alpha color values to pre-multiplied alpha
> color values before give pixel value to XRender for drawing.
> The main problem is we do this operation of conversion twice
. Your observation is correct. This is a good find.
. The double conversion occurs because of mis-match between two objects -
. XRCompositeManager that passes pre-multiplied alpha color to
prepare XRSolidSrcPict and
. XRSolidSrcPict that expects a "non pre-multiplied" alpha color in
its prepareSrcPict method.
> Also this logic of non-pre to pre color conversion at XRender was only used
> when source is Solid color and not Texture/Gradient.
> So I have completely removed this logic itself as it not needed anywhere else.
. In the proposed fix, you have removed a convenience logic in
XRColor.java and modified one (XRColor) method signature as well.
. In my view, this change is not required at all. Reasons are-
. Though the convenience logic isn't used presently, it could
definitely come handy in future whenever a need arises.
. The change to method's signature has resulted in recursive
changes in many other files as well.
. The bug and the fix are not related to XRColor object in general.
Combining the above observation, the fix would be to correct 2nd argument of
below mentioned line from "false" to "true".
Thus retain the convenience logic in XRColor.java as well.
File: XRSolidSrcPict.java
line 50 xrCol.setColorValues(pixelVal, false); // Change false
to true
Btw, the test case involves drawing on a VolatileImage & getting its snapshot.
But it doesn't check if the created volatile image is valid for a particular
Graphics configuration before invoking any drawing operation. You could refer
to one of the existing test cases and correct the test file.
Thank you
Have a good day
Prahalad N.
----- Original Message -----
From: Phil Race
Sent: Tuesday, December 12, 2017 12:15 AM
To: Jayathirth D V; 2d-dev
Subject: Re: [OpenJDK 2D-Dev] [10] RFR JDK-8176795: Wrong color drawn when
painting translucent colors on volatile images using XRender.
The explanation sounds reasonable, although I'd like to give Clemens a chance
to review this.
-phil.
On 12/07/2017 07:16 AM, Jayathirth D V wrote:
Hello All,
Please review the following fix in JDK10 :
Bug : https://bugs.openjdk.java.net/browse/JDK-8176795
Webrev : http://cr.openjdk.java.net/~jdv/8176795/webrev.00/
Issue : When we draw translucent color over an opaque color in Unix from JDK 8
we get different color after composition compared to any other platform.
Root cause : From JDK 8, X Rendering extension is enabled in Unix and we see
this problem only when we use XRender in Unix if we use GLX or X11 we don't
see any issue. Also X Rendering extension expects pre-multiplied alpha color
values, so we need to convert the non-premultiplied alpha color values to
pre-multiplied alpha color values before give pixel value to XRender for
drawing. The main problem is we do this operation of conversion twice:
1) When we call Graphics2D.setColor() it uses ArgbPre PixelConverter based on
SurfaceData(here it is XRenderPixMap ArgbPre surface) and converts the color to
pre-multiplied values.
2) When we call Graphics2D.fillRect() internally before we compose the
destination(opaque color) and source(translucent color) we prepare source
render rectangle for X Render extension. Here again we convert the already
converted color values to premultiplied values.
Solution : There is no need for us to do non-pre to pre color conversion again
at XRender level so it should be removed. Also this logic of non-pre to pre
color conversion at XRender was only used when source is Solid color and not
Texture/Gradient. So I have completely removed this logic itself as it not
needed anywhere else.
Thanks,
Jay
