Add BWE plot to event log analyzer.

Add BWE plot to event log analyzer.

The plot is constructed by actually running the congestion controller with
the logged rtp headers and rtcp feedback messages to reproduce the same behavior
as in the real call.

R=phoglund@webrtc.org, terelius@webrtc.org

Committed: https://crrev.com/2beea2a8c920000ef19eea20cce397507fc3d5e7
Cr-Commit-Position: refs/heads/master@{#13558}

[stefan-webrtc, 2016-07-28 09:46:19]

stefan@webrtc.org changed reviewers:
+ terelius@webrtc.org

[stefan-webrtc, 2016-07-28 09:47:08]

stefan@webrtc.org changed reviewers:
+ phoglund@webrtc.org

[stefan-webrtc, 2016-07-28 09:47:08]

+phoglund as owner for webrtc/tools/ and webrtc/test/

[phoglund, 2016-07-28 10:21:42]

rs lgtm

https://codereview.webrtc.org/2188033004/diff/1/webrtc/tools/BUILD.gn
File webrtc/tools/BUILD.gn (right):

https://codereview.webrtc.org/2188033004/diff/1/webrtc/tools/BUILD.gn#newcode201
webrtc/tools/BUILD.gn:201: #"../test:field_trial",
Nit: remove

https://codereview.webrtc.org/2188033004/diff/1/webrtc/tools/tools.gyp
File webrtc/tools/tools.gyp (right):

https://codereview.webrtc.org/2188033004/diff/1/webrtc/tools/tools.gyp#newcod...
webrtc/tools/tools.gyp:116: '<(webrtc_root)/test/test.gyp:field_trial',
This appears to be not needed in the gn file?

[terelius, 2016-07-28 13:22:35]

Nice!

https://codereview.webrtc.org/2188033004/diff/1/webrtc/call/rtc_event_log.h
File webrtc/call/rtc_event_log.h (right):

https://codereview.webrtc.org/2188033004/diff/1/webrtc/call/rtc_event_log.h#n...
webrtc/call/rtc_event_log.h:112: class NullRtcEventLog : public RtcEventLog {
There is a null-implementation in rtc_event_log.cc. Maybe that should be moved
to the header file if an empty implementation needs to be exposed?

https://codereview.webrtc.org/2188033004/diff/1/webrtc/test/fuzzers/congestio...
File webrtc/test/fuzzers/congestion_controller_feedback_fuzzer.cc (right):

https://codereview.webrtc.org/2188033004/diff/1/webrtc/test/fuzzers/congestio...
webrtc/test/fuzzers/congestion_controller_feedback_fuzzer.cc:35: NullRtcEventLog
event_log;
Would the MockRtcEventLog be useful here?

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

https://codereview.webrtc.org/2188033004/diff/1/webrtc/tools/event_log_visual...
webrtc/tools/event_log_visualizer/analyzer.cc:785:
*static_cast<rtcp::TransportFeedback*>(rtcp.packet.get()));
Is it safe to cast an RtcpPacket to a TransportFeedback?

https://codereview.webrtc.org/2188033004/diff/1/webrtc/tools/event_log_visual...
webrtc/tools/event_log_visualizer/analyzer.cc:802: if (cc.TimeUntilNextProcess()
<= 0)
Process is called at most once per packet in this code. I think something like
the following would be cleaner and more accurately simulate the real call.

auto NextRtpTime = [&]() {
  if (rtp_iterator != outgoing_rtp.end())
    return rtp_iterator->first;
  return std::numeric_limits<uint64_t>::max();
}

auto NextRtcpTime = [&]() {
  if (rtcp_iterator != incoming_rtcp.end())
    return rtcp_iterator->first;
  return std::numeric_limits<uint64_t>::max();
}

auto NextProcessTime = [&]() {
  if (rtcp_iterator != incoming_rtcp.end() ||
      rtp_iterator != outgoing_rtp.end())
    return clock->TimeinMicroseconds() +
               std::max(cc.TimeUntilNextProcess() * 1000, 0);
  return std::numeric_limits<uint64_t>::max();
}

time = std::min(NextRtpTime(), NextRtcpTime());
while (time != std::numeric_limits<uint64_t>::max()) {
  clock.AdvanceTimeMicroseconds(time - clock.TimeInMicroseconds());
  if (clock.TimeInMicroseconds() >= NextRtpTime()) {
    // Send RTP packet
  }
  if (clock.TimeInMicroseconds() >= NextRtcpTime()) {
    // Receive RTCP feedback
  }
  if (clock.TimeInMicroseconds() >= NextProcessTime()) {
    cc.Process()
  }
  time = std::min({NextRtpTime(),
                   NextRtcpTime(),
                   NextProcessTime()});
}

[stefan-webrtc, 2016-07-28 13:47:05]

Comments addressed.

[stefan-webrtc, 2016-07-28 13:47:23]

https://codereview.webrtc.org/2188033004/diff/1/webrtc/call/rtc_event_log.h
File webrtc/call/rtc_event_log.h (right):

https://codereview.webrtc.org/2188033004/diff/1/webrtc/call/rtc_event_log.h#n...
webrtc/call/rtc_event_log.h:112: class NullRtcEventLog : public RtcEventLog {
On 2016/07/28 13:22:35, terelius wrote:
> There is a null-implementation in rtc_event_log.cc. Maybe that should be moved
> to the header file if an empty implementation needs to be exposed?

Done.

https://codereview.webrtc.org/2188033004/diff/1/webrtc/test/fuzzers/congestio...
File webrtc/test/fuzzers/congestion_controller_feedback_fuzzer.cc (right):

https://codereview.webrtc.org/2188033004/diff/1/webrtc/test/fuzzers/congestio...
webrtc/test/fuzzers/congestion_controller_feedback_fuzzer.cc:35: NullRtcEventLog
event_log;
On 2016/07/28 13:22:35, terelius wrote:
> Would the MockRtcEventLog be useful here?

Fuzzers can't depend on gmock/gtest, that's why I'm using null implementations
instead.

https://codereview.webrtc.org/2188033004/diff/1/webrtc/tools/BUILD.gn
File webrtc/tools/BUILD.gn (right):

https://codereview.webrtc.org/2188033004/diff/1/webrtc/tools/BUILD.gn#newcode201
webrtc/tools/BUILD.gn:201: #"../test:field_trial",
On 2016/07/28 10:21:41, phoglund wrote:
> Nit: remove

Needed, thanks though.

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

https://codereview.webrtc.org/2188033004/diff/1/webrtc/tools/event_log_visual...
webrtc/tools/event_log_visualizer/analyzer.cc:785:
*static_cast<rtcp::TransportFeedback*>(rtcp.packet.get()));
On 2016/07/28 13:22:35, terelius wrote:
> Is it safe to cast an RtcpPacket to a TransportFeedback?

Yes, assuming the type is checked first (see line 783). This is how we're using
RtcpPacket in other places already.

https://codereview.webrtc.org/2188033004/diff/1/webrtc/tools/event_log_visual...
webrtc/tools/event_log_visualizer/analyzer.cc:802: if (cc.TimeUntilNextProcess()
<= 0)
On 2016/07/28 13:22:35, terelius wrote:
> Process is called at most once per packet in this code. I think something like
> the following would be cleaner and more accurately simulate the real call.
> 
> auto NextRtpTime = [&]() {
>   if (rtp_iterator != outgoing_rtp.end())
>     return rtp_iterator->first;
>   return std::numeric_limits<uint64_t>::max();
> }
> 
> auto NextRtcpTime = [&]() {
>   if (rtcp_iterator != incoming_rtcp.end())
>     return rtcp_iterator->first;
>   return std::numeric_limits<uint64_t>::max();
> }
> 
> auto NextProcessTime = [&]() {
>   if (rtcp_iterator != incoming_rtcp.end() ||
>       rtp_iterator != outgoing_rtp.end())
>     return clock->TimeinMicroseconds() +
>                std::max(cc.TimeUntilNextProcess() * 1000, 0);
>   return std::numeric_limits<uint64_t>::max();
> }
> 
> time = std::min(NextRtpTime(), NextRtcpTime());
> while (time != std::numeric_limits<uint64_t>::max()) {
>   clock.AdvanceTimeMicroseconds(time - clock.TimeInMicroseconds());
>   if (clock.TimeInMicroseconds() >= NextRtpTime()) {
>     // Send RTP packet
>   }
>   if (clock.TimeInMicroseconds() >= NextRtcpTime()) {
>     // Receive RTCP feedback
>   }
>   if (clock.TimeInMicroseconds() >= NextProcessTime()) {
>     cc.Process()
>   }
>   time = std::min({NextRtpTime(),
>                    NextRtcpTime(),
>                    NextProcessTime()});
> }

Agree, that's nicer, I'll do exactly like you suggest. :)

