Hi Phil, Thank you for the comments. You are right the invocation of isAAT() method might be too expensive especially on OSX. I have changed the if-statement as you suggested. Please find the new webrev here: http://cr.openjdk.java.net/~dmarkov/8201801/jdk8u/webrev.02/
Also I will be happy to port the fix for JDK-8210384 to 8u once it is ready. Thanks, Dmitry > On 4 Sep 2018, at 20:34, Phil Race <[email protected]> wrote: > > I see you noticed the problem with it not necessarily being an AAT font. > It seems this method is a copy/paste of the method added in JDK 9. > So this will work but looking at it, I think in JDK 9 I should have made this > more efficient. > Probably it was a detail in the larger work, that I never came back to. > On MacOS where it calls through the PhysicalFont interface, since CFont is > not a TrueTypeFont, > it will be retrieving all the data in the table from native to Java on every > call to layout. That will be expensive. > > You can mitigate this in your case by reversing the order here > + if (isAAT(font) && (typo_flags & 0x80000000) != 0) { > > to > + if (((typo_flags & 0x80000000) != 0) && isAAT(font)) { > > So it will only take the hit for RTL text which will save a lot of cases ... > I have filed https://bugs.openjdk.java.net/browse/JDK-8210384 > <https://bugs.openjdk.java.net/browse/JDK-8210384> which I will fix for 12. > > Up to you whether you also want to wait for that fix, or just update as I > have suggested > and maybe take a follow-on fix later. > > Note that whilst your fix will at least make sure text reads in the correct > direction, it > means it will be using default built-in shaping rules of ICU and so may miss > at least > some features provided by the font. > > -phil. > > On 09/03/2018 05:54 AM, Dmitry Markov wrote: >> Hi Prasanta, >> >> Thank you for the feedback. I missed the usage of OSX specific class in >> shared code for some reasons. Please find the updated webrev here: >> http://cr.openjdk.java.net/~dmarkov/8201801/jdk8u/webrev.01/ >> <http://cr.openjdk.java.net/%7Edmarkov/8201801/jdk8u/webrev.01/> >> I replaced the usage of CFont with a new private method which tests either a >> font is AAT or not. >> >> The issue is jdk8u (ICU) specific. It does not take place on jdk12 because >> starting from jdk9 we use Harfbuzz as default layout engine. >> >> Thanks, >> Dmitry >> >>> On 3 Sep 2018, at 11:33, Prasanta Sadhukhan <[email protected] >>> <mailto:[email protected]>> wrote: >>> >>> Hi Dmitry, >>> Not going into technicalities of the fix, but it seems it will break >>> non-macos build as you are checking for CFont in a shared class? >>> Also, if it's the issue still exists, then why you are fixing only in 8u >>> and not in jdk12? >>> >>> Regards >>> Prasanta >>> On 9/3/2018 3:20 PM, Dmitry Markov wrote: >>>> Hello, >>>> >>>> Could you review a fix for jdk8u, please? >>>> >>>> bug: https://bugs.openjdk.java.net/browse/JDK-8201801 >>>> <https://bugs.openjdk.java.net/browse/JDK-8201801> >>>> webrev: http://cr.openjdk.java.net/~dmarkov/8201801/jdk8u/webrev.00/ >>>> <http://cr.openjdk.java.net/%7Edmarkov/8201801/jdk8u/webrev.00/> >>>> >>>> Problem description: >>>> The fix for 7162125 [1] enabled font tables processing on OSX. However >>>> there is a lack of support for RTL languages in ICU layout engine. Quote >>>> from ICU guide: “…The AAT processing in the LayoutEngine is relatively >>>> basic as it only applies the default features in left-to-right text. This >>>> processing has been tested for Devanagari text. Since AAT processing is >>>> not script-specific, it might not work for other scripts…”, more details >>>> at http://userguide.icu-project.org/layoutengine >>>> <http://userguide.icu-project.org/layoutengine> >>>> As a result all RTL languages on OSX are incorrectly laid out, (i.e. the >>>> letters are reversely presented). >>>> >>>> Fix: >>>> Skip font tables for RTL languages on OSX platform. >>>> >>>> Thanks, >>>> Dmitry >>>> >>>> [1] - https://bugs.openjdk.java.net/browse/JDK-7162125 >>>> <https://bugs.openjdk.java.net/browse/JDK-7162125> >> >
