PDFBOX-3293 truetype hinting

[pcorless]

PDFBOX-3293

• cuts in font hinting into FontBox and PDFBox
• combined my background on this issue with help from Claude to get some of the finer points worked out
• some more cleanup is needed but the basic structure is in place.

[THausherr]

Thank you, very interesting. From having a quick look I didn't see any breaking changes. I will set up some testing to see how the rendering is going.

You don't need to include the liberation font, it's already there, and is used in existing tests:

getResourceAsStream("/org/apache/pdfbox/resources/ttf/LiberationSans-Regular.ttf"));

[pcorless]

I'll get that font path touched up.

[THausherr]

I did a fast (human) visual compare by scrolling through the diff files. Besides the one chinese file that the issue is about, many were slightly different but the few where I had a closer look didn't seem to have any problem, one was slightly better, the horizontal bar in an "e" was more in the middle than without hinting. The rendering with hinting was faster (which is surprising, we'll see if this keeps happening) One difference has less quality:
mozilla/pdf.js#1732

file: PDFJS-1732.pdf

new rendering at 96 dpi (from the third notes line):

old rendering:

another file also caught my attention:
genko_oc_shiryo1.pdf
the new rendering looks partly bold.

new:

old:

Another small thing: IMHO the enabling of the hinting feature should be in PDFRenderer, not in TrueType. This can be done like with setSubsamplingAllowed(), and passed to PageDrawerParameters, then to PageDrawer.

[THausherr]

Copilot comment:

High-risk issues I found

1. GraphicsState.round() / round-state behavior may not match TrueType precisely

The round-state implementation is central to many opcodes. I’d specifically re-check:

ROUND_UP_TO_GRID
ROUND_DOWN_TO_GRID
ROUND_TO_DOUBLE_GRID
setSuperRound()

Why it’s risky:

These behaviors feed directly into MDRP, MIRP, MDAP, and MIAP.
A small rounding mismatch can shift stems by a pixel.

This one isn’t obviously broken, but it is critical enough that I’d want more proof against FreeType for edge values and negative distances.
2) TrueTypeInterpreter.installLogicalOps() likely has semantic issues

These lines stand out:

ODD / EVEN use Fixed.round(ctx.pop()) >> 6
AND / OR use Java truthiness semantics
NOT checks == 0

The likely issue is the exact definition of ODD/EVEN in TT bytecode. They depend on the rounded distance and parity at the 1/64 pixel level, and I’d want to verify the exact threshold behavior.

This may already be covered by tests, but I’d still flag it as worth a deeper look.
3) ROLL looks suspicious

Current implementation:

pops c, b, a
pushes b, c, a

That is not obviously the standard TrueType ROLL behavior unless the stack order matches a specific convention.

I’d double-check against the spec. ROLL is one of those stack-manipulation ops that can be silently wrong and then corrupt later execution.
4) MINDEX appears fragile

The implementation pops k, then pops k values, then pushes them back in a rearranged order.

That logic is a common place for off-by-one or order bugs. I would explicitly test:

k=1
k=2
larger values
interaction with the exact stack ordering expected by the spec

I don’t think this should block the PR on its own, but it deserves scrutiny.
5) TrueTypeInterpreter.jump() / control flow is a likely source of subtle bugs

This is one of the most important areas.

Concern points:

Jumps are relative to instructionStart()
instructionStart() is set just before opcode fetch
skipToElseOrEif() / skipToEif() manually skip over nested control flow and push operands

Potential issues:

whether relative offsets are measured from the right byte
whether ELSE/EIF skipping preserves correct nesting for all nested cases
whether JMPR, JROT, JROF interact correctly with instructionStart

This is exactly the sort of logic that can work for tested fonts but fail on odd control-flow constructs.
6) FDEF / IDEF / function body handling needs special scrutiny

The function-definition model stores:

the original program bytes
entry point offset

Then later replays from the stored program using callBody().

Things I’d verify carefully:

whether nested function definitions inside a function body are handled correctly
whether ENDF return semantics behave correctly when called recursively
whether ctx.setReturnFromFunction(false) is reset safely for nested calls
whether skipFunctionBody() properly handles push opcodes and nested IFs inside function bodies

This area is conceptually sound, but it’s one of the easiest places for “it works for Liberation Sans” but not for other fonts.
7) ExecutionContext.movePoint() backward-compatibility behavior is very heuristic-heavy

