Issue 2722933002: Measure packet loss so we can use it to select ICE candidate pairs.

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+409 lines, -7 lines)			Patch
M	webrtc/p2p/BUILD.gn	View	1 2 3	2 chunks	+3 lines, -0 lines	0 comments	Download
A	webrtc/p2p/base/packetlossestimator.h	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+86 lines, -0 lines	0 comments	Download
A	webrtc/p2p/base/packetlossestimator.cc	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+126 lines, -0 lines	0 comments	Download
A	webrtc/p2p/base/packetlossestimator_unittest.cc	View	1 2 3 4 5 6 7 8 9	1 chunk	+172 lines, -0 lines	0 comments	Download
M	webrtc/p2p/base/port.h	View	1 2 3 4 5 6 7	3 chunks	+10 lines, -7 lines	0 comments	Download
M	webrtc/p2p/base/port.cc	View	1 2 3 4	4 chunks	+12 lines, -0 lines	0 comments	Download

Messages

Total messages: 17 (5 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

Zach Stein

Just the measurement code so far. Will look at other places to measure from (P2PTransportChannel::OnCheckAndPing?) ...

3 years, 9 months ago (2017-03-01 00:32:00 UTC) #2

Taylor Brandstetter

Looks good; and good idea making this a standalone CL. https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (right): https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port.cc#newcode152 ...

3 years, 9 months ago (2017-03-01 02:26:13 UTC) #3

Zach Stein

https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (right): https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port.cc#newcode152 webrtc/p2p/base/port.cc:152: // 3) we never expected to see this id ...

3 years, 9 months ago (2017-03-02 00:02:23 UTC) #4

https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port.cc
File webrtc/p2p/base/port.cc (right):

https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port.cc#n...
webrtc/p2p/base/port.cc:152: // 3) we never expected to see this id (something
else went wrong).
On 2017/03/01 02:26:12, Taylor Brandstetter wrote:
> nit: Capitalization of "we"

Done.

https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port.cc#n...
webrtc/p2p/base/port.cc:178: static_cast<double>(responses_received_) /
responses_expected_;
On 2017/03/01 02:26:12, Taylor Brandstetter wrote:
> Divide by zero?

Done.

https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port.cc#n...
webrtc/p2p/base/port.cc:939: packet_loss_estimator_(kPortTimeoutDelay),
On 2017/03/01 02:26:12, Taylor Brandstetter wrote:
> This value recently became very large (~45 seconds), so it probably won't be
> very useful.
> 
> Maybe we should have two time thresholds?
> 
> Threshold A (RTT and change): Response is counted as lost, but still
remembered
> in case a response arrives later than expected.
> 
> Threshold B (~45 seconds): Response completely forgotten.

I like this idea. As is, we probably won't get a useful signal until long after
a connection has been established. I would like to start with the simplest
possible implementation so that we have something to measure though.

https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port.h
File webrtc/p2p/base/port.h (right):

