w3c / mediacapture-record

MediaStream Recording
https://w3c.github.io/mediacapture-record/
Other
104 stars 22 forks source link

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

Open jan-ivar opened 7 years ago

jan-ivar commented 7 years ago

A long-overdue follow-up to https://github.com/w3c/mediacapture-record/issues/54, apart from start(), which I changed in https://github.com/w3c/mediacapture-record/pull/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 https://github.com/w3c/mediacapture-image/issues/184.

jan-ivar commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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