Closed Bug 1365088 Opened 7 years ago Closed 7 years ago

APZ hit testing unnecessarily calls HitTestingTreeNode::Untransform() twice on each node

Categories

(Core :: Panning and Zooming, enhancement, P3)

Product:
Core
Component:
Panning and Zooming
Type:
enhancement

Tracking

()

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

People

(Reporter: botond, Assigned: botond)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(1 file)

Bug 1365088 - Avoid calling HitTestingTreeNode::Untransform() twice for each node during hit testing.
59 bytes, text/x-review-board-request
kats
: review+
Details 
The first call is in the pre-action of the ForEachNode call in APZCTreeManager::GetAPZCAtPoint().

The second call is inside the HitTest() call in the post-action of the same ForEachNode call.

The two calls operate on the same point, so they're redundant.
I had to remove the MOZ_ASSERT(!IsOutsideClip(aPoint)) from HitTestingTreeNode::HitTest(), since it now takes the point already in Layer space.

I think that should be fine. HitTest() only has one call site, and it's clear from the structure of that code that we only call it if ItOutsideClip() was false.
Blocks: 1364622
Comment on attachment 8867921 [details]
Bug 1365088 - Avoid calling HitTestingTreeNode::Untransform() twice for each node during hit testing.

https://reviewboard.mozilla.org/r/139452/#review143038

r+ assuming the pop/top ordering change was an error and can be undone.

::: gfx/layers/apz/src/APZCTreeManager.cpp:1868
(Diff revision 1)
>          HitTestResult hitResult = aNode->HitTest(hitTestPoints.top());
> +        hitTestPoints.pop();

I don't understand why you swapped the order here. Shouldn't the pop() call still go before we read the top() value? The rest of the code seems to introduce no functional changes, so I wouldn't expect that a functional change is needed here.
Attachment #8867921 - Flags: review?(bugmail) → review+
Priority: -- → P3
Whiteboard: [gfx-noted]
The pop/top ordering change is deliberate, and important for correctness.

Consider a layer tree with a parent layer P and a child layer C. Suppose C has bounds (0,0,50,50), and a scale transform of 2x, so that it takes up (0,0,100,100) in screen space. Suppose P has bounds (0,0,200,200).

In this scenario, hit-testing the screen point (75, 75) should hit C.

Here's what happens during that hit testing before this change:

  start: push (75, 75) onto the stack
    pre-action for P: transform (75, 75) into (75, 75), push (75, 75) onto the stack
      pre-action for C: transform (75, 75) into (37.5, 37.5), push (37.5, 37.5) onto the stack
      post-action for C: {
        pop (37.5, 37.5) from the stack
        grab (75, 75) from the top of the stack
        pass (75, 75) into HitTest
          which transforms it into (37.5, 37.5) again
          which PASSES the hit test, so we hit C
      }
                         
Here's what happens with my change:

  start: push (75, 75) onto the stack
    pre-action for P: transform (75, 75) into (75, 75), push (75, 75) onto the stack
      pre-action for C: transform (75, 75) into (37.5, 37.5), push (37.5, 37.5) onto the stack
      post-action for C: {
        grab (37.5, 37.5) from the top of the stack
        pass (37.5, 37.5) into HitTest
          which no longer transforms it
          which PASSES the hit test, so we hit C
        pop (37.5, 37.5) from the stack
      }

Here's what would happen if we didn't switch the order of the pop and top calls:

  start: push (75, 75) onto the stack
    pre-action for P: transform (75, 75) into (75, 75), push (75, 75) onto the stack
      pre-action for C: transform (75, 75) into (37.5, 37.5), push (37.5, 37.5) onto the stack
      post-action for C: {
        pop (37.5, 37.5) from the stack
        grab (75, 75) from the top of the stack
        pass (75, 75) into HitTest
          which no longer transforms it
          which FAILS the hit test, so we end up hitting P instead
      }

I added a gtest exercising this scenario. The gtest passes with the code before the change, and with my change, but fails if we don't switch the order of the pop and top calls. (To exercise the scenario, I also had a fix a bug in the gtest fixture where we were incorrectly casting a LayerRect into ParentLayer space without using the transform to convert).
Here's a conceptual explanation of the change: 

Previously, the pre-action would push a point onto the stack for use by the node's children's post-actions. Since nodes wanted a ParentLayer point in their post-action (to pass into HitTest), the Layer point pushed by a node was suitable for consumption by its children.

Now, the pre-action pushes a point onto the stack for use by the node's own post-action. Since nodes now want a Layer point in their post-action (to pass into HitTest), the Layer point pushed by a node is suitable for consumption by its own post-action.

Since the post-action now wants to consume the point its own pre-action pushed, rather than the point its parent's pre-action pushed, it looks at the top of the stack before popping.
Ah, that makes sense. Thanks for the explanation!
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3f659e813e75
Avoid calling HitTestingTreeNode::Untransform() twice for each node during hit testing. r=kats
https://hg.mozilla.org/mozilla-central/rev/3f659e813e75
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: