Last Comment Bug 773460 - [Azure] pref on Azure canvas
: [Azure] pref on Azure canvas
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: 15 Branch
: All All
: -- normal with 2 votes (vote)
: mozilla18
Assigned To: Nick Cameron [:nrc]
:
Mentors:
: 784582 (view as bug list)
Depends on: 883590 746883 748116 764125 774775 779001 780392 781731 803658 814828
Blocks: 734668
  Show dependency treegraph
 
Reported: 2012-07-12 14:52 PDT by Nick Cameron [:nrc]
Modified: 2013-06-15 18:51 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
disabled


Attachments
turn on for Windows (1.33 KB, patch)
2012-07-30 18:54 PDT, Nick Cameron [:nrc]
no flags Details | Diff | Review
patch (1.36 KB, patch)
2012-07-30 20:25 PDT, Nick Cameron [:nrc]
roc: review+
Details | Diff | Review
patch: pref on Azure/Cairo for Windows (1.37 KB, patch)
2012-07-30 20:46 PDT, Nick Cameron [:nrc]
ncameron: review+
Details | Diff | Review
patch: pref on Azure/Cairo for Android (1.03 KB, patch)
2012-08-08 20:52 PDT, Nick Cameron [:nrc]
joe: review+
Details | Diff | Review
IsLinux should return false for Android (1.09 KB, patch)
2012-08-09 02:30 PDT, Nick Cameron [:nrc]
roc: review+
Details | Diff | Review
patch: pref on for b2g (959 bytes, patch)
2012-08-12 23:16 PDT, Nick Cameron [:nrc]
cjones.bugs: review+
Details | Diff | Review
perf on Azure/Cairo for everything else (842 bytes, patch)
2012-08-21 21:42 PDT, Anthony Jones (:kentuckyfriedtakahe, :k17e)
no flags Details | Diff | Review
pref on Azure/Cairo for everything else (1.27 KB, patch)
2012-08-21 22:38 PDT, Anthony Jones (:kentuckyfriedtakahe, :k17e)
no flags Details | Diff | Review
pref on Azure/Cairo for everything v2 (1.27 KB, patch)
2012-08-26 21:27 PDT, Anthony Jones (:kentuckyfriedtakahe, :k17e)
ncameron: review+
Details | Diff | Review
remove the pref from b2g.js (990 bytes, patch)
2012-08-26 22:04 PDT, Nick Cameron [:nrc]
cjones.bugs: review+
Details | Diff | Review
turn off for Linux again until we land performance improvements (1.24 KB, patch)
2012-08-29 18:51 PDT, Nick Cameron [:nrc]
roc: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Review

Description Nick Cameron [:nrc] 2012-07-12 14:52:05 PDT
We are going to land bug 764125 preffed off to a greater or lesser degree. We should at some point (or points) pref it on. Creating this bug to make a plan and so I don't forget.
Comment 1 Nick Cameron [:nrc] 2012-07-17 16:27:18 PDT
We should land wihout 774775, but we can't pref on Android until 774775 lands
Comment 2 Nick Cameron [:nrc] 2012-07-30 18:54:24 PDT
Created attachment 647399 [details] [diff] [review]
turn on for Windows
Comment 3 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-07-30 18:57:05 PDT
Comment on attachment 647399 [details] [diff] [review]
turn on for Windows

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

Why are you removing the non-Windows non-Mac "pref("gfx.canvas.azure.enabled", false);"?
Comment 4 Nick Cameron [:nrc] 2012-07-30 18:57:32 PDT
The plan is to turn Azure/Cairo on for Windows (as fallback if D2D is present, as preferred backend if not) now. We will turn on Cairo for Android, Linux, and Mac soon (as long as perf looks good), and switch to Skia (Windows XP at first) if the perf numbers are in favour (Joe is investigating).
Comment 5 Nick Cameron [:nrc] 2012-07-30 19:03:24 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #3)
> Comment on attachment 647399 [details] [diff] [review]
> turn on for Windows
> 
> Review of attachment 647399 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Why are you removing the non-Windows non-Mac
> "pref("gfx.canvas.azure.enabled", false);"?

