Closed Bug 588435 Opened 14 years ago Closed 14 years ago

Use mozRequestAnimationFrame to animate autoscroll

Categories

(Toolkit :: UI Widgets, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0b8

People

(Reporter: Swatinem, Assigned: Swatinem)

References

Details

Attachments

(1 file, 6 obsolete files)

Attached patch patch (obsolete) — Splinter Review
      No description provided.
Attachment #467032 - Flags: review?(neil)
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+
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.
Attached patch patch2 (obsolete) — Splinter Review
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)
Attached patch patch2 (obsolete) — Splinter Review
With your comment addressed, sorry for the inconvenience.
Attachment #467944 - Attachment is obsolete: true
Attachment #467945 - Flags: review?(neil)
Attachment #467944 - Flags: review?(neil)
Attachment #467945 - Flags: review?(neil) → review+
Attachment #467945 - Flags: approval2.0?
Attachment #467945 - Flags: approval2.0? → approval2.0+
http://hg.mozilla.org/mozilla-central/rev/634290477871
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
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.
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?
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.
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.
(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.
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.
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.
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 → ---
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 ago14 years ago
Resolution: --- → FIXED
argh...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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.
Attached patch patch v3 (obsolete) — Splinter Review
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.)
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.
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?
Attached patch patch4 (obsolete) — Splinter Review
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?
Attachment #476507 - Flags: feedback? → feedback?(dao)
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>
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 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).
Attached patch patch5 (obsolete) — Splinter Review
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 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.
Tests on try are green except for windows-debug, but that seems to be an unrelated failure.
Status: REOPENED → ASSIGNED
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 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+
Attached patch ready to landSplinter Review
(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 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?
Keywords: checkin-needed
Whiteboard: [can land]
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 ago14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [can land]
Target Milestone: --- → mozilla2.0b8
(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 → ---
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?
Attachment #478750 - Flags: approval2.0? → approval2.0+
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 ago14 years ago
Resolution: --- → FIXED
Depends on: 614643
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: