Closed Bug 1192813 Opened 9 years ago Closed 9 years ago

WebRTC SDP sets 0.0.0.0 in c line with multiple NICs

Categories

(Core :: WebRTC: Signaling, defect, P3)

39 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: steve, Assigned: mjf)

References

Details

Attachments

(1 file)

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/44.0.2403.125 Safari/537.36

Steps to reproduce:

Firefox 39.0.3 and 39.0 with WebRTC generates SDP with c=IN IP4 0.0.0.0. Originally existed as bug#1072384, which was reported resolved by bug#1091242.

v=0
o=mozilla...THIS_IS_SDPARTA-39.0.3 4294967295 0 IN IP4 0.0.0.0
s=-
t=0 0
a=sendrecv
a=ice-options:trickle
a=msid-semantic:WMS *
m=audio 9 RTP/SAVPF 8
c=IN IP4 0.0.0.0
a=candidate:0 1 UDP 2128609535 172.21.11.220 52813 typ host
a=candidate:4 1 UDP 2128543999 192.168.56.1 52814 typ host
a=candidate:0 2 UDP 2128609534 172.21.11.220 52815 typ host
a=candidate:4 2 UDP 2128543998 192.168.56.1 52816 typ host



Actual results:

Tested on Windows 7 thru Windows 10, if there is > 1 host address (ie. active network interfaces with an IP) this issue occurs, if there is only one host address, the c= line is generated correctly using a value from one of the SDP candidates.



Expected results:

Copied from bug#1072384:

"
This is causing an ICE mismatch (ref RFC5245 section 15.3) against standard SIP endpoints, since they expect to see a c= line that contains an IP address that is present in at least one of the ICE candidates.

I can understand that 0.0.0.0 could be valid to use with Trickle ICE, when no candidates are known (although I would probably expect the host address should be used here).

However, given my SIP endpoint does not support trickling, I am waiting for ICE candidate gathering to be complete, and as such Firefox really should populate that c= line before sending the final SDP.
"
Component: Untriaged → WebRTC: Networking
Product: Firefox → Core
Does the SDP have an a=end-of-candidates attribute? We do not try to set the c-line until ICE gathering concludes, since it doesn't make sense to have a default candidate until all the candidates are in.
Flags: needinfo?(steve)
(I should note that the presence of a=ice-options:trickle implies that ICE gathering is not done yet)
Thanks Byron, I believe I have a better understanding of what is happening now. The 2nd NIC is not usable, so ICE will eventually time-out probing that NIC, massively delaying the setting of the 'c' line, and making the SDP invalid until that time.

This only leads me to believe that the Firefox trickle-ice implementation is incomplete because the 'c' line should (must?) be set to the best-known default at the current time - I determined this from RFC5245 section 9.1.2.1: 

"The agent MAY change the default destination for media.  As with
 initial offers, there MUST be a set of candidate attributes in the
 offer matching this default destination."

Further:

RFC 5245 section 9.1.1.1:

"These rules imply that setting the IP address in the 'c' line to
 0.0.0.0 will cause an ICE restart.  Consequently, ICE implementations
 MUST NOT utilize this mechanism for call hold, and instead MUST use
 a=inactive and a=sendonly as described in RFC3264"

So leaving "c=IN IP4 0.0.0.0" is implying a restart before you've even started?

Also, RFC 5245 section 5.1:

"The agent will proceed with the ICE procedures defined in this
 specification if, for each media stream in the SDP it received, the
 default destination for each component of that media stream appears
 in a candidate attribute.  For example, in the case of RTP, the IP
 address and port in the c and m lines, respectively, appear in a
 candidate attribute and the value in the rtcp attribute appears in a
 candidate attribute.

 If this condition is not met, the agent MUST process the SDP based on
 normal RFC 3264 procedures, without using any of the ICE mechanisms
 described in the remainder of this specification..."

So ICE may not proceed unless there is a valid 'c' and 'm' line present which matches one of the initial candidates.

Additionally, the 'm' line has a port number set to 9 under these circumstances. I am not sure where FF gets the 9 from, but it seems a little arbitrary?

Of course my interpretation of the above may be completely flawed - I'd be interested to know if I've got it wrong :)
Flags: needinfo?(steve)
For many implementations I'd suggest that
  "it doesn't make sense to have a default candidate until all the candidates are in"
is not true.

