fix(detector): add character-volume floor so dense text in few operators is not read as a scan - #445
Conversation
…ors is not read as a scan A 25-page Arabic journal with a complete, extractable text layer was classified Mixed (0.7) with 24 of 25 pages flagged as needing OCR, even though pdftotext recovers the full text. The classifier used `text_operator_count` as its only proxy for text volume, and the sparse-text scan heuristics (`sparse_text_over_scan`, `looks_like_scan`, and `page_ocr_signals`'s `insufficient_text`) route any page below the per-page operator floor (min_text_ops_per_page.max(10)) to OCR when it also carries a template image. That proxy collapses to near zero for typesetters that emit a whole line or paragraph as a single TJ array — the norm for right-to-left / Arabic runs — so a page dense with genuine, decodable text drew only a handful of operators and looked identical to a near-empty scan. `alphanum_low` could not save it: raw CID operand bytes carry little ASCII alphanumeric diversity, and the existing decodable-font guard only applied when text_operator_count was already >= 10. This adds a minimum-evidence floor. `analyze_page_content` now tallies the total non-whitespace characters drawn by text-show operators (`text_char_count`), independent of operator count. A page clears the floor when it has decodable fonts and draws at least MIN_DECODABLE_TEXT_CHARS (200) characters; such pages are counted as text-bearing and are exempted from every sparse-text scan signal, so a real text layer packed into few operators stays native. The floor requires decodable fonts, so it never rescues an image-only scan or an undecodable-font page, and at 200 characters it sits well above incidental scan chrome (a masthead or date line is a few dozen chars) and comfortably below a body page of prose — the masthead-over-scan detection is preserved. Character counting is threaded through a new `scan_content_for_text_operators_counted`; the former `scan_content_for_text_operators` is retained as a test-only wrapper so the existing unit tests are unchanged. Perf: the reported ~14.7s classifyPdf latency is not addressed here. It is a separate hot path — `analyze_page_content` runs for nearly every page of a Mixed/large document (Phase 2/3 loops), each doing full content-stream decompression, recursive XObject scanning, per-page font map construction, and, for Identity-H fonts without ToUnicode, up to two embedded-font cmap parses per page via `embedded_font_has_cmap` with no cross-page cache. The reporter's file has a working text layer, so the dominant cost is most likely repeated decompression of large content streams or of an embedded CID font consulted through the cmap fallback. Confirming and caching that path needs the file and is left out of scope per the sampling-fix focus. Tests: adds test_dense_text_in_one_op_over_scan_stays_native (page-level page_ocr_signals) and test_dense_text_journal_not_routed_to_ocr (document-level detect_from_document → TextBased, no OCR pages).
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/detector.rs">
<violation number="1" location="src/detector.rs:247">
P1: When an image-only page retains an unused dense text Form XObject in `/Resources`, this predicate can classify the page as native text even though the Form is never drawn. Count dense evidence only from XObjects actually invoked by the page, or otherwise exclude unrendered resource Forms from this signal.</violation>
<violation number="2" location="src/detector.rs:1665">
P2: The new floor counts encoded bytes rather than displayed characters, so short UTF-16/CID or escaped text can cross the 200-character threshold and suppress OCR incorrectly. Count decoded code points or glyph/code units consistently before applying `MIN_DECODABLE_TEXT_CHARS`.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Fix all with cubic | Re-trigger cubic
| // into few operators (#213 minimum-evidence floor). The remaining | ||
| // guards still exclude vector-outlined and undecodable pages. | ||
| if (analysis.text_operator_count >= effective_min_ops | ||
| || page_has_dense_decodable_text(&analysis)) |
There was a problem hiding this comment.
P1: When an image-only page retains an unused dense text Form XObject in /Resources, this predicate can classify the page as native text even though the Form is never drawn. Count dense evidence only from XObjects actually invoked by the page, or otherwise exclude unrendered resource Forms from this signal.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/detector.rs, line 247:
<comment>When an image-only page retains an unused dense text Form XObject in `/Resources`, this predicate can classify the page as native text even though the Form is never drawn. Count dense evidence only from XObjects actually invoked by the page, or otherwise exclude unrendered resource Forms from this signal.</comment>
<file context>
@@ -239,7 +239,12 @@ pub(crate) fn detect_from_document(
+ // into few operators (#213 minimum-evidence floor). The remaining
+ // guards still exclude vector-outlined and undecodable pages.
+ if (analysis.text_operator_count >= effective_min_ops
+ || page_has_dense_decodable_text(&analysis))
&& !is_image_dominated
&& analysis.unique_text_chars >= 5
</file context>
There was a problem hiding this comment.
Not a regression of this PR — this is pre-existing behavior on main. scan_xobjects_in_resources has always walked every Form XObject in the resource dictionary regardless of invocation, and its text operators already feed text_operator_count, so an image-only page carrying an uninvoked dense-text Form already passed the operator gate (text_operator_count >= effective_min_ops) before this PR existed. The new character floor threads its tally through the exact same walk, so it neither widens nor narrows that behavior. Restricting the scan to Do-invoked forms would be a behavioral change to the established detector semantics (which earlier reviews deliberately shaped — see the P1/P2 scoped-resolution comments in that function) and belongs in its own change if wanted.
| for &ch in &content[k + 1..j] { | ||
| if !ch.is_ascii_whitespace() { | ||
| unique_chars.insert(ch); | ||
| *total_chars += 1; |
There was a problem hiding this comment.
P2: The new floor counts encoded bytes rather than displayed characters, so short UTF-16/CID or escaped text can cross the 200-character threshold and suppress OCR incorrectly. Count decoded code points or glyph/code units consistently before applying MIN_DECODABLE_TEXT_CHARS.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/detector.rs, line 1665:
<comment>The new floor counts encoded bytes rather than displayed characters, so short UTF-16/CID or escaped text can cross the 200-character threshold and suppress OCR incorrectly. Count decoded code points or glyph/code units consistently before applying `MIN_DECODABLE_TEXT_CHARS`.</comment>
<file context>
@@ -1582,6 +1662,7 @@ fn collect_text_chars_before(
for &ch in &content[k + 1..j] {
if !ch.is_ascii_whitespace() {
unique_chars.insert(ch);
+ *total_chars += 1;
}
}
</file context>
… text Address review feedback on the character-volume floor: - EarlyExit sampling no longer breaks on a page that clears the dense-text floor: such a page is text-bearing despite its low operator count, and breaking there ended sampling with text_ratio 1.0, misclassifying a dense-text-then-scan document as TextBased with no OCR pages. - Literal-string character counting now decodes escape sequences per the spec instead of counting raw bytes, so escapes with no visible glyphs (runs of \n, octal codes) cannot clear the floor and suppress OCR on a scanned page. Hex strings already decoded pairs and were unaffected.
There was a problem hiding this comment.
1 existing issue remains and no new issues found across 1 file (changes from recent commits).
Shadow auto-approve: would not auto-approve because issues were found.
Fix all with cubic | Re-trigger cubic
The floor's character tally is page-wide with no per-font attribution, so a page whose volume is drawn by an undecodable Identity-H font could qualify via a small decodable header font, letting garbled volume suppress OCR. The exemption now also requires that no used font is undecodable (Identity-H/V without ToUnicode or fallback, Type3 without ToUnicode, or an unresolvable font definition). Pages that fail the check simply fall back to the pre-floor routing.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/detector.rs">
<violation number="1" location="src/detector.rs:1359">
P2: The new `used_fonts_include_undecodable_text` duplicates the full traversal and classification logic of `used_fonts_have_decodable_text` (including the `identity_h_font_has_fallback` document lookups). Both are called inline in `analyze_page_content` when `text_ops > 0`, so every page pays the classification cost twice, and because the doc explicitly notes this is not the negation of the other, the two must be kept in lockstep or `page_has_dense_decodable_text` silently changes behavior. Merge them into a single pass that returns `(has_decodable, has_undecodable)` to remove the duplicate lookups and make the two checks structurally unable to diverge.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
| /// ([`page_has_dense_decodable_text`]) needs this distinction because its | ||
| /// character tally is page-wide — with any undecodable font in play, part of | ||
| /// that volume may be garbage, so the floor must not vouch for it. | ||
| fn used_fonts_include_undecodable_text( |
There was a problem hiding this comment.
P2: The new used_fonts_include_undecodable_text duplicates the full traversal and classification logic of used_fonts_have_decodable_text (including the identity_h_font_has_fallback document lookups). Both are called inline in analyze_page_content when text_ops > 0, so every page pays the classification cost twice, and because the doc explicitly notes this is not the negation of the other, the two must be kept in lockstep or page_has_dense_decodable_text silently changes behavior. Merge them into a single pass that returns (has_decodable, has_undecodable) to remove the duplicate lookups and make the two checks structurally unable to diverge.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/detector.rs, line 1359:
<comment>The new `used_fonts_include_undecodable_text` duplicates the full traversal and classification logic of `used_fonts_have_decodable_text` (including the `identity_h_font_has_fallback` document lookups). Both are called inline in `analyze_page_content` when `text_ops > 0`, so every page pays the classification cost twice, and because the doc explicitly notes this is not the negation of the other, the two must be kept in lockstep or `page_has_dense_decodable_text` silently changes behavior. Merge them into a single pass that returns `(has_decodable, has_undecodable)` to remove the duplicate lookups and make the two checks structurally unable to diverge.</comment>
<file context>
@@ -1327,6 +1345,48 @@ fn used_fonts_have_decodable_text(
+/// ([`page_has_dense_decodable_text`]) needs this distinction because its
+/// character tally is page-wide — with any undecodable font in play, part of
+/// that volume may be garbage, so the floor must not vouch for it.
+fn used_fonts_include_undecodable_text(
+ used_font_ids: &HashSet<ObjectId>,
+ font_map: &HashMap<ObjectId, FontInfo>,
</file context>
Address two review findings on the undecodable-font gate: - The gate previously judged every Tf-selected font, so an undecodable font that never draws text vetoed the dense-text rescue despite contributing no character volume. The content scanner now tracks the font current at each text-show operator, and the gate judges only those, treating text that cannot be attributed to a resolvable font as unvouchable. - Both font predicates now share a single per-font FontDecodability classification, so the any-decodable check and the no-undecodable gate can no longer drift apart, and the duplicated traversal logic is gone. Font accumulators are bundled into a FontUsage struct.
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
…treams The dense-text OCR rescue (firecrawl#213) judges a page by the fonts that actually draw characters. Two gaps in that attribution routed valid text pages to OCR. First, the scanner recorded the current font (or, absent a Tf, the unattributable-text flag) on every Tj/TJ regardless of whether the operand drew anything. An empty `()` Tj or a numeric-only `TJ` spacer under an undecodable font therefore added that font to the veto set even though it contributed no characters, so a page whose real volume came from a decodable font lost its rescue. Attribution now happens only when the show draws at least one counted non-whitespace character, measured as the delta in the character tally around collect_text_chars_before. Second, a page whose content is split across several streams (a /Contents array) reset the current-font and attribution state per stream, so a Tf in an earlier stream was forgotten before a later stream showed text. The show was then counted as font-less and set shows_text_without_font, denying the rescue. The PDF spec treats a page's content streams as one logical stream, so the current font now persists across them: current_font moves into TextShowAttribution (seeded and written back per scan), and analyze_page_content carries one attribution and one Tf-selected name set across the content-stream loop, resolving both once afterward against the shared page resource dict. Form XObject scans keep their own fresh state, so a Form's inherited-font case stays conservatively unattributable. Adds regression tests for both: an undecodable font showing only empty/numeric operands, and a two-stream page with the Tf in stream 1 and the dense body in stream 2.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/detector.rs">
<violation number="1" location="src/detector.rs:1625">
P2: When a later content stream starts a new `BT`, this carries the prior text object's font into the new object and can mark unattributed text as decodable, causing the dense-text floor to skip OCR. Track text-object boundaries and clear the carried font when `BT` begins, while preserving it only when the same text object continues across streams.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
| // Seed from the attribution so a `Tf` in an earlier content stream of the | ||
| // same page still governs shows in this one (see `TextShowAttribution`); | ||
| // written back before returning. A fresh attribution starts with no font. | ||
| let mut current_font: Option<Vec<u8>> = attribution.current_font.take(); |
There was a problem hiding this comment.
P2: When a later content stream starts a new BT, this carries the prior text object's font into the new object and can mark unattributed text as decodable, causing the dense-text floor to skip OCR. Track text-object boundaries and clear the carried font when BT begins, while preserving it only when the same text object continues across streams.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/detector.rs, line 1625:
<comment>When a later content stream starts a new `BT`, this carries the prior text object's font into the new object and can mark unattributed text as decodable, causing the dense-text floor to skip OCR. Track text-object boundaries and clear the carried font when `BT` begins, while preserving it only when the same text object continues across streams.</comment>
<file context>
@@ -1602,7 +1619,10 @@ fn scan_content_for_text_operators_counted(
+ // Seed from the attribution so a `Tf` in an earlier content stream of the
+ // same page still governs shows in this one (see `TextShowAttribution`);
+ // written back before returning. A fresh attribution starts with no font.
+ let mut current_font: Option<Vec<u8>> = attribution.current_font.take();
let mut text_ops = 0u32;
let image_count = 0u32;
</file context>
There was a problem hiding this comment.
Refuting this one — the premise contradicts the PDF specification. Per ISO 32000-1 §9.3.1, the font (Tf) is one of the text state parameters that live in the graphics state, and text state persists across text objects: BT resets only the text matrix and line matrix, and only a Q restoring an earlier q resets the font. BT /F1 12 Tf (a) Tj ET BT (b) Tj ET is legal PDF and (b) renders in F1.
This repo's own extractor agrees: the BT arm of the operator state machine in src/extractor/content_stream.rs resets only text_matrix, line_matrix, and text_rendering_mode — current_font is saved/restored exclusively by the q/Q arms. The scanner's carry models the same rule.
Clearing the carried font at BT would therefore introduce a regression, not fix one: a legitimate page that selects its font once and draws dense text across several BT blocks would have its later blocks marked unattributable, setting has_undecodable_text_fonts and denying the dense-text rescue — the exact failure class the last several commits eliminated.
Instead of changing the scanner I've pinned the correct behavior with a regression test in 0d6d56c: test_font_persists_across_text_objects_per_spec (font selected only in the first text object, dense body split across two BT/ET blocks, asserts the page keeps the rescue and stays native). Full suite green, clippy clean.
cubic flagged the current_font carry across a page's content streams, claiming a new BT must clear it or a stale font can wrongly mark text as decodable. Per ISO 32000-1 9.3.1/9.4.2 the font set by Tf is a text state parameter that lives in the graphics state and persists across BT/ET — only q/Q resets it; BT only resets the text and line matrices. The repo's own extractor (extractor/content_stream.rs) already agrees: its "BT" arm never touches current_font. Clearing the font at BT would wrongly veto the dense-text rescue for legitimate multi-text-object pages, so no production change is made. Add a regression test that pins the correct behavior instead.
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Shadow auto-approve: would not auto-approve. Auto-approval blocked by 4 unresolved issues from previous reviews.
Re-trigger cubic
1 participant
Addresses the misclassification half of #213.
Problem
A 25-page Arabic journal article with a complete, extractable text layer is classified
Mixed(confidence 0.7) with 24 of 25 pages flagged as needing OCR, whilepdftotextreads the same file perfectly. #213's first hypothesis — that the per-page needs-OCR verdict is made below any sensible minimum evidence — turns out to be exactly right, with a specific mechanism:The detector used
text_operator_countas its only proxy for a page's text volume. Every sparse-text scan heuristic routes a page below the per-page operator floor (min_text_ops_per_page.max(10)) toward OCR when the page also carries a template/background image — thepages_with_textgate, thealphanum_oktemplate-page counting, the Phase 2 Mixed routing branches (alphanum_low,sparse_text_over_scan,text_ops < min && has_images), andpage_ocr_signals.That proxy collapses for typesetters that emit a whole line or paragraph as a single
TJarray — the norm for RTL/Arabic runs. A page dense with genuine, decodable text draws only a handful of operators and becomes indistinguishable from a near-empty scan. Thealphanum_lowcheck cannot rescue it, because raw CID operand bytes have little ASCII-alphanumeric diversity, and the existing decodable-font guard only applied oncetext_operator_count >= 10.Change (all in
src/detector.rs)PageAnalysisgainstext_char_count: total non-whitespace characters drawn by text-show operators, tallied during the existing content scan (including recursive XObject scanning). The oldscan_content_for_text_operatorssignature is preserved as a test wrapper, so no existing unit tests changed.MIN_DECODABLE_TEXT_CHARS = 200characters counts as text-bearing, applied at all six sparse-text routing sites above.Tests
test_dense_text_in_one_op_over_scan_stays_native— page-level: template image plus a ~250-char body in oneTj; assertstext_operator_count < 10,text_char_count >= 200, andneeds_ocr = false.test_dense_text_journal_not_routed_to_ocr— document-level: 5-page journal (full-page background image + one-operator text layer per page); assertsTextBased, emptypages_needing_ocr,ocr_recommended = false.Both fail on the pre-fix logic and pass now. Full
cargo testsuite green (1017 lib + 165 integration + 2 doc-tests, 0 failures);cargo fmtandcargo clippy -- -D warningsclean.The latency half of #213 — documented, not fixed here
The ~15s
classifyPdfis a separate cost and this PR does not change it. The hot paths:analyze_page_contentruns for nearly every page of a Mixed/large document (Phase 2 + Phase 3b loops), each doing full content-stream decompression, recursive XObject scanning, per-page font-map construction with dict clones, and — for Identity-H fonts without ToUnicode — up to two embedded-font cmap parses per page viaembedded_font_has_cmap, with no cross-page cache and no early return. Confirming which of these dominates for the reporter's file needs the actual file; fixing it speculatively risked regressions, so it is left for a follow-up.Fixes the misclassification reported in #213.
Summary by cubic
Prevents dense RTL/Arabic pages packed into few operators from being flagged as scans by adding a character‑volume floor and judging only fonts that actually draw text. Old: operator‑count heuristics sent low‑op pages with images to OCR; new: pages with decodable fonts and ≥200 rendered characters count as text, bypass sparse‑text scan paths, and EarlyExit doesn’t break on them.
text_char_countand gatepage_has_dense_decodable_text(≥200 chars, has decodable fonts, no undecodable font that draws, no unattributable text). Applied to the pages‑with‑text gate,alphanum_low,looks_like_scan,sparse_text_over_scan,page_ocr_signals, and the EarlyExit break.Tj/TJrenders characters; empty()and numeric‑onlyTJdon’t add vetoing fonts. CarryTfacross a page’s/Contentsarray; treat unattributable shows as undecodable for the gate. Share a singleFontDecodabilityclassifier; track selected vs showing fonts separately.BT/ETper spec. No config or API changes; performance unchanged.Written for commit 0d6d56c. Summary will update on new commits.