927027 - First touchmove event in a pinch has incorrect touch data on hidpi devices

Status: RESOLVED FIXED

(Core Graveyard :: Widget: WinRT, defect)

Description

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

The symptoms I was investigating are something like this:
1) Do a pinch-zoom
2) Put two fingers on the screen
3) Move your fingers a teeny tiny bit to do a zoom
4) Lift the two fingers from the screen

The expected results from steps 2-4 is that the page zoom changes a tiny amount. The actual results are that the zoom of the page jumps drastically, usually right when you put your fingers on the screen.

I added some logging and found a couple of possible causes for this. I'm filing this bug to track one of the causes and will clone it to track the other cause.

The output I get with my logging is this:
---------------------
APZC got 2 touches with f=(1488 753) s=(1475 398) span=355.237946
APZC: 19CC0390 got a scale-begin in state 3
mozilla::widget::winrt::MetroInput::DispatchTouchCancel
APZC got 2 touches with f=(656 332) s=(650 175) span=157.114609
APZC: 19CC0390 got a scale in state 8
APZC: span ratio is 157.114609 / 355.237946 == 0.442280
APZC: Performed scale; new zoom is 1.400291
APZC got 2 touches with f=(1488 753) s=(1475 398) span=355.237946
APZC: 19CC0390 got a scale in state 8
APZC: span ratio is 355.237946 / 157.114609 == 2.261012
[snip]
---------------

The interesting bit here is the second "got 2 touches" line, where you can see the coordinates of the two touch points ("f" is first and "s" is second). Note how they are almost half of the touch point coordinates in the other two similar output lines. I believe these touch points are the untransformed coordinates returned by the APZC from the first touch points.

I tried poking around the code in MetroInput.cpp but didn't spend enough time to figure out exactly where the bogus values are coming from, but I suspect that the code in AppendToTouchList is part of the problem. This takes the points from mTouches and populates a WidgetTouchEvent instance, but it does so by copying the nsRefPtr<Touch> instances, rather than copying the Touch instances directly. So when the underlying Touch instances get modified by the APZC in APZCTreeManager::ReceiveInputEvent(WidgetInputEvent&), this is never accounted for in MetroInput.cpp, and it later sends another WidgetTouchEvent with the already-transformed touch events.

Comment 1

[Jim Mathies [:jimm]]

blocking bug 915289 which is a bad zoom bug.

Comment 2

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

Attachment: Patch

This fixes the problem I was seeing. The touch point instance was getting untransformed by the APZ code and that was mutating it directly in the touch list that is stored in MetroInput.cpp. I don't know if this patch is actually correct in other respects and/or breaks other use cases, so please review thoroughly (or just steal it and rework it however you wish). In particular I didn't really verify that this doesn't leak stuff but assumed that since it's in a nsRefPtr it'll get cleaned up appropriately.

Comment 3

[Jim Mathies [:jimm]]

Comment on attachment 828238 [details] [diff] [review]
Patch

Making a copy looks ok to me. Tim should probably look this over since he wrote our touch tracking code in MetroInput. I don't think we rely on mChanged anymore down in MetroInput either (we used to).

Comment 4

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Comment on attachment 828238 [details] [diff] [review]
Patch

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

> This fixes the problem I was seeing. The touch point instance was getting
> untransformed by the APZ code and that was mutating it directly in the touch
> list that is stored in MetroInput.cpp. I don't know if this patch is
> actually correct in other respects and/or breaks other use cases, so please
> review thoroughly (or just steal it and rework it however you wish).

I don't think we were relying on our event dispatcher to modify our touch points during dispatch (if we were, things would be really screwed up ever since we switched to async dispatching), so this patch looks good to me. I'm not able to effectively test that touch input is working properly since other bugs cause my usual test case to crash the browser, so I'm in favor of landing this patch and addressing other issues as they come up.

> In
> particular I didn't really verify that this doesn't leak stuff but assumed
> that since it's in a nsRefPtr it'll get cleaned up appropriately.

Looks fine to me on the "don't leak" front.

::: widget/windows/winrt/MetroInput.cpp
@@ +182,5 @@
> +               aData->mRefPoint,
> +               aData->mRadius,
> +               aData->mRotationAngle,
> +               aData->mForce);
> +    touches->AppendElement(copy);

It should be possible to do this without creating the temporary. I believe that one (or maybe both) of the following approaches would work:
  touches->AppendElement(new Touch(aData->mIdentifier, ...));
-OR-
  nsRefPtr<Touch>& element = touches->AppendElement();
  element = new Touch(aData->mIdentifier, ...);

That being said, the temporary isn't really creating any additional overhead so I don't mind keeping it around if we feel that it's more readable this way.

Comment 5

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Oops, I meant to mention earlier:
The only possible negative consequence of this patch that jumps out at me is performance. For every touchmove event, we're now making a copy of every touch point. For multi-finger apps on slow hardware, this might make a difference. We should monitor our perf to see if we need to take steps to mitigate this effect.

Comment 6

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

(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #4)
> It should be possible to do this without creating the temporary. I believe
> that one (or maybe both) of the following approaches would work:
>   touches->AppendElement(new Touch(aData->mIdentifier, ...));
> -OR-
>   nsRefPtr<Touch>& element = touches->AppendElement();
>   element = new Touch(aData->mIdentifier, ...);
> 
> That being said, the temporary isn't really creating any additional overhead
> so I don't mind keeping it around if we feel that it's more readable this
> way.

Yeah either of those should work as well but I figured giving the copy a name made the intent a little more explicit.

(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #5)
> Oops, I meant to mention earlier:
> The only possible negative consequence of this patch that jumps out at me is
> performance. For every touchmove event, we're now making a copy of every
> touch point. For multi-finger apps on slow hardware, this might make a
> difference. We should monitor our perf to see if we need to take steps to
> mitigate this effect.

Copying the touch point is pretty cheap so I don't think it'll be much of a problem but yeah, still something to keep an eye out for.

Landed on fx-team:

https://hg.mozilla.org/integration/fx-team/rev/6483170e11c3

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6483170e11c3