Remove duplicate-handling logic from PacedSender

Remove duplicate-handling logic from PacedSender

Remove the code that handles duplicated messages from being queued for transmission in the PacedSender. This code avoids multiple retransmissions of the same packet from being scheduled at the same time. It is deemed that this code's utility is too low for its added complexity.

BUG=webrtc:7786

[eladalon, 2017-06-09 19:50:01]

eladalon@webrtc.org changed reviewers:
+ danilchap@webrtc.org, nisse@webrtc.org, sprang@webrtc.org, stefan@webrtc.org

[eladalon, 2017-06-09 19:50:02]

PTAL

[eladalon, 2017-06-12 07:42:57]

Description was changed from

==========
Remove duplicate-handling logic from PacedSender

Remove the code that handles duplicated messages from being queued for
transmission in the PacedSender. This code avoids multiple retransmissions of
the same packet from being scheduled at the same time. It is deemed that this
code's utility is too low for its added complexity.

BUG=webrtc:7786
==========

to

==========
Remove duplicate-handling logic from PacedSender

Remove the code that handles duplicated messages from being queued for
transmission in the PacedSender. This code avoids multiple retransmissions of
the same packet from being scheduled at the same time. It is deemed that this
code's utility is too low for its added complexity.

BUG=webrtc:7786
==========

[eladalon, 2017-06-12 07:42:57]

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

[eladalon, 2017-06-12 07:43:13]

Added Björn, who would also help me with the PSA.

[terelius, 2017-06-12 07:49:13]

https://codereview.webrtc.org/2929213002/diff/20001/webrtc/modules/pacing/pac...
File webrtc/modules/pacing/paced_sender_unittest.cc (right):

https://codereview.webrtc.org/2929213002/diff/20001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/paced_sender_unittest.cc:259: // Send one packet, than two
retransmissions of that packet.
s/than/then

[eladalon, 2017-06-12 08:08:14]

https://codereview.webrtc.org/2929213002/diff/20001/webrtc/modules/pacing/pac...
File webrtc/modules/pacing/paced_sender_unittest.cc (right):

https://codereview.webrtc.org/2929213002/diff/20001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/paced_sender_unittest.cc:259: // Send one packet, than two
retransmissions of that packet.
On 2017/06/12 07:49:12, terelius wrote:
> s/than/then

Done.

[danilchap, 2017-06-12 09:21:52]

Did you check how little this feature is used (e.g. by running loopback with
packet loss)?

[nisse-webrtc, 2017-06-13 08:26:29]

lgtm

[nisse-webrtc, 2017-06-13 08:29:13]

On 2017/06/13 08:26:29, nisse-webrtc wrote:
> lgtm

In the cl description, it would be nice with some brief explanation of why this
is no longer needed. As I understood the discussion, it's irrelevant when using
rtx, which is the common case, and for non-rtx, there's logic elsewhere to limit
retransmissions of the same packet to once per rtt.

[terelius, 2017-06-13 14:05:45]

lgtm

[eladalon, 2017-06-13 14:07:51]

On 2017/06/13 14:05:45, terelius wrote:
> lgtm

Niels, Erik has written a detailed description in the bug-report (webrtc:7786),
and that bug is referenced in the CL description. Is that enough?

[sprang_webrtc, 2017-06-13 14:23:24]

lgtm if you've done a short test to verify perf on lossy link with no rtx, like
the full stack screenshare test with lossy link modified to not use rtx

[nisse-webrtc, 2017-06-14 09:04:16]

On 2017/06/13 14:07:51, eladalon wrote:
> On 2017/06/13 14:05:45, terelius wrote:
> > lgtm
> 
> Niels, Erik has written a detailed description in the bug-report
(webrtc:7786),
> and that bug is referenced in the CL description. Is that enough?

That description is less concrete than the discussion I overheard in the office.

So I still think it would be nice with a brief explanation in the cl
description, referring
explicitly to the rtx case (which is unaffected) and the class responsible for
the 
one retransmission-per-rtt limit (not sure which one that is).

[sprang_webrtc, 2017-06-14 09:10:47]

On 2017/06/14 09:04:16, nisse-webrtc wrote:
> On 2017/06/13 14:07:51, eladalon wrote:
> > On 2017/06/13 14:05:45, terelius wrote:
> > > lgtm
> > 
> > Niels, Erik has written a detailed description in the bug-report
> (webrtc:7786),
> > and that bug is referenced in the CL description. Is that enough?
> 
> That description is less concrete than the discussion I overheard in the
office.
> 
> So I still think it would be nice with a brief explanation in the cl
> description, referring
> explicitly to the rtx case (which is unaffected) and the class responsible for
> the 
> one retransmission-per-rtt limit (not sure which one that is).

That would be RtpPacketHistory::GetPacketAndSetSendTime()
Which I think also checks that we don't retransmit a packet before
actually sending it in the first place.

[eladalon, 2017-06-14 09:31:26]

I just want to chirp in to let everyone know that I'm not ignoring any of this,
I'm just archiving it for later. Right now I'm concentrating on the
receiver-side. Thanks for all your (past as well as future) input.