feat(pdf): expose the parse facts a writer needs

docs/design/pdf-annotation.md

@@ -1,9 +1,10 @@
 # PDF annotation design
 
-Status: **researched; not scheduled.** 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. Nothing here is
-implemented.
+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.
 
 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
@@ -166,7 +167,9 @@ Notes on the shape:
   spec's stated order (12.5.6.10) is counterclockwise; every implementation
   writes the Z-order above, and `pdfAnnotate`'s documentation says as much
   outright. Follow the implementations, and say so in a comment at the one place
-  that emits it.
+  that emits it. Still **unverified** — see *Validated against real viewers*:
+  an annotation carrying an `/AP` never has its `/QuadPoints` read, so a test
+  has to reach for a viewer that regenerates the appearance.
 - **`delete` only names an annotation we wrote**, identified by the `/NM` we
   minted. Deleting a foreign annotation is out of scope: we would have to prove
   nothing else references it.
@@ -203,29 +206,79 @@ and an appearance of `m`/`c`/`S` with round caps and joins.
 The transparency group on the form is what makes `/BM /Multiply` composite
 against the page rather than against the form's own backdrop.
 
+### Validated against real viewers
+
+A throwaway script wrote exactly the above — a highlight and an ink stroke, as
+one incremental update — onto `odr-public/pdf/style-various-1.pdf`, before any
+of it was committed to C++. `qpdf --check` passes and four independent engines
+paint both annotations with the page text showing through the highlight:
+ghostscript, PDFium (Chrome), CoreGraphics (Preview), and **our own renderer**,
+which emits the highlight as `<path fill="rgb(255,230,51)"
+style="mix-blend-mode:multiply">` and the ink as a round-capped stroke.
+
+So the following are facts, not assumptions: the transparency group composites
+against the page rather than a black backdrop; appending to a page's *existing*
+`/Annots` array works and the newer page object wins; a classic section listing
+only the changed ids is accepted everywhere; and `to_box` places the result
+correctly (user-space y 700/688 arrived at page-box y 92/104).
+
+Three things the spike did **not** settle, and Phase 1 and 2 owe tests for each:
+
+- **QuadPoints ordering.** With an `/AP` present, the appearance is what every
+  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.
+- **Appending to a file whose newest section is an xref stream.** The fixture's
+  was a classic table.
+
 ## Implementation plan
 
 Ordered so each step is verifiable on its own. Estimates are working days.
 
-### Phase 0 — serialization correctness (0.5 d, ~70 lines)
+### Phase 0 — serialization correctness — **done** (#843)
 
-`Transform2D::inverse`; escape `(`, `)`, `\` in `StandardString` (the existing
-`TODO`); `#`-escape `Name`; emit reals through `fmt` with no exponent and no
-locale (the repo's `to_string_significant` rule). Assertion tests per case.
+`Transform2D::inverse`; escaping for `StandardString`, `Name` and dictionary
+keys; reals through `util::number::to_string_significant`, since `{:.4g}` both
+rounded to four significant digits and reached for an exponent form 7.3.3 has
+no syntax for. Pinned by a round trip through `ObjectParser`.
+
+### Phase 0.5 — the parse facts a writer needs — **done** (#844)
+
+Appending needs four things about the file, and `DocumentParser` computed all
+four while keeping one. `xref()`/`trailer()` were reachable; the newest
+section's offset, its kind, and the recovery flag were not. Now
+`start_xref_position()`, `xref_kind()`, `is_recovered()` and
+`highest_object_id()`.
+
+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.** If the last section was an xref stream,
-  append an xref stream; otherwise a classic table.
+- **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 that came from xref recovery (decision 2), and an encrypted one
+- 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).
@@ -319,3 +372,8 @@ Medium:
   the `Decryptor` key accessor need to land in v1 after all?
 - **Where does the pending-annotation state live across a reload** in the mobile
   WebView — the browser only, or does the host persist the payload?
+- **How do we test `/QuadPoints` ordering at all?** Every engine we have as an
+  oracle paints the `/AP` and ignores them. Options: write one annotation
+  *without* an appearance and see where a viewer puts it, or check what Acrobat
+  does with our file. Cheap either way, but it needs deciding before Phase 2
+  claims the ordering is right.

src/odr/internal/pdf/pdf_document_parser.cpp

@@ -1554,6 +1554,21 @@ const Xref &DocumentParser::xref() const { return m_xref; }
 
 const Dictionary &DocumentParser::trailer() const { return m_trailer; }
 
+std::optional<std::uint32_t> DocumentParser::start_xref_position() const {
+  return m_start_xref_position;
+}
+
+std::optional<DocumentParser::XrefKind> DocumentParser::xref_kind() const {
+  return m_xref_kind;
+}
+
+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
+  return m_xref.table.empty() ? 0 : m_xref.table.rbegin()->first.id;
+}
+
 bool DocumentParser::is_encrypted() const { return m_is_encrypted; }
 
 bool DocumentParser::is_authenticated() const {
@@ -1767,7 +1782,7 @@ Object DocumentParser::deep_resolve_object_copy(Object object) {
   return object;
 }
 
-std::pair<Xref, Dictionary>
+DocumentParser::XrefSection
 DocumentParser::read_xref_section(const std::uint32_t position) {
   in().clear();
   in().seekg(position);
@@ -1778,7 +1793,7 @@ DocumentParser::read_xref_section(const std::uint32_t position) {
     Xref xref = parser().read_xref();
     parser().parser().skip_whitespace();
     Trailer trailer = parser().read_trailer();
-    return {std::move(xref), std::move(trailer.dictionary)};
+    return {std::move(xref), std::move(trailer.dictionary), XrefKind::table};
   }
 
   // cross-reference stream (ISO 32000-1 7.5.8); its dictionary doubles as
@@ -1829,7 +1844,7 @@ DocumentParser::read_xref_section(const std::uint32_t position) {
 
   util::stream::ViewStream in(decoded.data);
   Xref xref = FileParser(in).read_xref_stream_table(field_widths, subsections);
-  return {std::move(xref), dictionary};
+  return {std::move(xref), dictionary, XrefKind::stream};
 }
 
 std::pair<Xref, Dictionary> DocumentParser::read_trailer_chain() {
@@ -1842,15 +1857,22 @@ std::pair<Xref, Dictionary> DocumentParser::read_trailer_chain() {
   std::set<std::uint32_t> visited; // guards against `Prev` cycles
 
   while (position.has_value() && visited.insert(*position).second) {
-    auto [xref, trailer_dict] = read_xref_section(*position);
+    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.
+    if (!m_start_xref_position.has_value()) {
+      m_start_xref_position = position;
+      m_xref_kind = kind;
+    }
 
     // hybrid-reference file (7.5.8.4): the `XRefStm` entries fill in what
     // the classic table leaves absent or marks free, before older sections
     // are appended
     if (trailer_dict.has_key("XRefStm")) {
-      auto [stream_xref, stream_dict] =
+      const XrefSection stream_section =
           read_xref_section(trailer_dict["XRefStm"].as_integer());
-      xref.merge_hybrid(stream_xref);
+      xref.merge_hybrid(stream_section.xref);
     }
 
     result_xref.append(xref);
@@ -1877,6 +1899,10 @@ 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.
+  m_start_xref_position.reset();
+  m_xref_kind.reset();
 
   std::tie(m_xref, m_trailer) = parser().recover_xref();
 

src/odr/internal/pdf/pdf_document_parser.hpp

@@ -54,6 +54,31 @@ 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.
+  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.
+  [[nodiscard]] std::optional<std::uint32_t> start_xref_position() const;
+  /// How the newest cross-reference section is written. `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.
+  [[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.
+  [[nodiscard]] std::uint64_t highest_object_id() const;
+
   /// Whether the file declares an `/Encrypt` dictionary.
   [[nodiscard]] bool is_encrypted() const;
   /// Whether the file is encrypted and a decryptor is installed, so reads can
@@ -94,15 +119,21 @@ 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).
+  struct XrefSection {
+    Xref xref;
+    Dictionary trailer;
+    XrefKind kind{XrefKind::table};
+  };
+
   /// Read one cross-reference section (classic table or cross-reference
-  /// stream, ISO 32000-1 7.5.4 / 7.5.8) at `position`. The returned
-  /// dictionary is the trailer dictionary (the stream dictionary doubles as
-  /// one for cross-reference streams).
-  [[nodiscard]] std::pair<Xref, Dictionary>
-  read_xref_section(std::uint32_t position);
+  /// stream, ISO 32000-1 7.5.4 / 7.5.8) at `position`.
+  [[nodiscard]] XrefSection read_xref_section(std::uint32_t position);
 
   /// Walk the `startxref` → `Prev` chain and return the merged cross-reference
-  /// table together with the newest (first-seen) trailer dictionary.
+  /// table together with the newest (first-seen) trailer dictionary. Records
+  /// the newest section's position and kind on the way.
   [[nodiscard]] std::pair<Xref, Dictionary> read_trailer_chain();
 
   void recover_xref();
@@ -148,6 +179,8 @@ class DocumentParser {
   Xref m_xref;
   Dictionary m_trailer;
   bool m_recovered{false};
+  std::optional<std::uint32_t> m_start_xref_position;
+  std::optional<XrefKind> m_xref_kind;
 
   bool m_is_encrypted{false};
   std::optional<Authenticator> m_authenticator;

test/src/internal/pdf/pdf_document_parser.cpp

@@ -490,6 +490,56 @@ TEST(DocumentParser, simple_font_widths) {
 // `order-EK52VKL0.pdf` is an HTTP response saved as `.pdf`) has every xref
 // offset and the `startxref` shifted, so the chain walk fails. A forward scan
 // rebuilds the table from the actual object positions.
+namespace {
+
+/// The offset the file's own `startxref` names, which is what the parser must
+/// report back.
+std::uint32_t declared_start_xref(const std::string &pdf) {
+  const std::size_t pos = pdf.rfind("startxref\n") + std::strlen("startxref\n");
+  return static_cast<std::uint32_t>(
+      std::stoul(pdf.substr(pos, pdf.find('\n', pos) - pos)));
+}
+
+} // namespace
+
+// The facts a writer appending an incremental update needs: where the newest
+// cross-reference section sits, how it is written, and the last id in use.
+TEST(DocumentParser, parse_facts_of_classic_xref_table) {
+  const std::string pdf = two_object_mini_pdf(true);
+  const DocumentParser parser(std::make_unique<std::istringstream>(pdf));
+
+  EXPECT_FALSE(parser.is_recovered());
+  EXPECT_EQ(parser.xref_kind(), DocumentParser::XrefKind::table);
+  EXPECT_EQ(parser.start_xref_position(), declared_start_xref(pdf));
+  // the four objects the builder wrote
+  EXPECT_EQ(parser.highest_object_id(), 4u);
+}
+
+TEST(DocumentParser, parse_facts_of_xref_stream) {
+  const std::string pdf = two_object_mini_pdf(false);
+  const DocumentParser parser(std::make_unique<std::istringstream>(pdf));
+
+  EXPECT_FALSE(parser.is_recovered());
+  EXPECT_EQ(parser.xref_kind(), DocumentParser::XrefKind::stream);
+  EXPECT_EQ(parser.start_xref_position(), declared_start_xref(pdf));
+  // the four objects plus the cross-reference stream itself
+  EXPECT_EQ(parser.highest_object_id(), 5u);
+}
+
+// A rebuilt table has no section of the file's own to chain onto, so a writer
+// has nothing to point `/Prev` at — and must refuse the file.
+TEST(DocumentParser, parse_facts_of_a_recovered_file) {
+  const std::string pdf =
+      "HTTP/1.0 200 OK\r\nContent-Type: application/pdf\r\n\r\n" +
+      two_object_mini_pdf(true);
+  const DocumentParser parser(std::make_unique<std::istringstream>(pdf));
+
+  EXPECT_TRUE(parser.is_recovered());
+  EXPECT_FALSE(parser.start_xref_position().has_value());
+  EXPECT_FALSE(parser.xref_kind().has_value());
+  EXPECT_EQ(parser.highest_object_id(), 4u);
+}
+
 TEST(DocumentParser, recovers_from_prepended_garbage) {
   const std::string pdf =
       "HTTP/1.0 200 OK\r\nContent-Type: application/pdf\r\n\r\n" +