mirror of
https://github.com/anomalyco/opentui.git
synced 2026-09-19 01:26:03 +08:00
fix(core): pass transient FFI buffers directly (#1394)
## Summary - use `buffer` ABI parameters and pass typed-array owners directly for transient synchronous Bun FFI calls - lower portable `buffer` parameters to Node's stable `pointer` path while still passing owner objects directly - keep `ptr` only for nullable, mixed native-pointer, raw `ArrayBuffer`, callback, and retained-memory cases - stabilize retained text-memory views through `.buffer` before resolving their native address - preserve raw-pointer compatibility while allowing direct buffers in supersample, packed-buffer, matrix, and grayscale APIs - document the Bun 1.3.14 and Bun 1.4+ ownership rules in `AGENTS.md` ## Why A Bun `FastTypedArray` may store its data inline. Calling `ptr(view)` captures that address, but a later first access to `view.buffer` can move the data into separate `ArrayBuffer` storage and leave the native address stale. Passing the owner directly lets the FFI backend borrow the current storage for synchronous calls and allows Bun's `buffer` fast path to keep the owner visible to the JIT. This keeps nullable and true native-pointer parameters on `ptr`, where `buffer` cannot represent the ABI, while avoiding pre-resolved addresses for transient memory. Retained pointers explicitly materialize stable backing storage before `ptr(view)` and keep the view alive for the native lifetime. Node 26.4's Linux optimized `buffer` trampoline delivered a null pointer to a multi-argument audio call in CI. OpenTUI therefore maps its portable `buffer` descriptor to Node `pointer`, whose documented owner-borrowing path passes the same views correctly. Bun continues to receive the `buffer` descriptor. Related Bun investigation: oven-sh/bun#32054 and oven-sh/bun#32055. ## Testing - `bunx bun@1.3.14 test src/tests/ffi-borrowed-pointer-callsites.test.ts` (23 passed) - `bun test src/tests/ffi-borrowed-pointer-callsites.test.ts` on Bun 1.4 canary (23 passed) - `bun run test:js` (5,397 passed, 23 skipped) - `bun run test:js:node` with Node 26.4.0 (4,665 passed, 6 skipped) - `bun run test:dist` - `bun run build:lib` - `bun run fmt:check` - `bun run lint`
This commit is contained in:
Luke Parker
@@ -40,10 +40,16 @@ terminal or platform is unavailable locally.
|
||||
- Portable symbol signatures must stay within the `node:ffi`/`bun:ffi` intersection. Use explicit widths such as
|
||||
`u32`/`u64`, not backend-only ABI names such as `usize`, `napi_env`, or `napi_value`; represent `i64`/`u64` as `bigint`,
|
||||
native booleans as `0`/`1`, and shared pointers as `number | bigint`.
|
||||
- Pass transient `ArrayBuffer` values or views directly to synchronous pointer parameters so the backend borrows the
|
||||
owner. Do not pre-resolve them with `ptr()`.
|
||||
- Use `ptr(view)` only for addresses stored in structs or retained by native code, and keep the backing buffer alive for
|
||||
the complete native lifetime.
|
||||
- Default to `buffer` for transient, non-null `TypedArray` parameters and pass the view directly. Do not call `ptr()`.
|
||||
- Use `ptr` only when a parameter can be null, accepts a numeric native address, is a callback, or receives a raw
|
||||
`ArrayBuffer`. Pass transient owner objects directly to `ptr` parameters; do not pre-resolve them.
|
||||
- On Bun 1.3.14, `DataView` is not accepted by `buffer` or `ptr`. You MUST pass an equivalent typed array such as
|
||||
`new Uint8Array(view.buffer, view.byteOffset, view.byteLength)`.
|
||||
- On Bun 1.4+, a non-null `DataView` MUST use `buffer` and be passed directly.
|
||||
- Use `ptr(view)` only when native code stores the address beyond the call. Before resolving it, access `view.buffer` to
|
||||
move any inline typed-array storage into a stable `ArrayBuffer`, then keep the view alive for the complete native
|
||||
lifetime. The order is required: `const owner = view.buffer; const address = ptr(view)`. Calling `ptr(view)` first and
|
||||
accessing `view.buffer` later can move the storage and invalidate `address`.
|
||||
- Model C-string inputs as pointer parameters and pass owned, NUL-terminated byte buffers directly; string returns are
|
||||
not portable. Create callbacks through the loaded library/platform facade, not `new JSCallback(...)`, and assume only
|
||||
same-thread callbacks.
|
||||
|
||||
@@ -8,7 +8,6 @@ import { EditorView } from "../editor-view.js"
|
||||
import { BorderCharArrays } from "../lib/border.js"
|
||||
import { RGBA } from "../lib/RGBA.js"
|
||||
import { NativeImage } from "../image.js"
|
||||
import { ptr } from "../platform/ffi.js"
|
||||
import { TextBufferView } from "../text-buffer-view.js"
|
||||
import { TextBuffer } from "../text-buffer.js"
|
||||
import { createTestRenderer, type TestRenderer } from "../testing/test-renderer.js"
|
||||
@@ -635,7 +634,6 @@ function createSuperSampleScenario(
|
||||
pixels[index + 2] = (index >>> 8) & 0xff
|
||||
pixels[index + 3] = 0xff
|
||||
}
|
||||
const pixelsPtr = ptr(pixels)
|
||||
return {
|
||||
run: (operations) => {
|
||||
for (let index = 0; index < operations; index++) {
|
||||
@@ -643,7 +641,7 @@ function createSuperSampleScenario(
|
||||
buffer.ptr,
|
||||
0,
|
||||
0,
|
||||
pixelsPtr,
|
||||
pixels,
|
||||
pixels.byteLength,
|
||||
"rgba8unorm",
|
||||
alignedBytesPerRow,
|
||||
@@ -692,13 +690,12 @@ function createPackedBufferScenario(
|
||||
view.setUint32(offset + 32, 0x2588, true)
|
||||
}
|
||||
const packedBytes = new Uint8Array(packed)
|
||||
const packedPtr = ptr(packedBytes)
|
||||
return {
|
||||
run: (operations) => {
|
||||
for (let index = 0; index < operations; index++) {
|
||||
lib.bufferDrawPackedBuffer(
|
||||
buffer.ptr,
|
||||
packedPtr,
|
||||
packedBytes,
|
||||
packedBytes.byteLength,
|
||||
posX,
|
||||
posY,
|
||||
@@ -737,7 +734,6 @@ function createGrayscaleScenario(
|
||||
)
|
||||
const intensities = new Float32Array(sourceWidth * sourceHeight)
|
||||
for (let index = 0; index < intensities.length; index++) intensities[index] = (index % 17) / 16
|
||||
const intensitiesPtr = ptr(intensities)
|
||||
return {
|
||||
run: (operations) => {
|
||||
if (supersampled) {
|
||||
@@ -746,7 +742,7 @@ function createGrayscaleScenario(
|
||||
buffer.ptr,
|
||||
0,
|
||||
0,
|
||||
intensitiesPtr,
|
||||
intensities,
|
||||
sourceWidth,
|
||||
sourceHeight,
|
||||
COLORS.fg,
|
||||
@@ -759,7 +755,7 @@ function createGrayscaleScenario(
|
||||
buffer.ptr,
|
||||
0,
|
||||
0,
|
||||
intensitiesPtr,
|
||||
intensities,
|
||||
sourceWidth,
|
||||
sourceHeight,
|
||||
COLORS.fg,
|
||||
|
||||
@@ -1,6 +1,6 @@
|
||||
import { RGBA } from "./lib/index.js"
|
||||
import { resolveRenderLib, type OptimizedBufferHandle, type RenderLib } from "./zig.js"
|
||||
import { type Pointer, type PointerInput, toArrayBuffer, toPointer, ptr } from "./platform/ffi.js"
|
||||
import { type Pointer, type PointerInput, toArrayBuffer, toPointer } from "./platform/ffi.js"
|
||||
import type { NativeImage } from "./image.js"
|
||||
import type { ImageRenderProtocol } from "./types.js"
|
||||
|
||||
@@ -314,7 +314,7 @@ export class OptimizedBuffer {
|
||||
this.guard()
|
||||
if (matrix.length !== 16) throw new RangeError(`colorMatrix matrix must have length 16, got ${matrix.length}`)
|
||||
const cellMaskCount = Math.floor(cellMask.length / 3)
|
||||
this.lib.bufferColorMatrix(this.bufferPtr, ptr(matrix), ptr(cellMask), cellMaskCount, strength, target)
|
||||
this.lib.bufferColorMatrix(this.bufferPtr, matrix, cellMask, cellMaskCount, strength, target)
|
||||
}
|
||||
|
||||
public colorMatrixUniform(
|
||||
@@ -326,7 +326,7 @@ export class OptimizedBuffer {
|
||||
if (matrix.length !== 16)
|
||||
throw new RangeError(`colorMatrixUniform matrix must have length 16, got ${matrix.length}`)
|
||||
if (strength === 0.0) return
|
||||
this.lib.bufferColorMatrixUniform(this.bufferPtr, ptr(matrix), strength, target)
|
||||
this.lib.bufferColorMatrixUniform(this.bufferPtr, matrix, strength, target)
|
||||
}
|
||||
|
||||
public drawFrameBuffer(
|
||||
@@ -363,7 +363,7 @@ export class OptimizedBuffer {
|
||||
public drawSuperSampleBuffer(
|
||||
x: number,
|
||||
y: number,
|
||||
pixelDataPtr: PointerInput,
|
||||
pixelData: PointerInput | Uint8Array,
|
||||
pixelDataLength: number,
|
||||
format: "bgra8unorm" | "rgba8unorm",
|
||||
alignedBytesPerRow: number,
|
||||
@@ -373,7 +373,7 @@ export class OptimizedBuffer {
|
||||
this.bufferPtr,
|
||||
x,
|
||||
y,
|
||||
toPointer(pixelDataPtr),
|
||||
typeof pixelData === "number" || typeof pixelData === "bigint" ? toPointer(pixelData) : pixelData,
|
||||
pixelDataLength,
|
||||
format,
|
||||
alignedBytesPerRow,
|
||||
@@ -426,7 +426,7 @@ export class OptimizedBuffer {
|
||||
}
|
||||
|
||||
public drawPackedBuffer(
|
||||
dataPtr: PointerInput,
|
||||
data: PointerInput | Uint8Array,
|
||||
dataLen: number,
|
||||
posX: number,
|
||||
posY: number,
|
||||
@@ -436,7 +436,7 @@ export class OptimizedBuffer {
|
||||
this.guard()
|
||||
this.lib.bufferDrawPackedBuffer(
|
||||
this.bufferPtr,
|
||||
toPointer(dataPtr),
|
||||
typeof data === "number" || typeof data === "bigint" ? toPointer(data) : data,
|
||||
dataLen,
|
||||
posX,
|
||||
posY,
|
||||
@@ -455,7 +455,7 @@ export class OptimizedBuffer {
|
||||
bg: RGBA | null = null,
|
||||
): void {
|
||||
this.guard()
|
||||
this.lib.bufferDrawGrayscaleBuffer(this.bufferPtr, posX, posY, ptr(intensities), srcWidth, srcHeight, fg, bg)
|
||||
this.lib.bufferDrawGrayscaleBuffer(this.bufferPtr, posX, posY, intensities, srcWidth, srcHeight, fg, bg)
|
||||
}
|
||||
|
||||
public drawGrayscaleBufferSupersampled(
|
||||
@@ -468,16 +468,7 @@ export class OptimizedBuffer {
|
||||
bg: RGBA | null = null,
|
||||
): void {
|
||||
this.guard()
|
||||
this.lib.bufferDrawGrayscaleBufferSupersampled(
|
||||
this.bufferPtr,
|
||||
posX,
|
||||
posY,
|
||||
ptr(intensities),
|
||||
srcWidth,
|
||||
srcHeight,
|
||||
fg,
|
||||
bg,
|
||||
)
|
||||
this.lib.bufferDrawGrayscaleBufferSupersampled(this.bufferPtr, posX, posY, intensities, srcWidth, srcHeight, fg, bg)
|
||||
}
|
||||
|
||||
public resize(width: number, height: number): void {
|
||||
|
||||
@@ -371,7 +371,7 @@ describe("platform/ffi", () => {
|
||||
expect(symbolDefinitions).toEqual([
|
||||
{
|
||||
pointers: {
|
||||
arguments: ["pointer", "pointer", "pointer", "pointer", "buffer", "string"],
|
||||
arguments: ["pointer", "pointer", "pointer", "pointer", "pointer", "string"],
|
||||
return: "void",
|
||||
},
|
||||
},
|
||||
|
||||
@@ -756,7 +756,9 @@ function toNodeFFIType(type: FFITypeOrString, position: "parameter" | "result"):
|
||||
// Bun's N-API bridge types are not equivalent to raw Node FFI pointers.
|
||||
throw new Error(NODE_NAPI_UNSUPPORTED)
|
||||
case FFIType.buffer:
|
||||
return "buffer"
|
||||
// Node 26.4's Linux fast-buffer trampoline can pass a null pointer for
|
||||
// multi-argument signatures. The pointer path still borrows the owner.
|
||||
return "pointer"
|
||||
default:
|
||||
return unsupportedNodeFFIType(type)
|
||||
}
|
||||
|
||||
@@ -16,7 +16,8 @@ import {
|
||||
VisualCursorStruct,
|
||||
} from "../zig-structs.js"
|
||||
import { RGBA } from "../lib/RGBA.js"
|
||||
import { toArrayBuffer, type Pointer } from "../platform/ffi.js"
|
||||
import { ptr, toArrayBuffer, type Pointer } from "../platform/ffi.js"
|
||||
import { OptimizedBuffer } from "../buffer.js"
|
||||
|
||||
// Borrowed-pointer contract for styled text, styled placeholders, and cursor
|
||||
// options: packed struct buffers must reach the FFI symbol as object values so
|
||||
@@ -76,6 +77,92 @@ function readPackedColor(packed: ArrayBuffer, offset: number): number[] {
|
||||
}
|
||||
|
||||
describe("borrowed pointer call sites", () => {
|
||||
test("stabilizes retained text memory before resolving its pointer", () => {
|
||||
withStubbedSymbols(
|
||||
{
|
||||
textBufferRegisterMemBuffer: () => 1,
|
||||
textBufferReplaceMemBuffer: () => 1,
|
||||
textBufferAppend: () => undefined,
|
||||
},
|
||||
(calls) => {
|
||||
const registered = new Uint8Array([1, 2, 3])
|
||||
const replaced = new Uint8Array([4, 5, 6])
|
||||
const appended = new Uint8Array([7, 8, 9])
|
||||
|
||||
lib.textBufferRegisterMemBuffer(1 as any, registered)
|
||||
lib.textBufferReplaceMemBuffer(1 as any, 1, replaced)
|
||||
lib.textBufferAppend(1 as any, appended)
|
||||
|
||||
void registered.buffer
|
||||
void replaced.buffer
|
||||
void appended.buffer
|
||||
expect(calls.textBufferRegisterMemBuffer[0]![1]).toBe(ptr(registered))
|
||||
expect(calls.textBufferReplaceMemBuffer[0]![2]).toBe(ptr(replaced))
|
||||
expect(calls.textBufferAppend[0]![1]).toBe(ptr(appended))
|
||||
},
|
||||
)
|
||||
})
|
||||
|
||||
test("passes transient owners directly through remaining synchronous wrappers", () => {
|
||||
withStubbedSymbols(
|
||||
{
|
||||
bufferColorMatrix: () => undefined,
|
||||
bufferDrawGrayscaleBuffer: () => undefined,
|
||||
bufferDrawPackedBuffer: () => undefined,
|
||||
bufferDrawSuperSampleBuffer: () => undefined,
|
||||
bufferDrawText: () => undefined,
|
||||
bufferGetId: () => 0,
|
||||
getBuildOptions: () => undefined,
|
||||
editorViewGetViewport: () => undefined,
|
||||
audioGetPlaybackDeviceName: () => 0,
|
||||
streamDrainSpans: () => 0,
|
||||
},
|
||||
(calls) => {
|
||||
const fg = RGBA.fromValues(1, 1, 1, 1)
|
||||
const bg = RGBA.fromValues(0, 0, 0, 1)
|
||||
const matrix = new Float32Array(16)
|
||||
const mask = new Float32Array(3)
|
||||
const intensities = new Float32Array(4)
|
||||
const spans = new Uint8Array(64)
|
||||
const pixels = new Uint8Array(16)
|
||||
const packed = new Uint8Array(48)
|
||||
const buffer = new OptimizedBuffer(lib, 1 as any, 2, 2, {})
|
||||
|
||||
buffer.colorMatrix(matrix, mask)
|
||||
buffer.drawGrayscaleBuffer(0, 0, intensities, 2, 2, fg, bg)
|
||||
buffer.drawSuperSampleBuffer(0, 0, pixels, pixels.byteLength, "rgba8unorm", 8)
|
||||
buffer.drawPackedBuffer(packed, packed.byteLength, 0, 0, 1, 1)
|
||||
lib.bufferDrawText(1 as any, "x", 0, 0, fg, bg)
|
||||
lib.bufferGetId(1 as any)
|
||||
lib.getBuildOptions()
|
||||
lib.editorViewGetViewport(1 as any)
|
||||
lib.audioGetPlaybackDeviceName(1 as any, 0)
|
||||
lib.streamDrainSpans(1 as any, spans, 1)
|
||||
|
||||
expect(calls.bufferColorMatrix[0]![1]).toBe(matrix)
|
||||
expect(calls.bufferColorMatrix[0]![2]).toBe(mask)
|
||||
expect(calls.bufferDrawGrayscaleBuffer[0]![3]).toBe(intensities)
|
||||
expect(calls.bufferDrawGrayscaleBuffer[0]![6]).toBe(fg.buffer)
|
||||
expect(calls.bufferDrawGrayscaleBuffer[0]![7]).toBe(bg.buffer)
|
||||
expect(calls.bufferDrawSuperSampleBuffer[0]![3]).toBe(pixels)
|
||||
expect(calls.bufferDrawPackedBuffer[0]![1]).toBe(packed)
|
||||
expect(calls.bufferDrawText[0]![1]).toBeInstanceOf(Uint8Array)
|
||||
expect(calls.bufferDrawText[0]![5]).toBe(fg.buffer)
|
||||
expect(calls.bufferDrawText[0]![6]).toBe(bg.buffer)
|
||||
expect(calls.bufferGetId[0]![1]).toBeInstanceOf(Uint8Array)
|
||||
expect(calls.getBuildOptions[0]![0]).toBeInstanceOf(ArrayBuffer)
|
||||
expect(calls.editorViewGetViewport[0]!.slice(1)).toEqual([
|
||||
expect.any(Uint32Array),
|
||||
expect.any(Uint32Array),
|
||||
expect.any(Uint32Array),
|
||||
expect.any(Uint32Array),
|
||||
])
|
||||
expect(calls.audioGetPlaybackDeviceName[0]![2]).toBeInstanceOf(Uint8Array)
|
||||
expect(calls.streamDrainSpans[0]![1]).toBe(spans)
|
||||
},
|
||||
)
|
||||
})
|
||||
|
||||
test("reuses owned output storage while preserving public result identity", () => {
|
||||
const originals = {
|
||||
editBufferGetCursorPosition: symbols.editBufferGetCursorPosition,
|
||||
@@ -764,6 +851,29 @@ describe("borrowed pointer call sites", () => {
|
||||
}
|
||||
})
|
||||
|
||||
test("empty image inputs preserve nullable pointer semantics", () => {
|
||||
withStubbedSymbols(
|
||||
{
|
||||
imageInfo: () => 0,
|
||||
imageDecode: () => 0,
|
||||
imageCreateFromRgba: () => 0,
|
||||
imageCopyPixels: () => 0,
|
||||
},
|
||||
(calls) => {
|
||||
const empty = new Uint8Array()
|
||||
lib.imageInfo(empty)
|
||||
lib.imageDecode(empty)
|
||||
lib.imageCreateFromRgba(empty, 0, 0, 0)
|
||||
lib.imageCopyPixels(1 as any, empty, 0, false)
|
||||
|
||||
expect(calls.imageInfo[0]![0]).toBeNull()
|
||||
expect(calls.imageDecode[0]![0]).toBeNull()
|
||||
expect(calls.imageCreateFromRgba[0]![0]).toBeNull()
|
||||
expect(calls.imageCopyPixels[0]![1]).toBeNull()
|
||||
},
|
||||
)
|
||||
})
|
||||
|
||||
test("imageExtend rejects a short background before native access", () => {
|
||||
withStubbedSymbol("imageExtend", (calls) => {
|
||||
expect(lib.imageExtend(1 as any, 0, 0, 0, 0, Uint8Array.of(1, 2, 3))).toEqual({
|
||||
|
||||
+224
-211
File diff suppressed because it is too large
Load Diff
Reference in New Issue
Block a user