JDK-8209191 : [macOS] Distorted complex text rendering
  • Type: Bug
  • Component: javafx
  • Sub-Component: graphics
  • Affected Version: openjfx11
  • Priority: P2
  • Status: Resolved
  • Resolution: Fixed
  • OS: os_x
  • CPU: x86
  • Submitted: 2018-08-09
  • Updated: 2020-01-31
  • Resolved: 2018-08-14
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 8 Other
8u191Fixed openjfx11Fixed
Related Reports
Relates :  
JDK-8198354 - [macOS] Corrupt Thai characters displayed in word wrapped label
Relates :  
JDK-8209435 - [macOS] Text with bold font weight rendered as normal text when TTC font is used
Description
Complex text rendering is incorrect and distorted.

Steps to reproduce:
 - Compile and run the attached Ta.java test
Comments
Changeset: 15770e65250e Author: arajkumar Date: 2018-08-14 12:38 +0530 URL: http://hg.openjdk.java.net/openjfx/11-dev/rt/rev/15770e65250e 8209191: [macOS] Distorted complex text rendering Reviewed-by: kcr, prr
14-08-2018
[~prr] >>I don't know why CT now doesn't want to return a direct pointer. Some changes in memory management policy ? >>Some change in behaviour when we don't make the call on the AppThread ? Whatever the cause, this fix is needed. I'm not quite sure about why it is suddenly appearing after JDK compiler upgrade. But if I call the same code[1] from some random place in WebView native code, I'm not getting null. I guess it is all about timing and memory layout. From some blog post[2] I see the following explanation, CTRun has one of those annoying Get...Ptr constructs that are common in Core frameworks. CTRunGetPositionsPtr() will very quickly return you the internal pointer to the glyphs locations. But it might fail if the CTRun hasn���t calculating them yet. If that happens, then you have to call CTRunGetPositions() and hand it a buffer to copy into. To handle this, I keep around a buffer that I realloc() to the largest size I need. This almost never comes up because CTRunGetPositionsPtr() almost always returns a result. I see same kind of fix had gone into few opensource projects like harfbuzz and iterm2 iterm2: https://gitlab.com/gnachman/iterm2/commit/4cc6411fe9e99978d98b1123ce8c56ee8ccf7369 harfbuzz: https://github.com/harfbuzz/harfbuzz/blob/master/src/hb-coretext.cc#L1180 [1] https://bugs.openjdk.java.net/browse/JDK-8209191? focusedCommentId=14203625&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14203625 [2] http://robnapier.net/laying-out-text-with-coretext
14-08-2018
This is approved to push to 8u-dev for 8u202. Presuming that the backport applies cleanly (adjusting for the path to the file, there is no need for a new webrev.
13-08-2018
Fix request approved for openjfx 11.
13-08-2018
>JDK-8198354 takes care of NULL return from CTRunGetGlyphsPtr, the similar treatment has to be given for CTRunGetPositionsPtr[1] as well. > (CTRunGetStringIndicesPtr already got that treatment). CTRunGetStringIndicesPtr got that treatment under this bug fix. https://bugs.openjdk.java.net/browse/JDK-8130721 So we have a pattern here that all these CT calls ending in "Ptr" may return NULL and we have had to fix them all one at a time. I've looked and I don't see any more waiting to happen .. so I think we are done fixing this pattern. I don't know why CT now doesn't want to return a direct pointer. Some changes in memory management policy ? Some change in behaviour when we don't make the call on the AppThread ? Whatever the cause, this fix is needed. +1 -phil.
13-08-2018
openjfx11-fix-request: This is a hidden issue appeared after JDK-8205424 and it should be fixed in openjfx11 to avoid complex text distortion.
13-08-2018
The fix looks good to me. I guess we don't know for sure why the compiler change on the JDK side exposed this bug? Either way, I think this is a good and safe fix. +1 This needs a second reviewer (Phil) and also an 'openjfx11-fix-request'.
13-08-2018
[~kcr], Sure, that makes sense. Please find the new webrev, http://cr.openjdk.java.net/~arajkumar/8209191/webrev.01
13-08-2018
I think this should be handled as two separate bugs. The coretext.c bug is critical for 11 and also needs to be backported to 8u, even though the bug doesn't happen to show up right now. It seems like an accident waiting to happen that could show up, for example, if the Xcode used to build the JDK is updated for a future JDK 8u release. The issue in PrismFontFactory affecting bold fonts (regression caused by JDK-8139838) may not be critical for 11 (we could decide to fix it in 12), and in any case doesn't need to be backported to 8u. Can you file a new bug for this one?
13-08-2018
There is an extra space before "return Arrays.stream"
13-08-2018
Root cause Analysis: The other issue which I reported(in the earlier) comment is a regression of JDK-8139838 and reproducible from jdk-9+120. JDK-8139838 is not back-ported to 8u-dev, so it is not reproducible in 8u-dev. Problem happens when trying to rendering a complex text with "bold" font weight. For example the attached test(marquee-ta.html), renders a complex text(tamil: ���������������) with "bold" weight. In order to render that text, the fallback font created by Prism is "Tamil Sangam MN", which is a TTC type has both "normal" and "bold" ttfs. But the PrismFontFile.createFontResource doesn't load all the collection inside TTC which leads to creation of incorrect fallback font(regular "Tamil Sangam MN" is associated as a fallback instead "Tamil Sangam MN Bold"). When each time rendering a text(Glyph layout happens), the above step is repeated because there is no font is associated to render the complex text with bold weight, which leads to font slot to exhaust. I get the following error with -Dprism.debugfonts=true while loading marquee-ta.html in HelloWebView. or load http://tamil.thehindu.com ( ..truncated .. ) Fallback font= Tamil Sangam MN Bold slot=124 Fallback font= Tamil Sangam MN Bold slot=125 Too many font fallbacks! Fallback font= Tamil Sangam MN Bold slot=-1 Too many font fallbacks! ( ..truncated .. ) Proposed solution: PrismFontFile.PrismFontFile(String name, String filename) must load all fonts and find the matching font for fallback. webrev: http://cr.openjdk.java.net/~arajkumar/8209191/webrev Testing: Working on a unit test. [~kcr], [~prr], Since there are 2 problems associated with complex text rendering, should I address it in 2 bugs?
13-08-2018
diff -r 4eda4402380f -r dc5a8d770e82 modules/javafx.graphics/src/main/native-font/coretext.c --- a/modules/javafx.graphics/src/main/native-font/coretext.c Fri Aug 10 15:55:29 2018 +0530 +++ b/modules/javafx.graphics/src/main/native-font/coretext.c Sun Aug 12 16:26:20 2018 +0530 @@ -673,11 +673,26 @@ { CTRunRef run = (CTRunRef)runRef; const CGPoint* positions = CTRunGetPositionsPtr(run); + CFIndex count = CTRunGetGlyphCount(run); + if (count == 0) { + return 0; + } + + CGPoint* tempPositions = NULL; + if (!positions) { + tempPositions = (CGPoint*) malloc(count * sizeof(CGPoint)); + if (!tempPositions) { + return 0; + } + + CTRunGetPositions(run, CFRangeMake(0, 0), tempPositions); + positions = tempPositions; + } + int j = 0; if (positions) { jfloat* buffer = (*env)->GetPrimitiveArrayCritical(env, bufferRef, NULL); if (buffer) { - CFIndex count = CTRunGetGlyphCount(run); int i = 0; while (i < count) { CGPoint pos = positions[i++]; @@ -687,6 +702,10 @@ (*env)->ReleasePrimitiveArrayCritical(env, bufferRef, buffer, 0); } } + + if (tempPositions) { + free(tempPositions); + } return j; } I could match the behaviour of 9 & 10 with above posted fix, however still there is a problem in complex text rendering. It is regression reproducible from 9., however it works perfectly on 8-dev.
13-08-2018
JDK-8198354 takes care of NULL return from CTRunGetGlyphsPtr, the similar treatment has to be given for CTRunGetPositionsPtr[1] as well. (CTRunGetStringIndicesPtr already got that treatment). [1] https://developer.apple.com/documentation/coretext/1510044-ctrungetpositionsptr?language=objc
12-08-2018
According to the CoreText documentation[1], the above mentioned scenario seems to be valid. It is not necessary that CTRunGetGlyphsPtr has to always return valid ptr. [1] https://developer.apple.com/documentation/coretext/1509952-ctrungetglyphsptr?language=objc
11-08-2018
Debugged little bit, I see a difference in CTRunGetGlyphPtr(coretext.c) return value. It returns a valid CGGlyph pointer in a working scenario(tested in jdk8), but NULL on openjfx11 latest. To ensure the same, I just isolated few function call which does the same operation, ///// test code //// #include <TargetConditionals.h> #import <CoreFoundation/CoreFoundation.h> #import <ApplicationServices/ApplicationServices.h> static void taTest(float size) { // Prepare font CTFontRef font = CTFontCreateWithName(CFSTR("TamilSangamMN"), size, NULL); // Create an attributed string CFStringRef keys[] = { kCTFontAttributeName }; CFTypeRef values[] = { font }; CFDictionaryRef attr = CFDictionaryCreate(NULL, (const void **)&keys, (const void **)&values, sizeof(keys) / sizeof(keys[0]), &kCFTypeDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks); CFAttributedStringRef attrString = CFAttributedStringCreate(NULL, CFSTR("he"), attr); CFRelease(attr); // Draw the string CTLineRef line = CTLineCreateWithAttributedString(attrString); CFArrayRef arrayRef = CTLineGetGlyphRuns(line); CTRunRef id0 = CFArrayGetValueAtIndex(arrayRef, 0); const CGGlyph * glyphs = CTRunGetGlyphsPtr(id0); fprintf(stderr, "CTLineCreateWithAttributedString cfarrycount:%ld glyphs:%p\n", CFArrayGetCount(arrayRef), glyphs); } int main() { taTest(12); taTest(13); return 0; } /// clang ta.c -framework CoreText -framework CoreFoundation && ./a.out ///// test code end //// The above test prints valid glyph when run in isolation(standalone test program), but prints null when the same called from coretext.c(called from OS_NATIVE(CTFontCreateWithName) JNI function impl).
11-08-2018
Probably unrelated, since the update to Unicode 10 (JDK-8191410) went into jdk-11+13 and the update to the locale data (JDK-8202537) went into jdk-11+19, both of which work (i.e., don't exhibit this problem). And if I compile the jdk-11+19 sources with Xcode 9.x it fails.
11-08-2018
JDK also upgraded ICU to 60.2, I'm not sure whether it had any side effects.
11-08-2018
Raising to P2. This is a critical regression that makes complex text unusable in many cases.
10-08-2018
This bug wasn't caused by a change in FX. Taking the latest FX bits from jfx-dev and running it with an OpenJDK 10 build works. So does jdk-11+19. If you run the test program with jdk-11+20 or later it will fail. The one big change in jdk-11+20 was the compiler upgrade on Mac. As further proof that this was caused by the JDK compiler upgrade, I created a local JDK 11 build using the jdk-11+19 sources, using an Xcode 9.x compiler on my system (which is what I also use to build FX). That build of the JDK also fails. So either there is a bug in the JDK that we are tripping over, or the compiler upgrade has exposed a latent bug in FX. Note that FX has been using Xcode 9.x for more than a year. It was the compiler upgrade for the JDK itself that caused (or exposed) this bug.
10-08-2018
I ran it with the latest 8u-dev and it looks fine. So it probably isn't as simple as my guess above.
10-08-2018
I can reproduce it on Mac. It looks fine on Windows. This is a regression introduced in FX 11. It renders with correct spacing on JDK 10, but some of the characters are rendered as a blob. It is completely wrong using OpenJFX 11. It might be caused by the fix for JDK-8198354 which went into 11 and was backported to 8u192.
10-08-2018
I'm not seeing similar problem in Windows. [~kcr], [~mbilla], If you find some time, Could you please run the attached test(Ta.java) to ensure it is not a setup issue?
10-08-2018
It is even reproducible with simple JavaFX test case. (Refer Ta.java)
09-08-2018
Attached a simplified test case (ta.html).
09-08-2018
Seems to be reproducible only on openjfx11, latest 8u-dev is rendering properly. Please refer the attached screenshots. (Note:Though 8u191.png shows 8u151 in the title, it uses latest 8u-dev under the hood.)
09-08-2018