Expose RtpCodecParameters to VoiceMediaInfo stats.

Expose RtpCodecParameters to VoiceMediaInfo stats.

Payload type -> RtpCodecParameters maps added for sender and receiver.
This is a follow-up to https://codereview.webrtc.org/2484193002/ which
did the same thing for VideoMediaInfo. This information will be used to
produce RTCCodecStats[1].

Voice[Sender/Receiver]Info is updated with current codec payload type
for every stream which can be used to look up the codec in
VoiceMediaInfo.

[1] https://w3c.github.io/webrtc-stats/#codec-dict*

BUG=chromium:659117
Committed: https://crrev.com/1acfbd22cc30bc72af53187f25f3c389792968d3
Cr-Commit-Position: refs/heads/master@{#15144}

[hbos, 2016-11-16 14:00:09]

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

[commit-bot: I haz the power, 2016-11-16 14:00:17]

Dry run: 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-11-16 14:08:18]

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

[commit-bot: I haz the power, 2016-11-16 14:08:19]

Dry run: Try jobs failed on following builders:
  linux_baremetal on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_baremetal/builds/...)

[hbos, 2016-11-16 14:13:13]

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

[commit-bot: I haz the power, 2016-11-16 14:13:19]

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

[hbos, 2016-11-16 14:13:56]

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

[hbos, 2016-11-16 14:13:57]

Please take a look, solenberg and deadbeef.

[commit-bot: I haz the power, 2016-11-16 14:44:09]

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

[commit-bot: I haz the power, 2016-11-16 14:44:09]

Dry run: Try jobs failed on following builders:
  win_baremetal on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_baremetal/builds/16164)

[the sun, 2016-11-16 19:41:07]

Generally looks good - a few questions though.

https://codereview.webrtc.org/2503383002/diff/20001/webrtc/media/base/mediach...
File webrtc/media/base/mediachannel.h (right):

https://codereview.webrtc.org/2503383002/diff/20001/webrtc/media/base/mediach...
webrtc/media/base/mediachannel.h:602: rtc::Optional<uint32_t>
codec_payload_type;
Why uint32_t?
Payload types are 0-127, so if not just using an int, a uint8_t is a closer
match.

https://codereview.webrtc.org/2503383002/diff/20001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/2503383002/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:1412:
RTC_DCHECK_GE(codec_params.payload_type, 0);
Why are you DCHECKing this property here?

If the reason is just because of the cast to unsigned, change the map key type
to int instead.

If you want to make sure the payload type is something sane, you should also
check that it is <= 127 - and then it would be better to move the check into
RtpCodecParameters (making the field const, and adding DCHECks in the ctor) so
we don't have to remember DCHECKing in all places where we use the field.

https://codereview.webrtc.org/2503383002/diff/20001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/2503383002/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:2541:
RTC_DCHECK_GE(codec_params.payload_type, 0);
Same comment as for video engine - can we avoid the DCHECKs and casting here?

[Taylor Brandstetter, 2016-11-16 21:37:44]

https://codereview.webrtc.org/2503383002/diff/20001/webrtc/media/base/mediach...
File webrtc/media/base/mediachannel.h (right):

https://codereview.webrtc.org/2503383002/diff/20001/webrtc/media/base/mediach...
webrtc/media/base/mediachannel.h:602: rtc::Optional<uint32_t>
codec_payload_type;
On 2016/11/16 19:41:07, the sun wrote:
> Why uint32_t?
> Payload types are 0-127, so if not just using an int, a uint8_t is a closer
> match.

I agree; I'd just use an int here, and use RTC_DCHECKs to assert that values are
in the 0-127 range, wherever necessary. Ultimately it needs to turn into an
"unsigned long", according to the W3C spec, but that can happen further up.

[hbos, 2016-11-17 14:01:56]

Patchset #3 (id:40001) has been deleted

[hbos, 2016-11-17 14:05:19]

Patchset #4 (id:80001) has been deleted

[hbos, 2016-11-17 14:05:24]

Patchset #3 (id:60001) has been deleted

[hbos, 2016-11-17 14:08:10]

Patchset #3 (id:100001) has been deleted

[hbos, 2016-11-17 14:09:29]

Patchset #3 (id:120001) has been deleted

[hbos, 2016-11-17 14:09:53]

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

[commit-bot: I haz the power, 2016-11-17 14:10:03]

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

[hbos, 2016-11-17 14:11:37]

Patchset #3 (id:140001) has been deleted

[hbos, 2016-11-17 14:14:48]

PTAL solenberg and deadbeef.

https://codereview.webrtc.org/2503383002/diff/20001/webrtc/media/base/mediach...
File webrtc/media/base/mediachannel.h (right):

https://codereview.webrtc.org/2503383002/diff/20001/webrtc/media/base/mediach...
webrtc/media/base/mediachannel.h:602: rtc::Optional<uint32_t>
codec_payload_type;
On 2016/11/16 21:37:44, Taylor Brandstetter wrote:
> On 2016/11/16 19:41:07, the sun wrote:
> > Why uint32_t?
> > Payload types are 0-127, so if not just using an int, a uint8_t is a closer
> > match.
> 
> I agree; I'd just use an int here, and use RTC_DCHECKs to assert that values
are
> in the 0-127 range, wherever necessary. Ultimately it needs to turn into an
> "unsigned long", according to the W3C spec, but that can happen further up.

Done (using int). I picked uint32_t because that is the equivalent of "unsigned
long" in WebIDL, but I can restrict to that type when creating that stat.

https://codereview.webrtc.org/2503383002/diff/20001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/2503383002/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:1412:
RTC_DCHECK_GE(codec_params.payload_type, 0);
On 2016/11/16 19:41:07, the sun wrote:
> Why are you DCHECKing this property here?
> 
> If the reason is just because of the cast to unsigned, change the map key type
> to int instead.
> 
> If you want to make sure the payload type is something sane, you should also
> check that it is <= 127 - and then it would be better to move the check into
> RtpCodecParameters (making the field const, and adding DCHECks in the ctor) so
> we don't have to remember DCHECKing in all places where we use the field.

Since RtpCodecParameter and the data structure it gets its value from all use
int, I'll make this code use int too.

Modifying it to DHCECK in its constructor is a good idea but just making its
members const removes the copy assignment operator and breaks code using
std::vector<RtpCodecParameter>. I don't want to do a bunch of refactoring in
this CL, so I'm just using int, this CL does not depend on the range of the
value anymore.

https://codereview.webrtc.org/2503383002/diff/20001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/2503383002/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:2541:
RTC_DCHECK_GE(codec_params.payload_type, 0);
On 2016/11/16 19:41:07, the sun wrote:
> Same comment as for video engine - can we avoid the DCHECKs and casting here?

Done.

[hbos, 2016-11-17 14:15:34]

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

[commit-bot: I haz the power, 2016-11-17 14:15:42]

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-11-17 14:18:30]

lgtm

[hbos, 2016-11-17 14:24:14]

Cool, PTAL deadbeef.

[commit-bot: I haz the power, 2016-11-17 14:35:43]

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

[commit-bot: I haz the power, 2016-11-17 14:35:44]

Dry run: This issue passed the CQ dry run.

[Taylor Brandstetter, 2016-11-17 20:21:25]

lgtm

[hbos, 2016-11-18 07:42:06]

The CQ bit was checked by hbos@webrtc.org

[commit-bot: I haz the power, 2016-11-18 07:42:13]

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-11-18 07:43:34]

Committed patchset #3 (id:160001)

[commit-bot: I haz the power, 2016-11-18 07:43:46]

Description was changed from

==========
Expose RtpCodecParameters to VoiceMediaInfo stats.

Payload type -> RtpCodecParameters maps added for sender and receiver.
This is a follow-up to https://codereview.webrtc.org/2484193002/ which
did the same thing for VideoMediaInfo. This information will be used to
produce RTCCodecStats[1].

Voice[Sender/Receiver]Info is updated with current codec payload type
for every stream which can be used to look up the codec in
VoiceMediaInfo.

[1] https://w3c.github.io/webrtc-stats/#codec-dict*

BUG=chromium:659117
==========

to

==========
Expose RtpCodecParameters to VoiceMediaInfo stats.

Payload type -> RtpCodecParameters maps added for sender and receiver.
This is a follow-up to https://codereview.webrtc.org/2484193002/ which
did the same thing for VideoMediaInfo. This information will be used to
produce RTCCodecStats[1].

Voice[Sender/Receiver]Info is updated with current codec payload type
for every stream which can be used to look up the codec in
VoiceMediaInfo.

[1] https://w3c.github.io/webrtc-stats/#codec-dict*

BUG=chromium:659117

Committed: https://crrev.com/1acfbd22cc30bc72af53187f25f3c389792968d3
Cr-Commit-Position: refs/heads/master@{#15144}
==========

[commit-bot: I haz the power, 2016-11-18 07:43:47]

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