Added logging for audio send/receive stream configs.

Added logging for audio send/receive stream configs.

BUG=webrtc:4741, webrtc:6399
Committed: https://crrev.com/e0928d8002cb7d25f314c54c8d6ce20ab859aa4d
Cr-Commit-Position: refs/heads/master@{#14585}

[ivoc, 2016-09-20 14:18:28]

Description was changed from

==========
Added logging for audio send/receive stream configs.

BUG=webrtc:4741
==========

to

==========
Added logging for audio send/receive stream configs.

BUG=webrtc:4741
==========

[ivoc, 2016-09-20 14:18:28]

ivoc@webrtc.org changed reviewers:
+ kjellander@webrtc.org, solenberg@webrtc.org, terelius@webrtc.org

[ivoc, 2016-09-21 09:01:12]

Hi guys, please have a look at this CL to include configs for audio send/receive
streams in the event log. This also includes code to parse the configs.
kjellander: Please have a look at the changes in webrtc/tools.

[terelius, 2016-09-21 10:11:26]

This is great!

Could you add a unit test that generates a single "random"
AudioSendStreamConfig, writes it to file, parses the file and checks that it
gets the same config back? Same for AudioReceiveStream, VideoSendStream and
VideoReceiveStream.

https://codereview.webrtc.org/2353543003/diff/20001/webrtc/call/rtc_event_log...
File webrtc/call/rtc_event_log_parser.cc (right):

https://codereview.webrtc.org/2353543003/diff/20001/webrtc/call/rtc_event_log...
webrtc/call/rtc_event_log_parser.cc:417: config->rtp.ssrc =
sender_config.ssrc();
RTC_CHECK(sender_config.has_ssrc())

[ivoc, 2016-09-21 11:32:28]

Description was changed from

==========
Added logging for audio send/receive stream configs.

BUG=webrtc:4741
==========

to

==========
Added logging for audio send/receive stream configs.

BUG=webrtc:4741,webrtc:6399
==========

[the sun, 2016-09-22 06:51:02]

lgtm

https://codereview.webrtc.org/2353543003/diff/20001/webrtc/call/rtc_event_log...
File webrtc/call/rtc_event_log_parser.cc (right):

https://codereview.webrtc.org/2353543003/diff/20001/webrtc/call/rtc_event_log...
webrtc/call/rtc_event_log_parser.cc:420: for (int i = 0; i <
sender_config.header_extensions_size(); i++) {
Can we make a utility function out of this loop, or abstract it some other way?

[kjellander_webrtc, 2016-09-22 06:59:11]

lgtm, although I don't have much knowledge about this code...

[ivoc, 2016-09-22 09:20:56]

I added tests for logging and parsing randomly generated audio and video
configs. PTAL.

https://codereview.webrtc.org/2353543003/diff/20001/webrtc/call/rtc_event_log...
File webrtc/call/rtc_event_log_parser.cc (right):

https://codereview.webrtc.org/2353543003/diff/20001/webrtc/call/rtc_event_log...
webrtc/call/rtc_event_log_parser.cc:417: config->rtp.ssrc =
sender_config.ssrc();
On 2016/09/21 10:11:26, terelius wrote:
> RTC_CHECK(sender_config.has_ssrc())

Good catch, added.

https://codereview.webrtc.org/2353543003/diff/20001/webrtc/call/rtc_event_log...
webrtc/call/rtc_event_log_parser.cc:420: for (int i = 0; i <
sender_config.header_extensions_size(); i++) {
On 2016/09/22 06:51:02, the sun wrote:
> Can we make a utility function out of this loop, or abstract it some other
way?

It's not as simple as it seems, because the type of both |sender_config| and
|config| are different in each of the functions where this loop appears. I guess
a function with two template parameters could work, but I'm not sure if it's
worth the added complexity, WDYT?

[the sun, 2016-09-22 19:22:05]

https://codereview.webrtc.org/2353543003/diff/20001/webrtc/call/rtc_event_log...
File webrtc/call/rtc_event_log_parser.cc (right):

https://codereview.webrtc.org/2353543003/diff/20001/webrtc/call/rtc_event_log...
webrtc/call/rtc_event_log_parser.cc:420: for (int i = 0; i <
sender_config.header_extensions_size(); i++) {
On 2016/09/22 09:20:55, ivoc wrote:
> On 2016/09/22 06:51:02, the sun wrote:
> > Can we make a utility function out of this loop, or abstract it some other
> way?
> 
> It's not as simple as it seems, because the type of both |sender_config| and
> |config| are different in each of the functions where this loop appears. I
guess
> a function with two template parameters could work, but I'm not sure if it's
> worth the added complexity, WDYT?

All the stream config objects store the extensions in a
std::vector<RtpExtension> so that type should need no templating. The protobuf
side would, unless you can do something with the result from e.g. this method in
rtc_event_log.pb.h:

  const ::google::protobuf::RepeatedPtrField<
::webrtc::rtclog::RtpHeaderExtension >&
      header_extensions() const;

?
(I haven't used the C++ proto API in a while so I'm not sure it's useful.)

[ivoc, 2016-09-23 12:40:27]

https://codereview.webrtc.org/2353543003/diff/20001/webrtc/call/rtc_event_log...
File webrtc/call/rtc_event_log_parser.cc (right):

https://codereview.webrtc.org/2353543003/diff/20001/webrtc/call/rtc_event_log...
webrtc/call/rtc_event_log_parser.cc:420: for (int i = 0; i <
sender_config.header_extensions_size(); i++) {
On 2016/09/22 19:22:05, the sun wrote:
> On 2016/09/22 09:20:55, ivoc wrote:
> > On 2016/09/22 06:51:02, the sun wrote:
> > > Can we make a utility function out of this loop, or abstract it some other
> > way?
> > 
> > It's not as simple as it seems, because the type of both |sender_config| and
> > |config| are different in each of the functions where this loop appears. I
> guess
> > a function with two template parameters could work, but I'm not sure if it's
> > worth the added complexity, WDYT?
> 
> All the stream config objects store the extensions in a
> std::vector<RtpExtension> so that type should need no templating. The protobuf
> side would, unless you can do something with the result from e.g. this method
in
> rtc_event_log.pb.h:
> 
>   const ::google::protobuf::RepeatedPtrField<
> ::webrtc::rtclog::RtpHeaderExtension >&
>       header_extensions() const;
> 
> ?
> (I haven't used the C++ proto API in a while so I'm not sure it's useful.)

Thanks for the suggestion, that's a really good idea. Done.

[terelius, 2016-09-23 15:14:02]

LGTM with one nit:

https://codereview.webrtc.org/2353543003/diff/60001/webrtc/call/rtc_event_log...
File webrtc/call/rtc_event_log_parser.cc (right):

https://codereview.webrtc.org/2353543003/diff/60001/webrtc/call/rtc_event_log...
webrtc/call/rtc_event_log_parser.cc:327: // Set header extensions.
I'd prefer to call it Get or Read to match the rest of the function names in the
file.

[ivoc, 2016-10-03 08:04:36]

The CQ bit was checked by ivoc@webrtc.org

[ivoc, 2016-10-03 08:04:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
solenberg@webrtc.org, kjellander@webrtc.org, terelius@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2353543003/#ps80001 (title:
"Renamed SetHeaderExtensions to GetHeaderExtensions.")

[commit-bot: I haz the power, 2016-10-03 08:04:42]

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-03 08:43:45]

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

[commit-bot: I haz the power, 2016-10-03 08:43:46]

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

[ivoc, 2016-10-03 09:35:34]

The CQ bit was checked by ivoc@webrtc.org

[commit-bot: I haz the power, 2016-10-03 09:35:43]

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-03 10:05:15]

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

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

Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[ivoc, 2016-10-04 15:05:20]

The CQ bit was checked by ivoc@webrtc.org

[commit-bot: I haz the power, 2016-10-04 15:05:30]

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-04 15:06:39]

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

[commit-bot: I haz the power, 2016-10-04 15:06:41]

Try jobs failed on following builders:
  ios64_gyp_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gyp_dbg/builds/1152)
  ios64_sim_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/11243)
  ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/13547)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/18799)
  ios_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/17468)
  mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)

[ivoc, 2016-10-04 15:49:10]

The CQ bit was checked by ivoc@webrtc.org

[ivoc, 2016-10-04 15:49:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
solenberg@webrtc.org, terelius@webrtc.org, kjellander@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2353543003/#ps100001 (title:
"Rebase.")

[commit-bot: I haz the power, 2016-10-04 15:49:14]

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-04 17:49:38]

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

[commit-bot: I haz the power, 2016-10-04 17:49:39]

Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[ivoc, 2016-10-10 11:15:23]

The CQ bit was checked by ivoc@webrtc.org

[ivoc, 2016-10-10 11:15:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
solenberg@webrtc.org, terelius@webrtc.org, kjellander@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2353543003/#ps120001 (title:
"Another rebase.")

[commit-bot: I haz the power, 2016-10-10 11:15:35]

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-10 12:12:54]

Description was changed from

==========
Added logging for audio send/receive stream configs.

BUG=webrtc:4741,webrtc:6399
==========

to

==========
Added logging for audio send/receive stream configs.

BUG=webrtc:4741,webrtc:6399
==========

[commit-bot: I haz the power, 2016-10-10 12:12:55]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-10-10 12:13:03]

Description was changed from

==========
Added logging for audio send/receive stream configs.

BUG=webrtc:4741,webrtc:6399
==========

to

==========
Added logging for audio send/receive stream configs.

BUG=webrtc:4741,webrtc:6399

Committed: https://crrev.com/e0928d8002cb7d25f314c54c8d6ce20ab859aa4d
Cr-Commit-Position: refs/heads/master@{#14585}
==========

[commit-bot: I haz the power, 2016-10-10 12:13:05]

Patchset 7 (id:??) landed as
https://crrev.com/e0928d8002cb7d25f314c54c8d6ce20ab859aa4d
Cr-Commit-Position: refs/heads/master@{#14585}