8328896: Fontmetrics for large Fonts has zero width #18703

prrace · 2024-04-09T18:50:17Z

The main problem here is that we do not curate what font point size we passed to freetype,
and would pass in one larger than freetype's maximum of FT_USHORT_MAX which is USHRT_MAX.
This isn't documented, SFAICS, and is checked a couple of calls deep from the specific API we use.
But generally anywhere near that size seems to cause freetype to choke as it uses signed 16.16
values, so 32767 is really the max.
But we have no need to go anywhere near that - 16384 seems like a plenty large enough pt size.
And we can request bigger sizes than that by making use of the transform.
At normal size ranges we use that just to account for rotation and decompose the glyph transform
into point size and that rotation.

But at large sizes - which are not usefully rendered anyway - there are no hints etc to be lost
from not specifying the target point size. So we can extend the range of sizes we allow.

If this is still too large to be held decomposed into a pt size in the range less than 16384 and a scale of up to 32766 then we substitute the null scaler, as we generally do when values are out of range, such
a for a NaN transform.

These extreme values aren't useful.

In looking at this I did find that getGlyphPixelBounds doesn't follow the maximum image size we use
for rendering. Which is not useful and could lead to inconsistent results. I fixed that.

Also whilst macOS didn't have these problems it could be cleaned up a bit for consistency in the reported sizes for some cases.

The test is mainly around making sure that "sensible" things are returned for not sensible input.
There's no 100% right answer to these extreme unrenderable sizes.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8328896: Fontmetrics for large Fonts has zero width (Bug - P3)

Reviewers

