From 4b968ad61920769e6d6ebdcc885b6636ff8dccbb Mon Sep 17 00:00:00 2001 From: tannevaled Date: Thu, 27 Aug 2026 14:41:41 +0200 Subject: [PATCH] inline images: stop letting map order decide what a page looks like MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An inline image dictionary may say the same thing twice and disagree with itself: /W 20 beside /Width 10, /CS /RGB beside /ColorSpace /3chanRGB, /D beside /Decode with the components reversed. Expanded() walked the dictionary in one pass and wrote each key into the output, so when both spellings were present the winner was whichever Go's map iteration happened to yield last — and that order is deliberately randomised per range. The same file therefore drew a different picture on different runs of the same binary, with nothing to tell a caller which one was meant. Eight renders of safedocs' Inline_Image_Abbreviations fixture: before 79b9edef 79b9edef b2594aef 6bf89747 6bf89747 544c8e6b 79b9edef 43030bf3 after 87f31291 × 8 Five different answers out of eight, against one. The scope is now known rather than guessed. Rendering the first page of 3 668 corpus files three times over — the 1 633 real forms, 1 400 arXiv papers and all 635 vendor fixtures — turns up exactly one file that disagrees with itself, and it is that fixture, which safedocs built for this case: its seven images each pair an abbreviation against the written-out name, with comments marking the line to remove "to see the effect". So the corpus measurements already taken are not contaminated; one file is, and it is the one designed to be. WHICH SPELLING WINS The specification permits both in an inline image and says nothing about which takes precedence, and the public implementations disagree: pdf.js asks for the abbreviation and falls back to the written-out name, MuPDF asks the other way round. The abbreviation wins here, for two reasons — it is the spelling Table 93 gives for an inline image, the written-out name being the tolerated alias; and safedocs marks the abbreviation as the line whose removal changes what a viewer shows, which is a statement about the reference viewer's behaviour. Two passes settle it: the written-out names first, then the abbreviations over them. No order within either pass can matter, because the names in each are distinct — so the answer no longer depends on the run whichever way it is read. The test expands the same dictionary two hundred times and requires every answer to match the first. Against the parent commit it fails on the first comparison, reporting Height = 40 where the first gave 10. Co-Authored-By: Claude Opus 5 --- content.go | 53 ++++++++++++++++++++++++++++++++++++++++--------- content_test.go | 47 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 91 insertions(+), 9 deletions(-) diff --git a/content.go b/content.go index 91c4f44..d22b6c2 100644 --- a/content.go +++ b/content.go @@ -48,24 +48,59 @@ var inlineColourSpaces = map[Name]Name{ // Expanded returns the image's dictionary with the abbreviated keys and colour // space names written out, so it can be read like any image XObject. +// +// A dictionary may say the same thing twice and disagree with itself: /W 20 +// beside /Width 10, /CS /RGB beside /ColorSpace /3chanRGB. The specification +// permits both spellings in an inline image and says nothing about which wins, +// and the public implementations disagree — pdf.js asks for the abbreviation +// and falls back to the written-out name, MuPDF asks the other way round. +// +// The abbreviation wins here. Two reasons: it is the spelling Table 93 gives +// for an inline image, the written-out name being the tolerated alias; and +// safedocs' own fixture for this case marks the abbreviation as the line to +// remove "to see the effect", which is a statement about what the reference +// viewer does. +// +// What is not defensible is the answer changing between runs. Expanding in one +// pass over the map made the winner depend on Go's map iteration order, which +// is deliberately random: the same file drew differently each time it was +// opened, with no way for a caller to notice. Two passes settle it — the +// written-out names first, then the abbreviations over them — and no order +// within either pass can matter, because the names in each are distinct. func (im *InlineImage) Expanded() Dict { out := Dict{} for k, v := range im.Dict { - if long, ok := inlineKeys[k]; ok { - k = long + if _, abbreviated := inlineKeys[k]; abbreviated { + continue } - if k == "ColorSpace" { - if n, ok := ToName(v); ok { - if long, ok := inlineColourSpaces[n]; ok { - v = long - } - } + out[k] = expandColourSpace(k, v) + } + for k, v := range im.Dict { + long, abbreviated := inlineKeys[k] + if !abbreviated { + continue } - out[k] = v + out[long] = expandColourSpace(long, v) } return out } +// expandColourSpace writes out an abbreviated colour space name, which is the +// one value that is abbreviated as well as its key. +func expandColourSpace(k Name, v Object) Object { + if k != "ColorSpace" { + return v + } + n, ok := ToName(v) + if !ok { + return v + } + if long, ok := inlineColourSpaces[n]; ok { + return long + } + return v +} + // A ContentScanner walks a content stream one operation at a time. A stream // with rubbish in it yields the operations around the rubbish rather than // nothing at all, which is what a renderer needs; [ContentScanner.Err] reports diff --git a/content_test.go b/content_test.go index b3200d7..a0964c5 100644 --- a/content_test.go +++ b/content_test.go @@ -297,3 +297,50 @@ func TestOperandThatIsNotAnObject(t *testing.T) { t.Fatalf("got %+v", ops) } } + +// TestInlineImageExpandedIsDeterministic pins the one property that matters +// more than which spelling wins: that the answer is the same every time. +// +// safedocs' Inline_Image_Abbreviations fixture carries seven images that each +// say the same thing twice and disagree — /W 20 beside /Width 10, /CS /RGB +// beside /ColorSpace /3chanRGB. Expanding in one pass over the map let Go's +// randomised iteration order pick the winner, so the same page drew a +// different picture on different runs of the same binary. Two hundred +// expansions of the same dictionary is enough to catch that: against the one +// pass this loop sees both answers within the first few. +func TestInlineImageExpandedIsDeterministic(t *testing.T) { + im := &InlineImage{Dict: Dict{ + "W": Integer(20), + "Width": Integer(10), + "H": Integer(10), + "Height": Integer(40), + "BPC": Integer(8), + "CS": Name("RGB"), + "ColorSpace": Name("3chanRGB"), + "I": Bool(false), + }} + first := im.Expanded() + for i := 0; i < 200; i++ { + got := im.Expanded() + if len(got) != len(first) { + t.Fatalf("expansion %d has %d keys, the first had %d", i, len(got), len(first)) + } + for k, v := range first { + if got[k] != v { + t.Fatalf("expansion %d gave %s = %v, the first gave %v", i, k, got[k], v) + } + } + } + // And the abbreviation is what wins, which is the choice this makes and + // the reason it is written down in Expanded's own comment. + for k, want := range map[Name]Object{ + "Width": Integer(20), + "Height": Integer(10), + "ColorSpace": Name("DeviceRGB"), + "Interpolate": Bool(false), + } { + if first[k] != want { + t.Errorf("%s = %v, want %v", k, first[k], want) + } + } +}