From f1e85fff3e6b97880be4ce9102557e809cb8531c Mon Sep 17 00:00:00 2001 From: stumpylog <797416+stumpylog@users.noreply.github.com> Date: Wed, 7 Oct 2026 14:51:09 -0700 Subject: [PATCH] Chore: Simplify the PDF thumbnail helpers and their tests The qpdf repair step in the thumbnail fallback is now its own helper, so the fallback has a single ParseError handler instead of a nested try. The fallback also stops re-wrapping temp_dir in Path and annotating obvious locals. Parameters no caller used are removed: the cropbox toggle on the rasterizer, which now always passes -cropbox, and the width and height limits on the WebP encoder, which uses the module constants directly. The comments around the 300 DPI cap are consolidated so the purpose is stated once. In the tests, the clamp and supersample encoder cases are merged into one parametrized test, the PDF thumbnail tests share a work directory fixture and a blank PDF helper, redundant assertions are dropped, and the tesseract fallback test uses a plain import of the rasterizer. A garbled docstring is rewritten. --- src/documents/parsers.py | 85 ++++++--------- src/documents/tests/test_parsers.py | 103 +++++++----------- .../tests/parsers/test_tesseract_parser.py | 5 +- 3 files changed, 74 insertions(+), 119 deletions(-) diff --git a/src/documents/parsers.py b/src/documents/parsers.py index a669c20ac..2e26eb4fc 100644 --- a/src/documents/parsers.py +++ b/src/documents/parsers.py @@ -79,8 +79,8 @@ _THUMBNAIL_MAX_WIDTH = 500 _THUMBNAIL_MAX_HEIGHT = 5000 # Used only when the page geometry cannot be read _THUMBNAIL_FALLBACK_DPI = 150 -# Cap on the target density, applied before supersampling: a capped tiny page -# is rasterized at twice this and halved to the size a 300 DPI render gave +# Cap on the target density, applied before supersampling, so a tiny page is +# not blown up past the size a plain 300 DPI render would give _THUMBNAIL_MAX_DPI = 300 # Pages with known geometry are rendered at this multiple of the computed DPI # and downsampled with Lanczos, which keeps text noticeably crisper than @@ -93,7 +93,6 @@ def rasterize_pdf_page_to_png( out_path: Path, *, dpi: int, - use_cropbox: bool = True, logging_group=None, ) -> None: """ @@ -113,10 +112,10 @@ def rasterize_pdf_page_to_png( str(dpi), "-png", "-singlefile", + "-cropbox", + str(in_path), + str(out_path.with_suffix("")), ] - if use_cropbox: - args.append("-cropbox") - args += [str(in_path), str(out_path.with_suffix(""))] logger.debug("Execute: " + " ".join(args), extra={"group": logging_group}) @@ -132,18 +131,15 @@ def encode_thumbnail_webp( png_path: Path, out_path: Path, *, - max_width: int = _THUMBNAIL_MAX_WIDTH, - max_height: int = _THUMBNAIL_MAX_HEIGHT, supersample: int = 1, ) -> None: """ Flattens any alpha onto white and saves the image as WebP. A render made at supersample times the target density is first - downsampled by that factor with Lanczos. max_width/max_height then trim - the result to the exact thumbnail size, since the computed DPI is rounded - up and lands at or slightly above it. The image is never enlarged, - matching the previous "-scale WxH>" behavior. + downsampled by that factor with Lanczos. The result is then trimmed to the + thumbnail size, since the computed DPI is rounded up and lands at or + slightly above it. The image is never enlarged. """ from PIL import Image @@ -164,7 +160,7 @@ def encode_thumbnail_webp( Image.Resampling.LANCZOS, ) - flattened.thumbnail((max_width, max_height)) + flattened.thumbnail((_THUMBNAIL_MAX_WIDTH, _THUMBNAIL_MAX_HEIGHT)) flattened.save(out_path, format="WEBP") except (OSError, Image.DecompressionBombError) as e: raise ParseError(f"Unable to encode thumbnail from {png_path}") from e @@ -172,15 +168,8 @@ def encode_thumbnail_webp( def _compute_thumbnail_dpi(in_path: Path, logging_group=None) -> tuple[int, int]: """ - Computes the DPI at which the first page of the PDF reaches at or just - above the thumbnail size, never above the 300 DPI the thumbnail was - previously rendered at before being scaled down, and the supersampling - factor to render with. - - Returns (dpi, supersample). The page is rendered at supersample * dpi and - downsampled by supersample afterwards. When the page geometry cannot be - read the fixed fallback DPI is used without supersampling, as that render - is not bounded by the thumbnail size. + Returns (dpi, supersample): render at dpi * supersample, then downsample + by supersample. Unknown page geometry gives the fallback DPI, unsupersampled. """ from paperless.parsers.utils import get_pdf_first_page_size_points @@ -195,17 +184,13 @@ def _compute_thumbnail_dpi(in_path: Path, logging_group=None) -> tuple[int, int] width_pts, height_pts = size dpi_for_width = _THUMBNAIL_MAX_WIDTH * 72 / width_pts dpi_for_height = _THUMBNAIL_MAX_HEIGHT * 72 / height_pts - # The old pipeline rendered at 300 DPI and then shrank to fit, so only - # pages too small to reach the thumbnail size even at 300 DPI end up - # smaller than it. Rounding up keeps the downsampled render at or above - # the target, so the shrink-only clamp in encode_thumbnail_webp trims it - # to exactly the thumbnail size instead of leaving it a few pixels short. + # Round up so the downsampled render is never a few pixels short of the + # target; the shrink-only clamp in encode_thumbnail_webp trims the excess. dpi = max( 1, math.ceil(min(_THUMBNAIL_MAX_DPI, dpi_for_width, dpi_for_height)), ) - # A page so large it hits the 1 DPI floor is already oversized, so - # doubling it would only quadruple the pixel count for no benefit + # At the 1 DPI floor the page is already oversized, so do not supersample return dpi, 1 if dpi == 1 else _THUMBNAIL_SUPERSAMPLE @@ -220,20 +205,33 @@ def _render_pdf_thumbnail( in_path, png_path, dpi=dpi * supersample, - use_cropbox=True, logging_group=logging_group, ) encode_thumbnail_webp(png_path, out_path, supersample=supersample) +def _repair_pdf_with_qpdf(in_path: Path, out_path: Path) -> None: + # qpdf rewrites in place, so work on a copy and leave the original alone. + # qpdf exits 3 when it had to repair the file, which is the expected + # outcome here, so warnings must not count as failure. + try: + shutil.copy(in_path, out_path) + run_subprocess( + ["qpdf", "--warning-exit-0", "--replace-input", str(out_path)], + logger=logger, + ) + except (subprocess.CalledProcessError, OSError) as e: + raise ParseError(f"qpdf repair failed for {in_path}") from e + + def make_thumbnail_from_pdf_qpdf_fallback( in_path: Path, temp_dir: Path, logging_group=None, ) -> Path: - png_path: Path = Path(temp_dir) / "page1_repaired.png" - out_path: Path = Path(temp_dir) / "convert_qpdf.webp" - repaired_path: Path = Path(temp_dir) / "repaired.pdf" + png_path = temp_dir / "page1_repaired.png" + out_path = temp_dir / "convert_qpdf.webp" + repaired_path = temp_dir / "repaired.pdf" logger.warning( "Thumbnail generation with pdftoppm failed, attempting qpdf repair and retry.", @@ -241,25 +239,8 @@ def make_thumbnail_from_pdf_qpdf_fallback( ) try: - # qpdf rewrites in place, so work on a copy and leave the original alone. - # qpdf exits 3 when it had to repair the file, which is the expected - # outcome here, so warnings must not count as failure. - try: - shutil.copy(in_path, repaired_path) - run_subprocess( - [ - "qpdf", - "--warning-exit-0", - "--replace-input", - str(repaired_path), - ], - logger=logger, - ) - except (subprocess.CalledProcessError, OSError) as e: - raise ParseError(f"qpdf repair failed for {in_path}") from e - + _repair_pdf_with_qpdf(in_path, repaired_path) _render_pdf_thumbnail(repaired_path, png_path, out_path, logging_group) - return out_path except ParseError as e: @@ -267,7 +248,7 @@ def make_thumbnail_from_pdf_qpdf_fallback( # The caller might expect a generated thumbnail that can be moved, # so we need to copy it before it gets moved. # https://github.com/paperless-ngx/paperless-ngx/issues/3631 - default_thumbnail_path: Path = Path(temp_dir) / "document.webp" + default_thumbnail_path = temp_dir / "document.webp" copy_file_with_basic_stats(get_default_thumbnail(), default_thumbnail_path) return default_thumbnail_path diff --git a/src/documents/tests/test_parsers.py b/src/documents/tests/test_parsers.py index 23155b60e..835aea2a5 100644 --- a/src/documents/tests/test_parsers.py +++ b/src/documents/tests/test_parsers.py @@ -169,10 +169,11 @@ class TestComputeThumbnailDpi: WHEN: - The thumbnail DPI is computed THEN: - - The DPI is rounded up so the render reaches 500x5000, never - exceeds 300, is at least 1, and is supersampled 2x (except at the - 1 DPI floor); an unknown - size gives the plain 150 DPI fallback without supersampling + - The DPI is rounded up so the render reaches 500x5000, is capped + at 300 and floored at 1, and is supersampled 2x except at the + 1 DPI floor + - An unknown size gives the plain 150 DPI fallback without + supersampling """ mocker.patch( "paperless.parsers.utils.get_pdf_first_page_size_points", @@ -182,6 +183,19 @@ class TestComputeThumbnailDpi: class TestMakeThumbnailFromPdf: + @pytest.fixture + def work_dir(self, tmp_path: Path) -> Path: + path = tmp_path / "work" + path.mkdir() + return path + + @staticmethod + def _write_blank_pdf(path: Path, page_size: tuple[int, int] = (612, 792)) -> Path: + pdf = pikepdf.new() + pdf.add_blank_page(page_size=page_size) + pdf.save(path, object_stream_mode=pikepdf.ObjectStreamMode.disable) + return path + @pytest.mark.parametrize( ("size", "expected_dpi", "expected_supersample"), [ @@ -193,6 +207,7 @@ class TestMakeThumbnailFromPdf: self, mocker: MockerFixture, tmp_path: Path, + work_dir: Path, size: tuple[float, float] | None, expected_dpi: int, expected_supersample: int, @@ -213,8 +228,6 @@ class TestMakeThumbnailFromPdf: ) rasterize = mocker.patch("documents.parsers.rasterize_pdf_page_to_png") encode = mocker.patch("documents.parsers.encode_thumbnail_webp") - work_dir = tmp_path / "work" - work_dir.mkdir() make_thumbnail_from_pdf(tmp_path / "in.pdf", work_dir) @@ -234,6 +247,7 @@ class TestMakeThumbnailFromPdf: def test_thumbnail_width( self, tmp_path: Path, + work_dir: Path, page_size: tuple[int, int], expected_width: int, ) -> None: @@ -246,12 +260,7 @@ class TestMakeThumbnailFromPdf: - The WebP thumbnail is exactly 500px wide, unless the page is too small to reach that even at 300 DPI """ - pdf = pikepdf.new() - pdf.add_blank_page(page_size=page_size) - pdf_path = tmp_path / "in.pdf" - pdf.save(pdf_path) - work_dir = tmp_path / "work" - work_dir.mkdir() + pdf_path = self._write_blank_pdf(tmp_path / "in.pdf", page_size) thumb = make_thumbnail_from_pdf(pdf_path, work_dir) @@ -260,20 +269,22 @@ class TestMakeThumbnailFromPdf: assert im.format == "WEBP" assert im.width == expected_width - @staticmethod - def _write_pdf_without_xref(path: Path) -> Path: + @classmethod + def _write_pdf_without_xref(cls, path: Path) -> Path: """ Writes a valid one page PDF, then cuts off its cross reference table and trailer, which poppler cannot recover from but qpdf can. """ - pdf = pikepdf.new() - pdf.add_blank_page(page_size=(612, 792)) - pdf.save(path, object_stream_mode=pikepdf.ObjectStreamMode.disable) + cls._write_blank_pdf(path) data = path.read_bytes() path.write_bytes(data[: data.rindex(b"\nxref")]) return path - def test_qpdf_repair_produces_real_thumbnail(self, tmp_path: Path) -> None: + def test_qpdf_repair_produces_real_thumbnail( + self, + tmp_path: Path, + work_dir: Path, + ) -> None: """ GIVEN: - A PDF without a cross reference table or trailer, which @@ -288,8 +299,6 @@ class TestMakeThumbnailFromPdf: """ pdf_path = self._write_pdf_without_xref(tmp_path / "broken.pdf") original_bytes = pdf_path.read_bytes() - work_dir = tmp_path / "work" - work_dir.mkdir() with pytest.raises(ParseError): rasterize_pdf_page_to_png(pdf_path, work_dir / "probe.png", dpi=50) @@ -297,7 +306,6 @@ class TestMakeThumbnailFromPdf: thumb = make_thumbnail_from_pdf(pdf_path, work_dir) assert thumb == work_dir / "convert_qpdf.webp" - assert thumb.read_bytes() != get_default_thumbnail().read_bytes() with Image.open(thumb) as im: assert im.format == "WEBP" assert im.width == 500 @@ -314,6 +322,7 @@ class TestMakeThumbnailFromPdf: self, mocker: MockerFixture, tmp_path: Path, + work_dir: Path, qpdf_error: subprocess.CalledProcessError | None, ) -> None: """ @@ -332,17 +341,11 @@ class TestMakeThumbnailFromPdf: ) if qpdf_error is not None: mocker.patch("documents.parsers.run_subprocess", side_effect=qpdf_error) - pdf = pikepdf.new() - pdf.add_blank_page(page_size=(612, 792)) - pdf_path = tmp_path / "in.pdf" - pdf.save(pdf_path) - work_dir = tmp_path / "work" - work_dir.mkdir() + pdf_path = self._write_blank_pdf(tmp_path / "in.pdf") thumb = make_thumbnail_from_pdf(pdf_path, work_dir) assert thumb == work_dir / "document.webp" - assert thumb != get_default_thumbnail() assert thumb.read_bytes() == get_default_thumbnail().read_bytes() @@ -457,49 +460,19 @@ class TestEncodeThumbnailWebp: red, green, blue = im.getpixel((10, 5)) assert min(red, green, blue) >= 250 - @pytest.mark.parametrize( - ("in_size", "expected_size"), - [ - pytest.param((1000, 2000), (500, 1000), id="too-wide-shrunk"), - pytest.param((100, 10000), (50, 5000), id="too-tall-shrunk"), - pytest.param((100, 200), (100, 200), id="small-not-enlarged"), - ], - ) - def test_size_clamped_without_enlarging( - self, - tmp_path: Path, - in_size: tuple[int, int], - expected_size: tuple[int, int], - ) -> None: - """ - GIVEN: - - A rendered page image of the given size - WHEN: - - It is encoded as a thumbnail - THEN: - - It is shrunk to fit 500x5000 keeping aspect ratio, and never - enlarged - """ - png_path = tmp_path / "in.png" - Image.new("RGB", in_size, (255, 255, 255)).save(png_path) - out_path = tmp_path / "out.webp" - - encode_thumbnail_webp(png_path, out_path) - - with Image.open(out_path) as im: - assert im.size == expected_size - @pytest.mark.parametrize( ("in_size", "supersample", "expected_size"), [ + pytest.param((1000, 2000), 1, (500, 1000), id="too-wide-shrunk"), + pytest.param((100, 10000), 1, (50, 5000), id="too-tall-shrunk"), + pytest.param((100, 200), 1, (100, 200), id="small-not-enlarged"), pytest.param((1000, 1400), 2, (500, 700), id="2x-halved"), pytest.param((1001, 1401), 2, (500, 700), id="2x-odd-rounded"), - pytest.param((1000, 1400), 1, (500, 700), id="no-supersample-clamped"), pytest.param((600, 800), 2, (300, 400), id="2x-small-not-enlarged"), pytest.param((900, 1200), 2, (450, 600), id="2x-below-clamp"), ], ) - def test_supersampled_render_downsampled( + def test_size_clamped_and_downsampled( self, tmp_path: Path, in_size: tuple[int, int], @@ -508,11 +481,13 @@ class TestEncodeThumbnailWebp: ) -> None: """ GIVEN: - - A rendered page image made at a supersampling factor + - A rendered page image of the given size, made at the given + supersampling factor WHEN: - It is encoded as a thumbnail with that factor THEN: - - It is downsampled by the factor before the 500x5000 clamp + - It is downsampled by the factor, then shrunk to fit 500x5000 + keeping aspect ratio, and never enlarged """ png_path = tmp_path / "in.png" Image.new("RGB", in_size, (255, 255, 255)).save(png_path) diff --git a/src/paperless/tests/parsers/test_tesseract_parser.py b/src/paperless/tests/parsers/test_tesseract_parser.py index 2effa0a8a..c47c5a2f9 100644 --- a/src/paperless/tests/parsers/test_tesseract_parser.py +++ b/src/paperless/tests/parsers/test_tesseract_parser.py @@ -17,8 +17,8 @@ import pytest from ocrmypdf import SubprocessOutputError from PIL import Image -import documents.parsers from documents.parsers import ParseError +from documents.parsers import rasterize_pdf_page_to_png from paperless.models import ModeChoices from paperless.parsers import ParserProtocol from paperless.parsers.tesseract import RasterisedDocumentParser @@ -328,13 +328,12 @@ class TestGetThumbnail: - The PDF is repaired with qpdf and rasterized again, producing a real thumbnail rather than the default placeholder """ - real_rasterize = documents.parsers.rasterize_pdf_page_to_png original = tesseract_samples_dir / "simple-digital.pdf" def _fail_on_original(in_path: Path, out_path: Path, **kwargs) -> None: if in_path == original: raise ParseError("Does not compute.") - real_rasterize(in_path, out_path, **kwargs) + rasterize_pdf_page_to_png(in_path, out_path, **kwargs) rasterize = mocker.patch( "documents.parsers.rasterize_pdf_page_to_png",