Repository navigation
fix(pdf): share short destination names and keep short view arrays - #1325
Merged
Merged
Conversation
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
approved these changes
Nov 10, 2026
KnifeLemon
left a comment
Owner
There was a problem hiding this comment.
꼼꼼하게 검증해 주셔서 고마워요. 질문 주신 네 가지에 답할게요.
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로 주시면 돼요.
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
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.
Summary
Two follow-ups from the review of #1227 (PDF merge keeps bookmarks, links and form fields, #965). Only
PdfMergeStructure.csand its tests change.[p /XYZ 10 20]or[p /FitH]used to be dropped. The missing values are now filled withnull.What you suggested
[p /XYZ 10 20]and[p /FitH], as being dropped.Choices I made, for your opinion
-2suffix and after the UTF-16 BOM for Unicode names, so a 31-byte name that becomesname-2is 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?string.Empty, so I split the constant:SharedNameBytes = 32for the output,CacheLength = 256for the caches. Is this split fine with you, or would you rather lower both?/Names /Dests, so the name is not written twice. Should the tree keys stay direct strings instead?nullkeeps the current value of that parameter (ISO 32000-1, 12.3.2.2, Table 151), so/XYZwith 0–2 values and/FitH/FitV/FitBH/FitBVwithout their value are filled./FitRstill needs all four numbers, and arrays with too many values or a value that is not a number ornullare still dropped. Is filling the right default here, or would you rather keep dropping them?What changed
PdfMergeStructure.cs: splitSharedLengthintoSharedNameBytesandCacheLength;MapDestinationfills missing trailing values withnull; the name tree writes the registered name object.destination-countcase ofBroken_items_are_dropped_and_their_neighbours_keptused[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.Validation
All CLI runs compare the
mainbuild (fd842fb3) with this branch, each in its ownFILEE_DATA_DIR, on generated PDFs and on four real pairs (LibreOffice, reportlab, Chrome, Ghostscript). The branch was then rebased onto9a593de3;mainhad not changed the merge code sincefd842fb3, and the build and PDF tests were run again after the rebase.mainoutputmain(largest case about 16 s and 1.0 GB on both), and outputs are the same size or smaller.PdfMergeStructureTests: 183 passed.dotnet formatis clean, and Core and Cli tests pass. In Engines and App, the only failures are tests that also fail onmainon 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 bothmainand this branch.Not in this PR
/A, link/Next,/URIand/Namedbookmarks,/CO) and the page editor's Save.maindoes the same, so I left it.