Looks nice, but I'm concerned you're covering what looks like an enormous amount
of functionality in just one test. I have some suggestions on how to split them
up into a bunch more tests on the C++ level (looks like you'd want like 8 or
more of them instead of your single one today).
https://codereview.chromium.org/2476693002/diff/100001/content/test/data/media/getusermedia-depth-capture.htmlFile content/test/data/media/getusermedia-depth-capture.html (right):
https://codereview.chromium.org/2476693002/diff/100001/content/test/data/media/getusermedia-depth-capture.html#newcode64content/test/data/media/getusermedia-depth-capture.html:64: {
Nit: pull up braces to previous line (apply throughout)
https://codereview.chromium.org/2476693002/diff/100001/content/test/data/media/getusermedia-depth-capture.html#newcode113content/test/data/media/getusermedia-depth-capture.html:113: function
testVideoToWebGLTexture(videoElementName, success, error)
Will you get clear test errors when any of the sub-tests in this
function fails? Will test errors obscure other test errors? You're
testing a lot of things here for one browser test, so you run the risk
of really confusing test errors. Also, if one of the sub-tests goes
flaky your entire test will probably be disabled even if the other
sub-tests are fine.
You can remedy this by splitting up the test; for instance by splitting
up the below into one 2D and one Cubemap test, or even 2d_unsigned_byte,
2d_float, cubemap_unsigned_byte, cubemap_float. Each such test gets
called by one browser test on the c++ level. You can solve the resulting
duplication problem using a higher-order function, e.g.
function runOneGlTest(glTestCaseFunction) {
getFake16bitStream().then(function(stream) {
detectVideoInLocalView1(stream, function() {
glTestCaseFunction('local-view-1', function(skip_info) {
if (skip_info) {
console.log("SKIP " + glTestCaseFunction + ": " +
skip_info);
}
stream.getVideoTracks()[0].stop();
waitForVideoToStop('local-view-1');
}, failedCallback);
});
}, failedCallback);
It's your call, but if you keep this as-is, at least test manually
introducing some failures. That way you can check that you get
reasonable test failure messages and that the tests don't interfere with
each other.
https://codereview.chromium.org/2476693002/diff/100001/content/test/data/media/getusermedia-depth-capture.html#newcode177content/test/data/media/getusermedia-depth-capture.html:177: //For
cubemap, binding_target is gl.TEXTURE_CUBE_MAP and target is a face id.
Nit: Space between // and For; this and next line
https://codereview.chromium.org/2476693002/diff/100001/content/test/data/media/getusermedia-depth-capture.html#newcode181content/test/data/media/getusermedia-depth-capture.html:181: var tex =
gl.createTexture();
Nit: indent 2 here, not 1
https://codereview.chromium.org/2476693002/diff/100001/content/test/data/media/getusermedia-depth-capture.html#newcode207content/test/data/media/getusermedia-depth-capture.html:207: if
(use_sub_image_2d) {
I don't like this kind of complexity in tests; they should generally
have a cyclomatic complexity of one (e.g. no branches). You're basically
at the level where the test is complex enough to need a test of its own.
One suggestion is to replace
var p3 = runWebGLTextureTest(gl, gl.TEXTURE_CUBE_MAP,
gl.TEXTURE_CUBE_MAP_POSITIVE_Z, type, video, false/*sub_image*/,
true/*flip_y*/, true/*premultiply_alpha*/);
with
var texture = createTexture(gl, binding_target, ...);
uploadVideoFrameSubImage2D(texture, binding_target, ...);
var p3 = readAndVerifyPixels(texture, ...);
That way you can compose your tests by calling
uploadVideoFrameSubImage2D, which is now a simple three-line function,
or uploadVideoFrame which is a one-liner.
https://codereview.chromium.org/2476693002/