feat(pdf): append an incremental update

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"
 

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)
 

src/odr/internal/pdf/pdf_document_parser.cpp

@@ -1565,7 +1565,7 @@ std::optional<DocumentParser::XrefKind> 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<Xref, Dictionary> 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();
 

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<std::uint32_t> 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<XrefKind> 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<Xref, Dictionary> read_trailer_chain();
 
   void recover_xref();

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) {