search

w3c / mediacapture-extensions

Extensions to Media Capture and Streams by the WebRTC Working Group
https://w3c.github.io/mediacapture-extensions/
Other
19 stars 15 forks source link

Add support for transferable MediaStreamTracks #21

Closed youennf closed 3 years ago

youennf commented 3 years ago

Preview | Diff

youennf commented 3 years ago

@dontcallmedom, do you know why there is an issue with IPR?

dontcallmedom commented 3 years ago

it's a bug in the IPR manager AFAICT, which I have reported and hope to see resolved soon

youennf commented 3 years ago

Thanks!

dontcallmedom commented 3 years ago

the IPR manager bug has been fixed, although for some reason I can't seem to reset the IPR flag - anyway, hopefully this is already clear it should not block merging this PR

youennf commented 3 years ago

Adding a small commit fixes the issue

jan-ivar commented 3 years ago

Note I was mostly thinking about camera, microphone, and screen-capture in this review. I suspect those are the short-term targets here, correct? If so, should we perhaps scope this down?

E.g. for things like canvas/element capture (less sure about RTCPeerConnection?), tunnel semantics may make more sense.

youennf commented 3 years ago

Thanks for the review!

E.g. I'd expect user agents to mute/unmute or end the source, not individual tracks.

I agree but the media capture spec is not talking about that, it is merely talking about muting/unmuting a track. Spec says things about stopping a source but it only happens when all tracks tied to a source get stopped, so again, the spec is currently scoped in terms of UA controls MediaStreamTrack, which might affect sources. This PR is following that principle.

Maybe we should improve media capture main spec, mute/unmute/stop sources, more clearly define sources and how they relate to sinks... This can probably be done in parallel though.

If so, should we perhaps scope this down?

Maybe. I would first try to have something working generally and scope it down if we cannot agree on the general procedure. Like for ReadableStream, I would hope we can have a consistent MediaStreamTrack behavior whatever the actual source. To me, all MediaStreamTracks should have a source that is tied to a given realm: if realm goes away, MediaStreamTrack gets ended.

With the current PR, if transferring from R1 to R2 then to R3, the final MediaStreamTrack living in R3 will not get ended when R2 goes away. It will only happen when R1 goes away. That is the reason behind creating MessagePorts in the original track and transferring the receiving port for transferred tracks.

youennf commented 3 years ago

Fixes https://github.com/w3c/mediacapture-extensions/issues/16

alvestrand commented 3 years ago

I passed on a suggestion in https://github.com/w3c/mediacapture-extensions/issues/16#issuecomment-843950789 that might simplify this PR a bit. Suggest evaluating that before progressing with this CL.

youennf commented 3 years ago

@alvestrand, can you clarify the potential simplifications you have in mind?

alvestrand commented 3 years ago

A version of this PR that uses serializable is uploaded as #24 - the main simplification is to drop the whole internal port stuff by referring to the track source as the object that has the power to end the track.

youennf commented 3 years ago

The port stuff is somehow orthogonal to serialisable vs. transferable. @jan-ivar suggested to drop it as well in this PR, which is feasible.

To do so, we would need to more precisely describe the notion of a source, its lifetime, its muting in mediacapture-main. I agree that it makes sense to mute a source and not a track, but AFAIK, this is not how the spec defines things currently.

Let's take the following example:

The port stuff tries to handle these cases as an algorithm based on what is observable and interoperable in UAs, i.e. tracks.

alvestrand commented 3 years ago

To Youenn's example: The question here is whether TA1 and TB1 shoud be considered to have different sources or the same source. If TA1 and TA2 have, for instance, two different permissions that they depend on, and the permission for TA1 is revoked, it seems obvious that TB1 should end. So a source may be considered to be origin-specific (since origins is what we grant access for)..

If TA1 and TB1 have the same origin, the argument for considering them to be separate sources is strictly utilitarian; it binds them to their original context, so we can close TA2 when iframe A closes; if they instead are considered the same source, TB1 and TB2 have no real reason to stop (but then the argument for stopping them at all as long as the worker lives becoms weaker).

Suggest that the principle of least surprise says that we define a (device) source to be specific to the document/context it is created in.

youennf commented 3 years ago

As per WG discussion, I split the lifetime management from this PR and only kept the transferable pieces.

youennf commented 3 years ago

It might be a good approach to define a source and a sink model (for this PR but also for raw access) but this is a hard task and we might encounter issues with how implementations actually work.

That is why the lifetime management I removed from this PR was based on tracks which is more solid from an interop point of view. Implementations might not need to actually use tracks to implement the same behavior.

Suggest that the principle of least surprise says that we define a (device) source to be specific to the document/context it is created in.

Right, this seems to go well with the lifetime management pieces I removed from this PR. Let's discuss this as a separate issue though once we settle on the reduced PR.

youennf commented 3 years ago

Can we make it more clear where data holder is defined (e.g. HTML spec)?

The transfer steps refer to the HTML spec (https://html.spec.whatwg.org/multipage/structured-data.html#transfer-steps), shortly before dataHolder is defined. Not sure how much I can improve things.

youennf commented 3 years ago

Rebasing on top of main to have correct sections