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.
