More Simulation Framework features

Supports logging for dynamic and histogram plots on Simulation Framework.

---- Dynamic receiving rate.
---- Dynamic packet-loss.
---- Dynamic objective function.
---- Dynamic available capacity.
---- Dynamic available capacity per flow.
---- Average delay Histogram with standard deviation or 5th/95th percentiles.
---- Average bitrate Histogram with error bars.
---- Optimal average bitrate dashed line.
---- Average packet-loss Histogram.
---- Total objective function Histogram.

Added media Pause/Resume methods to Video and TcpSender.
Modified LinkedSet: computing GlobalPacketLossRatio even if packet's sequence_number overflows.
Added small randomization to frame send times, modified bwe_test_framework_unittest accordingly.
Taking offset time into account for plotting.

Added nada_unittests.
Added bwe_unittests.
Added a RateCounter to BweReceiver (replaced ReceivingRate)
Added LossAccount.

Fixed NadaBweReceiver issue: using sender_timestamp instead of creation_time.
Fixed memory leaks.
Fixed int division rounding issues.

Supporting plots on bandwidth Estimators:
Logging received packet information on on SubClassesBweReceiver::ReceivePacket
Updating RateCounter, updating packet loss account and relieving LinkedSet when necessary.

R=stefan@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/9c261f2d13793fbb5a0d07b26bec4154bc38342b

[magalhaesc, 2015-06-24 14:55:54]

Replaced std::valarray with std::vector

[magalhaesc, 2015-06-24 14:58:08]

magalhaesc@google.com changed reviewers:
+ holmer@chromium.org

[magalhaesc, 2015-06-24 15:04:35]

Replaced int->size_t

[magalhaesc, 2015-06-24 16:39:35]

Added media Pause/Resume support, added units to filter's methods

[magalhaesc, 2015-06-24 16:53:44]

Addressing trybot failures

[magalhaesc, 2015-06-24 17:21:41]

Next upcoming CL's and affected files:

- Plot scripts
----- bwe_plot.sh
----- plot_dynamics.py


- Supporting plots on Estimators
----- nada.h
----- nada.cc
----- remb.cc
----- send_side.cc
----- tcp.cc


- Evaluation Test Cases
----- bwe_test.h
----- bwe_test.cc
----- bwe_simulations.cc


// Add the gentler one and an easy way to change it:
- Jitter models 
----- bwe_test_framework.h
----- bwe_test_framework.cc
----- bwe_test.h
----- bwe_test.cc

[magalhaesc, 2015-06-25 09:03:35]

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/bwe.cc (right):

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe.cc:198: size_t
BweReceiver::RecentReceivingRate() {
Since a RateCounter was added, RecentReceivingRate will be removed.

[magalhaesc, 2015-06-25 11:09:16]

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/bwe.cc (right):

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe.cc:29: const int kSetCapacity =
1000;
2 options:

-- Replace 1000 with 1000, then recalculate the 97% value

or

-- Keep 1000, then replace 10000 with 1000 on comment line 133/

[stefan-webrtc, 2015-06-25 14:44:02]

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

[stefan-webrtc, 2015-06-25 14:44:05]

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/bwe.cc (right):

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe.cc:29: const int kSetCapacity =
1000;
On 2015/06/25 11:09:16, magalhaesc wrote:
> 2 options:
> 
> -- Replace 1000 with 1000, then recalculate the 97% value
> 
> or
> 
> -- Keep 1000, then replace 10000 with 1000 on comment line 133/

Right... I would personally prefer if we could have a smaller history here. It
would be doable by having an accumulated loss rate that we update whenever the
history becomes full. Then we can remove an item every time we add an item (when
we have filled the history), and update the global loss rate with the removed
item. WDYT?

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe.cc:121: }
But if this method gets called whenever we remove packets it should all be fine
for the global loss ratio right?

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe.cc:244: }
This method seems overly complicated to me. I would prefer it if we implemented
this using the RateCounter instead.

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/bwe.h (right):

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe.h:71: uint16_t
upper_bound(uint16_t key) {
Isn't it very confusing to call this upper_bound now that it is not the
upper_bound? :)

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe.h:122: float
LinkedSetPacketLossRatio();
It's not clear what this is supposed to be used for. What is different between
this, Global and Recent?

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe.h:122: float
LinkedSetPacketLossRatio();
Should be private I think? Maybe other methods as well?

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe.h:127: }
Do we need both RecentKbps and RecentReceivingRate()?

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe.h:129: void
UpdateLossAccount();
Just UpdateLoss()

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc (right):

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc:135:
capacity_per_flow_kbps_(0) {
As discussed offline, we should find a better way to do this. I currently don't
know exactly why you need it, so I can't yet propose a better way...

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc:315:
plot_kbps = 0;
I prefer to swap this with 313, so you do
plot_kbps = 0;
if (timestamp >= start)
  plot_kbps = rate_counter->...;

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc:320:
(void)plot_kbps;
RTC_UNUSED

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc:482:
available_capacity_kbps_(kDefaultKbps),
Just capacity_kbps_

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc:525:
(*it)->set_capacity_per_flow_kbps(available_capacity_per_flow_kbps);
If we are going to pass the capacity per flow with a packet, we should make sure
to update it only if it's lower than the previously set capacity per flow, so
that we in the end get the capacity available on the bottleneck.

I think that can be useful for plotting.

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc:726:
(void)plot_kbps;
RTC_UNUSED

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc:768:
new_packet->set_sending_estimate_kbps(bits_per_second_ / 1000);
If possible, I'd prefer to not have to set this.

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc:771:
next_frame_ms_ +=
Add a comment saying that this adds a 1 ms uniform variance to the frame period.

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc:780:
SetBitrateBps(0);
I'm not sure it's correct to set to 0 here? I'd suggest leaving it unchanged.

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc:786:
SetBitrateBps(kResumeBitrateBps);
I would actually suspect that we would resume at the same bitrate as before
after a pause in webrtc. Or does the evaluation draft specify that it should be
reset to the initial bitrate?

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.h (right):

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.h:226: //
|send_time_us_|. The simu lation time |time_ms| is optional to use.
simulation

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.h:255: int64_t
start_plotting_ms);
start_plotting_time_ms_

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.h:348: void
ResumeFlow(int flow_id);  // Decreases available capacity per flow.
I don't understand what this does. If this is about pausing flows, shouldn't
that be done on the Source objects?

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.h:442:
std::string alg_name_;
It feels odd that we have to know about an algorithm name in the VideoSource
object. I would like to move this to the BWE objects instead. We may have to
change how things are plotted then somehow... Maybe break out the plotting that
is currently being done in VideoSource to somewhere outside, and call
bits_per_second() from outside to get the bitrate. Additionally, the name of the
algorithm can be queried from the BweSender or something like that.

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/packet_receiver.cc (right):

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/packet_receiver.cc:149: now_ms_ =
std::max(now_ms_, arrival_time_ms);
Break this plotting stuff out to a method.

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/packet_receiver.cc:213: template
<typename T>
All of these methods should go in the beginning and be in under namespace
vector_operation {} I think.

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/packet_receiver.cc:214: inline T
Sum(std::vector<T>& input) {
No need for inline.

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/packet_receiver.cc:298: double
PacketReceiver::ObjectiveFunction() {
Not sure we ended up using this? Maybe better to remove?

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/packet_receiver.cc:372:
(void)optimal_bitrate_per_flow_kbps;
RTC_UNUSED

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/packet_receiver.cc:400:
(void)tenth_sigma_ms;
RTC_UNUSED

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/packet_receiver.h (right):

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/packet_receiver.h:89: int64_t
last_per_flow_capacity_plot_ms_;
We should group the string with the last plot time and the bool. Make a struct

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/packet_sender.cc (right):

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/packet_sender.cc:304:
estimated_kbps_(0),
bitrate_kbps_?

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/packet_sender.cc:440: }
Can we break this out to a method UpdateSendBitrateEstimate()?

[magalhaesc, 2015-07-01 12:48:41]

Comments addressed

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/bwe.cc (right):

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe.cc:29: const int kSetCapacity =
1000;
On 2015/06/25 14:44:03, stefan-webrtc (holmer) wrote:
> On 2015/06/25 11:09:16, magalhaesc wrote:
> > 2 options:
> > 
> > -- Replace 1000 with 1000, then recalculate the 97% value
> > 
> > or
> > 
> > -- Keep 1000, then replace 10000 with 1000 on comment line 133/
> 
> Right... I would personally prefer if we could have a smaller history here. It
> would be doable by having an accumulated loss rate that we update whenever the
> history becomes full. Then we can remove an item every time we add an item
(when
> we have filled the history), and update the global loss rate with the removed
> item. WDYT?

All right, I will keep it 1000. 
What do you think about a implementation in between clearing all packets and the
one dropping just the tail? 
I was thinking about preserving, let's say 10%=100 packets, calculate the packet
loss over the other 900 that will be deleted, and then update loss account

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe.cc:121: }
On 2015/06/25 14:44:03, stefan-webrtc (holmer) wrote:
> But if this method gets called whenever we remove packets it should all be
fine
> for the global loss ratio right?

Yes, all should be fine for the global loss ratio (we were using it to plot loss
histograms)

The UpdateLoss() should anyway be called in GlobalReceiverPacketLossRatio() in
order to compute remaining packets in the LinkedSet that weren't deleted.

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe.cc:244: }
On 2015/06/25 14:44:03, stefan-webrtc (holmer) wrote:
> This method seems overly complicated to me. I would prefer it if we
implemented
> this using the RateCounter instead.

Right, this method will be removed.

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/bwe.h (right):

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe.h:71: uint16_t
upper_bound(uint16_t key) {
On 2015/06/25 14:44:04, stefan-webrtc (holmer) wrote:
> Isn't it very confusing to call this upper_bound now that it is not the
> upper_bound? :)

I agree.

For a set containing values {2, 1, 0, 65535, 65534}, 
"lower_bound" returns 65534
"upper_bound" returns 2

So I renamed those methods "OldestSeqNumber" and "NewestSeqNumber" (not time
based), what do you think?

I also added unittests for them.

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe.h:122: float
LinkedSetPacketLossRatio();
On 2015/06/25 14:44:03, stefan-webrtc (holmer) wrote:
> It's not clear what this is supposed to be used for. What is different between
> this, Global and Recent?

The only difference is the amount of packets considered:

-- RecentPacketLossRatio
Only packets in the latest time window (here = 500ms)
Used to plot packet loss dynamics.

-- LinkedSetPacketLoss
All packets stored in the LinkedSet, up to 10000.
Used to update global loss account whenever the set is filled and cleared.

-- GlobalPacketLoss
Computes the packet loss during an entire simulation, up to 4 billion packets.

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe.h:122: float
LinkedSetPacketLossRatio();
On 2015/06/25 14:44:04, stefan-webrtc (holmer) wrote:
> Should be private I think? Maybe other methods as well?

Yes, some methods are private now.

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe.h:127: }
On 2015/06/25 14:44:03, stefan-webrtc (holmer) wrote:
> Do we need both RecentKbps and RecentReceivingRate()?

No, we don't.
RecentReceivingRate will be removed.

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe.h:129: void
UpdateLossAccount();
On 2015/06/25 14:44:04, stefan-webrtc (holmer) wrote:
> Just UpdateLoss()

Done.

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc (right):

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc:135:
capacity_per_flow_kbps_(0) {
On 2015/06/25 14:44:04, stefan-webrtc (holmer) wrote:
> As discussed offline, we should find a better way to do this. I currently
don't
> know exactly why you need it, so I can't yet propose a better way...

Fields removed from Packet class.

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc:315:
plot_kbps = 0;
On 2015/06/25 14:44:04, stefan-webrtc (holmer) wrote:
> I prefer to swap this with 313, so you do
> plot_kbps = 0;
> if (timestamp >= start)
>   plot_kbps = rate_counter->...;

Done.

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc:320:
(void)plot_kbps;
On 2015/06/25 14:44:04, stefan-webrtc (holmer) wrote:
> RTC_UNUSED

Done.

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc:482:
available_capacity_kbps_(kDefaultKbps),
On 2015/06/25 14:44:04, stefan-webrtc (holmer) wrote:
> Just capacity_kbps_

Done.

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc:525:
(*it)->set_capacity_per_flow_kbps(available_capacity_per_flow_kbps);
On 2015/06/25 14:44:04, stefan-webrtc (holmer) wrote:
> If we are going to pass the capacity per flow with a packet, we should make
sure
> to update it only if it's lower than the previously set capacity per flow, so
> that we in the end get the capacity available on the bottleneck.
> 
> I think that can be useful for plotting.

Right,
actually the capacity_per_flow field was removed from Packet class.
Now the MetricRecorder can directly read it from the ChokeFilter whenever needed
to plot.

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc:726:
(void)plot_kbps;
On 2015/06/25 14:44:04, stefan-webrtc (holmer) wrote:
> RTC_UNUSED

This plot_sending_estimate, which was set false by default, was removed from
VideoSource.

Now the MetricRecorder has access to a method
PacketSender::GetTargetBitrateKbps() and can plot it if we want so.

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc:768:
new_packet->set_sending_estimate_kbps(bits_per_second_ / 1000);
On 2015/06/25 14:44:04, stefan-webrtc (holmer) wrote:
> If possible, I'd prefer to not have to set this.

Removed.

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc:771:
next_frame_ms_ +=
On 2015/06/25 14:44:04, stefan-webrtc (holmer) wrote:
> Add a comment saying that this adds a 1 ms uniform variance to the frame
period.

Done.

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.h (right):

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.h:226: //
|send_time_us_|. The simu lation time |time_ms| is optional to use.
On 2015/06/25 14:44:04, stefan-webrtc (holmer) wrote:
> simulation

Done.

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.h:255: int64_t
start_plotting_ms);
On 2015/06/25 14:44:04, stefan-webrtc (holmer) wrote:
> start_plotting_time_ms_

Done.

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.h:348: void
ResumeFlow(int flow_id);  // Decreases available capacity per flow.
On 2015/06/25 14:44:04, stefan-webrtc (holmer) wrote:
> I don't understand what this does. If this is about pausing flows, shouldn't
> that be done on the Source objects?

Yes, it's done on the Source. It is also done here in order to update the
available capacity per flow, since the ChokeFilter is the one knowing the
available capacity.

Do you think we should add this information (AvailableCapacity,
NumberActiveFlows) elsewhere?

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.h:442:
std::string alg_name_;
On 2015/06/25 14:44:04, stefan-webrtc (holmer) wrote:
> It feels odd that we have to know about an algorithm name in the VideoSource
> object. I would like to move this to the BWE objects instead. We may have to
> change how things are plotted then somehow... Maybe break out the plotting
that
> is currently being done in VideoSource to somewhere outside, and call
> bits_per_second() from outside to get the bitrate. Additionally, the name of
the
> algorithm can be queried from the BweSender or something like that.

I agree, done.

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/packet_receiver.cc (right):

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/packet_receiver.cc:149: now_ms_ =
std::max(now_ms_, arrival_time_ms);
On 2015/06/25 14:44:05, stefan-webrtc (holmer) wrote:
> Break this plotting stuff out to a method.

Done, moved to MetricRecorder

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/packet_receiver.cc:213: template
<typename T>
On 2015/06/25 14:44:05, stefan-webrtc (holmer) wrote:
> All of these methods should go in the beginning and be in under namespace
> vector_operation {} I think. 

