refactor(inference): reuse the displayed asset for browser pixel preparation - #874
Merged
Merged
Conversation
…y browser pixel source AssetImage retains the exact fetched Blob behind its object URL (one content request, one Blob, one URL; abort+revoke on replacement or unmount) while still exposing only src to the existing render-prop consumers. AnnotatorCanvas exposes an optional onImageReady callback carrying a generation-aware decoded-image lease over the real rendered annotator-image element. The lease's readRgb performs lazy, descriptor-frame RGBA-to-RGB conversion and refuses once its generation or src is stale, so a retained source can never read a replacement asset through a reused DOM node. No annotator/browser-inference dependency, model change, or exposed inference target is introduced; the headless annotator-core boundary is untouched.
`docs/content/architecture/frontend/ui-core.md`'s durable rule now states laziness, no-second-Image, and no-exposed-inference-capability explicitly, and draws the line on what is proved versus claimed: no universal browser/Pillow byte parity, only what a real browser measurably does. `e2e/assetPixels.spec.ts` and its test-only harness (`app/src/demo/AssetPixelsFixture.tsx`, routed at `/asset-pixels-fixture`, ungated like `/demo` and `/styleguide`) are the real-browser proof: a tiny RGBA image encoded to a PNG at runtime, no binary fixture, showing one content request, the same decoded `<img>` reused as the pixel source, and the exact descriptor-frame RGB a real 2D context produced. `decodedAssetImage.test.ts`'s existing pixel-math test is strengthened to assert the exact image reference and exact `getImageData` args, closing the one gap jsdom's canvas leaves for a unit test to catch on its own.
tsconfig.e2e.json's project does not include src/demo, so the ambient Window augmentation declared in AssetPixelsFixture.tsx was invisible to e2e/assetPixels.spec.ts's own typecheck, failing frontend lint. Use the same window-as-unknown-cast idiom _bench.ts already uses for this exact cross-project situation instead of relying on a global declared outside the e2e program.
…ease on unmount
The package's real public barrel (src/index.ts) re-exported the React
adapter through an explicit named list that omitted the two lease types,
so `import type { DecodedAssetImage } from "@visionset/annotator"` never
compiled even though the adapter module itself exported them. Add both
to the barrel's existing named list.
A real host switches assets by unmounting the old AnnotatorCanvas and
mounting a fresh one, not by changing imageSrc on a live instance, so the
generation bump keyed on imageSrc never fires on that path. A lease
handed out just before teardown kept its captured generation equal to
the (unmounted) instance's counter and its detached <img> still carried
the old src, so readRgb's guard could pass and hand back the previous
asset's pixels. Bump the generation ref in an unmount-only effect
cleanup so every outstanding lease from that instance genuinely refuses
afterward, through the same guard readRgb already checks.
Covering tests in decodedImageLease.test.tsx: an unmount() + readRgb
refusal test for the invalidation path, and a compile-time check (typed
via the real @visionset/annotator import, not a relative path) proving
DecodedAssetImage/RgbPixels actually surface publicly. Verified by
temporarily reverting the export and confirming ui-core's typecheck
fails with TS2459 before restoring it.
Updates the architecture doc's lease-refusal sentence to cover both
the imageSrc-replacement and the unmount path.
The Chromium proof for the browser pixel-reuse seam had its own standalone route, asset-pixels-fixture, which is a test-only production surface with no product link. Move it behind the existing /demo route's scene query parameter, the same pattern the annotator benchmark already uses, so no dedicated route exists purely to host a Playwright harness.
…ImageReady The final review for this seam found that React always invokes the most recently committed onLoad handler, never the one attached when a particular load event was queued, so the existing generation/src guard cannot by itself refuse a delayed load delivered after imageSrc changes twice on a live, non-remounted instance. No host exercises that path today: every real asset switch unmounts this component, which the separate unmount effect already covers, and browsers do not dispatch load/error for an abandoned image request in the first place. Record the residual in the two places a future consumer would read them, rather than carry an unverifiable code change for a race nothing here can exercise or test.
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
1 participant
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.
Summary
BlobbehindAssetImage's single object URL, so the visible<img>stays the one and only decode of a displayed asset.DecodedAssetImageseam (AnnotatorCanvas's newonImageReady) that hands back the exact rendered<img>once loaded, with lazy, on-demand RGB extraction (readRgb) drawn at the descriptor's frame — never the natural one — and never invoked during ordinary rendering.imageSrcchanges on a live instance and when the component unmounts (the path every real host actually takes on an asset switch).e2e/assetPixels.spec.ts) against a runtime-encoded PNG, behind/demo?scene=asset-pixels— the same test-only-route pattern the existing benchmark already uses, so no dedicated production route exists purely to host a Playwright harness.@visionset/annotatorand@visionset/browser-inferencein either direction; no model, geometry, or inference capability is introduced by this phase.Test plan
pnpm --filter @visionset/annotator test— 1960/1960 passingpnpm --filter @visionset/annotator lintpnpm --filter @visionset/annotator buildpnpm --filter @visionset/ui-core test— 1393/1393 passingpnpm --filter @visionset/ui-core lintpnpm --filter @visionset/ui-core buildbash scripts/verify_npm_packages.shpnpm test:scripts— 153/153 passingbash scripts/check.sh— all groups (python, frontend, generated, browser) passedbash scripts/check.sh docspnpm exec playwright test e2e/assetPixels.spec.ts— 1 passed, real Chromium, real dev serverKnown limitation for a future phase
AnnotatorCanvas'sonImageReadyguard cannot, by itself, distinguish a delayedloadevent from a fully live-instanceimageSrcchange from a legitimate one, because React always invokes the most recently committed handler regardless of when the underlying event was queued. No host exercises this today — every real asset switch unmounts the component (covered separately) — and browsers do not dispatchload/errorfor an abandoned image request in the first place. Documented inAnnotatorCanvas.tsxbeside the guard and theonImageReadyprop for whichever future phase adds the first live-instance consumer.