If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

While reducing overscroll, we don't composite enough

RESOLVED FIXED in Firefox 32, Firefox OS v2.0
(NeedInfo from)

Status

()

Core
Panning and Zooming
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: kats, Assigned: kats, NeedInfo)

Tracking

Trunk
mozilla33
All
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(firefox31 wontfix, firefox32 fixed, firefox33 fixed, b2g-v1.4 unaffected, b2g-v2.0 fixed, b2g-v2.1 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

STR:
- Load some scrollable page in the browser (e.g. https://staktrace.com)
- Pull down to go into overscroll
- Slowly scroll back to reduce overscroll

Expected:
- We composite frequently so that you can see the overscroll reducing a pixel at a time

Actual:
- We composite infrequently, and the overscroll seems to jank, reducing a number of pixels at a time.

Looking at the code, I believe this is because the call to Axis::AdjustDisplacement silently reduces the overscroll amount without notifying the call site in AsyncPanZoomController. Therefore we don't call ScheduleComposite as frequently as we should.
Assignee: nobody → bugmail.mozilla
status-b2g-v1.4: --- → unaffected
status-b2g-v2.0: --- → affected
status-b2g-v2.1: --- → affected
Created attachment 8449481 [details] [diff] [review]
Patch
Comment on attachment 8449481 [details] [diff] [review]
Patch

I'm not really happy with the way this code has evolved, any suggestions on how to better structure it are welcome.
Attachment #8449481 - Attachment description: WIP → Patch
Attachment #8449481 - Flags: review?(drs+bugzilla)
Comment on attachment 8449481 [details] [diff] [review]
Patch

Review of attachment 8449481 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, I agree, this could really use some rethinking. My thoughts on this basically center around the distinction between increasing and decreasing overscroll while panning; I can't think of a reason why they couldn't take the same code path, if this is done correctly.

In particular, I think this could be written more cleverly:
http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp#1622

We could just consume overscroll regardless of which direction we're going. So for example, if we're overscrolled and we slowly pan back to the quiescent state (as in the STR), we could consume "negative overscroll" here:
http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/Axis.cpp#65

Do you have any thoughts on this?
Attachment #8449481 - Flags: review?(drs+bugzilla)
Not sure I'm following what you're suggesting.

(In reply to Doug Sherk (:drs) from comment #3)
> So for example, if we're overscrolled and we slowly pan back to the
> quiescent state (as in the STR), we could consume "negative overscroll" here:
> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/Axis.cpp#65

This is what is happening with the current code in m-c, no?

(In reply to Doug Sherk (:drs) from comment #3)
> Yeah, I agree, this could really use some rethinking. My thoughts on this
> basically center around the distinction between increasing and decreasing
> overscroll while panning; I can't think of a reason why they couldn't take
> the same code path, if this is done correctly.

So yes, I would also like to unify the "increase overscroll" versus the "decrease overscroll" paths. However there is a distinction between these two that we need to respect, which is that when increasing overscroll we might have to walk the handoff chain to find the right APZ and while decreasing overscroll we need to operate directly on the APZ we're in. I'm not sure if we can restructure the code to avoid this.

The other thing to keep in mind is that AdjustDisplacement can provide either a negative or a positive overscroll amount, depending if we're overscrolling at the top or the bottom of the page. That might affect how many variables/conditions we need in order to capture all the relevant state.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #4)
> This is what is happening with the current code in m-c, no?

Do we get back an allowedDisplacement? I assume not because we'd immediately get that ScheduleCompositeAndMaybeRepaint() call if we did.

> So yes, I would also like to unify the "increase overscroll" versus the
> "decrease overscroll" paths. However there is a distinction between these
> two that we need to respect, which is that when increasing overscroll we
> might have to walk the handoff chain to find the right APZ and while
> decreasing overscroll we need to operate directly on the APZ we're in. I'm
> not sure if we can restructure the code to avoid this.

It looks like the code responsible for handing off the overscroll happens after we know everything about the overscroll, so we could just special case it to not try to hand off in the "consuming negative overscroll" case:
http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp#1639

> The other thing to keep in mind is that AdjustDisplacement can provide
> either a negative or a positive overscroll amount, depending if we're
> overscrolling at the top or the bottom of the page. That might affect how
> many variables/conditions we need in order to capture all the relevant state.

Yeah, that's true, but as long as we keep most of that logic in Axis, I don't think it'll be too bad. Anyways, if you don't want to try my suggestion, we can just file a followup and you can flag me for review on this again. I can try to think of smaller changes to make this clearer. I think it's worth a shot though.
(In reply to Doug Sherk (:drs) from comment #5)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #4)
> > This is what is happening with the current code in m-c, no?
> 
> Do we get back an allowedDisplacement? I assume not because we'd immediately
> get that ScheduleCompositeAndMaybeRepaint() call if we did.

With the STR I described, we get back 0 for both the allowed displacement and the overscroll values.

> It looks like the code responsible for handing off the overscroll happens
> after we know everything about the overscroll, so we could just special case
> it to not try to hand off in the "consuming negative overscroll" case:
> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/
> AsyncPanZoomController.cpp#1639
> 
> > The other thing to keep in mind is that AdjustDisplacement can provide
> > either a negative or a positive overscroll amount, depending if we're
> > overscrolling at the top or the bottom of the page. That might affect how
> > many variables/conditions we need in order to capture all the relevant state.
> 
> Yeah, that's true, but as long as we keep most of that logic in Axis, I
> don't think it'll be too bad. Anyways, if you don't want to try my
> suggestion, we can just file a followup and you can flag me for review on
> this again. I can try to think of smaller changes to make this clearer. I
> think it's worth a shot though.

Maybe I'm still suffering from PTO-brain or something but I don't understand what you're actually suggesting to do code-wise. Do you mind throwing in some pseudocode to explain what you mean?
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #6)
> Maybe I'm still suffering from PTO-brain or something but I don't understand
> what you're actually suggesting to do code-wise. Do you mind throwing in
> some pseudocode to explain what you mean?

I'll take a stab at it later.
Comment on attachment 8449481 [details] [diff] [review]
Patch

Review of attachment 8449481 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, I've got nothing. I didn't understand the code enough when I wrote my comment. After messing around for a bit I think this is the best path without stepping back and seriously rethinking it. Thanks for waiting for me to try it.

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +529,5 @@
> +    // Ordinarily we might need to do a ScheduleComposite if either of
> +    // the following AdjustDisplacement calls returns true, but this
> +    // is already running as part of a FlingAnimation, so we'll be scheduling
> +    // a composite per frame anyway when we return true at the end of this
> +    // function.

We're referring to a spot in the code which is fairly far away from this comment. Maybe we should add another comment there.

::: gfx/layers/apz/src/Axis.cpp
@@ +90,2 @@
>    }
> +  return consumedOverscroll != 0;

I don't think this is safe if it's set anywhere other than on initialization to ~0. Regardless that I don't think that can happen, it would be slightly more future-proof to use something like |return fabsf(consumedOverscroll) < EPSILON;| instead.

::: gfx/layers/apz/src/Axis.h
@@ +89,2 @@
>     */
> +  bool AdjustDisplacement(float& aDisplacementInOut, float& aOverscrollAmountOut);

I think it's a bit awkward to use a parameter as both an in and out parameter because it makes the client code really confusing, especially when it's a reference and we're not making it clear from the call site that is passing a pointer. Maybe we should separate this into two parameters: an in, and an out.
Created attachment 8450540 [details] [diff] [review]
Patch v2

Updated as per comments. I'm not really convinced that having two parameters instead of the in/out parameter is really better. C++ is just annoying at times like these.
Attachment #8449481 - Attachment is obsolete: true
Attachment #8450540 - Flags: review?(drs+bugzilla)
Created attachment 8450541 [details] [diff] [review]
Patch v2

Forgot to qref
Attachment #8450540 - Attachment is obsolete: true
Attachment #8450540 - Flags: review?(drs+bugzilla)
Attachment #8450541 - Flags: review?(drs+bugzilla)
Comment on attachment 8450541 [details] [diff] [review]
Patch v2

Review of attachment 8450541 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +1615,5 @@
>      CSSToScreenScale zoom = mFrameMetrics.GetZoom();
>  
>      // Inversely scale the offset by the resolution (when you're zoomed further in,
>      // the same swipe should move you a shorter distance).
>      CSSPoint cssDisplacement = displacement / zoom;

It would be more clear to add a new variable, perhaps "cssAdjustedDisplacement" or something, for the out variable. Same for cssOffset above.
Attachment #8450541 - Flags: review?(drs+bugzilla) → review+
Changed and landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/60051d7558c4
https://hg.mozilla.org/mozilla-central/rev/60051d7558c4
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
status-b2g-v2.1: affected → fixed
Comment on attachment 8450541 [details] [diff] [review]
Patch v2

Approval Request Comment
[Feature/regressing bug #]: overscroll implementation
[User impact if declined]: after dragging content into overscroll, slowly reducing the overscroll (by dragging the content back into the normal position) doesn't render smoothly - the animation is very janky
[Describe test coverage new/current, TBPL]: tested locally
[Risks and why]: fairly low-risk. affects b2g only. most of the patch is rearranging code so i insert the one-line change that actually fixes the bug.
[String/UUID change made/needed]: none
Attachment #8450541 - Flags: approval-mozilla-aurora?
Is there any way to automate testing of overscroll? Can we use Eideticker for this?
Flags: needinfo?(bugmail.mozilla)
Kats - Now that the fix is on m-c, can you confirm that the fix is good there before uplifting to Aurora?
(In reply to Lawrence Mandel [:lmandel] from comment #15)
> Is there any way to automate testing of overscroll? Can we use Eideticker
> for this?

We might be able to, yeah.

(In reply to Lawrence Mandel [:lmandel] from comment #16)
> Kats - Now that the fix is on m-c, can you confirm that the fix is good
> there before uplifting to Aurora?

Yup, the fix is good on m-c.
Flags: needinfo?(bugmail.mozilla)
Comment on attachment 8450541 [details] [diff] [review]
Patch v2

Kats has confirmed the fix on m-c. Aurora+

Kats - Do you want to file a follow-up bug for testing overscroll behaviour?
Attachment #8450541 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Filed bug 1034733 for testing using Eideticker.
https://hg.mozilla.org/releases/mozilla-aurora/rev/3f9d7a3a0b7b
status-b2g-v2.0: affected → fixed
status-firefox31: --- → wontfix
status-firefox32: --- → fixed
status-firefox33: --- → fixed
Hi Hubert,

    I'm not very sure expected results, please help to confirm it.

Thank you very much!


This issue has been successfully verified on Flame v2.1&2.0 through these steps.
See attachment: verified_v2.1.mp4.
Reproduce rate: 0/5.

STR:
1. Load some scrollable page in the Browser app(e.g. https://staktrace.com ->tap "Blog").
2. Pull down frequently to go into overscroll.
**The overscroll reduce a pixel at a time.
3. Slowly scroll back to reduce overscroll to browse.
**The overscroll does not reduce a pixel and user can browse the content clearly.

Flame 2.1 build:
Gaia-Rev        38e17b0219cbc50a4ad6f51101898f89e513a552
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/8b92c4b8f59a
Build-ID        20141205001201
Version         34.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141205.035305
FW-Date         Fri Dec  5 03:53:16 EST 2014
Bootloader      L1TC00011880

Flame 2.0 build:
Gaia-Rev        856863962362030174bae4e03d59c3ebbc182473
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/e40fe21e37f1
Build-ID        20141207000206
Version         32.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141207.034341
FW-Date         Sun Dec  7 03:43:52 EST 2014
Bootloader      L1TC00011880
Flags: needinfo?(hlu)
Created attachment 8533077 [details]
verified_v2.1.mp4
You need to log in before you can comment on or make changes to this bug.