fix(api): resolve page boxes as corner pairs, not origin plus size - #88
Open
cloudyjerry wants to merge 1 commit into
Open
fix(api): resolve page boxes as corner pairs, not origin plus size#88cloudyjerry wants to merge 1 commit into
cloudyjerry wants to merge 1 commit into
Conversation
A PDF box entry is `[llx lly urx ury]` — two opposite corners — but `getBox()` returned entries 2 and 3 as `width` and `height`. For the common origin-zero box that happens to be right, which is why it went unnoticed; for any box with a non-zero origin the reported size is too large by exactly the origin. A page with `/MediaBox [10 25 380 550]` measured 380 x 550 instead of 370 x 525. The mislabeling also escaped the accessors: `embedPage()` writes a Form XObject BBox as `[x, y, x + width, y + height]`, so embedding such a page produced a bounding box of `[10 25 390 575]` — a box grown by its own origin. Three related resolution gaps are fixed alongside it, because they decide the same number and a caller cannot get a correct page size while any of them is wrong: - Corner order is no longer trusted. The spec does not require the first corner to be the lower-left one, and `[512 692 20 30]` describes the same region as `[20 30 512 692]`. - MediaBox, CropBox and Rotate are inheritable page attributes (ISO 32000-2, 7.7.3.4), but were read from the page dictionary only, so a document that carries them on a `/Pages` node fell back to US Letter and no rotation. The doc comment on `getMediaBox()` already claimed this worked. The walk stops at the nearest ancestor that *has* the entry rather than the nearest truthy one, so an explicit `/Rotate 0` under a rotated parent keeps the page unrotated, and it is depth-capped because `/Parent` is file-supplied and can be circular. - `width`/`height` now measure the CropBox intersected with the MediaBox. A CropBox may crop a page down but never extend it (ISO 32000-2, 14.11.2); the previous check picked one box or the other whole, which is wrong whenever the two only partly overlap. Expected values in the new tests are measured against PDFium rather than derived by hand: 12 of the 13 cases match it exactly. The 13th is a CropBox sharing no area with the MediaBox, where PDFium reports a 0 x 0 page; this reports the MediaBox instead, so that a degenerate box cannot push a zero into every dimension a caller computes from it.
Contributor
|
@cloudyjerry is attempting to deploy a commit to the mythie's projects Team on Vercel. A member of the Team first needs to authorize it. |
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.
Fixes #81
What this fixes
A PDF box entry is
[llx lly urx ury]— two opposite corners — butgetBox()returned entries 2 and 3 as
widthandheight. For an origin-zero box that isaccidentally correct, which is why it went unnoticed; for any box with a non-zero
origin the size is too large by exactly the origin.
While measuring that against a reference implementation I found three further
resolution steps in the same code path that are missing or wrong. They are in one
PR because they decide the same number — a caller cannot get a correct page size
while any of them is wrong, and fixing them separately would mean three rounds of
the same tests.
corner to be the lower-left one, so
[512 692 20 30]describes the sameregion as
[20 30 512 692]. Before this, a MediaBox written[612 792 0 0]gave a page size of
0 x 0./MediaBox,/CropBoxand/Rotateare now resolved through the pagetree. They are inheritable page attributes (ISO 32000-2, 7.7.3.4) but were
read from the page dictionary only, so a document carrying them on a
/Pagesnode fell back to US Letter at rotation 0. The doc comment on
getMediaBox()already claimed this worked. The walk stops at the nearest ancestor that
has the entry rather than the nearest truthy one — an explicit
/Rotate 0under a rotated parent keeps the page unrotated — and it is depth-capped,
because
/Parentis file-supplied and can be circular in a malformeddocument.
BleedBox/TrimBox/ArtBoxare not inheritable and still readfrom the page only.
width/heightmeasure the CropBox intersected with the MediaBox. ACropBox may crop a page down but never extend it (ISO 32000-2, 14.11.2). The
previous check picked one box or the other whole, which is wrong whenever the
two only partly overlap.
The mislabeling also escaped the accessors:
embedPage()writes a Form XObjectBBoxas[x, y, x + width, y + height], so embedding a page with/MediaBox [10 25 380 550]produced[10 25 390 575]— a box grown by its ownorigin. That is covered by a test here.
Where the expected values come from
They are measured, not derived. Each case was built as a minimal PDF and run
through PDFium (
pypdfium25.12.1 / pdfium 152.0.7947.0), readingFPDF_GetPageWidthF/HeightFand cross-checking against the size of the bitmapit rasterizes. Page size as
width x height:/MediaBox [0 0 612 792]/MediaBox [10 25 380 550]/MediaBox [0 0 612 792] /CropBox [20 30 512 692]/MediaBox [0 0 612 792] /CropBox [512 692 20 30]/MediaBox [612 792 0 0]/MediaBox [0 0 612 792] /CropBox [-10 -10 700 900]/MediaBox [0 0 612 792] /CropBox [-50 100 400 900]/MediaBox [0 0 612 792] /CropBox [700 900 800 1000]/MediaBox [0 0 612 792] /CropBox [20 30 512 692] /Rotate 90/MediaBox [0 0 612 792] /CropBox [20 30 512 692] /Rotate 180/MediaBox [0 0 612 792] /CropBox [20 30 512 692] /Rotate 270/MediaBox [0 0 792 612] /Rotate 90on the parent/Rotate 0on the page,/Rotate 90on the parent† The one deliberate divergence. For a CropBox sharing no area with the
MediaBox, PDFium reports a
0 x 0page and refuses to rasterize it. This reportsthe MediaBox instead, on the grounds that a library returning
0propagates itinto every dimension a caller computes, and there is no correct answer for
degenerate input anyway. Happy to switch to strict PDFium parity if you would
rather — it is one line, and I have no strong claim here beyond "a zero is worse
than a fallback".
‡ Matches by coincidence, not agreement: 0.4.1 arrives at
612 x 792from the USLetter fallback at rotation 0, PDFium from an inherited
792 x 612box turned 90degrees.
getMediaBox()androtationdisagree on that row even though the sizedoes not, so the test for it asserts the box and the rotation rather than just the
size.
Compatibility
This changes returned values, which is the point, but worth stating plainly:
getMediaBox()/getCropBox()/getBleedBox()/getTrimBox()/getArtBox()return a different
width/heightfor any box with a non-zero origin orunordered corners. Origin-zero boxes with ordered corners — the overwhelming
majority — are unchanged.
page.width/page.heightchange for those same pages, plus pages thatinherit their attributes and pages whose CropBox partly overhangs.
embedPage()produces a correctedBBoxfor such pages.Test plan
bun run test:run,bun run typecheck,bun run lintall pass.50 tests added in a new
src/api/pdf-page.test.ts; no existing test changedstatus. 32 of the 50 fail against
main— the other 18 are origin-zero andalready-correct cases, kept as regression guards so a future refactor cannot
reintroduce the bug from the other direction.
AI assistance disclosure
Per CONTRIBUTING, this contribution was written with AI assistance and here is
the disclosure.
What the AI did. Wrote the implementation in
src/api/pdf-page.tsand thetests in
src/api/pdf-page.test.ts, and built the PDFium measurement harnessused to derive the expected values.
The prompts. These are the instructions I gave, in substance. They are
paraphrased rather than pasted verbatim, because the originals referenced the
private codebase where I hit the bug and those details are not relevant to this
repository; nothing technical has been added or removed in the paraphrase.
The instruction to measure rather than assume changed two rows: the degenerate
CropBox case (PDFium reports
0 x 0, not the MediaBox fallback we had assumed)and the inherited case (where the page size matches by coincidence and only the
box and rotation reveal the defect). Both are called out above rather than
smoothed over.
Review. I have read and understand every line of this diff, including the
test expectations and the one place where I chose to diverge from PDFium; I am
submitting it as my own work and can answer for any part of it.