Closed
Bug 1033398
Opened 9 years ago
Closed 9 years ago
While reducing overscroll, we don't composite enough
Categories
(Core :: Panning and Zooming, defect)
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: kats, Assigned: kats, NeedInfo)
References
Details
Attachments
(2 files, 2 obsolete files)
6.80 KB,
patch
|
drs
:
review+
lmandel
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
3.97 MB,
video/mp4
|
Details |
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 | ||
Updated•9 years ago
|
Assignee: nobody → bugmail.mozilla
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Comment 1•9 years ago
|
||
Assignee | ||
Comment 2•9 years ago
|
||
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 3•9 years ago
|
||
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)
Assignee | ||
Comment 4•9 years ago
|
||
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•9 years ago
|
||
(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.
Assignee | ||
Comment 6•9 years ago
|
||
(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?
Comment 7•9 years ago
|
||
(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 8•9 years ago
|
||
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.
Assignee | ||
Comment 9•9 years ago
|
||
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)
Assignee | ||
Comment 10•9 years ago
|
||
Forgot to qref
Attachment #8450540 -
Attachment is obsolete: true
Attachment #8450540 -
Flags: review?(drs+bugzilla)
Attachment #8450541 -
Flags: review?(drs+bugzilla)
Comment 11•9 years ago
|
||
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+
Assignee | ||
Comment 12•9 years ago
|
||
Changed and landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/60051d7558c4
Comment 13•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/60051d7558c4
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Comment 14•9 years ago
|
||
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?
Comment 15•9 years ago
|
||
Is there any way to automate testing of overscroll? Can we use Eideticker for this?
Flags: needinfo?(bugmail.mozilla)
Comment 16•9 years ago
|
||
Kats - Now that the fix is on m-c, can you confirm that the fix is good there before uplifting to Aurora?
Assignee | ||
Comment 17•9 years ago
|
||
(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 18•9 years ago
|
||
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+
Assignee | ||
Comment 19•9 years ago
|
||
Filed bug 1034733 for testing using Eideticker.
Updated•9 years ago
|
Comment 21•9 years ago
|
||
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)
Comment 22•9 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•