Skip to content

Navigation Menu

Sign in
Sign up

refactor: move image re-encoding from story providers to render time - #145

Open
Smengerl wants to merge 16 commits into
j6k4m8:master from
Smengerl:feature/render-time-image-sizing
Open

refactor: move image re-encoding from story providers to render time #145
Smengerl wants to merge 16 commits into
j6k4m8:master from
Smengerl:feature/render-time-image-sizing

Conversation

@Smengerl

@Smengerl Smengerl commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

What

Follows a suggestion from @jpco on #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 #142/#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 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 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, 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:

current (#142/#133) this branch
stories 101 101
PDF pages 75 75
images successfully inlined 40/44 40/44
max embedded pixel dimension 1200 (flat guess) 656 (derived from paper_pro/2-col)
total embedded image payload 2.78 MB 1.68 MB (-40%)

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-inlined data: URI, HTTP error response, non-image response, relative/missing src, a leading <style> block surviving inlining (the shape comic.py's CSS + strip <div> has - lxml relocates a body-level <style> into an implied <head> that a naive re-serialize would drop).
  • End-to-end: a real ReadwiseReaderStoryProvider story (the one built-in provider besides RSS/comics whose real code path can carry an <img> today) through a real Goosepaper.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); flake8 clean.

Depends on

Built on top of #142 and #133's imageutil.py and the per-provider embedding they added - this branch is based on master merged 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)

  • A story whose image fails to fetch/decode fails soft (keeps the original, unprocessed link) rather than the story being dropped - matches the behavior RSS images already had; comic strips previously had a stronger "raise and drop cleanly" guarantee at the provider level that this removes.
  • Every image in a document gets the same size cap regardless of placement - an ear/sidebar image gets the same cap as a main-column image even though it renders narrower. Would need styles.py changes 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

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.

Copy link
Copy Markdown
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.

Smengerl and others added 16 commits August 18, 2026 22:38
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 f6a68e5 to 3da52be Compare August 18, 2026 20:40
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).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

AltStyle によって変換されたページ (->オリジナル) /