Cast: First stab at implementing adaptive latency

Cast: First stab at implementing adaptive latency

After a bunch of testing, it seems like there is a direct linear
relationship between RTT and the optimal playout delay. Packet loss
is also a factor, but since we expect packet loss to be low in
normal networks, perhaps we can make the formula as easy as RTT * 4.

This is disabled by default, the extension needs to set min_latency_ms
in order to activate it.

BUG=405339
Committed: https://crrev.com/a7dbb823c2d7d5cfc7e310aca7acd6efd8e4a009
Cr-Commit-Position: refs/heads/master@{#294655}

[hubbe, 2014-09-10 19:39:29]

hubbe@chromium.org changed reviewers:
+ hclam@chromium.org

[miu, 2014-09-10 19:50:07]

miu@chromium.org changed reviewers:
+ miu@chromium.org

[miu, 2014-09-10 19:50:08]

Drive by!  :)

https://codereview.chromium.org/562653004/diff/1/media/cast/sender/frame_send...
File media/cast/sender/frame_sender.cc (right):

https://codereview.chromium.org/562653004/diff/1/media/cast/sender/frame_send...
media/cast/sender/frame_sender.cc:15: const size_t kRTTWindow = 8192;
This is a pretty huge window.  If RTT is updated every 1/2 second, this is 4096
seconds of history.

https://codereview.chromium.org/562653004/diff/1/media/cast/sender/frame_send...
media/cast/sender/frame_sender.cc:245: }
BTW--As I was poking around code recently, I noticed that there might be a
missing statement here:

  send_target_playout_delay_ = false;

[hubbe, 2014-09-10 20:40:47]

https://codereview.chromium.org/562653004/diff/1/media/cast/sender/frame_send...
File media/cast/sender/frame_sender.cc (right):

https://codereview.chromium.org/562653004/diff/1/media/cast/sender/frame_send...
media/cast/sender/frame_sender.cc:15: const size_t kRTTWindow = 8192;
On 2014/09/10 19:50:08, miu wrote:
> This is a pretty huge window.  If RTT is updated every 1/2 second, this is
4096
> seconds of history.

changed to 1200 (10 minutes)

https://codereview.chromium.org/562653004/diff/1/media/cast/sender/frame_send...
media/cast/sender/frame_sender.cc:245: }
On 2014/09/10 19:50:08, miu wrote:
> BTW--As I was poking around code recently, I noticed that there might be a
> missing statement here:
> 
>   send_target_playout_delay_ = false;
> 

It's intentional.
If we allow frame dropping, then there is no guarantee that setting this on a
single packet will have any effect, so instead we set this on *every* packet.

[Alpha Left Google, 2014-09-11 01:24:26]

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

https://codereview.chromium.org/562653004/diff/40001/media/cast/sender/video_...
media/cast/sender/video_sender.cc:22: const int kRoundTripsNeeded = 4;
nit: This constant needs to be more descriptive or a comment to explain what it
does. My first glance I'm thinking "round trips needed" for what?

https://codereview.chromium.org/562653004/diff/80001/media/cast/sender/video_...
File media/cast/sender/video_sender.cc (right):

https://codereview.chromium.org/562653004/diff/80001/media/cast/sender/video_...
media/cast/sender/video_sender.cc:138: current_round_trip_time_ *
kRoundTripsNeeded,
Have you considered applying a low pass filter? Or rate limit the frequency we
change the playout delay?

[hubbe, 2014-09-11 19:17:19]

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

https://codereview.chromium.org/562653004/diff/40001/media/cast/sender/video_...
media/cast/sender/video_sender.cc:22: const int kRoundTripsNeeded = 4;
On 2014/09/11 01:24:26, Alpha wrote:
> nit: This constant needs to be more descriptive or a comment to explain what
it
> does. My first glance I'm thinking "round trips needed" for what?

