Skip to content

fix(pdf): do not discard a text-based document over one confirmed OCR page - #166

Open
STiFLeR7 wants to merge 2 commits into
firecrawl:mainfrom
STiFLeR7:fix/pdf-partial-ocr-page-does-not-discard-document
Open

fix(pdf): do not discard a text-based document over one confirmed OCR page#166
STiFLeR7 wants to merge 2 commits into
firecrawl:mainfrom
STiFLeR7:fix/pdf-partial-ocr-page-does-not-discard-document

Conversation

@STiFLeR7

@STiFLeR7 STiFLeR7 commented Sep 7, 2026

Copy link
Copy Markdown

Fixes #162.

Root cause

process_pdf_mem() can classify a document TextBased (producing full, real markdown for it) while separately confirming that one or a handful of pages still need OCR — most commonly a page whose only font is Identity-H/V with no ToUnicode CMap and no decodable fallback (pdf-inspector cannot map that page's CIDs to Unicode, independent of the rest of the document). This is a different code path than the whole-document ImageBased/Mixed classification #153 addresses — for a genuinely TextBased document, pages_needing_ocr can only become non-empty through this per-page font-decodability check (src/detector.rs's used_fonts_have_identity_h_no_tounicode/Phase 3 in pdf-inspector 1.14.2), not sampling noise.

to_markdown() treated any confirmed page in this list as fatal for the entire document:

if !pages.is_empty() {
    return Err(ConvertError::NeedsOcr { pages, page_count: result.page_count });
}

