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

reject with InvalidAccessError when mux policy does not match #1942

Merged
merged 2 commits into from
Aug 2, 2018

Conversation

fippo
Copy link
Contributor

@fippo fippo commented Jul 22, 2018

fixes #1235


Preview | Diff

@fippo
Copy link
Contributor Author

fippo commented Jul 22, 2018

needs test. Basically:

  • create a peerconnection with muxpolicy
  • set a short sdp without mux
  • expect error to be thrown

webrtc.html Outdated
and the remote description does not use RTCP mux,
then <a>reject</a> <var>p</var> with a newly <a data-link-for="exception"
data-lt="create">created</a>
<code>NotSupportedError</code> and abort these steps.
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the argument for using NotSupportedError and not either InvalidAccessError (thrown for all other invalid SDP) or OperationError (thrown for all other errors)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is the same error as in "4.4.1.7 Set the configuration" when trying to set 'negotiate' in the constructor and this is unsupported.
I do not think this is invalidaccesserror as the SDP is valid. OperationError would wfm too

Copy link
Contributor

Choose a reason for hiding this comment

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

Or InvalidStateError, which we throw when trying to perform an operation that is otherwise supported but while the PC is in an invalid state (such as being closed).

But InvalidAccessError is probably the error most consistent with other usages in the spec (rejected SDP, invalid parameters, invalid initEncodings, invalid use of addTrack/removeTrack).

NotSupportedError on the other hand is typically used when what you're doing is not implemented by the browser, e.g. non-muxed RTCP, url scheme name, keygenAlgorithm. This usage makes it sound like non-muxed RTCP is not implemented (like the other usage) as opposed to PC not being configured that way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you can't configure the PC to actually do non-muxed -- mux is either negotiate or require. And I think the usage of NotSupportedError is exactly what you say hbos :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

Given the comments above it feels like we should go with InvalidAccessError. Would you agree @fippo ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i have no strong opinion either way, changed to invalidaccesserror.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

var pc = new RTCPeerConnection({rtcpMuxPolicy: 'require'}); pc.setRemoteDescription({type: 'offer', sdp: "v=0\r\no=- 166855176514521964 2 IN IP4 127.0.0.1\r\ns=-\r\nt=0 0\r\nm=audio 9 UDP/TLS/RTP/SAVPF 111\r\nc=IN IP4 0.0.0.0\r\na=rtcp:9 IN IP4 0.0.0.0\r\na=ice-ufrag:someufrag\r\na=ice-pwd:somelongpwdwithenoughrandomness\r\na=fingerprint:sha-256 00:00:00:00:00:00:00:00:00:00:00:00:00:00:00:00:00:00:00:00:00:00:00:00:00:00:00:00:00:00:00:00\r\na=setup:actpass\r\na=mid:audio1\r\na=sendonly\r\na=rtcp-rsize\r\na=rtpmap:111 opus/48000/2\r\n"}).catch(e => console.error(e.name))

throws InvalidAccessError. Not changing implementations is a good argument :-)

fippo added a commit to fippo/web-platform-tests that referenced this pull request Jul 30, 2018
…emoteDescription

throws InvalidAccessError when setting a remote description
without rtcp-mux and rtcpMuxPolicy is set to 'require'.

test for w3c/webrtc-pc#1942
fippo added a commit to fippo/web-platform-tests that referenced this pull request Jul 30, 2018
…emoteDescription

throws InvalidAccessError when setting a remote description
without rtcp-mux and rtcpMuxPolicy is set to 'require'.

test for w3c/webrtc-pc#1942
@fippo
Copy link
Contributor Author

fippo commented Aug 2, 2018

while implementing this: we don't care about the rather weird case where only one m-line does not do rtcp-mux?

If we do care this would actually mean the m-line should be rejected instead.

JSEP says

Applying a description with an m= section that does not contain an "a=rtcp-mux" attribute
will cause an error to be returned.

which I interpret as "error if you find any non-muxed"

@fippo fippo changed the title throw NotSupportedError when mux policy does not match reject with InvalidAccessError when mux policy does not match Aug 2, 2018
@aboba aboba merged commit af944d0 into w3c:master Aug 2, 2018
@fippo fippo deleted the muxpolicy branch August 2, 2018 17:27
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.

setRemoteDescription error when rtcpMuxPolicy is "require" and SDP is missing "a=rtcp-mux"
5 participants