Cast: Avoid retransmit if we sent the same packet recently (less than RTT)

Cast: Avoid retransmit if we sent the same packet recently (less than RTT)

When transmitting 4Mbit over the "bad" udp_proxy profile, this reduces the
amount of data sent from 12Mbit/s to 7Mbit/s, and seems to make it able to
actually send more frames across the wire and recover faster.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278774

[Alpha Left Google, 2014-06-18 01:43:32]

https://codereview.chromium.org/343523005/diff/40001/media/cast/transport/cas...
File media/cast/transport/cast_transport_sender.h (right):

https://codereview.chromium.org/343523005/diff/40001/media/cast/transport/cas...
media/cast/transport/cast_transport_sender.h:100: virtual void ResendPackets(
nit: Need a comment to explain what RTT does and what value should be used.

https://codereview.chromium.org/343523005/diff/40001/media/cast/transport/pac...
File media/cast/transport/pacing/paced_sender.cc (right):

https://codereview.chromium.org/343523005/diff/40001/media/cast/transport/pac...
media/cast/transport/pacing/paced_sender.cc:78: VLOG(0) <<
"RESENDPACKETS!!!!!!!";
This shouldn't be there.

https://codereview.chromium.org/343523005/diff/40001/media/cast/transport/pac...
media/cast/transport/pacing/paced_sender.cc:88: // TODO(hubbe): Log this.
We should log a PACKET_RTX_REJECTED event in the log.

See here:
https://code.google.com/p/chromium/codesearch#chromium/src/media/cast/logging...

It is important to evaluate the impact of this change from logs. Once we have a
good and proven algorithm then we can drop the event later.

https://codereview.chromium.org/343523005/diff/40001/media/cast/transport/pac...
media/cast/transport/pacing/paced_sender.cc:231: if (sent_time_buffer_.size() >
kMaxBurstSize * 1000 / kPacingIntervalMs) {
nit: Define 1000 as a constant at the top of this file.

https://codereview.chromium.org/343523005/diff/40001/media/cast/transport/pac...
media/cast/transport/pacing/paced_sender.cc:235: state_ = State_Unblocked;
Please put a DCHECK for the sizes of the sent_time_ and sent_time_buffer_.

https://codereview.chromium.org/343523005/diff/40001/media/cast/transport/pac...
File media/cast/transport/pacing/paced_sender.h (right):

https://codereview.chromium.org/343523005/diff/40001/media/cast/transport/pac...
media/cast/transport/pacing/paced_sender.h:123: std::map<PacketKey,
base::TimeTicks> sent_time_;
nit: There should be a typedef.

https://codereview.chromium.org/343523005/diff/40001/media/cast/transport/pac...
File media/cast/transport/pacing/paced_sender_unittest.cc (right):

https://codereview.chromium.org/343523005/diff/40001/media/cast/transport/pac...
media/cast/transport/pacing/paced_sender_unittest.cc:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
There should be a test for the behavior change caused by the rtt parameter.

https://codereview.chromium.org/343523005/diff/40001/media/cast/video_sender/...
File media/cast/video_sender/video_sender.cc (right):

https://codereview.chromium.org/343523005/diff/40001/media/cast/video_sender/...
media/cast/video_sender/video_sender.cc:395: false, avg_rtt);
We use rtt in other places why using avg_rtt here?

https://codereview.chromium.org/343523005/diff/60001/media/cast/audio_sender/...
File media/cast/audio_sender/audio_sender.cc (right):

https://codereview.chromium.org/343523005/diff/60001/media/cast/audio_sender/...
media/cast/audio_sender/audio_sender.cc:119: true, missing_frames_and_packets,
false, min_rtt);
In VideoSender we used rtt and avg_rtt while min_rtt is used here. They should
be consistent.

[miu, 2014-06-18 02:44:15]

Drive by...

https://codereview.chromium.org/343523005/diff/60001/media/cast/transport/cas...
File media/cast/transport/cast_transport_sender.h (right):

https://codereview.chromium.org/343523005/diff/60001/media/cast/transport/cas...
media/cast/transport/cast_transport_sender.h:104: base::TimeDelta rtt) = 0;
I don't think "rtt" is the right name for this argument.  Something like
"dedupe_window" would be better, IMHO.  ...and ditto everywhere else...

[hubbe, 2014-06-18 20:22:28]

PTAL

https://codereview.chromium.org/343523005/diff/40001/media/cast/transport/cas...
File media/cast/transport/cast_transport_sender.h (right):

