Closed Bug 1193495 Opened 6 years ago Closed 6 years ago

JsepSessionImpl can create reoffers with duplicate payload types in some situations

Categories

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

defect

Tracking

()

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

People

(Reporter: bwc, Assigned: bwc)

Details

Attachments

(2 files)

JsepSessionImpl attempts to remember the payload types used by the other side, but does not check whether these clash with the defaults used by other codecs. For example, if we receive an offer that has only payload type 0 which is mapped to opus, and send an answer, our next reoffer will contain both opus and PCMU as payload type 0.

A related bug is that when the other side uses different payload types for the same codec in different m-sections, we will only remember the latest one encountered and apply it across the reoffer, which isn't quite right.
It so happens that fixing this is going to mesh well with implementing simulcast, so I'm bumping the priority.
backlog: --- → webrtc/webaudio+
Rank: 10
Priority: -- → P1
Bug 1193495 - Part 1: Test case.
Attachment #8648230 - Flags: review?(martin.thomson)
Bug 1193495 - Part 2: Maintain clones of supported codecs for each level, and do necessary checking to prevent payload-type clashes.
Attachment #8648231 - Flags: review?(martin.thomson)
Comment on attachment 8648230 [details]
MozReview Request: Bug 1193495 - Part 1: Test case. r=mt

https://reviewboard.mozilla.org/r/16157/#review14459

::: media/webrtc/signaling/test/jsep_session_unittest.cpp:2892
(Diff revision 1)
>           bool sending,

An enumeration for sending (kDirectionSend/kDirectionRecv) would have made this a whole lot easier.

(Yeah, I know...juberti)

::: media/webrtc/signaling/test/jsep_session_unittest.cpp:3558
(Diff revision 1)
> +  SetPayloadTypeNumber(mSessionOff, "opus", "0");
> +  SetCodecEnabled(mSessionOff, "PCMU", false);

Does the first line here mean that two codecs have a PT set to "0"?  It might pay to disable PCMU first.

::: media/webrtc/signaling/test/jsep_session_unittest.cpp:3576
(Diff revision 1)
> +  // Now, make sure that mSessionAns does not put a=rtpmap:0 PCMU in a reoffer,
> +  // since pt 0 is taken for opus

Add a note that the answerer hasn't had PCMU disabled...  I missed that first time.
Attachment #8648230 - Flags: review?(martin.thomson)
Attachment #8648230 - Flags: review+
Comment on attachment 8648230 [details]
MozReview Request: Bug 1193495 - Part 1: Test case. r=mt

https://reviewboard.mozilla.org/r/16157/#review14465

Ship It!
Comment on attachment 8648231 [details]
MozReview Request: Bug 1193495 - Part 2: Maintain clones of supported codecs for each level, and do necessary checking to prevent payload-type clashes. r=mt

https://reviewboard.mozilla.org/r/16159/#review14471

I'll let you use your judgment here, but that new function took me quite some time to grok.  I'm not entirely happy with the amount of cloning of stuff that is going on here, but it looks like it will work.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:703
(Diff revision 1)
> +  if (mCodecsByLevel.size() <= level) {
> +    PopulateCodecsByLevel(level + 1);
> +    return;
> +  }

Comment: 
// If this is a new m-section, populate it with default codecs.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:712
(Diff revision 1)
> +  // First, get the set of all payload types that were negotiated last time,
> +  // and while we're at it, revert any codecs that weren't negotiated back to
> +  // defaults.
> +  for (size_t i = 0; i < mSupportedCodecs.values.size(); ++i) {

I'm thinking that this loop needs to be extracted: nsresult ResetCodecState(mCodecsByLevel[level])

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:731
(Diff revision 1)
> +  // Next, make sure that there are no duplicate payload types by tweaking the
> +  // codecs that were not negotiated last time.
> +  for (JsepCodecDescription* codec : mCodecsByLevel[level]) {

void EnsureUniqueCodecPts(std::vector<Codec*>* codecs, std::set<uint16_t>* pts)

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:755
(Diff revision 1)
> +    for (uint16_t freePt = 0; freePt <= 128; ++freePt) {
> +      // Not super efficient, but readability is probably more important.
> +      if (!payloadTypes.count(freePt)) {
> +        payloadTypes.insert(freePt);
> +        codec->mEnabled = true;
> +        std::ostringstream os;
> +        os << freePt;
> +        codec->mDefaultPt = os.str();
> +        break;
> +      }

uint16_t FindUnusedPt(const std::set<uint16_t>& used)

OR

slightly more aggressive:

```
if (codec->mNegotiated || !codec.mEnabled) {
  continue;
}
uint16_t pt;
nsresult rv = SelectPtForCodec(payloadTypes, &pt));
if (NS_FAILED(rv)) {
  codec->mEnabled = false;
  continue;
}
payloadTypes.insert(pt);
codec->mDefaultPt = ToString(pt);
```

Of course, your code avoids a string conversion in most cases, but I think that's a marginal gain.
Attachment #8648231 - Flags: review?(martin.thomson) → review+
Comment on attachment 8648230 [details]
MozReview Request: Bug 1193495 - Part 1: Test case. r=mt

Bug 1193495 - Part 1: Test case. r=mt
Attachment #8648230 - Attachment description: MozReview Request: Bug 1193495 - Part 1: Test case. → MozReview Request: Bug 1193495 - Part 1: Test case. r=mt
Comment on attachment 8648231 [details]
MozReview Request: Bug 1193495 - Part 2: Maintain clones of supported codecs for each level, and do necessary checking to prevent payload-type clashes. r=mt

Bug 1193495 - Part 2: Maintain clones of supported codecs for each level, and do necessary checking to prevent payload-type clashes. r=mt
Attachment #8648231 - Attachment description: MozReview Request: Bug 1193495 - Part 2: Maintain clones of supported codecs for each level, and do necessary checking to prevent payload-type clashes. → MozReview Request: Bug 1193495 - Part 2: Maintain clones of supported codecs for each level, and do necessary checking to prevent payload-type clashes. r=mt
Check back on try.
Flags: needinfo?(docfaraday)
Comment on attachment 8648230 [details]
MozReview Request: Bug 1193495 - Part 1: Test case. r=mt

Bug 1193495 - Part 1: Test case. r=mt
Comment on attachment 8648231 [details]
MozReview Request: Bug 1193495 - Part 2: Maintain clones of supported codecs for each level, and do necessary checking to prevent payload-type clashes. r=mt

Bug 1193495 - Part 2: Maintain clones of supported codecs for each level, and do necessary checking to prevent payload-type clashes. r=mt
Flags: needinfo?(docfaraday)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2d2b71f2c970
https://hg.mozilla.org/mozilla-central/rev/8a585b9dd1db
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.