Closed Bug 976035 Opened 10 years ago Closed 10 years ago

Add a preffable velocity cap when flinging


(Core :: Panning and Zooming, defect)

Gonk (Firefox OS)
Not set



blocking-b2g 1.4+
Tracking Status
firefox28 --- wontfix
firefox29 --- wontfix
firefox30 --- fixed
b2g-v1.4 --- fixed


(Reporter: kats, Assigned: bkelly)



(Keywords: perf, Whiteboard: [c= p= s= u=])


(2 files, 1 obsolete file)

Spinoff from - Ben Kelly suggested putting a velocity cap to limit how fast we can scroll, thereby limiting the chances of running into checkerboarding. Seems like a reasonable idea so I'm spinning out a dedicated bug for it.
Thanks for filing kats!  I'll take this since its simple enough for me to do and I imagine you guys are busy with heavier lifting in gfx for tiling, etc.
Assignee: nobody → bkelly
Keywords: perf
Whiteboard: [c= p= s= u=]
Initial patch introducing the velocity limitation and preference.  I'm going to see if I can write a gtest for this before asking for review.
Add the preference to b2g so it can be easily tuned.  Took me a bit to realize float values must be specified as strings in the file.
Comment on attachment 8380656 [details] [diff] [review]

Review of attachment 8380656 [details] [diff] [review]:

::: gfx/layers/ipc/Axis.cpp
@@ +96,5 @@
>  }
>  void Axis::UpdateWithTouchAtDevicePoint(int32_t aPos, const TimeDuration& aTimeDelta) {
>    float newVelocity = mAxisLocked ? 0 : (mPos - aPos) / aTimeDelta.ToMilliseconds();
> +  float cappedVelocity = std::min(newVelocity, gMaxVelocity);

I'd like there to be a way to explicitly disable it too. I think if gMaxVelocity < 0 then just use newVelocity without capping it. Also make gMaxVelocity -1 by default so that on non-b2g platforms no pref changes are needed to keep the current behaviour. b2g.js can set it to 6.0 to cap it on b2g.
Excellent point.  Updated to treat negative values as unlimited velocity.  Defaults to unlimited velocity.
Attachment #8380656 - Attachment is obsolete: true
Comment on attachment 8380709 [details] [diff] [review]
bug976035_part1_max_velocity.patch (v2)

Review of attachment 8380709 [details] [diff] [review]:

Pre-emptive r+. It might be a bit hard to write a test for this code so I'd rather land the patch now and worry about that later.
Attachment #8380709 - Flags: review+
Comment on attachment 8380700 [details] [diff] [review]

Review of attachment 8380700 [details] [diff] [review]:

Would be good to check if this is suitable for higher DPI devices like the nexus 4 and nexus 5 as well.
Attachment #8380700 - Flags: review+
Try build before pushing:

Also starting a nexus4 build locally to test there.  Since I have never flashed this nexus4, though, I probably will not hold up landing this if I run into problems with the device.
Nom for 1.4? to help mitigate checkerboarding issues.
blocking-b2g: --- → 1.4?
blocking-b2g: 1.4? → 1.4+
Try was green.  Still working on my nexus-4, but even if the setting is not quite right there I don't think we want to hold it back from our main release phones.  Ultimately we probably need a device-specific pref strategy, but thats a separate bug.

Landed in b2g-inbound:

Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Note, scrolling does seem a bit slow with this in place on the nexus-4.  Perhaps we need to adjust this to be something like device-screens/second and then calculate the corresponding pixels/ms.

Kats, what do you think?
Flags: needinfo?(bugmail.mozilla)
Kats away until 3/6.  BenWa, Botond?
(In reply to Ben Kelly [:bkelly] from comment #12)
> Note, scrolling does seem a bit slow with this in place on the nexus-4. 
> Perhaps we need to adjust this to be something like device-screens/second
> and then calculate the corresponding pixels/ms.

That sounds reasonable to me. I'm happy to review a patch that does this.
Thanks.  I'll write a new bug up tonight.
Blocks: 979720
Flags: needinfo?(bugmail.mozilla)
You need to log in before you can comment on or make changes to this bug.