fix: sort panic and missing CID garbage check in extract_text_in_regions_mem
Two bugs in collect_text_in_region / extract_text_in_regions_mem: 1. The threshold-based sort comparator in collect_text_in_region was not transitive, causing Rust's sort to panic on certain PDFs. Replaced with strict total_cmp ordering — the line-grouping phase already handles fuzzy Y matching via threshold. 2. The needs_ocr check was missing is_cid_garbage, so Identity-H fonts with CID garbage (C1 control chars, high Latin mojibake) could pass all quality checks and be served as real text with needs_ocr=false. Also adds 7 integration tests for extract_text_in_regions_mem (previously had zero coverage): basic extraction, Identity-H needs_ocr, multiple regions, nonexistent page, empty region, invalid input, and a fast-vs-normal comparison test across all text-based fixtures. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.6
parent
57673ebb69
commit
30244078d8
+5
-22
@@ -406,6 +406,7 @@ pub fn extract_text_in_regions_mem(
|
||||
let needs_ocr = text.trim().is_empty()
|
||||
|| page_has_gid
|
||||
|| is_garbage_text(&text)
|
||||
|| is_cid_garbage(&text)
|
||||
|| detect_encoding_issues(&text);
|
||||
|
||||
page_results.push(RegionText { text, needs_ocr });
|
||||
@@ -482,29 +483,11 @@ pub fn collect_text_in_region(
|
||||
}
|
||||
|
||||
// Sort top→bottom (descending Y in bottom-left coords), then left→right.
|
||||
// Uses total_cmp to avoid panics on NaN values from bogus font metrics.
|
||||
// Uses strict total_cmp ordering to guarantee transitivity (required by
|
||||
// Rust's sort). The line-grouping phase below handles fuzzy Y matching.
|
||||
matched.sort_by(|a, b| {
|
||||
let fs_a = if a.font_size.is_finite() {
|
||||
a.font_size
|
||||
} else {
|
||||
0.0
|
||||
};
|
||||
let fs_b = if b.font_size.is_finite() {
|
||||
b.font_size
|
||||
} else {
|
||||
0.0
|
||||
};
|
||||
let line_threshold = fs_a.max(fs_b) * 0.5;
|
||||
let ay = if a.y.is_finite() { a.y } else { 0.0 };
|
||||
let by = if b.y.is_finite() { b.y } else { 0.0 };
|
||||
let y_diff = by - ay; // descending Y = top to bottom
|
||||
if y_diff.abs() < line_threshold {
|
||||
let ax = if a.x.is_finite() { a.x } else { 0.0 };
|
||||
let bx = if b.x.is_finite() { b.x } else { 0.0 };
|
||||
ax.total_cmp(&bx)
|
||||
} else {
|
||||
by.total_cmp(&ay)
|
||||
}
|
||||
b.y.total_cmp(&a.y) // descending Y = top to bottom
|
||||
.then(a.x.total_cmp(&b.x)) // ascending X = left to right
|
||||
});
|
||||
|
||||
// Group into lines and join
|
||||
|
||||
+190
-2
@@ -4,9 +4,11 @@ use pdf_inspector::detector::{DetectionConfig, ScanStrategy};
|
||||
use pdf_inspector::extractor::group_into_lines;
|
||||
use pdf_inspector::types::TextLine;
|
||||
use pdf_inspector::{
|
||||
detect_pdf_type, extract_text, extract_text_with_positions, process_pdf_with_options,
|
||||
to_markdown, MarkdownOptions, PdfError, PdfOptions, PdfType, TextItem,
|
||||
detect_pdf_type, extract_text, extract_text_in_regions_mem, extract_text_with_positions,
|
||||
process_pdf_mem, process_pdf_with_options, to_markdown, MarkdownOptions, PdfError, PdfOptions,
|
||||
PdfType, TextItem,
|
||||
};
|
||||
use std::collections::HashSet;
|
||||
|
||||
// Helper to create test TextItems
|
||||
fn make_text_item(text: &str, x: f32, y: f32, font_size: f32, page: u32) -> TextItem {
|
||||
@@ -1107,3 +1109,189 @@ fn test_rotated_table_layout_correction() {
|
||||
"District data should be in a markdown table row"
|
||||
);
|
||||
}
|
||||
|
||||
// =========================================================================
|
||||
// extract_text_in_regions_mem tests
|
||||
// =========================================================================
|
||||
|
||||
/// Build full-page region args for `page_count` pages.
|
||||
/// Uses a generously large bbox (1200x1200) to capture any page size.
|
||||
fn full_page_regions(page_count: u32) -> Vec<(u32, Vec<[f32; 4]>)> {
|
||||
(0..page_count)
|
||||
.map(|p| (p, vec![[0.0, 0.0, 1200.0, 1200.0]]))
|
||||
.collect()
|
||||
}
|
||||
|
||||
/// Normalize text for comparison: lowercase, strip non-alphanumeric, split into words.
|
||||
fn normalize_words(text: &str) -> HashSet<String> {
|
||||
text.split(|c: char| !c.is_alphanumeric())
|
||||
.map(|w| w.to_lowercase())
|
||||
.filter(|w| w.len() > 3)
|
||||
.collect()
|
||||
}
|
||||
|
||||
/// Fraction of normalized words in `a` that also appear in `b`.
|
||||
fn word_overlap_ratio(a: &str, b: &str) -> f64 {
|
||||
let words_a = normalize_words(a);
|
||||
if words_a.is_empty() {
|
||||
return if normalize_words(b).is_empty() {
|
||||
1.0
|
||||
} else {
|
||||
0.0
|
||||
};
|
||||
}
|
||||
let words_b = normalize_words(b);
|
||||
let overlap = words_a.intersection(&words_b).count();
|
||||
overlap as f64 / words_a.len() as f64
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_extract_regions_mem_basic_text_pdf() {
|
||||
let buf = std::fs::read("tests/fixtures/nexo-price-en.pdf").unwrap();
|
||||
let result = process_pdf_mem(&buf).unwrap();
|
||||
let page_count = result.page_count;
|
||||
|
||||
let regions = extract_text_in_regions_mem(&buf, &full_page_regions(page_count)).unwrap();
|
||||
assert_eq!(regions.len(), page_count as usize);
|
||||
|
||||
// Each result should have exactly 1 region (we passed one per page)
|
||||
for r in ®ions {
|
||||
assert_eq!(r.regions.len(), 1);
|
||||
}
|
||||
|
||||
// First page should have non-empty text
|
||||
let first = ®ions[0].regions[0];
|
||||
assert!(!first.text.trim().is_empty(), "First page should have text");
|
||||
assert_eq!(regions[0].page, 0);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_extract_regions_mem_identity_h_needs_ocr() {
|
||||
let buf = std::fs::read("tests/fixtures/shinagawa_identity_h.pdf").unwrap();
|
||||
let regions =
|
||||
extract_text_in_regions_mem(&buf, &[(0, vec![[0.0, 0.0, 1200.0, 1200.0]])]).unwrap();
|
||||
assert_eq!(regions.len(), 1);
|
||||
assert!(
|
||||
regions[0].regions[0].needs_ocr,
|
||||
"Identity-H font without ToUnicode should trigger needs_ocr"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_extract_regions_mem_multiple_regions_per_page() {
|
||||
let buf = std::fs::read("tests/fixtures/nexo-price-en.pdf").unwrap();
|
||||
let regions = extract_text_in_regions_mem(
|
||||
&buf,
|
||||
&[(
|
||||
0,
|
||||
vec![
|
||||
[0.0, 0.0, 300.0, 100.0], // small top-left
|
||||
[0.0, 0.0, 1200.0, 1200.0], // full page
|
||||
],
|
||||
)],
|
||||
)
|
||||
.unwrap();
|
||||
|
||||
assert_eq!(regions.len(), 1);
|
||||
assert_eq!(regions[0].regions.len(), 2);
|
||||
|
||||
let small_len = regions[0].regions[0].text.len();
|
||||
let full_len = regions[0].regions[1].text.len();
|
||||
assert!(
|
||||
full_len >= small_len,
|
||||
"Full-page region ({full_len}) should have at least as much text as small region ({small_len})"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_extract_regions_mem_nonexistent_page() {
|
||||
let buf = std::fs::read("tests/fixtures/nexo-price-en.pdf").unwrap();
|
||||
let regions =
|
||||
extract_text_in_regions_mem(&buf, &[(9999, vec![[0.0, 0.0, 1200.0, 1200.0]])]).unwrap();
|
||||
assert_eq!(regions.len(), 1);
|
||||
assert!(
|
||||
regions[0].regions[0].needs_ocr,
|
||||
"Nonexistent page should trigger needs_ocr"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_extract_regions_mem_empty_region() {
|
||||
let buf = std::fs::read("tests/fixtures/nexo-price-en.pdf").unwrap();
|
||||
let regions = extract_text_in_regions_mem(&buf, &[(0, vec![[0.0, 0.0, 0.0, 0.0]])]).unwrap();
|
||||
assert_eq!(regions.len(), 1);
|
||||
assert!(
|
||||
regions[0].regions[0].needs_ocr,
|
||||
"Zero-area region should trigger needs_ocr"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_extract_regions_mem_not_a_pdf() {
|
||||
let result = extract_text_in_regions_mem(b"not a pdf", &[(0, vec![[0.0, 0.0, 100.0, 100.0]])]);
|
||||
assert!(result.is_err(), "Non-PDF input should return an error");
|
||||
}
|
||||
|
||||
// =========================================================================
|
||||
// Fast vs normal extraction comparison
|
||||
// =========================================================================
|
||||
|
||||
/// For each text-based fixture PDF, compare `extract_text_in_regions_mem` (fast path)
|
||||
/// against `process_pdf_mem` (normal path). If the fast path claims needs_ocr=false
|
||||
/// for a page, verify the extracted text has meaningful overlap with the normal
|
||||
/// markdown output — catching silent quality regressions.
|
||||
#[test]
|
||||
fn test_extract_regions_fast_vs_normal_comparison() {
|
||||
let fixtures = [
|
||||
"tests/fixtures/nexo-price-en.pdf",
|
||||
"tests/fixtures/td9264.pdf",
|
||||
"tests/fixtures/p1244-1996.pdf",
|
||||
"tests/fixtures/real-estate-pricing.pdf",
|
||||
"tests/fixtures/2013-app2.pdf",
|
||||
"tests/fixtures/firecrawl_docs_tagged.pdf",
|
||||
"tests/fixtures/thermo-freon12.pdf",
|
||||
];
|
||||
|
||||
for fixture in &fixtures {
|
||||
let buf = std::fs::read(fixture).unwrap();
|
||||
let normal = process_pdf_mem(&buf).unwrap();
|
||||
let normal_md = normal.markdown.as_deref().unwrap_or("");
|
||||
let page_count = normal.page_count;
|
||||
let ocr_pages: HashSet<u32> = normal.pages_needing_ocr.iter().copied().collect();
|
||||
|
||||
let regions = extract_text_in_regions_mem(&buf, &full_page_regions(page_count)).unwrap();
|
||||
|
||||
assert_eq!(
|
||||
regions.len(),
|
||||
page_count as usize,
|
||||
"{fixture}: result count should match page count"
|
||||
);
|
||||
|
||||
for pr in ®ions {
|
||||
let region = &pr.regions[0];
|
||||
if !region.needs_ocr && !region.text.trim().is_empty() {
|
||||
// Fast path claims this text is trustworthy.
|
||||
// Check that its words appear in the normal markdown output.
|
||||
let overlap = word_overlap_ratio(®ion.text, normal_md);
|
||||
assert!(
|
||||
overlap >= 0.3,
|
||||
"{fixture} page {}: fast path says needs_ocr=false but only {:.0}% word \
|
||||
overlap with normal extraction (threshold 30%). \
|
||||
Fast text sample: {:?}",
|
||||
pr.page,
|
||||
overlap * 100.0,
|
||||
®ion.text[..region.text.len().min(200)],
|
||||
);
|
||||
}
|
||||
|
||||
// If fast path flags needs_ocr but normal path didn't, that's overly
|
||||
// conservative but not a bug — just worth knowing.
|
||||
if region.needs_ocr && !ocr_pages.contains(&(pr.page + 1)) {
|
||||
eprintln!(
|
||||
"INFO: {fixture} page {}: fast path says needs_ocr=true but normal path extracted fine (conservative, not a bug)",
|
||||
pr.page,
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user