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

Off-the-main thread processing by default #18

Closed
youennf opened this issue May 4, 2020 · 11 comments
Closed

Off-the-main thread processing by default #18

youennf opened this issue May 4, 2020 · 11 comments

Comments

@youennf
Copy link
Collaborator

youennf commented May 4, 2020

Since this is dedicated to RTC, it is important that this processing does not get blocked by other processing. One solution is to define an API and a processing model that would set things up from main thread but runs in a background thread by default.

Similarly to WebAudio, this could be defined in terms of:

  • A pipeline processing some data (frames + metadata) as seems to do the current proposal, but on a background thread
  • A way to set up the pipeline by connecting nodes together, with an input and a destination node
  • A way to create native nodes having a specific functionality
  • An optional way to create JS processing nodes a la AudioWorket
@alvestrand
Copy link
Contributor

I think the choice of where processing happens can and should be left to the application.
The API to move processing to a worker is extremely simple once Transferable Streams are implemented, doesn't involve exposing any part of the WebRTC API to the worker, and creates no requirement to involve the main thread in frame processing; I don't see a reason to complicate the API with another way to designate the chosen processing node.

@youennf
Copy link
Collaborator Author

youennf commented May 7, 2020

I think the choice of where processing happens can and should be left to the application.

Can you detail the benefits of having that processing on the main thread and why this decision should be left to the application?

The API to move processing to a worker is extremely simple once Transferable Streams are implemented

Conversely, it should be easy for web developers to migrate to main thread if they really want to, once transferable streams are implemented.

@murillo128
Copy link

I think current API is easy enough to do the processing on a web worker via TransferableStreams. As an example just check my code for the face detection hack:

https://sgmedooze.cosmosoftware.io/insertable-face/js/worker.js

However, if we could isolate the context of execution of the web worker (like in IdP) something like what Youenn is proposing would make a lot of sense, as we could decouple the asp js from the e2ee js, which could be provided by different parties and would allow us to implement the untrusted js use case.

@alvestrand
Copy link
Contributor

Hm. The concept of untouchable streams makes as much sense as isolated media streams ever did..... a stream that you could only pass to authorized contexts?
That seems like a whatwg-level idea....

@youennf
Copy link
Collaborator Author

youennf commented May 11, 2020

I think the point @murillo128 is making is that key negotiation and encryption/decryption could be done outside of the page for the e2ee case. A worker is same origin as the page and usually executes in the same process as its context. This seems different from untouchable streams.

Getting back to the initial point, all the bricks of the pipeline @alvestrand described in slide 7 of https://docs.google.com/presentation/d/1t5dsr8saSVwYBWRm-nzZsDiKanqbodr1GCDH0jGTY5g/ are currently implemented in background threads AFAIK.

If we anticipate most applications to do that processing in the main thread, the current design might be fine with that regards.

If we anticipate all applications to instantiate a worker, post message to the worker to transfer the streams, we should bake that pattern in the API. A good API is usually trying to make the right approach implementable as easily as possible and the wrong approach harder.

@alvestrand
Copy link
Contributor

I'm reminded of the discussions of why UNIX doesn't have a move() syscall.
All the components (link, unlink, delete) are there, but there's no move.
Because it turned out that each of those pieces had their own properties, and combining them into a single call made the system less well structured.

In this case: the peerconnection doesn't need to know about workers, the streams don't need to know about peerconnection, and the messaging doesn't need to know about streams.
Some of these elements touch upon each other, so the implementation can optimize certain combinations of operations, but model-wise, the concerns are separted.

As for key negotiation .... as soon as we have a key negotiation API, I think we should recommend its usage. But we don't. And I don't think we should say that "none of these problems can be solved until they are all solved".

@guidou
Copy link
Collaborator

guidou commented Aug 12, 2020

Closing since, while off-the-main thread processing by default would be nice, the current model with Transferable Streams and Workers sufficiently addresses this issue.
Other models about isolated/untouchable streams, etc., are probably better addressed in the Streams API spec.

@guidou guidou closed this as completed Aug 12, 2020
@youennf
Copy link
Collaborator Author

youennf commented Aug 13, 2020

I do not think we reached consensus on this and hope we can discuss insertable streams design at virtual TPAC.
In the meantime, let's reopen the issue.

@youennf youennf reopened this Aug 13, 2020
@guidou
Copy link
Collaborator

guidou commented Aug 13, 2020

My point was not that we reached consensus, but that the default threading for streams looks more like a matter to be discussed in the context of the Streams API than this one. But let's keep it open.

@alvestrand
Copy link
Contributor

Suggest adding the new API in one PR and removing the old API in another, so that we can keep the discussions separate.

@alvestrand
Copy link
Contributor

The currently defined API does off-the-main-thread processing by default, so closing this issue (with the reason being opposite the reason from August).
Revisiting the convolution of transfer with creation should be the subject of another bug.

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

4 participants