RFR: 8318364: Add an FFM-based implementation of harfbuzz OpenType layout [v4]

[Phil Race]

On Thu, 16 Nov 2023 13:48:38 GMT, Prasanta Sadhukhan <psadhukhan at openjdk.org> wrote:

>> Phil Race has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   indentation
>
> src/java.desktop/share/classes/sun/font/HBShaper.java line 628:
> 
>> 626:         MemorySegment glyphInfoArr = glyphInfo.reinterpret(glyphInfoSize);
>> 627: 
>> 628:          for (int i=0; i<glyphCount; i++) {
> 
> Spacing between operators

fixed

> src/java.desktop/share/classes/sun/font/HBShaper.java line 658:
> 
>> 656:         startPt.x = advX;
>> 657:         startPt.y = advY;
>> 658:         startPt.x = advX;
> 
> duplicate assignment of startPt.x to advX...

fixed

> src/java.desktop/share/classes/sun/font/SunLayoutEngine.java line 167:
> 
>> 165:     }
>> 166: 
>> 167:     static boolean useFFM = true;
> 
> So, we want to enable FFM by default?

yes

> src/java.desktop/share/classes/sun/font/SunLayoutEngine.java line 184:
> 
>> 182:         FontStrike strike = font.getStrike(desc);
>> 183:         if (useFFM) {
>> 184:             java.lang.foreign.MemorySegment face = HBShaper.getFace(font);
> 
> Guess import at start will be more nicer...

ok .. although I do sometimes do this pattern when there's only one usage in the file.

> src/java.desktop/share/native/libfontmanager/HBShaper_Panama.c line 141:
> 
>> 139:      hb_buffer_destroy (buffer);
>> 140:      hb_font_destroy(hbfont);
>> 141:      if (features != NULL) free(features);
> 
> Guess coding style warrants braces { and next statement in separate line...

fixed

> src/java.desktop/share/native/libfontmanager/hb-jdk-font-p.cc line 238:
> 
>> 236:                       HBFloatToFixed(ptSize*devScale),
>> 237:                       HBFloatToFixed(ptSize*devScale));
>> 238:   return font;
> 
> indentation is off..

fixed

> test/jdk/java/awt/font/GlyphVector/LayoutCompatTest.java line 29:
> 
>> 27: /*
>> 28:    @test
>> 29:    @summary verify JNI and FFM harfbuzz OpenType layout implementations are equivalent.
> 
> bug id is missing

Because there isn't a "bug". It is just a test for new functionality.

> test/jdk/java/awt/font/GlyphVector/LayoutCompatTest.java line 45:
> 
>> 43: public class LayoutCompatTest {
>> 44: 
>> 45:    static String jni = "jni.txt";
> 
> Seems test is failing without fix with Exception in jtreg
> java.io.FileNotFoundException: jni.txt (The system cannot find the file specified)
> 
> Also in standalone mode. I was expecting it will fail with RuntimeException "files differ byte offset"

I'm not sure why it matters what this test does in a JDK without the fix, although logically, since the new system property isn't known, both cases would  end up using JNI, and I'd expect the test to pass. I am not sure why you say it should fail.

And I can't reproduce your first problem, I ran this test in jtreg on an unmodified JDK22 and it passed, as I expected, for the reason given above.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/15476#discussion_r1399596923
PR Review Comment: https://git.openjdk.org/jdk/pull/15476#discussion_r1399597001
PR Review Comment: https://git.openjdk.org/jdk/pull/15476#discussion_r1399597224
PR Review Comment: https://git.openjdk.org/jdk/pull/15476#discussion_r1399597892
PR Review Comment: https://git.openjdk.org/jdk/pull/15476#discussion_r1399598026
PR Review Comment: https://git.openjdk.org/jdk/pull/15476#discussion_r1399598126
PR Review Comment: https://git.openjdk.org/jdk/pull/15476#discussion_r1399598208
PR Review Comment: https://git.openjdk.org/jdk/pull/15476#discussion_r1399596012