Make PDF output reproducible

[za3k]

Due to this code:

Pillow/src/PIL/PdfImagePlugin.py

Line 224 in 97a4d1f

existing_pdf.write_comment(f"created by Pillow {__version__} PDF driver")

It is not possible produce a bitwise identical PDF across versions. Would you accept an argument to set/disable this comment during generation?

[radarhere]

Could you explain why it is important to you to produce exactly the same file across versions? I'm concerned that this comment may not be your only obstacle to that goal, and that one day we might make a change like https://pillow.readthedocs.io/en/stable/releasenotes/9.0.0.html#switched-to-libjpeg-turbo-in-macos-and-linux-wheels again, and your images will change within the PDF.

Rather than adding a new argument, I'd personally be more interested in just removing the version string, and have 'created by Pillow PDF driver'. How does that sound?

[za3k]

Yes. The answer is "tests".

I have a backup/restore program which outputs PDFs in Pillow, and it is very easy to write tests if I can just check whether the output is literally the same hash as some expected output. (Currently I am doing this via monkey patching the Pillow code, which is not ideal.)

Of course this is not the only style of tests one can write, but it's a useful one.

Edit: Any such solution sounds great, I would have no objection to yours.

[radarhere]

I've created #9176

[hugovk]

The version was added back in PIL 1.1.1 from 2000, where it was a PdfImagePlugin-specific number:

https://github.com/hugovk/PIL/blame/645e19c47556b9086c0d0fdccc1db1b9e6f79f7f/PIL/PdfImagePlugin.py#L64

We replaced all the plugin-specific numbers with the Pillow version in #4197.

No big objections from me to removing it. Before we do, does the PDF spec say anything about adding the creator tool version? What do other popular tools do?

[radarhere]

https://opensource.adobe.com/dc-acrobat-sdk-docs/pdfstandards/PDF32000_2008.pdf states

Comments (other than the %PDF-n.m and %%EOF comments described in 7.5, "File Structure") have no semantics. They are not necessarily preserved by applications that edit PDF files.

So I don't think the spec places a lot of importance on them.

I think the more expected thing would be to use "/Creator"

Page 550 of that PDF shows

/Creator (Adobe FrameMarker 5.5.3 for Power Macintosh®)

GIMP and ImageMagick don't include any such data. Saving a PDF from macOS Preview, I see

/Creator (Preview)

[za3k]

Would it be reasonable to add a test of reproducible PDF output to the Pillow suite, as well? I agree stuff like images or fonts are too finicky, but something like a blank document? A test could provide safety rails against accidentally breaking anything.

It wouldn't have to be a "definitely don't break this" test, but at least one that mentions why, all else equal, it would be better not to break. My thought process is that this is the kind of effect on users most devs wouldn't actively think about while doing things like adding PDF comments.

[aclark4life]

Since we're doing work on the PDF plugin it sounds reasonable to add a test and test data, including a small PDF document.

[radarhere]

I think we already test the PDF output in

Pillow/Tests/test_file_pdf.py

