Stop letting map order decide what a page looks like

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

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)
+		}
+	}
+}