[OpenJDK 2D-Dev] [2D-Dev] Review Request: JDK-8015070: Antialiased text on translucent backgrounds gets bright artifacts

[Jim Graham]

Hi Prahalad,

This basically boils down to "alpha blending math needs to be performed 
in premultiplied form" and the existing code was not doing that.

Your changes do add a pre-multiplication step to the math in two places 
- when you mix the src alpha and the glyph alpha at the top of the 
macro, and again when you do the Multiply(dst, dstA, dst) step in the 
middle.  In that sense, the new math is correct.

However, it is not the optimal way to implement this.  In particular, 
the macro used here to load the data from the destination is the one 
that loads it into 4 ARGB non-premultiplied values.  If the destination 
is non-PRE, then your added multiply step is exactly what is needed, but 
it could be combined with the multiply that will happen later in the 
blending equation, so it is an added step rather than a fixed fraction 
in the existing MultMultAdd parameters.  Additionally, if the 
destination is already PRE, then the macro being used to load the dst 
pixel data there will have done a divide step to divide out the alpha 
from the destination, only to have you reverse that math with your new 
Multiply() step.  That's a lot of math to end up with a NOP.

The MUL8 you added for the srcA and glyph value is needed, that change 
is good.  Since it is common for glyph pixels to have zero alpha, you 
might want to test the glyph alpha for 0 and skip the pixel before you 
do the multiply, though.  This would add one more if, but it would be a 
common case.

The trickier part is to load the destination components without 
un-premultiplying them.  Unfortunately there is no "Load...ToArgbPre" 
macro to parallel the Load macro used in the function.  Perhaps there 
should be, but you'd still end up with an extra multiply step as I 
mentioned above because you can fold the premultiplication of the dst 
data into the MultMultAdd by carefully choosing the parameters you use 
in the math there.

The good news is that the way that the SRCOVER_MASKFILL uses the various 
type-specific macros works around this a bit and minimizes the number of 
multiplies that happen.  You could check out DEFINE_SRCOVER_MASKFILL and 
see how it works in the case where pMask is not null (pMask is an alpha 
mask with values very similar to the glyphAA data).  Modeling this code 
on that code would correct the math and minimize it as well...

			...jim

On 3/17/16 3:00 AM, Prahalad Kumar Narayanan wrote:
> Hello Everyone on Java2D Group
>
> Good day to you.
>
> Herewith, I 'm sharing the webrev for two identical Java2D Bugs.
>
> Bug ID : JDK-8015070
>
>         Title     : Antialiased text on translucent backgrounds gets
> bright artifacts
>
>         Link      : https://bugs.openjdk.java.net/browse/JDK-8015070
>
> Bug ID : JDK-8013564 ( currently closed as duplicate )
>
>         Title     : Font rendering anti-aliasing in translucent
> BufferedImages broken
>
>         Link      : https://bugs.openjdk.java.net/browse/JDK-8013564
>
> Webrev Link :
> http://cr.openjdk.java.net/~psadhukhan/prahlad/8015070/webrev.00/
>
> Quick Summary on Bugs :
>
> ````````````````````````````````````````````````
>
> 1.  Artifacts appear on Edges of text characters when anti-aliased text
> is drawn on Translucent background
>
> 2.  The issue is reproducible on all platforms - windows, linux and mac os.
>
> 3.  Besides, the issue is reproduced with the commonly used pixel
> format- 4ByteArgb.
>
> Root Cause & Solution :
>
> ````````````````````````````````````````````````
>
> 1.  The Macro: GlyphListAABlend4ByteArgb in File: LoopMacros.h uses the
> standard blending algorithm
>
>          dstColor = [ srcColor * glyphAlpha + dstColor * (1 -
> glyphAlpha) ] / dstAlpha
>
> 2.  The above equation works only when the srcColor and dstColor are Opaque.
>
> 3.  When srcColor and dstColor are Translucent, their alphaComponent
> will influence the visibility of the color, and visibility of the color
> below.
>
> 4.  The new set of calculations for blending Translucent source and
> destination colors is given as
>
>          resAlpha = 1 - ((1 - srcAlpha) * (1 - dstAlpha))
>
>          resColor = [ srcColor * srcAlpha + dstColor * dstAlpha * (1 -
> srcAlpha) ] / resAlpha
>
> 5.  Reference text for the equation:
> https://en.wikipedia.org/wiki/Alpha_compositing
>
> 6.  With the above modification to the blending logic, the artifacts do
> not appear and issues are fixed.
>
> Jtreg & Jprt Results :
>
> ````````````````````````````````````````````````
>
> 1.  A simple test-file: AADrawStringArtifact.java has been created to be
> a part of jtreg test cases.
>
>           The test file is well commented to explain - nature of
> artifact and how the test tries to identify them.
>
>           As required, the test case fails with Jdk 7, Jdk 8 and
> succeeds with Jdk 9-internal (built with changes for the bug fix)
>
> 2.  The changes to blending logic lies within
> java.desktop/src/share/native/...
>
>           Henceforth, JPRT was used to ensure successful build across
> all supported platforms
>
>           Jprt Job Link :
> http://scaaa637.us.oracle.com//archive/2016/03/2016-03-17-072001.prahnara-linux.client
>
>           The build succeeds on all platforms.
>
> Kindly review the webrev link and provide your views and suggestions.
>
> Webrev Link :
> http://cr.openjdk.java.net/~psadhukhan/prahlad/8015070/webrev.00/
>
> If the changes look good, we could take up the changes for source checkin.
>
> Thank you for your time in review
>
> Have a good day
>
> Prahalad N.
>