Done, moved to MetricReceiver

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/packet_receiver.cc:214: inline T
Sum(std::vector<T>& input) {
On 2015/06/25 14:44:05, stefan-webrtc (holmer) wrote:
> No need for inline.

Done.

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/packet_receiver.cc:298: double
PacketReceiver::ObjectiveFunction() {
On 2015/06/25 14:44:05, stefan-webrtc (holmer) wrote:
> Not sure we ended up using this? Maybe better to remove?

They are now in the MetricRecorder file, so separated from the framework. 
We can remove it or keep it in case we think it could be useful.
WDYT?

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/packet_receiver.cc:372:
(void)optimal_bitrate_per_flow_kbps;
On 2015/06/25 14:44:05, stefan-webrtc (holmer) wrote:
> RTC_UNUSED

Done.

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/packet_receiver.cc:400:
(void)tenth_sigma_ms;
On 2015/06/25 14:44:05, stefan-webrtc (holmer) wrote:
> RTC_UNUSED

Done.

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/packet_receiver.h (right):

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/packet_receiver.h:89: int64_t
last_per_flow_capacity_plot_ms_;
On 2015/06/25 14:44:05, stefan-webrtc (holmer) wrote:
> We should group the string with the last plot time and the bool. Make a struct

Plot time and bool were moved to MetricRecorder, were they will be grouped.

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/packet_sender.cc (right):

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/packet_sender.cc:304:
estimated_kbps_(0),
On 2015/06/25 14:44:05, stefan-webrtc (holmer) wrote:
> bitrate_kbps_?

Done.

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/packet_sender.cc:440: }
On 2015/06/25 14:44:05, stefan-webrtc (holmer) wrote:
> Can we break this out to a method UpdateSendBitrateEstimate()?

Sure

[magalhaesc, 2015-07-01 13:07:27]

Comments addressed

[magalhaesc, 2015-07-01 13:17:20]

Comments addressed

[magalhaesc, 2015-07-01 13:18:01]

Patchset #6 (id:100001) has been deleted

[magalhaesc, 2015-07-01 13:18:12]

Patchset #6 (id:100001) has been deleted

[magalhaesc, 2015-07-01 13:22:03]

Patchset #6 (id:120001) has been deleted

[magalhaesc, 2015-07-01 13:22:42]

Comments addressed

[magalhaesc, 2015-07-01 13:30:20]

Comments addressed

[magalhaesc, 2015-07-01 13:31:16]

Patchset #6 (id:140001) has been deleted

[magalhaesc, 2015-07-01 13:31:23]

Patchset #6 (id:140001) has been deleted

[magalhaesc, 2015-07-01 13:41:17]

Comments addressed

[magalhaesc, 2015-07-01 13:43:39]

Patchset #6 (id:160001) has been deleted

[magalhaesc, 2015-07-01 13:44:55]

Comments addressed.

Main changes:

1) Added MetricRecorder class,

--- Containing:
------ A pointer to the PacketSender (able to get sending bitrate via
TargetBitrateKbps())
------ A pointer to the ChokeFilter (able to get total/per_flow available
capacity)
------ fields algorithm_name, flow_id

--- Most of the plot support was moved from PacketReceiver to MetricRecorder.

2) Removed metric fields from Packet and VideoSender/Source, MetricRecorder has
access to them whenever needed.

3) Modified LinkedSet, clearing 90% of its elements at a time, keeping 10%
newest; made some methods private; changed headers.

4) Added struct LossAccount to BweReceiver.

5) Added bwe_unittests for LinkedSet (others may be added)

6) Removed RecentReceivedRate from BweReceiver, now using RateCounter.
nada_unittest must be modified.

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc (right):

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc:780:
SetBitrateBps(0);
On 2015/06/25 14:44:04, stefan-webrtc (holmer) wrote:
> I'm not sure it's correct to set to 0 here? I'd suggest leaving it unchanged.

Done.

https://codereview.webrtc.org/1202253003/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc:786:
SetBitrateBps(kResumeBitrateBps);
On 2015/06/25 14:44:04, stefan-webrtc (holmer) wrote:
> I would actually suspect that we would resume at the same bitrate as before
> after a pause in webrtc. Or does the evaluation draft specify that it should
be
> reset to the initial bitrate?

Ok, it will be left unchanged.

[stefan-webrtc, 2015-07-02 11:03:42]

Still some things to resolve here, but it's a lot better.

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/bwe.cc (right):

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe.cc:37: bitrate_kbps_ = 0;
I'm not sure this is the right way to handle a Pause. Has that been defined in
some ietf doc? I would like to keep the bitrate_kbps_ as is.

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe.cc:130:
buffer.push_back((*node_it)->copy());
Just do buffer.push_back(*node_it). In fact, I think you can do this even
simpler:
buffer.insert(buffer.begin(), received_packets_.begin(),
received_packets_.begin() + num_preserved_elements);

Or why not do:
received_packets_.erase(received_packets_.begin() + num_preserved_elements,
received_packets_.end());
that way you don't even need to have a temporary buffer.

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe.cc:157: uint16_t newest_seq_nb
= received_packets_.NewestSeqNumber();
nb -> num, it better matches other code in webrtc.

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe.cc:164: }
This code can be replaced with;
gap = static_cast<uint16_t>(newest_seq_nb - oldest_seq_nb + 1);

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe.cc:168: size_t
set_received_packets = received_packets_.size();
This variable can be defined on line 165 and used in the loss ratio calculation.

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe.cc:169: size_t
set_total_packets = set_received_packets / (1.0f - set_loss_ratio);
This is a bit odd, and I would be worried that you get rounding errors. Isn't it
better to keep a count of the number of packets received? Isn't gap equal to the
number of packets you should have received?

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe.cc:173:
static_cast<uint32_t>(set_lost_packets));
Why do you have to cast? Change the input types to LossAccount instead maybe?

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/bwe.h (right):

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe.h:36: PacketIdentifierNode
copy();
Not sure why this is needed. You can simply use the copy constructor.

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe.h:100: void
Subtract(LossAccount subtrahend);
Switch addend and subtrahend to "loss" or "rhs" (right hand side) or something.

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe.h:107: const int
kMaxBitrateKbps = 1500;
Should make these 30 / 2500, right?

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe.h:146: static const int64_t
kReceivingRateTimeWindowMs = 1000;
Do these constants have to be public?

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc (right):

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc:239: }
Good suggestion to move this to the PacketProcessors!

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc:300: //
Silencing unused variable compiling error.
Remove this comment, RTC_UNUSED is enough

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc:489: uint32_t
ChokeFilter::AvailablePerFlowKbps(int flow_id) {
Or just a getter for the total capacity, and then you can have the test compute
the per flow rate based on what flows it has running? Just to avoid having Pause
and Resume on a Choke, which doesn't seem too logical.

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc:728: //
dynamics_->set_sending_estimate_kbps(bits_per_second_ / 1000);
Can this and line 740 be removed?

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc:736: new
MediaPacket(flow_id_, send_time_us, size, prototype_header_);
Why exactly do we have to create empty packets when not running? :)

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc:743: // A
variance picked uniformly from {-1, 0, 1}ms is added to the frame
Space between } and ms

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.h (right):

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.h:236:
rtc::scoped_ptr<RateCounter> rate_counter_;
No need to use a scoped ptr here. Just do
RateCounter rate_counter_;

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.h:349: void
ResumeFlow(int flow_id);  // Decreases available capacity per flow.
What exactly are these used for? Isn't it enough to pause the sender of that
flow?

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/estimators/nada_unittest.cc
(right):

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/estimators/nada_unittest.cc:320: //
TEST_F(NadaReceiverSideTest, ReceivingRateSinglePacket) {
I assume these shouldn't be disabled?

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/metric_recorder.cc (right):

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/metric_recorder.cc:72: for (auto it
= output.begin(); it != output.end(); ++it) {
Can't you do this?
for (T val : output)
  val = val > 0 ? val : 0;

Similarly for other loops above and below.

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/packet_receiver.cc (right):

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/packet_receiver.cc:70:
metric_recorder_ = metric_recorder;
Set this in the initializer list.

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/packet_receiver.cc:94: if
(metric_recorder_ != NULL) {
nullptr

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/packet_receiver.cc:100:
PlotPacketLoss(arrival_time_ms);
Just replace these with a single function called Plot(). Maybe the
MetricRecorder can have a method Plot() which plots everything?

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/packet_receiver.cc:105:
arrival_time_ms);
Should we plot prior to adding new data? Maybe switch the order?

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/packet_sender.cc (right):

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/packet_sender.cc:334:
choke_filter_->ResumeFlow(*flow_ids().begin());
I don't like that the TcpSender can access chokes... That's weird.

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/packet_sender.h (right):

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/packet_sender.h:43: virtual void
set_choke_filter(ChokeFilter* choke_filter) {}
Remove? Why is it needed?

[magalhaesc, 2015-07-02 17:06:19]

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/bwe.cc (right):

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe.cc:37: bitrate_kbps_ = 0;
On 2015/07/02 11:03:41, stefan-webrtc (holmer) wrote:
> I'm not sure this is the right way to handle a Pause. Has that been defined in
> some ietf doc? I would like to keep the bitrate_kbps_ as is.

Makes sense, it was set to zero since previously we were plotting sending
estimate.
The bitrate will remain unchanged and it will be handled before plotting

plot_kbps = 0
if (running)
   plot_kbps = bitrate_kbps

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe.cc:130:
buffer.push_back((*node_it)->copy());
On 2015/07/02 11:03:41, stefan-webrtc (holmer) wrote:
> Just do buffer.push_back(*node_it). In fact, I think you can do this even
> simpler:
> buffer.insert(buffer.begin(), received_packets_.begin(),
> received_packets_.begin() + num_preserved_elements);
> 
> Or why not do:
> received_packets_.erase(received_packets_.begin() + num_preserved_elements,
> received_packets_.end());
> that way you don't even need to have a temporary buffer.

received_packets contains a list and a map, both have to be erased. So I should
iterate through the list to also clean elements from the map (they're not
necessarily sorted in the same order).

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe.cc:157: uint16_t newest_seq_nb
= received_packets_.NewestSeqNumber();
On 2015/07/02 11:03:41, stefan-webrtc (holmer) wrote:
> nb -> num, it better matches other code in webrtc.

Done.

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe.cc:164: }
On 2015/07/02 11:03:41, stefan-webrtc (holmer) wrote:
> This code can be replaced with;
> gap = static_cast<uint16_t>(newest_seq_nb - oldest_seq_nb + 1);

Done.

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe.cc:168: size_t
set_received_packets = received_packets_.size();
On 2015/07/02 11:03:41, stefan-webrtc (holmer) wrote:
> This variable can be defined on line 165 and used in the loss ratio
calculation.

Done.

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe.cc:169: size_t
set_total_packets = set_received_packets / (1.0f - set_loss_ratio);
On 2015/07/02 11:03:41, stefan-webrtc (holmer) wrote:
> This is a bit odd, and I would be worried that you get rounding errors. Isn't
it
> better to keep a count of the number of packets received? Isn't gap equal to
the
> number of packets you should have received?

Yes, right. Gap is the total number of packets that should have been received.

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe.cc:173:
static_cast<uint32_t>(set_lost_packets));
On 2015/07/02 11:03:41, stefan-webrtc (holmer) wrote:
> Why do you have to cast? Change the input types to LossAccount instead maybe?