https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port.h#ne...
webrtc/p2p/base/port.h:436: class PacketLossEstimator {
On 2017/03/01 02:26:12, Taylor Brandstetter wrote:
> Some comments around this class may be helpful. Such as: "Tracks the
percentage
> of STUN pings that received a response. A response is counted as lost only
after
> |expire_after| milliseconds."

Done.

https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port.h#ne...
webrtc/p2p/base/port.h:438: struct PacketInfo {
On 2017/03/01 02:26:12, Taylor Brandstetter wrote:
> It looks like this struct can be private.

Don't need this now that outstanding_packets_ is an unordered_map (see below).

https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port.h#ne...
webrtc/p2p/base/port.h:446: PacketLossEstimator(int64_t expire_after);
On 2017/03/01 02:26:12, Taylor Brandstetter wrote:
> Doesn't have to be in this CL, but it may be useful to be able to set "expire
> after" after creation. For example, to set it to "RTT * 1.5", or "RTT + some
> factor of RTT variance" after RTT converges.

I like this idea but will leave it for another CL. I renamed expire_after to
consider_lost_after. Maybe we can differentiate between messages that are
considered expired (we will still listen for the responses) and lost (we will
stop listening for the responses) later.

https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port.h#ne...
webrtc/p2p/base/port.h:454: // This is updated during ReceivedResponse and
RemoveExpiredPackets.
On 2017/03/01 02:26:12, Taylor Brandstetter wrote:
> The code may be more readable if only "RemoveExpiredPackets" updates the rate.
> And actually, it could just be called "UpdateResponseRate" instead. 

Good suggestion, I think this will be cleaner.

https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port.h#ne...
webrtc/p2p/base/port.h:462: int64_t get_responses_received() const { return
responses_received_; }
On 2017/03/01 02:26:13, Taylor Brandstetter wrote:
> I'd prefer not having "for test" methods unless absolutely necessary; in this
> case I think you should be able to test everything just using
> get_response_rate().

I think this gives better coverage (ResponseLost/ResponseLate will fail if
responses_expected_ is not incremented, but we wouldn't notice if we were only
checking response_rate_), but I will delete them if you feel strongly.

https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port.h#ne...
webrtc/p2p/base/port.h:471: std::vector<PacketInfo> outstanding_packets_;
On 2017/03/01 02:26:12, Taylor Brandstetter wrote:
> nit: Probably doesn't matter, but would an unordered_map of IDs to times be
more
> efficient?

Maybe. Thinking about this did make me realize that I was assuming the vector
would be sorted by sent_time in RemoveExpiredPackets (which isn't documented or
necessarily true), so I will switch to an unordered_map. Hashing strings might
be slower, but I agree it probably doesn't matter (though I have never seen
profiles of any of this code).

https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port.h#ne...
webrtc/p2p/base/port.h:475: };
On 2017/03/01 02:26:12, Taylor Brandstetter wrote:
> I think we can move this to its own file; port.h already has a lot of stuff,
and
> more files doesn't really hurt.

Acknowledged. I will do this in a separate upload so the diffs are easier to
read.

https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port_unit...
File webrtc/p2p/base/port_unittest.cc (right):

https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port_unit...
webrtc/p2p/base/port_unittest.cc:2889: class PacketLossEstimatorTest : public
testing::Test {};
On 2017/03/01 02:26:13, Taylor Brandstetter wrote:
> Could have a test for calling RemoveExpiredPackets/get_response_rate before
> ExpectResponse has been called.

Done.

https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port_unit...
webrtc/p2p/base/port_unittest.cc:2952: ple.ReceivedResponse("1", 7);  // "1"
should already be expired
On 2017/03/01 02:26:13, Taylor Brandstetter wrote:
> nit: Generally our style is to use full capitalization/punctuation for
comments,
> even one-liners.

Done.

Taylor Brandstetter

https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port.h File webrtc/p2p/base/port.h (right): https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port.h#newcode462 webrtc/p2p/base/port.h:462: int64_t get_responses_received() const { return responses_received_; } On 2017/03/02 ...

3 years, 9 months ago (2017-03-02 03:09:58 UTC) #5

https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port.h
File webrtc/p2p/base/port.h (right):

https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port.h#ne...
webrtc/p2p/base/port.h:462: int64_t get_responses_received() const { return
responses_received_; }
On 2017/03/02 00:02:23, zstein wrote:
> On 2017/03/01 02:26:13, Taylor Brandstetter wrote:
> > I'd prefer not having "for test" methods unless absolutely necessary; in
this
> > case I think you should be able to test everything just using
> > get_response_rate().
> 
> I think this gives better coverage (ResponseLost/ResponseLate will fail if
> responses_expected_ is not incremented, but we wouldn't notice if we were only
> checking response_rate_), but I will delete them if you feel strongly.

ResponseLate and ResponseLost could be updated to send one request (or multiple)
that succeeds, then one whose response is lost, and check that the percentage is
what's expected. Maybe I'm a little opinionated about this, but my experience so
far with "for_testing" stuff is usually negative; it's hard to maintain and
often results in less overall test coverage. And almost always, you can get the
same test coverage with a black box test.

Maybe this CL isn't the best example, but we have lots of tests in this area of
code that call internal event handler methods (like "OnReadPacket"). We'd like
to change the implementation of these objects but it's made more difficult by
all the tests depending on implementation details. Plus, in some cases they're
only testing a small part of the normal code path.