This is the riskiest logic in the whole patch.

The method now suppresses movement in certain cases when:

backward compatibility is enabled
IUP has run on both axes
y movement should freeze

That may be correct for the intended render mode, but it is a policy layer mixed into the VM’s primitive point-move operation.

Risks:

it can affect multiple opcodes unexpectedly
it changes behavior based on timing of IUP, which is hard to reason about
it may make the interpreter less spec-faithful than intended

If this is intended to emulate FreeType’s grayscale compatibility mode, I’d strongly suggest isolating it more explicitly so the core point move semantics stay clean.
8) hintingPpem() in PageDrawer looks plausible but needs proof for transforms

The code computes ppem from the transform’s vertical basis vector magnitude.

That’s clever, but because hinting is sensitive to device resolution and transform composition, I’d want:

explicit tests for rotation
explicit tests for anisotropic scaling
explicit tests for fractional transforms

You added some tests, which is good, but this is still a place where “looks right” can hide bugs.
Most concerning likely bug class

If I had to prioritize, I’d focus on:

control flow / stack ops
rounding semantics
function-definition replay
backward-compatibility movement suppression

Those are the areas most likely to produce subtle regressions.

[THausherr]

I can't comment on the copilot comments because I didn't look at the new code yet (e.g. because I have no knowledge whatsoever about hinting, the only thing I know is that it adjusts the points to a grid). I mostly looked at changes in the existing code and it looked like the new feature peacefully coexist, which is nice.

[pcorless]

I'll start working through those copilot comments.

Performance is an interesting one. On my test suite I also see no penalty for having hinting enabled. The caching works really well for most fonts.

I referenced pdf.js on my initial research as I thought it would be a good reference. As I dug in though it turned out their hinting was coming from a native system library not from JavaScript.

The two sample files you posted both fall into the "tricky" fonts category. FreeType has identified a list of fonts that need to be handled as special case. Differently being they need the bytecode to assembled/scale the glpyhs on every size. FreeTypes license is pretty flexible so I suspect it can be tied back in but it will take some legal work to properly update the LICENSE and NOTICE files.

Pushed changes for your two comments.

[THausherr]

Thank you; please merge the latest changes from the trunk, and enable line 708 in PagePane.java, the call renderer.setHintingEnabled(...) this way you (and me) can see the difference in PDFDebugger.

[THausherr]

Re Freetype license, it is not mentioned on
https://www.apache.org/legal/resolved.html
so we may have to ask, but they accept BSD and the freetype license is said to be "BSD like".
Re license, we need an ICLA from you
https://www.apache.org/licenses/contributor-agreements.html

[pcorless]

I've pushed the hinting hookup in the debugger.

I think I was was wrong on the "tricky" fonts on those two samples, will continue to investigate. I have some notes on the issue and need to revisit them.

I'll get you those contributor agreements soon.

[pcorless]

Push changes to address the review comment for github and fix for genko_oc_shiryo1.pdf bolding. PDFJS-1732.pdf is a different beast, hinting shouldn't be applied on this one, still looking for a why.

[THausherr]

not derivatives of FreeType (see hinting_plan.md "Oracle licensing").

Is this referring to a file in the FreeType distribution?

[pcorless]

I've touched up that wording. FreeType uses "oracle" quite a bit when comparing to a golden set. I've renamed it "reference" to make it more clear. I've also pulled out references to my local notes I used to build the plan for this.

I've been testing withe the sneaky changes on/off and I think it will be worth while figuring out how to handle the licensing as it's a real value add against my test set.

[THausherr]

Please merge the latest changes from the trunk, I have removed the trailing spaces so that the actual change is better visible.

Here's the latest copilot comments:

Blocking review comments

fontbox/src/main/java/org/apache/fontbox/ttf/BytecodeStream.java, around nextByte() and nextWord() — setEnd() establishes a function-body limit, and seek() honors that limit, but both read methods only check code.length. Consequently, a PUSHB/PUSHW operand at the end of a function body can consume bytes beyond the matching ENDF, including instructions from the surrounding program. This defeats the function-boundary protection and can produce incorrect execution or allow malformed font bytecode to escape its function body. Both methods should validate against limit rather than code.length, and nextWord() should verify that both bytes remain within the limit.

BytecodeStream.java

