1095793 - JsepSessionImpl should be able to use mid on candidates to find which m-section they should be placed in

Status: RESOLVED FIXED

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

Comment 1

[Michael Froman [:mjf]]

Attachment: MozReview Request: Bug 1095793 - use mid if provided to place candidate in msection.

Bug 1095793 - use mid if provided to place candidate in msection.

Comment 2

[Byron Campen [:bwc]]

Comment on attachment 8659393 [details]
MozReview Request: Bug 1095793 - use mid if provided to place candidate in msection.

https://reviewboard.mozilla.org/r/18809/#review16831

I can give this another look once there's some test-code (we need to at least verify is that the consistency checking fails when it is supposed to, which should be an easy test).

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:2397
(Diff revision 1)
> +  // If we're given an mid, check to make sure it matches what we'd get by
> +  // looking up the m= line using the level. (mjf)
> +  if (!mid.empty()) {
> +    std::string checkMid;
> +    nsresult rv = mSdpHelper.GetMidFromLevel(*sdp, level, &checkMid);
> +    if (NS_FAILED(rv)) {
> +      return rv;
> +    }
> +    if (mid != checkMid) {
> +      JSEP_SET_ERROR("Mismatched given mid (" << mid << ") with GetMidFromLevel (" << checkMid << ")");
> +      return NS_ERROR_UNEXPECTED;
> +    }
> +  }
> +

I wonder if this checking code should go in SdpHelper::AddCandidateToSdp? This could also help us find bugs in the local candidate handling code.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:2406
(Diff revision 1)
> +      JSEP_SET_ERROR("Mismatched given mid (" << mid << ") with GetMidFromLevel (" << checkMid << ")");

Fold to 80 columns. Also, given that these strings are targeted at the web developer, we might want to rephrase to state more clearly what went wrong. (eg; "foo" is not the mid for level 0; "bar" is)

::: media/webrtc/signaling/test/jsep_session_unittest.cpp:652
(Diff revision 1)
> -          session.AddRemoteIceCandidate(levelAndCandidate.second,
> +          Level level; Mid mid; Candidate candidate;

We typically break these up onto their own lines.

Comment 3

[Michael Froman [:mjf]]

Comment on attachment 8659393 [details]
MozReview Request: Bug 1095793 - use mid if provided to place candidate in msection.

Bug 1095793 - use mid if provided to place candidate in msection.

Comment 4

[Michael Froman [:mjf]]

Comment on attachment 8659393 [details]
MozReview Request: Bug 1095793 - use mid if provided to place candidate in msection.

Bug 1095793 - use mid if provided to place candidate in msection.

Comment 5

[Byron Campen [:bwc]]

Comment on attachment 8659393 [details]
MozReview Request: Bug 1095793 - use mid if provided to place candidate in msection.

https://reviewboard.mozilla.org/r/18809/#review17257

Just one change I'd like before landing.

::: dom/media/tests/mochitest/test_peerConnection_addIceCandidate.html:85
(Diff revision 3)
> +            is(err.name, "InvalidStateError", "Error is InvalidStateError");

I think this should be InvalidCandidateError instead.

::: media/webrtc/signaling/src/sdp/SdpHelper.cpp:293
(Diff revision 3)
> +      return NS_ERROR_UNEXPECTED;

Let's make this NS_ERROR_INVALID_ARG, so we get InvalidCandidateError out the other side.

Comment 6

[Michael Froman [:mjf]]

Comment on attachment 8659393 [details]
MozReview Request: Bug 1095793 - use mid if provided to place candidate in msection.

Bug 1095793 - use mid if provided to place candidate in msection.

Comment 7

[Michael Froman [:mjf]]

Comment on attachment 8659393 [details]
MozReview Request: Bug 1095793 - use mid if provided to place candidate in msection.

carry forward r=bwc

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/372b1c30dc09

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/372b1c30dc09