https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port.h#ne...
webrtc/p2p/base/port.h:475: };
On 2017/03/02 00:02:23, zstein wrote:
> On 2017/03/01 02:26:12, Taylor Brandstetter wrote:
> > I think we can move this to its own file; port.h already has a lot of stuff,
> and
> > more files doesn't really hurt.
> 
> Acknowledged. I will do this in a separate upload so the diffs are easier to
> read.

Thanks; that does help a lot.

Zach Stein

I think I responded to everything + more. Please take another look, thanks!

3 years, 9 months ago (2017-03-06 21:11:12 UTC) #6

Taylor Brandstetter

https://codereview.webrtc.org/2722933002/diff/120001/webrtc/p2p/base/packetlossestimator.cc File webrtc/p2p/base/packetlossestimator.cc (right): https://codereview.webrtc.org/2722933002/diff/120001/webrtc/p2p/base/packetlossestimator.cc#newcode2 webrtc/p2p/base/packetlossestimator.cc:2: * Copyright 2004 The WebRTC Project Authors. All rights ...

3 years, 9 months ago (2017-03-07 18:16:32 UTC) #7

Zach Stein

Thanks for all the feedback so far! Here is my next round of thinking. https://codereview.webrtc.org/2722933002/diff/120001/webrtc/p2p/base/packetlossestimator.cc ...

3 years, 9 months ago (2017-03-08 06:21:15 UTC) #8

Thanks for all the feedback so far! Here is my next round of thinking.

https://codereview.webrtc.org/2722933002/diff/120001/webrtc/p2p/base/packetlo...
File webrtc/p2p/base/packetlossestimator.cc (right):

https://codereview.webrtc.org/2722933002/diff/120001/webrtc/p2p/base/packetlo...
webrtc/p2p/base/packetlossestimator.cc:2: *  Copyright 2004 The WebRTC Project
Authors. All rights reserved.
On 2017/03/07 18:16:31, Taylor Brandstetter wrote:
> nit: 2017 

Done.

https://codereview.webrtc.org/2722933002/diff/120001/webrtc/p2p/base/packetlo...
webrtc/p2p/base/packetlossestimator.cc:17: :
consider_lost_after_(consider_lost_after), forget_after_(forget_after) {}
On 2017/03/07 18:16:32, Taylor Brandstetter wrote:
> Could add an "RTC_DCHECK_LT(consider_lost_after, forget_after)" (from
> webrtc/base/checks.h)

Done.

https://codereview.webrtc.org/2722933002/diff/120001/webrtc/p2p/base/packetlo...
webrtc/p2p/base/packetlossestimator.cc:22: UpdateResponseRateIfStale(sent_time);
On 2017/03/07 18:16:32, Taylor Brandstetter wrote:
> Is this called just to ensure the the map doesn't grow indefinitely if the
> client doesn't call "UpdateResponseRate"? If so I'd suggest making it *not*
> actually update the response rate since that may be an unexpected side effect.
> 
> There could be a method called "ForgetOldRequests", for example, that only
> serves to clear things from the map and is called by both this and
> UpdateResponseRate.

Having ForgetOldRequests not update the response rate does sound better.

Do you have a preference for how to test this gets called from
ReceivedResponse/UpdateResponseRate if we aren't updating the response rate? I
don't see how to do it without either expanding the public interface or
providing something like a protected getter just for the tests (though maybe
that is more okay in this case, since we are explicitly testing something that
isn't part of the public interface).

https://codereview.webrtc.org/2722933002/diff/120001/webrtc/p2p/base/packetlo...
File webrtc/p2p/base/packetlossestimator.h (right):

https://codereview.webrtc.org/2722933002/diff/120001/webrtc/p2p/base/packetlo...
webrtc/p2p/base/packetlossestimator.h:23: // |consider_lost_after| ms. If a
responses is received after
On 2017/03/07 18:16:32, Taylor Brandstetter wrote:
> "response", singular

Done.

https://codereview.webrtc.org/2722933002/diff/120001/webrtc/p2p/base/packetlo...
webrtc/p2p/base/packetlossestimator.h:24: // |consider_lost_after| ms, it is
still considered to be a response.
On 2017/03/07 18:16:32, Taylor Brandstetter wrote:
> Maybe "counted as" instead of "considered to be"?

Done.

https://codereview.webrtc.org/2722933002/diff/120001/webrtc/p2p/base/packetlo...
webrtc/p2p/base/packetlossestimator.h:26: // anymore. The response rate is
initially assumed to be 1.0. If the current
On 2017/03/07 18:16:32, Taylor Brandstetter wrote:
> Paragraph break before "If"?

Done.

https://codereview.webrtc.org/2722933002/diff/120001/webrtc/p2p/base/packetlo...
webrtc/p2p/base/packetlossestimator.h:27: // time is 7, |forget_after| is 6, and
|consider_lost_after| is 2, the response
On 2017/03/07 18:16:32, Taylor Brandstetter wrote:
> Since "forget_after" now means "forget both sent requests and received
> responses", maybe "window_size" would be a more clear name.

I don't really like the name forget after, but I would think of window size as
forget_after - consider_lost_after (forget_after is relative to now, not the
right/near end of the window). Using window_size and window_offset does sound a
bit more formal though (forget_after = window_size + window_offset,
consider_lost_after = window_offset). Do you like that better?

https://codereview.webrtc.org/2722933002/diff/120001/webrtc/p2p/base/packetlo...
webrtc/p2p/base/packetlossestimator.h:32: // Wind:   <------->   |
On 2017/03/07 18:16:32, Taylor Brandstetter wrote:
> I'm not sure this is accurate; if a request is sent at time 6 and received at
> time 7, won't it be counted?

It will be. I was trying to show the window where responses count against the
response rate, but that is confusing. Maybe I should draw the window like this:

// Time: 0 1 2 3 4 5 6 7
// Wind:   <=======|--->

And say that responses expected in the = region are counted against the loss
rate but responses expected in the - region are not. Or we could just not count
responses received in the - region yet, but that seems like it might be a useful
signal.

I could also define things a bit more mathematically (I need to double check
inclusive/exclusive bounds):

// response_rate = received_full_window / (expected_left + received_right)
// received_full_window := #responses received for tracked IDs in [wind_left,
now]
// expected_left := #responses expected for tracked IDs sent in [wind_left,
wind_right)
// received_right := #responses received for tracked IDs from [wind_right, now]

Where wind_left is < and wind_right is > in the window from the diff. I'll try
to clarify this all and see where I end up.

https://codereview.webrtc.org/2722933002/diff/120001/webrtc/p2p/base/packetlo...
webrtc/p2p/base/packetlossestimator.h:64: int64_t consider_lost_after_;  // In
milliseconds.
On 2017/03/07 18:16:32, Taylor Brandstetter wrote:
> I forgot if I suggested this, but if these variables were named with "_ms_" at
> the end, the "In milleseconds" comment wouldn't be necessary.

Done.

Taylor Brandstetter

lgtm after addressing comment about "PacketLossEstimatorInterface" https://codereview.webrtc.org/2722933002/diff/120001/webrtc/p2p/base/packetlossestimator.cc File webrtc/p2p/base/packetlossestimator.cc (right): https://codereview.webrtc.org/2722933002/diff/120001/webrtc/p2p/base/packetlossestimator.cc#newcode22 webrtc/p2p/base/packetlossestimator.cc:22: UpdateResponseRateIfStale(sent_time); On 2017/03/08 ...

3 years, 9 months ago (2017-03-08 17:48:21 UTC) #9

lgtm after addressing comment about "PacketLossEstimatorInterface"

https://codereview.webrtc.org/2722933002/diff/120001/webrtc/p2p/base/packetlo...
File webrtc/p2p/base/packetlossestimator.cc (right):

