Closed
Bug 757680
Opened 12 years ago
Closed 12 years ago
Too few touch events dispatched for drawing application
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(blocking-fennec1.0 -, fennec+)
RESOLVED
FIXED
Firefox 19
People
(Reporter: birtles, Assigned: kats)
References
()
Details
Attachments
(4 files)
We have a drawing application that uses touch events for drawing line art using SVG (see URL). The touch event handler adds points to an SVG polyline (the smoothing of the polyline into a path takes place on touchend). On a Toshiba Regza AT200 running Android 3.2 the drawing is smooth when running Fennec 10. However, on Fennec 14 far fewer touch events appear to be dispatched and the intermediate polyline is very jagged and the resulting path is very inaccurate. If I get a chance I'll try to produce a reduced version of this test. For now, the source in question is here: https://github.com/mozilla/parapara/blob/fa8904d949b7cd94fdf56803648971cdc4f47422/editor/lib/js/parapara.js#L321 We've been using this tool with Fennec 10 for a while (and its been working great, thanks!) and we plan to use it as part of the Mozilla Webmaker Summer Code Party (starting 23 June) and Osaka Firefox Developer Conference (30 June) so I'll do my best to contribute to fixing this soon.
Reporter | ||
Updated•12 years ago
|
OS: Windows 7 → Android
Hardware: x86_64 → All
Reporter | ||
Comment 1•12 years ago
|
||
CC's wesj and kats since they were so helpful earlier on and I didn't get to post the bug before they disappeared.
Reporter | ||
Comment 2•12 years ago
|
||
This is a somewhat reduced version of the application. The difference is not as pronounced as the actual application (which suggests that it may, in part, be related to the complexity of the touch event handlers) but still very noticeable. Try drawing some curvy lines very quickly first in Fennec 10 on a tablet, then Fennec 14. I see much more jagged lines in Fennec 14 suggesting fewer touch events are being delivered.
Assignee | ||
Comment 3•12 years ago
|
||
What device were you testing on? On the Galaxy Nexus the test case works great. http://people.mozilla.org/~kgupta/tmp/circle.png
Reporter | ||
Comment 4•12 years ago
|
||
Toshiba Regza AT200 running Android 3.2
Reporter | ||
Comment 5•12 years ago
|
||
(In reply to Kartikaya Gupta (:kats) from comment #3) > On the Galaxy Nexus the test case works great. How does the URL on the bug look? That is, the fully-fledged app?
Assignee | ||
Comment 6•12 years ago
|
||
Not bad, it's a little more jagged than the test version but still quite good. https://people.mozilla.com/~kgupta/tmp/parapara.png
Assignee | ||
Comment 7•12 years ago
|
||
I also tried it on the Galaxy Tab 10.1, which seems very similar in specifications to the Toshiba Regza, and it was noticeably worse there. This is what I ended up with when I tried drawing circles quickly: https://people.mozilla.com/~kgupta/tmp/parapara-tab.jpg
Assignee | ||
Comment 8•12 years ago
|
||
The simplified test case works much better on the Galaxy Tab. https://people.mozilla.com/~kgupta/tmp/circle-tab.jpg Since the Galaxy Tab hardware should be generating events at the same rate for the two pages, I suspect it's just the extra JS computation that's causing the jaggedness on the full version. The stock browser on the Galaxy Tab does better on both pages than we do; either they are not coalescing touch events, or they are executing scripts faster, or both.
Reporter | ||
Comment 9•12 years ago
|
||
Here's what I'm seeing on the Regza: Fennec 10, reduced test case: http://people.mozilla.org/~bbirtles/touch-testing/reduced-f10.jpg Fennec 14, reduced test case: http://people.mozilla.org/~bbirtles/touch-testing/reduced-f14.jpg Fennec 10, full app: http://people.mozilla.org/~bbirtles/touch-testing/full-f10.jpg Fennec 14, full app: http://people.mozilla.org/~bbirtles/touch-testing/full-f14.jpg As you can see from the last shot, despite drawing the same pattern the result looks completely different. The app is unuseable in this case. What the next step for tracking down the cause here?
Comment 10•12 years ago
|
||
I'd be surprised if the problem is our JS. We're usually on par, if not faster than stock, and should be the same (if not faster) in Native vs. XUL. My first instinct would be to look at the coalescing and see what's going in vs coming out, but kats might have some better ideas.
Assignee | ||
Comment 11•12 years ago
|
||
So event coalescing is definitely what's going on here. I turned on logging for when it happens and a lot of events are getting coalesced. Turning off event coalescing makes the circles much more circular, although there's definitely some noticeable processing lag. Next step is to see what we're doing that causes the events to get backed up in the queue and coalesced.
Assignee | ||
Comment 12•12 years ago
|
||
It seems like one of the first draw events we run takes a bunch of time (I'm seeing ~180ms in the trace I generated) and during that time 8 touch-move events get coalesced. Later there's another ~180ms draw event that takes out a couple more touch-move events. I don't see anything particularly abnormal happening here; things are working more or less as we designed them except that the draws are taking a really long time and that degrades the number of touch events the page receives. On most pages this is the right tradeoff, but obviously for a drawing app this is not good. Next step is probably to profile the draw and see why it's taking so long.
Comment 13•12 years ago
|
||
Personally, I'd like to make this a priority before we ship. Drawing apps are common on mobile devices, and I'd like to be good at them.
Updated•12 years ago
|
tracking-fennec: --- → ?
Reporter | ||
Comment 14•12 years ago
|
||
(In reply to Kartikaya Gupta (:kats) from comment #12) > Next step is probably to profile the draw and see why it's taking so long. That would be good. Although there are things that can be done to speed up the app in question and SVG in general, I wonder how much they'll help with the delay from the initial draw event.
Reporter | ||
Comment 15•12 years ago
|
||
This is a variant on the original test case. The original test case uses setAttribute to build up the list of points. As the line gets longer, so does the string and the amount of processing required to add each point. The alternative is to use the SVG DOM and just append points directly to the line. Prior to bug 629200 (which landed in mozilla13), however, the SVG DOM was very slow for this. This test case makes use of the SVG DOM. It will be somewhat slower in Fennec 10 than the previous test case (due to bug 629200). For Fennec 14, however, it should be slightly faster. In my testing, however, it doesn't make any noticeable difference. I'm including it here simply to eliminate one possible cause of delay, namely, poor usage of the DOM. (Note that even if this were to help, there are still many apps that rely on setAttribute, such as SVG Edit.)
Updated•12 years ago
|
tracking-fennec: ? → 15+
blocking-fennec1.0: ? → -
Reporter | ||
Comment 17•12 years ago
|
||
At the Firefox Devcon in Osaka at the end of June (June 30 to be precise) we'd like to get everyone in the audience to fire up Fennec on their smartphones and participate in a group drawing session (using the Parapara animation workshop concept). This bug is currently blocking us from doing that (unless we tell everyone to install Fennec 10). If there are any workarounds we can implement in JS for this then I'd love to hear them! Thanks! (And if we can fix this by June 30 I'll hand-deliver as many random Japanese sweet treats as I can carry to the Toronto office at the gfx/layout work week in July. :)
Assignee | ||
Comment 18•12 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #17) > At the Firefox Devcon in Osaka at the end of June (June 30 to be precise) > we'd like to get everyone in the audience to fire up Fennec on their > smartphones and participate in a group drawing session (using the Parapara > animation workshop concept). Will they be using phones or tablets? Note that currently Firefox Beta from the Google Play store will install the XUL version on tablets, and this problem might only affect tablets. (Well, it probably affects lower-end phones as well but it didn't affect the Galaxy Nexus). It would be good if we could get an idea of how much of an improvement is needed to satisfy your requirements on the target devices. (I don't really have a good metric for this, but maybe we could create a test page where you have to draw a line as quickly as you can, and the page ensures some minimum number of touch events are received to pass the test). That being said, I was thinking about this and I wanted to try disabling coalescing of touch events in cases where Gecko is processing events other than touch events. This would result in us only delivering fewer touch events to pages that take a long time to process those touch events, and not penalize pages in cases where our drawing is slow. I'll give that a whirl today.
Comment 19•12 years ago
|
||
(In reply to Kartikaya Gupta (:kats) from comment #18) > ... note that currently Firefox Beta from > the Google Play store will install the XUL version on tablets, and this > problem might only affect tablets Drive by comment; currently tablets are blocked for Firefox Beta on Google Play (bug 759445).
Assignee | ||
Comment 20•12 years ago
|
||
I tried un-coalescing as I described in comment 18 but it ends up still being very laggy. We definitely need to figure out what we're drawing that's taking so long and how we can make it faster.
Comment 21•12 years ago
|
||
Since the app is adding SVG elements to the DOM, would that cause Fennec to start a screenshot? If so, maybe we are doing too much work. A logcat would show the screenshot message, if it's happening.
Assignee | ||
Comment 22•12 years ago
|
||
I don't see us taking an unreasonable number of screenshots. Most of the slow events are of type 7 (DRAW events).
Assignee | ||
Comment 23•12 years ago
|
||
Here is one profile I pulled off while drawing random shapes in the full app (the one in the bug URL): http://people.mozilla.com/~bgirard/cleopatra/?report=AMIfv961WOKuR9fBMI6Oy7p9FlDyTcuDWXIVE-66OBzH0QgBcmQWTyUZ1YERcmRiKbfh9GKcnH3Rvb-5Oprfzu2ub84duU6Tul5nkut-VomcMmqSJTdQ_CX3wUjvh43tgOrFetjDEO5JCZeh5H8944ciS22dO5sP1A It's spending 32% of the time painting, mostly drawing the root layer. I don't really know what that means (cc'ing :jrmuizel for help here) but if I had to guess I would say that we're not doing partial invalidation of the SVG and we're redrawing the whole thing every time?
Assignee | ||
Comment 24•12 years ago
|
||
(Also note my profile is from a debug build, so should be taken with appropriate grains of salt)
Comment 25•12 years ago
|
||
(In reply to Kartikaya Gupta (:kats) from comment #24) > (Also note my profile is from a debug build, so should be taken with > appropriate grains of salt) The symbols are wrong in this profile and debug build profiles are not always that easy to reason about...
Reporter | ||
Comment 26•12 years ago
|
||
(In reply to Kartikaya Gupta (:kats) from comment #18) > (In reply to Brian Birtles (:birtles) from comment #17) > > At the Firefox Devcon in Osaka at the end of June (June 30 to be precise) > > we'd like to get everyone in the audience to fire up Fennec on their > > smartphones and participate in a group drawing session (using the Parapara > > animation workshop concept). > > Will they be using phones or tablets? Phones primarily. Also, I'm seeing the same symptoms on my phone (HTC J, HTC Sense 4.0). > It would be good if > we could get an idea of how much of an improvement is needed to satisfy your > requirements on the target devices. (I don't really have a good metric for > this, but maybe we could create a test page where you have to draw a line as > quickly as you can, and the page ensures some minimum number of touch events > are received to pass the test). Yes, this is hard to spec. We really want the audience to load up the app on their own phones so the target devices are really anything :) From what I'm seeing on my phone, I think I'd need to see at least double the number of events on my phone to get acceptably smooth drawing, triple would be ideal. (In reply to Kartikaya Gupta (:kats) from comment #20) > I tried un-coalescing as I described in comment 18 but it ends up still > being very laggy. We definitely need to figure out what we're drawing that's > taking so long and how we can make it faster. Oh, that's disappointing. :( The performance is fine in Fennec 10 so it doesn't seem like a device limitation.
Reporter | ||
Comment 28•12 years ago
|
||
(In reply to Mark Finkle (:mfinkle) from comment #27) > Brian - How does this work in Fx17? Any better? I'm afraid not. I just tested with Fx10, Fx13, Fx 15, and Fx17. Performs smoothly in Fx10 and Fx13 and is jagged in Fx15 and Fx17. There's no obvious difference between Fx15 and Fx17.
Comment 29•12 years ago
|
||
cc'ing dholbert to see if his svg skillz can help. I'm tempted to remove the touchmove coalescing. Its way too late for Brian's use, but we could pref it for testing at least...
Reporter | ||
Comment 30•12 years ago
|
||
(In reply to Wesley Johnston (:wesj) from comment #29) > I'm tempted to remove the touchmove coalescing. Its way too late for Brian's > use, but we could pref it for testing at least... I'd like to at least run a test with touchmove coalescing turned off to see if that solves it. We'll probably be using this app often in the future so I'm still interested in finding a solution.
Comment 31•12 years ago
|
||
This adds a pref for touch event coalescing.
Attachment #650367 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 32•12 years ago
|
||
Comment on attachment 650367 [details] [diff] [review] Patch Review of attachment 650367 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/android/nsAppShell.cpp @@ +262,5 @@ > if (NS_FAILED(rv)) { > rv = Preferences::GetString(PREFNAME_UA_LOCALE, &locale); > } > > + mAllowCoalescingTouches = Preferences::GetBool(PREFNAME_COALESCE_TOUCHES, true); Move this line down a couple of lines, so it goes after the bridge->SetSelectedLocale call and before the return rv; @@ +307,5 @@ > } > > bridge->SetSelectedLocale(locale); > + > + mAllowCoalescingTouches = Preferences::GetBool(PREFNAME_COALESCE_TOUCHES, true); Not part of this bug, but this duplicated code for reading prefs should be refactored...
Attachment #650367 -
Flags: review?(bugmail.mozilla) → review+
Comment 33•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ba91b3e9f38 I defaulted this coalesce Brian, but you can change it on about:config dom.events.touch.coalescing.enabled (I added it to our defaults so it should be easy to find with a search). I'll try running with it for a weekend and if things seems ok maybe we can turn it off?
Whiteboard: [leave open]
Assignee | ||
Comment 35•12 years ago
|
||
Ping. Brian, did you find that turning off the coalescing helped things?
Flags: needinfo?(birtles)
Reporter | ||
Comment 36•12 years ago
|
||
(In reply to Kartikaya Gupta (:kats) from comment #35) > Ping. Brian, did you find that turning off the coalescing helped things? Thanks, sorry I hadn't commented on this yet. Yes, it helps tremendously. Performance is much better. Thank you very much!
Flags: needinfo?(birtles) needinfo?(birtles) → needinfo+
Assignee | ||
Comment 37•12 years ago
|
||
Great. :wesj, did you try running with the coalescing off? I'll give it a shot on the lowest-end phone that I have and see if it's noticeably worse. Either way we should close this bug out once we decide whether to leave it on or off by default.
Assignee | ||
Comment 38•12 years ago
|
||
I tried it on a Galaxy Q on a variety of sites and didn't see any observable slowdowns. It might still impact sites that run a lot of script for each touch event but I'd like to turn it off by default and then see if people complain.
Assignee: nobody → bugmail.mozilla
Attachment #673220 -
Flags: review?(wjohnston)
Assignee | ||
Updated•12 years ago
|
Attachment #650367 -
Flags: checkin+
Comment 39•12 years ago
|
||
Comment on attachment 673220 [details] [diff] [review] Turn off coalescing by default Review of attachment 673220 [details] [diff] [review]: ----------------------------------------------------------------- I was thinking the same thing yesterday.
Attachment #673220 -
Flags: review?(wjohnston) → review+
Assignee | ||
Comment 40•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0cc577753007
Whiteboard: [leave open]
Assignee | ||
Updated•12 years ago
|
Attachment #673220 -
Flags: checkin+
Comment 41•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0cc577753007
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•