Repository navigation
feat(pdf-codec): leave document-level surfaces out of a read - #1693
Merged
Merged
Conversation
ReadPdfOptions.surfaces takes one boolean per document-level surface (outline, destinations, form, layers, structure, residue), each read unless set to false, on a full read or a page-selected one. A skipped surface's reader is never entered, its field is absent from the result, and the result names it in skippedSurfaces, so "not read" can be told apart from "none". Page items lose only what depended on a skipped surface: internal links without destinations, layer names without layers, structure owners without the structure tree. writePdf refuses a layout carrying skippedSurfaces with PdfSkippedSurfacesError. Refs #1692
…nversions The pdfTo* conversions, convertDocument with a pdf source and readNativeDocumentTree/readContentDocument take a surfaces option, handed to the same first hop pages goes to, so an intermediate pdf a route renders is always read whole. readDocumentMetadata now reads a pdf with every document-level surface skipped as well as no page, and returns the same metadata a full read does. openPdf takes neither option. ReadPdfSurfaces, LayoutDocumentSurface and PdfSkippedSurfacesError are exported from the root barrel. Refs #1692
…ations The reader names each direct destination array with the first unused destN in the order it meets them, and the outline is read before any page, so leaving the outline out renumbers the names minted for page links and drops the entries only the outline minted. The surfaces doc comment and README said the outline was page-item neutral; they now state the renumbering, and the surfaces fixture carries a direct-array outline destination so the per-surface comparison and an explicit renumbering test cover it. Opening the document is named among what an every-surface-skipped read still does. Refs #1692
… change The surfaces section said a tree read with surfaces skipped rebuilds to pdf without them, but the rebuild never re-emits the outline, destinations, form, layers, structure or residue in any case. It now names the two differences that do reach a rebuild (hidden-layer text drawn visibly, geometry-inferred blocks), lists the structure definitions and form contentControl constructs among what a skip leaves out, and states that skipping the outline renumbers the minted destination names internal link anchors carry. Refs #1692
…sidue skipped The surfaces fixture's XMP packet was empty, so nothing showed that a metadata read with every surface skipped still fills fields /Info does not carry from the XMP packet. A pdf whose title and author live only in XMP now reads the same metadata through readDocumentMetadata as through a full read. Refs #1692
The destination registry comment said a direct array shared by a link and a bookmark mints one destination. intern mints a fresh entry for every direct array with no identity or value check, so one indirect array both reference mints two entries naming the same target. The comment now says what the code does; behaviour is unchanged. Refs #1692
…ne document The outline case's link-target normalisation took a document and a separate source for its pages and destinations, so the expected side's own pages were silently replaced by the full read's. It now normalises one document, and the expected side is the full read normalised with its outline removed. Refs #1692
The surfaces section said a rebuild draws the PDF from the tree's metadata, pages and images alone, but it also emits the positioned text, vectors, formulas and URI links and uses the tree's embedded fonts. It now says so, states the skippedSurfaces refusal before the rebuild sentences and that a rebuild is never refused, and notes that a skipped outline drops the destN entries only it minted from the tree's destinations table. Refs #1692
…xactly The helper comment said linked destinations are keyed by what they resolve to; they are an ordered list of resolved targets compared in order. The comment now says so and why the order matches in both reads. Refs #1692
…a rebuild draws The outline is not a table of its own: its items are outline-N entries in the destinations table, so skipping the outline removes those entries rather than a table. The rebuild's content list now includes table-cell rectangles and border lines and sheet-cell rectangles, and says a URI link is re-emitted only when it is attached to a recovered run. Refs #1692
The surfaces doc comments spoke of outline or bookmark tables, but the outline lives as outline-N entries in the tree's destinations table. They now list what each skip removes, matching the README. The README adds the border lines a sheet cell rebuilds with and says a read outline's outline-N entries stay, without a destination, when only destinations is skipped. Refs #1692
Mearman
marked this pull request as ready for review
October 17, 2026 06:58
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
This security review couldn’t start because the paying account has exhausted its Codex allowance and has insufficient available credits. The payer can check their balance and add credits in the Codex usage dashboard, then try again. If you do not manage the paying account, contact this repository's admins. |
Contributor
|
🎉 This PR is included in version 14.6.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
readPdftakes asurfacesoption: one boolean per document-level surface (outline,destinations,form,layers,structure,residue), each read unless set tofalse. It is independent ofpages, so it works on a full read too. A skipped surface's reader is never entered, its field is left off theLayoutDocument, and the result lists it inskippedSurfaces, so a caller can tell "not read" from "none" the same waypageCountalready covers attachments.Page items only lose what depended on the skipped surface. Without
destinations, internal links are dropped (URI links stay) and outline entries carry nodestination. Withoutlayers, no item carrieslayer. Withoutstructure, no item carries astructureowner.formandresiduedon't touch page items, apart from residue taking thepage-boxesrow with it.outlinedoesn't remove anything from page items, but it does rename things: the reader names each direct destination array with the first unuseddestNin the order it meets them, and the outline is read before any page, so skipping it renumbers the names minted for page links (and drops the entries only the outline minted). Each link still resolves to the same target. I left the minting order alone, since changing it would change full-read output. The rules are in theReadPdfOptions.surfacesdoc comment and the pdf-codec README.writePdfrefuses a layout carryingskippedSurfaceswith a newPdfSkippedSurfacesError(checked afterPdfPartialDocumentError, so a page-selected read that also skipped surfaces still reports the selection).documents.js threads
surfacesthrough the pdfTo* conversions,convertDocument(first hop with a pdf source only, the same aspages) andreadNativeDocumentTree/readContentDocument.readDocumentMetadatanow reads a pdf with every surface skipped as well aspages: [], and returns the same metadata.openPdftakes neither. Two of these change reconstruction, and the README says so: withstructureskipped a tagged PDF's headings and tables are inferred from geometry, and withlayersskipped hidden-layer text is kept as if it were visible.The tests use a fixture where every surface carries a trap, a reference to an object the xref doesn't hold, so a parse of that surface reports a diagnostic naming that object. A skipped surface's trap never fires, and the reader spies confirm the reader isn't called. For each surface, the skipped read is compared to the full read with exactly the stated losses taken out, which covers every page item field by field. For the outline the comparison resolves link destinations to their targets instead of comparing minted names, and a separate test asserts the renumbering itself on an outline entry with a direct destination array.
Local observation, not a benchmark: peak RSS growth (
process.resourceUsage().maxRSSafter the read minus before it, file already in memory, one node 24.21.0 process per run, three runs each, on a heavily loaded shared Mac).A 25 MB, 638 page tagged PDF:
pages: []: 206 to 210 MiBpages: [0, 1, 2, -1]: 212 MiBpages: [0, 1, 2, -1],structureskipped only (two runs): 50 MiBpages: [0, 1, 2, -1], every surface skipped: 44 to 45 MiBpages: [], every surface skipped: 41 MiBA 25 MB, 205 page file with a much smaller structure tree, for comparison: 35 to 38 MiB for
pages: [], 43 to 44 MiB forpages: [0, 1, 2, -1], 34 to 37 MiB with every surface skipped as well, and 25 to 31 MiB forpages: []with every surface skipped.So on the tagged file almost all of a page-selected read's cost was the structure tree. What's left with every surface skipped is 41 to 45 MiB on the tagged file and 25 to 37 MiB on the other, the lower end in each case being
pages: []. Measured the same way, opening the document (openPdfDocument: reading the cross-reference data by following the/Prevchain of tables or streams, which is bounded and stops at a repeated offset, then the trailer, the/Encryptdictionary when there is one, and the catalog; on that normal path an object stream is decoded only when the catalog or/Encryptlives in one, while a broken cross-reference falls back to a recovery scan that parses every object in the file and decodes every object stream. Opening either test file raised nopdf/xref-recovereddiagnostic, so neither took the recovery scan) accounts for about 33 MiB of that on the tagged file and about 7 MiB on the other. Walking the page tree adds a further 1 to 4 MiB and 12 to 14 MiB respectively.Refs #1692