THausherr commented on PR #532:
URL: https://github.com/apache/pdfbox/pull/532#issuecomment-5694862824
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
