[OpenJDK 2D-Dev] [9] Review request for 8023794: [macosx] LCD Rendering hints seems not working without FRACTIONALMETRICS=ON
Denis S. Fokin
denis.fokin at gmail.com
Wed Nov 5 13:57:44 UTC 2014
Hi Andrew,
I have noticed an issue with quartz (at list with our way to use it).
This is a simple project that allows to illustrate the issue with a pure Quartz and Cocoa.
https://github.com/denis-fokin/OffscreenVsQuartzFontRendering <https://github.com/denis-fokin/OffscreenVsQuartzFontRendering>
Below you can see results.
I used the same MBP with an external Thunderbolt display.
Rendering of a glyph in the view and in the offscreen bitmap is the same for non-retina display.
http://imgur.com/Y33C0tF,vnn5Ajt#0 <http://imgur.com/Y33C0tF,vnn5Ajt#0>
In case of retina, it seems that the image uses improper number of pixels.
http://imgur.com/Y33C0tF,vnn5Ajt#1 <http://imgur.com/Y33C0tF,vnn5Ajt#1>
I see the same effect in the java-2d sub pixel rendering implementation.
Right now I am not sure, how to handle this properly.
Thank you,
Denis.
> On 24 Oct 2014, at 21:56, Andrew Brygin <andrew.brygin at oracle.com> wrote:
>
> Hello,
>
> please take a look to updated version of the fix:
>
> http://cr.openjdk.java.net/~bae/8023794/9/webrev.03/ <http://cr.openjdk.java.net/~bae/8023794/9/webrev.03/>
>
> TODOs were removed:
> * CGLSurfaceData.java
> the condition for lcd rendering is inherited from OGL surface data, but
> here we have to remove the check for the transparency, because we always
> create transparent volatile images for swing backbuffers on macosx in order
> to support shaped windows.
>
> * CGGlyphImages.m and AWTStrike.m
> NSException is used to handle invalid (unevaluated) text antialising hint values.
> We actually have already used NSException to handle memory allocation failure,
> so this change just makes usage of get/release of primitive arrays a bit more safe.
>
>
> Known differences comparing to apple jdk6:
>
> a) we do not interpret TEXT_ANTIALIASING_ON as lcd text.
>
> b) we do not render lcd text in a case of non-opaque paint. This behavior is common for
> all cases (software loops, OGL, and D3D), so it seems to deserve a separate bug if we
> want to handle this case.
>
> Thanks,
> Andrew
>
> On 10/14/2014 3:39 AM, Phil Race wrote:
>> On 10/13/2014 05:41 AM, Andrew Brygin wrote:
>>> Hello Phil,
>>>
>>> please see my comments inline.
>>>
>>> On 10/13/2014 12:05 AM, Phil Race wrote:
>>>> 373 fontHints.put(RenderingHints.KEY_ANTIALIASING, RenderingHints.VALUE_ANTIALIAS_ON);
>>>> 374 fontHints.put(RenderingHints.KEY_TEXT_ANTIALIASING, RenderingHints.VALUE_TEXT_ANTIALIAS_LCD_HBGR);
>>>>
>>>>
>>>> I am not sure why we have the graphics AA hint "ON" set here.
>>>> Its not truly a font hint even though if its set it implcitly triggers font AA to be on too.
>>>> If you remove that first line do you notice any problems with Swing apps ?
>>> I failed to spot any difference in the SwingSet2 demo after removal the line 373.
>>
>> Hmm .. Sergey seemed to suggest it was needed.
>> I was concerned that it might subvert the text rendering hint
>> as it used to cause us to do everything via the graphics AA loops.
>> So I guess I am not sure what the impact of removing it will be
>> You probably should check if it changes Metal & Nimbus rendering
>> in SwingSet as it looks to be read by those but I don't see anywhere
>> Aqua is reading these hints which is puzzling as you'd think that's
>> where they were wanted.
>>
>>>
>>>>
>>>>
>>>> The logic in the following code looks odd to me ..
>>>> 310 static CGGI_GlyphInfoDescriptor grey =
>>>> 311 { 1, &CGGI_CopyImageFromCanvasToAlphaInfo };
>>>> 312 static CGGI_GlyphInfoDescriptor rgb =
>>>> 313 { 3, &CGGI_CopyImageFromCanvasToRGBInfo };
>>>> 314
>>>> 315 static inline CGGI_RenderingMode
>>>> 316 CGGI_GetRenderingMode(const AWTStrike *strike)
>>>> 317 {
>>>> 318 CGGI_RenderingMode mode;
>>>> 319 mode.cgFontMode = strike->fStyle;
>>>> 320
>>>> 321 switch (strike->fAAStyle) {
>>>>
>>>> 322 case sun_awt_SunHints_INTVAL_TEXT_ANTIALIAS_OFF:
>>>> 323 case sun_awt_SunHints_INTVAL_TEXT_ANTIALIAS_ON:
>>>>
>>>>
>>>> 324 mode.glyphDescriptor = &grey;
>>>> 325 break;
>>>> 326 case sun_awt_SunHints_INTVAL_TEXT_ANTIALIAS_LCD_HRGB:
>>>> 327 case sun_awt_SunHints_INTVAL_TEXT_ANTIALIAS_LCD_HBGR:
>>>> 328 case sun_awt_SunHints_INTVAL_TEXT_ANTIALIAS_LCD_VRGB:
>>>> 329 case sun_awt_SunHints_INTVAL_TEXT_ANTIALIAS_LCD_VBGR:
>>>> 330 mode.glyphDescriptor = &rgb;
>>>> 331 break;
>>>> 332 case sun_awt_SunHints_INTVAL_TEXT_ANTIALIAS_GASP:
>>>> 333 case sun_awt_SunHints_INTVAL_TEXT_ANTIALIAS_DEFAULT:
>>>> 334 default:
>>>> 335 // TODO: generate an error?
>>>> 336 break;
>>>> 337 }
>>>>
>>>>
>>>> .. If we get to default does this mean aliased rendering ?
>>>> If so why would "OFF" go the grey path - seems it should go default
>>>> And "GASP" looks like there is no support to specify the intended meaning there
>>>> Given that OS X text is unhinted I'd think "ON" is a better option here
>>>> even though most fonts would do "OFF" at typical screen sizes .. but that's
>>>> because they are expecting hinted rendering. So I think you need to get
>>>> hinted glyphs to default this to aliased ..
>>>
>>> If my understanding is correct, we never get TEXT_ANTIALIAS_GASP and
>>> TEXT_ANTIALIAS_DEFAULT here, because these hints must be already evaluated
>>> and translated to either TEXT_ANTIALIAS_OFF, or TEXT_ANTIALIAS_ON.
>>> It should be done in SunGraphisc2D.checkFontInfo(), lines 699 - 775.
>>
>> this is probably right so its OK ..
>>
>> -phil.
>>
>>>
>>> At this point we have no enough info to evaluate these hints properly,
>>> and a best solution is probably to raise an error.
>>>
>>> Thanks,
>>> Andrew
>>>
>>>>
>>>> -phil.
>>>>
>>>>
>>>> On 10/10/14 2:13 PM, Andrew Brygin wrote:
>>>>> Hello Phil,
>>>>>
>>>>> please see my comments inline.
>>>>>
>>>>> On 10/11/2014 12:49 AM, Phil Race wrote:
>>>>>> On 10/10/14 11:05 AM, Andrew Brygin wrote:
>>>>>>> Hello Phil,
>>>>>>>
>>>>>>> changes in SunGraphics2D and SurfaceData were made in order to
>>>>>>> implement an approach 'LCD instead of greyscale AA if possible'.
>>>>>>>
>>>>>>> Without this part of change, we get results according to the hints:
>>>>>>> greyscale for TEXT_ANTIALIAS_ON, and subpixels for lcd hints:
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~bae/8023794/9/webrev.01/ <http://cr.openjdk.java.net/%7Ebae/8023794/9/webrev.01/>
>>>>>>
>>>>>> So you are changing the meaning of application-specified "ON" across platforms ?
>>>>>> That sounds wrong - even for Mac ...
>>>>>
>>>>> now I do not: the fix revision 01 does not change the meaning of the hints,
>>>>> and just makes text rendering on macosx to behave in a similar manner as
>>>>> on windows.
>>>>>
>>>>> Please take a look at the webrev.01.
>>>>>
>>>>> Thanks,
>>>>> Andrew
>>>>>>
>>>>>>>
>>>>>>> I am not sure whether do we need to change the 'default'
>>>>>>> behavior: at the moment it produces aliased text.
>>>>>>
>>>>>> default has meant aliased in all the non-mac sun/oracle implementations
>>>>>> since JDK 1.2. We have talked about changing it to something
>>>>>> more modern but should not do it lightly ..
>>>>>>
>>>>>> -phil.
>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Andrew
>>>>>>>
>>>>>>> On 10/10/2014 8:54 PM, Phil Race wrote:
>>>>>>>> I don't have my head around all these changes but a lot of it seems to
>>>>>>>> imply we really weren't asking for LCDon Mac when we could/should.
>>>>>>>>
>>>>>>>> The change in the shared SurfaceData.javais something I want to
>>>>>>>> ask about as you have commented out as follows ..
>>>>>>>> ;
>>>>>>>> 749
>>>>>>>> 750 // TODO: we have to take into account aaHint on macosx
>>>>>>>> 751 //case SunHints.INTVAL_TEXT_ANTIALIAS_ON:
>>>>>>>> 752 // return aaTextRenderer;
>>>>>>>>
>>>>>>>> ...
>>>>>>>>
>>>>>>>> that looks like the case where the application code has *explicitly*
>>>>>>>> specified greyscale (ON==greyscale) so I don't see why you need
>>>>>>>> to go check the fontinfo in this case ?
>>>>>>>>
>>>>>>>> -phil.
>>>>>>>>
>>>>>>>> On 10/10/2014 07:34 AM, Andrew Brygin wrote:
>>>>>>>>> Hello Denis,
>>>>>>>>>
>>>>>>>>> could you please take a look at a preliminary version of the fix?
>>>>>>>>>
>>>>>>>>> http://cr.openjdk.java.net/~bae/8023794/9/webrev.00/ <http://cr.openjdk.java.net/%7Ebae/8023794/9/webrev.00/>
>>>>>>>>>
>>>>>>>>> This fix promotes the text antialiasing from grayscale to LCD if
>>>>>>>>> destination surface data is able to render LCD, and provides
>>>>>>>>> selection of an appropriate text pipeline for both cases.
>>>>>>>>> It also separates production of gatyscale and LCD glyph images.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Andrew
>>>>>>>>>
>>>>>>>>> On 10/9/2014 4:13 PM, Denis Fokin wrote:
>>>>>>>>>> Hi Andrew,
>>>>>>>>>>
>>>>>>>>>> I am happy about you participation in this work!
>>>>>>>>>>
>>>>>>>>>> Looks like, I have missed this letter, while being sick. Sorry about this.
>>>>>>>>>>
>>>>>>>>>> I signed OCA, but I have not gotten access to cr.openjdk.java.net <http://cr.openjdk.java.net/> yet. This is the reason why I have not updated the webrev.
>>>>>>>>>>
>>>>>>>>>> I think that an API that is consistent with other platforms is a great solution. It just requires more efforts and multi-platform testing. On the other hand, a property could be a safe start.
>>>>>>>>>>
>>>>>>>>>> As for the offscreen rendering, I have some kind of a workaround with the next approach.
>>>>>>>>>>
>>>>>>>>>> In BufImgSurfaceData.getRenderLoops() I always return super.getRenderLoops(sg2d), and never solid loops.
>>>>>>>>>>
>>>>>>>>>> In case if “useQuartz" flag is specified, I return only lcdTextRenderer from SurfaceData.getTextPipe()
>>>>>>>>>>
>>>>>>>>>> Of course it is a brute force approach, but it allows producing a legible text in case of offscreen rendering.
>>>>>>>>>>
>>>>>>>>>> Thank you,
>>>>>>>>>> Denis.
>>>>>>>>>>
>>>>>>>>>> On 29 Sep 2014, at 19:30, Andrew Brygin <andrew.brygin at oracle.com <mailto:andrew.brygin at oracle.com>> wrote:
>>>>>>>>>>
>>>>>>>>>>> Hello Denis,
>>>>>>>>>>>
>>>>>>>>>>> I am not sure whether we should use 'apple.awt.graphics.UseQuartz' property.
>>>>>>>>>>> Probably we have to change the text antialiasing defaults for macosx instead.
>>>>>>>>>>>
>>>>>>>>>>> I am working on the issue with software loops. I will update the thread
>>>>>>>>>>> with my findings.
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>> Andrew
>>>>>>>>>>>
>>>>>>>>>>> On 9/3/2014 3:32 PM, Denis Fokin wrote:
>>>>>>>>>>>> Hi Sergey and 2d team,
>>>>>>>>>>>>
>>>>>>>>>>>> I have rewritten the fix. It works fine for text rendered on window using OpenGL.
>>>>>>>>>>>>
>>>>>>>>>>>> http://web-dot.ru/openjdk/8023794/webrev.00/index.html <http://web-dot.ru/openjdk/8023794/webrev.00/index.html>
>>>>>>>>>>>>
>>>>>>>>>>>> It is incomplete though. It does not work for rendering in a buffered image.
>>>>>>>>>>>>
>>>>>>>>>>>> Additionally, I have not tested the code on other platforms except MacOS X.
>>>>>>>>>>>>
>>>>>>>>>>>> To enable the antialiasing you should pass
>>>>>>>>>>>>
>>>>>>>>>>>> -Dapple.awt.graphics.UseQuartz=true
>>>>>>>>>>>>
>>>>>>>>>>>> to java.
>>>>>>>>>>>>
>>>>>>>>>>>> The current issue now is the glyph info bytes that are passed from CGGlyphImage to AATextRenderer.
>>>>>>>>>>>>
>>>>>>>>>>>> To render data we use DEFINE_SOLID_DRAWGLYPHLIST* macros. Basing on the macros a set of functions is generated for the next loops.
>>>>>>>>>>>>
>>>>>>>>>>>> sun/java2d/loops/ByteGray.c
>>>>>>>>>>>> sun/java2d/loops/ByteIndexed.c
>>>>>>>>>>>> sun/java2d/loops/FourByteAbgr.c
>>>>>>>>>>>> sun/java2d/loops/FourByteAbgrPre.c
>>>>>>>>>>>> sun/java2d/loops/Index12Gray.c
>>>>>>>>>>>> sun/java2d/loops/Index8Gray.c
>>>>>>>>>>>> sun/java2d/loops/IntArgb.c
>>>>>>>>>>>> sun/java2d/loops/IntArgbBm.c
>>>>>>>>>>>> sun/java2d/loops/IntArgbPre.c
>>>>>>>>>>>> sun/java2d/loops/IntBgr.c
>>>>>>>>>>>> sun/java2d/loops/IntRgb.c
>>>>>>>>>>>> sun/java2d/loops/IntRgbx.c
>>>>>>>>>>>> sun/java2d/loops/LoopMacros.h
>>>>>>>>>>>> sun/java2d/loops/ThreeByteBgr.c
>>>>>>>>>>>> sun/java2d/loops/Ushort555Rgb.c
>>>>>>>>>>>> sun/java2d/loops/Ushort555Rgbx.c
>>>>>>>>>>>> sun/java2d/loops/Ushort565Rgb.c
>>>>>>>>>>>> sun/java2d/loops/UshortGray.c
>>>>>>>>>>>> sun/java2d/loops/UshortIndexed.c
>>>>>>>>>>>>
>>>>>>>>>>>> For instance, C preprocessor generates the next code for IntRgb.c
>>>>>>>>>>>>
>>>>>>>>>>>> void IntRgbDrawGlyphListLCD(/*…*/){
>>>>>>>>>>>> jint glyphCounter, bpp;
>>>>>>>>>>>> jint scan = pRasInfo->scanStride;
>>>>>>>>>>>> IntRgbDataType *pPix;
>>>>>>>>>>>> fprintf(__stderrp, "NAME_SOLID_DRAWGLYPHLISTLC\n");
>>>>>>>>>>>> jint srcA;
>>>>>>>>>>>> jint srcR , srcG, srcB;;;;
>>>>>>>>>>>> do {
>>>>>>>>>>>> (srcB) = (argbcolor) & 0xff;
>>>>>>>>>>>> (srcG) = ((argbcolor) >> 8) & 0xff;
>>>>>>>>>>>> (srcR) = ((argbcolor) >> 16) & 0xff;
>>>>>>>>>>>> (srcA) = ((argbcolor) >> 24) & 0xff;
>>>>>>>>>>>> } while (0);;
>>>>>>>>>>>> // and so on…
>>>>>>>>>>>>
>>>>>>>>>>>> Looks like rgb loop expects to see 4 8-bit color channels per pixel.
>>>>>>>>>>>>
>>>>>>>>>>>> For now, I do not understand which contract should be honoured to meet DEFINE_SOLID_DRAWGLYPHLIST* expectations, i.e. how should I place bytes in GlyphInfo.
>>>>>>>>>>>>
>>>>>>>>>>>> May be it should be set somewhere in Java code.
>>>>>>>>>>>>
>>>>>>>>>>>> Could anyone share this knowledge with me?
>>>>>>>>>>>>
>>>>>>>>>>>> Thank you,
>>>>>>>>>>>> Denis.
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On 09 Jul 2014, at 19:22, Sergey Bylokhov <Sergey.Bylokhov at oracle.com <mailto:Sergey.Bylokhov at oracle.com>> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>> Hello, Denis.
>>>>>>>>>>>>> Thanks for this research!
>>>>>>>>>>>>> On 09.07.2014 15:13, Denis Fokin wrote:
>>>>>>>>>>>>>> The current version consist of three parts.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> 1. We are rendering glyphs in offscreen images using Quartz functions. This does not work without kCGBitmapByteOrder32Host mask.
>>>>>>>>>>>>> I assume LCD hint does not work? this looks good.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> 2. We assume that subpixel antialiasing should not be used on a non-opaque surface. As I understand the vImage in CGLVolatileSurfaceManager is not related directly to our window. For a start, I have hardcoded Transparency.OPAQUE, but it requires much better understanding of the architecture to make a more proper solution.
>>>>>>>>>>>>> It is related to the CGLOffScreenSurfaceData, which is used as a surface for VolatileImages. I check this code and looks like we ignore type of the ColorModel and create a transparent native texture anyway.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> 3. When I started using CGGI_CopyImageFromCanvasToRGBInfo as a rendering mode, I had found that the little endian mode should be undefined. Again, it might be an improper way to do this.
>>>>>>>>>>>>> It seems __LITTLE_ENDIAN__usage in this file should be checked.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thank you,
>>>>>>>>>>>>>> Denis.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> diff -r f87c5be90e01 src/macosx/classes/sun/java2d/opengl/CGLVolatileSurfaceManager.java
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> --- a/src/macosx/classes/sun/java2d/opengl/CGLVolatileSurfaceManager.javaFri Jun 20 10:15:30 2014 -0700
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> +++ b/src/macosx/classes/sun/java2d/opengl/CGLVolatileSurfaceManager.javaWed Jul 09 14:50:09 2014 +0400
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> @@ -108,7 +108,7 @@
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> } else {
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> CGLGraphicsConfig gc =
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> (CGLGraphicsConfig)vImg.getGraphicsConfig();
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> - ColorModel cm = gc.getColorModel(vImg.getTransparency());
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> + ColorModel cm = gc.getColorModel(Transparency.OPAQUE);
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> int type = vImg.getForcedAccelSurfaceType();
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> // if acceleration type is forced (type != UNDEFINED) then
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> // use the forced type, otherwise choose one based on caps
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> diff -r f87c5be90e01 src/macosx/native/sun/font/CGGlyphImages.m
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> --- a/src/macosx/native/sun/font/CGGlyphImages.mFri Jun 20 10:15:30 2014 -0700
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> +++ b/src/macosx/native/sun/font/ .mWed Jul 09 14:50:09 2014 +0400
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> @@ -196,6 +196,8 @@
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> #pragma mark --- Font Rendering Mode Descriptors ---
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> +#undef __LITTLE_ENDIAN__
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> static inline void
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> CGGI_CopyARGBPixelToRGBPixel(const UInt32 p, UInt8 *dst)
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> {
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> @@ -366,7 +368,8 @@
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> canvas->context = CGBitmapContextCreate(canvas->image->data,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> width, height, 8, bytesPerRow,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> colorSpace,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> - kCGImageAlphaPremultipliedFirst);
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> + kCGImageAlphaPremultipliedFirst
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> + | kCGBitmapByteOrder32Host);
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> CGContextSetRGBFillColor(canvas->context, 0.0f, 0.0f, 0.0f, 1.0f);
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> CGContextSetFontSize(canvas->context, 1);
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> --
>>>>>>>>>>>>> Best regards, Sergey.
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/2d-dev/attachments/20141105/7a0d4600/attachment.html>
More information about the 2d-dev
mailing list