The ICE candidates may take an arbitrarily long time to arrive, but the negotiation of the connection can be started immediately if the immediately available 'host' candidates are embedded in the SDP, and the default ip/port values are set. In fact for the perfectly possible case where the 2 endpoints are able to communicate directly on local interfaces, no further candidates are needed, and this would be a significant optimisation.

This was I believe FF's behaviour prior to FF v37, where host candidates were provided, and other candidates trickled.

Chrome's answer to this is I believe elegant in that it constantly updates its internal SDP as candidates are added, and updates the 'c' line and 'm' line at the same time to be the best candidate so far, that way the controlling application can decide to read a very early (but still valid) SDP, or wait for either all candidates or a timeout before reading an updated (but always valid) SDP.
Thanks Eric - Something else I need to implement at my end that had escaped my notice previously :)
I realise that all of my statements above are based on the assumption of an intention to retain backward compatibility and compatibility with non-FF based WebRTC implementations.

It has crossed my mind that it may have been decided that FF will make support for trickle ICE mandatory, and forgo non-trickle ICE completely as is suggested as one option in the draft.

Is there a thread anywhere discussing the addition of trickle ice to FF, and what if any concessions to backward compatibility have been decided upon (eg half-trickle) which is what I had assumed would be the case.
WebRTC is specified as trickle-only, so we're generally conforming to the specification.

It might be good to update the default pairs, but this is principally a question for the
IETF MMUSIC/ICE WG, and then we would just make ourselves compatible.
I think bug 1072384 was resolved because we switched to trickle-only, per ekr in comment 8.  Would updating the default pairs as Chrome is doing be out of spec?  Also, what's the actual impact of this in practice?
(In reply to Steve Davies from comment #3)
> Thanks Byron, I believe I have a better understanding of what is happening
> now. The 2nd NIC is not usable, so ICE will eventually time-out probing that
> NIC, massively delaying the setting of the 'c' line, and making the SDP
> invalid until that time.
> 

   The only reason the c-line (and a=rtcp) is set in ICE is to allow endpoints that do not support ICE at all to interop.

> This only leads me to believe that the Firefox trickle-ice implementation is
> incomplete because the 'c' line should (must?) be set to the best-known
> default at the current time - I determined this from RFC5245 section
> 9.1.2.1: 
> 
> "The agent MAY change the default destination for media.  As with
>  initial offers, there MUST be a set of candidate attributes in the
>  offer matching this default destination."
> 
> Further:
> 
> RFC 5245 section 9.1.1.1:
> 
> "These rules imply that setting the IP address in the 'c' line to
>  0.0.0.0 will cause an ICE restart.  Consequently, ICE implementations
>  MUST NOT utilize this mechanism for call hold, and instead MUST use
>  a=inactive and a=sendonly as described in RFC3264"
> 
> So leaving "c=IN IP4 0.0.0.0" is implying a restart before you've even
> started?

   These two are talking about renegotiation, since this spec pre-dates trickle ICE.

> 
> Also, RFC 5245 section 5.1:
> 
> "The agent will proceed with the ICE procedures defined in this
>  specification if, for each media stream in the SDP it received, the
>  default destination for each component of that media stream appears
>  in a candidate attribute.  For example, in the case of RTP, the IP
>  address and port in the c and m lines, respectively, appear in a
>  candidate attribute and the value in the rtcp attribute appears in a
>  candidate attribute.
> 
>  If this condition is not met, the agent MUST process the SDP based on
>  normal RFC 3264 procedures, without using any of the ICE mechanisms
>  described in the remainder of this specification..."
> 
> So ICE may not proceed unless there is a valid 'c' and 'm' line present
> which matches one of the initial candidates.

   This is all correct for non-trickle implementations. None of these rules really make sense for trickle ICE, since it is expected that the SDP is incomplete wrt ICE.

(In reply to Steve Davies from comment #4)
> For many implementations I'd suggest that
>   "it doesn't make sense to have a default candidate until all the
> candidates are in"
> is not true.
> 
> The ICE candidates may take an arbitrarily long time to arrive, but the
> negotiation of the connection can be started immediately if the immediately
> available 'host' candidates are embedded in the SDP, and the default ip/port
> values are set. In fact for the perfectly possible case where the 2
> endpoints are able to communicate directly on local interfaces, no further
> candidates are needed, and this would be a significant optimisation.
>

Let's break this down:

Case 1: Both endpoints are trickle. In this case it is a non-issue.

Case 2: One endpoint is a non-trickle ICE agent. This agent will not actually use the c-line or a=rtcp, for starters. And, sending the SDP prematurely (with only host candidates) will probably cause ICE to fail irrecoverably anyway, since you don't get a chance to send more candidates.

Case 3: One endpoint doesn't support ICE at all. Sending the SDP prematurely (with a host candidate in the c-line and a=rtcp) will probably cause ICE to fail irrecoverably.

   The point is, setting the c-line and a=rtcp early isn't actually useful for anything but endpoints that don't support ICE at all, and only in cases where gathering takes pathologically long due to misconfiguration. I've never heard of anyone trying to do webrtc with a non-ICE endpoint, so I'm not all that inclined to prioritize this work.
> The only reason the c-line (and a=rtcp) is set in ICE is to allow endpoints that do not support
> ICE at all to interop.

Not true, the ICE RFC requires that c= and m= contain a default IP/port pair that match a candidate before (non-trickle) ICE may proceed.

Case 1 - Agreed.

Case 2 - IMHO misses a couple of elements.

a) As above, a non-trickle ICE endpoint MUST receive a valid c= line because the RFC says that it MUST have valid c=/m= lines in order to enable the use of ICE.

b) There is an opportunity for the client application to choose to either wait or not wait for candidates to arrive. In fact in one app we use the compromise of waiting up to 2 seconds for candidates in order to prevent massive delays caused by unusable network routes.

