Optimizer: split oversized paragraphs/chapters, strip fonts, extract data-URI images#39
Optimizer: split oversized paragraphs/chapters, strip fonts, extract data-URI images#39ituri wants to merge 4 commits into
Conversation
The firmware lays out one paragraph at a time, holding every word of the paragraph in parallel in-RAM vectors (ParsedText.cpp). On ~380KB-RAM devices a single multi-KB <p> — common in literary fiction with page-long paragraphs (Fosse, Knausgård, Bernhard, ...) — exhausts the heap during section indexing and crashes the reader with out-of-memory, even when the containing file is small. Large spine files and multi-KB base64 data-URI images cause the same failure mode (see crosspoint-reader/crosspoint-reader#2163, #1752). Add an optional text pass (on by default, gated by the existing optimizer checkbox plus a new "Split large chapters/paragraphs, remove fonts" option) that runs after the image optimizer: - split every <p> larger than 1.6 KB into ~1.2 KB siblings at sentence boundaries, keeping inline tags atomic and dropping duplicate id attributes on continuation paragraphs - split spine files whose <body> exceeds 9.5 KB into ~7 KB files, expanding the OPF manifest/spine and remapping href="...#fragment" references onto the chunk that now holds the anchor - extract base64 data-URI images into real zip entries (re-encoded through the existing image pipeline) so neither the attribute value nor the device's streaming parser has to swallow them - remove embedded font files, their manifest items and @font-face rules - drop page-list navs (print page numbers, dead weight on-device) Every transformation verifies the visible text is unchanged and reverts itself on any mismatch; any unexpected error leaves the EPUB as the image pass produced it, so a transfer is never blocked. Tested against a library of 250 EPUBs including worst cases: a 4.2 MB EPUB with 88 KB single-paragraph chapters and an 88 KB data-URI page (492 KB after, text identical), and a 3.9 MB EPUB with a 1.6 MB single spine file (240 files after, text identical). Books that already fit the limits pass through untouched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 30 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe EPUB optimizer adds an optional text-restructuring pass that splits paragraphs and spine files, remaps links, extracts data images, removes fonts and page-list navigation, verifies visible text, and reports transformation counts. ChangesEPUB text optimization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant optimize_epub
participant split_epub_text
participant EPUB_ZIP_OPF
optimize_epub->>split_epub_text: invoke enabled text processing
split_epub_text->>EPUB_ZIP_OPF: extract images and rewrite XHTML
split_epub_text->>EPUB_ZIP_OPF: expand and remap OPF/spine links
EPUB_ZIP_OPF-->>split_epub_text: rewritten EPUB entries
split_epub_text-->>optimize_epub: return transformation counts
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crosspoint_reader/README.md (1)
63-69: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStep 3 option list omits the new split checkbox.
The feature is described at lines 34-44, but this walkthrough enumerates Device target, JPEG quality, grayscale, and auto-crop without mentioning "Split large chapters/paragraphs, remove fonts" (on by default). Add a bullet so users know it exists and can disable it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crosspoint_reader/README.md` around lines 63 - 69, Add a Step 3 bullet for “Split large chapters/paragraphs, remove fonts,” noting that it is enabled by default and can be disabled. Keep the existing option descriptions unchanged and place the new bullet with the other checkbox options.
🧹 Nitpick comments (1)
crosspoint_reader/textsplit.py (1)
387-387: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
if anchor_map or True:is always true.The
or Truemakes the guard a no-op, which reads like leftover debugging and obscures intent (page-list nav removal is meant to always run, link remap only when anchors exist). Consider splitting the two concerns or dropping the dead condition.♻️ Suggested clarification
- # Remap fragment links onto the chunk holding the anchor; drop page-list navs. - if anchor_map or True: - for n in list(order): + # Remap fragment links onto the chunk holding the anchor; drop page-list navs. + for n in list(order):(dedent the loop body accordingly)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crosspoint_reader/textsplit.py` at line 387, The condition around the page-list navigation removal and link remapping is always true due to `or True`. Split these concerns so page-list navigation removal always runs, while link remapping remains guarded by `anchor_map`, and dedent the loop body as needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crosspoint_reader/textsplit.py`:
- Around line 394-402: Update the split-chunk remapping logic around remap and
its re.sub pattern to also handle fragment-only references such as href="#...".
Resolve these refs against the current source file when looking up anchor_map,
and rewrite them to the owning sibling chunk while preserving the fragment and
existing handling for explicit filenames.
---
Outside diff comments:
In `@crosspoint_reader/README.md`:
- Around line 63-69: Add a Step 3 bullet for “Split large chapters/paragraphs,
remove fonts,” noting that it is enabled by default and can be disabled. Keep
the existing option descriptions unchanged and place the new bullet with the
other checkbox options.
---
Nitpick comments:
In `@crosspoint_reader/textsplit.py`:
- Line 387: The condition around the page-list navigation removal and link
remapping is always true due to `or True`. Split these concerns so page-list
navigation removal always runs, while link remapping remains guarded by
`anchor_map`, and dedent the loop body as needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 12d081c2-e582-4965-897c-db59f77abdb7
📒 Files selected for processing (5)
crosspoint_reader/README.mdcrosspoint_reader/config.pycrosspoint_reader/driver.pycrosspoint_reader/optimizer.pycrosspoint_reader/textsplit.py
📜 Review details
🧰 Additional context used
🪛 ast-grep (0.44.1)
crosspoint_reader/textsplit.py
[warning] 208-208: Regex pattern passed to re is built from a non-literal (variable, call, concatenation, or f-string) value. If that value is attacker-controlled it can introduce a malicious pattern with catastrophic backtracking (ReDoS). Use a hardcoded literal pattern, or validate/escape untrusted input with re.escape() and bound the regex complexity before compiling.
Context: re.search(r'\b%s="([^"]*)"' % attr, tag)
Note: [CWE-1333] Inefficient Regular Expression Complexity.
(redos-non-literal-regex-python)
[warning] 379-380: Regex pattern passed to re is built from a non-literal (variable, call, concatenation, or f-string) value. If that value is attacker-controlled it can introduce a malicious pattern with catastrophic backtracking (ReDoS). Use a hardcoded literal pattern, or validate/escape untrusted input with re.escape() and bound the regex complexity before compiling.
Context: re.sub(r'<(?:\w+:)?itemref\b[^>]\bidref="%s"[^>]/?>' % re.escape(sid),
lambda _m: '\n '.join(new_refs), opf, count=1)
Note: [CWE-1333] Inefficient Regular Expression Complexity.
(redos-non-literal-regex-python)
🪛 Ruff (0.15.20)
crosspoint_reader/textsplit.py
[warning] 122-122: Function definition does not bind loop variable groups
(B023)
[warning] 189-189: Unpacked variable dot is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
[warning] 223-223: Do not catch blind exception: Exception
(BLE001)
[warning] 248-248: Do not catch blind exception: Exception
(BLE001)
[warning] 258-258: Do not catch blind exception: Exception
(BLE001)
[warning] 330-330: Do not catch blind exception: Exception
(BLE001)
[warning] 346-346: Do not catch blind exception: Exception
(BLE001)
[warning] 381-381: Function definition does not bind loop variable new_refs
(B023)
🔇 Additional comments (3)
crosspoint_reader/optimizer.py (1)
82-89: LGTM!Also applies to: 603-610
crosspoint_reader/config.py (1)
35-35: LGTM!Also applies to: 57-58, 77-77, 114-114, 143-150
crosspoint_reader/driver.py (1)
468-468: LGTM!
- Fragment-only refs (href="#note1") are relative to the file they sit in; after a file split the anchor may live in a sibling chunk. Track each chunk's original filename and rewrite such refs to the chunk that owns the anchor (verified with a synthetic EPUB: 25 local footnote links across 3 chunks plus one cross-file ref all resolve). - Drop the dead `or True` condition; page-list nav removal always runs, link remapping only when anchors moved. - List the new split checkbox in the README's step-by-step options. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ituri
force-pushed
the
split-text-for-low-ram-firmware
branch
from
a35c55c to
029ee7d
Compare
- parse_nodes treated XML processing instructions (<?dp ...?> page markers, common in Random House EPUBs) as unclosed opening tags, aborting the text pass for every affected file. - split_text_sentences dropped the boundary whitespace between pieces; when two pieces later landed in the same paragraph group the sentences were joined without a space. The visible-text verification caught it and reverted those files (no corrupt output), but the split was lost. Boundary whitespace now stays with the preceding piece, so ''.join(pieces) == text always holds. Found by batch-converting a 253-book library: 4 books with PI page markers now split normally, 2 books that previously reverted now pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Some publishers wrap entire multi-KB paragraphs in one <span>; such a paragraph survived splitting because tag tokens were atomic. Oversized inline wrappers (span/em/i/b/strong/a/...) are now split recursively into several same-tag siblings. <img> no longer blocks paragraph splitting either — it is inline and stays atomic during grouping. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Problem
The firmware lays out one paragraph at a time, holding every word of the paragraph in parallel in-RAM vectors (
ParsedText.cppin crosspoint-reader). On the ~380 KB-RAM devices, a single multi-KB<p>exhausts the heap during section indexing and crashes the reader with an out-of-memory error — even when the containing spine file is small. Page-long single paragraphs are common in literary fiction (Fosse, Knausgård, Bernhard, …), so affected users hit a hard crash mid-book that no image optimization can fix. Oversized spine files and multi-KB base64data:URI images cause the same failure mode (see crosspoint-reader/crosspoint-reader#2163, crosspoint-reader/crosspoint-reader#1752).Diagnosed on an X4: a German Fosse EPUB crashed reproducibly at the exact page where the layout engine had to ingest the next ~6 KB single-paragraph block; splitting the paragraphs (not the files) fixed it.
Change
Adds a text pass (
textsplit.py) that runs after the existing image optimizer, gated by the optimizer checkbox plus a new "Split large chapters/paragraphs, remove fonts (prevents out-of-memory)" option (on by default):<p>larger than 1.6 KB is split into ~1.2 KB siblings at sentence boundaries. Inline tags are kept atomic; continuation paragraphs reuse the opening tag minus itsid. Paragraphs containing block-level tags are left alone.<body>exceeds 9.5 KB are split into ~7 KB files; the OPF manifest/spine is expanded andhref="…#fragment"references (content, nav, NCX) are remapped onto the chunk that now holds the anchor. The first chunk keeps the original filename and manifest id, so fragment-less links and guide references stay valid.data:image/*attributes are decoded into real zip entries, re-encoded through the existingprocess_imagepipeline, and added to the manifest (found in the wild: an 88 KB base64 attribute inside a spine file, invisible to the image optimizer).@font-facerules are dropped.Safety
Every text transformation verifies that the visible text of its output is identical to its input and reverts itself on any mismatch; any unexpected error leaves the EPUB exactly as the image pass produced it. This matches the existing optimizer contract that a transfer is never blocked or corrupted.
Testing
Version is intentionally not bumped (left to your
make bump-*release flow).🤖 Generated with Claude Code