Don't suspend the pipeline before HaveFutureData while decoding progressing

Stop suspending the pipeline before HaveFutureData while decoding

Previously we could suspend the pipeline before HaveFutureData but
after the media had been completely loaded, relying on the assumption
that we could use the signal of didLoadingProgress() to resume and check
if the new data got us to HaveFutureData. However, if the media is
completely downloaded, there will be no more didLoadingProgress() calls
which meant we could leave the player suspended forever.

Now we check whether loading is stalled (didLoadingProgress() has not
returned true for least 3 seconds) before suspending before
HaveFutureData. If loading is stalled but we haven't reached
HaveFutureData we assume that the pipeline is waiting for more data to
complete preroll.

BUG=655630
TEST=new tests

Committed: https://crrev.com/d026f79cc5949df6bae399f58128b470f8061ad7
Cr-Commit-Position: refs/heads/master@{#430086}

[watk, 2016-10-21 23:52:34]

watk@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[watk, 2016-10-21 23:52:34]

Tests coming. Just wanted to make sure you liked the approach. I don't really
like it tbh, but it seems fine for now at least.

[watk, 2016-10-28 01:23:55]

Description was changed from

==========
Stop suspending the pipeline before HaveFutureData while decoding

Previously we could suspend the pipeline before HaveFutureData but
after the media had been completely loaded, relying on the assumption
that we could use the signal of didLoadingProgress() to resume and check
if the new data got us to HaveFutureData. However, if the media is
completely downloaded, there will be no more didLoadingProgress() calls
which meant we could leave the player suspended forever.

Now we check whether decoding is stalled (no decodes have completed for
at least 3 seconds) before suspending before HaveFutureData. If decoding
is stalled, it's relatively safe to suspend because the loading of more
data will resume the pipeline. If decoding is not stalled, we shouldn't
suspend to avoid the above. Using a 3 second timeout is fragile in
theory, because decodes that take longer than 3 seconds could trigger it.
But in practice it should very rarely occur.

BUG=655630
==========

to

==========
Stop suspending the pipeline before HaveFutureData while decoding

Previously we could suspend the pipeline before HaveFutureData but
after the media had been completely loaded, relying on the assumption
that we could use the signal of didLoadingProgress() to resume and check
if the new data got us to HaveFutureData. However, if the media is
completely downloaded, there will be no more didLoadingProgress() calls
which meant we could leave the player suspended forever.

Now we check whether loading is stalled (didLoadingProgress() has not
returned true for least 3 seconds) before suspending before
HaveFutureData. If loading is stalled but we haven't reached
HaveFutureData we assume that the pipeline is waiting for more data to
complete preroll.

BUG=655630
==========

[watk, 2016-10-28 01:23:55]

watk@chromium.org changed reviewers:
+ sandersd@chromium.org

[watk, 2016-10-28 01:25:57]

PTAL. Added sandersd to make sure he likes the updateplaystate stuff.

I'm not sure about 3 seconds. If a file is loaded immediately and we only emit
one progress event, I wonder if 3 seconds is enough time for preroll to
complete?

[DaleCurtis, 2016-10-28 19:11:42]

https://codereview.chromium.org/2445533002/diff/20001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/2445533002/diff/20001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:1332: // Before HaveFutureData blink will not
call play(), so we must be careful to
This will never be called again. So even after 3 seconds elapse the player won't
be suspended. We need to PostDelayTask this check for another few seconds in the
future or change this method from setting is_idle_ to setting something like
suspend_requested_ when < have_future_data. The didLoadingProgress() callback
can then schedule the idle suspend.

https://codereview.chromium.org/2445533002/diff/20001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.h (right):

https://codereview.chromium.org/2445533002/diff/20001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.h:571: base::TickClock* tick_clock_;
Nowadays I like just having a single std::unique_ptr<base::TickClock> member
that a test can set.

[watk, 2016-10-28 20:03:23]

https://codereview.chromium.org/2445533002/diff/20001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/2445533002/diff/20001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:1332: // Before HaveFutureData blink will not
call play(), so we must be careful to
On 2016/10/28 19:11:42, DaleCurtis wrote:
> This will never be called again. So even after 3 seconds elapse the player
won't
> be suspended. We need to PostDelayTask this check for another few seconds in
the
> future or change this method from setting is_idle_ to setting something like
> suspend_requested_ when < have_future_data. The didLoadingProgress() callback
> can then schedule the idle suspend.

What about calling delegate_->DidPause()? Then the delegate can try to suspend
us again later.

[watk, 2016-11-02 20:33:03]

The CQ bit was checked by watk@chromium.org to run a CQ dry run

[watk, 2016-11-02 20:33:17]

Patchset #3 (id:40001) has been deleted

[commit-bot: I haz the power, 2016-11-02 20:34:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[watk, 2016-11-02 20:35:00]

The CQ bit was checked by watk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 20:35:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[watk, 2016-11-02 20:35:48]

PTAL

[commit-bot: I haz the power, 2016-11-02 21:02:47]

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

[commit-bot: I haz the power, 2016-11-02 21:02:49]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[watk, 2016-11-02 21:05:11]

The CQ bit was checked by watk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 21:05:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[watk, 2016-11-02 21:11:02]

The delegate test refactor has made the diff kinda messy. Sorry about that. I
can put it in a new CL if desired.

[commit-bot: I haz the power, 2016-11-03 00:01:21]

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

[commit-bot: I haz the power, 2016-11-03 00:01:21]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[sandersd (OOO until July 31), 2016-11-03 18:01:58]

lgtm.

https://codereview.chromium.org/2445533002/diff/60001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/2445533002/diff/60001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:1340: base::TimeDelta::FromSeconds(3)) {
Extract this constant?

[DaleCurtis, 2016-11-03 18:05:48]

lgtm

https://codereview.chromium.org/2445533002/diff/80001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.h (right):

https://codereview.chromium.org/2445533002/diff/80001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.h:570: // Points to |default_tick_clock_| by
default, but can be overridden by tests.
Just have a single std::unique_ptr<base::TickClock> member that a test can set?

[watk, 2016-11-03 19:21:58]

Patchset #5 (id:100001) has been deleted

[watk, 2016-11-03 19:25:02]

https://codereview.chromium.org/2445533002/diff/60001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/2445533002/diff/60001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:1340: base::TimeDelta::FromSeconds(3)) {
On 2016/11/03 18:01:58, sandersd wrote:
> Extract this constant?

Done. Hope you're ok with the name kLoadingToIdleTimeout.

https://codereview.chromium.org/2445533002/diff/80001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.h (right):

https://codereview.chromium.org/2445533002/diff/80001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.h:570: // Points to |default_tick_clock_| by
default, but can be overridden by tests.
On 2016/11/03 18:05:47, DaleCurtis wrote:
> Just have a single std::unique_ptr<base::TickClock> member that a test can
set?

oops, done.

[watk, 2016-11-03 19:25:54]

Description was changed from

==========
Stop suspending the pipeline before HaveFutureData while decoding

Previously we could suspend the pipeline before HaveFutureData but
after the media had been completely loaded, relying on the assumption
that we could use the signal of didLoadingProgress() to resume and check
if the new data got us to HaveFutureData. However, if the media is
completely downloaded, there will be no more didLoadingProgress() calls
which meant we could leave the player suspended forever.

Now we check whether loading is stalled (didLoadingProgress() has not
returned true for least 3 seconds) before suspending before
HaveFutureData. If loading is stalled but we haven't reached
HaveFutureData we assume that the pipeline is waiting for more data to
complete preroll.

BUG=655630
==========

to

==========
Stop suspending the pipeline before HaveFutureData while decoding

Previously we could suspend the pipeline before HaveFutureData but
after the media had been completely loaded, relying on the assumption
that we could use the signal of didLoadingProgress() to resume and check
if the new data got us to HaveFutureData. However, if the media is
completely downloaded, there will be no more didLoadingProgress() calls
which meant we could leave the player suspended forever.

Now we check whether loading is stalled (didLoadingProgress() has not
returned true for least 3 seconds) before suspending before
HaveFutureData. If loading is stalled but we haven't reached
HaveFutureData we assume that the pipeline is waiting for more data to
complete preroll.

BUG=655630
TEST=new tests
==========

[watk, 2016-11-03 19:26:03]

The CQ bit was checked by watk@chromium.org

[watk, 2016-11-03 19:26:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
dalecurtis@chromium.org, sandersd@chromium.org
Link to the patchset: https://codereview.chromium.org/2445533002/#ps120001
(title: "comments")

[commit-bot: I haz the power, 2016-11-03 19:26:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-03 22:00:40]

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

[commit-bot: I haz the power, 2016-11-03 22:00:40]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[watk, 2016-11-04 19:14:49]

The CQ bit was checked by watk@chromium.org

[commit-bot: I haz the power, 2016-11-04 19:15:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-04 20:00:55]

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

[commit-bot: I haz the power, 2016-11-04 20:00:56]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[watk, 2016-11-04 20:39:11]

The CQ bit was checked by watk@chromium.org

[watk, 2016-11-04 20:39:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
dalecurtis@chromium.org, sandersd@chromium.org
Link to the patchset: https://codereview.chromium.org/2445533002/#ps140001
(title: "fix compiler error from rebase")

[commit-bot: I haz the power, 2016-11-04 20:40:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-04 22:21:05]

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

[commit-bot: I haz the power, 2016-11-04 22:21:08]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[watk, 2016-11-04 22:21:43]

The CQ bit was checked by watk@chromium.org

[commit-bot: I haz the power, 2016-11-04 22:22:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-04 23:11:41]

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

[commit-bot: I haz the power, 2016-11-04 23:11:42]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[watk, 2016-11-04 23:13:02]

The CQ bit was checked by watk@chromium.org

[commit-bot: I haz the power, 2016-11-04 23:13:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-05 00:29:37]

Description was changed from

==========
Stop suspending the pipeline before HaveFutureData while decoding

Previously we could suspend the pipeline before HaveFutureData but
after the media had been completely loaded, relying on the assumption
that we could use the signal of didLoadingProgress() to resume and check
if the new data got us to HaveFutureData. However, if the media is
completely downloaded, there will be no more didLoadingProgress() calls
which meant we could leave the player suspended forever.

Now we check whether loading is stalled (didLoadingProgress() has not
returned true for least 3 seconds) before suspending before
HaveFutureData. If loading is stalled but we haven't reached
HaveFutureData we assume that the pipeline is waiting for more data to
complete preroll.

BUG=655630
TEST=new tests
==========

to

==========
Stop suspending the pipeline before HaveFutureData while decoding

Previously we could suspend the pipeline before HaveFutureData but
after the media had been completely loaded, relying on the assumption
that we could use the signal of didLoadingProgress() to resume and check
if the new data got us to HaveFutureData. However, if the media is
completely downloaded, there will be no more didLoadingProgress() calls
which meant we could leave the player suspended forever.

Now we check whether loading is stalled (didLoadingProgress() has not
returned true for least 3 seconds) before suspending before
HaveFutureData. If loading is stalled but we haven't reached
HaveFutureData we assume that the pipeline is waiting for more data to
complete preroll.

BUG=655630
TEST=new tests
==========

[commit-bot: I haz the power, 2016-11-05 00:29:38]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2016-11-05 00:33:08]

Description was changed from

==========
Stop suspending the pipeline before HaveFutureData while decoding

Previously we could suspend the pipeline before HaveFutureData but
after the media had been completely loaded, relying on the assumption
that we could use the signal of didLoadingProgress() to resume and check
if the new data got us to HaveFutureData. However, if the media is
completely downloaded, there will be no more didLoadingProgress() calls
which meant we could leave the player suspended forever.

Now we check whether loading is stalled (didLoadingProgress() has not
returned true for least 3 seconds) before suspending before
HaveFutureData. If loading is stalled but we haven't reached
HaveFutureData we assume that the pipeline is waiting for more data to
complete preroll.

BUG=655630
TEST=new tests
==========

to

==========
Stop suspending the pipeline before HaveFutureData while decoding

Previously we could suspend the pipeline before HaveFutureData but
after the media had been completely loaded, relying on the assumption
that we could use the signal of didLoadingProgress() to resume and check
if the new data got us to HaveFutureData. However, if the media is
completely downloaded, there will be no more didLoadingProgress() calls
which meant we could leave the player suspended forever.

Now we check whether loading is stalled (didLoadingProgress() has not
returned true for least 3 seconds) before suspending before
HaveFutureData. If loading is stalled but we haven't reached
HaveFutureData we assume that the pipeline is waiting for more data to
complete preroll.

BUG=655630
TEST=new tests

Committed: https://crrev.com/d026f79cc5949df6bae399f58128b470f8061ad7
Cr-Commit-Position: refs/heads/master@{#430086}
==========

[commit-bot: I haz the power, 2016-11-05 00:33:09]

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