discarding result.markdown, which had already been extracted successfully for the whole document. One bad page condemned the whole file — exactly the "refusal is document-level and all-or-nothing" behavior described in the issue, and reproducible on real-world Prince-generated PDFs (the issue's salesforce_release_notes sample) where a Table-of-Contents-style page is rendered with a subset CID font.

Fix

Only fail the document when there is truly nothing readable (result.markdown is empty/None — the pre-existing fallback for genuinely scanned/image-based documents, unaffected by this change). Otherwise, keep the already-extracted markdown and report the confirmed pages via log::warn!, matching the pre-0.2.4 degrade-gracefully behavior for this case, while still keeping 0.2.4's original improvement (#140) of not silently dropping pages from documents that classify as Mixed/ImageBased overall (those still hard-fail exactly as before, since result.markdown is empty for them).

if !pages.is_empty() {
    if !has_text {
        return Err(ConvertError::NeedsOcr { pages, page_count: result.page_count });
    }
    log::warn!("{} of {} pages need OCR and were not extracted", pages.len(), result.page_count);
}

Test

Added tests/fixtures/pdf/handmade-partial-ocr.pdf: a document classified TextBased overall, with one page whose only font is Identity-H/no-ToUnicode/no-DescendantFonts (built with per-page /Resources, generated via tests/gen_fixtures.py's new partial_ocr_pdf()), surrounded by pages with real, substantial text. Verified this reproduces the exact bug mechanism against the real pdf-inspector 1.14.2 dependency (not mocked) before writing the fix.

Red-before-green: reverting just the pdf.rs change makes the new test fail with page 4 of 8 needs OCR (the whole document discarded); with the fix, the test passes and the surrounding pages' text comes through.

cargo fmt --check, cargo clippy --all-targets -- -D warnings, and cargo test --workspace (Rust core + wasm bindings; Python/Node bindings not re-verified locally, no code in those crates was touched) all clean — 287 unit/integration tests plus the fixture-corpus snapshot suite pass, including the existing scanned_pages_are_reported_not_dropped test (confirming genuinely image-based documents are unaffected by this change).


Summary by cubic

Fixes #162 by keeping the extracted markdown for text-based PDFs where only some pages need OCR, instead of failing the whole document. Pages that couldn't be decoded are now reported via a warning.

Behavior changes

  • Documents with no readable text at all still return NeedsOcr as before.
  • Image-based and mixed documents still hard-fail, preserving the no-silent-dropping behavior from feat(pdf): report pages that need ocr #140.
  • Adds a fixture and snapshot test covering a text-based document with one undecodable page; the test counts page blocks so any dropped page fails.

Written for commit 1061020. Summary will update on new commits.

Review in cubic

… page

process_pdf_mem() can classify a document TextBased and extract full
markdown for it, while separately confirming that one or a few pages
(e.g. an Identity-H font without a ToUnicode CMap and no decodable
fallback) still need OCR. to_markdown() treated any such confirmation
as fatal for the whole document, discarding markdown that had already
been extracted successfully.

Only fail the document when there is truly nothing readable (no
markdown at all); otherwise keep the extracted text and report the
confirmed pages via a warning, matching the pre-0.2.4 degrade-gracefully
behavior for this case while still avoiding the silent page-dropping
that motivated the original confirmation step.

Fixes firecrawl#162

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 5 files

Reply with feedback, questions, or to request a fix.

Fix all with cubic | Re-trigger cubic

Comment thread src/formats/pdf.rs
Comment thread tests/snapshots.rs Outdated
@mshresponse

Copy link
Copy Markdown

One data point on the real documents, in case it is useful when you review this.

Winter '27 page 3 is the page 0.2.4 refuses the whole 1,010 page document over. Running that page through pdf.js yields 4,727 characters of correct text, beginning:

CONTENTS
Salesforce Winter '27 Release Notes. . . . . . . . . . . . . . . . . .

0.2.3 rendered the same page as a normal contents list. That is a different engine, so it does not prove pdf-inspector can decode it, but the page does have recoverable text and does not need OCR. In 11 of the 13 documents listed in the issue, the objected page is the table of contents.

With this change those documents convert again, which is the important half of the regression. The page itself is still dropped from the output (the new fixture is 8 pages, the snapshot holds 7), so listing it under pages_needing_ocr stays inaccurate for these files, and 0.2.3's output for that page is not restored.

Happy to run a build against the 83 document corpus if that would help.

@STiFLeR7

STiFLeR7 commented Sep 7, 2026

Copy link
Copy Markdown
Author

Thanks for digging into the real corpus — that's a useful, precise data point.

To confirm exactly what this PR does and doesn't touch, from the actual diff:

let has_text = result.markdown.as_deref().is_some_and(|m| !m.trim().is_empty());
...
if !has_text {
    return Err(ConvertError::NeedsOcr { pages, page_count: result.page_count });
}
log::warn!("{} of {} pages need OCR and were not extracted", pages.len(), result.page_count);

This only changes the document-level decision: fail the whole conversion vs. keep whatever pdf_inspector::process_pdf_mem() already extracted. The confirmed pages' text is never in result.markdown to begin with — process_pdf_mem didn't extract it — so there's nothing in anydoc's own code for this PR to "restore" for that page. The committed fixture makes this explicit: it inserts one genuinely-undecodable page (Identity-H font, no ToUnicode CMap, no fallback) into 7 normal pages, and the snapshot holds exactly 7 blocks of text — the 8th is fully absent, matching what you saw on the Winter '27 doc (8-page fixture, 7-page snapshot).

Your TOC example is a different failure mode than the fixture models, though: pdf.js decodes 4,727 real characters from that page, so it isn't undecodable in the way the fixture's page is — pdf-inspector's own extraction is producing a false "needs OCR" on a page that does have recoverable text. anydoc has no independent text-extraction path of its own for this (see the doc comment on ConvertError::NeedsOcr: "which anydoc does not do") — it delegates entirely to pdf-inspector for PDFs, so there's no fallback here to reach into and pull that page's text from. Fixing that means improving pdf-inspector's own decode/OCR-need classification for whatever font/encoding shape that TOC page uses, which is a firecrawl/pdf-inspector issue, not something addressable inside to_markdown() here.

Given that, I'd treat this PR's scope as: stop losing entire documents to one-or-few confirmed pages (what #162 reported and what's fixed here), and treat the per-page false-positive-OCR-need as a separate, pdf-inspector-side issue — worth filing there with your TOC example and the "11 of 13 documents = table of contents" pattern, since that's a strong, specific signal about what shape of page it's misclassifying.

If you're up for running the 83-document corpus, that'd genuinely help both threads: against this branch it'd confirm how many of the 13 stop erroring out entirely (this PR's claim), and the per-document list of which specific pages still report needs_ocr would be great supporting data for a pdf-inspector issue on the classification false-positive itself. Happy to help write that up if you'd rather I did.

@mshresponse

Copy link
Copy Markdown

Ran the corpus against your branch. Built both sides from source, pdf-inspector 1.14.2 from crates.io.

main  261fc25   19 of 83 converted
PR    ef2a14c   32 of 83 converted

19 reproduces 0.2.4 and 32 reproduces 0.2.3. The 13 documents that gain are exactly the 13 listed in the issue, nothing that converted on main regresses, and the Winter '27 output is byte identical to the 0.2.3 output I archived on 2 September, sha256 9a37bc70. Use any of that in the PR if it helps get it reviewed.

Correcting myself: I said the objected page is still dropped. It is not. On these files process_pdf_mem extracts the flagged page into result.markdown, so keeping the markdown keeps the page, and all 13 have their page's content. I generalised from your fixture, where the page truly cannot be decoded, without running the corpus first.

The false positive is separate and still real, since those pages carry recoverable text and are still flagged. That one is yours to take upstream if you want it, and the data is here if it helps: 24 pages across the 13, every one yielding text through pdf.js, 307 to 5,697 characters, and 9 of them are not tables of contents (crm_analytics p1365 opens showTitle="false", p1613 is a filter expression). Say the word and I will paste the per page table.

The module comment still described the pre-firecrawl#162 behavior (always error on
any confirmed-OCR page). Update it to describe the actual gate: fail only
when no readable text remains, otherwise keep the readable pages and warn.

The partial-OCR regression test asserted `markdown.contains(...)` on a
sentence every readable page repeats, so it would still pass even if 6 of
the 7 readable pages were dropped. Assert the number of page blocks
matches instead - verified this catches the drop (a 1-page simulation
still satisfies the old substring check but reports block count 1, not 7).
@STiFLeR7

STiFLeR7 commented Sep 8, 2026

Copy link
Copy Markdown
Author

Thanks for running the full corpus and the self-correction — that's exactly the confirmation this needed.

Also pushed a follow-up commit (1061020) addressing cubic's review:

  • Updated the module doc comment, which still described the pre-0.2.4's needsOcr gate refuses born-digital PDFs that 0.2.3 converts correctly #162 behavior (always error on any confirmed-OCR page) rather than the actual gate this PR adds (fail only when no readable text remains at all).
  • Strengthened partial_ocr_document_keeps_its_readable_pages: the old assertion (markdown.contains("substantial born digital text")) would still pass even if 6 of the 7 readable pages were dropped, since every readable page repeats the same sentence. It now asserts the page-block count is exactly 7. Verified this actually catches the failure mode cubic described — a simulated 1-of-7-pages-survived string still satisfies the old substring check but reports a block count of 1, not 7.

On the false-positive OCR classification: agreed that's real and worth taking to firecrawl/pdf-inspector separately. Your per-page table (24 pages, 307–5,697 characters of recoverable text each, 9 of them not tables of contents) is strong, specific evidence for that report — happy to help write it up if useful, but since it's a decode/classification issue in that crate rather than anydoc's document-level gate, I'll leave it to you to file there with credit for the corpus work, unless you'd rather I did.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0.2.4's needsOcr gate refuses born-digital PDFs that 0.2.3 converts correctly

2 participants