1173117 - Use OS snapshotting for the gfx sanity test component

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 1, wait for painting to flush

This isn't needed for drawWindow but it is needed for OS snapshotting.

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2, refactor

Refactor a little bit so we can perform multiple snapshots.

Comment 3

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8617597 [details] [diff] [review]
part 2, refactor

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

::: toolkit/components/gfx/SanityTest.js
@@ +22,5 @@
>  const TEST_PASSED=0;
>  const TEST_FAILED_RENDER=1;
>  const TEST_FAILED_VIDEO=2;
>  const TEST_CRASHED=3;
> +const TEST_ABORTED=4;

We need to update the histogram description with this new value.

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 1, refactor

The popup problem exposed bugs in the approach I was going to use here. New approach will be more reliable, I hope.

This refactors SanityTest.js so we can run multiple tests, triggered by different events if necessary.

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2, add a notification for WM_PAINT

Send a notice for the first WM_PAINT for each widget. I'm not sure whether this has the same meaning on Linux/OS X as it does for Windows (in terms of snapshotting), but we can cross that bridge when necessary.

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 3, bad size check during snapshot

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 4, OS snapshotting

This patch adds OS snapshotting as an experimental test. If the compositor snapshot succeeds, we wait for a WM_PAINT and for all MozAfterPaints to flush. Then, we snapshot the widget via the OS.

We report via telemetry the result of the low-level snapshot. It can either pass, fail, abort due to an error, or timeout.

This histogram is intended to be temporary. Once we get sufficient data on how well it works relative to the compositor test, we can remove the instrumentation and make it the canonical test.

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8620698 [details] [diff] [review]
part 4, OS snapshotting

Vladan, could you look at the telemetry changes here? Explanation is in comment #7.

Comment 9

[David Anderson [:dvander] - inactive, e-mail if emergency]

Note: this patch assumes we will get rid of the popup=1 property on the test window. If we end up having to proceed with the popup, we will need other fixes in place (but those can be done as additional patches).

Comment 10

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8620615 [details] [diff] [review]
part 1, refactor

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

::: toolkit/components/gfx/SanityTest.js
@@ +52,4 @@
>    // TODO: drawWindow reads back from the gpu's backbuffer, which won't catch issues with presenting
>    // the front buffer via the window manager. Ideally we'd use an OS level API for reading back
>    // from the desktop itself to get a more accurate test.
>    var ctx = canvas.getContext("2d");

This is a constant for the canvas, so we might as well move it out to the caller.

