diff --git a/docs/design/pdf-annotation.md b/docs/design/pdf-annotation.md index 912721f00..a638ff66f 100644 --- a/docs/design/pdf-annotation.md +++ b/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,15 +206,54 @@ 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 `` 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) @@ -219,13 +261,24 @@ 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. diff --git a/src/odr/internal/pdf/pdf_document_parser.cpp b/src/odr/internal/pdf/pdf_document_parser.cpp index 797553326..dd04136fe 100644 --- a/src/odr/internal/pdf/pdf_document_parser.cpp +++ b/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 DocumentParser::start_xref_position() const { + return m_start_xref_position; +} + +std::optional 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 +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 DocumentParser::read_trailer_chain() { @@ -1842,15 +1857,22 @@ std::pair DocumentParser::read_trailer_chain() { std::set 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(); diff --git a/src/odr/internal/pdf/pdf_document_parser.hpp b/src/odr/internal/pdf/pdf_document_parser.hpp index e148d8ab1..38c13c8d5 100644 --- a/src/odr/internal/pdf/pdf_document_parser.hpp +++ b/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 start_xref_position() const; + /// How the newest cross-reference section is written. `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. + [[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 - 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 read_trailer_chain(); void recover_xref(); @@ -148,6 +179,8 @@ class DocumentParser { Xref m_xref; Dictionary m_trailer; bool m_recovered{false}; + std::optional m_start_xref_position; + std::optional m_xref_kind; bool m_is_encrypted{false}; std::optional m_authenticator; diff --git a/test/src/internal/pdf/pdf_document_parser.cpp b/test/src/internal/pdf/pdf_document_parser.cpp index 463b01f34..fb8a7d4d4 100644 --- a/test/src/internal/pdf/pdf_document_parser.cpp +++ b/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::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(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(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(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" +