1033398 - While reducing overscroll, we don't composite enough

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch

Comment 2

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 3

[Doug Sherk (:drs) (inactive)]

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?

Comment 4

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 5

[Doug Sherk (:drs) (inactive)]

(In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

(In reply to Doug Sherk (:drs) from comment #5)
> (In reply to Kartikaya Gupta (email:[e-mail]) 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?

Comment 7

[Doug Sherk (:drs) (inactive)]

(In reply to Kartikaya Gupta (email:[e-mail]) 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 8

[Doug Sherk (:drs) (inactive)]

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.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Attachment: 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.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch v2

Forgot to qref

Comment 11

[Doug Sherk (:drs) (inactive)]

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.

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Changed and landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/60051d7558c4

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/60051d7558c4

Comment 14

[Kartikaya Gupta (email:[e-mail])]

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

Comment 15

[Lawrence Mandel [:lmandel] (use needinfo)]

Is there any way to automate testing of overscroll? Can we use Eideticker for this?

Comment 16

[Lawrence Mandel [:lmandel] (use needinfo)]

Kats - Now that the fix is on m-c, can you confirm that the fix is good there before uplifting to Aurora?

Comment 17

[Kartikaya Gupta (email:[e-mail])]

(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.

Comment 18

[Lawrence Mandel [:lmandel] (use needinfo)]

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?

Comment 19

[Kartikaya Gupta (email:[e-mail])]

Filed bug 1034733 for testing using Eideticker.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3f9d7a3a0b7b

Comment 21

[Shally Li（leave from Mozilla）]

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

Comment 22

[Shally Li（leave from Mozilla）]

Attachment: verified_v2.1.mp4