Case 3 - While I agree with the sentiment (ie. this would be silly), in an Internet scenario your statement is true, but what about an office LAN? It would work 100% of the time. Your statement prejudiced to a specific use-case - Browser are used for the strangest of projects these days!

> The point is, setting the c-line and a=rtcp early isn't actually useful for anything but
> endpoints that don't support ICE at all, and only in cases where gathering takes pathologically
> long due to misconfiguration. I've never heard of anyone trying to do webrtc with a non-ICE
> endpoint, so I'm not all that inclined to prioritize this work.

As an experiment, try installing VirtualBox on a PC - This creates a virtual NIC that is very useful for VirtualBox, but causes almost all ICE transactions to take 10+ seconds to complete gathering. There is nothing broken or misconfigured, but ICE hates it!

Whether this work is worthwhile or not seems to depend on what the stated intention of the FF WebRTC team is - If it is to be 100% trickle-only, then close this ticket as we don't need to follow the suggested compatibility helpers in the trickle-ice draft. OTOH my impressions of the FF 'ethos' are that it would follow as many of the compatibility helpers as is practical, and indeed up until about FF 36, it seemed to be doing just that.
As an aside, "c=IN IP4 0.0.0.0" is invalid in trickle-ice except to indicate renegotiation.

From https://tools.ietf.org/html/draft-ietf-mmusic-trickle-ice-02#section-5.1

> ...a trickle ICE session
> description MAY contain no candidates at all.  In such cases the
> agent would still need to place an address in the "c=" line(s).  If
> the use of a host address there is undesirable (e.g. for privacy
> reasons), the agent MAY set the connection address to IP6 ::. In this
> case it MUST also set the port number to 9 (Discard).  There is no
> need to include a fictitious candidate for the IP6 :: address when
> doing so.

Which I believe means you must use "c=IN IP6 ::", even if IPv6 is not being used. It also implies (but does not clearly state) that this can only be used if there are no candidates, reverting to the non-trickle ICE requirement of having valid candidate-based c=/m= lines once candidates exist in the SDP.

I suspect that this is what prompted the constant-updating Chrome behaviour I mentioned earlier.

