Enable coalescing mouse events to be once per refresh cycle (enable the pref)

RESOLVED FIXED in Firefox 57

Status

()

P3
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: stone, Assigned: stone)

Tracking

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [qf:p3])

Attachments

(1 attachment)

Comment hidden (empty)
(Assignee)

Updated

2 years ago
Depends on: 1361067

Updated

2 years ago
Whiteboard: [qf]

Comment 1

2 years ago
Any idea what this is blocked on Stone?
(Assignee)

Comment 2

2 years ago
(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog, Away 8/23) from comment #1)
> Any idea what this is blocked on Stone?

Not sure if we should implement PointerEvent.getCoalescedEvents() before shipping this? Now we only enable PointerEvent on nightly. Also not sure if any other blockers.

Comment 3

2 years ago
OK, makes sense.  Shipping pointer events is probably a bigger blocker at this point then!
Depends on: 960316
(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #3)
> OK, makes sense.  Shipping pointer events is probably a bigger blocker at
> this point then!

Ehsan, are you recommending that we wait until after we ship PointerEvents before enabling dom.event.coalesce_mouse_move by default?
Flags: needinfo?(ehsan)

Comment 5

2 years ago
I defer to smaug.
Flags: needinfo?(ehsan) → needinfo?(bugs)
I wonder if there is something else we could do before we have pointer events. Like enable unless the page (or iframes) has <canvas>.

Perhaps Jukka has an opinion from games point of view.
Flags: needinfo?(jujjyl)

Comment 7

2 years ago
Oh my god, thanks for bringing this to my attention.

Agreed that some applications only care about the latest data that aligns with a rAF, so doing this sounds fine for many applications. The problem is that not all applications align with a rAF, especially now that Emscripten WebAssembly applications are in the process of migrating to run in a Web Worker. Such applications can run their own main loop, and use OffscreenCanvas to render (which has a .commit() API to push a canvas present). These application continue to use web events from the main browser thread to receive input, which are then routed to the WebAssembly heap, where a queue exists for these applications to read from.

An application that runs in a web worker, but does not run a synchronously blocking loop given by OffscreenCanvas, but wanted to keep using rAFs, will need bug 1203382 to land. I.e. currently an app running in a Web Worker cannot align accurately with rAF() (except by manually pinging postMessage()s over from main thread to the worker)

This kind of change would be a fine thing to do, as long as the API to opt out from coalescing is an appropriate one. Reading at https://w3c.github.io/pointerevents/extension.html#extensions-to-the-pointerevent-interface, is looks like the current API to opt out from coalescing seems to be that once a coalesced event finally arrives, one can then look at that event to ask from all the events that fell "in the middle" between the previously sent event and the current one.

This API means that applications that do want to opt out from coalescing still have to suffer from the added latency that coalescing introduced. That is, one does not get the uncoalesced input events until when the final coalesced event arrives.

In addition to the above "app renders in a web worker" case, there are VR applications that might have support for a mouse. Presenting VR content is not based on the usual rAFs (but a special version of it: https://developer.mozilla.org/en-US/docs/Web/API/VRDisplay/requestAnimationFrame), and for VR minimizing latency is a priority, so it would suffer from the added latency when the API to opt out from coalescing still takes a latency hit.

The feature to coalesce itself sounds fine, although I'd hope that the proposed method to opt out would not cause an added latency hit, and that the API to opt out from coalescing would be separate from the code that does the actual input handling. This way the decision of opting vs the code that does the actual mouse input could be treated orthogonally to each other - now the opting is tied to the specific locations in code that does the input handling.

Would it be possible to have an API where getting high refresh rate/low latency mouse and touch input would be some kind of option on .addEventListener(), or by calling a function somewhere to opt to it? Perhaps a foo.addEventListener('rapidmousemove') or similar would work better as a mechanism for applications that know they need the fast rate? That would be breaking, but equally breaking as a .getCoalescedEvents() API. The advantage of a foo.addEventListener('rapidmousemove') would be that the event object granularity would stay the same - in a .getCoalescedEvents() there is one Event object that is raised for N actual move events, and applications may need to refactor one Event callback to actually internally fire N event callbacks, one for each coalesced event. With a foo.addEventListener('rapidmousemove') vs foo.addEventListener('mousemove') apps would have an easy time testing out if it matters for them, and the code that actually processes the input events would not have to change?
Flags: needinfo?(jujjyl)

Comment 8

2 years ago
All the above being said, OffscreenCanvas is not yet a spec that ships, Wasm multithreading is not yet here, and VR's main use case is not with mouse, so a big green stamp to go forward from that perspective - perhaps "rapidmousemove" and "rapidpointermove"  events might be good ideas to improve support for high refresh rate/low latency input devices for those use cases in the future(?)
Some related discussion is happening in https://github.com/w3c/pointerevents/issues/214
Flags: needinfo?(bugs)
Smaug, could you give this a [qf] rating?  Bas, emma, and I aren't clear on what the correct rating is here, given the possible-spec-work-dependence.

(I'm also tweaking the summary to make it clearer how this differs from bug 1361067.)
Flags: needinfo?(bugs)
Summary: Enable coalescing mouse events to be once per refresh cycle → Enable coalescing mouse events to be once per refresh cycle (enable the pref)
Let's say qf:p3. It isn't clear whether we can enable this for FF57.
Flags: needinfo?(bugs)
Whiteboard: [qf] → [qf:p3]
Priority: -- → P3
Based on some comments from rbyers that blink folks haven't seen regression bugs filed and what Jukka said on IRC, perhaps we could try to enable this on Nightly and see if this helps with any measurements. If not, which might be very possible, it might be still safest to switch this off for FF57.
Stone, does that sound reasonable to you?
Flags: needinfo?(sshih)
(Assignee)

Comment 13

2 years ago
(In reply to Olli Pettay [:smaug] from comment #12)
> Based on some comments from rbyers that blink folks haven't seen regression
> bugs filed and what Jukka said on IRC, perhaps we could try to enable this
> on Nightly and see if this helps with any measurements. If not, which might
> be very possible, it might be still safest to switch this off for FF57.
> Stone, does that sound reasonable to you?

Agree with you. I'll follow up.
Flags: needinfo?(sshih)
(Assignee)

Updated

2 years ago
Assignee: nobody → sshih
(Assignee)

Comment 15

2 years ago
(Assignee)

Updated

2 years ago
Attachment #8903415 - Flags: review?(bugs)
Attachment #8903415 - Flags: review?(bugs) → review+
If this works well, need to remember to remove that ifdef!
(Assignee)

Comment 17

2 years ago
(In reply to Olli Pettay [:smaug] from comment #16)
> If this works well, need to remember to remove that ifdef!

I'll keep an eye on this and monitor if any improvements or problems.

Comment 18

2 years ago
Pushed by sshih@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/79c73def3bdf
Enable coalescing mouse events to be once per refresh cycle (enable the pref). r=smaug.

Comment 19

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/79c73def3bdf
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Stone, can we remove the nightly ifdef?  Or is there a follow-up filed to do that I can follow?
Flags: needinfo?(sshih)
(Assignee)

Updated

2 years ago
Blocks: 1403743
(Assignee)

Comment 21

2 years ago
(In reply to Ben Kelly [:bkelly] from comment #20)
> Stone, can we remove the nightly ifdef?  Or is there a follow-up filed to do
> that I can follow?

I think we can. Create bug 1403743 to remove it and ride the next train.
Flags: needinfo?(sshih)
(Assignee)

Updated

2 years ago
Blocks: 1401450
(Assignee)

Updated

a year ago
No longer blocks: 1403743
You need to log in before you can comment on or make changes to this bug.