Hello Phil,
please see my comments inline.
On 4/23/2015 11:15 PM, Phil Race wrote:
373 fontHints.put(RenderingHints.KEY_ANTIALIASING,
RenderingHints.VALUE_ANTIALIAS_ON);
Why do we need this ? Historically in the (non-OSX) code this would
slow things down by making
text rendering go via ductus rendering.
If this really is a 'fontsHint' map, it seems it should not be here.
I agree that this particular hint has no direct relation to the font
hints,
and probably should not be here. I guess that KEY_ANTALIASING was put
here in order to force text antialiasing on when default text
antailiasing is evaluating:
http://hg.openjdk.java.net/jdk9/client/jdk/file/0e483e64c1e4/src/java.desktop/share/classes/sun/java2d/SurfaceData.java#l741
I do not think that it has any effect now, because we set text
antialiasing hint explicitly.
I will check whether it can be safely removed.
410 sg2d.surfaceData.getTransparency() == Transparency.OPAQUE &&
I thought you were removing this condition ?
I have rolled this change back, because at the moment lcd shader
produces ugly results in the case of translucent destination.
There is a separate bug regarding the translucent destination support:
https://bugs.openjdk.java.net/browse/JDK-8078516
I am going to relax this condition when(if) our lcd shader will be
ready.
In fact, this problem is not limited by ogl, because d3d and software
loops
has the same limitation.
CGGlyphImages.m
202 #if __LITTLE_ENDIAN__
203 *(dst + 2) = 0xFF - (p >> 24 & 0xFF);
204 *(dst + 1) = 0xFF - (p >> 16 & 0xFF);
205 *(dst) = 0xFF - (p >> 8 & 0xFF);
206 #else
207 *(dst) = 0xFF - (p >> 16 & 0xFF);
208 *(dst + 1) = 0xFF - (p >> 8 & 0xFF);
209 *(dst + 2) = 0xFF - (p & 0xFF);
210 #endif
211 }
became
217 {
218 *(dst + 0) = 0xFF - (p >> 16 & 0xFF); // red
219 *(dst + 1) = 0xFF - (p >> 8 & 0xFF); // green
220 *(dst + 2) = 0xFF - (p & 0xFF); // blue
221 }
I started by assuming you were removing the BIG_ENDIAN code that
was probably legacy PPC code but instead I see that the
LITTLE_ENDIAN case is removed,
so I don't understand this.
And further down the file now I see that in ConvertBWPixelToByteGray
you did remove the big endian case.
Note that we are
Please note that we force the lcd glyph generation by requesting
host (i.e. LITTLE_ENDIAN) byte order for the canvas bitmap:
407 uint32_t bmpInfo = kCGImageAlphaPremultipliedFirst;
408 if (mode->glyphDescriptor == &rgb) {
409 bmpInfo |= kCGBitmapByteOrder32Host;
410 }
So, before the fix (and for grayscale case now) the buffer has default
BIG_ENDIAN order. I.e. grayscale canvas stores data in following format:
as bytes: A R G B
as int: 0xBBGGRRAA
The same byte order was used for the rgb canvas before the fix.
But now the canvas is configured to store data in following format:
as bytes: B G R A
as int: 0xAARRGGBB
So, data extraction routines were updated accordingly.
365 case sun_awt_SunHints_INTVAL_TEXT_ANTIALIAS_GASP:
366 case sun_awt_SunHints_INTVAL_TEXT_ANTIALIAS_DEFAULT:
367 default:
368 e = [NSException
369 exceptionWithName:@"IllegalArgumentException"
370 reason:@"Invalid hint value"
371 userInfo:nil];
I think this means that by the time we get here we should not have
DEFAULT or GASP
but we should have the actual interpretation for this surface, font
and point size.
So this looks correct but maybe you can add a comment on this.
will do.
The heuristic of blending black and white is interesting.
How did you arrive at this ? It suggests that the glyph bitmap we
are caching is
not 'colour neutral' which is odd. Maybe we are missing code to apply
the 'reverse gamma correction' ?
I have played with the idea about 'reverse gamma correction', it
seemed very attractive to me.
Roughly speaking, gamma > 1 makes black-on-white glyphs a bit
narrower, and white-no-black
glyphs a bit wider (bolder?), and it is the same effect that we need.
Unfortunately, direct comparison of black-on-white and
white-on-black glyphs makes me think
that difference between these glyphs can not be explained only by
gamma correction.
Please take a look at this screenshot:
http://cr.openjdk.java.net/~bae/8023794/bw-wb-comparision.png
<http://cr.openjdk.java.net/%7Ebae/8023794/bw-wb-comparision.png>
row 1: text is rendered by jdk6 as-is.
row 2: simulates our pipeline where black-on-white glyphs are used
(max error on white-on-black)
row 3: simulates our pipeline where white-on-black glyphs are used
(max error on black-on-white)
row 4: blended glyphs are used.
The basic idea of blending is to minimize max error (difference)
between produced glyph image
and original color-aware glyphs.
If better results can be achieved with 'reverse gamma correction', we
can revise this code later.
And can (should) any of these changes also be applied to D3D ?
1. We can check how gamma correction is done in d3d. If a lookup is
also used there,
we can assess how rough the interpolation is.
2. translucent destination support (as a part of JDK-8078516)?
Thanks,
Andrew
-phil.
On 03/27/2015 07:50 AM, Andrew Brygin wrote:
There is a minor update in the fix: it does not worth to blend
black-on-white
and white-on-black glyphs for the case of AA glyphs, because it
makes no difference.
CGGlyphImages.m, lines 294 - 325, 755 - 763, and 787 - 794:
http://cr.openjdk.java.net/~bae/8023794/9/webrev.08
I have also modified the AntialiasDemo a bit in order to render the
sample text in different colors, and in order to render into a
volatile
image as well:
http://cr.openjdk.java.net/~bae/8023794/demo/AntialiasDemo.java
It could be used to assess the change in glyph generation.
Thanks,
Andrew
On 3/26/2015 3:59 PM, Andrew Brygin wrote:
Hi Chris,
thank you for the comments and explanation. I have updated the
OGLContext_IsLCDShaderSupportAvailable()
and comments in OGLTextRenderer.c accordingly:
http://cr.openjdk.java.net/~bae/8023794/9/webrev.07/
Good to know that you keep an eye on the OGL pipeline :)
Thanks,
Andrew
On 3/25/2015 8:24 PM, Chris Campbell wrote:
Hi Andrew,
That's a good find re: pow(). In the comment at lines 277-283 I
mentioned that there was only a scalar variant of pow(). I
wonder if that was a limitation in some early version of GLSL I
was using or perhaps some driver bug/restriction. According to
all the docs I can find the vector variants have been there all
along:
https://www.opengl.org/sdk/docs/man4/index.php
So now I'm wondering if the slowness was actually due to me
trying 3 scalar pow() calls versus one vector pow() call.
Oh, aha! I think this explains part of it:
269 * This is the GLSL fragment shader source code for
rendering LCD-optimized
270 * text. Do not be frightened; it is much easier to
understand than the
271 * equivalent ASM-like fragment program!
Looks like I wrote the original version of this shader using the
GL_ARB_fragment_program language, which indeed only had a scalar
POW instruction (see section 3.11.5.20):
https://www.opengl.org/registry/specs/ARB/fragment_program.txt
And then I'm guessing that when I rewrote it in more modern GLSL,
I didn't notice that pow() supported vectors. Sigh. That 3D
texture LUT was a lot of work for a whole lot of nothing.
In any case, you might want to rewrite the comment at lines
277-283 to suit the new code. Same goes for the comment at line
315:
// gamma adjust the dest color using the invgamma LUT
Also, I noticed that the restrictions in
OGLContext_IsLCDShaderSupportAvailable() could be loosened since
you only need 2 texture units now. Just in the extremely
unlikely case that anyone's running this stuff on ancient
hardware :)
Thanks,
Chris
Andrew Brygin wrote:
Hello Phil,
could you please take a look to updated version of the fix?
http://cr.openjdk.java.net/~bae/8023794/9/webrev.06/
* OGLTextRenderer.c
It was discovered, that in some cases lcd glyphs had visible 'dark
border' around.
This border appeared due to the gamma correction in lcd shader,
which
uses lookup
on 3D texture instead of using 'pow' routine. The texture is
populated
with significant
granularity (16 points per edge), what is a root cause of rough
interpolation of color values.
Suggested change is to eliminate the interpolation and use 'pow'
routine
directly.
It gives more accurate color values, and does not introduce
significant
performance hit
(see benchmark summary below).
However, this part of the fix affects not only macosx, but any
platform
where OGL text
shader can be used, so it probably worth to split into a
separate fix.
Summary:
lcd-ogl-3Dlookup:
Number of tests: 4
Overall average: 42.68027553311743
Best spread: 3.49% variance
Worst spread: 29.59% variance
(Basis for results comparison)
lcd-ogl-pow:
Number of tests: 4
Overall average: 42.468941501367084
Best spread: 2.51% variance
Worst spread: 29.44% variance
Comparison to basis:
Best result: 103.28% of basis
Worst result: 97.36% of basis
Number of wins: 1
Number of ties: 2
Number of losses: 1
Thanks,
Andrew
