1298373 - Perfherder graphs look very distorted on first load with recent Firefox (probably a canvas issue)

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[William Lachance (:wlach)]

Steps to reproduce:

Go to https://treeherder.mozilla.org/perf.html#/graphs click "add test data", select any series, then click "add". The perfherder graphs will appear very distorted (problem goes away if you reload the page). This started happening recently, not reproducible with older builds of Firefox or Chrome, which led me to think this was a platform issue. Perfherder uses canvas (via the flot js library).

Using mozregression I narrowed the range to this with the nightlies: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=97a52326b06a07930216ebefa5af333271578904&tochange=23c2ec5544b9e0a74a047b87b594e4c36a8fe95c

Which I then narrowed to this with inbound:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=491bc7e98f2aecc40a4fb096e9f3bc7a548989fa&tochange=d35d1cbc59e81f5efa4f3737c0ea6a9c9b90da14

Which means it looks like the perf regression fixes in bug 1294351 caused the problem?

I have only reproduced this problem on Mac, uncertain if other platforms have this issue.

Comment 1

[Nicolas Silva [:nical]]

I haven't managed to reproduce this so far, could you attach a screen shot showing the issue ?

Comment 2

[William Lachance (:wlach)]

Attachment: Distorted view on initial load

Comment 3

[William Lachance (:wlach)]

Attachment: View after reloading page

Comment 4

[William Lachance (:wlach)]

I ran into this bug again today. Any progress on determining the root cause? It's pretty frustrating and I worry that this will be reproducible on many sites other than just Perfherder.

Comment 5

[Nicolas Silva [:nical]]

I landed something in bug 1298345 that aims at addressing this sort of issue (problems restoring the canvas state between frames, in the case of this bug, transforms). I haven't been able to test it on a computer that reproduces the perfherder issue, could you check whether it works with the latest nightly?

Comment 6

[William Lachance (:wlach)]

