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

Define c= attribute in the case of only mDNS candidates #91

Closed
jeroendeb opened this issue Feb 7, 2019 · 8 comments · Fixed by #113
Closed

Define c= attribute in the case of only mDNS candidates #91

jeroendeb opened this issue Feb 7, 2019 · 8 comments · Fixed by #113
Assignees
Labels
triaged Author team has reviewed
Milestone

Comments

@jeroendeb
Copy link
Collaborator

jeroendeb commented Feb 7, 2019

Currently it contains:

c=IN IP4 address/ttl

Obviously, putting one of the candidate's IP address there would defeat the purpose of use mDNS.

Options:

Can this be omitted?
Should it have a fake IP address of the right address family?
Should it be allowed to have an mDNS name?

@juberti
Copy link
Collaborator

juberti commented Feb 7, 2019

Just use the mDNS name.

@juberti
Copy link
Collaborator

juberti commented Feb 7, 2019

If we run into issues, we could use 0.0.0.0:9 as we do when there are no candidates, but that starts to get complicated (i.e. may require updates to other specs)

@jeroendeb
Copy link
Collaborator Author

mDNS name with IP4?

@juberti
Copy link
Collaborator

juberti commented Feb 7, 2019

good point - shouldn't matter that much, but IP4 seems like the safest choice.

@juberti juberti added this to the IETF 104 milestone Feb 13, 2019
@juberti
Copy link
Collaborator

juberti commented Feb 28, 2019

Agreed to use mDNS + IP4. We'll reevaluate if this causes problems.

@juberti
Copy link
Collaborator

juberti commented Mar 11, 2019

Fixed by #102

@juberti juberti closed this as completed Mar 11, 2019
@tQsW
Copy link
Collaborator

tQsW commented Jul 14, 2019

We have observed an interop issue in the experimental deployment. Endpoints can fail to parse the SDP when the c= line contains a FQDN.

If we run into issues, we could use 0.0.0.0:9 as we do when there are no candidates, but that starts to get complicated (i.e. may require updates to other specs)

This is the current solution.

@tQsW tQsW reopened this Jul 14, 2019
@juberti
Copy link
Collaborator

juberti commented Jul 15, 2019

Well, at least we thought of the solution already :)

caiolima pushed a commit to caiolima/webkit that referenced this issue Jul 16, 2019
https://bugs.webkit.org/show_bug.cgi?id=199791

Reviewed by Eric Carlson.

Source/WebCore:

As discussed in rtcweb-wg/mdns-ice-candidates#91,
use 0.0.0.0 for c lines when filtering the SDP.
Covered by updated test.

* Modules/mediastream/PeerConnectionBackend.cpp:
(WebCore::PeerConnectionBackend::filterSDP const):

LayoutTests:

* webrtc/datachannel/filter-ice-candidate.html:


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@247430 268f45cc-cd09-0410-ab3c-d52691b4dbfc
juberti added a commit that referenced this issue Aug 14, 2019
youennf pushed a commit that referenced this issue Sep 26, 2019
* Update c= line guidance
* Clarify that "::" is acceptable in c= line
* Also clean up and provide reference for 0.0.0.0 in raddr
Fixes #91
ryanhaddad pushed a commit to WebKit/WebKit that referenced this issue Dec 22, 2020
https://bugs.webkit.org/show_bug.cgi?id=199791

Reviewed by Eric Carlson.

Source/WebCore:

As discussed in rtcweb-wg/mdns-ice-candidates#91,
use 0.0.0.0 for c lines when filtering the SDP.
Covered by updated test.

* Modules/mediastream/PeerConnectionBackend.cpp:
(WebCore::PeerConnectionBackend::filterSDP const):

LayoutTests:

* webrtc/datachannel/filter-ice-candidate.html:


Canonical link: https://commits.webkit.org/213663@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@247430 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triaged Author team has reviewed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants