Home Original page

[OpenJDK 2D-Dev] RFR: 8143177: Integrate harfbuzz opentype layout engine per JEP 258

Vadim Pakhnushev vadim.pakhnushev at oracle.com
Sun Nov 22 12:08:32 UTC 2015 
Looks good to me.

Vadim

On 22.11.2015 2:07, Philip Race wrote:
> http://cr.openjdk.java.net/~prr/8143177.2
>
> is uploaded. See comments below.
>
> On 11/21/15, 2:41 AM, Vadim Pakhnushev wrote:
>> Phil,
>>
>> In the HBShaper.c in the init_JNI_IDs function I think CHECK_NULL is 
>> missing for the gvdIndicesFID.
>> I guess it's just a copy from SunLayoutEngine.cpp but still.
>
> Actually this points out that I only return from the init function
> and don't return from the calling function. I will restructure this a 
> bit so that we
> return a success value and the caller checks it.
>
>> Calling init_JNI_IDs each time seems unneeded (not very harmful 
>> though), could it be somehow merged with the 
>> Java_sun_font_SunLayoutEngine_initGVIDs?
>
> They are static and I have added a static flag that indicates they 
> have all been initialised.
> If it fails then we will not proceed. In the normal case overhead 
> should be down to
> near zero.
>
>> What's the point of this super optimized euclidianDistance if it's 
>> called only 2 times while creating a layout?
>
> I just copied this from ICU code so we are doing the same thing.
>
>> pScaler is not used anywhere it seems, can it be removed altogether?
> It was used when I was directly testing freetype (using harfbuzz's 
> hb-ft.cc)
> That was where I started out but I moved on to a different approach.
> It is left it there in case I wanted to test ft to isolate any bugs.
>
>>
>> typo in the hb-jdk-font.cc:124    * which *be could* based on some 
>> on-the fly glyph analysis.
>
> ok
>
> I had to make one additional change. The hb call to create a coretext font
> was being handed the pt size in device pixels as used elsewhere but in
> the coretext path we must pass the user pt size so I had to pass that
> down and in .. it does not affect anything except the coretext path for
> AAT fonts on OS X. I spotted this when testing with a transform in
> that code path.
>
> -phil.
>>
>> Thanks,
>> Vadim
>>
>> On 21.11.2015 2:09, Phil Race wrote:
>>>
>>> On 11/19/2015 12:51 PM, Steven Loomis wrote:
>>>> I have reviewed the non-harfbuzz portion (outside of the /harfbuzz/ 
>>>> directory) and it looks good so far.
>>>
>>> Thanks. The harfbuzz code itself probably does not need careful 
>>> reviewing as it is just
>>> the upstream harfbuzz. I have updated it to remove the gpl text from 
>>> the hb src files but
>>> I haven't changed anything else :- 
>>> http://cr.openjdk.java.net/~prr/8143177.1/
>>>
>>> Any takers to be 2nd reviewer ?
>>>
>>> -phil.
>>>
>>>>
>>>> -s
>>>>
>>>> On 11/17/2015 4:05 PM, Philip Race wrote:
>>>>> Webrev : http://cr.openjdk.java.net/~prr/8143177/
>>>>> Bug : https://bugs.openjdk.java.net/browse/JDK-8143177
>>>>>
>>>>> This webrev contains the changes accumulated in the harfbuzz 
>>>>> project forest
>>>>> that migrate JDK from using ICU for opentype font layout to 
>>>>> harfbuzz for the same.
>>>>>
>>>>> The change does not introduce API. It is mostly adding the 
>>>>> harfbuzz library.
>>>>> The ICU library remains there but is no longer the default.
>>>>> Eventually (not necessarily in 9) it may be removed once we are 
>>>>> comfortable
>>>>> with harfbuzz.
>>>>> You may select ICU by using -Dsun.font.layoutengine=icu
>>>>>
>>>>> You may see which layout engine is in use by using 
>>>>> -Dsun.font.layoutengine.verbose=true
>>>>> it will print either "Using harfbuzz", or "Using ICU".
>>>>>
>>>>> The change has no impact on typical Latin script rendering but is 
>>>>> a big advance
>>>>> for complex scripts and also applies if you select kerning or 
>>>>> ligatures for Latin.
>>>>> However the latter is only detectable if the font implements 
>>>>> support for these.
>>>>> By "big advance" I mean that ICU has not been updated to recognise 
>>>>> recent opentype features.
>>>>> So harfbuzz should fix a number of things but unexpected changes 
>>>>> that look wrong
>>>>> should be reported so we can investigate.
>>>>>
>>>>> To use harfbuzz we invoke its shaper and we provide a way to get 
>>>>> jdk font information.
>>>>> This means that we do not need a different layer depending on 
>>>>> whether freetype or t2k
>>>>> is used. It also enables some caching in the JDK font layer.
>>>>>
>>>>> On macosx harfbuzz does not natively read the AAT tables but will 
>>>>> invoke CoreText.
>>>>> This does mean that an AAT font installed on Linux would not be 
>>>>> processed but
>>>>> this is not a significant issue since AAT fonts are not found 
>>>>> other than on macosx.
>>>>>
>>>>> The majority of the files in the webrev are harfbuzz itself, 
>>>>> changed only to comply
>>>>> with JDK whitespace rules. Reviewers should probably concentrate 
>>>>> on all of the rest.
>>>>> I've listed it so that all those files are at the beginning and 
>>>>> you can ignore all those
>>>>> that follow that are in the "harfbuzz" directory.
>>>>>
>>>>> The harfbuzz version used here is 1.0.6 which is the latest source 
>>>>> release (at the time of writing).
>>>>> We expect to update this to keep reasonably current.
>>>>>
>>>>> I would like to push this in on Friday, but at the very latest 
>>>>> Monday because of the
>>>>> upcoming Feature Complete date so there are a couple of days to 
>>>>> make small
>>>>> tweaks for serious problems but there will be plenty of time after 
>>>>> integration to fix
>>>>> remaining problems.
>>>>>
>>>>> -phil.
>>>>>
>>>>
>>>
>>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/2d-dev/attachments/20151122/d34485f9/attachment.html>
More information about the 2d-dev mailing list