fix(pdf): do not discard a text-based document over one confirmed OCR page - #166
fix(pdf): do not discard a text-based document over one confirmed OCR page#166STiFLeR7 wants to merge 2 commits into
Conversation
… 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
There was a problem hiding this comment.
All reported issues were addressed across 5 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
|
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: 0.2.3 rendered the same page as a normal contents list. That is a different engine, so it does not prove 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 Happy to run a build against the 83 document corpus if that would help. |
|
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 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 — 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, 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 |
|
Ran the corpus against your branch. Built both sides from source, 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 Correcting myself: I said the objected page is still dropped. It is not. On these files 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 ( |
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).
|
Thanks for running the full corpus and the self-correction — that's exactly the confirmation this needed. Also pushed a follow-up commit (
On the false-positive OCR classification: agreed that's real and worth taking to |
Fixes #162.
Root cause
process_pdf_mem()can classify a documentTextBased(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 noToUnicodeCMap and no decodable fallback (pdf-inspectorcannot map that page's CIDs to Unicode, independent of the rest of the document). This is a different code path than the whole-documentImageBased/Mixedclassification#153addresses — for a genuinelyTextBaseddocument,pages_needing_ocrcan only become non-empty through this per-page font-decodability check (src/detector.rs'sused_fonts_have_identity_h_no_tounicode/Phase 3 inpdf-inspector1.14.2), not sampling noise.to_markdown()treated any confirmed page in this list as fatal for the entire document: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'ssalesforce_release_notessample) 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.markdownis 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 vialog::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 asMixed/ImageBasedoverall (those still hard-fail exactly as before, sinceresult.markdownis empty for them).Test
Added
tests/fixtures/pdf/handmade-partial-ocr.pdf: a document classifiedTextBasedoverall, with one page whose only font is Identity-H/no-ToUnicode/no-DescendantFonts (built with per-page/Resources, generated viatests/gen_fixtures.py's newpartial_ocr_pdf()), surrounded by pages with real, substantial text. Verified this reproduces the exact bug mechanism against the realpdf-inspector1.14.2 dependency (not mocked) before writing the fix.Red-before-green: reverting just the
pdf.rschange makes the new test fail withpage 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, andcargo 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 existingscanned_pages_are_reported_not_droppedtest (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
NeedsOcras before.Written for commit 1061020. Summary will update on new commits.