1451461 - Have pinch locking consider more than just the one most recent pinch gesture event

Status: RESOLVED FIXED

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

Description

[Botond Ballo [:botond]]

In bug 1180865, we implemented a "pinch locking" feature where we detect whether a two-finger touch gesture looks more like a two-finger pan than a pinch, and if so, "lock" it so that it can only do panning, not zooming.

To determine whether a gesture looks like a two-finger pan or not, we look at the most recent pinch gesture event and compare its span change and focus change to some thresholds.

There is a potential problem with this, as described in bug 1428387 comment 8:

> If you have sufficiently sensitive hardware, it might send events 
> frequently enough that the change between one event and the next 
> won't be larger than our thresholds even if the movement (over 
> multiple events) is relatively large. It may be better to make 
> the decisions based on some sort of *cumulative* span and focus 
> changes over multiple events.

This bug tracks the suggested improvement on tracking cumulative span and focus changes and making the decision based on that.

Comment 1

[Botond Ballo [:botond]]

Hey jlogandavison, are you interested in working on this? If not, I have another contributor who may be interested.

Comment 2

[jlogandavison]

(In reply to Botond Ballo [:botond] from comment #1)
> Hey jlogandavison, are you interested in working on this? If not, I have
> another contributor who may be interested.

Yes, sorry. I've been looking at this. Been away this past week though, so I'm still in an early investigatory stage. I'll make sure to post an update asap.

Comment 3

[jlogandavison]

I'm afraid to say I've wasted a little bit of time on this. I thought a good starting point would be to write a test that exposed the issue at hand. Essentially sending a pinch gesture split into many-many small PINCH_MOVE events and observing the resulting behavior of the APZC. Problem was it became very difficult to verify that my test code was correct. APZC behavior would either not break when you expect that it would, or it would break but possibly because of bad input from bad test code.

In any case, I've decided to step back and take a different approach. I've been looking at other code in the APZC and GestureEventListener to see how thresholding was being handled elsewhere.

Pinch gestures are triggered in the GestureEventListener based on a threshold [1], but it appears to work with cumulative distances only, so it should be immune to sensitive hardware.

HandlePanning in APZC uses an angle based threshold [2]. Despite the fact it only operates with (I think) delta distances from the most recent event, the angle it calculates should be consistent irrespective of the actual magnitude of those deltas, so it shouldn't be affected by sensitive hardware.

HandlePanningUpdate uses a threshold to decide when to break the pan lock [3]. It's working based on |PanGestureInput::mPanDisplacement| [4] which I think is also a cumulative value, I'm not sure.

=============

I think an angle based threshold might work quite well for us in this case. With a pinch, the angle between the vectors of each finger's path should be close to 180 deg (and pointing in opposite directions). Whereas with a two finger pan the vectors should be closer to 0 deg (and pointing in the same direction).

Only issue with this is I'm not sure that we have enough information in |PinchGestureInput| to derive the vectors for each individual finger. We can take spanDistance and focusChange to make an estimate I think, but that would tell us nothing of the actual angle of the span between two fingers (ie: we don't know whether the fingers are placed horizontally on the screen and panning vertically or placed vertically and panning horizontally).

=============

A cumulative threshold I think would also work in theory. The main issue I can see with that approach is that it cannot very easily detect a change in direction mid-gesture. For example, for our PINCH_STICKY mode requires us to be able to detect when a gesture stops being a pan and begins being a pinch again.

=============

One other approach that might work would be a buffered event approach. Essentially we accumulate PINCH_MOVE events until either a PINCH_END event or the cumulative focusChange/spanDistance reaches a reasonable size. Then we pass the cumulative event to the |HandlePinchLocking| as a single event. This way |HandlePinchLocking| will never receive events too frequently, but it does feel a little like patching over the problem rather than actually fixing it.

=============

What are your thoughts?

[1]: https://searchfox.org/mozilla-central/source/gfx/layers/apz/src/GestureEventListener.cpp#336-368
[2]: https://searchfox.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp#2798,2803
[3]: https://searchfox.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp#2820
[4]: https://searchfox.org/mozilla-central/source/widget/InputData.h#361-362

Comment 4

[Botond Ballo [:botond]]

Great analysis, thanks!

