Repository navigation
[US-1979] Add dump structure command for tagged-PDF structure trees - #37
Conversation
anovik
left a comment
There was a problem hiding this comment.
Nice work overall. The phase split, the stream memo and the number-tree walker read well. I found no blockers. The first two inline comments are worth fixing in this PR because they make the report wrong on valid files. The rest are minor.
Two nits outside the inline comments:
structure.go:784decodeStream: this always decodes again throughdecodeContentStreamLocked, even whenGetContentStreamhas already put the tokenized stream indoc.streamCache. Not filling the cache makes sense, but reading it (understreamMu) would skip the extra decode for streams the user has already opened.cmd_structure.go:71:r.HasStructTree || len(r.Elements) > 0can be justr.HasStructTree, becauseElementsis only filled when there is a StructTreeRoot.
| switch v := contents.(type) { | ||
| case pdfcpu_types.IndirectRef: | ||
| return []string{fmt.Sprintf("obj:%d:%d", v.GenerationNumber.Value(), v.ObjectNumber.Value())}, nil | ||
| return []pdfcpu_types.IndirectRef{v}, nil |
There was a problem hiding this comment.
An indirect /Contents can point to an array of stream refs (7.8.2 allows it, and some producers write it after incremental updates). This returns the ref to the array as if it were a stream ref. scanPage then gets "node is not a stream object", finds no marked content on that page, and every ParentTree and tree MCID row on the page gets content count 0 and *, even when the content is tagged correctly.
GetContentStream had the same limitation before this PR, but now the structure report depends on it too. pageWalker.contents in pageindex.go:583 already follows the reference to the array, so the two /Contents readers disagree on the same page. Could this follow the ref the same way, for example derefIn with an Array case under pdfMu, which every caller already holds?
There was a problem hiding this comment.
@anovik Fixed in 81af806. contentStreamRefs now takes the context and derefs a /Contents ref the same way pageWalker.contents does, so a ref to an array is read as the array. All callers already hold pdfMu. GetPageContentStream works on those pages now too. Test: TestGetStructure_ContentsReferenceToArray.
| e := ParentTreeEntry{Key: k} | ||
| v := w.parentTree[k] | ||
| if _, isArr := w.deref(v).(pdfcpu_types.Array); isArr { | ||
| e.Err = "value is an array, but no page's /StructParents names this key" |
There was a problem hiding this comment.
A Form XObject that contains structural marked content has its own /StructParents, and its ParentTree value is an array, the same as a page's (14.7.4.4). On such a file, this key is listed under "Other ParentTree keys" with error: value is an array, but no page's /StructParents names this key, so a valid file is reported as broken.
The PR description says Form XObject MCIDs aren't aligned, which is fine for now, but this error isn't covered by that. Could an unnamed array value be listed without Err (maybe with a note that a Form XObject may own it), or could Form XObject /StructParents be counted as names here? docs/cli/structure.md should match whichever you pick.
There was a problem hiding this comment.
@anovik Went with the first option in 81af806: an unnamed array value is listed without Err, with a new Note field saying it may belong to a Form XObject's /StructParents. Plain text prints it as note: ..., and docs/cli/structure.md says the same. Counting Form XObject /StructParents as names can come with Form XObject alignment later.
| propStart := operandStart(operands, n-1) | ||
| // With no closed dictionary last, a "<<" still open is the property list. | ||
| if operands[propStart].Value != "<<" { | ||
| if u := unclosedDictStart(operands); u > 0 { |
There was a problem hiding this comment.
u > 0 misses a << left open at operand 0. For << /MCID 0 BDC (the tag is missing), propStart stays at the 0, the tag becomes MCID, and the error is BDC property list operand "0" is not a dictionary or a name. With u >= 0, propStart becomes 0 and you get the intended "BDC has 1 of its 2 operands" row.
There was a problem hiding this comment.
| row.Tag = strings.TrimPrefix(operands[tagStart].Value, "/") | ||
| prop := operands[propStart:] | ||
| switch { | ||
| case prop[0].Type == "operator" && prop[0].Value == "<<": |
There was a problem hiding this comment.
When the property list is a << that is never closed (/P <</MCID 0 BDC ... EMC), inlineMCID still finds /MCID 0 and the row is clean (Err empty), so it counts as consistent. The MCID is fine to keep, but should the row also say the dictionary has no >>, so the malformed BDC shows up in the report?
There was a problem hiding this comment.
@anovik Yes. The row keeps its MCID and now also carries property-list dictionary has no closing >> (81af806). The content count, parent-MCID stack and alignment now key on MCID >= 0 instead of an empty Err, so the MCID still lines up. Tests: TestScanMarkedContentTokens_UnclosedPropertyListKeepsMCIDWithError, TestGetStructure_UnclosedPropertyListMCIDCountsInAlignment.
| w.problem(label, fmt.Sprintf("key %d outside /Limits [%d %d]", key, lo, hi)) | ||
| } | ||
| if havePrev && key <= prev { | ||
| w.problem(label, fmt.Sprintf("key %d after key %d: keys not in ascending order", key, prev)) |
There was a problem hiding this comment.
A key repeated inside one /Nums array (/Nums [0 a 0 b]) gets two problems: key 0 after key 0: keys not in ascending order here and key 0 already in node ...; first value kept at line 176. Checking key < prev here, or skipping this check when the key is already in keyNode, would report it once.
| if k.Kind != "error" && k.Kind != "element" && k.Err != "" { | ||
| line += " error: " + k.Err | ||
| } | ||
| b.WriteString(indent + asciiSafe(line) + "\n") |
There was a problem hiding this comment.
asciiSafe runs on the whole line, and elementLine has already quoted or escaped the text fields. An element with a non-ASCII /S (from a #xx role name) and an ASCII /Alt prints the whole line as one Go-quoted string, and the /Alt quotes come out as \"Chart\". Applying asciiSafe to the /S, MappedS and ref parts inside elementLine, the same way text does, would avoid the double escaping.
| var tb strings.Builder | ||
| _ = t.Render(&tb) | ||
| for line := range strings.SplitSeq(strings.TrimSuffix(tb.String(), "\n"), "\n") { | ||
| b.WriteString(" " + asciiSafe(strings.TrimRight(line, " ")) + "\n") |
There was a problem hiding this comment.
Same issue on the alignment table: one non-ASCII cell (for example a ParentTree element's /S) quotes the whole rendered row, and it no longer lines up with the header. Escaping each cell before t.AddRow would keep the columns aligned.
|
@anovik Thanks. All inline comments are addressed in 81af806, and both nits too:
|
Description and links
There was no way to see a tagged PDF's structure tree in pdfdebug. Checking
/S,/KMCIDs and/Pgper element, or whether a page's ParentTree slots point at the elements that claim its MCIDs, meantqpdf --jsonplus a script, or a one-off Go program on unipdf.This adds
dump structure [--json] [--pretty] <file>. It walks the StructTreeRoot, reads the ParentTree, scans each page's content streams for marked content, and lines the three up per MCID. CLI and pdfcore only; the GUI view comes later. It reports what it finds and gives no PDF/UA verdicts. Those rules are a separate change that will build on this report.Jira: US-1979
Technical changes
internal/pdfcore/structure.go(new):GetStructure(tabID)returns aStructureReport. Phase 1 runs under onepdfMuhold. It reads the page walk fromdump pagesfor page numbers, catalog/Langand/MarkInfo, RoleMap, ParentTreeNextKey, the/Kwalk, each page's/StructParentsand its ParentTree array, andOtherParentTree. Phase 2 takespdfMuonce per page, checksdocClosed, then decodes and scans that page's content streams. Elements, indirect/Karrays reached a second time, and a tree deeper than 256 levels become error kids. They are never skipped silently.internal/pdfcore/numbertree.go(new):walkNumberTree, the one number-tree walker (/Kids,/Nums,/Limits). It reports bad node shapes, keys outside/Limits, out-of-order or duplicate keys, cycles, and depth over 64.internal/pdfcore/markedcontent.go(new): the BDC/BMC/EMC scan over the existing tokenizer output. A page's streams are scanned as one stream: the open stack and pending operands carry across stream boundaries. The tag and property list are the last two operands beforeBDC. Named property lists are looked up in/Propertiesafter#xxdecoding.true/false/nullinside a property list don't break operand reading.internal/pdfcore/stream.go:decodeContentStreamLockedandcontentStreamRefsare pulled out so the walk can decode underpdfMuwithout touchingstreamCache. A stream shared by several pages is decoded once per walk and held only until its last page is scanned.GetContentStreambehaves as before./Kclaim it. Rows where these disagree are marked inconsistent. Other problems are listed too: pages that share a/StructParentskey, ParentTree keys no page uses, and a wrong/Typeon an element.dump structureincmd/cli/cmd_structure.go. Exit 0 for any file that opens (problems are in the output), 1 for usage errors, 2 for open or internal failures. Plain text is ASCII and clamped like the other dumps;--jsoncarries the full report.testdata/structure.pdf(clean) andtestdata/structure-broken.pdf(one planted fault of each kind), fromtestdata/generate_test.go. Depth and cycle cases are built in memory.tests/dump-structure.docs/cli/structure.md,docs/cli-usage.mdand a CHANGELOG entry under Unreleased.Considerations
pdfcpu validates the structure tree and number trees when it opens a file, and that validation recurses with no guard on a
/Kidscycle. That is a Go stack overflow, whichsafeCallcan't recover from. Our walker handles cycles, but a file with that shape still fails at open, before our code runs. The cycle tests use the unvalidated open helpers already inpageindex_test.go.MCIDs inside Form XObjects are not aligned. Only page content streams are scanned.
The scan decodes streams without filling
streamCache. Otherwise one structure dump would keep every page's decoded stream until the tab closes.How was this tested?
GOFLAGS=-count=1 scripts/test-all.shaftergo clean -testcache: go vet, golangci-lint 0 issues, the root module, everytests/*/suite includingtests/dump-structure, and frontend typecheck, lint and Vitest. All suites passed.go test -race -count=1 ./internal/pdfcore/... ./cmd/...passed./Kkid shape, close during the walk, andstreamCachestaying empty.ReadContextFileandqpdf --checkwith no warnings.qpdf --json. I'll add the excerpt to this PR.What could go wrong?
A file whose structure tree has a
/Kidscycle still crashes at open inside pdfcpu, as it does today with every other command.Plain-text layout isn't a contract, and the plain-text tests pin exact lines. Changing the format means updating those tests.
On a very large tagged file the per-page
pdfMuholds are short, but phase 1 holds the lock for the whole tree walk.Screenshots/videos (if appropriate)
Checklist
devbranch, notmaster