Show a preview for images too large for the browser to display (BL-16597) - #8111
Conversation
The image chooser accepts .tif/.tiff, but the orientation tag was only looked for at the JPEG APP1 query path, so a rotated TIFF was treated as upright and previewed sideways. Try the TIFF top-level IFD path as well. Also: the comment in HandleLocalFilePreview claimed the stand-in is "generated once and reused". That is true of the stand-in, but for an image small enough to serve as-is there is nothing to cache, so the (cheap) header read does repeat per request. Say what actually happens. Both noticed by Devin (informational flags on PR #8111). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The cached downscaled stand-in was keyed only on "one exists", so nothing in HandleLocalFilePreview itself established that it belonged to the file being requested; that was an invariant maintained at a distance, by the order in which HandlePickLocalImageFile cleared the cache and reassigned the authorized path. Unreachable in practice — picking a file means working a modal dialog, and the front-end only asks for the preview URL after the pick replies — but it is cheaper to make the cache say what it is about than to keep the reasoning. Remembering the source file also lets us remember "this one needs no stand-in", so an image small enough to serve as-is is no longer re-examined on every request. Devin raised this on PR #8111; the suggested hardening is what is implemented here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This is the only place in Bloom that uses WPF's WIC-backed imaging, and the API server calls it from a thread-pool thread. That is fine as long as the bitmaps are frozen, which MakeBrowserSafePreview does — but NUnit runs this project's tests on an STA thread, so nothing here actually exercised the apartment the real caller uses. Run it on an MTA thread and check the result. Raised by Devin on PR #8111, which asked for a runtime sanity check; a test seemed better than checking once by hand. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
(Claude Opus 5) Consulted Devin on 2026-07-27, up to commit What it raised, and where each landed:
CI ( |
|
(Claude Opus 5) Consulted Devin again on 2026-07-27, up to commit Since the last log, John made the calls on the two findings that were waiting, and both are now fixed with their threads resolved:
One thing to be aware of when reading Devin's page directly: its last three jobs each re-list findings that were already fixed, quoting line numbers from before the fix — the transparency bug is still reported at CI green. Greptile still silent on this PR — as on #8110 — so it has been recorded as timed out rather than clean. |
…597) Chromium (and therefore WebView2) refuses to decode an image whose pixel count times 4 bytes overflows a signed 32-bit int, so the image chooser showed an empty preview pane for a 30000x23756 scan. Decoding at a reduced size does not help: the limit is tested against the image's natural size before any scaling. So when a picked file is over 40 megapixels, the localFilePreview endpoint now serves a downscaled JPEG instead of the original. WIC's DecodePixelWidth does the scaling as part of the decode, so a 713 megapixel JPEG costs about 2.5 seconds and under 100MB rather than the ~2.6GB a full decode would need. Any EXIF rotation is baked into the stand-in, since it is re-encoded without an EXIF block. The stand-in is generated once per picked file and reused, because the gallery asks for the same URL as both thumbnail and large preview; it is deleted when another file is picked or when the project's lifetime scope is disposed. pickLocalImageFile now also reports the original file's dimensions and byte count, so the gallery describes the file the user actually chose rather than the downscaled stand-in it is being shown. Tests cover reading dimensions (including the not-an-image fallback), the below-threshold "serve the original" case, and the downscaling itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The image chooser accepts .tif/.tiff, but the orientation tag was only looked for at the JPEG APP1 query path, so a rotated TIFF was treated as upright and previewed sideways. Try the TIFF top-level IFD path as well. Also: the comment in HandleLocalFilePreview claimed the stand-in is "generated once and reused". That is true of the stand-in, but for an image small enough to serve as-is there is nothing to cache, so the (cheap) header read does repeat per request. Say what actually happens. Both noticed by Devin (informational flags on PR #8111). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The cached downscaled stand-in was keyed only on "one exists", so nothing in HandleLocalFilePreview itself established that it belonged to the file being requested; that was an invariant maintained at a distance, by the order in which HandlePickLocalImageFile cleared the cache and reassigned the authorized path. Unreachable in practice — picking a file means working a modal dialog, and the front-end only asks for the preview URL after the pick replies — but it is cheaper to make the cache say what it is about than to keep the reasoning. Remembering the source file also lets us remember "this one needs no stand-in", so an image small enough to serve as-is is no longer re-examined on every request. Devin raised this on PR #8111; the suggested hardening is what is implemented here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This is the only place in Bloom that uses WPF's WIC-backed imaging, and the API server calls it from a thread-pool thread. That is fine as long as the bitmaps are frozen, which MakeBrowserSafePreview does — but NUnit runs this project's tests on an STA thread, so nothing here actually exercised the apartment the real caller uses. Run it on an MTA thread and check the result. Raised by Devin on PR #8111, which asked for a runtime sanity check; a test seemed better than checking once by hand. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two things the reviewers turned up, both decided by John. Substituting a stand-in was keyed on size alone, but size is only one of the two reasons a browser won't display a picked file. The chooser's filter accepts .tif/.tiff and no browser will draw one, so a TIFF showed the same empty pane this card is about — and, once big TIFFs started being converted on their way past the size threshold, whether a TIFF previewed depended on how many pixels it had, which is not explicable to anyone. Bloom already applies the rule "convert what browsers can't draw so they can draw it" when importing: a TIFF lands in the book folder as a PNG. This is the same rule, applied to the preview. Matched on extension rather than on the bytes, deliberately: ReplyWithImage takes the response's content type from the extension, so the extension is what the browser goes on. An image already smaller than kPreviewMaxDimension is no longer enlarged, since a stand-in made only because of format has no reason to grow. A stand-in is a JPEG, which has no alpha channel, so a picture with see-through areas previewed with them solid black — whatever sits in the colour channels under a transparent pixel. Composite onto white first. Done unconditionally rather than only for images carrying transparency: deciding that reliably means enumerating the pixel formats that can hold alpha and checking indexed palettes for a transparent entry, against one pass over an image already capped at 1600px. It is pixel arithmetic rather than the WPF rendering stack, so it stays safe on the API server's threads. The transparency test was confirmed to fail without the fix (corner pixel came out 0,0,0), so it is testing what it claims to. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Devin pointed out that every image fixture so far was orientation 1, so none of the rotated paths were exercised and a regression in them would go unnoticed. It also observed that baking in the rotation by hand is only correct if WPF's decoder doesn't already do it — true as far as anyone knew, but nothing established it. Both are now settled by the same test. A JPEG fixture carrying a chosen EXIF orientation is built by splicing a hand-written APP1 segment in after the start-of-image marker (System.Drawing cannot construct a PropertyItem to attach one, and pulling WPF into the test project just to write a fixture costs more than twenty bytes of well-specified header). All eight orientations are then checked: 5-8 must report the stored dimensions swapped, 1-4 unswapped. Those first four would pass even if the tag were being ignored entirely, but 5-8 cannot — and they would also fail if WPF were auto-rotating, since the swap would then be applied twice. Also covers an orientation value outside 1-8 being ignored rather than reaching the rotation table. Riding along: a "seen again" note on the existing papercut about the nine tests build/agent-dotnet.sh cannot run, which cost time again on this branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Noticed while preflighting this branch. One full-suite run showed 12 failures rather than the usual 9 environmental ones; a second run at the same commit showed 9, and the three extras passed in isolation. None of them are near this branch's subject. Recording it because the cost is that a run of 12 can no longer be read at a glance — you have to run the suite twice to find out which failures are noise. Docs only; no product code in this commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
6a28662 to
202bc79
Compare
…BL-16597) Devin caught this on the rebased branch. RegisterEndpointHandler's requiresSync parameter defaults to true, and imageGallery/localFilePreview was registered with only three arguments, so it ran inside BloomApiHandler's main SyncObj. That lock is what serializes most api requests against each other, and this handler holds it for as long as it takes to decode and re-encode the picked image — about two and a half seconds for the 713 megapixel scan in the tests. Every other synchronized request waits that out, so choosing a very large image in the chooser could briefly freeze unrelated parts of the UI. Nothing here needs that lock. The only state shared between requests is the preview cache, and _previewLock already guards every read and write of it (and still coalesces the gallery's near-simultaneous thumbnail and large-preview requests for the same file into a single build). The endpoint is also not on the short list that BloomApiHandler routes to ThumbnailsAndPreviewsSyncObj, so opting out is the same move the i18n endpoints already make, for the same reason. One consequence worth recording: serving the file (ReplyWithImage) already happened outside _previewLock, and pickLocalImageFile still takes the global lock, so the two can now overlap where previously they could not. If a new file were picked in the window between choosing the path and streaming it, the temp stand-in could be deleted mid-serve and the preview would fail to load. That needs a modal file dialog to be opened and confirmed inside a file-serve window, and the worst outcome is a broken preview image, so it is left alone rather than paid for with a longer lock hold. Riding along, per the papercut convention: the entry about three intermittently failing C# tests no longer opens by counting them "on top of the nine environmental ones". Master's PR #8107 fixed those nine and deleted their papercut, and a full run this session came back 0 of 3027 — so the entry now states its own case, and records that a green baseline makes those three the only remaining noise. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
(Claude Opus 5 (1M context)) Consulted Devin on 2026-08-04, across the rebase — two jobs, up to commit The branch was rebased onto current Two findings were acted on, each with its own thread:
The rest of what it reported describes code this branch has already changed, and each already has a resolved thread from the 27 July run: the transparency bug (the JPEG encode is wrapped in Also green at this commit: CI ( |
|
I think I'm lost on the premise of this change... |
JohnThomson
left a comment
JohnThomson
left a comment
There was a problem hiding this comment.
[Claude Opus 5 (1M context)] Good question — the confusion dissolves once you notice that steps 2 and 3 of the card are looking at two different files.
Step 2, pasting into Edit, imports the image. Import resizes anything larger than 3840x2800 and saves the smaller copy into the book folder — the card predicts the result itself: "should shrink to about 3535x2800". So what edit mode displays is a ~10 megapixel copy, which WebView2 draws without complaint. That resize is also where the ~8 seconds in step 2 goes.
Step 3, choosing that image in the chooser, points at the original file still sitting in the Van Gogh folder: 30000x23756, about 713 megapixels. Nothing has touched it. WebView2 gives up somewhere above 537 megapixels (int32 max divided by 4 bytes per pixel), firing error instead of load, so the pane stays empty.
So both statements are true and not in tension: WebView2 genuinely cannot draw the original, and edit mode never asks it to — it only ever sees the imported copy. The image chooser is the one place in Bloom that displays a file before it has been imported, which is why this is the only place the problem appears.
On the duplication question, which is the fair one to ask: the server does already have image processing that resizes (RuntimeImageProcessor / ImageUtils.AdjustImageForDisplay, down to the same 3840x2800). Two things stop it covering this case:
- It only runs for files that are already in the book folder —
processImage = !isSvg && sourceDir == CurrentBook?.FolderPathinBloomServer. The chooser is previewing a file that is still just sitting on the user's disk, so it goes out untouched. - It opens the image with
PalasoImage.FromFileRobustlybefore it can measure or resize it, which is a full decode — the 8 seconds you see on import, and about 2.6GB for this file. Doing that in the chooser would mean paying it just to look at a candidate the user may not even pick.
The new code's one distinctive move is asking WIC to scale during the decode (DecodePixelWidth), so the huge file never becomes a full bitmap in memory at all. So this isn't a second copy of the resize logic — it is the case that logic can't reach, minus the expensive part of it.
@JohnThomson+AGNT made 1 comment.
Reviewable status: 0 of 4 files reviewed, 8 unresolved discussions (waiting on JohnThomson).
andrew-polk
left a comment
andrew-polk
left a comment
There was a problem hiding this comment.
@andrew-polk reviewed 4 files and all commit messages, and resolved 8 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on JohnThomson).
andrew-polk
left a comment
andrew-polk
left a comment
There was a problem hiding this comment.
Reviewable status:
3 participants
Chromium — and therefore WebView2 — refuses to decode an image whose pixel count times 4 bytes/pixel overflows a signed 32-bit int (about 536.9 megapixels). The
<img>fireserrora moment after the bytes arrive and nothing paints, which is why the image chooser showed an empty preview pane for a 30000x23756 scan. Decoding at a reduced size doesn't help:createImageBitmap()withresizeWidthand the WebCodecsImageDecoderwithdesiredWidthboth fail identically, because the limit is tested against the image's natural size before any scaling is applied. And well below that hard ceiling, handing the renderer a hundred-megapixel image still costs seconds of decode and gigabytes of RAM to fill a preview pane a few hundred pixels tall.So when a picked file is over 40 megapixels,
imageGallery/localFilePreviewnow serves a downscaled JPEG instead of the original:DecodePixelWidth/DecodePixelHeightdoes the scaling as part of the decode, so a 713 megapixel JPEG costs about 2.5 seconds and under 100MB rather than the ~2.6GB a full decode would need.imageGallery/pickLocalImageFilenow also reports the original file's pixel dimensions and byte count, so the gallery describes the file the user actually chose rather than the downscaled stand-in it is being shown.Tests cover reading dimensions (including the not-an-image fallback), the below-threshold "serve the original" case, and the downscaling itself.
Ref: https://issues.bloomlibrary.org/youtrack/issue/BL-16597
Devin review
This change is