From 4dc168f2d2b39675c12c215902df5e45ccf439c8 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Sun, 6 Sep 2026 17:01:04 +0200 Subject: [PATCH 1/2] feat(pdf): append an incremental update MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `IncrementalWriter` pipes the source through untouched and writes the objects it collected after it, under a cross-reference section naming only their ids and a trailer chaining back through `/Prev`. Nothing is re-serialized, so every read-side gap — an unmodelled key, a filter we pass through, an object stream we never recompressed — survives by being copied rather than rewritten. It matches the file's own cross-reference flavor, a classic table or a stream that mints an id and an entry for itself. A recovered file and an encrypted one are refused in the constructor, which is also where the two facts the rest of the writer needs stop being optional. `/ID[1]` comes from the update's own bytes rather than a clock, so the same update written twice gives the same file. Verified plumbing-first: a no-op update that re-parses identically, then a `/Rotate` rewrite. `qpdf --check` passes and ghostscript, CoreGraphics and our own renderer all turn the page. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018e3PEzyU2oAFSzsEoWsSmz --- CMakeLists.txt | 1 + docs/design/pdf-annotation.md | 60 ++++--- src/odr/internal/pdf/pdf_writer.cpp | 235 +++++++++++++++++++++++++ src/odr/internal/pdf/pdf_writer.hpp | 57 ++++++ test/CMakeLists.txt | 1 + test/src/internal/pdf/pdf_writer.cpp | 251 +++++++++++++++++++++++++++ 6 files changed, 574 insertions(+), 31 deletions(-) create mode 100644 src/odr/internal/pdf/pdf_writer.cpp create mode 100644 src/odr/internal/pdf/pdf_writer.hpp create mode 100644 test/src/internal/pdf/pdf_writer.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index 13a519722..579af93f1 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -249,6 +249,7 @@ set(ODR_SOURCE_FILES "src/odr/internal/pdf/pdf_object_parser.cpp" "src/odr/internal/pdf/pdf_page_extractor.cpp" "src/odr/internal/pdf/pdf_shading.cpp" + "src/odr/internal/pdf/pdf_writer.cpp" "src/odr/internal/png/png_util.cpp" diff --git a/docs/design/pdf-annotation.md b/docs/design/pdf-annotation.md index a638ff66f..74a032612 100644 --- a/docs/design/pdf-annotation.md +++ b/docs/design/pdf-annotation.md @@ -3,8 +3,8 @@ Status: **underway.** This records the architecture for adding markup annotations — text highlight and freehand drawing first — to an existing PDF, the alternatives weighed, and the effort it costs. The format model is -validated against four viewers; Phases 0 and 0.5 have landed, and the writer -itself has not started. +validated against four viewers, and Phases 0 through 1 have landed: the +incremental writer works, the annotations it will carry are not written yet. Scope is **markup only**: draw on top of a page, highlight/underline/strike text. Editing or removing the *existing* text of a PDF is explicitly out — that @@ -228,9 +228,10 @@ Three things the spike did **not** settle, and Phase 1 and 2 owe tests for each: one of those engines painted — the `/QuadPoints` were never consulted. The ordering matters only to a viewer that regenerates the appearance, and to text-selection semantics. The note below stands as a note. -- **A page dictionary inside an object stream.** The fixture's was plain. +- **A page dictionary inside an object stream.** The fixture's was plain, and + Phase 1 did not close this either — no fixture we have puts one there. - **Appending to a file whose newest section is an xref stream.** The fixture's - was a classic table. + was a classic table; Phase 1's tests cover both flavors. ## Implementation plan @@ -255,33 +256,30 @@ The first two are `std::optional` and recovery clears them — a rebuilt table has no section of the file's own to chain onto, so the missing value and decision 2's refusal gate are the same fact. -### Phase 1 — the incremental writer (2–3 d, ~340 lines) - -New `pdf/pdf_writer.{hpp,cpp}`: copy the source stream, append indirect objects, -emit the changed-ids xref, write the trailer with `/Prev` and a regenerated -second `/ID` element. - -- **Match the file's xref flavor** (`xref_kind()`). If the last section was an - xref stream, append an xref stream; otherwise a classic table. -- **A page dictionary living in an object stream is rewritten uncompressed** in - the new section — legal, the newer entry wins. -- Refuse a file where `is_recovered()` (decision 2), and an encrypted one - (decision 6). - -Sequence the first two steps so the plumbing fails separately from the -annotation semantics: - -1. **A no-op incremental update** — append a section that changes nothing; - assert the file re-parses identically and `qpdf --check` passes. -2. **Page `/Rotate` as the first real write** — one integer on an existing - dictionary, no new object types, no appearance. It exercises the genuinely - risky part (rewriting an object that may live in an object stream, in a file - of either xref flavor) and lands a feature from *What the writer unlocks - next* on the way. - -Verification: round-trip through our own parser, then `qpdf --check`, then -LibreOffice and ghostscript as external oracles (the standing oracles for this -repo). +### Phase 1 — the incremental writer — **done** (#845) + +`pdf/pdf_writer.{hpp,cpp}`: `IncrementalWriter` pipes the source through +untouched, appends the objects it collected, and closes with a cross-reference +section naming only their ids and a trailer chaining back through `/Prev`. + +- **Matches the file's xref flavor** (`xref_kind()`), classic table or + cross-reference stream — the latter minting an id and an entry for the stream + object itself. +- **Refuses** a recovered file (decision 2) and an encrypted one (decision 6), + resolving both gates in the constructor so nothing downstream re-asks. +- **`/ID[1]` is derived from the update's own bytes**, not from a clock, so + writing the same update twice gives the same file and a test can pin it. +- Only the update is buffered; the source is piped, so appending to a large + file does not hold it in memory. + +Verified in the order the plan asked for — a no-op update that re-parses +identically first, then a `/Rotate` rewrite. `qpdf --check` passes, and +ghostscript, CoreGraphics and our own renderer all honour the new rotation +(the page box turns 8.5×11in into 11×8.5in). + +**Still untested: a page dictionary living inside an object stream.** It has to +be rewritten uncompressed in the new section — legal, the newer entry wins — +but no fixture we have puts one there. Owed before Phase 2 ships. ### Phase 2 — highlight (2 d, ~200 lines) diff --git a/src/odr/internal/pdf/pdf_writer.cpp b/src/odr/internal/pdf/pdf_writer.cpp new file mode 100644 index 000000000..25e44b3f5 --- /dev/null +++ b/src/odr/internal/pdf/pdf_writer.cpp @@ -0,0 +1,235 @@ +#include + +#include +#include +#include + +#include +#include +#include +#include +#include +#include + +#include + +namespace odr::internal::pdf { + +namespace { + +struct Placement { + std::uint32_t offset{0}; + std::uint32_t gen{0}; +}; + +using Placements = std::map; + +/// A cross-reference subsection (7.5.4): `first count`, then that many entries. +struct Subsection { + std::uint64_t first{0}; + std::vector placements; +}; + +std::vector group_into_subsections(const Placements &placements) { + std::vector result; + for (const auto &[id, placement] : placements) { + if (result.empty() || + result.back().first + result.back().placements.size() != id) { + result.push_back(Subsection{id, {}}); + } + result.back().placements.push_back(placement); + } + return result; +} + +/// Entry table for `/W [1 4 2]` (7.5.8.3); every entry is type 1, in use. +std::string xref_stream_table(const Placements &placements) { + std::string result; + for (const auto &[id, placement] : placements) { + result.push_back(1); + for (int shift = 24; shift >= 0; shift -= 8) { + result.push_back(static_cast((placement.offset >> shift) & 0xff)); + } + result.push_back(static_cast((placement.gen >> 8) & 0xff)); + result.push_back(static_cast(placement.gen & 0xff)); + } + return result; +} + +struct SourceExtent { + std::uint32_t size{0}; + bool ends_with_eol{false}; +}; + +SourceExtent measure(std::istream &in) { + in.clear(); + in.seekg(0, std::ios::end); + const auto size = static_cast(in.tellg()); + if (size == 0) { + return {0, true}; + } + in.seekg(-1, std::ios::end); + const char last = static_cast(in.get()); + return {size, last == '\n' || last == '\r'}; +} + +} // namespace + +IncrementalWriter::IncrementalWriter(DocumentParser &parser) + : m_parser{&parser} { + if (parser.is_recovered()) { + throw std::runtime_error( + "cannot append to a file whose cross-reference table was recovered"); + } + if (parser.is_encrypted()) { + throw std::runtime_error("cannot append to an encrypted file"); + } + const std::optional position = parser.start_xref_position(); + const std::optional kind = parser.xref_kind(); + if (!position.has_value() || !kind.has_value()) { + throw std::runtime_error("no cross-reference section to append to"); + } + m_previous_xref_position = *position; + m_xref_kind = *kind; + m_next_id = parser.highest_object_id() + 1; +} + +ObjectReference IncrementalWriter::mint_object() { + return ObjectReference(m_next_id++, 0); +} + +void IncrementalWriter::set_object(const ObjectReference &reference, + Object object) { + m_entries[reference] = Entry{std::move(object), std::nullopt}; + m_next_id = std::max(m_next_id, reference.id + 1); +} + +void IncrementalWriter::set_stream_object(const ObjectReference &reference, + Dictionary dictionary, + std::string stream) { + dictionary["Length"] = Object(static_cast(stream.size())); + m_entries[reference] = + Entry{Object(std::move(dictionary)), std::move(stream)}; + m_next_id = std::max(m_next_id, reference.id + 1); +} + +Dictionary +IncrementalWriter::build_trailer(const std::uint64_t size, + const std::string_view revision_seed) const { + const Dictionary &source = m_parser->trailer(); + + Dictionary result; + result["Size"] = Object(static_cast(size)); + result["Root"] = source.get("Root"); + if (source.has_value("Info")) { + result["Info"] = source.get("Info"); + } + + // 14.4: `/ID[0]` carries over, `/ID[1]` names this revision — off the + // update's own bytes, not a clock, so the same update writes the same file. + const Object &id = source.get("ID"); + if (id.is_array() && id.as_array().size() == 2 && + id.as_array()[0].is_string()) { + Array result_id; + result_id.holder().emplace_back(HexString{id.as_array()[0].as_string()}); + result_id.holder().emplace_back( + HexString{crypto::util::md5(revision_seed)}); + result["ID"] = Object(std::move(result_id)); + } + + result["Prev"] = Object(static_cast(m_previous_xref_position)); + return result; +} + +void IncrementalWriter::write(std::ostream &out) const { + std::istream &in = m_parser->in(); + const std::streampos resume = in.tellg(); + + const SourceExtent source = measure(in); + // An object must start on its own line; a `%%EOF` may end the file bare. + const std::string separator = source.ends_with_eol ? "" : "\n"; + + // Only the update is buffered; the source is piped. + std::string update = separator; + const auto position = [&] { + return static_cast(source.size + update.size()); + }; + + Placements placements; + for (const auto &[reference, entry] : m_entries) { + placements[reference.id] = + Placement{position(), static_cast(reference.gen)}; + update += fmt::format("{} {} obj\n", reference.id, reference.gen); + update += entry.object.to_string(); + if (entry.stream.has_value()) { + update += "\nstream\n"; + update += *entry.stream; + update += "\nendstream"; + } + update += "\nendobj\n"; + } + + const std::uint32_t xref_position = position(); + std::uint64_t trailer_size = m_parser->highest_object_id() + 1; + for (const auto &[id, placement] : placements) { + trailer_size = std::max(trailer_size, id + 1); + } + + if (m_xref_kind == DocumentParser::XrefKind::table) { + update += "xref\n"; + for (const Subsection &subsection : group_into_subsections(placements)) { + update += fmt::format("{} {}\n", subsection.first, + subsection.placements.size()); + for (const Placement &placement : subsection.placements) { + // 7.5.4: exactly 20 bytes, the two-character EOL included + update += + fmt::format("{:010} {:05} n \n", placement.offset, placement.gen); + } + } + update += "trailer\n"; + update += build_trailer(trailer_size, update).to_string(); + update += '\n'; + } else { + // The stream is an object, so it takes an id and an entry of its own; its + // dictionary doubles as the trailer (7.5.8). + const ObjectReference reference(trailer_size, 0); + placements[reference.id] = Placement{xref_position, 0}; + ++trailer_size; + + const std::string table = xref_stream_table(placements); + + Dictionary dictionary = build_trailer(trailer_size, update); + dictionary["Type"] = Object(Name{"XRef"}); + Array widths; + widths.holder().emplace_back(Integer{1}); + widths.holder().emplace_back(Integer{4}); + widths.holder().emplace_back(Integer{2}); + dictionary["W"] = Object(std::move(widths)); + Array index; + for (const Subsection &subsection : group_into_subsections(placements)) { + index.holder().emplace_back(static_cast(subsection.first)); + index.holder().emplace_back( + static_cast(subsection.placements.size())); + } + dictionary["Index"] = Object(std::move(index)); + dictionary["Length"] = Object(static_cast(table.size())); + + update += fmt::format("{} {} obj\n", reference.id, reference.gen); + update += Object(std::move(dictionary)).to_string(); + update += "\nstream\n"; + update += table; + update += "\nendstream\nendobj\n"; + } + + update += fmt::format("startxref\n{}\n%%EOF\n", xref_position); + + in.clear(); + in.seekg(0); + util::stream::pipe(in, out); + out.write(update.data(), static_cast(update.size())); + + in.clear(); + in.seekg(resume); +} + +} // namespace odr::internal::pdf diff --git a/src/odr/internal/pdf/pdf_writer.hpp b/src/odr/internal/pdf/pdf_writer.hpp new file mode 100644 index 000000000..1d7c094d7 --- /dev/null +++ b/src/odr/internal/pdf/pdf_writer.hpp @@ -0,0 +1,57 @@ +#pragma once + +#include +#include + +#include +#include +#include +#include +#include + +namespace odr::internal::pdf { + +/// Appends an incremental update (ISO 32000-1 7.5.6) to the file a +/// `DocumentParser` read. The source is copied, never re-serialized. +class IncrementalWriter final { +public: + /// @throws std::runtime_error for a file that cannot take one: recovered + /// cross-reference table, or encrypted. + explicit IncrementalWriter(DocumentParser &parser); + + /// An id past every one the file uses. + [[nodiscard]] ObjectReference mint_object(); + + /// Write `object` at `reference`, overriding what the file has there. Keeps + /// the generation; 7.5.6 raises it only where a freed id is reused. + void set_object(const ObjectReference &reference, Object object); + /// `/Length` is computed; the caller supplies any `/Filter` and the matching + /// pre-encoded bytes. + void set_stream_object(const ObjectReference &reference, + Dictionary dictionary, std::string stream); + + [[nodiscard]] std::size_t size() const noexcept { return m_entries.size(); } + + /// Legal with nothing set: the result parses identically. + void write(std::ostream &out) const; + +private: + struct Entry { + Object object; + std::optional stream; + }; + + /// `/Root`, `/Info` and `/ID` from the source, plus `/Size` and `/Prev`. + [[nodiscard]] Dictionary build_trailer(std::uint64_t size, + std::string_view revision_seed) const; + + DocumentParser *m_parser{nullptr}; + /// Resolved in the constructor, which rejects a file lacking them. + std::uint32_t m_previous_xref_position{0}; + DocumentParser::XrefKind m_xref_kind{DocumentParser::XrefKind::table}; + + std::map m_entries; + std::uint64_t m_next_id{0}; +}; + +} // namespace odr::internal::pdf diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 21e1d89cf..f1d862188 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -114,6 +114,7 @@ add_executable(odr_test "src/internal/pdf/pdf_page_extractor.cpp" "src/internal/pdf/pdf_shading.cpp" "src/internal/pdf/pdf_test_file_builder.cpp" + "src/internal/pdf/pdf_writer.cpp" "src/internal/png/png_util_test.cpp" diff --git a/test/src/internal/pdf/pdf_writer.cpp b/test/src/internal/pdf/pdf_writer.cpp new file mode 100644 index 000000000..999f5a4d6 --- /dev/null +++ b/test/src/internal/pdf/pdf_writer.cpp @@ -0,0 +1,251 @@ +#include + +#include +#include +#include +#include + +#include + +#include + +#include +#include +#include +#include + +#include + +using namespace odr::internal; +using namespace odr::internal::pdf; +using namespace odr::test; +using PdfFileBuilder = odr::test::pdf::PdfFileBuilder; + +namespace { + +std::string mini_pdf(const bool classic) { + PdfFileBuilder builder; + builder.object("<< /Type /Catalog /Pages 2 0 R >>") + .object("<< /Type /Pages /Kids [3 0 R] /Count 1 >>") + .object("<< /Type /Page /Parent 2 0 R /MediaBox [0 0 612 792] " + "/Resources << >> /Contents 4 0 R >>") + .stream_object("", "BT ET") + .trailer("/Root 1 0 R /ID [<0102> <0304>]"); + return classic ? builder.build_classic() : builder.build_xref_stream(); +} + +std::unique_ptr stream_of(const std::string &pdf) { + return std::make_unique(pdf); +} + +template +std::string append(const std::string &pdf, SetUp &&set_up) { + DocumentParser parser(stream_of(pdf)); + IncrementalWriter writer(parser); + set_up(parser, writer); + std::ostringstream out; + writer.write(out); + return std::move(out).str(); +} + +std::string append_nothing(const std::string &pdf) { + return append(pdf, [](DocumentParser &, IncrementalWriter &) {}); +} + +const Page *first_page(const Document &document) { + const auto &kids = document.catalog->pages->kids; + return kids.empty() ? nullptr : dynamic_cast(kids.front()); +} + +} // namespace + +// An update that changes nothing still has to parse. +TEST(IncrementalWriter, no_op_update_preserves_the_document) { + for (const bool classic : {true, false}) { + SCOPED_TRACE(classic ? "classic" : "xref stream"); + + const std::string source = mini_pdf(classic); + const std::string result = append_nothing(source); + + ASSERT_GT(result.size(), source.size()); + EXPECT_EQ(result.substr(0, source.size()), source); + + DocumentParser parser(stream_of(result)); + EXPECT_FALSE(parser.is_recovered()); + EXPECT_EQ(parser.xref_kind(), classic ? DocumentParser::XrefKind::table + : DocumentParser::XrefKind::stream); + + const std::unique_ptr document = parser.parse_document(); + ASSERT_EQ(document->catalog->pages->count, 1); + const Page *page = first_page(*document); + ASSERT_NE(page, nullptr); + ASSERT_EQ(page->contents_reference.size(), 1); + EXPECT_EQ(parser.read_decoded_stream(page->contents_reference.front()), + "BT ET"); + } +} + +// `/Prev` keeps the older objects reachable. +TEST(IncrementalWriter, trailer_chains_to_the_previous_section) { + const std::string source = mini_pdf(true); + const std::string result = append_nothing(source); + + const std::size_t previous = source.rfind("startxref\n"); + ASSERT_NE(previous, std::string::npos); + const std::string previous_position = source.substr( + previous + 10, source.find('\n', previous + 10) - previous - 10); + + EXPECT_NE(result.find("/Prev " + previous_position), std::string::npos); + // `/ID[0]` carries over, `/ID[1]` names this revision + EXPECT_NE(result.rfind("<0102>"), std::string::npos); + EXPECT_EQ(result.rfind("<0304>"), source.rfind("<0304>")); +} + +// Nothing in the writer reads a clock. +TEST(IncrementalWriter, output_is_deterministic) { + const std::string source = mini_pdf(true); + EXPECT_EQ(append_nothing(source), append_nothing(source)); +} + +// The newer definition wins; the older one stays, unreferenced. +TEST(IncrementalWriter, rewrites_page_rotate) { + for (const bool classic : {true, false}) { + SCOPED_TRACE(classic ? "classic" : "xref stream"); + + const std::string result = + append(mini_pdf(classic), [](DocumentParser &parser, + IncrementalWriter &writer) { + const std::unique_ptr document = parser.parse_document(); + const Page *page = first_page(*document); + ASSERT_NE(page, nullptr); + + Dictionary rotated = page->object.as_dictionary(); + rotated["Rotate"] = Object(Integer{90}); + writer.set_object(page->object_reference, Object(std::move(rotated))); + }); + + DocumentParser parser(stream_of(result)); + const std::unique_ptr document = parser.parse_document(); + const Page *page = first_page(*document); + ASSERT_NE(page, nullptr); + EXPECT_EQ(page->rotate, 90); + ASSERT_EQ(page->contents_reference.size(), 1); + EXPECT_EQ(parser.read_decoded_stream(page->contents_reference.front()), + "BT ET"); + EXPECT_EQ(page->media_box.as_array()[2].as_real(), 612.0); + } +} + +TEST(IncrementalWriter, appends_a_new_object) { + const std::string result = append( + mini_pdf(true), [](DocumentParser &parser, IncrementalWriter &writer) { + const std::unique_ptr document = parser.parse_document(); + const Page *page = first_page(*document); + ASSERT_NE(page, nullptr); + + // four objects in the mini pdf + const ObjectReference annotation = writer.mint_object(); + EXPECT_EQ(annotation.id, 5u); + + Dictionary dictionary; + dictionary["Type"] = Object(Name{"Annot"}); + dictionary["Subtype"] = Object(Name{"Square"}); + Array rect; + rect.holder().emplace_back(Integer{10}); + rect.holder().emplace_back(Integer{20}); + rect.holder().emplace_back(Integer{30}); + rect.holder().emplace_back(Integer{40}); + dictionary["Rect"] = Object(std::move(rect)); + writer.set_object(annotation, Object(std::move(dictionary))); + + Dictionary annotated = page->object.as_dictionary(); + Array annotations; + annotations.holder().emplace_back(annotation); + annotated["Annots"] = Object(std::move(annotations)); + writer.set_object(page->object_reference, Object(std::move(annotated))); + }); + + DocumentParser parser(stream_of(result)); + const std::unique_ptr document = parser.parse_document(); + const Page *page = first_page(*document); + ASSERT_NE(page, nullptr); + ASSERT_EQ(page->annotations.size(), 1); + EXPECT_EQ(page->annotations.front() + ->object.as_dictionary() + .get("Subtype") + .as_string(), + "Square"); +} + +TEST(IncrementalWriter, appends_a_stream_object) { + const std::string content = "0 0 10 10 re f"; + + ObjectReference written; + const std::string result = + append(mini_pdf(true), [&written, &content](DocumentParser &, + IncrementalWriter &writer) { + written = writer.mint_object(); + Dictionary dictionary; + dictionary["Type"] = Object(Name{"XObject"}); + dictionary["Subtype"] = Object(Name{"Form"}); + writer.set_stream_object(written, std::move(dictionary), content); + }); + + DocumentParser parser(stream_of(result)); + EXPECT_EQ(parser.read_decoded_stream(written), content); + const Object &dictionary = parser.read_object(written).object; + EXPECT_EQ(dictionary.as_dictionary().get("Length").as_integer(), + static_cast(content.size())); +} + +// A rebuilt table has no section of the file's own to chain onto. +TEST(IncrementalWriter, refuses_a_recovered_file) { + const std::string pdf = + "HTTP/1.0 200 OK\r\nContent-Type: application/pdf\r\n\r\n" + + mini_pdf(true); + DocumentParser parser(stream_of(pdf)); + ASSERT_TRUE(parser.is_recovered()); + EXPECT_ANY_THROW((void)IncrementalWriter(parser)); +} + +TEST(IncrementalWriter, refuses_an_encrypted_file) { + const auto file = std::make_shared( + TestData::test_file_path("odr-public/pdf/Casio_WVA-M650-7AJF.pdf")); + DocumentParser parser(file->stream()); + ASSERT_TRUE(parser.is_encrypted()); + EXPECT_ANY_THROW((void)IncrementalWriter(parser)); +} + +TEST(IncrementalWriter, rewrites_a_page_of_a_real_fixture) { + const auto file = std::make_shared( + TestData::test_file_path("odr-public/pdf/style-various-1.pdf")); + + std::ostringstream out; + { + DocumentParser parser(file->stream()); + const std::unique_ptr document = parser.parse_document(); + const Page *page = first_page(*document); + ASSERT_NE(page, nullptr); + + IncrementalWriter writer(parser); + Dictionary rotated = page->object.as_dictionary(); + rotated["Rotate"] = Object(Integer{270}); + writer.set_object(page->object_reference, Object(std::move(rotated))); + writer.write(out); + } + + DocumentParser parser( + std::make_unique(std::move(out).str())); + const std::unique_ptr document = parser.parse_document(); + const std::vector pages = document->collect_pages(); + ASSERT_EQ(pages.size(), 2); + EXPECT_EQ(pages[0]->rotate, 270); + EXPECT_EQ(pages[1]->rotate, 0); + + EXPECT_FALSE(pages[0]->annotations.empty()); + for (const Page *page : pages) { + for (const auto &content_reference : page->contents_reference) { + EXPECT_FALSE(parser.read_decoded_stream(content_reference).empty()); + } + } +} From c7a3896d7fe463791640ac3c80c0a5e1c95d53ee Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Sun, 6 Sep 2026 17:09:09 +0200 Subject: [PATCH 2/2] docs(pdf): cut the prose from the object and parser comments Comments restating the code, justifying a choice at paragraph length, or naming what the implementation used to do. `{:g}` having rounded to four significant digits is what the commit that changed it is for. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018e3PEzyU2oAFSzsEoWsSmz --- src/odr/internal/pdf/pdf_document_parser.cpp | 8 +++--- src/odr/internal/pdf/pdf_document_parser.hpp | 26 ++++++++------------ src/odr/internal/pdf/pdf_object.cpp | 7 +++--- test/src/internal/pdf/pdf_object.cpp | 9 +++---- test/src/internal/util/math_util_test.cpp | 3 --- 5 files changed, 19 insertions(+), 34 deletions(-) diff --git a/src/odr/internal/pdf/pdf_document_parser.cpp b/src/odr/internal/pdf/pdf_document_parser.cpp index dd04136fe..109c76c54 100644 --- a/src/odr/internal/pdf/pdf_document_parser.cpp +++ b/src/odr/internal/pdf/pdf_document_parser.cpp @@ -1565,7 +1565,7 @@ std::optional DocumentParser::xref_kind() const { bool DocumentParser::is_recovered() const { return m_recovered; } std::uint64_t DocumentParser::highest_object_id() const { - // the table is keyed by `ObjectReference`, which orders by id first + // keyed by `ObjectReference`, which orders by id first return m_xref.table.empty() ? 0 : m_xref.table.rbegin()->first.id; } @@ -1859,8 +1859,7 @@ std::pair DocumentParser::read_trailer_chain() { while (position.has_value() && visited.insert(*position).second) { auto [xref, trailer_dict, kind] = read_xref_section(*position); - // The newest section is the one an appended section chains onto, so it is - // the one whose position and kind a writer needs. + // the newest section is the one an appended section chains onto if (!m_start_xref_position.has_value()) { m_start_xref_position = position; m_xref_kind = kind; @@ -1899,8 +1898,7 @@ void DocumentParser::recover_xref() { m_objects.clear(); m_object_streams.clear(); m_recovered = true; - // A partially walked chain may have recorded these before it threw, and a - // rebuilt table has no section of the file's own to chain onto anyway. + // a partially walked chain may have recorded these before it threw m_start_xref_position.reset(); m_xref_kind.reset(); diff --git a/src/odr/internal/pdf/pdf_document_parser.hpp b/src/odr/internal/pdf/pdf_document_parser.hpp index 38c13c8d5..c063edbcf 100644 --- a/src/odr/internal/pdf/pdf_document_parser.hpp +++ b/src/odr/internal/pdf/pdf_document_parser.hpp @@ -54,29 +54,24 @@ class DocumentParser { [[nodiscard]] const Xref &xref() const; [[nodiscard]] const Dictionary &trailer() const; - /// How a cross-reference section states itself. A section appended to the - /// file has to match the newest one: a reader arriving over `/Prev` expects - /// what it already found. + /// How a cross-reference section is written. An appended section has to + /// match the newest one. enum class XrefKind { table, ///< classic `xref` table plus `trailer` (7.5.4) stream, ///< cross-reference stream (7.5.8) }; - /// The byte offset of the newest cross-reference section — what an appended - /// section's `/Prev` points back at. `nullopt` when the xref was recovered. + /// Byte offset of the newest cross-reference section, what an appended + /// section's `/Prev` points at. `nullopt` when the xref was recovered. [[nodiscard]] std::optional start_xref_position() const; - /// How the newest cross-reference section is written. `nullopt` when the - /// xref was recovered. + /// `nullopt` when the xref was recovered. [[nodiscard]] std::optional xref_kind() const; - /// Whether the cross-reference table was rebuilt by scanning the file - /// instead of read from the file's own. Nothing may be appended to such a - /// file: its structure is broken, so an incremental update onto it would - /// only be readable by us. + /// Whether the cross-reference table was rebuilt by scanning the file. + /// Nothing may be appended to such a file. [[nodiscard]] bool is_recovered() const; - /// The highest object id the cross-reference table carries, so new ids - /// continue past it. 0 for an empty table. + /// Highest object id in the cross-reference table; 0 when empty. [[nodiscard]] std::uint64_t highest_object_id() const; /// Whether the file declares an `/Encrypt` dictionary. @@ -119,8 +114,7 @@ class DocumentParser { [[nodiscard]] Object deep_resolve_object_copy(Object object); private: - /// One cross-reference section as read. `trailer` is the trailer dictionary - /// (a cross-reference stream's own dictionary doubles as one). + /// A cross-reference stream's own dictionary doubles as the trailer. struct XrefSection { Xref xref; Dictionary trailer; @@ -133,7 +127,7 @@ class DocumentParser { /// Walk the `startxref` → `Prev` chain and return the merged cross-reference /// table together with the newest (first-seen) trailer dictionary. Records - /// the newest section's position and kind on the way. + /// the newest section's position and kind. [[nodiscard]] std::pair read_trailer_chain(); void recover_xref(); diff --git a/src/odr/internal/pdf/pdf_object.cpp b/src/odr/internal/pdf/pdf_object.cpp index 525850ee1..985b0d4ce 100644 --- a/src/odr/internal/pdf/pdf_object.cpp +++ b/src/odr/internal/pdf/pdf_object.cpp @@ -26,10 +26,9 @@ bool name_char_is_regular(const unsigned char c) { } // namespace void StandardString::to_stream(std::ostream &out) const { - // 7.3.4.2: only the reverse solidus and unbalanced parentheses need - // escaping, but balance is a property of the whole string, so escape every - // parenthesis rather than track it. A literal carriage return is read back - // as an end-of-line marker, i.e. as `\n`, so it has to be escaped too. + // 7.3.4.2: balance is a property of the whole string, so escape every + // parenthesis rather than track it. A carriage return would read back as + // `\n`. out << "("; for (const char c : string) { switch (c) { diff --git a/test/src/internal/pdf/pdf_object.cpp b/test/src/internal/pdf/pdf_object.cpp index f8552d0b0..d99e2f4e3 100644 --- a/test/src/internal/pdf/pdf_object.cpp +++ b/test/src/internal/pdf/pdf_object.cpp @@ -167,15 +167,13 @@ TEST(PdfObject, to_string) { EXPECT_EQ(Object(ObjectReference(12, 0)).to_string(), "12 0 R"); } -// 7.3.3 knows no exponent form, and the host locale must not reach the output. +// 7.3.3 has no exponent form, and the host locale must not reach the output. TEST(PdfObject, real_to_string_is_plain_decimal) { EXPECT_EQ(Object(Real{1.5}).to_string(), "1.5"); EXPECT_EQ(Object(Real{0.0}).to_string(), "0"); EXPECT_EQ(Object(Real{-72.25}).to_string(), "-72.25"); - // `{:g}` would have written these as `1e-05` and `1.44e+04` EXPECT_EQ(Object(Real{0.00001}).to_string(), "0.00001"); EXPECT_EQ(Object(Real{14400.0}).to_string(), "14400"); - // and would have rounded this one to four significant digits EXPECT_EQ(Object(Real{612.345}).to_string(), "612.345"); } @@ -185,7 +183,7 @@ TEST(PdfObject, standard_string_escapes_delimiters) { EXPECT_EQ(Object(StandardString{R"(back\slash)"}).to_string(), R"((back\\slash))"); EXPECT_EQ(Object(StandardString{"cr\rlf"}).to_string(), R"((cr\rlf))"); - // a line feed stands for itself (7.3.4.2), so it is written raw + // 7.3.4.2: a line feed stands for itself EXPECT_EQ(Object(StandardString{"a\nb"}).to_string(), "(a\nb)"); } @@ -217,8 +215,7 @@ TEST(PdfObject, container_to_string) { EXPECT_EQ(Object(Dictionary{}).to_string(), "<<>>"); } -// A key is a name and is escaped as one, or a space in it would split the -// dictionary in two on the way back in. +// A space in a key would split the dictionary in two on the way back in. TEST(PdfObject, dictionary_key_is_escaped) { Dictionary dictionary; dictionary["Odd Key"] = Object(Integer{1}); diff --git a/test/src/internal/util/math_util_test.cpp b/test/src/internal/util/math_util_test.cpp index a0c9a7b74..6c1253609 100644 --- a/test/src/internal/util/math_util_test.cpp +++ b/test/src/internal/util/math_util_test.cpp @@ -38,8 +38,6 @@ TEST(Transform2D, compose_is_ordered) { EXPECT_DOUBLE_EQ(q[1], 10); } -// Applying a transform then its inverse returns the original point, whatever -// the transform is made of. TEST(Transform2D, inverse_undoes_apply) { const Transform2D m = Transform2D::translation(-30, -40) * Transform2D::scaling_translation(1, -1, 0, 800) * @@ -62,7 +60,6 @@ TEST(Transform2D, inverse_of_identity_is_identity) { EXPECT_DOUBLE_EQ(p[1], 4); } -// A singular linear part collapses the plane, so nothing undoes it. TEST(Transform2D, inverse_of_singular_is_nullopt) { EXPECT_FALSE(Transform2D::scaling(0, 1).inverse().has_value()); EXPECT_FALSE((Transform2D{1, 2, 2, 4, 5, 6}).inverse().has_value());