https://codereview.webrtc.org/2188033004/diff/1/webrtc/tools/tools.gyp
File webrtc/tools/tools.gyp (right):

https://codereview.webrtc.org/2188033004/diff/1/webrtc/tools/tools.gyp#newcod...
webrtc/tools/tools.gyp:116: '<(webrtc_root)/test/test.gyp:field_trial',
On 2016/07/28 10:21:41, phoglund wrote:
> This appears to be not needed in the gn file?

It is needed for gn.

[stefan-webrtc, 2016-07-28 13:49:03]

.

[stefan-webrtc, 2016-07-28 13:49:27]

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

[commit-bot: I haz the power, 2016-07-28 13:49:34]

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-07-28 14:01:44]

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

[commit-bot: I haz the power, 2016-07-28 14:01:45]

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

[stefan-webrtc, 2016-07-28 14:15:38]

Set a default bitrate.

[terelius, 2016-07-28 14:28:33]

lgtm

[stefan-webrtc, 2016-07-28 14:33:03]

The CQ bit was checked by stefan@webrtc.org

[stefan-webrtc, 2016-07-28 14:33:03]

The patchset sent to the CQ was uploaded after l-g-t-m from phoglund@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2188033004/#ps60001 (title:
"Set a default bitrate.")

[commit-bot: I haz the power, 2016-07-28 14:33: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-07-28 14:38:43]

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

