Repository navigation
fix(extract): a page with nothing to read gets the no_text reason in per-page extraction - #633
erkurtharun wants to merge 1 commit into
Conversation
![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 4 files
Confidence score: 4/5
src/detector.rscan label a whitespace-only page with an undecodable Identity-H or Type3 font asno_text, even thoughpage_ocr_reasonsprioritizessuspected_garbled_text. Keep the detector’s reason aligned with that higher-priority result.tests/integration_tests.rswon’t catch per-page extraction dropping the existingscannedreason. Assert thatpages.pages[0].ocr_reasonisSome(OCR_REASON_SCANNED).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/integration_tests.rs">
<violation number="1" location="tests/integration_tests.rs:2966">
P2: This test passes even if per-page extraction drops the existing `scanned` reason entirely. Assert that `pages.pages[0].ocr_reason` is `Some(OCR_REASON_SCANNED)` so the regression test protects the positive signal too.</violation>
</file>
<file name="src/detector.rs">
<violation number="1" location="src/detector.rs:2272">
P2: This marks a whitespace-only page using an undecodable Identity-H or Type3 font as `no_text`, although `page_ocr_reasons` prioritizes `suspected_garbled_text` for the same analysis. Carry that higher-priority reason into per-page routing before emitting `no_text`.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
View guided diff | Re-trigger cubic
| assert_ne!( | ||
| pages.pages[0].ocr_reason.as_deref(), | ||
| Some(OCR_REASON_NO_TEXT) | ||
| ); |
There was a problem hiding this comment.
P2: This test passes even if per-page extraction drops the existing scanned reason entirely. Assert that pages.pages[0].ocr_reason is Some(OCR_REASON_SCANNED) so the regression test protects the positive signal too.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At tests/integration_tests.rs, line 2966:
<comment>This test passes even if per-page extraction drops the existing `scanned` reason entirely. Assert that `pages.pages[0].ocr_reason` is `Some(OCR_REASON_SCANNED)` so the regression test protects the positive signal too.</comment>
<file context>
@@ -2929,6 +2930,124 @@ fn test_invisible_text_layer_under_covering_image_needs_ocr() {
+ }]);
+ let pages = extract_pages_markdown_mem(&buf, None).unwrap();
+ assert!(pages.pages[0].needs_ocr);
+ assert_ne!(
+ pages.pages[0].ocr_reason.as_deref(),
+ Some(OCR_REASON_NO_TEXT)
</file context>
| assert_ne!( | |
| pages.pages[0].ocr_reason.as_deref(), | |
| Some(OCR_REASON_NO_TEXT) | |
| ); | |
| assert_eq!( | |
| pages.pages[0].ocr_reason.as_deref(), | |
| Some(OCR_REASON_SCANNED) | |
| ); |
…per-page extraction Classification gives a page it sends to OCR with no extractable text, no image and no other reason the no_text reason, but extract_pages_markdown_mem only added the invisible_text_layer, suspected_garbled_text, scanned and vector_text reasons, so a blank page came back needing OCR with no reason at all. page_ocr_signals now reports a no_text signal when page_ocr_reasons gives the page no_text alone, from the same content pass, and the per-page path adds no_text when the page's Markdown is empty and no other reason applies. A page that shows only spaces through a font with no Unicode mapping stays suspected_garbled_text to classification and so is not called no_text here. needs_ocr and pages_needing_ocr are unchanged.
e394212 to
e385fd6
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| template_image_needs_ocr: needs_ocr_for_template_image, | ||
| has_vector_text: analysis.has_vector_text, | ||
| has_invisible_text_layer: analysis.has_invisible_text_layer, | ||
| no_text: page_ocr_reasons(&analysis) == [crate::OCR_REASON_NO_TEXT], |
There was a problem hiding this comment.
🟡 Unreadable font pages lose OCR reason
When an unmapped font renders only spaces, signals.no_text becomes false while the page's Markdown is empty. The extractor has no font-based reason to add, so it reports needs_ocr without any reason.
Learn more
Per-page extraction determines needs_ocr from its Markdown and its own reason list in extract_pages_markdown_mem_impl. A font without a Unicode mapping can produce only spaces, so neither Markdown decoding checks nor the template, vector, and invisible-layer signals add a reason. Classification's font check prevents page_ocr_reasons from returning no_text, making this new signal false; the page still routes to OCR with no explanation.
Example: For the unmapped Identity-H font showing <2020> in the added test, classification reports suspected_garbled_text. The extracted Markdown contains no readable text; per-page extraction can report needs_ocr: true with ocr_reason: None rather than a font-related reason.
Recommended fix: Carry the classification font reason through PageOcrSignals and add it to the per-page reason list when the font is undecodable. Assert the per-page reason explicitly in the new test, not just that it differs from no_text.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
0 issues found across 4 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Shadow auto-approve: would not auto-approve. Auto-approval blocked by 1 unresolved P0–P2 issue from previous reviews.
View guided diff | Re-trigger cubic
|
Closing this: I no longer use pdf-inspector and can't follow it through. |
Classification gives the
no_textreason to a page it sends to OCR with no text, no image and no other reason, butextract_pages_markdown_memreported such a page withneeds_ocrand no reason.The per-page path now reads the same signal from the page content and reports
no_textwhen the page's Markdown is empty and no other reason applies.needs_ocris unchanged.Tested with a blank page, which now matches
detect_pdf_mem, an image-only page, which does not get the reason, a page of outlined glyphs, which keepsvector_textalone, a page of spaces in a font with no Unicode mapping, which does not get it, and a page whose only text is a filled form field, which keeps its text and needs no OCR. Related to #319.