Fixing some issues with payload type mappings.

Fixing some issues with payload type mappings.

This fixes a couple major issues.

#1: If the payload type that an RTX codec refers to has been reassigned, and then the RTX codec is added in a subsequent offer, it refers to the wrong payload type.

#2: If we receive an offer with two payload types referring to the same codec (which we support), our answer contains both (instead of just one), which causes issues down the road since the video engine only supports one payload type per codec.

BUG=webrtc:5450, webrtc:5499
R=pthatcher@webrtc.org

Committed: https://crrev.com/6ec641b0ee9bb1a5d941050221964ac865ba3d2c
Cr-Commit-Position: refs/heads/master@{#11880}

[Taylor Brandstetter, 2016-01-21 21:15:05]

deadbeef@webrtc.org changed reviewers:
+ honghaiz@webrtc.org, pthatcher@webrtc.org

[Taylor Brandstetter, 2016-01-21 21:15:05]

Honghai: If you're not familiar with how payload types work, feel free to skip
reviewing this one.

https://codereview.webrtc.org/1616033002/diff/1/talk/session/media/mediasessi...
File talk/session/media/mediasession.cc (left):

https://codereview.webrtc.org/1616033002/diff/1/talk/session/media/mediasessi...
talk/session/media/mediasession.cc:830: if (IsRtxCodec(negotiated)) {
This logic is now contained inside of FindMatchingCodec.

https://codereview.webrtc.org/1616033002/diff/1/talk/session/media/mediasessi...
File talk/session/media/mediasession.cc (right):

https://codereview.webrtc.org/1616033002/diff/1/talk/session/media/mediasessi...
talk/session/media/mediasession.cc:868: LOG(LS_INFO) << "RTX associated codecs
don't match.";
I changed this from LS_WARNING to LS_INFO, since it can occur in normal
situations.

https://codereview.webrtc.org/1616033002/diff/1/talk/session/media/mediasessi...
File talk/session/media/mediasession_unittest.cc (right):

https://codereview.webrtc.org/1616033002/diff/1/talk/session/media/mediasessi...
talk/session/media/mediasession_unittest.cc:1825: // to add another.
Question for Peter: I'm assuming this is allowed, right? The code allowed this
before in initial offers, but not reoffers. For example, say you have codec 1,
codec 2, RTX for codec 1, RTX for codec 2.

[Taylor Brandstetter, 2016-02-20 02:35:26]

Just FYI, Peter: I need to do work in this exact area to make
MediaSessionDescriptionFactory support unified plan. So it would be convenient
to get this CL landed soon, to avoid dealing with messy merge conflicts later.

[Taylor Brandstetter, 2016-02-20 02:43:39]

Description was changed from

==========
Fixing some issues with RTX payload type association.

The main issue occurred when an RTX codec is added when the payload
type it refers to has already been reassigned.

BUG=webrtc:5450
==========

to

==========
Fixing some issues with payload type negotiation.

This fixes a couple major issues.

#1: If the payload type that an RTX codec refers to has been reassigned, and
then the RTX codec is added in a subsequent offer, it refers to the wrong
payload type.

#2: If we receive an offer with two payload types referring to the same codec
(which we support), our answer contains both (instead of just one), which causes
issues down the road since the video engine only supports one payload type per
codec. 

BUG=webrtc:5450,webrtc:5499
==========

[Taylor Brandstetter, 2016-02-20 02:44:21]

Description was changed from

==========
Fixing some issues with payload type negotiation.

This fixes a couple major issues.

#1: If the payload type that an RTX codec refers to has been reassigned, and
then the RTX codec is added in a subsequent offer, it refers to the wrong
payload type.

#2: If we receive an offer with two payload types referring to the same codec
(which we support), our answer contains both (instead of just one), which causes
issues down the road since the video engine only supports one payload type per
codec. 

BUG=webrtc:5450,webrtc:5499
==========

to

==========
Fixing some issues with payload type mappings.

This fixes a couple major issues.

#1: If the payload type that an RTX codec refers to has been reassigned, and
then the RTX codec is added in a subsequent offer, it refers to the wrong
payload type.

#2: If we receive an offer with two payload types referring to the same codec
(which we support), our answer contains both (instead of just one), which causes
issues down the road since the video engine only supports one payload type per
codec. 

BUG=webrtc:5450,webrtc:5499
==========

[Taylor Brandstetter, 2016-02-20 02:45:35]

On 2016/02/20 02:35:26, Taylor Brandstetter wrote:
> Just FYI, Peter: I need to do work in this exact area to make
> MediaSessionDescriptionFactory support unified plan. So it would be convenient
> to get this CL landed soon, to avoid dealing with messy merge conflicts later.

Oh, and I just realized, I accidentally fixed bug 5499 in this CL (the thing
where Firefox offers two VP8 payload types, and we answer with both). So all the
more reason to get it landed. I just need to add a unit test for that specific
scenario.

[Taylor Brandstetter, 2016-02-25 17:05:39]

deadbeef@webrtc.org changed reviewers:
- honghaiz@webrtc.org

[Taylor Brandstetter, 2016-02-25 17:05:40]

Peter: after thinking about this more, I think we should land this CL as it is,
and start enforcing the "only offer codecs in the remote description" rule in a
separate CL, for a few reasons:

1. This bug fix still applies for very weird corner cases.
2. It's possible that after enforcing the "only offer codecs in the remote
description" rule, we get pushback and realize the rule should be changed.
3. Enforcing said rule is going to require passing the remote description into
the session description factory, so it's not a trivial change.

[pthatcher1, 2016-02-26 21:30:14]

lgtm

https://codereview.webrtc.org/1616033002/diff/20001/talk/session/media/medias...
File talk/session/media/mediasession.cc (right):

https://codereview.webrtc.org/1616033002/diff/20001/talk/session/media/medias...
talk/session/media/mediasession.cc:868: LOG(LS_INFO) << "RTX associated codecs
don't match.";
Since this is FindMatchingCodec, LS_INFO here seems a bit strong.  Would
LS_VERBOSE make more sense?

[Taylor Brandstetter, 2016-03-04 16:12:24]

https://codereview.webrtc.org/1616033002/diff/20001/talk/session/media/medias...
File talk/session/media/mediasession.cc (right):

https://codereview.webrtc.org/1616033002/diff/20001/talk/session/media/medias...
talk/session/media/mediasession.cc:868: LOG(LS_INFO) << "RTX associated codecs
don't match.";
On 2016/02/26 21:30:14, pthatcher1 wrote:
> Since this is FindMatchingCodec, LS_INFO here seems a bit strong.  Would
> LS_VERBOSE make more sense?

It was strong even before (in fact I changed it from WARNING to INFO). But I
agree it happens often enough that it's annoying. If you have N RTX codecs and
each has a match, you get N*(N-1) log messages. In fact, this is just a basic
step in the algorithm so I don't know why we log anything in the first place.

[commit-bot: I haz the power, 2016-03-05 00:48:10]

Description was changed from

==========
Fixing some issues with payload type mappings.

This fixes a couple major issues.

#1: If the payload type that an RTX codec refers to has been reassigned, and
then the RTX codec is added in a subsequent offer, it refers to the wrong
payload type.

#2: If we receive an offer with two payload types referring to the same codec
(which we support), our answer contains both (instead of just one), which causes
issues down the road since the video engine only supports one payload type per
codec. 

BUG=webrtc:5450,webrtc:5499
==========

to

==========
Fixing some issues with payload type mappings.

This fixes a couple major issues.

#1: If the payload type that an RTX codec refers to has been reassigned, and
then the RTX codec is added in a subsequent offer, it refers to the wrong
payload type.

#2: If we receive an offer with two payload types referring to the same codec
(which we support), our answer contains both (instead of just one), which causes
issues down the road since the video engine only supports one payload type per
codec.

BUG=webrtc:5450,webrtc:5499
R=pthatcher@webrtc.org

Committed: https://crrev.com/6ec641b0ee9bb1a5d941050221964ac865ba3d2c
Cr-Commit-Position: refs/heads/master@{#11880}
==========

[Taylor Brandstetter, 2016-03-05 00:48:10]

Description was changed from

==========
Fixing some issues with payload type mappings.

This fixes a couple major issues.

#1: If the payload type that an RTX codec refers to has been reassigned, and
then the RTX codec is added in a subsequent offer, it refers to the wrong
payload type.

#2: If we receive an offer with two payload types referring to the same codec
(which we support), our answer contains both (instead of just one), which causes
issues down the road since the video engine only supports one payload type per
codec.

BUG=webrtc:5450,webrtc:5499
R=pthatcher@webrtc.org

Committed: https://crrev.com/6ec641b0ee9bb1a5d941050221964ac865ba3d2c
Cr-Commit-Position: refs/heads/master@{#11880}
==========

to

==========
Fixing some issues with payload type mappings.

This fixes a couple major issues.

#1: If the payload type that an RTX codec refers to has been reassigned, and
then the RTX codec is added in a subsequent offer, it refers to the wrong
payload type.

#2: If we receive an offer with two payload types referring to the same codec
(which we support), our answer contains both (instead of just one), which causes
issues down the road since the video engine only supports one payload type per
codec.

BUG=webrtc:5450,webrtc:5499
R=pthatcher@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/6ec641b0ee9bb1a5d94105022...
==========

[commit-bot: I haz the power, 2016-03-05 00:48:10]

Patchset 3 (id:??) landed as
https://crrev.com/6ec641b0ee9bb1a5d941050221964ac865ba3d2c
Cr-Commit-Position: refs/heads/master@{#11880}

[Taylor Brandstetter, 2016-03-05 00:48:11]

Committed patchset #3 (id:40001) manually as
6ec641b0ee9bb1a5d941050221964ac865ba3d2c (presubmit successful).