967471 - Moving your finger up and down quickly engages skating and causes massive checkerboarding

Status: RESOLVED WONTFIX

(Core :: Panning and Zooming, defect)

Description

[Doug Sherk (:drs) (inactive)]

I think the way to fix this is to not allow skating until the user's finger has been lifted off the screen for the first time since they were stationary. We can also reset this flag every time velocity hits 0.

Comment 1

[Doug Sherk (:drs) (inactive)]

:vingtetun says this is pretty bad, so I'm going to nom it for 1.3 and have a patch soon.

Comment 2

[Doug Sherk (:drs) (inactive)]

Attachment: Don't skate on first touch even if going fast

This seems to mostly mitigate checkerboarding when panning up and down quickly. I don't think this needs to be wrapped in a pref and I don't think it should regress anything.

Comment 3

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8370054 [details] [diff] [review]
Don't skate on first touch even if going fast

I think :botond has done more work on skating. :kats, feel free to take this over if you want.

Comment 4

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

My concern again with any patches of this nature (i.e. that can cause the displayport size to change rapidly in quick succession) is that it is expensive to resize the displayport unless we have tiling support. BenWa, can you confirm that this is still the case?

Comment 5

[Doug Sherk (:drs) (inactive)]

This isn't actually resizing the displayport any differently than we did before. All it does is prevent skating from kicking in until we really start moving. I just realized it might be better to just check if mAcceleration >= 1 instead.

Comment 6

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

Based on the code and what I've seen in practice mAcceleration is useless and should be removed. You have to fling five times in a row without the velocity dropping below some threshold for the acceleration factor to ever be nonzero. That's why I chose to not use the acceleration factor at all when I rewrote CalculatePendingDisplayPort recently.

Comment 7

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

To be clear, I think the current code that does acceleration should be removed and we should come up with a proper design for acceleration in APZ. I would be very reluctant to add code that depends on mAcceleration or the acceleration factor at this point.

Comment 8

[Benoit Girard (:BenWa)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #4)
> My concern again with any patches of this nature (i.e. that can cause the
> displayport size to change rapidly in quick succession) is that it is
> expensive to resize the displayport unless we have tiling support. BenWa,
> can you confirm that this is still the case?

Yes, the answer depends what backend we use:
1) Buffer Rotation: Resize can be very expensive because we risk having to reallocate the entire buffer saturating memory bandwidth and skipping frames.
2) Tiling: Resize are proportional to the area added but the allocation still isn't cheap.

So if 'rapidly' means ~ once a frame then even with tiling I'd avoid it.

Comment 9

[Doug Sherk (:drs) (inactive)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #7)
> To be clear, I think the current code that does acceleration should be
> removed and we should come up with a proper design for acceleration in APZ.
> I would be very reluctant to add code that depends on mAcceleration or the
> acceleration factor at this point.

We could tune the acceleration instead. On a side note, I've been experimenting with including the acceleration factor in skating calculations (I think it used to be there), though I have yet to see any obvious benefit yet.

Comment 10

[Doug Sherk (:drs) (inactive)]

(In reply to Benoit Girard (:BenWa) from comment #8)
> So if 'rapidly' means ~ once a frame then even with tiling I'd avoid it.

In this case, this was already happening. The proposed patch just delays it until later based on some heuristics.

Comment 11

[Botond Ballo [:botond]]

Comment on attachment 8370054 [details] [diff] [review]
Don't skate on first touch even if going fast

Review of attachment 8370054 [details] [diff] [review]:
-----------------------------------------------------------------

I am convinced by Doug's arguments in comment #5 and comment #10 that this does not need to be behind a pref. I do think we should test this on both B2G and Metro to make sure it doesn't regress checkerboarding behaviour before landing.

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +1285,4 @@
>  {
>    // Scale up the length using the appropriate multiplier and center the
>    // displayport around the visible area.
> +  bool skating = IsAxisSkating(aVelocity) && aAllowSkating;

Putting aAllowSkating first allows us to avoid evaluating IsAxisSkating() if aAllowSkating is false.

Comment 12

[Dylan Oliver [:doliver]]

triage: blocking a blocker

Comment 13

[Kai-Chih Hu [:khu]]

Can we get ETA to fix this bug? Thank you.

Comment 14

[Doug Sherk (:drs) (inactive)]

This isn't helping. I'm going back to the drawing board on it. I think there may still be something we can do here, but this patch isn't it.

Comment 15

[Jean Gong :jgong]

Doug, can you update the ETA on this? 
Thanks!

Comment 16

[Doug Sherk (:drs) (inactive)]

(In reply to Jean Gong from comment #15)
> Doug, can you update the ETA on this? 
> Thanks!

This is just a mitigation patch and isn't absolutely necessary so we can basically just give up on it at any time if needed.

Comment 17

[Doug Sherk (:drs) (inactive)]

I realized with tiling that this isn't an issue, since the old buffer doesn't get completely discarded when requesting a paint in a completely different region like it does with buffer rotation.

Comment 18

[Benoit Girard (:BenWa)]

(In reply to Doug Sherk (:drs) from comment #17)
> I realized with tiling that this isn't an issue, since the old buffer
> doesn't get completely discarded when requesting a paint in a completely
> different region like it does with buffer rotation.

Wasn't this bug tracking a correctness issue where the panning was zig-zaging? Tiling might provide better performance when the buffer region changes but it doesn't fix the original correctness bug. I don't understand why this was closed.

Comment 19

[Doug Sherk (:drs) (inactive)]

(In reply to Benoit Girard (:BenWa) from comment #18)
> Wasn't this bug tracking a correctness issue where the panning was
> zig-zaging? Tiling might provide better performance when the buffer region
> changes but it doesn't fix the original correctness bug. I don't understand
> why this was closed.

My idea here was that we should not enter the skating state while panning quickly up and down. The problem is that the tiny amount of time between those pans is important in initiating the paint in the case that the user doesn't go back and forth, but instead just in one direction repeatedly. It's hard to distinguish those until the user is actually panning in the opposite direction, and by then it's already too late.

So, without tiling, there was a trade-off of more checkerboarding when panning in the same direction quickly vs. not checkerboarding when changing directions many times quickly. With tiling, we don't have the latter issue, so there's no trade-off anymore.

Do you still see a problem here? If you've got any ideas, I'm all ears.