Lazy-load each page's image, not just its text layer - #151
Conversation
SupernoteView.onLoadFile() rasterized every page of a note (via the worker pool), decoded every page's PNG into an ImageBitmap, and drew every page's canvas — all unconditionally, all up front, regardless of page count or how much of the note the user ever actually scrolls to. Memory scales linearly with page count: roughly a decoded ImageBitmap plus a canvas backing store (up to 9x pixel count at capped 3x DPR) resident at once, for every page, for as long as the note is open. The text layer (selection/search) already avoids exactly this via pageObserver + ensureTextLayer() - only building once a page nears the viewport. Canvas rendering had no equivalent laziness at all. Adds ensurePageImage(), the same idempotent/de-duplicated pattern as ensureTextLayer(), triggered from the same pageObserver plus explicit page-jump (goToPage) and "Save image to vault" (which can't assume the observer already fired). Page containers/canvases are still built for every page immediately - cheap, since sn.pageWidth/pageHeight size them without rasterizing anything - but each page's actual image loads only once it's about to scroll into view. The thumbnail sidebar previously reused the same eagerly-rasterized images; it now starts with empty thumbnails that fill in as pages load, or all at once if the sidebar is opened before scrolling (it shows every page at once, so opening it needs every page's image). Motivated by #147 (crash/"boot loop" opening a .note file on iOS) - this is a plausible contributing factor (more resident memory going into whatever's actually crashing downstream) but not a confirmed fix for that specific report; it's a real, independently-worthwhile memory reduction regardless of whether it's the root cause there.
The blank/wrong-page bug just introduced by lazy page-image loading: processChunk() assigned worker.onmessage fresh for every call, safe only because processPages() used to be the sole caller and always split one call's work 1:1 across distinct workers in a single Promise.all - so no worker ever had two in-flight requests at once. ensurePageImage() (this branch's own lazy-loading commit) breaks that: each page load is its own separate processPages() call for a single page, which chunkPageNumbers() always turns into exactly one chunk - previously always dispatched to workers[0] (a one-chunk call's index is always 0). Several pages loading nearly at once (fast scroll, or opening the thumbnail sidebar, which loads every page at once) sent several concurrent requests to worker 0, each overwriting the last's onmessage handler before its response arrived - silently losing that page's result (renders blank forever) or resolving the wrong page's promise with it (wrong image on the wrong page). Reproduced in a standalone simulation (not included - depends on nothing from this repo) with fake workers responding after a random delay: the old logic hangs nearly every time under 12 concurrent single-page requests across 4 workers; queuing per-worker and round-robining across every call (not just within one processPages() call) resolves all 12 correctly, every time across 5 runs. Fix: queue requests per worker (a second request to the same worker now waits for the first's response before sending, rather than clobbering its handler), and round-robin worker selection across every call this pool ever makes, not reset per processPages() call - so concurrent single-page requests still spread across every worker instead of piling onto worker 0.
|
Fixed a real bug this patch introduced, matching exactly what was reported (12-page note: page 1 blank then wrong/late, pages 8 and 10 permanently blank). Root cause: `WorkerPool.processChunk()` assigned `worker.onmessage` fresh on every call - safe only because `processPages()` used to be the sole caller and always split one call's work 1:1 across distinct workers in a single `Promise.all`, so no worker ever had two in-flight requests at once. `ensurePageImage()` breaks that assumption: each page load is its own separate `processPages()` call for a single page, and a single-page chunk always dispatched to `workers[0]`. Several pages loading nearly at once (fast scroll, or opening the thumbnail sidebar, which loads every page at once) sent concurrent requests to worker 0, each overwriting the previous one's `onmessage` handler before its response arrived - silently losing that page's result (blank forever) or resolving the wrong page's promise with it (wrong/late image). Verified with a standalone simulation (fake workers, random response delay, not part of the repo): the old logic hangs on ~most runs under 12 concurrent single-page requests across 4 workers; the fix (queue per worker, round-robin across every call rather than resetting per Latest commit ( |
Pre-existing bug, not something the lazy-loading changes in this branch introduced - found while manually testing this PR's "drag a page's canvas out" item: the canvas was never draggable at all, while sidebar thumbnails (plain <img>, no overlay) were. .textLayer and .supernote-links-layer are both position: absolute; inset: 0, fully covering the canvas, and stacked on top of it in paint order. pointer-events was never set anywhere in this stylesheet, so both left it at the default `auto` - meaning they captured every mouse event anywhere on the page, not just clicks on an actual text span or link rect, blocking the canvas's own manually-wired drag-out underneath entirely. Sets pointer-events: none on both containers and pointer-events: auto back on the actual interactive children (text spans/br, link rects) - the standard pattern for an overlay with occasional interactive hotspots, and what .textLayer's own comment already said it was supposed to mirror from pdf.js's text_layer_builder.css.
|
Found and fixed the drag bug too - your text-layer hypothesis was exactly right. `.textLayer` and `.supernote-links-layer` are both `position: absolute; inset: 0`, fully covering the canvas and stacked on top of it. `pointer-events` was never set anywhere in `styles.css`, so both stayed at the default `auto` - meaning they captured every mouse event anywhere on the page (not just clicks on an actual text span/link), blocking the canvas's manually-wired drag-out entirely. This is a pre-existing bug, not something this branch's lazy-loading change introduced - the code comment even flagged the drag feature as never fully verified. Fix (commit `5ff55a6`): `pointer-events: none` on both overlay containers, `pointer-events: auto` back on the actual interactive children (text spans, link rects) - the standard "overlay with occasional hotspots" pattern, matching what `.textLayer`'s own comment already said it was supposed to mirror from pdf.js. Worth re-testing drag-out now along with the blank-page fix from before. |
…p Obsidian
Reported live after the pointer-events fix unblocked the canvas from
ever receiving drag events at all: "dragging the image on my macbook
m1 13 to a new markdown note creates a HUGE PNG as a base64 encoded
URL and locks up obsidian."
The drag wiring put the page's raw data:image/png;base64,... straight
into the drag data - there was never a real vault file for the drop to
link to, so dropping it into a note inserts the entire base64 blob as
literal text. For a real page-resolution image that's enough inline
text to lock up the editor. The original code's own comment already
flagged this as unverified ("worth confirming it actually reproduces
useful drag-and-drop behavior in a real vault"), so it likely never
worked correctly - the pointer-events fix just unblocked the canvas
from ever receiving the events that would have exposed it.
A correct fix needs the same real-vault-attachment step "Save image to
vault" already does before the drop, but dataTransfer can only be
populated synchronously inside dragstart, while creating that
attachment is async - not a small change, and this repo already treats
attachment creation as a deliberate, explicit action rather than
something that happens just from viewing a page. Removed rather than
shipped broken; see #152 for what a real fix would need.
|
Removed canvas drag-out entirely (commit `fd48318`) rather than try to patch it further. The pointer-events fix unblocked the canvas from ever receiving drag events at all - which is what surfaced this: the drag wiring put the page's raw `data:image/png;base64,...` straight into the drag data. There was never a real vault file backing it, so dropping it into a note inserts the entire base64 blob as literal text - enough to lock up the editor for a real page-resolution image, exactly as reported. A correct version needs the same real-attachment-file step "Save image to vault" already does, but that's async (`app.vault.createBinary`) while `dataTransfer` can only be populated synchronously inside `dragstart` - not a small fix, and this repo already treats attachment creation as a deliberate, explicit action (the button), not something that happens just from viewing a page. Filed #152 to track a real fix if it's ever worth the complexity; for now "Save image to vault" is the safe equivalent. Test plan updated: drag-out is no longer something to verify (it's gone), "Save image to vault" still is. |
Fixes philips#154: a 100-page note still crashed on an iPhone 13 mini around page 14-20, even with philips#151's lazy loading - because that only bounds *when* a page's image loads, not whether it ever gets released. Nothing freed an earlier page's decoded ImageBitmap or canvas backing store once loaded, so memory still grew monotonically with how many pages you'd scrolled past, not how many are actually near the viewport right now. Adds evictPageImage(), called from the same pageObserver that already triggers lazy loading (see onOpen()): once a page's container leaves the 100%-margin window, its ImageBitmap is explicitly closed and its canvas backing store shrunk to 1x1, reclaiming that memory. Scrolling back re-triggers ensurePageImage() exactly as if the page had never loaded, since both its imageBitmap and imageLoadPromise are reset to null. Also fixes a related gap this exposed in drawPageImage(): it resized every page's canvas backing store unconditionally, including pages with no image loaded at all. commitZoom() calls drawPageImage() for every page in the document on every zoom change - so touching zoom on a long document would have reallocated a full-size backing store for every page regardless of whether it was ever loaded, undoing lazy loading's benefit immediately. Split so CSS (layout) size still always updates for every page (scroll position/page-anchor links depend on that even for unloaded pages), but the backing store itself is now only resized when there's an actual image to draw. Text layers (search/selection) are deliberately left alone - much lighter than a decoded bitmap, and losing highlights/search state on scroll-past would be a worse trade for a small saving. Known related gap, not fixed here: toggleThumbnails() still loads every page's image at once when the sidebar is first opened, which would reintroduce the same unbounded-memory problem for a long document if a user opens it. Out of scope for this fix since philips#154's report is specifically about scrolling, not thumbnails.
2 participants
Summary
Candidate memory-reduction patch for #147 (crash/"boot loop" opening a
.notefile on iOS). Not a confirmed fix - we still don't have a root cause pinned down for that specific report (see the issue for the ongoing investigation via #150's diagnostic logging) - but this is a real, independently-worthwhile change regardless:onLoadFile()currently rasterizes, decodes, and canvas-renders every page of a note unconditionally on open, so memory scales linearly with page count no matter how much of the note is ever actually looked at. On a memory-constrained device that's a real contributing risk even if it's not the exact trigger for this report.The text layer (selection/search) already avoids exactly this shape of problem via
pageObserver+ensureTextLayer()- only building once a page nears the viewport. Canvas image rendering had no equivalent laziness.Changes
ensurePageImage(), mirroringensureTextLayer()'s idempotent/de-duplicated pattern, triggered by the samepageObserver, plus explicit page-jump (goToPage) and "Save image to vault" (which can't assume the observer already fired).sn.pageWidth/pageHeightsize them without rasterizing anything), but each page's actual image loads only once it's about to scroll into view.state.imageDataUrl(populated onceensurePageImageloads) instead of a closure variable that no longer exists once images aren't all rasterized up front.What I could not verify
I don't have Obsidian, an X server, or Electron tooling available in my environment, so I could not launch the app and click through this - verification here is
tsc/eslint/the existing test suite plus a careful manual trace of every consumer of the changed state (zoom'scommitZoom(), thumbnails, drag-out, save-button, find, page-jump). Several of those already had graceful "not loaded yet" handling before this change (drawPageImage()'s null-bitmap guard,commitZoom()'s "only rebuild what's loaded" check), which is what makes me reasonably confident, but please smoke-test zoom, thumbnails, drag-out, and save-to-vault on a real multi-page note before merging - this touches more interacting features than a typical patch.Test plan
npx tsc --noEmit- cleannpx eslint src/main.ts- 0 errorsnpx vitest run- 95 passednpm run build- clean