Skip to content

Commit 2158827

Browse files
anthonykim1anthonykim1
committed
remove non-null assertion, improve lifecycle for bitmap
1 parent b406797 commit 2158827

File tree

2 files changed

+141
-125
lines changed

2 files changed

+141
-125
lines changed

‎addons/addon-image/src/kitty/KittyGraphicsHandler.ts‎

Lines changed: 132 additions & 125 deletions
Original file line numberDiff line numberDiff line change
@@ -377,14 +377,15 @@ export class KittyGraphicsHandler implements IApcHandler, IResetHandler, IDispos
377377
if (cmd.id === undefined) {
378378
return true;
379379
}
380-
const image = this._kittyStorage.getImage(cmd.id);
380+
const id = cmd.id;
381+
const image = this._kittyStorage.getImage(id);
381382
if (!image) {
382-
this._sendResponse(cmd.id, 'ENOENT:image not found', cmd.quiet ?? 0, cmd.placementId);
383+
this._sendResponse(id, 'ENOENT:image not found', cmd.quiet ?? 0, cmd.placementId);
383384
return true;
384385
}
385386
const result = this._displayImage(image, cmd);
386387
return result.then(success => {
387-
this._sendResponse(cmd.id!, success ? 'OK' : 'EINVAL:image rendering failed', cmd.quiet ?? 0, cmd.placementId);
388+
this._sendResponse(id, success ? 'OK' : 'EINVAL:image rendering failed', cmd.quiet ?? 0, cmd.placementId);
388389
return true;
389390
});
390391
}
@@ -511,6 +512,10 @@ export class KittyGraphicsHandler implements IApcHandler, IResetHandler, IDispos
511512
break;
512513
case 'i':
513514
case 'I':
515+
// TODO: When placement id tracking is implemented (see TODO in
516+
// KittyImageStorage), d=i with p=<pid> should delete only that
517+
// specific placement, while d=i without p should delete all
518+
// placements for the image.
514519
if (cmd.id !== undefined) {
515520
const pending = this._pendingTransmissions.get(cmd.id);
516521
if (pending) {
@@ -546,135 +551,137 @@ export class KittyGraphicsHandler implements IApcHandler, IResetHandler, IDispos
546551
}
547552

548553
private async _decodeAndDisplay(image: IKittyImageData, cmd: IKittyCommand): Promise<void> {
549-
let bitmap = await this._createBitmap(image);
550-
551-
const cropX = Math.max(0, cmd.x ?? 0);
552-
const cropY = Math.max(0, cmd.y ?? 0);
553-
const cropW = cmd.sourceWidth || (bitmap.width - cropX);
554-
const cropH = cmd.sourceHeight || (bitmap.height - cropY);
555-
556-
const maxCropW = Math.max(0, bitmap.width - cropX);
557-
const maxCropH = Math.max(0, bitmap.height - cropY);
558-
const finalCropW = Math.max(0, Math.min(cropW, maxCropW));
559-
const finalCropH = Math.max(0, Math.min(cropH, maxCropH));
560-
561-
if (finalCropW === 0 || finalCropH === 0) {
562-
bitmap.close();
563-
throw new Error('invalid source rectangle');
564-
}
565-
566-
if (cropX !== 0 || cropY !== 0 || finalCropW !== bitmap.width || finalCropH !== bitmap.height) {
567-
const cropped = await createImageBitmap(bitmap, cropX, cropY, finalCropW, finalCropH);
568-
bitmap.close();
569-
bitmap = cropped;
570-
}
571-
572-
const cw = this._renderer.dimensions?.css.cell.width || CELL_SIZE_DEFAULT.width;
573-
const ch = this._renderer.dimensions?.css.cell.height || CELL_SIZE_DEFAULT.height;
574-
575-
// Per spec: c/r default to image's natural cell dimensions.
576-
// If only one of c/r is specified, compute the other from image aspect ratio.
577-
let imgCols: number;
578-
let imgRows: number;
579-
if (cmd.columns !== undefined && cmd.rows !== undefined) {
580-
imgCols = cmd.columns;
581-
imgRows = cmd.rows;
582-
} else if (cmd.columns !== undefined) {
583-
imgCols = cmd.columns;
584-
imgRows = Math.max(1, Math.ceil((bitmap.height / bitmap.width) * (imgCols * cw) / ch));
585-
} else if (cmd.rows !== undefined) {
586-
imgRows = cmd.rows;
587-
imgCols = Math.max(1, Math.ceil((bitmap.width / bitmap.height) * (imgRows * ch) / cw));
588-
} else {
589-
imgCols = Math.ceil(bitmap.width / cw);
590-
imgRows = Math.ceil(bitmap.height / ch);
591-
}
592-
593-
let w = bitmap.width;
594-
let h = bitmap.height;
595-
596-
// Scale bitmap to fit placement rectangle when c/r are specified
597-
if (cmd.columns !== undefined || cmd.rows !== undefined) {
598-
w = Math.round(imgCols * cw);
599-
h = Math.round(imgRows * ch);
600-
}
601-
602-
if (w * h > this._opts.pixelLimit) {
603-
bitmap.close();
604-
throw new Error('image exceeds pixel limit');
605-
}
606-
607-
// Save cursor position before addImage modifies it
608-
const buffer = this._coreTerminal._core.buffer;
609-
const savedX = buffer.x;
610-
const savedY = buffer.y;
611-
const savedYbase = buffer.ybase;
612-
613-
// Determine layer based on z-index: negative = behind text, 0+ = on top.
614-
// When z<0 we always use the bottom layer even without allowTransparency —
615-
// the image will simply be hidden behind the opaque text background, which
616-
// is the correct behavior (client asked for "behind text").
617-
const wantsBottom = cmd.zIndex !== undefined && cmd.zIndex < 0;
618-
const layer: ImageLayer = wantsBottom ? 'bottom' : 'top';
619-
620-
let finalBitmap = bitmap;
621-
if (w !== bitmap.width || h !== bitmap.height) {
622-
finalBitmap = await createImageBitmap(bitmap, { resizeWidth: w, resizeHeight: h });
623-
bitmap.close();
624-
}
625-
626-
// Per spec: X/Y are pixel offsets within the first cell, so clamp to cell dimensions
627-
const xOffset = Math.min(Math.max(0, cmd.xOffset ?? 0), cw - 1);
628-
const yOffset = Math.min(Math.max(0, cmd.yOffset ?? 0), ch - 1);
629-
if (xOffset !== 0 || yOffset !== 0) {
630-
// Per spec: X/Y is not added to c/r area. When c/r are explicit, the
631-
// total placement area remains c*cw × r*ch pixels and the offset image
632-
// is clipped to fit. When c/r are unset, the padded canvas determines
633-
// the natural cell dimensions.
634-
const canvasW = (cmd.columns !== undefined) ? Math.round(imgCols * cw) : finalBitmap.width + xOffset;
635-
const canvasH = (cmd.rows !== undefined) ? Math.round(imgRows * ch) : finalBitmap.height + yOffset;
636-
const offsetCanvas = ImageRenderer.createCanvas(window.document, canvasW, canvasH);
637-
const offsetCtx = offsetCanvas.getContext('2d');
638-
if (!offsetCtx) {
639-
finalBitmap.close();
640-
throw new Error('Failed to create offset canvas context');
554+
let bitmap: ImageBitmap | undefined = await this._createBitmap(image);
555+
556+
try {
557+
const cropX = Math.max(0, cmd.x ?? 0);
558+
const cropY = Math.max(0, cmd.y ?? 0);
559+
const cropW = cmd.sourceWidth || (bitmap.width - cropX);
560+
const cropH = cmd.sourceHeight || (bitmap.height - cropY);
561+
562+
const maxCropW = Math.max(0, bitmap.width - cropX);
563+
const maxCropH = Math.max(0, bitmap.height - cropY);
564+
const finalCropW = Math.max(0, Math.min(cropW, maxCropW));
565+
const finalCropH = Math.max(0, Math.min(cropH, maxCropH));
566+
567+
if (finalCropW === 0 || finalCropH === 0) {
568+
throw new Error('invalid source rectangle');
569+
}
570+
571+
if (cropX !== 0 || cropY !== 0 || finalCropW !== bitmap.width || finalCropH !== bitmap.height) {
572+
const cropped = await createImageBitmap(bitmap, cropX, cropY, finalCropW, finalCropH);
573+
bitmap.close();
574+
bitmap = cropped;
575+
}
576+
577+
const cw = this._renderer.dimensions?.css.cell.width || CELL_SIZE_DEFAULT.width;
578+
const ch = this._renderer.dimensions?.css.cell.height || CELL_SIZE_DEFAULT.height;
579+
580+
// Per spec: c/r default to image's natural cell dimensions.
581+
// If only one of c/r is specified, compute the other from image aspect ratio.
582+
let imgCols: number;
583+
let imgRows: number;
584+
if (cmd.columns !== undefined && cmd.rows !== undefined) {
585+
imgCols = cmd.columns;
586+
imgRows = cmd.rows;
587+
} else if (cmd.columns !== undefined) {
588+
imgCols = cmd.columns;
589+
imgRows = Math.max(1, Math.ceil((bitmap.height / bitmap.width) * (imgCols * cw) / ch));
590+
} else if (cmd.rows !== undefined) {
591+
imgRows = cmd.rows;
592+
imgCols = Math.max(1, Math.ceil((bitmap.width / bitmap.height) * (imgRows * ch) / cw));
593+
} else {
594+
imgCols = Math.ceil(bitmap.width / cw);
595+
imgRows = Math.ceil(bitmap.height / ch);
596+
}
597+
598+
let w = bitmap.width;
599+
let h = bitmap.height;
600+
601+
// Scale bitmap to fit placement rectangle when c/r are specified
602+
if (cmd.columns !== undefined || cmd.rows !== undefined) {
603+
w = Math.round(imgCols * cw);
604+
h = Math.round(imgRows * ch);
641605
}
642-
offsetCtx.drawImage(finalBitmap, xOffset, yOffset);
643-
644-
const offsetBitmap = await createImageBitmap(offsetCanvas);
645-
offsetCanvas.width = offsetCanvas.height = 0;
646-
finalBitmap.close();
647-
finalBitmap = offsetBitmap;
648-
w = finalBitmap.width;
649-
h = finalBitmap.height;
606+
650607
if (w * h > this._opts.pixelLimit) {
651-
finalBitmap.close();
652608
throw new Error('image exceeds pixel limit');
653609
}
654-
if (cmd.columns === undefined) {
655-
imgCols = Math.ceil(finalBitmap.width / cw);
656-
}
657-
if (cmd.rows === undefined) {
658-
imgRows = Math.ceil(finalBitmap.height / ch);
610+
611+
// Save cursor position before addImage modifies it
612+
const buffer = this._coreTerminal._core.buffer;
613+
const savedX = buffer.x;
614+
const savedY = buffer.y;
615+
const savedYbase = buffer.ybase;
616+
617+
// Determine layer based on z-index: negative = behind text, 0+ = on top.
618+
// When z<0 we always use the bottom layer even without allowTransparency —
619+
// the image will simply be hidden behind the opaque text background, which
620+
// is the correct behavior (client asked for "behind text").
621+
const wantsBottom = cmd.zIndex !== undefined && cmd.zIndex < 0;
622+
const layer: ImageLayer = wantsBottom ? 'bottom' : 'top';
623+
624+
if (w !== bitmap.width || h !== bitmap.height) {
625+
const scaled = await createImageBitmap(bitmap, { resizeWidth: w, resizeHeight: h });
626+
bitmap.close();
627+
bitmap = scaled;
659628
}
660-
}
661629

662-
const zIndex = cmd.zIndex ?? 0;
663-
this._kittyStorage.addImage(image.id, finalBitmap, true, layer, zIndex);
630+
// Per spec: X/Y are pixel offsets within the first cell, so clamp to cell dimensions
631+
const xOffset = Math.min(Math.max(0, cmd.xOffset ?? 0), cw - 1);
632+
const yOffset = Math.min(Math.max(0, cmd.yOffset ?? 0), ch - 1);
633+
if (xOffset !== 0 || yOffset !== 0) {
634+
// Per spec: X/Y is not added to c/r area. When c/r are explicit, the
635+
// total placement area remains c*cw × r*ch pixels and the offset image
636+
// is clipped to fit. When c/r are unset, the padded canvas determines
637+
// the natural cell dimensions.
638+
const canvasW = (cmd.columns !== undefined) ? Math.round(imgCols * cw) : bitmap.width + xOffset;
639+
const canvasH = (cmd.rows !== undefined) ? Math.round(imgRows * ch) : bitmap.height + yOffset;
640+
const offsetCanvas = ImageRenderer.createCanvas(window.document, canvasW, canvasH);
641+
const offsetCtx = offsetCanvas.getContext('2d');
642+
if (!offsetCtx) {
643+
throw new Error('Failed to create offset canvas context');
644+
}
645+
offsetCtx.drawImage(bitmap, xOffset, yOffset);
646+
647+
const offsetBitmap = await createImageBitmap(offsetCanvas);
648+
offsetCanvas.width = offsetCanvas.height = 0;
649+
bitmap.close();
650+
bitmap = offsetBitmap;
651+
w = bitmap.width;
652+
h = bitmap.height;
653+
if (w * h > this._opts.pixelLimit) {
654+
throw new Error('image exceeds pixel limit');
655+
}
656+
if (cmd.columns === undefined) {
657+
imgCols = Math.ceil(bitmap.width / cw);
658+
}
659+
if (cmd.rows === undefined) {
660+
imgRows = Math.ceil(bitmap.height / ch);
661+
}
662+
}
664663

665-
// Kitty cursor movement
666-
// Per spec: cursor placed at first column after last image column,
667-
// on the last row of the image. C=1 means don't move cursor.
668-
if (cmd.cursorMovement === 1) {
669-
// C=1: restore cursor to position before image was placed
670-
const scrolled = buffer.ybase - savedYbase;
671-
buffer.x = savedX;
672-
// Can't restore cursor to scrollback?
673-
buffer.y = Math.max(savedY - scrolled, 0);
674-
} else {
675-
// Default (C=0): advance cursor horizontally past the image
676-
// addImage already positioned cursor on the last row via lineFeeds
677-
buffer.x = Math.min(savedX + imgCols, this._coreTerminal.cols);
664+
const zIndex = cmd.zIndex ?? 0;
665+
this._kittyStorage.addImage(image.id, bitmap, true, layer, zIndex);
666+
bitmap = undefined; // ownership transferred to storage
667+
668+
// Kitty cursor movement
669+
// Per spec: cursor placed at first column after last image column,
670+
// on the last row of the image. C=1 means don't move cursor.
671+
if (cmd.cursorMovement === 1) {
672+
// C=1: restore cursor to position before image was placed
673+
const scrolled = buffer.ybase - savedYbase;
674+
buffer.x = savedX;
675+
// Can't restore cursor to scrollback?
676+
buffer.y = Math.max(savedY - scrolled, 0);
677+
} else {
678+
// Default (C=0): advance cursor horizontally past the image
679+
// addImage already positioned cursor on the last row via lineFeeds
680+
buffer.x = Math.min(savedX + imgCols, this._coreTerminal.cols);
681+
}
682+
} catch (e) {
683+
bitmap?.close();
684+
throw e;
678685
}
679686
}
680687

‎addons/addon-image/src/kitty/KittyImageStorage.ts‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,15 @@ export class KittyImageStorage implements IDisposable {
2020

2121
private _nextImageId = 1;
2222
private readonly _images: Map<number, IKittyImageData> = new Map();
23+
// TODO: Support multiple placements per image. The kitty spec identifies
24+
// placements by an (image id, placement id) pair — same i + different p
25+
// values should coexist, and same i + same p should replace the prior
26+
// placement. Currently we track only one storage entry per kitty image id,
27+
// so multiple placements of the same image overwrite each other. Fixing
28+
// this requires changing these maps to Map<number, Map<number, number>>
29+
// (kittyId → placementId → storageId) and updating addImage/deleteById
30+
// accordingly. The underlying shared ImageStorage would also need to
31+
// support multiple entries per logical image.
2332
private readonly _kittyIdToStorageId: Map<number, number> = new Map();
2433
private readonly _storageIdToKittyId: Map<number, number> = new Map();
2534

0 commit comments

Comments
 (0)