Done.

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/bwe.h (right):

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe.h:36: PacketIdentifierNode
copy();
On 2015/07/02 11:03:41, stefan-webrtc (holmer) wrote:
> Not sure why this is needed. You can simply use the copy constructor.

Done.

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe.h:100: void
Subtract(LossAccount subtrahend);
On 2015/07/02 11:03:41, stefan-webrtc (holmer) wrote:
> Switch addend and subtrahend to "loss" or "rhs" (right hand side) or
something.

Done.

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe.h:107: const int
kMaxBitrateKbps = 1500;
On 2015/07/02 11:03:41, stefan-webrtc (holmer) wrote:
> Should make these 30 / 2500, right?

right

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe.h:146: static const int64_t
kReceivingRateTimeWindowMs = 1000;
On 2015/07/02 11:03:41, stefan-webrtc (holmer) wrote:
> Do these constants have to be public?

We made them public to use on nada_unittests.cc
Do you think it's better to make them private and add get() methods?

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc (right):

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc:300: //
Silencing unused variable compiling error.
On 2015/07/02 11:03:41, stefan-webrtc (holmer) wrote:
> Remove this comment, RTC_UNUSED is enough

Done.

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc:489: uint32_t
ChokeFilter::AvailablePerFlowKbps(int flow_id) {
On 2015/07/02 11:03:41, stefan-webrtc (holmer) wrote:
> Or just a getter for the total capacity, and then you can have the test
compute
> the per flow rate based on what flows it has running? Just to avoid having
Pause
> and Resume on a Choke, which doesn't seem too logical.

Right, the per flow one was moved to a new class in metric_recorder

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc:728: //
dynamics_->set_sending_estimate_kbps(bits_per_second_ / 1000);
On 2015/07/02 11:03:41, stefan-webrtc (holmer) wrote:
> Can this and line 740 be removed?

Yes, removed

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc:736: new
MediaPacket(flow_id_, send_time_us, size, prototype_header_);
On 2015/07/02 11:03:41, stefan-webrtc (holmer) wrote:
> Why exactly do we have to create empty packets when not running? :)

This was for plotting reasons, for not having a jump line once the flow resumes.

This will be removed and the case will be handled directly in the
MetricRecorder.

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc:743: // A
variance picked uniformly from {-1, 0, 1}ms is added to the frame
On 2015/07/02 11:03:41, stefan-webrtc (holmer) wrote:
> Space between } and ms

Done.

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.h (right):

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.h:236:
rtc::scoped_ptr<RateCounter> rate_counter_;
On 2015/07/02 11:03:41, stefan-webrtc (holmer) wrote:
> No need to use a scoped ptr here. Just do
> RateCounter rate_counter_;

Done.

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.h:349: void
ResumeFlow(int flow_id);  // Decreases available capacity per flow.
On 2015/07/02 11:03:41, stefan-webrtc (holmer) wrote:
> What exactly are these used for? Isn't it enough to pause the sender of that
> flow?

This was now moved to a new class LinkShare in metric_recorder.h

In order to plot the total available capacity, available capacity per flow
(plotted as zero for the paused flows) and compute estimate_error for throughput
histogram plots, the MetricRecorder had a pointer to the ChokeFilter. Now
MetricRecorder has a pointer to LinkShare (with Pause/Resume methods) that
itself has a pointer to ChokeFilter. WDYT?