https://codereview.chromium.org/343523005/diff/40001/media/cast/transport/cas...
media/cast/transport/cast_transport_sender.h:100: virtual void ResendPackets(
On 2014/06/18 01:43:32, Alpha wrote:
> nit: Need a comment to explain what RTT does and what value should be used.

Done.

https://codereview.chromium.org/343523005/diff/40001/media/cast/transport/pac...
File media/cast/transport/pacing/paced_sender.cc (right):

https://codereview.chromium.org/343523005/diff/40001/media/cast/transport/pac...
media/cast/transport/pacing/paced_sender.cc:78: VLOG(0) <<
"RESENDPACKETS!!!!!!!";
On 2014/06/18 01:43:32, Alpha wrote:
> This shouldn't be there.

Already gone.

https://codereview.chromium.org/343523005/diff/40001/media/cast/transport/pac...
media/cast/transport/pacing/paced_sender.cc:88: // TODO(hubbe): Log this.
On 2014/06/18 01:43:32, Alpha wrote:
> We should log a PACKET_RTX_REJECTED event in the log.
> 
> See here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/cast/logging...
> 
> It is important to evaluate the impact of this change from logs. Once we have
a
> good and proven algorithm then we can drop the event later.

Done.

https://codereview.chromium.org/343523005/diff/40001/media/cast/transport/pac...
media/cast/transport/pacing/paced_sender.cc:231: if (sent_time_buffer_.size() >
kMaxBurstSize * 1000 / kPacingIntervalMs) {
On 2014/06/18 01:43:32, Alpha wrote:
> nit: Define 1000 as a constant at the top of this file.

It's just the number of milliseconds in a second.
Do we really need a constant for that?

https://codereview.chromium.org/343523005/diff/40001/media/cast/transport/pac...
media/cast/transport/pacing/paced_sender.cc:235: state_ = State_Unblocked;
On 2014/06/18 01:43:32, Alpha wrote:
> Please put a DCHECK for the sizes of the sent_time_ and sent_time_buffer_.

Done.

https://codereview.chromium.org/343523005/diff/40001/media/cast/transport/pac...
File media/cast/transport/pacing/paced_sender.h (right):

https://codereview.chromium.org/343523005/diff/40001/media/cast/transport/pac...
media/cast/transport/pacing/paced_sender.h:123: std::map<PacketKey,
base::TimeTicks> sent_time_;
On 2014/06/18 01:43:32, Alpha wrote:
> nit: There should be a typedef.

Why?

https://codereview.chromium.org/343523005/diff/40001/media/cast/video_sender/...
File media/cast/video_sender/video_sender.cc (right):

https://codereview.chromium.org/343523005/diff/40001/media/cast/video_sender/...
media/cast/video_sender/video_sender.cc:395: false, avg_rtt);
On 2014/06/18 01:43:32, Alpha wrote:
> We use rtt in other places why using avg_rtt here?

Done.

https://codereview.chromium.org/343523005/diff/60001/media/cast/audio_sender/...
File media/cast/audio_sender/audio_sender.cc (right):

https://codereview.chromium.org/343523005/diff/60001/media/cast/audio_sender/...
media/cast/audio_sender/audio_sender.cc:119: true, missing_frames_and_packets,
false, min_rtt);
On 2014/06/18 01:43:32, Alpha wrote:
> In VideoSender we used rtt and avg_rtt while min_rtt is used here. They should
> be consistent.

Done.

https://codereview.chromium.org/343523005/diff/60001/media/cast/transport/cas...
File media/cast/transport/cast_transport_sender.h (right):

https://codereview.chromium.org/343523005/diff/60001/media/cast/transport/cas...
media/cast/transport/cast_transport_sender.h:104: base::TimeDelta rtt) = 0;
On 2014/06/18 02:44:15, miu wrote:
> I don't think "rtt" is the right name for this argument.  Something like
> "dedupe_window" would be better, IMHO.  ...and ditto everywhere else...

Done.

[Alpha Left Google, 2014-06-18 20:39:15]

LGTM. It's up to you for the nit.

https://codereview.chromium.org/343523005/diff/40001/media/cast/transport/pac...
File media/cast/transport/pacing/paced_sender.h (right):

https://codereview.chromium.org/343523005/diff/40001/media/cast/transport/pac...
media/cast/transport/pacing/paced_sender.h:123: std::map<PacketKey,
base::TimeTicks> sent_time_;
On 2014/06/18 20:22:28, hubbe wrote:
> On 2014/06/18 01:43:32, Alpha wrote:
> > nit: There should be a typedef.
> 
> Why?

There's once place that does "std::map<PacketKey,
base::TimeTicks>::const_iterator". It's up to you if you think this is minor.

