Botond Ballo [:botond] Reporter
	Description • 7 years ago

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.

	Botond Ballo [:botond] Reporter
	Comment 1 • 6 years ago

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

	jlogandavison Assignee
	Comment 2 • 6 years ago

(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.

	jlogandavison Assignee
	Comment 3 • 6 years ago

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

	Botond Ballo [:botond] Reporter
	Comment 4 • 6 years ago

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?

	Botond Ballo [:botond] Reporter
	Comment 5 • 6 years ago

(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!

	jlogandavison Assignee
	Comment 6 • 6 years ago

(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

	Botond Ballo [:botond] Reporter
	Comment 7 • 6 years ago

(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 :)

	jlogandavison Assignee
	Comment 8 • 6 years ago

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.

	Botond Ballo [:botond] Reporter
	Comment 9 • 6 years ago

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 //

	Botond Ballo [:botond] Reporter
	Comment 10 • 6 years ago

(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!

	Botond Ballo [:botond] Reporter
	Comment 11 • 6 years ago

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.

	Botond Ballo [:botond] Reporter
	Comment 12 • 6 years ago

(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.

	jlogandavison Assignee
	Comment 13 • 6 years ago

(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|.

	jlogandavison Assignee
	Comment 14 • 6 years ago

(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?

	Botond Ballo [:botond] Reporter
	Comment 15 • 6 years ago

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().

	Botond Ballo [:botond] Reporter
	Comment 16 • 6 years ago

(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.

	Botond Ballo [:botond] Reporter
	Comment 17 • 6 years ago

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!

	Botond Ballo [:botond] Reporter
	Comment 18 • 6 years ago

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.

	Botond Ballo [:botond] Reporter
	Comment 19 • 6 years ago

(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.

	jlogandavison Assignee
	Comment 20 • 6 years ago

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?

	Botond Ballo [:botond] Reporter
	Comment 21 • 6 years ago

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

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

Thanks!

	Botond Ballo [:botond] Reporter
	Comment 22 • 6 years ago

(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/

	Botond Ballo [:botond] Reporter
	Comment 23 • 6 years ago

For now, I pushed the new patch to Try:

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

	Botond Ballo [:botond] Reporter
	Comment 24 • 6 years ago

(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?

	jlogandavison Assignee
	Comment 25 • 6 years ago

(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?

	Botond Ballo [:botond] Reporter
	Comment 26 • 6 years ago

(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

	Botond Ballo [:botond] Reporter
	Comment 27 • 6 years ago

(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 :)

	Botond Ballo [:botond] Reporter
	Comment 28 • 6 years ago

Hey, how are things going here? :)

	Botond Ballo [:botond] Reporter
	Comment 39 • 6 years ago

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

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

LGTM, thanks!

	Pulsebot
	Comment 52 • 6 years ago

Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b3afa9f33be
Make pinch locking unaffected by input sensitivity. r=botond