So whenever we change the ChokeFilter capacity and/or pause/resume flows, we
don't have to worry about modifying anything else, the MetricRecorded will
become acquainted.

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/estimators/nada_unittest.cc
(right):

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/estimators/nada_unittest.cc:320: //
TEST_F(NadaReceiverSideTest, ReceivingRateSinglePacket) {
On 2015/07/02 11:03:41, stefan-webrtc (holmer) wrote:
> I assume these shouldn't be disabled?


Some unittests will be removed as they are not handled in particular by
RateCounter.

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/metric_recorder.cc (right):

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/metric_recorder.cc:72: for (auto it
= output.begin(); it != output.end(); ++it) {
On 2015/07/02 11:03:41, stefan-webrtc (holmer) wrote:
> Can't you do this?
> for (T val : output)
>   val = val > 0 ? val : 0;
> 
> Similarly for other loops above and below.

Done.

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/packet_receiver.cc (right):

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/packet_receiver.cc:70:
metric_recorder_ = metric_recorder;
On 2015/07/02 11:03:42, stefan-webrtc (holmer) wrote:
> Set this in the initializer list.

Done.

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/packet_receiver.cc:94: if
(metric_recorder_ != NULL) {
On 2015/07/02 11:03:41, stefan-webrtc (holmer) wrote:
> nullptr

Done.

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/packet_receiver.cc:100:
PlotPacketLoss(arrival_time_ms);
On 2015/07/02 11:03:42, stefan-webrtc (holmer) wrote:
> Just replace these with a single function called Plot(). Maybe the
> MetricRecorder can have a method Plot() which plots everything?

Done.

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/packet_receiver.cc:105:
arrival_time_ms);
On 2015/07/02 11:03:42, stefan-webrtc (holmer) wrote:
> Should we plot prior to adding new data? Maybe switch the order?

The plots above are dynamics while the added date is for histograms.
The order will be switched as a method Plot will be added.

[magalhaesc, 2015-07-02 17:10:39]

Comments Addressed [2]

[magalhaesc, 2015-07-02 17:17:03]

Comments addressed

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/packet_sender.cc (right):

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/packet_sender.cc:334:
choke_filter_->ResumeFlow(*flow_ids().begin());
On 2015/07/02 11:03:42, stefan-webrtc (holmer) wrote:
> I don't like that the TcpSender can access chokes... That's weird.

Removed

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/packet_sender.h (right):

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/packet_sender.h:43: virtual void
set_choke_filter(ChokeFilter* choke_filter) {}
On 2015/07/02 11:03:42, stefan-webrtc (holmer) wrote:
> Remove? Why is it needed?

Done.

[magalhaesc, 2015-07-02 17:19:30]

https://codereview.webrtc.org/1202253003/diff/200001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/estimators/nada_unittest.cc
(right):

https://codereview.webrtc.org/1202253003/diff/200001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/estimators/nada_unittest.cc:357: //
TEST_F(NadaReceiverSideTest, ReceivingRateLargePackets) {}
I think these commented tests should be removed:

ReceivingRateLargePackets
ReceivingRateIntermittentPackets
ReceivingRateDuplicatedPackets

as they were related to specific cases where RecentReceivingRate was computed in
a different way and differ from how is being computed now.

WDYT?

[stefan-webrtc, 2015-07-03 09:32:11]

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/bwe.h (right):

https://codereview.webrtc.org/1202253003/diff/180001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe.h:146: static const int64_t
kReceivingRateTimeWindowMs = 1000;
On 2015/07/02 17:06:18, magalhaesc wrote:
> On 2015/07/02 11:03:41, stefan-webrtc (holmer) wrote:
> > Do these constants have to be public?
> 
> We made them public to use on nada_unittests.cc
> Do you think it's better to make them private and add get() methods?

No, this is OK.

https://codereview.webrtc.org/1202253003/diff/200001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/bwe.cc (right):

https://codereview.webrtc.org/1202253003/diff/200001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe.cc:123: size_t
num_deleted_elements = (9 * received_packets_.size()) / 10;
num_elements_to_delete

https://codereview.webrtc.org/1202253003/diff/200001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe.cc:127:
received_packets_.Erase(it);
This is still a fairly slow way of doing the erase, since you will have to do a
logarithmic lookup in your map for every element, even though they likely are in
a sequence. You could do it like this instead:

received_packets_.EraseOldPackets(num_elements_to_delete);

And in that method you first erase all packets from the list, and record the
last sequence number you have erased (last_erased_seq_num). Then you do a find
on the map on that sequence number and erase from there:
it = map.find(last_erased_seq_num);
map.erase(it, it + num_elements_to_erase);

I at least _think_ that should work. Makes the erase n + log(n) instead of
n*log(n).

Not sure if it's worth doing?

https://codereview.webrtc.org/1202253003/diff/200001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc (right):

https://codereview.webrtc.org/1202253003/diff/200001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc:225: :
rate_counter_(), listener_(listener), flow_ids_(flow_ids) {
Actually no need to have rate_counter_() in the initializer list. Same on line
216.

https://codereview.webrtc.org/1202253003/diff/200001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/metric_recorder.h (right):

https://codereview.webrtc.org/1202253003/diff/200001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/metric_recorder.h:57: explicit
MetricRecorder(const std::string algorithm_name,
No explicit since this takes multiple args

[magalhaesc, 2015-07-03 11:54:24]

Comments Addressed [3]

[magalhaesc, 2015-07-03 11:55:13]

Comments addressed, fixed bugs

https://codereview.webrtc.org/1202253003/diff/200001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/bwe.cc (right):

https://codereview.webrtc.org/1202253003/diff/200001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe.cc:123: size_t
num_deleted_elements = (9 * received_packets_.size()) / 10;
On 2015/07/03 09:32:11, stefan-webrtc (holmer) wrote:
> num_elements_to_delete

Done.

https://codereview.webrtc.org/1202253003/diff/200001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe.cc:127:
received_packets_.Erase(it);
On 2015/07/03 09:32:11, stefan-webrtc (holmer) wrote:
> This is still a fairly slow way of doing the erase, since you will have to do
a
> logarithmic lookup in your map for every element, even though they likely are
in
> a sequence. You could do it like this instead:
> 
> received_packets_.EraseOldPackets(num_elements_to_delete);
> 
> And in that method you first erase all packets from the list, and record the
> last sequence number you have erased (last_erased_seq_num). Then you do a find
> on the map on that sequence number and erase from there:
> it = map.find(last_erased_seq_num);
> map.erase(it, it + num_elements_to_erase);
> 
> I at least _think_ that should work. Makes the erase n + log(n) instead of
> n*log(n).
> 
> Not sure if it's worth doing?

Kept with the map lookup for every list element to be removed due to possible
disarrangement on packets arrivals.

https://codereview.webrtc.org/1202253003/diff/200001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc (right):

https://codereview.webrtc.org/1202253003/diff/200001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc:225: :
rate_counter_(), listener_(listener), flow_ids_(flow_ids) {
On 2015/07/03 09:32:11, stefan-webrtc (holmer) wrote:
> Actually no need to have rate_counter_() in the initializer list. Same on line
> 216.

Done.

https://codereview.webrtc.org/1202253003/diff/200001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/metric_recorder.h (right):

https://codereview.webrtc.org/1202253003/diff/200001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/metric_recorder.h:57: explicit
MetricRecorder(const std::string algorithm_name,
On 2015/07/03 09:32:11, stefan-webrtc (holmer) wrote:
> No explicit since this takes multiple args

Done.

[magalhaesc, 2015-07-03 11:59:14]

Removed std::next

[stefan-webrtc, 2015-07-06 08:24:51]

LG, no major things left.

https://codereview.webrtc.org/1202253003/diff/200001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/estimators/nada_unittest.cc
(right):

https://codereview.webrtc.org/1202253003/diff/200001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/estimators/nada_unittest.cc:357: //
TEST_F(NadaReceiverSideTest, ReceivingRateLargePackets) {}
On 2015/07/02 17:19:30, magalhaesc wrote:
> I think these commented tests should be removed:
> 
> ReceivingRateLargePackets
> ReceivingRateIntermittentPackets
> ReceivingRateDuplicatedPackets
> 
> as they were related to specific cases where RecentReceivingRate was computed
in
> a different way and differ from how is being computed now.
> 
> WDYT?

Agree

https://codereview.webrtc.org/1202253003/diff/240001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/bwe.cc (right):

https://codereview.webrtc.org/1202253003/diff/240001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe.cc:251: return
(--map_.lower_bound(0x8000))->first;
Mostly as an FYI (no need to change):
In jitter_buffer.h/.cc I solved this type of problem by overriding the
comparison for the map and taking wrap arounds into account in there. Not sure
if that would work for you at all, but it looks like this:


class TimestampLessThan {
 public:
  bool operator() (uint32_t timestamp1,
                   uint32_t timestamp2) const {
    return IsNewerTimestamp(timestamp2, timestamp1);
  }
};

std::map<uint32_t, VCMFrameBuffer*, TimestampLessThan>

https://codereview.webrtc.org/1202253003/diff/240001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/bwe.h (right):

https://codereview.webrtc.org/1202253003/diff/240001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe.h:62: PacketNodeIt begin() {
return list_.begin(); }
Many of the methods below should be const.

https://codereview.webrtc.org/1202253003/diff/240001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe.h:83: void Erase(PacketNodeIt
node_rit);
This doesn't delete a reverse iterator right? If it does I don't see how a user
can get access to a reverse iterator using the methods of this class?

https://codereview.webrtc.org/1202253003/diff/240001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe.h:140: uint32_t RecentKbps(); 
// Receiving Rate.
This method should be const, right? Same with some other methods below.

https://codereview.webrtc.org/1202253003/diff/240001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc (right):

https://codereview.webrtc.org/1202253003/diff/240001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc:741: void
AdaptiveVideoSource::SetBitrateBps(int bitrate_bps) {
I'd prefer to not move this up here, it should be after the AdaptiveVideoSource
constructor definition.

https://codereview.webrtc.org/1202253003/diff/240001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.h (right):

https://codereview.webrtc.org/1202253003/diff/240001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.h:64: int64_t
windows_size_us_;
window_size_us_

https://codereview.webrtc.org/1202253003/diff/240001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/estimators/nada.cc (right):

https://codereview.webrtc.org/1202253003/diff/240001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/estimators/nada.cc:46:
rate_counter_.set_windows_size_ms(kReceivingRateTimeWindowMs);
Is this actually ever changed? Maybe set it in the constructor instead?

https://codereview.webrtc.org/1202253003/diff/240001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/estimators/nada_unittest.cc
(right):

https://codereview.webrtc.org/1202253003/diff/240001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/estimators/nada_unittest.cc:363: //
TEST_F(NadaReceiverSideTest, ReceivingRateDuplicatedPackets) {}
Please remove these as discussed.

https://codereview.webrtc.org/1202253003/diff/240001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/metric_recorder.cc (right):

https://codereview.webrtc.org/1202253003/diff/240001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/metric_recorder.cc:133: const
std::vector<std::string>& prefixs) {
prefixes

https://codereview.webrtc.org/1202253003/diff/240001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/metric_recorder.cc:242: // time_ms
was still not updated here.
I don't understand this comment. time_ms isn't updated at all in this method
afaict. Should it?

https://codereview.webrtc.org/1202253003/diff/240001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/packet_receiver.cc (right):

https://codereview.webrtc.org/1202253003/diff/240001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/packet_receiver.cc:38:
std::vector<std::string> prefixs;
prefixes

https://codereview.webrtc.org/1202253003/diff/240001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/packet_sender.cc (right):

https://codereview.webrtc.org/1202253003/diff/240001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/packet_sender.cc:316:
send_limit_bytes_ = send_limit_bytes;
Set in initializer list.

[magalhaesc, 2015-07-06 09:28:07]

https://codereview.webrtc.org/1202253003/diff/240001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/bwe.h (right):

https://codereview.webrtc.org/1202253003/diff/240001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe.h:62: PacketNodeIt begin() {
return list_.begin(); }
On 2015/07/06 08:24:51, stefan-webrtc (holmer) wrote:
> Many of the methods below should be const.

Done.

https://codereview.webrtc.org/1202253003/diff/240001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe.h:83: void Erase(PacketNodeIt
node_rit);
On 2015/07/06 08:24:51, stefan-webrtc (holmer) wrote:
> This doesn't delete a reverse iterator right? If it does I don't see how a
user
> can get access to a reverse iterator using the methods of this class?

Previously it was for reverse_iterator, I forgot to change node_rit to node_it.

Done.

https://codereview.webrtc.org/1202253003/diff/240001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe.h:140: uint32_t RecentKbps(); 
// Receiving Rate.
On 2015/07/06 08:24:51, stefan-webrtc (holmer) wrote:
> This method should be const, right? Same with some other methods below.

Done.

https://codereview.webrtc.org/1202253003/diff/240001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc (right):

https://codereview.webrtc.org/1202253003/diff/240001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc:741: void
AdaptiveVideoSource::SetBitrateBps(int bitrate_bps) {
On 2015/07/06 08:24:51, stefan-webrtc (holmer) wrote:
> I'd prefer to not move this up here, it should be after the
AdaptiveVideoSource
> constructor definition.

Done.

https://codereview.webrtc.org/1202253003/diff/240001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.h (right):

https://codereview.webrtc.org/1202253003/diff/240001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.h:64: int64_t
windows_size_us_;
On 2015/07/06 08:24:51, stefan-webrtc (holmer) wrote:
> window_size_us_

Done.

https://codereview.webrtc.org/1202253003/diff/240001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/estimators/nada.cc (right):

https://codereview.webrtc.org/1202253003/diff/240001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/estimators/nada.cc:46:
rate_counter_.set_windows_size_ms(kReceivingRateTimeWindowMs);
On 2015/07/06 08:24:51, stefan-webrtc (holmer) wrote:
> Is this actually ever changed? Maybe set it in the constructor instead?

Yes, done.

https://codereview.webrtc.org/1202253003/diff/240001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/estimators/nada_unittest.cc
(right):

https://codereview.webrtc.org/1202253003/diff/240001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/estimators/nada_unittest.cc:363: //
TEST_F(NadaReceiverSideTest, ReceivingRateDuplicatedPackets) {}
On 2015/07/06 08:24:51, stefan-webrtc (holmer) wrote:
> Please remove these as discussed.

Done.

https://codereview.webrtc.org/1202253003/diff/240001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/metric_recorder.cc (right):

https://codereview.webrtc.org/1202253003/diff/240001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/metric_recorder.cc:133: const
std::vector<std::string>& prefixs) {
On 2015/07/06 08:24:51, stefan-webrtc (holmer) wrote:
> prefixes

Done.

https://codereview.webrtc.org/1202253003/diff/240001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/metric_recorder.cc:242: // time_ms
was still not updated here.
On 2015/07/06 08:24:51, stefan-webrtc (holmer) wrote:
> I don't understand this comment. time_ms isn't updated at all in this method
> afaict. Should it?

The comment will be moved to UpdateThroughput method: PushTroughput must be
called before Update

https://codereview.webrtc.org/1202253003/diff/240001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/packet_receiver.cc (right):

https://codereview.webrtc.org/1202253003/diff/240001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/packet_receiver.cc:38:
std::vector<std::string> prefixs;
On 2015/07/06 08:24:51, stefan-webrtc (holmer) wrote:
> prefixes

Done.

https://codereview.webrtc.org/1202253003/diff/240001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/packet_sender.cc (right):

https://codereview.webrtc.org/1202253003/diff/240001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/packet_sender.cc:316:
send_limit_bytes_ = send_limit_bytes;
On 2015/07/06 08:24:51, stefan-webrtc (holmer) wrote:
> Set in initializer list.

Done.

[magalhaesc, 2015-07-06 11:10:45]

Comments addressed [4]

[magalhaesc, 2015-07-06 11:19:12]

Comments addressed [4]

[magalhaesc, 2015-07-06 11:19:31]

Patchset #10 (id:260001) has been deleted

[magalhaesc, 2015-07-06 11:19:37]

https://codereview.webrtc.org/1202253003/diff/240001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/bwe.cc (right):

https://codereview.webrtc.org/1202253003/diff/240001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe.cc:251: return
(--map_.lower_bound(0x8000))->first;
On 2015/07/06 08:24:51, stefan-webrtc (holmer) wrote:
> Mostly as an FYI (no need to change):
> In jitter_buffer.h/.cc I solved this type of problem by overriding the
> comparison for the map and taking wrap arounds into account in there. Not sure
> if that would work for you at all, but it looks like this:
> 
> 
> class TimestampLessThan {
>  public:
>   bool operator() (uint32_t timestamp1,
>                    uint32_t timestamp2) const {
>     return IsNewerTimestamp(timestamp2, timestamp1);
>   }
> };
> 
> std::map<uint32_t, VCMFrameBuffer*, TimestampLessThan>

Thanks, good suggestion.

[magalhaesc, 2015-07-06 11:23:10]

Comments addressed [4]

[magalhaesc, 2015-07-06 11:29:03]

Comments addressed [4]

[magalhaesc, 2015-07-06 11:29:27]

Patchset #10 (id:280001) has been deleted

[magalhaesc, 2015-07-06 11:29:50]

Patchset #10 (id:300001) has been deleted

[magalhaesc, 2015-07-06 11:36:22]

Comments addressed

[stefan-webrtc, 2015-07-07 12:46:37]

A few nits, but otherwise lgtm.

Please check build errors on android though.

https://codereview.webrtc.org/1202253003/diff/320001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc (right):

https://codereview.webrtc.org/1202253003/diff/320001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc:686: int i =
0;
Remove

https://codereview.webrtc.org/1202253003/diff/320001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/estimators/nada.cc (right):

https://codereview.webrtc.org/1202253003/diff/320001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/estimators/nada.cc:155:
bitrate_kbps_ = kMinRefRateKbps;  // Referred as "Reference Rate" = R_n.
Why don't we set this in the initializer list any longer?

https://codereview.webrtc.org/1202253003/diff/320001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/metric_recorder.h (right):

https://codereview.webrtc.org/1202253003/diff/320001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/metric_recorder.h:71: void
PlotLine(int windows_id, const std::string& prefix, int64_t x, T y);
Maybe switch x for time_ms?

[magalhaesc, 2015-07-07 13:02:50]

Added BWE unittests, comments addressed

[magalhaesc, 2015-07-07 13:08:48]

Fixing trybots

[magalhaesc, 2015-07-07 13:12:08]

Patchset #12 (id:360001) has been deleted

[magalhaesc, 2015-07-07 14:26:11]

Added unittests, fixing issues

[magalhaesc, 2015-07-07 15:25:18]

Fixing patch problem

[magalhaesc, 2015-07-08 10:51:02]

Fixing patch issue

[magalhaesc, 2015-07-08 10:51:24]

Patchset #13 (id:400001) has been deleted

[magalhaesc, 2015-07-08 10:51:36]

Patchset #13 (id:420001) has been deleted

[magalhaesc, 2015-07-08 10:59:07]

Fixing patch issues

[magalhaesc, 2015-07-08 10:59:15]

Patchset #13 (id:440001) has been deleted

[magalhaesc, 2015-07-08 11:22:00]

Rebased

[magalhaesc, 2015-07-08 11:22:27]

Patchset #13 (id:460001) has been deleted

[magalhaesc, 2015-07-08 11:30:17]

Patchset #13 (id:480001) has been deleted

[magalhaesc, 2015-07-08 11:33:39]

Rebased

[magalhaesc, 2015-07-08 11:46:19]

Fixing issues

[magalhaesc, 2015-07-08 13:42:17]

Fixing patch issues [2]

[magalhaesc, 2015-07-08 15:13:27]

Fixing patch issues [2]

[magalhaesc, 2015-07-08 15:13:42]

Patchset #15 (id:540001) has been deleted

[magalhaesc, 2015-07-08 15:13:47]

Patchset #15 (id:540001) has been deleted

[magalhaesc, 2015-07-08 15:14:05]

Patchset #14 (id:520001) has been deleted

[magalhaesc, 2015-07-08 16:53:08]

Fixing unittest issues

[magalhaesc, 2015-07-08 18:12:31]

https://codereview.webrtc.org/1202253003/diff/320001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc (right):

https://codereview.webrtc.org/1202253003/diff/320001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc:686: int i =
0;
On 2015/07/07 12:46:37, stefan-webrtc (holmer) wrote:
> Remove

Done.

https://codereview.webrtc.org/1202253003/diff/320001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/estimators/nada.cc (right):

https://codereview.webrtc.org/1202253003/diff/320001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/estimators/nada.cc:155:
bitrate_kbps_ = kMinRefRateKbps;  // Referred as "Reference Rate" = R_n.
On 2015/07/07 12:46:37, stefan-webrtc (holmer) wrote:
> Why don't we set this in the initializer list any longer?

Now it is a member of the upper class BweSender. I will add a BweSender(int
bitrate) constructor to allow initializing on the list here.

https://codereview.webrtc.org/1202253003/diff/320001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/metric_recorder.h (right):

https://codereview.webrtc.org/1202253003/diff/320001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/metric_recorder.h:71: void
PlotLine(int windows_id, const std::string& prefix, int64_t x, T y);
On 2015/07/07 12:46:37, stefan-webrtc (holmer) wrote:
> Maybe switch x for time_ms?

Done.

[magalhaesc, 2015-07-08 18:25:37]

Removed unused lines

[magalhaesc, 2015-07-09 09:42:56]

Comments addressed, issues fixed.

[stefan-webrtc, 2015-07-09 09:55:00]

I have a few comments on your recent patch-sets.

https://codereview.webrtc.org/1202253003/diff/380001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/bwe.h (right):

https://codereview.webrtc.org/1202253003/diff/380001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe.h:117: BweSender(int
bitrate_kbps) : bitrate_kbps_(bitrate_kbps) {}
explicit

https://codereview.webrtc.org/1202253003/diff/380001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe.h:150: double window_size_s()
const { return rate_counter_.window_size_s(); }
BitrateWindowS() instead, so that we understand what the window is for.

https://codereview.webrtc.org/1202253003/diff/380001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/bwe_unittest.cc (right):

https://codereview.webrtc.org/1202253003/diff/380001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe_unittest.cc:130: class
BweReceiverTest : public ::testing::Test, public BweReceiver {
I would prefer to make BweReceiver a member of this class instead.

https://codereview.webrtc.org/1202253003/diff/380001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/estimators/nada_unittest.cc
(right):

https://codereview.webrtc.org/1202253003/diff/380001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/estimators/nada_unittest.cc:351:
static_cast<float>(kReceivingRateKbps) / 100);
divide with 100.0f here and above.

https://codereview.webrtc.org/1202253003/diff/580001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework_unittest.cc
(right):

https://codereview.webrtc.org/1202253003/diff/580001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework_unittest.cc:778:
EXPECT_NEAR(expected_packets, packets.size(), expected_packets / 10 + 20);
I think it's hard to understand the thresholds here and in the changes below.
Can you make the error interval a constant instead? E.g., 100 or whatever is
needed?

https://codereview.webrtc.org/1202253003/diff/580001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/bwe_unittest.cc (right):

https://codereview.webrtc.org/1202253003/diff/580001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe_unittest.cc:165: (8 *
kPacketSizeBytes) / (1000 * window_size_s * 10));
Same here, make this error interval easier to understand by using a constant,
say 100?

[magalhaesc, 2015-07-09 11:20:07]

https://codereview.webrtc.org/1202253003/diff/380001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/bwe.h (right):

https://codereview.webrtc.org/1202253003/diff/380001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe.h:117: BweSender(int
bitrate_kbps) : bitrate_kbps_(bitrate_kbps) {}
On 2015/07/09 09:54:59, stefan-webrtc (holmer) wrote:
> explicit

