1193495 - JsepSessionImpl can create reoffers with duplicate payload types in some situations

Status: RESOLVED FIXED

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

Description

[Byron Campen [:bwc]]

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.

Comment 1

[Byron Campen [:bwc]]

It so happens that fixing this is going to mesh well with implementing simulcast, so I'm bumping the priority.

Comment 2

[Byron Campen [:bwc]]

Attachment: MozReview Request: Bug 1193495 - Part 1: Test case. r=mt

Bug 1193495 - Part 1: Test case.

Comment 3

[Byron Campen [:bwc]]

Attachment: 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.

Comment 4

[Martin Thomson [:mt:]]

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.

Comment 5

[Martin Thomson [:mt:]]

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 6

[Martin Thomson [: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

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.

Comment 7

[Byron Campen [:bwc]]

Comment on attachment 8648230 [details]
MozReview Request: Bug 1193495 - Part 1: Test case. r=mt

Bug 1193495 - Part 1: Test case. r=mt

Comment 8

[Byron Campen [:bwc]]

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

Comment 9

[Byron Campen [:bwc]]

Check back on try.

Comment 10

[Byron Campen [:bwc]]

Comment on attachment 8648230 [details]
MozReview Request: Bug 1193495 - Part 1: Test case. r=mt

Bug 1193495 - Part 1: Test case. r=mt

Comment 11

[Byron Campen [:bwc]]

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

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2d2b71f2c970
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a585b9dd1db

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2d2b71f2c970
https://hg.mozilla.org/mozilla-central/rev/8a585b9dd1db