I don't think it should be there - previously it was not there, I added it with one of the last set of patches, but I don't think I should have.
Comment 6 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-07-30 19:32:13 PDT
Why shouldn't it be there? Something should define this pref for non-Win non-Mac platforms.
Comment 7 Nick Cameron [:nrc] 2012-07-30 19:49:26 PDT
Because it wherever it is checked the default is false. Should I leave the pref in?
Comment 8 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-07-30 19:52:24 PDT
Yes. Generally speaking all prefs should be in all.js. That helps people flip them in about:config.
Comment 9 Nick Cameron [:nrc] 2012-07-30 20:25:10 PDT
Created attachment 647418 [details] [diff] [review]
patch

leave the pref for non-Win, non-Mac
Comment 10 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-07-30 20:39:45 PDT
Comment on attachment 647418 [details] [diff] [review]
patch

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

r+ with that

::: modules/libpref/src/init/all.js
@@ +236,5 @@
>  pref("gfx.canvas.azure.enabled", true);
>  pref("gfx.canvas.azure.backends", "cg");
>  #else
>  pref("gfx.canvas.azure.enabled", false);
> +pref("gfx.canvas.azure.backends", "");

make this "cairo" so that someone manually switching azure on will get something.
Comment 11 Nick Cameron [:nrc] 2012-07-30 20:46:39 PDT
Created attachment 647421 [details] [diff] [review]
patch: pref on Azure/Cairo for Windows

addressed comment, carrying r=roc
Comment 13 Nick Cameron [:nrc] 2012-07-30 22:20:26 PDT
backed out for Android XUL bustage (!): https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=8051eeb12d0c
Comment 15 Ed Morley [:emorley] 2012-08-03 07:33:07 PDT
https://hg.mozilla.org/mozilla-central/rev/032ba64ab1f1
Comment 16 Nick Cameron [:nrc] 2012-08-08 20:52:55 PDT
Created attachment 650433 [details] [diff] [review]
patch: pref on Azure/Cairo for Android
Comment 17 Nick Cameron [:nrc] 2012-08-09 02:28:19 PDT
Try push for Android (Android M1 broken): https://tbpl.mozilla.org/?tree=Try&rev=8305f258db00

With the fix: https://tbpl.mozilla.org/?tree=Try&rev=1d5599ccf295
Comment 18 Nick Cameron [:nrc] 2012-08-09 02:30:08 PDT
Created attachment 650477 [details] [diff] [review]
IsLinux should return false for Android
Comment 19 Joe Drew (not getting mail) 2012-08-09 09:00:31 PDT
Comment on attachment 650433 [details] [diff] [review]
patch: pref on Azure/Cairo for Android

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

Only concern here is that I believe this will also turn it on for B2G. Not too big a deal, though.
Comment 20 Nick Cameron [:nrc] 2012-08-09 12:17:38 PDT
(In reply to Joe Drew (:JOEDREW!) from comment #19)
> Comment on attachment 650433 [details] [diff] [review]
> patch: pref on Azure/Cairo for Android
> 
> Review of attachment 650433 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Only concern here is that I believe this will also turn it on for B2G. Not
> too big a deal, though.

Got to happend and some point, and hopefully it will get us fixing bugs quicker :-)
Comment 23 Nick Cameron [:nrc] 2012-08-12 23:16:50 PDT
Created attachment 651272 [details] [diff] [review]
patch: pref on for b2g
Comment 24 Nick Cameron [:nrc] 2012-08-12 23:17:41 PDT
Turns out that Android patch won't turn on Azure/Cairo canvas for b2g (so I'm told), so the new patch hopefully does that.
Comment 25 Nick Cameron [:nrc] 2012-08-14 15:28:00 PDT
Benchmarking
------------

Win7 no D2D, D3d9 Layers
------------------------

Cairo

Fish Bowl       - 10 fish, 8fps
Asteroids       - 47-52 fps
Paintball       - 55.68 secs, 93.76 pb/m
Webvizbench     - 3-5 fps


Skia

Fish Bowl       - 10 fish, 6fps
Asteroids       - 47-52 fps
Paintball       - 38.66 secs, 135.03 pb/m
Webvizbench     - 11-12 fps


