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

Shouldn't remaining async methods run "in parallel"? #131

Open
jan-ivar opened this issue Aug 14, 2017 · 3 comments
Open

Shouldn't remaining async methods run "in parallel"? #131

jan-ivar opened this issue Aug 14, 2017 · 3 comments

Comments

@jan-ivar
Copy link
Member

A long-overdue follow-up to #54, apart from start(), which I changed in #61, the other async methods still run solely on the main thread. Take requestData for example:

"When ... invoked, the UA MUST run the following steps:

  1. If state is inactive throw an InvalidStateError DOMException and terminate these steps. Otherwise the UA MUST queue a task, using the DOM manipulation task source, that runs the following steps:"

While this works, it begs the question of why it's asynchronous in the first place. Anything we do in a task queued on main-thread, we could arguably have done synchronously already.

Instead, the intent was likely to let implementations communicate with its off-main-thread recording thread without blocking main-thread. The current text allows very limited time to do this, if at all.

I suggest we rewrite the steps to run in parallel, reference what needs to be done in parallel, e.g. data gathering, and queue the main-thread task from there, which references the data and puts it into a Blob (Blob technically is a DOM object, and shouldn't be accessed off-main-thread, but in this spec it appears used as a shorthand for the target buffer of off-main-thread data gathering. We might want to clean that up as well).

For some of the methods, like resume() and pause(), there appears to be very little to do off-main-thread, but unless we want them synchronous, they should probably receive the same treatment?

I have a PR for a similar problem in imageCapture in w3c/mediacapture-image#184.

@jan-ivar
Copy link
Member Author

Existing implementations appear to manage with the steps as written. Changing this to "in parallel" would lose some ordering guarantees, so maybe we should close this?

@Pehrsons
Copy link
Collaborator

It turns out implementations are not starting and stopping the gathering of data in the queued tasks per the spec's current language, but doing it immediately. At least for pause() and resume() which the issue mentions. Only the firing of the event is done in the queued task. This is true in Firefox 70 (and earlier) and Chrome 77 (assuming it hasn't change since then until chromium's master branch as of today).

These are in the end done on a background thread, so depending on the definition of "{start|stop} gathering data" current implementations are either doing it synchronously in the algorithm, or "in parallel".

@jan-ivar
Copy link
Member Author

@Pehrsons Aligning with implementations on pause() and resume() seems like a good idea.

In Chrome, pause/resume appears to work reliably from requestAnimationFrame(f), a useful property that would be broken if it followed the spec (queued a task).

But for methods like requestData, you made a good point offline that waiting for the recording to stop "in parallel" before queuing the task might make more sense, since there might still be data in the queue to encode, and the data might have to be written to disk when writing to the blob.

@jan-ivar jan-ivar added this to the Future Version milestone Feb 6, 2024
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

2 participants