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

RTCRtpSender.setTransport may raise a IncompatibleMediaStreamTrackError due to identity check #467

Closed
murillo128 opened this Issue Apr 13, 2016 · 8 comments

Comments

Projects
None yet
4 participants
@murillo128

murillo128 commented Apr 13, 2016

The RTCRtpSender needs to ensure that the transport identity matches the media track requested identity.

This is only done currently when changing the track:

 "If withTrack.kind differs from RTCRtpSender.track.kind or if withTrack has different peerIdentity constraints, then reject p with IncompatibleMediaStreamTrackError and abort these steps. "

but it is not checked when the transport is changed.

When the transport is changed, the transport identity has to be checked against the media track constraints and if not matching, either raise an IncompatibleMediaStreamTrackError or return a promise and reject it.

@robin-raymond robin-raymond added the 1.1 label Apr 13, 2016

@robin-raymond

This comment has been minimized.

Show comment
Hide comment
@robin-raymond

robin-raymond Apr 14, 2016

Contributor

@murillo128 which transport is changed?

Contributor

robin-raymond commented Apr 14, 2016

@murillo128 which transport is changed?

@murillo128

This comment has been minimized.

Show comment
Hide comment
@murillo128

murillo128 Apr 14, 2016

For example, auser has two transports (tA and tB) with two different peers(A and B), each one with a different identity, and does a gUM with A identity constraint.

Then he creates a sender with the track and transport tA. If then he does sender.setTransport(tB), transport tB doesn't match the identity constraint off the track

murillo128 commented Apr 14, 2016

For example, auser has two transports (tA and tB) with two different peers(A and B), each one with a different identity, and does a gUM with A identity constraint.

Then he creates a sender with the track and transport tA. If then he does sender.setTransport(tB), transport tB doesn't match the identity constraint off the track

@robin-raymond

This comment has been minimized.

Show comment
Hide comment
@robin-raymond

robin-raymond Apr 14, 2016

Contributor

@murillo128 Ah, I see, yes, setTransport() vs setTrack(), one mapping has exception, the other does not.

Yes, this issue is valid and needs a fix.

I'm also thinking this is another reason these might need to need Promises result because I'm not convinced this kind of check can always be done synchronously. It might not even be possible as an exception.

Contributor

robin-raymond commented Apr 14, 2016

@murillo128 Ah, I see, yes, setTransport() vs setTrack(), one mapping has exception, the other does not.

Yes, this issue is valid and needs a fix.

I'm also thinking this is another reason these might need to need Promises result because I'm not convinced this kind of check can always be done synchronously. It might not even be possible as an exception.

@murillo128

This comment has been minimized.

Show comment
Hide comment
@murillo128

murillo128 Apr 14, 2016

I also think all sender methods should return a promise.
El 14/4/2016 22:23, "Robin Raymond" notifications@github.com escribió:

@murillo128 https://github.com/murillo128 Ah, I see, yes, setTransport()
vs setTrack(), one mapping has exception, the other does not.

Yes, this issue is valid and needs a fix.

I'm also thinking this is another reason these might need to need Promises
result because I'm not convinced this kind of check can always be done
synchronously. It might not even be possible as an exception.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#467 (comment)

murillo128 commented Apr 14, 2016

I also think all sender methods should return a promise.
El 14/4/2016 22:23, "Robin Raymond" notifications@github.com escribió:

@murillo128 https://github.com/murillo128 Ah, I see, yes, setTransport()
vs setTrack(), one mapping has exception, the other does not.

Yes, this issue is valid and needs a fix.

I'm also thinking this is another reason these might need to need Promises
result because I'm not convinced this kind of check can always be done
synchronously. It might not even be possible as an exception.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#467 (comment)

@robin-raymond

This comment has been minimized.

Show comment
Hide comment
@robin-raymond

robin-raymond Apr 14, 2016

Contributor

I also think all sender methods should return a promise.

@murillo128 I'm not sure of "all" but certainly there is an issue outstanding on the RTCRtpSender.send() returning a promise and your issue above suggests setTransport() needs to be a promise result. If you feel other APIs require promises too then you should file issues on each and explain why it is required. Issues are easier to get resolved if they stay as focused as possible.

Contributor

robin-raymond commented Apr 14, 2016

I also think all sender methods should return a promise.

@murillo128 I'm not sure of "all" but certainly there is an issue outstanding on the RTCRtpSender.send() returning a promise and your issue above suggests setTransport() needs to be a promise result. If you feel other APIs require promises too then you should file issues on each and explain why it is required. Issues are easier to get resolved if they stay as focused as possible.

@robin-raymond

This comment has been minimized.

Show comment
Hide comment
@robin-raymond

robin-raymond Apr 21, 2016

Contributor

$related #468

This peerIdentity stuff has been removed from 1.0 and changed to "do not send". No more exception, nor promise related issues.

Contributor

robin-raymond commented Apr 21, 2016

$related #468

This peerIdentity stuff has been removed from 1.0 and changed to "do not send". No more exception, nor promise related issues.

@robin-raymond

This comment has been minimized.

Show comment
Hide comment
@robin-raymond

robin-raymond Apr 21, 2016

Contributor

Currently a 1.0 compatibility issue, need to strip errors/promise resolution issues for peerIdentity, we need to fix, and we will follow lead of webrtc 1.0 on this one. They currently say "don't transmit" so that's what we will do too.

Contributor

robin-raymond commented Apr 21, 2016

Currently a 1.0 compatibility issue, need to strip errors/promise resolution issues for peerIdentity, we need to fix, and we will follow lead of webrtc 1.0 on this one. They currently say "don't transmit" so that's what we will do too.

@aboba aboba added PR exists and removed 1.1 PR exists labels Apr 25, 2016

@elagerway

This comment has been minimized.

Show comment
Hide comment
@elagerway

elagerway Apr 26, 2016

Contributor

Closing, this needs to be taken up in the WebRTC Working Group as per my recent message to the mail list:
https://lists.w3.org/Archives/Public/public-ortc/2016Apr/0165.html

Contributor

elagerway commented Apr 26, 2016

Closing, this needs to be taken up in the WebRTC Working Group as per my recent message to the mail list:
https://lists.w3.org/Archives/Public/public-ortc/2016Apr/0165.html

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