Closed Bug 960565 Opened 6 years ago Closed 6 years ago

mozilla::layers::GestureEventListener gets into inconsistent state when pinching

Categories

(Core :: Panning and Zooming, defect)

29 Branch
ARM
Mer
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29
Tracking Status
firefox27 --- unaffected
firefox28 --- fixed
firefox29 --- fixed
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- verified

People

(Reporter: dmitry.rojkov, Assigned: dmitry.rojkov)

References

Details

(Keywords: verifyme)

Attachments

(1 file, 1 obsolete file)

When zooming in or out a page in SailfishOS browser built with Gecko 29.0 with pich-to-zoom gesture it gets scaled up or down unpredictably quite often.

As far as I understood this happens because at the moment of transition from GESTURE_NONE to GESTURE_PINCH GestureEventListener.mTouches may still contain touch points left from a previous gesture.

The patch below seems to work around the problem:

diff --git a/gfx/layers/ipc/GestureEventListener.cpp b/gfx/layers/ipc/GestureEventListener.cpp
index 597fb16..d6fc536 100644
--- a/gfx/layers/ipc/GestureEventListener.cpp
+++ b/gfx/layers/ipc/GestureEventListener.cpp
@@ -64,6 +64,9 @@ nsEventStatus GestureEventListener::HandleInputEvent(const InputData& aEvent)
   {
   case MultiTouchInput::MULTITOUCH_START:
   case MultiTouchInput::MULTITOUCH_ENTER: {
+    if (mState == GESTURE_NONE) {
+      mTouches.Clear();
+    }
     for (size_t i = 0; i < event.mTouches.Length(); i++) {
       bool foundAlreadyExistingTouch = false;
       for (size_t j = 0; j < mTouches.Length(); j++) {

But it's not clear why the listener ends up with non-empty mTouches after a gesture has been handled. Probably would make sense to have better controlled transitions from one state to another.
This might be related to bug 937343. Are you willing to look into this bug a bit more to track down why it happens?
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #1)
> This might be related to bug 937343. Are you willing to look into this bug a
> bit more to track down why it happens?

Yes, the bug may well happen because of the way how MultiTouchInput events are generated in our embedding. I've just realized that when user lifts up one finger while zooming the embedding stops to generate events for the second finger. Effectively outdating the content of mTouches. This might be an adaptation specific issue.

I'll update the bug when find out more what's going on.
I've looked into how MultiTouchInput is handled in APZCTreeManager and found that MultiTouchInput of type MULTITOUCH_START is supposed to contain only one touch point. Here

http://hg.mozilla.org/mozilla-central/file/1b52aa569ced/gfx/layers/composite/APZCTreeManager.cpp#l323

mTouchCount gets incremented by 1 for every incoming MULTITOUCH_START input and decremented by multiTouchInput.mTouches.Length() upon MULTITOUCH_END.

But our embedding may send several touch points in one MULTITOUCH_START input. It's been done this way to make Google Maps handle pinch-to-zoom correctly. If every "touch start" event contains only one touch point then the map starts to zoom in only if one puts down two fingers at once.
IIUC this is the place in gecko code that mandated this behaviour
http://hg.mozilla.org/mozilla-central/annotate/1d9c510b3742/layout/base/nsPresShell.cpp#l6135

Thus
1. if user puts down two fingers at once then only one MULTITOUCH_START input is sent with two touch points
2. APZCTreeManager::mTouchCount becomes equal 1 (despite the fact there are two touch points)
3. then user lifts one finger up -> MULTITOUCH_END input is sent with one released touch point
4. APZCTreeManager::mTouchCount becomes equal 0 (despite the fact one finger is still down)
5. APZCTreemanager::mApzcForInputBlock gets nullptr -> no updates get sent to APZController any more
6. user lifts his second finger -> NS_WARNING("Got an unexpected touchend/touchcancel") gets triggered.
7. this last MULTITOUCH_END never reaches APZController and the obsolete touch point gets stuck in AsyncPanZoomController::mTouches.

So, when I fixed the embedding to send MULTITOUCH_START event with only one touch point as expected by APZCTreeManager pinch-to-zoom started to work much better, but GoogleMaps got broken.

Then again the line http://hg.mozilla.org/mozilla-central/file/1b52aa569ced/gfx/layers/composite/APZCTreeManager.cpp#l325 suggests that MULTITOUCH_START input actually might contain more than one touch point.

Is there any spec describing what is expected from embedding regarding MultiTouchInput?
Thanks for looking into this! You are right, there is a bug at this line:

> http://hg.mozilla.org/mozilla-central/file/1b52aa569ced/gfx/layers/composite/
> APZCTreeManager.cpp#l323
> 
> mTouchCount gets incremented by 1 for every incoming MULTITOUCH_START input
> and decremented by multiTouchInput.mTouches.Length() upon MULTITOUCH_END.

mTouchCount should be set to the number of touch points that are active, which I think will be touches.Length(). The code at http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/APZCTreeManager.cpp#441 needs to change similarly. Would you be able to write a patch for that?
Status: UNCONFIRMED → NEW
Ever confirmed: true
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #4)
> mTouchCount should be set to the number of touch points that are active,
> which I think will be touches.Length(). The code at

Not really. If there are two MULTITOUCH_START coming in then the second one contains the point already counted in the first MULTITOUCH_START.

> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/
> APZCTreeManager.cpp#441 needs to change similarly. Would you be able to
> write a patch for that?

Sure, will try to do that tomorrow.
(In reply to dmitry.rojkov from comment #5)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #4)
> > mTouchCount should be set to the number of touch points that are active,
> > which I think will be touches.Length(). The code at
> 
> Not really. If there are two MULTITOUCH_START coming in then the second one
> contains the point already counted in the first MULTITOUCH_START.

Yeah, so "mTouchCount = multiTouchInput.touches.Length()" should work, right?

> > http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/
> > APZCTreeManager.cpp#441 needs to change similarly. Would you be able to
> > write a patch for that?
> 
> Sure, will try to do that tomorrow.

Thanks!
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #6)
> (In reply to dmitry.rojkov from comment #5)
> > (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #4)
> > > mTouchCount should be set to the number of touch points that are active,
> > > which I think will be touches.Length(). The code at
> > 
> > Not really. If there are two MULTITOUCH_START coming in then the second one
> > contains the point already counted in the first MULTITOUCH_START.
> 
> Yeah, so "mTouchCount = multiTouchInput.touches.Length()" should work, right?

Ah, that's right. I misunderstood your previous comment. Now pinching seems to work flawlessly.
Comment on attachment 8363551 [details] [diff] [review]
Reset touch counter with every touch start

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

Looks good. If you want me to land it I can fix the typo and land it. Otherwise please upload an updated copy with the typo fixed and word "[PATCH]" removed from the subject line, because otherwise mercurial will include that as part of the commit message. Thanks!

::: gfx/layers/composite/APZCTreeManager.cpp
@@ +280,5 @@
>      case MULTITOUCH_INPUT: {
>        const MultiTouchInput& multiTouchInput = aEvent.AsMultiTouchInput();
>        if (multiTouchInput.mType == MultiTouchInput::MULTITOUCH_START) {
> +        // MULTITOUCH_START input contains all active touches of the current
> +        // session thus reseting mTouchCount.

typo: "reseting" should "resetting"

@@ +409,5 @@
>      return ret;
>    }
>    if (aEvent.message == NS_TOUCH_START) {
> +    // NS_TOUCH_START event contains all active touches of the current
> +    // session thus reseting mTouchCount.

Same here
Attachment #8363551 - Flags: review+
Tanks! Typo fixed.
Attachment #8363551 - Attachment is obsolete: true
Comment on attachment 8363646 [details] [diff] [review]
Reset touch counter with every touch start

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

Awesome, thanks!
Attachment #8363646 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/5eb6c6cdbcdd
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Comment on attachment 8363646 [details] [diff] [review]
Reset touch counter with every touch start

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 927033
User impact if declined: as described in comment 3 (starting a pinch with both fingers simultaneously will misbehave on some hardware)
Testing completed (on m-c, etc.): by contributor
Risk to taking this patch (and alternatives if risky): low-risk. affects b2g and metro, but the code is well understood
String or IDL/UUID changes made by this patch: none
Attachment #8363646 - Flags: approval-mozilla-aurora?
Attachment #8363646 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
1.3 Zooming is difficult, only works when both fingers are placed onscreen at the same time, Pinching only works when both fingers are placed onscreen at the same time.

1.4: Pinching and zooming work consistently when adding and removing fingers.

1.3 Environmental Variables:
Device: Buri 1.3 MOZ
BuildID: 20140528024003
Gaia: 0ce948e378cab7ed3db20231281dd7ca2eb99779
Gecko: ce0dd450bffb
Version: 28.0
Firmware Version: v1.2-device.cfg

1.4 Environmental Variables:
Device: Buri 1.4 MOZ
BuildID: 20140527000202
Gaia: 0542778892a294d224e75af4a76be5d42938bc90
Gecko: d583ae109f54
Version: 30.0
Firmware Version: v1.2-device.cfg
You need to log in before you can comment on or make changes to this bug.