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

[Vadim Pakhnushev]

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.
Calling init_JNI_IDs each time seems unneeded (not very harmful though), 
could it be somehow merged with the Java_sun_font_SunLayoutEngine_initGVIDs?
What's the point of this super optimized euclidianDistance if it's 
called only 2 times while creating a layout?
pScaler is not used anywhere it seems, can it be removed altogether?

typo in the hb-jdk-font.cc:124    * which *be could* based on some 
on-the fly glyph analysis.

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/da1b09ea/attachment.html>