Right now if mouse drag events and touch events are interleaved, the code in InputQueue.cpp can't really manage them well, and it keeps clobbering and creating new input blocks. We need to be able to handle this scenario better, in particular before releasing touch on Windows. Also because the touch simulation in devtools exercises this scenario, where it synthesizes native touch events based on mouse input and so they can get interleaved very easily.

Fixing the interleaving between drag and wheel should also take care of bug 1269067.

Currently the input queue keeps a list of input blocks, and each input block has a list of input events. This setup makes it hard to handle interleaved input types properly - if you get a touch, mouse, touch, the two touch inputs should really go into the same block but right now they might get split up. What I would like to do is change it so that the InputQueue just keeps a list of input events, and each input event can then have a pointer to a block state object, which is shared across all events in the same block. We do somthing similar to this with the DragTracker object hanging off the InputQueue but we'll have to change it up a bit.

WIP so far: https://treeherder.mozilla.org/#/jobs?repo=try&revision=752836bbf5d5

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

I'll put my patches up for review tomorrow, after bug 1302093 and bug 1302128 are in central and I can rebase and push to MozReview more sanely. The try run in comment 4 is green; I didn't make any substantive changes since that try push. I did verify that the patches fix bug 1269067, and I wrote a gtest for that bug as well (which I'll post to that bug). The patches I have are structured like so: - The first few patches add the new data structure that will be used in InputQueue to track the input events. This is similar to the mInputBlockQueue that is currently there, but instead of being a list of blocks, each of which holds a list of events, the new mQueuedInputs just holds a list of "QueuedInput" objects. Each QueuedInput object is an event along with a pointer to the associated block. Effectively, InputQueue changes from holding a list of blocks to a list of events. - Once the new data structure is in place (parallel to the existing data structure), I update various pieces of code to use the new data structure instead of the old one. - Finally, once all the stuff is migrated over, I remove the old data structures and do some misc cleanup. Note that there are some semantic differences between how the old structures worked and the new ones worked. In particular, in the new code, there isn't just one active block, but potentially one active block of each input type. Fundamentally, this is the main change that allows us to interleave input events and still associate the new events with the right input blocks. However it also changes the meaning of CurrentBlock() and friends, so I renamed those and updated the call sites accordingly in one of the patches.

Comment on attachment 8790522 [details] Bug 1289432 - Use RefPtr instead of UniquePtr to hold the block state objects, since there will be multiple references to them soon. https://reviewboard.mozilla.org/r/78294/#review76982

Comment on attachment 8790523 [details] Bug 1289432 - Have the InputQueue keep RefPtrs to the active block of each input type. https://reviewboard.mozilla.org/r/78296/#review76986

Comment on attachment 8790524 [details] Bug 1289432 - Introduce a skeleton QueuedInput class. https://reviewboard.mozilla.org/r/78298/#review77002 Please add to the commit description: "Note that this patch makes the InputData hierarchy polymorphic".

Comment on attachment 8790525 [details] Bug 1289432 - Start populating the mQueuedInput array with queued input objects. https://reviewboard.mozilla.org/r/78300/#review77014 ::: gfx/layers/apz/src/InputQueue.cpp:178 (Diff revision 1) > INPQ_LOG("dropping event due to block %p being in mini-slop\n", block); > result = nsEventStatus_eConsumeNoDefault; > } > if (!MaybeHandleCurrentBlock(block, aEvent)) { > block->AddEvent(aEvent.AsMultiTouchInput()); > + mQueuedInputs.AppendElement(MakeUnique<QueuedInput>(aEvent.AsMultiTouchInput(), *block)); |aEvent| already has type |MultiTouchInput|, so there is no need to call |AsMultiTouchInput()| on it. (The same goes for the previous line, which was pre-existing.) ::: gfx/layers/apz/src/InputQueue.cpp:238 (Diff revision 1) > *aOutInputBlockId = block->GetBlockId(); > } > > if (!MaybeHandleCurrentBlock(block, aEvent)) { > block->AddEvent(aEvent.AsMouseInput()); > + mQueuedInputs.AppendElement(MakeUnique<QueuedInput>(aEvent.AsMouseInput(), *block)); Likewise here. ::: gfx/layers/apz/src/InputQueue.cpp:291 (Diff revision 1) > if (aOutInputBlockId) { > *aOutInputBlockId = block->GetBlockId(); > } > > // Copy the event, since WheelBlockState needs to affix a counter. > ScrollWheelInput event(aEvent); Do we need to copy the event twice? Can we unify this copy with the one made by the QueuedInput constructor? ::: gfx/layers/apz/src/InputQueue.cpp:335 (Diff revision 1) > if (!mInputBlockQueue.IsEmpty() && > aEvent.mType != PanGestureInput::PANGESTURE_START) { > block = mInputBlockQueue.LastElement()->AsPanGestureBlock(); > } > > PanGestureInput event = aEvent; Likewise here, can we avoid the second copy? ::: gfx/layers/apz/src/InputQueue.cpp:386 (Diff revision 1) > // null) should take priority. This is equivalent to just always using the > // target (confirmed or not) from the block, which is what > // MaybeHandleCurrentBlock() does. > if (!MaybeHandleCurrentBlock(block, event)) { > block->AddEvent(event.AsPanGestureInput()); > + mQueuedInputs.AppendElement(MakeUnique<QueuedInput>(event.AsPanGestureInput(), *block)); No need to call AsPanGestureInput().

Comment on attachment 8790525 [details] Bug 1289432 - Start populating the mQueuedInput array with queued input objects. https://reviewboard.mozilla.org/r/78300/#review77024

Comment on attachment 8790526 [details] Bug 1289432 - Switch over to using the mActiveXXXBlock in some places instead of fishing around in mInputBlockQueue. https://reviewboard.mozilla.org/r/78302/#review77026

Comment on attachment 8790523 [details] Bug 1289432 - Have the InputQueue keep RefPtrs to the active block of each input type. https://reviewboard.mozilla.org/r/78296/#review77028 ::: gfx/layers/apz/src/InputQueue.cpp:727 (Diff revision 1) > } > > // If we get here, we know there are more touch blocks in the queue after > // |curBlock|, so we can remove |curBlock| and try to process the next one. > INPQ_LOG("discarding processed %s block %p\n", curBlock->Type(), curBlock); > + ClearActiveBlock(curBlock); On second thought, I don't understand why it makes sense to clear the active block here. Suppose you have an input queue with two touch blocks, A and B, with A having arrived first. At this point, the active block is B. If we now process A and remove it from the queue - shouldn't the active block remain B?

Comment on attachment 8790527 [details] Bug 1289432 - Update checks for unprocessed events to use mQueuedInputs instead of checking the block is at the head. https://reviewboard.mozilla.org/r/78304/#review77032 ::: gfx/layers/apz/src/InputQueue.cpp:72 (Diff revision 1) > } > > bool > InputQueue::MaybeHandleCurrentBlock(CancelableBlockState *block, > const InputData& aEvent) { > - if (block == CurrentBlock() && block->IsReadyForHandling()) { > + if (mQueuedInputs.IsEmpty() && block->IsReadyForHandling()) { Is this equivalent? What if the block is the current block, but it already has some inputs?

(In reply to Botond Ballo [:botond] from comment #24) > On second thought, I don't understand why it makes sense to clear the active > block here. > > Suppose you have an input queue with two touch blocks, A and B, with A > having arrived first. > > At this point, the active block is B. > > If we now process A and remove it from the queue - shouldn't the active > block remain B? Yes. I think this is handled by the fact that ClearActiveBlock does checks if aBlock == mActiveTouchBlock. So in your example it would be A == B which would fail and mActiveTouchBlock would not get cleared. (Also this code is cleaned up some more in a later patch)

(In reply to Botond Ballo [:botond] from comment #25) > > - if (block == CurrentBlock() && block->IsReadyForHandling()) { > > + if (mQueuedInputs.IsEmpty() && block->IsReadyForHandling()) { > > Is this equivalent? What if the block is the current block, but it already > has some inputs? If it already has some inputs, then we don't want to be dispatching aEvent right away (which is what that function does, by calling block->DispatchImmediate(aEvent). So in the scenario you describe we should actually return false without dispatching the event, and the mQueuedInputs.IsEmpty() check should ensure that. This function is also removed entirely in a later patch. :)

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #26) > Yes. I think this is handled by the fact that ClearActiveBlock does checks > if aBlock == mActiveTouchBlock. So in your example it would be A == B which > would fail and mActiveTouchBlock would not get cleared. You're right, of course. I need more coffee :) (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #27) > (In reply to Botond Ballo [:botond] from comment #25) > > > - if (block == CurrentBlock() && block->IsReadyForHandling()) { > > > + if (mQueuedInputs.IsEmpty() && block->IsReadyForHandling()) { > > > > Is this equivalent? What if the block is the current block, but it already > > has some inputs? > > If it already has some inputs, then we don't want to be dispatching aEvent > right away (which is what that function does, by calling > block->DispatchImmediate(aEvent). I see, and DispatchImmediate() asserts !HasEvents(). If previously we would ever have gotten into the body of the if statement with the block already having events, that assertion would have fired anyways. LGTM then!

Comment on attachment 8790523 [details] Bug 1289432 - Have the InputQueue keep RefPtrs to the active block of each input type. https://reviewboard.mozilla.org/r/78296/#review77068

Comment on attachment 8790527 [details] Bug 1289432 - Update checks for unprocessed events to use mQueuedInputs instead of checking the block is at the head. https://reviewboard.mozilla.org/r/78304/#review77070

Comment on attachment 8790528 [details] Bug 1289432 - Allow the CurrentBlock() and CurrentXXXBlock() functions to return null, and guard/assert at the callsites. https://reviewboard.mozilla.org/r/78306/#review77072 ::: gfx/layers/apz/src/AsyncPanZoomController.cpp:1100 (Diff revision 1) > mY.StartTouch(point.y, aEvent.mTime); > if (RefPtr<GeckoContentController> controller = GetGeckoContentController()) { > + MOZ_ASSERT(GetCurrentTouchBlock()); > controller->NotifyAPZStateChange( > GetGuid(), APZStateChange::eStartTouch, > - CurrentTouchBlock()->GetOverscrollHandoffChain()->CanBePanned(this)); > + GetCurrentTouchBlock()->GetOverscrollHandoffChain()->CanBePanned(this));

Comment on attachment 8790529 [details] Bug 1289432 - Drop the DispatchImmediate codepath. https://reviewboard.mozilla.org/r/78308/#review77086

Comment on attachment 8790530 [details] Bug 1289432 - Stop relying on the mEvents array inside the SetConfirmedTargetApzc implementations. https://reviewboard.mozilla.org/r/78310/#review77088 If we're getting a large volume of events per block (e.g. touch-moves or mouse-moves), could we be turning fast linear searches of blocks into slow linear searches of events? ::: gfx/layers/apz/src/InputQueue.h:184 (Diff revision 1) > + * corresponds to the block, or null if no such input was found. Note that > + * even if there are no inputs in mQueuedInputs, this function can return > + * non-null if the block id provided matches one of the depleted-but-still- > + * active blocks (mActiveTouchBlock, mActiveWheelBlock, etc.). > + */ > + CancelableBlockState* FindBlockForId(const uint64_t& aInputBlockId, Elsewhere we pass uint64_t by value. (Except in MainThreadTimeout(), I notice. Feel free to change that as well.) ::: gfx/layers/apz/src/InputQueue.cpp:554 (Diff revision 1) > > +CancelableBlockState* > +InputQueue::FindBlockForId(const uint64_t& aInputBlockId, > + InputData** aOutFirstInput) > +{ > + for (size_t i = 0; i < mQueuedInputs.Length(); i++) { for (const auto& input : mQueuedInputs) { if (input->Block()-> ...) { ... } } (|input| will have type |const UniquePtr<QueuedInput>&|. Or you can spell that out instead of |const auto&|, if you prefer.)

Comment on attachment 8790531 [details] Bug 1289432 - Update a couple more functions to stop fishing around inside mInputBlockQueue. https://reviewboard.mozilla.org/r/78312/#review77090

Comment on attachment 8790532 [details] Bug 1289432 - Migrate remaining InputQueue code to use the new mQueuedInput structure instead of the block-based queue. https://reviewboard.mozilla.org/r/78314/#review77106 ::: gfx/layers/apz/src/InputQueue.cpp (Diff revision 1) > } else if (mActivePanGestureBlock && mActivePanGestureBlock->GetBlockId() == aInputBlockId) { > block = mActivePanGestureBlock.get(); > } > // Since we didn't encounter this block while iterating through mQueuedInputs, > // it must have no events associated with it at the moment. > - MOZ_ASSERT(!block || !block->HasEvents()); Why is this assertion being removed? ::: gfx/layers/apz/src/InputQueue.cpp:697 (Diff revision 1) > + !aBlock->IsReadyForHandling() || > + aBlock->MustStayActive()) { > + return false; > + } > + InputData* firstInput = nullptr; > + FindBlockForId(aBlock->GetBlockId(), &firstInput); (Again, are we potentially doing an excessive amount of linear-searching through events here?)

(In reply to Botond Ballo [:botond] from comment #33) > If we're getting a large volume of events per block (e.g. touch-moves or > mouse-moves), could we be turning fast linear searches of blocks into slow > linear searches of events? > In theory, yes. However if we're getting a lot of events for the same block, then the search should end pretty quickly because most of the events will have the same block id, and the first one that matches will end the search. The scenario that would get slower is if we get a lot of input events for one block that queue up, and then while they're sitting in the queue we get a lot of input events for another block, and we have to search past the events from the first block in order to find the second block. I expect this scenario to be pretty rare, and overall I don't expect the perf impact to be noticeable since the iteration itself should be pretty quick.

(In reply to Botond Ballo [:botond] from comment #35) > > // Since we didn't encounter this block while iterating through mQueuedInputs, > > // it must have no events associated with it at the moment. > > - MOZ_ASSERT(!block || !block->HasEvents()); > > Why is this assertion being removed? Mostly because I removed the HasEvents() function. I could reimplement the HasEvents but it would just iterate through the queue again which we already did above in this function. > ::: gfx/layers/apz/src/InputQueue.cpp:697 > > + FindBlockForId(aBlock->GetBlockId(), &firstInput); > > (Again, are we potentially doing an excessive amount of linear-searching > through events here?) Potentially, yeah. Again I think it probably won't be noticeable, same as comment 36.

Comment on attachment 8790533 [details] Bug 1289432 - Miscellaneous function renaming and documentation touchups. https://reviewboard.mozilla.org/r/78316/#review77112 Thanks for a very nicely broken up and easy to review patch series!

I addressed the remaining review comments (comment 21, comment 33) with an extra patch at the end of the series, since it avoided some painful rebasing, and in some places was modifying pre-existing code (such as the removal of many spurious AsTouchEvent() calls). The one thing I didn't do was get rid of the extra PanGestureInput copy in ReceivePanGestureInput because doing that would require complicated code gymnastics in the function (or breaking the symmetry of the QueuedInput constructors) and I didn't feel it was worth it considering the extra copy is on the stack rather than the heap. The ScrollWheelInput copy was relatively trivial to remove by comparison. If there's a simple way to remove the PanGestureInput copy I can do it, but I didn't see anything obvious. New try push just to double-check everything is sane: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6439938c33f2

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #39) > The one thing I didn't do was get rid of the extra PanGestureInput copy in > ReceivePanGestureInput because doing that would require complicated code > gymnastics in the function (or breaking the symmetry of the QueuedInput > constructors) Not sure if the attached patch constitutes code gymnastics - feel free to take it or leave it. (As you point out, it's just a stack copy so it's not a big deal.)

Whoops, patdiff doesn't seem to work well for generating diff files.

Comment on attachment 8790993 [details] [diff] [review] Avoid an extra copy of a PanGestureInput Review of attachment 8790993 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/apz/src/InputQueue.cpp @@ +336,5 @@ > + // turn this input into a PANGESTURE_START. > + if (event.mType != PanGestureInput::PANGESTURE_START) { > + INPQ_LOG("transmogrifying pan input %d to PANGESTURE_START for new block\n", > + event.mType); > + event.mType = PanGestureInput::PANGESTURE_START; This looks like it will turn *all* PanGestureInput events into PANGESTURE_START events?

Pushed by kgupta@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/3ff934bf3205 Use RefPtr instead of UniquePtr to hold the block state objects, since there will be multiple references to them soon. r=botond https://hg.mozilla.org/integration/mozilla-inbound/rev/e03c153b2fe5 Have the InputQueue keep RefPtrs to the active block of each input type. r=botond https://hg.mozilla.org/integration/mozilla-inbound/rev/73db06b5db9a Introduce a skeleton QueuedInput class. r=botond https://hg.mozilla.org/integration/mozilla-inbound/rev/5544fb9a3c9a Start populating the mQueuedInput array with queued input objects. r=botond https://hg.mozilla.org/integration/mozilla-inbound/rev/1e58cc81dbef Switch over to using the mActiveXXXBlock in some places instead of fishing around in mInputBlockQueue. r=botond https://hg.mozilla.org/integration/mozilla-inbound/rev/525c3eceb106 Update checks for unprocessed events to use mQueuedInputs instead of checking the block is at the head. r=botond https://hg.mozilla.org/integration/mozilla-inbound/rev/4284c891813d Allow the CurrentBlock() and CurrentXXXBlock() functions to return null, and guard/assert at the callsites. r=botond https://hg.mozilla.org/integration/mozilla-inbound/rev/b40d59513ee6 Drop the DispatchImmediate codepath. r=botond https://hg.mozilla.org/integration/mozilla-inbound/rev/868e5e9845ac Stop relying on the mEvents array inside the SetConfirmedTargetApzc implementations. r=botond https://hg.mozilla.org/integration/mozilla-inbound/rev/8ff1c50ceb8b Update a couple more functions to stop fishing around inside mInputBlockQueue. r=botond https://hg.mozilla.org/integration/mozilla-inbound/rev/da3219c8f5b7 Migrate remaining InputQueue code to use the new mQueuedInput structure instead of the block-based queue. r=botond https://hg.mozilla.org/integration/mozilla-inbound/rev/3644e59ed455 Miscellaneous function renaming and documentation touchups. r=botond https://hg.mozilla.org/integration/mozilla-inbound/rev/ca284bdb324a Miscellaneous tweaks to address review comments. rs=botond

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #42) > This looks like it will turn *all* PanGestureInput events into > PANGESTURE_START events? Indeed. That can be fixed, though it's looking more and more like "code gymnastics", so let's just leave it as is.

https://hg.mozilla.org/mozilla-central/rev/3ff934bf3205 https://hg.mozilla.org/mozilla-central/rev/e03c153b2fe5 https://hg.mozilla.org/mozilla-central/rev/73db06b5db9a https://hg.mozilla.org/mozilla-central/rev/5544fb9a3c9a https://hg.mozilla.org/mozilla-central/rev/1e58cc81dbef https://hg.mozilla.org/mozilla-central/rev/525c3eceb106 https://hg.mozilla.org/mozilla-central/rev/4284c891813d https://hg.mozilla.org/mozilla-central/rev/b40d59513ee6 https://hg.mozilla.org/mozilla-central/rev/868e5e9845ac https://hg.mozilla.org/mozilla-central/rev/8ff1c50ceb8b https://hg.mozilla.org/mozilla-central/rev/da3219c8f5b7 https://hg.mozilla.org/mozilla-central/rev/3644e59ed455 https://hg.mozilla.org/mozilla-central/rev/ca284bdb324a