Measure bandwidth for chromoting video channel

Measure bandwidth for chromoting video channel

Define RunningAverage, TimedRunningAverage and use that to record
video bandwidth.

This doesn't account for overhead of protobuf envelop. However the number
should be small that can be ignored.

BUG=None
TEST=None

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=80486

[simonmorris, 2011-03-25 09:24:59]

http://codereview.chromium.org/6736009/diff/1/remoting/base/running_average.cc
File remoting/base/running_average.cc (right):

http://codereview.chromium.org/6736009/diff/1/remoting/base/running_average.c...
remoting/base/running_average.cc:69: return 1000.0 * average /
delta.InMilliseconds();
I'm not sure the math is right here. For a rate, don't you
want total_bytes/total_time, not
average_bytes_per_packet/total_time? And if data_points[i]
is the number of bytes delivered between time[i-1] and
time[i], shouldn't the sum of data_points[i] (0 <= i < n)
be compared to time[n-1] - time[-1]? Maybe one class would
be simpler than two here.

http://codereview.chromium.org/6736009/diff/1/remoting/base/running_average.h
File remoting/base/running_average.h (right):

http://codereview.chromium.org/6736009/diff/1/remoting/base/running_average.h...
remoting/base/running_average.h:11: // TimedRunningAverage keeps track of
running average in a timed faction.
"Fashion" instead of "faction"?

http://codereview.chromium.org/6736009/diff/1/remoting/base/running_average.h...
remoting/base/running_average.h:30: // Construct a running average counter for a
specific window size. The most
"|windows_size| most"

http://codereview.chromium.org/6736009/diff/1/remoting/base/running_average.h...
remoting/base/running_average.h:62: TimedRunningAverage(int window_size);
Is it better for the window to be a length of time, instead
of a number of samples? For example, consider the case where
big buffers mean that many packets are delivered at the same
time.

[Wez, 2011-03-25 10:43:19]

Drive-by...

Sorry, I'm not clear on what this change is intended to achieve, other than two
allow us to display a rough estimate of the bandwidth used in the last
half-second or so.

We previously discussed having a display of the total bytes used this session,
for which we need a running total; would it make more sense to implement that
and have the bandwidth estimate hang off it?

http://codereview.chromium.org/6736009/diff/1/remoting/base/running_average.cc
File remoting/base/running_average.cc (right):

http://codereview.chromium.org/6736009/diff/1/remoting/base/running_average.c...
remoting/base/running_average.cc:69: return 1000.0 * average /
delta.InMilliseconds();
On 2011/03/25 09:24:59, simonmorris wrote:
> I'm not sure the math is right here. For a rate, don't you
> want total_bytes/total_time, not
> average_bytes_per_packet/total_time? And if data_points[i]
> is the number of bytes delivered between time[i-1] and
> time[i], shouldn't the sum of data_points[i] (0 <= i < n)
> be compared to time[n-1] - time[-1]? Maybe one class would
> be simpler than two here.

Yes.  The rate (bandwidth in our case) would normally just be bytes_in_interval
/ second_in_interval.

It's also conventional to have exponential drop-off for bandwidth estimates of
this sort, so that if you happen to sample close to a sudden spike or trough in
traffic you don't get misleading values.

http://codereview.chromium.org/6736009/diff/1/remoting/client/chromoting_stat...
File remoting/client/chromoting_stats.cc (right):

http://codereview.chromium.org/6736009/diff/1/remoting/client/chromoting_stat...
remoting/client/chromoting_stats.cc:10: static const int kBandwidthWindow = 10;
Why size the bandwidth sampling window in terms of frames rather than time
elapsed?

[Alpha Left Google, 2011-03-28 16:29:02]

On 2011/03/25 10:43:19, Wez wrote:
> Drive-by...
> 
> Sorry, I'm not clear on what this change is intended to achieve, other than
two
> allow us to display a rough estimate of the bandwidth used in the last
> half-second or so.

Yes the intention is display the most recent bandwidth consumption to be used
for tuning codec. I'd like to see how bandwidth varies on different screen
content, then i can better target the tuning.

Looks like the math I used here is incorrect, I'll come up with a new patch.
Thanks guys.