Done.

https://codereview.webrtc.org/1202253003/diff/380001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe.h:150: double window_size_s()
const { return rate_counter_.window_size_s(); }
On 2015/07/09 09:54:59, stefan-webrtc (holmer) wrote:
> BitrateWindowS() instead, so that we understand what the window is for.

Done.

https://codereview.webrtc.org/1202253003/diff/380001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/bwe_unittest.cc (right):

https://codereview.webrtc.org/1202253003/diff/380001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe_unittest.cc:130: class
BweReceiverTest : public ::testing::Test, public BweReceiver {
On 2015/07/09 09:55:00, stefan-webrtc (holmer) wrote:
> I would prefer to make BweReceiver a member of this class instead.

Right, some methods will become public then.

[magalhaesc, 2015-07-09 11:32:52]

Comments addressed [5]

[magalhaesc, 2015-07-09 11:42:53]

Comments addressed [5]

[magalhaesc, 2015-07-09 11:43:05]

Patchset #17 (id:600001) has been deleted

[magalhaesc, 2015-07-09 11:43:23]

https://codereview.webrtc.org/1202253003/diff/380001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/estimators/nada_unittest.cc
(right):

https://codereview.webrtc.org/1202253003/diff/380001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/estimators/nada_unittest.cc:351:
static_cast<float>(kReceivingRateKbps) / 100);
On 2015/07/09 09:55:00, stefan-webrtc (holmer) wrote:
> divide with 100.0f here and above.

Done.

https://codereview.webrtc.org/1202253003/diff/580001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework_unittest.cc
(right):

https://codereview.webrtc.org/1202253003/diff/580001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework_unittest.cc:778:
EXPECT_NEAR(expected_packets, packets.size(), expected_packets / 10 + 20);
On 2015/07/09 09:55:00, stefan-webrtc (holmer) wrote:
> I think it's hard to understand the thresholds here and in the changes below.
> Can you make the error interval a constant instead? E.g., 100 or whatever is
> needed?

Right, done.

https://codereview.webrtc.org/1202253003/diff/580001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/bwe_unittest.cc (right):

https://codereview.webrtc.org/1202253003/diff/580001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe_unittest.cc:165: (8 *
kPacketSizeBytes) / (1000 * window_size_s * 10));
On 2015/07/09 09:55:00, stefan-webrtc (holmer) wrote:
> Same here, make this error interval easier to understand by using a constant,
> say 100?

Right, I agree

[magalhaesc, 2015-07-09 12:47:27]

Comments addressed.
Thanks for the multiple reviews.

[stefan-webrtc, 2015-07-09 13:11:18]

https://codereview.webrtc.org/1202253003/diff/620001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/bwe.h (right):

https://codereview.webrtc.org/1202253003/diff/620001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe.h:145: void
RelieveSetAndUpdateLoss();
Use FRIEND_TEST() instead of making these public.

https://code.google.com/p/googletest/wiki/AdvancedGuide#Testing_Private_Code

https://codereview.webrtc.org/1202253003/diff/620001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework_unittest.cc
(right):

https://codereview.webrtc.org/1202253003/diff/620001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework_unittest.cc:791:
EXPECT_NEAR(expected_payload_size, media_packet->payload_size(), 600);
Break out these error parameters and make them inputs to the TestVideoSender()
instead. These are too wide.

[magalhaesc, 2015-07-10 11:33:34]

Added delay percentiles, comments addressed

[magalhaesc, 2015-07-10 11:33:48]

https://codereview.webrtc.org/1202253003/diff/620001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/bwe.h (right):

https://codereview.webrtc.org/1202253003/diff/620001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe.h:145: void
RelieveSetAndUpdateLoss();
On 2015/07/09 13:11:18, stefan-webrtc (holmer) wrote:
> Use FRIEND_TEST() instead of making these public.
> 
> https://code.google.com/p/googletest/wiki/AdvancedGuide#Testing_Private_Code

Done.

https://codereview.webrtc.org/1202253003/diff/620001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework_unittest.cc
(right):

https://codereview.webrtc.org/1202253003/diff/620001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework_unittest.cc:791:
EXPECT_NEAR(expected_payload_size, media_packet->payload_size(), 600);
On 2015/07/09 13:11:18, stefan-webrtc (holmer) wrote:
> Break out these error parameters and make them inputs to the TestVideoSender()
> instead. These are too wide.

Done.

[magalhaesc, 2015-07-10 12:15:23]

Increased error margins

[magalhaesc, 2015-07-10 12:53:58]

Removed received packet logging from NadaBweReceiver

[magalhaesc, 2015-07-12 14:57:20]

Replaced some nada unittests by bwe unittests. Added more nada unittests

[magalhaesc, 2015-07-12 15:01:39]

Made frame timestamps indepedent from previous randomizations

[magalhaesc, 2015-07-12 15:05:07]

Updated bwe_simulations.cc, now building with test_logging enabled

[magalhaesc, 2015-07-12 15:54:19]