Now, in recent FF builds, IIRC, candidates are never inserted into the SDP, so a permanent "c=IN IP6 ::" line would be a valid (if slightly unfriendly) option.
(In reply to Steve Davies from comment #11)
> > The only reason the c-line (and a=rtcp) is set in ICE is to allow endpoints that do not support
> > ICE at all to interop.
> 
> Not true, the ICE RFC requires that c= and m= contain a default IP/port pair
> that match a candidate before (non-trickle) ICE may proceed.

Yes, but as Byron says the only reason to do this is for non-ICE.
JSEP explicitly states that you are not supposed to use these.

http://rtcweb-wg.github.io/jsep/#sec.session-level-parse
"The "c=" line MUST be checked for syntax but its value is not used. This supersedes the guidance in [RFC5245], Section 6.1, to use "ice-mismatch" to indicate mismatches between "c=" and the candidate lines; because JSEP always uses ICE, "ice-mismatch" is not useful in this context."


> Case 1 - Agreed.
> 
> Case 2 - IMHO misses a couple of elements.
> 
> a) As above, a non-trickle ICE endpoint MUST receive a valid c= line because
> the RFC says that it MUST have valid c=/m= lines in order to enable the use
> of ICE.
> 
> b) There is an opportunity for the client application to choose to either
> wait or not wait for candidates to arrive. In fact in one app we use the
> compromise of waiting up to 2 seconds for candidates in order to prevent
> massive delays caused by unusable network routes.
> 
> Case 3 - While I agree with the sentiment (ie. this would be silly), in an
> Internet scenario your statement is true, but what about an office LAN? It
> would work 100% of the time. Your statement prejudiced to a specific
> use-case - Browser are used for the strangest of projects these days!

Browsers simply will not communicate with non-ICE endpoints for security
reasons. See:
http://rtcweb-wg.github.io/security-arch/


> > The point is, setting the c-line and a=rtcp early isn't actually useful for anything but
> > endpoints that don't support ICE at all, and only in cases where gathering takes pathologically
> > long due to misconfiguration. I've never heard of anyone trying to do webrtc with a non-ICE
> > endpoint, so I'm not all that inclined to prioritize this work.
> 
> As an experiment, try installing VirtualBox on a PC - This creates a virtual
> NIC that is very useful for VirtualBox, but causes almost all ICE
> transactions to take 10+ seconds to complete gathering. There is nothing
> broken or misconfigured, but ICE hates it!

This is the reason for trickle.


> Whether this work is worthwhile or not seems to depend on what the stated
> intention of the FF WebRTC team is - If it is to be 100% trickle-only, then
> close this ticket as we don't need to follow the suggested compatibility
> helpers in the trickle-ice draft. OTOH my impressions of the FF 'ethos' are
> that it would follow as many of the compatibility helpers as is practical,
> and indeed up until about FF 36, it seemed to be doing just that.

As I noted in a previous comment, the specification requires that browsers be trickle-only.
The only question is what the c= line should be set to in the period after
one candidate has been found and before ICE gathering has completed. But this
is a specification issue, not really a Firefox issue. I've filed a bug on this at:

https://github.com/rtcweb-wg/jsep/issues/171