https://codereview.webrtc.org/2722933002/diff/120001/webrtc/p2p/base/packetlo...
webrtc/p2p/base/packetlossestimator.cc:22: UpdateResponseRateIfStale(sent_time);
On 2017/03/08 06:21:14, zstein wrote:
> On 2017/03/07 18:16:32, Taylor Brandstetter wrote:
> > Is this called just to ensure the the map doesn't grow indefinitely if the
> > client doesn't call "UpdateResponseRate"? If so I'd suggest making it *not*
> > actually update the response rate since that may be an unexpected side
effect.
> > 
> > There could be a method called "ForgetOldRequests", for example, that only
> > serves to clear things from the map and is called by both this and
> > UpdateResponseRate.
> 
> Having ForgetOldRequests not update the response rate does sound better.
> 
> Do you have a preference for how to test this gets called from
> ReceivedResponse/UpdateResponseRate if we aren't updating the response rate? I
> don't see how to do it without either expanding the public interface or
> providing something like a protected getter just for the tests (though maybe
> that is more okay in this case, since we are explicitly testing something that
> isn't part of the public interface).

How to test that the map doesn't grow indefinitely, you mean? I haven't seen
many examples of that kind of test, but that's a situation where it does seem
appropriate to have a "tracked_request_count_for_testing()" (for example)
method. We add the "for_testing" suffix to make it clear that the method is only
used for testing, and I believe there's a presubmit check that complains if you
call a "for_testing" method outside a test.

https://codereview.webrtc.org/2722933002/diff/120001/webrtc/p2p/base/packetlo...
File webrtc/p2p/base/packetlossestimator.h (right):

https://codereview.webrtc.org/2722933002/diff/120001/webrtc/p2p/base/packetlo...
webrtc/p2p/base/packetlossestimator.h:27: // time is 7, |forget_after| is 6, and
|consider_lost_after| is 2, the response
On 2017/03/08 06:21:15, zstein wrote:
> On 2017/03/07 18:16:32, Taylor Brandstetter wrote:
> > Since "forget_after" now means "forget both sent requests and received
> > responses", maybe "window_size" would be a more clear name.
> 
> I don't really like the name forget after, but I would think of window size as
> forget_after - consider_lost_after (forget_after is relative to now, not the
> right/near end of the window). Using window_size and window_offset does sound
a
> bit more formal though (forget_after = window_size + window_offset,
> consider_lost_after = window_offset). Do you like that better?

I guess "window size" is still ambiguous since there are two windows (window of
responses being counted as received and window of responses being counted as
lost). Let's just stick with forget_after.

https://codereview.webrtc.org/2722933002/diff/120001/webrtc/p2p/base/packetlo...
webrtc/p2p/base/packetlossestimator.h:32: // Wind:   <------->   |
On 2017/03/08 06:21:15, zstein wrote:
> On 2017/03/07 18:16:32, Taylor Brandstetter wrote:
> > I'm not sure this is accurate; if a request is sent at time 6 and received
at
> > time 7, won't it be counted?
> 
> It will be. I was trying to show the window where responses count against the
> response rate, but that is confusing. Maybe I should draw the window like
this:
> 
> // Time: 0 1 2 3 4 5 6 7
> // Wind:   <=======|--->
> 
> And say that responses expected in the = region are counted against the loss
> rate but responses expected in the - region are not. Or we could just not
count
> responses received in the - region yet, but that seems like it might be a
useful
> signal.
> 
> I could also define things a bit more mathematically (I need to double check
> inclusive/exclusive bounds):
> 
> // response_rate = received_full_window / (expected_left + received_right)
> // received_full_window := #responses received for tracked IDs in [wind_left,
> now]
> // expected_left := #responses expected for tracked IDs sent in [wind_left,
> wind_right)
> // received_right := #responses received for tracked IDs from [wind_right,
now]
> 
> Where wind_left is < and wind_right is > in the window from the diff. I'll try
> to clarify this all and see where I end up.

That diagram looks good (or, even just the sentence you added in the latest
patchset is good). I wouldn't worry about it too much; someone can always just
look a the code or tests to confirm the expected behavior.

https://codereview.webrtc.org/2722933002/diff/140001/webrtc/p2p/base/packetlo...
File webrtc/p2p/base/packetlossestimator.h (right):

https://codereview.webrtc.org/2722933002/diff/140001/webrtc/p2p/base/packetlo...
webrtc/p2p/base/packetlossestimator.h:23: // TODO(zstein): The above comment
describes a PacketLossEstimatorInterface.
I don't think an interface is necessary until we have multiple implementations.
Generally our policy is "only add something once you need it". If you add things
you think you *might* need, half the time you don't end up needing them (or at
least, that's my experience).

Zach Stein

Would you mind taking one more quick look before I submit? Thanks! https://codereview.webrtc.org/2722933002/diff/120001/webrtc/p2p/base/packetlossestimator.cc File webrtc/p2p/base/packetlossestimator.cc ...

3 years, 9 months ago (2017-03-08 23:00:09 UTC) #10

Would you mind taking one more quick look before I submit? Thanks!

