From 3ae73a7c5e740cd5eae9496576c07cedb9fa049e Mon Sep 17 00:00:00 2001 From: Vanilagy <1696106+Vanilagy@users.noreply.github.com> Date: Sun, 10 May 2026 17:31:56 +0200 Subject: [PATCH] Loosen constraint on format returned by toRgbSample, fix X->A conversio, fix tests --- src/sample.ts | 93 ++++++++++++++++---- test/browser/custom-sample-resources.test.ts | 66 +++++++------- test/browser/video-samples.test.ts | 46 ++++++++++ 3 files changed, 152 insertions(+), 53 deletions(-) diff --git a/src/sample.ts b/src/sample.ts index d8bd447..9824d7e 100644 --- a/src/sample.ts +++ b/src/sample.ts @@ -693,7 +693,7 @@ export class VideoSample implements Disposable { } /** - * Returns the number of bytes required to hold this video sample's pixel data. Throws if `format` is `null`. + * Returns the number of bytes required to hold this video sample's pixel data. */ allocationSize(options: VideoFrameCopyToOptions = {}): number { validateVideoFrameCopyToOptions(options); @@ -718,7 +718,7 @@ export class VideoSample implements Disposable { } /** - * Copies this video sample's pixel data to an ArrayBuffer or ArrayBufferView. Throws if `format` is `null`. + * Copies this video sample's pixel data to an ArrayBuffer or ArrayBufferView. * @returns The byte layout of the planes of the copied data. */ async copyTo(destination: AllowSharedBufferSource, options: VideoFrameCopyToOptions = {}): Promise { @@ -731,7 +731,7 @@ export class VideoSample implements Disposable { throw new Error('VideoSample is closed.'); } if ((options.format ?? this.format) == null) { - throw new Error('Cannot copy video sample data when format is null. Sorry!'); + throw new Error('Cannot copy video sample data when format is null.'); } assert(this._data !== null); @@ -740,9 +740,10 @@ export class VideoSample implements Disposable { return this._data.copyTo(destination, options); } + // Detect non-RGB to RGB conversion if ( options.format - && this.format !== options.format + && !['RGBA', 'RGBX', 'BGRA', 'BGRX'].includes(this.format!) && ['RGBA', 'RGBX', 'BGRA', 'BGRX'].includes(options.format) ) { // RGB conversion for custom VideoSampleResource @@ -759,15 +760,16 @@ export class VideoSample implements Disposable { if (!(rgbSample instanceof VideoSample)) { throw new TypeError('toRgbSample() must return a VideoSample.'); } - if (rgbSample.format !== options.format) { + if (!['RGBA', 'RGBX', 'BGRA', 'BGRX'].includes(rgbSample.format!)) { throw new Error( - `Sample returned by toRgbSample was expected to have format '${options.format}', got` + `Sample returned by toRgbSample was expected to have an RGB format, got` + ` '${rgbSample.format}' instead.`, ); } + // Note that we DON'T force the RGB format to be exactly what was requested; any RGB format will do return await rgbSample.copyTo(destination, options); // 'await' is intentional here cuz of using - } else if (!['RGBA', 'RGBX', 'BGRA', 'BGRX'].includes(this.format!)) { + } else { if (typeof VideoFrame === 'undefined') { throw new Error( 'For this sample, converting from a non-RGB to an RGB format requires VideoFrame to' @@ -841,6 +843,8 @@ export class VideoSample implements Disposable { }]; } + // Algo taken from WebCodecs spec: + // 6. Let p be a new Promise. (Implicit) // 7. Let copyStepsQueue be the result of starting a new parallel queue. (Implicit) // 8. Let planeLayouts be a new list. @@ -908,16 +912,27 @@ export class VideoSample implements Disposable { planeLayouts.push(layout); } - // RGB conversion for ArrayBuffer and canvas-backed samples - if (options.format !== undefined && !(this._data instanceof VideoSampleResource)) { + // Now, handle converting between different RGB formats + if (options.format !== undefined) { const needsRgbConversion = this.format.startsWith('RGB') !== options.format.startsWith('RGB'); - if (needsRgbConversion) { - // Loop over the destination bytes, swapping R and B + + // Going X->A requires setting the alpha to 255, going the other way doesn't since the value of X is w/e + const needsAlphaConversion = this.format.includes('X') && options.format.includes('A'); + + if (needsRgbConversion || needsAlphaConversion) { + // Loop over the destination bytes for (let i = 0; i < combinedLayout.allocationSize; i += 4) { - const r = destBytes[i]!; - const b = destBytes[i + 2]!; - destBytes[i] = b; - destBytes[i + 2] = r; + if (needsRgbConversion) { + // Swap R with B + const r = destBytes[i]!; + const b = destBytes[i + 2]!; + destBytes[i] = b; + destBytes[i + 2] = r; + } + + if (needsAlphaConversion) { + destBytes[i + 3] = 255; + } } } } @@ -938,10 +953,48 @@ export class VideoSample implements Disposable { assert(this._data !== null); if (this._data instanceof VideoSampleResource) { - throw new Error( - 'Creating a VideoFrame from a VideoSampleResource is currently not supported. To work around this,' - + ' copy the data into a buffer and then create a VideoFrame from it.', - ); + if (this.format === null) { + throw new Error( + 'Cannot convert a VideoSampleResource-backed VideoSample to VideoFrame if format is null.', + ); + } + + const planes = this._data.getDataPlanes(); + if (planes instanceof Promise) { + throw new Error( + 'Cannot convert a VideoSampleResource-backed VideoSample to VideoFrame if getDataPlanes() returns' + + ' a promise.', + ); + } + + // We can't use allocationSize since that method assumes a tight packing + const size = planes.reduce((a, b) => a + b.data.byteLength, 0); + const buffer = new Uint8Array(size); + + let offset = 0; + const offsets: number[] = []; + + for (const plane of planes) { + buffer.set(plane.data, offset); + offsets.push(offset); + + offset += plane.data.byteLength; + } + + return new VideoFrame(buffer, { + format: this.format as VideoPixelFormat, + layout: planes.map((x, i) => ({ + offset: offsets[i]!, + stride: x.stride, + })), + codedWidth: this.codedWidth, + codedHeight: this.codedHeight, + timestamp: this.microsecondTimestamp, + duration: this.microsecondDuration, + colorSpace: this.colorSpace, + displayWidth: this.squarePixelWidth, // Not display* since we're not passing rotation + displayHeight: this.squarePixelHeight, + }); } else if (isVideoFrame(this._data)) { return new VideoFrame(this._data, { timestamp: this.microsecondTimestamp, @@ -955,6 +1008,8 @@ export class VideoSample implements Disposable { timestamp: this.microsecondTimestamp, duration: this.microsecondDuration || undefined, colorSpace: this.colorSpace, + displayWidth: this.squarePixelWidth, // Not display* since we're not passing rotation + displayHeight: this.squarePixelHeight, }); } else { return new VideoFrame(this._data, { diff --git a/test/browser/custom-sample-resources.test.ts b/test/browser/custom-sample-resources.test.ts index 7fb945c..3d3b09a 100644 --- a/test/browser/custom-sample-resources.test.ts +++ b/test/browser/custom-sample-resources.test.ts @@ -5,8 +5,10 @@ import { VideoSample, AudioSampleResource, AudioSample, + VideoDataPlane, + VideoSampleInit, } from '../../src/sample.js'; -import { toUint8Array } from '../../src/misc.js'; +import { MaybePromise, SetRequired, toUint8Array } from '../../src/misc.js'; class ImageVideoSampleResource extends VideoSampleResource { constructor(public image: HTMLImageElement | null) { @@ -42,25 +44,30 @@ class ImageVideoSampleResource extends VideoSampleResource { }); } - allocationSize() { - return this.image!.width * this.image!.height * 4; - } - - copyTo(destination: AllowSharedBufferSource): PlaneLayout[] { + getDataPlanes(): MaybePromise { const canvas = new OffscreenCanvas(this.image!.width, this.image!.height); const ctx = canvas.getContext('2d')!; ctx.drawImage(this.image!, 0, 0); const imageData = ctx.getImageData(0, 0, this.image!.width, this.image!.height); - toUint8Array(destination).set(imageData.data); return [{ - offset: 0, + data: toUint8Array(imageData.data), stride: this.image!.width * 4, }]; } + toRgbSample( + init: SetRequired, + // eslint-disable-next-line @typescript-eslint/no-unused-vars + format: 'RGBA' | 'RGBX' | 'BGRA' | 'BGRX', + // eslint-disable-next-line @typescript-eslint/no-unused-vars + colorSpace: PredefinedColorSpace, + ): MaybePromise { + return new VideoSample(this, init); + } + close() { this.image = null; } @@ -92,6 +99,15 @@ test('Custom VideoSample resource usage', async () => { videoFrame.close(); + const size = sample.allocationSize(); + expect(size).toBe(2048 * 2048 * 4); + + const buf = new ArrayBuffer(size); + await sample.copyTo(buf); + await sample.copyTo(buf, { format: 'RGBX' }); + await sample.copyTo(buf, { format: 'BGRA' }); + await sample.copyTo(buf, { format: 'BGRX' }); + const clone = sample.clone(); sample.close(); expect(resource.image).not.toBe(null); @@ -140,32 +156,9 @@ class AudioBufferAudioSampleResource extends AudioSampleResource { return this.timestamp; } - allocationSize() { - const frameCount = this.audioBuffer!.length; - return frameCount * Float32Array.BYTES_PER_ELEMENT; - } - - copyTo(destination: AllowSharedBufferSource, options: AudioDataCopyToOptions): void { - const frameCount = this.audioBuffer!.length; - const planeIndex = options.planeIndex; - - const channel = this.audioBuffer!.getChannelData(planeIndex); - - let destView: Float32Array; - if (destination instanceof ArrayBuffer) { - destView = new Float32Array(destination); - } else if (destination instanceof SharedArrayBuffer) { - destView = new Float32Array(destination); - } else { - destView = new Float32Array(destination.buffer, destination.byteOffset, destination.byteLength / 4); - } - - const frameOffset = options.frameOffset ?? 0; - const copyFrameCount = options.frameCount !== undefined ? options.frameCount : (frameCount - frameOffset); - - for (let i = 0; i < copyFrameCount; i++) { - destView[i] = channel[i + frameOffset]!; - } + getDataPlane(planeIndex: number): Uint8Array { + const data = this.audioBuffer!.getChannelData(planeIndex); + return toUint8Array(data); } close() { @@ -195,6 +188,11 @@ test('Custom AudioSample resource usage', async () => { expect(sample.timestamp).toBe(0); expect(sample.duration).toBeCloseTo(1); + const size1 = sample.allocationSize({ planeIndex: 0 }); + const size2 = sample.allocationSize({ planeIndex: 1 }); + expect(size1).toBe(48000 * 4); + expect(size2).toBe(48000 * 4); + const buffer = new ArrayBuffer(48000 * 4); sample.copyTo(buffer, { planeIndex: 0 }); diff --git a/test/browser/video-samples.test.ts b/test/browser/video-samples.test.ts index e1cf0ff..2057799 100644 --- a/test/browser/video-samples.test.ts +++ b/test/browser/video-samples.test.ts @@ -287,6 +287,52 @@ test('RGB conversion for ArrayBuffer-backed data', async () => { expect(Array.from(dest)).toEqual(Array.from(src)); } + + { + // RGBX -> RGBA: X (whatever it is) becomes 255 to mean A + const xSrc = new Uint8Array([ + 10, 20, 30, 0, 40, 50, 60, 77, + 70, 80, 90, 128, 100, 110, 120, 200, + ]); + + using sample = new VideoSample(xSrc.slice(), { + timestamp: 0, + codedWidth: 2, + codedHeight: 2, + format: 'RGBX', + }); + + const dest = new Uint8Array(2 * 2 * 4); + await sample.copyTo(dest, { format: 'RGBA' }); + + expect(Array.from(dest)).toEqual([ + 10, 20, 30, 255, 40, 50, 60, 255, + 70, 80, 90, 255, 100, 110, 120, 255, + ]); + } + + { + // RGBX -> BGRA: R and B swap, X becomes 255 to mean A + const xSrc = new Uint8Array([ + 10, 20, 30, 0, 40, 50, 60, 77, + 70, 80, 90, 128, 100, 110, 120, 200, + ]); + + using sample = new VideoSample(xSrc.slice(), { + timestamp: 0, + codedWidth: 2, + codedHeight: 2, + format: 'RGBX', + }); + + const dest = new Uint8Array(2 * 2 * 4); + await sample.copyTo(dest, { format: 'BGRA' }); + + expect(Array.from(dest)).toEqual([ + 30, 20, 10, 255, 60, 50, 40, 255, + 90, 80, 70, 255, 120, 110, 100, 255, + ]); + } }); test('crop (rect option) for ArrayBuffer-backed data', async () => {