Bound column-detection histogram and harden coordinate handling - #328
Bound column-detection histogram and harden coordinate handling#328abimaelmartell wants to merge 3 commits into
Conversation
Derive the projection histogram from a clamped bin count and skip non-finite page widths. Extreme or malformed text-item coordinates (from the content-stream text matrix) could otherwise drive a very large allocation. 65,536 bins is ~9x the largest legal page, so real layouts are unaffected. Adds regression tests. Co-authored-by: Abimael Martell <abimaelmartell@users.noreply.github.com>
d903564 to
0eda2eb
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue 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/extractor/layout.rs">
<violation number="1" location="src/extractor/layout.rs:61">
P2: A NaN X coordinate alongside valid items still enters histogram detection because the min/max folds discard a lone NaN; treat non-finite item X/effective widths as malformed before building bins so this path consistently falls back to one region.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Fix all with cubic | Re-trigger cubic
|
|
||
| let page_width = x_max - x_min; | ||
| if page_width < 200.0 { | ||
| if !page_width.is_finite() || page_width < 200.0 { |
There was a problem hiding this comment.
P2: A NaN X coordinate alongside valid items still enters histogram detection because the min/max folds discard a lone NaN; treat non-finite item X/effective widths as malformed before building bins so this path consistently falls back to one region.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/extractor/layout.rs, line 61:
<comment>A NaN X coordinate alongside valid items still enters histogram detection because the min/max folds discard a lone NaN; treat non-finite item X/effective widths as malformed before building bins so this path consistently falls back to one region.</comment>
<file context>
@@ -48,8 +48,17 @@ pub(crate) fn detect_columns(
+
let page_width = x_max - x_min;
- if page_width < 200.0 {
+ if !page_width.is_finite() || page_width < 200.0 {
return vec![ColumnRegion { x_min, x_max }];
}
</file context>
| if !page_width.is_finite() || page_width < 200.0 { | |
| if !page_width.is_finite() || page_items.iter().any(|item| !item.x.is_finite() || !effective_width(item).is_finite()) || page_width < 200.0 { |
Items at NaN/inf positions are now skipped when folding the page bounds, so a malformed coordinate can no longer escape as a ColumnRegion boundary, and an all-non-finite page returns no columns. Bad items are dropped individually rather than failing the page, so one stray glyph does not disable column detection. Addresses review feedback on the finite-width guard. Co-authored-by: Abimael Martell <abimaelmartell@users.noreply.github.com>
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 1 unresolved issue from previous reviews.
Re-trigger cubic
|
@abimaelmartell I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 1 file
Shadow auto-approve: would not auto-approve because issues were found.
Fix all with cubic | Re-trigger cubic
Gutter margins, spanning-item width and the XY-cut margin are all fractions of page_width, so a single far-but-finite item (x=50_000 is enough) set the scale for the whole page: real gutters fell inside the rejected margin band and a genuine two-column page collapsed to one region. When the span exceeds one legal page (14_400 units), re-derive the bounds from items clustered around the median x. Outliers keep their text because column assignment buckets by nearest overlap. The MAX_BINS ceiling stays as an allocation bound that does not depend on this heuristic. Co-authored-by: Abimael Martell <abimaelmartell@users.noreply.github.com>
There was a problem hiding this comment.
3 issues 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/extractor/layout.rs">
<violation number="1" location="src/extractor/layout.rs:47">
P2: The median-clustering trim can silently discard real content on valid large-format documents, and the justification comment is not accurate for current PDFs. The 14_400-unit (200-inch) cap applies to the default user space in pre-1.6 PDF; ISO 32000-2 (PDF 2.0) sets no page-size limit and since PDF 1.6 the `UserUnit` key lets a page legitimately exceed it (cartography, posters, engineering drawings). Because the trim fires on any page whose content spans >14_400 user units — not just on malformed outliers — a real multi-region layout spread wider than that gets reduced to the ~14_400-wide cluster around the global median, and any genuine items/regions outside the trimmed `x_min..x_max` are dropped from column detection (the returned `ColumnRegion`s, and downstream reading order, are all derived from the trimmed bounds). This is extra behavior on top of the `MAX_BINS` hard cap, which already bounds memory safely for adversarial inputs. Consider scoping the trim to co-ordinates that are actually implausible relative to the MediaBox (which you have access to here) rather than a fixed 14_400 constant, and at minimum correct the comment so the threshold is documented as a heuristic rather than a spec cap.</violation>
<violation number="2" location="src/extractor/layout.rs:89">
P1: A malformed huge text width at an ordinary X still poisons `x_max`, so gutter margins scale to the huge extent and can collapse a real multi-column page; trim/filter both item edges (or reject oversized spans) when recomputing bounds.</violation>
<violation number="3" location="src/extractor/layout.rs:2664">
P3: The new regression assertion compares against a hard-coded 14_400.0 while the production threshold lives in the `MAX_PAGE_EXTENT` const. If the trim threshold is ever tuned, the test will silently drift (it asserts the emitted region stays within one page, which should mirror that constant). Consider referencing the constant or at least a named local so the test and the invariant it guards stay coupled.</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
| xs.sort_by(f32::total_cmp); | ||
| let median = xs[xs.len() / 2]; | ||
|
|
||
| let (lo, hi) = bounds(&|x| (x - median).abs() <= MAX_PAGE_EXTENT); |
There was a problem hiding this comment.
P1: A malformed huge text width at an ordinary X still poisons x_max, so gutter margins scale to the huge extent and can collapse a real multi-column page; trim/filter both item edges (or reject oversized spans) when recomputing bounds.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/extractor/layout.rs, line 89:
<comment>A malformed huge text width at an ordinary X still poisons `x_max`, so gutter margins scale to the huge extent and can collapse a real multi-column page; trim/filter both item edges (or reject oversized spans) when recomputing bounds.</comment>
<file context>
@@ -41,35 +41,71 @@ pub(crate) fn detect_columns(
+ xs.sort_by(f32::total_cmp);
+ let median = xs[xs.len() / 2];
+
+ let (lo, hi) = bounds(&|x| (x - median).abs() <= MAX_PAGE_EXTENT);
+ if lo.is_finite() && hi.is_finite() {
+ debug!(
</file context>
| const MAX_PAGE_EXTENT: f32 = 14_400.0; | ||
|
|
||
| // Find page bounds. Coordinates come straight from the content-stream text |
There was a problem hiding this comment.
P2: The median-clustering trim can silently discard real content on valid large-format documents, and the justification comment is not accurate for current PDFs. The 14_400-unit (200-inch) cap applies to the default user space in pre-1.6 PDF; ISO 32000-2 (PDF 2.0) sets no page-size limit and since PDF 1.6 the UserUnit key lets a page legitimately exceed it (cartography, posters, engineering drawings). Because the trim fires on any page whose content spans >14_400 user units — not just on malformed outliers — a real multi-region layout spread wider than that gets reduced to the ~14_400-wide cluster around the global median, and any genuine items/regions outside the trimmed x_min..x_max are dropped from column detection (the returned ColumnRegions, and downstream reading order, are all derived from the trimmed bounds). This is extra behavior on top of the MAX_BINS hard cap, which already bounds memory safely for adversarial inputs. Consider scoping the trim to co-ordinates that are actually implausible relative to the MediaBox (which you have access to here) rather than a fixed 14_400 constant, and at minimum correct the comment so the threshold is documented as a heuristic rather than a spec cap.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/extractor/layout.rs, line 47:
<comment>The median-clustering trim can silently discard real content on valid large-format documents, and the justification comment is not accurate for current PDFs. The 14_400-unit (200-inch) cap applies to the default user space in pre-1.6 PDF; ISO 32000-2 (PDF 2.0) sets no page-size limit and since PDF 1.6 the `UserUnit` key lets a page legitimately exceed it (cartography, posters, engineering drawings). Because the trim fires on any page whose content spans >14_400 user units — not just on malformed outliers — a real multi-region layout spread wider than that gets reduced to the ~14_400-wide cluster around the global median, and any genuine items/regions outside the trimmed `x_min..x_max` are dropped from column detection (the returned `ColumnRegion`s, and downstream reading order, are all derived from the trimmed bounds). This is extra behavior on top of the `MAX_BINS` hard cap, which already bounds memory safely for adversarial inputs. Consider scoping the trim to co-ordinates that are actually implausible relative to the MediaBox (which you have access to here) rather than a fixed 14_400 constant, and at minimum correct the comment so the threshold is documented as a heuristic rather than a spec cap.</comment>
<file context>
@@ -41,35 +41,71 @@ pub(crate) fn detect_columns(
+ // The PDF specification caps a page at 14_400 units (200 inches). Content
+ // may sit a little outside the MediaBox, but a span far beyond one whole
+ // page means stray items, not a real layout.
+ const MAX_PAGE_EXTENT: f32 = 14_400.0;
+
// Find page bounds. Coordinates come straight from the content-stream text
</file context>
| const MAX_PAGE_EXTENT: f32 = 14_400.0; | |
| // Find page bounds. Coordinates come straight from the content-stream text | |
| // Historical default user space is 1/72in, so 14_400 units = 200in. PDF 1.6+ | |
| // may scale pages with UserUnit and PDF 2.0 imposes no size cap, so treat this | |
| // only as a robustness heuristic for stray far-outlier items, not a spec limit. | |
| const MAX_PAGE_EXTENT: f32 = 14_400.0; |
| ); | ||
| for col in &cols { | ||
| assert!( | ||
| col.x_max - col.x_min <= 14_400.0, |
There was a problem hiding this comment.
P3: The new regression assertion compares against a hard-coded 14_400.0 while the production threshold lives in the MAX_PAGE_EXTENT const. If the trim threshold is ever tuned, the test will silently drift (it asserts the emitted region stays within one page, which should mirror that constant). Consider referencing the constant or at least a named local so the test and the invariant it guards stay coupled.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/extractor/layout.rs, line 2664:
<comment>The new regression assertion compares against a hard-coded 14_400.0 while the production threshold lives in the `MAX_PAGE_EXTENT` const. If the trim threshold is ever tuned, the test will silently drift (it asserts the emitted region stays within one page, which should mirror that constant). Consider referencing the constant or at least a named local so the test and the invariant it guards stay coupled.</comment>
<file context>
@@ -2606,16 +2642,49 @@ mod tests {
+ );
+ for col in &cols {
+ assert!(
+ col.x_max - col.x_min <= 14_400.0,
+ "bad_x {bad_x}: region {}..{} exceeds one page",
+ col.x_min,
</file context>
2 participants
Summary
detect_columns()insrc/extractor/layout.rsderives its page bounds from text-item coordinates, which come straight from the content-stream text matrix (Tm/Td). A document can therefore place a run at any position, and those bounds fed two problems:page_width / BIN_WIDTH) had no upper bound, so an extreme coordinate drove an arbitrarily largevec![0u32; num_bins].page_width, so one far item shrank the effective detection window and silently disabled column detection for the whole page.Changes
MAX_BINS = 65_536ceiling (~128k points atBIN_WIDTH = 2.0, roughly 9x the largest legal page). Kept as a hard allocation bound that does not depend on the heuristics below staying correct.f32::min/f32::maxwhile an inf propagates through and can escape as aColumnRegionboundary. Bad items are skipped individually rather than failing the page.MAX_PAGE_EXTENT = 14_400units, the spec maximum), re-derive the bounds from items clustered around the medianx. Outliers keep their text — column assignment buckets by nearest overlap, not containment.len() <= 1as single-column).saturating_subfor the siblingVec::with_capacityinsplit_column_stragglers()as cheap defence-in-depth (capacity is only a hint).Testing
cargo fmt,cargo clippy -- -D warnings,cargo testall pass (851 lib + 152 integration + doc tests).+inf,-inf, NaN); an all-non-finite page yields no columns; a two-column page keeps both columns across NaN, inf,50_000and1e12outliers; a wide-format but legal page (8_000pt) is not trimmed.Output-stability check
The
pdf-evalssnapshot suite is not reachable from this environment, sobench.py test/bench.py scorehave not been run. As a substitute, both binaries were built (mainvs. this branch) and run over all 27 fixture PDFs intests/fixtures/:That matches the design: every new branch is gated behind a condition well-formed PDFs cannot meet —
MAX_BINSneeds a span above 131,072pt, trimming needs one above 14,400pt (the spec page maximum), and the non-finite filters need a NaN/inf coordinate. Snapshots should only move for a document that genuinely has a stray off-page item, where the expected change is recovering column detection that is silently lost today.Summary by cubic
Bound the column-detection histogram, trim far-outlier coordinates, and ignore non-finite positions to prevent huge allocations and invalid region bounds. Real layouts are unchanged; one stray glyph or outlier no longer breaks column detection.
split_column_stragglers(); add tests for extreme/outlier coords, non-finite pages, and one-bad-item resilience.Written for commit d79bd93. Summary will update on new commits.