[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-2d-dev
Subject: Re: [OpenJDK 2D-Dev] [9] Review request for 8023794: [macosx] LCD Rendering hints seems not working
From: Phil Race <philip.race () oracle ! com>
Date: 2015-05-27 16:31:24
Message-ID: 5565F15C.1080705 () oracle ! com
[Download RAW message or body]
OK .. it's a step forward, so approved, although there is more to be done.
I do ask that you however restore this comment until you actually remove the \
constraint : 405 * - and the destination is opaque
-phil.
On 05/21/2015 07:23 AM, Andrew Brygin wrote:
> Hello Phil,
>
> I have changed the default reverse gamma to 180:
>
> http://cr.openjdk.java.net/~bae/8023794/9/webrev.10/
>
> I also did some experiments with the lcd contrast, and found that
> value 160 decreases the discrepancy a bit: 21 instead of 29 with
> the default lcd contrast value (140).
> However, there still is the discrepancy, and at the moment I do not
> see how we can avoid it completely with our rendering model.
> It looks like that there is something more sophisticated than
> just gamma correction, and we are unable to derive 'color independent'
> glyphs from black-on-white glyphs provided by the native system.
>
> I have also played with changing display profiles but it seems to
> have no detectable effect.
>
> Thanks,
> Andrew
>
> On 5/18/2015 11:19 PM, Phil Race wrote:
> > Hi,
> >
> > So 1.79 is essentially 1.8 which is what Mac historically used as
> > gamma. So I'd pick 180 instead of 179
> > Since that value minimises the discrepancy it's looking positive but
> > there's still a discrepancy.
> > Before we 'live with it', I'd be interested to know what happens if
> > you set your display's profile to a generic RGB one.
> >
> > How do the glyph shapes (if you try your best to ignore the colour)
> > match what other apps produce ?
> >
> > -phil.
> >
> >
> > On 04/30/2015 06:21 AM, Andrew Brygin wrote:
> > > Hello Phil,
> > >
> > > please take a look to updated version of the fix:
> > >
> > > http://cr.openjdk.java.net/~bae/8023794/9/webrev.09/
> > >
> > > The main difference is in glyph generation. I have implemented
> > > 'reverse gamma correction'
> > > approach instead of 'glyph blending'. By default we use gamma value
> > > which provides minimum
> > > of discrepancy with gyph images produced by Apple's jdk. However,
> > > it can be adjusted via
> > > environment variable J2D_LCD_REVERSE_GAMMA (CGGlyphImages.m, lines
> > > 198 - 220).
> > >
> > > Following chart illustrates dependency between the 'reverse gamma'
> > > and a difference
> > > in pixel values comparing to jdk6 from Apple:
> > > http://cr.openjdk.java.net/~bae/8023794/best_reverse_gamma.png
> > >
> > > Following set of images has been used for the comparison:
> > > http://cr.openjdk.java.net/~bae/8023794/images/
> > > An index of image corresponds to the value of reverse gamma used
> > > for image
> > > generation.
> > >
> > > Beside this, following minor changes were done:
> > > * RenderingHints.KEY_ANTIALIASING was removed form fontHints,
> > > because it has no direct relation to text rendering, and does
> > > not have
> > > any effect at the moment.
> > > * A comment regarding unevaluated rendering hints was added.
> > >
> > > Please take a look.
> > >
> > > Thanks,
> > > Andrew
> > >
> > > On 4/24/2015 7:33 PM, Andrew Brygin wrote:
> > > > 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
> > > > > > > > >
> > > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic