Skip to content

perf(images): encode embedded PNGs with adaptive filtering, not NoFilter - #908

Draft
ajbufort wants to merge 2 commits into
yfedoseev:mainfrom
ajbufort:fix/png-adaptive-filter
Draft

perf(images): encode embedded PNGs with adaptive filtering, not NoFilter#908
ajbufort wants to merge 2 commits into
yfedoseev:mainfrom
ajbufort:fix/png-adaptive-filter

Conversation

@ajbufort

Copy link
Copy Markdown
Contributor

Problem

PdfImage::to_png_bytes() encodes every non-JPEG image (Flate / CCITT / JBIG2 / JPX) with:

PngEncoder::new_with_quality(&mut buffer, CompressionType::Fast, FilterType::NoFilter)

FilterType::NoFilter disables PNG's per-scanline Sub/Up/Average/Paeth filtering — the exact step that lets deflate compress smooth / scanned content — so the stream stays near-raw. A full-page scanned background that the source PDF stores in ~110 KB (FlateDecode) re-encodes at ~4.4 MB (~40×). Any consumer that inlines these images (e.g. base64 data URIs in an HTML export) inherits the blow-up; on a sample of image-heavy documents this alone accounted for hundreds of MB.

Fix

Switch to CompressionType::Default + FilterType::Adaptive. This is purely a container-size choice — the decoded pixels are byte-identical either way (PNG is lossless) — trading a little encode CPU for large size reductions.

Measured

Over a sample of image-heavy documents, total inlined output 2200 MB → 928 MB (0.42×); individual flat-scan docs up to 83× smaller; pixel-fidelity and text metrics byte-identical (max delta 0.0000). The re-encode cost is per-image-decode, not per-render.

Full gate on this branch off main: cargo test --lib (5750 passed), cargo test --doc (141 passed), cargo clippy --lib clean, cargo fmt --check clean.

to_png_bytes() encoded every non-JPEG image (Flate/CCITT/JBIG2/JPX) with
CompressionType::Fast + FilterType::NoFilter. NoFilter disables PNG's
per-scanline Sub/Up/Average/Paeth filtering - exactly the step that lets
deflate compress smooth/scanned content - so the PNG stream stayed near-raw.
A full-page scanned background that the source PDF stores in ~110 KB was
re-emitted at ~4.4 MB (~40x), and any consumer inlining these images (e.g.
base64 data URIs) inherited the blow-up.

Switch to CompressionType::Default + FilterType::Adaptive. This is purely a
container-size choice - the decoded pixels are byte-identical (PNG is
lossless) - trading a little encode CPU for large size wins.

Measured over a sample of image-heavy documents: total inlined-HTML output
2200 MB -> 928 MB (0.42x); individual flat-scan docs up to 83x smaller;
pixel-fidelity and text metrics byte-identical (max delta 0.0000).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Anthony J. (Tony) Bufort <ajbufort@ajbconsulting.us>
@ajbufort
ajbufort requested a review from yfedoseev as a code owner July 19, 2026 10:44
@ajbufort
ajbufort marked this pull request as draft July 19, 2026 18:10
@yfedoseev

Copy link
Copy Markdown
Owner

Thanks @ajbufort — this is a clean, well-reasoned change, and the write-up is exactly the right level of detail. I verified it end-to-end and it holds up strongly.

First, the red CI is a false alarm — it's base staleness, not your code. I merged your branch onto current main locally: no conflicts, cargo test --lib builds clean, and the 65 image tests (including the to_png_bytes panic-safety/correctness cases) all pass. A rebase on main will turn the checks green.

Verification of the claim (built extract_html_simple on main vs your branch, ran on scanned/image-heavy PDFs):

PDF main HTML branch HTML ratio
freeculture (JPEG images) 1,538,978 1,538,978 1.00×
Kahle scan A 11,737,972 675,572 17.4×
Kahle scan B 23,161,230 1,342,946 17.3×
Kahle scan C 12,592,346 84,758 148.6×

And I confirmed the lossless half directly — extracted the embedded PNG from both outputs on one doc (8,799,699 → 502,897 bytes), decoded both, and the pixels are byte-identical: ImageMagick compare -metric AE = 0, and identical decoded-RGBA md5. The JPEG-image doc being unchanged is the correct signature — your change only touches the non-JPEG to_png_bytes path, exactly as scoped. Nice.

So the substance is a clear win. Before it can merge, the standard process items — same bar every recent PR went through:

  1. Link an accepted issueCloses #NNN in the description. Open a short tracking issue for the PNG-encoding blow-up (rule 1); happy to see it filed.
  2. Ship a regression test that goes red if the change is reverted — e.g. encode a synthetic smooth/scanned raster and assert the Adaptive-filtered output is materially smaller than the NoFilter baseline (and, ideally, assert decoded-pixel identity). Right now the diff is source-only.
  3. Corpus note (rule 2) — you can fold in the numbers above; a one-line summary in the PR that the change is byte-identical on pixels and only shrinks the container is enough.
  4. Sign the CLA — comment on this PR with exactly this line and nothing else (no indentation, no surrounding dashes):
    I have read the CLA Document and I hereby sign the CLA
  5. Disclose AI assistance — which tool and how much, or "none".

(DCO sign-off is already there — thanks for that.) Rebase + those items and I'll review promptly; the technical case is already made.

@yfedoseev yfedoseev left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Approving on the technical merits — I verified it end-to-end (see my detailed comment above): compiles clean on current main, all 65 image tests pass, and every embedded PNG across the scanned test docs is byte-identical after decode (zero information loss) while the container shrinks 17–150×. Solid, well-scoped change.

Before this can actually merge, three items still need to land per CONTRIBUTING.md — the same bar every recent PR followed:

  1. Sign the CLA (currently the hard merge blocker) — comment exactly this line, nothing else:
    I have read the CLA Document and I hereby sign the CLA
  2. Add a fails-on-revert regression test (assert Adaptive output is materially smaller than NoFilter + decoded-pixel identity).
  3. Link an accepted issue (Closes #NNN) and disclose AI assistance (tool + extent, or "none").

Rebase on main to clear the stale-base CI. The review's done — these are the remaining gates.

@github-actions

Copy link
Copy Markdown
Contributor


Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.


I have read the CLA Document and I hereby sign the CLA


You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot.

@github-actions github-actions Bot added the ai-assisted PR discloses AI co-authorship (informational) label Jul 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-assisted PR discloses AI co-authorship (informational)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants