Cut the downscaled pano copy on demand, at the width the viewer asks for - #5257
Merged
Conversation
…for (#5256) We have now twice precomputed a downscaled copy of every wide panorama so that Pannellum can texture it -- nightly in the app, which OOM-killed prod JVMs, and at stitch time in the scraper, which leaves a multi-terabyte backfill for the existing store. Measured on prod, the five largest cities hold 140,599 expired wide panos between them and served 29 views of one in ninety days: roughly 4,850 copies cut per copy looked at. The threshold was also wrong by a factor of two. Pannellum uploads an equirect as two halves, so its refusal test is max(width/2, height) > MAX_TEXTURE_SIZE -- a device advertising 8192 renders a 16384-wide pano, which is the widest GSV produces. Most hardware never needed a copy at all, and no fixed server-side cap can know which does. So the viewer asks. PannellumViewer reads 2 x MAX_TEXTURE_SIZE from a throwaway context and appends ?maxWidth= only when the stored pano exceeds it; PanoDisplayCopyService cuts that width on demand and caches it under the crop store, single-flighted per pano and capped at two concurrent cuts so a burst cannot stack allocations the way #5239 did. Cutting one costs ~105 MB and ~2 s, not the ~390 MB and ~10 s that took prod down: ImageReadParam.setSourceSubsampling reduces inside the decode, so no full-size raster and no rescale pass ever exist. It drops pixels where an area average would blend them (PSNR ~30.6 dB against a box-filtered reference), accepted deliberately for a copy this rarely needed. Widths are snapped to an allowlist because the HMAC covers the path and not the query. Also corrects the comments in PanoDataService, CropService and architecture.md that said a whole-pano derivative needs more heap than a city stage has. It does not; precomputing one for the whole store nightly was the problem. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J4MKBB8ZAQqsH1rzb5xG65
…says it will The capability check reads what the driver advertises, and that is a promise about dimensions rather than about memory. Measured on two phones: an empty texImage2D reservation of two 8192-square textures returns in ~1 ms, while actually writing every row of them takes 380-486 ms and half a gigabyte of RGBA. A device under memory pressure can therefore pass its own MAX_TEXTURE_SIZE test and still fail the allocation, and nothing readable from the page predicts which devices those are or when. So the viewer now tries progressively smaller copies when a load fails, in both initialize() and loadPano(): the width the advertised limit calls for, then half, then a quarter, floored at the narrowest width the server will cut. Two retries, because a GPU that cannot hold a quarter of what it advertised will not be rescued by an eighth. The failed viewer or scene is torn down first, since both hold the URL that just failed. On the hardware measured this never runs -- both phones allocate the full 512 MiB -- but it is the only path that covers a device whose advertised limit is not the one it can honour. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J4MKBB8ZAQqsH1rzb5xG65
/backupImage could serve a `<panoId>.w8192.jpg` copy written beside the native file by the scraper, and the nightly crop job counted how many wide panos were missing one, warning when any were. Both rest on a threshold wrong by a factor of two -- Pannellum uploads an equirect as two halves, so a device advertising 8192 renders the 16384-wide frames GSV produces -- and on a scraper-side writer that does not exist: sidewalk-panorama-tools#115, the issue that would have produced these files, was closed unbuilt. So the count was 100% missing forever, warning on every nightly background_job_run row about devices that either render the native frame fine or now get a copy cut at the width they asked for. Removes pano.downscaled.max-width, PanoDataService's three sidecar members, PanoDataTable.getWideBackupPanos (its only caller was the count), and CropService's countSidecars with its four run-result fields and JSON keys. /backupImage without ?maxWidth= now serves the native file. deployment-and-stages.md described the pano store as holding derived .w8192.jpg files; display copies are cut on demand into SIDEWALK_IMAGES_DIR under pano-display/ instead, so that store holds nothing derived and a copy of it can take the directory whole. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J4MKBB8ZAQqsH1rzb5xG65
Reviewed by Opus 5 and by Gemini 3.1 Pro and 3.8 Flash through agy. Each found something the others missed; the fixes are grouped by what they were. The fallback ladder was anchored on the GPU's advertised cap rather than on the width rung 0 is actually served at. The server answers any maxWidth at or above a pano's own width with the native file, so on a device whose cap exceeds the pano -- a 16384 MAX_TEXTURE_SIZE against an 8192 pano, say -- every rung re-fetched, re-decoded and re-uploaded the image that had just failed, making the next allocation likelier to fail rather than less. On that hardware the ladder never stepped down at all. It now starts from min(cap, width), and a new jest suite pins the widths for six device/pano pairs; it fails on the old line. Cuts no longer block a cpu-intensive thread. That pool has parallelism-max = 4 and is also pekko.stream.materializer.dispatcher, so two cuts waiting on the old semaphore could park half the threads serving every streamed API response, the crop job and the access-score pass -- for up to thirty seconds, doing nothing. A dedicated two-thread pool with a bounded queue puts the waiting in the queue, and a refused cut serves the native file at once, which is what the device's own ladder wants anyway. A cut that failed fatally returned 500 rather than the native file. OutOfMemory -- the failure this whole design exists to survive -- is Fatal, so the NonFatal catch missed it and the route withheld the file exactly when memory was what went wrong. Caught by name in the service, with the fallback also stated at the route so it holds however the service changes. 16384 leaves the width allowlist. It cannot cut anything today, since nothing GSV produces is wider; it could only ever apply to a source wider than itself, and would then allocate a 384 MiB raster against the ~105 MB two concurrent cuts are budgeted for. Restoring it means budgeting in bytes rather than in cuts. Also: inFlight now removes by value, so a completed entry handed to a second caller cannot evict a third caller's live cut; a total load failure clears currentSceneId, so a later label on that pano can't take the already-loaded path and draw its marker over Pannellum's error table; and the retry path's removeScene call is gone -- addScene overwrites, and removeScene refuses the current scene, which is exactly what a just-failed scene is. Its two comments described a library that throws where this one returns false. pano-display/ is documented as derived, disposable and deliberately unpruned, with the numbers that make that safe: three files per wide pano at most, cut only for hardware that cannot texture the native file, against five cities holding 140,599 wide expired panos that drew 29 views in ninety days. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J4MKBB8ZAQqsH1rzb5xG65
The paragraph explaining why `pano-display/` needs no sweeper leaned on counts of how many wide expired panoramas exist and how often one is viewed. Those numbers are not load-bearing: the conditions for cutting a copy rarely coincide, and a given pano is seldom looked at twice. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J4MKBB8ZAQqsH1rzb5xG65
Conflict in docs/architecture.md: develop added a fifth crop-marker surface (the dashboard's mistake cards) and extended the attribution sentence to name PanoViewerLogo.js and the crop cards, while this branch rewrote the pano paragraph. Kept both -- develop's surface list and attribution wording, this branch's on-demand downscale prose -- and dropped develop's sidecar-counting sentences, which describe the mechanism this branch removes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J4MKBB8ZAQqsH1rzb5xG65
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.
Fixes #5256.
Why
We've precomputed a downscaled copy of every wide panorama twice now — nightly in the app (OOM-killed prod JVMs, #5239) and at stitch time in the scraper, which needs a long backfill before it helps at all. Both spend that work up front, across the board, on frames that are individually almost never looked at — so precomputing cuts overwhelmingly more copies than anyone ever asks for.
And the threshold was wrong by 2x. Pannellum uploads an equirect as two halves, so its refusal test is
max(width/2, height) > MAX_TEXTURE_SIZE— a device advertising 8192 renders a 16384-wide pano, exactly the widest GSV produces. Most hardware never needed a copy, and no fixed server-side cap can know which does.What this does
The viewer asks.
PannellumViewerreads2 * MAX_TEXTURE_SIZEfrom a throwaway context (cached, released immediately) and appends?maxWidth=only when the stored pano exceeds it. Every device that can render the pano as stored gets it untouched, and there's no failed first load to recover from — the device's limit is knowable before the request, so this never depends on parsing Pannellum's localized error string.The server cuts it on demand.
PanoDisplayCopyServiceproduces that width and caches it under the crop store (derived, disposable, app-writable) atpano-display/<xx>/<panoId>.w<width>.jpg— a new directory, so the crop job's retiredpano-downscaled/stays unambiguously deletable on prod.The scraper sidecar path is removed, not kept alongside.
/backupImagecould serve a<panoId>.w8192.jpgwritten beside the native file, and the nightly crop job counted how many wide panos were missing one, warning when any were. Both rested on the same 2x-wrong threshold, and on a scraper-side writer that does not exist — ProjectSidewalk/sidewalk-panorama-tools#115, the issue that would have produced these files, was closed unbuilt. So the count was 100% missing forever, warning on every nightlybackground_job_runrow about devices that either render the native frame fine or now get a copy cut at the width they asked for.That drops
pano.downscaled.max-width,PanoDataService's three sidecar members,PanoDataTable.getWideBackupPanos(its only caller was the count), andCropService'scountSidecarswith its four run-result fields and JSON keys./backupImagewithout?maxWidth=now serves the native file. Nothing here touches the scraper or EC2.The measurement this rests on
Benchmarked on JDK 17 against a real 16384x8192 Chicago pano:
-XmxsetSourceSubsampling(2,2,0,0)Letting the JPEG decoder subsample does the reduction inside the decode, so no full-size raster and no rescale pass ever exist. 3.5x less memory and 5x faster than the strip-and-
getScaledInstancecode that took prod down, and ~7% of a 1.5 GB heap.112mis near the floor, since the 8192x40963BYTE_BGRoutput is 100 MB by itself.Also settled by measurement so it stops being re-litigated: ImageIO cannot stream a JPEG encode. Given a genuinely multi-tile
RenderedImage,JPEGImageWriterrequests zero tiles and callsgetData()for the whole raster. (A single-tile probe looks like streaming —getTile()=1— which is what makes this easy to get wrong.) It doesn't matter here: 112 MB doesn't need streaming.The cost is quality. Subsampling drops pixels where an area average would blend them — PSNR ~30.6 dB against a box-filtered reference. Accepted deliberately given how rarely this runs; @jonfroehlich signed off on the trade.
Guardrails
cpu-intensive, and the bound is the pool itself rather than a permit a thread blocks on — without a cap a burst puts several 105 MB allocations in a 1.5 GB heap at once, Nightly crop job OOM-kills prod JVMs: move the downscaled-pano derivative to the scraper, delete it from the app #5239's shape from the other direction. Four may queue behind them; past that a request is refused outright and served the native file. A device asking for a copy is one that already failed to texture the native frame, and it has its own ladder of smaller widths to walk, so a fast refusal sends it down that ladder while a long wait would only delay the same outcome.Noneand serves the native file — which is what the route did before this existed.Tests
7 new cases: the period arithmetic (including a non-2:1 pano where the height binds, and the 16384-on-an-8192-device case that needs no copy), the allowlist invariant, and a real subsampled read and JPEG write. 51 existing crop/
backupImagespecs still pass; ESLint clean.Docs
Corrects
PanoDataService,CropServiceanddocs/architecture.md, which all asserted a whole-pano derivative needs more heap than a city stage has. It doesn't — precomputing one for the whole store, nightly, was the problem.🤖 Generated with Claude Code — Opus 5 (1M context),
claude-opus-5[1m]