Thebes

Fish Bowl       - 10 fish, 7-8fps
Asteroids       - 47-52 fps
Paintball       - 55.67 secs, 93.77 pb/m
Webvizbench     - 4-6 fps


Android, Nexus 7
----------------

(Fish bowl is pretty useless at 10 fish here)

Cairo

Fish Bowl       - 10 fish, 2fps
Asteroids       - 14-15 fps
http://www.smashcat.org/av/canvas_test/
                - 11 fps 

Skia

Fish Bowl       - 10 fish, 1fps
Asteroids       - 27-31 fps
http://www.smashcat.org/av/canvas_test/
                - 10 fps


Thebes

Fish Bowl       - 10 fish, 2fps
Asteroids       - 14-15 fps
http://www.smashcat.org/av/canvas_test/
                - 9-11 fps

                
Android, HTC One X
------------------

Cairo

Fish Bowl       - 10 fish, 1fps
Asteroids       - 11-16 fps
http://www.smashcat.org/av/canvas_test/
                - 10 fps
          

Skia

Fish Bowl       - 10 fish, 1fps
Asteroids       - 25-28 fps
http://www.smashcat.org/av/canvas_test/
                - 8 fps

          
Thebes

Fish Bowl       - 10 fish, 1fps
Asteroids       - 11-12 fps
http://www.smashcat.org/av/canvas_test/
                - 8-9 fps
Comment 26 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-08-14 16:36:00 PDT
Comment on attachment 651272 [details] [diff] [review]
patch: pref on for b2g

There are two gaia "demo games" that are fps sensitive.  One is broken (argh!), but the other shows no change in fps.
Comment 28 Ed Morley [:emorley] 2012-08-15 09:47:59 PDT
https://hg.mozilla.org/mozilla-central/rev/5ac49037cb77
Comment 29 Michael Wu [:mwu] 2012-08-15 09:56:49 PDT
(In reply to Nick Cameron [:nrc] from comment #24)
> Turns out that Android patch won't turn on Azure/Cairo canvas for b2g (so
> I'm told), so the new patch hopefully does that.

It should and it appears to do so. ANDROID is defined on B2G.
Comment 30 Nick Cameron [:nrc] 2012-08-15 17:55:59 PDT
(In reply to Michael Wu [:mwu] from comment #29)
> (In reply to Nick Cameron [:nrc] from comment #24)
> > Turns out that Android patch won't turn on Azure/Cairo canvas for b2g (so
> > I'm told), so the new patch hopefully does that.
> 
> It should and it appears to do so. ANDROID is defined on B2G.

I was told (possibly incorrectly) that b2g does not look at libpref/.../all.js, and so the patch is needed to add the pref to b2g.js. Are you saying that b2g uses both all.js and b2g.js? And therefore the pref can be removed from b2g.js? I'm afraid I have no idea about b2g, nor a working b2g build to test this with, so I am going on hearsay. If you could confirm what needs doing, that would be helpful.
Comment 31 Anthony Jones (:kentuckyfriedtakahe, :k17e) 2012-08-21 20:54:03 PDT
*** Bug 784582 has been marked as a duplicate of this bug. ***
Comment 32 Anthony Jones (:kentuckyfriedtakahe, :k17e) 2012-08-21 21:42:52 PDT
Created attachment 654087 [details] [diff] [review]
perf on Azure/Cairo for everything else
Comment 33 Nick Cameron [:nrc] 2012-08-21 21:48:02 PDT
Comment on attachment 654087 [details] [diff] [review]
perf on Azure/Cairo for everything else

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

::: modules/libpref/src/init/all.js
@@ +241,5 @@
>  #ifdef ANDROID
>  pref("gfx.canvas.azure.enabled", true);
>  pref("gfx.canvas.azure.backends", "cairo");
>  #else
> +pref("gfx.canvas.azure.enabled", true);

Why not move this outside the ifdefs now that it holds everywhere?
Comment 34 Anthony Jones (:kentuckyfriedtakahe, :k17e) 2012-08-21 21:50:01 PDT
I considered going that but figured that we're probably going to very soon move to skia on Android so I may as well just let sleeping dogs lie.
Comment 35 Anthony Jones (:kentuckyfriedtakahe, :k17e) 2012-08-21 22:38:44 PDT
Created attachment 654102 [details] [diff] [review]
pref on Azure/Cairo for everything else
Comment 36 Joe Drew (not getting mail) 2012-08-22 19:25:29 PDT
(In reply to Anthony Jones (:kentuckyfriedtakahe) from comment #34)
> I considered going that but figured that we're probably going to very soon
> move to skia on Android so I may as well just let sleeping dogs lie.

Only if it's faster!
Comment 37 Anthony Jones (:kentuckyfriedtakahe, :k17e) 2012-08-26 21:27:36 PDT
Created attachment 655502 [details] [diff] [review]
pref on Azure/Cairo for everything v2
Comment 38 Nick Cameron [:nrc] 2012-08-26 22:04:29 PDT
Created attachment 655508 [details] [diff] [review]
remove the pref from b2g.js

As discussed with cjones on irc (and mentioned in comments above), b2g uses the ANDROID pref in all.js, so this change to b2g.js was unnecessary. Thus, removing it.
Comment 39 Anthony Jones (:kentuckyfriedtakahe, :k17e) 2012-08-26 23:10:39 PDT
http://tbpl.mozilla.org/?tree=Try&rev=f4c7e27c718e
Comment 41 :Ms2ger 2012-08-27 06:09:08 PDT
https://hg.mozilla.org/mozilla-central/rev/caffdfa95b07

Going to resolve this now, because the default has changed.
Comment 42 Nick Cameron [:nrc] 2012-08-29 18:39:06 PDT
Reopening, because we need to backout the Linux pref patch, and put it back later.
Comment 43 Nick Cameron [:nrc] 2012-08-29 18:45:30 PDT
Removing the pref from b2g.js:

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=856299bce84f
Comment 44 Nick Cameron [:nrc] 2012-08-29 18:51:20 PDT
Created attachment 656705 [details] [diff] [review]
turn off for Linux again until we land performance improvements

[Approval Request Comment]
Bug caused by (feature/regressing bug #): attachment 655502 [details] [diff] [review]
User impact if declined: performance regressions
Testing completed (on m-c, etc.): n/a - just backing out a previous patch
Risk to taking this patch (and alternatives if risky): n/a
String or UUID changes made by this patch: none

The patch in 655502 shouldn't have made the Aurora uplift, other patches it depended on didn't make it.
Comment 45 Nick Cameron [:nrc] 2012-08-29 19:00:12 PDT
Backing out the pref on for Linux:

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=6885860f1319
Comment 46 Nick Cameron [:nrc] 2012-09-04 14:05:38 PDT
Adding tracking flag because I need to pref off for Aurora (FF17), see comment 44, attachment 656705 [details] [diff] [review]
Comment 47 Lukas Blakk [:lsblakk] use ?needinfo 2012-09-04 16:15:00 PDT
Comment on attachment 656705 [details] [diff] [review]
turn off for Linux again until we land performance improvements

Approving, please set the status flag for 17 to disabled once this has landed.
Comment 48 Nick Cameron [:nrc] 2012-09-04 16:18:56 PDT
Turn off by default for Linux on Aurora: https://tbpl.mozilla.org/?tree=Mozilla-Aurora&rev=1bd8120a4e40
Comment 49 Nick Cameron [:nrc] 2012-09-04 17:03:28 PDT
Try push for pref'ing Linux on by default: https://tbpl.mozilla.org/?tree=Try&rev=fec6e51fba98
Comment 50 Anthony Jones (:kentuckyfriedtakahe, :k17e) 2012-09-04 17:04:53 PDT
Both these try runs include this patch and are all green:

https://tbpl.mozilla.org/?tree=Try&rev=fec6e51fba98
https://tbpl.mozilla.org/?tree=Try&rev=dc20a1747a53
Comment 52 Ryan VanderMeulen [:RyanVM] 2012-09-05 19:44:59 PDT
https://hg.mozilla.org/mozilla-central/rev/1661cbbc3759

Note You need to log in before you can comment on or make changes to this bug.