Closed
Bug 1018473
Opened 11 years ago
Closed 11 years ago
Answerer frequently emits duplicate trickle candidates
Categories
(Core :: WebRTC: Networking, defect)
Core
WebRTC: Networking
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: bwc, Assigned: bwc)
Details
Attachments
(2 files, 2 obsolete files)
1.16 KB,
patch
|
bwc
:
review+
|
Details | Diff | Splinter Review |
3.41 KB,
patch
|
bwc
:
review+
|
Details | Diff | Splinter Review |
When we set a remote offer, sipcc constructs a throwaway local description by calling vcmRxAllocICE, which connects to a signal that fires when candidates are gathered. Then, when the answer is created, vcmRxAllocICE is called again, which connects to the same signal again. From there on, any trickled candidate is signaled twice, leading to duplicates.
Assignee | ||
Comment 1•11 years ago
|
||
Initial cut.
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → docfaraday
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•11 years ago
|
||
Comment on attachment 8431942 [details] [diff] [review]
Detect when vcmRxAllocICE has already been called for a given stream, and suppress the (duplicate) connection to SignalCandidate.
Review of attachment 8431942 [details] [diff] [review]:
-----------------------------------------------------------------
Pushing to try, in case somehow something is relying on the duplicate candidates.
https://tbpl.mozilla.org/?tree=Try&rev=ffe0cf97cb2b
Attachment #8431942 -
Flags: review?(ekr)
Assignee | ||
Comment 3•11 years ago
|
||
Go ahead and remove a param that I was worried would change, but that we don't actually use.
Assignee | ||
Updated•11 years ago
|
Attachment #8431942 -
Attachment is obsolete: true
Attachment #8431942 -
Flags: review?(ekr)
Assignee | ||
Updated•11 years ago
|
Attachment #8431995 -
Flags: review?(ekr)
Comment 4•11 years ago
|
||
Updated•11 years ago
|
Assignee: docfaraday → ekr
Updated•11 years ago
|
Attachment #8432123 -
Flags: review?(docfaraday)
Comment 5•11 years ago
|
||
Comment on attachment 8431995 [details] [diff] [review]
Detect when vcmRxAllocICE has already been called for a given stream, and suppress the (duplicate) connection to SignalCandidate.
Review of attachment 8431995 [details] [diff] [review]:
-----------------------------------------------------------------
lgtm
::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
@@ +592,5 @@
> *candidatesp = nullptr;
> *candidate_ctp = 0;
>
> + // This can be called multiple times; don't connect to the signal more than
> + // once.
Please add a comment on why and maybe the bug # so people have background.
Attachment #8431995 -
Flags: review?(ekr) → review+
Assignee | ||
Comment 6•11 years ago
|
||
Add a pointer to this bug for the sake of explanation.
Assignee | ||
Updated•11 years ago
|
Attachment #8431995 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Assignee: ekr → docfaraday
Assignee | ||
Comment 7•11 years ago
|
||
Comment on attachment 8432169 [details] [diff] [review]
Detect when vcmRxAllocICE has already been called for a given stream, and suppress the (duplicate) connection to SignalCandidate.
Review of attachment 8432169 [details] [diff] [review]:
-----------------------------------------------------------------
Carry forward r=ekr
Attachment #8432169 -
Flags: review+
Assignee | ||
Comment 8•11 years ago
|
||
Comment on attachment 8432123 [details] [diff] [review]
Unit test for duplicate trickle candidates
Review of attachment 8432123 [details] [diff] [review]:
-----------------------------------------------------------------
lgtm
Attachment #8432123 -
Flags: review?(docfaraday) → review+
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 9•11 years ago
|
||
Comment 10•11 years ago
|
||
removing checkin needed since it seems ekr landed this in comment 9
Keywords: checkin-needed
Comment 11•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/01cd5e37935a
https://hg.mozilla.org/mozilla-central/rev/7c9493f52d19
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in
before you can comment on or make changes to this bug.
Description
•