> 
> We previously discussed having a display of the total bytes used this session,
> for which we need a running total; would it make more sense to implement that
> and have the bandwidth estimate hang off it?
> 
> http://codereview.chromium.org/6736009/diff/1/remoting/base/running_average.cc
> File remoting/base/running_average.cc (right):
> 
>
http://codereview.chromium.org/6736009/diff/1/remoting/base/running_average.c...
> remoting/base/running_average.cc:69: return 1000.0 * average /
> delta.InMilliseconds();
> On 2011/03/25 09:24:59, simonmorris wrote:
> > I'm not sure the math is right here. For a rate, don't you
> > want total_bytes/total_time, not
> > average_bytes_per_packet/total_time? And if data_points[i]
> > is the number of bytes delivered between time[i-1] and
> > time[i], shouldn't the sum of data_points[i] (0 <= i < n)
> > be compared to time[n-1] - time[-1]? Maybe one class would
> > be simpler than two here.
> 
> Yes.  The rate (bandwidth in our case) would normally just be
bytes_in_interval
> / second_in_interval.
> 
> It's also conventional to have exponential drop-off for bandwidth estimates of
> this sort, so that if you happen to sample close to a sudden spike or trough
in
> traffic you don't get misleading values.
> 
>
http://codereview.chromium.org/6736009/diff/1/remoting/client/chromoting_stat...
> File remoting/client/chromoting_stats.cc (right):
> 
>
http://codereview.chromium.org/6736009/diff/1/remoting/client/chromoting_stat...
> remoting/client/chromoting_stats.cc:10: static const int kBandwidthWindow =
10;
> Why size the bandwidth sampling window in terms of frames rather than time
> elapsed?

[Alpha Left Google, 2011-03-28 18:47:23]

I've added RateCounter that does sum of value / time window.

Also verified with iftop so the bandwidth measurement is pretty accurate.

I still need RunningAverage to smooth out readings of latencies so i'm keeping
this class.

[Alpha Left Google, 2011-04-01 19:40:26]

ping. please review this patch.

[simonmorris, 2011-04-04 10:27:34]

Sorry for the slow review.

http://codereview.chromium.org/6736009/diff/8001/remoting/base/rate_counter.cc
File remoting/base/rate_counter.cc (right):