Lines 192 to 332 in f9db7a3

	def test_pdf_open(tmp_path: Path) -> None:
	# fail on a buffer full of null bytes
	with pytest.raises(PdfParser.PdfFormatError):
	PdfParser.PdfParser(buf=bytearray(65536))
	# make an empty PDF object
	with PdfParser.PdfParser() as empty_pdf:
	assert len(empty_pdf.pages) == 0
	assert len(empty_pdf.info) == 0
	assert not empty_pdf.should_close_buf
	assert not empty_pdf.should_close_file
	# make a PDF file
	pdf_filename = helper_save_as_pdf(tmp_path, "RGB")
	# open the PDF file
	with PdfParser.PdfParser(filename=pdf_filename) as hopper_pdf:
	assert len(hopper_pdf.pages) == 1
	assert hopper_pdf.should_close_buf
	assert hopper_pdf.should_close_file
	# read a PDF file from a buffer with a non-zero offset
	with open(pdf_filename, "rb") as f:
	content = b"xyzzy" + f.read()
	with PdfParser.PdfParser(buf=content, start_offset=5) as hopper_pdf:
	assert len(hopper_pdf.pages) == 1
	assert not hopper_pdf.should_close_buf
	assert not hopper_pdf.should_close_file
	# read a PDF file from an already open file
	with open(pdf_filename, "rb") as f:
	with PdfParser.PdfParser(f=f) as hopper_pdf:
	assert len(hopper_pdf.pages) == 1
	assert hopper_pdf.should_close_buf
	assert not hopper_pdf.should_close_file
	def test_pdf_append_fails_on_nonexistent_file() -> None:
	im = hopper("RGB")
	with tempfile.TemporaryDirectory() as temp_dir:
	with pytest.raises(OSError):
	im.save(os.path.join(temp_dir, "nonexistent.pdf"), append=True)
	def check_pdf_pages_consistency(pdf: PdfParser.PdfParser) -> None:
	assert pdf.pages_ref is not None
	pages_info = pdf.read_indirect(pdf.pages_ref)
	assert b"Parent" not in pages_info
	assert b"Kids" in pages_info
	kids_not_used = pages_info[b"Kids"]
	for page_ref in pdf.pages:
	while True:
	if page_ref in kids_not_used:
	kids_not_used.remove(page_ref)
	page_info = pdf.read_indirect(page_ref)
	assert b"Parent" in page_info
	page_ref = page_info[b"Parent"]
	if page_ref == pdf.pages_ref:
	break
	assert pdf.pages_ref == page_info[b"Parent"]
	assert kids_not_used == []
	def test_pdf_append(tmp_path: Path) -> None:
	# make a PDF file
	pdf_filename = helper_save_as_pdf(tmp_path, "RGB", producer="PdfParser")
	# open it, check pages and info
	with PdfParser.PdfParser(pdf_filename, mode="r+b") as pdf:
	assert len(pdf.pages) == 1
	assert len(pdf.info) == 4
	assert pdf.info.Title == os.path.splitext(os.path.basename(pdf_filename))[0]
	assert pdf.info.Producer == "PdfParser"
	assert b"CreationDate" in pdf.info
	assert b"ModDate" in pdf.info
	check_pdf_pages_consistency(pdf)
	# append some info
	pdf.info.Title = "abc"
	pdf.info.Author = "def"
	pdf.info.Subject = "ghi\uabcd"
	pdf.info.Keywords = "qw)e\\r(ty"
	pdf.info.Creator = "hopper()"
	pdf.start_writing()
	pdf.write_xref_and_trailer()
	# open it again, check pages and info again
	with PdfParser.PdfParser(pdf_filename) as pdf:
	assert len(pdf.pages) == 1
	assert len(pdf.info) == 8
	assert pdf.info.Title == "abc"
	assert b"CreationDate" in pdf.info
	assert b"ModDate" in pdf.info
	check_pdf_pages_consistency(pdf)
	# append two images
	mode_cmyk = hopper("CMYK")
	mode_p = hopper("P")
	mode_cmyk.save(pdf_filename, append=True, save_all=True, append_images=[mode_p])
	# open the PDF again, check pages and info again
	with PdfParser.PdfParser(pdf_filename) as pdf:
	assert len(pdf.pages) == 3
	assert len(pdf.info) == 8
	assert PdfParser.decode_text(pdf.info[b"Title"]) == "abc"
	assert pdf.info.Title == "abc"
	assert pdf.info.Producer == "PdfParser"
	assert pdf.info.Keywords == "qw)e\\r(ty"
	assert pdf.info.Subject == "ghi\uabcd"
	assert b"CreationDate" in pdf.info
	assert b"ModDate" in pdf.info
	check_pdf_pages_consistency(pdf)
	def test_pdf_info(tmp_path: Path) -> None:
	# make a PDF file
	pdf_filename = helper_save_as_pdf(
	tmp_path,
	"RGB",
	title="title",
	author="author",
	subject="subject",
	keywords="keywords",
	creator="creator",
	producer="producer",
	creationDate=time.strptime("2000", "%Y"),
	modDate=time.strptime("2001", "%Y"),
	)
	# open it, check pages and info
	with PdfParser.PdfParser(pdf_filename) as pdf:
	assert len(pdf.info) == 8
	assert pdf.info.Title == "title"
	assert pdf.info.Author == "author"
	assert pdf.info.Subject == "subject"
	assert pdf.info.Keywords == "keywords"
	assert pdf.info.Creator == "creator"
	assert pdf.info.Producer == "producer"
	assert pdf.info.CreationDate == time.strptime("2000", "%Y")
	assert pdf.info.ModDate == time.strptime("2001", "%Y")
	check_pdf_pages_consistency(pdf)

It wouldn't have to be a "definitely don't break this" test, but at least one that mentions why, all else equal, it would be better not to break.

Just for the record, I don't think automated testing really makes that distinction.