-
Notifications
You must be signed in to change notification settings - Fork 76
fix: emit glyph marker for glyph-index names instead of leaking them as text #302
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
wittjeff
wants to merge
2
commits into
docling-project:main
Choose a base branch
from
wittjeff:fix/gid-glyph-name-fallback
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+157
−2
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,138 @@ | ||
| #!/usr/bin/env python | ||
| """Glyph-index names in /Encoding/Differences must not leak as reading text. | ||
|
|
||
| Subset generators (FontForge, fontTools, mPDF, ...) name glyphs by bare | ||
| index: /gid00043, /g43, /glyph43. Such a name identifies the glyph inside | ||
| the embedded font program but carries no reading text. When neither a | ||
| /ToUnicode CMap nor the glyph-tables can resolve the code, keeping the name | ||
| fabricates plausible-looking garbage ('gid00043gid00049...') that downstream | ||
| quality gates cannot detect (docling-project/docling-parse#238). The parser | ||
| must emit a GLYPH marker instead. | ||
|
|
||
| Meaningful unknown names (custom ligatures like /Th) keep the existing | ||
| name-as-text fallback, and a valid /ToUnicode CMap stays authoritative. | ||
|
|
||
| These tests build the PDFs in memory: a Type1 font whose /Differences maps | ||
| codes 0x41-0x43 to gid-style names, drawn as the string (ABC). | ||
| """ | ||
|
|
||
| from io import BytesIO | ||
|
|
||
| from docling_parse.pdf_parser import DecodeConfig, DoclingPdfParser | ||
|
|
||
| _TOUNICODE = ( | ||
| "/CIDInit /ProcSet findresource begin\n" | ||
| "12 dict begin\n" | ||
| "begincmap\n" | ||
| "/CIDSystemInfo << /Registry (Adobe) /Ordering (UCS) /Supplement 0 >> def\n" | ||
| "/CMapName /Adobe-Identity-UCS def\n" | ||
| "/CMapType 2 def\n" | ||
| "1 begincodespacerange\n" | ||
| "<00> <FF>\n" | ||
| "endcodespacerange\n" | ||
| "3 beginbfchar\n" | ||
| "<41> <0043>\n" # C | ||
| "<42> <0049>\n" # I | ||
| "<43> <0041>\n" # A | ||
| "endbfchar\n" | ||
| "endcmap\n" | ||
| "CMapName currentdict /CMap defineresource pop\n" | ||
| "end\n" | ||
| "end" | ||
| ) | ||
|
|
||
|
|
||
| def _build_pdf(glyph_names: list, include_tounicode: bool) -> bytes: | ||
| differences = " ".join(f"/{name}" for name in glyph_names) | ||
| tounicode = " /ToUnicode 6 0 R" if include_tounicode else "" | ||
| content = "BT /F1 24 Tf 72 700 Td (ABC) Tj ET" | ||
| objects = [ | ||
| "<< /Type /Catalog /Pages 2 0 R >>", | ||
| "<< /Type /Pages /Kids [3 0 R] /Count 1 >>", | ||
| "<< /Type /Page /Parent 2 0 R /MediaBox [0 0 612 792] " | ||
| "/Resources << /Font << /F1 4 0 R >> >> /Contents 5 0 R >>", | ||
| "<< /Type /Font /Subtype /Type1 /BaseFont /XXXXXX+FakeSubset " | ||
| "/Encoding << /Type /Encoding " | ||
| f"/Differences [ 65 {differences} ] >>{tounicode} >>", | ||
| f"<< /Length {len(content)} >>\nstream\n{content}\nendstream", | ||
| ] | ||
| if include_tounicode: | ||
| objects.append( | ||
| f"<< /Length {len(_TOUNICODE)} >>\nstream\n{_TOUNICODE}\nendstream" | ||
| ) | ||
|
|
||
| out = b"%PDF-1.4\n" | ||
| offsets = [] | ||
| for index, obj in enumerate(objects, start=1): | ||
| offsets.append(len(out)) | ||
| out += f"{index} 0 obj\n{obj}\nendobj\n".encode("latin-1") | ||
|
|
||
| startxref = len(out) | ||
| out += f"xref\n0 {len(objects) + 1}\n".encode("latin-1") | ||
| out += b"0000000000 65535 f \n" | ||
| for offset in offsets: | ||
| out += f"{offset:010d} 00000 n \n".encode("latin-1") | ||
| out += ( | ||
| f"trailer\n<< /Size {len(objects) + 1} /Root 1 0 R >>\n" | ||
| f"startxref\n{startxref}\n%%EOF" | ||
| ).encode("latin-1") | ||
| return out | ||
|
|
||
|
|
||
| def _extract_text( | ||
| glyph_names: list, include_tounicode: bool, keep_glyphs: bool = True | ||
| ) -> str: | ||
| parser = DoclingPdfParser(loglevel="fatal") | ||
| config = DecodeConfig(keep_glyphs=keep_glyphs) | ||
| doc = parser.load( | ||
| path_or_stream=BytesIO(_build_pdf(glyph_names, include_tounicode)), | ||
| decode_config=config, | ||
| ) | ||
| _, page = next(doc.iterate_pages()) | ||
| return "".join(cell.text for cell in page.textline_cells) | ||
|
|
||
|
|
||
| def test_gid_names_without_tounicode_yield_glyph_markers(): | ||
| # No fabricated 'gid00043...' reading text: an unresolvable glyph-index | ||
| # name must surface as a GLYPH marker. | ||
| text = _extract_text(["gid00043", "gid00049", "gid00041"], include_tounicode=False) | ||
| assert "GLYPH<name:gid00043>" in text | ||
| assert "GLYPH<name:gid00049>" in text | ||
| assert "GLYPH<name:gid00041>" in text | ||
| assert "gid00043gid00049" not in text.replace("GLYPH<name:gid00043>", "") | ||
|
|
||
|
|
||
| def test_gid_name_variants_yield_glyph_markers(): | ||
| text = _extract_text(["g43", "glyph49", "index41"], include_tounicode=False) | ||
| assert "GLYPH<name:g43>" in text | ||
| assert "GLYPH<name:glyph49>" in text | ||
| assert "GLYPH<name:index41>" in text | ||
|
|
||
|
|
||
| def test_tounicode_stays_authoritative_over_gid_names(): | ||
| # PDF 32000-1 section 9.10.2: the /ToUnicode CMap is the first method; | ||
| # gid-style names never shadow it. | ||
| text = _extract_text(["gid00043", "gid00049", "gid00041"], include_tounicode=True) | ||
| assert text == "CIA" | ||
|
|
||
|
|
||
| def test_meaningful_unknown_names_keep_name_fallback(): | ||
| # Custom ligature names like /Th carry reading text; the existing | ||
| # name-as-text fallback stays. | ||
| text = _extract_text(["Th", "ft", "tt"], include_tounicode=False) | ||
| assert text == "Thfttt" | ||
| assert "GLYPH" not in text | ||
|
|
||
|
|
||
| def test_default_config_strips_gid_markers(): | ||
| # Production default (keep_glyphs=False, see config.h): the marker is | ||
| # stripped to a space in pdf_states/text.h, so neither GLYPH<...> nor | ||
| # the fabricated gid-name text ever reaches the output. | ||
| text = _extract_text( | ||
| ["gid00043", "gid00049", "gid00041"], | ||
| include_tounicode=False, | ||
| keep_glyphs=False, | ||
| ) | ||
| assert "GLYPH" not in text | ||
| assert "gid" not in text | ||
| assert text.strip() == "" |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should only be returned if
keep_glyphsin the config istrue(see https://github.com/docling-project/docling-parse/blob/main/src/parse/config.h#L62)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does respect
keep_glyphs— via the mechanism #231 added: anyGLYPH<...>string returned bypage_font.his stripped to a space inpdf_states/text.h(if((not config.keep_glyphs) and chars_.rfind("GLYPH<", 0) == 0)) whenkeep_glyphsis false, the same path the existing markers in this file use (GLYPH<c=...,font=...>,GLYPH<UNKNOWN>,GLYPH<numb>).page_fontitself has no access to the decode config, so gating at emission would mean plumbing the config through the resource layer; it would also breakinit_space_index(), which probesget_string(32) == " "to locate the space glyph.I've added an explicit test (
test_default_config_strips_gid_markers) asserting that with the default config neither the marker nor the fabricated gid text reaches the output. Happy to plumb the config through instead if you'd prefer the gate at emission.