@@
public int nextByte()
{

• if (ip >= code.length)

• if (ip >= limit)
  {

Other important findings

fontbox/src/main/java/org/apache/fontbox/ttf/TrueTypeInterpreter.java, jump/function handling — Add an explicit regression test for a PUSHW or PUSHB whose operand bytes straddle the function limit. The current tests cover jumps out of a function body, but not operand reads crossing the limit, so they would not catch the defect above.

TrueTypeInterpreterTest.java

@@
+@test
+void testFunctionBodyCannotReadOperandsPastEndf()
+{

• // The PUSHW is positioned so its second operand would be ENDF or code after ENDF.
• // The interpreter must reject the read rather than consume bytes outside the body.

fontbox/src/test/resources/ttf/hinting/generate_golden.py and README.md — The documentation describes inconsistent FreeType targets: the generator uses FT_LOAD_TARGET_NORMAL, while the README describes monochrome/full-pixel tracing in places and the tests claim grayscale/v40 behavior. The golden-test contract should state one exact FreeType load target and use the same target in all scripts and documentation; otherwise regenerated reference files may silently validate a different interpreter mode.

fontbox/src/main/java/org/apache/fontbox/ttf/GlyphHinter.java — The implementation silently returns null for all composites when the component assembly or hinting path fails, causing the renderer to fall back to the raw outline. That is a reasonable resilience policy, but the tests should distinguish “unsupported composite hinting” from “successful composite hinting” and assert that representative composites actually receive instructions and produce the intended hinted result.

[pcorless]

OK thanks and I'll start taking a look at those comments.

[THausherr]

Copilot writes this:

fontbox/src/main/java/org/apache/fontbox/ttf/Fixed.java:83-85 — ceil() is incorrect for negative non-integral values. The implementation (value + 63) & ~63 only behaves like a ceiling for some negative inputs. For example, ceil(-65) returns -128, but the mathematical ceiling in F26Dot6 should be -64. Since the interpreter exposes this through the TrueType CEILING opcode, fonts using CEILING with negative distances will receive a value one pixel too small, potentially shifting hinted points and causing incorrect outlines. Please implement ceiling explicitly for signed values and add negative-value tests, e.g. ceil(-1) == 0, ceil(-63) == 0, ceil(-65) == -64, and ceil(-128) == -128.

I have no idea if this is realistic, i.e. if such values can occur (I tried with the regression tests and found nothing). Also this part of the code was there before so I wonder why copilot didn't complain until now.

Btw fontbox has not only some more ttf fonts in the repository (I see you did some tests with them), it also loads some ttf fonts during the tests. There's a dejavu font (you mentioned the font in a commit message), plus Keyboard.ttf, NotoEmoji-Regular.ttf, NotoMono-Regular.ttf, ipag.ttf. They are in the directory target/fonts.

[pcorless]

I've been testing with sample.pdf from the PDFBOX-3293 ticket and it's a good example of "tricky" vs. "No tricky". Render fine when enabled and skips hinting without it. I'll take another look at finding another way to detect these fonts.

[THausherr]

Thank you; please change GlyphHinter line 232 from " catch (IOException | RuntimeException e)" to "catch (IOException | HintingException e)", we want to know the hard way if something really nasty happens.

I have looked at the test coverage, there are parts of TrueTypeInterpreter that aren't covered so I ran my visual tests to see if any of the missing parts were hit. Only two were, flipRange and NPUSHW, and only with microsoft fonts, e.g. with this code:

    @Test
    void testArial() throws IOException
    {
        TrueTypeFont ttf = new TTFParser().parse(new RandomAccessReadBufferedFile("c:/windows/fonts/arial.ttf"));
        ttf.getHintedPath(81, 13); // flipRange + NPUSHW
    }

So we can't really test this on the CI, but locally with people who have windows (me). Could you add some code like this that checks that the result is what is expected?

    @Test
    void testArial() throws IOException
    {
        File file = new File("c:/windows/fonts/arial.ttf");
        assumeTrue(file.exists());
        TrueTypeFont ttf = new TTFParser().parse(new RandomAccessReadBufferedFile(file));
        GeneralPath hintedPath = ttf.getHintedPath(81, 13); // hits flipRange + NPUSHW
        // .... todo
    }

(I'm not expecting 100% test coverage, our code is only about 60%, this is just about improving when it's possible)