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

Add support for transferable MediaStreamTracks #21

Merged
merged 8 commits into from
Jun 3, 2021

Conversation

youennf
Copy link
Contributor

@youennf youennf commented May 3, 2021

@youennf
Copy link
Contributor Author

youennf commented May 4, 2021

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

@dontcallmedom
Copy link
Member

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

@youennf
Copy link
Contributor Author

youennf commented May 4, 2021

Thanks!

@youennf youennf requested a review from jan-ivar May 4, 2021 08:00
@dontcallmedom
Copy link
Member

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
Copy link
Contributor Author

youennf commented May 4, 2021

Adding a small commit fixes the issue

Copy link
Member

@jan-ivar jan-ivar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall I'm positive about doing this, though I have some questions about design.

Unlike the streams spec, the desirable semantics here seem closer to track.clone() than to a data tunnel, except when it comes to lifetime which seems tied to permission granted the original document.

index.html Outdated
Comment on lines 355 to 357
<p>To preserve the existing privacy and security infrastructure, in particular for capture tracks,
a transferred track remains tightly bound to the original track.
In particular, if the original track gets muted, unmuted or ended, the transferred track will automatically be muted, unmuted or ended.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find this sentence confusing, as no "original track" survives a transfer in any functional sense to have experiences like "gets muted" (only a "detached" interface remains).

I also think the (un)muted and enabled properties are implicit in being a MediaStreamTrack (we say less in clone).

But I think we need to expressly limit the track being live to when the realm it was originally granted to exists. And this mustn't be transitive (if the transferred track is transferred yet again). How about:

Suggested change
<p>To preserve the existing privacy and security infrastructure, in particular for capture tracks,
a transferred track remains tightly bound to the original track.
In particular, if the original track gets muted, unmuted or ended, the transferred track will automatically be muted, unmuted or ended.
<p>To preserve existing privacy, security, and permission infrastructure, in particular for tracks from
capture sources, transferred tracks MUST [= track/ended | end =], if they haven't already, when they
would have ended had they never been transferred.

This also exposes that mediacapture-main doesn't expressly say anywhere that tracks must end on navigation. We should probably specify that with some unloading document cleanup steps. — Luckily browsers already do this.

// Remove partial once we figure out how to do it without creating respec warnings.
// No change to MediaStreamTrack exposed API.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can we remove partial? This isn't the full interface.

This might be one of those cases where we need to say "wait for the document to be merged with mediacapture-main"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, that is the best tradeoff I found to still add useful IDL information in that PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why partial should be removed, given that this is only part of the interface, and that the WebIDL convention is that all IDL that a browser implements should be considered merged in the implementation.
Having an extended visibility list in a partial is unusual for sure, but if the IDL parser doesn't complain, I'm not going to.

index.html Outdated
Comment on lines 387 to 388
<li><p>Let <var>port1</var> be a new {{MessagePort}} in <a href="https://tc39.es/ecma262/#current-realm">the current Realm</a>.</p></li>
<li><p>Let <var>port2</var> be a new {{MessagePort}} in <a href="https://tc39.es/ecma262/#current-realm">the current Realm</a>.</p></li>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced we need these ports. They seem to be used solely to convey signals from the user agent — track.stop() on the detached track won't work — which I think may be better described as requirements on the user agent, if they aren't already, without going through the original track. This also avoids complexity if a transferred track is transferred around further.

E.g. I'd expect user agents to mute/unmute or end the source, not individual tracks. To the extent this is not the case, I wouldn't begin to assume how this might work in a particular user agent, nor would I try to prescribe it.

We should probably audit our permission indicator logic after this though.

@jan-ivar
Copy link
Member

jan-ivar commented May 6, 2021

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
Copy link
Contributor Author

youennf commented May 7, 2021

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
Copy link
Contributor Author

youennf commented May 7, 2021

Fixes #16

@alvestrand
Copy link
Contributor

I passed on a suggestion in #16 (comment) that might simplify this PR a bit. Suggest evaluating that before progressing with this CL.

@youennf
Copy link
Contributor Author

youennf commented May 19, 2021

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

@alvestrand
Copy link
Contributor

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
Copy link
Contributor Author

youennf commented May 20, 2021

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:

  • getUserMedia is called on iframes A and B to produce TA1 and TB1.
  • TA1 and TB1 are transferred/copied as TA2 and TB2 in a third iframe C.
  • If iframe A goes away, I think the intent is to end TA2 but TB2 can remain live until B (or C) goes away.
    In this example, I think spec allows UAs to either have a single source for TA1 and TB1 or two different sources.
    The same applies for muting: in Safari, muting happens page-wide, the same source could be used for two pages.

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

@alvestrand
Copy link
Contributor

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
Copy link
Contributor Author

youennf commented May 25, 2021

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

@youennf
Copy link
Contributor Author

youennf commented May 25, 2021

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.

Copy link
Contributor

@aboba aboba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

youennf and others added 3 commits June 3, 2021 11:16
Co-authored-by: Jan-Ivar Bruaroey <jan-ivar@users.noreply.github.com>
Co-authored-by: Jan-Ivar Bruaroey <jan-ivar@users.noreply.github.com>
@youennf
Copy link
Contributor Author

youennf commented Jun 3, 2021

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 youennf force-pushed the transferable-mediastreamtrack branch from 579e9c4 to 0846b9c Compare June 3, 2021 09:24
@youennf
Copy link
Contributor Author

youennf commented Jun 3, 2021

Rebasing on top of main to have correct sections

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants