Closed
Bug 1219296
Opened 9 years ago
Closed 8 years ago
Ship snap points to the compositor so that we can snap asynchronously
Categories
(Core :: Graphics: Layers, defect)
Core
Graphics: Layers
Tracking
()
RESOLVED
FIXED
mozilla48
People
(Reporter: kats, Assigned: botond)
References
(Blocks 1 open bug)
Details
(Whiteboard: [gfx-noted])
Attachments
(12 files, 6 obsolete files)
58 bytes,
text/x-review-board-request
|
kats
:
review+
ritu
:
approval-mozilla-beta+
|
Details |
58 bytes,
text/x-review-board-request
|
kats
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
kats
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
kats
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
kats
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
kats
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
kats
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
kats
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
kats
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
kats
:
review+
|
Details |
14.03 KB,
image/png
|
Details | |
11.76 KB,
image/png
|
Details |
The current implementation of the CSS snap points lives on the main thread. When APZ does a scroll, it requests that the main thread figure out the appropriate snap point and trigger a smooth scroll to that destination. This works ok for touch-based panning in APZ, because we can estimate where the scroll will end up based on the velocity and physics at the time the finger is lifted. For trackpad-based wheel events also this is doable. However for "clicky" scroll wheel events the main-thread implementation simply smooth-scrolls to the next/prev snap point for each roll of the wheel, as described at [1]. Doing this in the compositor requires the compositor to know the next/prev snap points. Additionally, in some cases, waiting on the main thread to determine the snap point is too slow, resulting in visual glitching or jankiness. Cwiiis reported seeing such anomalies on B2G which is starting to make extensive use of snap points in both the homescreen and the app switcher. I think eventually we should try and send snap point information to the compositor if possible, so that snapping can be done entirely without waiting on the main-thread, except for degenerate cases. [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1141884#c20
Updated•9 years ago
|
Whiteboard: [gfx-noted]
Assignee | ||
Comment 1•8 years ago
|
||
I'm going to try implementing this, to make progress on bug 1249040.
Assignee: nobody → botond
Assignee | ||
Comment 2•8 years ago
|
||
I've had a look at how main thread scroll snapping works. It doesn't maintain a list of absolute snap points for the scroll frame that could be shipped to the compositor as is; rather, whenever a scroll that should result in scroll snapping occurs, a set of candidate snap points that are nearby to the original scroll destination is constructed on demand, with the following information as inputs: (a) The scroll frame's scroll-snap-type, scroll-snap-destination, scroll-snap-points, and scroll range. (b) Any scroll-snap-coordinates defined on frames in the scroll frame's subtree (used in conjunction with layout metrics for the relevant frame). (c) The start position and original (without snapping) destination for the current scroll operation. It then chooses the best snap point from the candidates (this really happens while building the candidates, too). Given that, I can envision two approaches to shipping snap points to the compositor: 1) Ship the inputs listed in (a) and (b) to the compositor, and have the compositor compute snap points on demand from these inputs (plus (c) which it knows). This would involve: - Creating a representation for (a) and (b) that can be shipped to the compositor. For (a) this is pretty straightforward. For (b), I believe we can collapse the "any scroll-snap-coordinates ... in conjunction with layout metrics" into a single value per scroll-snap-coordinate. - Reformulating the algorithm for choosing candidate snap points (and choosing the best candidate) to depend only on this new representation, and not explicitly on content- side data structures (like the frame tree), so that it can be run on the compositor side. The compositor should then be able to accurately choose a snap point and snap to it. 2) Keep the inputs and the snap point computation algorithm on the main thread, but run the algorithm "speculatively" when sending a layers transaction, to compute a few snap points to either side of the current scroll offset, and ship those pre-computed snap points to the compositor. The compositor should then be able to snap accurately if the original scroll destination lies within the area "covered" by the pre-computed snap points, and would have to defer to the main thread otherwise. This would involve some modifications to the algorithm for computing snap points, so it doesn't necessarily require a concrete known scroll destination. I'm leaning towards approach (1), because it produces the better (more accurate) outcome, and it doesn't seem like it would be significantly trickier than (2). However, I'm open to input on this question.
Assignee | ||
Comment 3•8 years ago
|
||
Kip in particular might have some valuable insights about which approach is more promising.
Flags: needinfo?(kgilbert)
Assignee | ||
Comment 4•8 years ago
|
||
Finally, it's worth noting that there's a proposal at the W3C to revamp how scroll snapping works [1]. I'm not sure what our plans are for implementing it, but it may be worth considering when choosing our approach here. In particular, since the new spec changes how snap points are computed, if we go with approach (1) then implementing the new spec will require changes to the information that we make available to the compositor; if we go with approach (2), the protocol with the compositor (sending pre-computed snap points) would remain the same, and only the algorithm to compute those snap points on the main thread would change. [1] https://drafts.csswg.org/css-scroll-snap/
The css-scroll-snap spec has already been resolved as the way forward in the CSSWG. (The css-snappoints editor has been slow to implement the change.)
Comment 6•8 years ago
|
||
(In reply to Botond Ballo [:botond] [C++ standards meeting Feb 29 - Mar 5] from comment #2) > I've had a look at how main thread scroll snapping works. > > It doesn't maintain a list of absolute snap points for the scroll frame that > could be shipped to the compositor as is; rather, whenever a scroll that > should result in scroll snapping occurs, a set of candidate snap points that > are nearby to the original scroll destination is constructed on demand, with > the following information as inputs: > > (a) The scroll frame's scroll-snap-type, scroll-snap-destination, > scroll-snap-points, and scroll range. > > (b) Any scroll-snap-coordinates defined on frames in the scroll > frame's subtree (used in conjunction with layout metrics for > the relevant frame). > > (c) The start position and original (without snapping) destination > for the current scroll operation. > > It then chooses the best snap point from the candidates (this really happens > while building the candidates, too). > > Given that, I can envision two approaches to shipping snap points to the > compositor: > > 1) Ship the inputs listed in (a) and (b) to the compositor, and have > the compositor compute snap points on demand from these inputs > (plus (c) which it knows). > > This would involve: > > - Creating a representation for (a) and (b) that can be > shipped to the compositor. For (a) this is pretty > straightforward. For (b), I believe we can collapse the > "any scroll-snap-coordinates ... in conjunction with layout > metrics" into a single value per scroll-snap-coordinate. > > - Reformulating the algorithm for choosing candidate snap > points (and choosing the best candidate) to depend only > on this new representation, and not explicitly on content- > side data structures (like the frame tree), so that it > can be run on the compositor side. > > The compositor should then be able to accurately choose a > snap point and snap to it. > > 2) Keep the inputs and the snap point computation algorithm on > the main thread, but run the algorithm "speculatively" when > sending a layers transaction, to compute a few snap points > to either side of the current scroll offset, and ship those > pre-computed snap points to the compositor. > > The compositor should then be able to snap accurately if the > original scroll destination lies within the area "covered" > by the pre-computed snap points, and would have to defer to > the main thread otherwise. > > This would involve some modifications to the algorithm for > computing snap points, so it doesn't necessarily require a > concrete known scroll destination. > > I'm leaning towards approach (1), because it produces the better (more > accurate) outcome, and it doesn't seem like it would be significantly > trickier than (2). However, I'm open to input on this question. These are both fine approaches; however, if #1 is not significantly more complex I would recommend it as it would indeed provide better results. There were cases, especially on mobile, where snap points were determined too late into the scroll due to main thread activity and the resulting snap was abrupt. This should be solved with the #1 approach.
Flags: needinfo?(kgilbert)
Assignee | ||
Comment 8•8 years ago
|
||
If we're going to be shipping snap point information to the compositor, the FrameMetrics is a logical place to store it. However, FrameMetrics is a very large structure already, and particularly for uses such as sending it back to content in repaint requests, this would just add unnecessary bloat. To avoid this, I propose splitting the fields not needed for lighter-weight uses such as repaint requests, out of FrameMetrics, and into a more general ScrollMetadata structure on the layer. Attached is a WIP patch for doing this. It doesn't compile yet.
Assignee | ||
Comment 9•8 years ago
|
||
Here's an updated patch that compiles and seems to work (at the "doesn't immediately crash the browser" level). I'd like to exercise it a bit more before posting it for review.
Assignee | ||
Updated•8 years ago
|
Attachment #8727685 -
Attachment is obsolete: true
Assignee | ||
Comment 10•8 years ago
|
||
Very much a WIP.
Assignee | ||
Comment 11•8 years ago
|
||
While working on this, I realized that the main-thread scroll snap implementation only uses APZ to scroll to the destination if the pref "layout.css.scroll-behavior.enabled" is set to true; if it's set to false, we use a main-thread animation (ScrollFrameHelper::AsyncScroll) instead. Kip, is this interaction between scroll snapping and scroll-behavior governed by the spec? Is it important for APZ to replicate it (i.e. to use main-thread scrolling to perform scroll snapping if "layout.css.scroll-behavior.enabled" is set to false)?
Flags: needinfo?(kgilbert)
Assignee | ||
Comment 12•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/40657/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40657/
Assignee | ||
Comment 13•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/40659/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40659/
Assignee | ||
Comment 14•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/40661/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40661/
Assignee | ||
Comment 15•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/40663/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40663/
Assignee | ||
Comment 16•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/40665/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40665/
Assignee | ||
Comment 17•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/40667/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40667/
Assignee | ||
Comment 18•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/40669/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40669/
Assignee | ||
Comment 19•8 years ago
|
||
Still a WIP, but starting to take shape.
Assignee | ||
Updated•8 years ago
|
Attachment #8728739 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8729773 -
Attachment is obsolete: true
Comment 20•8 years ago
|
||
It's probably follow-up, but I suggest that the behaviour for snap points should act like magnetism - whereby every snap point has a certain attract force, and once the momentum of the fling is too low, it won't escape this force. This will give the sensation to the user of being able to 'feel' each snap point, as well as be consistent with our overscroll oscillation bounce effect. It would be a really nice bit of polish that would put us ahead of other browsers' behaviour here.
Assignee | ||
Comment 21•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/41197/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/41197/
Assignee | ||
Comment 22•8 years ago
|
||
Comment on attachment 8731505 [details] MozReview Request: Bug 1219296 - Fix an include-what-you-use error. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40661/diff/1-2/
Assignee | ||
Comment 23•8 years ago
|
||
Comment on attachment 8731506 [details] MozReview Request: Bug 1219296 - Factor out the algorithm that computes a scroll snap destination into a reusable form. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40663/diff/1-2/
Assignee | ||
Comment 24•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/41199/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/41199/
Assignee | ||
Comment 25•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/41201/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/41201/
Assignee | ||
Updated•8 years ago
|
Attachment #8731504 -
Attachment description: MozReview Request: [WIP] Bug 1219296 - Ship snap point information to the compositor. r=kats → MozReview Request: Bug 1219296 - Ship scroll snap information to the compositor. r=kats
Assignee | ||
Comment 26•8 years ago
|
||
Comment on attachment 8731504 [details] MozReview Request: Bug 1219296 - Ship scroll snap information to the compositor. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40659/diff/1-2/
Assignee | ||
Comment 27•8 years ago
|
||
Comment on attachment 8731507 [details] MozReview Request: Bug 1219296 - Remove StartSmoothScroll()'s argument, which is no longer used. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40665/diff/1-2/
Assignee | ||
Comment 28•8 years ago
|
||
Comment on attachment 8731508 [details] MozReview Request: Bug 1219296 - Light refactoring to how a smooth scroll is launched inside APZC. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40667/diff/1-2/
Assignee | ||
Comment 29•8 years ago
|
||
Comment on attachment 8731509 [details] MozReview Request: Bug 1219296 - Scroll snap directly in APZ instead of going through the main thread. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40669/diff/1-2/
Attachment #8731509 -
Attachment description: MozReview Request: [WIP] Bug 1219296 - APZ support for scroll snapping without going through the main thread. r=kats → MozReview Request: Bug 1219296 - Scroll snap directly in APZ instead of going through the main thread. r=kats
Assignee | ||
Comment 30•8 years ago
|
||
Here's an updated patch series. The patches are now "feature-complete", but they need testing. (I've done some preliminary testing and fixed some crashes, but I haven't verified yet that APZ is actually driving smooth scrolling as intended.)
Assignee | ||
Comment 31•8 years ago
|
||
So APZ isn't hooked up to perform scroll snapping in response to wheel scrolling; we can hook that up that in a follow-up if we'd like. I tested it with touch scrolling on my laptop, and basic use cases seem to work, but there is some weird behaviour near the edge of a scrollframe, and some weird behaviour related to handoff, that I need to investigate more.
Assignee | ||
Updated•8 years ago
|
Attachment #8731503 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 32•8 years ago
|
||
Comment on attachment 8731503 [details] MozReview Request: Bug 1219296 - Split fields not needed for repaints out from FrameMetrics. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40657/diff/1-2/
Assignee | ||
Updated•8 years ago
|
Attachment #8732462 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 33•8 years ago
|
||
Comment on attachment 8732462 [details] MozReview Request: Bug 1219296 - Factor out scroll snap information into a form that's usable by the compositor. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41197/diff/1-2/
Assignee | ||
Comment 34•8 years ago
|
||
Comment on attachment 8731505 [details] MozReview Request: Bug 1219296 - Fix an include-what-you-use error. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40661/diff/2-3/
Attachment #8731505 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 35•8 years ago
|
||
Comment on attachment 8731506 [details] MozReview Request: Bug 1219296 - Factor out the algorithm that computes a scroll snap destination into a reusable form. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40663/diff/2-3/
Attachment #8731506 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 36•8 years ago
|
||
Comment on attachment 8732463 [details] MozReview Request: Bug 1219296 - Move the layout.css.scroll-snap.proximity-threshold pref to gfxPrefs, so it can be queried on the compositor thread. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41199/diff/1-2/
Attachment #8732463 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 37•8 years ago
|
||
Comment on attachment 8731504 [details] MozReview Request: Bug 1219296 - Ship scroll snap information to the compositor. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40659/diff/2-3/
Attachment #8731504 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 38•8 years ago
|
||
Comment on attachment 8731507 [details] MozReview Request: Bug 1219296 - Remove StartSmoothScroll()'s argument, which is no longer used. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40665/diff/2-3/
Attachment #8731507 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 39•8 years ago
|
||
Comment on attachment 8731508 [details] MozReview Request: Bug 1219296 - Light refactoring to how a smooth scroll is launched inside APZC. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40667/diff/2-3/
Attachment #8731508 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Updated•8 years ago
|
Attachment #8731509 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 40•8 years ago
|
||
Comment on attachment 8731509 [details] MozReview Request: Bug 1219296 - Scroll snap directly in APZ instead of going through the main thread. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40669/diff/2-3/
Assignee | ||
Updated•8 years ago
|
Attachment #8732464 -
Attachment is obsolete: true
Assignee | ||
Comment 41•8 years ago
|
||
The updated patch series fixes the bugs I found during testing. Markus also pointed out that we can avoid sending nsStyleCoord over IPC altogether, by moving some of the processing that doesn't depend on the destination of a particular scroll, from GetSnapPointForDestination() into ComputeScrollSnapInfo(). The updated patches implement this improvement. (Thanks, Markus!) Posted patch series for review.
Assignee | ||
Comment 42•8 years ago
|
||
Comment on attachment 8732462 [details] MozReview Request: Bug 1219296 - Factor out scroll snap information into a form that's usable by the compositor. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41197/diff/2-3/
Assignee | ||
Comment 43•8 years ago
|
||
Comment on attachment 8731505 [details] MozReview Request: Bug 1219296 - Fix an include-what-you-use error. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40661/diff/3-4/
Assignee | ||
Comment 44•8 years ago
|
||
Comment on attachment 8731506 [details] MozReview Request: Bug 1219296 - Factor out the algorithm that computes a scroll snap destination into a reusable form. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40663/diff/3-4/
Assignee | ||
Comment 45•8 years ago
|
||
Comment on attachment 8732463 [details] MozReview Request: Bug 1219296 - Move the layout.css.scroll-snap.proximity-threshold pref to gfxPrefs, so it can be queried on the compositor thread. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41199/diff/2-3/
Assignee | ||
Comment 46•8 years ago
|
||
Comment on attachment 8731504 [details] MozReview Request: Bug 1219296 - Ship scroll snap information to the compositor. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40659/diff/3-4/
Assignee | ||
Comment 47•8 years ago
|
||
Comment on attachment 8731507 [details] MozReview Request: Bug 1219296 - Remove StartSmoothScroll()'s argument, which is no longer used. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40665/diff/3-4/
Assignee | ||
Comment 48•8 years ago
|
||
Comment on attachment 8731508 [details] MozReview Request: Bug 1219296 - Light refactoring to how a smooth scroll is launched inside APZC. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40667/diff/3-4/
Assignee | ||
Comment 49•8 years ago
|
||
Comment on attachment 8731509 [details] MozReview Request: Bug 1219296 - Scroll snap directly in APZ instead of going through the main thread. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40669/diff/3-4/
Assignee | ||
Comment 50•8 years ago
|
||
(Fixed a patch-splitting error.)
Reporter | ||
Comment 51•8 years ago
|
||
Comment on attachment 8731503 [details] MozReview Request: Bug 1219296 - Split fields not needed for repaints out from FrameMetrics. r=kats https://reviewboard.mozilla.org/r/40657/#review38165 ::: gfx/layers/FrameMetrics.cpp:13 (Diff revision 2) > > namespace mozilla { > namespace layers { > > const FrameMetrics::ViewID FrameMetrics::NULL_SCROLL_ID = 0; > const FrameMetrics FrameMetrics::sNullMetrics; Can we get rid of the sNullMetrics now? ::: gfx/layers/Layers.h:880 (Diff revision 2) > * See the documentation in LayerMetricsWrapper.h for a more detailed > * explanation of this conceptual equivalence. > * > * Note also that there is actually a many-to-many relationship between > - * Layers and FrameMetrics, because multiple Layers may have identical > - * FrameMetrics objects. This happens when those layers belong to the > + * Layers and ScrollMetadata, because multiple Layers may have identical > + * ScrollMetadataobjects. This happens when those layers belong to the space before "objects" ::: gfx/layers/apz/src/AsyncPanZoomController.h:185 (Diff revision 2) > /** > - * A shadow layer update has arrived. |aLayerMetrics| is the new FrameMetrics > + * A shadow layer update has arrived. |aScrollMetdata| is the new ScrollMetadata > * for the container layer corresponding to this APZC. > * |aIsFirstPaint| is a flag passed from the shadow > - * layers code indicating that the frame metrics being sent with this call are > - * the initial metrics and the initial paint of the frame has just happened. > + * layers code indicating that the scroll metadata being sent with this call are > + * the initial ,etadata and the initial paint of the frame has just happened. typo
Attachment #8731503 -
Flags: review?(bugmail.mozilla) → review+
Reporter | ||
Comment 52•8 years ago
|
||
Comment on attachment 8732462 [details] MozReview Request: Bug 1219296 - Factor out scroll snap information into a form that's usable by the compositor. r=kats https://reviewboard.mozilla.org/r/41197/#review38187
Attachment #8732462 -
Flags: review?(bugmail.mozilla) → review+
Reporter | ||
Comment 53•8 years ago
|
||
There's build failures, btw: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4fdbddd742ac https://treeherder.mozilla.org/#/jobs?repo=try&revision=9dd576e7887d
Reporter | ||
Comment 54•8 years ago
|
||
Comment on attachment 8731505 [details] MozReview Request: Bug 1219296 - Fix an include-what-you-use error. r=kats https://reviewboard.mozilla.org/r/40661/#review38189
Attachment #8731505 -
Flags: review?(bugmail.mozilla) → review+
Assignee | ||
Comment 55•8 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #53) > There's build failures, btw: > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=4fdbddd742ac > https://treeherder.mozilla.org/#/jobs?repo=try&revision=9dd576e7887d Sorry about that! The cause is that gcc is strict about method names (in this case, LayerMetricsWrapper::ScrollMetadata()) that clash with class names (in this case, ScrollMetadata), while clang (which I was building with locally) is permissive about it. The fix is to rename LayerMetricsWrapper::ScrollMetadata().
Reporter | ||
Updated•8 years ago
|
Attachment #8731506 -
Flags: review?(bugmail.mozilla) → review+
Reporter | ||
Comment 56•8 years ago
|
||
Comment on attachment 8731506 [details] MozReview Request: Bug 1219296 - Factor out the algorithm that computes a scroll snap destination into a reusable form. r=kats https://reviewboard.mozilla.org/r/40663/#review38465 ::: layout/generic/ScrollSnap.cpp:311 (Diff revision 4) > + snapped = true; > + } > + return snapped ? Some(finalPos) : Nothing(); > +} > + > +} // namespace mozilla ::: layout/generic/nsGfxScrollFrame.cpp:5771 (Diff revision 4) > - snapped = true; > - } > - if (snapped) { > - aDestination = finalPos; > } > - return snapped; > + return snapPoint.isSome(); I'd prefer a |return true;| inside the if and a |return false;| here. Also I'm assuming that you didn't modify any code during the move - MozReview doesn't flag it as "moved from line X" but I didn't go over it line-by-line
Reporter | ||
Comment 57•8 years ago
|
||
Comment on attachment 8732463 [details] MozReview Request: Bug 1219296 - Move the layout.css.scroll-snap.proximity-threshold pref to gfxPrefs, so it can be queried on the compositor thread. r=kats https://reviewboard.mozilla.org/r/41199/#review38467 ::: gfx/thebes/gfxPrefs.h:397 (Diff revision 3) > DECL_GFX_PREF(Live, "layers.single-tile.enabled", LayersSingleTileEnabled, bool, true); > > DECL_GFX_PREF(Live, "layout.css.scroll-behavior.damping-ratio", ScrollBehaviorDampingRatio, float, 1.0f); > DECL_GFX_PREF(Live, "layout.css.scroll-behavior.enabled", ScrollBehaviorEnabled, bool, false); > DECL_GFX_PREF(Live, "layout.css.scroll-behavior.spring-constant", ScrollBehaviorSpringConstant, float, 250.0f); > + DECL_GFX_PREF(Live, "layout.css.scroll-snap.proximity-threshold", ScrollSnapProximityThreshold, int32_t, 200); Move this down two lines so it's alpha-sorted ::: layout/generic/ScrollSnap.cpp:10 (Diff revision 3) > > #include "FrameMetrics.h" > #include "ScrollSnap.h" > #include "mozilla/Maybe.h" > #include "mozilla/Preferences.h" > +#include "gfxPrefs.h" Move this up two lines so it's alpha sorted :) I'm assuming you're sticking with your "uppercase first" alpha ordering here.
Attachment #8732463 -
Flags: review?(bugmail.mozilla) → review+
Reporter | ||
Comment 58•8 years ago
|
||
Comment on attachment 8731504 [details] MozReview Request: Bug 1219296 - Ship scroll snap information to the compositor. r=kats https://reviewboard.mozilla.org/r/40659/#review38471
Attachment #8731504 -
Flags: review?(bugmail.mozilla) → review+
Reporter | ||
Updated•8 years ago
|
Attachment #8731507 -
Flags: review?(bugmail.mozilla) → review+
Reporter | ||
Comment 59•8 years ago
|
||
Comment on attachment 8731507 [details] MozReview Request: Bug 1219296 - Remove StartSmoothScroll()'s argument, which is no longer used. r=kats https://reviewboard.mozilla.org/r/40665/#review38473
Reporter | ||
Comment 60•8 years ago
|
||
Comment on attachment 8731508 [details] MozReview Request: Bug 1219296 - Light refactoring to how a smooth scroll is launched inside APZC. r=kats https://reviewboard.mozilla.org/r/40667/#review38477
Attachment #8731508 -
Flags: review?(bugmail.mozilla) → review+
Reporter | ||
Comment 61•8 years ago
|
||
https://reviewboard.mozilla.org/r/40663/#review38485 ::: layout/generic/ScrollSnap.h:26 (Diff revision 4) > + * |aSnapInfo|, |aScrollPortSize|, and |aScrollRange| are characteristics > + * of the scroll frame for which snapping is being performed. > + * If a suitable snap point could be found, it is returned. Otherwise, an > + * empty Maybe is returned. > + */ > + static Maybe<nsPoint> GetSnapPointForDestination( Add a big scary warning here that this function may be used off-main-thread and should not do anything that touches main-thread-only structures without appropriate locking.
Reporter | ||
Updated•8 years ago
|
Attachment #8731509 -
Flags: review?(bugmail.mozilla) → review+
Reporter | ||
Comment 62•8 years ago
|
||
Comment on attachment 8731509 [details] MozReview Request: Bug 1219296 - Scroll snap directly in APZ instead of going through the main thread. r=kats https://reviewboard.mozilla.org/r/40669/#review38487
Reporter | ||
Comment 63•8 years ago
|
||
Nice work! This seems like a pretty clean implementation overall :) Before this lands I'd like to make sure there are no significant talos regressions; I'm a little worried about the overhead of running the snap calculations even though it's likely not very significant. Please do a try push with talos and talos-e10s on linux (at least) to make sure. Thanks!
Assignee | ||
Comment 64•8 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #51) > > const FrameMetrics FrameMetrics::sNullMetrics; > > Can we get rid of the sNullMetrics now? Good idea. I replaced its remaining uses is LayerMetricsWrapper with sNullMetadata.GetMetrics(). (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #56) > Also I'm assuming that you didn't modify any code during the move - > MozReview doesn't flag it as "moved from line X" but I didn't go over it > line-by-line The body of GetSnapPointForDestination() was adjusted to get information about the scroll frame from its parameters, rather than from the fields of ScrollFrameHelper (which it was previously a method of). CalcSnapPoints and SnappingEdgeCallback were copied over verbatim.
Assignee | ||
Comment 65•8 years ago
|
||
Try push, including Talos tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=539ce0c92fc6
Assignee | ||
Comment 66•8 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #11) > While working on this, I realized that the main-thread scroll snap > implementation only uses APZ to scroll to the destination if the pref > "layout.css.scroll-behavior.enabled" is set to true; if it's set to false, > we use a main-thread animation (ScrollFrameHelper::AsyncScroll) instead. > > Kip, is this interaction between scroll snapping and scroll-behavior > governed by the spec? Is it important for APZ to replicate it (i.e. to use > main-thread scrolling to perform scroll snapping if > "layout.css.scroll-behavior.enabled" is set to false)? In the current patch series, APZ does not replicate this behaviour; all APZ-triggered scroll snaps result in APZ smooth scrolls. If you feel strongly that this shouldn't be the case, let me know.
Flags: needinfo?(kgilbert)
Assignee | ||
Comment 67•8 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #65) > Try push, including Talos tests: > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=539ce0c92fc6 There's bustage in Android-only code, which I've fixed locally. The debug build is also showing nsTArray leaks. I believe this is caused by the patch adding an nsTArray field to the global sNullMetadata object; we'll need to change that to use StaticAutoPtr instead.
Assignee | ||
Comment 68•8 years ago
|
||
Comment on attachment 8731503 [details] MozReview Request: Bug 1219296 - Split fields not needed for repaints out from FrameMetrics. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40657/diff/2-3/
Assignee | ||
Comment 69•8 years ago
|
||
Comment on attachment 8732462 [details] MozReview Request: Bug 1219296 - Factor out scroll snap information into a form that's usable by the compositor. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41197/diff/3-4/
Assignee | ||
Comment 70•8 years ago
|
||
Comment on attachment 8731505 [details] MozReview Request: Bug 1219296 - Fix an include-what-you-use error. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40661/diff/4-5/
Assignee | ||
Comment 71•8 years ago
|
||
Comment on attachment 8731506 [details] MozReview Request: Bug 1219296 - Factor out the algorithm that computes a scroll snap destination into a reusable form. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40663/diff/4-5/
Assignee | ||
Comment 72•8 years ago
|
||
Comment on attachment 8732463 [details] MozReview Request: Bug 1219296 - Move the layout.css.scroll-snap.proximity-threshold pref to gfxPrefs, so it can be queried on the compositor thread. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41199/diff/3-4/
Assignee | ||
Comment 73•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/42091/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/42091/
Attachment #8734110 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 74•8 years ago
|
||
Comment on attachment 8731504 [details] MozReview Request: Bug 1219296 - Ship scroll snap information to the compositor. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40659/diff/4-5/
Assignee | ||
Comment 75•8 years ago
|
||
Comment on attachment 8731507 [details] MozReview Request: Bug 1219296 - Remove StartSmoothScroll()'s argument, which is no longer used. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40665/diff/4-5/
Assignee | ||
Comment 76•8 years ago
|
||
Comment on attachment 8731508 [details] MozReview Request: Bug 1219296 - Light refactoring to how a smooth scroll is launched inside APZC. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40667/diff/4-5/
Assignee | ||
Comment 77•8 years ago
|
||
Comment on attachment 8731509 [details] MozReview Request: Bug 1219296 - Scroll snap directly in APZ instead of going through the main thread. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40669/diff/4-5/
Assignee | ||
Comment 78•8 years ago
|
||
Comment on attachment 8734110 [details] MozReview Request: Bug 1219296 - Make ScrollMetadata::sNullMetadata a StaticAutoPtr so that ScrollMetadata can admit nsTArray members. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42091/diff/1-2/
Assignee | ||
Comment 79•8 years ago
|
||
Comment on attachment 8731504 [details] MozReview Request: Bug 1219296 - Ship scroll snap information to the compositor. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40659/diff/5-6/
Assignee | ||
Comment 80•8 years ago
|
||
Comment on attachment 8731507 [details] MozReview Request: Bug 1219296 - Remove StartSmoothScroll()'s argument, which is no longer used. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40665/diff/5-6/
Assignee | ||
Comment 81•8 years ago
|
||
Comment on attachment 8731508 [details] MozReview Request: Bug 1219296 - Light refactoring to how a smooth scroll is launched inside APZC. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40667/diff/5-6/
Assignee | ||
Comment 82•8 years ago
|
||
Comment on attachment 8731509 [details] MozReview Request: Bug 1219296 - Scroll snap directly in APZ instead of going through the main thread. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40669/diff/5-6/
Reporter | ||
Comment 83•8 years ago
|
||
Comment on attachment 8734110 [details] MozReview Request: Bug 1219296 - Make ScrollMetadata::sNullMetadata a StaticAutoPtr so that ScrollMetadata can admit nsTArray members. r=kats https://reviewboard.mozilla.org/r/42091/#review38629
Attachment #8734110 -
Flags: review?(bugmail.mozilla) → review+
Assignee | ||
Comment 84•8 years ago
|
||
Whoops! We can't initialize sNullMetadata in AsyncPanZoomController::InitializeGlobalState(), because that only gets called if APZ is enabled, while there is some code (LayerHasCheckerboardingAPZC()) that tries to access sNullMetadata even if APZ is disabled.
Assignee | ||
Comment 85•8 years ago
|
||
Initializing it in the CompositorParent constructor instead: https://treeherder.mozilla.org/#/jobs?repo=try&revision=248686502573 This time, I'm going to wait for a clean Try push before requesting re-review :)
Assignee | ||
Comment 86•8 years ago
|
||
Of course, there's no CompositorParent in gtests. (No one calls AsyncPanZoomController::InitializeGlobalState(), either, but the gtests don't seem to need the things that sets up.)
Reporter | ||
Comment 87•8 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #84) > (LayerHasCheckerboardingAPZC()) that tries to access sNullMetadata even if > APZ is disabled. We can probably guard this function with an APZ-enabled check.
Assignee | ||
Comment 88•8 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #31) > So APZ isn't hooked up to perform scroll snapping in response to wheel > scrolling; we can hook that up that in a follow-up if we'd like. We definitely want to do this (among other things, it's required to fix bug 1249040). I filed bug 1259296 for it.
Assignee | ||
Comment 89•8 years ago
|
||
(In reply to Chris Lord [:cwiiis] from comment #20) > It's probably follow-up, but I suggest that the behaviour for snap points > should act like magnetism - whereby every snap point has a certain attract > force, and once the momentum of the fling is too low, it won't escape this > force. This will give the sensation to the user of being able to 'feel' each > snap point, as well as be consistent with our overscroll oscillation bounce > effect. It would be a really nice bit of polish that would put us ahead of > other browsers' behaviour here. Thanks for the suggestion! Filed bug 1259298 for this.
Assignee | ||
Comment 90•8 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #86) > Of course, there's no CompositorParent in gtests. (No one calls > AsyncPanZoomController::InitializeGlobalState(), either, but the gtests > don't seem to need the things that sets up.) I fixed the APZ gtests, but it turns out LayerMetricsWrapper has its own gtests in TestLayers.cpp. This StaticAutoPtr thing is turning out to be a lot more trouble than I imagined...
Assignee | ||
Comment 91•8 years ago
|
||
Comment on attachment 8734110 [details] MozReview Request: Bug 1219296 - Make ScrollMetadata::sNullMetadata a StaticAutoPtr so that ScrollMetadata can admit nsTArray members. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42091/diff/2-3/
Assignee | ||
Comment 92•8 years ago
|
||
Comment on attachment 8731504 [details] MozReview Request: Bug 1219296 - Ship scroll snap information to the compositor. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40659/diff/6-7/
Assignee | ||
Comment 93•8 years ago
|
||
Comment on attachment 8731507 [details] MozReview Request: Bug 1219296 - Remove StartSmoothScroll()'s argument, which is no longer used. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40665/diff/6-7/
Assignee | ||
Comment 94•8 years ago
|
||
Comment on attachment 8731508 [details] MozReview Request: Bug 1219296 - Light refactoring to how a smooth scroll is launched inside APZC. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40667/diff/6-7/
Assignee | ||
Comment 95•8 years ago
|
||
Comment on attachment 8731509 [details] MozReview Request: Bug 1219296 - Scroll snap directly in APZ instead of going through the main thread. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40669/diff/6-7/
Assignee | ||
Comment 96•8 years ago
|
||
All right, this Try push [1] shows everything except gtests passing, and this one [2] shows gtests passing (with the only code changes between the two being to gtest files). [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=248686502573 [2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=923412c1afb4
Assignee | ||
Comment 97•8 years ago
|
||
Comment on attachment 8734110 [details] MozReview Request: Bug 1219296 - Make ScrollMetadata::sNullMetadata a StaticAutoPtr so that ScrollMetadata can admit nsTArray members. r=kats Asking for re-review on this because I had to make a number of changes to get tests to pass. I'm not thrilled about having to do all this to get the StaticAutoPtr to be alive at the right times. If you have suggestions for better ways to do this, let me know.
Attachment #8734110 -
Flags: review+ → review?(bugmail.mozilla)
Assignee | ||
Comment 98•8 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #87) > (In reply to Botond Ballo [:botond] from comment #84) > > (LayerHasCheckerboardingAPZC()) that tries to access sNullMetadata even if > > APZ is disabled. > > We can probably guard this function with an APZ-enabled check. I already had the other solution (using the CompositorParent constructor) implemented and running on Try when I saw this, so I stuck to it. Let me know if you prefer doing this instead, I can change it. (Note that this is orthogonal to what we do in the gtests.)
Reporter | ||
Comment 99•8 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #98) > (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #87) > > (In reply to Botond Ballo [:botond] from comment #84) > > > (LayerHasCheckerboardingAPZC()) that tries to access sNullMetadata even if > > > APZ is disabled. > > > > We can probably guard this function with an APZ-enabled check. > > I already had the other solution (using the CompositorParent constructor) > implemented and running on Try when I saw this, so I stuck to it. Let me > know if you prefer doing this instead, I can change it. I kinda do prefer guarding LayerHasCheckerboardingAPZC. For one it will avoid unnecessary walking around in the tree when APZ is disabled, and it simplifies the startup code a lot. I don't particularly like having APZ-specific init stuff in CompositorParent. > (Note that this is orthogonal to what we do in the gtests.) I'm not sure what this means. AFAIK all our gtests create an APZCTreeManager, so they will all be calling AsyncPanZoomController::InitializeGlobalState. If we put the initialization of the StaticAutoPtr there we don't need a lot of the gtest changes you had to make in this last iteration (or maybe I'm misreading the interdiffs?).
Reporter | ||
Comment 100•8 years ago
|
||
Comment on attachment 8734110 [details] MozReview Request: Bug 1219296 - Make ScrollMetadata::sNullMetadata a StaticAutoPtr so that ScrollMetadata can admit nsTArray members. r=kats https://reviewboard.mozilla.org/r/42091/#review38733 Dropping review flag. As commented above, I'm fine with making this a StaticAutoPtr, but I think initializing it in InitializeGlobalState is the way to go. In the long term we'll have APZ enabled everywhere so it being uninitialized shouldn't be a problem.
Attachment #8734110 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 101•8 years ago
|
||
(In reply to (Back on Mar30) Kartikaya Gupta (email:kats@mozilla.com) from comment #99) > > (Note that this is orthogonal to what we do in the gtests.) > > I'm not sure what this means. AFAIK all our gtests create an > APZCTreeManager, so they will all be calling > AsyncPanZoomController::InitializeGlobalState. If we put the initialization > of the StaticAutoPtr there we don't need a lot of the gtest changes you had > to make in this last iteration (or maybe I'm misreading the interdiffs?). You're right in that we shouldn't need the changes to APZCTreeManagerTester. We'll still need the change to TestLayers.cpp. I agree that it's an improvement overall. Try push with these changes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d660faf4ad7
Assignee | ||
Comment 102•8 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #101) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d660faf4ad7 Unfortunately, LayerHasCheckerboardingAPZC() wasn't the only thing doing a LayerMetricsWrapper tree walk with APZ disabled. AlignFixedAndStickyLayers() does it too.
Assignee | ||
Comment 103•8 years ago
|
||
Here's a hacky workaround for the crash in AlignFixedAndStickyLayers(). With this, I have a green Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=69159359a46a However, I don't like this, because it muddies the interface of LayerMetricsWrapper. (Under what circumstances is it safe to call Metrics() or Metadata() without checking HasScrollMetadata() first?) To me, the fact that AlignFixedAndStickyLayers() is legitimately used in non-APZ setups, and it does a LayerMetricsWrapper tree walk, suggests that initializing things needed by LayerMetricsWrapper, such as sNullMetadata, in code that's not APZ-specific (such as CompositorParent (these days called CompositorBridgeParent)), would be reasonable. However, I'm open to alternative suggestions.
Attachment #8736074 -
Flags: feedback?(bugmail.mozilla)
Reporter | ||
Updated•8 years ago
|
Attachment #8736074 -
Attachment is patch: true
Reporter | ||
Comment 104•8 years ago
|
||
Comment on attachment 8736074 [details] [diff] [review] Hacky fix for crash in AlignFixedAndStickyLayers() Review of attachment 8736074 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Botond Ballo [:botond] from comment #103) > To me, the fact that AlignFixedAndStickyLayers() is legitimately used in > non-APZ setups, and it does a LayerMetricsWrapper tree walk, suggests that > initializing things needed by LayerMetricsWrapper, such as sNullMetadata, in > code that's not APZ-specific (such as CompositorParent (these days called > CompositorBridgeParent)), would be reasonable. Yeah, you're right. If we're using the sNullMetadata in non-APZ-specific code then we shouldn't initialize it in APZ-specific code. I didn't realize it was so widely used. In that case initializing it from CompositorBridgeParent or (if possible) gfxPlatform should be ok.
Attachment #8736074 -
Flags: feedback?(bugmail.mozilla)
Assignee | ||
Comment 105•8 years ago
|
||
So it turns out gtests have more things going for them than I orginally thought - they've got XPCOM, and they can handle called gfxPlatform::Init() as well. That made implementing the approach we discussed on IRC fairly simple.
Assignee | ||
Comment 106•8 years ago
|
||
Comment on attachment 8731503 [details] MozReview Request: Bug 1219296 - Split fields not needed for repaints out from FrameMetrics. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40657/diff/3-4/
Attachment #8734110 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 107•8 years ago
|
||
Comment on attachment 8732462 [details] MozReview Request: Bug 1219296 - Factor out scroll snap information into a form that's usable by the compositor. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41197/diff/4-5/
Assignee | ||
Comment 108•8 years ago
|
||
Comment on attachment 8731505 [details] MozReview Request: Bug 1219296 - Fix an include-what-you-use error. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40661/diff/5-6/
Assignee | ||
Comment 109•8 years ago
|
||
Comment on attachment 8731506 [details] MozReview Request: Bug 1219296 - Factor out the algorithm that computes a scroll snap destination into a reusable form. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40663/diff/5-6/
Assignee | ||
Comment 110•8 years ago
|
||
Comment on attachment 8732463 [details] MozReview Request: Bug 1219296 - Move the layout.css.scroll-snap.proximity-threshold pref to gfxPrefs, so it can be queried on the compositor thread. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41199/diff/4-5/
Assignee | ||
Comment 111•8 years ago
|
||
Comment on attachment 8734110 [details] MozReview Request: Bug 1219296 - Make ScrollMetadata::sNullMetadata a StaticAutoPtr so that ScrollMetadata can admit nsTArray members. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42091/diff/3-4/
Assignee | ||
Comment 112•8 years ago
|
||
Comment on attachment 8731504 [details] MozReview Request: Bug 1219296 - Ship scroll snap information to the compositor. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40659/diff/7-8/
Assignee | ||
Comment 113•8 years ago
|
||
Comment on attachment 8731507 [details] MozReview Request: Bug 1219296 - Remove StartSmoothScroll()'s argument, which is no longer used. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40665/diff/7-8/
Assignee | ||
Comment 114•8 years ago
|
||
Comment on attachment 8731508 [details] MozReview Request: Bug 1219296 - Light refactoring to how a smooth scroll is launched inside APZC. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40667/diff/7-8/
Assignee | ||
Comment 115•8 years ago
|
||
Comment on attachment 8731509 [details] MozReview Request: Bug 1219296 - Scroll snap directly in APZ instead of going through the main thread. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40669/diff/7-8/
Assignee | ||
Updated•8 years ago
|
Attachment #8736074 -
Attachment is obsolete: true
Assignee | ||
Comment 116•8 years ago
|
||
Green Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=85de12ecb795
Assignee | ||
Comment 117•8 years ago
|
||
And here's a Try push without the patches to get baseline Talos results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5eaf176c9d83
Reporter | ||
Comment 118•8 years ago
|
||
Comment on attachment 8734110 [details] MozReview Request: Bug 1219296 - Make ScrollMetadata::sNullMetadata a StaticAutoPtr so that ScrollMetadata can admit nsTArray members. r=kats https://reviewboard.mozilla.org/r/42091/#review40059 ::: gfx/layers/composite/ContainerLayerComposite.cpp:364 (Diff revision 4) > > static bool > -NeedToDrawCheckerboardingForLayer(Layer* aLayer, Color* aOutCheckerboardingColor) > +NeedToDrawCheckerboardingForLayer(LayerManagerComposite* aLayerManager, > + Layer* aLayer, Color* aOutCheckerboardingColor) > { > - return (aLayer->GetContentFlags() & Layer::CONTENT_OPAQUE) && > + return (aLayerManager->AsyncPanZoomEnabled() && This is fine, but you should also be able to get the LayerManager via aLayer->Manager() so you can avoid passing it in if you want.
Attachment #8734110 -
Flags: review?(bugmail.mozilla) → review+
Assignee | ||
Comment 119•8 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #63) > Before this lands I'd like to make sure there are no significant talos > regressions; I'm a little worried about the overhead of running the snap > calculations even though it's likely not very significant. Please do a try > push with talos and talos-e10s on linux (at least) to make sure. Thanks! Here's a Talos comparison: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=5eaf176c9d83&newProject=try&newRevision=85de12ecb795&framework=1&showOnlyImportant=0 All of the regressions are "low confidence" results, seemingly just noise.
Assignee | ||
Comment 120•8 years ago
|
||
Comment on attachment 8734110 [details] MozReview Request: Bug 1219296 - Make ScrollMetadata::sNullMetadata a StaticAutoPtr so that ScrollMetadata can admit nsTArray members. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42091/diff/4-5/
Assignee | ||
Comment 121•8 years ago
|
||
Comment on attachment 8731504 [details] MozReview Request: Bug 1219296 - Ship scroll snap information to the compositor. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40659/diff/8-9/
Assignee | ||
Comment 122•8 years ago
|
||
Comment on attachment 8731507 [details] MozReview Request: Bug 1219296 - Remove StartSmoothScroll()'s argument, which is no longer used. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40665/diff/8-9/
Assignee | ||
Comment 123•8 years ago
|
||
Comment on attachment 8731508 [details] MozReview Request: Bug 1219296 - Light refactoring to how a smooth scroll is launched inside APZC. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40667/diff/8-9/
Assignee | ||
Comment 124•8 years ago
|
||
Comment on attachment 8731509 [details] MozReview Request: Bug 1219296 - Scroll snap directly in APZ instead of going through the main thread. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40669/diff/8-9/
Assignee | ||
Comment 125•8 years ago
|
||
I submitted the patch series via autoland.
Assignee | ||
Comment 126•8 years ago
|
||
Comment on attachment 8731503 [details] MozReview Request: Bug 1219296 - Split fields not needed for repaints out from FrameMetrics. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40657/diff/4-5/
Assignee | ||
Comment 127•8 years ago
|
||
Comment on attachment 8732462 [details] MozReview Request: Bug 1219296 - Factor out scroll snap information into a form that's usable by the compositor. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41197/diff/5-6/
Assignee | ||
Comment 128•8 years ago
|
||
Comment on attachment 8731505 [details] MozReview Request: Bug 1219296 - Fix an include-what-you-use error. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40661/diff/6-7/
Assignee | ||
Comment 129•8 years ago
|
||
Comment on attachment 8731506 [details] MozReview Request: Bug 1219296 - Factor out the algorithm that computes a scroll snap destination into a reusable form. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40663/diff/6-7/
Assignee | ||
Comment 130•8 years ago
|
||
Comment on attachment 8732463 [details] MozReview Request: Bug 1219296 - Move the layout.css.scroll-snap.proximity-threshold pref to gfxPrefs, so it can be queried on the compositor thread. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41199/diff/5-6/
Assignee | ||
Comment 131•8 years ago
|
||
Comment on attachment 8734110 [details] MozReview Request: Bug 1219296 - Make ScrollMetadata::sNullMetadata a StaticAutoPtr so that ScrollMetadata can admit nsTArray members. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42091/diff/5-6/
Assignee | ||
Comment 132•8 years ago
|
||
Comment on attachment 8731504 [details] MozReview Request: Bug 1219296 - Ship scroll snap information to the compositor. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40659/diff/9-10/
Assignee | ||
Comment 133•8 years ago
|
||
Comment on attachment 8731507 [details] MozReview Request: Bug 1219296 - Remove StartSmoothScroll()'s argument, which is no longer used. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40665/diff/9-10/
Assignee | ||
Comment 134•8 years ago
|
||
Comment on attachment 8731508 [details] MozReview Request: Bug 1219296 - Light refactoring to how a smooth scroll is launched inside APZC. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40667/diff/9-10/
Assignee | ||
Comment 135•8 years ago
|
||
Comment on attachment 8731509 [details] MozReview Request: Bug 1219296 - Scroll snap directly in APZ instead of going through the main thread. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40669/diff/9-10/
Assignee | ||
Comment 136•8 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #125) > I submitted the patch series via autoland. (Had to rebase. Submitted again.)
Comment 137•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb2023f50288 https://hg.mozilla.org/integration/mozilla-inbound/rev/c55ab6afe5ad https://hg.mozilla.org/integration/mozilla-inbound/rev/453cae5a3261 https://hg.mozilla.org/integration/mozilla-inbound/rev/77337fbc28d5 https://hg.mozilla.org/integration/mozilla-inbound/rev/bf846e99c358 https://hg.mozilla.org/integration/mozilla-inbound/rev/a10ed3e7a53e https://hg.mozilla.org/integration/mozilla-inbound/rev/8e167bb3d4f5 https://hg.mozilla.org/integration/mozilla-inbound/rev/a5355966197c https://hg.mozilla.org/integration/mozilla-inbound/rev/0b0e5290d011 https://hg.mozilla.org/integration/mozilla-inbound/rev/28a2a7730361
Reporter | ||
Comment 138•8 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #119) > Here's a Talos comparison: > https://treeherder.mozilla.org/perf.html#/ > compare?originalProject=try&originalRevision=5eaf176c9d83&newProject=try&newR > evision=85de12ecb795&framework=1&showOnlyImportant=0 > > All of the regressions are "low confidence" results, seemingly just noise. Hm, there's some scary-looking regression numbers for TART and CART and some others. But yeah, they're low confidence, possibly because the tests are bimodal or because of bug 1260926. I'm concerned that they do still introduce a real regression. We can take another look once bug 1260926 is fixed.
Assignee | ||
Comment 139•8 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #138) > Hm, there's some scary-looking regression numbers for TART and CART and some > others. But yeah, they're low confidence, possibly because the tests are > bimodal or because of bug 1260926. I'm concerned that they do still > introduce a real regression. We can take another look once bug 1260926 is > fixed. Here are the inbound results: https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-inbound&originalRevision=95e61ede373d&newProject=mozilla-inbound&newRevision=28a2a7730361&framework=1&filter=linu&showOnlyImportant=0 The largest regression is 1.5%.
Reporter | ||
Comment 140•8 years ago
|
||
Sounds good, thanks for checking!
Comment 141•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/cb2023f50288 https://hg.mozilla.org/mozilla-central/rev/c55ab6afe5ad https://hg.mozilla.org/mozilla-central/rev/453cae5a3261 https://hg.mozilla.org/mozilla-central/rev/77337fbc28d5 https://hg.mozilla.org/mozilla-central/rev/bf846e99c358 https://hg.mozilla.org/mozilla-central/rev/a10ed3e7a53e https://hg.mozilla.org/mozilla-central/rev/8e167bb3d4f5 https://hg.mozilla.org/mozilla-central/rev/a5355966197c https://hg.mozilla.org/mozilla-central/rev/0b0e5290d011 https://hg.mozilla.org/mozilla-central/rev/28a2a7730361
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Comment 142•8 years ago
|
||
This shows a 9% improvement on the compositor microbenchmark: https://treeherder.mozilla.org/perf.html#/alerts?id=698 graph: https://treeherder.mozilla.org/perf.html#/graphs?timerange=1209600&series=%5Bmozilla-inbound,d1ef2786d9d4c555c38dada5755ac95cf2f2eafb,1%5D&selected=%5Bmozilla-inbound,d1ef2786d9d4c555c38dada5755ac95cf2f2eafb%5D
Reporter | ||
Comment 143•8 years ago
|
||
^ That's pretty surprising to me, I don't see anything in the code that should cause that.
Assignee | ||
Comment 144•8 years ago
|
||
I did some local runs; attached is the plot of the results. They're a bit noisy, but there does seem to be a pattern of improvement...
Reporter | ||
Comment 145•8 years ago
|
||
Hm, interesting. If it's easy enough, could you try to identify which patch of your patchset introduced the improvement? AFAICT none of that code should be getting exercised during this microbenchmark.
Assignee | ||
Comment 146•8 years ago
|
||
Here are the results of running the microbenchmark after applying each patch in the patch series in order. The values are the average scores over 20 local runs, and the error bars are the standard deviations. Again the results are a bit noisy, but it looks like, if any patch is responsible for the improvement, it's the first one (the ScrollMetadata refactoring).
Assignee | ||
Comment 147•8 years ago
|
||
This time with the error bars actually there.
Attachment #8740521 -
Attachment is obsolete: true
Reporter | ||
Comment 148•8 years ago
|
||
Thanks for looking into this. I guess we can chalk the improvement up to removing stuff from FrameMetrics in part 1, although I think the benchmark is really quite sensitive to respond to such a change.
Assignee | ||
Comment 149•8 years ago
|
||
I got profiles of local runs before and after the refactoring, with perf [1]: http://people.mozilla.org/~bballo/bug1219296-profile-before-refactoring http://people.mozilla.org/~bballo/bug1219296-profile-after-refactoring Haven't looked at / analyzed them yet, just posting them for reference. [1] https://perf.wiki.kernel.org/index.php/Main_Page
Assignee | ||
Comment 150•8 years ago
|
||
I looked at the profiles; the most frequently sampled code was the comparison between the composited test and reference images. This is fairly incidental to the performance part of the test, which aims to measure the performance of the compositing, not of the comparison, so I commented out the comparison and got new profiles. In the new profiles, the most frequently sampled code was in the loader, suggesting that the test wasn't spending enough time in the actual operation (compositing) compared to program startup. So I tweaked the test again, increasing the repetition count from 30 to 3000. In the resulting test runs, the patch shows about a 5% improvement over the baseline, down from 10% before the test changes. I got new before/after profiles, and uploaded them (same links as in comment 149). The profiles are now dominated by blitting, which is more expected. BenWa and I looked at the profiles in perf, and they looked very similar; we couldn't easily pinpoint a particular function that was responsible for the 5% difference. (Part of the reason for this is that perf's report view isn't nearly as rich as Cleopatra's UI, so it's harder to spot patterns.) Our best candidate explanation is that the changes to the FrameMetrics structure altered the code locality properties of the program. At this point, I don't think it's worth investigating further, so I'm not going to. The profiles are available if anyone feels like looking at them more (and I can get more with other tweaks to the tests if necessary).
Reporter | ||
Comment 151•8 years ago
|
||
Comment on attachment 8731503 [details] MozReview Request: Bug 1219296 - Split fields not needed for repaints out from FrameMetrics. r=kats (Applies to all patches on this bug) Approval Request Comment [Feature/regressing bug #]: This patchset moves the scroll snapping implementation off the main thread and into the compositor thread [User impact if declined]: much riskier to uplift bug 1255823, which is a fix for beta topcrasher [Describe test coverage new/current, TreeHerder]: there are automated tests that cover this code. I did a try push with this rebased to beta (everything applied pretty cleanly) at https://treeherder.mozilla.org/#/jobs?repo=try&revision=444d4e11fab6&group_state=expanded, try push is clean except for unrelated oranges [Risks and why]: fairly low risk, the code is well understood and there have been no regressions so far, just one follow-up patch in bug 1267471 which should be uplifted too. I'll flag that for uplift as well. [String/UUID change made/needed]: none
Attachment #8731503 -
Flags: approval-mozilla-beta?
Comment 152•8 years ago
|
||
Comment on attachment 8731503 [details] MozReview Request: Bug 1219296 - Split fields not needed for repaints out from FrameMetrics. r=kats We need this fix for addressing the crash spike seen in bug 1268881, I've been told this is the least risky option and this affects e10s only mode, Beta47+
Attachment #8731503 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
status-firefox47:
--- → affected
Reporter | ||
Comment 153•8 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/c4799db0ba51 https://hg.mozilla.org/releases/mozilla-beta/rev/44f54c8a08d7 https://hg.mozilla.org/releases/mozilla-beta/rev/555d371a0644 https://hg.mozilla.org/releases/mozilla-beta/rev/38f83a4a93ff https://hg.mozilla.org/releases/mozilla-beta/rev/b7c724aa60a9 https://hg.mozilla.org/releases/mozilla-beta/rev/3afd68069d6f https://hg.mozilla.org/releases/mozilla-beta/rev/2b8a637fd08e https://hg.mozilla.org/releases/mozilla-beta/rev/405420cecb4e https://hg.mozilla.org/releases/mozilla-beta/rev/755eeaa09615 https://hg.mozilla.org/releases/mozilla-beta/rev/e4bc0660f832
Comment 154•8 years ago
|
||
Pushed by kgupta@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/833474c9330b Followup to fix stale code comments. r=me and DONTBUILD
Comment 155•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/833474c9330b
You need to log in
before you can comment on or make changes to this bug.
Description
•