(In reply to Nicolas Silva [:nical] from comment #5)
> I landed something in bug 1298345 that aims at addressing this sort of issue
> (problems restoring the canvas state between frames, in the case of this
> bug, transforms). I haven't been able to test it on a computer that
> reproduces the perfherder issue, could you check whether it works with the
> latest nightly?

Still reproducible on September 2nd's nightly. :(

Comment 7

[William Lachance (:wlach)]

BTW, if it would be helpful I'd be happy to build a custom version of Firefox with some logging code or whatever to help track this down.

Comment 8

[Roman R.]

This bug may be related to bug 1298631

Comment 9

[Nicolas Silva [:nical]]

(In reply to Roman R. from comment #8)
> This bug may be related to bug 1298631

I don't think so. This is canvas-specific and I strongly suspect it is a bug in the code that records and restores the canvas transform between frames.

(In reply to William Lachance (:wlach) from comment #7)
> BTW, if it would be helpful I'd be happy to build a custom version of
> Firefox with some logging code or whatever to help track this down.

Thanks, this is towards the top of my list (although I have a few fires to put out beforehand), I'll come back with builds to test as soon as I've made some progress.

Comment 10

[Jonathan Kew [:jfkthame]]

William, just wondering -- are you on a Mac with Retina display? Or are you on a system with multiple screens, perhaps with different resolutions (a mixture of hi- and lo-dpi displays)? It looks like the bad rendering has a 2x scale factor being applied to the canvas paths, which might be related to the scaling between Cocoa points (and CSS px units) and device pixels that applies on Retina screens.

If you go to the Firefox app's Info panel (in the Finder), check the Open in Low Resolution checkbox, and relaunch the app, does that prevent this happening? If so, that would support the idea that it may be the device-pixel scale factor that's leaking out somewhere it doesn't belong.

Comment 11

[William Lachance (:wlach)]

(In reply to Jonathan Kew (:jfkthame) from comment #10)
> William, just wondering -- are you on a Mac with Retina display? Or are you
> on a system with multiple screens, perhaps with different resolutions (a
> mixture of hi- and lo-dpi displays)? It looks like the bad rendering has a
> 2x scale factor being applied to the canvas paths, which might be related to
> the scaling between Cocoa points (and CSS px units) and device pixels that
> applies on Retina screens.
> 
> If you go to the Firefox app's Info panel (in the Finder), check the Open in
> Low Resolution checkbox, and relaunch the app, does that prevent this
> happening? If so, that would support the idea that it may be the
> device-pixel scale factor that's leaking out somewhere it doesn't belong.

Yup, I do have a Mac with a retina display and if I do what you suggest the problem goes away (and comes back if I re-enable high resolution mode).

Comment 12

[Jonathan Kew [:jfkthame]]

Interesting.... I'm also on a retina Mac, but so far I haven't managed to reproduce this.

What OS version are you on? Do you use multiple displays, or are you seeing this on a single-display setup? Do you have any add-ons that might be involved somehow?

Comment 13

[Roman R.]

Attachment: Normal look

Comment 14

[Roman R.]

Attachment: distorted look

Added screenshots for normal and distorted look.

Comment 15

[Roman R.]

This (or something similar to it) also happens on Windows 8.1 (64-bit).

Comment 16

[Nicolas Silva [:nical]]

(In reply to Roman R. from comment #15)
> This (or something similar to it) also happens on Windows 8.1 (64-bit).

This is a different bug.

Comment 17

[Nicolas Silva [:nical]]

Plot.js appears to be applying a 2x scale when it detects hidpi screens, by the look of https://github.com/flot/flot/blob/958e5fd43c6dff4bab3e1fd5cb6109df5c1e8003/jquery.flot.js#L172

Somewhere in the canvas code we do something that causes this scale to be applied twice, I haven't figured out what that is yet.

Comment 18

[Nicolas Silva [:nical]]

Attachment: Reset the DrawTarget's transform if the canvas's state is reset.

This is caused by a sneaky interaction between two canvas optimizations:
 * If the PersistentBufferProvider can preserve the drawing state (clips ad transforms), we avoid clearing and the clips at the end of a frame and restoring them at the beginning of the next frame. Otherwise we record clips and transforms and re-apply them at the beginning of the next frame, starting by reinitializing the transform.
 * When SetDimensions/Reset() is called we drop our drawing state and clear the canvas draw target. In the process if doing that we force the canvas to go through the code path that clears the clips at the end of the frame, even if the provider preserves the drawing state (since per spec Reset must clear the drawing state). The second optimization is that we keep the same DrawTarget if the size of the canvas did not change to avoid an allocation.

As a result, if you are in this situation:
 * the canvas is using a PersistentBufferProvider that preserves the drawing state (the case for skiagl canvas on mac)
 * you have a transform applied to the canvas.
 * you call SetDimensions without actually changing the size

You end up clearing the clips but not clearing the transform. Then, next drawing command the canvas code assumes it has a freshly allocated DrawTaget with a pristine drawing state, except we kept the previous one around with its transform not reset. We don't go through the code that restores the clips and transforms so we don't reset the transform.

The fix is simply to clear the transform when clear the clips without expecting that the transform will be cleared at the next drawing command.

Comment 19

[Nicolas Silva [:nical]]

I have a doubt about this now because I'm struggling to create test case to verify the fix (emulating the context with a linux computer and some prefs).

Comment 20

[Nicolas Silva [:nical]]

William, could you try this build out and let me know if it still reproduces the issue https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-6b98a69044904c886532ef194ae21897c1681685/try-macosx64-debug/ ?

Comment 21

[William Lachance (:wlach)]

(In reply to Nicolas Silva [:nical] from comment #20)
> William, could you try this build out and let me know if it still reproduces
> the issue
> https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-
> 6b98a69044904c886532ef194ae21897c1681685/try-macosx64-debug/ ?

Actual link is:

https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-6b98a69044904c886532ef194ae21897c1681685/try-macosx64-debug/

(I think there's a wrong character in there or something)

Anyway, this build fixes the bug. Yay!

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b81f663f726f
Clear the transform when resetting the canvas target state. r=Bas

Comment 23

[Nicolas Silva [:nical]]

This may need to be uplifted to aurora.

Comment 24

[William Lachance (:wlach)]

(In reply to Nicolas Silva [:nical] from comment #23)
> This may need to be uplifted to aurora.

FWIW this bug is not reproducible on Firefox Developer Edition (50.0a2, September 12th). This makes sense as I think the code that caused the regression landed after the last uplift.

Comment 25

[Nicolas Silva [:nical]]

(In reply to William Lachance (:wlach) from comment #24)
> (In reply to Nicolas Silva [:nical] from comment #23)
> > This may need to be uplifted to aurora.
> 
> FWIW this bug is not reproducible on Firefox Developer Edition (50.0a2,
> September 12th). This makes sense as I think the code that caused the
> regression landed after the last uplift.

Excellent, thanks! I was unsure because of the various canvas-related patches that were uplifted recently.

Comment 26

[Carsten Book [:Tomcat]]

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