Skip to content

fix(pdf): share short destination names and keep short view arrays - #1325

Merged
KnifeLemon merged 1 commit into
mainfrom
fix/pdf-merge-followups
Nov 10, 2026
Merged

KnifeLemon merged 1 commit into
mainfrom
fix/pdf-merge-followups

Conversation

@SeoJCalive

Copy link
Copy Markdown
Collaborator

Summary

Two follow-ups from the review of #1227 (PDF merge keeps bookmarks, links and form fields, #965). Only PdfMergeStructure.cs and its tests change.

  • Shared destination names from 32 bytes. A name under 256 bytes was written again at every link that used it. Names of 32 bytes and longer are now written once as an indirect string.
  • Destinations with missing trailing values are kept. [p /XYZ 10 20] or [p /FitH] used to be dropped. The missing values are now filled with null.

What you suggested

  • In your follow-up comment you pointed out that one 255-byte name behind many small links can make the output several times the input, and suggested lowering the sharing threshold to about 32 bytes.
  • In your first review you listed destinations with too few values, such as [p /XYZ 10 20] and [p /FitH], as being dropped.

Choices I made, for your opinion

  • 32 bytes, counted on the final name, boundary included. The check uses the written bytes after a -2 suffix and after the UTF-16 BOM for Unicode names, so a 31-byte name that becomes name-2 is shared. A shared string adds an object, a cross-reference entry and a reference at each use, so for very short names it saves little; I kept names under 32 bytes direct. Would you prefer a different value?
  • The caches keep 256. The old constant also decided when string lengths and lookups are cached by string instance, and when a repeated name-tree key is skipped. Those caches compare instances, and short strings can be shared instances such as string.Empty, so I split the constant: SharedNameBytes = 32 for the output, CacheLength = 256 for the caches. Is this split fine with you, or would you rather lower both?
  • The name tree uses the same shared string. A shared name is now also the key in /Names /Dests, so the name is not written twice. Should the tree keys stay direct strings instead?
  • Only missing trailing values are filled. null keeps the current value of that parameter (ISO 32000-1, 12.3.2.2, Table 151), so /XYZ with 0–2 values and /FitH /FitV /FitBH /FitBV without their value are filled. /FitR still needs all four numbers, and arrays with too many values or a value that is not a number or null are still dropped. Is filling the right default here, or would you rather keep dropping them?

What changed

  • PdfMergeStructure.cs: split SharedLength into SharedNameBytes and CacheLength; MapDestination fills missing trailing values with null; the name tree writes the registered name object.
  • One existing assertion changed: the destination-count case of Broken_items_are_dropped_and_their_neighbours_kept used [p /XYZ 1] as a broken destination and expected it to be dropped. It now expects [p /XYZ 1 null null]. The other broken cases are unchanged.
  • New tests cover the 31/32/33-byte boundary (raw and Unicode), names that cross the boundary through a suffix, shared short string instances, every padded and rejected destination shape on links, bookmarks and both name dictionaries, the string budget spent once per name, and a long repeated key.

Validation

All CLI runs compare the main build (fd842fb3) with this branch, each in its own FILEE_DATA_DIR, on generated PDFs and on four real pairs (LibreOffice, reportlab, Chrome, Ghostscript). The branch was then rebased onto 9a593de3; main had not changed the merge code since fd842fb3, and the build and PDF tests were run again after the rebase.

Input (two files merged) Inputs main output This branch
255-byte name, many small links 2,177,892 B 7,061,412 B 2,121,510 B
128-byte name 2,177,638 B 4,521,158 B 2,121,256 B
33-byte name 2,177,448 B 2,620,968 B 2,121,066 B
32-byte name 2,177,446 B 2,600,966 B 2,121,064 B
31-byte name (stays direct) 2,177,444 B 2,580,964 B 2,580,964 B
Real pairs (4) same same size, same structure
  • Every output was reopened: no link lost its target, name bytes and pages are unchanged.
  • Many names just under and over 32 bytes (up to 10,000 names × 20 links) and long repeated keys (up to 65,536 bytes × 10,000 references), three runs each: time and peak memory stay within 30% of main (largest case about 16 s and 1.0 GB on both), and outputs are the same size or smaller.
  • PdfMergeStructureTests: 183 passed.
  • Mutations: reverting each change one at a time (16 cases, such as sharing from 256 bytes again, counting characters instead of bytes, re-spending the budget at each link, dropping short arrays again, or padding with 0) makes at least one new test fail.
  • The build adds no warnings (none in the changed files), dotnet format is clean, and Core and Cli tests pass. In Engines and App, the only failures are tests that also fail on main on this Linux machine (LibreOffice, system fonts, the GNOME socket), plus one annotation font test that failed only during the full App run and passes on its own on both main and this branch.

Not in this PR

  • The other follow-ups from your review (push-button /A, link /Next, /URI and /Named bookmarks, /CO) and the page editor's Save.
  • While comparing the LibreOffice pair, I saw that the page box is rounded to three decimals on merge (595.303937… → 595.304). main does the same, so I left it.

Follow-up to the review of #1227 (#965). A destination name under 256
bytes was written again at every link that used it, so one 255-byte name
behind many small links made the merged file several times larger than
its inputs. Names are now written once as a shared indirect string from
32 bytes, and the name tree uses the same object. The 256-character
limit stays for the measure and lookup caches, which key on string
instances.

A destination with trailing view values left out, such as [p /XYZ 10 20]
or [p /FitH], was dropped. The missing values are now filled with null,
which keeps the current view. /FitR still needs all four numbers, and
arrays with too many values or a value that is not a number are still
dropped.

Co-authored-by: sisyphus-dev-ai <sisyphus-dev-ai@users.noreply.github.com>

@KnifeLemon KnifeLemon left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

꼼꼼하게 검증해 주셔서 고마워요. 질문 주신 네 가지에 답할게요.

1. 32바이트 기준
좋아요. 최종 바이트(접미사, BOM 포함)로 세는 방식도 맞아요. 31바이트 이름이 그대로 남는 대신 공유 객체 비용이 안 드는 것도 표에서 잘 보여요.

2. SharedNameBytes / CacheLength 분리
이렇게 나누는 게 맞아요. 캐시는 문자열 인스턴스로 비교하니까, 짧은 문자열까지 넣으면 string.Empty 같은 공유 인스턴스가 섞일 수 있어요. 둘은 이유가 다른 값이라 상수도 따로 두는 게 읽기 쉬워요.

3. 이름 트리 키에도 공유 문자열 사용
그대로 가도 괜찮아요. 이름 트리 키를 간접 객체로 두는 걸 막는 규정은 없고, pdf.js·PDFium·MuPDF 모두 키를 읽을 때 참조를 따라가요. 다시 열어서 링크가 다 살아 있는 것도 확인해 주셨으니 문제없어요.

4. 뒤쪽 값만 null로 채우기
좋은 기본값이에요. null은 "현재 값 유지"라서 원래 문서가 뜻한 동작과 같고, /FitR처럼 값이 다 있어야 하는 경우와 값이 넘치거나 숫자가 아닌 경우는 계속 버리는 것도 맞아요.

CI 모두 통과했으니 머지할게요. 나머지 후속 항목(push-button /A, /Next, /URI·/Named 북마크, /CO)은 따로 PR로 주시면 돼요.

@KnifeLemon
KnifeLemon merged commit 4be2f96 into main Nov 10, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants