[OpenJDK 2D-Dev] RFR: 8143177: Integrate harfbuzz opentype layout engine per JEP 258
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:
> 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
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
> 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.
> 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 :-
>> Any takers to be 2nd reviewer ?
>>> 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
>>>> The ICU library remains there but is no longer the default.
>>>> Eventually (not necessarily in 9) it may be removed once we are
>>>> with harfbuzz.
>>>> You may select ICU by using -Dsun.font.layoutengine=icu
>>>> You may see which layout engine is in use by using
>>>> 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.
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the 2d-dev