Skip to content

Commit

Permalink
Fix color space conversion test (#3332)
Browse files Browse the repository at this point in the history
* Fix color space conversion test

The issue was makeInPlaceColorConversion was not actually doing
the conversion "in place". To find this I wrote a test to test
makeInPlaceColorConversion
  • Loading branch information
greggman authored Jan 26, 2024
1 parent e5c09e6 commit 99ff37e
Show file tree
Hide file tree
Showing 4 changed files with 114 additions and 6 deletions.
1 change: 1 addition & 0 deletions src/webgpu/listing_meta.json
Original file line number Diff line number Diff line change
Expand Up @@ -2056,6 +2056,7 @@
"webgpu:shader,validation,uniformity,uniformity:pointers:*": { "subcaseMS": 1.738 },
"webgpu:shader,validation,uniformity,uniformity:short_circuit_expressions:*": { "subcaseMS": 1.401 },
"webgpu:shader,validation,uniformity,uniformity:unary_expressions:*": { "subcaseMS": 1.279 },
"webgpu:util,texture,color_space_conversions:util_matches_2d_canvas:*": { "subcaseMS": 1.001 },
"webgpu:util,texture,texel_data:float_texel_data_in_shader:*": { "subcaseMS": 2.042 },
"webgpu:util,texture,texel_data:sint_texel_data_in_shader:*": { "subcaseMS": 2.573 },
"webgpu:util,texture,texel_data:snorm_texel_data_in_shader:*": { "subcaseMS": 4.645 },
Expand Down
5 changes: 3 additions & 2 deletions src/webgpu/util/color_space_conversion.ts
Original file line number Diff line number Diff line change
Expand Up @@ -230,9 +230,10 @@ export function makeInPlaceColorConversion({
// This technically represents colors outside the src gamut, so no clamping yet.

if (requireColorSpaceConversion) {
// WebGPU currently only supports dstColorSpace = 'srgb'.
if (srcColorSpace === 'display-p3' && dstColorSpace === 'srgb') {
rgba = displayP3ToSrgb(rgba);
Object.assign(rgba, displayP3ToSrgb(rgba));
} else if (srcColorSpace === 'srgb' && dstColorSpace === 'display-p3') {
Object.assign(rgba, srgbToDisplayP3(rgba));
} else {
unreachable();
}
Expand Down
108 changes: 108 additions & 0 deletions src/webgpu/util/texture/color_space_conversions.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
export const description = 'Color space conversion helpers';

import { Fixture } from '../../../common/framework/fixture.js';
import { makeTestGroup } from '../../../common/framework/test_group.js';
import { ErrorWithExtra } from '../../../common/util/util.js';
import { makeInPlaceColorConversion } from '../color_space_conversion.js';
import { clamp } from '../math.js';

import { TexelView } from './texel_view.js';
import { findFailedPixels } from './texture_ok.js';

const kTestColors = [
[0xff, 0, 0],
[0, 0xff, 0],
[0, 0, 0xff],
[0x80, 0x80, 0],
[0, 0x80, 0x80],
[0x80, 0, 0x80],
] as const;

function floatToU8(v: number) {
return clamp(Math.round(v * 255), { min: 0, max: 255 });
}

export const g = makeTestGroup(Fixture);

g.test('util_matches_2d_canvas')
.desc(`Test color space conversion helpers matches canvas 2d's color space conversion`)
.params(u =>
u.combineWithParams([
{ srcColorSpace: 'srgb', dstColorSpace: 'display-p3' },
{ srcColorSpace: 'display-p3', dstColorSpace: 'srgb' },
] as { srcColorSpace: PredefinedColorSpace; dstColorSpace: PredefinedColorSpace }[])
)
.fn(t => {
const { srcColorSpace, dstColorSpace } = t.params;

// putImageData an ImageData(srcColorSpace) in to a canvas2D(dstColorSpace)
// then call getImageData. This will convert the colors via the canvas 2D API
const width = kTestColors.length;
const height = 1;
const imgData = new ImageData(
new Uint8ClampedArray(kTestColors.map(v => [...v, 255]).flat()),
width,
height,
{ colorSpace: srcColorSpace }
);
const ctx = new OffscreenCanvas(width, height).getContext('2d', {
colorSpace: dstColorSpace,
})!;
ctx.putImageData(imgData, 0, 0);
const expectedData = ctx.getImageData(0, 0, width, height).data;

const conversionFn = makeInPlaceColorConversion({
srcPremultiplied: false,
dstPremultiplied: false,
srcColorSpace,
dstColorSpace,
});

// Convert the data via our conversion functions
const convertedData = new Uint8ClampedArray(
kTestColors
.map(color => {
const [R, G, B] = color.map(v => v / 255);
const floatColor = { R, G, B, A: 1 };
conversionFn(floatColor);
return [
floatToU8(floatColor.R),
floatToU8(floatColor.G),
floatToU8(floatColor.B),
floatToU8(floatColor.A),
];
})
.flat()
);

const subrectOrigin = [0, 0, 0];
const subrectSize = [width, height, 1];
const areaDesc = {
bytesPerRow: width * 4,
rowsPerImage: height,
subrectOrigin,
subrectSize,
};

const format = 'rgba8unorm';
const actTexelView = TexelView.fromTextureDataByReference(format, convertedData, areaDesc);
const expTexelView = TexelView.fromTextureDataByReference(format, expectedData, areaDesc);

const failedPixelsMessage = findFailedPixels(
format,
{ x: 0, y: 0, z: 0 },
{ width, height, depthOrArrayLayers: 1 },
{ actTexelView, expTexelView },
{ maxDiffULPsForNormFormat: 0 }
);

if (failedPixelsMessage !== undefined) {
const msg = 'Color space conversion had unexpected results:\n' + failedPixelsMessage;
t.expectOK(
new ErrorWithExtra(msg, () => ({
expTexelView,
actTexelView,
}))
);
}
});
6 changes: 2 additions & 4 deletions src/webgpu/web_platform/copyToTexture/canvas.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -97,9 +97,7 @@ class F extends CopyToTextureUtils {
}

const imageData = new ImageData(imagePixels, width, height, { colorSpace });
// MAINTENANCE_TODO: Remove as any when tsc support imageData.colorSpace
/* eslint-disable-next-line @typescript-eslint/no-explicit-any */
if (typeof (imageData as any).colorSpace === 'undefined') {
if (typeof imageData.colorSpace === 'undefined') {
this.skip('color space attr is not supported for ImageData');
}

Expand Down Expand Up @@ -762,7 +760,7 @@ g.test('color_space_conversion')
.params(u =>
u
.combine('srcColorSpace', ['srgb', 'display-p3'] as const)
.combine('dstColorSpace', ['srgb'] as const)
.combine('dstColorSpace', ['srgb', 'display-p3'] as const)
.combine('dstColorFormat', kValidTextureFormatsForCopyE2T)
.combine('dstPremultiplied', [true, false])
.combine('srcDoFlipYDuringCopy', [true, false])
Expand Down

0 comments on commit 99ff37e

Please sign in to comment.