[hubbe, 2014-06-18 20:40:36]

Adding palmer for security review.

[Alpha Left Google, 2014-06-18 21:07:37]

https://codereview.chromium.org/343523005/diff/80001/media/cast/audio_sender/...
File media/cast/audio_sender/audio_sender.cc (right):

https://codereview.chromium.org/343523005/diff/80001/media/cast/audio_sender/...
media/cast/audio_sender/audio_sender.cc:119: true, missing_frames_and_packets,
false, rtt);
Sorry one last thing I missed in the last review. I think it is best to leave
audio RTX untouched. Audio packets don't degrade performance as bad as video RTX
packets. And receiver currently only NACK once that means audio packet has at
most 2 chance to send. Removing the resend can lead to only once chance to send.

[palmer, 2014-06-18 21:18:02]

IPC security LGTM.

https://codereview.chromium.org/343523005/diff/80001/media/cast/audio_sender/...
File media/cast/audio_sender/audio_sender.cc (right):

https://codereview.chromium.org/343523005/diff/80001/media/cast/audio_sender/...
media/cast/audio_sender/audio_sender.cc:117: // rather than the min.
File a bug to study this, so it doesn't get forgotten. (Maybe you already have
done so.) Then reference the bug # here.

[hubbe, 2014-06-18 21:20:35]

https://codereview.chromium.org/343523005/diff/80001/media/cast/audio_sender/...
File media/cast/audio_sender/audio_sender.cc (right):

https://codereview.chromium.org/343523005/diff/80001/media/cast/audio_sender/...
media/cast/audio_sender/audio_sender.cc:117: // rather than the min.
On 2014/06/18 21:18:02, Chromium Palmer wrote:
> File a bug to study this, so it doesn't get forgotten. (Maybe you already have
> done so.) Then reference the bug # here.

No real need, alpha is deep in that study right now..

https://codereview.chromium.org/343523005/diff/80001/media/cast/audio_sender/...
media/cast/audio_sender/audio_sender.cc:119: true, missing_frames_and_packets,
false, rtt);
On 2014/06/18 21:07:37, Alpha wrote:
> Sorry one last thing I missed in the last review. I think it is best to leave
> audio RTX untouched. Audio packets don't degrade performance as bad as video
RTX
> packets. And receiver currently only NACK once that means audio packet has at
> most 2 chance to send. Removing the resend can lead to only once chance to
send.

Changed back to min_rtt

[hubbe, 2014-06-18 21:20:39]

The CQ bit was checked by hubbe@chromium.org

[commit-bot: I haz the power, 2014-06-18 21:22:46]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hubbe@chromium.org/343523005/140001

[commit-bot: I haz the power, 2014-06-19 10:12:36]

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

[commit-bot: I haz the power, 2014-06-19 10:12:37]

Try jobs failed on following builders:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[hubbe, 2014-06-19 20:45:49]

The CQ bit was checked by hubbe@chromium.org

[commit-bot: I haz the power, 2014-06-19 20:47:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hubbe@chromium.org/343523005/160001

[commit-bot: I haz the power, 2014-06-20 00:12:57]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/19222)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/22688)

[commit-bot: I haz the power, 2014-06-20 00:15:34]

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

[commit-bot: I haz the power, 2014-06-20 00:15:35]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/19263)

[hubbe, 2014-06-20 00:46:04]

The CQ bit was checked by hubbe@chromium.org

[commit-bot: I haz the power, 2014-06-20 00:50:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hubbe@chromium.org/343523005/180001

[commit-bot: I haz the power, 2014-06-20 05:45:55]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[hubbe, 2014-06-20 05:49:25]

The CQ bit was checked by hubbe@chromium.org

[commit-bot: I haz the power, 2014-06-20 05:52:00]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hubbe@chromium.org/343523005/200001

[hubbe, 2014-06-20 08:01:40]

The CQ bit was checked by hubbe@chromium.org

[commit-bot: I haz the power, 2014-06-20 08:04:44]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hubbe@chromium.org/343523005/220001

[commit-bot: I haz the power, 2014-06-20 12:16:16]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-06-20 13:04:53]

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

[commit-bot: I haz the power, 2014-06-20 13:04:54]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[hubbe, 2014-06-20 16:07:25]

The CQ bit was checked by hubbe@chromium.org

[commit-bot: I haz the power, 2014-06-20 16:09:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hubbe@chromium.org/343523005/220001

[commit-bot: I haz the power, 2014-06-20 17:12:13]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-06-20 18:23:18]

Change committed as 278774