Phil, Prasanta, Any thoughts/concerns regarding the latest webrev: http://cr.openjdk.java.net/~dmarkov/8201801/jdk8u/webrev.02/ <http://cr.openjdk.java.net/~dmarkov/8201801/jdk8u/webrev.02/>
Thanks, Dmitry > On 5 Sep 2018, at 12:27, Dmitry Markov <[email protected]> wrote: > > 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/ > <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] >> <mailto:[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> >>> >> >
