Add unpackColorSpace tests to tex-image-and-sub-image-2d-with-video #3404
Conversation
|
|
| */ | ||
| var crossProductTestCasesWithUnpackColorSpaces = function(cases, unpackColorSpaces) | ||
| { | ||
| var caseWithColorSpace = function(c, cs) { return Object.assign({}, c, {unpackColorSpace:cs}); } |
There was a problem hiding this comment.
return Object.assign({}, c, {unpackColorSpace:cs})
can be shortened to
return {...c, ...{unpackColorSpace:cs}};
There was a problem hiding this comment.
This didn't work when I tested against FF, so I left it unchanged.
There was a problem hiding this comment.
How did you test? Works in Firefox just fine AFAIK
https://jsgist.org/?src=c212a3fa63ff82bc55c88b73fd996b1b
I've been using it for years
There was a problem hiding this comment.
Hmm, no idea what I did wrong. Now it works fine. Updated.
| { | ||
| var caseWithColorSpace = function(c, cs) { return Object.assign({}, c, {unpackColorSpace:cs}); } | ||
| var casesList = unpackColorSpaces.map(cs => cases.map(c => caseWithColorSpace(c, cs))); | ||
| return [].concat.apply([], casesList); |
There was a problem hiding this comment.
Could this just be
return caseList;
?
There was a problem hiding this comment.
casesList ends up being
[[list of cases with srgb], [list of cases with display-p3]]
and so the concat moves them all into a single list.
There was a problem hiding this comment.
In that case you can use flat
return caseList.flat()
There was a problem hiding this comment.
Thanks @ccameron-chromium for writing this - looks like a solid improvement to start handling color spaces. I verified that this is a no-op in current Chrome Canary on macOS, but didn't test it with experimental web features turned on.
Approving now; a few small comments and questions throughout.
| */ | ||
| var crossProductTestCasesWithUnpackColorSpaces = function(cases, unpackColorSpaces) | ||
| { | ||
| var caseWithColorSpace = function(c, cs) { return {...c, ...{unpackColorSpace:cs}}; } |
There was a problem hiding this comment.
Seeing the earlier discussion with @greggman , having done some reading, and not having seen this trick before - could you please add a comment like:
// Merge test case and unpackColorSpace using spread operator
? I spent some time wondering if two iterables with multiple items each were being folded into a dictionary.
There was a problem hiding this comment.
I want to respond constructively. Using the spread operator in 2022 is normal idiomatic JavaScript. Does it really need it described? I get that you didn't know it but now that you do if you browse most modern JavaScript you'll see it everywhere. For example here's the react docs using it. Here's the redux toolkit using it. And svelte
If you really feel it's important then sure, keep the comment but then it seems like every use of the spread operator will need to be explained by a comment since you can't assume devs will find this one explanation.
Is it just this particular one off usage?
There was a problem hiding this comment.
I'm certainly not helping the situation by using 1 and 2 character variable names almost exclusively :)
I've updated the variable names to be more self-documenting.
| if ('unpackColorSpace' in gl) | ||
| return ['srgb', 'display-p3']; | ||
| else | ||
| return [null]; |
There was a problem hiding this comment.
Would it be better to use undefined rather than null here? (Maybe not.)
There was a problem hiding this comment.
changed to undefined.
sdk/tests/js/webgl-test-utils.js
Outdated
| var namedColorInColorSpace = function(colorName, colorSpace) { | ||
| var result; | ||
| switch (colorSpace) { | ||
| case null: |
There was a problem hiding this comment.
If null is changed to undefined in unpackColorSpacesToTest, above, then this would have to be changed too.
Follow-on to KhronosGroup#3404 . Associated with http://crbug.com/1208480 .
Follow-on to #3404 . Associated with http://crbug.com/1208480 .
Change the test to use named colors, such as 'Red' and 'Green', and add the utility function namedColorInColorSpace to provide color space conversion.
Several tests replicate the behavior of converting a color from an 8-bit RGBA value to how that color will appear when sampled with a given internal format. Move this functionality to a colorAsSampledWithInternalFormat function.
Some of the tests started failing when using RGBA4 as an internal format, because the values being tested were no longer 0 or 255. Add a tolerance utility function to compute the appropriate for a given internalformat, format, and type.
To all test cases in tex-image-and-sub-image-2d-with-video.js, add an unpackColorSpace parameter. Cross-product all tests with the set of all color spaces. Add a utility function to perform this cross-product.