Closed Bug 1095793 Opened 5 years ago Closed 4 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed
Blocking Flags:

People

(Reporter: bwc, Assigned: mjf)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

No description provided.
Blocks: webrtc_spec
backlog: --- → webRTC+
Rank: 45
Priority: -- → P4
Assignee: nobody → mfroman
Bug 1095793 - use mid if provided to place candidate in msection.
Attachment #8659393 - Flags: review?(docfaraday)
Attachment #8659393 - Flags: review?(docfaraday)
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 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.
Attachment #8659393 - Flags: review?(docfaraday)
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 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.
Attachment #8659393 - Flags: review?(docfaraday) → review+
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.
Attachment #8659393 - Flags: review+ → review?(docfaraday)
Comment on attachment 8659393 [details]
MozReview Request: Bug 1095793 - use mid if provided to place candidate in msection.

carry forward r=bwc
Attachment #8659393 - Flags: review?(docfaraday) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/372b1c30dc09
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.