feat(render): add optional selected-page rasterization - #280
feat(render): add optional selected-page rasterization#280massimodeluisa wants to merge 5 commits into
Conversation
New off-by-default `render` feature with a single entry point, `render_pages_mem`: rasterizes selected zero-based pages through Hayro into opaque RGBA8 buffers, caller order and duplicates preserved, 200 DPI by default, 300 max. DPI, page dimensions, per-page pixels, combined output bytes and page-entry count are all validated before the first page renders. Interpreter font and image failures surface as typed per-page `RenderWarning`s; `RenderError` is a separate non-exhaustive enum. Includes renderer unit tests, a shared image fixture reused by the wasm runtime tests, and an ignored checksum-verified py-pdf corpus suite.
Adds render-feature CI: native tests, lints, docs and a release build with the feature on, an exact Rust 1.92.0 check for native and wasm32, release-mode wasm runtime tests on Node and headless Chrome, and the pinned py-pdf corpus job with per-file SHA-256 verification.
![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 16 files
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="tests/support/render_fixture.rs">
<violation number="1" location="tests/support/render_fixture.rs:29">
P3: The new fixture's add_object closure and xref/trailer generation duplicate the identical logic already in make_solid_page_pdf_with_page_options (tests/render_tests.rs). Both are new in this PR; extract the shared add_object + xref/trailer writer into a common helper the two builders reuse, so offset/link/trailer fixes don't have to be kept in sync in two places.</violation>
</file>
<file name=".github/workflows/ci.yml">
<violation number="1" location=".github/workflows/ci.yml:41">
P3: The corpus CI step uses `--ignored`, which runs every ignored test in this file — including `measures_ocr_positive_page_at_configured_dpi`, whose docstring says it is meant to be run separately (by an external process monitor to capture peak memory). As a result, every PR CI silently re-renders the page at the default 200 DPI and evaluates a detector classification assertion that has nothing to do with the corpus compatibility loop. Consider running only the pinned-rendering test explicitly (e.g. `-- --ignored renders_pinned_py_pdf_sample_files --exact`) so the on-demand measurement test isn't a hidden dependency of the render CI step.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Fix all with cubic | Re-trigger cubic
| let mut pdf = b"%PDF-1.4\n".to_vec(); | ||
| let mut offsets = vec![0_usize]; | ||
|
|
||
| fn add_object(pdf: &mut Vec<u8>, offsets: &mut Vec<usize>, id: usize, body: &[u8]) { |
There was a problem hiding this comment.
P3: The new fixture's add_object closure and xref/trailer generation duplicate the identical logic already in make_solid_page_pdf_with_page_options (tests/render_tests.rs). Both are new in this PR; extract the shared add_object + xref/trailer writer into a common helper the two builders reuse, so offset/link/trailer fixes don't have to be kept in sync in two places.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/support/render_fixture.rs, line 29:
<comment>The new fixture's add_object closure and xref/trailer generation duplicate the identical logic already in make_solid_page_pdf_with_page_options (tests/render_tests.rs). Both are new in this PR; extract the shared add_object + xref/trailer writer into a common helper the two builders reuse, so offset/link/trailer fixes don't have to be kept in sync in two places.</comment>
<file context>
@@ -0,0 +1,87 @@
+ let mut pdf = b"%PDF-1.4\n".to_vec();
+ let mut offsets = vec![0_usize];
+
+ fn add_object(pdf: &mut Vec<u8>, offsets: &mut Vec<usize>, id: usize, body: &[u8]) {
+ assert_eq!(id, offsets.len());
+ offsets.push(pdf.len());
</file context>
| - name: Run PDF renderer corpus | ||
| env: | ||
| PDF_INSPECTOR_SAMPLE_FILES: ${{ runner.temp }}/pdf-inspector-sample-files | ||
| run: cargo test --features render --test render_corpus_tests -- --ignored |
There was a problem hiding this comment.
P3: The corpus CI step uses --ignored, which runs every ignored test in this file — including measures_ocr_positive_page_at_configured_dpi, whose docstring says it is meant to be run separately (by an external process monitor to capture peak memory). As a result, every PR CI silently re-renders the page at the default 200 DPI and evaluates a detector classification assertion that has nothing to do with the corpus compatibility loop. Consider running only the pinned-rendering test explicitly (e.g. -- --ignored renders_pinned_py_pdf_sample_files --exact) so the on-demand measurement test isn't a hidden dependency of the render CI step.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/ci.yml, line 41:
<comment>The corpus CI step uses `--ignored`, which runs every ignored test in this file — including `measures_ocr_positive_page_at_configured_dpi`, whose docstring says it is meant to be run separately (by an external process monitor to capture peak memory). As a result, every PR CI silently re-renders the page at the default 200 DPI and evaluates a detector classification assertion that has nothing to do with the corpus compatibility loop. Consider running only the pinned-rendering test explicitly (e.g. `-- --ignored renders_pinned_py_pdf_sample_files --exact`) so the on-demand measurement test isn't a hidden dependency of the render CI step.</comment>
<file context>
@@ -27,6 +27,22 @@ jobs:
+ - name: Run PDF renderer corpus
+ env:
+ PDF_INSPECTOR_SAMPLE_FILES: ${{ runner.temp }}/pdf-inspector-sample-files
+ run: cargo test --features render --test render_corpus_tests -- --ignored
+
+ - name: Check rendering API documentation
</file context>
| run: cargo test --features render --test render_corpus_tests -- --ignored | |
| run: cargo test --features render --test render_corpus_tests -- --ignored renders_pinned_py_pdf_sample_files --exact --nocapture |
PR Review — #280: feat(render): add optional selected-page rasterizationRepository: firecrawl/pdf-inspector 📋 SummaryWhat changed: feat(render): add optional selected-page rasterization. Touches 16 files (.github/, root, docs/, src/, …) with +1808/-5 lines. Why it exists: Impact areas: Complexity score: High — 16 files, 1813 changed lines, 6 impact areas. Files changed summary:
🔒 Security AnalysisRisk level: 🟡 Medium Summary: 0 critical · 0 high · 1 medium · 3 low Findings:
Categories checked: secrets ✅ · injection ✅ · auth ✅ · dependencies External scanners: gitleaks, Semgrep OSS, Trivy ran — no findings beyond the rule engine. Suppress a false positive by adding 📚 DocumentationMissing docstrings/JSDoc:
README impact: README was updated in this PR ✅ (covers: new/changed environment variable). Changelog entry: ## [Unreleased]
### Added
- feat(render): add optional selected-page rasterization.Auto-generated docstrings: ✅ Review Decision
Overall recommendation: NEEDS DISCUSSION Generated by PR-Review-Agent on 2026-08-07 01:38:16 UTC |
There was a problem hiding this comment.
0 issues found across 4 files (changes from recent commits).
Shadow auto-approve: would not auto-approve. Auto-approval blocked by 2 unresolved issues from previous reviews.
Re-trigger cubic
|
@princeamir56 is everything fine now? |
2 participants
Hi! This is my first contribution here, and honestly one of my first real pieces of Rust; I come from other ecosystems, so if something below is nonsense or I misused a pattern you consider obvious, please be patient and point it out, I genuinely want to learn from the review.
Why
pages_needing_ocrand the per pageneeds_ocrflag tell callers which pages need OCR, but nothing in the package hands them the pixels of those pages. Any library that depends on this package for its PDF handling and needs to act on that flag is stuck: it knows page 3 needs OCR, it has no way to rasterize page 3, and working around it means dragging in a second PDF stack and parsing the same bytes twice. Since PDF ownership lives here by design (the issue templates even route PDF matters to this repo), IMO the rendering primitive belongs here too.What
A pluggable
renderfeature with a single entry point:Page indexes are zero based, results come back in caller order with duplicates preserved, as opaque row-major RGBA8 on a white background at 200 DPI by default (300 max).
Rendering goes through Hayro 0.7.1 (a pure Rust PDF interpreter, which seems very well done made) so wasm32 keeps working and the dependency graph stays free of native PDF runtimes.
Every limit is validated before the first page renders:
When Hayro hits something it can't draw (a font kind it doesn't support, an image that fails to decode) you get a typed
RenderWarningon that page instead of silently wrong pixels; I added a test with an intentionally broken JPEG to be sure this doesn't regress. Render errors have their own non-exhaustive enum, separate from the extraction ones, and no Hayro type is exposed in the public API.With default features nothing changes: hayro and bytemuck sit behind
render = ["dep:bytemuck", "dep:hayro"].Numbers
Letter page, release profile, Rust 1.95, Apple M3 Pro.
With a median of 20 runs in separate processes, first warmup run discarded:
Cost on the stripped release binary: ~704 bytes when the feature is compiled but never called, ~4.75 MB once a caller actually renders; six Letter pages at 200 DPI fit one call before the output cap.
The input was
018-base64-image/base64image.pdffrom the pinned corpus (the only page is classified as needing OCR, so it felt like the right sample).To reproduce (from the package root):
git clone https://github.com/py-pdf/sample-files.git ../sample-files git -C ../sample-files checkout 89039b6078fd0c9f98bf3d6fcb5583fac6b0ecaf PDF_INSPECTOR_SAMPLE_FILES=../sample-files cargo test --release --features render --test render_corpus_tests renders_pinned -- --ignored --nocaptureFor the RSS column I wrapped the single runs with
/usr/bin/time -l. The full protocol (and a more detailed table with p95, min–max and standard deviation) is indocs/benchmarking.md, which this PR adds to.Tests
For the tests I generate a small PDF in memory (a page with a single image XObject) and assert on the rendered content, not just on buffer sizes. There are 14 new renderer tests and they go through page selection, caller order and duplicates, crop and rotation, the DPI default, all the public limits, out of range indexes, empty selections and passwords. The 820 unit and 152 integration tests that were already there are untouched.
The same generated PDF is rendered in the wasm tests too, in release mode, both on Node and on headless Chrome.
I also wrote a suite that renders six files from py-pdf/sample-files, pinned at commit 89039b6 and verified by SHA-256. It is marked as ignored and runs only when
PDF_INSPECTOR_SAMPLE_FILESpoinst at that checkout.The CI changes are in a separate commit. I added jobs for the render feature, one pinned at Rust 1.92.0 (the minimum the feature compiles with, I check it on native and wasm32) and one for the corpus.
Known limitations
Hayro is a young project and some advanced graphic effects of the PDF spec are not implemented yet (transparency things like blend isolation, knockout groups and color-key masking), so a few PDFs will not look pixel-perfect compared to Acrobat or Preview. While testing the
py-pdfsamples I also noticed that annotation widgets are not drawn...My limits protect the output buffers but not what happens inside Hayro of course... so a malicious PDF can declare a huge embedded image and make the decoder allocate a lot of memory before my caps even enter the picture, meaning that if you render PDFs you must not just trust them: it's better to do it in a separate process or in a Worker (I know it seems obvious, but could be very dangerous if not well-checked).
The wasm package compiles fine with the feature but I did not expose a JS API for the renderer yet.
I didn't run the pdf-evals suite, so the coverage here is only what is described in Tests.
Important notes
ci.ymlusing the existing jobs as a template, but please make a profound check of the workflow changes.LICENSEfiles untouched. Hayro embedsPDFium/Foxitfallback fonts and Adobe CMap resources, so before pushing any artifact with render enabled the third party notices would probably need to be added; LICENSE probably should be updated as part of the review.Vec<u8>is the part of the Rust code I'm least sure I got right. Please take take a careful look there.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Adds optional selected-page rasterization via the
renderfeature, returning opaque RGBA8 pixels for pages flagged for OCR. This removes the need for a second PDF stack and works on native and WebAssembly (no JS API exposed yet).New Features
render_pages_mem(&[u8], &[u32], RenderOptions) -> Result<Vec<RenderedPage>, RenderError>renders zero-based pages; caller order and duplicates preserved.UnsupportedFont,ImageDecodeFailure(skip OCR on these).wasm32-unknown-unknown. Includes unit tests, wasm runtime tests (Node + headless Chrome), and an ignored checksum-verified corpus.renderfeature; default builds unchanged. Skip the call entirely when no pages need OCR and batch large renders to control memory.Dependencies
hayro@0.7.1andbytemuck@1.25behindrender; default feature set remainslopdf-only.render, docs build, MSRV check (1.92), the pinned corpus job, and Node + Chrome runtime tests using awasm-bindgen-cliversion pinned towasm/Cargo.lock.Written for commit 433629e. Summary will update on new commits.