[OpenJDK 2D-Dev] RFR: JDK-8187100: support Variation Selectors(Resend)
Phil Race
philip.race at oracle.com
Mon Jun 11 20:46:10 UTC 2018
It seems that although there is code added at the harfbuzz level
to locate variations, this is being treated analagously to the surrogates
where the basic font code can handle it, inserting empty glyphs, and
not caching the glyphs for the base unicode code point.
This
- leads to the need to update the DefaultEditorKit code but I
am not sure if this is missing anything.
- adds complexity and overhead to that basic fast path, that seems to
much exceed
what we have with surrogates.
Did you look at just handling variations purely by layout ?
For example we could explore having isComplexCharCode return true for
these and
directing all uses down the layout path ? Then we can maybe remove all
this logic from
case used by the fast path code.
Parsing of the table ..
Somewhere you should catch IOException or any other exception
that might occur for a corrupt or invalid table
Probably wrap the "new UVS(..)" call in a catch Throwable.
Then we can ignore an invalid table.
The values read should all be positive but it seems to me we may read
some of them as negative.
+ int tableOffset = buffer.getInt(offset + 10 + i * 11 + 3);
+ tableOffset = buffer.getInt(offset + 10 + i * 11 + 7); If these are
negative we should bail ! And this :
+ additionalCount[i][j] = buffer.get(); is storing a uint8 into a byte,
so it can't even hold the valid range! You will need to store into a
short after masking with &0FF ' Is there some logic we can use to verify the size of arrays such as these is valid ?
+ selector = new int[numSelectors]; + startUnicodeValue[i] = new
int[numUnicodeValueRanges[i]]; + unicodeValue[i] = new
int[numUVSMapping[i]]; For example I think if iterating over the number
would inevitably make us read off the end of the table, we should bail
there and then. Also the spec says some things must be stored in
increasing order. As we parse the table, we should validate that and
bail if the table does not follow the rules.
And also either when parsing the table, for the non-default case,
where a glyphID is specified, or later, when a lookup happens,
there should be some validation of this against the actual number
of glyphs supported in the font so we don't hand off an invalid
glyph ID to calling code.
+ if (index >=0 && add a space : " >= 0"
+
+ /* getGlyph for Variation selector
+ return value:
+ 0: A special glyph for the variation selector is Not found
+ -1: Default glyph should be used
+ 0>: A special glyph is found
+ */
+ int getGlyph(int charCode, int variationSelector) { I don't understand
why we need or want 0 here. Looking at
+ public char getGlyph(int charCode, int variationSelector) { + if (uvs
== null) { + return 0; + } + int result = uvs.getGlyph(charCode,
variationSelector); + if (result == -1) { + result =
this.getGlyph(charCode); + } + return (char)(result & 0xFFFF); + } It
seems that if the UVS subtable has no entry that corresponds we'll
return the missing glyph. don't you just want to ignore the variation
selector and so behave as if -1 was returned ? Put another way,
supposing someone provides an invalid selector, or that the unicode spec
added a new standardised selector after this font was created. It may
have no mapping for it because it just is unaware. You'll return a
missing glyph instead of the far more logical base glyph .. It is not
even clear to me that the spec says you must map every single unicode
point, so this seems more or less inevitable.
I do see that in TrueTypeGlyphMapper.java
96 private char getGlyphFromCMAP(int charCode, int variationSelector) {
It looks for the zero return and will handle that there (as well as the
out of range glyph) but I don't see why we can't handle that directly in
the CMAP code ? I'd really appreciate some comments pointing to the spec
for these and describing them :+ public static final int VS_START = 0xFE00; + public static final int
VS_END = 0xFE0F; + public static final int VSS_START = 0xE0100; + public
static final int VSS_END = 0xE01FF; http://cr.openjdk.java.net/~srl/8187100/webrev.01/src/java.desktop/share/classes/sun/font/TrueTypeGlyphMapper.java.sdiff.html
118 } catch(Exception e) { missing space. Look at this : 219 if (i <
count - step &&
220 isVariationSelector(unicodes[i + step]) &&
221 isBaseChar(code)) {
222 variationSelector = unicodes[i + step];
223 glyphs[i] = getGlyphFromCMAP(code, variationSelector);
224 glyphs[i+step] = INVISIBLE_GLYPH_ID;
225 i += 1;
226 } else if (i < count - step -1 &&
227 isVariationSelector(unicodes[i + step],
228 unicodes[i + step + 1]) && It seems to me that except for the last
code point, you will be calling isVariationSelector() twice for every
code point which in all likelihood is always going to return false. This
is already adding gobs of overhead and we can't afford to be wasteful
like this.
I think maybe the problem is that this is being inserted into the low
level chars to glyph used even by drawGlyphVector If this can't be made
more efficient, then maybe it should be just part of the TextLayout path
as mentioned earlier. can we have a boolean maybeVariationSelector(char
c) { return c >= HI_SURROGATE_START;} 99.99% of the time that one test will return false and then you know
you will not see a variation selector, or a surrogate encoded variation
selector
supplement code point since all possibilities are excluded. So you can
write boolean maybe = maybeVariationSelector(unicodes[i+step]);
if (maybe) {
... only then do the detailed checking
}
56 env->ExceptionDescribe();
This is for debugging. Don't use it in production code.
Since we are trying to be careful about clearing exceptions, I am fairly
sure
the allocation calls like this :
65 unicodes = env->NewIntArray(2); and 69 results = env->NewIntArray(2);
may throw OutOfMemoryError as well as returning NULL. So cleanup: should
start with if (env->ExceptionOccurred()) { env->ExceptionClear();
} This : 74 tmp = new jint[2]; would be more efficient as a stack
allocation and then you don't need to free it either -phil.
On 05/31/2018 03:19 PM, Steven R. Loomis wrote:
> Updated webrev:
>
> http://cr.openjdk.java.net/~srl/8187100/webrev.01/
> <http://cr.openjdk.java.net/%7Esrl/8187100/webrev.01/>
>
> On Fri, May 18, 2018 at 9:16 AM, Toshio 5 Nakamura
> <TOSHIONA at jp.ibm.com <mailto:TOSHIONA at jp.ibm.com>> wrote:
>
> Thank you for your review, Phil.
> I'm working to handle your points.
>
> Toshio Nakamura
>
> Philip Race <philip.race at oracle.com
> <mailto:philip.race at oracle.com>> wrote on 2018/05/18 13:39:35:
>
> > From: Philip Race <philip.race at oracle.com
> <mailto:philip.race at oracle.com>>
> > To: Toshio 5 Nakamura <TOSHIONA at jp.ibm.com
> <mailto:TOSHIONA at jp.ibm.com>>
> > Cc: 2d-dev <2d-dev at openjdk.java.net
> <mailto:2d-dev at openjdk.java.net>>
> > Date: 2018/05/18 13:39
>
>
> > Subject: Re: [OpenJDK 2D-Dev] RFR: JDK-8187100: support Variation
> > Selectors(Resend)
> >
> > There's a lot to think about here but I have some requests to first
> > clean up the proposed code to conform to coding standards.
> >
> > I see lots of lines > 80 chars. Please fix
> >
> > I see if(foo){ and else{ which should be "if (foo) {" and "else {"
> >
> > Basically always have a space before { and after ) and around
> "=" and "=="
> >
> > One line that WAS
> > 51 hb_codepoint_t u = (variation_selector==0) ? unicode :
> > variation_selector;
> >
> > has no spaces around "==" almost certainly to avoid going over
> 80 chars.
> > Now you've broken the line you can fix that.
>
> >
> > Eliminate all wild card imports and import exactly what is needed.
> > Maybe this is only about the test.
> >
> > remove what looks like SCCS style comments on the @test line.
> > Make the test simply print a warning if the person didn't install
> > fonts where you expected.
> > Why? Because @ignore defaults to .. not being ignored.
> >
> > For the JNI methods calls read the spec and see if calling them may
> > throw an exception
> >
> > I'm looking at sequences like
> > env->SetIntArrayRegion(unicodes, 0, 2, tmp);
> > 71 env->CallVoidMethod(font2D,
> > sunFontIDs.f2dCharsToGlyphsMID, 2, unicodes, results);
> > 72 env->GetIntArrayRegion(results, 0, 2, tmp);
> > 73 *glyph = tmp[0];
> > 74 cleanup:
> > 75 if (unicodes != NULL) env->DeleteLocalRef(unicodes);
> > 76 if (results != NULL) env->DeleteLocalRef(results);
> >
>
> > If call GetIntArrayRegion may leave a pending exception it needs to
> > be checked and cleared
> > before you make another call.
> >
> > Look at the performance implications of things like the extra
> > work done now in FontUtilities.isSimpleChar() and see how to
> minimise
> > it since it could affect all text rendering in a way that is
> measurable
> > in at least some way.
> >
> > I idly wonder if
> >
> >
> > public static boolean isBaseChar(int charCode){ ...
> >
> > might be more cleanly or efficiently implemented with a switch ?
> >
> > Not sure.
> >
> > -phil.
> >
> > On 5/14/18, 1:28 AM, Toshio 5 Nakamura wrote:
> > Could someone review our proposal to support Unicode Variation
> Selectors?
> > > Bug: https://bugs.openjdk.java.net/browse/JDK-8187100
> <https://bugs.openjdk.java.net/browse/JDK-8187100>
> > > Webrev: http://cr.openjdk.java.net/~srl/8187100/webrev.00/
> <http://cr.openjdk.java.net/%7Esrl/8187100/webrev.00/>
> >
> > Toshio Nakamura
> >
> > > From: "Steven R. Loomis" <srl295 at gmail.com
> <mailto:srl295 at gmail.com>>
> > > To: 2d-dev <2d-dev at openjdk.java.net
> <mailto:2d-dev at openjdk.java.net>>
> > > Date: 2018/05/03 03:27
> > > Subject: Re: [OpenJDK 2D-Dev] RFR: JDK-8187100: support Variation
> > > Selectors (Resend)
> > > Sent by: "2d-dev" <2d-dev-bounces at openjdk.java.net
> <mailto:2d-dev-bounces at openjdk.java.net>>
> > >
> > > I added a screenshot to
> https://bugs.openjdk.java.net/browse/JDK-8187100
> <https://bugs.openjdk.java.net/browse/JDK-8187100>
> > > if anyone wants to see what the impact of this fix is
> > >
> > > On Wed, Apr 25, 2018 at 8:39 AM, Steven R. Loomis
> <srl295 at gmail.com <mailto:srl295 at gmail.com>> wrote:
> > > (Retrying as actual text)
> > >
> > > Support Unicode Variation Selectors.
> > >
> > > Code by my colleague Toshio Nakamura, I added a simple test, and
> > > include a test that was part of JDK 8187100. (Both tests are
> run manually.)
> > >
> > > Bug: https://bugs.openjdk.java.net/browse/JDK-8187100
> <https://bugs.openjdk.java.net/browse/JDK-8187100>
> > > Webrev: http://cr.openjdk.java.net/~srl/8187100/webrev.00/
> <http://cr.openjdk.java.net/%7Esrl/8187100/webrev.00/>
> > >
> > > On 04/08/2018 11:46 PM, Toshio 5 Nakamura wrote:
> > > >
> > > > Hello
> > > >
> > > > IBM would like to propose Unicode Variation Selector[1]
> capability to AWT
> > > > and Swing components.
> > > > (This proposal was posted to i18n-dev first, and I got a
> suggestion to
> > > > discuss
> > > > in 2d-dev.)
> > > >
> > > > This proposal changed the following files:
> > > > src/java.desktop/share/classes/sun/font/CMap.java
> > > > src/java.desktop/share/classes/sun/font/CharToGlyphMapper.java
> > > >
> src/java.desktop/share/classes/sun/font/CompositeGlyphMapper.java
> > > > src/java.desktop/share/classes/sun/font/Font2D.java
> > > > src/java.desktop/share/classes/sun/font/FontRunIterator.java
> > > > src/java.desktop/share/classes/sun/font/FontUtilities.java
> > > > src/java.desktop/share/classes/sun/font/TrueTypeGlyphMapper.java
> > > > src/java.desktop/share/native/common/font/sunfontids.h
> > > > src/java.desktop/share/native/libfontmanager/hb-jdk-font.cc
> > > > src/java.desktop/share/native/libfontmanager/sunFont.c
> > > >
> src/java.desktop/share/classes/javax/swing/text/DefaultEditorKit.java
> > > > 542 lines will be changed.
> > > >
> > > > There are three parts.
> > > > 1) Adding CMap format 14 support
> > > > 2) Adding CharsToGlyphs functions support Variation Selector
> Sequences
> > > > 3) Swing text component's DEL and BS key operations change
> > > >
> > > >
> > > > How would I go about obtaining a sponsor?
> > > >
> > > > [1] _http://www.unicode.org/versions/Unicode10.0.0/ch23.pdf_
> <http://www.unicode.org/versions/Unicode10.0.0/ch23.pdf_>
> > > > Chapter 23.4 Variation Selectors
> > > >
> > > > Best regards,
> > > >
> > > > Toshio Nakamura
> > > > IBM Japan
> > >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/2d-dev/attachments/20180611/844d3d58/attachment-0001.html>
More information about the 2d-dev
mailing list