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

[Prasanta Sadhukhan]

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/%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>> 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/%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>> 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 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 
>>>>>> <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
>>>>>>> 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
>>>>>>> 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
>>>>>>
>>>>>
>>>>
>>>
>>
>

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