http://codereview.chromium.org/6736009/diff/8001/remoting/base/rate_counter.c...
remoting/base/rate_counter.cc:10: : time_window_(time_window) {
Initialise sum_.

http://codereview.chromium.org/6736009/diff/8001/remoting/base/rate_counter.c...
remoting/base/rate_counter.cc:16: void RateCounter::Record(int64 value) {
It might be worth adding a Record(int64, base::Time) and a
Rate(base::Time), so this class can be unit tested.

http://codereview.chromium.org/6736009/diff/8001/remoting/base/rate_counter.c...
remoting/base/rate_counter.cc:29: return 1.0 *
base::Time::kMillisecondsPerSecond * sum_ /
Clearer to explicitly cast to a double.

http://codereview.chromium.org/6736009/diff/8001/remoting/base/rate_counter.c...
remoting/base/rate_counter.cc:30: time_window_.InMilliseconds();
What if the first data point was taken more recently than
time_window_ ago? Maybe it's OK to give inaccurate rates
during the first time_window_, but if so, it should be
documented.

http://codereview.chromium.org/6736009/diff/8001/remoting/base/rate_counter.c...
remoting/base/rate_counter.cc:40: if (window_start < data_points_.front().first)
Clearer to reverse the order, so the non-const is first,
and the const second?

http://codereview.chromium.org/6736009/diff/8001/remoting/base/running_average.h
File remoting/base/running_average.h (right):

http://codereview.chromium.org/6736009/diff/8001/remoting/base/running_averag...
remoting/base/running_average.h:7: // these data come in as a stream and
fluctuates a lot. They are processed by
"this...comes...fluctuates" or "these...come...fluctuate".

http://codereview.chromium.org/6736009/diff/8001/remoting/base/running_averag...
remoting/base/running_average.h:27: // |windows_size| recent values are kept and
the average is reported.
"|windows_size| most recent", not "most |windows_size| recent".

http://codereview.chromium.org/6736009/diff/8001/remoting/base/running_averag...
remoting/base/running_average.h:36: int64 Average();
Is int64 better than the expected double?

[Alpha Left Google, 2011-04-05 00:16:06]

http://codereview.chromium.org/6736009/diff/8001/remoting/base/rate_counter.cc
File remoting/base/rate_counter.cc (right):

http://codereview.chromium.org/6736009/diff/8001/remoting/base/rate_counter.c...
remoting/base/rate_counter.cc:10: : time_window_(time_window) {
On 2011/04/04 10:27:34, simonmorris wrote:
> Initialise sum_.

Done.

http://codereview.chromium.org/6736009/diff/8001/remoting/base/rate_counter.c...
remoting/base/rate_counter.cc:16: void RateCounter::Record(int64 value) {
On 2011/04/04 10:27:34, simonmorris wrote:
> It might be worth adding a Record(int64, base::Time) and a
> Rate(base::Time), so this class can be unit tested.

How is this related to unit testing? I mean this interface can be unit tested
too, is it?

http://codereview.chromium.org/6736009/diff/8001/remoting/base/rate_counter.c...
remoting/base/rate_counter.cc:29: return 1.0 *
base::Time::kMillisecondsPerSecond * sum_ /
On 2011/04/04 10:27:34, simonmorris wrote:
> Clearer to explicitly cast to a double.

Done.

http://codereview.chromium.org/6736009/diff/8001/remoting/base/rate_counter.c...
remoting/base/rate_counter.cc:30: time_window_.InMilliseconds();
On 2011/04/04 10:27:34, simonmorris wrote:
> What if the first data point was taken more recently than
> time_window_ ago? Maybe it's OK to give inaccurate rates
> during the first time_window_, but if so, it should be
> documented.

Done.

http://codereview.chromium.org/6736009/diff/8001/remoting/base/rate_counter.c...
remoting/base/rate_counter.cc:40: if (window_start < data_points_.front().first)
On 2011/04/04 10:27:34, simonmorris wrote:
> Clearer to reverse the order, so the non-const is first,
> and the const second?

Done.

http://codereview.chromium.org/6736009/diff/8001/remoting/base/running_average.h
File remoting/base/running_average.h (right):

http://codereview.chromium.org/6736009/diff/8001/remoting/base/running_averag...
remoting/base/running_average.h:7: // these data come in as a stream and
fluctuates a lot. They are processed by
On 2011/04/04 10:27:34, simonmorris wrote:
> "this...comes...fluctuates" or "these...come...fluctuate".

Done.

http://codereview.chromium.org/6736009/diff/8001/remoting/base/running_averag...
remoting/base/running_average.h:7: // these data come in as a stream and
fluctuates a lot. They are processed by
On 2011/04/04 10:27:34, simonmorris wrote:
> "this...comes...fluctuates" or "these...come...fluctuate".

Done.

http://codereview.chromium.org/6736009/diff/8001/remoting/base/running_averag...
remoting/base/running_average.h:27: // |windows_size| recent values are kept and
the average is reported.
On 2011/04/04 10:27:34, simonmorris wrote:
> "|windows_size| most recent", not "most |windows_size| recent".

Done.

http://codereview.chromium.org/6736009/diff/8001/remoting/base/running_averag...
remoting/base/running_average.h:36: int64 Average();
On 2011/04/04 10:27:34, simonmorris wrote:
> Is int64 better than the expected double?

In a later patch this is changed to double.

[simonmorris, 2011-04-05 10:58:10]

On 2011/04/05 00:16:06, Alpha wrote:
> On 2011/04/04 10:27:34, simonmorris wrote:
> > It might be worth adding a Record(int64, base::Time) and a
> > Rate(base::Time), so this class can be unit tested.
> 
> How is this related to unit testing? I mean this interface can
> be unit tested too, is it?
> 

To unit test this class, you'd need to sleep between calls to
Record() and Rate(), and then hope the unit test doesn't sleep too
long, which some day it will. If you added versions of Record()
and Rate() that take time parameters, and let the existing Record()
and Rate() call those extended versions with Time::now(), then you
could reliably simulate the passing of time in a unit test.
Put another way, I'm suggesting injecting the dependency on time.

That could be a separate CL, though, so LGTM.