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

Inconsistent setting of receiver.track.readyState violates Mediacapture #1575

Closed
jan-ivar opened this issue Sep 7, 2017 · 7 comments · Fixed by #2296
Closed

Inconsistent setting of receiver.track.readyState violates Mediacapture #1575

jan-ivar opened this issue Sep 7, 2017 · 7 comments · Fixed by #2296
Assignees

Comments

@jan-ivar
Copy link
Member

jan-ivar commented Sep 7, 2017

Three things wrong: First, the sole mention of track readyState in WebRTC is in pc.close():

"For every RTCRtpTransceiver transceiver in transceivers, run the following steps:
1. If transceiver's [[Stopped]] slot is true, abort these steps.
2. ...
3. ...
4. ...
5. ...
6. ...
7. Set receiver.track.readyState to "ended"."

This means that while the following makes sense:

pc.close();
console.log(pc.getTransceivers()[0].receiver.track.readyState); // ended

...the following makes no sense:

pc.stop();
console.log(pc.getTransceivers()[0].receiver.track.readyState); // live
pc.close();
console.log(pc.getTransceivers()[0].receiver.track.readyState); // live

The second wrong thing is readyState is a read-only property, thus can't be set, but that's nit.

The third wrong thing is not firing the ended event here seems to violate the mediacapture spec:

"When a MediaStreamTrack track ends for any reason other than the stop() method being invoked, the User Agent MUST queue a task that runs the following steps:
1. ...
2. ...
3. ...
4. Fire a simple event named ended at the object."

The stop() method referred to there is track.stop(), not any other stop().

Should we fire ended events on tracks from peer connection on transceiver.stop() and/or pc.close(), or fix mediacapture?

@jan-ivar
Copy link
Member Author

jan-ivar commented Sep 7, 2017

Also, should we fire ended events on tracks from peer connection when a transceiver is indirectly stopped through SLD or SRD, e.g from a "rollback"?

@taylor-b
Copy link
Contributor

taylor-b commented Sep 7, 2017

The steps in RTCRtpTransceiver.stop actually seem to say the right thing:

receiver.track is now said to be ended.

Which links to the steps for changing readyState in the mediacapture spec, which you quoted. Below it also explicitly says:

the steps described in track ended must be followed.

So, I think all these issues would be fixed if the RTCPeerConnection.close algorithm referenced the RTCRtpTransceiver.stop algorithm instead of duplicating the steps. Which I believe has been suggested in another issue.

@jan-ivar
Copy link
Member Author

jan-ivar commented Sep 7, 2017

@taylor-b Thanks for finding that!

We could also improve the language here to avoid passive language normative references IMHO.

the steps described in track ended must be followed.

It seems unusual for a method to normatively call out its callers. I'd say this whole paragraph belongs under set an RTCSessionDescription, not under stop().

@taylor-b
Copy link
Contributor

taylor-b commented Sep 7, 2017

I'd say this whole paragraph belongs under set an RTCSessionDescription, not under stop().

I assume you mean the paragraph that says:

When a remote description is applied with a zero port in the media description for the corresponding transceiver, as defined in [JSEP] (section 4.2.2), the user agent must run the above steps as if stop had been called.

I agree. Both the RTCPeerConnection.close and "set an RTCSessionDescription" sections should just reference the RTCRtpTransceiver.stop steps directly, where applicable.

@aboba aboba assigned jan-ivar and unassigned adam-be Aug 6, 2018
@aboba aboba added the TPAC 2018 label Oct 6, 2018
@jan-ivar
Copy link
Member Author

I'll work on a long-overdue PR here. I think firing ended is an exception to #565 because the target is different, and we don't want to violate mediacapture/downstream expectations.

Sure, a workaround would be to always remember to do

pc.getTransceivers().forEach(tc => tc.stop());
pc.close();

...but that seems like an API bug. pc.close already mimics stop() wrt its effect on the other side.

@dontcallmedom
Copy link
Member

I believe this was fixed in a combination of c8247aa and 3fc6d18 - @jan-ivar can you confirm?

@jan-ivar
Copy link
Member Author

The remaining issue in #1575 (comment) required a PR, so I added one.

With this merged, we'll have the following spec behavior (with a running loop as starting point):

Method Event fired on pc2.getTransceivers()[0].receiver.track
pc1.close() mute¹
pc1.getTransceivers()[0].stop() mute¹, followed by ended on renegotiation
pc2.close() ended (this PR #2296)
pc2.getTransceivers()[0].stop() ended
pc1.getTransceivers()[0].sender.track .stop() ²
pc2.getTransceivers()[0].receiver.track .stop()

1. Chrome and Firefox have bugs where they don't fire mute on receiving RTCP BYE, which the spec says they must.
2. Chrome has a bug here where it fires mute when it shouldn't.

WebRTC 1.0 to PR automation moved this from To do to Done Sep 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging a pull request may close this issue.

5 participants