[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-2d-dev
Subject:    Re: [OpenJDK 2D-Dev] [10] RFR JDK-8176795: Wrong color drawn when painting translucent colors on vol
From:       Jayathirth D V <jayathirth.d.v () oracle ! com>
Date:       2017-12-14 13:18:48
Message-ID: 294c9548-0095-4b68-bb6b-1c8d0eba2cc1 () default
[Download RAW message or body]

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
 


[prev in list] [next in list] [prev in thread] [next in thread]