Tejesh R (@TejeshR13 - Committer)
Sergey Bylokhov (@mrserb - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18703/head:pull/18703
$ git checkout pull/18703

Update a local copy of the PR:
$ git checkout pull/18703
$ git pull https://git.openjdk.org/jdk.git pull/18703/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18703

View PR using the GUI difftool:
$ git pr show -t 18703

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18703.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-04-09T18:50:52Z

👋 Welcome back prr! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-04-09T18:52:11Z

@prrace This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8328896: Fontmetrics for large Fonts has zero width

Reviewed-by: tr, serb

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 164 new commits pushed to the master branch:

57ebd04: 8330153: C2: dump barrier information for all Mach nodes
58ad399: 8330821: Rename UnsafeCopyMemory
1d52234: 8330704: Clean up non-standard use of /** comments in some langtools tests
83c74d7: 8329717: Missing @since tags in elements in DocumentationTool and Taglet
0b9350e: 8322992: Javac fails with StackOverflowError when compiling deeply nested synchronized blocks
20be5e0: 8314846: Do not store Klass::_secondary_super_cache in CDS archive
7e421ce: 8330585: Refactor/rename forwardee handling
3e65d90: 8330820: Remove remnants of operator_new.cpp in build system
936a47d: 8330607: Deprecate -XX:+UseEmptySlotsInSupers
ee7b2e9: 8330051: Small ObjectMonitor spinning code cleanups
... and 154 more: https://git.openjdk.org/jdk/compare/b80ba0851841a8490e61371ac4ef3514dc6eddf5...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2024-04-09T18:52:36Z

@prrace The following label will be automatically applied to this pull request:

client

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

turbanoff · 2024-04-09T19:03:22Z

src/java.desktop/share/classes/sun/font/FileFontStrike.java

+            if (obds.isEmpty()) {
+                Rectangle bds = getGlyphOutline(glyphCode, pt.x, pt.y).getBounds();
+                result.setBounds(bds);
+            }  else {


Suggested change

} else {

} else {

mlbridge · 2024-04-09T19:12:33Z

Webrevs

00: Full (47716f2c)

TejeshR13

Can you please update the copywrite year for updated files.

TejeshR13 · 2024-04-18T04:03:38Z

src/java.desktop/share/native/libfontmanager/freetypeScaler.c

    context->ptsz = (int)(ptsz * 64);
+    if (TOO_LARGE(dmat[0], ptsz) || TOO_LARGE(dmat[1], ptsz) ||
+        TOO_LARGE(dmat[2], ptsz) || TOO_LARGE(dmat[3], ptsz))
+    {


Suggested change

{

TOO_LARGE(dmat[2], ptsz) || TOO_LARGE(dmat[3], ptsz)) {

No, that was deliberate. We generally prefer the { on a new line if the conditional is multi-line.

Still looking for an actual "reviewer" review here.

TejeshR13

LGTM.

prrace · 2024-04-24T17:53:40Z

/integrate

openjdk · 2024-04-24T17:54:12Z

Going to push as commit 25871af.
Since your change was applied there have been 206 commits pushed to the master branch:

1d06170: 8300148: Consider using a StoreStore barrier instead of Release barrier on ctor exit
f1d0e71: 8324577: [REDO] - [IMPROVE] OPEN_MAX is no longer the max limit on macOS >= 10.6 for RLIMIT_NOFILE
74b11cc: 8331004: Serial: Remove unused GenClosure
45ed97f: 8330464: hserr generic events - add entry for the before_exit calls
e311ba3: 8330626: ZGC: Windows address space placeholders not managed correctly
e923dfe: 8314592: Add shortcut to SymbolLookup::find
1519081: 8330303: Crash: assert(_target_jt == nullptr || _target_jt->vthread() == target_h()) failed
2bb5cf5: 8330155: Serial: Remove TenuredSpace
7bb59dc: 8330670: Obsolete ScavengeBeforeFullGC
174d626: 8328741: serviceability/jvmti/ObjectMonitorUsage/ObjectMonitorUsage.java failed with unexpected owner
... and 196 more: https://git.openjdk.org/jdk/compare/b80ba0851841a8490e61371ac4ef3514dc6eddf5...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-04-24T17:54:23Z

@prrace Pushed as commit 25871af.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8328896

2a36d1b

openjdk bot changed the title ~~8328896~~ 8328896: Fontmetrics for large Fonts has zero width Apr 9, 2024

openjdk bot added the client client-libs-dev@openjdk.org label Apr 9, 2024

turbanoff reviewed Apr 9, 2024

View reviewed changes

prrace added 2 commits April 9, 2024 12:06

8328896

0d314e7

8328896

47716f2

openjdk bot added the rfr Pull request is ready for review label Apr 9, 2024

TejeshR13 reviewed Apr 18, 2024

View reviewed changes

mrserb approved these changes Apr 23, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 23, 2024

TejeshR13 approved these changes Apr 23, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Apr 24, 2024

openjdk bot closed this Apr 24, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 24, 2024

kumarabhi006 mentioned this pull request May 31, 2024

8160755: bug6492108.java test fails with exception Image comparison failed at (0, 0) for image 4 in GTK L&F #19381

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8328896: Fontmetrics for large Fonts has zero width #18703

8328896: Fontmetrics for large Fonts has zero width #18703

prrace commented Apr 9, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Apr 9, 2024

openjdk bot commented Apr 9, 2024 •

edited

Loading

openjdk bot commented Apr 9, 2024

turbanoff Apr 9, 2024

mlbridge bot commented Apr 9, 2024

TejeshR13 left a comment

TejeshR13 Apr 18, 2024

prrace Apr 18, 2024

TejeshR13 left a comment

prrace commented Apr 24, 2024

openjdk bot commented Apr 24, 2024

openjdk bot commented Apr 24, 2024

8328896: Fontmetrics for large Fonts has zero width #18703

8328896: Fontmetrics for large Fonts has zero width #18703

Conversation

prrace commented Apr 9, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Apr 9, 2024

openjdk bot commented Apr 9, 2024 • edited Loading

openjdk bot commented Apr 9, 2024

turbanoff Apr 9, 2024

Choose a reason for hiding this comment

mlbridge bot commented Apr 9, 2024

Webrevs

TejeshR13 left a comment

Choose a reason for hiding this comment

TejeshR13 Apr 18, 2024

Choose a reason for hiding this comment

prrace Apr 18, 2024

Choose a reason for hiding this comment

TejeshR13 left a comment

Choose a reason for hiding this comment

prrace commented Apr 24, 2024

openjdk bot commented Apr 24, 2024

openjdk bot commented Apr 24, 2024

prrace commented Apr 9, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Apr 9, 2024 •

edited

Loading