Closed Bug 1042291 Opened 10 years ago Closed 10 years ago

Implement a better heuristic for when to use HW accelerated <canvas>

Categories

(Core :: Graphics: Layers, defect)

Product:
Core
Component:
Graphics: Layers
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla36
Project Flags:
blocking-b2g 2.2+
Tracking Flags:
Tracking Status
firefox34 --- wontfix
firefox35 --- wontfix
firefox36 --- fixed
b2g-v2.1 --- affected
b2g-v2.2 --- fixed

People

(Reporter: khuey, Assigned: milan)

References

(Depends on 1 open bug)

Details

(Keywords: memory-footprint, perf, Whiteboard: [MemShrink:P1])

Attachments

(1 file, 3 obsolete files)

Canvas observer approach cleaned up, with review comments. Carry r=gw280,snorp
18.38 KB, patch
milan
: review+
Details | Diff | Splinter Review
+++ This bug was initially created as a clone of Bug #1041112 +++ The current size-based heuristic is not good enough. We need something better. And I think we need it for 2.1.
Yup, I agree. In bug 884226 I added the 'willReadFrequently' flag with the goal that it be part of a more sophisticated heuristic for determining which backend we should be using. The thing I was thinking about goes like this: * Initial backend determined by prefs (gfx.canvas.azure.accelerated, etc) and willReadFrequently flag. Also consider relying on whether or not the canvas is attached to a document. If it's not, you're going to have to do readback at some point to get the result, which implies a software backend may be appropriate. * Per 'frame', keep some kind of journal of what types of operations are being done. Also note how often changes are occurring. * Based on the above, periodically score the current backend and the alternative backend. If the alternative has a higher score, switch over. If the above is implemented, I think we'll actually want to make the software backend the default. If you are a game doing a bunch of drawImage(), this heuristic should upgrade you to a hardware backend in short order. However, in the case of b2g home screen images and things like that, you may have already lost if we initially choose the hardware backend.
Whiteboard: [MemShrink][systemsfe] → [MemShrink:P1][systemsfe]
Heuristic sounds reasonable - An even simpler suggestion (I think comment #1 is better, but may be difficult to implement): * Initial backend always software * Switch to hardware after X consecutive drawImage's when attached to a document X can be any arbitrary number, or perhaps it should be based on coverage, but I say consecutive as if you started doing lots of vector/text drawing as part of your draw loop, I assume software would be faster here(?)
[Blocking Requested - why for this release]: This shouldn't be a "blocker". Should we move this to feature-b2g:2.2? Thanks.
blocking-b2g: 2.1+ → ---
feature-b2g: --- → 2.2
To provide more information here. Taipei engineering managers had discussions about this bug, and we don't think this is a bug. It should be a feature. Because this bug has systemfe in the whiteboard, needinfo Candice here, to make sure this bug has corresponding plan(in 2.1, or in 2.2, or later version of FxOS). Thanks.
Flags: needinfo?(cserran)
This is the root cause of many bugs every release. We can split hairs about "feature vs bug", but fixing it should be a high priority IMO.
This has systemsfe in it because I cloned it from another bug that has systemsfe. This is an issue in the platform, and it needs to be fixed ASAP. It really needs to be fixed for 2.0, but I think it's too late for that.
Flags: needinfo?(cserran)
Whiteboard: [MemShrink:P1][systemsfe] → [MemShrink:P1]
It does need to be fixed, but in the meantime, all uses of canvas in gaia should be modified to use software canvas. I don't think there are any exceptions.
If that's the case why don't we just turn off hw accelerated canvas entirely?
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #8) > If that's the case why don't we just turn off hw accelerated canvas entirely? Because games will want it on, which is the more common use of canvas in the wild.
Do we have data to support that? Why would we not believe that other things are likely to use <canvas> in the same way Gaia does?
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #10) > Do we have data to support that? Why would we not believe that other things > are likely to use <canvas> in the same way Gaia does? I'm only going on anecdotal evidence, so this is a fair point. What is worth mentioning though is that using software instead of hardware causes much worse issues than the other way round. There are plenty of games on the Firefox Marketplace that use canvas and they'd be rendered unplayable if they suddenly got software canvas.
Were these games unplayable on 1.4? We had software <canvas> then, no?
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #12) > Were these games unplayable on 1.4? We had software <canvas> then, no? I think skia canvas acceleration was added in v1.2, wasn't it?
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #10) > Do we have data to support that? Why would we not believe that other things > are likely to use <canvas> in the same way Gaia does? Yes, there have been bugs in the past where we failed to get an accelerated canvas for games. A good example is the FishIETank demo: http://ie.microsoft.com/testdrive/Mobile/Performance/FishIETank/Default.html Which regressed when the max skia canvas size was set to low. See bug 999841.
I don't think so. We didn't see any of these memory usage issues during the 1.3 CS cycle.
Anyways, we need to do something good here for 2.1 at the earliest.
blocking-b2g: --- → 2.1+
feature-b2g: 2.2 → ---
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #15) > I don't think so. We didn't see any of these memory usage issues during the > 1.3 CS cycle. There were skia memory problems in 1.3. See bug 982237. It does seem like skia is using more memory now, though. Was a new version imported recently?
Ok. We changed something then ...
We typically take a new Skia update every cycle, so yes, it's possible things changed.
See the problems with skia memory in bug 1006088. It appears we never solved that and the app developer worked around the issue.
Well, the vertical homescreen has been the major offender here, right? Did we use canvas for icons in the same way before? I don't think there's a good argument for making canvas acceleration opt-in.
Pretty much every app that uses <canvas> sees multiple MB of memory usage in Skia stuff, just in user space. Before bug 1039150 every app that uses FontSizeUtils was spinning up this subsystem and using at least 1.5 MB of memory.
If our memory usage has increase so significantly, then I think we need to make our default be to use software rendering with the ability to opt in to hardware rendering. (In reply to Chris Lord [:cwiiis] from comment #11) > What is worth mentioning though is that using software instead of hardware causes much > worse issues than the other way round. This is only true to a point. If a game uses enough memory that it starts crashing, then using hardware does in fact cause much worse issues than the other way around. It's very concerning if the current recommendation is for gaia to everywhere opt in to a non-default value. If that's the case for gaia it's likely the case for most content in the marketplace. If we can't come up with anything better, making the default be software backed, and enabling opting in to hardware accelerated, might be the way to go for now :(
The default is set for the people that use the canvas to display it. Gaia uses canvases to do image processing, often just to serialize data, so the default that is setup for "you're going to draw something in this canvas and just display it" is not appropriate. We could argue that the default usage for canvas is "to process images and serialize them", but I'm not sure that's the case, regardless of how Gaia's using it.
+1 Milan's comment. I implore us not to use software canvas by default, it's a killer for games and gfx team have worked hard to make games viable on the web. Let's not pretend like there are loads of apps available on FxOS and we're ruining them all by slightly increasing memory use of canvas. If I was an app developer looking at FxOS (which I am) and I saw that canvas performance was unacceptably bad for games, I'd just give up at that point and write it off. The memory increase only made a difference on 256 meg devices, and only because we use canvas in this pathological way - let's not extrapolate from this, our use of canvas in Gaia is quite a different situation to the use of canvas in a single app.
[Blocking Requested - why for this release]: If we want to switch the whole device to have software canvas, it is easy enough. In the device specific preferences, we can just set gfx.canvas.azure.accelerated to false. Whether that should be done wholesale or not is a product/UX decision. If particular apps are using too much memory, and that can be attributed to accelerated canvas, and they're OK with the performance degradation when going with software, we already have a way to deal with that. So, "we must fix this for 2.1" is not clear to me, and even if it was, it would be a feature-b2g: 2.1, rather than blocking-b2g: 2.1+. Writing a heuristic to do something is new code that we have to invent and write. Back to triage, in the meantime I will raise this as a feature request for 2.1.
blocking-b2g: 2.1+ → 2.1?
The accelerated canvases would be using up to 2mb of cache on actual 256mb devices, but up to >17mb on "simulated" 256mb devices (where we set the actual memory size to 273mb or higher.) So, we could be seeing that as well. See bug 1045680.
(In reply to Milan Sreckovic [:milan] from comment #26) > ... > Back to triage, in the meantime I will raise this as a feature request for > 2.1. There is no room for this as a feature in 2.1. We will consider it for 2.2
blocking-b2g: 2.1? → backlog
The idea that it requires writing code therefore it's a feature is ludicrous. We simply cannot ship things in a state where the default is something that most apps do not want and you have to opt into what is usually the desired behavior. I'm honestly not sure what more to say here because that should be obvious.
blocking-b2g: backlog → 2.1?
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #29) > We simply cannot ship things in a state where the default is something that > most apps do not want and you have to opt into what is usually the desired > behavior. I'm honestly not sure what more to say here because that should > be obvious. This isn't true though. I'd posit that most apps do want accelerated canvas and it's just Gaia that doesn't want it. Not to mention that the side-effect of picking an accelerated canvas when you don't need it is just a little more memory use (which is usually fine). I think before you start saying stuff like this, you should try surveying some of the uses of canvas on the marketplace, rather than assuming that just because Gaia has some memory issues because of this, it's a huge, pervasive problem (it isn't). I'm honestly not sure what to say here, that applications want a canvas that performs on par with other browsers should be obvious.
Why do we think that all of the apps we ship with the phone are unrepresentative? That's a pretty extraordinary claim. We know that the homescreen has lots of users. What apps on the marketplace want this and how many users do they have?
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #31) > Why do we think that all of the apps we ship with the phone are > unrepresentative? That's a pretty extraordinary claim. Really? I don't see many dialer, contacts, homescreen, browser, settings, camera, etc. apps on the marketplace. I do see a lot of games though. > We know that the > homescreen has lots of users. What apps on the marketplace want this and > how many users do they have? Gaia apps can already request the behaviour they need, I don't see what the issue is here. The only place where this has been a problem is the homescreen, because apparently the extra, what, 3 megs(?) that this ends up using is an issue. An issue that can be worked around of course, and is partly down to bad behaviour of the homescreen (keeping loads of canvas elements alive). Let's not talk about number of users, we don't have the numbers to really play that game. Maybe spend some time watching people play with their phones and see what they do though. I see Facebook, Twitter, general phone stuff (sms, phone, camera) and games. I should probably put games first though, based on usage. If you made accelerated canvas an opt-in, or disabled it entirely, you'd be crippling a good number of games - anything based off of impact.js, phaser, pixi.js, construct, etc. (some have prototype webgl backends, but asking developers to enable them is asking them to special-case FirefoxOS - a non-starter until it actually has a respectable user-base) Maybe I'm not sure what you're asking for here, but if you're asking to disable accelerated canvas, or make it opt-in, that is really not a good idea. Like, really.
The 21 top apps in the "popular" section on marketplace right now are (grouped, rather than in order): Social: - ConnectA2 - InfoWhatsapp - Loqui IM - LINE - Facebook - MessageMe - OpenWaap - Twitter Games: - Pacman Canvas - Penguin Pop - Zombie Apocalypse - CutTheRope - Candy Crush - Slice Fruits Music: - SoundCloud - YouZeek - MusicGraph - Piano Other: - Wallpaper Wonderful - File Manager - Flashlight That's about one quarter games. I haven't seen any actual data on time users spend on various things. Do we have that somewhere? I would love to read that. Anyways, I'm not asking for anyone to cripple games. I'm asking for exactly what is in the title of the bug: a better heuristic than the size-based guessing we do today.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #33) > That's about one quarter games. I haven't seen any actual data on time > users spend on various things. Do we have that somewhere? I would love to > read that. You should cut this list to users of canvas where the element stays around for long periods/the lifetime of the app. > Anyways, I'm not asking for anyone to cripple games. I'm asking for exactly > what is in the title of the bug: a better heuristic than the size-based > guessing we do today. Fair enough, this is needed (thus the bug) - I still don't think it's a blocking issue for 2.1 though.
With a reasonable Skia cache size I guess this is not a huge deal. It still affects our testing because the Flame is more than 256 MB in the low memory configuration but there's a bug on file about that already.
blocking-b2g: 2.1? → backlog
Sotaro found in bug 1042305 that the CanvasImageCache limit greatly affects SkiaGL performance. If we are thrashing that we should demote to software.
[Blocking Requested - why for this release]: Re-nominate to 2.1. As in Comment 36, if CanvasImageCache hit it's limit and canvas uses SkiaGL, that application's performance regress until almost useless level.
blocking-b2g: backlog → 2.1?
We can and should work on this without making it a blocker. In fact, we work on the "backlog" items unless we have blockers already.
Assignee: nobody → gwright
blocking-b2g: 2.1? → 2.1+
Flags: needinfo?(gwright)
This bug already has a lot of comments in it, but we will probably create a meta bug for "deal with all things Skia caching related", where we can summarize all the caches that exist, figuring out which ones we may want to play with, track making sure those are accessible as options, see where we need a heuristic and where we just need a good default, decide if any of the defaults should be recommended to the OEMs as "configure your phone using these", track if the caches are being emptied based on low memory pressure events, etc. This bug would then be blocking that new meta bug, and I imagine we'd open a number of other new bugs, if we don't have them already, and decide which ones of those we may want for 2.1.
So I'm currently digesting the information here but before I comment on the heuristic itself, I'd like to chime in and say that almost all of the canvas use I've seen in the wild have been pretty much blitting images around as sprites. This is where the GPU really helps and the jump from a software to a hardware backed canvas is a huge win. There are a few marginal cases where they do vector drawing like generating graphs and charts, but the vast majority are the former use case. That being said, there's selection bias in what I've seen, but it's worth sharing.
Flags: needinfo?(gwright)
How does this look so far? Any comments on the heuristic itself would be appreciated as I can't really think of a better way to determine what backend mode to use right now.
Attachment #8487441 - Flags: feedback?(snorp)
Comment on attachment 8487441 [details] [diff] [review] 0001-Add-a-CanvasDrawObserver-which-observes-the-first-fe.patch Review of attachment 8487441 [details] [diff] [review]: ----------------------------------------------------------------- It's a start, and is probably better than what we're doing now. I'm not sure I'm sold on only making a single switch, but again, might be ok for 1.0. Two things I still want to see: 1) Check if the canvas element is attached to a document. If not, prefer software. 2) Check if we are being called from rAF. If not, probably want software. Or at least lean that way. ::: dom/canvas/CanvasRenderingContext2D.h @@ +732,5 @@ > > + // This observes our draw calls at the beginning of the canvas > + // lifetime and switches to software or GPU mode depending on > + // what it thinks is best > + CanvasDrawObserver *mDrawObserver; Do we really need a pointer here? @@ +1066,5 @@ > + TimeDuration timeElapsed = TimeStamp::NowLoRes() - mCreationTime; > + > + // We log the first 30 frames of any canvas object then make a > + // call to determine whether it should be GPU or CPU backed > + if (mFramesRendered >= 30 || timeElapsed.ToSeconds() >= 5.0) { I would set the time threshold lower. Like 2s. @@ +1073,5 @@ > + } else { > + mCanvasContext->SwitchRenderingMode(CanvasRenderingContext2D::RenderingMode::SoftwareBackendMode); > + } > + > + mDisabled = true; So we're only going to make at most one switch? Is that really what we want?
Attachment #8487441 - Flags: feedback?(snorp) → feedback+
I'm worried about thrashing between software/GPU, and so the single switch makes sense to me. That being said, are the first few frames/seconds of a canvas lifetime indicative of its long-term usage? We don't need a pointer, you're right. I'll prepare some more patches. Do you have any further feedback on the heuristic itself? Specifically, any other draw calls I should be considering, and is the current inequality test too simplistic? That's what I was mainly aiming for with this feedback request, as the patch isn't meant to be ready to land (not even close) in this state.
Any progress there? George, do you need help?
(In reply to Fabrice Desré [:fabrice] from comment #44) > Any progress there? George, do you need help? I'm working on a different team now, but I discussed this with snorp earlier today and we think we can probably land this in its current state for now, and then file another bug later to further refine the heuristic itself.
Comment on attachment 8487441 [details] [diff] [review] 0001-Add-a-CanvasDrawObserver-which-observes-the-first-fe.patch Review of attachment 8487441 [details] [diff] [review]: ----------------------------------------------------------------- This is ok for now assuming it doesn't cause massive bustage
Attachment #8487441 - Flags: review+
(In reply to Fabrice Desré [:fabrice] from comment #47) > https://hg.mozilla.org/integration/b2g-inbound/rev/12ec3e08ee67 Hrm, I don't think the patch ever went through try?
Milan, do you have someone that could finish that? I could not reproduce the test failure locally.
Flags: needinfo?(milan)
(In reply to Fabrice Desré [:fabrice] from comment #50) > Milan, do you have someone that could finish that? I could not reproduce the > test failure locally. Welcome to graphics. Nothing ever reproduces locally. :) I'll follow up.
Flags: needinfo?(milan)
OK, this stalled. George, can you do a try push, just to see if anything changed?
Flags: needinfo?(gwright)
Just wanted to check, did the try push happened? (In reply to Milan Sreckovic [:milan] from comment #52) > OK, this stalled. George, can you do a try push, just to see if anything > changed?
Sorry, forgot about this. Will prepare that now.
Flags: needinfo?(gwright)
So that try run was a huge mess :(
Hi Milan, so does the bug still exist? (just wanted to check on its status)
Yes, the bug exists, and the fix is breaking try.
I'll see if I can green this out.
Assignee: gwright → milan
So... in order to evaluate a heuristic, we need examples and the answers we hope to get for those examples. What are the representative examples for "we should get software" and "we should get hardware" where we today guess wrong? I know it's weird to ask for this in comment 60, but we seemed to have jumped into all sorts of different solutions without explicitly recording what we're trying to solve. This bug is tagged as "MemShrink", so I imagine the primary goal is to have less memory used, which would suggest the heuristic that starts with software and upgrades to hardware based on some information (comment 1 and comment 2 were moving in that direction.) What are some good examples?
Flags: needinfo?(khuey)
(In reply to Milan Sreckovic [:milan] from comment #60) > What are some good examples? "We should get software" for basically all of Gaia, yes? As for where we should get hardware, Where's my Water or Cut the Rope seem like the obvious candidates. AIUI it's primarily games that benefit from acceleration.
Flags: needinfo?(khuey)
I think anything using canvas for off-screen calculation, such as working with fonts, should be using software. For example. things using font.js[1] like (I believe) poppit. [1] https://github.com/Pomax/Font.js
Where's My Water is WebGL iirc, so we'd want to look at the Cut the Rope case if we are going to use either to test this heuristic.
(In reply to George Wright (:gw280) from comment #63) > Where's My Water is WebGL iirc, so we'd want to look at the Cut the Rope > case if we are going to use either to test this heuristic. I think WMW is also 2d canvas.
Same intended functionality as George's patch, but modified to not force into accelerated the configurations that shouldn't be right now (e.g., old Android, non-Skia.)
Attachment #8520675 - Flags: review?(snorp)
Attachment #8520675 - Flags: review?(gwright)
Comment on attachment 8520675 [details] [diff] [review] Canvas observer approach cleaned up Review of attachment 8520675 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me if we pass try! ::: dom/canvas/CanvasRenderingContext2D.cpp @@ +936,5 @@ > if (!gfxPlatform::GetPlatform()->UseAcceleratedSkiaCanvas()) { > mRenderingMode = RenderingMode::SoftwareBackendMode; > } > > + if (gfxPlatform::GetPlatform()->BothAcceleratedAndNotCanvasAvailable()) { Maybe I'm being stupid today, but it took me a little while to work out what "BothAcceleratedAndNotCanvasAvailable" meant. Actually, do we even really need this because software canvas is *always* available? Can we just check if accelerated is available and because we know there are no cases where software is unavailable, have the same thing? @@ +1510,5 @@ > if (attributes.mWillReadFrequently) { > + > + // We want to lock into software, so remove the observer that > + // may potentially change that... > + RemoveDrawObserver(); This is still up for debate. I personally think that if the dev has made this request (which strongly hints at "don't use the GPU you fools!"), then we should honour it 100% as you've done here. However, snorp disagrees and thinks we should only use it to set initial state.
Attachment #8520675 - Flags: review?(gwright) → review+
(In reply to George Wright (:gw280) from comment #67) > Comment on attachment 8520675 [details] [diff] [review] > Canvas observer approach cleaned up > > Review of attachment 8520675 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good to me if we pass try! > > ::: dom/canvas/CanvasRenderingContext2D.cpp > @@ +936,5 @@ > > if (!gfxPlatform::GetPlatform()->UseAcceleratedSkiaCanvas()) { > > mRenderingMode = RenderingMode::SoftwareBackendMode; > > } > > > > + if (gfxPlatform::GetPlatform()->BothAcceleratedAndNotCanvasAvailable()) { > > Maybe I'm being stupid today, but it took me a little while to work out what > "BothAcceleratedAndNotCanvasAvailable" meant. Actually, do we even really > need this because software canvas is *always* available? Can we just check > if accelerated is available and because we know there are no cases where > software is unavailable, have the same thing? If both are not available, there is no reason to count commands - we'd never want to switch. And while the software canvas is always available in principle, we'd need to clean up the code, because D2D scenario is handled elsewhere, and not in this code/class. This seems safer for now. I'll take suggestions for a better function name that fits in a single line though :) > > @@ +1510,5 @@ > > if (attributes.mWillReadFrequently) { > > + > > + // We want to lock into software, so remove the observer that > > + // may potentially change that... > > + RemoveDrawObserver(); > > This is still up for debate. I personally think that if the dev has made > this request (which strongly hints at "don't use the GPU you fools!"), then > we should honour it 100% as you've done here. However, snorp disagrees and > thinks we should only use it to set initial state. I'll leave it to :snorp to comment then. I have no emotional attachment to this, and am quite proud I thought to cover this special case :), but outside of that, :snorp was the author of will-read-frequently, so he'd have the best data as to how we want to use it. Perhaps it's worth going beyond a boolean, and give it a third "we think so, but count for a bit and see what happens" value...
(In reply to Milan Sreckovic [:milan] from comment #69) > > > > This is still up for debate. I personally think that if the dev has made > > this request (which strongly hints at "don't use the GPU you fools!"), then > > we should honour it 100% as you've done here. However, snorp disagrees and > > thinks we should only use it to set initial state. > > I'll leave it to :snorp to comment then. I have no emotional attachment to > this, and am quite proud I thought to cover this special case :), but > outside of that, :snorp was the author of will-read-frequently, so he'd have > the best data as to how we want to use it. Perhaps it's worth going beyond > a boolean, and give it a third "we think so, but count for a bit and see > what happens" value... I do still think will-read-frequently should just set the initial state. It's a 'hint', after all, not a different "mode" for the canvas. That said, if we are going to switch to a GL canvas when the hint is set, we should probably be extra-sure we want to do that, so a more sophisticated heuristic may be necessary. For 1.0 I'm ok with never switching.
Comment on attachment 8520675 [details] [diff] [review] Canvas observer approach cleaned up Review of attachment 8520675 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/canvas/CanvasRenderingContext2D.cpp @@ +717,5 @@ > + > +private: > + const float mMinimumSecondsBeforeDecision = 5.0; > + const int32_t mMinimumFramesBeforeDecision = 30; > + const int32_t mMinimumCallsBeforeDecision = 4; We should prefify these to make it easier to play around with the heuristic. ::: gfx/thebes/gfxAndroidPlatform.cpp @@ +419,5 @@ > + > +bool gfxAndroidPlatform::BothAcceleratedAndNotCanvasAvailable() > +{ > +#ifdef MOZ_WIDGET_ANDROID > + if (AndroidBridge::Bridge()->GetAPIVersion() < 11) { I think we just want to call gfxAndroidPlatform::UseAcceleratedSkiaCanvas() here? ::: gfx/thebes/gfxPlatform.h @@ +259,5 @@ > bool SupportsAzureContentForType(mozilla::gfx::BackendType aType) { > return BackendTypeBit(aType) & mContentBackendBitmask; > } > > + virtual bool BothAcceleratedAndNotCanvasAvailable(); The name is weird enough that I think a comment could help :)
Attachment #8520675 - Flags: review?(snorp) → review-
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #71) > Comment on attachment 8520675 [details] [diff] [review] > Canvas observer approach cleaned up > > Review of attachment 8520675 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/canvas/CanvasRenderingContext2D.cpp > @@ +717,5 @@ > > + > > +private: > > + const float mMinimumSecondsBeforeDecision = 5.0; > > + const int32_t mMinimumFramesBeforeDecision = 30; > > + const int32_t mMinimumCallsBeforeDecision = 4; > > We should prefify these to make it easier to play around with the heuristic. Makes sense. Especially once I parsed what "prefify" means :) > > ::: gfx/thebes/gfxAndroidPlatform.cpp > @@ +419,5 @@ > > + > > +bool gfxAndroidPlatform::BothAcceleratedAndNotCanvasAvailable() > > +{ > > +#ifdef MOZ_WIDGET_ANDROID > > + if (AndroidBridge::Bridge()->GetAPIVersion() < 11) { > > I think we just want to call gfxAndroidPlatform::UseAcceleratedSkiaCanvas() > here? Don't think so - that will also check the state of the preference gfx.canvas.azure.accelerated, and that's not what we want. Makes the documentation you suggest below even more important :) > ::: gfx/thebes/gfxPlatform.h > @@ +259,5 @@ > > bool SupportsAzureContentForType(mozilla::gfx::BackendType aType) { > > return BackendTypeBit(aType) & mContentBackendBitmask; > > } > > > > + virtual bool BothAcceleratedAndNotCanvasAvailable(); > > The name is weird enough that I think a comment could help. Good idea.
Carrying George's patch, none of the relevant parts changed.
Attachment #8520675 - Attachment is obsolete: true
Attachment #8522490 - Flags: review?(snorp)
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #70) > > I do still think will-read-frequently should just set the initial state. > It's a 'hint', after all, not a different "mode" for the canvas. > > That said, if we are going to switch to a GL canvas when the hint is set, we > should probably be extra-sure we want to do that, so a more sophisticated > heuristic may be necessary. For 1.0 I'm ok with never switching. Just a note here; with the patch currently in review, and "don't touch it if will-read-frequently" is set, there is (still) no way to force the canvas to start in software, and "upgrade" to HW as necessary. I agree it's a separate bug, or a continuation, but wanted to mention it.
Comment on attachment 8522490 [details] [diff] [review] Canvas observer approach cleaned up, with review comments. r=gw280,snorp Review of attachment 8522490 [details] [diff] [review]: ----------------------------------------------------------------- one nit still, but looks good ::: gfx/thebes/gfxAndroidPlatform.cpp @@ +418,5 @@ > } > + > +bool gfxAndroidPlatform::BothAcceleratedAndNotCanvasAvailable() > +{ > +#ifdef MOZ_WIDGET_ANDROID If you're not gonna call gfxPlatform::UseAcceleratedSkiaCanvas() at least factor out the API version check (which is duplicated a few lines above)
Attachment #8522490 - Flags: review?(snorp) → review+
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #76) > ... > > + > > +bool gfxAndroidPlatform::BothAcceleratedAndNotCanvasAvailable() > > +{ > > +#ifdef MOZ_WIDGET_ANDROID > > If you're not gonna call gfxPlatform::UseAcceleratedSkiaCanvas() at least > factor out the API version check (which is duplicated a few lines above) Can't call it (changes the logic), and I like the idea of factoring it out. Thanks!
(In reply to Milan Sreckovic [:milan] from comment #77) > (In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #76) > > ... > > > + > > > +bool gfxAndroidPlatform::BothAcceleratedAndNotCanvasAvailable() > > > +{ > > > +#ifdef MOZ_WIDGET_ANDROID > > > > If you're not gonna call gfxPlatform::UseAcceleratedSkiaCanvas() at least > > factor out the API version check (which is duplicated a few lines above) > > Can't call it (changes the logic), and I like the idea of factoring it out. > Thanks! I don't think I understand what this is supposed to do, actually. You want this to return whether or not it's possible to choose between accelerated and software, right? On Android, this can only happen if UseAcceleratedSkia() returns true.
Address review comments, renamed the weirdly named function, reusing the try run.
Attachment #8522490 - Attachment is obsolete: true
Attachment #8523033 - Flags: review+
Attachment #8487441 - Attachment is obsolete: true
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #78) > (In reply to Milan Sreckovic [:milan] from comment #77) > > (In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #76) > > > ... > > > > + > > > > +bool gfxAndroidPlatform::BothAcceleratedAndNotCanvasAvailable() > > > > +{ > > > > +#ifdef MOZ_WIDGET_ANDROID > > > > > > If you're not gonna call gfxPlatform::UseAcceleratedSkiaCanvas() at least > > > factor out the API version check (which is duplicated a few lines above) > > > > Can't call it (changes the logic), and I like the idea of factoring it out. > > Thanks! > > I don't think I understand what this is supposed to do, actually. You want > this to return whether or not it's possible to choose between accelerated > and software, right? On Android, this can only happen if > UseAcceleratedSkia() returns true. Scenario 1: * gfx.canvas.azure.backends set to Skia * gfx.canvas.azure.accelerated set to true * UseAcceleratedSkia() returns true * Both...() should return true Scenario 2: * gfx.canvas.azure.backends set to Skia * gfx.canvas.azure.accelerated set to false * UseAcceleratedSkia() returns false * Both...() should return true So, Both...() != UseAcceleratedSkia().
(In reply to Milan Sreckovic [:milan] from comment #80) > > Scenario 2: > * gfx.canvas.azure.backends set to Skia > * gfx.canvas.azure.accelerated set to false > * UseAcceleratedSkia() returns false > * Both...() should return true > > So, Both...() != UseAcceleratedSkia(). Why should Both...() return true here? If UseAcceleratedSkia() returns false, we shouldn't ever create an accelerated context, so we should only have software available.
Hey James, we seem to have a different understanding of what gfx.canvas.azure.accelerated set to true means. You think it means "acceleration is available and should be used", I think it means "acceleration should be used". Until we decided to introduce this "automatically choose" those two were the same. These are the scenarios that exist, and we want to decide what we want to cover: 1. Platform supports software only. 1a) Start in software, stay in software. 2. Platform supports accelerated only. 2a) Start accelerated, stay accelerated. 3. Platform supports software and accelerated. 3a) Start in software, stay in software. 3b) Start accelerated, stay accelerated. 3c) Start in software, run the heuristic, maybe switch. 3d) Start accelerated, run the heuristic, maybe switch. 1 and 2 are not really interesting, so let's focus on 3. With the patch above, this is what we have * On non-Skia platforms, we don't hit case 3 at all - we only get into this scenario when gfx.canvas.azure.backends has skia in it. Once it does: 3a) Set will-read-frequently to true 3b) gfx.canvas.azure.accelerated is true, will-read-frequently is not set, negative values for the new *.auto_accelerate.* prefs 3c) gfx.canvas.azure.accelerated is false, will-read-frequently is not set 3d) gfx.canvas.azure.accelerated is true, will-read-frequently is not set but based on the default preference settings, you're really just choosing between 3a and 3d. What should we do instead? George, what should the be the meaning of gfx.canvas.azure.accelerated pref?
Flags: needinfo?(gwright)
https://hg.mozilla.org/mozilla-central/rev/fcb277a52bdd
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Ran manual graphics smoke test, and found no issues with the overall performance. version info: Gaia-Rev 1b231b87aad384842dfc79614b2a9ca68a4b4ff3 Gecko-Rev https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/95fbd7635152 Build-ID 20141118001204 Version 34.0 Device-Name flame FW-Release 4.4.2 FW-Incremental 40 FW-Date Tue Oct 21 15:59:42 CST 2014 Bootloader L1TC10011880
Status: RESOLVED → VERIFIED
(In reply to Milan Sreckovic [:milan] from comment #82) > Hey James, we seem to have a different understanding of what > gfx.canvas.azure.accelerated set to true means. You think it means > "acceleration is available and should be used", I think it means > "acceleration should be used". Until we decided to introduce this > "automatically choose" those two were the same. > > These are the scenarios that exist, and we want to decide what we want to > cover: > > 1. Platform supports software only. > 1a) Start in software, stay in software. > 2. Platform supports accelerated only. > 2a) Start accelerated, stay accelerated. > 3. Platform supports software and accelerated. > 3a) Start in software, stay in software. > 3b) Start accelerated, stay accelerated. > 3c) Start in software, run the heuristic, maybe switch. > 3d) Start accelerated, run the heuristic, maybe switch. > > 1 and 2 are not really interesting, so let's focus on 3. With the patch > above, this is what we have > > * On non-Skia platforms, we don't hit case 3 at all - we only get into this > scenario when gfx.canvas.azure.backends has skia in it. Once it does: > > 3a) Set will-read-frequently to true > 3b) gfx.canvas.azure.accelerated is true, will-read-frequently is not set, > negative values for the new *.auto_accelerate.* prefs > 3c) gfx.canvas.azure.accelerated is false, will-read-frequently is not set > 3d) gfx.canvas.azure.accelerated is true, will-read-frequently is not set > > but based on the default preference settings, you're really just choosing > between 3a and 3d. > > What should we do instead? George, what should the be the meaning of > gfx.canvas.azure.accelerated pref? Doh, I was trying not to get involved, but I failed to see you'd needinfo'd me :) I always visioned the "accelerated" pref as "skiagl should be seen as a viable option". In fact, before we settled upon using that pref we debated whether to have "skiagl" as a separate backendtype instead in the allowable canvas backends pref. Regarding the scenarios you've laid out: 1) Spot on 2) This should never be the case. Software is always available as a fallback option even if hardware is the preferred option. If this isn't codified as a requirement, we probably should make it one. 3) 3a is the contentious one here, because I feel that if "willReadFreq" is true, we should conform to 3a above and not 3c. My understanding is that James prefers we nuke the 3a and 3b options and always either go down 3c or 3d.
Flags: needinfo?(gwright)
Does something need to land on b2g34 for v2.1 here still? If so, please request approval on the patch :)
status-b2g-v2.1: --- → affected
status-b2g-v2.2: --- → fixed
status-firefox34: --- → wontfix
status-firefox35: --- → wontfix
status-firefox36: --- → fixed
Flags: needinfo?(milan)
I'd like to revisit the 2.1+, in view of the modified plans.
Flags: needinfo?(milan) → needinfo?(bbajaj)
(In reply to Milan Sreckovic [:milan] from comment #88) > I'd like to revisit the 2.1+, in view of the modified plans. sounds good, I do not see demonstration of any real example's listed here that the patch helps(please flag me if I missed any comments that have user facing scenarios/bugs) so its fine to just ride the trains.
Flags: needinfo?(bbajaj)
blocking-b2g: 2.1+ → 2.2+
Depends on: 1149954
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: