[OpenJDK 2D-Dev] [8u] RFR 8201801: RTL language (Hebrew) is presented from left to right

Dmitry Markov dmitry.markov at oracle.com
Wed Sep 12 14:27:58 UTC 2018


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 <prasanta.sadhukhan at oracle.com> 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 <dmitry.markov at oracle.com <mailto:dmitry.markov at oracle.com> <mailto:dmitry.markov at oracle.com> <mailto:dmitry.markov at oracle.com>> 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 <philip.race at oracle.com <mailto:philip.race at oracle.com> <mailto:philip.race at oracle.com> <mailto:philip.race at oracle.com>> 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 <prasanta.sadhukhan at oracle.com <mailto:prasanta.sadhukhan at oracle.com> <mailto:prasanta.sadhukhan at oracle.com> <mailto:prasanta.sadhukhan at oracle.com>> 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> 
>>>>>>> 
>>>>>> 
>>>>> 
>>>> 
>>> 
>> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/2d-dev/attachments/20180912/e183f90a/attachment.html>


More information about the 2d-dev mailing list