693703 - Make WebGL mochitest log useful information about failures

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

mochitest-1 logs currently don't help much debugging WebGL test failures.

We need to dump() some message on all subtest failures. For example, in js-test-pre.js, in the shouldBe() function, dump() something on failure. Likewise in conformance/resources/webgl-test-utils.js the glErrorShouldBe function and its friends. Probably no need to dump the test URL as we dump it already when we start running the page.

Wherever applicable, on failures of the form "X should be equal to Y but isn't" it's useful to print the values of X and Y.

In the case of functions comparing canvas pixels to a reference rendering, like checkCanvasRect in conformance/resources/webgl-test-utils.js, it would be very useful to dump DataURL's of both renderings. This would help e.g. bug 630728.

Comment 1

[Doug Sherk (:drs) (inactive)]

Attachment: Patch v1.0, print out mochitest error debug info.

This should implement everything discussed in parent comment. Also note that I checked into dump() and it does get print in non-debug mode, but you have to set something which I'm guessing the test slaves probably already have on:

https://developer.mozilla.org/en/Debugging_JavaScript
"To see anything, you need to set the pref browser.dom.window.dump.enabled to true, e.g. in about:config (add new pref, it doesn't exist per default)."

If it's not enabled, we should probably ask them to enable it anyway.

@bjacob: let me know if this is everything you wanted. This seems rather small and it covers everything you talked about, but I'm not sure if you were looking for more things that I might just not be thinking of.

Comment 2

[Doug Sherk (:drs) (inactive)]

Attachment: Patch v1.0, print out mochitest error debug info.

Instantly noticed a small mistake, corrected.

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 566434 [details] [diff] [review]
Patch v1.0, print out mochitest error debug info.

Review of attachment 566434 [details] [diff] [review]:
-----------------------------------------------------------------

So, I have probably been confusing by pointing specifically to checkCanvasRect which is just one of the functions that needed to be instrumented, and not the general case as it's only comparing to a solid color.

Below are review comments for checkCanvasRect but please grep the conformance/ directory for occurences of readPixels and check what other uses of readPixels are similar tests, and instrument them in the same way. For example, the GLSL conformance tests compare two images using compareRendering() in glsl-function-nodes.html.

::: content/canvas/test/webgl/conformance/resources/webgl-test-utils.js
@@ +350,5 @@
> +            }
> +            // Set the alpha channel to max.
> +            imgd.data[offset + 3] = 255;
> +        }
> +        ctx.putImageData(imgd, 0, 0);

A canvas can have an alpha channel, and likewise color can be a RGBA vector. Why overwrite with 255?

Also, I don't think you need getImageData/putImageData for this part (sorry, I was probably confusing). You should be able to achieve the same with something like

    ctx.fillStyle="rgba(r,g,b,a)";
    ctx.fillRect(0, 0, width, height);

no?

@@ +360,5 @@
> +                imgd.data[offset + j] = buf[offset + j];
> +            }
> +        }
> +        ctx.putImageData(imgd, 0, 0);
> +        var testData = cv.toDataURL();

Remembering that buf was the result of a ReadPixels on the WebGL context... why don't we just replace that by a .toDataURL() directly on the WebGL context? I think you can get the canvas from the gl context by doing gl.canvas, it's an attribute.

Comment 4

[Doug Sherk (:drs) (inactive)]

Attachment: Patch v1.1, print out mochitest error debug info.

Added much more debug info and partially addressed code review comments. We discussed it in person and decided to scrap one of the changes.

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 569221 [details] [diff] [review]
Patch v1.1, print out mochitest error debug info.

Review of attachment 569221 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/test/webgl/conformance/misc/uninitialized-test.html
@@ +62,5 @@
>              if (x >= skipX && x < skipX + skipWidth && y >= skipY && y < skipY + skipHeight) {
>                  if (data[index] != skipR || data[index + 1] != skipG || data[index + 2] != skipB || data[index + 3] != skipA) {
> +                    testFailed("non-zero pixel values are wrong, (" +
> +                               data[index] + "," + data[index + 1] + "," + data[index + 2] + "," + data[index + 3] +
> +                               ") should have been (" + skipR + "," + skipG + "," + skipB + "," + skipA + ")");

maybe dump x and y here

::: content/canvas/test/webgl/conformance/resources/webgl-test-utils.js
@@ +397,5 @@
>          var was = buf[offset + 0].toString();
>          for (j = 1; j < color.length; ++j) {
>            was += "," + buf[offset + j];
>          }
>          debug('expected: ' + color + ' was ' + was);

keep that after the testFailed

::: content/canvas/test/webgl/resources/js-test-pre.js
@@ +135,5 @@
> +    testFailed(msg);
> +
> +    var data = 'REFTEST TEST-UNEXPECTED-FAIL | ' + msg + ' | image comparison (==)\n' +
> +                   'REFTEST   IMAGE 1 (TEST): ' + testData + '\n' +
> +                   'REFTEST   IMAGE 2 (REFERENCE): ' + refData;

indentation. also, add a comment saying that the precise formatting is important for reftest-analyzer.

Comment 6

[Doug Sherk (:drs) (inactive)]

Attachment: Patch v1.2, print out mochitest error debug info.

Addressed code review comments.

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce1330a5c9cb

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ce1330a5c9cb