- Filter data channel codecs based on codec name instead of payload type, which may have been remap…

- Filter data channel codecs based on codec name instead of payload type, which may have been remapped.
- Rename the data codec payload types to end with "PlType" instead of "Id", for consistency.

BUG=webrtc:2795
Committed: https://crrev.com/9fa49759e5e51a45cbc7c03a17c269e3d2658fc8
Cr-Commit-Position: refs/heads/master@{#14581}

[the sun, 2016-10-07 11:52:58]

solenberg@webrtc.org changed reviewers:
+ deadbeef@webrtc.org, ossu@webrtc.org

[ossu, 2016-10-07 12:11:26]

https://codereview.webrtc.org/2397413002/diff/20001/webrtc/media/base/rtpdata...
File webrtc/media/base/rtpdataengine.cc (right):

https://codereview.webrtc.org/2397413002/diff/20001/webrtc/media/base/rtpdata...
webrtc/media/base/rtpdataengine.cc:87: DataCodec
data_codec(kGoogleRtpDataCodecPlType, kGoogleRtpDataCodecName);
Does this not need to be changed if we want to avoid using the hard-coded
payload types for the data codec stuff?

[Taylor Brandstetter, 2016-10-07 17:35:53]

Won't this need to happen in two stages? First make sure we filter based on
name. Then once applications have time to update, change the default payload
type. What happens if a new application generating "109" talks to an old one
expecting "101"?

https://codereview.webrtc.org/2397413002/diff/20001/webrtc/api/webrtcsdp.cc
File webrtc/api/webrtcsdp.cc (right):

https://codereview.webrtc.org/2397413002/diff/20001/webrtc/api/webrtcsdp.cc#n...
webrtc/api/webrtcsdp.cc:1263: if (_stricmp(it->name.c_str(),
cricket::kGoogleSctpDataCodecName) == 0
nit: Use CodecNamesEq here and elsewhere

[the sun, 2016-10-07 21:23:54]

On 2016/10/07 17:35:53, Taylor Brandstetter wrote:
> Won't this need to happen in two stages? First make sure we filter based on
> name. Then once applications have time to update, change the default payload
> type. What happens if a new application generating "109" talks to an old one
> expecting "101"?

Are you saying SDP isn't used to negotiate codec mappings for data channel?

> https://codereview.webrtc.org/2397413002/diff/20001/webrtc/api/webrtcsdp.cc
> File webrtc/api/webrtcsdp.cc (right):
> 
>
https://codereview.webrtc.org/2397413002/diff/20001/webrtc/api/webrtcsdp.cc#n...
> webrtc/api/webrtcsdp.cc:1263: if (_stricmp(it->name.c_str(),
> cricket::kGoogleSctpDataCodecName) == 0
> nit: Use CodecNamesEq here and elsewhere

[Taylor Brandstetter, 2016-10-07 21:43:23]

On 2016/10/07 21:23:54, the sun wrote:
> On 2016/10/07 17:35:53, Taylor Brandstetter wrote:
> > Won't this need to happen in two stages? First make sure we filter based on
> > name. Then once applications have time to update, change the default payload
> > type. What happens if a new application generating "109" talks to an old one
> > expecting "101"?
> 
> Are you saying SDP isn't used to negotiate codec mappings for data channel?

It is (for the RTP data channels). But I'm realizing now that since RTP data
channels aren't officially supported anymore, meaning people need to generate
their own SDP to use them, this is fine. They can generate SDP using the old
payload type.

So, lgtm.

[the sun, 2016-10-08 18:52:52]

The CQ bit was checked by solenberg@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-08 18:52:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[the sun, 2016-10-08 18:55:06]

https://codereview.webrtc.org/2397413002/diff/20001/webrtc/media/base/rtpdata...
File webrtc/media/base/rtpdataengine.cc (right):

https://codereview.webrtc.org/2397413002/diff/20001/webrtc/media/base/rtpdata...
webrtc/media/base/rtpdataengine.cc:87: DataCodec
data_codec(kGoogleRtpDataCodecPlType, kGoogleRtpDataCodecName);
On 2016/10/07 12:11:26, ossu wrote:
> Does this not need to be changed if we want to avoid using the hard-coded
> payload types for the data codec stuff?

Not really. Look at Codec::Matches() - it'll compare only on name, if PT is >
95, which it is here.

[commit-bot: I haz the power, 2016-10-08 19:29:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-08 19:29:40]

Dry run: This issue passed the CQ dry run.

[the sun, 2016-10-08 19:34:30]

The CQ bit was checked by solenberg@webrtc.org

[the sun, 2016-10-08 19:34:30]

The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2397413002/#ps40001 (title:
"reviewer comments")

[commit-bot: I haz the power, 2016-10-08 19:34:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-10-08 20:02:49]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-08 20:03:00]

Description was changed from

==========
- Filter data channel codecs based on codec name instead of payload type, which
may have been remapped.
- Rename the data codec payload types to end with "PlType" instead of "Id", for
consistency.

BUG=webrtc:2795
==========

to

==========
- Filter data channel codecs based on codec name instead of payload type, which
may have been remapped.
- Rename the data codec payload types to end with "PlType" instead of "Id", for
consistency.

BUG=webrtc:2795

Committed: https://crrev.com/9fa49759e5e51a45cbc7c03a17c269e3d2658fc8
Cr-Commit-Position: refs/heads/master@{#14581}
==========

[commit-bot: I haz the power, 2016-10-08 20:03:01]

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