Updating BweTestFramework VideoSender unittests - accordingly to new
randomization

[magalhaesc, 2015-07-13 10:21:50]

Adapted PacedVideoSender::TimeToSendPacket

[magalhaesc, 2015-07-13 11:59:17]

Addressed topics discussed on Friday

[magalhaesc, 2015-07-13 13:17:14]

Adapted PacedVideoSender::TimeToSendPacket

[magalhaesc, 2015-07-13 13:18:13]

Patchset #25 (id:780001) has been deleted

[stefan-webrtc, 2015-07-14 13:49:04]

https://codereview.webrtc.org/1202253003/diff/800001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc (right):

https://codereview.webrtc.org/1202253003/diff/800001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc:693:
next_frame_ms_ - 1 + 2 * static_cast<float>(rand()) / RAND_MAX;
Make sure this can't be negative.

https://codereview.webrtc.org/1202253003/diff/800001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.h (right):

https://codereview.webrtc.org/1202253003/diff/800001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.h:423: int64_t
GetTimeUntilNextFrameMs() const { return next_frame_ms_ - now_ms_; }
Make sure this takes the randomization into account.

https://codereview.webrtc.org/1202253003/diff/800001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/packet_sender.cc (right):

https://codereview.webrtc.org/1202253003/diff/800001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/packet_sender.cc:266:
assert(pace_out_time_ms >= (media_packet->send_time_us() - 500) / 1000);
This should not be needed.

[magalhaesc, 2015-07-14 16:33:07]

diff

[magalhaesc, 2015-07-14 16:43:12]

diff2

[magalhaesc, 2015-07-14 16:43:17]

Patchset #26 (id:820001) has been deleted

[magalhaesc, 2015-07-14 17:18:39]

Rebased, comments addressed

[magalhaesc, 2015-07-14 17:19:01]

Patchset #26 (id:840001) has been deleted

[magalhaesc, 2015-07-14 17:19:33]

Rebased, comments addressed

https://codereview.webrtc.org/1202253003/diff/800001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc (right):

https://codereview.webrtc.org/1202253003/diff/800001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc:693:
next_frame_ms_ - 1 + 2 * static_cast<float>(rand()) / RAND_MAX;
On 2015/07/14 13:49:04, stefan-webrtc (holmer) wrote:
> Make sure this can't be negative.

Done.

https://codereview.webrtc.org/1202253003/diff/800001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.h (right):

https://codereview.webrtc.org/1202253003/diff/800001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.h:423: int64_t
GetTimeUntilNextFrameMs() const { return next_frame_ms_ - now_ms_; }
On 2015/07/14 13:49:04, stefan-webrtc (holmer) wrote:
> Make sure this takes the randomization into account.

Done.

https://codereview.webrtc.org/1202253003/diff/800001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/packet_sender.cc (right):

https://codereview.webrtc.org/1202253003/diff/800001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/packet_sender.cc:266:
assert(pace_out_time_ms >= (media_packet->send_time_us() - 500) / 1000);
On 2015/07/14 13:49:04, stefan-webrtc (holmer) wrote:
> This should not be needed.

Done.

[magalhaesc, 2015-07-14 17:25:58]

Rebased, comments addressed

[magalhaesc, 2015-07-14 17:28:07]

Patchset #26 (id:860001) has been deleted

[magalhaesc, 2015-07-14 17:51:06]

Fixing issues

[stefan-webrtc, 2015-07-15 11:28:20]

lgtm

https://codereview.webrtc.org/1202253003/diff/900001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc (right):

https://codereview.webrtc.org/1202253003/diff/900001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc:703:
std::max(next_frame_ms_ + next_frame_rand_ms_, static_cast<int64_t>(0));
nit

You can also do
std::max<int64_t>()

[magalhaesc, 2015-07-15 11:31:10]

Fixing issues [2]

[stefan-webrtc, 2015-07-15 11:31:42]

https://codereview.webrtc.org/1202253003/diff/900001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc (right):

https://codereview.webrtc.org/1202253003/diff/900001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc:679: return
next_frame_ms_ + next_frame_rand_ms_ - now_ms_;
next_frame_rand_ms_ must be initialized.

[stefan-webrtc, 2015-07-15 11:35:54]

https://codereview.webrtc.org/1202253003/diff/920001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/estimators/nada.cc (right):

https://codereview.webrtc.org/1202253003/diff/920001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/estimators/nada.cc:22: #include
<iostream>
Remove

https://codereview.webrtc.org/1202253003/diff/920001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/packet_sender.cc (right):

https://codereview.webrtc.org/1202253003/diff/920001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/packet_sender.cc:136: 500) /
Odd to do rounding here and not on line 134. I don't think you actually need to
round this?

[magalhaesc, 2015-07-15 11:41:59]

Comments addressed

https://codereview.webrtc.org/1202253003/diff/900001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc (right):

https://codereview.webrtc.org/1202253003/diff/900001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc:679: return
next_frame_ms_ + next_frame_rand_ms_ - now_ms_;
On 2015/07/15 11:31:41, stefan-webrtc (holmer) wrote:
> next_frame_rand_ms_ must be initialized.

Done.

https://codereview.webrtc.org/1202253003/diff/900001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc:703:
std::max(next_frame_ms_ + next_frame_rand_ms_, static_cast<int64_t>(0));
On 2015/07/15 11:28:20, stefan-webrtc (holmer) wrote:
> nit
> 
> You can also do
> std::max<int64_t>()

Done.

https://codereview.webrtc.org/1202253003/diff/920001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/estimators/nada.cc (right):

https://codereview.webrtc.org/1202253003/diff/920001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/estimators/nada.cc:22: #include
<iostream>
On 2015/07/15 11:35:54, stefan-webrtc (holmer) wrote:
> Remove

Done.

https://codereview.webrtc.org/1202253003/diff/920001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/packet_sender.cc (right):

https://codereview.webrtc.org/1202253003/diff/920001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/packet_sender.cc:136: 500) /
On 2015/07/15 11:35:54, stefan-webrtc (holmer) wrote:
> Odd to do rounding here and not on line 134. I don't think you actually need
to
> round this?

Done.

[magalhaesc, 2015-07-15 11:44:17]

Fixing issues [2]

[magalhaesc, 2015-07-15 11:50:40]

Fixing issues [2]

[magalhaesc, 2015-07-15 11:51:06]

Patchset #29 (id:940001) has been deleted

[magalhaesc, 2015-07-15 12:15:32]

Fixed memory leak

[stefan-webrtc, 2015-07-15 12:17:51]

lgtm

https://codereview.webrtc.org/1202253003/diff/980001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/estimators/nada_unittest.cc
(right):

https://codereview.webrtc.org/1202253003/diff/980001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/estimators/nada_unittest.cc:316:
NadaFeedback* nada_feedback =
scoped_ptr

[magalhaesc, 2015-07-15 12:29:39]

Using rtc::scoped_ptr on nada_unittest.cc

[magalhaesc, 2015-07-15 12:32:35]

Thanks, comments addressed

https://codereview.webrtc.org/1202253003/diff/980001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/estimators/nada_unittest.cc
(right):

https://codereview.webrtc.org/1202253003/diff/980001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/estimators/nada_unittest.cc:316:
NadaFeedback* nada_feedback =
On 2015/07/15 12:17:50, stefan-webrtc (holmer) wrote:
> scoped_ptr

Done.

[magalhaesc, 2015-07-15 14:11:57]

Modified JitterFilter and JitterFilter unittest

[magalhaesc, 2015-07-15 14:12:16]

Modified JitterFilter and JitterFilter unittest

[magalhaesc, 2015-07-15 14:15:15]

https://codereview.webrtc.org/1202253003/diff/1010001/webrtc/modules/remote_b...
File webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework_unittest.cc
(right):

https://codereview.webrtc.org/1202253003/diff/1010001/webrtc/modules/remote_b...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework_unittest.cc:483:
ASSERT_TRUE(IsTimeSorted(original));
Perhaps make another unittest setting reordering_ = true ?
WDYT?

[magalhaesc, 2015-07-15 14:26:34]

Patchset #32 (id:1010001) has been deleted

[magalhaesc, 2015-07-15 14:30:18]

same

[magalhaesc, 2015-07-15 14:31:02]

Patchset #32 (id:1020001) has been deleted

[magalhaesc, 2015-07-15 14:31:33]

Committed patchset #31 (id:860002) manually as
9c261f2d13793fbb5a0d07b26bec4154bc38342b (presubmit successful).