(In reply to jlogandavison from comment #3)
> Pinch gestures are triggered in the GestureEventListener based on a
> threshold [1], but it appears to work with cumulative distances only, so it
> should be immune to sensitive hardware.

Agreed.

> HandlePanning in APZC uses an angle based threshold [2]. Despite the fact it
> only operates with (I think) delta distances from the most recent event, the
> angle it calculates should be consistent irrespective of the actual
> magnitude of those deltas, so it shouldn't be affected by sensitive hardware.
> 
> HandlePanningUpdate uses a threshold to decide when to break the pan lock
> [3]. It's working based on |PanGestureInput::mPanDisplacement| [4] which I
> think is also a cumulative value, I'm not sure.

HandlePanning and HandlePanningUpdate both have two call sites: one for touch events (used for touchscreens), and one for pan gesture events (used for Mac trackpads). The touch event ones definitely use a cumulative delta. The pan gesture event ones appear to use a delta that's not cumulative; this could probably use some improvement.

> =============
> 
> I think an angle based threshold might work quite well for us in this case.
> With a pinch, the angle between the vectors of each finger's path should be
> close to 180 deg (and pointing in opposite directions). Whereas with a two
> finger pan the vectors should be closer to 0 deg (and pointing in the same
> direction).
> 
> Only issue with this is I'm not sure that we have enough information in
> |PinchGestureInput| to derive the vectors for each individual finger. We can
> take spanDistance and focusChange to make an estimate I think, but that
> would tell us nothing of the actual angle of the span between two fingers
> (ie: we don't know whether the fingers are placed horizontally on the screen
> and panning vertically or placed vertically and panning horizontally).
> 
> =============
> 
> A cumulative threshold I think would also work in theory. The main issue I
> can see with that approach is that it cannot very easily detect a change in
> direction mid-gesture. For example, for our PINCH_STICKY mode requires us to
> be able to detect when a gesture stops being a pan and begins being a pinch
> again.
> 
> =============
> 
> One other approach that might work would be a buffered event approach.
> Essentially we accumulate PINCH_MOVE events until either a PINCH_END event
> or the cumulative focusChange/spanDistance reaches a reasonable size. Then
> we pass the cumulative event to the |HandlePinchLocking| as a single event.
> This way |HandlePinchLocking| will never receive events too frequently, but
> it does feel a little like patching over the problem rather than actually
> fixing it.
> 
> =============
> 
> What are your thoughts?

Using the angle between the vectors representing each finger's movement is a very interesting idea! If we wanted to implement this, we could modify PinchGestureInput to retain the individual touch event coordinates.

That said, I think that the problem of detecting a change in direction mid-pinch, is shared by both the angle approach, and the cumulative-delta approach. As an example: suppose you do a two-finger pan for a long time, like 500px, and then switch to a pinch, where you move the fingers 100px apart. Assuming you're measuring the angle from the beginning of the gesture, the vectors will still be fairly close to parallel.

One approach to deal with this is to maintain a history of pinch gesture events, but limited to a certain amount of time in the past, rather than going all the way back to the beginning of the gesture. Every time a new pinch gesture event arrives, it's added to the history, and events older than the time threshold are removed from the history. We then take a cumulative metric over the events in the history, and make a locking decision based on that.

This would work with either cumulative span / focus change, or with an angle. I would suggest the span / focus change approach, as it's easier to implement (no need to store the touch coordinates in the pinch gesture event).

How does that sound?

Comment 5

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #4)
> HandlePanning and HandlePanningUpdate both have two call sites: one for
> touch events (used for touchscreens), and one for pan gesture events (used
> for Mac trackpads). [...] 
> The pan gesture event ones appear to use a delta that's not cumulative; this
> could probably use some improvement.

Actually, we may have just stumbled onto the cause of bug 1467380!

Comment 6

[jlogandavison]

(In reply to Botond Ballo [:botond] from comment #4)

Okay, so I've attempted an implementation for both the angle based and the span/focus change (where a short history is maintained) approaches.

The angle based approach did not turn out to be as promising as I first thought. There are a fair few edge cases that lead to the lock triggering during a legitimate pinch gesture. For example, two fingers moving in parallel (so the angle is close to zero, triggering the lock), but one finger moves a significantly shorter distance than the other. The fingers are diverging, this a pinch gesture, the lock shouldn't engage here.

The logic for the angle based lock is fairly simplistic, so maybe it could work better with some re-thinking.

> void AsyncPanZoomController::HandlePinchLocking(ParentLayerPoint pinchVector1,
>                                                 ParentLayerPoint pinchVector2) {
>   //Math to calculate the angle
>   float dotProduct = (pinchVector1.x * pinchVector2.x)
>                    + (pinchVector1.y * pinchVector2.y);
>   float magnitudes = ((float)pinchVector1.Length()) * ((float)pinchVector2.Length());
>   float angle = acos(dotProduct / magnitudes);
> 
>   // Check is angle is close to zero (parallel vectors)
>   if (fabs(angle) > M_PI / 4) {
>     if(GetPinchLockMode() == PINCH_STICKY) {
>       mPinchLocked = false;
>     }
>   } else {
>     if(GetPinchLockMode() != PINCH_FREE) {
>       mPinchLocked = true;
>     }
>   }
> }


=======

In any case, I've also attempted an implementation that draws from a short history of events. To do this I'm using the nsDeque data structure. It seemed a good fit, a small ring-buffer sounds like it should work efficiently in this case (a small number of events in a queue with regular evictions from the front and regular insertions onto the back).

nsDeque works with void pointers. Since we're being passed references to |PinchGestureInput| objects I suppose I need to allocate a copy that I have ownership of and then pass _that_ pointer to the nsDeque. What is the usual approach to heap allocations in apz code? Is it best to use a unique_ptr, or similar?

My current implementation just declares an nsDeque as a member of the APZC and handles the queue in |OnScale/Begin/End()|, but I've been toying with a slightly different design.

What do you think to creating an |InputEventBuffer| class? This would essentially wrap nsDeque and deal internally with evicting events older than a given time. This way you could peek() and iterate the buffer freely in APZC code with some guarantee that the events in the buffer are consistent. We could even make the wrapper type-safe (ie: InputEventBuffer<PinchGestureInput>). I've seen examples of other type-safe wrappers of nsDeque (eg: MediaQueue [1])

Overkill for this sort of thing?

[1]: https://searchfox.org/mozilla-central/source/dom/media/MediaQueue.h

Comment 7

[Botond Ballo [:botond]]

(In reply to jlogandavison from comment #6)
> In any case, I've also attempted an implementation that draws from a short
> history of events. To do this I'm using the nsDeque data structure. It
> seemed a good fit, a small ring-buffer sounds like it should work
> efficiently in this case (a small number of events in a queue with regular
> evictions from the front and regular insertions onto the back).

The problem with keeping a fixed number of events in the history is that it doesn't control for hardware sensitivity. If one piece of hardware sends events every 10ms and another every 50ms, then the history would cover a 5x shorter time period for the first one.

I think it would be better to pick a threshold based on elapsed time, and keep all events within that time window; the number of events in the history would be variable.

> nsDeque works with void pointers. Since we're being passed references to
> |PinchGestureInput| objects I suppose I need to allocate a copy that I have
> ownership of and then pass _that_ pointer to the nsDeque. What is the usual
> approach to heap allocations in apz code? Is it best to use a unique_ptr, or
> similar?

We have some existing code that keeps copies of input events in a queue, in InputQueue.h. It uses nsTArray as the data structure, and UniquePtr<InputData> to store the events (where PinchGestureInput derives from InputData).

I don't have a strong opinion on what to use as the container: we could use nsDeque, or std::deque (I think we tend to use the latter in newer code, like in APZUpdater.h), or even nsTArray or std::vector (since the number of elements stored will be fairly small, removing elements from the beginning not being constant time isn't a big problem; InputQueue also removes elements from the beginning even though it uses nsTArray [1]).

I also don't have a strong opinion on what to use as the element type, but just to throw another option out there: rather than storing the entire event object, we could store a smaller structure that just contains the relevant fields (timestamp, focus point, and span change).

> What do you think to creating an |InputEventBuffer| class? This would
> essentially wrap nsDeque and deal internally with evicting events older than
> a given time. This way you could peek() and iterate the buffer freely in
> APZC code with some guarantee that the events in the buffer are consistent.
> We could even make the wrapper type-safe (ie:
> InputEventBuffer<PinchGestureInput>). I've seen examples of other type-safe
> wrappers of nsDeque (eg: MediaQueue [1])
> 
> Overkill for this sort of thing?

I would be fine with using a wrapper class.

I'll leave these choices (container type, element type, and whether to use a wrapper class) up to you. Use your judgment :)

Comment 8

[jlogandavison]

Attachment: patch.diff

I've opted to implement this by wrapping std::deque.

I've added a new class InputBuffer which delegates most of its functionality to the internal deque. The only added functionality is that it maintains a TimeDuration "lifetime" value and, when |InputBuffer::push()| is called, it flushes any events from the front of the deque that are older than the newest event by the given lifetime (eg: if the lifetime is 50ms, the oldest event in the buffer is guaranteed to be no more than 50ms older than the newest event).

I've adapted OnScale/OnScaleBegin/OnScaleEnd to push events to this buffer, and the pinch locking code has been adapted to derive the spanDistance and focusChange across the buffer (using InputBuffer::front()/back()).

The code still maintains un-buffered spanDistance and focusChange values because these are required for the actual animation.

-----

An few things I'm unsure about, mostly with regards to my implementation of InputBuffer. Is the general style and layout acceptable?

It's a template, really only intended to work with subclasses of |InputData|, but technically I guess it'll compile as long as the class has mTimeStamp as a field. I considered possibly adding a static_assert to explicitly restrict usage to |InputData| subclasses, but I'm not sure if that's necessary.

Of course, the other option is to change the push() method to accept a timestamp argument (eg: void push(T element, TimeStamp ts); ). Store the elements as a struct agnostic to their internal fields eg:

struct buffer_element {
    T element,
    TimeStamp timestamp
}

And then the buffer will be compatible with any type. But I digress...

The only other thing I haven't considered is thread safety. Is it expected that containers in the Mozilla codebase _are_ threadsafe (by way of internally implemented mutexes, etc), or should we be doing concurrency control in the APZC code.

----

Last note: I think we might have to adjust our breakout thresholds for this change. In particular I've noticed that the STICKY mode is not very sticky any more, but improves if span_breakout_threshold is adjusted.

Comment 9

[Botond Ballo [:botond]]

Comment on attachment 9005819 [details] [diff] [review]
patch.diff

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

Thanks, this looks pretty good!

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +825,4 @@
>       mY(this),
>       mPanDirRestricted(false),
>       mPinchLocked(false),
> +     mPinchEventBuffer(InputBuffer<PinchGestureInput>(TimeDuration::FromMilliseconds(50))),

It's probably worth exposing this quantity as a pref.

@@ +1478,5 @@
>      RecursiveMutexAutoLock lock(mRecursiveMutex);
>  
> +    mPinchEventBuffer.push(aEvent);
> +
> +    focusPoint = mPinchEventBuffer.back().mLocalFocusPoint - mFrameMetrics.GetCompositionBounds().TopLeft();

What was wrong with using aEvent.mLocalFocusPoint here?

@@ +1480,5 @@
> +    mPinchEventBuffer.push(aEvent);
> +
> +    focusPoint = mPinchEventBuffer.back().mLocalFocusPoint - mFrameMetrics.GetCompositionBounds().TopLeft();
> +
> +    // Focus change and span distance calculated from an event buffer

It might be cleaner to modify HandlePinchLocking() to take just |focusPoint| as an argument, and compute all the |buffered...| variables (and make the ToScreenCoordinates() calls) inside HandlePinchLocking(). What do you think?

::: gfx/layers/apz/src/InputBuffer.h
@@ +11,5 @@
> +
> +namespace mozilla {
> +namespace layers {
> +
> +template <typename T>

Let's call the template parameter something more descriptive, like "Event". Let's also document that the class requires it to have a field of type TimeStamp named |mTimeStamp|.

@@ +12,5 @@
> +namespace mozilla {
> +namespace layers {
> +
> +template <typename T>
> +class InputBuffer {

I would give this class a name that's a bit more specific, especially as we already have an InputQueue class in APZ code. Suggestion: RecentEventsBuffer?

@@ +28,5 @@
> +  typedef typename std::deque<T>::iterator iterator;
> +  typedef typename std::deque<T>::const_iterator const_iterator;
> +  iterator begin() { return mBuffer.begin(); }
> +  iterator end() { return mBuffer.end(); }
> +  const_iterator cbegin() { return mBuffer.cbegin(); }

cbegin() and cend() can be const.

@@ +40,5 @@
> +  reference back() { return mBuffer.back(); }
> +  const_reference back() const { return mBuffer.back(); }
> +
> +private:
> +  TimeDuration mBufferLifetime;

I would call this "mEventsLifetime", since it refers to the lifetime of the events in the buffer, not the lifetime of the buffer itself (which will be much longer).
"mMaxAge" would also work.

@@ +47,5 @@
> +
> +template <typename T>
> +InputBuffer<T>::InputBuffer(TimeDuration lifetime)
> +    : mBufferLifetime(lifetime)
> +    , mBuffer(std::deque<T>()) {}

mBuffer() is sufficient (it has the same effect)

@@ +51,5 @@
> +    , mBuffer(std::deque<T>()) {}
> +
> +template <typename T>
> +void InputBuffer<T>::push(T element) {
> +  // Events must be push in chronological order

push -> pushed

@@ +56,5 @@
> +  MOZ_ASSERT(mBuffer.empty() || mBuffer.back().mTimeStamp <= element.mTimeStamp);
> +
> +  mBuffer.push_back(element);
> +
> +  //Flush all events older than the given lifetime

nit: space after //

Comment 10

[Botond Ballo [:botond]]

(In reply to jlogandavison from comment #8)
> An few things I'm unsure about, mostly with regards to my implementation of
> InputBuffer. Is the general style and layout acceptable?

Yup, style and layout looks great!

> It's a template, really only intended to work with subclasses of
> |InputData|, but technically I guess it'll compile as long as the class has
> mTimeStamp as a field. I considered possibly adding a static_assert to
> explicitly restrict usage to |InputData| subclasses, but I'm not sure if
> that's necessary.
> 
> Of course, the other option is to change the push() method to accept a
> timestamp argument (eg: void push(T element, TimeStamp ts); ). Store the
> elements as a struct agnostic to their internal fields eg:
> 
> struct buffer_element {
>     T element,
>     TimeStamp timestamp
> }
> 
> And then the buffer will be compatible with any type. But I digress...

Modulo my comments above, I think it's fine like this.

> The only other thing I haven't considered is thread safety. Is it expected
> that containers in the Mozilla codebase _are_ threadsafe (by way of
> internally implemented mutexes, etc), or should we be doing concurrency
> control in the APZC code.

General-purpose containers, which may have both single-threaded and multi-threaded uses, tend not to be threadsafe, so as to not impose the cost of synchronization on their single-threaded users.

Special-purpose containers tend to be threadsafe or not depending on whether they are used from multiple threads.

In this case, InputBuffer is only used from the OnScale...() functions, which are only called from the APZ controller thread*, so there is no need for thread safety. However, it is worth documenting at the declaration of mPinchEventBuffer that it should only be accessed on the controller thread.

* That the OnScale...() functions are only called on the controller thread isn't documented super well, but as a general rule, all input handling functions will be called on the controller thread, and the only caller of the OnScale...() functions is HandleGestureEvent() which asserts at the beginning that we're on the controller thread.

> Last note: I think we might have to adjust our breakout thresholds for this
> change. In particular I've noticed that the STICKY mode is not very sticky
> any more, but improves if span_breakout_threshold is adjusted.

Sounds good!

Comment 11

[Botond Ballo [:botond]]

By the way, feel free to try out our new code review system, Phabricator:

https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html

Its use is optional at this point, but I believe the plan is to eventually deprecate Splinter (the Bugzilla extension used to review patch files uploaded as Bugzilla attachments), so it's probably worth trying Phabricator at some point, especially if you plan to continue contributing to the codebase.

Comment 12

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #9)
> @@ +1480,5 @@
> > +    mPinchEventBuffer.push(aEvent);
> > +
> > +    focusPoint = mPinchEventBuffer.back().mLocalFocusPoint - mFrameMetrics.GetCompositionBounds().TopLeft();
> > +
> > +    // Focus change and span distance calculated from an event buffer
> 
> It might be cleaner to modify HandlePinchLocking() to take just |focusPoint|
> as an argument, and compute all the |buffered...| variables (and make the
> ToScreenCoordinates() calls) inside HandlePinchLocking(). What do you think?

Correction: HandlePinchLocking() would also need to take the composition bounds origin (Metrics().GetCompositionBounds().TopLeft()) as an argument, because we can't access Metrics() outside of the lock.

Not sure if that changes the analysis, I'll leave it up to you.

Comment 13

[jlogandavison]

Attachment: patch.diff

(In reply to Botond Ballo [:botond] from comment #9)

I've implemented your suggestions.

Renamed the class to |RecentEventsBuffer|. I also preferred |maxAge| in place of |lifetime| so I've changed that name too.

I've extracted the maxAge of the buffer into a pref |APZPinchLockBufferMaxAge|.
I've set it as a |Once| pref instead of a |Live| pref, mainly because changing the pref won't effect the buffer until it is reinitialized. There's no indication of this in about:config though. No prompt to restart the browser. Does this exist as a feature?

Improved documentation in a couple of places. Firstly, a comment above |RecentEventsBuffer| to specify the need for a |mTimeStamp| field. Secondly, to document the thread-unsafety of |mPinchEventBuffer|.

Comment 14

[jlogandavison]

Attachment: potential_refactor.diff

(In reply to Botond Ballo [:botond] from comment #9)

This is a potential approach to cleaning up pinch-locking code in |OnScale()|.

Essentially I've moved all the buffered calculations into |HandlePinchLocking()|. This includes moving the mutex block into the function as well. These calculations depend on mPinchEventBuffer and mLastZoomFocus only, so HandlePinchLocking() now has zero arguments.

Furthermore, I've moved the un-buffered focusPoint, focusChange, and mLastZoomFocus calculations back to where they were before the patch for bug1180865. This eliminates the need for the first Mutex blocks in |OnScale()|

How does this look?

Comment 15

[Botond Ballo [:botond]]

Comment on attachment 9008236 [details] [diff] [review]
patch.diff

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

Looks good, one minor comment:

::: gfx/layers/apz/src/RecentEventsBuffer.h
@@ +14,5 @@
> +/**
> + * RecentEventsBuffer: maintains an age constrained buffer of events
> + *
> + * Intended for use with elements of type InputData, but the only requirement
> + * is a member called "mTimeStamp"

That member should also have type TimeStamp, since we subtract a TimeDuration from it in push().

Comment 16

[Botond Ballo [:botond]]

(In reply to jlogandavison from comment #13)
> I've extracted the maxAge of the buffer into a pref
> |APZPinchLockBufferMaxAge|.
> I've set it as a |Once| pref instead of a |Live| pref, mainly because
> changing the pref won't effect the buffer until it is reinitialized. There's
> no indication of this in about:config though. No prompt to restart the
> browser. Does this exist as a feature?

Unfortunately not.

In general, about:config is intended mostly for developers and the occasional power user who "knows what they're doing". My main motivation for making things like this prefs, is so that if e.g. we decide that the parameter needs some tweaking, new values can be tested without needing a local build.

Comment 17

[Botond Ballo [:botond]]

Comment on attachment 9008239 [details] [diff] [review]
potential_refactor.diff

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

This is even nicer than what I suggested - thanks!

Comment 18

[Botond Ballo [:botond]]

I pushed the patches to Try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=89949bcff776e6c3dc9fd15e0f2f809e6fb6c710

It probably makes sense to combine the two patches into one prior to landing.

Comment 19

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #18)
> I pushed the patches to Try:
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=89949bcff776e6c3dc9fd15e0f2f809e6fb6c710

There are some build failures that need fixing. Based on a quick skim, a missing #incude <deque>, and a missing "explicit" on the RecentEventsBuffer constructor.

Comment 20

[jlogandavison]

Attachment: patch.diff

Squashed the two commits + modifications to address build errors.

My local built fine, so I guess this is to do with differences between compilers/c++ versions right?

Out of interest, is there a public try server, or is commit access required?

Comment 21

[Botond Ballo [:botond]]

Comment on attachment 9009450 [details] [diff] [review]
patch.diff

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

Thanks!

Comment 22

[Botond Ballo [:botond]]

(In reply to jlogandavison from comment #20)
> My local built fine, so I guess this is to do with differences between
> compilers/c++ versions right?

The #include <deque> issue is likely due to "unified compilation". This is a feature of our build system where we concatenate several .cpp files into one prior to compilation, to save time (by reducing the number of times the compiler has to process common headers).

The exact sets of files that are concatenated varies from platform to platform. What likely happened, is that on Windows, AsyncPanZoomController.cpp was concatenated with another file that included <deque>, thereby providing the necessary dependency implicitly. On other platforms, this was not the case, and we got an error.

The solution to issues like this is to practise "include what you use" - e.g. if your file uses std::deque, then #include <deque>, regardless of whether you actually need it for the code to compile in your current build configuration.

The error about the implicit constructor is produced by our custom static analysis plugin, which doesn't run on local builds unless you explicitly enable it.

> Out of interest, is there a public try server, or is commit access required?

Good question!

Pushing to Try requires "Level 1" commit access. You can read about the levels here [1].

As you've contributed a number of patches by now, it would be appropriate for you to apply for Level 1 commit access, if you'd like. Instructions can be found here [2]. I would be happy to vouch for you!

[1] https://www.mozilla.org/en-US/about/governance/policies/commit/access-policy/
[2] https://www.mozilla.org/en-US/about/governance/policies/commit/

Comment 23

[Botond Ballo [:botond]]

For now, I pushed the new patch to Try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ebd793d00b69aad89eb5d82f99f977653d56ba28

Comment 24

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #23)
> For now, I pushed the new patch to Try:
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=ebd793d00b69aad89eb5d82f99f977653d56ba28

So now the patch is compiling on all platforms, but we have some GTest failures.

 * APZCPinchLockingTester.Pinch_Locking_Sticky_Lock_Break_Lock is failing

 * In debug builds, we're getting an assertion failure in code called
   from RecentEventsBuffer::push() (you can see the failure log for a
   stack trace)

I also remembered this earlier note:

(In reply to jlogandavison from comment #8)
> Last note: I think we might have to adjust our breakout thresholds for this
> change. In particular I've noticed that the STICKY mode is not very sticky
> any more, but improves if span_breakout_threshold is adjusted.

Did you want to make such adjustment as part of this patch?

Comment 25

[jlogandavison]

(In reply to Botond Ballo [:botond] from comment #24)
> Did you want to make such adjustment as part of this patch?

I opted, not to modify anything after re-evaluating the behavior. It seems to operate fine on a device, including successful re-locking after a lock "break" in STICKY_MODE.

I believe the reason why the test fails is because time doesn't progress throughout the duration of the gestures. So, during the second panning gesture, the buffer will still contain the pinch, which I guess is large enough that it outweighs the final  pan.

May have to adjust the test code to simulate the passage of time between gestures. Not sure how to achieve that. I tried using |mcc-AdvanceBy()|, but that doesn't seem to work (all the events still have the same mTimeStamp).

=======

As for the other error.
> Assertion failure: !IsNull() (Cannot compute with a null value), at /.../include/mozilla/TimeStamp.h:541

This would occur if the mTimeStamp property of an event passed to |RecentEventsBuffer::push| was the "null" moment.

We probably want some sort of error if this occurs in non-test code right? Or alternatively a default behavior, but I can't think of a reasonable behavior in this case. Maybe flush the buffer and revert to un-buffered behavior?

Comment 26

[Botond Ballo [:botond]]

(In reply to jlogandavison from comment #25)
> I believe the reason why the test fails is because time doesn't progress
> throughout the duration of the gestures. So, during the second panning
> gesture, the buffer will still contain the pinch, which I guess is large
> enough that it outweighs the final  pan.
> 
> May have to adjust the test code to simulate the passage of time between
> gestures. Not sure how to achieve that. I tried using |mcc-AdvanceBy()|, but
> that doesn't seem to work (all the events still have the same mTimeStamp).

We had a similar issue in bug 1355656. As you can see in the patch that landed there [1], the solution involved not only calling mcc->AdvanceBy(), but also passing mcc->Time() to the constructors of the events so their timestamp fields are initialized to the correct time.

> As for the other error.
> > Assertion failure: !IsNull() (Cannot compute with a null value), at /.../include/mozilla/TimeStamp.h:541
> 
> This would occur if the mTimeStamp property of an event passed to
> |RecentEventsBuffer::push| was the "null" moment.
> 
> We probably want some sort of error if this occurs in non-test code right?
> Or alternatively a default behavior, but I can't think of a reasonable
> behavior in this case. Maybe flush the buffer and revert to un-buffered
> behavior?

I expect this will be fixed by the change described above.

[1] https://hg.mozilla.org/mozilla-central/rev/21861ee0ddb8

Comment 27

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #26)
> We had a similar issue in bug 1355656. As you can see in the patch that
> landed there [1], the solution involved not only calling mcc->AdvanceBy(),
> but also passing mcc->Time() to the constructors of the events so their
> timestamp fields are initialized to the correct time.

Actually, you were the one who wrote that patch :)

Comment 28

[Botond Ballo [:botond]]

Hey, how are things going here? :)

Comment 29

[Botond Ballo [:botond]]

I'm going to return this bug to the pool of mentored bugs available for others to pick up.

jlogandavison, if you'd like to continue working on this after all, feel free to say so!

Comment 30

[jlogandavison]

Attachment: patch.diff

Apologies for the long absence. I'm still around.

I've rebased my current patch onto a more recent changeset. It seems that the APZC code has been linted since I last touched it. Should I be running a linter on my changes too?

Comment 31

[jlogandavison]

Attachment: test_patch.diff

And this patch fixes the failing test in TestPinching.cpp.

It was simply a case of properly simulating the progress of time with mcc-AdvanceBy() and events with properly initialised mTimeStamp attributes.

I'll squash this in once we're happy with it.

Comment 32

[Botond Ballo [:botond]]

New Try push:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a826aa745316139676dec299e98fefaebec6b708

Comment 33

[Botond Ballo [:botond]]

(In reply to jlogandavison from comment #30)

It seems that the APZC code has been linted since I last touched it.

The entire codebase was formatted with clang-format.

Should I be running a linter on my changes too?

If you could run mach clang-format, that would be helpful. You may need to re-run mach bootstrap to install the clang-format binary.

Comment 34

[jlogandavison]

Attachment: patch.diff

Comment 35

[jlogandavison]

Attachment: test_patch.diff

Comment 36

[jlogandavison]

(In reply to Botond Ballo [:botond] from comment #33)

I've attached clang-formatted versions of the patches.

(In reply to Botond Ballo [:botond] from comment #32)

Seems there is still a failure relating to this:

Assertion failure: !IsNull() (Cannot compute with a null value), at /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/TimeStamp.h:525

The failing test is APZCBasicTester.Overzoom which does indeed make use of the CreatePinchGestureInput utility with the default null timestamp.

We have other tests using null timestamped events, should we prefer to "fix" these tests, or include some form of null check in the RecentEventsBuffer code?

Comment 37

[Botond Ballo [:botond]]

I would prefer that we fix the tests. In fact, I would make the timestamp parameter to CreatePinchGestureInput() required. There are only a handful of other call sites.

Comment 38

[jlogandavison]

Attachment: test_patch.diff

So this became a bit of a chunky change. Similar to bug 1355656, I've migrated PinchWithPinchInput into APZCTesterBase. This is so that it has access to the mcc member.

So PinchWithPinchInput now passes mcc-Time() to CreatePinchGestureInput and also uses mcc->AdvanceBy(50) between events.

Plus a few calls in TestPinching.cpp, this covers all the call sites of CreatePinchGestureInput.

Comment 39

[Botond Ballo [:botond]]

Comment on attachment 9044917 [details] [diff] [review]
test_patch.diff

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

LGTM, thanks!

Comment 40

[Botond Ballo [:botond]]

The patch needs rebasing across bug 1515774.

Comment 41

[jlogandavison]

Attachment: patch.diff

Rebased

Comment 42

[jlogandavison]

Attachment: test_patch.diff

Rebased. Changes to CreatePinchGestureInput incorporated.

Comment 43

[Botond Ballo [:botond]]

Thanks! Try push:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf750ef4dcf9917a59d5c87241294c2edd102b66

Comment 44

[Botond Ballo [:botond]]

Looks like APZCGestureDetectorTester.Pan_After_Pinch is still failing with an assertion in debug builds.

Comment 45

[Botond Ballo [:botond]]

The backtrace in the Treeherder log is not symbolicated, but I got one from a local run:

#0  mozilla::layers::RecentEventsBuffer<mozilla::PinchGestureInput>::push (this=0x7fffdfa89600, event=...)
    at /home/botond/dev/projects/mozilla/sabaton/gfx/layers/apz/src/RecentEventsBuffer.h:66
#1  0x00007fffe7f87e7a in mozilla::layers::AsyncPanZoomController::OnScaleBegin (this=<optimized out>, aEvent=...)
    at /home/botond/dev/projects/mozilla/sabaton/gfx/layers/apz/src/AsyncPanZoomController.cpp:1526
#2  0x00007fffe7f874c8 in mozilla::layers::AsyncPanZoomController::HandleGestureEvent (this=0x7fffdfa89000, aEvent=...)
    at /home/botond/dev/projects/mozilla/sabaton/gfx/layers/apz/src/AsyncPanZoomController.cpp:1210
#3  0x00007fffe7f96ee0 in mozilla::layers::GestureEventListener::HandleInputTouchMove (this=0x7fffdfae3820)
    at /home/botond/dev/projects/mozilla/sabaton/gfx/layers/apz/src/GestureEventListener.cpp:349
#4  0x00007fffe7f85278 in mozilla::layers::AsyncPanZoomController::HandleInputEvent (this=0x7fffdfa89000, aEvent=..., aTransformToApzc=...)
    at /home/botond/dev/projects/mozilla/sabaton/gfx/layers/apz/src/AsyncPanZoomController.cpp:1094
#5  0x00007fffe7fb5c7e in mozilla::layers::InputQueue::ProcessQueue (this=0x7fffdfaaaba0)
    at /home/botond/dev/projects/mozilla/sabaton/gfx/layers/apz/src/InputQueue.cpp:746
#6  0x00007fffe7fb4c4b in mozilla::layers::InputQueue::ReceiveTouchInput (this=<optimized out>, aTarget=..., aFlags=..., aEvent=..., aOutInputBlockId=0x12c00000002)
    at /home/botond/dev/projects/mozilla/sabaton/gfx/layers/apz/src/InputQueue.cpp:164
#7  0x00007fffe7fb4699 in mozilla::layers::InputQueue::ReceiveInputEvent (this=0x7fffdfaaaba0, aTarget=..., aFlags=..., aEvent=..., aOutInputBlockId=0x0)
    at /home/botond/dev/projects/mozilla/sabaton/gfx/layers/apz/src/InputQueue.cpp:38
#8  0x00007fffeb4d10b0 in TestAsyncPanZoomController::ReceiveInputEvent (this=0x7fffdfa89000, aEvent=..., aOutInputBlockId=0x0)
    at /home/botond/dev/projects/mozilla/sabaton/gfx/layers/apz/test/gtest/APZTestCommon.h:254
#9  APZCGestureDetectorTester_Pan_After_Pinch_Test::TestBody (this=0x7fffde5213c0)
    at /home/botond/dev/projects/mozilla/sabaton/gfx/layers/apz/test/gtest/TestGestureDetector.cpp:64

This is the codepath where a gesture event is synthesized from touch. Should be easy to fix by using non-zero timestamps in the Pan_After_Pinch testcase.

Comment 46

[Botond Ballo [:botond]]

By the way, you can run the APZ gtests in debug mode locally by doing a debug build (add ac_add_options --enable-debug to your .mozconfig) and running ./mach gtest 'APZ*'.

Comment 47

[jlogandavison]

Attachment: patch.diff

Rebased onto fd67a4332060

Comment 48

[jlogandavison]

Attachment: test_patch.diff

Rebased also. Includes further fixes.

Thank you Botond for surfacing the backtrace for me. It seems my local build didn't have debug flags enabled. Not sure how that happened, so my apologies for that.

For clarity, the adjusted tests are:

TestGestureDetector.cpp

• Pan_After_Pinch

TestPinching.cpp

• Pinch_NoSpan
• Pinch_TwoFinger_APZZoom_Disabled_Bug1354185
• Methods in the APZCPinchLockingTester class

This is on top of the modifications to PinchWithPinchInput (now found in APZTestCommon.h)

Comment 49

[Botond Ballo [:botond]]

Thanks for the update! New Try push:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b98426adbf361f2f04ad158f69aed02ebedad2e3

Comment 50

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #49)

New Try push:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b98426adbf361f2f04ad158f69aed02ebedad2e3

Whoops, hg import managed to commit a version control conflict marker...

There doesn't seem to be an actual conflict though. Let's try that again:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=17041f9f494133b40008fbddb39420e7feebe4a3

Comment 51

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #50)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=17041f9f494133b40008fbddb39420e7feebe4a3

That's looking good! I'll go ahead and fold the two patches and land the result.

Comment 52

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b3afa9f33be
Make pinch locking unaffected by input sensitivity. r=botond

Comment 53

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/5b3afa9f33be

Comment 54

[Botond Ballo [:botond]]

Looks like we're all done here :)

Thanks for your contribution and your patience in seeing it through to the end!

Comment 55

[jlogandavison]

(In reply to Botond Ballo [:botond] from comment #54)

Thanks for your contribution and your patience in seeing it through to the end!

Awesome! And thanks to you too for helping to get this landed.