[10] RFR JDK-8176795: Wrong color drawn when painting translucent colors on volatile images using XRender.

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

[10] RFR JDK-8176795: Wrong color drawn when painting translucent colors on volatile images using XRender.

Jayathirth D v

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

 

Reply | Threaded
Open this post in threaded view
|

Re: [10] RFR JDK-8176795: Wrong color drawn when painting translucent colors on volatile images using XRender.

Jayathirth D v

Hello All,

 

Gentle reminder for review.

 

Thanks,

Jay

 

From: Jayathirth D V
Sent: Thursday, December 07, 2017 8:47 PM
To: 2d-dev
Subject: [OpenJDK 2D-Dev] [10] RFR JDK-8176795: Wrong color drawn when painting translucent colors on volatile images using XRender.

 

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

 

Reply | Threaded
Open this post in threaded view
|

Re: [10] RFR JDK-8176795: Wrong color drawn when painting translucent colors on volatile images using XRender.

Phil Race
In reply to this post by Jayathirth D v
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

 


Reply | Threaded
Open this post in threaded view
|

Re: [10] RFR JDK-8176795: Wrong color drawn when painting translucent colors on volatile images using XRender.

Prahalad kumar Narayanan
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
 
Reply | Threaded
Open this post in threaded view
|

Re: [10] RFR JDK-8176795: Wrong color drawn when painting translucent colors on volatile images using XRender.

Jayathirth D v
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
 
Reply | Threaded
Open this post in threaded view
|

Re: [10] RFR JDK-8176795: Wrong color drawn when painting translucent colors on volatile images using XRender.

Prahalad kumar Narayanan
Hello Jay

> 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)
    . I understand your point that the snippet of code is presently not being used by XRender pipeline.
    . But I 'm not convinced of removing the logic for reasons that I put in previous review.
    . If you believe, the code should be removed, my suggestion would be to take this code removal as a separate bug.
    . This way the revert operation to bring the logic back is easier.

> 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
    . Thanks for clarification. Your approach is right.

On a lighter note:
This being an open forum, there is a possibility that other reviewers might approve the change in its current version.
Hence for the benefit of engineer who has proposed the fix, I didn't respond to your email until now.

Thanks
Have a good day

Prahalad N.


-----Original Message-----
From: Jayathirth D V
Sent: Thursday, December 14, 2017 6:49 PM
To: Prahalad Kumar Narayanan; Philip Race; 2d-dev
Subject: RE: [OpenJDK 2D-Dev] [10] RFR JDK-8176795: Wrong color drawn when painting translucent colors on volatile images using XRender.

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
 
Reply | Threaded
Open this post in threaded view
|

Re: [10] RFR JDK-8176795: Wrong color drawn when painting translucent colors on volatile images using XRender.

Jayathirth D v
Hi Prahalad,

Thanks for your inputs.

I have no concerns about providing overall solution(Fix + Dead code removal) in two separate bugs.
I have removed the code corresponding to dead code removal and created a separate bug https://bugs.openjdk.java.net/browse/JDK-8195131 in which dead code will be removed.

Please review the updated webrev:
http://cr.openjdk.java.net/~jdv/8176795/webrev.01/ 

Thanks,
Jay

-----Original Message-----
From: Prahalad Kumar Narayanan
Sent: Tuesday, January 16, 2018 8:27 AM
To: Jayathirth D V; Philip Race; 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

> 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)
    . I understand your point that the snippet of code is presently not being used by XRender pipeline.
    . But I 'm not convinced of removing the logic for reasons that I put in previous review.
    . If you believe, the code should be removed, my suggestion would be to take this code removal as a separate bug.
    . This way the revert operation to bring the logic back is easier.

> 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
    . Thanks for clarification. Your approach is right.

On a lighter note:
This being an open forum, there is a possibility that other reviewers might approve the change in its current version.
Hence for the benefit of engineer who has proposed the fix, I didn't respond to your email until now.

Thanks
Have a good day

Prahalad N.


-----Original Message-----
From: Jayathirth D V
Sent: Thursday, December 14, 2017 6:49 PM
To: Prahalad Kumar Narayanan; Philip Race; 2d-dev
Subject: RE: [OpenJDK 2D-Dev] [10] RFR JDK-8176795: Wrong color drawn when painting translucent colors on volatile images using XRender.

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
 
Reply | Threaded
Open this post in threaded view
|

Re: [10] RFR JDK-8176795: Wrong color drawn when painting translucent colors on volatile images using XRender.

Prahalad kumar Narayanan
Hello Jay

The code change to fix the bug is good.

Thanks
Have a good day

Prahalad N.

-----Original Message-----
From: Jayathirth D V
Sent: Tuesday, January 16, 2018 12:21 PM
To: Prahalad Kumar Narayanan; 2d-dev
Subject: RE: [OpenJDK 2D-Dev] [10] RFR JDK-8176795: Wrong color drawn when painting translucent colors on volatile images using XRender.

Hi Prahalad,

Thanks for your inputs.

I have no concerns about providing overall solution(Fix + Dead code removal) in two separate bugs.
I have removed the code corresponding to dead code removal and created a separate bug https://bugs.openjdk.java.net/browse/JDK-8195131 in which dead code will be removed.

Please review the updated webrev:
http://cr.openjdk.java.net/~jdv/8176795/webrev.01/ 

Thanks,
Jay

-----Original Message-----
From: Prahalad Kumar Narayanan
Sent: Tuesday, January 16, 2018 8:27 AM
To: Jayathirth D V; Philip Race; 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

> 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)
    . I understand your point that the snippet of code is presently not being used by XRender pipeline.
    . But I 'm not convinced of removing the logic for reasons that I put in previous review.
    . If you believe, the code should be removed, my suggestion would be to take this code removal as a separate bug.
    . This way the revert operation to bring the logic back is easier.

> 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
    . Thanks for clarification. Your approach is right.

On a lighter note:
This being an open forum, there is a possibility that other reviewers might approve the change in its current version.
Hence for the benefit of engineer who has proposed the fix, I didn't respond to your email until now.

Thanks
Have a good day

Prahalad N.


-----Original Message-----
From: Jayathirth D V
Sent: Thursday, December 14, 2017 6:49 PM
To: Prahalad Kumar Narayanan; Philip Race; 2d-dev
Subject: RE: [OpenJDK 2D-Dev] [10] RFR JDK-8176795: Wrong color drawn when painting translucent colors on volatile images using XRender.

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
 
Reply | Threaded
Open this post in threaded view
|

Re: [10] RFR JDK-8176795: Wrong color drawn when painting translucent colors on volatile images using XRender.

Clemens Eisserer
Hi Jay,

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

Thanks a lot for identifying the issue and working on it.
The fix itself looks fine and low-risk.

Thanks again, Clemens
Reply | Threaded
Open this post in threaded view
|

Re: [10] RFR JDK-8176795: Wrong color drawn when painting translucent colors on volatile images using XRender.

Phil Race
+1 from me as well. I had approved the first version of the fix but this
one looks fine too.

-phil.

On 01/16/2018 11:05 AM, Clemens Eisserer wrote:
> Hi Jay,
>
>> 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.
> Thanks a lot for identifying the issue and working on it.
> The fix itself looks fine and low-risk.
>
> Thanks again, Clemens

Reply | Threaded
Open this post in threaded view
|

Re: [10] RFR JDK-8176795: Wrong color drawn when painting translucent colors on volatile images using XRender.

Clemens Eisserer
Hi,

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

Sorry for the dumb question, but what is the most recent 2d-dev repo,
which includes this change.
I have some significant performance enhancements for xrender regarding
MaskFill/Blit which I would like to re-base onto the lastest
"official" version.

I checked out the JDK and the JDK10 repo, but both did not contain this fix.

Thank you in advance & best regards, Clemens
Reply | Threaded
Open this post in threaded view
|

Re: [10] RFR JDK-8176795: Wrong color drawn when painting translucent colors on volatile images using XRender.

Jayathirth D v
Hi Clemens,

Forked repo for all the latest client changes is : http://hg.openjdk.java.net/jdk/client 

Thanks,
Jay

-----Original Message-----
From: Clemens Eisserer [mailto:[hidden email]]
Sent: Tuesday, February 20, 2018 7:04 PM
To: 2d-dev
Subject: Re: [OpenJDK 2D-Dev] [10] RFR JDK-8176795: Wrong color drawn when painting translucent colors on volatile images using XRender.

Hi,

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

Sorry for the dumb question, but what is the most recent 2d-dev repo, which includes this change.
I have some significant performance enhancements for xrender regarding MaskFill/Blit which I would like to re-base onto the lastest "official" version.

I checked out the JDK and the JDK10 repo, but both did not contain this fix.

Thank you in advance & best regards, Clemens
Reply | Threaded
Open this post in threaded view
|

Re: [10] RFR JDK-8176795: Wrong color drawn when painting translucent colors on volatile images using XRender.

Phil Race
Clemens,

What Jay said .. going forward the repos with a number in them are now
all stabilisation repos
to prepare for a release. So it won't be in JDK10 and there are very few
forests so not 2d specific one.
Just one for all the client code.

I am not sure what you meant by "JDK" repo .. it definitely is also
pushed into
the "master" as welll .. aka

http://hg.openjdk.java.net/jdk/jdk


See http://hg.openjdk.java.net/jdk/jdk/rev/e4b03365ddbf

But you should work in  http://hg.openjdk.java.net/jdk/client

-phil.

On 02/20/2018 05:39 AM, Jayathirth D V wrote:

> Hi Clemens,
>
> Forked repo for all the latest client changes is : http://hg.openjdk.java.net/jdk/client
>
> Thanks,
> Jay
>
> -----Original Message-----
> From: Clemens Eisserer [mailto:[hidden email]]
> Sent: Tuesday, February 20, 2018 7:04 PM
> To: 2d-dev
> Subject: Re: [OpenJDK 2D-Dev] [10] RFR JDK-8176795: Wrong color drawn when painting translucent colors on volatile images using XRender.
>
> Hi,
>
>>>> 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.
> Sorry for the dumb question, but what is the most recent 2d-dev repo, which includes this change.
> I have some significant performance enhancements for xrender regarding MaskFill/Blit which I would like to re-base onto the lastest "official" version.
>
> I checked out the JDK and the JDK10 repo, but both did not contain this fix.
>
> Thank you in advance & best regards, Clemens