-
Notifications
You must be signed in to change notification settings - Fork 35
refactor: move image re-encoding from story providers to render time - #145
Open
Smengerl wants to merge 16 commits into
Open
refactor: move image re-encoding from story providers to render time #145Smengerl wants to merge 16 commits into
Smengerl wants to merge 16 commits into
Conversation
Smengerl
added a commit
to Smengerl/goosepaper-logicpuzzles
that referenced
this pull request
Aug 15, 2026
...#145 Cherry-picks the polish round that only happened on the clean PR branch (feature/render-time-image-sizing), not on this branch's own history: removes a reintroduced "verified live" anecdotal-claim docstring/comment pattern, corrects docs/reference/storyprovider/ comic.py.md (still described the old download-and-embed behavior), and adds an end-to-end test running the real DailyComicStoryProvider through Goosepaper.to_html(). 255/255 tests pass.
Smengerl
commented
Aug 17, 2026
Contributor
Author
Added a small follow-up fix (f6a68e5): an image that fails to inline is now removed from the story instead of being left as a dead http(s):// link. Leaving it in place meant WeasyPrint's own image loader tried (and failed) to fetch the exact same URL again at render time - same failure, logged twice, plus a wasted request.
Found and verified against a real case in production: cst.cam.ac.uk's own article markup has a duplicated <img> tag whose src contains another tag's raw HTML pasted into it (their bug, not ours) - always 404s. Before: two log lines for one failure. After: one, and the PDF still renders correctly (the page's other, correctly-formed <img> for the same photo still embeds fine). 145/145 tests pass.
RSS article images were embedded exactly as the source served them - a remote <img src> that WeasyPrint fetches and decodes itself while rendering the PDF, with zero control over what it gets. That reproduces a known WeasyPrint failure mode: certain source images (oversized, wrong color mode, unsupported format) make it silently drop the *entire* story, not just the image - no exception, no log line. Verified against a real daily edition (110 stories, 26 feeds): 28 of 53 embedded images failed, and for 12 of those the whole story vanished from the rendered PDF. Every failure traced back to the source image: multi-megapixel photos (up to 4000px, 500KB-1.4MB), a palette-mode PNG encoding photo content far less efficiently than JPEG, or WebP (a format WeasyPrint's image backend can't decode at all, regardless of size). Fetches each <img src="http(s)://..."> found in a story's body_html, decodes and re-encodes it through Pillow - capped dimensions, normalized color mode, always JPEG - and inlines the result as a data: URI instead. An image that fails to download or decode is left as its original remote link rather than aborting the story. 🤖 Generated with Claude Code
Pulls the Pillow re-encode step (bound dimensions, normalize color mode, composite transparency, always emit JPEG) out into storyprovider/imageutil.py, generic over any source of fetched image bytes - not RSS-specific. This is designed to land the same way in the comic-provider PR (j6k4m8#133), which needs the identical re-encode step for comic strip images and previously had its own separate, bespoke copy of this logic. Sharing one module lets both PRs merge in either order: each carries its own copy of imageutil.py (unavoidable, since neither branch can depend on the other not being merged first), which is at worst a trivial identical-content conflict for whichever merges second - same category of expected mechanical overlap already called out for this PR's config schema changes. Also adds direct unit tests for the re-encode step itself (test_imageutil.py), decoupled from HTTP mocking - the RSS-specific wiring tests (does _inline_remote_images fetch/skip/tolerate-failure correctly) stay in test_rss.py. 🤖 Generated with Claude Code
Downloads today's XKCD, Calvin and Hobbes, or Garfield strip and embeds it as a single image Story. The fetch mechanism - page URL, per-comic request headers, and the XPath used to locate the strip's <img> tag - is ported from evidlo/remarkable_news's systemd service definitions (services/ xkcd.service, cah.service, garfield.service), which use the same approach to push comics onto a reMarkable's suspend screen. The downloaded strip is inlined as a base64 data: URI rather than linked by remote URL: gocomics.com requires the same browser-like headers for the image request as for the page request, and WeasyPrint (which fetches <img src> URLs itself while rendering the PDF) has no way to attach them. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Embedding the raw fetched bytes directly as a base64 data: URI seemed fine in isolation, but broke silently on the real Garfield source: arcamax.com serves a CMYK JPEG with a large embedded Photoshop/ICC metadata block, and passing that straight to WeasyPrint made it drop the *entire* story with no exception and no log line - just an empty gap where "Comics" should have been, confirmed only by diffing rendered PDF text against the source HTML. Decode with Pillow and re-encode as a clean PNG before embedding, converting to RGB/L first when the source isn't already one of those modes (handles CMYK and any other decode-only mode a comic site might serve). This mirrors remarkable_news's own Go tool, which never embeds fetched bytes directly either - it decodes then re-encodes via imaging.Decode/imaging.Save. Pillow was already a transitive dependency via weasyprint; now declared directly since comic.py imports it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The Pillow re-encode from the previous commit fixed the CMYK/metadata case but not the general one: gocomics.com's CDN can serve a strip at 2800px+ wide with no smaller variant requested, and even at a source's default resolution, a lossless PNG re-encode of a dithered/gradient-heavy color strip is several times larger than the same content as JPEG. Either way the resulting base64 payload (multi-hundred-KB, sometimes >1MB for a single story) combined with the hundreds of other images already in a full newspaper was enough to make WeasyPrint silently drop the story entirely - confirmed by bisecting a real "Julians Zeitung" generation down to the image size specifically, after ruling out layout, section ordering, and provider wrapping as causes. Cap the long edge to _MAX_IMAGE_DIMENSION (1200px) and switch the re-encode target from PNG to JPEG (quality=90) - matching remarkable_news's own Go tool, which resizes to the target screen size and always saves via imaging.JPEGQuality, never PNG. Verified against all three live sources: body_html size dropped from >1MB to ~300KB (Calvin and Hobbes) and ~125KB (Garfield). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Previously: XKCD's headline was the strip's own per-day title (from its <img> alt text), while Garfield/Calvin and Hobbes fell back to "<name> - <date>" - and every comic additionally set byline=<name>. Two problems in practice: - XKCD showed headline "Main Span" with byline "XKCD" right underneath it - looks like a subheading, but for a single-panel strip there's no "subheading" to show, just the same source name the reader already knows from the section it's in. - Garfield/CaH's "<name> - <date>" headline duplicated the name that was *also* set as the byline right below it - e.g. headline "Garfield - August 02, 2026" directly above byline "Garfield" is the same identifier twice in a two-line block, worse when two comics share one "Comics" section and both do this right next to each other. Now every comic gets a single, fixed headline - "XKCD", "Garfield", or "Calvin and Hobbes" - and no byline. Unlike an RSS article, where the byline distinguishes otherwise-anonymous entries pulled from different feeds into one section, a comic's headline already *is* that identifier - a byline under it, or a per-day headline that just restates it, adds nothing. XKCD's own per-day title/mouseover joke aren't lost: the title still ends up in the embedded <img>'s alt attribute, and the joke still renders as a caption underneath, exactly as before - only the *headline* source changed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Pillow's Image.convert("RGB") does not composite transparent pixels against
anything - it just drops the alpha channel and keeps whatever RGB value (or,
for a GIF's transparency-color-key, whatever palette color) was stored
underneath. Verified directly against Pillow: a semi-transparent black RGBA
pixel converts to solid black, not white; a color-keyed "transparent" GIF
pixel converts to its own arbitrary palette color. Applied unconditionally,
this could leave visible phantom colors/edges wherever a source image used
transparency - not observed against any of the three real sources today (all
consistently serve opaque images), but a real gap for a hypothetical source
that does.
Composite onto white before dropping alpha whenever the source image has any
transparency (RGBA/LA/PA mode, or a GIF-style transparency color-key) - white
because a comic strip always sits on a plain newspaper page, and every
bundled goosepaper style renders that page white.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
pyproject.toml has declared pillow directly (see the earlier "comic" source type commit) but uv.lock's own per-package listing for goosepaper never picked it up as a direct dependency - only the transitive entry already present via weasyprint existed. Regenerated via `uv sync`. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Replaced the two site-specific, hardcoded comic_type entries ("cah" for
Calvin and Hobbes, "garfield") with two generic site-backed ones:
comic_type "gocomics"/"arcamax" plus a required comic_name (the comic's
own slug on that site, e.g. "garfield" or "beetlebailey"). Both sites
serve every comic in their catalog through one identical URL/markup
template - only the slug differs - so this was verified live against
several strips on each site rather than assumed.
The one wrinkle this raises - a fixed per-comic headline no longer works
once one _ComicSource entry covers hundreds of comics - is solved by
deriving the label straight from the fetched page itself: gocomics.com's
schema.org JSON-LD ComicSeries name, arcamax.com's og:title meta tag.
No hardcoded label table to maintain as comics get added.
Also folds in a since-diagnosed reliability fix for the date-scoped
gocomics.com URL: generation running earlier in the day than the site's
(undocumented) daily rollover was failing outright every time. Rather
than guess the rollover's timezone (an earlier version of this commit
assumed US-Eastern - wrong, and unverifiable either way), it now retries
up to a few days back on a miss and logs when that happens, instead of
dropping the section. This now benefits every gocomics comic, not just
Calvin and Hobbes.
xkcd is unchanged - it only ever serves one comic.
🤖 Generated with Claude Code
Follows the same per-provider reference doc convention master now uses for every other built-in source (see docs: add story provider guides, upstream a556ff9) instead of the inline "## comic source options" README section this branch carried before rebasing onto that change.
Addresses j6k4m8's review comment on this PR: type comic_type as Literal["xkcd", "cah", "garfield"] so editors can hint/autocomplete it. Same idea, updated to the current three values (xkcd/gocomics/arcamax) after the generic-by-slug redesign. Kept as a manually-declared alias next to _COMIC_SOURCES rather than derived from its keys - Literal can't be built from a dict at type-check time (his other suggestion, which he'd flagged as untested). The runtime membership check in __init__ stays: Literal only helps callers written directly in Python, not config-driven ones (see util.py) that pass a plain str straight from JSON.
Replaces comic.py's own inline Pillow re-encode block (bound dimensions, normalize color mode, composite transparency, always JPEG) with a call to the new shared storyprovider/imageutil.py - identical logic, now factored out since RSS-sourced article images need the exact same defensive treatment for the exact same reason (fix/rss-image-embedding, not yet upstreamed). Both PRs carry their own copy of imageutil.py so either can merge first without depending on the other - at worst a trivial identical- content conflict for whichever merges second, same category of expected mechanical overlap already called out for this PR's config.py/util.py registry changes. No behavior change: same tests (comic image processing already had dedicated CMYK/transparency/oversized-image regression tests, now also exercised directly against imageutil in test_imageutil.py), verified live against arcamax.com/gocomics.com. 🤖 Generated with Claude Code
Same issue the maintainer flagged in j6k4m8#141 for rss.py - an unverifiable anecdotal claim in a docstring rather than documentation.
Follows a suggestion from @jpco on j6k4m8#142: image size/format normalization belongs in rendering code, which knows the actual page_profile/layout being rendered, rather than being opt-in per story provider with a guessed size constant. Moves the image re-encode step out of RSSFeedStoryProvider and DailyComicStoryProvider (added in j6k4m8#142/j6k4m8#133) and into Goosepaper itself: - New _image_max_dimension() derives the embedded-image pixel cap from the actual page geometry (page_profile's content width / effective_columns * a target DPI), instead of the flat 1200 both providers guessed at independently. - New _inline_story_images() walks a story's body_html and fetches/ decodes/normalizes any <img> it finds - remote http(s):// or an already-inlined data: URI. _inline_all_story_images() runs it over every story, isolating one story's failure from the rest. - _render_html_document() (used by to_html()/to_pdf()) and to_epub() both call it now. to_epub() previously got no image protection at all from either j6k4m8#142 or j6k4m8#133 - it now does too, sized with a flat fallback constant since there's no page_profile/layout concept for a reflowable epub. - RSSFeedStoryProvider no longer fetches/re-encodes images - leaves <img src> exactly as the source article served it. - DailyComicStoryProvider no longer fetches the strip image at all - only resolves which URL is the actual strip and leaves it as a remote link, same as RSS. image_url is escaped before embedding (a source page's own src attribute value, already entity-decoded by lxml, is otherwise attacker-controlled markup). Net effect: every story provider gets the same image protection uniformly (previously only RSS and comics were covered, each with its own copy of the size-capping logic), and images are sized for the page they're actually rendered on instead of a guess. Verified against a real daily edition (26 RSS feeds + XKCD + puzzles + weather/wikipedia, page_profile: paper_pro, 2-column): identical story/page/image-inlining counts vs. the current per-provider handling, ~40% smaller total embedded image payload. Depends on j6k4m8#142 and j6k4m8#133 - built on top of imageutil.py and the per-provider embedding they added. Known limitations, not hidden: - A story whose image fails to fetch/decode fails soft (keeps the original, unprocessed link) rather than the story being dropped - matches RSS's existing behavior; comic strips previously had a stronger guarantee at the provider level that this removes. - Every image in a document gets the same size cap regardless of placement (ear/sidebar vs. main column) - would need styles.py changes to do properly. - to_epub()'s fallback size is a flat constant, not per-device - real support for j6k4m8#143's use case would need a target-device parameter threaded through. 🤖 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
- comic.py's class docstring and a test_comic.py comment had reintroduced the "verified live" debug-log-style anecdotal-claim pattern that a commit already on this branch (d4fcde5) had deliberately removed from this same file per maintainer feedback on j6k4m8#141 - almost certainly pasted back in from an earlier draft of this docstring written before that fix existed. Restated the same facts without the anecdotal framing. - docs/reference/storyprovider/comic.py.md (added by j6k4m8#133, not touched by the previous commit) still said the strip image "is downloaded, decoded, and re-encoded as JPEG... not linked by remote URL" - exactly backwards after this branch's change. Updated to describe the current behavior (left as a remote link; fetch/ normalize/inline happens centrally in Goosepaper). - Added an end-to-end test running the actual DailyComicStoryProvider.get_stories() (not a hand-built stand-in for its <style>+<div> shape) through a real Goosepaper.to_html() - matching the treatment the Readwise integration test already had, which comic.py's own image-inlining path was missing. 145/145 tests pass.
...RL in place Leaving the original http(s):// src untouched on failure meant WeasyPrint's own image loader tried (and failed) to fetch the exact same dead URL a second time at render time - the same failure got logged twice, once by this function's own "Sad honk :/ Failed to inline image" and once by WeasyPrint's "Failed to load image at ...: HTTPError", plus a wasted second network round trip. Verified live against a real broken case: cst.cam.ac.uk's own page markup has a duplicated <img> tag whose src attribute contains another tag's raw HTML pasted into it (a bug on their end, not ours) - resolves to a URL that always 404s. Before this change, to_pdf() logged both the "Sad honk" line and WeasyPrint's own ERROR line for it. After: exactly one "Sad honk" line, no WeasyPrint error, PDF still renders correctly (the page's other, correctly-formed <img> pointing at the same underlying photo still embeds fine). 145/145 tests pass.
Smengerl
force-pushed
the
feature/render-time-image-sizing
branch
from
August 18, 2026 20:40
f6a68e5 to
3da52be
Compare
Smengerl
added a commit
to Smengerl/goosepaper-logicpuzzles
that referenced
this pull request
Aug 21, 2026
Marks the PRs actually merged upstream since the table was last touched (j6k4m8#118-120, j6k4m8#122, j6k4m8#126, j6k4m8#129, j6k4m8#130, j6k4m8#134, j6k4m8#135, j6k4m8#137, j6k4m8#141), notes j6k4m8#123 as closed in favor of feature/puzzle-explanations, and adds the four rows that were missing entirely: j6k4m8#141 (fix/rss-absolute-url-wrapper-leak, merged), j6k4m8#142 (fix/rss-image-embedding, closed in favor of j6k4m8#145), j6k4m8#144 (fix/rss-prefer-feed-title-config-wiring, open), and j6k4m8#145 (feature/render-time-image-sizing, open).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Follows a suggestion from @jpco on #142: image size/format normalization belongs in rendering code, which knows the actual
page_profile/layoutbeing rendered, rather than being opt-in per story provider with a guessed size constant.Moves the image re-encode step out of
RSSFeedStoryProviderandDailyComicStoryProvider(added in #142/#133) and intoGoosepaperitself:_image_max_dimension()derives the embedded-image pixel cap from the actual page geometry (page_profile's content width /effective_columns* a target DPI), instead of the flat1200both providers guessed at independently._inline_story_images()walks a story'sbody_htmland fetches/decodes/normalizes any<img>it finds - remotehttp(s)://or an already-inlineddata:URI._inline_all_story_images()runs it over every story, isolating one story's failure from the rest._render_html_document()(used byto_html()/to_pdf()) andto_epub()both call it now.to_epub()previously got no image protection at all from either fix: re-encode RSS-embedded article images before rendering, like comics #142 or feat: add "comic" source type for daily comic strips (XKCD, gocomics.com, arcamax.com) #133 - it now does too, sized with a flat fallback constant since there's nopage_profile/layoutconcept for a reflowable epub.RSSFeedStoryProviderno longer fetches/re-encodes images - leaves<img src>exactly as the source article served it.DailyComicStoryProviderno longer fetches the strip image at all - only resolves which URL is the actual strip and leaves it as a remote link, same as RSS.image_urlis escaped before embedding (a source page's ownsrcattribute value, already entity-decoded by lxml, is otherwise attacker-controlled markup).Net effect: every story provider gets the same image protection uniformly (previously only RSS and comics were covered, and each had its own copy of the size-capping logic), and images are sized for the page they're actually rendered on instead of a guess.
Evidence
Same daily edition (26 RSS feeds + XKCD + puzzles + weather/wikipedia,
page_profile: paper_pro, 2-column), current per-provider image handling vs. this branch, generated back to back:paper_pro/2-col)Identical reliability (same story/page/image counts) at meaningfully lower output size.
Testing
_image_max_dimension(): column count, margins, mm vs. in units, the fallback-on-unparseable-size path._inline_story_images(): remote fetch, already-inlineddata:URI, HTTP error response, non-image response, relative/missingsrc, a leading<style>block surviving inlining (the shapecomic.py's CSS + strip<div>has - lxml relocates a body-level<style>into an implied<head>that a naive re-serialize would drop).ReadwiseReaderStoryProviderstory (the one built-in provider besides RSS/comics whose real code path can carry an<img>today) through a realGoosepaper.to_html().to_epub()actually inlining images, and isolating one story's image failure from the rest of the render.Full suite passes (144 tests);
flake8clean.Depends on
Built on top of #142 and #133's
imageutil.pyand the per-provider embedding they added - this branch is based onmastermerged with both of those branches, and would need to land after both are merged upstream (rebased onto whatever their final merged shape looks like).Known limitations (not hidden in the diff)
styles.pychanges to do properly.to_epub()'s fallback size is a flat constant, not per-device - real support for Improved epub rendering? #143 's use case (packing appropriately-sized images for a specific low-power e-reader) would still need a target-device parameter threaded through.🤖 Generated with Claude Code