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

Philip Race philip.race at oracle.com
Sat Nov 21 23:07:57 UTC 2015


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.


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.

> 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/20151121/8473a76e/attachment.html>

More information about the 2d-dev mailing list