Closed
Bug 588435
Opened 14 years ago
Closed 14 years ago
Use mozRequestAnimationFrame to animate autoscroll
Categories
(Toolkit :: UI Widgets, defect)
Toolkit
UI Widgets
Tracking
()
RESOLVED
FIXED
mozilla2.0b8
People
(Reporter: Swatinem, Assigned: Swatinem)
References
Details
Attachments
(1 file, 6 obsolete files)
10.78 KB,
patch
|
Swatinem
:
review+
benjamin
:
approval2.0+
|
Details | Diff | Splinter Review |
No description provided.
Attachment #467032 -
Flags: review?(neil)
Comment 1•14 years ago
|
||
Comment on attachment 467032 [details] [diff] [review] patch >- clearInterval(this._autoScrollTimer); >+ window.removeEventListener("MozBeforePaint", this, true); I guess you don't get to cancel the next paint, not that it matters. > switch(aEvent.type) { >+ case "MozBeforePaint": { >+ this.autoScrollLoop(); >+ break; >+ } r=me without the superfluous {}s.
Attachment #467032 -
Flags: review?(neil) → review+
Comment 2•14 years ago
|
||
I think the animation should also be made independent of the frame rate. At the moment it expects a frame rate of 50 FPS; increasing it to 60 FPS (bug 587887) will increase the scrolling speed.
Assignee | ||
Comment 3•14 years ago
|
||
Changing the speed from 12 to 10, to match the new fps. Also removing an unused prop that may be confused for the speed setting. Not sure how this could be really made independent of the frame rate, as it is not a time based animation.
Attachment #467032 -
Attachment is obsolete: true
Attachment #467944 -
Flags: review?(neil)
Assignee | ||
Comment 4•14 years ago
|
||
With your comment addressed, sorry for the inconvenience.
Attachment #467944 -
Attachment is obsolete: true
Attachment #467945 -
Flags: review?(neil)
Attachment #467944 -
Flags: review?(neil)
Updated•14 years ago
|
Attachment #467945 -
Flags: review?(neil) → review+
Assignee | ||
Updated•14 years ago
|
Attachment #467945 -
Flags: approval2.0?
Updated•14 years ago
|
Attachment #467945 -
Flags: approval2.0? → approval2.0+
Comment 5•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/634290477871
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Comment 6•14 years ago
|
||
Erm, the frame rate is *at most* 60 fps. If this isn't reached, which is often the case, you will still slow down autoscroll.
Comment 7•14 years ago
|
||
So this patch pretty much misses the point of mozRequestAnimationFrame. It doesn't guarantee a frame rate, quite the contrary. Comment 2 was seemingly addressed, but not really. I'd like to back this out -- Objections?
Comment 8•14 years ago
|
||
I'm not objecting, but I think we're not worse off than before. Even without this patch, autoscroll got slower when the timer wasn't keeping up. We'll need to make the animation independent of the frame rate either way.
Assignee | ||
Comment 9•14 years ago
|
||
So the issue here is what would happen when the browser became unresponsive and won’t be able to sustain the given frame rate. Dão claims the setInterval callbacks are queued, which means that after a hang, the scrolling offset would still be increased, which means that it suddenly jumps a long distance. mozBeforePaint does not fire when the browser is unresponsive, thus in a case of a hang, scrolling stops and restarts at the same point it left of when the browser is responsive again. Well as an end user I would rather prefer the case 2, as in it does not suddenly scroll half a page after the browser was unresponsive for a short time.
Comment 10•14 years ago
|
||
(In reply to comment #9) > Dão claims the setInterval callbacks are queued I don't think that's the case. I think it's currently the case for TYPE_REPEATING_PRECISE nsITimers but we don't use them here.
Assignee | ||
Comment 11•14 years ago
|
||
On the other hand, when the scrolling (rendering) itself slows down the browser (which I hope it won’t do any more with layers), it will indeed make the autoscroll slower too, which is not what you want.
Comment 12•14 years ago
|
||
I don't think they get queued either, but there is always the magic hidden parameter to the callback which tells you by how much the call was late. I don't think setInterval guarantees a paint between callbacks, so if anything mozRequestAnimationFrame will give a smoother scroll on hard to scroll pages.
Assignee | ||
Comment 13•14 years ago
|
||
Backed out, the tests are failing randomly on osx64. The tests are using a timeout to make sure the scroll happened at least once. We could use MozAfterPaint instead.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 14•14 years ago
|
||
AFAIK setInterval will at least try to keep up a bit. Either way, it seems that using nsITimer with the right parameter is closer to what we really want here. On hard to scroll pages, using mozRequestAnimationFrame without taking the time into account /must/ slow down scrolling; you really don't want to use the API this way.
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 years ago
Resolution: --- → FIXED
Comment 16•14 years ago
|
||
(In reply to comment #14) > Either way, it seems that > using nsITimer with the right parameter is closer to what we really want here. I disagree. We should use mozRequestAnimationFrame, but also take the elapsed time into account so that we have a constant scrolling speed. We can put a hard limit on the considered elapsed time so that we don't jump too far after a momentary freeze.
Comment 17•14 years ago
|
||
This skips up to four frames when the frame rate drops. (Skipping too many isn't good as it causes jumps that are hard to follow.)
Comment 18•14 years ago
|
||
Nice, I hadn't seen comment 16 yet when I wrote the patch... I didn't say that nsITimer would produce ideal results. It's better than listening to MozBeforePaint without taking the time into account, though, which leads to stuttering of the worst kind as soon as the frame rate drops.
Assignee | ||
Comment 19•14 years ago
|
||
Comment on attachment 475523 [details] [diff] [review] patch v3 >+ this._lastFrame = window.mozAnimationStartTime; … >+ window.mozRequestAnimationFrame(); I need to take a look into the docs, but is mozAnimationStartTime valid at all before you call mozRequestAnimationFrame? > switch(aEvent.type) { >+ case "MozBeforePaint": { >+ const now = aEvent.timeStamp; >+ const timePassed = now - this._lastFrame; >+ const desiredFrameLength = 1000/60; >+ const maxStepsPerFrame = 5; >+ const steps = Math.min(maxStepsPerFrame, >+ 1 || Math.round(timePassed / desiredFrameLength)); >+ for (let i = steps; i--; i > 0) >+ this.autoScrollLoop(); >+ this._lastFrame = now; >+ break; Would be better to move the time handling into _accelerate so we don’t execute all of autoScrollLoop all over again, it’s an expensive function. Want to take this bug?
Assignee | ||
Comment 20•14 years ago
|
||
To clarify: - const speed = 12; - var val = (curr - start) / speed; + const pixelPerMs = 0.004; + var val = delta * pixelPerMs * time; 1/12 * 50 Hz = ~4px, also the value feels right in manual tests. + // wait for more than one frame to paint + var wait = 3; + var checkScroll = function () { + if (--wait) + return; … + EventUtils.synthesizeMouse(elem, 50, 50, { button: 1 }, + gBrowser.contentWindow); + EventUtils.synthesizeMouse(elem, 100, 100, + { type: "mousemove", clickCount: "0" }, + gBrowser.contentWindow); + window.addEventListener("MozAfterPaint", checkScroll, false); I have no idea why it lags 2 frames behind, but that was the value I needed to set to get all tests passing. I have submitted this to try to see how the other platforms apart from linux are doing.
Attachment #467945 -
Attachment is obsolete: true
Attachment #475523 -
Attachment is obsolete: true
Attachment #476507 -
Flags: review?(neil)
Attachment #476507 -
Flags: feedback?
Assignee | ||
Updated•14 years ago
|
Attachment #476507 -
Flags: feedback? → feedback?(dao)
Comment 21•14 years ago
|
||
Comment on attachment 476507 [details] [diff] [review] patch4 This seems a bit jerky when scrolling slowly... > <method name="_accelerate"> >- <parameter name="curr"/> >- <parameter name="start"/> >+ <parameter name="delta"/> >+ <parameter name="time"/> > <body> > <![CDATA[ >- const speed = 12; >- var val = (curr - start) / speed; >+ const pixelPerMs = 0.004; >+ var val = delta * pixelPerMs * time; Given what happens with 'val' below, I'm not sure this is the right place for this. "pixel per ms" really means something completely different, doesn't it? > if (val > 1) > return val * Math.sqrt(val) - 1; > if (val < -1) > return val * Math.sqrt(-val) + 1; > return 0; > ]]> > </body> > </method>
Assignee | ||
Comment 22•14 years ago
|
||
There was one test failure on try on leopard. Looks like I need to increase the wait time by one.(In reply to comment #21) > This seems a bit jerky when scrolling slowly... I just reproduced that. Probably some rounding error? 60Hz means 16⅔ ms per frame. That may lead to ⅓ of the frames being under the 1px threshold. Tuning the speed factor should take care of that. > Given what happens with 'val' below, I'm not sure this is the right place for > this. "pixel per ms" really means something completely different, doesn't it? The naming is wrong, you are right.
Comment 23•14 years ago
|
||
Comment on attachment 476507 [details] [diff] [review] patch4 IMHO the animation here feels not only jerkier but also fiercer (faster scrolling for a given mouse offset).
Assignee | ||
Comment 24•14 years ago
|
||
After thinking this through again, I realized my logic was completely off. Now scrolling is the same speed, and _scrollErrorX/Y should take care of any rounding errors. Also feels smooth for small deltas. Please take a look at the test changes, there was some hacking involved to get that working correctly with AfterPaint events. Lets see how it goes on try. I wonder if the tests can be made more reliable somehow.
Attachment #476507 -
Attachment is obsolete: true
Attachment #476771 -
Flags: review?(neil)
Attachment #476771 -
Flags: feedback?(dao)
Attachment #476507 -
Flags: review?(neil)
Attachment #476507 -
Flags: feedback?(dao)
Comment 25•14 years ago
|
||
Comment on attachment 476771 [details] [diff] [review] patch5 > <method name="autoScrollLoop"> >+ <parameter name="timestamp"/> > <body> > <![CDATA[ >+ // avoid long jumps when the browser hangs for more than >+ // |maxTimeDelta| ms >+ const maxTimeDelta = 250; 250 seems too high to me.
Assignee | ||
Comment 26•14 years ago
|
||
Tests on try are green except for windows-debug, but that seems to be an unrelated failure.
Status: REOPENED → ASSIGNED
Comment 27•14 years ago
|
||
Comment on attachment 476771 [details] [diff] [review] patch5 Looks right to me, except that maxTimeDelta is probably way too high. I'd try 80 or so.
Attachment #476771 -
Flags: feedback?(dao) → feedback+
Comment 28•14 years ago
|
||
Comment on attachment 476771 [details] [diff] [review] patch5 >- <field name="_autoScrollTimer">null</field> ... >+ delete this._lastFrame; I'd have a slight preference to make _lastFrame a field instead of deleting it. Apart from that, I didn't notice any difference to autoscrolling when the patch was applied. At least, I think I had it applied ;-)
Attachment #476771 -
Flags: review?(neil) → review+
Assignee | ||
Comment 29•14 years ago
|
||
(In reply to comment #28) > Comment on attachment 476771 [details] [diff] [review] > >+ delete this._lastFrame; > I'd have a slight preference to make _lastFrame a field instead of deleting it. Done. (In reply to comment #27) > Comment on attachment 476771 [details] [diff] [review] > Looks right to me, except that maxTimeDelta is probably way too high. I'd try > 80 or so. I made it 100, which is exactly 6 frames. Thanks a lot for catching my earlier mistakes.
Attachment #476771 -
Attachment is obsolete: true
Attachment #478750 -
Flags: review+
Attachment #478750 -
Flags: approval2.0?
Comment 30•14 years ago
|
||
Comment on attachment 478750 [details] [diff] [review] ready to land I don't think you need to re-request approval. Attachment 467945 [details] [diff] has already been approved.
Attachment #478750 -
Flags: approval2.0?
Updated•14 years ago
|
Keywords: checkin-needed
Whiteboard: [can land]
Comment 31•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/e3ce3780f870 Thanks for the patch. I should have checked the bug number was in the commit message, but including the bug number in ready-to-land patches would save others from having to add it.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago → 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [can land]
Target Milestone: --- → mozilla2.0b8
Comment 32•14 years ago
|
||
(In reply to comment #30) > Comment on attachment 478750 [details] [diff] [review] > ready to land > > I don't think you need to re-request approval. Attachment 467945 [details] [diff] has already > been approved. This isn't correct. Approval is for specific patches and in general doesn't carry over except maybe for small bustage or nit fixes. Approvals for the current release should also be assumed to be fairly time limited as the tree rules and things that are allowed to land change rapidly. The approval on this bug was given 2 months ago for a patch that is fairly different to the one that landed, it doesn't apply. I've backed this out on the basis that it essentially landed without approval. If you want to re-land it then please re-request approval and give a good summary of what the benefits/risks are of taking this at this point and whether the changes to the fields on the binding constitute an API change.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 33•14 years ago
|
||
Comment on attachment 478750 [details] [diff] [review] ready to land The patch improves the smoothness of the autoscroll animation by using the same frame rate as the refresh driver and reduces the cpu load in the case of stalled animation. The patch has passing tests and is thus low risk. The removed binding field was unused inside the code (I don’t know about extensions) so removing it seems fine.
Attachment #478750 -
Flags: approval2.0?
Updated•14 years ago
|
Attachment #478750 -
Flags: approval2.0? → approval2.0+
Assignee | ||
Comment 34•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/3b157fcde708 http://hg.mozilla.org/mozilla-central/rev/86f686735122 Stupid me, haven’t commited in such a long time I forgot the bug number :D
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•