[commit-bot: I haz the power, 2016-07-28 14:38:44]

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

[stefan-webrtc, 2016-07-28 15:38:22]

Add includes.

[stefan-webrtc, 2016-07-28 15:40:24]

.

[commit-bot: I haz the power, 2016-07-28 15:42:35]

Description was changed from

==========
Add BWE plot to event log analyzer.

The plot is constructed by actually running the congestion controller with
the logged rtp headers and rtcp feedback messages to reproduce the same behavior
as in the real call.
==========

to

==========
Add BWE plot to event log analyzer.

The plot is constructed by actually running the congestion controller with
the logged rtp headers and rtcp feedback messages to reproduce the same behavior
as in the real call.

R=phoglund@webrtc.org, terelius@webrtc.org

Committed: https://crrev.com/2beea2a8c920000ef19eea20cce397507fc3d5e7
Cr-Commit-Position: refs/heads/master@{#13558}
==========

[stefan-webrtc, 2016-07-28 15:42:36]

Description was changed from

==========
Add BWE plot to event log analyzer.

The plot is constructed by actually running the congestion controller with
the logged rtp headers and rtcp feedback messages to reproduce the same behavior
as in the real call.

R=phoglund@webrtc.org, terelius@webrtc.org

Committed: https://crrev.com/2beea2a8c920000ef19eea20cce397507fc3d5e7
Cr-Commit-Position: refs/heads/master@{#13558}
==========

to

==========
Add BWE plot to event log analyzer.

The plot is constructed by actually running the congestion controller with
the logged rtp headers and rtcp feedback messages to reproduce the same behavior
as in the real call.

R=phoglund@webrtc.org, terelius@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/2beea2a8c920000ef19eea20c...
==========

[commit-bot: I haz the power, 2016-07-28 15:42:37]

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

[stefan-webrtc, 2016-07-28 15:42:38]

Committed patchset #6 (id:100001) manually as
2beea2a8c920000ef19eea20cce397507fc3d5e7 (presubmit successful).

[terelius, 2016-07-28 16:21:14]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.webrtc.org/2190013002/ by terelius@webrtc.org.

The reason for reverting is: Breaks downstream targets..