@@ +84,5 @@
>      testPixel(ctx, 50, 114, 255, 0, 0, 255, 64);
>  }
>  
> +function testCompositor(win, canvas) {
> +  var canvas = takeWindowSnapshot(win, canvas);

Shadowing of the variable 'canvas' with different types is a bit weird. Moving the ctx var out here will fix that.

Comment 11

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8620698 [details] [diff] [review]
part 4, OS snapshotting

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

::: toolkit/components/gfx/SanityTest.js
@@ +204,5 @@
> +    if (this.utils.isMozAfterPaintPending) {
> +      // Wait for any pending paints to flush.
> +      this.win.addEventListener("MozAfterPaint", (() => {
> +        if (this.utils && !this.utils.isMozAfterPaintPending) {
> +          this.onWindowReady();

Where is this function defined?

Comment 12

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Matt Woodrow (:mattwoodrow) from comment #11)
> Comment on attachment 8620698 [details] [diff] [review]
> part 4, OS snapshotting
> 
> Review of attachment 8620698 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/gfx/SanityTest.js
> @@ +204,5 @@
> > +    if (this.utils.isMozAfterPaintPending) {
> > +      // Wait for any pending paints to flush.
> > +      this.win.addEventListener("MozAfterPaint", (() => {
> > +        if (this.utils && !this.utils.isMozAfterPaintPending) {
> > +          this.onWindowReady();
> 
> Where is this function defined?

Whoops, that's a bug. It should be this.waitForPaintsFlushed(), thanks.

Comment 13

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8620698 [details] [diff] [review]
part 4, OS snapshotting

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

::: toolkit/components/gfx/SanityTest.js
@@ +157,5 @@
>  
>    onWindowLoaded: function() {
>      this.canvas = this.win.document.createElementNS("http://www.w3.org/1999/xhtml", "canvas");
>      this.canvas.setAttribute("width", PAGE_WIDTH);
>      this.canvas.setAttribute("height", PAGE_HEIGHT);

As mentioned for the earlier patch, it would be better if we called getContext("2d") here, and passed the context around instead of the element.

@@ +202,5 @@
> +    }
> +
> +    if (this.utils.isMozAfterPaintPending) {
> +      // Wait for any pending paints to flush.
> +      this.win.addEventListener("MozAfterPaint", (() => {

Should we clear this listener once we've finished?

Comment 14

[Vladan Djeric (:vladan)]

Comment on attachment 8620698 [details] [diff] [review]
part 4, OS snapshotting

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

::: toolkit/components/gfx/SanityTest.js
@@ +16,5 @@
>  const DEVICE_PREF="sanity-test.device-id";
>  const VERSION_PREF="sanity-test.version";
>  const DISABLE_VIDEO_PREF="media.hardware-video-decoding.failed";
>  const RUNNING_PREF="sanity-test.running";
> +const OS_SNAPSHOT_TIMEOUT_SEC = 3;

nit: consistent spacing

@@ +56,5 @@
> +function reportSnapshotResult(val) {
> +  try {
> +    let histogram = Services.telemetry.getHistogramById("GRAPHICS_SANITY_TEST_OS_SNAPSHOT");
> +    histogram.add(val);
> +  } catch (e) {}

You don't need a try-catch for this, this could only throw if the histogram was undefined

::: toolkit/components/telemetry/Histograms.json
@@ +8130,5 @@
>      "releaseChannelCollection": "opt-out",
>      "description": "Reports results from the graphics sanity test to track which drivers are having problems"
> +  },
> +  "GRAPHICS_SANITY_TEST_OS_SNAPSHOT": {
> +    "expires_in_version": "never",

- don't forget the alert_emails field
- add it to the other sanity test histograms as well

Comment 15

[Vladan Djeric (:vladan)]

Comment on attachment 8620698 [details] [diff] [review]
part 4, OS snapshotting

restoring mattwoodrow's r+.. sorta

Comment 16

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Is this heading for late beta uplift? Please let me know (and if so, nominate the bug for tracking for 39). Thanks!

Comment 17

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Liz Henry (:lizzard) from comment #16)
> Is this heading for late beta uplift? Please let me know (and if so,
> nominate the bug for tracking for 39). Thanks!

Nope.

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4aefa21c8ead
https://hg.mozilla.org/integration/mozilla-inbound/rev/451e24d7ed64
https://hg.mozilla.org/integration/mozilla-inbound/rev/9266896a26d4
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0a391c7c14a

Comment 19

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8620620 [details] [diff] [review]
part 2, add a notification for WM_PAINT

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

::: layout/base/nsPresShell.cpp
@@ +8844,5 @@
>      return;
>    }
> +
> +  if (!mHasReceivedPaintMessage) {
> +    mHasReceivedPaintMessage = true;

We purposely want the set this flag even if we don't have the document/service/window and notify the observers?

Comment 20

[David Anderson [:dvander] - inactive, e-mail if emergency]

Those conditions shouldn't change at a future point in time for the same window (I think), unless we are painting before we have a document.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4aefa21c8ead
https://hg.mozilla.org/mozilla-central/rev/451e24d7ed64
https://hg.mozilla.org/mozilla-central/rev/9266896a26d4
https://hg.mozilla.org/mozilla-central/rev/b0a391c7c14a