[
https://issues.apache.org/jira/browse/PDFBOX-6293?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18135203#comment-18135203
]
Tilman Hausherr commented on PDFBOX-6293:
-----------------------------------------
Claude Sonnet 5.5 review below. I've committed this with minor changes (1, 2
and 5 below). I've removed "Isolated" for 2.0 because it isn't supported nor
needed there. I've also remove an unused method in the test.
====
h2. Review of apache/pdfbox#551: resolve glyphs via Unicode for CIDFontType2
with an Identity-ordered substitute (PDFBOX-6293)
I could read the {{PDCIDFontType2}} change, the LICENSE change and
{{{}IdentityCJKFixture.java{}}}. I could not see
{{IdentityCJKSubstituteTest.java}} (the fetch was truncated and the commit page
blocks automated access) or the rest of {{{}codeToGID{}}}, and I did not build
or run anything. So I’m reviewing the logic as shown, not the tests.
h3. What it does
When a non-embedded CID font is replaced by an installed CFF-based OpenType
font that is CID-keyed with Adobe-Identity-0 (Noto Sans CJK, Source Han), the
old code treated the PDF’s CIDs as glyph IDs in that font. That is wrong when
the PDF is, say, Adobe-GB1. The fixture comment gives a concrete case: U+4E2D
is CID 4559 in GB1 but GID 9544 in the substitute, so the wrong glyph gets
drawn.
The PR adds {{{}isForeignCIDSubstitute{}}}. It is true when the font is not
embedded, the substitute is a CFF CID font with ordering “Identity”, and the
PDF’s ROS is missing or differs. In that case the CID-equals-GID branch in
{{codeToGID}} is skipped and lookup falls through to the Unicode path. The
diagnosis is sound and the fix is small and scoped to non-embedded fonts only.
h3. Requested changes
# *Possible NPE on malformed PDFs.* {{ros.getRegistry().equals(...)}} and
{{ros.getOrdering().equals(...)}} throw if {{/Registry}} or {{/Ordering}} is
missing in a broken {{{}CIDSystemInfo{}}}. PDFBox is routinely fed such files,
and this runs in the constructor, so it would fail the whole font load. Use
{{Objects.equals(...)}} (or flip the receivers). Please add a test with a
{{CIDSystemInfo}} that lacks {{{}/Registry{}}}.
# *Comment and code disagree.* The field comment says “substitute is CID-keyed
with a different ROS”, but the code also requires the substitute’s ordering to
be “Identity”. A substitute with a real but different ordering (e.g.
Adobe-Japan1 used for a GB1 PDF) has the same wrong-glyph problem and is not
handled. Either broaden the condition to any ROS mismatch, or fix the comment
and state that this is deliberately limited to Identity.
# *Related existing problem, probably out of scope.* The old “Identity is the
default for CFF-based OpenTypeFonts” branch also still applies when the ROS
matches. In a CID-keyed CFF font, CID to GID normally goes through the charset
and is not identity, except for fonts built so that CIDs equal GIDs, as
Identity-0 builds are. A comment or follow-up issue would be enough.
# *Verify the fall-through path.* I couldn’t see the code the new condition
falls into. Please confirm it does {{parent.toUnicode(code)}} followed by
{{{}cmap.getGlyphId(...){}}}, that {{cmap}} is non-null for the CFF-flavoured
OTF, and that the no-mapping case is handled. I’d also check the other callers
of {{{}codeToGID{}}}: width, path, {{{}hasGlyph{}}}.
# *LICENSE placement.* The Noto CJK subset lives in
{{{}src/test/resources{}}}, but the OFL text is added to
{{{}src/main/appended-resources/META-INF/LICENSE{}}}, which goes into the
shipped artifacts. The existing entries there are for fonts that are actually
bundled. Please confirm with the project’s release practice whether a test-only
font belongs there. Also compare the notice with the upstream one, since I
believe upstream may include a Reserved Font Name clause that this entry omits.
I’m not certain of that.
# *Fixture robustness.* {{new File("src/test/resources/...")}} only works when
the working directory is the module directory, so it breaks when run from the
repo root or some IDE setups. Load it as a classpath resource instead. Please
also state the size of the {{.otf}} in the PR description. The {{@Isolated}}
note for the JVM-wide {{FontMappers.set}} is good.
# *PR description.* The body is effectively just “Same. 100% 🤖”, which refers
to nothing. Please add:
** the user-visible symptom
** a before/after example (ideally a small PDF or a rendering)
** the JIRA link
** a disclosure of how AI-generated code was used, in line with ASF
generative-tooling guidance
h3. Minor
* {{isForeignCIDFont}} is an instance method called from the constructor and
reads {{{}getCIDSystemInfo(){}}}. It works today since the dictionary is set by
the super constructor, but making the helper {{private static}} and passing the
{{PDCIDSystemInfo}} in avoids the subtlety.
* Style follows PDFBox conventions (Allman braces, 4-space indentation).
h3. Verdict
Request changes. The approach is correct, but items 1, 2 and 7 should be fixed
first, and the LICENSE question (item 5) needs a maintainer’s answer. Once
those are addressed this looks mergeable, assuming the tests I couldn’t see are
sound.
> Non-embedded CIDFontType2 fonts render wrong glyphs when the substitute is an
> Identity-ordered CJK font
> -------------------------------------------------------------------------------------------------------
>
> Key: PDFBOX-6293
> URL: https://issues.apache.org/jira/browse/PDFBOX-6293
> Project: PDFBox
> Issue Type: Bug
> Components: Rendering
> Reporter: Tim Allison
> Priority: Trivial
>
> Follow on to PDFBOX-6249
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