However, given that browsers require you to do ICE, all that's required here is to
have something non-bogus in the c= and m= lines (where non-bogus is defined as
not breaking the other end). This can be done by simply modifying the SDP in your
JS/signaling layer.
(In reply to Steve Davies from comment #12)
> As an aside, "c=IN IP4 0.0.0.0" is invalid in trickle-ice except to indicate
> renegotiation.
> 
> From https://tools.ietf.org/html/draft-ietf-mmusic-trickle-ice-02#section-5.1
> 
> > ...a trickle ICE session
> > description MAY contain no candidates at all.  In such cases the
> > agent would still need to place an address in the "c=" line(s).  If
> > the use of a host address there is undesirable (e.g. for privacy
> > reasons), the agent MAY set the connection address to IP6 ::. In this
> > case it MUST also set the port number to 9 (Discard).  There is no
> > need to include a fictitious candidate for the IP6 :: address when
> > doing so.
> 
> Which I believe means you must use "c=IN IP6 ::", even if IPv6 is not being
> used.

This turns out to break a lot of people and IETF has decided to change it
back to 0.0.0.0.
See:
https://github.com/rtcweb-wg/jsep/issues/170



> Now, in recent FF builds, IIRC, candidates are never inserted into the SDP,
> so a permanent "c=IN IP6 ::" line would be a valid (if slightly unfriendly)
> option.

This is not confirmed by my tests on Firefox Nightly. If you can reproduce
this, please provide a test case that demonstrates it.
OK, would someone care to close this 'as designed'

I personally find this direction unfortunate, but Mozilla/FF cannot be blamed for following the "standards".
(In reply to Steve Davies from comment #15)
> OK, would someone care to close this 'as designed'

I'm holding this open until the JSEP issue above is resolved.


> I personally find this direction unfortunate, but Mozilla/FF cannot be
> blamed for following the "standards".
We discussed this on the JSEP call today. Consensus was Firefox current behavior was suboptimal, so we should fix this to generate a valid c= line once anything has been gathered. It's probably not going to be super-high priority, though. Patches welcome.

https://github.com/rtcweb-wg/jsep/issues/171
Status: UNCONFIRMED → NEW
backlog: --- → webrtc/webaudio+
Rank: 35
Ever confirmed: true
Priority: -- → P3
Component: WebRTC: Networking → WebRTC: Signaling
Assignee: nobody → mfroman
Bug 1192813 - update the default candidate as new candidates arrive.
Attachment #8672669 - Flags: review?(docfaraday)
Comment on attachment 8672669 [details]
MozReview Request: Bug 1192813 - update the default candidate as new candidates arrive.

Bug 1192813 - update the default candidate as new candidates arrive.
https://reviewboard.mozilla.org/r/21763/#review20735

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:1059
(Diff revision 2)
> -  SignalCandidate(candidate, aMLine);
> +  SignalCandidate(candidateLine, aMLine);
> +  if (!aDefaultAddr.empty()) {
> +    SignalUpdateDefaultCandidate(aDefaultAddr,
> +                                 aDefaultPort,
> +                                 aDefaultRtcpAddr,
> +                                 aDefaultRtcpPort,
> +                                 aMLine);
> +  }

Try switching the order of these two; I would expect that trickling the candidate first would result in onicecandidate firing in JS before the SDP is updated with the default candidate. Also, we probably want to check !aDefaultRtcpAddr.empty() as well.
Comment on attachment 8672669 [details]
MozReview Request: Bug 1192813 - update the default candidate as new candidates arrive.

Bug 1192813 - update the default candidate as new candidates arrive.
Comment on attachment 8672669 [details]
MozReview Request: Bug 1192813 - update the default candidate as new candidates arrive.

Bug 1192813 - update the default candidate as new candidates arrive.
Comment on attachment 8672669 [details]
MozReview Request: Bug 1192813 - update the default candidate as new candidates arrive.

Bug 1192813 - update the default candidate as new candidates arrive.
Comment on attachment 8672669 [details]
MozReview Request: Bug 1192813 - update the default candidate as new candidates arrive.

Bug 1192813 - update the default candidate as new candidates arrive.
Comment on attachment 8672669 [details]
MozReview Request: Bug 1192813 - update the default candidate as new candidates arrive.

Bug 1192813 - update the default candidate as new candidates arrive.
Comment on attachment 8672669 [details]
MozReview Request: Bug 1192813 - update the default candidate as new candidates arrive.

https://reviewboard.mozilla.org/r/21765/#review21205

There's very little here besides coding convention tweaks and some naming/phrasing improvements.

::: dom/media/tests/mochitest/pc.js:1281
(Diff revision 7)
> +      var mLines = this.localDescription.sdp.split("\r\nm=");
> +      sdputils.checkSdpCLineNotDefault(mLines[anEvent.candidate.sdpMLineIndex+1],

Let's use "mSections" here instead of "mLines"

::: dom/media/tests/mochitest/sdpUtils.js:23
(Diff revision 7)
> +  info("CLINE-ND-SDP: " + JSON.stringify(sdpStr));

"CLINE-ND-SDP" is hard to understand when seeing it in a logfile.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.h:127
(Diff revision 7)
> -                                        const std::string& defaultRtcpCandidateAddr,
> +                                          const std::string& defaultRtcpCandidateAddr,

Let's make sure this stuff wraps to 80 columns.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.h:274
(Diff revision 7)
> +  mozilla::Sdp* GetLocalSdp() const;
> +  mozilla::Sdp* GetRemoteSdp() const;

Maybe call these GetParsedLocal/RemoteDescription in order to distinguish better from GetLocal/RemoteDescription?

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:2184
(Diff revision 7)
> -                                      uint16_t defaultCandidatePort,
> +                                    uint16_t defaultCandidatePort,
> -                                      const std::string& defaultRtcpCandidateAddr,
> +                                    const std::string& defaultRtcpCandidateAddr,
> -                                      uint16_t defaultRtcpCandidatePort,
> +                                    uint16_t defaultRtcpCandidatePort,
> -                                      uint16_t level)
> +                                    uint16_t level)

Update indent, but make sure it doesn't go over 80 columns.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:2234
(Diff revision 7)
> +JsepSessionImpl::EndOfLocalCandidates(uint16_t level)
> +{
> +  mLastError.clear();
> +
> +  mozilla::Sdp* sdp = GetLocalSdp();
> +
> +  if (!sdp) {
> +    JSEP_SET_ERROR("Cannot add ICE candidate in state " << GetStateStr(mState));

Update logging here.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:2249
(Diff revision 7)
> +  // If offer/answer isn't done, it is too early to tell whether these defaults
> +  // need to be applied to other m-sections.

Update comment.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:2315
(Diff revision 7)
> +  mozilla::Sdp* sdp = 0;

These could be made slightly more terse without the temporary variable. Also, convention for null pointers is to use "nullptr".

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.h:463
(Diff revision 7)
> +  void GetDefaultCandidates(const NrIceMediaStream& aStream,
> +                            NrIceCandidate* candidate,
> +                            NrIceCandidate* rtcpCandidate);

Let's prefix all of these args with an 'a' for consistency.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.h:471
(Diff revision 7)
> -  void OnCandidateFound_m(const std::string &candidate, uint16_t aMLine);
> +  void OnCandidateFound_m(const std::string& candidateLine,

Same here too.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:959
(Diff revision 7)
> -                                        const std::string &candidate)
> +                                        const std::string &candidateLine)

'a' for arg prefix here.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:1051
(Diff revision 7)
> -PeerConnectionMedia::OnCandidateFound_m(const std::string &candidate,
> +PeerConnectionMedia::OnCandidateFound_m(const std::string& candidateLine,

'a' for arg prefix here.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:1075
(Diff revision 7)
> -  SignalEndOfLocalCandidates(aDefaultAddr,
> +  ASSERT_ON_THREAD(mMainThread);

Good call.

::: media/webrtc/signaling/src/sdp/SdpHelper.cpp:326
(Diff revision 7)
> +  if (msection.GetAttributeList().HasAttribute(SdpAttribute::kMidAttribute)) {
> +    std::string mid(msection.GetAttributeList().GetMid());
> +    if (bundledMids.count(mid)) {
> +      const SdpMediaSection* masterBundleMsection(bundledMids[mid]);
> +      if (msection.GetLevel() != masterBundleMsection->GetLevel()) {
> +        // Slave bundle m-section. Skip.
> +        return;
> +      }
> +    }
> +  }

This is a good opportunity to break some stuff out into a function that returns an enum describing what kind of m-section this is (returns kMasterBundle, kSlaveBundle, or kNoBundle). I guess it would need to set the master bundle m-section in an outparam too. This would avoid the copied code, and make both SetDefaultAddresses and SetIceGatheringComplete more readable and self-documenting.

::: media/webrtc/signaling/src/sdp/SdpHelper.cpp:390
(Diff revision 7)
>    // TODO(bug 1095793): Will this have an mid someday?

We missed this TODO in bug 1095793, go ahead and nuke it since the question is answered.
Attachment #8672669 - Flags: review?(docfaraday)
Comment on attachment 8672669 [details]
MozReview Request: Bug 1192813 - update the default candidate as new candidates arrive.

Bug 1192813 - update the default candidate as new candidates arrive.
Attachment #8672669 - Flags: review?(docfaraday)
Comment on attachment 8672669 [details]
MozReview Request: Bug 1192813 - update the default candidate as new candidates arrive.

https://reviewboard.mozilla.org/r/21765/#review21595

Looks good to me.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:2242
(Diff revisions 7 - 8)
> -    JSEP_SET_ERROR("Cannot add ICE candidate in state " << GetStateStr(mState));
> +    JSEP_SET_ERROR("Cannot mark end of local ICE candidates in state " << GetStateStr(mState));

Fold to 80 here.
Attachment #8672669 - Flags: review?(docfaraday) → review+
Comment on attachment 8672669 [details]
MozReview Request: Bug 1192813 - update the default candidate as new candidates arrive.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21765/diff/8-9/
Attachment #8672669 - Flags: review+ → review?(docfaraday)
Keywords: checkin-needed
Comment on attachment 8672669 [details]
MozReview Request: Bug 1192813 - update the default candidate as new candidates arrive.

carry r+ forward
Attachment #8672669 - Flags: review?(docfaraday) → review+
https://hg.mozilla.org/mozilla-central/rev/a3acea01e422
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: