Closed Bug 1364525 Opened 7 years ago Closed 7 years ago

Move scrollbars with async scrolling

Categories

(Core :: Graphics: WebRender, enhancement, P3)

Product:
Core
Component:
Graphics: WebRender
Version:
Other Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(5 files)

Bug 1364525 - Ensure all scroll thumbs have an animations id.
59 bytes, text/x-review-board-request
pchang
: review+
Details
Bug 1364525 - Send the animation id for scrollbar thumbs over to the parent in the scroll data.
59 bytes, text/x-review-board-request
botond
: review+
Details
Bug 1364525 - Update APIs to allow APZ to produce scrollbar transforms.
59 bytes, text/x-review-board-request
botond
: review+
pchang
: review+
Details
Bug 1364525 - Produce scrollbar thumb transforms.
59 bytes, text/x-review-board-request
botond
: review+
Details
Bug 1364525 - Handle null-pointer dereference condition.
59 bytes, text/x-review-board-request
botond
: review+
Details 
This is the next WR/APZ thing I plan to work on - having the scrollbars move in the compositor with async scrolling.
Comment on attachment 8869449 [details]
Bug 1364525 - Send the animation id for scrollbar thumbs over to the parent in the scroll data.

https://reviewboard.mozilla.org/r/141082/#review144644
Attachment #8869449 - Flags: review?(botond) → review+
Comment on attachment 8869450 [details]
Bug 1364525 - Update APIs to allow APZ to produce scrollbar transforms.

https://reviewboard.mozilla.org/r/141084/#review144646

::: gfx/layers/apz/src/APZCTreeManager.h:163
(Diff revision 1)
> +   * to reflect the async scroll position, the updated transforms are appended
> +   * to the provided aTransformArray.
>     * Returns true if any APZ animations are in progress and we need to keep
>     * compositing.
>     */
>    bool PushStateToWR(wr::WebRenderAPI* aWrApi,

I don't love the fact that some of the information is provided to WR by calling methods on the WebRenderAPI, while some more is passed out through a raw out-parameter, but looking at how this is used in CompositeToTarget I understand why it's that way, so that's fine.

::: gfx/layers/wr/WebRenderBridgeParent.cpp:803
(Diff revision 1)
> -  mApi->GenerateFrame();
> +    mApi->GenerateFrame();
> +  }
>  
>    // XXX Enable it when async video is supported.
>    // if (!mCompositableHolder->GetCompositeUntilTime().IsNull()) {
> -  //   ScheduleComposition();
> +  //   scheduleComposite = true;

+1 for updating even commented code :)
Attachment #8869450 - Flags: review?(botond) → review+
Comment on attachment 8869451 [details]
Bug 1364525 - Produce scrollbar thumb transforms.

https://reviewboard.mozilla.org/r/141086/#review144652

::: gfx/layers/apz/src/APZCTreeManager.cpp:381
(Diff revision 1)
>    MutexAutoLock lock(mTreeLock);
>  
> +  // During the first pass through the tree, we build a cache of guid->HTTN so
> +  // that we can find the relevant APZC instances quickly in subsequent passes,
> +  // such as the one below to generate scrollbar transforms. Without this, perf
> +  // could end up being O(n^2) instead of O(n) because we'd have to search the

Lookups in std::map are O(log n). Consider using std::unordered_map, or adjust the comment to say O(n log n) instead of O(n) (similarly below).

::: gfx/layers/apz/src/APZCTreeManager.cpp:416
(Diff revision 1)
>          }
>  
> +        // Use a 0 presShellId because when we do a lookup in this map for the
> +        // scrollbar below we don't have (or care about) the presShellId.
> +        ScrollableLayerGuid guid(lastLayersId, 0, apzc->GetGuid().mScrollId);
> +        httnMap.insert(std::make_pair(guid, aNode));

Now that we can use C++11 library features, it's better to do:

  httnMap.emplace(guid, aNode);
  
(Fun fact: this avoids *two* copies of the pair. First, from the pair<Key, Value> we're constructing to the pair<const Key, Value> that insert() takes as an argument, and second, from the argument to the copy of the pair that actually lives inside the map.)

::: gfx/layers/apz/src/APZCTreeManager.cpp:454
(Diff revision 1)
> +        }
> +
> +        HitTestingTreeNode* scrollTargetNode = it->second;
> +        AsyncPanZoomController* scrollTargetApzc = scrollTargetNode->GetApzc();
> +        MOZ_ASSERT(scrollTargetApzc);
> +        LayerToParentLayerMatrix4x4 transform = scrollTargetApzc->CallWithLastContentPaintMetrics(

This snippet of code looks very familiar :)
Attachment #8869451 - Flags: review?(botond) → review+
Comment on attachment 8869452 [details]
Bug 1364525 - Handle null-pointer dereference condition.

https://reviewboard.mozilla.org/r/141088/#review144662
Attachment #8869452 - Flags: review?(botond) → review+
Comment on attachment 8869448 [details]
Bug 1364525 - Ensure all scroll thumbs have an animations id.

https://reviewboard.mozilla.org/r/141080/#review145002

::: gfx/layers/wr/WebRenderContainerLayer.cpp:103
(Diff revision 1)
> +      GetScrollThumbData().mDirection != ScrollDirection::NONE) {
> +    // A scroll thumb better not have a transform animation already or we're
> +    // going to end up clobbering it with APZ animating it too.
> +    MOZ_ASSERT(transformForSC);
> +
> +    EnsureAnimationsId();

Do we need to clean up this animation id somewhere for scrolling case? But I think it's fine for OMTA because it can reuse the id and attach with new animation data.
Attachment #8869448 - Flags: review?(howareyou322) → review+
Comment on attachment 8869450 [details]
Bug 1364525 - Update APIs to allow APZ to produce scrollbar transforms.

https://reviewboard.mozilla.org/r/141084/#review144998

r+ with comment addressed.

::: gfx/layers/wr/WebRenderBridgeParent.cpp:796
(Diff revision 1)
> +  if (PushAPZStateToWR(transformArray)) {
> +    scheduleComposite = true;
> +  }
> +
> +  if (!transformArray.IsEmpty() || !opacityArray.IsEmpty()) {
> +    mApi->GenerateFrame(opacityArray, transformArray);

If the animated transform/opacity array is not empty, we can just call generateFrame API with the animated array and schedule next composition directly. Then we don't need to introduce extra 'scheduleComposite' flag in line 787 or 792.


if (!transformArray.IsEmpty() || !opacityArray.IsEmpty()) {
  mApi->GenerateFrame(opacityArray, transformArray);
  ScheduleComposition();
} else {
  mApi->GenerateFrame();
}
Attachment #8869450 - Flags: review?(howareyou322) → review+
Comment on attachment 8869448 [details]
Bug 1364525 - Ensure all scroll thumbs have an animations id.

https://reviewboard.mozilla.org/r/141080/#review145502

::: gfx/layers/wr/WebRenderContainerLayer.cpp:103
(Diff revision 1)
> +      GetScrollThumbData().mDirection != ScrollDirection::NONE) {
> +    // A scroll thumb better not have a transform animation already or we're
> +    // going to end up clobbering it with APZ animating it too.
> +    MOZ_ASSERT(transformForSC);
> +
> +    EnsureAnimationsId();

I don't think any cleanup is needed here, since unlike with OMTA we're not storing any animation data in the compositor - we generate it on-the-fly in the APZ code.
Comment on attachment 8869450 [details]
Bug 1364525 - Update APIs to allow APZ to produce scrollbar transforms.

https://reviewboard.mozilla.org/r/141084/#review145504

::: gfx/layers/wr/WebRenderBridgeParent.cpp:796
(Diff revision 1)
> +  if (PushAPZStateToWR(transformArray)) {
> +    scheduleComposite = true;
> +  }
> +
> +  if (!transformArray.IsEmpty() || !opacityArray.IsEmpty()) {
> +    mApi->GenerateFrame(opacityArray, transformArray);

We also need to call ScheduleComposite if PushAPZStateToWR returns true. This can happen even if there's nothing in the transform array (e.g. if there is an APZ animation in progress but no scrollbars in the layer tree, because they are offscreen or something). I didn't want to end up calling ScheduleComposite() twice which is why I added the flag.
Comment on attachment 8869451 [details]
Bug 1364525 - Produce scrollbar thumb transforms.

https://reviewboard.mozilla.org/r/141086/#review145522

::: gfx/layers/apz/src/APZCTreeManager.cpp:381
(Diff revision 1)
>    MutexAutoLock lock(mTreeLock);
>  
> +  // During the first pass through the tree, we build a cache of guid->HTTN so
> +  // that we can find the relevant APZC instances quickly in subsequent passes,
> +  // such as the one below to generate scrollbar transforms. Without this, perf
> +  // could end up being O(n^2) instead of O(n) because we'd have to search the

Ah, for some reason I always assumed std::map was a standard hashtable with constant lookup! It looks like using unordered_map would require defining a hash function for ScrollableLayerGuid so for now I'll just update the comments and defer using unordered_map to a follow-up bug. I filed bug 1367062 for it. Might be a decent first bug for a new contributor.

::: gfx/layers/apz/src/APZCTreeManager.cpp:416
(Diff revision 1)
>          }
>  
> +        // Use a 0 presShellId because when we do a lookup in this map for the
> +        // scrollbar below we don't have (or care about) the presShellId.
> +        ScrollableLayerGuid guid(lastLayersId, 0, apzc->GetGuid().mScrollId);
> +        httnMap.insert(std::make_pair(guid, aNode));

Changed as you suggested.

::: gfx/layers/apz/src/APZCTreeManager.cpp:454
(Diff revision 1)
> +        }
> +
> +        HitTestingTreeNode* scrollTargetNode = it->second;
> +        AsyncPanZoomController* scrollTargetApzc = scrollTargetNode->GetApzc();
> +        MOZ_ASSERT(scrollTargetApzc);
> +        LayerToParentLayerMatrix4x4 transform = scrollTargetApzc->CallWithLastContentPaintMetrics(

Yeah, that's why I was quite happy to see your refactoring of that code :p
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2f39f53864f
Ensure all scroll thumbs have an animations id. r=pchang
https://hg.mozilla.org/integration/mozilla-inbound/rev/22bc11104b41
Send the animation id for scrollbar thumbs over to the parent in the scroll data. r=botond
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa1a20ec07bf
Update APIs to allow APZ to produce scrollbar transforms. r=pchang,botond
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e0b016f2d7a
Produce scrollbar thumb transforms. r=botond
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f91a68250b8
Handle null-pointer dereference condition. r=botond
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: