Fire canplaythrough as soon as download defers to fix autoplay

Fire canplaythrough as soon as download defers to fix autoplay

Reenables delayed firing of canplaythrough for media elements, and fixes
the bug that had been introduced where a video with autoplay=true sometimes
never starts. With this change, a video with autoplay=true should always
(though not necessarily immediately) start playback on its own.

BUG=106480, 73609
TEST=media_unitests, manually checking video files in various conditions

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

[vrk (LEFT CHROMIUM), 2012-01-05 21:18:22]

Tentatively re-enabling the delayed firing of the canplaythrough event, with an
attempt to fix the autoplay issue.

If there are any more problems after this, I'll perma-disable that
canplaythrough change!

[acolwell GONE FROM CHROMIUM, 2012-01-05 22:33:55]

http://codereview.chromium.org/9113023/diff/1/media/base/download_rate_monito...
File media/base/download_rate_monitor_unittest.cc (left):

http://codereview.chromium.org/9113023/diff/1/media/base/download_rate_monito...
media/base/download_rate_monitor_unittest.cc:80: TEST_F(DownloadRateMonitorTest,
DownloadRateGreaterThanBitrate_Pause) {
Based on what the comment sayd, I think this test case is still valid and we
should keep it.

http://codereview.chromium.org/9113023/diff/1/media/base/download_rate_monito...
File media/base/download_rate_monitor_unittest.cc (right):

http://codereview.chromium.org/9113023/diff/1/media/base/download_rate_monito...
media/base/download_rate_monitor_unittest.cc:46:
monitor_.SetBufferedBytes(bytes_buffered, packet_time);
Consider creating a helper object that models how the XXXDataSource make calls
on host() under different scenarios. That might make it easier to describe
various things like deferral behavior and help to ensure we have everything
covered.

Obviously this only makes sense if a relatively simple model object can be
created.

http://codereview.chromium.org/9113023/diff/1/webkit/media/buffered_data_sour...
File webkit/media/buffered_data_source.cc (right):

http://codereview.chromium.org/9113023/diff/1/webkit/media/buffered_data_sour...
webkit/media/buffered_data_source.cc:601: if (host() && total_bytes_ !=
kPositionNotSpecified) {
Consider updating this code to call UpdateHostState_Locked() to help keep host()
method call order consistent.

http://codereview.chromium.org/9113023/diff/1/webkit/media/buffered_data_sour...
webkit/media/buffered_data_source.cc:637: if (is_downloading_data !=
is_downloading_data_) {
Consider updating this code to call UpdateHostState_Locked() to help keep host()
method call order consistent.

http://codereview.chromium.org/9113023/diff/1/webkit/media/buffered_data_sour...
webkit/media/buffered_data_source.cc:656: host()->SetTotalBytes(total_bytes_);
Please make sure that download_rate_monitor unit tests mimic the same order that
these three methods are called in, just to make sure we don't miss any subtle
bugs related to their call order.

http://codereview.chromium.org/9113023/diff/1/webkit/media/buffered_data_sour...
webkit/media/buffered_data_source.cc:658:
host()->SetNetworkActivity(is_downloading_data_);
Please update the BufferedDataSource unit test to check for this new behavior
and document why it is necessary.

[vrk (LEFT CHROMIUM), 2012-01-09 19:01:17]

Reworked the unit tests a bit. PTAL!

http://codereview.chromium.org/9113023/diff/1/media/base/download_rate_monito...
File media/base/download_rate_monitor_unittest.cc (left):

http://codereview.chromium.org/9113023/diff/1/media/base/download_rate_monito...
media/base/download_rate_monitor_unittest.cc:80: TEST_F(DownloadRateMonitorTest,
DownloadRateGreaterThanBitrate_Pause) {
On 2012/01/05 22:33:56, acolwell wrote:
> Based on what the comment sayd, I think this test case is still valid and we
> should keep it.

Ahhh, now I remember why I deleted this. The test is trying to catch the case
where the video is in a deferred state for a long time, and to make sure a stall
in download due to deferring doesn't hinder download speed.

However, now the DownloadRateMonitor will call CanPlayThrough as soon as you
defer, so the test doesn't really make sense anymore -- even if the download
speed estimation was flawed, CanPlayThrough already fired so we don't need the
download speed estimation anymore.

http://codereview.chromium.org/9113023/diff/1/media/base/download_rate_monito...
File media/base/download_rate_monitor_unittest.cc (right):

http://codereview.chromium.org/9113023/diff/1/media/base/download_rate_monito...
media/base/download_rate_monitor_unittest.cc:46:
monitor_.SetBufferedBytes(bytes_buffered, packet_time);
On 2012/01/05 22:33:56, acolwell wrote:
> Consider creating a helper object that models how the XXXDataSource make calls
> on host() under different scenarios. That might make it easier to describe
> various things like deferral behavior and help to ensure we have everything
> covered.
> 
> Obviously this only makes sense if a relatively simple model object can be
> created.

Made two objects, a FakeFilterHost and FakeDataSource. The test much more
accurately reflects what actually happens in the code now, and I think it's
still pretty readable. Let me know what you think.

http://codereview.chromium.org/9113023/diff/1/webkit/media/buffered_data_sour...
File webkit/media/buffered_data_source.cc (right):

http://codereview.chromium.org/9113023/diff/1/webkit/media/buffered_data_sour...
webkit/media/buffered_data_source.cc:601: if (host() && total_bytes_ !=
kPositionNotSpecified) {
On 2012/01/05 22:33:56, acolwell wrote:
> Consider updating this code to call UpdateHostState_Locked() to help keep
host()
> method call order consistent.

Done.

http://codereview.chromium.org/9113023/diff/1/webkit/media/buffered_data_sour...
webkit/media/buffered_data_source.cc:637: if (is_downloading_data !=
is_downloading_data_) {
On 2012/01/05 22:33:56, acolwell wrote:
> Consider updating this code to call UpdateHostState_Locked() to help keep
host()
> method call order consistent.

Done; I also had to change the unit tests to become more lenient about # of
times the various Set* methods got called.

http://codereview.chromium.org/9113023/diff/1/webkit/media/buffered_data_sour...
webkit/media/buffered_data_source.cc:656: host()->SetTotalBytes(total_bytes_);
On 2012/01/05 22:33:56, acolwell wrote:
> Please make sure that download_rate_monitor unit tests mimic the same order
that
> these three methods are called in, just to make sure we don't miss any subtle
> bugs related to their call order.

Done.

http://codereview.chromium.org/9113023/diff/1/webkit/media/buffered_data_sour...
webkit/media/buffered_data_source.cc:658:
host()->SetNetworkActivity(is_downloading_data_);
On 2012/01/05 22:33:56, acolwell wrote:
> Please update the BufferedDataSource unit test to check for this new behavior
> and document why it is necessary.

Done.

[acolwell GONE FROM CHROMIUM, 2012-01-09 23:16:57]

looking good. Just a little more tuning in the unit tests and I think we'll be
there.

http://codereview.chromium.org/9113023/diff/8001/media/base/download_rate_mon...
File media/base/download_rate_monitor.cc (right):

http://codereview.chromium.org/9113023/diff/8001/media/base/download_rate_mon...
media/base/download_rate_monitor.cc:94: is_downloading_data_ = true;
Do we still need this? It seems like it may trigger unexpected transitions in
SetNetworkActivity().

http://codereview.chromium.org/9113023/diff/8001/media/base/download_rate_mon...
File media/base/download_rate_monitor_unittest.cc (right):

http://codereview.chromium.org/9113023/diff/8001/media/base/download_rate_mon...
media/base/download_rate_monitor_unittest.cc:40: // network activity.
Is this last sentence true still? Isn't the return value the amount of data
"received" inside this call?

http://codereview.chromium.org/9113023/diff/8001/media/base/download_rate_mon...
media/base/download_rate_monitor_unittest.cc:41: int SimulateNetwork(double
starting_time, int bytes_per_packet,
I think this signature still makes the tests a little confusing. Don't you just
want to say, "Download x bytes at y rate?" It seems like packet size/count could
be hidden in this method. I think start_time could be a member variable that
could get incremented on each 'receive'. If we need to introduce a delay, we
could add a SimulateNetworkDelay(). What do you think?

http://codereview.chromium.org/9113023/diff/8001/media/base/download_rate_mon...
media/base/download_rate_monitor_unittest.cc:70: data_source_.reset(new
FakeDataSource(
Consider converting this to a Seek() method on FakeDataSource. I think that
would be clearer.

http://codereview.chromium.org/9113023/diff/8001/media/base/download_rate_mon...
media/base/download_rate_monitor_unittest.cc:81: class FakeFilterHost {
I think you should just put these methods in FakeDataSource. I don't think
pulling them out into a separate class makes things clearer.

http://codereview.chromium.org/9113023/diff/8001/media/base/download_rate_mon...
media/base/download_rate_monitor_unittest.cc:116: if (buffered_bytes_ >
defer_threshold_)
How about a comment stating that you are simulating a deferral here?

http://codereview.chromium.org/9113023/diff/8001/media/base/download_rate_mon...
media/base/download_rate_monitor_unittest.cc:249: 
We should also have some test simulating the SimpleDataSource & FileDataSource
cases where only SetTotalBytes() & SetBufferedBytes() are called w/o a
SetNetworkActivity().

[vrk (LEFT CHROMIUM), 2012-01-11 00:23:12]

Thanks for reviewing!

http://codereview.chromium.org/9113023/diff/8001/media/base/download_rate_mon...
File media/base/download_rate_monitor.cc (right):

http://codereview.chromium.org/9113023/diff/8001/media/base/download_rate_mon...
media/base/download_rate_monitor.cc:94: is_downloading_data_ = true;
On 2012/01/09 23:16:57, acolwell wrote:
> Do we still need this? It seems like it may trigger unexpected transitions in
> SetNetworkActivity().

You're right, this should be deleted! Done.

http://codereview.chromium.org/9113023/diff/8001/media/base/download_rate_mon...
File media/base/download_rate_monitor_unittest.cc (right):

http://codereview.chromium.org/9113023/diff/8001/media/base/download_rate_mon...
media/base/download_rate_monitor_unittest.cc:40: // network activity.
On 2012/01/09 23:16:57, acolwell wrote:
> Is this last sentence true still? Isn't the return value the amount of data
> "received" inside this call?

No longer true, but n/a now.

http://codereview.chromium.org/9113023/diff/8001/media/base/download_rate_mon...
media/base/download_rate_monitor_unittest.cc:41: int SimulateNetwork(double
starting_time, int bytes_per_packet,
On 2012/01/09 23:16:57, acolwell wrote:
> I think this signature still makes the tests a little confusing. Don't you
just
> want to say, "Download x bytes at y rate?" It seems like packet size/count
could
> be hidden in this method. I think start_time could be a member variable that
> could get incremented on each 'receive'. If we need to introduce a delay, we
> could add a SimulateNetworkDelay(). What do you think?

Yes, a lot cleaner! Thanks, done!

http://codereview.chromium.org/9113023/diff/8001/media/base/download_rate_mon...
media/base/download_rate_monitor_unittest.cc:70: data_source_.reset(new
FakeDataSource(
On 2012/01/09 23:16:57, acolwell wrote:
> Consider converting this to a Seek() method on FakeDataSource. I think that
> would be clearer.

Yeah, I think I was trying too hard to replicate what the Pipeline actually does
:) (recreates the resource loader on seeks). Done.

http://codereview.chromium.org/9113023/diff/8001/media/base/download_rate_mon...
media/base/download_rate_monitor_unittest.cc:81: class FakeFilterHost {
On 2012/01/09 23:16:57, acolwell wrote:
> I think you should just put these methods in FakeDataSource. I don't think
> pulling them out into a separate class makes things clearer.

Done.

http://codereview.chromium.org/9113023/diff/8001/media/base/download_rate_mon...
media/base/download_rate_monitor_unittest.cc:116: if (buffered_bytes_ >
defer_threshold_)
On 2012/01/09 23:16:57, acolwell wrote:
> How about a comment stating that you are simulating a deferral here?

Done.

http://codereview.chromium.org/9113023/diff/8001/media/base/download_rate_mon...
media/base/download_rate_monitor_unittest.cc:249: 
On 2012/01/09 23:16:57, acolwell wrote:
> We should also have some test simulating the SimpleDataSource & FileDataSource
> cases where only SetTotalBytes() & SetBufferedBytes() are called w/o a
> SetNetworkActivity().

Both SimpleDataSource and FileDataSource are "local" (non-http/https) sources,
so I think the MediaSourceIsLocal() test should cover it. Let me know if you
were thinking of something else.

[acolwell GONE FROM CHROMIUM, 2012-01-11 18:51:01]

LGTM  assuming the answer to the comment in SeekTo() doesn't trigger major
changes.

http://codereview.chromium.org/9113023/diff/12001/media/base/download_rate_mo...
File media/base/download_rate_monitor.cc (right):

http://codereview.chromium.org/9113023/diff/12001/media/base/download_rate_mo...
media/base/download_rate_monitor.cc:129: if (!is_downloading_data &&
!has_deferred_) {
Is it possible for this method to get called with is_downloading_data == false
before we get a call with is_downloading_data == true? If so I think we might
accidentally trigger this code. 

Perhaps put this in the section above with a (is_downloading_data_ &&
!is_downloading_data && !has_deferred_) ?

http://codereview.chromium.org/9113023/diff/12001/media/base/download_rate_mo...
File media/base/download_rate_monitor_unittest.cc (right):

http://codereview.chromium.org/9113023/diff/12001/media/base/download_rate_mo...
media/base/download_rate_monitor_unittest.cc:44: packet_size,time_elapsed_ +
base::Time::UnixEpoch());
space after ,

http://codereview.chromium.org/9113023/diff/12001/media/base/download_rate_mo...
media/base/download_rate_monitor_unittest.cc:101: is_downloading_data_ = true;
Should this trigger SetXXX calls?

http://codereview.chromium.org/9113023/diff/12001/media/base/download_rate_mo...
media/base/download_rate_monitor_unittest.cc:136: SimulateNetwork(20 *
kMediaByterate, 2 * kMediaByterate);
It's a little weird seeing byte rates for both parameters here and below. It
makes it hard to tell which is the actual transfer rate. Perhaps use a
SecondsToBytes() helper function or a kOneSecondOfBytes?

http://codereview.chromium.org/9113023/diff/12001/media/base/download_rate_mo...
media/base/download_rate_monitor_unittest.cc:221: // CanPlayThrough should not
be fired because of deferring.
I think this comment is a little confusing. CanPlayThrough does fire right?
Perhaps something like "Verify deferring is not what caused CanPlayThrough to
fire"?

[vrk (LEFT CHROMIUM), 2012-01-13 01:22:48]

PTAL, thanks!!

http://codereview.chromium.org/9113023/diff/12001/media/base/download_rate_mo...
File media/base/download_rate_monitor.cc (right):

http://codereview.chromium.org/9113023/diff/12001/media/base/download_rate_mo...
media/base/download_rate_monitor.cc:129: if (!is_downloading_data &&
!has_deferred_) {
On 2012/01/11 18:51:01, acolwell wrote:
> Is it possible for this method to get called with is_downloading_data == false
> before we get a call with is_downloading_data == true? 

Yes, on very short files or very fast downloads, where the download defers as a
result of the first call to didReceiveData.

> If so I think we might accidentally trigger this code. 

Actually, I don't think it's wrong to trigger this code in this case.
SetNetworkActivity() will always be called after data has started downloading --
it wouldn't make sense for the BufferedResourceLoader to "defer" if it wasn't
already downloading.

But now that you mention it, I think the code *above* this block is unnecessary.
Since we're going to trigger CanPlayThrough when downloading |has_deferred_|
regardless, there's no need to keep track of the transitions between the state
of |is_downloading_data_|. 

I deleted this code and the |is_downloading_data_| field.

http://codereview.chromium.org/9113023/diff/12001/media/base/download_rate_mo...
File media/base/download_rate_monitor_unittest.cc (right):

http://codereview.chromium.org/9113023/diff/12001/media/base/download_rate_mo...
media/base/download_rate_monitor_unittest.cc:44: packet_size,time_elapsed_ +
base::Time::UnixEpoch());
On 2012/01/11 18:51:01, acolwell wrote:
> space after ,

Done.

http://codereview.chromium.org/9113023/diff/12001/media/base/download_rate_mo...
media/base/download_rate_monitor_unittest.cc:101: is_downloading_data_ = true;
On 2012/01/11 18:51:01, acolwell wrote:
> Should this trigger SetXXX calls?

I don't think so. When a seek occurs (to a location outside of the in-memory
buffer), BufferedDataSource recreates BufferedResourceLoader. This doesn't
trigger a Network Event, so the SetXXX calls shouldn't get called.

http://codereview.chromium.org/9113023/diff/12001/media/base/download_rate_mo...
media/base/download_rate_monitor_unittest.cc:136: SimulateNetwork(20 *
kMediaByterate, 2 * kMediaByterate);
On 2012/01/11 18:51:01, acolwell wrote:
> It's a little weird seeing byte rates for both parameters here and below. It
> makes it hard to tell which is the actual transfer rate. Perhaps use a
> SecondsToBytes() helper function or a kOneSecondOfBytes?

Good idea. Done.

http://codereview.chromium.org/9113023/diff/12001/media/base/download_rate_mo...
media/base/download_rate_monitor_unittest.cc:221: // CanPlayThrough should not
be fired because of deferring.
On 2012/01/11 18:51:01, acolwell wrote:
> I think this comment is a little confusing. CanPlayThrough does fire right?
> Perhaps something like "Verify deferring is not what caused CanPlayThrough to
> fire"?

GOOD POINT! Haha I was reading it in my head with different intonation! Changed
here and elsewhere :)

[acolwell GONE FROM CHROMIUM, 2012-01-13 22:00:37]

LGTM

[vrk (LEFT CHROMIUM), 2012-01-18 22:56:25]

Haven't landed this yet because it breaks some layout tests.

The problem appears to be that the NetworkState is getting set to Idle so
quickly that a progress event never fires. It seems like this bug has always
been here, but it didn't manifest in layout tests for reasons that are kind of
hard to explain...

 - For the media/ tests: prior to this CL, SetNetworkState(Idle) was always
called *after* SetNetworkState(Loaded), and SetNetworkState(Loaded) *does* fire
a progress event in case it loaded very quickly
(http://code.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/W...).
 - For http/tests/media/video-play-progress.html: This is an http source, so
SetNetworkState(Loaded) is never called. However, the video loads so quickly
that SetNetworkActivity is never called at all, so SetNetworkState(Idle) is
never called, and the progress timer doesn't get stopped.


Anyway! The WebKit fix is here: https://bugs.webkit.org/show_bug.cgi?id=76568
Just got r+ed, so I will try to land the WK CL this afternoon and land this CL
after WK gets rolled in Chromium.

[sadrul, 2012-01-21 02:56:12]

Hi! This seems to be causing PrerenderHTML5VideoNetwork (in browser_tests) to
timeout on windows and linux. We are going to revert it?