fix(gallery): release the canvas decode when it is drawn, not at unmount
Review follow-up. The release only ran from the effect cleanup, so it fired on unmount or a src change — while the commit message and the test header both explained that grid tiles never unmount, which is the whole reason the decode piles up. For the case the change exists for, it never ran at all. drawToCanvas now reports whether it drew, and the source Image is released as soon as the pixels are on the canvas. Nothing redraws from imageRef afterwards; drawToCanvas has exactly one caller. The cleanup stays as the fallback for the paths onload cannot cover: the draw failed, or the source changed before onload fired. The new test pins release while still mounted, on the same src. It fails against the previous version. Refs #1287
This commit is contained in:
@@ -98,14 +98,16 @@ export const AuthenticatedImage: React.FC<AuthenticatedImageProps> = ({
|
|||||||
const imageRef = useRef<HTMLImageElement | null>(null);
|
const imageRef = useRef<HTMLImageElement | null>(null);
|
||||||
|
|
||||||
// Draw image to canvas when canvas rendering is enabled
|
// Draw image to canvas when canvas rendering is enabled
|
||||||
|
// Returns whether the pixels actually made it onto the canvas, so the
|
||||||
|
// caller knows if the source image is still needed (#1287).
|
||||||
const drawToCanvas = useCallback(() => {
|
const drawToCanvas = useCallback(() => {
|
||||||
if (!useCanvasRendering || !canvasRef.current || !imageRef.current) return;
|
if (!useCanvasRendering || !canvasRef.current || !imageRef.current) return false;
|
||||||
|
|
||||||
const canvas = canvasRef.current;
|
const canvas = canvasRef.current;
|
||||||
const img = imageRef.current;
|
const img = imageRef.current;
|
||||||
const ctx = canvas.getContext('2d');
|
const ctx = canvas.getContext('2d');
|
||||||
|
|
||||||
if (!ctx || !img.complete || img.naturalWidth === 0) return;
|
if (!ctx || !img.complete || img.naturalWidth === 0) return false;
|
||||||
|
|
||||||
// Set canvas dimensions to match image
|
// Set canvas dimensions to match image
|
||||||
canvas.width = img.naturalWidth;
|
canvas.width = img.naturalWidth;
|
||||||
@@ -115,6 +117,7 @@ export const AuthenticatedImage: React.FC<AuthenticatedImageProps> = ({
|
|||||||
ctx.drawImage(img, 0, 0);
|
ctx.drawImage(img, 0, 0);
|
||||||
|
|
||||||
setCanvasReady(true);
|
setCanvasReady(true);
|
||||||
|
return true;
|
||||||
}, [useCanvasRendering]);
|
}, [useCanvasRendering]);
|
||||||
|
|
||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
@@ -271,7 +274,18 @@ export const AuthenticatedImage: React.FC<AuthenticatedImageProps> = ({
|
|||||||
|
|
||||||
img.onload = () => {
|
img.onload = () => {
|
||||||
imageRef.current = img;
|
imageRef.current = img;
|
||||||
drawToCanvas();
|
const drawn = drawToCanvas();
|
||||||
|
// Once drawImage has copied the pixels into the canvas the source
|
||||||
|
// decode is dead weight, so drop it here rather than at unmount. The
|
||||||
|
// grid is not virtualised — a 546-photo event mounts 546 of these and
|
||||||
|
// none of them unmount while the gallery is open — so a cleanup-only
|
||||||
|
// release never actually runs for the case it was meant to fix
|
||||||
|
// (#1287). Nothing redraws from `imageRef` afterwards: drawToCanvas
|
||||||
|
// has this one caller.
|
||||||
|
if (drawn) {
|
||||||
|
imageRef.current = null;
|
||||||
|
img.removeAttribute('src');
|
||||||
|
}
|
||||||
onLoad?.();
|
onLoad?.();
|
||||||
};
|
};
|
||||||
|
|
||||||
@@ -286,16 +300,13 @@ export const AuthenticatedImage: React.FC<AuthenticatedImageProps> = ({
|
|||||||
return () => {
|
return () => {
|
||||||
img.onload = null;
|
img.onload = null;
|
||||||
img.onerror = null;
|
img.onerror = null;
|
||||||
// Release the decoded bitmap (#1287). `imageRef` is what drawToCanvas
|
// Fallback release for the paths the onload handler above cannot
|
||||||
// reads, and it was never cleared — so a detached Image, and the
|
// cover: the draw failed, or the source changed / the component
|
||||||
// decode behind it, stayed pinned by a live JS reference for the
|
// unmounted before onload ever fired. `imageRef` is what drawToCanvas
|
||||||
// lifetime of the component. A decoded <img> in the document is
|
// reads and it was never cleared, so a detached Image — and the decode
|
||||||
// evictable under memory pressure; one held by a ref is not.
|
// behind it — stayed pinned by a live JS reference. A decoded <img> in
|
||||||
//
|
// the document is evictable under memory pressure; one held by a ref
|
||||||
// This matters at gallery scale because the grid is not virtualised:
|
// is not.
|
||||||
// a 546-photo event mounts 546 of these and none of them ever unmount,
|
|
||||||
// so nothing was ever released. Dropping the src first lets the
|
|
||||||
// browser reclaim the decode without waiting for GC to notice.
|
|
||||||
if (imageRef.current === img) {
|
if (imageRef.current === img) {
|
||||||
imageRef.current = null;
|
imageRef.current = null;
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -50,6 +50,12 @@ beforeEach(() => {
|
|||||||
constructor() {
|
constructor() {
|
||||||
super();
|
super();
|
||||||
created.push(this as unknown as HTMLImageElement);
|
created.push(this as unknown as HTMLImageElement);
|
||||||
|
// jsdom leaves these at 0/false for a blob: src, which makes
|
||||||
|
// drawToCanvas bail before it draws. Present a decoded image so the
|
||||||
|
// draw path is reachable.
|
||||||
|
Object.defineProperty(this, 'complete', { get: () => true });
|
||||||
|
Object.defineProperty(this, 'naturalWidth', { get: () => 10 });
|
||||||
|
Object.defineProperty(this, 'naturalHeight', { get: () => 10 });
|
||||||
// jsdom never fires load for a blob: src, so drive it manually.
|
// jsdom never fires load for a blob: src, so drive it manually.
|
||||||
setTimeout(() => this.onload?.(new Event('load')), 0);
|
setTimeout(() => this.onload?.(new Event('load')), 0);
|
||||||
}
|
}
|
||||||
@@ -59,6 +65,31 @@ beforeEach(() => {
|
|||||||
afterEach(() => vi.unstubAllGlobals());
|
afterEach(() => vi.unstubAllGlobals());
|
||||||
|
|
||||||
describe('AuthenticatedImage canvas mode', () => {
|
describe('AuthenticatedImage canvas mode', () => {
|
||||||
|
it('releases the decoded image as soon as it is drawn, without waiting for unmount', async () => {
|
||||||
|
// The case this whole change exists for. Every other test here asserts
|
||||||
|
// release on unmount or src change — neither of which happens to a grid
|
||||||
|
// tile, because the grid is not virtualised and the tiles stay mounted
|
||||||
|
// for as long as the gallery is open. Once drawImage has copied the
|
||||||
|
// pixels the source decode is dead weight and must go immediately.
|
||||||
|
const ctx = { drawImage: vi.fn() };
|
||||||
|
const getContext = vi
|
||||||
|
.spyOn(HTMLCanvasElement.prototype, 'getContext')
|
||||||
|
.mockReturnValue(ctx as unknown as CanvasRenderingContext2D);
|
||||||
|
|
||||||
|
render(
|
||||||
|
<AuthenticatedImage src="/api/gallery/demo/thumbnail/1" alt="t" useCanvasRendering />
|
||||||
|
);
|
||||||
|
|
||||||
|
await waitFor(() => expect(created.length).toBeGreaterThan(0));
|
||||||
|
const img = created[0];
|
||||||
|
|
||||||
|
await waitFor(() => expect(ctx.drawImage).toHaveBeenCalled());
|
||||||
|
// Still mounted, still the same src — and already released.
|
||||||
|
await waitFor(() => expect(img.getAttribute('src')).toBeNull());
|
||||||
|
|
||||||
|
getContext.mockRestore();
|
||||||
|
});
|
||||||
|
|
||||||
it('releases the decoded image on unmount', async () => {
|
it('releases the decoded image on unmount', async () => {
|
||||||
const { unmount } = render(
|
const { unmount } = render(
|
||||||
<AuthenticatedImage src="/api/gallery/demo/thumbnail/1" alt="t" useCanvasRendering />
|
<AuthenticatedImage src="/api/gallery/demo/thumbnail/1" alt="t" useCanvasRendering />
|
||||||
|
|||||||
Reference in New Issue
Block a user