Closed
Bug 829757
Opened 11 years ago
Closed 11 years ago
Accept a=candidate ICE candidates (for now)
Categories
(Core :: WebRTC: Signaling, defect, P2)
Tracking
()
RESOLVED
FIXED
mozilla21
People
(Reporter: ekr, Assigned: ekr)
Details
(Whiteboard: [webrtc][blocking-webrtc+][qa-])
Attachments
(2 files, 4 obsolete files)
5.85 KB,
patch
|
ehugg
:
review+
|
Details | Diff | Splinter Review |
4.50 KB,
patch
|
abr
:
review+
|
Details | Diff | Splinter Review |
Google emits candidates with "a=...\r\n" The spec says not to do this but Justin wants to discuss at the meeting. In the interim, let's accept it.
Updated•11 years ago
|
Assignee: nobody → ekr
Priority: -- → P2
Summary: Accept Google's a=candidate ICE candidates → Accept a=candidate ICE candidates (for now)
Whiteboard: [webrtc][blocking-webrtc+]
Assignee | ||
Comment 1•11 years ago
|
||
Assignee | ||
Comment 2•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #703356 -
Attachment is obsolete: true
Assignee | ||
Comment 3•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #703424 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #703425 -
Flags: review?(ethanhugg)
Comment 4•11 years ago
|
||
Comment on attachment 703425 [details] [diff] [review] Strip a= from trickle ICE candidates Review of attachment 703425 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. Built and ran on Linux, no new warnings.
Attachment #703425 -
Flags: review?(ethanhugg) → review+
Assignee | ||
Comment 5•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d6a8a229453
Comment 6•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7d6a8a229453
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Assignee | ||
Comment 7•11 years ago
|
||
Assignee | ||
Comment 8•11 years ago
|
||
Unfortunately, putting this where we put it doesn't work for candidates received before we CreateAnswer because they are processed via the SDP. See patch.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Updated•11 years ago
|
Attachment #704273 -
Flags: review?(adam)
Comment 9•11 years ago
|
||
Comment on attachment 704273 [details] [diff] [review] Move stripping to SIPCC Review of attachment 704273 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c @@ +3646,3 @@ > level = msg->data.candidate.level; > gsmsdp_set_ice_attribute (SDP_ATTR_ICE_CANDIDATE, level, > + dcb->sdp->dest_sdp, candidate); I don't *think* this is quite right. In the case of "a=<candidate-attribute>\r\n", you're stripping the "a=", but leaving the "\r\n" If you follow the call chain down through gsmsdp_set_ice_attribute -> sdp_attr_set_ice_attribute, you'll see that we do a straight sstrncpy into attr_p->ice_attr -- including the \r and the \n If I examine what these strings are supposed to look like, the debugger has: ice_attr = "candidate:0 1 UDP 2113601791 192.168.0.240 57162 typ host", '\0' <repeats 199 times>, Without the \r\n. I think you need to add logic that searches for a '\r' and replaces it with a '\0' before passing the buffer in to gsmsdp_set_ice_attribute; otherwise it is pretty likely that we'll end up spitting the SDP out with "\r\n\r\n" in it. (Sorry, I've run out of time to verify, but it should be possible to double-check this by adding an ice candidate with a \r\n at the end, and then examining the remote description in the PeerConnectionImpl -- it's certainly possible that I'm wrong here).
Assignee | ||
Comment 10•11 years ago
|
||
I certainly agree that i'm not stripping the \r\n, but I'm not following why this is a problem: the parser is OK with having a \r\n on the end because it's OK with anything trailing. WRT the last graf, wouldn't we emit \r\na=\r\n?
Assignee | ||
Comment 11•11 years ago
|
||
Oh, never mind. I see your point about the SDP generator. I had forgotten we needed that to work as well. I'll add code to strip it.
Assignee | ||
Comment 12•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #704273 -
Attachment is obsolete: true
Attachment #704273 -
Flags: review?(adam)
Assignee | ||
Updated•11 years ago
|
Attachment #704582 -
Flags: review?(adam)
Comment 13•11 years ago
|
||
Comment on attachment 704582 [details] [diff] [review] Move stripping to SIPCC Review of attachment 704582 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c @@ +3634,5 @@ > /* Update remote SDP with new candidate information */ > + candidate = (char *)msg->data.candidate.candidate; > + if (strlen(candidate) < 2) { > + FSM_DEBUG_SM(DEB_F_PREFIX"bogus candidate value.\n", DEB_F_PREFIX_ARGS(FSM, __FUNCTION__)); > + return SM_RC_CLEANUP; I think we want SM_RC_END here. I believe that returning SM_RC_CLEANUP would result in the whole call going away, which is a bit severe for this kind of syntax error (at least, that's my understanding -- we might want to run this by Ethan to be sure). The need to perform this length check is also not obvious. If we actually need it, please add a comment indicating why.
Assignee | ||
Comment 14•11 years ago
|
||
OK, I can live with END. The length check is an abundance of caution. I could ok with removing it.
Assignee | ||
Comment 15•11 years ago
|
||
That has the nice side effect of ignoring the error check question.
Assignee | ||
Comment 16•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #704582 -
Attachment is obsolete: true
Attachment #704582 -
Flags: review?(adam)
Assignee | ||
Comment 17•11 years ago
|
||
Comment on attachment 704605 [details] [diff] [review] Move stripping to SIPCC Review of attachment 704605 [details] [diff] [review]: ----------------------------------------------------------------- one more time
Attachment #704605 -
Flags: review?(adam)
Comment 18•11 years ago
|
||
Comment on attachment 704605 [details] [diff] [review] Move stripping to SIPCC Review of attachment 704605 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. ::: media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c @@ +3630,5 @@ > /* comment until mid is properly updated > cause = gsmsdp_find_level_from_mid(dcb, (const char *)msg->data.candidate.mid, &level); > */ > > + /* XXX: Need to sanity check that this is actually a valid candidate */ This comment would be better if it contained a bug number -- I agree that this check is a necessary improvement; we should make sure it's tracked.
Attachment #704605 -
Flags: review?(adam) → review+
Assignee | ||
Comment 19•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=9ef706ddb4b8
Comment 20•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/77802f9c2b7f
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
Whiteboard: [webrtc][blocking-webrtc+] → [webrtc][blocking-webrtc+][qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•