Added graph for plotting the audio level from an Rtc event log.

Added graph for plotting the audio level from an Rtc event log.

This uses the audio level values that are stored in the RTP header extension.

BUG=webrtc:4741
Committed: https://crrev.com/aac9d6fb25338ab2e345a1a0c478590757202ae0
Cr-Commit-Position: refs/heads/master@{#14352}

[ivoc, 2016-09-20 11:27:34]

Description was changed from

==========
Added graph for plotting the audio level from an Rtc event log.

This uses the audio level values that are stored in the RTP header extension.

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

to

==========
Added graph for plotting the audio level from an Rtc event log.

This uses the audio level values that are stored in the RTP header extension.

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

[ivoc, 2016-09-20 11:27:35]

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

[ivoc, 2016-09-20 11:42:16]

ivoc@webrtc.org changed reviewers:
+ henrik.lundin@webrtc.org

[ivoc, 2016-09-20 11:42:16]

Hi guys,

Please have a look at this CL to add a new graph for audio level to the
visualization tool. Thanks!

[hlundin-webrtc, 2016-09-21 08:51:14]

LGTM with one comment.

https://codereview.webrtc.org/2346363003/diff/1/webrtc/tools/event_log_visual...
File webrtc/tools/event_log_visualizer/analyzer.cc (right):

https://codereview.webrtc.org/2346363003/diff/1/webrtc/tools/event_log_visual...
webrtc/tools/event_log_visualizer/analyzer.cc:103: // TODO(ivoc): Remove this
once this is stored in the event log.
Do we have a tracking bug for this work? If not, please, create one, and
reference it in this TODO.

[terelius, 2016-09-21 09:44:32]

https://codereview.webrtc.org/2346363003/diff/1/webrtc/tools/event_log_visual...
File webrtc/tools/event_log_visualizer/analyzer.cc (right):

https://codereview.webrtc.org/2346363003/diff/1/webrtc/tools/event_log_visual...
webrtc/tools/event_log_visualizer/analyzer.cc:104: extension_map->Register(
Could you do this for audio streams only please.

https://codereview.webrtc.org/2346363003/diff/1/webrtc/tools/event_log_visual...
webrtc/tools/event_log_visualizer/analyzer.cc:368:
RegisterHeaderExtensions(extensions, &extension_map);
Mark this with a TODO. We don't want to reregister extensions for each packet.

[terelius, 2016-09-21 10:00:15]

https://codereview.webrtc.org/2346363003/diff/1/webrtc/tools/event_log_visual...
File webrtc/tools/event_log_visualizer/analyzer.cc (right):

https://codereview.webrtc.org/2346363003/diff/1/webrtc/tools/event_log_visual...
webrtc/tools/event_log_visualizer/analyzer.cc:629: const
std::vector<LoggedRtpPacket>& packet_stream = kv.second;
In the future, you should only search the packets if the stream has been
configured for audio. Please add a TODO.

[ivoc, 2016-09-21 11:37:46]

Patchset #2 (id:20001) has been deleted

[ivoc, 2016-09-21 11:48:55]

ivoc@webrtc.org changed reviewers:
- kjellander@webrtc.org

[ivoc, 2016-09-21 11:50:05]

PTAL. Removed kjellander as reviewer, because I didn't realize that terelius is
also an owner of webrtc/tools/event_log_visualizer.

https://codereview.webrtc.org/2346363003/diff/1/webrtc/tools/event_log_visual...
File webrtc/tools/event_log_visualizer/analyzer.cc (right):

https://codereview.webrtc.org/2346363003/diff/1/webrtc/tools/event_log_visual...
webrtc/tools/event_log_visualizer/analyzer.cc:103: // TODO(ivoc): Remove this
once this is stored in the event log.
On 2016/09/21 08:51:13, hlundin-webrtc wrote:
> Do we have a tracking bug for this work? If not, please, create one, and
> reference it in this TODO.

I have a CL up to include this information in the event log:
https://codereview.webrtc.org/2353543003/
However, we will probably want to wait for a while after that lands before we
remove these default mappings, since there are currently a lot of event logs in
the wild that don't have the configs in them. I made a tracking bug here:
https://bugs.chromium.org/p/webrtc/issues/detail?id=6399

https://codereview.webrtc.org/2346363003/diff/1/webrtc/tools/event_log_visual...
webrtc/tools/event_log_visualizer/analyzer.cc:104: extension_map->Register(
On 2016/09/21 09:44:32, terelius wrote:
> Could you do this for audio streams only please.

I moved this to a seperate function, which returns a default
RtpHeaderExtensionMap, which is only used for streams without other header
extensions (so presumably audio streams).

https://codereview.webrtc.org/2346363003/diff/1/webrtc/tools/event_log_visual...
webrtc/tools/event_log_visualizer/analyzer.cc:368:
RegisterHeaderExtensions(extensions, &extension_map);
On 2016/09/21 09:44:32, terelius wrote:
> Mark this with a TODO. We don't want to reregister extensions for each packet.

Good point, I moved the creation of the extension map outside of the packet
loop, so it can be reused for all packets without config, and I sprinkled some
TODOs around.

https://codereview.webrtc.org/2346363003/diff/1/webrtc/tools/event_log_visual...
webrtc/tools/event_log_visualizer/analyzer.cc:629: const
std::vector<LoggedRtpPacket>& packet_stream = kv.second;
On 2016/09/21 10:00:14, terelius wrote:
> In the future, you should only search the packets if the stream has been
> configured for audio. Please add a TODO.

Good point, added.

[terelius, 2016-09-21 12:35:56]

Thanks.

LGTM

[ivoc, 2016-09-21 14:01:57]

The CQ bit was checked by ivoc@webrtc.org

[ivoc, 2016-09-21 14:01:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
henrik.lundin@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2346363003/#ps40001 (title:
"Addressed comments by hlundin and terelius.")

[commit-bot: I haz the power, 2016-09-21 14:02:00]

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-09-21 16:02:41]

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

[commit-bot: I haz the power, 2016-09-21 16:02:42]

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

[kjellander_webrtc, 2016-09-21 17:45:24]

kjellander@webrtc.org changed reviewers:
+ kjellander@webrtc.org

[kjellander_webrtc, 2016-09-21 17:45:25]

I don't see a single unit test in
https://cs.chromium.org/chromium/src/third_party/webrtc/tools/event_log_visua...
- why is that?

[ivoc, 2016-09-22 11:32:14]

On 2016/09/21 17:45:25, kjellander_webrtc wrote:
> I don't see a single unit test in
>
https://cs.chromium.org/chromium/src/third_party/webrtc/tools/event_log_visua...
> - why is that?

That's a good question, but since this is the first time I'm contributing to
tools/event_log_visualizer, I'm probably not the best person to answer. 
I think it would be challenging to write good tests for the event log
visualizer, since it's hard to automatically check if generated graphs are
correct, do you have any ideas for how to do that? I'm also not entirely
convinced that unit tests are really necessary here, since we already have
plenty of unittests for the event log itself in webrtc/call, and this is only a
debug tool to analyze those, WDYT?

[kjellander_webrtc, 2016-09-22 13:34:14]

On 2016/09/22 11:32:14, ivoc wrote:
> On 2016/09/21 17:45:25, kjellander_webrtc wrote:
> > I don't see a single unit test in
> >
>
https://cs.chromium.org/chromium/src/third_party/webrtc/tools/event_log_visua...
> > - why is that?
> 
> That's a good question, but since this is the first time I'm contributing to
> tools/event_log_visualizer, I'm probably not the best person to answer. 
> I think it would be challenging to write good tests for the event log
> visualizer, since it's hard to automatically check if generated graphs are
> correct, do you have any ideas for how to do that? I'm also not entirely
> convinced that unit tests are really necessary here, since we already have
> plenty of unittests for the event log itself in webrtc/call, and this is only
a
> debug tool to analyze those, WDYT?

I guess that makes sense. I was mostly wondering since it's quite a lot of code
in there, and at least analyzer.cc could probably benefit from a few unit tests
to ensure intended behavior and prevent breakages.

[kjellander_webrtc, 2016-09-22 13:34:31]

lgtm

[ivoc, 2016-09-22 13:51:35]

The CQ bit was checked by ivoc@webrtc.org

[commit-bot: I haz the power, 2016-09-22 13:51:38]

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-09-22 14:01:50]

Description was changed from

==========
Added graph for plotting the audio level from an Rtc event log.

This uses the audio level values that are stored in the RTP header extension.

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

to

==========
Added graph for plotting the audio level from an Rtc event log.

This uses the audio level values that are stored in the RTP header extension.

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

[commit-bot: I haz the power, 2016-09-22 14:01:51]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2016-09-22 14:01:59]

Description was changed from

==========
Added graph for plotting the audio level from an Rtc event log.

This uses the audio level values that are stored in the RTP header extension.

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

to

==========
Added graph for plotting the audio level from an Rtc event log.

This uses the audio level values that are stored in the RTP header extension.

BUG=webrtc:4741

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

[commit-bot: I haz the power, 2016-09-22 14:02:00]

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