https://codereview.webrtc.org/2722933002/diff/120001/webrtc/p2p/base/packetlo...
File webrtc/p2p/base/packetlossestimator.cc (right):

https://codereview.webrtc.org/2722933002/diff/120001/webrtc/p2p/base/packetlo...
webrtc/p2p/base/packetlossestimator.cc:22: UpdateResponseRateIfStale(sent_time);
On 2017/03/08 17:48:20, Taylor Brandstetter wrote:
> On 2017/03/08 06:21:14, zstein wrote:
> > On 2017/03/07 18:16:32, Taylor Brandstetter wrote:
> > > Is this called just to ensure the the map doesn't grow indefinitely if the
> > > client doesn't call "UpdateResponseRate"? If so I'd suggest making it
*not*
> > > actually update the response rate since that may be an unexpected side
> effect.
> > > 
> > > There could be a method called "ForgetOldRequests", for example, that only
> > > serves to clear things from the map and is called by both this and
> > > UpdateResponseRate.
> > 
> > Having ForgetOldRequests not update the response rate does sound better.
> > 
> > Do you have a preference for how to test this gets called from
> > ReceivedResponse/UpdateResponseRate if we aren't updating the response rate?
I
> > don't see how to do it without either expanding the public interface or
> > providing something like a protected getter just for the tests (though maybe
> > that is more okay in this case, since we are explicitly testing something
that
> > isn't part of the public interface).
> 
> How to test that the map doesn't grow indefinitely, you mean? I haven't seen
> many examples of that kind of test, but that's a situation where it does seem
> appropriate to have a "tracked_request_count_for_testing()" (for example)
> method. We add the "for_testing" suffix to make it clear that the method is
only
> used for testing, and I believe there's a presubmit check that complains if
you
> call a "for_testing" method outside a test.

Done.

https://codereview.webrtc.org/2722933002/diff/140001/webrtc/p2p/base/packetlo...
File webrtc/p2p/base/packetlossestimator.h (right):

https://codereview.webrtc.org/2722933002/diff/140001/webrtc/p2p/base/packetlo...
webrtc/p2p/base/packetlossestimator.h:23: // TODO(zstein): The above comment
describes a PacketLossEstimatorInterface.
On 2017/03/08 17:48:21, Taylor Brandstetter wrote:
> I don't think an interface is necessary until we have multiple
implementations.
> Generally our policy is "only add something once you need it". If you add
things
> you think you *might* need, half the time you don't end up needing them (or at
> least, that's my experience).

That's why I just put this in a comment :)

I don't feel super strongly about this case, but in general I think there can be
a lot of value in introducing interfaces early. One advantage in this case would
be the separation of the intended client API and the implementation details.
This is the whole interface:

class IPacketLossEstimator {
 public:
  ExpectResponse(string id, int64_t sent_time);
  ReceivedResponse(string id, int64_t received_time);
  void UpdateResponseRate(int64_t now);
  double response_rate() const;
}

The details of what is considered lost, the private helpers and members, and
methods for testing/debugging make it harder to understand the class and easier
to depend more heavily on the class' internals.

If I had made an interface from the beginning, I also might not have overwritten
my previous implementation that used a vector (it could be a separate subclass
instead). I could also have added a simpler version that just tracked the
response rate without a window (which was my original plan). Having that would
have allowed me to start getting the infrastructure in place to track this data
somewhere real.

Anyway, I'll just get rid of this TODO for now and decide what to do next later
:)

Taylor Brandstetter

lgtm https://codereview.webrtc.org/2722933002/diff/180001/webrtc/p2p/base/packetlossestimator.h File webrtc/p2p/base/packetlossestimator.h (right): https://codereview.webrtc.org/2722933002/diff/180001/webrtc/p2p/base/packetlossestimator.h#newcode59 webrtc/p2p/base/packetlossestimator.h:59: std::string TrackedPacketsString(std::size_t max) const; Append "ForTesting"?

3 years, 9 months ago (2017-03-09 00:06:43 UTC) #11

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2722933002/200001

3 years, 9 months ago (2017-03-20 17:07:45 UTC) #14

commit-bot: I haz the power

3 years, 9 months ago (2017-03-20 17:53:15 UTC) #17

Message was sent while issue was closed.

Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/external/webrtc/+/abbacbf4892b98d77a53d2a5a...

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages