Closed Bug 1173117 Opened 4 years ago Closed 4 years ago

Use OS snapshotting for the gfx sanity test component

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: dvander, Assigned: dvander)

References

Details

Attachments

(4 files, 2 obsolete files)

No description provided.
This isn't needed for drawWindow but it is needed for OS snapshotting.
Attachment #8617583 - Flags: review?(matt.woodrow)
Attached patch part 2, refactor (obsolete) — Splinter Review
Refactor a little bit so we can perform multiple snapshots.
Attachment #8617597 - Flags: review?(matt.woodrow)
Attachment #8617583 - Flags: review?(matt.woodrow) → review+
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.
Attachment #8617597 - Flags: review?(matt.woodrow) → review+
Attached patch part 1, refactorSplinter Review
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.
Attachment #8617583 - Attachment is obsolete: true
Attachment #8617597 - Attachment is obsolete: true
Attachment #8620615 - Flags: review?(matt.woodrow)
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.
Attachment #8620620 - Flags: review?(matt.woodrow)
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.
Attachment #8620698 - Flags: review?(matt.woodrow)
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.
Attachment #8620698 - Flags: review?(vdjeric)
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 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.
Attachment #8620615 - Flags: review?(matt.woodrow) → review+
Attachment #8620623 - Flags: review?(matt.woodrow) → review+
Attachment #8620620 - Flags: review?(matt.woodrow) → review?(roc)
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?
(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 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?
Attachment #8620698 - Flags: review?(matt.woodrow) → review+
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
Attachment #8620698 - Flags: review?(vdjeric)
Attachment #8620698 - Flags: review?(matt.woodrow)
Attachment #8620698 - Flags: review+
Comment on attachment 8620698 [details] [diff] [review]
part 4, OS snapshotting

restoring mattwoodrow's r+.. sorta
Attachment #8620698 - Flags: review?(matt.woodrow) → review+
Is this heading for late beta uplift? Please let me know (and if so, nominate the bug for tracking for 39). Thanks!
Flags: needinfo?(dvander)
(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.
Flags: needinfo?(dvander)
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?
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.
You need to log in before you can comment on or make changes to this bug.