Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reason for "Events MUST NOT be fired more than 30 times per second." #2

Open
happylinks opened this issue Mar 2, 2023 · 8 comments

Comments

@happylinks
Copy link

I was curious what the reasoning behind this restriction is. Is it just to set a maximum in the spec, or is this a limitation to browsers/performance?
I could imagine some tools wanting to have the mouse coordinates at 60fps.

@eladalon1983
Copy link
Contributor

Agreed. :-)

I've used 30fps as an arbitrary initial limit to keep things "sane" and performant. Imagine the extreme case of dragging the cursor across a 4k screen in the span of a second; you'd not want an individual event each time the cursor moves by a single pixel, causing the capturing app to slow down to a crawl.

But different applications will have different needs here. What do you think would be a good way to let an application tweak the behavior?

@happylinks
Copy link
Author

Could maybe pass a configuration to the CaptureController? "mouseMoveInterval: 30" (bad name/bad value)

@eladalon1983
Copy link
Contributor

That works for me. I wonder if we want to iterate over that later, though. IMHO, the MVP is to expose the coordinates with some reasonable regularity.

@happylinks
Copy link
Author

Alright, then 30fps works for me!

@eladalon1983
Copy link
Contributor

eladalon1983 commented Mar 2, 2023

Sorry if I phrased things misleadingly. I think it's perfectly fine to kickstart the discussion right away. I was just proposing to view it as non-blocking for the initial specification and implementation. So if you're still interested, please re-open the issue, I'm happy to continue.

@happylinks happylinks reopened this Mar 2, 2023
@fred-wang
Copy link
Contributor

IIUC, this issue is about two things:

  1. Explain the rationale for this 30 times per second.
  2. Enhancement request to make the limit configurable in future iterations of the spec.

I guess (1) can be addressed immediately by adding some text in the spec, perhaps in a non-normative <div class="note">. Do we want that?

For (2) the discussions we had in other issues, @eladalon1983's proposal was to use labels on GitHub issues for future features and then to close them immediately (personally I prefer keep them open with labels).

In any case for (2) I'd suggest either to open a separate issue or to change the title, something like "Make frequency of CapturedMouseEvents configurable" so it's easier to understand what the issue is about when reviewing the list of open issues.

@eladalon1983
Copy link
Contributor

For (2) the discussions we had in other issues, @eladalon1983's proposal was to use labels on GitHub issues for future features

It makes sense to me to keep this issue open, because it deals with either (a) something already in the spec or (b) something we're proposing to add to the spec right now. Does that make sense?

"Make frequency of CapturedMouseEvents configurable" so it's easier to understand what the issue is about when reviewing the list of open issues.

That works for me.

@fred-wang
Copy link
Contributor

It makes sense to me to keep this issue open, because it deals with either (a) something already in the spec or (b) something we're proposing to add to the spec right now. Does that make sense?

OK sure, but from your comment "non-blocking for the initial specification and implementation" I understood it was for a later iteration.

That works for me.

@happylinks Are you also ok to rename this issue ""Make frequency of CapturedMouseEvents configurable" ?

aarongable pushed a commit to chromium/chromium that referenced this issue Nov 17, 2023
I. First prototype for Captured Mouse Events

This is part of a series of patches the first prototype for the
Captured Mouse Events specification [1], focusing on mouse coordinates.
In CLs #4542243 and #4517372 the basic IDL support was put in place:
CaptureController is an EventTarget and a new interface
CapturedMouseEvent encapsulates mouse coordinates over the captured
surface.

The proposal requires to pass coordinates (-1, -1) when the mouse is not
over the captured surface which can be determined using
platform-specific checks. The specification also provides hints to limit
the frequency with which events are fired, by briefly buffering mouse
moves if an event was recently sent, and sending the last coordinates
received after some delay.

II. Limitations and future work

This first prototype is implemented as follows:

* Transmit events as long as a CaptureController is provided. This
  must be improved before turning the runtime flag on, as discussed
  in [2].
* Only dispatch mouse events for captured Chrome tab (not windows or
  screen).
* Ensure two events are separated by a minimal interval of
  1000/(30 - 1) ms, corresponding to at most 30 events per seconds.
  This arbitrary value could be configurable in the future  [4].
* Pass mouse events as gfx::Point, could be extended in the future to
  include data that can't already be inferred from the captured frames
  (e.g mouse clicks).
* Do not automatically transmit mouse coordinates via RTCPeerConnection,
  users would do it themselves via RTCDataChannel if they wish [3].

Testing this new feature involves asking permission for screen capturing
a tab as well as performing moves of the mouse, which is difficult to
perform with current WPT infrastructure. Except for a simple manual WPT
test [5], our test coverage relies on internal unit tests.

III. Changes done in this specific CL

This CL is the first patch of the series of patches. It relies on the
existing class MouseCursorOverlayController, which watches mouse events
when screen-capturing web content so that we can draw the mouse cursor
on top of video frames. This class is extended so that we can transmit
mouse events to ClientFrameSinkVideoCapturer, with the convention and
frequency described above, following this backtrace:

viz::ClientFrameSinkVideoCapturer::Overlay::OnCapturedMouseEvent()
content::MouseCursorOverlayController::SendMouseEvent()

New content_browsertests of names
MouseCursorOverlayControllerBrowserTests.* verify this change, but
some cases are only covered by tests added by remaining patches of the
series.

[1] https://groups.google.com/a/chromium.org/g/blink-dev/c/DYb5fXICJvo
[2] screen-share/captured-mouse-events#14
[3] screen-share/captured-mouse-events#13
[4] screen-share/captured-mouse-events#2
[5] https://wpt.live/captured-mouse-events/capturing-mouse-coordinates-manual.tentative.https.html

Bug: 1444712
Change-Id: I94799dde6b324728049b67787ce64c889cdb31ff
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4665589
Reviewed-by: Tom Sepez <[email protected]>
Reviewed-by: Ahmed Fakhry <[email protected]>
Reviewed-by: Elad Alon <[email protected]>
Reviewed-by: Nick Harper <[email protected]>
Commit-Queue: Frédéric Wang <[email protected]>
Reviewed-by: Jordan Bayles <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1225833}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants