Hi Prasanta, I have updated the fix based on your recommendations. The new webrev is located at http://cr.openjdk.java.net/~dmarkov/8201801/jdk8u/webrev.03/ <http://cr.openjdk.java.net/~dmarkov/8201801/jdk8u/webrev.03/>
Thanks, Dmitry > On 12 Sep 2018, at 07:09, Prasanta Sadhukhan <[email protected]> > wrote: > > I guess we could check for > > if (((typo_flags & 0x80000000) != 0) && isAAT(font)) > before and could save on calling > > font.getLayoutTableCache() > In present condition, it overwrites layoutTables to 0 making calling > getLayoutTableCache meaningless. Maybe , do something like > long layoutTables = 0; > if !(((typo_flags & 0x80000000) != 0) && isAAT(font)) { > layoutTables = font.getLayoutTableCache(); > } > BTW, please correct the indentation of l156 and l160. > Regards > Prasanta > On 11-Sep-18 10:27 PM, Phil Race wrote: >> fine by me. >> >> -phil. >> >> On 9/10/2018 1:18 AM, Dmitry Markov wrote: >>> 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/><http://cr.openjdk.java.net/%7Edmarkov/8201801/jdk8u/webrev.02/> >>> <http://cr.openjdk.java.net/%7Edmarkov/8201801/jdk8u/webrev.02/> >>> >>> Thanks, >>> Dmitry >>> >>>> On 5 Sep 2018, at 12:27, Dmitry Markov <[email protected] >>>> <mailto:[email protected]> <mailto:[email protected]> >>>> <mailto:[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/><http://cr.openjdk.java.net/%7Edmarkov/8201801/jdk8u/webrev.02/> >>>> <http://cr.openjdk.java.net/%7Edmarkov/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]> <mailto:[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/~dmarkov/8201801/jdk8u/webrev.01/> >>>>>> <http://cr.openjdk.java.net/%7Edmarkov/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]> >>>>>>> <mailto:[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/~dmarkov/8201801/jdk8u/webrev.00/> >>>>>>>> <http://cr.openjdk.java.net/%7Edmarkov/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> >>>>>>> >>>>>> >>>>> >>>> >>> >> >
