Skip to content

♻️ Refactor PDF processing code to align with PDFium thread-safety and rendering best practices#1209

Open
balazs-szucs wants to merge 15 commits into
stumpapp:nightlyfrom
balazs-szucs:pdf-rework
Open

♻️ Refactor PDF processing code to align with PDFium thread-safety and rendering best practices#1209
balazs-szucs wants to merge 15 commits into
stumpapp:nightlyfrom
balazs-szucs:pdf-rework

Conversation

@balazs-szucs

@balazs-szucs balazs-szucs commented Jun 2, 2026

Copy link
Copy Markdown
Contributor
  • Resolves concurrent rendering crashes by serializing PDFium FFI calls with a global mutex lock.
  • Moves blocking page analysis tasks to a dedicated thread pool to prevent problems due to newly added Locks.
  • Improves low-resolution rendering by using configured DPI settings and preserving aspect ratios.
  • Refactors PDF stuff to use PDFium-render, and removes now (unused) pdf.rs library.

On the last point, generally, PDFIum should be much more reliable/performant than from what I can moreso a hobbyist Rust PDF library. So, using PDF.rs then to fallback PDFium, that seemed like bit of strange choice, and when I flipped them, i thought, there is very unlikely that PDFium would fail and PDF.rs would catch it, so at that point better to just remove. PDF.rs was also pulling in seemingly quite large of other deps. So, I think that's a win.

Date parsing still hacky. But it is mostly in working condition.

I am still not happy with date stuff, but otherwise this should be improvements. It's able parse in the bit more metadata (e.g., Summary/Description that was previously ignored), i have not been able to trigger segfault, but yeah bit slower overall (not a huge surprise.)

@balazs-szucs balazs-szucs changed the title ♻️ Refactor PDF processing stack to align with PDFium thread-safety and rendering best practices ♻️ Refactor PDF processing code to align with PDFium thread-safety and rendering best practices Jun 2, 2026
@balazs-szucs balazs-szucs marked this pull request as ready for review June 2, 2026 17:03

@aaronleopold aaronleopold left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for starting this! I had a few comments to sort out but nothing major

Comment thread core/src/filesystem/media/format/pdf.rs
Comment thread core/src/filesystem/media/format/pdf.rs Outdated
Comment thread core/src/filesystem/media/format/pdf.rs Outdated
Comment thread core/Cargo.toml Outdated
Comment thread core/src/filesystem/media/format/pdf.rs Outdated
Comment thread core/src/filesystem/media/format/pdf.rs Outdated
@aaronleopold

Copy link
Copy Markdown
Collaborator

I haven't had much Stump time this week unfortunately, but I've pushed a few changes based on our discussions and will aim to give a final review by the weekend to get this into nightly. Thanks again for getting the ball rolling here, and sorry for how delayed getting it in has been!

@codecov

codecov Bot commented Jul 9, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 40.92742% with 293 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
core/src/filesystem/media/format/pdf.rs 22.03% 283 Missing ⚠️
core/src/filesystem/media/analysis/analyze.rs 0.00% 5 Missing ⚠️
core/src/filesystem/media/utils.rs 96.00% 4 Missing ⚠️
core/src/filesystem/media/builder.rs 75.00% 1 Missing ⚠️
Files with missing lines Coverage Δ
core/src/filesystem/error.rs 0.00% <ø> (ø)
core/src/filesystem/media/metadata.rs 89.94% <100.00%> (+6.46%) ⬆️
core/src/filesystem/media/builder.rs 81.62% <75.00%> (-2.36%) ⬇️
core/src/filesystem/media/utils.rs 97.71% <96.00%> (-1.45%) ⬇️
core/src/filesystem/media/analysis/analyze.rs 0.00% <0.00%> (ø)
core/src/filesystem/media/format/pdf.rs 19.68% <22.03%> (+6.30%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants