The default bug view has changed. See this FAQ.

Make WebGL mochitest log useful information about failures

RESOLVED FIXED in mozilla10

Status

()

Core
Canvas: WebGL
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bjacob, Assigned: drs)

Tracking

unspecified
mozilla10
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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.
(Reporter)

Updated

6 years ago
Blocks: 630728
(Assignee)

Updated

6 years ago
Assignee: nobody → dsherk
(Assignee)

Comment 1

6 years ago
Created attachment 566432 [details] [diff] [review]
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.
Attachment #566432 - Flags: review?(bjacob)
(Assignee)

Comment 2

6 years ago
Created attachment 566434 [details] [diff] [review]
Patch v1.0, print out mochitest error debug info.

Instantly noticed a small mistake, corrected.
Attachment #566432 - Attachment is obsolete: true
Attachment #566432 - Flags: review?(bjacob)
Attachment #566434 - Flags: review?(bjacob)
(Reporter)

Updated

6 years ago
Attachment #566434 - Attachment is patch: true
(Reporter)

Comment 3

6 years ago
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.
Attachment #566434 - Flags: review?(bjacob) → review-
(Assignee)

Comment 4

6 years ago
Created attachment 569221 [details] [diff] [review]
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.
Attachment #566434 - Attachment is obsolete: true
Attachment #569221 - Flags: review?(bjacob)
(Reporter)

Comment 5

6 years ago
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.
Attachment #569221 - Flags: review?(bjacob) → review+
(Assignee)

Comment 6

6 years ago
Created attachment 571558 [details] [diff] [review]
Patch v1.2, print out mochitest error debug info.

Addressed code review comments.
Attachment #569221 - Attachment is obsolete: true
Attachment #571558 - Flags: review?(bjacob)
(Reporter)

Updated

6 years ago
Attachment #571558 - Flags: review?(bjacob) → review+
(Reporter)

Comment 7

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce1330a5c9cb
https://hg.mozilla.org/mozilla-central/rev/ce1330a5c9cb
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
Depends on: 699626
You need to log in before you can comment on or make changes to this bug.