I actually think the name is pretty good (but I'm open to suggestions)
I added some comments to provide context instead.

https://codereview.chromium.org/562653004/diff/80001/media/cast/sender/video_...
File media/cast/sender/video_sender.cc (right):

https://codereview.chromium.org/562653004/diff/80001/media/cast/sender/video_...
media/cast/sender/video_sender.cc:138: current_round_trip_time_ *
kRoundTripsNeeded,
On 2014/09/11 01:24:26, Alpha wrote:
> Have you considered applying a low pass filter? Or rate limit the frequency we
> change the playout delay?

My original upload used a sliding window of RTT reports for this. However, in my
manual testing(*), I found that this was unhelpful as it has the effect of
stepping up the delay slowly over time, which causes lots of skipped frames
until it stabilizes.

Using the latest rtt tends to directly jump to a larger, but still fairly
reasonable value, which means that it stabilizes faster. In real life, this
could be too aggressive, but we really need more testing to find out.

As for rate limiting: If rate limiting is needed, that just means that the
formula is not aggressive enough in raising the delay. Ideally, this should only
happen *once*, or when network conditions becomes significantly worse, so why
would we need any rate limiting?

* I tested setting min delay to 10 ms and max delay to 1000 ms and  ran it
through the tap_proxy to see how it adjusts.

[Alpha Left Google, 2014-09-11 20:33:44]

Please fix the comment. Then LGTM. We should discuss how to roll this out to
users and evaluate the impact.

https://codereview.chromium.org/562653004/diff/100001/media/cast/sender/video...
File media/cast/sender/video_sender.cc (right):

https://codereview.chromium.org/562653004/diff/100001/media/cast/sender/video...
media/cast/sender/video_sender.cc:28: // This is an estimate of all the the
constant time needed
There's an extra "needed" here. Please remove.

[hubbe, 2014-09-11 20:47:13]

https://codereview.chromium.org/562653004/diff/100001/media/cast/sender/video...
File media/cast/sender/video_sender.cc (right):

https://codereview.chromium.org/562653004/diff/100001/media/cast/sender/video...
media/cast/sender/video_sender.cc:28: // This is an estimate of all the the
constant time needed
On 2014/09/11 20:33:44, Alpha wrote:
> There's an extra "needed" here. Please remove.

Done.

[hubbe, 2014-09-11 23:42:27]

The CQ bit was checked by hubbe@chromium.org

[commit-bot: I haz the power, 2014-09-12 00:14:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/562653004/120001

[commit-bot: I haz the power, 2014-09-12 01:04:19]

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

[commit-bot: I haz the power, 2014-09-12 01:04:21]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[hubbe, 2014-09-12 17:47:18]

hubbe@chromium.org changed reviewers:
+ yoz@chromium.org

[hubbe, 2014-09-12 17:47:19]

+yoz for extensions approval

[Yoyo Zhou, 2014-09-12 18:32:42]

LGTM for extensions

https://codereview.chromium.org/562653004/diff/120001/chrome/renderer/media/c...
File chrome/renderer/media/cast_rtp_stream.h (right):

https://codereview.chromium.org/562653004/diff/120001/chrome/renderer/media/c...
chrome/renderer/media/cast_rtp_stream.h:45: // Minimum latency.
Comment on default value?

[hubbe, 2014-09-12 18:41:15]

https://codereview.chromium.org/562653004/diff/120001/chrome/renderer/media/c...
File chrome/renderer/media/cast_rtp_stream.h (right):

https://codereview.chromium.org/562653004/diff/120001/chrome/renderer/media/c...
chrome/renderer/media/cast_rtp_stream.h:45: // Minimum latency.
On 2014/09/12 18:32:42, Yoyo Zhou wrote:
> Comment on default value?

Done.

[hubbe, 2014-09-12 19:54:02]

The CQ bit was checked by hubbe@chromium.org

[commit-bot: I haz the power, 2014-09-12 19:55:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/562653004/140001

[commit-bot: I haz the power, 2014-09-12 20:56:22]

Committed patchset #8 (id:140001) as a8d9f5fc01f5f3e55e54465bc52588ecfaed9c34

[commit-bot: I haz the power, 2014-09-12 21:00:38]

Patchset 8 (id:??) landed as
https://crrev.com/a7dbb823c2d7d5cfc7e310aca7acd6efd8e4a009
Cr-Commit-Position: refs/heads/master@{#294655}