Closed
Bug 773460
Opened 12 years ago
Closed 12 years ago
[Azure] pref on Azure canvas
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
mozilla18
People
(Reporter: nrc, Assigned: nrc)
References
Details
Attachments
(7 files, 4 obsolete files)
(deleted),
patch
|
nrc
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
joe
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
cjones
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nrc
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
cjones
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
roc
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•12 years ago
|
||
We should land wihout 774775, but we can't pref on Android until 774775 lands
Assignee | ||
Comment 2•12 years ago
|
||
Attachment #647399 -
Flags: review?(roc)
Assignee | ||
Updated•12 years ago
|
Whiteboard: [leave open]
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);"?
Assignee | ||
Comment 4•12 years ago
|
||
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).
Assignee | ||
Comment 5•12 years ago
|
||
(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.
Why shouldn't it be there? Something should define this pref for non-Win non-Mac platforms.
Assignee | ||
Comment 7•12 years ago
|
||
Because it wherever it is checked the default is false. Should I leave the pref in?
Yes. Generally speaking all prefs should be in all.js. That helps people flip them in about:config.
Assignee | ||
Comment 9•12 years ago
|
||
leave the pref for non-Win, non-Mac
Attachment #647399 -
Attachment is obsolete: true
Attachment #647399 -
Flags: review?(roc)
Attachment #647418 -
Flags: review?(roc)
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.
Attachment #647418 -
Flags: review?(roc) → review+
Assignee | ||
Comment 11•12 years ago
|
||
addressed comment, carrying r=roc
Attachment #647418 -
Attachment is obsolete: true
Attachment #647421 -
Flags: review+
Assignee | ||
Comment 12•12 years ago
|
||
Assignee | ||
Comment 13•12 years ago
|
||
backed out for Android XUL bustage (!): https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=8051eeb12d0c
Assignee | ||
Comment 14•12 years ago
|
||
Comment 15•12 years ago
|
||
Assignee | ||
Comment 16•12 years ago
|
||
Attachment #650433 -
Flags: review?(joe)
Assignee | ||
Comment 17•12 years ago
|
||
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
Assignee | ||
Comment 18•12 years ago
|
||
Attachment #650477 -
Flags: review?(roc)
Attachment #650477 -
Flags: review?(roc) → review+
Comment 19•12 years ago
|
||
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.
Attachment #650433 -
Flags: review?(joe) → review+
Assignee | ||
Comment 20•12 years ago
|
||
(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 :-)
Assignee | ||
Comment 21•12 years ago
|
||
Comment 22•12 years ago
|
||
Assignee | ||
Comment 23•12 years ago
|
||
Attachment #651272 -
Flags: review?(jones.chris.g)
Assignee | ||
Comment 24•12 years ago
|
||
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.
Assignee | ||
Comment 25•12 years ago
|
||
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 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.
Attachment #651272 -
Flags: review?(jones.chris.g) → review+
Assignee | ||
Comment 27•12 years ago
|
||
Comment 28•12 years ago
|
||
Comment 29•12 years ago
|
||
(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.
Assignee | ||
Comment 30•12 years ago
|
||
(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 32•12 years ago
|
||
Attachment #654087 -
Flags: review?(ncameron)
Assignee | ||
Comment 33•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
Attachment #654087 -
Attachment is obsolete: true
Attachment #654087 -
Flags: review?(ncameron)
Attachment #654102 -
Flags: review?(ncameron)
Comment 36•12 years ago
|
||
(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•12 years ago
|
||
Attachment #654102 -
Attachment is obsolete: true
Attachment #654102 -
Flags: review?(ncameron)
Attachment #655502 -
Flags: review?(ncameron)
Assignee | ||
Updated•12 years ago
|
Attachment #655502 -
Flags: review?(ncameron) → review+
Assignee | ||
Comment 38•12 years ago
|
||
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.
Attachment #655508 -
Flags: review?(jones.chris.g)
Assignee | ||
Updated•12 years ago
|
QA Contact: ncameron
Comment 39•12 years ago
|
||
Updated•12 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•12 years ago
|
Assignee: ajones → ncameron
QA Contact: ncameron
Assignee | ||
Comment 40•12 years ago
|
||
Comment 41•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/caffdfa95b07
Going to resolve this now, because the default has changed.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Target Milestone: --- → mozilla17
Updated•12 years ago
|
Attachment #655508 -
Flags: review?(jones.chris.g) → review+
Assignee | ||
Comment 42•12 years ago
|
||
Reopening, because we need to backout the Linux pref patch, and put it back later.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [leave open]
Assignee | ||
Comment 43•12 years ago
|
||
Removing the pref from b2g.js:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=856299bce84f
Assignee | ||
Comment 44•12 years ago
|
||
[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.
Attachment #656705 -
Flags: review?(roc)
Attachment #656705 -
Flags: approval-mozilla-aurora?
Attachment #656705 -
Flags: review?(roc) → review+
Assignee | ||
Comment 45•12 years ago
|
||
Backing out the pref on for Linux:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=6885860f1319
Assignee | ||
Comment 46•12 years ago
|
||
Adding tracking flag because I need to pref off for Aurora (FF17), see comment 44, attachment 656705 [details] [diff] [review]
status-firefox17:
--- → affected
tracking-firefox17:
--- → ?
Updated•12 years ago
|
Comment 47•12 years ago
|
||
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.
Attachment #656705 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 48•12 years ago
|
||
Turn off by default for Linux on Aurora: https://tbpl.mozilla.org/?tree=Mozilla-Aurora&rev=1bd8120a4e40
Assignee | ||
Comment 49•12 years ago
|
||
Try push for pref'ing Linux on by default: https://tbpl.mozilla.org/?tree=Try&rev=fec6e51fba98
Comment 50•12 years ago
|
||
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
Assignee | ||
Comment 51•12 years ago
|
||
Whiteboard: [leave open]
Comment 52•12 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Target Milestone: mozilla17 → mozilla18
You need to log in
before you can comment on or make changes to this bug.
Description
•